Merge "Add an ability to disable workflow text validation"

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,

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

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 = [

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.")
     )
 ]
 

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
+    :param validate: If it's False then semantics and schema validation
-        will be skipped
+        will be skipped.
     :type validate: bool
     """
 

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
     )
 
 

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

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,
+    db_wfs = _create_or_update_workflows(
-                                         wb_spec.get_workflows(),
+        wb_db,
-                                         namespace)
+        wb_spec.get_workflows(),
+        namespace
+    )
 
     return db_actions, db_wfs
 

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=''):
+                     namespace='', validate=True):
-    LOG.debug("Updating workflows")
+    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:

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")

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)
 

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_text = WORKFLOW_WITH_VAR_TASK_NAME.format(task_name=name)
-            wf_service.create_workflows(wf)
+
+            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)

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.