From 795a6463ee0152bbdd0484ad6903b9c750c3c9d1 Mon Sep 17 00:00:00 2001 From: Nick Maludy Date: Fri, 1 May 2020 15:26:51 -0400 Subject: [PATCH 1/7] #4932 - Fixes for field escaping in orquesta workflows --- .../actions/orquesta-test-field-escaping.yaml | 8 ++++++ .../actions/python-mock-core-remote.py | 23 ++++++++++++++++ .../actions/python-mock-core-remote.yaml | 18 +++++++++++++ .../tests/orquesta-test-field-escaping.yaml | 26 +++++++++++++++++++ st2common/st2common/models/db/workflow.py | 6 ++--- st2tests/integration/orquesta/test_wiring.py | 20 ++++++++++++++ 6 files changed, 98 insertions(+), 3 deletions(-) create mode 100644 contrib/examples/actions/orquesta-test-field-escaping.yaml create mode 100644 contrib/examples/actions/python-mock-core-remote.py create mode 100644 contrib/examples/actions/python-mock-core-remote.yaml create mode 100644 contrib/examples/actions/workflows/tests/orquesta-test-field-escaping.yaml 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..653ccdea33 --- /dev/null +++ b/contrib/examples/actions/python-mock-core-remote.yaml @@ -0,0 +1,18 @@ +--- +name: python-mock-core-remote +pack: default +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/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) From 7cb9e302aad13c6d66591a60e0e10f7f5fd26832 Mon Sep 17 00:00:00 2001 From: Nick Maludy Date: Mon, 4 May 2020 11:09:42 -0400 Subject: [PATCH 2/7] Debuggign pip problems --- scripts/fixate-requirements.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/scripts/fixate-requirements.py b/scripts/fixate-requirements.py index 1aa892b7fb..2f2becb133 100755 --- a/scripts/fixate-requirements.py +++ b/scripts/fixate-requirements.py @@ -69,6 +69,7 @@ print('Using pip: %s' % (str(pip_version))) sys.exit(1) +print("DEBUG: pip version = {}".format(pip.__version__)) def parse_args(): parser = argparse.ArgumentParser(description='Tool for requirements.txt generation.') @@ -117,6 +118,7 @@ def merge_source_requirements(sources): for infile_path in (locate_file(p, must_exist=True) for p in sources): for req in load_requirements(infile_path): # Requirements starting with project name "project ..." + print("DEBUG: type(req) = {}".format(type(req))) if req.req: # Skip already added project name if req.name in projects: From 4d1c7838130ec59e11be1e7ba165df7e53f763cf Mon Sep 17 00:00:00 2001 From: Nick Maludy Date: Mon, 4 May 2020 11:17:19 -0400 Subject: [PATCH 3/7] Fixing pip problem --- scripts/fixate-requirements.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/scripts/fixate-requirements.py b/scripts/fixate-requirements.py index 2f2becb133..e542409bf1 100755 --- a/scripts/fixate-requirements.py +++ b/scripts/fixate-requirements.py @@ -108,6 +108,14 @@ def locate_file(path, must_exist=False): print("Error: couldn't locate file `{0}'".format(path)) return path +def get_req_attr(req): + # pip >= 20.0 + requirement = getattr(req, 'requirement', None) + if not requirement: + # pip < 20.0 + requirement = getattr(req, 'req', None) + return requirement + def merge_source_requirements(sources): """ @@ -119,7 +127,7 @@ def merge_source_requirements(sources): for req in load_requirements(infile_path): # Requirements starting with project name "project ..." print("DEBUG: type(req) = {}".format(type(req))) - if req.req: + if get_req_attr(req): # Skip already added project name if req.name in projects: continue @@ -150,7 +158,7 @@ def write_requirements(sources=None, fixed_requirements=None, output_file=None, for req in fixed: project_name = req.name - if not req.req: + if not get_req_attr(req): continue if project_name in fixedreq_hash: @@ -171,11 +179,11 @@ def write_requirements(sources=None, fixed_requirements=None, output_file=None, if req.editable: rline = '-e %s' % (rline) - elif req.req: + elif get_req_attr(req): project = req.name req_obj = fixedreq_hash.get(project, req) - rline = str(req_obj.req) + rline = str(get_req_attr(req_obj)) # Also write out environment markers if req_obj.markers: From ffb3ac685c515f364bd6c9378782b6a09995d5d1 Mon Sep 17 00:00:00 2001 From: Nick Maludy Date: Mon, 4 May 2020 11:23:42 -0400 Subject: [PATCH 4/7] Adding debug output for pip version --- Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/Makefile b/Makefile index 336b1b4024..8df2befc36 100644 --- a/Makefile +++ b/Makefile @@ -496,6 +496,7 @@ distclean: clean .PHONY: .requirements .requirements: virtualenv # Generate all requirements to support current CI pipeline. + $(VIRTUALENV_DIR)/bin/pip --version $(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 # Remove any *.egg-info files which polute PYTHONPATH From 576fbe2a4459dce9b3d3a27fa82fadfe6794bd14 Mon Sep 17 00:00:00 2001 From: Nick Maludy Date: Mon, 4 May 2020 11:48:23 -0400 Subject: [PATCH 5/7] Fixing wrong pack name for python-mock-core-remote --- contrib/examples/actions/python-mock-core-remote.yaml | 1 - 1 file changed, 1 deletion(-) diff --git a/contrib/examples/actions/python-mock-core-remote.yaml b/contrib/examples/actions/python-mock-core-remote.yaml index 653ccdea33..f72702f235 100644 --- a/contrib/examples/actions/python-mock-core-remote.yaml +++ b/contrib/examples/actions/python-mock-core-remote.yaml @@ -1,6 +1,5 @@ --- name: python-mock-core-remote -pack: default 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 From b601b580ca2e1106cebab7f3b9ad5ed38a62f9de Mon Sep 17 00:00:00 2001 From: Nick Maludy Date: Mon, 4 May 2020 11:53:15 -0400 Subject: [PATCH 6/7] Reverting debugging in fixate-requirements.py --- Makefile | 3 ++- scripts/fixate-requirements.py | 18 ++++-------------- 2 files changed, 6 insertions(+), 15 deletions(-) diff --git a/Makefile b/Makefile index 8df2befc36..7390776e84 100644 --- a/Makefile +++ b/Makefile @@ -495,8 +495,9 @@ distclean: clean .PHONY: .requirements .requirements: virtualenv - # Generate all requirements to support current CI pipeline. + # 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 # Remove any *.egg-info files which polute PYTHONPATH diff --git a/scripts/fixate-requirements.py b/scripts/fixate-requirements.py index e542409bf1..1aa892b7fb 100755 --- a/scripts/fixate-requirements.py +++ b/scripts/fixate-requirements.py @@ -69,7 +69,6 @@ print('Using pip: %s' % (str(pip_version))) sys.exit(1) -print("DEBUG: pip version = {}".format(pip.__version__)) def parse_args(): parser = argparse.ArgumentParser(description='Tool for requirements.txt generation.') @@ -108,14 +107,6 @@ def locate_file(path, must_exist=False): print("Error: couldn't locate file `{0}'".format(path)) return path -def get_req_attr(req): - # pip >= 20.0 - requirement = getattr(req, 'requirement', None) - if not requirement: - # pip < 20.0 - requirement = getattr(req, 'req', None) - return requirement - def merge_source_requirements(sources): """ @@ -126,8 +117,7 @@ def merge_source_requirements(sources): for infile_path in (locate_file(p, must_exist=True) for p in sources): for req in load_requirements(infile_path): # Requirements starting with project name "project ..." - print("DEBUG: type(req) = {}".format(type(req))) - if get_req_attr(req): + if req.req: # Skip already added project name if req.name in projects: continue @@ -158,7 +148,7 @@ def write_requirements(sources=None, fixed_requirements=None, output_file=None, for req in fixed: project_name = req.name - if not get_req_attr(req): + if not req.req: continue if project_name in fixedreq_hash: @@ -179,11 +169,11 @@ def write_requirements(sources=None, fixed_requirements=None, output_file=None, if req.editable: rline = '-e %s' % (rline) - elif get_req_attr(req): + elif req.req: project = req.name req_obj = fixedreq_hash.get(project, req) - rline = str(get_req_attr(req_obj)) + rline = str(req_obj.req) # Also write out environment markers if req_obj.markers: From f98750b2b3c7a24ec3f7cf8af10a738510bde693 Mon Sep 17 00:00:00 2001 From: Nick Maludy Date: Mon, 4 May 2020 15:51:25 -0400 Subject: [PATCH 7/7] Added changelog for fix of #4932 --- CHANGELOG.rst | 12 ++++++++++++ 1 file changed, 12 insertions(+) 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 ----------------------