airship
/
deckhand
Code Issues Proposed changes

Layering edge case: Multiple empty layers 

This PS handles yet another annoying edge case with layering:
What if multiple layers in the layerOrder defined in the
LayeringPolicy are empty?

https://review.gerrithub.io/#/c/395388/ fixed substitution
for a layerOrder edge case like:

  - region (empty)
  - site (document needs substitution)

But that PS does not handle the following case:

  - empty layer
  - empty layer
  - site (document needs substitution)

Or even:

  - empty layer
  - empty layer
  - global (document needs substitution)
  - empty layer
  - site (document needs substitution)

Change-Id: Ia64ce627454289b18739964bdc488c0713c6b39a
This commit is contained in:
Felipe Monteiro 2018-01-20 16:30:56 -05:00
parent 3dc3f4c47b
commit 5108f56d36
4 changed files with 141 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
deckhand/engine/document_wrapper.py
View File

@ -58,6 +58,10 @@ class DocumentDict(dict):
return utils.jsonpath_parse(
self, 'metadata.layeringDefinition.layer')
@property
def layer_order(self):
return utils.jsonpath_parse(self, 'data.layerOrder')
@property
def parent_selector(self):
return utils.jsonpath_parse(

51
deckhand/engine/layering.py
View File

@ -19,7 +19,7 @@ from oslo_log import log as logging
from deckhand.engine import document_wrapper
from deckhand.engine import secrets_manager
from deckhand.engine import utils
from deckhand.engine import utils as engine_utils
from deckhand import errors
from deckhand import types
@ -65,9 +65,6 @@ class DocumentLayering(object):
:raises IndeterminateDocumentParent: If more than one parent document
was found for a document.
"""
layered_docs = list(
filter(lambda x: 'layeringDefinition' in x.metadata,
self._documents))
# ``all_children`` is a counter utility for verifying that each
# document has exactly one parent.
@ -88,34 +85,34 @@ class DocumentLayering(object):
# empty list.
return children
for other_doc in layered_docs:
for other_doc in self._layered_docs:
# Documents with different schemas are never layered together,
# so consider only documents with same schema as candidates.
is_potential_child = (
other_doc.layer == children_doc_layer and
other_doc.schema == doc.schema
)
if (is_potential_child):
if is_potential_child:
# A document can have many labels but should only have one
# explicit label for the parentSelector.
parent_sel = other_doc.parent_selector
parent_sel_key = list(parent_sel.keys())[0]
parent_sel_val = list(parent_sel.values())[0]
doc_labels = doc.labels
try:
parent_sel_key = list(parent_sel.keys())[0]
parent_sel_val = list(parent_sel.values())[0]
except IndexError:
continue
if (parent_sel_key in doc_labels and
parent_sel_val == doc_labels[parent_sel_key]):
if (parent_sel_key in doc.labels and
parent_sel_val == doc.labels[parent_sel_key]):
children.append(other_doc)
return children
for layer in self._layer_order:
docs_by_layer = list(filter(
(lambda x: x.layer == layer), layered_docs))
(lambda x: x.layer == layer), self._layered_docs))
for doc in docs_by_layer:
children = _get_children(doc)
if children:
all_children.update(children)
self._children.setdefault((doc.name, doc.schema),
@ -124,7 +121,8 @@ class DocumentLayering(object):
all_children_elements = list(all_children.elements())
secondary_docs = list(
filter(lambda d: d.layer != self._layer_order[0],
layered_docs))
self._layered_docs)
) if self._layer_order else []
for doc in secondary_docs:
# Unless the document is the topmost document in the
# `layerOrder` of the LayeringPolicy, it should be a child document
@ -144,7 +142,21 @@ class DocumentLayering(object):
doc.parent_selector)
raise errors.IndeterminateDocumentParent(document=doc)
return layered_docs
return self._layered_docs
def _get_layering_order(self, layering_policy):
# Pre-processing stage that removes empty layers from the
# ``layerOrder`` in the layering policy.
layer_order = list(layering_policy.layer_order)
for layer in layer_order[:]:
docs_by_layer = list(filter(
(lambda x: x.layer == layer), self._layered_docs))
if not docs_by_layer:
LOG.info('%s is an empty layer with no documents. It will be '
'discarded from the layerOrder during the layering '
'process.', layer)
layer_order.remove(layer)
return layer_order
def _extract_layering_policy(self, documents):
for doc in documents:
@ -178,7 +190,10 @@ class DocumentLayering(object):
'documents.')
LOG.error(error_msg)
raise errors.LayeringPolicyNotFound()
self._layer_order = list(self._layering_policy['data']['layerOrder'])
self._layered_docs = list(
filter(lambda x: 'layeringDefinition' in x.metadata,
self._documents))
self._layer_order = self._get_layering_order(self._layering_policy)
self._parentless_documents = []
self._layered_documents = self._calc_document_children()
self._substitution_sources = substitution_sources or []
@ -234,7 +249,7 @@ class DocumentLayering(object):
# do a simple merge.
if (isinstance(rendered_data[last_key], dict)
and isinstance(child_data[last_key], dict)):
utils.deep_merge(
engine_utils.deep_merge(
rendered_data[last_key], child_data[last_key])
else:
rendered_data.setdefault(last_key, child_data[last_key])

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

@ -12,6 +12,8 @@
# See the License for the specific language governing permissions and
# limitations under the License.
import mock
from deckhand import factories
from deckhand.tests.unit.engine import test_document_layering
@ -54,8 +56,11 @@ class TestDocumentLayeringWithSubstitution(
substitution_sources=[certificate])
def test_layering_and_substitution_no_children(self):
# Validate that a document with no children still undergoes
# substitution.
"""Validate that a document with no children undergoes substitution.
global -> (no children) requires substitution
site -> (no parent) do nothing
"""
mapping = {
"_GLOBAL_DATA_1_": {"data": {"a": {"x": 1, "y": 2}}},
"_GLOBAL_SUBSTITUTIONS_1_": [{
@ -91,8 +96,12 @@ class TestDocumentLayeringWithSubstitution(
global_expected=global_expected,
substitution_sources=[certificate])
def test_layering_and_substitution_no_parent(self):
# Validate that a document with no parent undergoes substitution.
def test_substitution_without_parent_document(self):
"""Validate that a document with no parent undergoes substitution.
global -> do nothing
site -> (no parent & no children) requires substitution
"""
mapping = {
"_GLOBAL_DATA_1_": {"data": {"a": {"x": 1, "y": 2}}},
"_SITE_DATA_1_": {"data": {"b": 4}},
@ -129,7 +138,15 @@ class TestDocumentLayeringWithSubstitution(
global_expected=global_expected,
substitution_sources=[certificate])
def test_layering_parent_and_child_undergo_substitution(self):
def test_parent_and_child_undergo_layering_and_substitution(self):
"""Validate that parent and child documents both undergo layering and
substitution.
global -> requires substitution
|
v
site -> requires substitution
"""
mapping = {
"_GLOBAL_DATA_1_": {"data": {"a": {"x": 1, "y": 2}}},
"_GLOBAL_SUBSTITUTIONS_1_": [{
@ -177,3 +194,82 @@ class TestDocumentLayeringWithSubstitution(
documents, site_expected=site_expected,
global_expected=global_expected,
substitution_sources=[certificate, certificate_key])
@mock.patch('deckhand.engine.layering.LOG', autospec=True)
def test_parent_and_child_undergo_layering_and_substitution_empty_layers(
self, mock_log):
"""Validate that parent and child documents both undergo substitution
and layering.
empty layer -> discard
|
v
empty layer -> discard
|
v
global -> requires substitution
|
v
empty layer -> discard
|
V
site -> requires substitution (layered with global)
Where the site's parent is actually the global document.
"""
mapping = {
"_GLOBAL_DATA_1_": {"data": {"a": {"x": 1, "y": 2}}},
"_GLOBAL_SUBSTITUTIONS_1_": [{
"dest": {
"path": ".b"
},
"src": {
"schema": "deckhand/Certificate/v1",
"name": "global-cert",
"path": "."
}
}],
"_SITE_DATA_1_": {"data": {"c": "need-site-secret"}},
"_SITE_ACTIONS_1_": {
"actions": [{"method": "merge", "path": "."}]},
"_SITE_SUBSTITUTIONS_1_": [{
"dest": {
"path": ".c"
},
"src": {
"schema": "deckhand/CertificateKey/v1",
"name": "site-cert",
"path": "."
}
}],
}
doc_factory = factories.DocumentFactory(2, [1, 1])
documents = doc_factory.gen_test(mapping, site_abstract=False)
documents[0]['data']['layerOrder'] = [
'empty_1', 'empty_2', 'global', 'empty_3', 'site']
secrets_factory = factories.DocumentSecretFactory()
global_expected = {'a': {'x': 1, 'y': 2}, 'b': 'global-secret'}
site_expected = {'a': {'x': 1, 'y': 2}, 'b': 'global-secret',
'c': 'site-secret'}
certificate = secrets_factory.gen_test(
'Certificate', 'cleartext', data='global-secret',
name='global-cert')
certificate_key = secrets_factory.gen_test(
'CertificateKey', 'cleartext', data='site-secret',
name='site-cert')
self._test_layering(
documents, site_expected=site_expected,
global_expected=global_expected,
substitution_sources=[certificate, certificate_key])
expected_message = (
'%s is an empty layer with no documents. It will be discarded '
'from the layerOrder during the layering process.')
expected_log_calls = [mock.call(expected_message, layer)
for layer in ('empty_1', 'empty_2', 'empty_3')]
mock_log.info.assert_has_calls(expected_log_calls)

4
deckhand/tests/unit/engine/test_document_layering_negative.py
View File

@ -79,7 +79,9 @@ class TestDocumentLayeringNegative(
# The site will not be able to find a correct parent.
layering.DocumentLayering(documents)
self.assertRegexpMatches(mock_log.info.mock_calls[0][1][0],
'Could not find parent for document .*')
'%s is an empty layer with no documents. '
'It will be discarded from the layerOrder'
' during the layering process.')
mock_log.info.reset_mock()
@mock.patch.object(layering, 'LOG', autospec=True)