Merge "Validate additional 'metadata.replacement' scenarios"

This commit is contained in:
Zuul 2018-10-30 21:40:18 +00:00 committed by Gerrit Code Review
commit 265a5bf18f
6 changed files with 264 additions and 83 deletions

View File

@ -75,8 +75,50 @@ def check_only_one_level_of_replacement(src_ref):
# If the document has a replacement, use the replacement as the
# substitution source instead.
if src_ref.is_replacement:
error_message = ('A replacement document cannot itself'
' be replaced by another document.')
error_message = ('A replacement document cannot itself be replaced by '
'another document.')
raise errors.InvalidDocumentReplacement(
schema=src_ref.schema, name=src_ref.name,
layer=src_ref.layer, reason=error_message)
def check_replacement_is_false_uniqueness(
document, non_replacement_documents):
"""Validate uniqueness of ``replacement: false`` for each document
identifier.
This check essentially validates that each raw document (which is uniquely
defined by (name, schema, layer)) maps to a unique rendered document
(which is uniquely defined by (name, schema)). This can be done by ensuring
that each (name, schema) pair only has one occurrence of
``replacement: false``.
Normally, a ``replacement: true`` document nukes the ``replacement: false``
parent. But when > 1 ``replacement: false`` documents with same (name,
schema) exist, the raw document unique constraint predominates over the
rendered document unique constraint, resulting in a breakdown in the
rendering process, as confusion occurs over which document to reference
for substitution data and the like.
:param document: current document in the collection that is being processed
:param non_replacement_documents: a set containing tuples of the names and
schemas of all the non-replacement documents
"""
if not document.is_control:
document_identifier = (
document['metadata']['name'],
document['metadata']['schema']
)
if document_identifier in non_replacement_documents:
error_message = (
'Documents with the same name and schema existing in '
'different layers without any of them having '
'`replacement = true` cannot exist as Deckhand will '
'arbitrarily select any of them for processing and none are '
'distinguishable from one another because none are a '
'parent or child or a replacement document.')
raise errors.InvalidDocumentReplacement(
schema=document.schema, name=document.name,
layer=document.layer, reason=error_message)
else:
non_replacement_documents.add(document_identifier)

View File

@ -60,6 +60,10 @@ class DocumentLayering(object):
def _calc_replacements_and_substitutions(
self, substitution_sources):
# Used to track document names and schemas for documents that are not
# replacement documents
non_replacement_documents = set()
for document in self._documents_by_index.values():
parent_meta = self._parents.get(document.meta)
parent = self._documents_by_index.get(parent_meta)
@ -71,8 +75,13 @@ class DocumentLayering(object):
parent, document)
parent.replaced_by = document
else:
# Handles case where parent and child have replacement: false
# as in this case both documents should not be replacement
# documents, requiring them to have different schema/name pair.
replacement.check_child_and_parent_different_metadata_name(
parent, document)
replacement.check_replacement_is_false_uniqueness(
document, non_replacement_documents)
# Since a substitution source only provides the document's
# `metadata.name` and `schema`, their tuple acts as the dictionary key.

View File

@ -185,14 +185,14 @@ class TestValidationMessageFormatting(test_base.BaseControllerTest):
headers={'Content-Type': 'application/x-yaml'},
body=payload)
with mock.patch('deckhand.control.revision_documents.db_api'
'.revision_documents_get', autospec=True) \
as mock_get_rev_documents:
with mock.patch('deckhand.control.revision_documents.common'
'.get_rendered_docs', autospec=True) \
as mock_get_rendered_docs:
invalid_document = document_wrapper.DocumentDict(
yaml.safe_load(payload))
invalid_document.pop('metadata')
mock_get_rendered_docs.return_value = ([invalid_document], False)
mock_get_rev_documents.return_value = [invalid_document]
resp = self.app.simulate_get(
'/api/v1.0/revisions/1/rendered-documents',
headers={'Content-Type': 'application/x-yaml'})

View File

@ -12,12 +12,89 @@
# See the License for the specific language governing permissions and
# limitations under the License.
import inspect
import itertools
import os
import yaml
from deckhand.tests.unit.engine import test_document_layering
REPLACEMENT_3_TIER_SAMPLE = list(yaml.safe_load_all(inspect.cleandoc(
"""
---
schema: deckhand/LayeringPolicy/v1
metadata:
schema: metadata/Control/v1
name: layering-policy
storagePolicy: cleartext
data:
layerOrder:
- global
- region
- site
---
schema: armada/Chart/v1
metadata:
schema: metadata/Document/v1
name: nova-global
storagePolicy: cleartext
labels:
name: nova-global
component: nova
layeringDefinition:
abstract: false
layer: global
data:
values:
pod:
replicas:
server: 16
---
schema: armada/Chart/v1
metadata:
schema: metadata/Document/v1
name: nova
storagePolicy: cleartext
labels:
name: nova-5ec
component: nova
layeringDefinition:
abstract: false
layer: region
parentSelector:
name: nova-global
actions:
- method: merge
path: .
data: {}
---
schema: armada/Chart/v1
metadata:
schema: metadata/Document/v1
replacement: true
storagePolicy: cleartext
name: nova
layeringDefinition:
abstract: false
layer: site
parentSelector:
name: nova-5ec
actions:
- method: merge
path: .
data:
values:
pod:
replicas:
api_metadata: 16
placement: 2
osapi: 16
conductor: 16
consoleauth: 2
scheduler: 2
novncproxy: 2
""")))
class TestDocumentLayeringWithReplacement(
test_document_layering.TestDocumentLayering):
@ -266,82 +343,8 @@ data:
* Global document called nova-global
* Region document called nova (layers with nova-global)
* Site document (replaces nova)
* Site document (replaces region document)
"""
self.documents = list(yaml.safe_load_all("""
---
schema: deckhand/LayeringPolicy/v1
metadata:
schema: metadata/Control/v1
name: layering-policy
storagePolicy: cleartext
data:
layerOrder:
- global
- region
- site
---
schema: armada/Chart/v1
metadata:
schema: metadata/Document/v1
name: nova-global
storagePolicy: cleartext
labels:
name: nova-global
component: nova
layeringDefinition:
abstract: false
layer: global
data:
values:
pod:
replicas:
server: 16
---
schema: armada/Chart/v1
metadata:
schema: metadata/Document/v1
name: nova
storagePolicy: cleartext
labels:
name: nova-5ec
component: nova
layeringDefinition:
abstract: false
layer: region
parentSelector:
name: nova-global
actions:
- method: merge
path: .
data: {}
---
schema: armada/Chart/v1
metadata:
schema: metadata/Document/v1
replacement: true
storagePolicy: cleartext
name: nova
layeringDefinition:
abstract: false
layer: site
parentSelector:
name: nova-5ec
actions:
- method: merge
path: .
data:
values:
pod:
replicas:
api_metadata: 16
placement: 2
osapi: 16
conductor: 16
consoleauth: 2
scheduler: 2
novncproxy: 2
"""))
site_expected = [
{
@ -372,7 +375,56 @@ data:
}
}
]
self._test_layering(self.documents,
self._test_layering(REPLACEMENT_3_TIER_SAMPLE,
site_expected=site_expected,
region_expected=None,
global_expected=global_expected)
def test_multi_layer_replacement_with_intermediate_replacement(self):
"""Validate the following scenario:
* Global document called nova-replace
* Region document called nova-replace (layers with nova-replace and
replaces it)
* Site document (layers with region document)
"""
replacement_sample = list(REPLACEMENT_3_TIER_SAMPLE)
replacement_sample[1]['metadata']['name'] = 'nova-replace'
replacement_sample[2]['metadata']['name'] = 'nova-replace'
replacement_sample[2]['metadata']['replacement'] = True
replacement_sample[3]['metadata']['replacement'] = False
site_expected = [
{
"values": {
"pod": {
"replicas": {
"api_metadata": 16,
"placement": 2,
"osapi": 16,
"conductor": 16,
"consoleauth": 2,
"scheduler": 2,
"novncproxy": 2,
"server": 16
}
}
}
}
]
region_expected = [
{
"values": {
"pod": {
"replicas": {
"server": 16
}
}
}
}
]
self._test_layering(replacement_sample,
site_expected=site_expected,
region_expected=region_expected,
global_expected=None)

View File

@ -24,6 +24,10 @@ class TestDocumentLayeringReplacementNegative(
"""Validate that attempting to replace a child with its parent when
they don't have the same ``metadata.name`` and ``schema`` results in
exception.
global
|
site (replacement: true, incompatible with parent)
"""
doc_factory = factories.DocumentFactory(2, [1, 1])
documents = doc_factory.gen_test({})
@ -52,6 +56,10 @@ class TestDocumentLayeringReplacementNegative(
"""Validate that a non-replacement document (i.e. regular document
without `replacement: true`) cannot have the same schema/name as
another document.
global (replacement: false)
|
site (replacement: false)
"""
doc_factory = factories.DocumentFactory(2, [1, 1])
documents = doc_factory.gen_test({})
@ -68,6 +76,10 @@ class TestDocumentLayeringReplacementNegative(
def test_replacement_without_parent_raises_exc(self):
"""Validate that attempting to do replacement without a parent document
raises an exception.
None
|
site (replacement: true)
"""
doc_factory = factories.DocumentFactory(2, [1, 1])
documents = doc_factory.gen_test({})
@ -80,9 +92,35 @@ class TestDocumentLayeringReplacementNegative(
self.assertRaisesRegexp(errors.InvalidDocumentReplacement, error_re,
self._test_layering, documents)
def test_replacement_with_parent_replace_true_raises_exc(self):
"""Validate that a parent document with replacement: true necessarily
fails if it itself doesn't have a parent.
None
|
global (replacement: true)
|
site
"""
doc_factory = factories.DocumentFactory(2, [1, 1])
documents = doc_factory.gen_test({})
documents[1]['metadata']['replacement'] = True
error_re = (r'Document replacement requires that the document with '
'`replacement: true` have a parent.')
self.assertRaisesRegexp(errors.InvalidDocumentReplacement, error_re,
self._test_layering, documents)
def test_replacement_that_is_replaced_raises_exc(self):
"""Validate that attempting replace a replacement document raises an
"""Validate that attempting to replace a replacement document raises an
exception.
global
|
region (replacement: true)
|
site (replacement: true)
"""
doc_factory = factories.DocumentFactory(3, [1, 1, 1])
documents = doc_factory.gen_test({}, region_abstract=False,
@ -99,3 +137,25 @@ class TestDocumentLayeringReplacementNegative(
'another document.')
self.assertRaisesRegexp(errors.InvalidDocumentReplacement, error_re,
self._test_layering, documents)
def test_replacement_true_with_parent_replacement_true_raises_exc(self):
"""Validate that when documents have the same `metadata.name` and
`metadata.schema` existing in different layers without any of them
having `replacement = true` raises an exception
"""
doc_factory = factories.DocumentFactory(2, [1, 1])
documents = doc_factory.gen_test({})
for document in documents[1:]:
document['metadata']['name'] = 'foo'
document['schema'] = 'example/Kind/v1'
document['metadata']['replacement'] = False
if 'parentSelector' in document['metadata']['layeringDefinition']:
document['metadata']['layeringDefinition'].pop(
'parentSelector')
error_re = (r'Documents with the same name and schema existing in '
'different layers without any of them having '
'`replacement = true` cannot exist.*')
self.assertRaisesRegexp(errors.InvalidDocumentReplacement, error_re,
self._test_layering, documents)

View File

@ -80,6 +80,24 @@ The following result in validation errors:
* A replacement document cannot itself be replaced. That is, only one level
of replacement is allowed.
Here are the following possible scenarios regarding child and parent
replacement values:
+-------------------------------------------------------------+
| Child | Parent | Status |
+=============================================================+
| True | True | Throws InvalidDocumentReplacement exception|
+-------------------------------------------------------------+
| False | True | Throws InvalidDocumentReplacement exception|
+-------------------------------------------------------------+
| True | False | Valid scenario |
+-------------------------------------------------------------+
| False | False | Throws InvalidDocumentReplacement exception|
+-------------------------------------------------------------+
Examples
--------
Note that each key in the examples below is *mandatory* and that the
``parentSelector`` labels should be able to select the parent to be replaced.