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.