From b793f08a922ebc9ad1034e47b4d364d3914822d0 Mon Sep 17 00:00:00 2001 From: Brian Coca Date: Tue, 12 Mar 2019 18:18:38 -0400 Subject: [PATCH] fixes for stripping (#52930) function changed to do in place replacement, should be less expensive even with copy as it avoids 'sub copies', can compose with module_args_copy to create replacement for old behavior attempt to fix #52910 * handle lists and subdicts correctly * added missing exception case, which was not noticed since 'cleaning' was not working * added comments to clarify exceptions --- changelogs/fragments/strip_keys_fixes.yml | 3 ++ lib/ansible/executor/task_result.py | 10 ++++- lib/ansible/plugins/callback/__init__.py | 4 +- lib/ansible/plugins/callback/yaml.py | 4 +- lib/ansible/plugins/strategy/__init__.py | 4 +- lib/ansible/vars/clean.py | 42 ++++++++++++------- test/integration/targets/delegate_to/runme.sh | 3 +- test/integration/targets/loop_control/aliases | 1 + .../targets/loop_control/label.yml | 23 ++++++++++ .../integration/targets/loop_control/runme.sh | 11 +++++ test/integration/targets/loops/tasks/main.yml | 25 ----------- 11 files changed, 83 insertions(+), 47 deletions(-) create mode 100644 changelogs/fragments/strip_keys_fixes.yml create mode 100644 test/integration/targets/loop_control/aliases create mode 100644 test/integration/targets/loop_control/label.yml create mode 100755 test/integration/targets/loop_control/runme.sh diff --git a/changelogs/fragments/strip_keys_fixes.yml b/changelogs/fragments/strip_keys_fixes.yml new file mode 100644 index 00000000000..d155fecdd88 --- /dev/null +++ b/changelogs/fragments/strip_keys_fixes.yml @@ -0,0 +1,3 @@ +bugfixes: + - change function to in place replacement, compose with module_args_copy for 'new clean copy' + - avoid making multiple 'sub copies' when traversing already 'clean copy' of dict diff --git a/lib/ansible/executor/task_result.py b/lib/ansible/executor/task_result.py index cf1a522199a..3a8dd0c7a01 100644 --- a/lib/ansible/executor/task_result.py +++ b/lib/ansible/executor/task_result.py @@ -13,6 +13,14 @@ _IGNORE = ('failed', 'skipped') _PRESERVE = ('attempts', 'changed', 'retries') _SUB_PRESERVE = {'_ansible_delegated_vars': ('ansible_host', 'ansible_port', 'ansible_user', 'ansible_connection')} +# stuff callbacks need +CLEAN_EXCEPTIONS = ( + '_ansible_verbose_always', # for debug and other actions, to always expand data (pretty jsonification) + '_ansible_item_label', # to know actual 'item' variable + '_ansible_no_log', # jic we didnt clean up well enough, DON'T LOG + '_ansible_verbose_override', # controls display of ansible_facts, gathering would be very noise with -v otherwise +) + class TaskResult: ''' @@ -137,6 +145,6 @@ class TaskResult: del result._result[remove_key] # remove almost ALL internal keys, keep ones relevant to callback - strip_internal_keys(result._result, exceptions=('_ansible_verbose_always', '_ansible_item_label', '_ansible_no_log')) + strip_internal_keys(result._result, exceptions=CLEAN_EXCEPTIONS) return result diff --git a/lib/ansible/plugins/callback/__init__.py b/lib/ansible/plugins/callback/__init__.py index 4cf3f01ef58..59a83278cda 100644 --- a/lib/ansible/plugins/callback/__init__.py +++ b/lib/ansible/plugins/callback/__init__.py @@ -35,7 +35,7 @@ from ansible.parsing.ajson import AnsibleJSONEncoder from ansible.plugins import AnsiblePlugin, get_plugin_class from ansible.utils.color import stringc from ansible.utils.display import Display -from ansible.vars.clean import strip_internal_keys +from ansible.vars.clean import strip_internal_keys, module_response_deepcopy if PY3: # OrderedDict is needed for a backwards compat shim on Python3.x only @@ -104,7 +104,7 @@ class CallbackBase(AnsiblePlugin): indent = 4 # All result keys stating with _ansible_ are internal, so remove them from the result before we output anything. - abridged_result = strip_internal_keys(result) + abridged_result = strip_internal_keys(module_response_deepcopy(result)) # remove invocation unless specifically wanting it if not keep_invocation and self._display.verbosity < 3 and 'invocation' in result: diff --git a/lib/ansible/plugins/callback/yaml.py b/lib/ansible/plugins/callback/yaml.py index 3bd8b285eaa..dcab68769e5 100644 --- a/lib/ansible/plugins/callback/yaml.py +++ b/lib/ansible/plugins/callback/yaml.py @@ -28,7 +28,7 @@ import sys from ansible.module_utils._text import to_bytes, to_text from ansible.module_utils.six import string_types from ansible.parsing.yaml.dumper import AnsibleDumper -from ansible.plugins.callback import CallbackBase, strip_internal_keys +from ansible.plugins.callback import CallbackBase, strip_internal_keys, module_response_deepcopy from ansible.plugins.callback.default import CallbackModule as Default @@ -85,7 +85,7 @@ class CallbackModule(Default): return json.dumps(dict(censored="The output has been hidden due to the fact that 'no_log: true' was specified for this result")) # All result keys stating with _ansible_ are internal, so remove them from the result before we output anything. - abridged_result = strip_internal_keys(result) + abridged_result = strip_internal_keys(module_response_deepcopy(result)) # remove invocation unless specifically wanting it if not keep_invocation and self._display.verbosity < 3 and 'invocation' in result: diff --git a/lib/ansible/plugins/strategy/__init__.py b/lib/ansible/plugins/strategy/__init__.py index d3a136d258f..d182c456bd9 100644 --- a/lib/ansible/plugins/strategy/__init__.py +++ b/lib/ansible/plugins/strategy/__init__.py @@ -49,7 +49,7 @@ from ansible.plugins.loader import action_loader, connection_loader, filter_load from ansible.template import Templar from ansible.utils.display import Display from ansible.utils.vars import combine_vars -from ansible.vars.clean import strip_internal_keys +from ansible.vars.clean import strip_internal_keys, module_response_deepcopy display = Display() @@ -436,7 +436,7 @@ class StrategyBase: if original_task.register: host_list = self.get_task_hosts(iterator, original_host, original_task) - clean_copy = strip_internal_keys(task_result._result) + clean_copy = strip_internal_keys(module_response_deepcopy(task_result._result)) if 'invocation' in clean_copy: del clean_copy['invocation'] diff --git a/lib/ansible/vars/clean.py b/lib/ansible/vars/clean.py index 04d5de33aca..06417823c25 100644 --- a/lib/ansible/vars/clean.py +++ b/lib/ansible/vars/clean.py @@ -9,11 +9,14 @@ import os import re from ansible import constants as C -from ansible.module_utils._text import to_text +from ansible.errors import AnsibleError from ansible.module_utils import six +from ansible.module_utils._text import to_text +from ansible.module_utils.common._collections_compat import MutableMapping, MutableSequence from ansible.plugins.loader import connection_loader from ansible.utils.display import Display + display = Display() @@ -65,21 +68,32 @@ def module_response_deepcopy(v): def strip_internal_keys(dirty, exceptions=None): - ''' - All keys starting with _ansible_ are internal, so create a copy of the 'dirty' dict - and remove them from the clean one before returning it - ''' + # All keys starting with _ansible_ are internal, so change the 'dirty' mapping and remove them. if exceptions is None: - exceptions = () - clean = dirty.copy() - for k in dirty.keys(): - if isinstance(k, six.string_types) and k.startswith('_ansible_'): - if k not in exceptions: - del clean[k] - elif isinstance(dirty[k], dict): - clean[k] = strip_internal_keys(dirty[k]) - return clean + exceptions = tuple() + + if isinstance(dirty, MutableSequence): + + for element in dirty: + if isinstance(element, (MutableMapping, MutableSequence)): + strip_internal_keys(element, exceptions=exceptions) + + elif isinstance(dirty, MutableMapping): + + # listify to avoid updating dict while iterating over it + for k in list(dirty.keys()): + if isinstance(k, six.string_types): + if k.startswith('_ansible_') and k not in exceptions: + del dirty[k] + continue + + if isinstance(dirty[k], (MutableMapping, MutableSequence)): + strip_internal_keys(dirty[k], exceptions=exceptions) + else: + raise AnsibleError("Cannot strip invalid keys from %s" % type(dirty)) + + return dirty def remove_internal_keys(data): diff --git a/test/integration/targets/delegate_to/runme.sh b/test/integration/targets/delegate_to/runme.sh index 9cb64ed8808..a9056eb8806 100755 --- a/test/integration/targets/delegate_to/runme.sh +++ b/test/integration/targets/delegate_to/runme.sh @@ -5,7 +5,8 @@ set -eux ANSIBLE_SSH_ARGS='-C -o ControlMaster=auto -o ControlPersist=60s -o UserKnownHostsFile=/dev/null' \ ANSIBLE_HOST_KEY_CHECKING=false ansible-playbook test_delegate_to.yml -i inventory -v "$@" -ansible-playbook test_loop_control.yml -v "$@" +# this test is not doing what it says it does, also relies on var that should not be available +#ansible-playbook test_loop_control.yml -v "$@" ansible-playbook test_delegate_to_loop_randomness.yml -v "$@" diff --git a/test/integration/targets/loop_control/aliases b/test/integration/targets/loop_control/aliases new file mode 100644 index 00000000000..765b70da796 --- /dev/null +++ b/test/integration/targets/loop_control/aliases @@ -0,0 +1 @@ +shippable/posix/group2 diff --git a/test/integration/targets/loop_control/label.yml b/test/integration/targets/loop_control/label.yml new file mode 100644 index 00000000000..5ac85fdf321 --- /dev/null +++ b/test/integration/targets/loop_control/label.yml @@ -0,0 +1,23 @@ +- name: loop_control/label https://github.com/ansible/ansible/pull/36430 + hosts: localhost + gather_facts: false + tasks: + - set_fact: + loopthis: + - name: foo + label: foo_label + - name: bar + label: bar_label + + - name: check that item label is updated each iteration + debug: + msg: "{{ looped_var.name }}" + with_items: "{{ loopthis }}" + loop_control: + loop_var: looped_var + label: "looped_var {{ looped_var.label }}" +# +# - assert: +# that: +# - "output.results[0]['_ansible_item_label'] == 'looped_var foo_label'" +# - "output.results[1]['_ansible_item_label'] == 'looped_var bar_label'" diff --git a/test/integration/targets/loop_control/runme.sh b/test/integration/targets/loop_control/runme.sh new file mode 100755 index 00000000000..01f178e9da5 --- /dev/null +++ b/test/integration/targets/loop_control/runme.sh @@ -0,0 +1,11 @@ +#!/usr/bin/env bash + +set -eux + +# user output has: +#ok: [localhost] => (item=looped_var foo_label) => { +#ok: [localhost] => (item=looped_var bar_label) => { +MATCH='foo_label +bar_label' +[ "$(ansible-playbook label.yml "$@" |grep 'item='|sed -e 's/^.*(item=looped_var \(.*\)).*$/\1/')" == "${MATCH}" ] + diff --git a/test/integration/targets/loops/tasks/main.yml b/test/integration/targets/loops/tasks/main.yml index c0a4f14da08..db666873611 100644 --- a/test/integration/targets/loops/tasks/main.yml +++ b/test/integration/targets/loops/tasks/main.yml @@ -196,31 +196,6 @@ loop_control: index_var: my_idx -# -# loop_control/label -# https://github.com/ansible/ansible/pull/36430 -# - -- set_fact: - loopthis: - - name: foo - label: foo_label - - name: bar - label: bar_label - -- name: check that item label is updated each iteration - debug: - msg: "{{ looped_var.name }}" - with_items: "{{ loopthis }}" - loop_control: - loop_var: looped_var - label: "looped_var {{ looped_var.label }}" - register: output - -- assert: - that: - - "output.results[0]['_ansible_item_label'] == 'looped_var foo_label'" - - "output.results[1]['_ansible_item_label'] == 'looped_var bar_label'" # The following test cases are to ensure that we don't have a regression on # GitHub Issue https://github.com/ansible/ansible/issues/35481