From 59836a0e64babf519edc9880bf68de1b80659ec0 Mon Sep 17 00:00:00 2001 From: Felipe Monteiro Date: Tue, 4 Sep 2018 21:28:50 +0100 Subject: [PATCH] [fix] Substitution source documents accidentally modified This resolves a regression introduced in [0] which was causing substitution source documents to become modified while performing substitution into destination documents. A unit test has been included to prevent the regression. [0] https://github.com/openstack/airship-deckhand/commit/1583b7890294532c89213f66a61153df6f9a1634 Change-Id: I737b8688cfea54dc6b14d8ca243fb43f8363dcaf --- deckhand/common/utils.py | 20 +- ...test_document_layering_and_substitution.py | 251 ++++++++++++++++++ 2 files changed, 264 insertions(+), 7 deletions(-) diff --git a/deckhand/common/utils.py b/deckhand/common/utils.py index dab9f80d..c0bdf3a3 100644 --- a/deckhand/common/utils.py +++ b/deckhand/common/utils.py @@ -13,6 +13,7 @@ # limitations under the License. import ast +import copy import re import string @@ -170,6 +171,11 @@ def jsonpath_replace(data, value, jsonpath, pattern=None): doc['data'].update(replaced_data) """ + # These are O(1) reference copies to avoid accidentally modifying source + # data. We only want to update destination data. + data_copy = copy.copy(data) + value_copy = copy.copy(value) + jsonpath = _normalize_jsonpath(jsonpath) if not jsonpath == '$' and not jsonpath.startswith('$.'): @@ -180,7 +186,7 @@ def jsonpath_replace(data, value, jsonpath, pattern=None): def _execute_replace(path, path_to_change): if path_to_change: - new_value = value + new_value = value_copy if pattern: to_replace = path_to_change[0].value # `new_value` represents the value to inject into `to_replace` @@ -192,23 +198,23 @@ def jsonpath_replace(data, value, jsonpath, pattern=None): # Raise an exception in case the path isn't present in the # data and a pattern has been provided since it is # otherwise impossible to do the look-up. - new_value = re.sub(pattern, str(value), to_replace) + new_value = re.sub(pattern, str(value_copy), to_replace) except TypeError as e: LOG.error('Failed to substitute the value %s into %s ' - 'using pattern %s. Details: %s', str(value), + 'using pattern %s. Details: %s', str(value_copy), to_replace, pattern, six.text_type(e)) raise errors.MissingDocumentPattern(jsonpath=jsonpath, pattern=pattern) - return path.update(data, new_value) + return path.update(data_copy, new_value) # Deckhand should be smart enough to create the nested keys in the # data if they don't exist and a pattern isn't required. path = _jsonpath_parse(jsonpath) - path_to_change = path.find(data) + path_to_change = path.find(data_copy) if not path_to_change: - _execute_data_expansion(jsonpath, data) - path_to_change = path.find(data) + _execute_data_expansion(jsonpath, data_copy) + path_to_change = path.find(data_copy) return _execute_replace(path, path_to_change) 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 6728385a..5ccf58b7 100644 --- a/deckhand/tests/unit/engine/test_document_layering_and_substitution.py +++ b/deckhand/tests/unit/engine/test_document_layering_and_substitution.py @@ -15,6 +15,7 @@ import itertools import mock +import yaml from deckhand import factories from deckhand.tests.unit.engine import test_document_layering @@ -711,3 +712,253 @@ class TestDocumentLayeringWithSubstitution( global_expected = None self._test_layering(documents, site_expected, global_expected=global_expected, strict=False) + + def test_substitution_does_not_alter_source_document(self): + """Regression test to ensure that substitution source documents aren't + accidentally modified during a substitution into the destination + document. + + :note: This data below is quite long because it's supposed to be + mirror production-level data, which is where the issue was + caught. + + """ + + documents = """ +--- +schema: deckhand/LayeringPolicy/v1 +metadata: + schema: metadata/Control/v1 + name: layering-policy +data: + layerOrder: + - global + - site +--- +data: {} +id: 91 +metadata: + layeringDefinition: {abstract: false, layer: global} + name: ucp-rabbitmq + replacement: false + schema: metadata/Document/v1 + storagePolicy: cleartext + substitutions: + - dest: {path: .source} + src: + name: software-versions + path: .charts.ucp.rabbitmq + schema: pegleg/SoftwareVersions/v1 + - dest: {path: .values.images.tags} + src: + name: software-versions + path: .images.ucp.rabbitmq + schema: pegleg/SoftwareVersions/v1 + - dest: {path: .values.endpoints.oslo_messaging} + src: + name: ucp_endpoints + path: .ucp.oslo_messaging + schema: pegleg/EndpointCatalogue/v1 + - dest: {path: .values.endpoints.oslo_messaging.auth.user} + src: + name: ucp_service_accounts + path: .ucp.oslo_messaging.admin + schema: pegleg/AccountCatalogue/v1 + - dest: {path: .values.endpoints.oslo_messaging.auth.erlang_cookie} + src: + name: ucp_rabbitmq_erlang_cookie + path: . + schema: deckhand/Passphrase/v1 + - dest: {path: .values.endpoints.oslo_messaging.auth.user.password} + src: + name: ucp_oslo_messaging_password + path: . + schema: deckhand/Passphrase/v1 +schema: armada/Chart/v1 +status: {bucket: design, revision: 1} +--- +schema: pegleg/SoftwareVersions/v1 +metadata: + schema: metadata/Document/v1 + name: software-versions + labels: + name: software-versions + layeringDefinition: + abstract: false + layer: global + storagePolicy: cleartext +data: + charts: + ucp: + rabbitmq: + type: git + location: https://git.openstack.org/openstack/openstack-helm + subpath: rabbitmq + reference: f902cd14fac7de4c4c9f7d019191268a6b4e9601 + images: + ucp: + rabbitmq: + rabbitmq: docker.io/rabbitmq:3.7 + dep_check: quay.io/stackanetes/kubernetes-entrypoint:v0.3.1 +--- +schema: pegleg/EndpointCatalogue/v1 +metadata: + schema: metadata/Document/v1 + name: ucp_endpoints + layeringDefinition: + abstract: false + layer: site + storagePolicy: cleartext +data: + ucp: + oslo_messaging: + namespace: null + hosts: + default: rabbitmq + host_fqdn_override: + default: null + path: /openstack + scheme: rabbit + port: + amqp: + default: 5672 +--- +schema: pegleg/AccountCatalogue/v1 +metadata: + schema: metadata/Document/v1 + name: ucp_service_accounts + layeringDefinition: + abstract: false + layer: site + storagePolicy: cleartext +data: + ucp: + oslo_messaging: + admin: + username: rabbitmq +--- +schema: deckhand/Passphrase/v1 +metadata: + schema: metadata/Document/v1 + name: ucp_rabbitmq_erlang_cookie + layeringDefinition: + abstract: false + layer: site + storagePolicy: cleartext +data: 111df8c05b0f041d4764 +--- +schema: deckhand/Passphrase/v1 +metadata: + schema: metadata/Document/v1 + name: ucp_oslo_messaging_password + layeringDefinition: + abstract: false + layer: site + storagePolicy: cleartext +data: password15 +""" + + global_expected = [ + { + 'charts': { + 'ucp': { + 'rabbitmq': { + 'subpath': 'rabbitmq', + 'type': 'git', + 'location': ('https://git.openstack.org/openstack/' + 'openstack-helm'), + 'reference': ( + 'f902cd14fac7de4c4c9f7d019191268a6b4e9601') + } + } + }, + 'images': { + 'ucp': { + 'rabbitmq': { + 'rabbitmq': 'docker.io/rabbitmq:3.7', + 'dep_check': ( + 'quay.io/stackanetes/kubernetes-entrypoint:' + 'v0.3.1') + } + } + } + }, + { + 'source': { + 'subpath': 'rabbitmq', + 'type': 'git', + 'location': ( + 'https://git.openstack.org/openstack/openstack-helm'), + 'reference': 'f902cd14fac7de4c4c9f7d019191268a6b4e9601' + }, + 'values': { + 'images': { + 'tags': { + 'rabbitmq': 'docker.io/rabbitmq:3.7', + 'dep_check': ( + 'quay.io/stackanetes/kubernetes-entrypoint:' + 'v0.3.1') + } + }, + 'endpoints': { + 'oslo_messaging': { + 'namespace': None, + 'scheme': 'rabbit', + 'port': {'amqp': {'default': 5672}}, + 'hosts': {'default': 'rabbitmq'}, + 'path': '/openstack', + 'auth': { + 'user': { + 'username': 'rabbitmq', + 'password': 'password15' + }, + 'erlang_cookie': '111df8c05b0f041d4764' + }, + 'host_fqdn_override': {'default': None} + } + } + } + } + ] + site_expected = [ + { + 'ucp': { + 'oslo_messaging': { + 'admin': {'username': 'rabbitmq'} + } + } + }, + '111df8c05b0f041d4764', + { + 'ucp': { + # If a copy of the source document isn't made, then these + # values below may be incorrectly modified. The data + # should be unmodified: + # + # oslo_messaging: + # namespace: null + # hosts: + # default: rabbitmq + # host_fqdn_override: + # default: null + # path: /openstack + # scheme: rabbit + # port: + # amqp: + # default: 5672 + 'oslo_messaging': { + 'namespace': None, + 'hosts': {'default': 'rabbitmq'}, + 'host_fqdn_override': {'default': None}, + 'path': '/openstack', + 'scheme': 'rabbit', + 'port': {'amqp': {'default': 5672}}, + } + } + }, + 'password15' + ] + + docs = list(yaml.safe_load_all(documents)) + self._test_layering(docs, site_expected=site_expected, + global_expected=global_expected)