From 517f91ce27299a2a1e4551a8310e3a1ce1151b09 Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Fri, 20 Nov 2015 21:27:13 -0500 Subject: [PATCH] Fix HTTP error codes due to invalid templates This reverts commit 9a4d719ea1a6ca63f527b5c78308fd288058d42b and creates a new exception type, InvalidGlobalResource, that is raised when a non-existent template is referenced in the global environment and which results in an HTTP 500 error. This fixes the cases where we could return 404 or 500 errors as a result of template validation failures, when we should return a 400 error. Change-Id: I3c7b3182957448bb13514696cc2e12f5aaddd253 Closes-Bug: #1518458 Related-Bug: #1447194 --- heat/api/middleware/fault.py | 1 + heat/common/exception.py | 9 ++---- heat/engine/environment.py | 10 +++---- heat/engine/resource.py | 2 +- .../openstack/heat/resource_group.py | 2 +- heat/engine/resources/template_resource.py | 6 ++-- heat/engine/service.py | 24 ++++++++++----- heat/tests/api/openstack_v1/test_stacks.py | 21 +++++++++++++ heat/tests/engine/test_resource_type.py | 5 ++-- heat/tests/test_provider_template.py | 14 +++++---- heat/tests/test_resource.py | 6 ++-- heat/tests/test_resource_group.py | 5 ++-- heat/tests/test_stack_resource.py | 30 ++++++++----------- 13 files changed, 79 insertions(+), 56 deletions(-) diff --git a/heat/api/middleware/fault.py b/heat/api/middleware/fault.py index 2b27c379c3..2a2fe4b57f 100644 --- a/heat/api/middleware/fault.py +++ b/heat/api/middleware/fault.py @@ -60,6 +60,7 @@ class FaultWrapper(wsgi.Middleware): 'NotFound': webob.exc.HTTPNotFound, 'ResourceActionNotSupported': webob.exc.HTTPBadRequest, 'ResourceNotFound': webob.exc.HTTPNotFound, + 'InvalidGlobalResource': webob.exc.HTTPInternalServerError, 'SnapshotNotFound': webob.exc.HTTPNotFound, 'ResourceNotAvailable': webob.exc.HTTPNotFound, 'PhysicalResourceNameAmbiguity': webob.exc.HTTPBadRequest, diff --git a/heat/common/exception.py b/heat/common/exception.py index af18b780ea..8d9fb538d9 100644 --- a/heat/common/exception.py +++ b/heat/common/exception.py @@ -209,12 +209,9 @@ class SnapshotNotFound(HeatException): "could not be found.") -class TemplateNotFound(HeatException): - msg_fmt = _("%(message)s") - - -class InvalidResourceType(HeatException): - msg_fmt = _("%(message)s") +class InvalidGlobalResource(HeatException): + msg_fmt = _("There was an error loading the definition of the global " + "resource type %(type_name)s.") class InvalidBreakPointHook(HeatException): diff --git a/heat/engine/environment.py b/heat/engine/environment.py index cbc12bb529..9f8cd0d89f 100644 --- a/heat/engine/environment.py +++ b/heat/engine/environment.py @@ -419,20 +419,20 @@ class ResourceRegistry(object): def get_class(self, resource_type, resource_name=None, files=None): if resource_type == "": msg = _('Resource "%s" has no type') % resource_name - raise exception.InvalidResourceType(message=msg) + raise exception.StackValidationFailed(message=msg) elif resource_type is None: msg = _('Non-empty resource type is required ' 'for resource "%s"') % resource_name - raise exception.InvalidResourceType(message=msg) + raise exception.StackValidationFailed(message=msg) elif not isinstance(resource_type, six.string_types): msg = _('Resource "%s" type is not a string') % resource_name - raise exception.InvalidResourceType(message=msg) + raise exception.StackValidationFailed(message=msg) info = self.get_resource_info(resource_type, resource_name=resource_name) if info is None: - raise exception.EntityNotFound(entity='Resource Type', - name=resource_type) + msg = _("Unknown resource Type : %s") % resource_type + raise exception.StackValidationFailed(message=msg) return info.get_class(files=files) def as_dict(self): diff --git a/heat/engine/resource.py b/heat/engine/resource.py index 9f9dc26be4..043a3831d0 100644 --- a/heat/engine/resource.py +++ b/heat/engine/resource.py @@ -138,7 +138,7 @@ class Resource(object): ResourceClass = registry.get_class(definition.resource_type, resource_name=name, files=stack.t.files) - except exception.TemplateNotFound: + except exception.NotFound: ResourceClass = template_resource.TemplateResource assert issubclass(ResourceClass, Resource) diff --git a/heat/engine/resources/openstack/heat/resource_group.py b/heat/engine/resources/openstack/heat/resource_group.py index 4a08ee579d..a3d7b1a1ae 100644 --- a/heat/engine/resources/openstack/heat/resource_group.py +++ b/heat/engine/resources/openstack/heat/resource_group.py @@ -276,7 +276,7 @@ class ResourceGroup(stack_resource.StackResource): # make sure we can resolve the nested resource type try: self.stack.env.get_class(res_def.resource_type) - except exception.TemplateNotFound: + except exception.NotFound: # its a template resource pass diff --git a/heat/engine/resources/template_resource.py b/heat/engine/resources/template_resource.py index ed1f95c307..c91cefd3a2 100644 --- a/heat/engine/resources/template_resource.py +++ b/heat/engine/resources/template_resource.py @@ -98,7 +98,7 @@ class TemplateResource(stack_resource.StackResource): args = {'name': template_name, 'exc': six.text_type(r_exc)} msg = _('Could not fetch remote template ' '"%(name)s": %(exc)s') % args - raise exception.TemplateNotFound(message=msg) + raise exception.NotFound(msg_fmt=msg) @staticmethod def get_schemas(tmpl, param_defaults): @@ -111,7 +111,7 @@ class TemplateResource(stack_resource.StackResource): self._parsed_nested = None try: tmpl = template.Template(self.child_template()) - except (exception.TemplateNotFound, ValueError) as download_error: + except (exception.NotFound, ValueError) as download_error: self.validation_exception = download_error tmpl = template.Template( {"HeatTemplateFormatVersion": "2012-12-12"}) @@ -199,7 +199,7 @@ class TemplateResource(stack_resource.StackResource): try: t_data = self.get_template_file(self.template_name, self.allowed_schemes) - except exception.TemplateNotFound as err: + except exception.NotFound as err: if self.action == self.UPDATE: raise reported_excp = err diff --git a/heat/engine/service.py b/heat/engine/service.py index baf271a92b..5f5e707911 100644 --- a/heat/engine/service.py +++ b/heat/engine/service.py @@ -1220,10 +1220,14 @@ class EngineService(service.Service): self.resource_enforcer.enforce(cnxt, type_name) try: resource_class = resources.global_env().get_class(type_name) - except (exception.InvalidResourceType, - exception.EntityNotFound, - exception.TemplateNotFound) as ex: - raise ex + except exception.StackValidationFailed: + raise exception.EntityNotFound(entity='Resource Type', + name=type_name) + except exception.NotFound: + LOG.exception(_LE('Error loading resource type %s ' + 'from global environment.'), + type_name) + raise exception.InvalidGlobalResource(type_name=type_name) if resource_class.support_status.status == support.HIDDEN: raise exception.NotSupported(type_name) @@ -1269,10 +1273,14 @@ class EngineService(service.Service): raise exception.NotSupported(type_name) return resource_class.resource_to_template(type_name, template_type) - except (exception.InvalidResourceType, - exception.EntityNotFound, - exception.TemplateNotFound) as ex: - raise ex + except exception.StackValidationFailed: + raise exception.EntityNotFound(entity='Resource Type', + name=type_name) + except exception.NotFound: + LOG.exception(_LE('Error loading resource type %s ' + 'from global environment.'), + type_name) + raise exception.InvalidGlobalResource(type_name=type_name) @context.request_context def list_events(self, cnxt, stack_identity, filters=None, limit=None, diff --git a/heat/tests/api/openstack_v1/test_stacks.py b/heat/tests/api/openstack_v1/test_stacks.py index ebe92c60fa..3ca41ce58c 100644 --- a/heat/tests/api/openstack_v1/test_stacks.py +++ b/heat/tests/api/openstack_v1/test_stacks.py @@ -2310,6 +2310,27 @@ class StackControllerTest(tools.ControllerTest, common.HeatTestCase): self.assertEqual('EntityNotFound', resp.json['error']['type']) self.m.VerifyAll() + def test_resource_schema_faulty_template(self, mock_enforce): + self._mock_enforce_setup(mock_enforce, 'resource_schema', True) + req = self._get('/resource_types/FaultyTemplate') + type_name = 'FaultyTemplate' + + error = heat_exc.InvalidGlobalResource(type_name='FaultyTemplate') + self.m.StubOutWithMock(rpc_client.EngineClient, 'call') + rpc_client.EngineClient.call( + req.context, + ('resource_schema', {'type_name': type_name}) + ).AndRaise(tools.to_remote_error(error)) + self.m.ReplayAll() + + resp = tools.request_with_middleware(fault.FaultWrapper, + self.controller.resource_schema, + req, tenant_id=self.tenant, + type_name=type_name) + self.assertEqual(500, resp.json['code']) + self.assertEqual('InvalidGlobalResource', resp.json['error']['type']) + self.m.VerifyAll() + def test_resource_schema_err_denied_policy(self, mock_enforce): self._mock_enforce_setup(mock_enforce, 'resource_schema', False) req = self._get('/resource_types/BogusResourceType') diff --git a/heat/tests/engine/test_resource_type.py b/heat/tests/engine/test_resource_type.py index 665dad6014..13e1243746 100644 --- a/heat/tests/engine/test_resource_type.py +++ b/heat/tests/engine/test_resource_type.py @@ -134,11 +134,12 @@ class ResourceTypeTest(common.HeatTestCase): mock_iterable = mock.MagicMock(return_value=iter([info])) with mock.patch('heat.engine.environment.ResourceRegistry.iterable_by', new=mock_iterable): - ex = self.assertRaises(exception.TemplateNotFound, + ex = self.assertRaises(exception.InvalidGlobalResource, function, self.ctx, type_name='ResourceWithWrongRefOnFile') - msg = 'Could not fetch remote template "not_existing.yaml"' + msg = ('There was an error loading the definition of the global ' + 'resource type ResourceWithWrongRefOnFile.') self.assertIn(msg, six.text_type(ex)) def test_resource_schema_no_template_file(self): diff --git a/heat/tests/test_provider_template.py b/heat/tests/test_provider_template.py index 91cbe91b1a..813b4ee2ff 100644 --- a/heat/tests/test_provider_template.py +++ b/heat/tests/test_provider_template.py @@ -614,7 +614,7 @@ class ProviderTemplateTest(common.HeatTestCase): env_str = {'resource_registry': {'resources': {'fred': { "OS::ResourceType": "some_magic.yaml"}}}} env = environment.Environment(env_str) - ex = self.assertRaises(exception.TemplateNotFound, env.get_class, + ex = self.assertRaises(exception.NotFound, env.get_class, 'OS::ResourceType', 'fred') self.assertIn('Could not fetch remote template "some_magic.yaml"', six.text_type(ex)) @@ -934,16 +934,18 @@ class TemplateDataTest(common.HeatTestCase): self.res.action = self.res.UPDATE self.res.nested = mock.MagicMock() self.res.get_template_file = mock.Mock( - side_effect=exception.TemplateNotFound( - message='test_resource.template')) - self.assertRaises(exception.TemplateNotFound, self.res.template_data) + side_effect=exception.NotFound( + msg_fmt='Could not fetch remote template ' + '"test_resource.template": file not found')) + self.assertRaises(exception.NotFound, self.res.template_data) def test_template_data_in_create_without_template_file(self): self.res.action = self.res.CREATE self.res.nested = mock.MagicMock() self.res.get_template_file = mock.Mock( - side_effect=exception.TemplateNotFound( - message='test_resource.template')) + side_effect=exception.NotFound( + msg_fmt='Could not fetch remote template ' + '"test_resource.template": file not found')) self.assertEqual('{}', self.res.template_data()) diff --git a/heat/tests/test_resource.py b/heat/tests/test_resource.py index 4effd1943f..f4000d6ddf 100644 --- a/heat/tests/test_resource.py +++ b/heat/tests/test_resource.py @@ -73,7 +73,7 @@ class ResourceTest(common.HeatTestCase): self.assertEqual(generic_rsrc.GenericResource, cls) def test_get_class_noexist(self): - self.assertRaises(exception.EntityNotFound, + self.assertRaises(exception.StackValidationFailed, resources.global_env().get_class, 'NoExistResourceType') @@ -173,13 +173,13 @@ class ResourceTest(common.HeatTestCase): def test_resource_new_err(self): snippet = rsrc_defn.ResourceDefinition('aresource', 'NoExistResourceType') - self.assertRaises(exception.EntityNotFound, + self.assertRaises(exception.StackValidationFailed, resource.Resource, 'aresource', snippet, self.stack) def test_resource_non_type(self): resource_name = 'aresource' snippet = rsrc_defn.ResourceDefinition(resource_name, '') - ex = self.assertRaises(exception.InvalidResourceType, + ex = self.assertRaises(exception.StackValidationFailed, resource.Resource, resource_name, snippet, self.stack) self.assertIn(_('Resource "%s" has no type') % resource_name, diff --git a/heat/tests/test_resource_group.py b/heat/tests/test_resource_group.py index a67e92abca..433f362dfa 100644 --- a/heat/tests/test_resource_group.py +++ b/heat/tests/test_resource_group.py @@ -417,10 +417,9 @@ class ResourceGroupTest(common.HeatTestCase): stack = utils.parse_stack(tmp) snip = stack.t.resource_definitions(stack)['group1'] resg = resource_group.ResourceGroup('test', snip, stack) - exc = self.assertRaises(exception.EntityNotFound, + exc = self.assertRaises(exception.StackValidationFailed, resg.validate) - exp_msg = 'The Resource Type (idontexist) could not be found.' - self.assertIn(exp_msg, six.text_type(exc)) + self.assertIn('Unknown resource Type', six.text_type(exc)) def test_reference_attr(self): stack = utils.parse_stack(template2) diff --git a/heat/tests/test_stack_resource.py b/heat/tests/test_stack_resource.py index e4d45275d5..841b50cd20 100644 --- a/heat/tests/test_stack_resource.py +++ b/heat/tests/test_stack_resource.py @@ -387,28 +387,22 @@ class StackResourceTest(StackResourceBaseTest): self.assertEqual(raise_exc_msg, six.text_type(exc)) def _test_validate_unknown_resource_type(self, stack_name, tmpl, - resource_name, - stack_resource=True): - raise_exc_msg = 'The Resource Type (idontexist) could not be found.' + resource_name): + raise_exc_msg = ('Unknown resource Type : idontexist') stack = parser.Stack(utils.dummy_context(), stack_name, tmpl) rsrc = stack[resource_name] - if stack_resource: - exc = self.assertRaises(exception.StackValidationFailed, - rsrc.validate) - else: - exc = self.assertRaises(exception.EntityNotFound, - rsrc.validate) + + exc = self.assertRaises(exception.StackValidationFailed, + rsrc.validate) self.assertIn(raise_exc_msg, six.text_type(exc)) def test_validate_resource_group(self): - # resource group validate without nested template is a normal - # resource validation + # test validate without nested template stack_name = 'validate_resource_group_template' t = template_format.parse(resource_group_template) tmpl = templatem.Template(t) self._test_validate_unknown_resource_type(stack_name, tmpl, - 'my_resource_group', - stack_resource=False) + 'my_resource_group') # validate with nested template res_prop = t['resources']['my_resource_group']['properties'] @@ -419,7 +413,7 @@ class StackResourceTest(StackResourceBaseTest): 'my_resource_group') def test_validate_heat_autoscaling_group(self): - # Autoscaling validation is a nested stack validation + # test validate without nested template stack_name = 'validate_heat_autoscaling_group_template' t = template_format.parse(heat_autoscaling_group_template) tmpl = templatem.Template(t) @@ -881,8 +875,8 @@ class WithTemplateTest(StackResourceBaseTest): class RaiseLocalException(StackResourceBaseTest): def test_heat_exception(self): - local = exception.InvalidResourceType(message='test') - self.assertRaises(exception.InvalidResourceType, + local = exception.StackValidationFailed(message='test') + self.assertRaises(exception.StackValidationFailed, self.parent_resource.raise_local_exception, local) def test_messaging_timeout(self): @@ -891,9 +885,9 @@ class RaiseLocalException(StackResourceBaseTest): self.parent_resource.raise_local_exception, local) def test_remote_heat_ex(self): - class InvalidResourceType_Remote(exception.InvalidResourceType): + class StackValidationFailed_Remote(exception.StackValidationFailed): pass - local = InvalidResourceType_Remote(message='test') + local = StackValidationFailed_Remote(message='test') self.assertRaises(exception.ResourceFailure, self.parent_resource.raise_local_exception, local)