Validate additional 'metadata.replacement' scenarios

This patch set adds additional documentation and unit tests
to validate further replacement scenarios.

In particular this commit adds an additional document check that
looks for documents exisitng in different layers that contain the
same name and same schema without any of them having `replacement: true`

Change-Id: I7c033d32a6755f36e609789a748cbc6d4af06bc2

deckhand/engine/_replacement.py

@@ -75,8 +75,50 @@ def check_only_one_level_of_replacement(src_ref):
     # If the document has a replacement, use the replacement as the
     # substitution source instead.
     if src_ref.is_replacement:
-        error_message = ('A replacement document cannot itself'
+        error_message = ('A replacement document cannot itself be replaced by '
-                         ' be replaced by another document.')
+                         'another document.')
         raise errors.InvalidDocumentReplacement(
             schema=src_ref.schema, name=src_ref.name,
             layer=src_ref.layer, reason=error_message)
+
+
+def check_replacement_is_false_uniqueness(
+        document, non_replacement_documents):
+    """Validate uniqueness of ``replacement: false`` for each document
+    identifier.
+
+    This check essentially validates that each raw document (which is uniquely
+    defined by (name, schema, layer)) maps to a unique rendered document
+    (which is uniquely defined by (name, schema)). This can be done by ensuring
+    that each (name, schema) pair only has one occurrence of
+    ``replacement: false``.
+
+    Normally, a ``replacement: true`` document nukes the ``replacement: false``
+    parent. But when > 1 ``replacement: false`` documents with same (name,
+    schema) exist, the raw document unique constraint predominates over the
+    rendered document unique constraint, resulting in a breakdown in the
+    rendering process, as confusion occurs over which document to reference
+    for substitution data and the like.
+
+    :param document: current document in the collection that is being processed
+    :param non_replacement_documents: a set containing tuples of the names and
+        schemas of all the non-replacement documents
+    """
+    if not document.is_control:
+        document_identifier = (
+            document['metadata']['name'],
+            document['metadata']['schema']
+        )
+        if document_identifier in non_replacement_documents:
+            error_message = (
+                'Documents with the same name and schema existing in '
+                'different layers without any of them having '
+                '`replacement = true` cannot exist as Deckhand will '
+                'arbitrarily select any of them for processing and none are '
+                'distinguishable from one another because none are a '
+                'parent or child or a replacement document.')
+            raise errors.InvalidDocumentReplacement(
+                schema=document.schema, name=document.name,
+                layer=document.layer, reason=error_message)
+        else:
+            non_replacement_documents.add(document_identifier)

deckhand/engine/layering.py

@@ -60,6 +60,10 @@ class DocumentLayering(object):
     def _calc_replacements_and_substitutions(
             self, substitution_sources):
 
+        # Used to track document names and schemas for documents that are not
+        # replacement documents
+        non_replacement_documents = set()
+
         for document in self._documents_by_index.values():
             parent_meta = self._parents.get(document.meta)
             parent = self._documents_by_index.get(parent_meta)
@@ -71,8 +75,13 @@ class DocumentLayering(object):
                     parent, document)
                 parent.replaced_by = document
             else:
+                # Handles case where parent and child have replacement: false
+                # as in this case both documents should not be replacement
+                # documents, requiring them to have different schema/name pair.
                 replacement.check_child_and_parent_different_metadata_name(
                     parent, document)
+                replacement.check_replacement_is_false_uniqueness(
+                    document, non_replacement_documents)
 
         # Since a substitution source only provides the document's
         # `metadata.name` and `schema`, their tuple acts as the dictionary key.

deckhand/tests/unit/control/test_errors.py

@@ -185,14 +185,14 @@ class TestValidationMessageFormatting(test_base.BaseControllerTest):
             headers={'Content-Type': 'application/x-yaml'},
             body=payload)
 
-        with mock.patch('deckhand.control.revision_documents.db_api'
+        with mock.patch('deckhand.control.revision_documents.common'
-                        '.revision_documents_get', autospec=True) \
+                        '.get_rendered_docs', autospec=True) \
-                as mock_get_rev_documents:
+                as mock_get_rendered_docs:
             invalid_document = document_wrapper.DocumentDict(
                 yaml.safe_load(payload))
             invalid_document.pop('metadata')
+            mock_get_rendered_docs.return_value = ([invalid_document], False)
 
-            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'})

deckhand/tests/unit/engine/test_document_layering_and_replacement.py

@@ -12,12 +12,89 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import inspect
 import itertools
 import os
 import yaml
 
 from deckhand.tests.unit.engine import test_document_layering
 
+REPLACEMENT_3_TIER_SAMPLE = list(yaml.safe_load_all(inspect.cleandoc(
+    """
+    ---
+    schema: deckhand/LayeringPolicy/v1
+    metadata:
+      schema: metadata/Control/v1
+      name: layering-policy
+      storagePolicy: cleartext
+    data:
+      layerOrder:
+        - global
+        - region
+        - site
+    ---
+    schema: armada/Chart/v1
+    metadata:
+      schema: metadata/Document/v1
+      name: nova-global
+      storagePolicy: cleartext
+      labels:
+        name: nova-global
+        component: nova
+      layeringDefinition:
+        abstract: false
+        layer: global
+    data:
+      values:
+        pod:
+          replicas:
+            server: 16
+    ---
+    schema: armada/Chart/v1
+    metadata:
+      schema: metadata/Document/v1
+      name: nova
+      storagePolicy: cleartext
+      labels:
+        name: nova-5ec
+        component: nova
+      layeringDefinition:
+        abstract: false
+        layer: region
+        parentSelector:
+          name: nova-global
+        actions:
+          - method: merge
+            path: .
+    data: {}
+    ---
+    schema: armada/Chart/v1
+    metadata:
+      schema: metadata/Document/v1
+      replacement: true
+      storagePolicy: cleartext
+      name: nova
+      layeringDefinition:
+        abstract: false
+        layer: site
+        parentSelector:
+          name: nova-5ec
+        actions:
+          - method: merge
+            path: .
+    data:
+      values:
+        pod:
+          replicas:
+            api_metadata: 16
+            placement: 2
+            osapi: 16
+            conductor: 16
+            consoleauth: 2
+            scheduler: 2
+            novncproxy: 2
+    """)))
+
 
 class TestDocumentLayeringWithReplacement(
         test_document_layering.TestDocumentLayering):
@@ -266,82 +343,8 @@ data:
 
         * Global document called nova-global
         * Region document called nova (layers with nova-global)
-        * Site document (replaces nova)
+        * Site document (replaces region document)
         """
-        self.documents = list(yaml.safe_load_all("""
----
-schema: deckhand/LayeringPolicy/v1
-metadata:
-  schema: metadata/Control/v1
-  name: layering-policy
-  storagePolicy: cleartext
-data:
-  layerOrder:
-    - global
-    - region
-    - site
----
-schema: armada/Chart/v1
-metadata:
-  schema: metadata/Document/v1
-  name: nova-global
-  storagePolicy: cleartext
-  labels:
-    name: nova-global
-    component: nova
-  layeringDefinition:
-    abstract: false
-    layer: global
-data:
-  values:
-    pod:
-      replicas:
-        server: 16
----
-schema: armada/Chart/v1
-metadata:
-  schema: metadata/Document/v1
-  name: nova
-  storagePolicy: cleartext
-  labels:
-    name: nova-5ec
-    component: nova
-  layeringDefinition:
-    abstract: false
-    layer: region
-    parentSelector:
-      name: nova-global
-    actions:
-      - method: merge
-        path: .
-data: {}
----
-schema: armada/Chart/v1
-metadata:
-  schema: metadata/Document/v1
-  replacement: true
-  storagePolicy: cleartext
-  name: nova
-  layeringDefinition:
-    abstract: false
-    layer: site
-    parentSelector:
-      name: nova-5ec
-    actions:
-      - method: merge
-        path: .
-data:
-  values:
-    pod:
-      replicas:
-        api_metadata: 16
-        placement: 2
-        osapi: 16
-        conductor: 16
-        consoleauth: 2
-        scheduler: 2
-        novncproxy: 2
-"""))
 
         site_expected = [
             {
@@ -372,7 +375,56 @@ data:
                 }
             }
         ]
-        self._test_layering(self.documents,
+        self._test_layering(REPLACEMENT_3_TIER_SAMPLE,
                             site_expected=site_expected,
                             region_expected=None,
                             global_expected=global_expected)
+
+    def test_multi_layer_replacement_with_intermediate_replacement(self):
+        """Validate the following scenario:
+
+        * Global document called nova-replace
+        * Region document called nova-replace (layers with nova-replace and
+          replaces it)
+        * Site document (layers with region document)
+        """
+
+        replacement_sample = list(REPLACEMENT_3_TIER_SAMPLE)
+        replacement_sample[1]['metadata']['name'] = 'nova-replace'
+        replacement_sample[2]['metadata']['name'] = 'nova-replace'
+        replacement_sample[2]['metadata']['replacement'] = True
+        replacement_sample[3]['metadata']['replacement'] = False
+
+        site_expected = [
+            {
+                "values": {
+                    "pod": {
+                        "replicas": {
+                            "api_metadata": 16,
+                            "placement": 2,
+                            "osapi": 16,
+                            "conductor": 16,
+                            "consoleauth": 2,
+                            "scheduler": 2,
+                            "novncproxy": 2,
+                            "server": 16
+                        }
+                    }
+                }
+            }
+        ]
+        region_expected = [
+            {
+                "values": {
+                    "pod": {
+                        "replicas": {
+                            "server": 16
+                        }
+                    }
+                }
+            }
+        ]
+        self._test_layering(replacement_sample,
+                            site_expected=site_expected,
+                            region_expected=region_expected,
+                            global_expected=None)

deckhand/tests/unit/engine/test_document_layering_and_replacement_negative.py

@@ -24,6 +24,10 @@ class TestDocumentLayeringReplacementNegative(
         """Validate that attempting to replace a child with its parent when
         they don't have the same ``metadata.name`` and ``schema`` results in
         exception.
+
+        global
+          |
+        site (replacement: true, incompatible with parent)
         """
         doc_factory = factories.DocumentFactory(2, [1, 1])
         documents = doc_factory.gen_test({})
@@ -52,6 +56,10 @@ class TestDocumentLayeringReplacementNegative(
         """Validate that a non-replacement document (i.e. regular document
         without `replacement: true`) cannot have the same schema/name as
         another document.
+
+        global (replacement: false)
+          |
+        site (replacement: false)
         """
         doc_factory = factories.DocumentFactory(2, [1, 1])
         documents = doc_factory.gen_test({})
@@ -68,6 +76,10 @@ class TestDocumentLayeringReplacementNegative(
     def test_replacement_without_parent_raises_exc(self):
         """Validate that attempting to do replacement without a parent document
         raises an exception.
+
+        None
+          |
+        site (replacement: true)
         """
         doc_factory = factories.DocumentFactory(2, [1, 1])
         documents = doc_factory.gen_test({})
@@ -80,9 +92,35 @@ class TestDocumentLayeringReplacementNegative(
         self.assertRaisesRegexp(errors.InvalidDocumentReplacement, error_re,
                                 self._test_layering, documents)
 
+    def test_replacement_with_parent_replace_true_raises_exc(self):
+        """Validate that a parent document with replacement: true necessarily
+        fails if it itself doesn't have a parent.
+
+        None
+          |
+        global (replacement: true)
+          |
+        site
+        """
+        doc_factory = factories.DocumentFactory(2, [1, 1])
+        documents = doc_factory.gen_test({})
+
+        documents[1]['metadata']['replacement'] = True
+
+        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
+        """Validate that attempting to replace a replacement document raises an
         exception.
+
+        global
+          |
+        region (replacement: true)
+          |
+        site (replacement: true)
         """
         doc_factory = factories.DocumentFactory(3, [1, 1, 1])
         documents = doc_factory.gen_test({}, region_abstract=False,
@@ -99,3 +137,25 @@ class TestDocumentLayeringReplacementNegative(
                     'another document.')
         self.assertRaisesRegexp(errors.InvalidDocumentReplacement, error_re,
                                 self._test_layering, documents)
+
+    def test_replacement_true_with_parent_replacement_true_raises_exc(self):
+        """Validate that when documents have the same `metadata.name` and
+        `metadata.schema` existing in different layers without any of them
+        having `replacement = true` raises an exception
+        """
+        doc_factory = factories.DocumentFactory(2, [1, 1])
+        documents = doc_factory.gen_test({})
+
+        for document in documents[1:]:
+            document['metadata']['name'] = 'foo'
+            document['schema'] = 'example/Kind/v1'
+            document['metadata']['replacement'] = False
+            if 'parentSelector' in document['metadata']['layeringDefinition']:
+                document['metadata']['layeringDefinition'].pop(
+                    'parentSelector')
+
+        error_re = (r'Documents with the same name and schema existing in '
+                    'different layers without any of them having '
+                    '`replacement = true` cannot exist.*')
+        self.assertRaisesRegexp(errors.InvalidDocumentReplacement, error_re,
+                                self._test_layering, documents)

doc/source/users/replacement.rst

@@ -80,6 +80,24 @@ The following result in validation errors:
 * A replacement document cannot itself be replaced. That is, only one level
   of replacement is allowed.
 
+Here are the following possible scenarios regarding child and parent
+replacement values:
+
++-------------------------------------------------------------+
+| Child | Parent | Status                                     |
++=============================================================+
+| True  | True   | Throws InvalidDocumentReplacement exception|
++-------------------------------------------------------------+
+| False | True   | Throws InvalidDocumentReplacement exception|
++-------------------------------------------------------------+
+| True  | False  | Valid scenario                             |
++-------------------------------------------------------------+
+| False | False  | Throws InvalidDocumentReplacement exception|
++-------------------------------------------------------------+
+
+Examples
+--------
+
 Note that each key in the examples below is *mandatory* and that the
 ``parentSelector`` labels should be able to select the parent to be replaced.