From 514338c3bf2f5fac1a1bb07df396827bdce2281b Mon Sep 17 00:00:00 2001 From: Felipe Monteiro Date: Tue, 24 Oct 2017 15:47:30 +0100 Subject: [PATCH] Add expected errors decorator for more resiliency To increase resiliency, add Deckhand error handling hooks to format unknown errors into something more useful for debugging. Also override exception formatting to be consistent with UCP error formatting standard. Most of this logic is borrowed from Shipyard for consistency. Also includes basic unit tests to validate error formatting. Change-Id: If7f8c3bf6b6ada7697611a0bef7bf8f635fc0b7f --- deckhand/control/revision_documents.py | 13 +- deckhand/errors.py | 132 ++++++++++++++++++ deckhand/service.py | 6 + deckhand/tests/unit/control/test_errors.py | 104 ++++++++++++++ .../tests/unit/control/test_middleware.py | 66 +++++++-- 5 files changed, 309 insertions(+), 12 deletions(-) create mode 100644 deckhand/tests/unit/control/test_errors.py diff --git a/deckhand/control/revision_documents.py b/deckhand/control/revision_documents.py index 639ab3f1..839323e7 100644 --- a/deckhand/control/revision_documents.py +++ b/deckhand/control/revision_documents.py @@ -14,6 +14,7 @@ import falcon from oslo_log import log as logging +import six from deckhand.control import base as api_base from deckhand.control import common @@ -58,6 +59,7 @@ class RevisionDocumentsResource(api_base.BaseResource): documents = db_api.revision_get_documents( revision_id, **filters) except errors.RevisionNotFound as e: + LOG.exception(six.text_type(e)) raise falcon.HTTPNotFound(description=e.format_message()) resp.status = falcon.HTTP_200 @@ -95,7 +97,8 @@ class RenderedDocumentsResource(api_base.BaseResource): try: documents = db_api.revision_get_documents( revision_id, **filters) - except (errors.RevisionNotFound) as e: + except errors.RevisionNotFound as e: + LOG.exception(six.text_type(e)) raise falcon.HTTPNotFound(description=e.format_message()) # TODO(fmontei): Currently the only phase of rendering that is @@ -104,7 +107,13 @@ class RenderedDocumentsResource(api_base.BaseResource): # a separate module that handles layering alongside substitution once # layering has been fully integrated into this endpoint. secrets_substitution = secrets_manager.SecretsSubstitution(documents) - rendered_documents = secrets_substitution.substitute_all() + try: + rendered_documents = secrets_substitution.substitute_all() + except errors.DocumentNotFound as e: + LOG.error('Failed to render the documents because a secret ' + 'document could not be found.') + LOG.exception(six.text_type(e)) + raise falcon.HTTPNotFound(description=e.format_message()) resp.status = falcon.HTTP_200 resp.body = self.view_builder.list(rendered_documents) diff --git a/deckhand/errors.py b/deckhand/errors.py index 443cf955..5de31341 100644 --- a/deckhand/errors.py +++ b/deckhand/errors.py @@ -12,6 +12,138 @@ # See the License for the specific language governing permissions and # limitations under the License. +import traceback +import yaml + +import falcon +from oslo_log import log as logging +import six + +LOG = logging.getLogger(__name__) + + +def get_version_from_request(req): + """Attempt to extract the API version string.""" + for part in req.path.split('/'): + if '.' in part and part.startswith('v'): + return part + return 'N/A' + + +def format_error_resp(req, + resp, + status_code=falcon.HTTP_500, + message="", + reason="", + error_type=None, + error_list=None, + info_list=None): + """Generate a error message body and throw a Falcon exception to trigger + an HTTP status. + + :param req: ``falcon`` request object. + :param resp: ``falcon`` response object to update. + :param status_code: ``falcon`` status_code constant. + :param message: Optional error message to include in the body. + This should be the summary level of the error + message, encompassing an overall result. If + no other messages are passed in the error_list, + this message will be repeated in a generated + message for the output message_list. + :param reason: Optional reason code to include in the body + :param error_type: If specified, the error type will be used; + otherwise, this will be set to + 'Unspecified Exception'. + :param error_list: optional list of error dictionaries. Minimally, + the dictionary will contain the 'message' field, + but should also contain 'error': ``True``. + :param info_list: optional list of info message dictionaries. + Minimally, the dictionary needs to contain a + 'message' field, but should also have a + 'error': ``False`` field. + """ + + if error_type is None: + error_type = 'Unspecified Exception' + + # Since we're handling errors here, if error list is None, set up a default + # error item. If we have info items, add them to the message list as well. + # In both cases, if the error flag is not set, set it appropriately. + if error_list is None: + error_list = [{'message': 'An error occurred, but was not specified', + 'error': True}] + else: + for error_item in error_list: + if 'error' not in error_item: + error_item['error'] = True + + if info_list is None: + info_list = [] + else: + for info_item in info_list: + if 'error' not in info_item: + info_item['error'] = False + + message_list = error_list + info_list + + error_response = { + 'kind': 'status', + 'apiVersion': get_version_from_request(req), + 'metadata': {}, + 'status': 'Failure', + 'message': message, + 'reason': reason, + 'details': { + 'errorType': error_type, + 'errorCount': len(error_list), + 'messageList': message_list + }, + 'code': status_code, + # TODO(fmontei): Make this class-specific later. For now, retry + # is set to True only for internal server errors. + 'retry': True if status_code is falcon.HTTP_500 else False + } + + resp.body = yaml.safe_dump(error_response) + resp.status = status_code + + +def default_exception_handler(ex, req, resp, params): + """Catch-all execption handler for standardized output. + + If this is a standard falcon HTTPError, rethrow it for handling by + ``default_exception_serializer`` below. + """ + if isinstance(ex, falcon.HTTPError): + # Allow the falcon http errors to bubble up and get handled. + raise ex + else: + # Take care of the uncaught stuff. + exc_string = traceback.format_exc() + LOG.error('Unhanded Exception being handled: \n%s', exc_string) + format_error_resp( + req, + resp, + error_type=ex.__class__.__name__, + message="Unhandled Exception raised: %s" % six.text_type(ex) + ) + + +def default_exception_serializer(req, resp, exception): + """Serializes instances of :class:`falcon.HTTPError` into YAML format and + formats the error body so it adheres to the UCP error formatting standard. + """ + format_error_resp( + req, + resp, + status_code=exception.status, + # TODO(fmontei): Provide an overall error message instead. + message=exception.description, + reason=exception.title, + error_type=exception.__class__.__name__, + error_list=[{'message': exception.description, 'error': True}] + ) + class DeckhandException(Exception): """Base Deckhand Exception diff --git a/deckhand/service.py b/deckhand/service.py index 028012c1..1ea92b8f 100644 --- a/deckhand/service.py +++ b/deckhand/service.py @@ -27,6 +27,7 @@ from deckhand.control import revisions from deckhand.control import rollback from deckhand.control import validations from deckhand.control import versions +from deckhand import errors CONF = cfg.CONF LOG = log.getLogger(__name__) @@ -61,6 +62,11 @@ def configure_app(app, version=''): app.add_route(os.path.join('/api/%s' % version, path), res) app.add_route('/versions', versions.VersionsResource()) + # Error handlers (FILO handling). + app.add_error_handler(Exception, errors.default_exception_handler) + # Built-in error serializer. + app.set_error_serializer(errors.default_exception_serializer) + return app diff --git a/deckhand/tests/unit/control/test_errors.py b/deckhand/tests/unit/control/test_errors.py new file mode 100644 index 00000000..385fcd1b --- /dev/null +++ b/deckhand/tests/unit/control/test_errors.py @@ -0,0 +1,104 @@ +# Copyright 2017 AT&T Intellectual Property. All other rights reserved. +# +# 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. + +import yaml + +import falcon +import mock + +from deckhand import policy +from deckhand.tests.unit.control import base as test_base + + +class TestErrorFormatting(test_base.BaseControllerTest): + """Test suite for validating error formatting. + + Use mocked exceptions below to guarantee consistent results. + """ + + def test_base_exception_formatting(self): + """Verify formatting for an exception class that inherits from + :class:`Exception`. + """ + with mock.patch.object(policy, '_do_enforce_rbac', autospec=True) \ + as m_enforce_rbac: + m_enforce_rbac.side_effect = Exception + resp = self.app.simulate_put( + '/api/v1.0/bucket/test/documents', + headers={'Content-Type': 'application/x-yaml'}, body=None) + + expected = { + 'status': 'Failure', + 'kind': 'status', + 'code': '500 Internal Server Error', + 'apiVersion': 'v1.0', + 'reason': '', + 'retry': True, + 'details': { + 'errorType': 'Exception', + 'errorCount': 1, + 'messageList': [ + { + 'message': 'An error occurred, but was not specified', + 'error': True + } + ] + }, + 'message': 'Unhandled Exception raised: ', + 'metadata': {} + } + body = yaml.safe_load(resp.text) + + self.assertEqual(500, resp.status_code) + self.assertEqual(expected, body) + + def test_falcon_exception_formatting(self): + """Verify formatting for an exception class that inherits from + :class:`falcon.HTTPError`. + """ + expected_msg = ( + 'deckhand:create_cleartext_documents is disallowed by policy') + + with mock.patch.object(policy, '_do_enforce_rbac', autospec=True) \ + as m_enforce_rbac: + m_enforce_rbac.side_effect = falcon.HTTPForbidden( + description=expected_msg) + resp = self.app.simulate_put( + '/api/v1.0/bucket/test/documents', + headers={'Content-Type': 'application/x-yaml'}, body=None) + + expected = { + 'status': 'Failure', + 'kind': 'status', + 'code': '403 Forbidden', + 'apiVersion': 'v1.0', + 'reason': '403 Forbidden', + 'retry': False, + 'details': { + 'errorType': 'HTTPForbidden', + 'errorCount': 1, + 'messageList': [ + { + 'message': expected_msg, + 'error': True + } + ] + }, + 'message': expected_msg, + 'metadata': {} + } + body = yaml.safe_load(resp.text) + + self.assertEqual(403, resp.status_code) + self.assertEqual(expected, body) diff --git a/deckhand/tests/unit/control/test_middleware.py b/deckhand/tests/unit/control/test_middleware.py index b60e7e47..51aaf538 100644 --- a/deckhand/tests/unit/control/test_middleware.py +++ b/deckhand/tests/unit/control/test_middleware.py @@ -14,6 +14,8 @@ import yaml +import mock + from deckhand.tests.unit.control import base as test_base @@ -38,8 +40,22 @@ class TestYAMLTranslatorNegative(test_base.BaseControllerTest): self.assertEqual(400, resp.status_code) expected = { - 'description': 'The Content-Type header is required.', - 'title': 'Missing header value' + 'apiVersion': mock.ANY, + 'code': '400 Bad Request', + 'details': { + 'errorCount': 1, + 'errorType': 'HTTPMissingHeader', + 'messageList': [{ + 'error': True, + 'message': "The Content-Type header is required." + }] + }, + 'kind': 'status', + 'message': "The Content-Type header is required.", + 'metadata': {}, + 'reason': 'Missing header value', + 'retry': False, + 'status': 'Failure' } self.assertEqual(expected, yaml.safe_load(resp.content)) @@ -49,10 +65,25 @@ class TestYAMLTranslatorNegative(test_base.BaseControllerTest): self.assertEqual(415, resp.status_code) expected = { - 'description': "Unexpected content type: application/json. " - "Expected content types are: " - "['application/x-yaml'].", - 'title': 'Unsupported media type' + 'apiVersion': mock.ANY, + 'code': '415 Unsupported Media Type', + 'details': { + 'errorCount': 1, + 'errorType': 'HTTPUnsupportedMediaType', + 'messageList': [{ + 'error': True, + 'message': ( + "Unexpected content type: application/json. Expected " + "content types are: ['application/x-yaml'].") + }] + }, + 'kind': 'status', + 'message': ("Unexpected content type: application/json. Expected " + "content types are: ['application/x-yaml']."), + 'metadata': {}, + 'reason': 'Unsupported media type', + 'retry': False, + 'status': 'Failure' } self.assertEqual(expected, yaml.safe_load(resp.content)) @@ -65,9 +96,24 @@ class TestYAMLTranslatorNegative(test_base.BaseControllerTest): self.assertEqual(415, resp.status_code) expected = { - 'description': "Unexpected content type: application/yaml. " - "Expected content types are: " - "['application/x-yaml'].", - 'title': 'Unsupported media type' + 'apiVersion': 'N/A', + 'code': '415 Unsupported Media Type', + 'details': { + 'errorCount': 1, + 'errorType': 'HTTPUnsupportedMediaType', + 'messageList': [{ + 'error': True, + 'message': ( + "Unexpected content type: application/yaml. Expected " + "content types are: ['application/x-yaml'].") + }] + }, + 'kind': 'status', + 'message': ("Unexpected content type: application/yaml. Expected " + "content types are: ['application/x-yaml']."), + 'metadata': {}, + 'reason': 'Unsupported media type', + 'retry': False, + 'status': 'Failure' } self.assertEqual(expected, yaml.safe_load(resp.content))