airship
/
deckhand
Code Issues Proposed changes

Fix: Document should not layer with parent if no layering actions 

Currently, if a document has a parent but no layering actions,
the document immediately inherents its parents' data, which is a
bug. Instead, the child document should only layer with its
parent's data and then update its own data if it has at least
one layering action.

In addition, the base_schema.yaml under `deckhand.schemas`
has been updated to require that actions be required and
contain at least 1 layering action when parentSelector
is provided and that parentSelector be required when
actions is provided and that at least one key-value
pair be provided. (Empty actions array or empty
parentSelector object is meaningless and should be
disallowed/discouraged.)

This means that actions and parentSelector must always
both be provided (though providing neither is also
legal because layering is optional).

Unit tests have been added to verify the schema updates.

Change-Id: I77d54e2b216efc54b466f94d82ee8d36ca169c26
This commit is contained in:
Felipe Monteiro 2018-03-05 13:30:12 -05:00 committed by Anthony Lin
parent d27814cb1e
commit 5c411dd05b
12 changed files with 193 additions and 52 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

34
deckhand/engine/layering.py
View File

@ -475,20 +475,26 @@ class DocumentLayering(object):
if parent_meta:
parent = self._documents_by_index[parent_meta]
rendered_data = parent
# Apply each action to the current document.
for action in doc.actions:
LOG.debug('Applying action %s to document with '
'name=%s, schema=%s, layer=%s.', action,
doc.name, doc.schema, doc.layer)
rendered_data = self._apply_action(
action, doc, rendered_data)
if not doc.is_abstract:
doc.data = rendered_data.data
self.secrets_substitution.update_substitution_sources(
doc.schema, doc.name, rendered_data.data)
self._documents_by_index[(doc.schema, doc.name)] = (
rendered_data)
if doc.actions:
rendered_data = parent
for action in doc.actions:
LOG.debug('Applying action %s to document with '
'name=%s, schema=%s, layer=%s.', action,
doc.name, doc.schema, doc.layer)
rendered_data = self._apply_action(
action, doc, rendered_data)
if not doc.is_abstract:
doc.data = rendered_data.data
self.secrets_substitution.update_substitution_sources(
doc.schema, doc.name, rendered_data.data)
self._documents_by_index[(doc.schema, doc.name)] = (
rendered_data)
else:
LOG.info('Skipped layering for document [%s] %s which '
'has a parent [%s] %s, but no associated '
'layering actions.', doc.schema, doc.name,
parent.schema, parent.name)
# Perform substitutions on abstract data for child documents that
# inherit from it, but only update the document's data if concrete.

23
deckhand/engine/schemas/base_schema.yaml
View File

@ -19,6 +19,23 @@ metadata:
schema: metadata/Control/v1
data:
$schema: http://json-schema.org/schema#
definitions:
parent_selector_requires_actions:
dependencies:
# Requires that if parentSelector is provided, then actions is
# required and must contain at least 1 item.
parentSelector:
required:
- actions
actions_requires_parent_selector:
dependencies:
# Requires that if actions are provided, then so too must
# parentSelector.
actions:
required:
- parentSelector
properties:
schema:
type: string
@ -45,8 +62,10 @@ data:
type: boolean
parentSelector:
type: object
minProperties: 1
actions:
type: array
minItems: 1
items:
type: object
properties:
@ -62,6 +81,9 @@ data:
- method
- path
additionalProperties: false
allOf:
- $ref: "#/definitions/parent_selector_requires_actions"
- $ref: "#/definitions/actions_requires_parent_selector"
substitutions:
type: array
items:
@ -112,6 +134,7 @@ data:
- integer
- array
- object
additionalProperties: false
required:
- schema

6
deckhand/factories.py
View File

@ -105,8 +105,7 @@ class DocumentFactory(DeckhandFactory):
"labels": {"": ""},
"layeringDefinition": {
"abstract": False,
"layer": "",
"actions": []
"layer": ""
},
"name": "",
"schema": "metadata/Document/v1"
@ -290,7 +289,6 @@ class DocumentFactory(DeckhandFactory):
except KeyError as e:
LOG.debug('Could not map %s because it was not found in '
'the `mapping` dict.', e.args[0])
pass
try:
layer_template['metadata']['layeringDefinition'][
@ -298,7 +296,6 @@ class DocumentFactory(DeckhandFactory):
except KeyError as e:
LOG.debug('Could not map %s because it was not found in '
'the `mapping` dict.', e.args[0])
pass
try:
layer_template['metadata']['substitutions'] = mapping[
@ -306,7 +303,6 @@ class DocumentFactory(DeckhandFactory):
except KeyError as e:
LOG.debug('Could not map %s because it was not found in '
'the `mapping` dict.', e.args[0])
pass
rendered_template.append(layer_template)

6
deckhand/tests/unit/control/test_buckets_controller.py
View File

@ -129,7 +129,11 @@ class TestBucketsController(test_base.BaseControllerTest):
rules = {'deckhand:create_cleartext_documents': '@'}
self.policy.set_rules(rules)
payload = factories.DocumentFactory(2, [1, 1]).gen_test({})
payload = factories.DocumentFactory(2, [1, 1]).gen_test({
'_SITE_ACTIONS_1_': {
'actions': [{'method': 'merge', 'path': '.'}]
}
})
bucket_name = test_utils.rand_name('bucket')
alt_bucket_name = test_utils.rand_name('bucket')

21
deckhand/tests/unit/control/test_rendered_documents_controller.py
View File

@ -34,8 +34,11 @@ class TestRenderedDocumentsController(test_base.BaseControllerTest):
# Create 2 docs: one concrete, one abstract.
documents_factory = factories.DocumentFactory(2, [1, 1])
payload = documents_factory.gen_test(
{}, global_abstract=False, region_abstract=True)
payload = documents_factory.gen_test({
'_SITE_ACTIONS_1_': {
'actions': [{'method': 'merge', 'path': '.'}]
}
}, global_abstract=False)
concrete_doc = payload[1]
resp = self.app.simulate_put(
@ -134,8 +137,11 @@ class TestRenderedDocumentsController(test_base.BaseControllerTest):
for x in range(4):
bucket_name = bucket_names[x]
documents_factory = factories.DocumentFactory(2, [1, 1])
payload = documents_factory.gen_test({}, global_abstract=False,
site_abstract=False)
payload = documents_factory.gen_test({
'_SITE_ACTIONS_1_': {
'actions': [{'method': 'merge', 'path': '.'}]
}
}, global_abstract=False, site_abstract=False)
# Fix up the labels so that each document has a unique parent to
# avoid layering errors.
payload[-2]['metadata']['labels'] = {
@ -299,8 +305,11 @@ class TestRenderedDocumentsControllerSorting(test_base.BaseControllerTest):
self.policy.set_rules(rules)
documents_factory = factories.DocumentFactory(2, [1, 1])
documents = documents_factory.gen_test({}, global_abstract=False,
region_abstract=False, site_abstract=False)
documents = documents_factory.gen_test({
'_SITE_ACTIONS_1_': {
'actions': [{'method': 'merge', 'path': '.'}]
}
}, global_abstract=False, site_abstract=False)
expected_names = ['bar', 'baz', 'foo']
for idx in range(len(documents)):
documents[idx]['metadata']['name'] = expected_names[idx]

18
deckhand/tests/unit/control/test_revision_documents_controller.py
View File

@ -90,7 +90,11 @@ class TestRevisionDocumentsControllerSorting(test_base.BaseControllerTest):
self.policy.set_rules(rules)
documents_factory = factories.DocumentFactory(2, [1, 1])
documents = documents_factory.gen_test({})
documents = documents_factory.gen_test({
'_SITE_ACTIONS_1_': {
'actions': [{'method': 'merge', 'path': '.'}]
}
})
expected_names = ['bar', 'baz', 'foo']
for idx in range(len(documents)):
documents[idx]['metadata']['name'] = expected_names[idx]
@ -121,7 +125,11 @@ class TestRevisionDocumentsControllerSorting(test_base.BaseControllerTest):
self.policy.set_rules(rules)
documents_factory = factories.DocumentFactory(2, [1, 1])
documents = documents_factory.gen_test({})
documents = documents_factory.gen_test({
'_SITE_ACTIONS_1_': {
'actions': [{'method': 'merge', 'path': '.'}]
}
})
expected_names = ['foo', 'baz', 'bar']
expected_schemas = ['deckhand/Certificate/v1',
'deckhand/Certificate/v1',
@ -158,7 +166,11 @@ class TestRevisionDocumentsControllerSorting(test_base.BaseControllerTest):
self.policy.set_rules(rules)
documents_factory = factories.DocumentFactory(2, [1, 1])
documents = documents_factory.gen_test({})
documents = documents_factory.gen_test({
'_SITE_ACTIONS_1_': {
'actions': [{'method': 'merge', 'path': '.'}]
}
})
expected_schemas = ['deckhand/Certificate/v1',
'deckhand/CertificateKey/v1',
'deckhand/LayeringPolicy/v1']

34
deckhand/tests/unit/control/test_validations_controller.py
View File

@ -28,7 +28,7 @@ from deckhand import types
CONF = cfg.CONF
VALIDATION_RESULT = """
VALIDATION_FAILURE_RESULT = """
---
status: failure
errors:
@ -43,7 +43,7 @@ validator:
version: 1.1.2
"""
VALIDATION_RESULT_ALT = """
VALIDATION_SUCCESS_RESULT = """
---
status: success
errors: []
@ -114,7 +114,7 @@ class TestValidationsControllerPostValidate(ValidationsControllerBaseTest):
revision_id = self._create_revision()
validation_name = test_utils.rand_name('validation')
resp = self._create_validation(revision_id, validation_name,
VALIDATION_RESULT)
VALIDATION_FAILURE_RESULT)
self.assertEqual(201, resp.status_code)
expected_body = {
@ -161,7 +161,7 @@ class TestValidationsControllerPostValidate(ValidationsControllerBaseTest):
# service, it is listed as well.
validation_name = test_utils.rand_name('validation')
resp = self._create_validation(revision_id, validation_name,
VALIDATION_RESULT)
VALIDATION_FAILURE_RESULT)
resp = self.app.simulate_get(
'/api/v1.0/revisions/%s/validations' % revision_id,
@ -211,7 +211,7 @@ class TestValidationsControllerPostValidate(ValidationsControllerBaseTest):
# Add the result of a validation to a revision.
validation_name = test_utils.rand_name('validation')
resp = self._create_validation(revision_id, validation_name,
VALIDATION_RESULT)
VALIDATION_FAILURE_RESULT)
# Validate that the entry is present.
resp = self.app.simulate_get(
@ -238,7 +238,7 @@ class TestValidationsControllerPostValidate(ValidationsControllerBaseTest):
# Add the result of a validation to a revision.
validation_name = test_utils.rand_name('validation')
resp = self._create_validation(revision_id, validation_name,
VALIDATION_RESULT)
VALIDATION_FAILURE_RESULT)
# Validate that the entry is present.
resp = self.app.simulate_get(
@ -256,7 +256,7 @@ class TestValidationsControllerPostValidate(ValidationsControllerBaseTest):
# Add the result of another validation to the same revision.
resp = self._create_validation(revision_id, validation_name,
VALIDATION_RESULT_ALT)
VALIDATION_SUCCESS_RESULT)
# Validate that 2 entries now exist.
resp = self.app.simulate_get(
@ -282,9 +282,9 @@ class TestValidationsControllerPostValidate(ValidationsControllerBaseTest):
revision_id = self._create_revision()
validation_name = test_utils.rand_name('validation')
self._create_validation(revision_id, validation_name,
VALIDATION_RESULT)
VALIDATION_FAILURE_RESULT)
self._create_validation(revision_id, validation_name,
VALIDATION_RESULT_ALT)
VALIDATION_SUCCESS_RESULT)
resp = self.app.simulate_get(
'/api/v1.0/revisions/%s/validations/%s' % (revision_id,
@ -310,7 +310,7 @@ class TestValidationsControllerPostValidate(ValidationsControllerBaseTest):
revision_id = self._create_revision()
validation_name = test_utils.rand_name('validation')
resp = self._create_validation(revision_id, validation_name,
VALIDATION_RESULT)
VALIDATION_FAILURE_RESULT)
resp = self.app.simulate_get(
'/api/v1.0/revisions/%s/validations/%s/entries/0' % (revision_id,
@ -351,7 +351,7 @@ class TestValidationsControllerPostValidate(ValidationsControllerBaseTest):
revision_id = self._create_revision()
validation_name = test_utils.rand_name('validation')
resp = self._create_validation(revision_id, validation_name,
VALIDATION_RESULT)
VALIDATION_FAILURE_RESULT)
self.assertEqual(201, resp.status_code)
expected_error = ('The requested validation entry 5 was not found for '
'validation name %s and revision ID %d.' % (
@ -593,7 +593,6 @@ class TestValidationsControllerPostValidate(ValidationsControllerBaseTest):
'data': {'a': 'fail'},
'metadata': {'labels': {'global': 'global1'},
'layeringDefinition': {'abstract': False,
'actions': [],
'layer': 'global'},
'name': doc_to_test['metadata']['name'],
'schema': doc_to_test['metadata']['schema']},
@ -891,7 +890,8 @@ data:
# Create the external validation for "promenade-schema-validation".
resp = self._create_validation(
revision_id, 'promenade-schema-validation', VALIDATION_RESULT_ALT)
revision_id, 'promenade-schema-validation',
VALIDATION_SUCCESS_RESULT)
self.assertEqual(201, resp.status_code)
# Validate that the validation was created and reports success.
@ -949,7 +949,8 @@ data:
# Create the external validation for "promenade-schema-validation".
resp = self._create_validation(
revision_id, 'promenade-schema-validation', VALIDATION_RESULT_ALT)
revision_id, 'promenade-schema-validation',
VALIDATION_SUCCESS_RESULT)
self.assertEqual(201, resp.status_code)
# Validate that the validation was created and reports success.
@ -1077,7 +1078,8 @@ data:
# Register an extra validation not in the ValidationPolicy.
resp = self._create_validation(
revision_id, 'promenade-schema-validation', VALIDATION_RESULT)
revision_id, 'promenade-schema-validation',
VALIDATION_FAILURE_RESULT)
self.assertEqual(201, resp.status_code)
# Validate that the extra validation is ignored.
@ -1126,7 +1128,7 @@ data:
'%s: %s.' % (validation_policy['schema'],
validation_policy['metadata']['name'],
types.DECKHAND_SCHEMA_VALIDATION))
expected_errors = yaml.safe_load(VALIDATION_RESULT)['errors']
expected_errors = yaml.safe_load(VALIDATION_FAILURE_RESULT)['errors']
expected_errors.append({'message': expected_msg})
expected_body = {

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

@ -287,6 +287,31 @@ data:
documents, site_expected, region_expected, global_expected,
substitution_sources=[documents[1]])
@mock.patch.object(layering, 'LOG', autospec=True)
def test_layering_document_with_parent_but_no_actions_skips_layering(
self, mock_log):
"""Verify that a document that has a parent but no layering actions
skips over layering.
"""
doc_factory = factories.DocumentFactory(2, [1, 1])
documents = doc_factory.gen_test(
{'_SITE_DATA_1_': {'data': 'should not change'}},
site_abstract=False)
documents[-1]['metadata']['layeringDefinition']['actions'] = []
self.assertEqual(
documents[1]['metadata']['labels'],
documents[2]['metadata']['layeringDefinition']['parentSelector'])
site_expected = 'should not change'
self._test_layering(documents, site_expected, global_expected=None)
mock_log.info.assert_called_once_with(
'Skipped layering for document [%s] %s which has a parent [%s] '
'%s, but no associated layering actions.', documents[2]['schema'],
documents[2]['metadata']['name'], documents[1]['schema'],
documents[1]['metadata']['name'])
class TestDocumentLayering2Layers(TestDocumentLayering):

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

@ -102,7 +102,7 @@ class TestDocumentLayeringNegative(
documents[-1]['metadata']['layeringDefinition'][
'parentSelector'] = parent_selector
layering.DocumentLayering(documents)
layering.DocumentLayering(documents, validate=False)
self.assertTrue(any('Could not find parent for document' in
mock_log.debug.mock_calls[x][1][0])
for x in range(len(mock_log.debug.mock_calls)))
@ -117,7 +117,7 @@ class TestDocumentLayeringNegative(
# Second doc is the global doc, or parent.
documents[1]['metadata']['labels'] = parent_label
layering.DocumentLayering(documents)
layering.DocumentLayering(documents, validate=False)
self.assertTrue(any('Could not find parent for document' in
mock_log.debug.mock_calls[x][1][0])
for x in range(len(mock_log.debug.mock_calls)))
@ -132,7 +132,7 @@ class TestDocumentLayeringNegative(
documents[2]['metadata']['labels'] = documents[1]['metadata']['labels']
self.assertRaises(errors.IndeterminateDocumentParent,
layering.DocumentLayering, documents)
layering.DocumentLayering, documents, validate=False)
def test_layering_duplicate_parent_selector_3_layer(self):
# Validate that documents belonging to the same layer cannot have the
@ -143,7 +143,7 @@ class TestDocumentLayeringNegative(
documents[3]['metadata']['labels'] = documents[2]['metadata']['labels']
self.assertRaises(errors.IndeterminateDocumentParent,
layering.DocumentLayering, documents)
layering.DocumentLayering, documents, validate=False)
@mock.patch.object(layering, 'LOG', autospec=True)
def test_layering_document_references_itself(self, mock_log):
@ -156,7 +156,7 @@ class TestDocumentLayeringNegative(
documents[2]['metadata']['layeringDefinition'][
'parentSelector'] = self_ref
layering.DocumentLayering(documents)
layering.DocumentLayering(documents, validate=False)
self.assertTrue(any('Could not find parent for document' in
mock_log.debug.mock_calls[x][1][0])
for x in range(len(mock_log.debug.mock_calls)))

27
deckhand/tests/unit/engine/test_document_validation.py
View File

@ -40,8 +40,6 @@ class TestDocumentValidation(engine_test_base.TestDocumentValidationBase):
def test_document_missing_optional_sections(self):
properties_to_remove = (
'metadata.layeringDefinition.actions',
'metadata.layeringDefinition.parentSelector',
'metadata.substitutions',
'metadata.substitutions.2.dest.pattern')
@ -156,3 +154,28 @@ class TestDocumentValidation(engine_test_base.TestDocumentValidationBase):
self.assertEqual(1, len(validations[0]['errors']))
self.assertEqual('Sanitized to avoid exposing secret.',
validations[0]['errors'][0]['message'])
def test_parent_selector_and_actions_both_provided_is_valid(self):
test_document = self._read_data('sample_document')
data_schema_factory = factories.DataSchemaFactory()
data_schema = data_schema_factory.gen_test(test_document['schema'], {})
validations = document_validation.DocumentValidation(
test_document, existing_data_schemas=[data_schema],
pre_validate=False).validate_all()
self.assertEmpty(validations[0]['errors'])
def test_neither_parent_selector_nor_actions_provided_is_valid(self):
test_document = self._read_data('sample_document')
test_document['metadata']['layeringDefinition'].pop('actions')
test_document['metadata']['layeringDefinition'].pop('parentSelector')
data_schema_factory = factories.DataSchemaFactory()
data_schema = data_schema_factory.gen_test(test_document['schema'], {})
validations = document_validation.DocumentValidation(
test_document, existing_data_schemas=[data_schema],
pre_validate=False).validate_all()
self.assertEmpty(validations[0]['errors'])

39
deckhand/tests/unit/engine/test_document_validation_negative.py
View File

@ -243,3 +243,42 @@ class TestDocumentValidationNegative(test_base.TestDocumentValidationBase):
[document, data_schema], pre_validate=False)
with self.assertRaisesRegexp(RuntimeError, 'Unknown error'):
doc_validator.validate_all()
def test_parent_selector_but_no_actions_raises_validation_error(self):
# Verify that an error is thrown if parentSelector is specified but
# actions is missing altogether.
document = self._read_data('sample_document')
document['metadata']['layeringDefinition']['parentSelector'] = {
'some': 'label'
}
document['metadata']['layeringDefinition'].pop('actions')
doc_validator = document_validation.DocumentValidation(
[document], pre_validate=False)
self.assertRaises(
errors.InvalidDocumentFormat, doc_validator.validate_all)
# Verify that an error is thrown if parentSelector is specified but
# at least 1 action isn't specified.
document['metadata']['layeringDefinition']['actions'] = []
doc_validator = document_validation.DocumentValidation(
[document], pre_validate=False)
self.assertRaises(
errors.InvalidDocumentFormat, doc_validator.validate_all)
def test_actions_but_no_parent_selector_raises_validation_error(self):
# Verify that an error is thrown if actions are specified but
# parentSelector is missing altogether.
document = self._read_data('sample_document')
document['metadata']['layeringDefinition'].pop('parentSelector')
doc_validator = document_validation.DocumentValidation(
[document], pre_validate=False)
self.assertRaises(
errors.InvalidDocumentFormat, doc_validator.validate_all)
# Verify that an error is thrown if actions are specified but no
# parentSelector labels are.
document['metadata']['layeringDefinition']['parentSelector'] = {}
doc_validator = document_validation.DocumentValidation(
[document], pre_validate=False)
self.assertRaises(
errors.InvalidDocumentFormat, doc_validator.validate_all)

2
deckhand/tests/unit/resources/sample_document.yaml
View File

@ -18,6 +18,8 @@ metadata:
path: .path.to.merge.into.parent
- method: delete
path: .path.to.delete
parentSelector:
some: label
substitutions:
- dest:
path: .chart.values.tls.certificate