From 0f97918edc2413b44d9a01975ea87fa619b20742 Mon Sep 17 00:00:00 2001 From: Rabi Mishra Date: Fri, 5 Feb 2016 16:40:21 +0530 Subject: [PATCH] Don't mask exceptions in is_service_available We seem to be masking client exceptions when checking resource availability, it would be good to provide the proper error message to the user. Change-Id: I9b82b3d5d8382f2c253febf84b8d68ab7f128c5d Closes-Bug: #1542361 --- heat/common/exception.py | 4 +- heat/engine/resource.py | 60 +++++++++++++------------ heat/engine/service.py | 14 ++++-- heat/tests/engine/test_resource_type.py | 7 ++- heat/tests/test_resource.py | 55 +++++++++++++++++++---- 5 files changed, 94 insertions(+), 46 deletions(-) diff --git a/heat/common/exception.py b/heat/common/exception.py index dc78120b79..66bede06d0 100644 --- a/heat/common/exception.py +++ b/heat/common/exception.py @@ -33,8 +33,8 @@ LOG = logging.getLogger(__name__) # TODO(kanagaraj-manickam): Expose this to user via REST API ERROR_CODE_MAP = { - '99001': _("Service %(service_name)s does not have required endpoint in " - "service catalog for the resource type %(resource_type)s") + '99001': _("Service %(service_name)s is not available for resource " + "type %(resource_type)s, reason: %(reason)s") } diff --git a/heat/engine/resource.py b/heat/engine/resource.py index 13b32cddb0..d9089102ae 100644 --- a/heat/engine/resource.py +++ b/heat/engine/resource.py @@ -150,14 +150,23 @@ class Resource(object): @classmethod def _validate_service_availability(cls, context, resource_type): - if not cls.is_service_available(context): + try: + svc_available = cls.is_service_available(context) + except Exception as exc: ex = exception.ResourceTypeUnavailable( + resource_type=resource_type, service_name=cls.default_client_name, - resource_type=resource_type - ) - LOG.info(six.text_type(ex)) - + reason=six.text_type(exc)) + LOG.exception(exc) raise ex + else: + if not svc_available: + ex = exception.ResourceTypeUnavailable( + resource_type=resource_type, + service_name=cls.default_client_name, + reason='Service endpoint not in service catalog.') + LOG.info(six.text_type(ex)) + raise ex def _init_attributes(self): """The method that defines attribute initialization for a resource. @@ -558,31 +567,26 @@ class Resource(object): # resources as they are implemented within the engine. if cls.default_client_name is None: return True + client_plugin = clients.Clients(context).client_plugin( + cls.default_client_name) - try: - client_plugin = clients.Clients(context).client_plugin( - cls.default_client_name) - - service_types = client_plugin.service_types - if not service_types: - return True - - # NOTE(kanagaraj-manickam): if one of the service_type does - # exist in the keystone, then considered it as available. - for service_type in service_types: - endpoint_exists = client_plugin.does_endpoint_exist( - service_type=service_type, - service_name=cls.default_client_name) - if endpoint_exists: - req_extension = cls.required_service_extension - is_ext_available = ( - not req_extension or client_plugin.has_extension( - req_extension)) - if is_ext_available: - return True - except Exception as ex: - LOG.exception(ex) + service_types = client_plugin.service_types + if not service_types: + return True + # NOTE(kanagaraj-manickam): if one of the service_type does + # exist in the keystone, then considered it as available. + for service_type in service_types: + endpoint_exists = client_plugin.does_endpoint_exist( + service_type=service_type, + service_name=cls.default_client_name) + if endpoint_exists: + req_extension = cls.required_service_extension + is_ext_available = ( + not req_extension or client_plugin.has_extension( + req_extension)) + if is_ext_available: + return True return False def keystone(self): diff --git a/heat/engine/service.py b/heat/engine/service.py index 1dd3085dce..7a0991fcb5 100644 --- a/heat/engine/service.py +++ b/heat/engine/service.py @@ -1322,11 +1322,19 @@ class EngineService(service.Service): if resource_class.support_status.status == support.HIDDEN: raise exception.NotSupported(type_name) - if not resource_class.is_service_available(cnxt): + try: + svc_available = resource_class.is_service_available(cnxt) + except Exception as exc: raise exception.ResourceTypeUnavailable( service_name=resource_class.default_client_name, - resource_type=type_name - ) + resource_type=type_name, + reason=six.text_type(exc)) + else: + if not svc_available: + raise exception.ResourceTypeUnavailable( + service_name=resource_class.default_client_name, + resource_type=type_name, + reason='Service endpoint not in service catalog.') def properties_schema(): for name, schema_dict in resource_class.properties_schema.items(): diff --git a/heat/tests/engine/test_resource_type.py b/heat/tests/engine/test_resource_type.py index 2ebf79352c..b665107e62 100644 --- a/heat/tests/engine/test_resource_type.py +++ b/heat/tests/engine/test_resource_type.py @@ -166,10 +166,9 @@ class ResourceTypeTest(common.HeatTestCase): self.eng.resource_schema, self.ctx, type_name) - - msg = ('HEAT-E99001 Service sample does not have required endpoint' - ' in service catalog for the resource type' - ' ResourceWithDefaultClientName') + msg = ('HEAT-E99001 Service sample is not available for resource ' + 'type ResourceWithDefaultClientName, reason: ' + 'Service endpoint not in service catalog.') self.assertEqual(msg, six.text_type(ex), 'invalid exception message') diff --git a/heat/tests/test_resource.py b/heat/tests/test_resource.py index 5c763055e5..707c1becef 100644 --- a/heat/tests/test_resource.py +++ b/heat/tests/test_resource.py @@ -3070,12 +3070,13 @@ class ResourceAvailabilityTest(common.HeatTestCase): ['test_type'] ) mock_client_plugin.has_extension = mock.Mock( - side_effect=Exception("error")) + side_effect=exception.AuthorizationFailure()) mock_client_plugin_method.return_value = mock_client_plugin - self.assertFalse( - generic_rsrc.ResourceWithDefaultClientNameExt.is_service_available( - context=mock.Mock())) + self.assertRaises( + exception.AuthorizationFailure, + generic_rsrc.ResourceWithDefaultClientNameExt.is_service_available, + context=mock.Mock()) mock_client_plugin_method.assert_called_once_with( generic_rsrc.ResourceWithDefaultClientName.default_client_name) mock_service_types.assert_called_once_with() @@ -3112,8 +3113,8 @@ class ResourceAvailabilityTest(common.HeatTestCase): service_name=(generic_rsrc.ResourceWithDefaultClientName .default_client_name)) - def test_service_not_deployed_throws_exception(self): - """Test raising exception when the service is not deployed. + def test_service_not_available_returns_false(self): + """Test when the service is not in service catalog. When the service is not deployed, make sure resource is throwing ResourceTypeUnavailable exception. @@ -3138,9 +3139,45 @@ class ResourceAvailabilityTest(common.HeatTestCase): definition=definition, stack=mock_stack) - msg = ('HEAT-E99001 Service sample does not have required endpoint' - ' in service catalog for the resource type' - ' UnavailableResourceType') + msg = ('HEAT-E99001 Service sample is not available for resource ' + 'type UnavailableResourceType, reason: ' + 'Service endpoint not in service catalog.') + self.assertEqual(msg, + six.text_type(ex), + 'invalid exception message') + + # Make sure is_service_available is called on the right class + mock_method.assert_called_once_with(mock_stack.context) + + def test_service_not_available_throws_exception(self): + """Test for other exceptions when checking for service availability + + Ex. when client throws an error, make sure resource is throwing + ResourceTypeUnavailable that contains the orginal exception message. + """ + with mock.patch.object( + generic_rsrc.ResourceWithDefaultClientName, + 'is_service_available') as mock_method: + mock_method.side_effect = exception.AuthorizationFailure() + + definition = rsrc_defn.ResourceDefinition( + name='Test Resource', + resource_type='UnavailableResourceType') + + mock_stack = mock.MagicMock() + mock_stack.service_check_defer = False + + ex = self.assertRaises( + exception.ResourceTypeUnavailable, + generic_rsrc.ResourceWithDefaultClientName.__new__, + cls=generic_rsrc.ResourceWithDefaultClientName, + name='test_stack', + definition=definition, + stack=mock_stack) + + msg = ('HEAT-E99001 Service sample is not available for resource ' + 'type UnavailableResourceType, reason: ' + 'Authorization failed.') self.assertEqual(msg, six.text_type(ex), 'invalid exception message')