Don't pollute include_variables (#54687)

* Don't pollute include_variables. Fixes #51667. Fixes #54618.

* Rename include_variables to include_args, so we can make the distinction about what they are

* Track args and vars separately

* oops

* oops again

* linting fix

* Add test

lib/ansible/executor/task_executor.py

@@ -559,18 +559,18 @@ def _execute(self, variables=None):
         # if this task is a TaskInclude, we just return now with a success code so the
         # main thread can expand the task list for the given host
         if self._task.action in ('include', 'include_tasks'):
-            include_variables = self._task.args.copy()
-            include_file = include_variables.pop('_raw_params', None)
+            include_args = self._task.args.copy()
+            include_file = include_args.pop('_raw_params', None)
             if not include_file:
                 return dict(failed=True, msg="No include file was specified to the include")
 
             include_file = templar.template(include_file)
-            return dict(include=include_file, include_variables=include_variables)
+            return dict(include=include_file, include_args=include_args)
 
         # if this task is a IncludeRole, we just return now with a success code so the main thread can expand the task list for the given host
         elif self._task.action == 'include_role':
-            include_variables = self._task.args.copy()
-            return dict(include_variables=include_variables)
+            include_args = self._task.args.copy()
+            return dict(include_args=include_args)
 
         # Now we do final validation on the task, which sets all fields to their final values.
         self._task.post_validate(templar=templar)

lib/ansible/playbook/included_file.py

@@ -33,9 +33,10 @@
 
 class IncludedFile:
 
-    def __init__(self, filename, args, task, is_role=False):
+    def __init__(self, filename, args, vars, task, is_role=False):
         self._filename = filename
         self._args = args
+        self._vars = vars
         self._task = task
         self._hosts = []
         self._is_role = is_role
@@ -47,10 +48,13 @@ def add_host(self, host):
         raise ValueError()
 
     def __eq__(self, other):
-        return other._filename == self._filename and other._args == self._args and other._task._parent._uuid == self._task._parent._uuid
+        return (other._filename == self._filename and
+                other._args == self._args and
+                other._vars == self._vars and
+                other._task._parent._uuid == self._task._parent._uuid)
 
     def __repr__(self):
-        return "%s (%s): %s" % (self._filename, self._args, self._hosts)
+        return "%s (args=%s vars=%s): %s" % (self._filename, self._args, self._vars, self._hosts)
 
     @staticmethod
     def process_include_results(results, iterator, loader, variable_manager):
@@ -81,20 +85,21 @@ def process_include_results(results, iterator, loader, variable_manager):
                     except KeyError:
                         task_vars = task_vars_cache[cache_key] = variable_manager.get_vars(play=iterator._play, host=original_host, task=original_task)
 
-                    include_variables = include_result.get('include_variables', dict())
+                    include_args = include_result.get('include_args', dict())
+                    special_vars = {}
                     loop_var = 'item'
                     index_var = None
                     if original_task.loop_control:
                         loop_var = original_task.loop_control.loop_var
                         index_var = original_task.loop_control.index_var
                     if loop_var in include_result:
-                        task_vars[loop_var] = include_variables[loop_var] = include_result[loop_var]
+                        task_vars[loop_var] = special_vars[loop_var] = include_result[loop_var]
                     if index_var and index_var in include_result:
-                        task_vars[index_var] = include_variables[index_var] = include_result[index_var]
+                        task_vars[index_var] = special_vars[index_var] = include_result[index_var]
                     if '_ansible_item_label' in include_result:
-                        task_vars['_ansible_item_label'] = include_variables['_ansible_item_label'] = include_result['_ansible_item_label']
-                    if original_task.no_log and '_ansible_no_log' not in include_variables:
-                        task_vars['_ansible_no_log'] = include_variables['_ansible_no_log'] = original_task.no_log
+                        task_vars['_ansible_item_label'] = special_vars['_ansible_item_label'] = include_result['_ansible_item_label']
+                    if original_task.no_log and '_ansible_no_log' not in include_args:
+                        task_vars['_ansible_no_log'] = special_vars['_ansible_no_log'] = original_task.no_log
 
                     # get search path for this task to pass to lookup plugins that may be used in pathing to
                     # the included file
@@ -166,21 +171,21 @@ def process_include_results(results, iterator, loader, variable_manager):
                                 include_file = loader.path_dwim(include_result['include'])
 
                         include_file = templar.template(include_file)
-                        inc_file = IncludedFile(include_file, include_variables, original_task)
+                        inc_file = IncludedFile(include_file, include_args, special_vars, original_task)
                     else:
                         # template the included role's name here
-                        role_name = include_variables.pop('name', include_variables.pop('role', None))
+                        role_name = include_args.pop('name', include_args.pop('role', None))
                         if role_name is not None:
                             role_name = templar.template(role_name)
 
                         new_task = original_task.copy()
                         new_task._role_name = role_name
                         for from_arg in new_task.FROM_ARGS:
-                            if from_arg in include_variables:
+                            if from_arg in include_args:
                                 from_key = from_arg.replace('_from', '')
-                                new_task._from_files[from_key] = templar.template(include_variables.pop(from_arg))
+                                new_task._from_files[from_key] = templar.template(include_args.pop(from_arg))
 
-                        inc_file = IncludedFile(role_name, include_variables, new_task, is_role=True)
+                        inc_file = IncludedFile(role_name, include_args, special_vars, new_task, is_role=True)
 
                     idx = 0
                     orig_inc_file = inc_file

lib/ansible/plugins/strategy/__init__.py

@@ -762,7 +762,7 @@ def _copy_included_file(self, included_file):
         ti_copy._parent = included_file._task._parent
 
         temp_vars = ti_copy.vars.copy()
-        temp_vars.update(included_file._args)
+        temp_vars.update(included_file._vars)
 
         ti_copy.vars = temp_vars
 

test/integration/targets/include_import/roles/loop_name_assert/tasks/main.yml

@@ -0,0 +1,4 @@
+- assert:
+    that:
+      - name == 'name_from_loop_var'
+      - name != 'loop_name_assert'

test/integration/targets/include_import/runme.sh

@@ -94,3 +94,6 @@ test "$(egrep -c 'include handler task|ERROR! The requested handler '"'"'do_impo
 # https://github.com/ansible/ansible/issues/49969
 ansible-playbook -v parent_templating/playbook.yml 2>&1 | tee test_parent_templating.out
 test "$(egrep -c 'Templating the path of the parent include_tasks failed.' test_parent_templating.out)" = 0
+
+# https://github.com/ansible/ansible/issues/54618
+ansible-playbook test_loop_var_bleed.yaml "$@"

test/integration/targets/include_import/test_loop_var_bleed.yaml

@@ -0,0 +1,9 @@
+- hosts: localhost
+  gather_facts: false
+  tasks:
+    - include_role:
+        name: loop_name_assert
+      loop:
+        - name_from_loop_var
+      loop_control:
+        loop_var: name

test/units/playbook/test_included_file.py

@@ -51,11 +51,12 @@ def mock_variable_manager():
 def test_included_file_instantiation():
     filename = 'somefile.yml'
 
-    inc_file = IncludedFile(filename=filename, args=[], task=None)
+    inc_file = IncludedFile(filename=filename, args={}, vars={}, task=None)
 
     assert isinstance(inc_file, IncludedFile)
     assert inc_file._filename == filename
-    assert inc_file._args == []
+    assert inc_file._args == {}
+    assert inc_file._vars == {}
     assert inc_file._task is None
 
 
@@ -84,6 +85,7 @@ def test_process_include_results(mock_iterator, mock_variable_manager):
     assert res[0]._filename == os.path.join(os.getcwd(), 'include_test.yml')
     assert res[0]._hosts == ['testhost1', 'testhost2']
     assert res[0]._args == {}
+    assert res[0]._vars == {}
 
 
 def test_process_include_diff_files(mock_iterator, mock_variable_manager):
@@ -124,6 +126,9 @@ def test_process_include_diff_files(mock_iterator, mock_variable_manager):
     assert res[0]._args == {}
     assert res[1]._args == {}
 
+    assert res[0]._vars == {}
+    assert res[1]._vars == {}
+
 
 def test_process_include_simulate_free(mock_iterator, mock_variable_manager):
     hostname = "testhost1"
@@ -159,3 +164,6 @@ def test_process_include_simulate_free(mock_iterator, mock_variable_manager):
 
     assert res[0]._args == {}
     assert res[1]._args == {}
+
+    assert res[0]._vars == {}
+    assert res[1]._vars == {}