From 177675e96fffcda9799c68bbce831424c1167020 Mon Sep 17 00:00:00 2001 From: Felipe Monteiro Date: Sun, 13 May 2018 13:28:39 -0400 Subject: [PATCH] [fix] Parent substitution/layering before replacement Currently it doesn't seem document replacement works exactly as expected: The parent-replacement document can receive layering and substitution data prior to being replaced. Currently, Deckhand does not account for this scenario. A child-replacement depends on its parent-replacement the same way any child depends on its parent: so that the child layers with its parent only after the parent has received all layering and substitution data. But other documents that depend on the parent-replacement actually depend on the child-replacement instead as the child-replacement replaces its parent. So the dependency chain is: PR -> CR -> anything that layers with PR. A unit and functional test have been added for regression. Co-Authored-By: Felipe Monteiro Change-Id: I353393f416aa6e441d84add9ebedcd152944d7e8 --- deckhand/common/utils.py | 2 - deckhand/engine/layering.py | 67 +++++++++------- .../gabbits/replacement/replacement.yaml | 51 +++++++++++++ .../gabbits/resources/replacement.yaml | 76 +++++++++++++++++++ .../unit/engine/test_document_layering.py | 5 +- .../test_document_layering_and_replacement.py | 47 +++++++++++- 6 files changed, 212 insertions(+), 36 deletions(-) create mode 100644 deckhand/tests/functional/gabbits/replacement/replacement.yaml create mode 100644 deckhand/tests/functional/gabbits/resources/replacement.yaml diff --git a/deckhand/common/utils.py b/deckhand/common/utils.py index 1f0f142f..b5385a20 100644 --- a/deckhand/common/utils.py +++ b/deckhand/common/utils.py @@ -129,8 +129,6 @@ def _populate_data_with_attributes(jsonpath, data): # Handle case where an object needs to be created. elif path not in d: if '\'' or '\"' in path: - LOG.info('Stripping subpath %s of start and end quotes during ' - 'path vivification for jsonpath: %s.', path, jsonpath) path = path.strip('\'').strip('\"') d.setdefault(path, {}) d = d.get(path) diff --git a/deckhand/engine/layering.py b/deckhand/engine/layering.py index be109ddf..04d5d4d2 100644 --- a/deckhand/engine/layering.py +++ b/deckhand/engine/layering.py @@ -298,14 +298,32 @@ class DocumentLayering(object): """ result = [] + def _get_ancestor(doc, parent_meta): + parent = self._documents_by_index.get(parent_meta) + # Return the parent's replacement, but if that replacement is the + # document itself then return the parent. + use_replacement = ( + parent and parent.has_replacement and + parent.replaced_by is not doc + ) + if use_replacement: + parent = parent.replaced_by + return parent + g = networkx.DiGraph() for document in self._documents_by_index.values(): - if document.has_replacement: - g.add_edge(document.meta, document.replaced_by.meta) - elif document.parent_selector and not document.is_replacement: + if document.parent_selector: + # NOTE: A child-replacement depends on its parent-replacement + # the same way any child depends on its parent: so that the + # child layers with its parent only after the parent has + # received all layering and substitution data. But other + # non-replacement child documents must first wait for the + # child-relacement to layer with the parent, so that they + # can use the replaced data. parent_meta = self._parents.get(document.meta) - if parent_meta: - g.add_edge(document.meta, parent_meta) + ancestor = _get_ancestor(document, parent_meta) + if ancestor: + g.add_edge(document.meta, ancestor.meta) for sub in document.substitutions: # Retrieve the correct substitution source using @@ -591,22 +609,6 @@ class DocumentLayering(object): return overall_data - def _get_parent_or_replacement(self, doc, parent_meta): - """Returns the document's parent or the document's parent's - replacement. - - :param DocumentDict doc: Document used to get parent. - :param tuple parent_meta: Unique parent identifier. - :returns: Parent document or its replacement if the parent has one. - :rtype: DocumentDict - """ - parent = self._documents_by_index.get(parent_meta) - # Return the parent's replacement, but if that replacement is the - # document itself then return the parent. - if parent and parent.has_replacement and parent.replaced_by is not doc: - parent = parent.replaced_by - return parent - def render(self): """Perform layering on the list of documents passed to ``__init__``. @@ -629,11 +631,14 @@ class DocumentLayering(object): if doc.is_control: continue + LOG.debug("Rendering document %s:%s:%s", *doc.meta) + if doc.parent_selector: parent_meta = self._parents.get(doc.meta) if parent_meta: - parent = self._get_parent_or_replacement(doc, parent_meta) + LOG.debug("Using parent %s:%s:%s", *parent_meta) + parent = self._documents_by_index[parent_meta] if doc.actions: rendered_data = parent @@ -652,8 +657,7 @@ class DocumentLayering(object): doc, parent, action) except Exception: # nosec pass - if not doc.is_abstract: - doc.data = rendered_data.data + doc.data = rendered_data.data self.secrets_substitution.update_substitution_sources( doc.schema, doc.name, rendered_data.data) self._documents_by_index[doc.meta] = rendered_data @@ -679,10 +683,10 @@ class DocumentLayering(object): if substituted_data: rendered_data = substituted_data[0] # Update the actual document data if concrete. - if not doc.is_abstract: - doc.data = rendered_data.data - self.secrets_substitution.update_substitution_sources( - doc.schema, doc.name, rendered_data.data) + doc.data = rendered_data.data + if not doc.has_replacement: + self.secrets_substitution.update_substitution_sources( + doc.schema, doc.name, rendered_data.data) self._documents_by_index[doc.meta] = rendered_data # Otherwise, retrieve the encrypted data for the document if its # data has been encrypted so that future references use the actual @@ -696,6 +700,13 @@ class DocumentLayering(object): doc.schema, doc.name, encrypted_data) self._documents_by_index[doc.meta] = encrypted_data + # NOTE: Since the child-replacement is always prioritized, before + # other children, as soon as the child-replacement layers with the + # parent (which already has undergone layering and substitution + # itself), replace the parent data with that of the replacement. + if doc.is_replacement: + parent.data = doc.data + # 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] diff --git a/deckhand/tests/functional/gabbits/replacement/replacement.yaml b/deckhand/tests/functional/gabbits/replacement/replacement.yaml new file mode 100644 index 00000000..38d930c6 --- /dev/null +++ b/deckhand/tests/functional/gabbits/replacement/replacement.yaml @@ -0,0 +1,51 @@ +# Tests success path for advanced replacement scenario, where +# parent-replacement document receives substitutions and is then layered +# with by its child-replacement document, which replaces its parent. +# +# 1. Purges existing data to ensure test isolation. +# 2. Adds initial documents with replacement scenario described above. +# 3. Verifies correctly layered, substituted and replaced data. + +defaults: + request_headers: + content-type: application/x-yaml + response_headers: + content-type: application/x-yaml + verbose: true + +tests: + - name: purge + desc: Begin testing from known state. + DELETE: /api/v1.0/revisions + status: 204 + response_headers: null + + - name: initialize + desc: Create initial documents + PUT: /api/v1.0/buckets/mop/documents + status: 200 + data: <@resources/replacement.yaml + + - name: verify_replacement_document_receives_substitution + desc: | + Tests success path for advanced replacement scenario, where + parent-replacement document receives substitutions and is then layered + with by its child-replacement document, which replaces its parent. + GET: /api/v1.0/revisions/$RESPONSE['$.[0].status.revision']/rendered-documents + query_parameters: + schema: armada/Chart/v1 + status: 200 + response_multidoc_jsonpaths: + $.`len`: 1 + $.[*].metadata.name: example-chart-01 + $.[*].metadata.layeringDefinition.layer: site + $.[*].data: + chart: + details: + data: bar + values: + tls: + certificate: | + CERTIFICATE DATA + key: | + KEY DATA diff --git a/deckhand/tests/functional/gabbits/resources/replacement.yaml b/deckhand/tests/functional/gabbits/resources/replacement.yaml new file mode 100644 index 00000000..3caac479 --- /dev/null +++ b/deckhand/tests/functional/gabbits/resources/replacement.yaml @@ -0,0 +1,76 @@ +--- +schema: deckhand/LayeringPolicy/v1 +metadata: + schema: metadata/Control/v1 + name: layering-policy +data: + layerOrder: + - region + - site +--- +schema: deckhand/Certificate/v1 +metadata: + name: example-cert + schema: metadata/Document/v1 + layeringDefinition: + layer: site + storagePolicy: cleartext +data: | + CERTIFICATE DATA +--- +schema: deckhand/CertificateKey/v1 +metadata: + name: example-key + schema: metadata/Document/v1 + layeringDefinition: + layer: site + storagePolicy: cleartext +data: | + KEY DATA +--- +schema: armada/Chart/v1 +metadata: + name: example-chart-01 + schema: metadata/Document/v1 + labels: + name: parent-chart + layeringDefinition: + layer: region + substitutions: + - dest: + path: .chart.values.tls.certificate + src: + schema: deckhand/Certificate/v1 + name: example-cert + path: . +data: + chart: + details: + data: foo + values: {} +--- +schema: armada/Chart/v1 +metadata: + name: example-chart-01 + schema: metadata/Document/v1 + replacement: true + layeringDefinition: + layer: site + parentSelector: + name: parent-chart + actions: + - method: merge + path: . + substitutions: + - dest: + path: .chart.values.tls.key + src: + schema: deckhand/CertificateKey/v1 + name: example-key + path: . +data: + chart: + details: + data: bar + values: {} +... diff --git a/deckhand/tests/unit/engine/test_document_layering.py b/deckhand/tests/unit/engine/test_document_layering.py index ca6e0046..7f9efb61 100644 --- a/deckhand/tests/unit/engine/test_document_layering.py +++ b/deckhand/tests/unit/engine/test_document_layering.py @@ -323,8 +323,9 @@ data: site_expected = 'should not change' self._test_layering(documents, site_expected, global_expected=None) - error_re = r'^Skipped layering for document.*' - self.assertRegex(mock_log.debug.mock_calls[0][1][0], error_re) + msg_re = 'Skipped layering for document' + mock_calls = [x[1][0] for x in mock_log.debug.mock_calls] + self.assertTrue(any(x.startswith(msg_re) for x in mock_calls)) class TestDocumentLayering2Layers(TestDocumentLayering): diff --git a/deckhand/tests/unit/engine/test_document_layering_and_replacement.py b/deckhand/tests/unit/engine/test_document_layering_and_replacement.py index f8957928..2e72457e 100644 --- a/deckhand/tests/unit/engine/test_document_layering_and_replacement.py +++ b/deckhand/tests/unit/engine/test_document_layering_and_replacement.py @@ -13,6 +13,7 @@ # limitations under the License. import itertools +import os import yaml from deckhand.tests.unit.engine import test_document_layering @@ -106,10 +107,11 @@ data: # The replacer should be used as the source. self._test_layering(self.documents, site_expected, global_expected=global_expected) - # Attempt the same scenario but reverse the order of the documents, - # which verifies that the replacer always takes priority. - self._test_layering(self.documents, site_expected, - global_expected=global_expected) + # Try different permutations of document orders for good measure. + for documents in list(itertools.permutations(self.documents))[:10]: + self._test_layering( + documents, site_expected=site_expected, + global_expected=global_expected) def test_replacement_with_layering_with_replacer(self): """Verify that replacement works alongside layering. This scenario @@ -212,3 +214,40 @@ data: self._test_layering( documents, site_expected=site_expected, global_expected=global_expected) + + def test_replacement_document_receives_substitution(self): + """Verifies that the parent-replacement receives substitution data + prior to the child-replacement layering with it, which in turn is + done prior to any other document attempting to substitute from or + layer with the child-replacement (which replaces its parent). + """ + test_path = os.path.join( + os.getcwd(), 'deckhand', 'tests', 'functional', 'gabbits', + 'resources', 'replacement.yaml') + with open(test_path) as f: + self.documents = list(yaml.safe_load_all(f)) + + site_expected = [ + "CERTIFICATE DATA\n", + "KEY DATA\n", + { + 'chart': { + 'details': {'data': 'bar'}, + 'values': { + 'tls': { + 'certificate': 'CERTIFICATE DATA\n', + 'key': 'KEY DATA\n' + } + } + } + } + ] + + self._test_layering(self.documents, site_expected=site_expected, + region_expected=None) + + # Try different permutations of document orders for good measure. + for documents in list(itertools.permutations(self.documents))[:10]: + self._test_layering( + documents, site_expected=site_expected, + region_expected=None)