airship
/
deckhand
Code Issues Proposed changes

Fix various substitution issues 

This PS solves the following issues for which only minor changes
were needed:

1) Using copy.copy() around the substitution value passed in
   to jsonpath_replace so as to avoid updating the substitution
   entry referentially which was happening before this change,
   causing future substitutions using that entry to fail or
   behaving unexpectedly.
2) Supporting non-string substitution values when a substitution
   pattern is provided: before this change, this was failing
   because calling re.sub() and passing in a non-string
   value causes an error to be thrown.
3) Adding better logging and error handling to
   deckhand.utils.jsonpath_replace to assist with debugging.

Unit tests are included for some of the scenarios above.

Change-Id: I8562d43a717f477e3297504c1522331b3a993f88
This commit is contained in:
Felipe Monteiro 2018-02-05 20:44:23 +00:00
parent 88ac664883
commit 36f752bb93
6 changed files with 169 additions and 30 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
deckhand/control/revision_documents.py
View File

@ -117,7 +117,7 @@ class RenderedDocumentsResource(api_base.BaseResource):
errors.MissingDocumentKey) as e:
raise falcon.HTTPBadRequest(description=e.format_message())
except (errors.LayeringPolicyNotFound,
errors.SubstitutionDependencyNotFound) as e:
errors.SubstitutionFailure) as e:
raise falcon.HTTPConflict(description=e.format_message())
# Filters to be applied post-rendering, because many documents are

3
deckhand/engine/secrets_manager.py
View File

@ -215,8 +215,7 @@ class SecretsSubstitution(object):
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))
raise errors.SubstitutionFailure(details=six.text_type(e))
yield document

8
deckhand/errors.py
View File

@ -246,10 +246,10 @@ class LayeringPolicyNotFound(DeckhandException):
code = 409
class SubstitutionDependencyNotFound(DeckhandException):
msg_fmt = ('Failed to find a dependent source document required for '
'substitution. Details: %(details)s')
code = 409
class SubstitutionFailure(DeckhandException):
msg_fmt = ('An unknown exception occurred while trying to perform '
'substitution. Details: %(detail)s')
code = 400
class BarbicanException(DeckhandException):

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

@ -57,7 +57,7 @@ class TestDocumentValidation(engine_test_base.TestDocumentValidationBase):
test_document = self._read_data('sample_passphrase')
# Set the document to abstract.
abstract_document = utils.jsonpath_replace(
test_document, True, 'metadata.layeringDefinition.abstract')
test_document, True, '.metadata.layeringDefinition.abstract')
document_validation.DocumentValidation(
abstract_document).validate_all()
self.assertTrue(mock_log.info.called)

157
deckhand/tests/unit/engine/test_secrets_manager.py
View File

@ -13,6 +13,7 @@
# limitations under the License.
import copy
import yaml
from deckhand.db.sqlalchemy import api as db_api
from deckhand.engine import secrets_manager
@ -80,7 +81,7 @@ class TestSecretsSubstitution(test_base.TestDbBase):
self.document_factory = factories.DocumentFactory(1, [1])
self.secrets_factory = factories.DocumentSecretFactory()
def _test_secret_substitution(self, document_mapping, secret_documents,
def _test_doc_substitution(self, document_mapping, secret_documents,
expected_data):
payload = self.document_factory.gen_test(document_mapping,
global_abstract=False)
@ -99,7 +100,7 @@ class TestSecretsSubstitution(test_base.TestDbBase):
substituted_docs = list(secret_substitution.substitute_all(documents))
self.assertIn(expected_document, substituted_docs)
def test_secret_substitution_single_cleartext(self):
def test_doc_substitution_single_cleartext(self):
certificate = self.secrets_factory.gen_test(
'Certificate', 'cleartext', data='CERTIFICATE DATA')
certificate['metadata']['name'] = 'example-cert'
@ -126,7 +127,7 @@ class TestSecretsSubstitution(test_base.TestDbBase):
}
}
}
self._test_secret_substitution(
self._test_doc_substitution(
document_mapping, [certificate], expected_data)
def test_create_destination_path_with_array(self):
@ -158,7 +159,7 @@ class TestSecretsSubstitution(test_base.TestDbBase):
}
}]
}
self._test_secret_substitution(
self._test_doc_substitution(
document_mapping, [certificate], expected_data)
def test_create_destination_path_with_array_sequential_indices(self):
@ -210,7 +211,7 @@ class TestSecretsSubstitution(test_base.TestDbBase):
}
]
}
self._test_secret_substitution(
self._test_doc_substitution(
document_mapping, [certificate], expected_data)
def test_create_destination_path_with_array_multiple_subs(self):
@ -254,7 +255,7 @@ class TestSecretsSubstitution(test_base.TestDbBase):
}
}]
}
self._test_secret_substitution(
self._test_doc_substitution(
document_mapping, [certificate], expected_data)
def test_create_destination_path_with_nested_arrays(self):
@ -289,10 +290,10 @@ class TestSecretsSubstitution(test_base.TestDbBase):
}
]
}
self._test_secret_substitution(
self._test_doc_substitution(
document_mapping, [certificate], expected_data)
def test_secret_substitution_single_cleartext_with_pattern(self):
def test_doc_substitution_single_cleartext_with_pattern(self):
passphrase = self.secrets_factory.gen_test(
'Passphrase', 'cleartext', data='my-secret-password')
passphrase['metadata']['name'] = 'example-password'
@ -329,10 +330,10 @@ class TestSecretsSubstitution(test_base.TestDbBase):
}
}
}
self._test_secret_substitution(
self._test_doc_substitution(
document_mapping, [passphrase], expected_data)
def test_secret_substitution_double_cleartext(self):
def test_doc_substitution_double_cleartext(self):
certificate = self.secrets_factory.gen_test(
'Certificate', 'cleartext', data='CERTIFICATE DATA')
certificate['metadata']['name'] = 'example-cert'
@ -374,10 +375,10 @@ class TestSecretsSubstitution(test_base.TestDbBase):
}
}
}
self._test_secret_substitution(
self._test_doc_substitution(
document_mapping, [certificate, certificate_key], expected_data)
def test_secret_substitution_multiple_cleartext(self):
def test_doc_substitution_multiple_cleartext(self):
certificate = self.secrets_factory.gen_test(
'Certificate', 'cleartext', data='CERTIFICATE DATA')
certificate['metadata']['name'] = 'example-cert'
@ -446,7 +447,7 @@ class TestSecretsSubstitution(test_base.TestDbBase):
}
}
}
self._test_secret_substitution(
self._test_doc_substitution(
document_mapping, [certificate, certificate_key, passphrase],
expected_data)
@ -488,5 +489,133 @@ class TestSecretsSubstitution(test_base.TestDbBase):
}]
}
self._test_secret_substitution(
self._test_doc_substitution(
document_mapping, dependent_documents, expected_data=src_data)
def test_doc_substitution_multiple_pattern_non_string_values(self):
for test_value in (123, 3.2, False, None):
test_yaml = """
---
schema: deckhand/LayeringPolicy/v1
metadata:
schema: metadata/Control/v1
name: layering-policy
data:
layerOrder:
- global
- site
---
schema: armada/Chart/v1
metadata:
schema: metadata/Document/v1
name: ucp-drydock
layeringDefinition:
abstract: false
layer: global
storagePolicy: cleartext
substitutions:
- src:
schema: twigleg/CommonAddresses/v1
name: common-addresses
path: .node_ports.maas_api
dest:
path: .values.conf.drydock.maasdriver.maas_api_url
pattern: 'MAAS_PORT'
data:
values:
conf:
drydock:
maasdriver:
maas_api_url: http://10.24.31.31:MAAS_PORT/MAAS/api/2.0/
---
schema: twigleg/CommonAddresses/v1
metadata:
schema: metadata/Document/v1
name: common-addresses
layeringDefinition:
abstract: false
layer: site
storagePolicy: cleartext
data:
node_ports:
maas_api: {}
...
""".format(test_value)
documents = list(yaml.safe_load_all(test_yaml))
expected = copy.deepcopy(documents[1])
expected['data']['values']['conf']['drydock']['maasdriver'][
'maas_api_url'] = 'http://10.24.31.31:{}/MAAS/api/2.0/'.format(
test_value)
secret_substitution = secrets_manager.SecretsSubstitution(
documents)
substituted_docs = list(secret_substitution.substitute_all(
documents))
self.assertEqual(expected, substituted_docs[0])
def test_doc_substitution_multiple_pattern_substitutions(self):
test_yaml = """
---
schema: deckhand/LayeringPolicy/v1
metadata:
schema: metadata/Control/v1
name: layering-policy
data:
layerOrder:
- global
- site
---
schema: armada/Chart/v1
metadata:
schema: metadata/Document/v1
name: ucp-drydock
layeringDefinition:
abstract: false
layer: global
storagePolicy: cleartext
substitutions:
- src:
schema: twigleg/CommonAddresses/v1
name: common-addresses
path: .genesis.ip
dest:
path: .values.conf.drydock.maasdriver.maas_api_url
pattern: 'MAAS_IP'
- src:
schema: twigleg/CommonAddresses/v1
name: common-addresses
path: .node_ports.maas_api
dest:
path: .values.conf.drydock.maasdriver.maas_api_url
pattern: 'MAAS_PORT'
data:
values:
conf:
drydock:
maasdriver:
maas_api_url: http://MAAS_IP:MAAS_PORT/MAAS/api/2.0/
---
schema: twigleg/CommonAddresses/v1
metadata:
schema: metadata/Document/v1
name: common-addresses
layeringDefinition:
abstract: false
layer: site
storagePolicy: cleartext
data:
genesis:
ip: 10.24.31.31
node_ports:
maas_api: 30001
...
"""
documents = list(yaml.safe_load_all(test_yaml))
expected = copy.deepcopy(documents[1])
expected['data']['values']['conf']['drydock']['maasdriver'][
'maas_api_url'] = 'http://10.24.31.31:30001/MAAS/api/2.0/'
secret_substitution = secrets_manager.SecretsSubstitution(documents)
substituted_docs = list(secret_substitution.substitute_all(documents))
self.assertEqual(expected, substituted_docs[0])

27
deckhand/utils.py
View File

@ -18,10 +18,13 @@ import re
import string
import jsonpath_ng
from oslo_log import log as logging
import six
from deckhand import errors
LOG = logging.getLogger(__name__)
def to_camel_case(s):
"""Convert string to camel case."""
@ -115,11 +118,12 @@ def jsonpath_replace(data, value, jsonpath, pattern=None):
:param data: The `data` section of a document.
:param value: The new value for ``data[jsonpath]``.
:param jsonpath: A multi-part key that references a nested path in
``data``.
``data``. Must begin with "." (without quotes).
:param pattern: A regular expression pattern.
:returns: Updated value at ``data[jsonpath]``.
:raises: MissingDocumentPattern if ``pattern`` is not None and
``data[jsonpath]`` doesn't exist.
:raises ValueError: If ``jsonpath`` doesn't begin with "."
Example::
@ -138,27 +142,34 @@ def jsonpath_replace(data, value, jsonpath, pattern=None):
doc['data'].update(replaced_data)
"""
data = copy.copy(data)
value = copy.copy(value)
if jsonpath == '.':
jsonpath = '$'
elif jsonpath.startswith('.'):
jsonpath = '$' + jsonpath
else:
LOG.error('The provided jsonpath %s does not begin with "."', jsonpath)
raise ValueError('The provided jsonpath %s does not begin with "."',
jsonpath)
def _do_replace():
p = jsonpath_ng.parse(jsonpath)
p_to_change = p.find(data)
if p_to_change:
_value = value
new_value = value
if pattern:
to_replace = p_to_change[0].value
# `value` represents the value to inject into `to_replace` that
# matches the `pattern`.
# `new_value` represents the value to inject into `to_replace`
# that matches the `pattern`.
try:
_value = re.sub(pattern, value, to_replace)
except TypeError:
_value = None
return p.update(data, _value)
new_value = re.sub(pattern, str(value), to_replace)
except TypeError as e:
LOG.error('Failed to substitute the value %s into %s '
'using pattern %s. Details: %s', str(value),
to_replace, pattern, six.text_type(e))
return p.update(data, new_value)
result = _do_replace()
if result: