From 4b70927bb226d1f9a489b6a7435db850f0342ac0 Mon Sep 17 00:00:00 2001 From: Felipe Monteiro Date: Wed, 3 Jan 2018 19:44:20 +0000 Subject: [PATCH] Fix: Allow generic documents to be used as substitution sources. This PS fixes a bug related to Deckhand only using "secret" document types to be used as substitution sources; the substitution logic should be made generic, because it shouldn't just apply to secrets. This entailed removing the "is_secret" database column from the Document table as it's no longer needed and dropping it from a DB query made to find the source document for substitution in the secrets_manager module. This PS also increased resiliency via exception handling and some edge cases surrounding substitution. Finally, unit tests and functional tests were added to validate substitition using a generic document as the source. Change-Id: I2c4b49b2eb55473c56b8253a456803e793b0b0b0 --- deckhand/control/revision_documents.py | 3 +- deckhand/db/sqlalchemy/api.py | 2 - deckhand/db/sqlalchemy/models.py | 1 - deckhand/engine/document.py | 11 ++-- deckhand/engine/secrets_manager.py | 19 +++++-- deckhand/errors.py | 53 ++++++++++-------- ...ngle-bucket-with-substitution-generic.yaml | 37 +++++++++++++ ...esign-doc-substitution-generic-sample.yaml | 54 +++++++++++++++++++ deckhand/tests/unit/db/test_documents.py | 3 -- ...test_document_layering_and_substitution.py | 6 +-- .../tests/unit/engine/test_secrets_manager.py | 41 ++++++++++++++ deckhand/utils.py | 12 +++-- doc/source/substitution.rst | 21 ++++++-- 13 files changed, 212 insertions(+), 51 deletions(-) create mode 100644 deckhand/tests/functional/gabbits/document-render-success-single-bucket-with-substitution-generic.yaml create mode 100644 deckhand/tests/functional/gabbits/resources/design-doc-substitution-generic-sample.yaml diff --git a/deckhand/control/revision_documents.py b/deckhand/control/revision_documents.py index fe945986..28fb7de8 100644 --- a/deckhand/control/revision_documents.py +++ b/deckhand/control/revision_documents.py @@ -115,7 +115,8 @@ class RenderedDocumentsResource(api_base.BaseResource): errors.UnsupportedActionMethod, errors.MissingDocumentKey) as e: raise falcon.HTTPBadRequest(description=e.format_message()) - except errors.LayeringPolicyNotFound as e: + except (errors.LayeringPolicyNotFound, + errors.SubstitutionDependencyNotFound) as e: raise falcon.HTTPConflict(description=e.format_message()) # Filters to be applied post-rendering, because many documents are diff --git a/deckhand/db/sqlalchemy/api.py b/deckhand/db/sqlalchemy/api.py index f8cd31c6..5256bafe 100644 --- a/deckhand/db/sqlalchemy/api.py +++ b/deckhand/db/sqlalchemy/api.py @@ -246,8 +246,6 @@ def _documents_create(bucket_name, values_list, session=None): for values in values_list: values.setdefault('data', {}) values = _fill_in_metadata_defaults(values) - values['is_secret'] = values['schema'].startswith( - types.DOCUMENT_SECRET_TYPES) # Hash the document's metadata and data to later efficiently check # whether those data have changed. diff --git a/deckhand/db/sqlalchemy/models.py b/deckhand/db/sqlalchemy/models.py index 16f05752..00d7c172 100644 --- a/deckhand/db/sqlalchemy/models.py +++ b/deckhand/db/sqlalchemy/models.py @@ -135,7 +135,6 @@ class Document(BASE, DeckhandBase): data = Column(JSONB, nullable=True) data_hash = Column(String, nullable=False) metadata_hash = Column(String, nullable=False) - is_secret = Column(Boolean, nullable=False, default=False) bucket_id = Column(Integer, ForeignKey('buckets.id', ondelete='CASCADE'), nullable=False) revision_id = Column( diff --git a/deckhand/engine/document.py b/deckhand/engine/document.py index d04f7783..ece9614b 100644 --- a/deckhand/engine/document.py +++ b/deckhand/engine/document.py @@ -12,8 +12,6 @@ # See the License for the specific language governing permissions and # limitations under the License. -import six - class Document(object): """Object wrapper for documents. @@ -40,9 +38,7 @@ class Document(object): concrete. """ try: - abstract = self._inner['metadata']['layeringDefinition'][ - 'abstract'] - return six.text_type(abstract) == 'True' + return self._inner['metadata']['layeringDefinition']['abstract'] except KeyError: return False @@ -53,7 +49,10 @@ class Document(object): return self._inner['metadata']['name'] def get_layer(self): - return self._inner['metadata']['layeringDefinition']['layer'] + try: + return self._inner['metadata']['layeringDefinition']['layer'] + except Exception: + return None def get_parent_selector(self): """Return the `parentSelector` for the document. diff --git a/deckhand/engine/secrets_manager.py b/deckhand/engine/secrets_manager.py index e412c8c9..ecd19fe6 100644 --- a/deckhand/engine/secrets_manager.py +++ b/deckhand/engine/secrets_manager.py @@ -13,10 +13,12 @@ # limitations under the License. from oslo_log import log as logging +import six from deckhand.barbican import driver from deckhand.db.sqlalchemy import api as db_api from deckhand.engine import document as document_wrapper +from deckhand import errors from deckhand import utils LOG = logging.getLogger(__name__) @@ -141,7 +143,7 @@ class SecretsSubstitution(object): # TODO(fmontei): Use SecretsManager for this logic. Need to # check Barbican for the secret if it has been encrypted. src_doc = db_api.document_get( - schema=src_schema, name=src_name, is_secret=True, + schema=src_schema, name=src_name, **{'metadata.layeringDefinition.abstract': False}) # If the data is a dictionary, retrieve the nested secret @@ -159,9 +161,18 @@ class SecretsSubstitution(object): LOG.debug('Substituting from schema=%s name=%s src_path=%s ' 'into dest_path=%s, dest_pattern=%s', src_schema, src_name, src_path, dest_path, dest_pattern) - substituted_data = utils.jsonpath_replace( - doc['data'], src_secret, dest_path, dest_pattern) - doc['data'].update(substituted_data) + try: + substituted_data = utils.jsonpath_replace( + doc['data'], src_secret, dest_path, dest_pattern) + if isinstance(substituted_data, dict): + doc['data'].update(substituted_data) + else: + doc['data'] = substituted_data + except Exception as e: + LOG.error('Unexpected exception occurred while attempting ' + 'secret substitution. %s', six.text_type(e)) + raise errors.SubstitutionDependencyNotFound( + details=six.text_type(e)) substituted_docs.append(doc.to_dict()) return substituted_docs diff --git a/deckhand/errors.py b/deckhand/errors.py index afa50fc8..2fea8c14 100644 --- a/deckhand/errors.py +++ b/deckhand/errors.py @@ -182,20 +182,6 @@ class InvalidDocumentSchema(DeckhandException): code = 400 -class DocumentExists(DeckhandException): - msg_fmt = ("Document with schema %(schema)s and metadata.name " - "%(name)s already exists in bucket %(bucket)s.") - code = 409 - - -class SingletonDocumentConflict(DeckhandException): - msg_fmt = ("A singleton document by the name %(document)s already " - "exists in the system. The new document %(conflict)s cannot be " - "created. To create a document with a new name, delete the " - "current one first.") - code = 409 - - class IndeterminateDocumentParent(DeckhandException): msg_fmt = ("Too many parent documents found for document %(document)s.") code = 400 @@ -204,6 +190,7 @@ class IndeterminateDocumentParent(DeckhandException): class MissingDocumentKey(DeckhandException): msg_fmt = ("Missing document key %(key)s from either parent or child. " "Parent: %(parent)s. Child: %(child)s.") + code = 400 class UnsupportedActionMethod(DeckhandException): @@ -211,6 +198,12 @@ class UnsupportedActionMethod(DeckhandException): code = 400 +class RevisionTagBadFormat(DeckhandException): + msg_fmt = ("The requested tag data %(data)s must either be null or " + "dictionary.") + code = 400 + + class DocumentNotFound(DeckhandException): msg_fmt = ("The requested document %(document)s was not found.") code = 404 @@ -227,11 +220,6 @@ class RevisionTagNotFound(DeckhandException): code = 404 -class LayeringPolicyNotFound(DeckhandException): - msg_fmt = ("Required LayeringPolicy was not found for layering.") - code = 409 - - class ValidationNotFound(DeckhandException): msg_fmt = ("The requested validation entry %(entry_id)s was not found " "for validation name %(validation_name)s and revision ID " @@ -239,10 +227,29 @@ class ValidationNotFound(DeckhandException): code = 404 -class RevisionTagBadFormat(DeckhandException): - msg_fmt = ("The requested tag data %(data)s must either be null or " - "dictionary.") - code = 400 +class DocumentExists(DeckhandException): + msg_fmt = ("Document with schema %(schema)s and metadata.name " + "%(name)s already exists in bucket %(bucket)s.") + code = 409 + + +class SingletonDocumentConflict(DeckhandException): + msg_fmt = ("A singleton document by the name %(document)s already " + "exists in the system. The new document %(conflict)s cannot be " + "created. To create a document with a new name, delete the " + "current one first.") + code = 409 + + +class LayeringPolicyNotFound(DeckhandException): + msg_fmt = ("Required LayeringPolicy was not found for layering.") + code = 409 + + +class SubstitutionDependencyNotFound(DeckhandException): + msg_fmt = ('Failed to find a dependent source document required for ' + 'substitution. Details: %(details)s') + code = 409 class BarbicanException(DeckhandException): diff --git a/deckhand/tests/functional/gabbits/document-render-success-single-bucket-with-substitution-generic.yaml b/deckhand/tests/functional/gabbits/document-render-success-single-bucket-with-substitution-generic.yaml new file mode 100644 index 00000000..45378737 --- /dev/null +++ b/deckhand/tests/functional/gabbits/document-render-success-single-bucket-with-substitution-generic.yaml @@ -0,0 +1,37 @@ +# Tests success path for using a generic document as a substitution source. +# In this case, the DataSchema-registered document unusual/DictWithSecret/v1 +# is used as the source. +# +# 1. Purges existing data to ensure test isolation +# 2. Adds initial documents from substitution sample of design doc +# 3. Verifies fully substituted document data + +defaults: + request_headers: + content-type: application/x-yaml + response_headers: + content-type: application/x-yaml + +tests: + - name: purge + desc: Begin testing from known state. + DELETE: /api/v1.0/revisions + status: 204 + response_headers: null + + - name: initialize + desc: Create initial documents + PUT: /api/v1.0/buckets/mop/documents + status: 200 + data: <@resources/design-doc-substitution-generic-sample.yaml + + - name: verify_substitutions + desc: Check for expected substitutions + GET: /api/v1.0/revisions/$RESPONSE['$.[0].status.revision']/rendered-documents + query_parameters: + schema: armada/Chart/v1 + status: 200 + response_multidoc_jsonpaths: + $.`len`: 1 + $.[0].metadata.name: example-chart-01 + $.[0].data: secret-from-generic-document diff --git a/deckhand/tests/functional/gabbits/resources/design-doc-substitution-generic-sample.yaml b/deckhand/tests/functional/gabbits/resources/design-doc-substitution-generic-sample.yaml new file mode 100644 index 00000000..6ea6b25f --- /dev/null +++ b/deckhand/tests/functional/gabbits/resources/design-doc-substitution-generic-sample.yaml @@ -0,0 +1,54 @@ +--- +schema: deckhand/LayeringPolicy/v1 +metadata: + schema: metadata/Control/v1 + name: layering-policy +data: + layerOrder: + - region + - site +--- +schema: deckhand/DataSchema/v1 +metadata: + schema: metadata/Control/v1 + name: unusual/DictWithSecret/v1 +data: + $schema: http://json-schema.org/schema# + type: object + properties: + secret: + type: string + public: + type: string + additionalProperties: false + required: + - secret + - public +--- +schema: unusual/DictWithSecret/v1 +metadata: + schema: metadata/Document/v1 + name: dict-with-secret + storagePolicy: cleartext + layeringDefinition: + abstract: false + layer: site +data: + secret: secret-from-generic-document + public: random +--- +schema: armada/Chart/v1 +metadata: + name: example-chart-01 + schema: metadata/Document/v1 + layeringDefinition: + layer: region + substitutions: + - dest: + path: . + src: + schema: unusual/DictWithSecret/v1 + name: dict-with-secret + path: .secret +data: need secret from unusual/DictWithSecret/v1 +... diff --git a/deckhand/tests/unit/db/test_documents.py b/deckhand/tests/unit/db/test_documents.py index bc582b41..e3cdff6f 100644 --- a/deckhand/tests/unit/db/test_documents.py +++ b/deckhand/tests/unit/db/test_documents.py @@ -158,7 +158,6 @@ class TestDocuments(base.TestDbBase): self.assertIn('Certificate', created_documents[-1]['schema']) self.assertEqual(storage_policy, created_documents[-1][ 'metadata']['storagePolicy']) - self.assertTrue(created_documents[-1]['is_secret']) self.assertEqual(rand_secret, created_documents[-1]['data']) def test_create_certificate_key(self): @@ -176,7 +175,6 @@ class TestDocuments(base.TestDbBase): self.assertIn('CertificateKey', created_documents[-1]['schema']) self.assertEqual(storage_policy, created_documents[-1][ 'metadata']['storagePolicy']) - self.assertTrue(created_documents[-1]['is_secret']) self.assertEqual(rand_secret, created_documents[-1]['data']) def test_create_passphrase(self): @@ -194,7 +192,6 @@ class TestDocuments(base.TestDbBase): self.assertIn('Passphrase', created_documents[-1]['schema']) self.assertEqual(storage_policy, created_documents[-1][ 'metadata']['storagePolicy']) - self.assertTrue(created_documents[-1]['is_secret']) self.assertEqual(rand_secret, created_documents[-1]['data']) def test_delete_document(self): diff --git a/deckhand/tests/unit/engine/test_document_layering_and_substitution.py b/deckhand/tests/unit/engine/test_document_layering_and_substitution.py index 799e3817..e9547c5f 100644 --- a/deckhand/tests/unit/engine/test_document_layering_and_substitution.py +++ b/deckhand/tests/unit/engine/test_document_layering_and_substitution.py @@ -58,7 +58,7 @@ class TestDocumentLayeringWithSubstitution( global_expected=global_expected) mock_document_get.assert_called_once_with( schema=certificate['schema'], name=certificate['metadata']['name'], - is_secret=True, **{'metadata.layeringDefinition.abstract': False}) + **{'metadata.layeringDefinition.abstract': False}) def test_layering_and_substitution_no_children(self): mapping = { @@ -97,7 +97,7 @@ class TestDocumentLayeringWithSubstitution( global_expected=global_expected) mock_document_get.assert_called_once_with( schema=certificate['schema'], name=certificate['metadata']['name'], - is_secret=True, **{'metadata.layeringDefinition.abstract': False}) + **{'metadata.layeringDefinition.abstract': False}) def test_layering_parent_and_child_undergo_substitution(self): mapping = { @@ -152,10 +152,8 @@ class TestDocumentLayeringWithSubstitution( mock_document_get.assert_has_calls([ mock.call( schema="deckhand/Certificate/v1", name='global-cert', - is_secret=True, **{'metadata.layeringDefinition.abstract': False}), mock.call( schema="deckhand/CertificateKey/v1", name='site-cert', - is_secret=True, **{'metadata.layeringDefinition.abstract': False}) ]) diff --git a/deckhand/tests/unit/engine/test_secrets_manager.py b/deckhand/tests/unit/engine/test_secrets_manager.py index a493df16..527ff5e3 100644 --- a/deckhand/tests/unit/engine/test_secrets_manager.py +++ b/deckhand/tests/unit/engine/test_secrets_manager.py @@ -282,3 +282,44 @@ class TestSecretsSubstitution(test_base.TestDbBase): self._test_secret_substitution( document_mapping, [certificate, certificate_key, passphrase], expected_data) + + def test_substitution_with_generic_document_as_source(self): + src_data = 'data-from-generic-document' + + # Create DataSchema document to register generic source document. + dataschema_factory = factories.DataSchemaFactory() + dataschema = dataschema_factory.gen_test( + 'unusual/DictWithSecret/v1', {}) + # Create the generic source document from which data will be extracted. + generic_document_mapping = { + "_GLOBAL_DATA_1_": { + 'data': {'public': 'random', 'money': src_data} + } + } + payload = self.document_factory.gen_test(generic_document_mapping, + global_abstract=False) + payload[-1]['schema'] = "unusual/DictWithSecret/v1" + payload[-1]['metadata']['name'] = 'dict-with-secret' + + # Store both documents to be created by helper. + dependent_documents = [payload[-1], dataschema] + + # Mapping for destination document. + document_mapping = { + "_GLOBAL_DATA_1_": { + 'data': {} + }, + "_GLOBAL_SUBSTITUTIONS_1_": [{ + "dest": { + "path": "." + }, + "src": { + "schema": "unusual/DictWithSecret/v1", + "name": "dict-with-secret", + "path": ".money" + } + + }] + } + self._test_secret_substitution( + document_mapping, dependent_documents, expected_data=src_data) diff --git a/deckhand/utils.py b/deckhand/utils.py index d202df16..b582963e 100644 --- a/deckhand/utils.py +++ b/deckhand/utils.py @@ -13,6 +13,7 @@ # limitations under the License. import ast +import copy import re import string @@ -62,7 +63,9 @@ def jsonpath_parse(data, jsonpath, match_all=False): src_secret = utils.jsonpath_parse(src_doc['data'], src_path) # Do something with the extracted secret from the source document. """ - if jsonpath.startswith('.'): + if jsonpath == '.': + jsonpath = '$' + elif jsonpath.startswith('.'): jsonpath = '$' + jsonpath p = jsonpath_ng.parse(jsonpath) @@ -104,8 +107,11 @@ def jsonpath_replace(data, value, jsonpath, pattern=None): # http://admin:super-duper-secret@svc-name:8080/v1 doc['data'].update(replaced_data) """ - data = data.copy() - if jsonpath.startswith('.'): + data = copy.copy(data) + + if jsonpath == '.': + jsonpath = '$' + elif jsonpath.startswith('.'): jsonpath = '$' + jsonpath def _do_replace(): diff --git a/doc/source/substitution.rst b/doc/source/substitution.rst index e6d87954..600fa48c 100644 --- a/doc/source/substitution.rst +++ b/doc/source/substitution.rst @@ -16,13 +16,23 @@ .. _substitution: -Secret Substitution -=================== +Document Substitution +===================== + +Document substitution, simply put, allows one document to overwrite *parts* of +its own data with that of another document. Substitution involves a source +document sharing data with a destination document, which replaces its own data +with the shared data. Substitution is primarily designed as a mechanism for inserting secrets into configuration documents, but works for unencrypted source documents as well. -Substitution is applied at each layer after all merge actions occur. Further, -substitution is only applied to the ``data`` section of a document. +Substitution is applied at each layer after all merge actions occur. + +.. note:: + + Substitution is only applied to the ``data`` section of a document. This is + because a document's ``metadata`` and ``schema`` sections should be + immutable within the scope of a revision, for obvious reasons. Concrete (non-abstract) documents can be used as a source of substitution into other documents. This substitution is layer-independent, so given the 3 @@ -138,3 +148,6 @@ The rendered document will look like: key: | KEY DATA ... + +This substitution is also ``schema`` agnostic, meaning that source and +destination documents can have a different ``schema``.