Fail fast on bad substitution input during layering
This PS causes layering module to fail fast on malformed ``metadata.substitutions`` entry in a document by performing built-in schema validation when validate=True is passed to the DocumentLayering constructor. This kwarg is useful for when the layering module is called directly -- i.e. by Promenade or Pegleg. (The Deckhand server already performs document pre-validation during document ingestion so there is no need for documents stored inside Deckhand to be re-validated again for rendered-documents endpoint.) Next, a new exception was added -- SubstitutionSourceNotFound -- which is raised when a substitution document is referenced by another document but isn't found. Finally, the previous exception raised by the secrets_manager module has been renamed to UnknownSubstitutionError which now raises a 500 instead of a 400 as this exception will most likely be due to an internal server error of some kind. Unit tests were added and documentation changes were made. Change-Id: Idfd91a52ef9ffd8f9b1c06c6b84c3405acab6f16
This commit is contained in:
parent
0d9d243a5d
commit
b81cebb012
|
@ -109,18 +109,24 @@ class RenderedDocumentsResource(api_base.BaseResource):
|
||||||
substitution_sources = self._retrieve_substitution_sources()
|
substitution_sources = self._retrieve_substitution_sources()
|
||||||
|
|
||||||
try:
|
try:
|
||||||
|
# NOTE(fmontei): `validate` is False because documents have already
|
||||||
|
# been pre-validated during ingestion. Documents are post-validated
|
||||||
|
# below, regardless.
|
||||||
document_layering = layering.DocumentLayering(
|
document_layering = layering.DocumentLayering(
|
||||||
documents, substitution_sources)
|
documents, substitution_sources, validate=False)
|
||||||
rendered_documents = document_layering.render()
|
rendered_documents = document_layering.render()
|
||||||
except (errors.InvalidDocumentLayer,
|
except (errors.InvalidDocumentLayer,
|
||||||
errors.InvalidDocumentParent,
|
errors.InvalidDocumentParent,
|
||||||
errors.IndeterminateDocumentParent,
|
errors.IndeterminateDocumentParent,
|
||||||
errors.UnsupportedActionMethod,
|
errors.MissingDocumentKey,
|
||||||
errors.MissingDocumentKey) as e:
|
errors.UnsupportedActionMethod) as e:
|
||||||
raise falcon.HTTPBadRequest(description=e.format_message())
|
raise falcon.HTTPBadRequest(description=e.format_message())
|
||||||
except (errors.LayeringPolicyNotFound,
|
except (errors.LayeringPolicyNotFound,
|
||||||
errors.SubstitutionFailure) as e:
|
errors.SubstitutionSourceNotFound) as e:
|
||||||
raise falcon.HTTPConflict(description=e.format_message())
|
raise falcon.HTTPConflict(description=e.format_message())
|
||||||
|
except errors.errors.UnknownSubstitutionError as e:
|
||||||
|
raise falcon.HTTPInternalServerError(
|
||||||
|
description=e.format_message())
|
||||||
|
|
||||||
# Filters to be applied post-rendering, because many documents are
|
# Filters to be applied post-rendering, because many documents are
|
||||||
# involved in rendering. User filters can only be applied once all
|
# involved in rendering. User filters can only be applied once all
|
||||||
|
|
|
@ -20,6 +20,7 @@ from networkx.algorithms.cycles import find_cycle
|
||||||
from networkx.algorithms.dag import topological_sort
|
from networkx.algorithms.dag import topological_sort
|
||||||
from oslo_log import log as logging
|
from oslo_log import log as logging
|
||||||
|
|
||||||
|
from deckhand.engine import document_validation
|
||||||
from deckhand.engine import document_wrapper
|
from deckhand.engine import document_wrapper
|
||||||
from deckhand.engine import secrets_manager
|
from deckhand.engine import secrets_manager
|
||||||
from deckhand.engine import utils as engine_utils
|
from deckhand.engine import utils as engine_utils
|
||||||
|
@ -244,7 +245,27 @@ class DocumentLayering(object):
|
||||||
|
|
||||||
return result
|
return result
|
||||||
|
|
||||||
def __init__(self, documents, substitution_sources=None):
|
def _validate_documents(self, documents):
|
||||||
|
LOG.debug('%s performing document pre-validation.',
|
||||||
|
self.__class__.__name__)
|
||||||
|
validator = document_validation.DocumentValidation(
|
||||||
|
documents, pre_validate=True)
|
||||||
|
results = validator.validate_all()
|
||||||
|
val_errors = []
|
||||||
|
for result in results:
|
||||||
|
val_errors.extend(
|
||||||
|
[(e['schema'], e['name'], e['message'])
|
||||||
|
for e in result['errors']])
|
||||||
|
if val_errors:
|
||||||
|
for error in val_errors:
|
||||||
|
LOG.error(
|
||||||
|
'Document [%s] %s failed with pre-validation error: %s.',
|
||||||
|
*error)
|
||||||
|
raise errors.InvalidDocumentFormat(
|
||||||
|
details='The following pre-validation errors occurred '
|
||||||
|
'(schema, name, error): %s.' % val_errors)
|
||||||
|
|
||||||
|
def __init__(self, documents, substitution_sources=None, validate=True):
|
||||||
"""Contructor for ``DocumentLayering``.
|
"""Contructor for ``DocumentLayering``.
|
||||||
|
|
||||||
:param layering_policy: The document with schema
|
:param layering_policy: The document with schema
|
||||||
|
@ -256,6 +277,9 @@ class DocumentLayering(object):
|
||||||
:param substitution_sources: List of documents that are potential
|
:param substitution_sources: List of documents that are potential
|
||||||
sources for substitution. Should only include concrete documents.
|
sources for substitution. Should only include concrete documents.
|
||||||
:type substitution_sources: List[dict]
|
:type substitution_sources: List[dict]
|
||||||
|
:param validate: Whether to pre-validate documents using built-in
|
||||||
|
schema validation. Default is True.
|
||||||
|
:type validate: bool
|
||||||
|
|
||||||
:raises LayeringPolicyNotFound: If no LayeringPolicy was found among
|
:raises LayeringPolicyNotFound: If no LayeringPolicy was found among
|
||||||
list of ``documents``.
|
list of ``documents``.
|
||||||
|
@ -271,6 +295,9 @@ class DocumentLayering(object):
|
||||||
self._documents_by_labels = {}
|
self._documents_by_labels = {}
|
||||||
self._layering_policy = None
|
self._layering_policy = None
|
||||||
|
|
||||||
|
if validate:
|
||||||
|
self._validate_documents(documents)
|
||||||
|
|
||||||
layering_policies = list(
|
layering_policies = list(
|
||||||
filter(lambda x: x.get('schema').startswith(
|
filter(lambda x: x.get('schema').startswith(
|
||||||
types.LAYERING_POLICY_SCHEMA), documents))
|
types.LAYERING_POLICY_SCHEMA), documents))
|
||||||
|
|
|
@ -27,7 +27,10 @@ substitution_schema = {
|
||||||
'src': {
|
'src': {
|
||||||
'type': 'object',
|
'type': 'object',
|
||||||
'properties': {
|
'properties': {
|
||||||
'schema': {'type': 'string'},
|
'schema': {
|
||||||
|
'type': 'string',
|
||||||
|
'pattern': '^[A-Za-z]+/[A-Za-z]+/v\d+(.0)?$'
|
||||||
|
},
|
||||||
'name': {'type': 'string'},
|
'name': {'type': 'string'},
|
||||||
'path': {'type': 'string'}
|
'path': {'type': 'string'}
|
||||||
},
|
},
|
||||||
|
|
|
@ -139,6 +139,10 @@ class SecretsSubstitution(object):
|
||||||
:type documents: dict or List[dict]
|
:type documents: dict or List[dict]
|
||||||
:returns: List of fully substituted documents.
|
:returns: List of fully substituted documents.
|
||||||
:rtype: Generator[:class:`DocumentDict`]
|
:rtype: Generator[:class:`DocumentDict`]
|
||||||
|
:raises SubstitutionSourceNotFound: If a substitution source document
|
||||||
|
is referenced by another document but wasn't found.
|
||||||
|
:raises UnknownSubstitutionError: If an unknown error occurred during
|
||||||
|
substitution.
|
||||||
"""
|
"""
|
||||||
|
|
||||||
documents_to_substitute = []
|
documents_to_substitute = []
|
||||||
|
@ -164,28 +168,18 @@ class SecretsSubstitution(object):
|
||||||
src_name = sub['src']['name']
|
src_name = sub['src']['name']
|
||||||
src_path = sub['src']['path']
|
src_path = sub['src']['path']
|
||||||
|
|
||||||
if not src_schema:
|
|
||||||
LOG.warning('Source document schema "%s" is unspecified '
|
|
||||||
'under substitutions for document [%s] %s.',
|
|
||||||
src_schema, document.schema, document.name)
|
|
||||||
if not src_name:
|
|
||||||
LOG.warning('Source document name "%s" is unspecified'
|
|
||||||
' under substitutions for document [%s] %s.',
|
|
||||||
src_name, document.schema, document.name)
|
|
||||||
if not src_path:
|
|
||||||
LOG.warning('Source document path "%s" is unspecified '
|
|
||||||
'under substitutions for document [%s] %s.',
|
|
||||||
src_path, document.schema, document.name)
|
|
||||||
|
|
||||||
if (src_schema, src_name) in self._substitution_sources:
|
if (src_schema, src_name) in self._substitution_sources:
|
||||||
src_doc = self._substitution_sources[
|
src_doc = self._substitution_sources[
|
||||||
(src_schema, src_name)]
|
(src_schema, src_name)]
|
||||||
else:
|
else:
|
||||||
src_doc = {}
|
message = ('Could not find substitution source document '
|
||||||
LOG.warning('Could not find substitution source document '
|
'[%s] %s among the provided '
|
||||||
'[%s] %s among the provided '
|
'`substitution_sources`.', src_schema, src_name)
|
||||||
'`substitution_sources`.', src_schema,
|
LOG.error(message)
|
||||||
src_name)
|
raise errors.SubstitutionSourceNotFound(
|
||||||
|
src_schema=src_schema, src_name=src_name,
|
||||||
|
document_schema=document.schema,
|
||||||
|
document_name=document.name)
|
||||||
|
|
||||||
# If the data is a dictionary, retrieve the nested secret
|
# If the data is a dictionary, retrieve the nested secret
|
||||||
# via jsonpath_parse, else the secret is the primitive/string
|
# via jsonpath_parse, else the secret is the primitive/string
|
||||||
|
@ -199,11 +193,6 @@ class SecretsSubstitution(object):
|
||||||
dest_path = sub['dest']['path']
|
dest_path = sub['dest']['path']
|
||||||
dest_pattern = sub['dest'].get('pattern', None)
|
dest_pattern = sub['dest'].get('pattern', None)
|
||||||
|
|
||||||
if not dest_path:
|
|
||||||
LOG.warning('Destination document path "%s" is unspecified'
|
|
||||||
' under substitutions for document [%s] %s.',
|
|
||||||
dest_path, document.schema, document.name)
|
|
||||||
|
|
||||||
LOG.debug('Substituting from schema=%s name=%s src_path=%s '
|
LOG.debug('Substituting from schema=%s name=%s src_path=%s '
|
||||||
'into dest_path=%s, dest_pattern=%s', src_schema,
|
'into dest_path=%s, dest_pattern=%s', src_schema,
|
||||||
src_name, src_path, dest_path, dest_pattern)
|
src_name, src_path, dest_path, dest_pattern)
|
||||||
|
@ -222,15 +211,18 @@ class SecretsSubstitution(object):
|
||||||
if sub_source:
|
if sub_source:
|
||||||
sub_source['data'] = substituted_data
|
sub_source['data'] = substituted_data
|
||||||
else:
|
else:
|
||||||
LOG.warning(
|
message = (
|
||||||
'Failed to create JSON path "%s" in the '
|
'Failed to create JSON path "%s" in the '
|
||||||
'destination document [%s] %s. No data was '
|
'destination document [%s] %s. No data was '
|
||||||
'substituted.', dest_path, document.schema,
|
'substituted.', dest_path, document.schema,
|
||||||
document.name)
|
document.name)
|
||||||
|
LOG.error(message)
|
||||||
|
raise errors.UnknownSubstitutionError(details=message)
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
LOG.error('Unexpected exception occurred while attempting '
|
LOG.error('Unexpected exception occurred while attempting '
|
||||||
'secret substitution. %s', six.text_type(e))
|
'secret substitution. %s', six.text_type(e))
|
||||||
raise errors.SubstitutionFailure(details=six.text_type(e))
|
raise errors.UnknownSubstitutionError(
|
||||||
|
details=six.text_type(e))
|
||||||
|
|
||||||
yield document
|
yield document
|
||||||
|
|
||||||
|
|
|
@ -171,11 +171,11 @@ class DeckhandException(Exception):
|
||||||
|
|
||||||
|
|
||||||
class InvalidDocumentFormat(DeckhandException):
|
class InvalidDocumentFormat(DeckhandException):
|
||||||
"""Schema validations failed for the provided document.
|
"""Schema validations failed for the provided document(s).
|
||||||
|
|
||||||
**Troubleshoot:**
|
**Troubleshoot:**
|
||||||
"""
|
"""
|
||||||
msg_fmt = ("The provided document failed schema validation. Details: "
|
msg_fmt = ("The provided document(s) failed schema validation. Details: "
|
||||||
"%(details)s")
|
"%(details)s")
|
||||||
code = 400
|
code = 400
|
||||||
|
|
||||||
|
@ -184,6 +184,7 @@ class InvalidDocumentLayer(DeckhandException):
|
||||||
"""The document layer is invalid.
|
"""The document layer is invalid.
|
||||||
|
|
||||||
**Troubleshoot:**
|
**Troubleshoot:**
|
||||||
|
|
||||||
* Check that the document layer is contained in the layerOrder in the
|
* Check that the document layer is contained in the layerOrder in the
|
||||||
registered LayeringPolicy in the system.
|
registered LayeringPolicy in the system.
|
||||||
"""
|
"""
|
||||||
|
@ -198,6 +199,7 @@ class InvalidDocumentParent(DeckhandException):
|
||||||
"""The document parent is invalid.
|
"""The document parent is invalid.
|
||||||
|
|
||||||
**Troubleshoot:**
|
**Troubleshoot:**
|
||||||
|
|
||||||
* Check that the document `schema` and parent `schema` match.
|
* Check that the document `schema` and parent `schema` match.
|
||||||
* Check that the document layer is lower-order than the parent layer.
|
* Check that the document layer is lower-order than the parent layer.
|
||||||
"""
|
"""
|
||||||
|
@ -220,6 +222,7 @@ class SubstitutionDependencyCycle(DeckhandException):
|
||||||
"""An illegal substitution depdencency cycle was detected.
|
"""An illegal substitution depdencency cycle was detected.
|
||||||
|
|
||||||
**Troubleshoot:**
|
**Troubleshoot:**
|
||||||
|
|
||||||
* Check that there is no two-way substitution dependency between documents.
|
* Check that there is no two-way substitution dependency between documents.
|
||||||
"""
|
"""
|
||||||
msg_fmt = ('Cannot determine substitution order as a dependency '
|
msg_fmt = ('Cannot determine substitution order as a dependency '
|
||||||
|
@ -338,14 +341,19 @@ class LayeringPolicyNotFound(DeckhandException):
|
||||||
code = 409
|
code = 409
|
||||||
|
|
||||||
|
|
||||||
class SubstitutionFailure(DeckhandException):
|
class SubstitutionSourceNotFound(DeckhandException):
|
||||||
"""An unknown error occurred during substitution.
|
"""Required substitution source document was not found for layering.
|
||||||
|
|
||||||
**Troubleshoot:**
|
**Troubleshoot:**
|
||||||
|
|
||||||
|
* Ensure that the missing source document being referenced exists in
|
||||||
|
the system or was passed to the layering module.
|
||||||
"""
|
"""
|
||||||
msg_fmt = ('An unknown exception occurred while trying to perform '
|
msg_fmt = (
|
||||||
'substitution. Details: %(detail)s')
|
"Required substitution source document [%(src_schema)s] %(src_name)s "
|
||||||
code = 400
|
"was not found, yet is referenced by [%(document_schema)s] "
|
||||||
|
"%(document_name)s.")
|
||||||
|
code = 409
|
||||||
|
|
||||||
|
|
||||||
class BarbicanException(DeckhandException):
|
class BarbicanException(DeckhandException):
|
||||||
|
@ -365,3 +373,13 @@ class PolicyNotAuthorized(DeckhandException):
|
||||||
"""
|
"""
|
||||||
msg_fmt = "Policy doesn't allow %(action)s to be performed."
|
msg_fmt = "Policy doesn't allow %(action)s to be performed."
|
||||||
code = 403
|
code = 403
|
||||||
|
|
||||||
|
|
||||||
|
class UnknownSubstitutionError(DeckhandException):
|
||||||
|
"""An unknown error occurred during substitution.
|
||||||
|
|
||||||
|
**Troubleshoot:**
|
||||||
|
"""
|
||||||
|
msg_fmt = ('An unknown exception occurred while trying to perform '
|
||||||
|
'substitution. Details: %(details)s')
|
||||||
|
code = 500
|
||||||
|
|
|
@ -25,9 +25,9 @@ class TestDocumentLayering(test_base.DeckhandTestCase):
|
||||||
|
|
||||||
def _test_layering(self, documents, site_expected=None,
|
def _test_layering(self, documents, site_expected=None,
|
||||||
region_expected=None, global_expected=None,
|
region_expected=None, global_expected=None,
|
||||||
substitution_sources=None):
|
substitution_sources=None, validate=False, **kwargs):
|
||||||
document_layering = layering.DocumentLayering(
|
document_layering = layering.DocumentLayering(
|
||||||
documents, substitution_sources)
|
documents, substitution_sources, validate=validate, **kwargs)
|
||||||
|
|
||||||
site_docs = []
|
site_docs = []
|
||||||
region_docs = []
|
region_docs = []
|
||||||
|
|
|
@ -113,3 +113,23 @@ class TestDocumentLayeringWithSubstitutionNegative(
|
||||||
self.assertRaises(
|
self.assertRaises(
|
||||||
errors.SubstitutionDependencyCycle, self._test_layering, documents,
|
errors.SubstitutionDependencyCycle, self._test_layering, documents,
|
||||||
substitution_sources=documents)
|
substitution_sources=documents)
|
||||||
|
|
||||||
|
def test_layering_with_missing_substitution_source_raises_exc(self):
|
||||||
|
"""Validate that a missing substitution source document fails."""
|
||||||
|
mapping = {
|
||||||
|
"_SITE_SUBSTITUTIONS_1_": [{
|
||||||
|
"dest": {
|
||||||
|
"path": ".c"
|
||||||
|
},
|
||||||
|
"src": {
|
||||||
|
"schema": "example/Kind/v1",
|
||||||
|
"name": "nowhere-to-be-found",
|
||||||
|
"path": "."
|
||||||
|
}
|
||||||
|
}]
|
||||||
|
}
|
||||||
|
doc_factory = factories.DocumentFactory(2, [1, 1])
|
||||||
|
documents = doc_factory.gen_test(mapping, site_abstract=False)
|
||||||
|
|
||||||
|
self.assertRaises(
|
||||||
|
errors.SubstitutionSourceNotFound, self._test_layering, documents)
|
||||||
|
|
|
@ -12,6 +12,8 @@
|
||||||
# See the License for the specific language governing permissions and
|
# See the License for the specific language governing permissions and
|
||||||
# limitations under the License.
|
# limitations under the License.
|
||||||
|
|
||||||
|
import copy
|
||||||
|
|
||||||
import mock
|
import mock
|
||||||
|
|
||||||
from deckhand.engine import layering
|
from deckhand.engine import layering
|
||||||
|
@ -200,3 +202,44 @@ class TestDocumentLayeringNegative(
|
||||||
|
|
||||||
self.assertRaises(
|
self.assertRaises(
|
||||||
errors.InvalidDocumentParent, self._test_layering, documents)
|
errors.InvalidDocumentParent, self._test_layering, documents)
|
||||||
|
|
||||||
|
|
||||||
|
class TestDocumentLayeringValidationNegative(
|
||||||
|
test_document_layering.TestDocumentLayering):
|
||||||
|
|
||||||
|
def test_layering_invalid_substitution_format_raises_exc(self):
|
||||||
|
doc_factory = factories.DocumentFactory(1, [1])
|
||||||
|
layering_policy, document_template = doc_factory.gen_test({
|
||||||
|
"_GLOBAL_SUBSTITUTIONS_1_": [{
|
||||||
|
"dest": {
|
||||||
|
"path": ".c"
|
||||||
|
},
|
||||||
|
"src": {
|
||||||
|
"schema": "deckhand/Certificate/v1",
|
||||||
|
"name": "global-cert",
|
||||||
|
"path": "."
|
||||||
|
}
|
||||||
|
|
||||||
|
}],
|
||||||
|
}, global_abstract=False)
|
||||||
|
|
||||||
|
for key in ('src', 'dest'):
|
||||||
|
document = copy.deepcopy(document_template)
|
||||||
|
del document['metadata']['substitutions'][0][key]
|
||||||
|
self.assertRaises(errors.InvalidDocumentFormat,
|
||||||
|
self._test_layering, [layering_policy, document],
|
||||||
|
validate=True)
|
||||||
|
|
||||||
|
for key in ('schema', 'name', 'path'):
|
||||||
|
document = copy.deepcopy(document_template)
|
||||||
|
del document['metadata']['substitutions'][0]['src'][key]
|
||||||
|
self.assertRaises(errors.InvalidDocumentFormat,
|
||||||
|
self._test_layering, [layering_policy, document],
|
||||||
|
validate=True)
|
||||||
|
|
||||||
|
for key in ('path',):
|
||||||
|
document = copy.deepcopy(document_template)
|
||||||
|
del document['metadata']['substitutions'][0]['dest'][key]
|
||||||
|
self.assertRaises(errors.InvalidDocumentFormat,
|
||||||
|
self._test_layering, [layering_policy, document],
|
||||||
|
validate=True)
|
||||||
|
|
|
@ -89,8 +89,18 @@ Deckhand Exceptions
|
||||||
:members:
|
:members:
|
||||||
:show-inheritance:
|
:show-inheritance:
|
||||||
:undoc-members:
|
:undoc-members:
|
||||||
* - SubstitutionFailure
|
* - SubstitutionDependencyCycle
|
||||||
- .. autoexception:: deckhand.errors.SubstitutionFailure
|
- .. autoexception:: deckhand.errors.SubstitutionDependencyCycle
|
||||||
|
:members:
|
||||||
|
:show-inheritance:
|
||||||
|
:undoc-members:
|
||||||
|
* - SubstitutionSourceNotFound
|
||||||
|
- .. autoexception:: deckhand.errors.SubstitutionSourceNotFound
|
||||||
|
:members:
|
||||||
|
:show-inheritance:
|
||||||
|
:undoc-members:
|
||||||
|
* - UnknownSubstitutionError
|
||||||
|
- .. autoexception:: deckhand.errors.UnknownSubstitutionError
|
||||||
:members:
|
:members:
|
||||||
:show-inheritance:
|
:show-inheritance:
|
||||||
:undoc-members:
|
:undoc-members:
|
||||||
|
|
Loading…
Reference in New Issue