airship/deckhand
Code Issues Proposed changes

Deprecate substitution_sources from layering module

Browse Source 
Deprecate substitution_sources from layering module because we
can just use the concrete documents as all the substitution
sources to simplify things.

Change-Id: Ibd8dff50402508417457655c367ebc9b6f28d70a
This commit is contained in:
Felipe Monteiro 2018-03-23 20:14:53 +00:00
parent 44114dad3b
commit d86d87d16c
5 changed files with 46 additions and 44 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
deckhand/control/revision_documents.py
View File

@ -106,15 +106,13 @@ class RenderedDocumentsResource(api_base.BaseResource):
documents = self._retrieve_documents_for_rendering(revision_id, documents = self._retrieve_documents_for_rendering(revision_id,
**filters) **filters)
substitution_sources = self._retrieve_substitution_sources()
try: try:
# NOTE(fmontei): `validate` is False because documents have already # NOTE(fmontei): `validate` is False because documents have already
# been pre-validated during ingestion. Documents are post-validated # been pre-validated during ingestion. Documents are post-validated
# below, regardless. # below, regardless.
document_layering = layering.DocumentLayering( document_layering = layering.DocumentLayering(
documents, substitution_sources=substitution_sources, documents, validate=False)
validate=False)
rendered_documents = document_layering.render() rendered_documents = document_layering.render()
except (errors.InvalidDocumentLayer, except (errors.InvalidDocumentLayer,
errors.InvalidDocumentParent, errors.InvalidDocumentParent,
@ -176,11 +174,6 @@ class RenderedDocumentsResource(api_base.BaseResource):
return documents 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): def _post_validate(self, rendered_documents):
# Perform schema validation post-rendering to ensure that rendering # Perform schema validation post-rendering to ensure that rendering
# and substitution didn't break anything. # and substitution didn't break anything.

15
deckhand/engine/layering.py
View File

@ -19,6 +19,7 @@ import networkx
from networkx.algorithms.cycles import find_cycle from networkx.algorithms.cycles import find_cycle
from networkx.algorithms.dag import topological_sort from networkx.algorithms.dag import topological_sort
from oslo_log import log as logging from oslo_log import log as logging
from oslo_log import versionutils
from deckhand.engine import document_validation from deckhand.engine import document_validation
from deckhand.engine import document_wrapper from deckhand.engine import document_wrapper
@ -359,8 +360,20 @@ class DocumentLayering(object):
self._layer_order = self._get_layering_order(self._layering_policy) self._layer_order = self._get_layering_order(self._layering_policy)
self._calc_all_document_children() 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( self.secrets_substitution = secrets_manager.SecretsSubstitution(
substitution_sources or [], substitution_sources,
fail_on_missing_sub_src=fail_on_missing_sub_src) fail_on_missing_sub_src=fail_on_missing_sub_src)
self._sorted_documents = self._topologically_sort_documents(documents) self._sorted_documents = self._topologically_sort_documents(documents)

10
deckhand/tests/unit/engine/test_document_layering.py
View File

@ -28,9 +28,9 @@ class TestDocumentLayering(test_base.DeckhandTestCase):
def _test_layering(self, documents, site_expected=None, def _test_layering(self, documents, site_expected=None,
region_expected=None, global_expected=None, region_expected=None, global_expected=None,
substitution_sources=None, validate=False, **kwargs): validate=False, **kwargs):
document_layering = layering.DocumentLayering( document_layering = layering.DocumentLayering(
documents, substitution_sources, validate=validate, **kwargs) documents, validate=validate, **kwargs)
site_docs = [] site_docs = []
region_docs = [] region_docs = []
@ -197,8 +197,7 @@ data:
region_expected = None region_expected = None
global_expected = None global_expected = None
self._test_layering( self._test_layering(
documents, site_expected, region_expected, global_expected, documents, site_expected, region_expected, global_expected)
substitution_sources=[documents[1]])
def test_layering_verify_that_substitution_dependencies_includes_parents( def test_layering_verify_that_substitution_dependencies_includes_parents(
self): self):
@ -284,8 +283,7 @@ data:
{'application': {'conf': {'bar': 'override', 'foo': 'default'}}} {'application': {'conf': {'bar': 'override', 'foo': 'default'}}}
] ]
self._test_layering( self._test_layering(
documents, site_expected, region_expected, global_expected, documents, site_expected, region_expected, global_expected)
substitution_sources=[documents[1]])
@mock.patch.object(layering, 'LOG', autospec=True) @mock.patch.object(layering, 'LOG', autospec=True)
def test_layering_document_with_parent_but_no_actions_skips_layering( def test_layering_document_with_parent_but_no_actions_skips_layering(

51
deckhand/tests/unit/engine/test_document_layering_and_substitution.py
View File

@ -49,13 +49,13 @@ class TestDocumentLayeringWithSubstitution(
certificate = secrets_factory.gen_test( certificate = secrets_factory.gen_test(
'Certificate', 'cleartext', data='global-secret', 'Certificate', 'cleartext', data='global-secret',
name='global-cert') name='global-cert')
documents.append(certificate)
global_expected = {'a': {'x': 1, 'y': 2}, 'c': 'global-secret'} global_expected = {'a': {'x': 1, 'y': 2}, 'c': 'global-secret'}
site_expected = {'a': {'x': 1, 'y': 2}, 'b': 4, 'c': 'global-secret'} site_expected = {'a': {'x': 1, 'y': 2}, 'b': 4, 'c': 'global-secret'}
self._test_layering(documents, site_expected=site_expected, self._test_layering(documents, site_expected=site_expected,
global_expected=global_expected, global_expected=global_expected)
substitution_sources=[certificate])
def test_layering_and_substitution_no_children(self): def test_layering_and_substitution_no_children(self):
"""Validate that a document with no children undergoes substitution. """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 # Remove the labels from the global document so that the site document
# (the child) has no parent. # (the child) has no parent.
documents[1]['metadata']['labels'] = {} documents[1]['metadata']['labels'] = {}
secrets_factory = factories.DocumentSecretFactory() secrets_factory = factories.DocumentSecretFactory()
certificate = secrets_factory.gen_test( certificate = secrets_factory.gen_test(
'Certificate', 'cleartext', data='global-secret', 'Certificate', 'cleartext', data='global-secret',
name='global-cert') name='global-cert')
documents.append(certificate)
global_expected = {'a': {'x': 1, 'y': 2}, 'c': 'global-secret'} global_expected = {'a': {'x': 1, 'y': 2}, 'c': 'global-secret'}
site_expected = {'b': 4} site_expected = {'b': 4}
self._test_layering(documents, site_expected=site_expected, self._test_layering(documents, site_expected=site_expected,
global_expected=global_expected, global_expected=global_expected)
substitution_sources=[certificate])
def test_substitution_without_parent_document(self): def test_substitution_without_parent_document(self):
"""Validate that a document with no parent undergoes substitution. """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 # Remove the labels from the global document so that the site document
# (the child) has no parent. # (the child) has no parent.
documents[1]['metadata']['labels'] = {} documents[1]['metadata']['labels'] = {}
secrets_factory = factories.DocumentSecretFactory() secrets_factory = factories.DocumentSecretFactory()
certificate = secrets_factory.gen_test( certificate = secrets_factory.gen_test(
'Certificate', 'cleartext', data='site-secret', 'Certificate', 'cleartext', data='site-secret',
name='site-cert') name='site-cert')
documents.append(certificate)
global_expected = {'a': {'x': 1, 'y': 2}} global_expected = {'a': {'x': 1, 'y': 2}}
site_expected = {'b': 4, 'c': 'site-secret'} site_expected = {'b': 4, 'c': 'site-secret'}
self._test_layering(documents, site_expected=site_expected, self._test_layering(documents, site_expected=site_expected,
global_expected=global_expected, global_expected=global_expected)
substitution_sources=[certificate])
def test_parent_and_child_layering_and_substitution_different_paths(self): def test_parent_and_child_layering_and_substitution_different_paths(self):
"""Validate that parent and child documents both undergo layering and """Validate that parent and child documents both undergo layering and
@ -190,11 +192,11 @@ class TestDocumentLayeringWithSubstitution(
certificate_key = secrets_factory.gen_test( certificate_key = secrets_factory.gen_test(
'CertificateKey', 'cleartext', data='site-secret', 'CertificateKey', 'cleartext', data='site-secret',
name='site-cert') name='site-cert')
documents.extend([certificate, certificate_key])
self._test_layering( self._test_layering(
documents, site_expected=site_expected, documents, site_expected=site_expected,
global_expected=global_expected, global_expected=global_expected)
substitution_sources=[certificate, certificate_key])
def test_parent_and_child_layering_and_substitution_same_paths(self): def test_parent_and_child_layering_and_substitution_same_paths(self):
"""Validate that parent and child documents both undergo layering and """Validate that parent and child documents both undergo layering and
@ -245,11 +247,11 @@ class TestDocumentLayeringWithSubstitution(
certificate_key = secrets_factory.gen_test( certificate_key = secrets_factory.gen_test(
'CertificateKey', 'cleartext', data='site-secret', 'CertificateKey', 'cleartext', data='site-secret',
name='site-cert') name='site-cert')
documents.extend([certificate, certificate_key])
self._test_layering( self._test_layering(
documents, site_expected=site_expected, documents, site_expected=site_expected,
global_expected=global_expected, global_expected=global_expected)
substitution_sources=[certificate, certificate_key])
def test_parent_with_multi_child_layering_and_sub_different_paths(self): def test_parent_with_multi_child_layering_and_sub_different_paths(self):
"""Validate that parent and children documents both undergo layering """Validate that parent and children documents both undergo layering
@ -319,11 +321,11 @@ class TestDocumentLayeringWithSubstitution(
name='site-%d-cert' % idx) name='site-%d-cert' % idx)
for idx in range(1, 3) for idx in range(1, 3)
] ]
documents.extend([certificate] + certificate_keys)
self._test_layering( self._test_layering(
documents, site_expected=site_expected, documents, site_expected=site_expected,
global_expected=global_expected, global_expected=global_expected)
substitution_sources=[certificate] + certificate_keys)
def test_parent_with_multi_child_layering_and_sub_same_path(self): def test_parent_with_multi_child_layering_and_sub_same_path(self):
"""Validate that parent and children documents both undergo layering """Validate that parent and children documents both undergo layering
@ -393,11 +395,11 @@ class TestDocumentLayeringWithSubstitution(
name='site-%d-cert' % idx) name='site-%d-cert' % idx)
for idx in range(1, 3) for idx in range(1, 3)
] ]
documents.extend([certificate] + certificate_keys)
self._test_layering( self._test_layering(
documents, site_expected=site_expected, documents, site_expected=site_expected,
global_expected=global_expected, global_expected=global_expected)
substitution_sources=[certificate] + certificate_keys)
def test_parent_with_multi_child_layering_and_multi_substitutions(self): def test_parent_with_multi_child_layering_and_multi_substitutions(self):
"""Validate that parent and children documents both undergo layering """Validate that parent and children documents both undergo layering
@ -499,12 +501,11 @@ class TestDocumentLayeringWithSubstitution(
name='site-%d-cert' % idx) name='site-%d-cert' % idx)
for idx in range(1, 3) for idx in range(1, 3)
] ]
documents.extend([certificate] + certificate_keys + certificates)
self._test_layering( self._test_layering(
documents, site_expected=site_expected, documents, site_expected=site_expected,
global_expected=global_expected, global_expected=global_expected)
substitution_sources=[certificate] + certificate_keys +
certificates)
@mock.patch('deckhand.engine.layering.LOG', autospec=True) @mock.patch('deckhand.engine.layering.LOG', autospec=True)
def test_parent_and_child_undergo_layering_and_substitution_empty_layers( def test_parent_and_child_undergo_layering_and_substitution_empty_layers(
@ -571,11 +572,11 @@ class TestDocumentLayeringWithSubstitution(
certificate_key = secrets_factory.gen_test( certificate_key = secrets_factory.gen_test(
'CertificateKey', 'cleartext', data='site-secret', 'CertificateKey', 'cleartext', data='site-secret',
name='site-cert') name='site-cert')
documents.extend([certificate] + [certificate_key])
self._test_layering( self._test_layering(
documents, site_expected=site_expected, documents, site_expected=site_expected,
global_expected=global_expected, global_expected=global_expected)
substitution_sources=[certificate, certificate_key])
expected_message = ( expected_message = (
'%s is an empty layer with no documents. It will be discarded ' '%s is an empty layer with no documents. It will be discarded '
@ -661,21 +662,19 @@ class TestDocumentLayeringWithSubstitution(
certificate_key = secrets_factory.gen_test( certificate_key = secrets_factory.gen_test(
'CertificateKey', 'cleartext', data='site-secret', 'CertificateKey', 'cleartext', data='site-secret',
name='site-cert') name='site-cert')
documents.extend([certificate] + [certificate_key])
# Pass in the documents in reverse order to ensure that the dependency # Pass in the documents in reverse order to ensure that the dependency
# chain by default is not linear and thus requires sorting. # chain by default is not linear and thus requires sorting.
self._test_layering( self._test_layering(
list(reversed(documents)), site_expected=site_expected, list(reversed(documents)), site_expected=site_expected,
global_expected=global_expected, global_expected=global_expected)
substitution_sources=[certificate, certificate_key] + documents)
# Try different permutations of document orders for good measure. # Try different permutations of document orders for good measure.
for document_order in list(itertools.permutations(documents))[:10]: for document_order in list(itertools.permutations(documents))[:10]:
self._test_layering( self._test_layering(
document_order, site_expected=site_expected, document_order, site_expected=site_expected,
global_expected=global_expected, global_expected=global_expected)
substitution_sources=[
certificate, certificate_key] + documents)
def test_layering_and_substitution_site_abstract_and_global_concrete(self): def test_layering_and_substitution_site_abstract_and_global_concrete(self):
"""Verifies that if a global document is abstract, yet has """Verifies that if a global document is abstract, yet has
@ -706,10 +705,10 @@ class TestDocumentLayeringWithSubstitution(
doc_factory = factories.DocumentFactory(2, [1, 1]) doc_factory = factories.DocumentFactory(2, [1, 1])
documents = doc_factory.gen_test(mapping, site_abstract=False, documents = doc_factory.gen_test(mapping, site_abstract=False,
global_abstract=True) global_abstract=True)
documents.append(certificate)
site_expected = {"global": "random", "cert": "global-secret", site_expected = {"global": "random", "cert": "global-secret",
"site": "stuff"} "site": "stuff"}
global_expected = None global_expected = None
self._test_layering(documents, site_expected, self._test_layering(documents, site_expected,
global_expected=global_expected, global_expected=global_expected)
substitution_sources=[certificate])

5
deckhand/tests/unit/engine/test_document_layering_and_substitution_negative.py
View File

@ -80,7 +80,7 @@ class TestDocumentLayeringWithSubstitutionNegative(
# chain by default is not linear and thus requires sorting. # chain by default is not linear and thus requires sorting.
self.assertRaises( self.assertRaises(
errors.SubstitutionDependencyCycle, layering.DocumentLayering, errors.SubstitutionDependencyCycle, layering.DocumentLayering,
documents, substitution_sources=documents) documents)
def test_layering_with_substitution_self_reference_fails(self): def test_layering_with_substitution_self_reference_fails(self):
"""Validate that a substitution self-reference fails. """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 # Pass in the documents in reverse order to ensure that the dependency
# chain by default is not linear and thus requires sorting. # chain by default is not linear and thus requires sorting.
self.assertRaises( self.assertRaises(
errors.SubstitutionDependencyCycle, self._test_layering, documents, errors.SubstitutionDependencyCycle, self._test_layering, documents)
substitution_sources=documents)
def test_layering_with_missing_substitution_source_raises_exc(self): def test_layering_with_missing_substitution_source_raises_exc(self):
"""Validate that a missing substitution source document fails.""" """Validate that a missing substitution source document fails."""