From b81cebb01213164fd748d9fa17ff017106cc5012 Mon Sep 17 00:00:00 2001 From: Felipe Monteiro Date: Thu, 15 Feb 2018 22:14:51 +0000 Subject: [PATCH] Fail fast on bad substitution input during layering This PS causes layering module to fail fast on malformed ``metadata.substitutions`` entry in a document by performing built-in schema validation when validate=True is passed to the DocumentLayering constructor. This kwarg is useful for when the layering module is called directly -- i.e. by Promenade or Pegleg. (The Deckhand server already performs document pre-validation during document ingestion so there is no need for documents stored inside Deckhand to be re-validated again for rendered-documents endpoint.) Next, a new exception was added -- SubstitutionSourceNotFound -- which is raised when a substitution document is referenced by another document but isn't found. Finally, the previous exception raised by the secrets_manager module has been renamed to UnknownSubstitutionError which now raises a 500 instead of a 400 as this exception will most likely be due to an internal server error of some kind. Unit tests were added and documentation changes were made. Change-Id: Idfd91a52ef9ffd8f9b1c06c6b84c3405acab6f16 --- deckhand/control/revision_documents.py | 14 ++++-- deckhand/engine/layering.py | 29 ++++++++++++- .../engine/schema/v1_0/document_schema.py | 5 ++- deckhand/engine/secrets_manager.py | 42 ++++++++---------- deckhand/errors.py | 32 +++++++++++--- .../unit/engine/test_document_layering.py | 4 +- ...ment_layering_and_substitution_negative.py | 20 +++++++++ .../engine/test_document_layering_negative.py | 43 +++++++++++++++++++ doc/source/exceptions.rst | 14 +++++- 9 files changed, 161 insertions(+), 42 deletions(-) diff --git a/deckhand/control/revision_documents.py b/deckhand/control/revision_documents.py index 5cb6d55f..6dcf0c55 100644 --- a/deckhand/control/revision_documents.py +++ b/deckhand/control/revision_documents.py @@ -109,18 +109,24 @@ class RenderedDocumentsResource(api_base.BaseResource): substitution_sources = self._retrieve_substitution_sources() try: + # NOTE(fmontei): `validate` is False because documents have already + # been pre-validated during ingestion. Documents are post-validated + # below, regardless. document_layering = layering.DocumentLayering( - documents, substitution_sources) + documents, substitution_sources, validate=False) rendered_documents = document_layering.render() except (errors.InvalidDocumentLayer, errors.InvalidDocumentParent, errors.IndeterminateDocumentParent, - errors.UnsupportedActionMethod, - errors.MissingDocumentKey) as e: + errors.MissingDocumentKey, + errors.UnsupportedActionMethod) as e: raise falcon.HTTPBadRequest(description=e.format_message()) except (errors.LayeringPolicyNotFound, - errors.SubstitutionFailure) as e: + errors.SubstitutionSourceNotFound) as e: raise falcon.HTTPConflict(description=e.format_message()) + except errors.errors.UnknownSubstitutionError as e: + raise falcon.HTTPInternalServerError( + description=e.format_message()) # Filters to be applied post-rendering, because many documents are # involved in rendering. User filters can only be applied once all diff --git a/deckhand/engine/layering.py b/deckhand/engine/layering.py index 48781832..21abbcfd 100644 --- a/deckhand/engine/layering.py +++ b/deckhand/engine/layering.py @@ -20,6 +20,7 @@ from networkx.algorithms.cycles import find_cycle from networkx.algorithms.dag import topological_sort from oslo_log import log as logging +from deckhand.engine import document_validation from deckhand.engine import document_wrapper from deckhand.engine import secrets_manager from deckhand.engine import utils as engine_utils @@ -244,7 +245,27 @@ class DocumentLayering(object): return result - def __init__(self, documents, substitution_sources=None): + def _validate_documents(self, documents): + LOG.debug('%s performing document pre-validation.', + self.__class__.__name__) + validator = document_validation.DocumentValidation( + documents, pre_validate=True) + results = validator.validate_all() + val_errors = [] + for result in results: + val_errors.extend( + [(e['schema'], e['name'], e['message']) + for e in result['errors']]) + if val_errors: + for error in val_errors: + LOG.error( + 'Document [%s] %s failed with pre-validation error: %s.', + *error) + raise errors.InvalidDocumentFormat( + details='The following pre-validation errors occurred ' + '(schema, name, error): %s.' % val_errors) + + def __init__(self, documents, substitution_sources=None, validate=True): """Contructor for ``DocumentLayering``. :param layering_policy: The document with schema @@ -256,6 +277,9 @@ class DocumentLayering(object): :param substitution_sources: List of documents that are potential sources for substitution. Should only include concrete documents. :type substitution_sources: List[dict] + :param validate: Whether to pre-validate documents using built-in + schema validation. Default is True. + :type validate: bool :raises LayeringPolicyNotFound: If no LayeringPolicy was found among list of ``documents``. @@ -271,6 +295,9 @@ class DocumentLayering(object): self._documents_by_labels = {} self._layering_policy = None + if validate: + self._validate_documents(documents) + layering_policies = list( filter(lambda x: x.get('schema').startswith( types.LAYERING_POLICY_SCHEMA), documents)) diff --git a/deckhand/engine/schema/v1_0/document_schema.py b/deckhand/engine/schema/v1_0/document_schema.py index bc1c9185..d5591a4f 100644 --- a/deckhand/engine/schema/v1_0/document_schema.py +++ b/deckhand/engine/schema/v1_0/document_schema.py @@ -27,7 +27,10 @@ substitution_schema = { 'src': { 'type': 'object', 'properties': { - 'schema': {'type': 'string'}, + 'schema': { + 'type': 'string', + 'pattern': '^[A-Za-z]+/[A-Za-z]+/v\d+(.0)?$' + }, 'name': {'type': 'string'}, 'path': {'type': 'string'} }, diff --git a/deckhand/engine/secrets_manager.py b/deckhand/engine/secrets_manager.py index 050e4ab8..40bb5461 100644 --- a/deckhand/engine/secrets_manager.py +++ b/deckhand/engine/secrets_manager.py @@ -139,6 +139,10 @@ class SecretsSubstitution(object): :type documents: dict or List[dict] :returns: List of fully substituted documents. :rtype: Generator[:class:`DocumentDict`] + :raises SubstitutionSourceNotFound: If a substitution source document + is referenced by another document but wasn't found. + :raises UnknownSubstitutionError: If an unknown error occurred during + substitution. """ documents_to_substitute = [] @@ -164,28 +168,18 @@ class SecretsSubstitution(object): src_name = sub['src']['name'] src_path = sub['src']['path'] - if not src_schema: - LOG.warning('Source document schema "%s" is unspecified ' - 'under substitutions for document [%s] %s.', - src_schema, document.schema, document.name) - if not src_name: - LOG.warning('Source document name "%s" is unspecified' - ' under substitutions for document [%s] %s.', - src_name, document.schema, document.name) - if not src_path: - LOG.warning('Source document path "%s" is unspecified ' - 'under substitutions for document [%s] %s.', - src_path, document.schema, document.name) - if (src_schema, src_name) in self._substitution_sources: src_doc = self._substitution_sources[ (src_schema, src_name)] else: - src_doc = {} - LOG.warning('Could not find substitution source document ' - '[%s] %s among the provided ' - '`substitution_sources`.', src_schema, - src_name) + message = ('Could not find substitution source document ' + '[%s] %s among the provided ' + '`substitution_sources`.', src_schema, src_name) + LOG.error(message) + raise errors.SubstitutionSourceNotFound( + src_schema=src_schema, src_name=src_name, + document_schema=document.schema, + document_name=document.name) # If the data is a dictionary, retrieve the nested secret # via jsonpath_parse, else the secret is the primitive/string @@ -199,11 +193,6 @@ class SecretsSubstitution(object): dest_path = sub['dest']['path'] dest_pattern = sub['dest'].get('pattern', None) - if not dest_path: - LOG.warning('Destination document path "%s" is unspecified' - ' under substitutions for document [%s] %s.', - dest_path, document.schema, document.name) - LOG.debug('Substituting from schema=%s name=%s src_path=%s ' 'into dest_path=%s, dest_pattern=%s', src_schema, src_name, src_path, dest_path, dest_pattern) @@ -222,15 +211,18 @@ class SecretsSubstitution(object): if sub_source: sub_source['data'] = substituted_data else: - LOG.warning( + message = ( 'Failed to create JSON path "%s" in the ' 'destination document [%s] %s. No data was ' 'substituted.', dest_path, document.schema, document.name) + LOG.error(message) + raise errors.UnknownSubstitutionError(details=message) except Exception as e: LOG.error('Unexpected exception occurred while attempting ' 'secret substitution. %s', six.text_type(e)) - raise errors.SubstitutionFailure(details=six.text_type(e)) + raise errors.UnknownSubstitutionError( + details=six.text_type(e)) yield document diff --git a/deckhand/errors.py b/deckhand/errors.py index 536969e2..e255cf98 100644 --- a/deckhand/errors.py +++ b/deckhand/errors.py @@ -171,11 +171,11 @@ class DeckhandException(Exception): class InvalidDocumentFormat(DeckhandException): - """Schema validations failed for the provided document. + """Schema validations failed for the provided document(s). **Troubleshoot:** """ - msg_fmt = ("The provided document failed schema validation. Details: " + msg_fmt = ("The provided document(s) failed schema validation. Details: " "%(details)s") code = 400 @@ -184,6 +184,7 @@ class InvalidDocumentLayer(DeckhandException): """The document layer is invalid. **Troubleshoot:** + * Check that the document layer is contained in the layerOrder in the registered LayeringPolicy in the system. """ @@ -198,6 +199,7 @@ class InvalidDocumentParent(DeckhandException): """The document parent is invalid. **Troubleshoot:** + * Check that the document `schema` and parent `schema` match. * Check that the document layer is lower-order than the parent layer. """ @@ -220,6 +222,7 @@ class SubstitutionDependencyCycle(DeckhandException): """An illegal substitution depdencency cycle was detected. **Troubleshoot:** + * Check that there is no two-way substitution dependency between documents. """ msg_fmt = ('Cannot determine substitution order as a dependency ' @@ -338,14 +341,19 @@ class LayeringPolicyNotFound(DeckhandException): code = 409 -class SubstitutionFailure(DeckhandException): - """An unknown error occurred during substitution. +class SubstitutionSourceNotFound(DeckhandException): + """Required substitution source document was not found for layering. **Troubleshoot:** + + * Ensure that the missing source document being referenced exists in + the system or was passed to the layering module. """ - msg_fmt = ('An unknown exception occurred while trying to perform ' - 'substitution. Details: %(detail)s') - code = 400 + msg_fmt = ( + "Required substitution source document [%(src_schema)s] %(src_name)s " + "was not found, yet is referenced by [%(document_schema)s] " + "%(document_name)s.") + code = 409 class BarbicanException(DeckhandException): @@ -365,3 +373,13 @@ class PolicyNotAuthorized(DeckhandException): """ msg_fmt = "Policy doesn't allow %(action)s to be performed." code = 403 + + +class UnknownSubstitutionError(DeckhandException): + """An unknown error occurred during substitution. + + **Troubleshoot:** + """ + msg_fmt = ('An unknown exception occurred while trying to perform ' + 'substitution. Details: %(details)s') + code = 500 diff --git a/deckhand/tests/unit/engine/test_document_layering.py b/deckhand/tests/unit/engine/test_document_layering.py index 08fbb873..b779ef5a 100644 --- a/deckhand/tests/unit/engine/test_document_layering.py +++ b/deckhand/tests/unit/engine/test_document_layering.py @@ -25,9 +25,9 @@ class TestDocumentLayering(test_base.DeckhandTestCase): def _test_layering(self, documents, site_expected=None, region_expected=None, global_expected=None, - substitution_sources=None): + substitution_sources=None, validate=False, **kwargs): document_layering = layering.DocumentLayering( - documents, substitution_sources) + documents, substitution_sources, validate=validate, **kwargs) site_docs = [] region_docs = [] diff --git a/deckhand/tests/unit/engine/test_document_layering_and_substitution_negative.py b/deckhand/tests/unit/engine/test_document_layering_and_substitution_negative.py index 089c25a6..89144969 100644 --- a/deckhand/tests/unit/engine/test_document_layering_and_substitution_negative.py +++ b/deckhand/tests/unit/engine/test_document_layering_and_substitution_negative.py @@ -113,3 +113,23 @@ class TestDocumentLayeringWithSubstitutionNegative( self.assertRaises( errors.SubstitutionDependencyCycle, self._test_layering, documents, substitution_sources=documents) + + def test_layering_with_missing_substitution_source_raises_exc(self): + """Validate that a missing substitution source document fails.""" + mapping = { + "_SITE_SUBSTITUTIONS_1_": [{ + "dest": { + "path": ".c" + }, + "src": { + "schema": "example/Kind/v1", + "name": "nowhere-to-be-found", + "path": "." + } + }] + } + doc_factory = factories.DocumentFactory(2, [1, 1]) + documents = doc_factory.gen_test(mapping, site_abstract=False) + + self.assertRaises( + errors.SubstitutionSourceNotFound, self._test_layering, documents) diff --git a/deckhand/tests/unit/engine/test_document_layering_negative.py b/deckhand/tests/unit/engine/test_document_layering_negative.py index 65d7d811..099bed2f 100644 --- a/deckhand/tests/unit/engine/test_document_layering_negative.py +++ b/deckhand/tests/unit/engine/test_document_layering_negative.py @@ -12,6 +12,8 @@ # See the License for the specific language governing permissions and # limitations under the License. +import copy + import mock from deckhand.engine import layering @@ -200,3 +202,44 @@ class TestDocumentLayeringNegative( self.assertRaises( errors.InvalidDocumentParent, self._test_layering, documents) + + +class TestDocumentLayeringValidationNegative( + test_document_layering.TestDocumentLayering): + + def test_layering_invalid_substitution_format_raises_exc(self): + doc_factory = factories.DocumentFactory(1, [1]) + layering_policy, document_template = doc_factory.gen_test({ + "_GLOBAL_SUBSTITUTIONS_1_": [{ + "dest": { + "path": ".c" + }, + "src": { + "schema": "deckhand/Certificate/v1", + "name": "global-cert", + "path": "." + } + + }], + }, global_abstract=False) + + for key in ('src', 'dest'): + document = copy.deepcopy(document_template) + del document['metadata']['substitutions'][0][key] + self.assertRaises(errors.InvalidDocumentFormat, + self._test_layering, [layering_policy, document], + validate=True) + + for key in ('schema', 'name', 'path'): + document = copy.deepcopy(document_template) + del document['metadata']['substitutions'][0]['src'][key] + self.assertRaises(errors.InvalidDocumentFormat, + self._test_layering, [layering_policy, document], + validate=True) + + for key in ('path',): + document = copy.deepcopy(document_template) + del document['metadata']['substitutions'][0]['dest'][key] + self.assertRaises(errors.InvalidDocumentFormat, + self._test_layering, [layering_policy, document], + validate=True) diff --git a/doc/source/exceptions.rst b/doc/source/exceptions.rst index 564a3294..3a62e482 100644 --- a/doc/source/exceptions.rst +++ b/doc/source/exceptions.rst @@ -89,8 +89,18 @@ Deckhand Exceptions :members: :show-inheritance: :undoc-members: - * - SubstitutionFailure - - .. autoexception:: deckhand.errors.SubstitutionFailure + * - SubstitutionDependencyCycle + - .. autoexception:: deckhand.errors.SubstitutionDependencyCycle + :members: + :show-inheritance: + :undoc-members: + * - SubstitutionSourceNotFound + - .. autoexception:: deckhand.errors.SubstitutionSourceNotFound + :members: + :show-inheritance: + :undoc-members: + * - UnknownSubstitutionError + - .. autoexception:: deckhand.errors.UnknownSubstitutionError :members: :show-inheritance: :undoc-members: