From 60e82b7bd61fedcc50b46ea7b11963201f5c852f Mon Sep 17 00:00:00 2001 From: Rick Bartra Date: Mon, 29 Oct 2018 15:47:56 -0400 Subject: [PATCH] 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 | 46 +++- deckhand/engine/layering.py | 9 + deckhand/tests/unit/control/test_errors.py | 8 +- .../test_document_layering_and_replacement.py | 204 +++++++++++------- ...ument_layering_and_replacement_negative.py | 62 +++++- doc/source/users/replacement.rst | 18 ++ 6 files changed, 264 insertions(+), 83 deletions(-) diff --git a/deckhand/engine/_replacement.py b/deckhand/engine/_replacement.py index 1ac4755a..a420aad1 100644 --- a/deckhand/engine/_replacement.py +++ b/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' - ' be replaced by another document.') + 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) + + +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) diff --git a/deckhand/engine/layering.py b/deckhand/engine/layering.py index c4c0e589..d2719cb8 100644 --- a/deckhand/engine/layering.py +++ b/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. diff --git a/deckhand/tests/unit/control/test_errors.py b/deckhand/tests/unit/control/test_errors.py index 5e9f72af..ed184072 100644 --- a/deckhand/tests/unit/control/test_errors.py +++ b/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' - '.revision_documents_get', autospec=True) \ - as mock_get_rev_documents: + with mock.patch('deckhand.control.revision_documents.common' + '.get_rendered_docs', autospec=True) \ + 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'}) 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 057bd58d..b202bec6 100644 --- a/deckhand/tests/unit/engine/test_document_layering_and_replacement.py +++ b/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) diff --git a/deckhand/tests/unit/engine/test_document_layering_and_replacement_negative.py b/deckhand/tests/unit/engine/test_document_layering_and_replacement_negative.py index 192df146..7f73a940 100644 --- a/deckhand/tests/unit/engine/test_document_layering_and_replacement_negative.py +++ b/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) diff --git a/doc/source/users/replacement.rst b/doc/source/users/replacement.rst index 8f863603..60561a0c 100644 --- a/doc/source/users/replacement.rst +++ b/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.