diff --git a/CHANGELOG.rst b/CHANGELOG.rst index a1e7e6c805..93d826bdc0 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -4,6 +4,18 @@ Changelog in development -------------- +Fixed +~~~~~ +* Fixed a bug where persisting Orquesta to the MongoDB database returned an error + ``message: key 'myvar.with.period' must not contain '.'``. This happened anytime an + ``input``, ``output``, ``publish`` or context ``var`` contained a key with a ``.`` within + the name (such as with hostnames and IP addresses). This was a regression introduced by + trying to improve performance. Fixing this bug means we are sacrificing performance of + serialization/deserialization in favor of correctness for persisting workflows and + their state to the MongoDB database. (bug fix) #4932 + + Contributed by Nick Maludy (@nmaludy Encore Technologies) + 3.2.0 - April 27, 2020 ---------------------- diff --git a/Makefile b/Makefile index e900c3ae9a..f58b69e0fa 100644 --- a/Makefile +++ b/Makefile @@ -495,6 +495,8 @@ distclean: clean .PHONY: .requirements .requirements: virtualenv + # Print out pip version so we can see what version was restored from the Travis cache + $(VIRTUALENV_DIR)/bin/pip --version # Generate all requirements to support current CI pipeline. $(VIRTUALENV_DIR)/bin/python scripts/fixate-requirements.py --skip=virtualenv,virtualenv-osx -s st2*/in-requirements.txt contrib/runners/*/in-requirements.txt -f fixed-requirements.txt -o requirements.txt diff --git a/contrib/examples/actions/orquesta-test-field-escaping.yaml b/contrib/examples/actions/orquesta-test-field-escaping.yaml new file mode 100644 index 0000000000..a5b5de5004 --- /dev/null +++ b/contrib/examples/actions/orquesta-test-field-escaping.yaml @@ -0,0 +1,8 @@ +--- +name: orquesta-test-field-escaping +description: A workflow used to test escaping of things like vars, task names, results, outputs, etc. +pack: examples +runner_type: orquesta +entry_point: workflows/tests/orquesta-test-field-escaping.yaml +enabled: true +parameters: {} diff --git a/contrib/examples/actions/python-mock-core-remote.py b/contrib/examples/actions/python-mock-core-remote.py new file mode 100644 index 0000000000..cd4d44500e --- /dev/null +++ b/contrib/examples/actions/python-mock-core-remote.py @@ -0,0 +1,23 @@ +from st2common.runners.base_action import Action + + +class MockCoreRemoteAction(Action): + + def run(self, cmd, hosts, hosts_dict): + if hosts_dict: + return hosts_dict + + if not hosts: + return None + + host_list = hosts.split(',') + results = {} + for h in hosts: + results[h] = { + 'failed': False, + 'return_code': 0, + 'stderr': '', + 'succeeded': True, + 'stdout': cmd, + } + return results diff --git a/contrib/examples/actions/python-mock-core-remote.yaml b/contrib/examples/actions/python-mock-core-remote.yaml new file mode 100644 index 0000000000..f72702f235 --- /dev/null +++ b/contrib/examples/actions/python-mock-core-remote.yaml @@ -0,0 +1,17 @@ +--- +name: python-mock-core-remote +runner_type: python-script +description: Action which mocks the core.remote action. It returns results with hostnames/IPs in them to ensure we are escaping the . properly. We set the output to the value of the command passed in +enabled: true +entry_point: python-mock-core-remote.py +parameters: + cmd: + type: string + required: false + hosts: + type: string + required: false + hosts_dict: + type: object + description: "If specified, just returns this dict back out. Used for testing inputs and outputs of dicts with keys containing '.' character." + required: false diff --git a/contrib/examples/actions/workflows/tests/orquesta-test-field-escaping.yaml b/contrib/examples/actions/workflows/tests/orquesta-test-field-escaping.yaml new file mode 100644 index 0000000000..c3ea12e248 --- /dev/null +++ b/contrib/examples/actions/workflows/tests/orquesta-test-field-escaping.yaml @@ -0,0 +1,26 @@ +version: 1.0 + +description: A workflow used to test escaping of things like vars, task names, results, outputs, etc. + +vars: + - vars.key.with.periods: "vars.value.with.periods" + - vars.nested.with.periods: + nested.periods: "vars.nested.value.with.periods" + +tasks: + run: + action: examples.python-mock-core-remote + input: + hosts_dict: + hostname.domain.tld: "{{ ctx()['vars.key.with.periods'] }}" + hostname2.domain.tld: + stdout: "{{ ctx()['vars.nested.with.periods']['nested.periods'] }}" + next: + - when: "{{ succeeded() }}" + publish: + - published.hosts: "{{ result().result }}" + - published.field: "{{ result().result['hostname2.domain.tld']['stdout'] }}" + +output: + - wf.hostname.with.periods: "{{ ctx()['published.hosts'] }}" + - wf.output.with.periods: "{{ ctx()['published.field'] }}" diff --git a/st2common/st2common/models/db/workflow.py b/st2common/st2common/models/db/workflow.py index b642df8eb8..81722ece51 100644 --- a/st2common/st2common/models/db/workflow.py +++ b/st2common/st2common/models/db/workflow.py @@ -36,11 +36,11 @@ class WorkflowExecutionDB(stormbase.StormFoundationDB, stormbase.ChangeRevisionF RESOURCE_TYPE = types.ResourceType.EXECUTION action_execution = me.StringField(required=True) - spec = me.DictField() + spec = stormbase.EscapedDictField() graph = me.DictField() input = stormbase.EscapedDictField() notify = me.DictField() - context = me.DictField() + context = stormbase.EscapedDictField() state = stormbase.EscapedDictField() status = me.StringField(required=True) output = stormbase.EscapedDictField() @@ -62,7 +62,7 @@ class TaskExecutionDB(stormbase.StormFoundationDB, stormbase.ChangeRevisionField task_name = me.StringField(required=True) task_id = me.StringField(required=True) task_route = me.IntField(required=True, min_value=0) - task_spec = me.DictField() + task_spec = stormbase.EscapedDictField() delay = me.IntField(min_value=0) itemized = me.BooleanField(default=False) items_count = me.IntField(min_value=0) diff --git a/st2common/tests/unit/test_dist_utils.py b/st2common/tests/unit/test_dist_utils.py index 9c24320c21..9cd2b1bcf8 100644 --- a/st2common/tests/unit/test_dist_utils.py +++ b/st2common/tests/unit/test_dist_utils.py @@ -30,7 +30,6 @@ from dist_utils import fetch_requirements from dist_utils import apply_vagrant_workaround from dist_utils import get_version_string -from dist_utils_old import fetch_requirements as old_fetch_requirements __all__ = [ 'DistUtilsTestCase' @@ -138,18 +137,7 @@ def test_fetch_requirements(self): self.assertEqual(reqs, expected_reqs) self.assertEqual(links, expected_links) - # Verify output of old and new function is the same - reqs_old, links_old = old_fetch_requirements(REQUIREMENTS_PATH_1) - - self.assertEqual(reqs_old, expected_reqs) - self.assertEqual(links_old, expected_links) - - self.assertEqual(reqs_old, reqs) - self.assertEqual(links_old, links) - # Also test it on requirements.txt in repo root reqs, links = fetch_requirements(REQUIREMENTS_PATH_2) - reqs_old, links_old = old_fetch_requirements(REQUIREMENTS_PATH_2) - - self.assertEqual(reqs_old, reqs) - self.assertEqual(links_old, links) + self.assertGreater(len(reqs), 0) + self.assertGreater(len(links), 0) diff --git a/st2tests/integration/orquesta/test_wiring.py b/st2tests/integration/orquesta/test_wiring.py index 72af88fd55..729482d71f 100644 --- a/st2tests/integration/orquesta/test_wiring.py +++ b/st2tests/integration/orquesta/test_wiring.py @@ -156,3 +156,23 @@ def test_config_context_renders(self): self.assertEqual(ex.status, ac_const.LIVEACTION_STATUS_SUCCEEDED) self.assertDictEqual(ex.result, expected_result) + + def test_field_escaping(self): + wf_name = 'examples.orquesta-test-field-escaping' + + ex = self._execute_workflow(wf_name) + ex = self._wait_for_completion(ex) + + expected_output = { + 'wf.hostname.with.periods': { + 'hostname.domain.tld': 'vars.value.with.periods', + 'hostname2.domain.tld': { + 'stdout': 'vars.nested.value.with.periods', + }, + }, + 'wf.output.with.periods': 'vars.nested.value.with.periods', + } + expected_result = {'output': expected_output} + + self.assertEqual(ex.status, ac_const.LIVEACTION_STATUS_SUCCEEDED) + self.assertDictEqual(ex.result, expected_result)