Fix: Substitution sources not always updated during layering

This PS resolves a bug related to the _substitution_sources in
secrets_manager.SecretsSubstitution not getting updated with
the most recently updated layering data.

Currently, the DocumentLayering class, during initialization, passes
the list of substitution sources to the SecretsSubstitution class
as an optimization. During layering, documents that are substitution
sources can have their data updated -- and if their data is not
updated then that implies that a substitution source's data is
stale -- causing future substitutions using that substitution source
data to use stale data.

The solution is to introduce a new method called
`update_substitution_sources` which updates a specific substitution
source entry with the most update-to-date layered data following
every single layering update, such that all substitution sources
should always have the most up-to-date data.

Change-Id: Idc375cfdf17375d3c401342dff259bdcd1718941

deckhand/engine/layering.py

@@ -490,6 +490,8 @@ class DocumentLayering(object):
                     self.secrets_substitution.substitute_all(doc))
                 if substituted_data:
                     rendered_data_by_layer[layer_idx] = substituted_data[0]
+                    self.secrets_substitution.update_substitution_sources(
+                        doc.schema, doc.name, substituted_data[0].data)
             else:
                 rendered_data_by_layer[layer_idx] = doc
 
@@ -525,6 +527,8 @@ class DocumentLayering(object):
                 # children in deeper layers can reference the most up-to-date
                 # changes.
                 rendered_data_by_layer[child_layer_idx] = rendered_data
+                self.secrets_substitution.update_substitution_sources(
+                    child.schema, child.name, rendered_data.data)
 
         # Handle edge case for parentless documents that require substitution.
         # If a document has no parent, then the for loop above doesn't iterate
@@ -536,6 +540,8 @@ class DocumentLayering(object):
                     self.secrets_substitution.substitute_all(doc))
                 if substituted_data:
                     doc = substituted_data[0]
+                    self.secrets_substitution.update_substitution_sources(
+                        doc.schema, doc.name, substituted_data[0].data)
 
         # Return only concrete documents.
         return [d for d in self._documents_to_layer if d.is_abstract is False]

deckhand/engine/secrets_manager.py

@@ -105,6 +105,26 @@ class SecretsManager(object):
 class SecretsSubstitution(object):
     """Class for document substitution logic for YAML files."""
 
+    @staticmethod
+    def sanitize_potential_secrets(document):
+        """Sanitize all secret data that may have been substituted into the
+        document. Uses references in ``document.substitutions`` to determine
+        which values to sanitize. Only meaningful to call this on post-rendered
+        documents.
+
+        :param DocumentDict document: Document to sanitize.
+        """
+        to_sanitize = copy.deepcopy(document)
+        safe_message = 'Sanitized to avoid exposing secret.'
+
+        for sub in document.substitutions:
+            replaced_data = utils.jsonpath_replace(
+                to_sanitize['data'], safe_message, sub['dest']['path'])
+            if replaced_data:
+                to_sanitize['data'] = replaced_data
+
+        return to_sanitize
+
     def __init__(self, substitution_sources=None,
                  fail_on_missing_sub_src=True):
         """SecretSubstitution constructor.
@@ -207,17 +227,11 @@ class SecretsSubstitution(object):
                 try:
                     substituted_data = utils.jsonpath_replace(
                         document['data'], src_secret, dest_path, dest_pattern)
-                    sub_source = self._substitution_sources.get(
+                    if (isinstance(document['data'], dict)
-                        (document.schema, document.name))
+                            and isinstance(substituted_data, dict)):
-                    if (isinstance(document['data'], dict) and
-                        isinstance(substituted_data, dict)):
                         document['data'].update(substituted_data)
-                        if sub_source:
-                            sub_source['data'].update(substituted_data)
                     elif substituted_data:
                         document['data'] = substituted_data
-                        if sub_source:
-                            sub_source['data'] = substituted_data
                     else:
                         message = (
                             'Failed to create JSON path "%s" in the '
@@ -234,22 +248,12 @@ class SecretsSubstitution(object):
 
         yield document
 
-    @staticmethod
+    def update_substitution_sources(self, schema, name, data):
-    def sanitize_potential_secrets(document):
+        if (schema, name) not in self._substitution_sources:
-        """Sanitize all secret data that may have been substituted into the
+            return
-        document. Uses references in ``document.substitutions`` to determine
-        which values to sanitize. Only meaningful to call this on post-rendered
-        documents.
 
-        :param DocumentDict document: Document to sanitize.
+        substitution_src = self._substitution_sources[(schema, name)]
-        """
+        if isinstance(data, dict) and isinstance(substitution_src.data, dict):
-        to_sanitize = copy.deepcopy(document)
+            substitution_src.data.update(data)
-        safe_message = 'Sanitized to avoid exposing secret.'
+        else:
-
+            substitution_src.data = data
-        for sub in document.substitutions:
-            replaced_data = utils.jsonpath_replace(
-                to_sanitize['data'], safe_message, sub['dest']['path'])
-            if replaced_data:
-                to_sanitize['data'] = replaced_data
-
-        return to_sanitize

deckhand/factories.py

@@ -172,7 +172,7 @@ class DocumentFactory(DeckhandFactory):
                              "be equal to the value of 'num_layers'.")
 
         for doc_count in docs_per_layer:
-            if doc_count < 1:
+            if doc_count < 0:
                 raise ValueError(
                     "Each entry in 'docs_per_layer' must be >= 1.")
 

deckhand/tests/unit/engine/test_document_layering.py

@@ -118,6 +118,88 @@ class TestDocumentLayeringScenarios(TestDocumentLayering):
         self.assertRegex(m_log.warning.mock_calls[0][1][0][0],
                          r'Could not find substitution source document .*')
 
+    def test_layering_substitution_source_skips_layering(self):
+        """This scenario consists of a layerOrder with global, region, site,
+        with 1 global documents and 2 sites documents. 1 site document ('e')
+        layers with the parent (the global document) and the other site
+        document substitutes from the 1st site document.
+        """
+
+        payload = """
+---
+schema: aic/Versions/v1
+metadata:
+  name: d
+  schema: metadata/Document/v1
+  labels:
+    selector: foo1
+  layeringDefinition:
+    abstract: True
+    layer: global
+data:
+  conf:
+    foo: default
+---
+schema: aic/Versions/v1
+metadata:
+  name: e
+  schema: metadata/Document/v1
+  labels:
+    selector: baz1
+  layeringDefinition:
+    abstract: False
+    layer: site
+    parentSelector:
+      selector: foo1
+    actions:
+      - method: merge
+        path: .
+data:
+  conf:
+    bar: override
+---
+schema: armada/Chart/v1
+metadata:
+  name: f
+  schema: metadata/Document/v1
+  layeringDefinition:
+    abstract: False
+    layer: site
+  substitutions:
+    - src:
+        schema: aic/Versions/v1
+        name: e
+        path: .conf
+      dest:
+        path: .application.conf
+data:
+  application:
+    conf: {}
+---
+schema: deckhand/LayeringPolicy/v1
+metadata:
+  schema: metadata/Control/v1
+  name: layering-policy
+data:
+  layerOrder:
+    - global
+    - region
+    - site
+...
+"""
+
+        documents = list(yaml.safe_load_all(payload))
+
+        site_expected = [
+            {'conf': {'foo': 'default', 'bar': 'override'}},
+            {'application': {'conf': {'bar': 'override', 'foo': 'default'}}}
+        ]
+        region_expected = None
+        global_expected = None
+        self._test_layering(
+            documents, site_expected, region_expected, global_expected,
+            substitution_sources=[documents[1]])
+
 
 class TestDocumentLayering2Layers(TestDocumentLayering):