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

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

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

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:

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:

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']
 

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 change the 'dirty' mapping and remove them.
-    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
-    '''
 
     if exceptions is None:
-        exceptions = ()
+        exceptions = tuple()
-    clean = dirty.copy()
+
-    for k in dirty.keys():
+    if isinstance(dirty, MutableSequence):
-        if isinstance(k, six.string_types) and k.startswith('_ansible_'):
+
-            if k not in exceptions:
+        for element in dirty:
-                del clean[k]
+            if isinstance(element, (MutableMapping, MutableSequence)):
-        elif isinstance(dirty[k], dict):
+                strip_internal_keys(element, exceptions=exceptions)
-            clean[k] = strip_internal_keys(dirty[k])
+
-    return clean
+    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):

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

test/integration/targets/loop_control/aliases

@@ -0,0 +1 @@
+shippable/posix/group2

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

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}" ]
+

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