From 5f881ff030c119b50bf1929b9829f52493ac447c Mon Sep 17 00:00:00 2001 From: Chris Dent Date: Sat, 13 Jan 2018 16:11:51 +0000 Subject: [PATCH] [placement] use simple FaultWrapper Prior to this patch placement was using the nova FaultWrapper middleware as a final guard against unexpected exceptions. That middleware does more than required and also imports stuff that placement does not otherwise require. The new middleware is only for exceptions that have fallen through previous handling and need to be turned into 500 responses, formatted to JSON, and logged. While doing this it became clear that the limited Exception catching in handler.py was redundant if FaultWrapper and the Microversion middlewares (both required for the operation of the system) are involved, so that has been removed, with a comment. This should not require a microversion as the structure of successful responses and expected error responses does not change. What does change is that a 500 response will be formatted more as the api-wg errors guideline[1] desires. [1] http://specs.openstack.org/openstack/api-wg/guidelines/errors.html Change-Id: Ib2c3250b964a30239f298cdf13ea2020f205f67d Partial-Bug: #1743120 --- nova/api/openstack/placement/deploy.py | 6 +- nova/api/openstack/placement/fault_wrap.py | 48 ++++++++++++++ nova/api/openstack/placement/handler.py | 18 ++--- .../openstack/placement/test_fault_wrap.py | 66 +++++++++++++++++++ 4 files changed, 126 insertions(+), 12 deletions(-) create mode 100644 nova/api/openstack/placement/fault_wrap.py create mode 100644 nova/tests/unit/api/openstack/placement/test_fault_wrap.py diff --git a/nova/api/openstack/placement/deploy.py b/nova/api/openstack/placement/deploy.py index 053e43dd6911..bbd4d24eb56b 100644 --- a/nova/api/openstack/placement/deploy.py +++ b/nova/api/openstack/placement/deploy.py @@ -14,8 +14,8 @@ import oslo_middleware from oslo_middleware import cors -from nova.api import openstack as common_api from nova.api.openstack.placement import auth +from nova.api.openstack.placement import fault_wrap from nova.api.openstack.placement import handler from nova.api.openstack.placement import microversion from nova.api.openstack.placement import requestlog @@ -56,7 +56,7 @@ def deploy(conf): context_middleware = auth.PlacementKeystoneContext req_id_middleware = oslo_middleware.RequestId microversion_middleware = microversion.MicroversionMiddleware - fault_wrap = common_api.FaultWrapper + fault_middleware = fault_wrap.FaultWrapper request_log = requestlog.RequestLog application = handler.PlacementHandler() @@ -70,7 +70,7 @@ def deploy(conf): # all see the same contextual information including request id and # authentication information. for middleware in (microversion_middleware, - fault_wrap, + fault_middleware, request_log, context_middleware, auth_middleware, diff --git a/nova/api/openstack/placement/fault_wrap.py b/nova/api/openstack/placement/fault_wrap.py new file mode 100644 index 000000000000..764d628b496c --- /dev/null +++ b/nova/api/openstack/placement/fault_wrap.py @@ -0,0 +1,48 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or +# implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""Simple middleware for safely catching unexpected exceptions.""" + +# NOTE(cdent): This is a super simplified replacement for the nova +# FaultWrapper, which does more than placement needs. + +from oslo_log import log as logging +import six +from webob import exc + +from nova.api.openstack.placement import util + +LOG = logging.getLogger(__name__) + + +class FaultWrapper(object): + """Turn an uncaught exception into a status 500. + + Uncaught exceptions usually shouldn't happen, if it does it + means there is a bug in the placement service, which should be + fixed. + """ + + def __init__(self, application): + self.application = application + + def __call__(self, environ, start_response): + try: + return self.application(environ, start_response) + except Exception as unexpected_exception: + LOG.exception('Placement API unexpected error: %s', + unexpected_exception) + formatted_exception = exc.HTTPInternalServerError( + six.text_type(unexpected_exception)) + formatted_exception.json_formatter = util.json_error_formatter + return formatted_exception.generate_response( + environ, start_response) diff --git a/nova/api/openstack/placement/handler.py b/nova/api/openstack/placement/handler.py index 38ed64c51310..092125eaa6e9 100644 --- a/nova/api/openstack/placement/handler.py +++ b/nova/api/openstack/placement/handler.py @@ -223,12 +223,12 @@ class PlacementHandler(object): except exception.NotFound as exc: raise webob.exc.HTTPNotFound( exc, json_formatter=util.json_error_formatter) - # Trap the HTTPNotFound that can be raised by dispatch() - # when no route is found. The exception is passed through to - # the FaultWrap middleware without causing an alarming log - # message. - except webob.exc.HTTPNotFound: - raise - except Exception as exc: - LOG.exception("Uncaught exception") - raise + # Remaining uncaught exceptions will rise first to the Microversion + # middleware, where any WebOb generated exceptions will be caught and + # transformed into legit HTTP error responses (with microversion + # headers added), and then to the FaultWrapper middleware which will + # catch anything else and transform them into 500 responses. + # NOTE(cdent): There should be very few uncaught exceptions which are + # not WebOb exceptions at this stage as the handlers are contained by + # the wsgify decorator which will transform those exceptions to + # responses itself. diff --git a/nova/tests/unit/api/openstack/placement/test_fault_wrap.py b/nova/tests/unit/api/openstack/placement/test_fault_wrap.py new file mode 100644 index 000000000000..86ac0ad8f0b6 --- /dev/null +++ b/nova/tests/unit/api/openstack/placement/test_fault_wrap.py @@ -0,0 +1,66 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or +# implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""Tests for the placement fault wrap middleware.""" + +import mock + +from oslo_serialization import jsonutils +import webob + +from nova.api.openstack.placement import fault_wrap +from nova import test + + +ERROR_MESSAGE = 'that was not supposed to happen' + + +class Fault(Exception): + pass + + +class TestFaultWrapper(test.NoDBTestCase): + + @staticmethod + @webob.dec.wsgify + def failing_application(req): + raise Fault(ERROR_MESSAGE) + + def setUp(self): + super(TestFaultWrapper, self).setUp() + self.req = webob.Request.blank('/') + self.environ = self.req.environ + self.environ['HTTP_ACCEPT'] = 'application/json' + self.start_response_mock = mock.MagicMock() + self.fail_app = fault_wrap.FaultWrapper(self.failing_application) + + def test_fault_is_wrapped(self): + response = self.fail_app(self.environ, self.start_response_mock) + # response is a single member list + error_struct = jsonutils.loads(response[0]) + first_error = error_struct['errors'][0] + + self.assertIn(ERROR_MESSAGE, first_error['detail']) + self.assertEqual(500, first_error['status']) + self.assertEqual('Internal Server Error', first_error['title']) + + def test_fault_response_headers(self): + self.fail_app(self.environ, self.start_response_mock) + call_args = self.start_response_mock.call_args + self.assertEqual('500 Internal Server Error', call_args[0][0]) + + @mock.patch("nova.api.openstack.placement.fault_wrap.LOG") + def test_fault_log(self, mocked_log): + self.fail_app(self.environ, self.start_response_mock) + mocked_log.exception.assert_called_once_with( + 'Placement API unexpected error: %s', + mock.ANY)