From ddfed7f2fbabd1eba3b6ac700cee065c070433a7 Mon Sep 17 00:00:00 2001 From: Monty Taylor Date: Thu, 4 Aug 2016 16:18:42 -0500 Subject: [PATCH] Pass the argparse data into to validate_auth We have two contradictory precedence needs that are impossible to satisfy because we're losing knowledge of the source of data as we build the ultimate dict of data. If we carry the argparse data along in a separate bucket for longer, we can check to see if it's there so that it can win, but so that in situations where kwargs is complex and contains both a top-level and an auth dict we don't assume that the values that are not in the auth dict came from argparse. By doing this, we can establish that precedence for auth args is: - argparse - auth dict - top level kwargs Change-Id: I9eca5937077f5873f7896b6745951fb8d8c4747c --- os_client_config/config.py | 60 +++++++++++++++++++++++++++++--------- 1 file changed, 47 insertions(+), 13 deletions(-) diff --git a/os_client_config/config.py b/os_client_config/config.py index f1df797..2aa0530 100644 --- a/os_client_config/config.py +++ b/os_client_config/config.py @@ -763,7 +763,7 @@ class OpenStackConfig(object): cloud, region_name=region['name'])) return clouds - def _fix_args(self, args, argparse=None): + def _fix_args(self, args=None, argparse=None): """Massage the passed-in options Replace - with _ and strip os_ prefixes. @@ -771,6 +771,8 @@ class OpenStackConfig(object): Convert an argparse Namespace object to a dict, removing values that are either None or ''. """ + if not args: + args = {} if argparse: # Convert the passed-in Namespace @@ -831,7 +833,7 @@ class OpenStackConfig(object): config['auth']['token'] = 'notused' return loading.get_plugin_loader(config['auth_type']) - def _validate_auth_ksc(self, config, cloud): + def _validate_auth_ksc(self, config, cloud, fixed_argparse): try: import keystoneclient.auth as ksc_auth except ImportError: @@ -842,14 +844,22 @@ class OpenStackConfig(object): config['auth_type']).get_options() for p_opt in plugin_options: + # if it's in argparse, it was passed on the command line and wins # if it's in config.auth, win, kill it from config dict # if it's in config and not in config.auth, move it # deprecated loses to current # provided beats default, deprecated or not winning_value = self._find_winning_auth_value( - p_opt, config['auth']) - if not winning_value: - winning_value = self._find_winning_auth_value(p_opt, config) + p_opt, fixed_argparse) + if winning_value: + found_in_argparse = True + else: + found_in_argparse = False + winning_value = self._find_winning_auth_value( + p_opt, config['auth']) + if not winning_value: + winning_value = self._find_winning_auth_value( + p_opt, config) # if the plugin tells us that this value is required # then error if it's doesn't exist now @@ -865,7 +875,12 @@ class OpenStackConfig(object): # Clean up after ourselves for opt in [p_opt.name] + [o.name for o in p_opt.deprecated_opts]: opt = opt.replace('-', '_') - config.pop(opt, None) + # don't do this if the value came from argparse, because we + # don't (yet) know if the value in not-auth came from argparse + # overlay or from someone passing in a dict to kwargs + # TODO(mordred) Fix that data path too + if not found_in_argparse: + config.pop(opt, None) config['auth'].pop(opt, None) if winning_value: @@ -879,25 +894,38 @@ class OpenStackConfig(object): return config - def _validate_auth(self, config, loader): + def _validate_auth(self, config, loader, fixed_argparse): # May throw a keystoneauth1.exceptions.NoMatchingPlugin plugin_options = loader.get_options() for p_opt in plugin_options: + # if it's in argparse, it was passed on the command line and wins # if it's in config.auth, win, kill it from config dict # if it's in config and not in config.auth, move it # deprecated loses to current # provided beats default, deprecated or not winning_value = self._find_winning_auth_value( - p_opt, config['auth']) - if not winning_value: - winning_value = self._find_winning_auth_value(p_opt, config) + p_opt, fixed_argparse) + if winning_value: + found_in_argparse = True + else: + found_in_argparse = False + winning_value = self._find_winning_auth_value( + p_opt, config['auth']) + if not winning_value: + winning_value = self._find_winning_auth_value( + p_opt, config) # Clean up after ourselves for opt in [p_opt.name] + [o.name for o in p_opt.deprecated]: opt = opt.replace('-', '_') - config.pop(opt, None) + # don't do this if the value came from argparse, because we + # don't (yet) know if the value in not-auth came from argparse + # overlay or from someone passing in a dict to kwargs + # TODO(mordred) Fix that data path too + if not found_in_argparse: + config.pop(opt, None) config['auth'].pop(opt, None) if winning_value: @@ -932,6 +960,11 @@ class OpenStackConfig(object): """ args = self._fix_args(kwargs, argparse=argparse) + # Run the fix just for argparse by itself. We need to + # have a copy of the argparse options separately from + # any merged copied later in validate_auth so that we + # can determine precedence + fixed_argparse = self._fix_args(argparse=argparse) if cloud is None: if 'cloud' in args: @@ -995,7 +1028,7 @@ class OpenStackConfig(object): if validate: try: loader = self._get_auth_loader(config) - config = self._validate_auth(config, loader) + config = self._validate_auth(config, loader, fixed_argparse) auth_plugin = loader.load_from_options(**config['auth']) except Exception as e: # We WANT the ksa exception normally @@ -1005,7 +1038,8 @@ class OpenStackConfig(object): self.log.debug("Deferring keystone exception: {e}".format(e=e)) auth_plugin = None try: - config = self._validate_auth_ksc(config, cloud) + config = self._validate_auth_ksc( + config, cloud, fixed_argparse) except Exception: raise e else: