airship
/
deckhand
Code Issues Proposed changes

Fix: return only concrete documents from layering module 

Pegleg uses Deckhand to invoke the layering module
directly which returned all documents which were only later
filtered to only return concrete in the controller. However,
the Deckhand layering module must itself only return concrete
so that Promenade, when it calls the module directly, only
receives concrete documents.

Abstract documents should only be processed internally by DH,
never returning them to user.

This PS makes necessary changes to layering module and unit tests.
Functional tests, as they call the controller itself, didn't need
to be refactored.

Change-Id: Ib5a49c5d31124133a10b646f55100628aa442512
This commit is contained in:
Felipe Monteiro 2018-02-09 16:32:29 +00:00
parent 02528bc3af
commit 2da9aa5055
5 changed files with 46 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
deckhand/control/revision_documents.py
View File

@ -124,11 +124,11 @@ class RenderedDocumentsResource(api_base.BaseResource):
# Filters to be applied post-rendering, because many documents are
# involved in rendering. User filters can only be applied once all
# documents have been rendered.
# documents have been rendered. Note that `layering` module only
# returns concrete documents, so no filtering for that is needed here.
order_by = sanitized_params.pop('order', None)
sort_by = sanitized_params.pop('sort', None)
user_filters = sanitized_params.copy()
user_filters['metadata.layeringDefinition.abstract'] = False
rendered_documents = [
d for d in rendered_documents if utils.deepfilter(

6
deckhand/engine/layering.py
View File

@ -383,8 +383,7 @@ class DocumentLayering(object):
the child, and its ``metadata.labels`` must much the child's
``metadata.layeringDefinition.parentSelector``.
:returns: The list of rendered documents (does not include layering
policy document).
:returns: The list of concrete rendered documents.
:rtype: List[dict]
:raises UnsupportedActionMethod: If the layering action isn't found
@ -458,4 +457,5 @@ class DocumentLayering(object):
if substituted_data:
doc = substituted_data[0]
return self._documents_to_layer
# Return only concrete documents.
return [d for d in self._documents_to_layer if d.is_abstract is False]

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

@ -48,27 +48,33 @@ class TestDocumentLayering(test_base.DeckhandTestCase):
if layer == 'global':
global_docs.append(doc)
if site_expected:
if site_expected is not None:
if not isinstance(site_expected, list):
site_expected = [site_expected]
for idx, expected in enumerate(site_expected):
self.assertEqual(expected, site_docs[idx].get('data'),
'Actual site data does not match expected.')
if region_expected:
else:
self.assertEmpty(site_docs)
if region_expected is not None:
if not isinstance(region_expected, list):
region_expected = [region_expected]
for idx, expected in enumerate(region_expected):
self.assertEqual(expected, region_docs[idx].get('data'),
'Actual region data does not match expected.')
if global_expected:
else:
self.assertEmpty(region_docs)
if global_expected is not None:
if not isinstance(global_expected, list):
global_expected = [global_expected]
for idx, expected in enumerate(global_expected):
self.assertEqual(expected, global_docs[idx].get('data'),
'Actual global data does not match expected.')
else:
self.assertEmpty(global_docs)
class TestDocumentLayering2Layers(TestDocumentLayering):
@ -122,7 +128,7 @@ class TestDocumentLayering2Layers(TestDocumentLayering):
def test_layering_default_scenario_multi_parentselector_no_match(self):
mapping = {
"_GLOBAL_DATA_1_": {"data": {"a": {"x": 1, "y": 2}}},
"_SITE_DATA_1_": {"data": {"b": 4}},
"_SITE_DATA_1_": {},
"_SITE_ACTIONS_1_": {
"actions": [{"method": "merge", "path": "."}]}
}
@ -241,8 +247,8 @@ class TestDocumentLayering2LayersAbstractConcrete(TestDocumentLayering):
documents = doc_factory.gen_test(mapping, site_abstract=True,
global_abstract=True)
site_expected = {"a": {"x": 7, "z": 3}, "b": 4}
global_expected = {'a': {'x': 1, 'y': 2}, 'c': 9}
site_expected = None
global_expected = None
self._test_layering(documents, site_expected,
global_expected=global_expected)
@ -375,7 +381,7 @@ class TestDocumentLayering3Layers(TestDocumentLayering):
documents = doc_factory.gen_test(mapping, site_abstract=False)
site_expected = {'a': {'z': 3}, 'b': 4}
region_expected = {"a": {"z": 3}} # Region is abstract.
region_expected = None # Region is abstract.
self._test_layering(documents, site_expected, region_expected)
def test_layering_delete_everything(self):
@ -538,11 +544,11 @@ class TestDocumentLayering3LayersAbstractConcrete(TestDocumentLayering):
"actions": [{"method": "replace", "path": ".b"}]}
}
doc_factory = factories.DocumentFactory(3, [1, 1, 1])
documents = doc_factory.gen_test(mapping, site_abstract=False,
region_abstract=True)
documents = doc_factory.gen_test(
mapping, site_abstract=False, region_abstract=True)
site_expected = {"a": {"x": 1, "y": 2, "z": 3}, "b": 4}
region_expected = {"a": {"z": 3}, "b": 5, "c": 11}
region_expected = None
self._test_layering(documents, site_expected, region_expected)
def test_layering_site_region_and_global_concrete(self):
@ -739,13 +745,13 @@ class TestDocumentLayering3LayersScenario(TestDocumentLayering):
---
metadata:
labels: {name: kubernetes-etcd-global}
layeringDefinition: {abstract: true, layer: global}
layeringDefinition: {abstract: false, layer: global}
name: kubernetes-etcd-global
schema: metadata/Document/v1
storagePolicy: cleartext
schema: armada/Chart/v1
data:
chart_name: etcd
chart_name: global-etcd
---
# This document is included so that this middle layer isn't stripped away.
metadata:
@ -785,7 +791,9 @@ data:
...
"""
documents = list(yaml.safe_load_all(test_yaml))
self._test_layering(documents, site_expected={'chart_name': 'etcd'})
self._test_layering(
documents, site_expected={'chart_name': 'global-etcd'},
region_expected={}, global_expected={'chart_name': 'global-etcd'})
def test_layering_using_first_parent_as_actual_parent(self):
"""Test that layering works when a child document has layer N and has
@ -847,7 +855,8 @@ data:
"""
documents = list(yaml.safe_load_all(test_yaml))
self._test_layering(
documents, site_expected={'chart_name': 'region-etcd'})
documents, site_expected={'chart_name': 'region-etcd'},
region_expected={'chart_name': 'region-etcd'})
class TestDocumentLayering3Layers2Regions2Sites(TestDocumentLayering):
@ -884,8 +893,8 @@ class TestDocumentLayering3Layers2Regions2Sites(TestDocumentLayering):
site_expected = [{"a": 1, "b": 2, "c": 3, "d": 4, "g": 7, "h": 8},
{"a": 1, "b": 2, "e": 5, "f": 6, "i": 9, "j": 10}]
region_expected = [{"c": 3, "d": 4}, {"e": 5, "f": 6}]
global_expected = {"a": 1, "b": 2}
region_expected = None
global_expected = None
self._test_layering(documents, site_expected, region_expected,
global_expected)
@ -923,6 +932,6 @@ class TestDocumentLayering3Layers2Regions2Sites(TestDocumentLayering):
{"a": 1, "b": 2, "e": 5, "f": 6, "i": 9, "j": 10}]
region_expected = [{"a": 1, "b": 2, "c": 3, "d": 4},
{"a": 1, "b": 2, "e": 5, "f": 6}]
global_expected = {"a": 1, "b": 2}
global_expected = None
self._test_layering(documents, site_expected, region_expected,
global_expected)

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

@ -41,7 +41,8 @@ class TestDocumentLayeringWithSubstitution(
"actions": [{"method": "merge", "path": "."}]}
}
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=False)
secrets_factory = factories.DocumentSecretFactory()
certificate = secrets_factory.gen_test(
@ -79,7 +80,8 @@ class TestDocumentLayeringWithSubstitution(
"actions": [{"method": "merge", "path": "."}]}
}
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=False)
# Remove the labels from the global document so that the site document
# (the child) has no parent.
@ -121,7 +123,8 @@ class TestDocumentLayeringWithSubstitution(
"actions": [{"method": "merge", "path": "."}]}
}
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=False)
# Remove the labels from the global document so that the site document
# (the child) has no parent.
@ -176,7 +179,8 @@ class TestDocumentLayeringWithSubstitution(
}],
}
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=False)
secrets_factory = factories.DocumentSecretFactory()
global_expected = {'a': {'x': 1, 'y': 2}, 'b': 'global-secret'}
@ -245,7 +249,8 @@ class TestDocumentLayeringWithSubstitution(
}],
}
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=False)
documents[0]['data']['layerOrder'] = [
'empty_1', 'empty_2', 'global', 'empty_3', 'site']
secrets_factory = factories.DocumentSecretFactory()

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

@ -68,9 +68,10 @@ class TestDocumentLayeringNegative(
@mock.patch.object(layering, 'LOG', autospec=True)
def test_layering_with_empty_layer(self, mock_log):
doc_factory = factories.DocumentFactory(1, [1])
documents = doc_factory.gen_test({}, site_abstract=False)
documents = doc_factory.gen_test({}, global_abstract=False)
self._test_layering([documents[0]], global_expected={})
# Only pass in the LayeringPolicy.
self._test_layering([documents[0]], global_expected=None)
mock_log.info.assert_has_calls([
mock.call(
'%s is an empty layer with no documents. It will be discarded '
@ -161,11 +162,11 @@ class TestDocumentLayeringNegative(
@mock.patch.object(layering, 'LOG', autospec=True)
def test_multiple_layering_policy_logs_warning(self, mock_log):
doc_factory = factories.DocumentFactory(1, [1])
documents = doc_factory.gen_test({}, site_abstract=False)
documents = doc_factory.gen_test({}, global_abstract=False)
# Copy the same layering policy so that 2 are passed in, causing a
# warning to be raised.
documents.append(documents[0])
self._test_layering(documents, site_expected={})
self._test_layering(documents, global_expected={})
mock_log.warning.assert_called_once_with(
'More than one layering policy document was passed in. Using the '
'first one found: [%s] %s.', documents[0]['schema'],