From 820790225cdcb078604c2ec6dab20bd6d2761496 Mon Sep 17 00:00:00 2001 From: Eric Fried Date: Wed, 5 Jun 2019 16:19:01 -0500 Subject: [PATCH] Handle oslo.config exceptions in from_conf In the following cases, we used to happily allow the Connection to the service, using default Adapter settings: - If the conf section for a given service is missing, or present but without ksa adapter opts registered. - If the conf section for a given service has bogus values. Now for these scenarios, we disable that service with a helpful reason. The service disabling is supported at a broader level: If someone sets has_{service_type} to false in their config, we remove the adapter for that service and replace it with something that throws errors when people try to use it. They can optionally set {service_type}_disabled_reason to make the message more informative. Co-Authored-By: Monty Taylor Change-Id: I3aa1f1633790e6e958bbc510ac5e5a11c0c27a9f --- lower-constraints.txt | 2 +- openstack/cloud/openstackcloud.py | 2 +- openstack/config/cloud_region.py | 59 +++++++++++++++++-- openstack/connection.py | 1 + openstack/exceptions.py | 4 ++ openstack/service_description.py | 17 ++++++ openstack/tests/unit/config/test_from_conf.py | 54 +++++++++++------ requirements.txt | 2 +- 8 files changed, 115 insertions(+), 26 deletions(-) diff --git a/lower-constraints.txt b/lower-constraints.txt index affeed9e7..a8d5d7dfe 100644 --- a/lower-constraints.txt +++ b/lower-constraints.txt @@ -21,7 +21,7 @@ mox3==0.20.0 munch==2.1.0 netifaces==0.10.4 os-client-config==1.28.0 -os-service-types==1.2.0 +os-service-types==1.6.0 oslo.config==6.1.0 oslotest==3.2.0 pbr==2.0.0 diff --git a/openstack/cloud/openstackcloud.py b/openstack/cloud/openstackcloud.py index 899467799..f48ce13e5 100755 --- a/openstack/cloud/openstackcloud.py +++ b/openstack/cloud/openstackcloud.py @@ -670,7 +670,7 @@ class _OpenStackCloudMixin(object): return endpoint def has_service(self, service_key): - if not self.config.config.get('has_%s' % service_key, True): + if not self.config.has_service(service_key): # TODO(mordred) add a stamp here so that we only report this once if not (service_key in self._disable_warnings and self._disable_warnings[service_key]): diff --git a/openstack/config/cloud_region.py b/openstack/config/cloud_region.py index 766a3b901..99a85033a 100644 --- a/openstack/config/cloud_region.py +++ b/openstack/config/cloud_region.py @@ -39,6 +39,9 @@ from openstack.config import defaults as config_defaults from openstack import exceptions from openstack import proxy + +_logger = _log.setup_logging('openstack') + SCOPE_KEYS = { 'domain_id', 'domain_name', 'project_id', 'project_name', @@ -58,6 +61,15 @@ def _make_key(key, service_type): return "_".join([service_type, key]) +def _disable_service(config, service_type, reason=None): + service_type = service_type.lower().replace('-', '_') + key = 'has_{service_type}'.format(service_type=service_type) + config[key] = False + if reason: + d_key = _make_key('disabled_reason', service_type) + config[d_key] = reason + + def _get_implied_microversion(version): if not version: return @@ -130,18 +142,38 @@ def from_conf(conf, session=None, **kwargs): for st in stm.all_types_by_service_type: project_name = stm.get_project_name(st) if project_name not in conf: + _disable_service( + config_dict, st, + reason="No section for project '{project}' (service type " + "'{service_type}') was present in the config.".format( + project=project_name, service_type=st)) continue opt_dict = {} # Populate opt_dict with (appropriately processed) Adapter conf opts try: ks_load_adap.process_conf_options(conf[project_name], opt_dict) - except Exception: - # NOTE(efried): This is for oslo_config.cfg.NoSuchOptError, but we - # don't want to drag in oslo.config just for that. + except Exception as e: + # NOTE(efried): This is for (at least) a couple of scenarios: + # (1) oslo_config.cfg.NoSuchOptError when ksa adapter opts are not + # registered in this section. + # (2) TypeError, when opts are registered but bogus (e.g. + # 'interface' and 'valid_interfaces' are both present). + # We may want to consider (providing a kwarg giving the caller the + # option of) blowing up right away for (2) rather than letting them + # get all the way to the point of trying the service and having + # *that* blow up. + reason = ("Encountered an exception attempting to process config " + "for project '{project}' (service type " + "'{service_type}'): {exception}".format( + project=project_name, service_type=st, exception=e)) + _logger.warn("Disabling service '{service_type}'.".format( + service_type=st)) + _logger.warn(reason) + _disable_service(config_dict, st, reason=reason) continue # Load them into config_dict under keys prefixed by ${service_type}_ for raw_name, opt_val in opt_dict.items(): - config_name = '_'.join([st, raw_name]) + config_name = _make_key(raw_name, st) config_dict[config_name] = opt_val return CloudRegion( session=session, config=config_dict, **kwargs) @@ -929,3 +961,22 @@ class CloudRegion(object): ) registry._openstacksdk_counter = counter return counter + + def has_service(self, service_type): + service_type = service_type.lower().replace('-', '_') + key = 'has_{service_type}'.format(service_type=service_type) + return self.config.get( + key, self._service_type_manager.is_official(service_type)) + + def disable_service(self, service_type, reason=None): + _disable_service(self.config, service_type, reason=reason) + + def enable_service(self, service_type): + service_type = service_type.lower().replace('-', '_') + key = 'has_{service_type}'.format(service_type=service_type) + self.config[key] = True + + def get_disabled_reason(self, service_type): + service_type = service_type.lower().replace('-', '_') + d_key = _make_key('disabled_reason', service_type) + return self.config.get(d_key) diff --git a/openstack/connection.py b/openstack/connection.py index f4fed899a..c79d84587 100644 --- a/openstack/connection.py +++ b/openstack/connection.py @@ -446,6 +446,7 @@ class Connection(six.with_metaclass(_meta.ConnectionMeta, attr_name.replace('-', '_'), property(fget=getter) ) + self.config.enable_service(attr_name) def authorize(self): """Authorize this Connection diff --git a/openstack/exceptions.py b/openstack/exceptions.py index 8fa9637d9..a267c2c9f 100644 --- a/openstack/exceptions.py +++ b/openstack/exceptions.py @@ -253,3 +253,7 @@ class ValidationException(SDKException): class TaskManagerStopped(SDKException): """Operations were attempted on a stopped TaskManager.""" + + +class ServiceDisabledException(ConfigException): + """This service is disabled for reasons.""" diff --git a/openstack/service_description.py b/openstack/service_description.py index d2292b4b4..87acc909f 100644 --- a/openstack/service_description.py +++ b/openstack/service_description.py @@ -26,6 +26,18 @@ _logger = _log.setup_logging('openstack') _service_type_manager = os_service_types.ServiceTypes() +class _ServiceDisabledProxyShim(object): + def __init__(self, service_type, reason): + self.service_type = service_type + self.reason = reason + + def __getattr__(self, item): + raise exceptions.ServiceDisabledException( + "Service '{service_type}' is disabled because its configuration " + "could not be loaded. {reason}".format( + service_type=self.service_type, reason=self.reason or '')) + + class ServiceDescription(object): #: Dictionary of supported versions and proxy classes for that version @@ -83,6 +95,11 @@ class ServiceDescription(object): """ config = instance.config + if not config.has_service(self.service_type): + return _ServiceDisabledProxyShim( + self.service_type, + config.get_disabled_reason(self.service_type)) + # First, check to see if we've got config that matches what we # understand in the SDK. version_string = config.get_api_version(self.service_type) diff --git a/openstack/tests/unit/config/test_from_conf.py b/openstack/tests/unit/config/test_from_conf.py index c60a48f84..4f83b5f10 100644 --- a/openstack/tests/unit/config/test_from_conf.py +++ b/openstack/tests/unit/config/test_from_conf.py @@ -30,8 +30,8 @@ class TestFromConf(base.TestCase): self.oslo_config_dict = { # All defaults for nova 'nova': {}, - # monasca not in the service catalog - 'monasca': {}, + # monasca-api not in the service catalog + 'monasca-api': {}, # Overrides for heat 'heat': { 'region_name': 'SpecialRegion', @@ -43,6 +43,7 @@ class TestFromConf(base.TestCase): def _load_ks_cfg_opts(self): conf = cfg.ConfigOpts() for group, opts in self.oslo_config_dict.items(): + conf.register_group(cfg.OptGroup(group)) if opts is not None: ks_loading.register_adapter_conf_options(conf, group) for name, val in opts.items(): @@ -122,27 +123,42 @@ class TestFromConf(base.TestCase): self.assertEqual(s.name, server_name) self.assert_calls() - def test_no_adapter_opts(self): - """Adapter opts for service type not registered.""" - del self.oslo_config_dict['heat'] + def _test_missing_invalid_permutations(self, expected_reason): + # Do special things to self.oslo_config_dict['heat'] before calling + # this method. conn = self._get_conn() - # TODO(efried): This works, even though adapter opts are not - # registered. Should it? adap = conn.orchestration - self.assertIsNone(adap.region_name) - self.assertEqual('orchestration', adap.service_type) - self.assertEqual('public', adap.interface) - self.assertIsNone(adap.endpoint_override) + ex = self.assertRaises( + exceptions.ServiceDisabledException, getattr, adap, 'get') + self.assertIn("Service 'orchestration' is disabled because its " + "configuration could not be loaded.", ex.message) + self.assertIn(expected_reason, ex.message) - self.register_uris([ - dict(method='GET', - uri=self.get_mock_url( - 'orchestration', append=['foo']), - json={'foo': {}}) - ]) - adap.get('/foo') - self.assert_calls() + def test_no_such_conf_section(self): + """No conf section (therefore no adapter opts) for service type.""" + del self.oslo_config_dict['heat'] + self._test_missing_invalid_permutations( + "No section for project 'heat' (service type 'orchestration') was " + "present in the config.") + + def test_no_adapter_opts(self): + """Conf section present, but opts for service type not registered.""" + self.oslo_config_dict['heat'] = None + self._test_missing_invalid_permutations( + "Encountered an exception attempting to process config for " + "project 'heat' (service type 'orchestration'): no such option") + + def test_invalid_adapter_opts(self): + """Adapter opts are bogus, in exception-raising ways.""" + self.oslo_config_dict['heat'] = { + 'interface': 'public', + 'valid_interfaces': 'private', + } + self._test_missing_invalid_permutations( + "Encountered an exception attempting to process config for " + "project 'heat' (service type 'orchestration'): interface and " + "valid_interfaces are mutually exclusive.") def test_no_session(self): # TODO(efried): Currently calling without a Session is not implemented. diff --git a/requirements.txt b/requirements.txt index 9062efb83..4e188856e 100644 --- a/requirements.txt +++ b/requirements.txt @@ -7,7 +7,7 @@ appdirs>=1.3.0 # MIT License requestsexceptions>=1.2.0 # Apache-2.0 jsonpatch!=1.20,>=1.16 # BSD six>=1.10.0 # MIT -os-service-types>=1.2.0 # Apache-2.0 +os-service-types>=1.6.0 # Apache-2.0 keystoneauth1>=3.14.0 # Apache-2.0 munch>=2.1.0 # MIT