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

* 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

(cherry picked from commit 20baf29a2a)

* fix template (#84563)

also fix gather_subset warning and add some comments/notes
---------

Co-authored-by: Abhijeet Kasurde <akasurde@redhat.com>
(cherry picked from commit 675d7201d8)

* reserved vars, avoid gather_subset (#84575)

(cherry picked from commit 3398c102b5)

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/module_utils/facts/ansible_collector.py

@@ -113,7 +113,13 @@ class CollectorMetaDataCollector(collector.BaseFactCollector):
         self.module_setup = module_setup
 
     def collect(self, module=None, collected_facts=None):
+        # NOTE: deprecate/remove once DT lands
+        # we can return this data, but should not be top level key
         meta_facts = {'gather_subset': self.gather_subset}
+
+        # NOTE: this is just a boolean indicator that 'facts were gathered'
+        # and should be moved to the 'gather_facts' action plugin
+        # probably revised to handle modules/subsets combos
         if self.module_setup:
             meta_facts['module_setup'] = self.module_setup
         return meta_facts

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, hash_params
 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()
@@ -245,6 +244,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

@@ -41,6 +41,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()
 
@@ -417,6 +418,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")
 
@@ -703,6 +707,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:
@@ -726,15 +731,18 @@ 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:
             self._nonpersistent_fact_cache[host] = facts
 
     def set_host_variable(self, host, varname, value):
-        '''
+        """
         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):
-    ''' this function returns the list of reserved names associated with play objects'''
+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()
 
@@ -58,11 +60,15 @@ def get_reserved_names(include_private=True):
     else:
         result = public
 
+    # due to Collectors always adding, need to ignore this
+    # eventually should remove after we deprecate it in setup.py
+    result.remove('gather_subset')
+
     return result
 
 
-def warn_if_reserved(myvars, additional=None):
-    ''' this function warns if any variable passed conflicts with internally reserved names '''
+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:
         reserved = _RESERVED_NAMES
@@ -75,7 +81,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