Repository: ansible/ansible
Pull request: unsafe wrapping should only happen for with_ loops
##### SUMMARY
unsafe wrapping should only happen for `with_` lookp. Fixes #64379. Addresses #64169

##### ISSUE TYPE
- Bugfix Pull Request

##### COMPONENT NAME
lib/ansible/executor/task_executor.py

##### ADDITIONAL INFORMATION

cc @geerlingguy 

Problem statement:
[2.9] Iterating over a list of hashes with variables doesn't work
##### SUMMARY
<!--- Explain the problem briefly below -->

##### ISSUE TYPE
- Bug Report

##### COMPONENT NAME
loop

##### ANSIBLE VERSION
<!--- Paste verbatim output from "ansible --version" between quotes -->
```ansible 2.9.0
  config file = /Users/ansible.cfg
  configured module search path = ['/Users/library']
  ansible python module location = /Users/venv/lib/python3.7/site-packages/ansible
  executable location = /Users/venv/bin/ansible
  python version = 3.7.5 (default, Nov  1 2019, 02:16:23) [Clang 11.0.0 (clang-1100.0.33.8)]
```

##### CONFIGURATION
<!--- Paste verbatim output from "ansible-config dump --only-changed" between quotes -->
```ANSIBLE_FORCE_COLOR(/Users/ansible.cfg) = True
DEFAULT_FILTER_PLUGIN_PATH(/Users/ansible.cfg) = ['/Users/filter_plugins']
DEFAULT_MODULE_PATH(/Users/ansible.cfg) = ['/Users/library']
DEFAULT_ROLES_PATH(/Users/ansible.cfg) = ['/Users/roles']
DISPLAY_SKIPPED_HOSTS(/Users/ansible.cfg) = False
HOST_KEY_CHECKING(/Users/ansible.cfg) = False
```

##### OS / ENVIRONMENT
MACOSX

##### STEPS TO REPRODUCE
```yaml
- name: copy remote template to local ansible template in j2
  fetch:
    src: "{{ item.src }}"
    dest: "{{ item.dest }}"
    flat: yes
  loop: "{{ source_templates }}"
  when: not ansible_check_mode
```
 where
```yaml
  source_templates:
    - {src: '{{ path }}\{{ service_name }}\{{ project }}.{{ service_name }}.exe.aws.config', dest: "{{ role_path }}/templates/{{ project }}.{{ service_name }}.exe.aws.config.j2"}
    - {src: '{{ path }}\{{ service_name }}\whatever.{{ service_name }}.aws.config', dest: "{{ role_path }}/templates/whatever.{{ service_name }}.aws.config.j2"}
```

##### EXPECTED RESULTS

Task will execute twice, one per item.

##### ACTUAL RESULTS

It says the file is not found but in fact its there. For some reason, from version 2.9 ansible does not replace the variables.

```
TASK [procs-deploy : copy remote template to local ansible template in j2] ************************************************************************************
task path: /Users/roles/procs-deploy/tasks/configure.yml:64
failed: [192.168.55.167] (item={'src': '{{ path }}\\{{ service_name }}\\{{ project }}.{{ service_name }}.exe.aws.config', 'dest': '{{ role_path }}/templates/{{ project }}.{{ service_name }}.exe.aws.config.j2'}) => {
    "ansible_loop_var": "item",
    "changed": false,
    "item": {
        "dest": "{{ role_path }}/templates/{{ project }}.{{ service_name }}.exe.aws.config.j2",
        "src": "{{ path }}\\{{ service_name }}\\{{ project }}.{{ service_name }}.exe.aws.config"
    },
    "msg": "Path {{ path }}\\{{ service_name }}\\{{ project }}.{{ service_name }}.exe.aws.config is not found"
}
Using module file /Users/venv/lib/python3.7/site-packages/ansible/modules/windows/win_command.ps1
Pipelining is enabled.
<192.168.55.167> ESTABLISH WINRM CONNECTION FOR USER: whatever on PORT 5986 TO 192.168.55.167
EXEC (via pipeline wrapper)
```

Hints from issue discussion:
Files identified in the description:
None

If these files are inaccurate, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
seems related with https://github.com/ansible/ansible/issues/64169
@blastik thank you for the report. This is related to #64169, but while we believe the new behavior on `with_X` style loops is correct, it is incorrect for `loop`.

I am working on this bug and will update this issue in the coming days.

Commit message: typo in changelog fragment

Patch to review:
diff --git a/changelogs/fragments/64379-no-loop-unsafe.yml b/changelogs/fragments/64379-no-loop-unsafe.yml
new file mode 100644
index 00000000000000..0ec21f4e02e42e
--- /dev/null
+++ b/changelogs/fragments/64379-no-loop-unsafe.yml
@@ -0,0 +1,5 @@
+bugfixes:
+- loops - Do not indiscrimiately mark loop items as unsafe, only apply unsafe
+  to ``with_`` style loops. The items from ``loop`` should not be explicitly
+  wrapped in unsafe, the underlying templating mechanism should dictate this
+  (https://github.com/ansible/ansible/issues/64379)
diff --git a/docs/docsite/rst/porting_guides/porting_guide_2.9.rst b/docs/docsite/rst/porting_guides/porting_guide_2.9.rst
index fc6531fa7a7ec7..72ab7251a9345a 100644
--- a/docs/docsite/rst/porting_guides/porting_guide_2.9.rst
+++ b/docs/docsite/rst/porting_guides/porting_guide_2.9.rst
@@ -19,8 +19,19 @@ This document is part of a collection on porting. The complete list of porting g
 Playbook
 ========
 
+Inventory
+---------
+
  * ``hash_behaviour`` now affects inventory sources. If you have it set to ``merge``, the data you get from inventory might change and you will have to update playbooks accordingly. If you're using the default setting (``overwrite``), you will see no changes. Inventory was ignoring this setting.
 
+Loops
+-----
+
+Ansible 2.9 handles "unsafe" data more robustly, ensuring that data marked "unsafe" is not templated. In previous versions, Ansible recursively marked all data returned by the direct use of ``lookup()`` as "unsafe", but only marked structured data returned by indirect lookups using ``with_X`` style loops as "unsafe" if the returned elements were strings. Ansible 2.9 treats these two approaches consistently.
+
+As a result, if you use ``with_dict`` to return keys with templatable values, your templates may no longer work as expected in Ansible 2.9.
+
+To allow the old behavior, switch from using ``with_X`` to using ``loop`` with a filter as described at :ref:`migrating_to_loop`.
 
 Command Line
 ============
diff --git a/lib/ansible/executor/task_executor.py b/lib/ansible/executor/task_executor.py
index bcf919a1968d22..0516479adc2107 100644
--- a/lib/ansible/executor/task_executor.py
+++ b/lib/ansible/executor/task_executor.py
@@ -242,7 +242,7 @@ def _get_loop_items(self):
                 setattr(mylookup, '_subdir', subdir + 's')
 
                 # run lookup
-                items = mylookup.run(terms=loop_terms, variables=self._job_vars, wantlist=True)
+                items = wrap_var(mylookup.run(terms=loop_terms, variables=self._job_vars, wantlist=True))
             else:
                 raise AnsibleError("Unexpected failure in finding the lookup named '%s' in the available lookup plugins" % self._task.loop_with)
 
@@ -264,11 +264,6 @@ def _get_loop_items(self):
             else:
                 del self._job_vars[k]
 
-        if items:
-            for idx, item in enumerate(items):
-                if item is not None and not isinstance(item, AnsibleUnsafe):
-                    items[idx] = wrap_var(item)
-
         return items
 
     def _run_loop(self, items):
diff --git a/lib/ansible/utils/unsafe_proxy.py b/lib/ansible/utils/unsafe_proxy.py
index 3c74ea836994de..28fb10c5cb02d6 100644
--- a/lib/ansible/utils/unsafe_proxy.py
+++ b/lib/ansible/utils/unsafe_proxy.py
@@ -115,7 +115,7 @@ def _wrap_set(v):
 
 
 def wrap_var(v):
-    if isinstance(v, AnsibleUnsafe):
+    if v is None or isinstance(v, AnsibleUnsafe):
         return v
 
     if isinstance(v, Mapping):
diff --git a/lib/ansible/vars/manager.py b/lib/ansible/vars/manager.py
index d529047811c233..936a1874a1ee72 100644
--- a/lib/ansible/vars/manager.py
+++ b/lib/ansible/vars/manager.py
@@ -520,7 +520,7 @@ def _get_delegated_vars(self, play, task, existing_variables):
                 try:
                     loop_terms = listify_lookup_plugin_terms(terms=task.loop, templar=templar,
                                                              loader=self._loader, fail_on_undefined=True, convert_bare=False)
-                    items = lookup_loader.get(task.loop_with, loader=self._loader, templar=templar).run(terms=loop_terms, variables=vars_copy)
+                    items = wrap_var(lookup_loader.get(task.loop_with, loader=self._loader, templar=templar).run(terms=loop_terms, variables=vars_copy))
                 except AnsibleTemplateError:
                     # This task will be skipped later due to this, so we just setup
                     # a dummy array for the later code so it doesn't fail
diff --git a/test/integration/targets/loops/tasks/main.yml b/test/integration/targets/loops/tasks/main.yml
index 5dd7d26fe9554f..5575dd36497e36 100644
--- a/test/integration/targets/loops/tasks/main.yml
+++ b/test/integration/targets/loops/tasks/main.yml
@@ -359,3 +359,33 @@
 - assert:
     that:
       - loop_out['results'][1]['ansible_remote_tmp'] == '/tmp/test1'
+
+# https://github.com/ansible/ansible/issues/64169
+- include_vars: 64169.yml
+
+- set_fact: "{{ item.key }}={{ hostvars[inventory_hostname][item.value] }}"
+  with_dict:
+    foo: __foo
+
+- debug:
+    var: foo
+
+- assert:
+    that:
+      - foo[0] != 'foo1.0'
+      - foo[0] == unsafe_value
+  vars:
+    unsafe_value: !unsafe 'foo{{ version_64169 }}'
+
+- set_fact: "{{ item.key }}={{ hostvars[inventory_hostname][item.value] }}"
+  loop: "{{ dicty_dict|dict2items }}"
+  vars:
+    dicty_dict:
+      foo: __foo
+
+- debug:
+    var: foo
+
+- assert:
+    that:
+      - foo[0] == 'foo1.0'
diff --git a/test/integration/targets/loops/vars/64169.yml b/test/integration/targets/loops/vars/64169.yml
new file mode 100644
index 00000000000000..f48d616ac79e68
--- /dev/null
+++ b/test/integration/targets/loops/vars/64169.yml
@@ -0,0 +1,2 @@
+__foo:
+  - "foo{{ version_64169 }}"
diff --git a/test/integration/targets/loops/vars/main.yml b/test/integration/targets/loops/vars/main.yml
index 2d85bf824bb7e9..5d85370d4cce4a 100644
--- a/test/integration/targets/loops/vars/main.yml
+++ b/test/integration/targets/loops/vars/main.yml
@@ -5,3 +5,4 @@ phrases:
 filenames:
   - 'data1.txt'
   - 'data2.txt'
+version_64169: '1.0'


Code review comments:
diff_hunk: @@ -0,0 +1,5 @@
+bugfixes:
+- loops - Do not indiscrimiately mark loop items as unsafe, only apply unsafe
path: changelogs/fragments/64379-no-loop-unsafe.yml
text: @user1:
```suggestion
- loops - Do not indiscriminately mark loop items as unsafe, only apply unsafe
```

diff_hunk: @@ -0,0 +1,5 @@
+bugfixes:
+- loops - Do not indiscrimiately mark loop items as unsafe, only apply unsafe
+  to ``with_`` style loops. The items from ``loop`` should not be explicitly
+  wrapped in unsafe, the underlying templating mechanism should dictate this
path: changelogs/fragments/64379-no-loop-unsafe.yml
text: @user1:
```suggestion
  wrapped in unsafe. The underlying templating mechanism should dictate this.
```