From 433da2654372cf6e1841b24266d3318c7d0c14af Mon Sep 17 00:00:00 2001 From: "Luis A. Garcia" Date: Tue, 6 Aug 2013 18:36:02 +0000 Subject: [PATCH] Ensure all REST API error responses are consistent Heat exceptions are serialized into JSON/XML responses by the fault app, which is at the end of the WSGI pipeline. Some REST API errors however are raised as HTTPExceptions, which are treated by WSGI as responses ready to be sent back to the user, and they can't reach the fault app. This patch set disguises HTTPExceptions raised by the wsgi.Resource so they can reach the app that will serialize them just like all other errors. Fixes bug 1208620 Change-Id: Id98dbc1e3b208401be8f0119a6d72d4561a2221a --- heat/api/middleware/fault.py | 25 ++++++++++++++++++------- heat/common/exception.py | 10 ++++++++++ heat/common/wsgi.py | 14 ++++++++++++-- heat/tests/test_api_openstack_v1.py | 22 ++++++++++++++++++++++ heat/tests/test_wsgi.py | 12 +++++++++--- 5 files changed, 71 insertions(+), 12 deletions(-) diff --git a/heat/api/middleware/fault.py b/heat/api/middleware/fault.py index 5c128710..b84ba66c 100644 --- a/heat/api/middleware/fault.py +++ b/heat/api/middleware/fault.py @@ -76,6 +76,15 @@ class FaultWrapper(wsgi.Middleware): def _error(self, ex): + trace = None + webob_exc = None + if isinstance(ex, exception.HTTPExceptionDisguise): + # An HTTP exception was disguised so it could make it here + # let's remove the disguise and set the original HTTP exception + trace = ''.join(traceback.format_tb(ex.tb)) + ex = ex.exc + webob_exc = ex + ex_type = ex.__class__.__name__ if ex_type.endswith(rpc_common._REMOTE_POSTFIX): @@ -89,14 +98,16 @@ class FaultWrapper(wsgi.Middleware): else: message = ex.message - trace = str(ex) - if trace.find('\n') > -1: - unused, trace = trace.split('\n', 1) - else: - trace = traceback.format_exc() + if not trace: + trace = str(ex) + if trace.find('\n') > -1: + unused, trace = trace.split('\n', 1) + else: + trace = traceback.format_exc() - webob_exc = self.error_map.get(ex_type, - webob.exc.HTTPInternalServerError) + if not webob_exc: + webob_exc = self.error_map.get(ex_type, + webob.exc.HTTPInternalServerError) error = { 'code': webob_exc.code, diff --git a/heat/common/exception.py b/heat/common/exception.py index f0006d54..2037122f 100644 --- a/heat/common/exception.py +++ b/heat/common/exception.py @@ -293,3 +293,13 @@ class ResourcePropertyConflict(OpenstackException): def __init__(self, *args): self.message = self.message % ", ".join(args) super(ResourcePropertyConflict, self).__init__() + + +class HTTPExceptionDisguise(Exception): + """Disguises HTTP exceptions so they can be handled by the webob fault + application in the wsgi pipeline. + """ + + def __init__(self, exception): + self.exc = exception + self.tb = sys.exc_info()[2] diff --git a/heat/common/wsgi.py b/heat/common/wsgi.py index 717455f9..ac38e6ce 100644 --- a/heat/common/wsgi.py +++ b/heat/common/wsgi.py @@ -593,11 +593,21 @@ class Resource(object): msg = _('The server could not comply with the request since\r\n' 'it is either malformed or otherwise incorrect.\r\n') err = webob.exc.HTTPBadRequest(msg) - raise translate_exception(err, request.best_match_language()) + http_exc = translate_exception(err, request.best_match_language()) + # NOTE(luisg): We disguise HTTP exceptions, otherwise they will be + # treated by wsgi as responses ready to be sent back and they + # won't make it into the pipeline app that serializes errors + raise exception.HTTPExceptionDisguise(http_exc) except webob.exc.HTTPException as err: + if isinstance(err, (webob.exc.HTTPOk, webob.exc.HTTPRedirection)): + # Some HTTPException are actually not errors, they are + # responses ready to be sent back to the users, so we don't + # error log, disguise or translate those + raise logging.error(_("Returning %(code)s to user: %(explanation)s"), {'code': err.code, 'explanation': err.explanation}) - raise translate_exception(err, request.best_match_language()) + http_exc = translate_exception(err, request.best_match_language()) + raise exception.HTTPExceptionDisguise(http_exc) except Exception as err: logging.error(_("Unexpected error occurred serving API: %s") % err) raise translate_exception(err, request.best_match_language()) diff --git a/heat/tests/test_api_openstack_v1.py b/heat/tests/test_api_openstack_v1.py index 9acfbd7b..375f9e39 100644 --- a/heat/tests/test_api_openstack_v1.py +++ b/heat/tests/test_api_openstack_v1.py @@ -13,6 +13,7 @@ # under the License. import json +import mock from oslo.config import cfg import webob.exc @@ -641,6 +642,27 @@ class StackControllerTest(ControllerTest, HeatTestCase): self.assertEqual(resp.json['error']['type'], 'StackValidationFailed') self.m.VerifyAll() + def test_create_err_stack_bad_reqest(self): + template = {u'Foo': u'bar'} + parameters = {u'InstanceType': u'm1.xlarge'} + body = {'template': template, + 'parameters': parameters, + 'timeout_mins': 30} + + req = self._post('/stacks', json.dumps(body)) + + error = heat_exc.HTTPExceptionDisguise(webob.exc.HTTPBadRequest()) + self.controller.create = mock.MagicMock(side_effect=error) + + resp = request_with_middleware(fault.FaultWrapper, + self.controller.create, req, body) + + # When HTTP disguised exceptions reach the fault app, they are + # converted into regular responses, just like non-HTTP exceptions + self.assertEqual(resp.json['code'], 400) + self.assertEqual(resp.json['error']['type'], 'HTTPBadRequest') + self.assertIsNotNone(resp.json['error']['traceback']) + def test_lookup(self): identity = identifier.HeatIdentifier(self.tenant, 'wordpress', '1') diff --git a/heat/tests/test_wsgi.py b/heat/tests/test_wsgi.py index ab26f254..4db0c631 100644 --- a/heat/tests/test_wsgi.py +++ b/heat/tests/test_wsgi.py @@ -182,7 +182,13 @@ class ResourceTest(HeatTestCase): resource = wsgi.Resource(Controller(), wsgi.JSONRequestDeserializer(), None) - self.assertRaises(webob.exc.HTTPBadRequest, resource, request) + # The Resource does not throw webob.HTTPExceptions, since they + # would be considered responses by wsgi and the request flow would end, + # instead they are wrapped so they can reach the fault application + # where they are converted to a nice JSON/XML response + e = self.assertRaises(exception.HTTPExceptionDisguise, + resource, request) + self.assertIsInstance(e.exc, webob.exc.HTTPBadRequest) def test_resource_call_error_handle_localized(self): class Controller(object): @@ -208,8 +214,8 @@ class ResourceTest(HeatTestCase): try: resource(request) - except webob.exc.HTTPBadRequest as e: - self.assertEquals(message_es, e.message) + except exception.HTTPExceptionDisguise as e: + self.assertEquals(message_es, e.exc.message) self.m.VerifyAll() -- 2.45.2