From ac41f94d11cee8c7ed94152f159d3e0ad2a755a6 Mon Sep 17 00:00:00 2001 From: Renat Akhmerov Date: Fri, 20 Sep 2019 12:03:46 +0700 Subject: [PATCH] Add an ability to disable workflow text validation * For the sake of the service performance, it may make sense to disable validation of the workflow language syntax if it is affordable for a particular use case. For example, if all workflows are auto-generated by a 3rd party system and tested thoroughly (either by running them with Mistral or at least validating them via the special Mistral endpoint) then we can safely disable validation of the language syntax when uploading workflow definitions. For production systems it makes a big difference if workflow texts are large (thousands of tasks). This patch adds the boolean parameter "skip_validation" for API requests like "POST /v2/workflows" to disable validation, if needed, and the new configuration property "validation_mode" to set a desired validation mode. The option is an enumeration and has the following valid values: 1) "enabled" - enabled for all API requests unless it's explicitly disabled in the request itself 2) "mandatory" - enabled for all API requests regardless of the flag in the request 3) "disabled" - disabled for all API requrests regardless of the flag in the request "mandatory" is choosen as the default value for this new property to keep compatibility with the previous versions. * Minor style changes. Closes-Bug: #1844242 Change-Id: Ib509653d38254954f8449be3031457e5f636ccf2 --- mistral/api/controllers/v2/member.py | 2 + mistral/api/controllers/v2/workbook.py | 17 ++- mistral/api/controllers/v2/workflow.py | 26 ++++- mistral/config.py | 11 ++ mistral/lang/base.py | 4 +- mistral/lang/parser.py | 4 +- mistral/services/__init__.py | 29 +++++ mistral/services/workbooks.py | 21 ++-- mistral/services/workflows.py | 15 ++- mistral/tests/unit/api/v2/test_workbooks.py | 25 +++++ mistral/tests/unit/api/v2/test_workflows.py | 33 +++++- .../unit/services/test_workflow_service.py | 106 +++++++++++++++--- .../add_skip_validation-9e8b906c45bdb89f.yaml | 18 +++ 13 files changed, 274 insertions(+), 37 deletions(-) create mode 100644 releasenotes/notes/add_skip_validation-9e8b906c45bdb89f.yaml diff --git a/mistral/api/controllers/v2/member.py b/mistral/api/controllers/v2/member.py index b77c4d7c3..7dc0c805f 100644 --- a/mistral/api/controllers/v2/member.py +++ b/mistral/api/controllers/v2/member.py @@ -39,6 +39,7 @@ def auth_enable_check(func): msg = ("Resource sharing feature can only be supported with " "authentication enabled.") raise exc.WorkflowException(msg) + return func(*args, **kwargs) return wrapped @@ -68,6 +69,7 @@ class MembersController(rest.RestController): # Use retries to prevent possible failures. r = rest_utils.create_db_retry_object() + member_db = r.call( db_api.get_resource_member, self.resource_id, diff --git a/mistral/api/controllers/v2/workbook.py b/mistral/api/controllers/v2/workbook.py index b083b5e3f..039974498 100644 --- a/mistral/api/controllers/v2/workbook.py +++ b/mistral/api/controllers/v2/workbook.py @@ -68,6 +68,9 @@ class WorkbooksController(rest.RestController, hooks.HookController): """Update a workbook. :param namespace: Optional. Namespace of workbook to update. + :param validate: Optional. If set to False, disables validation of + the workflow YAML definition syntax, but only if allowed in the + service configuration. By default, validation is enabled. """ acl.enforce('workbooks:update', context.ctx()) @@ -75,6 +78,10 @@ class WorkbooksController(rest.RestController, hooks.HookController): definition = pecan.request.text scope = pecan.request.GET.get('scope', 'private') + # If "skip_validation" is present in the query string parameters + # then workflow language validation will be disabled. + skip_validation = 'skip_validation' in pecan.request.GET + resources.Workbook.validate_scope(scope) LOG.debug("Update workbook [definition=%s]", definition) @@ -83,7 +90,8 @@ class WorkbooksController(rest.RestController, hooks.HookController): workbooks.update_workbook_v2)( definition, namespace=namespace, - scope=scope + scope=scope, + validate=not skip_validation ) return resources.Workbook.from_db_model(wb_db).to_json() @@ -102,6 +110,10 @@ class WorkbooksController(rest.RestController, hooks.HookController): definition = pecan.request.text scope = pecan.request.GET.get('scope', 'private') + # If "skip_validation" is present in the query string parameters + # then workflow language validation will be disabled. + skip_validation = 'skip_validation' in pecan.request.GET + resources.Workbook.validate_scope(scope) LOG.debug("Create workbook [definition=%s]", definition) @@ -110,7 +122,8 @@ class WorkbooksController(rest.RestController, hooks.HookController): workbooks.create_workbook_v2)( definition, namespace=namespace, - scope=scope + scope=scope, + validate=not skip_validation ) pecan.response.status = 201 diff --git a/mistral/api/controllers/v2/workflow.py b/mistral/api/controllers/v2/workflow.py index aa8eae6ed..51b8ba24f 100644 --- a/mistral/api/controllers/v2/workflow.py +++ b/mistral/api/controllers/v2/workflow.py @@ -113,10 +113,20 @@ class WorkflowsController(rest.RestController, hooks.HookController): """ acl.enforce('workflows:update', context.ctx()) + # NOTE(rakhmerov): We can't use normal method arguments to access + # request data because it will break dynamic sub controller lookup + # functionality (see _lookup() above) so we have to get the data + # directly from the request object. + definition = pecan.request.text scope = pecan.request.GET.get('scope', 'private') + # If "skip_validation" is present in the query string parameters + # then workflow language validation will be disabled. + skip_validation = 'skip_validation' in pecan.request.GET + resources.Workflow.validate_scope(scope) + if scope == 'public': acl.enforce('workflows:publicize', context.ctx()) @@ -126,7 +136,8 @@ class WorkflowsController(rest.RestController, hooks.HookController): definition, scope=scope, identifier=identifier, - namespace=namespace + namespace=namespace, + validate=not skip_validation ) workflow_list = [ @@ -150,8 +161,18 @@ class WorkflowsController(rest.RestController, hooks.HookController): """ acl.enforce('workflows:create', context.ctx()) + # NOTE(rakhmerov): We can't use normal method arguments to access + # request data because it will break dynamic sub controller lookup + # functionality (see _lookup() above) so we have to get the data + # directly from the request object. + definition = pecan.request.text scope = pecan.request.GET.get('scope', 'private') + + # If "skip_validation" is present in the query string parameters + # then workflow language validation will be disabled. + skip_validation = 'skip_validation' in pecan.request.GET + pecan.response.status = 201 resources.Workflow.validate_scope(scope) @@ -164,7 +185,8 @@ class WorkflowsController(rest.RestController, hooks.HookController): db_wfs = rest_utils.rest_retry_on_db_error(workflows.create_workflows)( definition, scope=scope, - namespace=namespace + namespace=namespace, + validate=not skip_validation ) workflow_list = [ diff --git a/mistral/config.py b/mistral/config.py index 6f0282afb..5a592004a 100644 --- a/mistral/config.py +++ b/mistral/config.py @@ -87,6 +87,17 @@ api_opts = [ help=_('Number of workers for Mistral API service ' 'default is equal to the number of CPUs available if that can ' 'be determined, else a default worker count of 1 is returned.') + ), + cfg.StrOpt( + 'validation_mode', + default='mandatory', + choices=['enabled', 'mandatory', 'disabled'], + help=_("Defines in what cases Mistral will be validating the syntax " + "of workflow YAML definitions. If 'enabled' is set the service " + "will be validating the syntax but only if it's not explicitly " + "turned off in the API request. 'disabled' disables validation " + "for all API requests. 'mandatory' enables validation for all " + "API requests.") ) ] diff --git a/mistral/lang/base.py b/mistral/lang/base.py index a1393bdbd..ba26d6388 100644 --- a/mistral/lang/base.py +++ b/mistral/lang/base.py @@ -66,8 +66,8 @@ def instantiate_spec(spec_cls, data, validate=False): class that needs to be instantiated. :param data: Raw specification data as a dictionary. :type data: dict - :param validate: If it equals False then semantics and schema validation - will be skipped + :param validate: If it's False then semantics and schema validation + will be skipped. :type validate: bool """ diff --git a/mistral/lang/parser.py b/mistral/lang/parser.py index 0af0bf9b9..eadd814f1 100644 --- a/mistral/lang/parser.py +++ b/mistral/lang/parser.py @@ -136,7 +136,9 @@ def get_workflow_spec(spec_dict): def get_workflow_list_spec(spec_dict, validate): return base.instantiate_spec( - wf_v2.WorkflowListSpec, spec_dict, validate + wf_v2.WorkflowListSpec, + spec_dict, + validate ) diff --git a/mistral/services/__init__.py b/mistral/services/__init__.py index e69de29bb..3da904525 100644 --- a/mistral/services/__init__.py +++ b/mistral/services/__init__.py @@ -0,0 +1,29 @@ +# Copyright 2019 - Nokia Corporation +# +# 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. + +from oslo_config import cfg + + +def is_validation_enabled(validate): + validation_mode = cfg.CONF.api.validation_mode + + if validation_mode == 'mandatory': + result = True + elif validation_mode == 'disabled': + result = False + else: + # validation_mode = 'enabled' + result = validate + + return result diff --git a/mistral/services/workbooks.py b/mistral/services/workbooks.py index 77fafebc6..34b7ba904 100644 --- a/mistral/services/workbooks.py +++ b/mistral/services/workbooks.py @@ -14,12 +14,15 @@ from mistral.db.v2 import api as db_api_v2 from mistral.lang import parser as spec_parser +from mistral import services from mistral.services import actions -def create_workbook_v2(definition, namespace='', scope='private'): +def create_workbook_v2(definition, namespace='', scope='private', + validate=True): wb_spec = spec_parser.get_workbook_spec_from_yaml( - definition, validate=True + definition, + validate=services.is_validation_enabled(validate) ) wb_values = _get_workbook_values( @@ -37,9 +40,11 @@ def create_workbook_v2(definition, namespace='', scope='private'): return wb_db -def update_workbook_v2(definition, namespace='', scope='private'): +def update_workbook_v2(definition, namespace='', scope='private', + validate=True): wb_spec = spec_parser.get_workbook_spec_from_yaml( - definition, validate=True + definition, + validate=services.is_validation_enabled(validate) ) values = _get_workbook_values(wb_spec, definition, scope, namespace) @@ -55,9 +60,11 @@ def update_workbook_v2(definition, namespace='', scope='private'): def _on_workbook_update(wb_db, wb_spec, namespace): # TODO(hardikj) Handle actions for namespace db_actions = _create_or_update_actions(wb_db, wb_spec.get_actions()) - db_wfs = _create_or_update_workflows(wb_db, - wb_spec.get_workflows(), - namespace) + db_wfs = _create_or_update_workflows( + wb_db, + wb_spec.get_workflows(), + namespace + ) return db_actions, db_wfs diff --git a/mistral/services/workflows.py b/mistral/services/workflows.py index a4ef77362..e09c060db 100644 --- a/mistral/services/workflows.py +++ b/mistral/services/workflows.py @@ -11,11 +11,13 @@ # 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 yaml from mistral.db.v2 import api as db_api from mistral import exceptions as exc from mistral.lang import parser as spec_parser +from mistral import services from mistral.workflow import states from mistral_lib import utils from oslo_log import log as logging @@ -56,13 +58,14 @@ def sync_db(): def create_workflows(definition, scope='private', is_system=False, - run_in_tx=True, namespace=''): + run_in_tx=True, namespace='', validate=True): LOG.debug("Creating workflows...") wf_list_spec = spec_parser.get_workflow_list_spec_from_yaml( definition, - validate=True + validate=services.is_validation_enabled(validate) ) + db_wfs = [] if run_in_tx: @@ -113,12 +116,14 @@ def _append_all_workflows(definition, is_system, scope, namespace, def update_workflows(definition, scope='private', identifier=None, - namespace=''): - LOG.debug("Updating workflows") + namespace='', validate=True): + LOG.debug("Updating workflows...") wf_list_spec = spec_parser.get_workflow_list_spec_from_yaml( - definition, validate=True + definition, + validate=services.is_validation_enabled(validate) ) + wfs = wf_list_spec.get_workflows() if identifier and len(wfs) > 1: diff --git a/mistral/tests/unit/api/v2/test_workbooks.py b/mistral/tests/unit/api/v2/test_workbooks.py index 9058019b7..d9fff86fd 100644 --- a/mistral/tests/unit/api/v2/test_workbooks.py +++ b/mistral/tests/unit/api/v2/test_workbooks.py @@ -236,6 +236,19 @@ class TestWorkbooksController(base.APITest): self.assertEqual(400, resp.status_int) self.assertIn("Invalid DSL", resp.body.decode()) + @mock.patch.object(workbooks, "update_workbook_v2", MOCK_UPDATED_WORKBOOK) + def test_put_invalid_skip_validation(self): + self.override_config('validation_mode', 'enabled', 'api') + + resp = self.app.put( + '/v2/workbooks?skip_validation', + WB_DEF_INVALID_MODEL_EXCEPTION, + headers={'Content-Type': 'text/plain'}, + expect_errors=True + ) + + self.assertEqual(200, resp.status_int) + @mock.patch.object(db_api, "update_workbook") @mock.patch.object(db_api, "create_or_update_workflow_definition") @mock.patch.object(db_api, "create_or_update_action_definition") @@ -314,6 +327,18 @@ class TestWorkbooksController(base.APITest): self.assertEqual(400, resp.status_int) self.assertIn("Invalid DSL", resp.body.decode()) + def test_post_invalid_skip_validation(self): + self.override_config('validation_mode', 'enabled', 'api') + + resp = self.app.post( + '/v2/workbooks?skip_validation', + WB_DEF_INVALID_MODEL_EXCEPTION, + headers={'Content-Type': 'text/plain'}, + expect_errors=True + ) + + self.assertEqual(201, resp.status_int) + @mock.patch.object(db_api, "create_workbook") @mock.patch.object(db_api, "create_or_update_workflow_definition") @mock.patch.object(db_api, "create_or_update_action_definition") diff --git a/mistral/tests/unit/api/v2/test_workflows.py b/mistral/tests/unit/api/v2/test_workflows.py index 91aae7c54..e8ee14955 100644 --- a/mistral/tests/unit/api/v2/test_workflows.py +++ b/mistral/tests/unit/api/v2/test_workflows.py @@ -311,7 +311,8 @@ class TestWorkflowsController(base.APITest): UPDATED_WF_DEFINITION, scope='private', identifier='123e4567-e89b-12d3-a456-426655440000', - namespace='' + namespace='', + validate=True ) self.assertDictEqual(UPDATED_WF, resp.json) @@ -398,6 +399,21 @@ class TestWorkflowsController(base.APITest): self.assertEqual(400, resp.status_int) self.assertIn("Invalid DSL", resp.body.decode()) + @mock.patch.object( + db_api, "update_workflow_definition", MOCK_UPDATED_WF + ) + def test_put_invalid_skip_validation(self): + self.override_config('validation_mode', 'enabled', 'api') + + resp = self.app.put( + '/v2/workflows?skip_validation', + WF_DEF_INVALID_MODEL_EXCEPTION, + headers={'Content-Type': 'text/plain'}, + expect_errors=True + ) + + self.assertEqual(200, resp.status_int) + @mock.patch.object(db_api, "update_workflow_definition") def test_put_multiple(self, mock_mtd): self.app.put( @@ -504,6 +520,18 @@ class TestWorkflowsController(base.APITest): self.assertEqual(400, resp.status_int) self.assertIn("Invalid DSL", resp.body.decode()) + def test_post_invalid_skip_validation(self): + self.override_config('validation_mode', 'enabled', 'api') + + resp = self.app.post( + '/v2/workflows?skip_validation', + WF_DEF_INVALID_MODEL_EXCEPTION, + headers={'Content-Type': 'text/plain'}, + expect_errors=True + ) + + self.assertEqual(201, resp.status_int) + @mock.patch.object(db_api, "delete_workflow_definition", MOCK_DELETE) @mock.patch.object(db_api, "get_workflow_definition", MOCK_WF) def test_delete(self): @@ -804,7 +832,8 @@ class TestWorkflowsController(base.APITest): WF_DEFINITION, scope='private', identifier=id_, - namespace='abc' + namespace='abc', + validate=True ) self.assertDictEqual(WF_WITH_NAMESPACE, resp.json) diff --git a/mistral/tests/unit/services/test_workflow_service.py b/mistral/tests/unit/services/test_workflow_service.py index d2ddd7861..e12dda941 100644 --- a/mistral/tests/unit/services/test_workflow_service.py +++ b/mistral/tests/unit/services/test_workflow_service.py @@ -87,7 +87,6 @@ WORKFLOW_WITH_VAR_TASK_NAME = """ version: '2.0' engine_command_{task_name}: - tasks: {task_name}: action: nova.servers_list @@ -100,18 +99,30 @@ INVALID_WORKFLOW = """ verstion: '2.0' wf: - type: direct tasks: task1: action: std.echo output="Task 1" """ +INVALID_WORKFLOW_1 = """ +--- +version: '2.0' + +wf: + tasks: + task1: + action: std.noop + on-success: task2 # The task "task2" doesn't exist. + + task3: + action: std.noop +""" + WORKFLOW_WITH_LONG_TASK_NAME = """ --- version: '2.0' test_workflow: - tasks: {long_task_name}: action: std.noop @@ -123,7 +134,6 @@ WORKFLOW_WITH_LONG_JOIN_TASK_NAME = """ version: '2.0' test_workflow: - tasks: task1: on-success: @@ -158,8 +168,12 @@ class WorkflowServiceTest(base.DbTestCase): def test_engine_commands_are_valid_task_names(self): for name in workflows.ENGINE_COMMANDS: - wf = WORKFLOW_WITH_VAR_TASK_NAME.format(task_name=name) - wf_service.create_workflows(wf) + wf_text = WORKFLOW_WITH_VAR_TASK_NAME.format(task_name=name) + + wf_defs = wf_service.create_workflows(wf_text) + + self.assertIsNotNone(wf_defs) + self.assertEqual(1, len(wf_defs)) def test_update_workflows(self): db_wfs = wf_service.create_workflows(WORKFLOW_LIST) @@ -317,8 +331,10 @@ class WorkflowServiceTest(base.DbTestCase): def test_with_long_task_name(self): long_task_name = utils.generate_string(tasks.MAX_LENGTH_TASK_NAME + 1) + workflow = WORKFLOW_WITH_LONG_TASK_NAME.format( - long_task_name=long_task_name) + long_task_name=long_task_name + ) self.assertRaises( exc.InvalidModelException, @@ -328,27 +344,85 @@ class WorkflowServiceTest(base.DbTestCase): def test_upper_bound_length_task_name(self): long_task_name = utils.generate_string(tasks.MAX_LENGTH_TASK_NAME) - workflow = WORKFLOW_WITH_LONG_TASK_NAME.format( - long_task_name=long_task_name) - wf_service.create_workflows(workflow) + wf_text = WORKFLOW_WITH_LONG_TASK_NAME.format( + long_task_name=long_task_name + ) + + wf_defs = wf_service.create_workflows(wf_text) + + self.assertIsNotNone(wf_defs) + self.assertEqual(1, len(wf_defs)) def test_with_long_join_task_name(self): long_task_name = utils.generate_string( tasks.MAX_LENGTH_JOIN_TASK_NAME + 1 ) - workflow = WORKFLOW_WITH_LONG_JOIN_TASK_NAME.format( - long_task_name=long_task_name) + + wf_text = WORKFLOW_WITH_LONG_JOIN_TASK_NAME.format( + long_task_name=long_task_name + ) self.assertRaises( exc.InvalidModelException, wf_service.create_workflows, - workflow + wf_text ) def test_upper_bound_length_join_task_name(self): long_task_name = utils.generate_string(tasks.MAX_LENGTH_JOIN_TASK_NAME) - workflow = WORKFLOW_WITH_LONG_JOIN_TASK_NAME.format( - long_task_name=long_task_name) - wf_service.create_workflows(workflow) + wf_text = WORKFLOW_WITH_LONG_JOIN_TASK_NAME.format( + long_task_name=long_task_name + ) + + wf_defs = wf_service.create_workflows(wf_text) + + self.assertIsNotNone(wf_defs) + self.assertEqual(1, len(wf_defs)) + + def test_validation_mode_enabled_by_default(self): + self.override_config('validation_mode', 'enabled', 'api') + + self.assertRaises( + exc.InvalidModelException, + wf_service.create_workflows, + INVALID_WORKFLOW_1 + ) + + wf_defs = wf_service.create_workflows( + INVALID_WORKFLOW_1, + validate=False + ) + + # The workflow is created but it will never succeed since it's broken. + self.assertIsNotNone(wf_defs) + self.assertEqual(1, len(wf_defs)) + + def test_validation_mode_always_enabled(self): + self.override_config('validation_mode', 'mandatory', 'api') + + self.assertRaises( + exc.InvalidModelException, + wf_service.create_workflows, + INVALID_WORKFLOW_1 + ) + + self.assertRaises( + exc.InvalidModelException, + wf_service.create_workflows, + INVALID_WORKFLOW_1, + validate=False + ) + + def test_validation_mode_always_disabled(self): + self.override_config('validation_mode', 'disabled', 'api') + + wf_defs = wf_service.create_workflows(INVALID_WORKFLOW_1) + + self.assertIsNotNone(wf_defs) + self.assertEqual(1, len(wf_defs)) + + db_api.delete_workflow_definition(wf_defs[0].id) + + wf_service.create_workflows(INVALID_WORKFLOW_1, validate=True) diff --git a/releasenotes/notes/add_skip_validation-9e8b906c45bdb89f.yaml b/releasenotes/notes/add_skip_validation-9e8b906c45bdb89f.yaml new file mode 100644 index 000000000..7b0c15f3a --- /dev/null +++ b/releasenotes/notes/add_skip_validation-9e8b906c45bdb89f.yaml @@ -0,0 +1,18 @@ +--- +features: + - | + The new configuration option "validation_mode" was added. It can take one + of the values: "enabled", "mandatory", "disabled". If it is set to + "enabled" then Mistral will be validating the workflow language syntax + for all API operations that create or update workflows (either via + /v2/workflows or /v2/workbooks) unless it's explicitly disabled with the + API parameter "skip_validation" that has now been added to the + corresponding API endpoints. The "skip_validation" parameter doesn't have + to have any value since it's a boolean flag. If the configuration option + "validation_mode" is set to "mandatory" then Mistral will be always + validating the syntax of all workflows for the mentioned operations. + If set to "disabled" then validation will always be skipped. Note that + if validation is disabled (one way or another) then there's a risk of + breaking a workflow unexpectedly while it's running or getting another an + unexpected error when uploading it possible w/o a user-friendly description + of the error.