diff --git a/deckhand/control/revision_documents.py b/deckhand/control/revision_documents.py index e590213b..98d2680f 100644 --- a/deckhand/control/revision_documents.py +++ b/deckhand/control/revision_documents.py @@ -106,15 +106,13 @@ class RenderedDocumentsResource(api_base.BaseResource): documents = self._retrieve_documents_for_rendering(revision_id, **filters) - substitution_sources = self._retrieve_substitution_sources() try: # NOTE(fmontei): `validate` is False because documents have already # been pre-validated during ingestion. Documents are post-validated # below, regardless. document_layering = layering.DocumentLayering( - documents, substitution_sources=substitution_sources, - validate=False) + documents, validate=False) rendered_documents = document_layering.render() except (errors.InvalidDocumentLayer, errors.InvalidDocumentParent, @@ -176,11 +174,6 @@ class RenderedDocumentsResource(api_base.BaseResource): return documents - def _retrieve_substitution_sources(self): - # Return all concrete documents as potential substitution sources. - return db_api.document_get_all( - **{'metadata.layeringDefinition.abstract': False}) - def _post_validate(self, rendered_documents): # Perform schema validation post-rendering to ensure that rendering # and substitution didn't break anything. diff --git a/deckhand/engine/layering.py b/deckhand/engine/layering.py index 315fa919..0bfe9f9a 100644 --- a/deckhand/engine/layering.py +++ b/deckhand/engine/layering.py @@ -19,6 +19,7 @@ import networkx from networkx.algorithms.cycles import find_cycle from networkx.algorithms.dag import topological_sort from oslo_log import log as logging +from oslo_log import versionutils from deckhand.engine import document_validation from deckhand.engine import document_wrapper @@ -359,8 +360,20 @@ class DocumentLayering(object): self._layer_order = self._get_layering_order(self._layering_policy) self._calc_all_document_children() + if substitution_sources: + versionutils.report_deprecated_feature( + LOG, + "Usage of `substitution_sources` has been deprecated. All " + "concrete documents will be used by default." + ) + else: + substitution_sources = [ + d for d in self._documents_by_index.values() + if not d.is_abstract + ] + self.secrets_substitution = secrets_manager.SecretsSubstitution( - substitution_sources or [], + substitution_sources, fail_on_missing_sub_src=fail_on_missing_sub_src) self._sorted_documents = self._topologically_sort_documents(documents) diff --git a/deckhand/tests/unit/engine/test_document_layering.py b/deckhand/tests/unit/engine/test_document_layering.py index 6effbccf..e039fe7e 100644 --- a/deckhand/tests/unit/engine/test_document_layering.py +++ b/deckhand/tests/unit/engine/test_document_layering.py @@ -28,9 +28,9 @@ class TestDocumentLayering(test_base.DeckhandTestCase): def _test_layering(self, documents, site_expected=None, region_expected=None, global_expected=None, - substitution_sources=None, validate=False, **kwargs): + validate=False, **kwargs): document_layering = layering.DocumentLayering( - documents, substitution_sources, validate=validate, **kwargs) + documents, validate=validate, **kwargs) site_docs = [] region_docs = [] @@ -197,8 +197,7 @@ data: region_expected = None global_expected = None self._test_layering( - documents, site_expected, region_expected, global_expected, - substitution_sources=[documents[1]]) + documents, site_expected, region_expected, global_expected) def test_layering_verify_that_substitution_dependencies_includes_parents( self): @@ -284,8 +283,7 @@ data: {'application': {'conf': {'bar': 'override', 'foo': 'default'}}} ] self._test_layering( - documents, site_expected, region_expected, global_expected, - substitution_sources=[documents[1]]) + documents, site_expected, region_expected, global_expected) @mock.patch.object(layering, 'LOG', autospec=True) def test_layering_document_with_parent_but_no_actions_skips_layering( diff --git a/deckhand/tests/unit/engine/test_document_layering_and_substitution.py b/deckhand/tests/unit/engine/test_document_layering_and_substitution.py index 030ce0c3..2f45d0df 100644 --- a/deckhand/tests/unit/engine/test_document_layering_and_substitution.py +++ b/deckhand/tests/unit/engine/test_document_layering_and_substitution.py @@ -49,13 +49,13 @@ class TestDocumentLayeringWithSubstitution( certificate = secrets_factory.gen_test( 'Certificate', 'cleartext', data='global-secret', name='global-cert') + documents.append(certificate) global_expected = {'a': {'x': 1, 'y': 2}, 'c': 'global-secret'} site_expected = {'a': {'x': 1, 'y': 2}, 'b': 4, 'c': 'global-secret'} self._test_layering(documents, site_expected=site_expected, - global_expected=global_expected, - substitution_sources=[certificate]) + global_expected=global_expected) def test_layering_and_substitution_no_children(self): """Validate that a document with no children undergoes substitution. @@ -86,17 +86,18 @@ class TestDocumentLayeringWithSubstitution( # Remove the labels from the global document so that the site document # (the child) has no parent. documents[1]['metadata']['labels'] = {} + secrets_factory = factories.DocumentSecretFactory() certificate = secrets_factory.gen_test( 'Certificate', 'cleartext', data='global-secret', name='global-cert') + documents.append(certificate) global_expected = {'a': {'x': 1, 'y': 2}, 'c': 'global-secret'} site_expected = {'b': 4} self._test_layering(documents, site_expected=site_expected, - global_expected=global_expected, - substitution_sources=[certificate]) + global_expected=global_expected) def test_substitution_without_parent_document(self): """Validate that a document with no parent undergoes substitution. @@ -128,17 +129,18 @@ class TestDocumentLayeringWithSubstitution( # Remove the labels from the global document so that the site document # (the child) has no parent. documents[1]['metadata']['labels'] = {} + secrets_factory = factories.DocumentSecretFactory() certificate = secrets_factory.gen_test( 'Certificate', 'cleartext', data='site-secret', name='site-cert') + documents.append(certificate) global_expected = {'a': {'x': 1, 'y': 2}} site_expected = {'b': 4, 'c': 'site-secret'} self._test_layering(documents, site_expected=site_expected, - global_expected=global_expected, - substitution_sources=[certificate]) + global_expected=global_expected) def test_parent_and_child_layering_and_substitution_different_paths(self): """Validate that parent and child documents both undergo layering and @@ -190,11 +192,11 @@ class TestDocumentLayeringWithSubstitution( certificate_key = secrets_factory.gen_test( 'CertificateKey', 'cleartext', data='site-secret', name='site-cert') + documents.extend([certificate, certificate_key]) self._test_layering( documents, site_expected=site_expected, - global_expected=global_expected, - substitution_sources=[certificate, certificate_key]) + global_expected=global_expected) def test_parent_and_child_layering_and_substitution_same_paths(self): """Validate that parent and child documents both undergo layering and @@ -245,11 +247,11 @@ class TestDocumentLayeringWithSubstitution( certificate_key = secrets_factory.gen_test( 'CertificateKey', 'cleartext', data='site-secret', name='site-cert') + documents.extend([certificate, certificate_key]) self._test_layering( documents, site_expected=site_expected, - global_expected=global_expected, - substitution_sources=[certificate, certificate_key]) + global_expected=global_expected) def test_parent_with_multi_child_layering_and_sub_different_paths(self): """Validate that parent and children documents both undergo layering @@ -319,11 +321,11 @@ class TestDocumentLayeringWithSubstitution( name='site-%d-cert' % idx) for idx in range(1, 3) ] + documents.extend([certificate] + certificate_keys) self._test_layering( documents, site_expected=site_expected, - global_expected=global_expected, - substitution_sources=[certificate] + certificate_keys) + global_expected=global_expected) def test_parent_with_multi_child_layering_and_sub_same_path(self): """Validate that parent and children documents both undergo layering @@ -393,11 +395,11 @@ class TestDocumentLayeringWithSubstitution( name='site-%d-cert' % idx) for idx in range(1, 3) ] + documents.extend([certificate] + certificate_keys) self._test_layering( documents, site_expected=site_expected, - global_expected=global_expected, - substitution_sources=[certificate] + certificate_keys) + global_expected=global_expected) def test_parent_with_multi_child_layering_and_multi_substitutions(self): """Validate that parent and children documents both undergo layering @@ -499,12 +501,11 @@ class TestDocumentLayeringWithSubstitution( name='site-%d-cert' % idx) for idx in range(1, 3) ] + documents.extend([certificate] + certificate_keys + certificates) self._test_layering( documents, site_expected=site_expected, - global_expected=global_expected, - substitution_sources=[certificate] + certificate_keys + - certificates) + global_expected=global_expected) @mock.patch('deckhand.engine.layering.LOG', autospec=True) def test_parent_and_child_undergo_layering_and_substitution_empty_layers( @@ -571,11 +572,11 @@ class TestDocumentLayeringWithSubstitution( certificate_key = secrets_factory.gen_test( 'CertificateKey', 'cleartext', data='site-secret', name='site-cert') + documents.extend([certificate] + [certificate_key]) self._test_layering( documents, site_expected=site_expected, - global_expected=global_expected, - substitution_sources=[certificate, certificate_key]) + global_expected=global_expected) expected_message = ( '%s is an empty layer with no documents. It will be discarded ' @@ -661,21 +662,19 @@ class TestDocumentLayeringWithSubstitution( certificate_key = secrets_factory.gen_test( 'CertificateKey', 'cleartext', data='site-secret', name='site-cert') + documents.extend([certificate] + [certificate_key]) # Pass in the documents in reverse order to ensure that the dependency # chain by default is not linear and thus requires sorting. self._test_layering( list(reversed(documents)), site_expected=site_expected, - global_expected=global_expected, - substitution_sources=[certificate, certificate_key] + documents) + global_expected=global_expected) # Try different permutations of document orders for good measure. for document_order in list(itertools.permutations(documents))[:10]: self._test_layering( document_order, site_expected=site_expected, - global_expected=global_expected, - substitution_sources=[ - certificate, certificate_key] + documents) + global_expected=global_expected) def test_layering_and_substitution_site_abstract_and_global_concrete(self): """Verifies that if a global document is abstract, yet has @@ -706,10 +705,10 @@ class TestDocumentLayeringWithSubstitution( doc_factory = factories.DocumentFactory(2, [1, 1]) documents = doc_factory.gen_test(mapping, site_abstract=False, global_abstract=True) + documents.append(certificate) site_expected = {"global": "random", "cert": "global-secret", "site": "stuff"} global_expected = None self._test_layering(documents, site_expected, - global_expected=global_expected, - substitution_sources=[certificate]) + global_expected=global_expected) diff --git a/deckhand/tests/unit/engine/test_document_layering_and_substitution_negative.py b/deckhand/tests/unit/engine/test_document_layering_and_substitution_negative.py index 89144969..344da0a8 100644 --- a/deckhand/tests/unit/engine/test_document_layering_and_substitution_negative.py +++ b/deckhand/tests/unit/engine/test_document_layering_and_substitution_negative.py @@ -80,7 +80,7 @@ class TestDocumentLayeringWithSubstitutionNegative( # chain by default is not linear and thus requires sorting. self.assertRaises( errors.SubstitutionDependencyCycle, layering.DocumentLayering, - documents, substitution_sources=documents) + documents) def test_layering_with_substitution_self_reference_fails(self): """Validate that a substitution self-reference fails. @@ -111,8 +111,7 @@ class TestDocumentLayeringWithSubstitutionNegative( # Pass in the documents in reverse order to ensure that the dependency # chain by default is not linear and thus requires sorting. self.assertRaises( - errors.SubstitutionDependencyCycle, self._test_layering, documents, - substitution_sources=documents) + errors.SubstitutionDependencyCycle, self._test_layering, documents) def test_layering_with_missing_substitution_source_raises_exc(self): """Validate that a missing substitution source document fails."""