From e65710bf1a16426f51487635c81015cccd460c23 Mon Sep 17 00:00:00 2001 From: Felipe Monteiro Date: Wed, 11 Apr 2018 20:06:10 +0100 Subject: [PATCH] Make Deckhand validation exceptions adhere to UCP standard This PS makes Deckhand raise an exception formatted including the list ValidationMessage-formatted error messages following any validation error. This adheres to the format specified under [0]. To accomplish this, logic was added to raise an exception with a status code corresponding to the `code` attribute for each DeckhandException subclass. This means it is no longer necessary to raise a specific falcon exception as the process has been automated. In addition, the 'reason' key in the UCP error exception message is now populated if specified for any DeckhandException instance. The same is true for 'error_list'. TODO (in a follow up): * Allow 'info_list' to specified for any DeckhandException instance. * Pass the 'reason' and 'error_list' and etc. arguments to all instances of DeckhandException that are raised. [0] https://github.com/att-comdev/ucp-integration/blob/master/docs/source/api-conventions.rst#output-structure Change-Id: I0cc2909f515ace762be805288981224fc5098c9c --- deckhand/common/validation_message.py | 62 ++++++++ deckhand/control/buckets.py | 20 ++- deckhand/control/revision_diffing.py | 9 +- deckhand/control/revision_documents.py | 56 +++++-- deckhand/control/revision_tags.py | 22 ++- deckhand/control/revisions.py | 7 +- deckhand/control/rollback.py | 10 +- deckhand/control/validations.py | 16 +- deckhand/control/views/document.py | 12 +- deckhand/engine/document_validation.py | 25 ++- deckhand/engine/layering.py | 34 ++-- deckhand/errors.py | 58 ++++--- .../schema-validation-success.yaml | 82 +++------- deckhand/tests/unit/control/test_errors.py | 149 ++++++++++++++++-- .../tests/unit/control/test_middleware.py | 12 +- .../test_rendered_documents_controller.py | 59 ++++++- .../engine/test_document_layering_negative.py | 20 +-- .../test_document_validation_negative.py | 13 +- 18 files changed, 480 insertions(+), 186 deletions(-) create mode 100644 deckhand/common/validation_message.py diff --git a/deckhand/common/validation_message.py b/deckhand/common/validation_message.py new file mode 100644 index 00000000..6bf41535 --- /dev/null +++ b/deckhand/common/validation_message.py @@ -0,0 +1,62 @@ +# Copyright 2018 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. + + +class ValidationMessage(object): + """ValidationMessage per UCP convention: + https://github.com/att-comdev/ucp-integration/blob/master/docs/source/api-conventions.rst#output-structure # noqa + + Construction of ``ValidationMessage`` message: + + :param string message: Validation failure message. + :param boolean error: True or False, if this is an error message. + :param string name: Identifying name of the validation. + :param string level: The severity of validation result, as "Error", + "Warning", or "Info" + :param string schema: The schema of the document being validated. + :param string doc_name: The name of the document being validated. + :param string diagnostic: Information about what lead to the message, + or details for resolution. + """ + + def __init__(self, + message='Document validation error.', + error=True, + name='Deckhand validation error', + level='Error', + doc_schema='', + doc_name='', + doc_layer='', + diagnostic=''): + level = 'Error' if error else 'Info' + self._output = { + 'message': message, + 'error': error, + 'name': name, + 'documents': [], + 'level': level, + 'kind': self.__class__.__name__ + } + self._output['documents'].append( + dict(schema=doc_schema, name=doc_name, layer=doc_layer)) + if diagnostic: + self._output.update(diagnostic=diagnostic) + + def format_message(self): + """Return ``ValidationMessage`` message. + + :returns: The ``ValidationMessage`` for the Validation API response. + :rtype: dict + """ + return self._output diff --git a/deckhand/control/buckets.py b/deckhand/control/buckets.py index ca4b12a6..4124a393 100644 --- a/deckhand/control/buckets.py +++ b/deckhand/control/buckets.py @@ -14,7 +14,7 @@ import falcon from oslo_log import log as logging -import six +from oslo_utils import excutils from deckhand.control import base as api_base from deckhand.control.views import document as document_view @@ -48,8 +48,8 @@ class BucketsResource(api_base.BaseResource): documents, data_schemas, pre_validate=True) validations = doc_validator.validate_all() except deckhand_errors.InvalidDocumentFormat as e: - LOG.exception(e.format_message()) - raise falcon.HTTPBadRequest(description=e.format_message()) + with excutils.save_and_reraise_exception(): + LOG.exception(e.format_message()) for document in documents: if secrets_manager.SecretsManager.requires_encryption(document): @@ -59,11 +59,10 @@ class BucketsResource(api_base.BaseResource): try: documents = self._prepare_secret_documents(documents) - except deckhand_errors.BarbicanException as e: - LOG.error('An unknown exception occurred while trying to store ' - 'a secret in Barbican.') - raise falcon.HTTPInternalServerError( - description=e.format_message()) + except deckhand_errors.BarbicanException: + with excutils.save_and_reraise_exception(): + LOG.error('An unknown exception occurred while trying to store' + ' a secret in Barbican.') created_documents = self._create_revision_documents( bucket_name, documents, validations) @@ -86,8 +85,7 @@ class BucketsResource(api_base.BaseResource): bucket_name, documents, validations=validations) except (deckhand_errors.DuplicateDocumentExists, deckhand_errors.SingletonDocumentConflict) as e: - raise falcon.HTTPConflict(description=e.format_message()) - except Exception as e: - raise falcon.HTTPInternalServerError(description=six.text_type(e)) + with excutils.save_and_reraise_exception(): + LOG.exception(e.format_message()) return created_documents diff --git a/deckhand/control/revision_diffing.py b/deckhand/control/revision_diffing.py index 85f48bcd..6a33fbb4 100644 --- a/deckhand/control/revision_diffing.py +++ b/deckhand/control/revision_diffing.py @@ -13,12 +13,16 @@ # limitations under the License. import falcon +from oslo_log import log as logging +from oslo_utils import excutils from deckhand.control import base as api_base from deckhand.db.sqlalchemy import api as db_api from deckhand import errors from deckhand import policy +LOG = logging.getLogger(__name__) + class RevisionDiffingResource(api_base.BaseResource): """API resource for realizing revision diffing.""" @@ -33,8 +37,9 @@ class RevisionDiffingResource(api_base.BaseResource): try: resp_body = db_api.revision_diff( revision_id, comparison_revision_id) - except (errors.RevisionNotFound) as e: - raise falcon.HTTPNotFound(description=e.format_message()) + except errors.RevisionNotFound as e: + with excutils.save_and_reraise_exception(): + LOG.exception(e.format_message()) resp.status = falcon.HTTP_200 resp.body = resp_body diff --git a/deckhand/control/revision_documents.py b/deckhand/control/revision_documents.py index 7c6be839..3cd23373 100644 --- a/deckhand/control/revision_documents.py +++ b/deckhand/control/revision_documents.py @@ -14,9 +14,11 @@ import falcon from oslo_log import log as logging +from oslo_utils import excutils import six from deckhand.common import utils +from deckhand.common.validation_message import ValidationMessage from deckhand.control import base as api_base from deckhand.control import common from deckhand.control.views import document as document_view @@ -118,17 +120,14 @@ class RenderedDocumentsResource(api_base.BaseResource): errors.InvalidDocumentParent, errors.InvalidDocumentReplacement, errors.IndeterminateDocumentParent, + errors.LayeringPolicyNotFound, errors.MissingDocumentKey, errors.SubstitutionSourceDataNotFound, + errors.SubstitutionSourceNotFound, + errors.UnknownSubstitutionError, errors.UnsupportedActionMethod) as e: - raise falcon.HTTPBadRequest(description=e.format_message()) - except (errors.LayeringPolicyNotFound, - errors.SubstitutionSourceNotFound) as e: - raise falcon.HTTPConflict(description=e.format_message()) - except (errors.DeckhandException, - errors.UnknownSubstitutionError) as e: - raise falcon.HTTPInternalServerError( - description=e.format_message()) + with excutils.save_and_reraise_exception(): + LOG.exception(e.format_message()) # Filters to be applied post-rendering, because many documents are # involved in rendering. User filters can only be applied once all @@ -187,12 +186,37 @@ class RenderedDocumentsResource(api_base.BaseResource): try: validations = doc_validator.validate_all() except errors.InvalidDocumentFormat as e: - LOG.error('Failed to post-validate rendered documents.') - LOG.exception(e.format_message()) - raise falcon.HTTPInternalServerError( - description=e.format_message()) + with excutils.save_and_reraise_exception(): + # Post-rendering validation errors likely indicate an internal + # rendering bug, so override the default code to 500. + e.code = 500 + LOG.error('Failed to post-validate rendered documents.') + LOG.exception(e.format_message()) else: - failed_validations = [ - v for v in validations if v['status'] == 'failure'] - if failed_validations: - raise falcon.HTTPBadRequest(description=failed_validations) + error_list = [] + + for validation in validations: + if validation['status'] == 'failure': + error_list.extend([ + ValidationMessage( + error=True, + message=error['message'], + doc_schema=error['schema'], + doc_name=error['name'], + doc_layer=error['layer'], + diagnostic={ + k: v for k, v in error.items() if k in ( + 'schema_path', + 'validation_schema', + 'error_section' + ) + } + ) + for error in validation['errors'] + ]) + + if error_list: + raise errors.InvalidDocumentFormat( + error_list=error_list, + reason='Validation' + ) diff --git a/deckhand/control/revision_tags.py b/deckhand/control/revision_tags.py index 8b5c47db..ab770579 100644 --- a/deckhand/control/revision_tags.py +++ b/deckhand/control/revision_tags.py @@ -14,6 +14,7 @@ import falcon from oslo_log import log as logging +from oslo_utils import excutils from deckhand.control import base as api_base from deckhand.control.views import revision_tag as revision_tag_view @@ -34,10 +35,11 @@ class RevisionTagsResource(api_base.BaseResource): try: resp_tag = db_api.revision_tag_create(revision_id, tag, tag_data) - except (errors.RevisionNotFound, errors.RevisionTagNotFound) as e: - raise falcon.HTTPNotFound(description=e.format_message()) - except errors.RevisionTagBadFormat as e: - raise falcon.HTTPBadRequest(description=e.format_message()) + except (errors.RevisionNotFound, + errors.RevisionTagBadFormat, + errors.errors.RevisionTagNotFound) as e: + with excutils.save_and_reraise_exception(): + LOG.exception(e.format_message()) resp_body = revision_tag_view.ViewBuilder().show(resp_tag) resp.status = falcon.HTTP_201 @@ -57,7 +59,8 @@ class RevisionTagsResource(api_base.BaseResource): resp_tag = db_api.revision_tag_get(revision_id, tag) except (errors.RevisionNotFound, errors.RevisionTagNotFound) as e: - raise falcon.HTTPNotFound(description=e.format_message()) + with excutils.save_and_reraise_exception(): + LOG.exception(e.format_message()) resp_body = revision_tag_view.ViewBuilder().show(resp_tag) resp.status = falcon.HTTP_200 @@ -69,7 +72,8 @@ class RevisionTagsResource(api_base.BaseResource): try: resp_tags = db_api.revision_tag_get_all(revision_id) except errors.RevisionNotFound as e: - raise falcon.HTTPNotFound(e.format_message()) + with excutils.save_and_reraise_exception(): + LOG.exception(e.format_message()) resp_body = revision_tag_view.ViewBuilder().list(resp_tags) resp.status = falcon.HTTP_200 @@ -89,7 +93,8 @@ class RevisionTagsResource(api_base.BaseResource): db_api.revision_tag_delete(revision_id, tag) except (errors.RevisionNotFound, errors.RevisionTagNotFound) as e: - raise falcon.HTTPNotFound(description=e.format_message()) + with excutils.save_and_reraise_exception(): + LOG.exception(e.format_message()) resp.status = falcon.HTTP_204 @@ -99,6 +104,7 @@ class RevisionTagsResource(api_base.BaseResource): try: db_api.revision_tag_delete_all(revision_id) except errors.RevisionNotFound as e: - raise falcon.HTTPNotFound(description=e.format_message()) + with excutils.save_and_reraise_exception(): + LOG.exception(e.format_message()) resp.status = falcon.HTTP_204 diff --git a/deckhand/control/revisions.py b/deckhand/control/revisions.py index c6ce56b0..4e988b38 100644 --- a/deckhand/control/revisions.py +++ b/deckhand/control/revisions.py @@ -13,6 +13,8 @@ # limitations under the License. import falcon +from oslo_log import log as logging +from oslo_utils import excutils from deckhand.common import utils from deckhand.control import base as api_base @@ -22,6 +24,8 @@ from deckhand.db.sqlalchemy import api as db_api from deckhand import errors from deckhand import policy +LOG = logging.getLogger(__name__) + class RevisionsResource(api_base.BaseResource): """API resource for realizing CRUD operations for revisions.""" @@ -50,7 +54,8 @@ class RevisionsResource(api_base.BaseResource): try: revision = db_api.revision_get(revision_id) except errors.RevisionNotFound as e: - raise falcon.HTTPNotFound(description=e.format_message()) + with excutils.save_and_reraise_exception(): + LOG.exception(e.format_message()) revision_resp = self.view_builder.show(revision) resp.status = falcon.HTTP_200 diff --git a/deckhand/control/rollback.py b/deckhand/control/rollback.py index d03e9385..97988b05 100644 --- a/deckhand/control/rollback.py +++ b/deckhand/control/rollback.py @@ -13,6 +13,8 @@ # limitations under the License. import falcon +from oslo_log import log as logging +from oslo_utils import excutils from deckhand.control import base as api_base from deckhand.control.views import revision as revision_view @@ -20,6 +22,8 @@ from deckhand.db.sqlalchemy import api as db_api from deckhand import errors from deckhand import policy +LOG = logging.getLogger(__name__) + class RollbackResource(api_base.BaseResource): """API resource for realizing revision rollback.""" @@ -31,7 +35,8 @@ class RollbackResource(api_base.BaseResource): try: latest_revision = db_api.revision_get_latest() except errors.RevisionNotFound as e: - raise falcon.HTTPNotFound(description=e.format_message()) + with excutils.save_and_reraise_exception(): + LOG.exception(e.format_message()) for document in latest_revision['documents']: if document['metadata'].get('storagePolicy') == 'encrypted': @@ -43,7 +48,8 @@ class RollbackResource(api_base.BaseResource): rollback_revision = db_api.revision_rollback( revision_id, latest_revision) except errors.InvalidRollback as e: - raise falcon.HTTPBadRequest(description=e.format_message()) + with excutils.save_and_reraise_exception(): + LOG.exception(e.format_message()) revision_resp = self.view_builder.show(rollback_revision) resp.status = falcon.HTTP_201 diff --git a/deckhand/control/validations.py b/deckhand/control/validations.py index 1fa234d6..02ef5ec8 100644 --- a/deckhand/control/validations.py +++ b/deckhand/control/validations.py @@ -14,6 +14,7 @@ import falcon from oslo_log import log as logging +from oslo_utils import excutils from deckhand.control import base as api_base from deckhand.control.views import validation as validation_view @@ -44,7 +45,8 @@ class ValidationsResource(api_base.BaseResource): resp_body = db_api.validation_create( revision_id, validation_name, validation_data) except errors.RevisionNotFound as e: - raise falcon.HTTPNotFound(description=e.format_message()) + with excutils.save_and_reraise_exception(): + LOG.exception(e.format_message()) resp.status = falcon.HTTP_201 resp.append_header('Content-Type', 'application/x-yaml') @@ -77,8 +79,10 @@ class ValidationsResource(api_base.BaseResource): try: entry = db_api.validation_get_entry( revision_id, validation_name, entry_id) - except (errors.RevisionNotFound, errors.ValidationNotFound) as e: - raise falcon.HTTPNotFound(description=e.format_message()) + except (errors.RevisionNotFound, + errors.ValidationNotFound) as e: + with excutils.save_and_reraise_exception(): + LOG.exception(e.format_message()) resp_body = self.view_builder.show_entry(entry) return resp_body @@ -90,7 +94,8 @@ class ValidationsResource(api_base.BaseResource): entries = db_api.validation_get_all_entries(revision_id, validation_name) except errors.RevisionNotFound as e: - raise falcon.HTTPNotFound(description=e.format_message()) + with excutils.save_and_reraise_exception(): + LOG.exception(e.format_message()) resp_body = self.view_builder.list_entries(entries) return resp_body @@ -100,7 +105,8 @@ class ValidationsResource(api_base.BaseResource): try: validations = db_api.validation_get_all(revision_id) except errors.RevisionNotFound as e: - raise falcon.HTTPNotFound(description=e.format_message()) + with excutils.save_and_reraise_exception(): + LOG.exception(e.format_message()) resp_body = self.view_builder.list(validations) return resp_body diff --git a/deckhand/control/views/document.py b/deckhand/control/views/document.py index ed9d47d7..548b9084 100644 --- a/deckhand/control/views/document.py +++ b/deckhand/control/views/document.py @@ -35,14 +35,14 @@ class ViewBuilder(common.ViewBuilder): attrs = ['id', 'metadata', 'data', 'schema'] for document in documents: - if document['deleted']: + if document.get('deleted'): continue if document['schema'].startswith(types.VALIDATION_POLICY_SCHEMA): continue - resp_obj = {x: document[x] for x in attrs} + resp_obj = {x: document.get(x) for x in attrs} resp_obj.setdefault('status', {}) - resp_obj['status']['bucket'] = document['bucket_name'] - resp_obj['status']['revision'] = document['revision_id'] + resp_obj['status']['bucket'] = document.get('bucket_name') + resp_obj['status']['revision'] = document.get('revision_id') resp_list.append(resp_obj) # Edge case for when all documents are deleted from a bucket. To detect @@ -53,8 +53,8 @@ class ViewBuilder(common.ViewBuilder): # across all the documents in ``documents``. if not resp_list and documents: resp_obj = {'status': {}} - resp_obj['status']['bucket'] = documents[0]['bucket_name'] - resp_obj['status']['revision'] = documents[0]['revision_id'] + resp_obj['status']['bucket'] = documents[0].get('bucket_name') + resp_obj['status']['revision'] = documents[0].get('revision_id') return [resp_obj] return resp_list diff --git a/deckhand/engine/document_validation.py b/deckhand/engine/document_validation.py index 304593dc..e6f607b4 100644 --- a/deckhand/engine/document_validation.py +++ b/deckhand/engine/document_validation.py @@ -25,6 +25,7 @@ import six from deckhand.common import document as document_wrapper from deckhand.common import utils +from deckhand.common.validation_message import ValidationMessage from deckhand.engine.secrets_manager import SecretsSubstitution from deckhand import errors from deckhand import types @@ -107,6 +108,11 @@ class GenericValidator(BaseValidator): __slots__ = ('base_schema') + _diagnostic = ( + 'Ensure that each document has a metadata, schema and data section. ' + 'Each document must pass the schema defined under: ' + 'http://deckhand.readthedocs.io/en/latest/validation.html#base-schema') + def __init__(self): super(GenericValidator, self).__init__() self.base_schema = self._schema_map['v1']['deckhand/Base'] @@ -149,8 +155,16 @@ class GenericValidator(BaseValidator): 'Details: %s', document.schema, document.layer, document.name, error_messages) raise errors.InvalidDocumentFormat( - schema=document.schema, name=document.name, - layer=document.layer, errors=', '.join(error_messages)) + error_list=[ + ValidationMessage(message=message, + doc_schema=document.schema, + doc_name=document.name, + doc_layer=document.layer, + diagnostic=self._diagnostic) + for message in error_messages + ], + reason='Validation' + ) class DataSchemaValidator(GenericValidator): @@ -430,6 +444,7 @@ class DocumentValidation(object): } formatted_results = [] + for result in results: formatted_result = { 'name': types.DECKHAND_SCHEMA_VALIDATION, @@ -459,8 +474,7 @@ class DocumentValidation(object): error_outputs = validator.validate( document, pre_validate=self._pre_validate) if error_outputs: - for error_output in error_outputs: - result['errors'].append(error_output) + result['errors'].extend(error_outputs) if result['errors']: result.setdefault('status', 'failure') @@ -503,5 +517,4 @@ class DocumentValidation(object): result = self._validate_one(document) validation_results.append(result) - validations = self._format_validation_results(validation_results) - return validations + return self._format_validation_results(validation_results) diff --git a/deckhand/engine/layering.py b/deckhand/engine/layering.py index 9aca24fe..bc04c506 100644 --- a/deckhand/engine/layering.py +++ b/deckhand/engine/layering.py @@ -24,6 +24,7 @@ from oslo_utils import excutils from deckhand.common import document as document_wrapper from deckhand.common import utils +from deckhand.common.validation_message import ValidationMessage from deckhand.engine import document_validation from deckhand.engine import secrets_manager from deckhand.engine import utils as engine_utils @@ -343,21 +344,24 @@ class DocumentLayering(object): validator = document_validation.DocumentValidation( documents, pre_validate=True) results = validator.validate_all() - val_errors = [] + + error_list = [] for result in results: - val_errors.extend( - [(e['schema'], e['layer'], e['name'], e['message']) - for e in result['errors']]) - if val_errors: - for error in val_errors: - LOG.error( - 'Document [%s, %s] %s failed with pre-validation error: ' - '%s.', *error) - raise errors.InvalidDocumentFormat( - schema=', '.join(v[0] for v in val_errors), - layer=', '.join(v[1] for v in val_errors), - name=', '.join(v[2] for v in val_errors), - errors=', '.join(v[3] for v in val_errors)) + for e in result['errors']: + LOG.error('Document [%s, %s] %s failed with pre-validation ' + 'error: %s.', e['schema'], e['layer'], e['name'], + e['message']) + error_list.append( + ValidationMessage( + message=e['message'], + doc_schema=e['schema'], + doc_name=e['name'], + doc_layer=e['layer'] + ) + ) + + if error_list: + raise errors.InvalidDocumentFormat(error_list=error_list) def __init__(self, documents, substitution_sources=None, validate=True, fail_on_missing_sub_src=True): @@ -536,8 +540,10 @@ class DocumentLayering(object): if from_child is None: raise errors.MissingDocumentKey( child_schema=child_data.schema, + child_layer=child_data.layer, child_name=child_data.name, parent_schema=overall_data.schema, + parent_layer=overall_data.layer, parent_name=overall_data.name, action=action) diff --git a/deckhand/errors.py b/deckhand/errors.py index f6fb234d..5918d3b5 100644 --- a/deckhand/errors.py +++ b/deckhand/errors.py @@ -12,7 +12,6 @@ # See the License for the specific language governing permissions and # limitations under the License. -import traceback import yaml import falcon @@ -34,7 +33,7 @@ def format_error_resp(req, resp, status_code=falcon.HTTP_500, message="", - reason="", + reason=None, error_type=None, error_list=None, info_list=None): @@ -63,21 +62,20 @@ def format_error_resp(req, 'error': ``False`` field. """ - if error_type is None: - error_type = 'Unspecified Exception' + error_type = error_type or 'Unspecified Exception' + reason = reason or 'Unspecified' # 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}] + if not error_list: + error_list = [{'message': message, 'error': True}] else: for error_item in error_list: if 'error' not in error_item: error_item['error'] = True - if info_list is None: + if not info_list: info_list = [] else: for info_item in info_list: @@ -87,7 +85,7 @@ def format_error_resp(req, message_list = error_list + info_list error_response = { - 'kind': 'status', + 'kind': 'Status', 'apiVersion': get_version_from_request(req), 'metadata': {}, 'status': 'Failure', @@ -104,23 +102,35 @@ def format_error_resp(req, 'retry': True if status_code is falcon.HTTP_500 else False } - resp.body = yaml.safe_dump(error_response) + # Don't use yaml.safe_dump to handle unicode correctly. + resp.body = yaml.dump(error_response) resp.status = status_code def default_exception_handler(ex, req, resp, params): - """Catch-all execption handler for standardized output. + """Catch-all exception 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. + # Allow the falcon HTTP errors to bubble up and get handled. raise ex + elif isinstance(ex, DeckhandException): + status_code = (getattr(falcon, 'HTTP_%d' % ex.code, falcon.HTTP_500) + if hasattr(ex, 'code') else falcon.HTTP_500) + + format_error_resp( + req, + resp, + status_code=status_code, + message=ex.message, + error_type=ex.__class__.__name__, + error_list=getattr(ex, 'error_list', None), + reason=getattr(ex, 'reason', None) + ) 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, @@ -139,9 +149,9 @@ def default_exception_serializer(req, resp, exception): 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}] + error_list=getattr(exception, 'error_list', None), + reason=getattr(exception, 'reason', None) ) @@ -164,6 +174,18 @@ class DeckhandException(Exception): message = self.msg_fmt self.message = message + self.reason = kwargs.pop('reason', None) + + error_list = kwargs.pop('error_list', []) + self.error_list = [] + + for error in error_list: + if isinstance(error, str): + error = {'message': error, 'error': True} + else: + error = error.format_message() + self.error_list.append(error) + super(DeckhandException, self).__init__(message) def format_message(self): @@ -175,8 +197,7 @@ class InvalidDocumentFormat(DeckhandException): **Troubleshoot:** """ - msg_fmt = ("The provided document(s) schema=%(schema)s, layer=%(layer)s, " - "name=%(name)s failed schema validation. Errors: %(errors)s") + msg_fmt = ("The provided documents failed schema validation.") code = 400 @@ -206,6 +227,7 @@ class InvalidDocumentParent(DeckhandException): msg_fmt = ("The document parent [%(parent_schema)s] %(parent_name)s is " "invalid for document [%(document_schema)s] %(document_name)s. " "Reason: %(reason)s") + code = 400 class IndeterminateDocumentParent(DeckhandException): diff --git a/deckhand/tests/functional/gabbits/schema-validation/schema-validation-success.yaml b/deckhand/tests/functional/gabbits/schema-validation/schema-validation-success.yaml index f4cce45a..e2e51a5e 100644 --- a/deckhand/tests/functional/gabbits/schema-validation/schema-validation-success.yaml +++ b/deckhand/tests/functional/gabbits/schema-validation/schema-validation-success.yaml @@ -145,37 +145,22 @@ tests: status: 400 response_multidoc_jsonpaths: $.`len`: 1 + $.[0].apiVersion: v1.0 + $.[0].code: 400 Bad Request + $.[0].details.errorCount: 1 + $.[0].details.errorType: InvalidDocumentFormat + $.[0].details.messageList[0].documents: + - layer: site + name: bad + schema: example/Doc/v1 + $.[0].details.messageList[0].error: true + $.[0].details.messageList[0].kind: ValidationMessage + $.[0].details.messageList[0].level: Error + $.[0].details.messageList[0].name: Deckhand validation error + $.[0].kind: Status + $.[0].message: The provided documents failed schema validation. + $.[0].reason: Validation $.[0].status: Failure - $.[0].message: - - errors: - - validation_schema: - "$schema": http://json-schema.org/schema# - properties: - a: - type: string - b: - maximum: 100 - type: integer - minimum: 0 - type: object - required: - - a - - b - additionalProperties: false - error_section: - a: this-one-is-required-and-can-be-different - b: 177 - schema_path: ".properties.b.maximum" - name: bad - schema: example/Doc/v1 - layer: site - path: ".data.b" - message: 177 is greater than the maximum of 100 - name: deckhand-schema-validation - validator: - name: deckhand - version: '1.0' - status: failure - name: add_invalid_document_with_substitutions desc: Add a document that does not follow the schema @@ -216,34 +201,9 @@ tests: status: 400 response_multidoc_jsonpaths: $.`len`: 1 - $.[0].status: Failure - $.[0].message: - - errors: - - name: bad - layer: site - schema: example/Doc/v1 - path: .data.b - schema_path: .properties.b.maximum - error_section: - a: Sanitized to avoid exposing secret. - b: 177 - message: 177 is greater than the maximum of 100 - validation_schema: - $schema: http://json-schema.org/schema# - additionalProperties: False - properties: - a: - type: string - b: - maximum: 100 - minimum: 0 - type: integer - required: - - a - - b - type: object - name: deckhand-schema-validation - validator: - name: deckhand - version: '1.0' - status: failure + $.[0].code: 400 Bad Request + $.[0].details.errorCount: 1 + $.[0].details.errorType: InvalidDocumentFormat + $.[0].details.messageList[0].diagnostic.error_section: + a: 'Sanitized to avoid exposing secret.' + b: 177 diff --git a/deckhand/tests/unit/control/test_errors.py b/deckhand/tests/unit/control/test_errors.py index 195ed4a7..c0dcbfb2 100644 --- a/deckhand/tests/unit/control/test_errors.py +++ b/deckhand/tests/unit/control/test_errors.py @@ -12,6 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +import os import yaml import falcon @@ -22,44 +23,41 @@ from deckhand.tests.unit.control import base as test_base class TestErrorFormatting(test_base.BaseControllerTest): - """Test suite for validating error formatting. + """Test suite for validating error formatting.""" - Use mocked exceptions below to guarantee consistent results. - """ - - def test_base_exception_formatting(self): + def test_python_exception_formatting(self): """Verify formatting for an exception class that inherits from :class:`Exception`. """ with mock.patch.object( policy, '_do_enforce_rbac', spec_set=policy._do_enforce_rbac) as m_enforce_rbac: - m_enforce_rbac.side_effect = Exception + m_enforce_rbac.side_effect = Exception('test error') resp = self.app.simulate_put( '/api/v1.0/buckets/test/documents', headers={'Content-Type': 'application/x-yaml'}, body=None) expected = { 'status': 'Failure', - 'kind': 'status', + 'kind': 'Status', 'code': '500 Internal Server Error', 'apiVersion': 'v1.0', - 'reason': '', + 'reason': 'Unspecified', 'retry': True, 'details': { 'errorType': 'Exception', 'errorCount': 1, 'messageList': [ { - 'message': 'An error occurred, but was not specified', + 'message': 'Unhandled Exception raised: test error', 'error': True } ] }, - 'message': 'Unhandled Exception raised: ', + 'message': 'Unhandled Exception raised: test error', 'metadata': {} } - body = yaml.safe_load(resp.text) + body = yaml.load(resp.text) self.assertEqual(500, resp.status_code) self.assertEqual(expected, body) @@ -82,10 +80,10 @@ class TestErrorFormatting(test_base.BaseControllerTest): expected = { 'status': 'Failure', - 'kind': 'status', + 'kind': 'Status', 'code': '403 Forbidden', 'apiVersion': 'v1.0', - 'reason': '403 Forbidden', + 'reason': 'Unspecified', 'retry': False, 'details': { 'errorType': 'HTTPForbidden', @@ -104,3 +102,128 @@ class TestErrorFormatting(test_base.BaseControllerTest): self.assertEqual(403, resp.status_code) self.assertEqual(expected, body) + + +class TestValidationMessageFormatting(test_base.BaseControllerTest): + """Test suite for validating :class:`ValidationMessage` formatting.""" + + def test_put_bucket_validation_message_formatting(self): + """Verify formatting for pre-validation during updating a bucket.""" + rules = {'deckhand:create_cleartext_documents': '@'} + self.policy.set_rules(rules) + + resp = self.app.simulate_put( + '/api/v1.0/buckets/test/documents', + headers={'Content-Type': 'application/x-yaml'}, + body='name: test') + + expected = { + 'status': 'Failure', + 'kind': 'Status', + 'code': '400 Bad Request', + 'apiVersion': 'v1.0', + 'reason': 'Validation', + 'retry': False, + 'details': { + 'errorType': 'InvalidDocumentFormat', + 'errorCount': 2, + 'messageList': [ + { + 'diagnostic': mock.ANY, + 'documents': [{ + 'layer': None, + 'name': None, + 'schema': '' + }], + 'error': True, + 'kind': 'ValidationMessage', + 'level': 'Error', + 'message': mock.ANY, + 'name': 'Deckhand validation error' + }, + { + 'diagnostic': mock.ANY, + 'documents': [{ + 'layer': None, + 'name': None, + 'schema': '' + }], + 'error': True, + 'kind': 'ValidationMessage', + 'level': 'Error', + 'message': mock.ANY, + 'name': 'Deckhand validation error' + } + ] + }, + 'message': 'The provided documents failed schema validation.', + 'metadata': {} + } + body = yaml.safe_load(resp.text) + + self.assertEqual(400, resp.status_code) + self.assertEqual(expected, body) + + def test_rendered_documents_validation_message_formatting(self): + """Verify formatting for post-validation during rendering revision + documents. + """ + rules = {'deckhand:create_cleartext_documents': '@', + 'deckhand:list_cleartext_documents': '@', + 'deckhand:list_encrypted_documents': '@'} + self.policy.set_rules(rules) + + yaml_file = os.path.join(os.getcwd(), 'deckhand', 'tests', 'unit', + 'resources', 'sample_layering_policy.yaml') + with open(yaml_file) as yaml_stream: + payload = yaml_stream.read() + + resp = self.app.simulate_put( + '/api/v1.0/buckets/test/documents', + headers={'Content-Type': 'application/x-yaml'}, + body=payload) + + with mock.patch('deckhand.control.revision_documents.db_api' + '.revision_documents_get', autospec=True) \ + as mock_get_rev_documents: + invalid_document = yaml.safe_load(payload) + invalid_document.pop('metadata') + + mock_get_rev_documents.return_value = [invalid_document] + resp = self.app.simulate_get( + '/api/v1.0/revisions/1/rendered-documents', + headers={'Content-Type': 'application/x-yaml'}) + + expected = { + 'status': 'Failure', + 'kind': 'Status', + 'code': '500 Internal Server Error', + 'apiVersion': 'v1.0', + 'reason': 'Validation', + 'retry': True, + 'details': { + 'errorType': 'InvalidDocumentFormat', + 'errorCount': 1, + 'messageList': [ + { + 'diagnostic': mock.ANY, + 'documents': [{ + 'layer': None, + 'name': None, + 'schema': invalid_document['schema'] + }], + 'error': True, + 'kind': 'ValidationMessage', + 'level': 'Error', + 'message': mock.ANY, + 'name': 'Deckhand validation error' + } + ] + }, + 'message': 'The provided documents failed schema validation.', + 'metadata': {} + } + body = yaml.safe_load(resp.text) + + self.assertEqual(500, 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 c2c76442..55fa862d 100644 --- a/deckhand/tests/unit/control/test_middleware.py +++ b/deckhand/tests/unit/control/test_middleware.py @@ -121,10 +121,10 @@ class TestYAMLTranslatorNegative(test_base.BaseControllerTest): 'message': "The Content-Type header is required." }] }, - 'kind': 'status', + 'kind': 'Status', 'message': "The Content-Type header is required.", 'metadata': {}, - 'reason': 'Missing header value', + 'reason': 'Unspecified', 'retry': False, 'status': 'Failure' } @@ -153,11 +153,11 @@ class TestYAMLTranslatorNegative(test_base.BaseControllerTest): "content types are: ['application/x-yaml'].") }] }, - 'kind': 'status', + 'kind': 'Status', 'message': ("Unexpected content type: application/json. Expected " "content types are: ['application/x-yaml']."), 'metadata': {}, - 'reason': 'Unsupported media type', + 'reason': 'Unspecified', 'retry': False, 'status': 'Failure' } @@ -188,11 +188,11 @@ class TestYAMLTranslatorNegative(test_base.BaseControllerTest): "content types are: ['application/x-yaml'].") }] }, - 'kind': 'status', + 'kind': 'Status', 'message': ("Unexpected content type: application/yaml. Expected " "content types are: ['application/x-yaml']."), 'metadata': {}, - 'reason': 'Unsupported media type', + 'reason': 'Unspecified', 'retry': False, 'status': 'Failure' } diff --git a/deckhand/tests/unit/control/test_rendered_documents_controller.py b/deckhand/tests/unit/control/test_rendered_documents_controller.py index 13211e9d..d671f799 100644 --- a/deckhand/tests/unit/control/test_rendered_documents_controller.py +++ b/deckhand/tests/unit/control/test_rendered_documents_controller.py @@ -200,8 +200,8 @@ class TestRenderedDocumentsControllerNegative( test_base.BaseControllerTest): def test_rendered_documents_fail_schema_validation(self): - """Validates that when fully rendered documents fail schema validation, - the controller raises a 500 Internal Server Error. + """Validates that when fully rendered documents fail basic schema + validation (sanity-checking), a 500 is raised. """ rules = {'deckhand:list_cleartext_documents': '@', 'deckhand:list_encrypted_documents': '@', @@ -232,6 +232,61 @@ class TestRenderedDocumentsControllerNegative( # schema validation. self.assertEqual(500, resp.status_code) + def test_rendered_documents_fail_post_validation(self): + """Validates that when fully rendered documents fail schema validation, + a 400 is raised. + + For this scenario a DataSchema checks that the relevant document has + a key in its data section, a key which is removed during the rendering + process as the document uses a delete action. This triggers + post-rendering validation failure. + """ + rules = {'deckhand:list_cleartext_documents': '@', + 'deckhand:list_encrypted_documents': '@', + 'deckhand:create_cleartext_documents': '@'} + self.policy.set_rules(rules) + + # Create a document for a bucket. + documents_factory = factories.DocumentFactory(2, [1, 1]) + payload = documents_factory.gen_test({ + "_GLOBAL_DATA_1_": {"data": {"a": "b"}}, + "_SITE_DATA_1_": {"data": {"a": "b"}}, + "_SITE_ACTIONS_1_": { + "actions": [{"method": "delete", "path": "."}] + } + }, site_abstract=False) + + data_schema_factory = factories.DataSchemaFactory() + metadata_name = payload[-1]['schema'] + schema_to_use = { + '$schema': 'http://json-schema.org/schema#', + 'type': 'object', + 'properties': { + 'a': { + 'type': 'string' + } + }, + 'required': ['a'], + 'additionalProperties': False + } + data_schema = data_schema_factory.gen_test( + metadata_name, data=schema_to_use) + payload.append(data_schema) + + resp = self.app.simulate_put( + '/api/v1.0/buckets/mop/documents', + headers={'Content-Type': 'application/x-yaml'}, + body=yaml.safe_dump_all(payload)) + self.assertEqual(200, resp.status_code) + revision_id = list(yaml.safe_load_all(resp.text))[0]['status'][ + 'revision'] + + resp = self.app.simulate_get( + '/api/v1.0/revisions/%s/rendered-documents' % revision_id, + headers={'Content-Type': 'application/x-yaml'}) + + self.assertEqual(400, resp.status_code) + class TestRenderedDocumentsControllerNegativeRBAC( test_base.BaseControllerTest): diff --git a/deckhand/tests/unit/engine/test_document_layering_negative.py b/deckhand/tests/unit/engine/test_document_layering_negative.py index 1fec64fd..0500d27d 100644 --- a/deckhand/tests/unit/engine/test_document_layering_negative.py +++ b/deckhand/tests/unit/engine/test_document_layering_negative.py @@ -277,13 +277,15 @@ class TestDocumentLayeringValidationNegative( layering_policy = copy.deepcopy(lp_template) del layering_policy['data']['layerOrder'] - error_re = ("The provided document\(s\) schema=%s, layer=%s, name=%s " - "failed schema validation. Errors: 'layerOrder' is a " - "required property" % ( - layering_policy['schema'], - layering_policy['metadata']['layeringDefinition'][ - 'layer'], - layering_policy['metadata']['name'])) - self.assertRaisesRegexp( - errors.InvalidDocumentFormat, error_re, self._test_layering, + error_re = r"^'layerOrder' is a required property$" + e = self.assertRaises( + errors.InvalidDocumentFormat, self._test_layering, [layering_policy, document], validate=True) + self.assertRegex(e.error_list[0]['message'], error_re) + self.assertEqual(layering_policy['schema'], + e.error_list[0]['documents'][0]['schema']) + self.assertEqual(layering_policy['metadata']['name'], + e.error_list[0]['documents'][0]['name']) + self.assertEqual(layering_policy['metadata']['layeringDefinition'][ + 'layer'], + e.error_list[0]['documents'][0]['layer']) diff --git a/deckhand/tests/unit/engine/test_document_validation_negative.py b/deckhand/tests/unit/engine/test_document_validation_negative.py index 188533f9..251f74b9 100644 --- a/deckhand/tests/unit/engine/test_document_validation_negative.py +++ b/deckhand/tests/unit/engine/test_document_validation_negative.py @@ -149,21 +149,22 @@ class TestDocumentValidationNegative(test_base.TestDocumentValidationBase): parts = property_to_remove.split('.') missing_property = parts[-1] - expected_err = "'%s' is a required property" % missing_property - self.assertIn(expected_err, e.message) + error_re = r"%s is a required property" % missing_property + self.assertRegex(str(e.error_list).replace("\'", ""), error_re) def test_document_invalid_layering_definition_action(self): document = self._read_data('sample_document') missing_data = self._corrupt_data( document, 'metadata.layeringDefinition.actions.0.method', 'invalid', op='replace') - expected_err = ( - r".+ 'invalid' is not one of \['replace', 'delete', 'merge'\]") + error_re = ( + r".*invalid is not one of \[replace, delete, merge\]") payload = [missing_data] doc_validator = document_validation.DocumentValidation(payload) - self.assertRaisesRegexp(errors.InvalidDocumentFormat, expected_err, - doc_validator.validate_all) + e = self.assertRaises(errors.InvalidDocumentFormat, + doc_validator.validate_all) + self.assertRegex(str(e.error_list[0]).replace("\'", ""), error_re) def test_layering_policy_missing_required_sections(self): properties_to_remove = (