airship
/
deckhand
Code Issues Proposed changes

Improve validation error messages returned by Deckhand 

Currently the format of the validation error messages returned
by the Deckhand Validation API is lacking. The new response is a
dictionary with the following keys:

    * validation_schema: The schema body that was used to validate the
        document.
    * schema_path: The JSON path in the schema where the failure originated.
    * name: The document name.
    * schema: The document schema.
    * path: The JSON path in the document where the failure originated.
    * error_section: The "section" in the document above which the error
        originated (i.e. the dict in which ``path`` is found).
    * message: The error message returned by the ``jsonschema`` validator.

This PS updates the document validation module and associated unit tests
to return and verify the above format.

Change-Id: I9ef1c36db85233cbfb866dea786228ef1416468c
This commit is contained in:
Felipe Monteiro 2018-01-24 15:58:40 -05:00
parent a3e96dac29
commit 021090516b
5 changed files with 152 additions and 34 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

81
deckhand/engine/document_validation.py
View File

@ -22,6 +22,7 @@ import six
from deckhand.engine import document_wrapper
from deckhand.engine.schema import base_schema
from deckhand.engine.schema import v1_0
from deckhand.engine.secrets_manager import SecretsSubstitution
from deckhand import errors
from deckhand import types
from deckhand import utils
@ -131,7 +132,7 @@ class SchemaValidator(BaseValidator):
:rtype: dict
"""
schema_prefix, schema_version = get_schema_parts(document)
schema_prefix, schema_version = _get_schema_parts(document)
matching_schemas = []
relevant_schemas = self._schema_map.get(schema_version, {})
for candidae_schema_prefix, schema in relevant_schemas.items():
@ -194,9 +195,8 @@ class SchemaValidator(BaseValidator):
'Failed schema validation for document [%s] %s. '
'Details: %s.', document.schema, document.name,
error.message)
parent_path = root_path + '.'.join(
[six.text_type(x) for x in error.path])
yield error.message, parent_path
yield _generate_validation_error_output(
schema_to_use, document, error, root_path)
class DataSchemaValidator(SchemaValidator):
@ -218,7 +218,7 @@ class DataSchemaValidator(SchemaValidator):
continue
if 'data' not in data_schema:
continue
schema_prefix, schema_version = get_schema_parts(data_schema,
schema_prefix, schema_version = _get_schema_parts(data_schema,
'metadata.name')
class Schema(object):
@ -233,7 +233,7 @@ class DataSchemaValidator(SchemaValidator):
LOG.info('Skipping schema validation for abstract document [%s]: '
'%s.', document.schema, document.name)
return False
schema_prefix, schema_version = get_schema_parts(document)
schema_prefix, schema_version = _get_schema_parts(document)
return schema_prefix in self._schema_map.get(schema_version, {})
def validate(self, document):
@ -328,7 +328,7 @@ class DocumentValidation(object):
supported_schema_list = self._get_supported_schema_list()
document_schema = None if not document.get('schema') else '/'.join(
get_schema_parts(document))
_get_schema_parts(document))
if document_schema not in supported_schema_list:
error_msg = ("The provided document schema %s is invalid. "
"Supported schemas include: %s" % (
@ -344,15 +344,10 @@ class DocumentValidation(object):
for validator in self._validators:
if validator.matches(document):
error_messages = validator.validate(document)
if error_messages:
for error_msg, error_path in error_messages:
result['errors'].append({
'schema': document.schema,
'name': document.name,
'message': error_msg,
'path': error_path
})
error_outputs = validator.validate(document)
if error_outputs:
for error_output in error_outputs:
result['errors'].append(error_output)
if result['errors']:
result.setdefault('status', 'failure')
@ -420,10 +415,62 @@ class DocumentValidation(object):
return validations
def get_schema_parts(document, schema_key='schema'):
def _get_schema_parts(document, schema_key='schema'):
schema_parts = utils.jsonpath_parse(document, schema_key).split('/')
schema_prefix = '/'.join(schema_parts[:2])
schema_version = schema_parts[2]
if schema_version.endswith('.0'):
schema_version = schema_version[:-2]
return schema_prefix, schema_version
def _generate_validation_error_output(schema, document, error, root_path):
"""Returns a formatted output with necessary details for debugging why
a validation failed.
The response is a dictionary with the following keys:
* validation_schema: The schema body that was used to validate the
document.
* schema_path: The JSON path in the schema where the failure originated.
* name: The document name.
* schema: The document schema.
* path: The JSON path in the document where the failure originated.
* error_section: The "section" in the document above which the error
originated (i.e. the dict in which ``path`` is found).
* message: The error message returned by the ``jsonschema`` validator.
:returns: Dictionary in the above format.
"""
path_to_error_in_document = root_path + '.'.join(
[str(x) for x in error.path])
path_to_error_in_schema = '.' + '.'.join(
[str(x) for x in error.schema_path])
parent_path_to_error_in_document = '.'.join(
path_to_error_in_document.split('.')[:-1]) or '.'
try:
parent_error_section = utils.jsonpath_parse(
document, parent_path_to_error_in_document)
if 'data' in parent_error_section:
# NOTE(fmontei): Because validation is performed on fully rendered
# documents, it is necessary to omit the parts of the data section
# where substitution may have occurred to avoid exposing any
# secrets. While this may make debugging a few validation failures
# more difficult, it is a necessary evil.
SecretsSubstitution.sanitize_potential_secrets(document)
except Exception:
parent_error_section = (
'Failed to find parent section above where error occurred.')
error_output = {
'validation_schema': schema.schema,
'schema_path': path_to_error_in_schema,
'name': document.name,
'schema': document.schema,
'path': path_to_error_in_document,
'error_section': parent_error_section,
'message': error.message
}
return error_output

14
deckhand/engine/secrets_manager.py
View File

@ -216,3 +216,17 @@ class SecretsSubstitution(object):
substituted_docs.append(document)
return substituted_docs
@staticmethod
def sanitize_potential_secrets(document):
"""Sanitize all secret data that may have been substituted into the
document. Uses references in ``document.substitutions`` to determine
which values to sanitize. Only meaningful to call this on post-rendered
documents.
:param DocumentDict document: Document to sanitize.
"""
safe_message = 'Sanitized to avoid exposing secret.'
for sub in document.substitutions:
utils.jsonpath_replace(document['data'], safe_message,
sub['dest']['path'])

61
deckhand/tests/unit/control/test_validations_controller.py
View File

@ -17,6 +17,7 @@ import yaml
from oslo_config import cfg
from deckhand.engine.schema.v1_0 import document_schema
from deckhand import factories
from deckhand.tests import test_utils
from deckhand.tests.unit.control import base as test_base
@ -555,19 +556,29 @@ class TestValidationsController(test_base.BaseControllerTest):
self.assertEqual(expected_body, body)
# Validate that both expected errors are present for validation.
expected_errors = [
{
'message': "'layeringDefinition' is a required property",
'name': 'test_doc',
'schema': 'example/foo/v1',
'path': '.metadata'
}, {
'message': "'fail' is not of type 'integer'",
'name': 'test_doc',
'schema': 'example/foo/v1',
'path': '.data.a'
}
]
expected_errors = [{
'error_section': {
'data': {'a': 'fail'},
'metadata': {'labels': {'global': 'global1'},
'name': 'test_doc',
'schema': 'metadata/Document/v1.0'},
'schema': 'example/foo/v1'
},
'name': 'test_doc',
'path': '.metadata',
'schema': 'example/foo/v1',
'message': "'layeringDefinition' is a required property",
'validation_schema': document_schema.schema,
'schema_path': '.properties.metadata.required'
}, {
'error_section': {'a': 'fail'},
'name': 'test_doc',
'path': '.data.a',
'schema': 'example/foo/v1',
'message': "'fail' is not of type 'integer'",
'validation_schema': schema_to_use,
'schema_path': '.properties.a.type'
}]
resp = self.app.simulate_get(
'/api/v1.0/revisions/%s/validations/%s/entries/0' % (
revision_id, types.DECKHAND_SCHEMA_VALIDATION),
@ -598,6 +609,15 @@ class TestValidationsController(test_base.BaseControllerTest):
},
'required': ['a']
}
expected_errors = [{
'error_section': {'a': 'fail'},
'name': 'test_doc',
'path': '.data.a',
'schema': 'example/foo/v1',
'message': "'fail' is not of type 'integer'",
'validation_schema': schema_to_use,
'schema_path': '.properties.a.type'
}]
data_schema = data_schema_factory.gen_test(
metadata_name, data=schema_to_use)
@ -652,11 +672,15 @@ class TestValidationsController(test_base.BaseControllerTest):
self.assertEqual(200, resp.status_code)
body = yaml.safe_load(resp.text)
expected_errors = [{
'schema': 'example/foo/v1',
'error_section': {'a': 'fail'},
'name': 'test_doc',
'path': '.data.a',
'schema': 'example/foo/v1',
'message': "'fail' is not of type 'integer'",
'path': '.data.a'
'validation_schema': schema_to_use,
'schema_path': '.properties.a.type'
}]
self.assertIn('errors', body)
self.assertEqual(expected_errors, body['errors'])
@ -704,10 +728,13 @@ class TestValidationsController(test_base.BaseControllerTest):
self.assertEqual(200, resp.status_code)
body = yaml.safe_load(resp.text)
expected_errors = [{
'schema': document['schema'],
'error_section': document,
'name': document['metadata']['name'],
'path': '.',
'schema': document['schema'],
'message': "'data' is a required property",
'path': '.'
'validation_schema': document_schema.schema,
'schema_path': '.required'
}]
self.assertIn('errors', body)
self.assertEqual(expected_errors, body['errors'])

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

@ -63,3 +63,28 @@ class TestDocumentValidation(engine_test_base.TestDocumentValidationBase):
self.assertTrue(mock_log.info.called)
self.assertIn("Skipping schema validation for abstract document",
mock_log.info.mock_calls[0][1][0])
@mock.patch.object(document_validation, 'jsonschema', autospec=True)
def test_validation_failure_does_not_expose_secrets(self, mock_jsonschema):
m_args = mock.Mock()
mock_jsonschema.Draft4Validator(m_args).iter_errors.side_effect = [
# Return empty list of errors for base schema validator and pretend
# that 1 error is returned for next validator.
[], [mock.Mock(path=[], schema_path=[])]
]
test_document = self._read_data('sample_document')
for sub in test_document['metadata']['substitutions']:
substituted_data = utils.jsonpath_replace(
test_document['data'], 'scary-secret', sub['dest']['path'])
test_document['data'].update(substituted_data)
self.assertEqual(
'scary-secret', utils.jsonpath_parse(test_document['data'],
sub['dest']['path']))
validations = document_validation.DocumentValidation(
test_document).validate_all()
self.assertEqual(2, len(validations[0]['errors']))
self.assertIn('Sanitized to avoid exposing secret.',
str(validations[0]['errors'][-1]))
self.assertNotIn('scary-secret.', str(validations[0]['errors'][-1]))

5
deckhand/tests/unit/engine/test_document_validation_negative.py
View File

@ -44,6 +44,11 @@ class TestDocumentValidationNegative(test_base.TestDocumentValidationBase):
self.assertEqual(types.DECKHAND_SCHEMA_VALIDATION,
validations[-1]['name'])
self.assertEqual(1, len(validations[-1]['errors']))
for key in ('name', 'schema', 'path', 'error_section',
'validation_schema', 'schema_path', 'message'):
self.assertIn(key, validations[-1]['errors'][-1])
self.assertEqual(expected['metadata']['name'],
validations[-1]['errors'][-1]['name'])
self.assertEqual(expected['schema'],