openstack/mistral
Code Issues Proposed changes

Add an ability to disable workflow text validation

Browse Source 
* 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
This commit is contained in:
Renat Akhmerov 2019-09-20 12:03:46 +07:00
parent 0de247948b
commit ac41f94d11
13 changed files with 274 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
mistral/api/controllers/v2/member.py
View File

@ -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,

17
mistral/api/controllers/v2/workbook.py
View File

@ -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

26
mistral/api/controllers/v2/workflow.py
View File

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

11
mistral/config.py
View File

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

4
mistral/lang/base.py
View File

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

4
mistral/lang/parser.py
View File

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

29
mistral/services/__init__.py
View File

@ -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

21
mistral/services/workbooks.py
View File

@ -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

15
mistral/services/workflows.py
View File

@ -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:

25
mistral/tests/unit/api/v2/test_workbooks.py
View File

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

33
mistral/tests/unit/api/v2/test_workflows.py
View File

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

106
mistral/tests/unit/services/test_workflow_service.py
View File

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

18
releasenotes/notes/add_skip_validation-9e8b906c45bdb89f.yaml Normal file
View File

@ -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.