Merge "Validate additional 'metadata.replacement' scenarios"

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.