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