airship
/
deckhand
Code Issues Proposed changes

[fix] Parent substitution/layering before replacement 

Currently it doesn't seem document replacement works
exactly as expected: The parent-replacement document
can receive layering and substitution data prior to
being replaced. Currently, Deckhand does not account
for this scenario.

A child-replacement depends on its parent-replacement
the same way any child depends on its parent: so that the
child layers with its parent only after the parent has
received all layering and substitution data. But other
documents that depend on the parent-replacement actually
depend on the child-replacement instead as the
child-replacement replaces its parent. So the dependency
chain is: PR -> CR -> anything that layers with PR.

A unit and functional test have been added for regression.

Co-Authored-By: Felipe Monteiro <felipe.monteiro@att.com>
Change-Id: I353393f416aa6e441d84add9ebedcd152944d7e8
This commit is contained in:
Felipe Monteiro 2018-05-13 13:28:39 -04:00
parent a004c7a19e
commit 177675e96f
6 changed files with 212 additions and 36 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
deckhand/common/utils.py
View File

@ -129,8 +129,6 @@ def _populate_data_with_attributes(jsonpath, data):
# Handle case where an object needs to be created.
elif path not in d:
if '\'' or '\"' in path:
LOG.info('Stripping subpath %s of start and end quotes during '
'path vivification for jsonpath: %s.', path, jsonpath)
path = path.strip('\'').strip('\"')
d.setdefault(path, {})
d = d.get(path)

67
deckhand/engine/layering.py
View File

@ -298,14 +298,32 @@ class DocumentLayering(object):
"""
result = []
def _get_ancestor(doc, parent_meta):
parent = self._documents_by_index.get(parent_meta)
# Return the parent's replacement, but if that replacement is the
# document itself then return the parent.
use_replacement = (
parent and parent.has_replacement and
parent.replaced_by is not doc
)
if use_replacement:
parent = parent.replaced_by
return parent
g = networkx.DiGraph()
for document in self._documents_by_index.values():
if document.has_replacement:
g.add_edge(document.meta, document.replaced_by.meta)
elif document.parent_selector and not document.is_replacement:
if document.parent_selector:
# NOTE: A child-replacement depends on its parent-replacement
# the same way any child depends on its parent: so that the
# child layers with its parent only after the parent has
# received all layering and substitution data. But other
# non-replacement child documents must first wait for the
# child-relacement to layer with the parent, so that they
# can use the replaced data.
parent_meta = self._parents.get(document.meta)
if parent_meta:
g.add_edge(document.meta, parent_meta)
ancestor = _get_ancestor(document, parent_meta)
if ancestor:
g.add_edge(document.meta, ancestor.meta)
for sub in document.substitutions:
# Retrieve the correct substitution source using
@ -591,22 +609,6 @@ class DocumentLayering(object):
return overall_data
def _get_parent_or_replacement(self, doc, parent_meta):
"""Returns the document's parent or the document's parent's
replacement.
:param DocumentDict doc: Document used to get parent.
:param tuple parent_meta: Unique parent identifier.
:returns: Parent document or its replacement if the parent has one.
:rtype: DocumentDict
"""
parent = self._documents_by_index.get(parent_meta)
# Return the parent's replacement, but if that replacement is the
# document itself then return the parent.
if parent and parent.has_replacement and parent.replaced_by is not doc:
parent = parent.replaced_by
return parent
def render(self):
"""Perform layering on the list of documents passed to ``__init__``.
@ -629,11 +631,14 @@ class DocumentLayering(object):
if doc.is_control:
continue
LOG.debug("Rendering document %s:%s:%s", *doc.meta)
if doc.parent_selector:
parent_meta = self._parents.get(doc.meta)
if parent_meta:
parent = self._get_parent_or_replacement(doc, parent_meta)
LOG.debug("Using parent %s:%s:%s", *parent_meta)
parent = self._documents_by_index[parent_meta]
if doc.actions:
rendered_data = parent
@ -652,8 +657,7 @@ class DocumentLayering(object):
doc, parent, action)
except Exception: # nosec
pass
if not doc.is_abstract:
doc.data = rendered_data.data
doc.data = rendered_data.data
self.secrets_substitution.update_substitution_sources(
doc.schema, doc.name, rendered_data.data)
self._documents_by_index[doc.meta] = rendered_data
@ -679,10 +683,10 @@ class DocumentLayering(object):
if substituted_data:
rendered_data = substituted_data[0]
# Update the actual document data if concrete.
if not doc.is_abstract:
doc.data = rendered_data.data
self.secrets_substitution.update_substitution_sources(
doc.schema, doc.name, rendered_data.data)
doc.data = rendered_data.data
if not doc.has_replacement:
self.secrets_substitution.update_substitution_sources(
doc.schema, doc.name, rendered_data.data)
self._documents_by_index[doc.meta] = rendered_data
# Otherwise, retrieve the encrypted data for the document if its
# data has been encrypted so that future references use the actual
@ -696,6 +700,13 @@ class DocumentLayering(object):
doc.schema, doc.name, encrypted_data)
self._documents_by_index[doc.meta] = encrypted_data
# NOTE: Since the child-replacement is always prioritized, before
# other children, as soon as the child-replacement layers with the
# parent (which already has undergone layering and substitution
# itself), replace the parent data with that of the replacement.
if doc.is_replacement:
parent.data = doc.data
# Return only concrete documents and non-replacements.
return [d for d in self._sorted_documents
if d.is_abstract is False and d.has_replacement is False]

51
deckhand/tests/functional/gabbits/replacement/replacement.yaml Normal file
View File

@ -0,0 +1,51 @@
# Tests success path for advanced replacement scenario, where
# parent-replacement document receives substitutions and is then layered
# with by its child-replacement document, which replaces its parent.
#
# 1. Purges existing data to ensure test isolation.
# 2. Adds initial documents with replacement scenario described above.
# 3. Verifies correctly layered, substituted and replaced data.
defaults:
request_headers:
content-type: application/x-yaml
response_headers:
content-type: application/x-yaml
verbose: true
tests:
- name: purge
desc: Begin testing from known state.
DELETE: /api/v1.0/revisions
status: 204
response_headers: null
- name: initialize
desc: Create initial documents
PUT: /api/v1.0/buckets/mop/documents
status: 200
data: <@resources/replacement.yaml
- name: verify_replacement_document_receives_substitution
desc: |
Tests success path for advanced replacement scenario, where
parent-replacement document receives substitutions and is then layered
with by its child-replacement document, which replaces its parent.
GET: /api/v1.0/revisions/$RESPONSE['$.[0].status.revision']/rendered-documents
query_parameters:
schema: armada/Chart/v1
status: 200
response_multidoc_jsonpaths:
$.`len`: 1
$.[*].metadata.name: example-chart-01
$.[*].metadata.layeringDefinition.layer: site
$.[*].data:
chart:
details:
data: bar
values:
tls:
certificate: |
CERTIFICATE DATA
key: |
KEY DATA

76
deckhand/tests/functional/gabbits/resources/replacement.yaml Normal file
View File

@ -0,0 +1,76 @@
---
schema: deckhand/LayeringPolicy/v1
metadata:
schema: metadata/Control/v1
name: layering-policy
data:
layerOrder:
- region
- site
---
schema: deckhand/Certificate/v1
metadata:
name: example-cert
schema: metadata/Document/v1
layeringDefinition:
layer: site
storagePolicy: cleartext
data: |
CERTIFICATE DATA
---
schema: deckhand/CertificateKey/v1
metadata:
name: example-key
schema: metadata/Document/v1
layeringDefinition:
layer: site
storagePolicy: cleartext
data: |
KEY DATA
---
schema: armada/Chart/v1
metadata:
name: example-chart-01
schema: metadata/Document/v1
labels:
name: parent-chart
layeringDefinition:
layer: region
substitutions:
- dest:
path: .chart.values.tls.certificate
src:
schema: deckhand/Certificate/v1
name: example-cert
path: .
data:
chart:
details:
data: foo
values: {}
---
schema: armada/Chart/v1
metadata:
name: example-chart-01
schema: metadata/Document/v1
replacement: true
layeringDefinition:
layer: site
parentSelector:
name: parent-chart
actions:
- method: merge
path: .
substitutions:
- dest:
path: .chart.values.tls.key
src:
schema: deckhand/CertificateKey/v1
name: example-key
path: .
data:
chart:
details:
data: bar
values: {}
...

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

@ -323,8 +323,9 @@ data:
site_expected = 'should not change'
self._test_layering(documents, site_expected, global_expected=None)
error_re = r'^Skipped layering for document.*'
self.assertRegex(mock_log.debug.mock_calls[0][1][0], error_re)
msg_re = 'Skipped layering for document'
mock_calls = [x[1][0] for x in mock_log.debug.mock_calls]
self.assertTrue(any(x.startswith(msg_re) for x in mock_calls))
class TestDocumentLayering2Layers(TestDocumentLayering):

47
deckhand/tests/unit/engine/test_document_layering_and_replacement.py
View File

@ -13,6 +13,7 @@
# limitations under the License.
import itertools
import os
import yaml
from deckhand.tests.unit.engine import test_document_layering
@ -106,10 +107,11 @@ data:
# The replacer should be used as the source.
self._test_layering(self.documents, site_expected,
global_expected=global_expected)
# Attempt the same scenario but reverse the order of the documents,
# which verifies that the replacer always takes priority.
self._test_layering(self.documents, site_expected,
global_expected=global_expected)
# Try different permutations of document orders for good measure.
for documents in list(itertools.permutations(self.documents))[:10]:
self._test_layering(
documents, site_expected=site_expected,
global_expected=global_expected)
def test_replacement_with_layering_with_replacer(self):
"""Verify that replacement works alongside layering. This scenario
@ -212,3 +214,40 @@ data:
self._test_layering(
documents, site_expected=site_expected,
global_expected=global_expected)
def test_replacement_document_receives_substitution(self):
"""Verifies that the parent-replacement receives substitution data
prior to the child-replacement layering with it, which in turn is
done prior to any other document attempting to substitute from or
layer with the child-replacement (which replaces its parent).
"""
test_path = os.path.join(
os.getcwd(), 'deckhand', 'tests', 'functional', 'gabbits',
'resources', 'replacement.yaml')
with open(test_path) as f:
self.documents = list(yaml.safe_load_all(f))
site_expected = [
"CERTIFICATE DATA\n",
"KEY DATA\n",
{
'chart': {
'details': {'data': 'bar'},
'values': {
'tls': {
'certificate': 'CERTIFICATE DATA\n',
'key': 'KEY DATA\n'
}
}
}
}
]
self._test_layering(self.documents, site_expected=site_expected,
region_expected=None)
# Try different permutations of document orders for good measure.
for documents in list(itertools.permutations(self.documents))[:10]:
self._test_layering(
documents, site_expected=site_expected,
region_expected=None)