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.