From 5c411dd05bc4b9982b5ab929b5bab79742764825 Mon Sep 17 00:00:00 2001 From: Felipe Monteiro Date: Mon, 5 Mar 2018 13:30:12 -0500 Subject: [PATCH] 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 --- deckhand/engine/layering.py | 34 +++++++++------- deckhand/engine/schemas/base_schema.yaml | 23 +++++++++++ deckhand/factories.py | 6 +-- .../unit/control/test_buckets_controller.py | 6 ++- .../test_rendered_documents_controller.py | 21 +++++++--- .../test_revision_documents_controller.py | 18 +++++++-- .../control/test_validations_controller.py | 34 ++++++++-------- .../unit/engine/test_document_layering.py | 25 ++++++++++++ .../engine/test_document_layering_negative.py | 10 ++--- .../unit/engine/test_document_validation.py | 27 ++++++++++++- .../test_document_validation_negative.py | 39 +++++++++++++++++++ .../tests/unit/resources/sample_document.yaml | 2 + 12 files changed, 193 insertions(+), 52 deletions(-) diff --git a/deckhand/engine/layering.py b/deckhand/engine/layering.py index ed3c521c..e4452413 100644 --- a/deckhand/engine/layering.py +++ b/deckhand/engine/layering.py @@ -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. diff --git a/deckhand/engine/schemas/base_schema.yaml b/deckhand/engine/schemas/base_schema.yaml index 9066bb6b..8c3071c1 100644 --- a/deckhand/engine/schemas/base_schema.yaml +++ b/deckhand/engine/schemas/base_schema.yaml @@ -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 diff --git a/deckhand/factories.py b/deckhand/factories.py index b231660d..86a6d39f 100644 --- a/deckhand/factories.py +++ b/deckhand/factories.py @@ -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) diff --git a/deckhand/tests/unit/control/test_buckets_controller.py b/deckhand/tests/unit/control/test_buckets_controller.py index 377663d2..1422ace6 100644 --- a/deckhand/tests/unit/control/test_buckets_controller.py +++ b/deckhand/tests/unit/control/test_buckets_controller.py @@ -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') diff --git a/deckhand/tests/unit/control/test_rendered_documents_controller.py b/deckhand/tests/unit/control/test_rendered_documents_controller.py index dd9b28c2..29581d34 100644 --- a/deckhand/tests/unit/control/test_rendered_documents_controller.py +++ b/deckhand/tests/unit/control/test_rendered_documents_controller.py @@ -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] diff --git a/deckhand/tests/unit/control/test_revision_documents_controller.py b/deckhand/tests/unit/control/test_revision_documents_controller.py index c89c780c..9302b7ac 100644 --- a/deckhand/tests/unit/control/test_revision_documents_controller.py +++ b/deckhand/tests/unit/control/test_revision_documents_controller.py @@ -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'] diff --git a/deckhand/tests/unit/control/test_validations_controller.py b/deckhand/tests/unit/control/test_validations_controller.py index 5b3b0b1c..91569a0e 100644 --- a/deckhand/tests/unit/control/test_validations_controller.py +++ b/deckhand/tests/unit/control/test_validations_controller.py @@ -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 = { diff --git a/deckhand/tests/unit/engine/test_document_layering.py b/deckhand/tests/unit/engine/test_document_layering.py index 2e9ccd36..6effbccf 100644 --- a/deckhand/tests/unit/engine/test_document_layering.py +++ b/deckhand/tests/unit/engine/test_document_layering.py @@ -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): diff --git a/deckhand/tests/unit/engine/test_document_layering_negative.py b/deckhand/tests/unit/engine/test_document_layering_negative.py index aca187f5..5728767f 100644 --- a/deckhand/tests/unit/engine/test_document_layering_negative.py +++ b/deckhand/tests/unit/engine/test_document_layering_negative.py @@ -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))) diff --git a/deckhand/tests/unit/engine/test_document_validation.py b/deckhand/tests/unit/engine/test_document_validation.py index a18d9236..e34be4f5 100644 --- a/deckhand/tests/unit/engine/test_document_validation.py +++ b/deckhand/tests/unit/engine/test_document_validation.py @@ -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']) diff --git a/deckhand/tests/unit/engine/test_document_validation_negative.py b/deckhand/tests/unit/engine/test_document_validation_negative.py index 9c927dc3..188533f9 100644 --- a/deckhand/tests/unit/engine/test_document_validation_negative.py +++ b/deckhand/tests/unit/engine/test_document_validation_negative.py @@ -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) diff --git a/deckhand/tests/unit/resources/sample_document.yaml b/deckhand/tests/unit/resources/sample_document.yaml index 97a0294d..42ac33b4 100644 --- a/deckhand/tests/unit/resources/sample_document.yaml +++ b/deckhand/tests/unit/resources/sample_document.yaml @@ -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