From 964aed2973cca3e9b60933361f59798bf0d4cb96 Mon Sep 17 00:00:00 2001 From: Marshall Margenau Date: Thu, 15 Mar 2018 16:42:04 -0500 Subject: [PATCH] feat(validation) Validation messaging - Validation messaging to match UCP convention - Adding some missing fields to Chart validation schema - Minor update: Adding debug logging to each CLI call - Fixing some typos and exception messages Change-Id: I7dc1165432c8b3d138cabe6fd5f3a6e1878810ae --- armada/cli/apply.py | 4 +- armada/cli/delete.py | 10 ++- armada/cli/test.py | 6 +- armada/cli/tiller.py | 6 +- armada/cli/validate.py | 13 +++- armada/handlers/manifest.py | 10 +-- armada/schemas/armada-chart-schema.yaml | 8 +- armada/tests/unit/api/test_test_controller.py | 12 ++- armada/tests/unit/handlers/test_armada.py | 2 - armada/tests/unit/handlers/test_manifest.py | 4 +- armada/utils/source.py | 6 +- armada/utils/validate.py | 68 ++++++++++++----- armada/utils/validation_message.py | 73 +++++++++++++++++++ 13 files changed, 181 insertions(+), 41 deletions(-) create mode 100644 armada/utils/validation_message.py diff --git a/armada/cli/apply.py b/armada/cli/apply.py index dac91da1..f8ad4368 100644 --- a/armada/cli/apply.py +++ b/armada/cli/apply.py @@ -130,9 +130,7 @@ def apply_create(ctx, locations, api, disable_update_post, disable_update_pre, dry_run, enable_chart_cleanup, set, tiller_host, tiller_port, tiller_namespace, timeout, values, wait, target_manifest, debug): - if debug: - CONF.debug = debug - + CONF.debug = debug ApplyManifest(ctx, locations, api, disable_update_post, disable_update_pre, dry_run, enable_chart_cleanup, set, tiller_host, tiller_port, tiller_namespace, timeout, values, wait, diff --git a/armada/cli/delete.py b/armada/cli/delete.py index 4878829a..fbe33595 100644 --- a/armada/cli/delete.py +++ b/armada/cli/delete.py @@ -15,6 +15,7 @@ import yaml import click +from oslo_config import cfg from armada.cli import CliAction from armada import const @@ -22,6 +23,8 @@ from armada.handlers.manifest import Manifest from armada.handlers.tiller import Tiller from armada.utils.release import release_prefix +CONF = cfg.CONF + @click.group() def delete(): @@ -71,8 +74,13 @@ SHORT_DESC = "Command deletes releases." help="Tiller host port.", type=int, default=44134) +@click.option('--debug', + help="Enable debug logging.", + is_flag=True) @click.pass_context -def delete_charts(ctx, manifest, releases, no_purge, tiller_host, tiller_port): +def delete_charts(ctx, manifest, releases, no_purge, tiller_host, tiller_port, + debug): + CONF.debug = debug DeleteChartManifest(ctx, manifest, releases, no_purge, tiller_host, tiller_port).safe_invoke() diff --git a/armada/cli/test.py b/armada/cli/test.py index 01f2267c..07d0ff86 100644 --- a/armada/cli/test.py +++ b/armada/cli/test.py @@ -77,9 +77,13 @@ SHORT_DESC = "Command tests releases." help=("The target manifest to run. Required for specifying " "which manifest to run when multiple are available."), default=None) +@click.option('--debug', + help="Enable debug logging.", + is_flag=True) @click.pass_context def test_charts(ctx, file, release, tiller_host, tiller_port, tiller_namespace, - target_manifest): + target_manifest, debug): + CONF.debug = debug TestChartManifest( ctx, file, release, tiller_host, tiller_port, tiller_namespace, target_manifest).safe_invoke() diff --git a/armada/cli/tiller.py b/armada/cli/tiller.py index 307167da..9feecc24 100644 --- a/armada/cli/tiller.py +++ b/armada/cli/tiller.py @@ -67,9 +67,13 @@ SHORT_DESC = "Command gets Tiller information." @click.option('--status', help="Status of Armada services.", is_flag=True) +@click.option('--debug', + help="Enable debug logging.", + is_flag=True) @click.pass_context def tiller_service(ctx, tiller_host, tiller_port, tiller_namespace, releases, - status): + status, debug): + CONF.debug = debug TillerServices(ctx, tiller_host, tiller_port, tiller_namespace, releases, status).safe_invoke() diff --git a/armada/cli/validate.py b/armada/cli/validate.py index 72170a24..2133c069 100644 --- a/armada/cli/validate.py +++ b/armada/cli/validate.py @@ -14,11 +14,14 @@ import click import yaml +from oslo_config import cfg from armada.cli import CliAction from armada.utils.validate import validate_armada_documents from armada.handlers.document import ReferenceResolver +CONF = cfg.CONF + @click.group() def validate(): @@ -44,8 +47,12 @@ SHORT_DESC = "Command validates Armada Manifest." short_help=SHORT_DESC) @click.argument('locations', nargs=-1) +@click.option('--debug', + help="Enable debug logging.", + is_flag=True) @click.pass_context -def validate_manifest(ctx, locations): +def validate_manifest(ctx, locations, debug): + CONF.debug = debug ValidateManifest(ctx, locations).safe_invoke() @@ -65,7 +72,9 @@ class ValidateManifest(CliAction): try: valid, details = validate_armada_documents(documents) - if valid: + if not documents: + self.logger.warn('No documents to validate.') + elif valid: self.logger.info('Successfully validated: %s', self.locations) else: diff --git a/armada/handlers/manifest.py b/armada/handlers/manifest.py index b9819504..4bdbf6b6 100644 --- a/armada/handlers/manifest.py +++ b/armada/handlers/manifest.py @@ -71,7 +71,7 @@ class Manifest(object): def _find_documents(self, target_manifest=None): """Returns the chart documents, chart group documents, - and armada manifest + and Armada manifest If multiple documents with schema "armada/Manifest/v1" are provided, specify ``target_manifest`` to select the target one. @@ -203,10 +203,10 @@ class Manifest(object): return chart_group def build_armada_manifest(self): - """Builds the armmada manifest while pulling out data + """Builds the Armada manifest while pulling out data from the chart_group. - :returns: The armada manifest with the data of the chart groups. + :returns: The Armada manifest with the data of the chart groups. :rtype: dict :raises ManifestException: If a chart group's data listed under ``chart_group['data']`` could not be found. @@ -234,9 +234,9 @@ class Manifest(object): def get_manifest(self): """Builds all of the documents including the dependencies of the chart documents, the charts in the chart_groups, and the - armada manifest + Armada manifest - :returns: The armada manifest. + :returns: The Armada manifest. :rtype: dict """ self.build_charts_deps() diff --git a/armada/schemas/armada-chart-schema.yaml b/armada/schemas/armada-chart-schema.yaml index 8b31cf36..7e15e067 100644 --- a/armada/schemas/armada-chart-schema.yaml +++ b/armada/schemas/armada-chart-schema.yaml @@ -74,9 +74,11 @@ data: subpath: type: string reference: - type: - - string - - "null" + type: string + proxy_server: + type: string + auth_method: + type: string required: - location - subpath diff --git a/armada/tests/unit/api/test_test_controller.py b/armada/tests/unit/api/test_test_controller.py index 9c7a0532..90a7a757 100644 --- a/armada/tests/unit/api/test_test_controller.py +++ b/armada/tests/unit/api/test_test_controller.py @@ -138,7 +138,11 @@ class TestReleasesManifestControllerNegativeTest(base.BaseControllerTest): {'message': ( 'An error occurred while generating the manifest: Could not ' 'find dependency chart helm-toolkit in armada/Chart/v1.'), - 'error': True}, + 'error': True, + 'kind': 'ValidationMessage', + 'level': 'Error', + 'name': 'ARM001', + 'documents': []}, resp_body['details']['messageList']) self.assertEqual(('Failed to validate documents or generate Armada ' 'Manifest from documents.'), @@ -168,7 +172,11 @@ class TestReleasesManifestControllerNegativeTest(base.BaseControllerTest): self.assertEqual( [{'message': ( 'An error occurred while generating the manifest: foo.'), - 'error': True}], + 'error': True, + 'kind': 'ValidationMessage', + 'level': 'Error', + 'name': 'ARM001', + 'documents': []}], resp_body['details']['messageList']) self.assertEqual(('Failed to validate documents or generate Armada ' 'Manifest from documents.'), diff --git a/armada/tests/unit/handlers/test_armada.py b/armada/tests/unit/handlers/test_armada.py index b2d53963..c5ce8993 100644 --- a/armada/tests/unit/handlers/test_armada.py +++ b/armada/tests/unit/handlers/test_armada.py @@ -54,7 +54,6 @@ data: type: local location: /tmp/dummy/armada subpath: chart_2 - reference: null dependencies: [] timeout: 5 --- @@ -117,7 +116,6 @@ class ArmadaHandlerTestCase(base.ArmadaTestCase): 'release': 'test_chart_2', 'source': { 'location': '/tmp/dummy/armada', - 'reference': None, 'subpath': 'chart_2', 'type': 'local' }, diff --git a/armada/tests/unit/handlers/test_manifest.py b/armada/tests/unit/handlers/test_manifest.py index e0c9a403..71eb524c 100644 --- a/armada/tests/unit/handlers/test_manifest.py +++ b/armada/tests/unit/handlers/test_manifest.py @@ -191,7 +191,7 @@ class ManifestTestCase(testtools.TestCase): self.assertIsNotNone(built_armada_manifest) self.assertIsInstance(built_armada_manifest, dict) - # the first chart group in the armada manifest + # the first chart group in the Armada manifest keystone_infra_services_chart_group = armada_manifest. \ find_chart_group_document('keystone-infra-services') keystone_infra_services_chart_group_data = \ @@ -200,7 +200,7 @@ class ManifestTestCase(testtools.TestCase): self.assertEqual(keystone_infra_services_chart_group_data, built_armada_manifest['data']['chart_groups'][0]) - # the first chart group in the armada manifest + # the first chart group in the Armada manifest openstack_keystone_chart_group = armada_manifest. \ find_chart_group_document('openstack-keystone') openstack_keystone_chart_group_data = \ diff --git a/armada/utils/source.py b/armada/utils/source.py index 2fa348da..5a1b4929 100644 --- a/armada/utils/source.py +++ b/armada/utils/source.py @@ -163,7 +163,11 @@ def source_cleanup(git_path): LOG.warning('%s is not a valid git repository. Details: %s', git_path, e) else: - shutil.rmtree(git_path) + try: + shutil.rmtree(git_path) + except OSError as e: + LOG.warning('Could not delete the path %s. Details: %s', + git_path, e) else: LOG.warning('Could not delete the path %s. Is it a git repository?', git_path) diff --git a/armada/utils/validate.py b/armada/utils/validate.py index e05b20e2..5fad56c7 100644 --- a/armada/utils/validate.py +++ b/armada/utils/validate.py @@ -23,6 +23,7 @@ from oslo_log import log as logging from armada.const import KEYWORD_GROUPS, KEYWORD_CHARTS, KEYWORD_RELEASE from armada.handlers.manifest import Manifest from armada.exceptions.manifest_exceptions import ManifestException +from armada.utils.validation_message import ValidationMessage LOG = logging.getLogger(__name__) # Creates a mapping between ``metadata.name``: ``data`` where the @@ -65,14 +66,18 @@ def _validate_armada_manifest(manifest): the second value is the validation details with a minimum keyset of (message(str), error(bool)) :rtype: tuple. - """ details = [] try: armada_object = manifest.get_manifest().get('armada') except ManifestException as me: - details.append(dict(message=str(me), error=True)) + vmsg = ValidationMessage(message=str(me), + error=True, + name='ARM001', + level='Error') + LOG.error('ValidationMessage: %s', vmsg.get_output_json()) + details.append(vmsg.get_output()) return False, details groups = armada_object.get(KEYWORD_GROUPS) @@ -80,7 +85,12 @@ def _validate_armada_manifest(manifest): if not isinstance(groups, list): message = '{} entry is of wrong type: {} (expected: {})'.format( KEYWORD_GROUPS, type(groups), 'list') - details.append(dict(message=message, error=True)) + vmsg = ValidationMessage(message=message, + error=True, + name='ARM101', + level='Error') + LOG.info('ValidationMessage: %s', vmsg.get_output_json()) + details.append(vmsg.get_output()) for group in groups: for chart in group.get(KEYWORD_CHARTS): @@ -88,7 +98,12 @@ def _validate_armada_manifest(manifest): if KEYWORD_RELEASE not in chart_obj: message = 'Could not find {} keyword in {}'.format( KEYWORD_RELEASE, chart_obj.get('release')) - details.append(dict(message=message, error=True)) + vmsg = ValidationMessage(message=message, + error=True, + name='ARM102', + level='Error') + LOG.info('ValidationMessage: %s', vmsg.get_output_json()) + details.append(vmsg.get_output()) if len([x for x in details if x.get('error', False)]) > 0: return False, details @@ -108,8 +123,8 @@ def validate_armada_manifests(documents): for document in documents: if document.get('schema', '') == 'armada/Manifest/v1': target = document.get('metadata').get('name') - manifest = Manifest(documents, - target_manifest=target) + # TODO(MarshM) explore: why does this pass 'documents'? + manifest = Manifest(documents, target_manifest=target) is_valid, details = _validate_armada_manifest(manifest) all_valid = all_valid and is_valid messages.extend(details) @@ -138,28 +153,44 @@ def validate_armada_document(document): schema = document.get('schema', '') document_name = document.get('metadata', {}).get('name', None) details = [] + LOG.debug('Validating document [%s] %s', schema, document_name) if schema in SCHEMAS: try: validator = jsonschema.Draft4Validator(SCHEMAS[schema]) for error in validator.iter_errors(document.get('data')): - msg = "Invalid document [%s] %s: %s." % \ + error_message = "Invalid document [%s] %s: %s." % \ (schema, document_name, error.message) - details.append(dict(message=msg, - error=True, - doc_schema=schema, - doc_name=document_name)) + vmsg = ValidationMessage(message=error_message, + error=True, + name='ARM100', + level='Error', + schema=schema, + doc_name=document_name) + LOG.info('ValidationMessage: %s', vmsg.get_output_json()) + details.append(vmsg.get_output()) except jsonschema.SchemaError as e: error_message = ('The built-in Armada JSON schema %s is invalid. ' 'Details: %s.' % (e.schema, e.message)) - LOG.error(error_message) - details.append(dict(message=error_message, error=True)) + vmsg = ValidationMessage(message=error_message, + error=True, + name='ARM000', + level='Error', + diagnostic='Armada is misconfigured.') + LOG.error('ValidationMessage: %s', vmsg.get_output_json()) + details.append(vmsg.get_output()) else: - error_message = ( - 'Document [%s] %s is not supported.' % - (schema, document_name)) - LOG.info(error_message) - details.append(dict(message=error_message, error=False)) + vmsg = ValidationMessage(message='Unsupported document type.', + error=False, + name='ARM002', + level='Warning', + schema=schema, + doc_name=document_name, + diagnostic='Please ensure document is one of ' + 'the following schema types: %s' % + list(SCHEMAS.keys())) + LOG.info('ValidationMessage: %s', vmsg.get_output_json()) + # Validation API doesn't care about this type of message, don't send if len([x for x in details if x.get('error', False)]) > 0: return False, details @@ -202,6 +233,7 @@ def validate_manifest_url(value): return False +# TODO(MarshM) unused except in unit tests, is this useful? def validate_manifest_filepath(value): return os.path.isfile(value) diff --git a/armada/utils/validation_message.py b/armada/utils/validation_message.py new file mode 100644 index 00000000..2c36bd19 --- /dev/null +++ b/armada/utils/validation_message.py @@ -0,0 +1,73 @@ +# Copyright 2018 AT&T Intellectual Property. All other rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import json + + +class ValidationMessage(object): + """ ValidationMessage per UCP convention: + https://github.com/att-comdev/ucp-integration/blob/master/docs/source/api-conventions.rst#output-structure # noqa + + Construction of ValidationMessage message: + + :param string message: Validation failure message. + :param boolean error: True or False, if this is an error message. + :param string name: Identifying name of the validation. + :param string level: The severity of validation result, as "Error", + "Warning", or "Info" + :param string schema: The schema of the document being validated. + :param string doc_name: The name of the document being validated. + :param string diagnostic: Information about what lead to the message, + or details for resolution. + """ + + def __init__(self, + message='Document validation error.', + error=True, + name='Armada error', + level='Error', + schema=None, + doc_name=None, + diagnostic=None): + + # TODO(MarshM) should validate error and level inputs + + self.output = { + 'message': message, + 'error': error, + 'name': name, + 'documents': [], + 'level': level, + 'kind': 'ValidationMessage' + } + if schema and doc_name: + self.output['documents'].append(dict(schema=schema, name=doc_name)) + if diagnostic: + self.output.update(diagnostic=diagnostic) + + def get_output(self): + """ Return ValidationMessage message. + + :returns: The ValidationMessage for the Validation API response. + :rtype: dict + """ + return self.output + + def get_output_json(self): + """ Return ValidationMessage message as JSON. + + :returns: The ValidationMessage formatted in JSON, for logging. + :rtype: json + """ + return json.dumps(self.output, indent=2)