From e4a4d2ea68e53b635651b1595147be5af2889052 Mon Sep 17 00:00:00 2001 From: Felipe Monteiro Date: Fri, 23 Feb 2018 20:44:00 +0000 Subject: [PATCH] Add validation logic to Test endpoints This PS adds validation logic recently implemented in armada.utils.validate [0] for validating documents and Armada-generated Manifests to the Test and Tests controller classes. Also refactors some exception handling for both controller classes to better bubble up the appropriate exception. Finally unit tests have been added for the Armada Test controller to verify above changes work. [0] https://review.gerrithub.io/#/c/378700/ Change-Id: I01f73c1778bf7c2e38032d5fddabd327c013edbb --- armada/api/__init__.py | 19 +- armada/api/controller/test.py | 213 +++++++++++------- armada/exceptions/base_exception.py | 4 +- armada/tests/unit/api/test_test_controller.py | 113 ++++++++++ .../exceptions/guide-exceptions.rst | 2 +- ...{k8s-exceptions.rst => k8s-exceptions.inc} | 4 +- ...exceptions.inc => validate-exceptions.inc} | 22 +- entrypoint.sh | 2 +- 8 files changed, 278 insertions(+), 101 deletions(-) rename docs/source/operations/exceptions/{k8s-exceptions.rst => k8s-exceptions.inc} (92%) rename docs/source/operations/exceptions/{lint-exceptions.inc => validate-exceptions.inc} (61%) diff --git a/armada/api/__init__.py b/armada/api/__init__.py index db6df8cb..895fb2bf 100644 --- a/armada/api/__init__.py +++ b/armada/api/__init__.py @@ -17,6 +17,8 @@ import logging as log import uuid import yaml +from oslo_utils import excutils + import falcon from oslo_config import cfg from oslo_log import log as logging @@ -41,23 +43,22 @@ class BaseResource(object): resp.headers['Allow'] = ','.join(allowed_methods) resp.status = falcon.HTTP_200 - def req_yaml(self, req): + def req_yaml(self, req, default=None): if req.content_length is None or req.content_length == 0: - return None + return default raw_body = req.stream.read(req.content_length or 0) if raw_body is None: - return None + return default try: - return yaml.safe_load_all(raw_body.decode('utf-8')) + return list(yaml.safe_load_all(raw_body.decode('utf-8'))) except yaml.YAMLError as jex: - self.error( - req.context, - "Invalid YAML in request: \n%s" % raw_body.decode('utf-8')) - raise Exception( - "%s: Invalid YAML in body: %s" % (req.path, jex)) + with excutils.save_and_reraise_exception(): + self.error( + req.context, + "Invalid YAML in request: \n%s" % raw_body.decode('utf-8')) def req_json(self, req): if req.content_length is None or req.content_length == 0: diff --git a/armada/api/controller/test.py b/armada/api/controller/test.py index c76b678c..91cc2c96 100644 --- a/armada/api/controller/test.py +++ b/armada/api/controller/test.py @@ -13,6 +13,7 @@ # limitations under the License. import json +import yaml import falcon from oslo_config import cfg @@ -23,19 +24,20 @@ from armada import const from armada.handlers.tiller import Tiller from armada.handlers.manifest import Manifest from armada.utils.release import release_prefix +from armada.utils import validate CONF = cfg.CONF class Test(api.BaseResource): ''' - Test helm releases via release name + Test Helm releases via release name. ''' @policy.enforce('armada:test_release') def on_get(self, req, resp, release): + self.logger.info('RUNNING: %s', release) try: - self.logger.info('RUNNING: %s', release) tiller = Tiller( tiller_host=req.get_param('tiller_host'), tiller_port=req.get_param_as_int( @@ -43,45 +45,87 @@ class Test(api.BaseResource): tiller_namespace=req.get_param( 'tiller_namespace', default=CONF.tiller_namespace)) tiller_resp = tiller.testing_release(release) - msg = { - 'result': '', - 'message': '' - } - - if tiller_resp: - test_status = getattr( - tiller_resp.info.status, 'last_test_suite_run', 'FAILED') - - if test_status.result[0].status: - msg['result'] = 'PASSED: {}'.format(release) - msg['message'] = 'MESSAGE: Test Pass' - self.logger.info(msg) - else: - msg['result'] = 'FAILED: {}'.format(release) - msg['message'] = 'MESSAGE: Test Fail' - self.logger.info(msg) - else: - msg['result'] = 'FAILED: {}'.format(release) - msg['message'] = 'MESSAGE: No test found' - - resp.body = json.dumps(msg) - resp.status = falcon.HTTP_200 - resp.content_type = 'application/json' - + # TODO(fmontei): Provide more sensible exception(s) here. except Exception as e: err_message = 'Failed to test {}: {}'.format(release, e) self.error(req.context, err_message) - self.return_error( + return self.return_error( resp, falcon.HTTP_500, message=err_message) + msg = { + 'result': '', + 'message': '' + } + + if tiller_resp: + test_status = getattr( + tiller_resp.info.status, 'last_test_suite_run', 'FAILED') + + if test_status.result[0].status: + msg['result'] = 'PASSED: {}'.format(release) + msg['message'] = 'MESSAGE: Test Pass' + self.logger.info(msg) + else: + msg['result'] = 'FAILED: {}'.format(release) + msg['message'] = 'MESSAGE: Test Fail' + self.logger.info(msg) + else: + msg['result'] = 'FAILED: {}'.format(release) + msg['message'] = 'MESSAGE: No test found' + + resp.body = json.dumps(msg) + resp.status = falcon.HTTP_200 + resp.content_type = 'application/json' + class Tests(api.BaseResource): ''' - Test helm releases via a manifest + Test Helm releases via a Manifest. ''' + def _format_validation_response(self, req, resp, result, details): + resp.content_type = 'application/json' + resp_body = { + 'kind': 'Status', + 'apiVersion': 'v1.0', + 'metadata': {}, + 'reason': 'Validation', + 'details': {}, + } + + error_details = [m for m in details if m.get('error', False)] + + resp_body['details']['errorCount'] = len(error_details) + resp_body['details']['messageList'] = details + + if result: + resp.status = falcon.HTTP_200 + resp_body['status'] = 'Success' + resp_body['message'] = 'Armada validations succeeded.' + resp_body['code'] = 200 + else: + resp.status = falcon.HTTP_400 + resp_body['status'] = 'Failure' + resp_body['message'] = ( + 'Failed to validate documents or generate Armada Manifest ' + 'from documents.') + resp_body['code'] = 400 + self.error(req.context, resp_body['message']) + + resp.body = json.dumps(resp_body) + return result + + def _validate_documents(self, req, resp, documents): + result, details = validate.validate_armada_documents(documents) + return self._format_validation_response(req, resp, result, + details) + @policy.enforce('armada:tests_manifest') def on_post(self, req, resp): + # TODO(fmontei): Validation Content-Type is application/x-yaml. + + target_manifest = req.get_param('target_manifest', None) + try: tiller = Tiller( tiller_host=req.get_param('tiller_host'), @@ -89,57 +133,66 @@ class Tests(api.BaseResource): 'tiller_port') or CONF.tiller_port, tiller_namespace=req.get_param( 'tiller_namespace', default=CONF.tiller_namespace)) - - documents = self.req_yaml(req) - target_manifest = req.get_param('target_manifest', None) - armada_obj = Manifest( - documents, target_manifest=target_manifest).get_manifest() - prefix = armada_obj.get(const.KEYWORD_ARMADA).get( - const.KEYWORD_PREFIX) - known_releases = [release[0] for release in tiller.list_charts()] - - message = { - 'tests': { - 'passed': [], - 'skipped': [], - 'failed': [] - } - } - - for group in armada_obj.get(const.KEYWORD_ARMADA).get( - const.KEYWORD_GROUPS): - for ch in group.get(const.KEYWORD_CHARTS): - release_name = release_prefix( - prefix, ch.get('chart').get('chart_name')) - - if release_name in known_releases: - self.logger.info('RUNNING: %s tests', release_name) - resp = tiller.testing_release(release_name) - - if not resp: - continue - - test_status = getattr( - resp.info.status, 'last_test_suite_run', - 'FAILED') - if test_status.results[0].status: - self.logger.info("PASSED: %s", release_name) - message['test']['passed'].append(release_name) - else: - self.logger.info("FAILED: %s", release_name) - message['test']['failed'].append(release_name) - else: - self.logger.info( - 'Release %s not found - SKIPPING', release_name) - message['test']['skipped'].append(release_name) - - resp.status = falcon.HTTP_200 - - resp.body = json.dumps(message) - resp.content_type = 'application/json' - + # TODO(fmontei): Provide more sensible exception(s) here. except Exception as e: - err_message = 'Failed to test manifest: {}'.format(e) + err_message = 'Failed to initialize Tiller handler.' self.error(req.context, err_message) - self.return_error( + return self.return_error( resp, falcon.HTTP_500, message=err_message) + + try: + documents = self.req_yaml(req, default=[]) + except yaml.YAMLError: + err_message = 'Documents must be valid YAML.' + return self.return_error( + resp, falcon.HTTP_400, message=err_message) + + is_valid = self._validate_documents(req, resp, documents) + if not is_valid: + return resp + + armada_obj = Manifest( + documents, target_manifest=target_manifest).get_manifest() + + prefix = armada_obj.get(const.KEYWORD_ARMADA).get( + const.KEYWORD_PREFIX) + known_releases = [release[0] for release in tiller.list_charts()] + + message = { + 'tests': { + 'passed': [], + 'skipped': [], + 'failed': [] + } + } + + for group in armada_obj.get(const.KEYWORD_ARMADA).get( + const.KEYWORD_GROUPS): + for ch in group.get(const.KEYWORD_CHARTS): + release_name = release_prefix( + prefix, ch.get('chart').get('chart_name')) + + if release_name in known_releases: + self.logger.info('RUNNING: %s tests', release_name) + resp = tiller.testing_release(release_name) + + if not resp: + continue + + test_status = getattr( + resp.info.status, 'last_test_suite_run', + 'FAILED') + if test_status.results[0].status: + self.logger.info("PASSED: %s", release_name) + message['test']['passed'].append(release_name) + else: + self.logger.info("FAILED: %s", release_name) + message['test']['failed'].append(release_name) + else: + self.logger.info( + 'Release %s not found - SKIPPING', release_name) + message['test']['skipped'].append(release_name) + + resp.status = falcon.HTTP_200 + resp.body = json.dumps(message) + resp.content_type = 'application/json' diff --git a/armada/exceptions/base_exception.py b/armada/exceptions/base_exception.py index 67ba2310..d345dd32 100644 --- a/armada/exceptions/base_exception.py +++ b/armada/exceptions/base_exception.py @@ -29,9 +29,9 @@ class ArmadaBaseException(Exception): def __init__(self, message=None, **kwargs): self.message = message or self.message - try: + try: # nosec self.message = self.message % kwargs - except TypeError: + except Exception: pass super(ArmadaBaseException, self).__init__(self.message) diff --git a/armada/tests/unit/api/test_test_controller.py b/armada/tests/unit/api/test_test_controller.py index c28bcf60..38db8f30 100644 --- a/armada/tests/unit/api/test_test_controller.py +++ b/armada/tests/unit/api/test_test_controller.py @@ -12,11 +12,124 @@ # See the License for the specific language governing permissions and # limitations under the License. +import json +import os +import yaml + +import mock + +from armada.api.controller import test from armada.common.policies import base as policy_base +from armada.exceptions import manifest_exceptions from armada.tests import test_utils from armada.tests.unit.api import base +class TestControllerTest(base.BaseControllerTest): + + @mock.patch.object(test, 'Manifest') + @mock.patch.object(test, 'Tiller') + def test_test_controller_with_manifest(self, mock_tiller, mock_manifest): + rules = {'armada:tests_manifest': '@'} + self.policy.set_rules(rules) + + manifest_path = os.path.join(os.getcwd(), 'examples', + 'keystone-manifest.yaml') + with open(manifest_path, 'r') as f: + payload = f.read() + documents = list(yaml.safe_load_all(payload)) + + resp = self.app.simulate_post('/api/v1.0/tests', body=payload) + self.assertEqual(200, resp.status_code) + + result = json.loads(resp.text) + expected = { + "tests": {"passed": [], "skipped": [], "failed": []} + } + self.assertEqual(expected, result) + + mock_manifest.assert_called_once_with( + documents, target_manifest=None) + self.assertTrue(mock_tiller.called) + + +@test_utils.attr(type=['negative']) +class TestControllerNegativeTest(base.BaseControllerTest): + + @mock.patch.object(test, 'Manifest') + @mock.patch.object(test, 'Tiller') + def test_test_controller_tiller_exc_returns_500(self, mock_tiller, _): + rules = {'armada:tests_manifest': '@'} + self.policy.set_rules(rules) + + mock_tiller.side_effect = Exception + + resp = self.app.simulate_post('/api/v1.0/tests') + self.assertEqual(500, resp.status_code) + + @mock.patch.object(test, 'Manifest') + @mock.patch.object(test, 'Tiller') + def test_test_controller_validation_failure_returns_400( + self, *_): + rules = {'armada:tests_manifest': '@'} + self.policy.set_rules(rules) + + manifest_path = os.path.join(os.getcwd(), 'examples', + 'keystone-manifest.yaml') + with open(manifest_path, 'r') as f: + payload = f.read() + + documents = list(yaml.safe_load_all(payload)) + documents[0]['schema'] = 'totally-invalid' + invalid_payload = yaml.safe_dump_all(documents) + + resp = self.app.simulate_post('/api/v1.0/tests', body=invalid_payload) + self.assertEqual(400, resp.status_code) + + resp_body = json.loads(resp.text) + self.assertEqual(400, resp_body['code']) + self.assertEqual(1, resp_body['details']['errorCount']) + self.assertIn( + {'message': ( + 'An error occurred while generating the manifest: Could not ' + 'find dependency chart helm-toolkit in armada/Chart/v1.'), + 'error': True}, + resp_body['details']['messageList']) + self.assertEqual(('Failed to validate documents or generate Armada ' + 'Manifest from documents.'), + resp_body['message']) + + @mock.patch('armada.utils.validate.Manifest') + @mock.patch.object(test, 'Tiller') + def test_test_controller_manifest_failure_returns_400( + self, _, mock_manifest): + rules = {'armada:tests_manifest': '@'} + self.policy.set_rules(rules) + + mock_manifest.return_value.get_manifest.side_effect = ( + manifest_exceptions.ManifestException(details='foo')) + + manifest_path = os.path.join(os.getcwd(), 'examples', + 'keystone-manifest.yaml') + with open(manifest_path, 'r') as f: + payload = f.read() + + resp = self.app.simulate_post('/api/v1.0/tests', body=payload) + self.assertEqual(400, resp.status_code) + + resp_body = json.loads(resp.text) + self.assertEqual(400, resp_body['code']) + self.assertEqual(1, resp_body['details']['errorCount']) + self.assertEqual( + [{'message': ( + 'An error occurred while generating the manifest: foo.'), + 'error': True}], + resp_body['details']['messageList']) + self.assertEqual(('Failed to validate documents or generate Armada ' + 'Manifest from documents.'), + resp_body['message']) + + class TestControllerNegativeRbacTest(base.BaseControllerTest): @test_utils.attr(type=['negative']) diff --git a/docs/source/operations/exceptions/guide-exceptions.rst b/docs/source/operations/exceptions/guide-exceptions.rst index 5b58e631..a7083474 100644 --- a/docs/source/operations/exceptions/guide-exceptions.rst +++ b/docs/source/operations/exceptions/guide-exceptions.rst @@ -22,8 +22,8 @@ Armada Exceptions .. include:: base-exceptions.inc .. include:: chartbuilder-exceptions.inc .. include:: k8s-exceptions.inc -.. include:: lint-exceptions.inc .. include:: manifest-exceptions.inc .. include:: override-exceptions.inc .. include:: source-exceptions.inc .. include:: tiller-exceptions.inc +.. include:: validate-exceptions.inc diff --git a/docs/source/operations/exceptions/k8s-exceptions.rst b/docs/source/operations/exceptions/k8s-exceptions.inc similarity index 92% rename from docs/source/operations/exceptions/k8s-exceptions.rst rename to docs/source/operations/exceptions/k8s-exceptions.inc index d5a4ef8e..187c0428 100644 --- a/docs/source/operations/exceptions/k8s-exceptions.rst +++ b/docs/source/operations/exceptions/k8s-exceptions.inc @@ -26,8 +26,8 @@ :members: :show-inheritance: :undoc-members: - * - KubernetesUnknownStreamngEventTypeException - - .. autoexception:: armada.exceptions.k8s_exceptions.KubernetesUnknownStreamngEventTypeException + * - KubernetesUnknownStreamingEventTypeException + - .. autoexception:: armada.exceptions.k8s_exceptions.KubernetesUnknownStreamingEventTypeException :members: :show-inheritance: :undoc-members: diff --git a/docs/source/operations/exceptions/lint-exceptions.inc b/docs/source/operations/exceptions/validate-exceptions.inc similarity index 61% rename from docs/source/operations/exceptions/lint-exceptions.inc rename to docs/source/operations/exceptions/validate-exceptions.inc index 095ab254..f312b72e 100644 --- a/docs/source/operations/exceptions/lint-exceptions.inc +++ b/docs/source/operations/exceptions/validate-exceptions.inc @@ -20,13 +20,23 @@ * - Exception Name - Description - * - InvalidArmadaObjectException - - .. autoexception:: armada.exceptions.lint_exceptions.InvalidArmadaObjectException - :members: - :show-inheritance: - :undoc-members: * - InvalidManifestException - - .. autoexception:: armada.exceptions.lint_exceptions.InvalidManifestException + - .. autoexception:: armada.exceptions.validate_exceptions.InvalidManifestException + :members: + :show-inheritance: + :undoc-members: + * - InvalidChartDefinitionException + - .. autoexception:: armada.exceptions.validate_exceptions.InvalidChartDefinitionException + :members: + :show-inheritance: + :undoc-members: + * - InvalidReleaseException + - .. autoexception:: armada.exceptions.validate_exceptions.InvalidReleaseException + :members: + :show-inheritance: + :undoc-members: + * - InvalidArmadaObjectException + - .. autoexception:: armada.exceptions.validate_exceptions.InvalidArmadaObjectException :members: :show-inheritance: :undoc-members: diff --git a/entrypoint.sh b/entrypoint.sh index c483bfa5..099085b6 100755 --- a/entrypoint.sh +++ b/entrypoint.sh @@ -19,7 +19,7 @@ set -ex CMD="armada" # Define port -PORT=${ARMADA_API_PORT:-9000} +PORT=${ARMADA_API_PORT:-8000} # How long uWSGI should wait for each Armada response ARMADA_API_TIMEOUT=${ARMADA_API_TIMEOUT:-"3600"} # Number of uWSGI workers to handle API requests