From 4799acdbcc550bf6d2bee0e152608c394513335c Mon Sep 17 00:00:00 2001 From: Felipe Monteiro Date: Wed, 14 Mar 2018 19:41:38 +0000 Subject: [PATCH] Engine implementation for document replacement This adds support for document replacement to the Deckhand engine _only_ for the following scenarios _only_: * generic case (a replaces b, returns a only) * substitution case (a replaces b, c substitutes from a instead) TODO: * layering case (a replaces b, c layers with a instead) * Modify Document unique constraint to work with (schema, name, layer) throughout all of Deckhand (including controllers, database models, and anywhere else as needed) Change-Id: Ie2cea2a49ba3b9ebc42706fbe1060d94db2e5daa --- deckhand/common/document.py | 20 +++ deckhand/control/revision_documents.py | 1 + deckhand/engine/layering.py | 138 +++++++++++++----- deckhand/engine/secrets_manager.py | 21 ++- deckhand/errors.py | 16 ++ .../unit/engine/test_document_layering.py | 112 +++++++++++++- .../engine/test_document_layering_negative.py | 67 +++++++++ 7 files changed, 324 insertions(+), 51 deletions(-) diff --git a/deckhand/common/document.py b/deckhand/common/document.py index aca175c4..25662e5d 100644 --- a/deckhand/common/document.py +++ b/deckhand/common/document.py @@ -32,6 +32,10 @@ class DocumentDict(dict): """ + def __init__(self, *args, **kwargs): + super(DocumentDict, self).__init__(*args, **kwargs) + self._replaced_by = None + @classmethod def from_dict(self, documents): """Convert a list of documents or single document into an instance of @@ -123,6 +127,22 @@ class DocumentDict(dict): def is_encrypted(self): return self.storage_policy == 'encrypted' + @property + def is_replacement(self): + return utils.jsonpath_parse(self, 'metadata.replacement') is True + + @property + def has_replacement(self): + return isinstance(self._replaced_by, DocumentDict) + + @property + def replaced_by(self): + return self._replaced_by + + @replaced_by.setter + def replaced_by(self, other): + self._replaced_by = other + def __hash__(self): return hash(json.dumps(self, sort_keys=True)) diff --git a/deckhand/control/revision_documents.py b/deckhand/control/revision_documents.py index 29b62d07..ef59aa8e 100644 --- a/deckhand/control/revision_documents.py +++ b/deckhand/control/revision_documents.py @@ -116,6 +116,7 @@ class RenderedDocumentsResource(api_base.BaseResource): rendered_documents = document_layering.render() except (errors.InvalidDocumentLayer, errors.InvalidDocumentParent, + errors.InvalidDocumentReplacement, errors.IndeterminateDocumentParent, errors.MissingDocumentKey, errors.UnsupportedActionMethod) as e: diff --git a/deckhand/engine/layering.py b/deckhand/engine/layering.py index 330da20e..6d3da5cf 100644 --- a/deckhand/engine/layering.py +++ b/deckhand/engine/layering.py @@ -55,23 +55,73 @@ class DocumentLayering(object): _SUPPORTED_METHODS = (_MERGE_ACTION, _REPLACE_ACTION, _DELETE_ACTION) = ( 'merge', 'replace', 'delete') + def _calc_replacements_and_substitutions( + self, substitution_sources): + for document in self._documents_by_index.values(): + if document.is_replacement: + parent_meta = self._parents.get(document.meta) + parent = self._documents_by_index.get(parent_meta) + + if not parent_meta or not parent: + error_message = ( + 'Document replacement requires that the document with ' + '`replacement: true` have a parent.') + raise errors.InvalidDocumentReplacement( + schema=document.schema, name=document.name, + layer=document.layer, reason=error_message) + + # This checks that a document can only be a replacement for + # another document with the same `metadata.name` and `schema`. + if (document.schema == parent.schema and + document.name == parent.name): + parent.replaced_by = document + else: + error_message = ( + 'Document replacement requires that both documents ' + 'have the same `schema` and `metadata.name`.') + raise errors.InvalidDocumentReplacement( + schema=document.schema, name=document.name, + layer=document.layer, reason=error_message) + + # Since a substitution source only provides the document's + # `metadata.name` and `schema`, their tuple acts as the dictionary key. + # If a substitution source has a replacement, the replacement is used + # instead. + substitution_source_map = {} + + for src in substitution_sources: + src_ref = document_wrapper.DocumentDict(src) + if src_ref.meta in self._documents_by_index: + src_ref = self._documents_by_index[src_ref.meta] + # If the document has a replacement, use the replacement as the + # substitution source instead. + if src_ref.has_replacement: + if src_ref.is_replacement: + error_message = ('A replacement document cannot itself' + ' be replaced by another document.') + raise errors.InvalidDocumentReplacement( + schema=src_ref.schema, name=src_ref.name, + layer=src_ref.layer, reason=error_message) + + src_ref = src_ref.replaced_by + substitution_source_map[(src_ref.schema, src_ref.name)] = src_ref + + return substitution_source_map + def _replace_older_parent_with_younger_parent(self, child, parent, all_children): # If child has layer N, parent N+1, and current_parent N+2, then swap # parent with current_parent. In other words, if parent's layer is # closer to child's layer than current_parent's layer, then use parent. - current_parent_index = self._parents.get((child.schema, child.name)) - current_parent = self._documents_by_index.get( - current_parent_index, None) + parent_meta = self._parents.get(child.meta) + current_parent = self._documents_by_index.get(parent_meta, None) if current_parent: if (self._layer_order.index(parent.layer) > self._layer_order.index(current_parent.layer)): - self._parents[(child.schema, child.name)] = \ - (parent.schema, parent.name) + self._parents[child.meta] = parent.meta all_children[child] -= 1 else: - self._parents.setdefault((child.schema, child.name), - (parent.schema, parent.name)) + self._parents.setdefault(child.meta, parent.meta) def _is_actual_child_document(self, document, potential_child): if document == potential_child: @@ -213,26 +263,29 @@ class DocumentLayering(object): 'will be performed.') return layer_order - def _topologically_sort_documents(self, documents): + def _topologically_sort_documents(self, substitution_sources): """Topologically sorts the DAG formed from the documents' layering and substitution dependency chain. """ - documents_by_name = {} result = [] g = networkx.DiGraph() - for document in documents: - document = document_wrapper.DocumentDict(document) - documents_by_name.setdefault((document.schema, document.name), - document) + for document in self._documents_by_index.values(): if document.parent_selector: - parent = self._parents.get((document.schema, document.name)) + parent = self._parents.get(document.meta) if parent: - g.add_edge((document.schema, document.name), parent) + g.add_edge(document.meta, parent) for sub in document.substitutions: - g.add_edge((document.schema, document.name), - (sub['src']['schema'], sub['src']['name'])) + # Retrieve the correct substitution source using + # ``substitution_sources``. Necessary for 2 reasons: + # 1) It accounts for document replacements. + # 2) It effectively maps a 2-tuple key to a 3-tuple document + # unique identifier (meta). + src = substitution_sources.get( + (sub['src']['schema'], sub['src']['name'])) + if src: + g.add_edge(document.meta, src.meta) try: cycle = find_cycle(g) @@ -245,11 +298,12 @@ class DocumentLayering(object): sorted_documents = reversed(list(topological_sort(g))) - for document in sorted_documents: - if document in documents_by_name: - result.append(documents_by_name.pop(document)) - for document in documents_by_name.values(): - result.append(document) + for document_meta in sorted_documents: + if document_meta in self._documents_by_index: + result.append(self._documents_by_index[document_meta]) + for document in self._documents_by_index.values(): + if document not in result: + result.append(document) return result @@ -311,6 +365,8 @@ class DocumentLayering(object): self._sorted_documents = {} self._documents_by_index = {} + substitution_sources = substitution_sources or [] + # TODO(fmontei): Add a hook for post-validation too. if validate: self._pre_validate_documents(documents) @@ -336,8 +392,9 @@ class DocumentLayering(object): for document in documents: document = document_wrapper.DocumentDict(document) - self._documents_by_index.setdefault( - (document.schema, document.name), document) + + self._documents_by_index.setdefault(document.meta, document) + if document.layer: if document.layer not in self._layering_policy.layer_order: LOG.error('Document layer %s for document [%s] %s not ' @@ -375,11 +432,15 @@ class DocumentLayering(object): if not d.is_abstract ] + substitution_sources = self._calc_replacements_and_substitutions( + substitution_sources) + self.secrets_substitution = secrets_manager.SecretsSubstitution( substitution_sources, fail_on_missing_sub_src=fail_on_missing_sub_src) - self._sorted_documents = self._topologically_sort_documents(documents) + self._sorted_documents = self._topologically_sort_documents( + substitution_sources) del self._documents_by_layer del self._documents_by_labels @@ -494,30 +555,31 @@ class DocumentLayering(object): continue if doc.parent_selector: - parent_meta = self._parents.get((doc.schema, doc.name)) + parent_meta = self._parents.get(doc.meta) if parent_meta: parent = self._documents_by_index[parent_meta] if doc.actions: rendered_data = parent + # Apply each action to the current document. for action in doc.actions: LOG.debug('Applying action %s to document with ' - 'name=%s, schema=%s, layer=%s.', action, - doc.name, doc.schema, doc.layer) + 'schema=%s, name=%s, layer=%s.', action, + *doc.meta) rendered_data = self._apply_action( action, doc, rendered_data) if not doc.is_abstract: doc.data = rendered_data.data self.secrets_substitution.update_substitution_sources( doc.schema, doc.name, rendered_data.data) - self._documents_by_index[(doc.schema, doc.name)] = ( - rendered_data) + self._documents_by_index[doc.meta] = rendered_data else: - LOG.info('Skipped layering for document [%s] %s which ' - 'has a parent [%s] %s, but no associated ' - 'layering actions.', doc.schema, doc.name, - parent.schema, parent.name) + LOG.debug( + 'Skipped layering for document [%s, %s] %s which ' + 'has a parent [%s, %s] %s, but no associated ' + 'layering actions.', doc.schema, doc.layer, + doc.name, parent.schema, parent.layer, parent.name) # Perform substitutions on abstract data for child documents that # inherit from it, but only update the document's data if concrete. @@ -531,11 +593,11 @@ class DocumentLayering(object): doc.data = rendered_data.data self.secrets_substitution.update_substitution_sources( doc.schema, doc.name, rendered_data.data) - self._documents_by_index[(doc.schema, doc.name)] = ( - rendered_data) + self._documents_by_index[doc.meta] = rendered_data - # Return only concrete documents. - return [d for d in self._sorted_documents if d.is_abstract is False] + # Return only concrete documents and non-replacements. + return [d for d in self._sorted_documents + if d.is_abstract is False and d.has_replacement is False] @property def documents(self): diff --git a/deckhand/engine/secrets_manager.py b/deckhand/engine/secrets_manager.py index 35d571f2..7d3abde2 100644 --- a/deckhand/engine/secrets_manager.py +++ b/deckhand/engine/secrets_manager.py @@ -214,8 +214,9 @@ class SecretsSubstitution(object): the constructor. :param substitution_sources: List of documents that are potential - sources for substitution. Should only include concrete documents. - :type substitution_sources: List[dict] + sources for substitution. Or dict of documents keyed on tuple of + (schema, metadata.name). Should only include concrete documents. + :type substitution_sources: List[dict] or dict :param bool fail_on_missing_sub_src: Whether to fail on a missing substitution source. Default is True. """ @@ -227,12 +228,16 @@ class SecretsSubstitution(object): self._substitution_sources = {} self._fail_on_missing_sub_src = fail_on_missing_sub_src - for document in substitution_sources: - if not isinstance(document, document_wrapper.DocumentDict): - document = document_wrapper.DocumentDict(document) - if document.schema and document.name: - self._substitution_sources.setdefault( - (document.schema, document.name), document) + if isinstance(substitution_sources, dict): + self._substitution_sources = substitution_sources + else: + self._substitution_sources = dict() + for document in substitution_sources: + if not isinstance(document, document_wrapper.DocumentDict): + document = document_wrapper.DocumentDict(document) + if document.schema and document.name: + self._substitution_sources.setdefault( + (document.schema, document.name), document) def substitute_all(self, documents): """Substitute all documents that have a `metadata.substitutions` field. diff --git a/deckhand/errors.py b/deckhand/errors.py index 6108f744..b3f68dc5 100644 --- a/deckhand/errors.py +++ b/deckhand/errors.py @@ -261,6 +261,22 @@ class MissingDocumentPattern(DeckhandException): code = 400 +class InvalidDocumentReplacement(DeckhandException): + """The document replacement is invalid. + + **Troubleshoot:** + + * Check that the replacement document has the same ``schema`` and + ``metadata.name`` as the document it replaces. + * Check that the document with ``replacement: true`` has a parent. + * Check that the document replacement isn't being replaced by another + document. Only one level of replacement is permitted. + """ + msg_fmt = ("Replacement document [%(schema)s, %(layer)s] %(name)s is " + "invalid. Reason: %(reason)s") + code = 400 + + class UnsupportedActionMethod(DeckhandException): """The action is not in the list of supported methods. diff --git a/deckhand/tests/unit/engine/test_document_layering.py b/deckhand/tests/unit/engine/test_document_layering.py index e039fe7e..4e4b5352 100644 --- a/deckhand/tests/unit/engine/test_document_layering.py +++ b/deckhand/tests/unit/engine/test_document_layering.py @@ -304,11 +304,8 @@ data: site_expected = 'should not change' self._test_layering(documents, site_expected, global_expected=None) - mock_log.info.assert_called_once_with( - 'Skipped layering for document [%s] %s which has a parent [%s] ' - '%s, but no associated layering actions.', documents[2]['schema'], - documents[2]['metadata']['name'], documents[1]['schema'], - documents[1]['metadata']['name']) + error_re = r'^Skipped layering for document.*' + self.assertRegex(mock_log.debug.mock_calls[0][1][0], error_re) class TestDocumentLayering2Layers(TestDocumentLayering): @@ -1316,3 +1313,108 @@ class TestDocumentLayering3Layers2Regions2Sites(TestDocumentLayering): global_expected = None self._test_layering(documents, site_expected, region_expected, global_expected) + + +class TestDocumentLayeringWithReplacement(TestDocumentLayering): + + def setUp(self): + super(TestDocumentLayeringWithReplacement, self).setUp() + self.documents = list(yaml.safe_load_all(""" +--- +schema: deckhand/LayeringPolicy/v1 +metadata: + schema: metadata/Control/v1 + name: layering-policy +data: + layerOrder: + - global + - site +--- +schema: aic/Versions/v1 +metadata: + name: a + labels: + selector: foo + layeringDefinition: + abstract: False + layer: global +data: + conf: + foo: default +--- +schema: aic/Versions/v1 +metadata: + name: a + labels: + selector: baz + replacement: true + layeringDefinition: + abstract: False + layer: site + parentSelector: + selector: foo + actions: + - method: merge + path: . +data: + conf: + bar: override +--- +schema: armada/Chart/v1 +metadata: + name: c + layeringDefinition: + abstract: False + layer: global + substitutions: + - src: + schema: aic/Versions/v1 + name: a + path: .conf + dest: + path: .application.conf +data: + application: + conf: {} +... +""")) + + def test_basic_replacement(self): + """Verify that the replacement document is the only one returned.""" + site_expected = [{"conf": {"foo": "default", "bar": "override"}}] + global_expected = None + + self.documents = self.documents[:-1] + + self._test_layering(self.documents, site_expected, + global_expected=global_expected) + + def test_replacement_with_substitution_from_replacer(self): + """Verify that using a replacement document as a substitution source + works. + """ + site_expected = [{"conf": {"foo": "default", "bar": "override"}}] + global_expected = [ + {"application": {"conf": {"foo": "default", "bar": "override"}}}] + # Pass in the replacee and replacer as substitution sources. The + # replacer should be used as the source. + self._test_layering(self.documents, site_expected, + global_expected=global_expected, + substitution_sources=self.documents[1:3]) + # Attempt the same scenario but reverse the order of the substitution + # sources, which verifies that the replacer always takes priority. + self._test_layering( + self.documents, site_expected, global_expected=global_expected, + substitution_sources=list(reversed(self.documents[1:3]))) + + # Pass in the replacee as the only substitution source. The replacer + # should replace it and be used as the source. + self._test_layering(self.documents, site_expected, + global_expected=global_expected, + substitution_sources=[self.documents[1]]) + + # Pass in the replacer as the only substitution source, which should be + # used as the source. + self._test_layering(self.documents, site_expected, + global_expected=global_expected, + substitution_sources=[self.documents[2]]) diff --git a/deckhand/tests/unit/engine/test_document_layering_negative.py b/deckhand/tests/unit/engine/test_document_layering_negative.py index 5fdbeb10..587ca1b0 100644 --- a/deckhand/tests/unit/engine/test_document_layering_negative.py +++ b/deckhand/tests/unit/engine/test_document_layering_negative.py @@ -276,3 +276,70 @@ class TestDocumentLayeringValidationNegative( self.assertRaisesRegexp( errors.InvalidDocumentFormat, error_re, self._test_layering, [layering_policy, document], validate=True) + + +class TestDocumentLayeringReplacementNegative( + test_document_layering.TestDocumentLayering): + + def test_replacement_with_incompatible_name_or_schema_raises_exc(self): + """Validate that attempting to replace a child with its parent when + they don't have the same ``metadata.name`` and ``schema`` results in + exception. + """ + doc_factory = factories.DocumentFactory(2, [1, 1]) + documents = doc_factory.gen_test({}) + + # Validate case where names mismatch. + documents[1]['metadata']['name'] = 'foo' + documents[2]['metadata']['replacement'] = True + documents[2]['metadata']['name'] = 'bar' + + error_re = (r'.*Document replacement requires that both documents ' + 'have the same `schema` and `metadata.name`.') + self.assertRaisesRegexp(errors.InvalidDocumentReplacement, error_re, + self._test_layering, documents) + + # Validate case where schemas mismatch. + documents[1]['metadata']['schema'] = 'example/Kind/v1' + documents[2]['metadata']['replacement'] = True + documents[2]['metadata']['schema'] = 'example/Other/v1' + + error_re = (r'Document replacement requires that both documents ' + 'have the same `schema` and `metadata.name`.') + self.assertRaisesRegexp(errors.InvalidDocumentReplacement, error_re, + self._test_layering, documents) + + def test_replacement_without_parent_raises_exc(self): + """Validate that attempting to do replacement without a parent document + raises an exception. + """ + doc_factory = factories.DocumentFactory(2, [1, 1]) + documents = doc_factory.gen_test({}) + + documents[2]['metadata']['replacement'] = True + documents[2]['metadata']['layeringDefinition'].pop('parentSelector') + + error_re = (r'Document replacement requires that the document with ' + '`replacement: true` have a parent.') + self.assertRaisesRegexp(errors.InvalidDocumentReplacement, error_re, + self._test_layering, documents) + + def test_replacement_that_is_replaced_raises_exc(self): + """Validate that attempting replace a replacement document raises an + exception. + """ + doc_factory = factories.DocumentFactory(3, [1, 1, 1]) + documents = doc_factory.gen_test({}) + + for document in documents[1:]: + document['metadata']['name'] = 'foo' + document['schema'] = 'example/Kind/v1' + + documents[2]['metadata']['replacement'] = True + documents[3]['metadata']['replacement'] = True + + error_re = (r'A replacement document cannot itself be replaced by ' + 'another document.') + self.assertRaisesRegexp(errors.InvalidDocumentReplacement, error_re, + self._test_layering, documents, + substitution_sources=documents[1:])