fix warnings about reserved variable names to cover all sources (#84432)

Also remove redundant check from tqm
Now covers module output (set_fact/include_vars)
Includes play objects at any stage (tasks that error were not covered)
Added tests, moved them to role structure

changelogs/fragments/reserved_module_chekc.yml

@@ -0,0 +1,2 @@
+bugfixes:
+  - Ansible will now also warn when reserved keywords are set via a module (set_fact, include_vars, etc).

lib/ansible/executor/task_queue_manager.py

@@ -39,7 +39,6 @@ from ansible.plugins.loader import callback_loader, strategy_loader, module_load
 from ansible.plugins.callback import CallbackBase
 from ansible.template import Templar
 from ansible.vars.hostvars import HostVars
-from ansible.vars.reserved import warn_if_reserved
 from ansible.utils.display import Display
 from ansible.utils.lock import lock_decorator
 from ansible.utils.multiprocessing import context as multiprocessing_context
@@ -282,7 +281,6 @@ class TaskQueueManager:
 
         all_vars = self._variable_manager.get_vars(play=play)
         templar = Templar(loader=self._loader, variables=all_vars)
-        warn_if_reserved(all_vars, templar.environment.globals.keys())
 
         new_play = play.copy()
         new_play.post_validate(templar)

lib/ansible/playbook/play.py

@@ -31,7 +31,6 @@ from ansible.playbook.helpers import load_list_of_blocks, load_list_of_roles
 from ansible.playbook.role import Role
 from ansible.playbook.task import Task
 from ansible.playbook.taggable import Taggable
-from ansible.vars.manager import preprocess_vars
 from ansible.utils.display import Display
 
 display = Display()
@@ -236,6 +235,9 @@ class Play(Base, Taggable, CollectionSearch):
         return self.roles
 
     def _load_vars_prompt(self, attr, ds):
+        # avoid circular dep
+        from ansible.vars.manager import preprocess_vars
+
         new_ds = preprocess_vars(ds)
         vars_prompts = []
         if new_ds is not None:

lib/ansible/vars/manager.py

@@ -39,6 +39,7 @@ from ansible.utils.vars import combine_vars, load_extra_vars, load_options_vars
 from ansible.utils.unsafe_proxy import wrap_var
 from ansible.vars.clean import namespace_facts, clean_facts
 from ansible.vars.plugins import get_vars_from_inventory_sources, get_vars_from_path
+from ansible.vars.reserved import warn_if_reserved
 
 display = Display()
 
@@ -410,6 +411,9 @@ class VariableManager:
         # extra vars
         all_vars = _combine_and_track(all_vars, self._extra_vars, "extra vars")
 
+        # before we add 'reserved vars', check we didn't add any reserved vars
+        warn_if_reserved(all_vars.keys())
+
         # magic variables
         all_vars = _combine_and_track(all_vars, magic_variables, "magic vars")
 
@@ -555,6 +559,7 @@ class VariableManager:
         if not isinstance(facts, Mapping):
             raise AnsibleAssertionError("the type of 'facts' to set for host_facts should be a Mapping but is a %s" % type(facts))
 
+        warn_if_reserved(facts.keys())
         try:
             host_cache = self._fact_cache[host]
         except KeyError:
@@ -578,6 +583,7 @@ class VariableManager:
         if not isinstance(facts, Mapping):
             raise AnsibleAssertionError("the type of 'facts' to set for nonpersistent_facts should be a Mapping but is a %s" % type(facts))
 
+        warn_if_reserved(facts.keys())
         try:
             self._nonpersistent_fact_cache[host] |= facts
         except KeyError:
@@ -587,6 +593,8 @@ class VariableManager:
         """
         Sets a value in the vars_cache for a host.
         """
+
+        warn_if_reserved(varname)
         if host not in self._vars_cache:
             self._vars_cache[host] = dict()
         if varname in self._vars_cache[host] and isinstance(self._vars_cache[host][varname], MutableMapping) and isinstance(value, MutableMapping):

lib/ansible/vars/reserved.py

@@ -21,15 +21,17 @@ from ansible.playbook import Play
 from ansible.playbook.block import Block
 from ansible.playbook.role import Role
 from ansible.playbook.task import Task
+from ansible.template import Templar
 from ansible.utils.display import Display
 
 display = Display()
 
 
-def get_reserved_names(include_private=True):
+def get_reserved_names(include_private: bool = True) -> set[str]:
     """ this function returns the list of reserved names associated with play objects"""
 
-    public = set()
+    templar = Templar(loader=None)
+    public = set(templar.environment.globals.keys())
     private = set()
     result = set()
 
@@ -61,7 +63,7 @@ def get_reserved_names(include_private=True):
     return result
 
 
-def warn_if_reserved(myvars, additional=None):
+def warn_if_reserved(myvars: list[str], additional: list[str] | None = None) -> None:
     """ this function warns if any variable passed conflicts with internally reserved names """
 
     if additional is None:
@@ -75,7 +77,7 @@ def warn_if_reserved(myvars, additional=None):
         display.warning('Found variable using reserved name: %s' % varname)
 
 
-def is_reserved_name(name):
+def is_reserved_name(name: str) -> bool:
     return name in _RESERVED_NAMES
 
 

test/integration/targets/var_reserved/runme.sh

@@ -1,5 +0,0 @@
-#!/usr/bin/env bash
-
-set -eux
-
-ansible-playbook reserved_varname_warning.yml "$@" 2>&1| grep 'Found variable using reserved name: lipsum'

test/integration/targets/var_reserved/tasks/block_vars.yml

@@ -0,0 +1,8 @@
+- hosts: localhost
+  gather_facts: false
+  tasks:
+  - name: test block
+    vars:
+      query: jinja2 uses me internally
+    block:
+    - debug:

test/integration/targets/var_reserved/tasks/main.yml

@@ -0,0 +1,23 @@
+- name: check output for warning
+  vars:
+    canary: Found variable using reserved name
+  block:
+  - shell: ansible-playbook '{{[ role_path, "tasks", item ~ ".yml"] | path_join }}'
+    environment:
+      ANSIBLE_LOCALHOST_WARNING: 0
+    failed_when: false
+    loop:
+    - play_vars
+    - block_vars
+    - task_vars
+    - task_vars_used
+    - set_fact
+    register: play_out
+
+  - name: check they all complain about bad defined var
+    assert:
+      that:
+        - canary in item['stderr']
+    loop: '{{play_out.results}}'
+    loop_control:
+      label: '{{item.item}}'

test/integration/targets/var_reserved/tasks/set_fact.yml

@@ -0,0 +1,5 @@
+- hosts: localhost
+  gather_facts: false
+  tasks:
+    - set_fact:
+        lookup: jinja2 uses me internally

test/integration/targets/var_reserved/tasks/task_vars.yml

@@ -0,0 +1,6 @@
+- hosts: localhost
+  gather_facts: false
+  tasks:
+  - debug:
+    vars:
+      query: jinja2 uses me internally

test/integration/targets/var_reserved/tasks/task_vars_used.yml

@@ -0,0 +1,8 @@
+- hosts: localhost
+  gather_facts: false
+  tasks:
+  - name: task fails due to overriding q, but we should also see warning
+    debug:
+      msg:  "{{q('pipe', 'pwd'}}"
+    vars:
+      q: jinja2 uses me internally