fix loading vars_plugins in roles (#82273)

* Fix loading legacy vars plugins when the plugin loader cache is reset

* Remove extra cache layer by ensuring vars plugin names are cached (stateless or not) so that the plugin loader cache can double as the load order

changelogs/fragments/fix-vars-plugins-in-roles.yml

@@ -0,0 +1,2 @@
+bugfixes:
+  - Fix loading vars_plugins in roles (https://github.com/ansible/ansible/issues/82239).

lib/ansible/plugins/loader.py

@@ -237,10 +237,7 @@ class PluginLoader:
         self._module_cache = MODULE_CACHE[class_name]
         self._paths = PATH_CACHE[class_name]
         self._plugin_path_cache = PLUGIN_PATH_CACHE[class_name]
-        try:
-            self._plugin_instance_cache = {} if self.type == 'vars' else None
-        except ValueError:
-            self._plugin_instance_cache = None
+        self._plugin_instance_cache = {} if self.subdir == 'vars_plugins' else None
 
         self._searched_paths = set()
 
@@ -265,7 +262,7 @@ class PluginLoader:
             self._module_cache = MODULE_CACHE[self.class_name]
             self._paths = PATH_CACHE[self.class_name]
             self._plugin_path_cache = PLUGIN_PATH_CACHE[self.class_name]
-            self._plugin_instance_cache = {} if self.type == 'vars' else None
+            self._plugin_instance_cache = {} if self.subdir == 'vars_plugins' else None
             self._searched_paths = set()
 
     def __setstate__(self, data):
@@ -871,12 +868,12 @@ class PluginLoader:
         if name in self.aliases:
             name = self.aliases[name]
 
-        if self._plugin_instance_cache and (cached_load_result := self._plugin_instance_cache.get(name)):
+        if (cached_result := (self._plugin_instance_cache or {}).get(name)) and cached_result[1].resolved:
             # Resolving the FQCN is slow, even if we've passed in the resolved FQCN.
             # Short-circuit here if we've previously resolved this name.
             # This will need to be restricted if non-vars plugins start using the cache, since
             # some non-fqcn plugin need to be resolved again with the collections list.
-            return get_with_context_result(*cached_load_result)
+            return get_with_context_result(*cached_result)
 
         plugin_load_context = self.find_plugin_with_context(name, collection_list=collection_list)
         if not plugin_load_context.resolved or not plugin_load_context.plugin_resolved_path:
@@ -888,10 +885,10 @@ class PluginLoader:
             fq_name = '.'.join((plugin_load_context.plugin_resolved_collection, fq_name))
         resolved_type_name = plugin_load_context.plugin_resolved_name
         path = plugin_load_context.plugin_resolved_path
-        if self._plugin_instance_cache and (cached_load_result := self._plugin_instance_cache.get(fq_name)):
+        if (cached_result := (self._plugin_instance_cache or {}).get(fq_name)) and cached_result[1].resolved:
             # This is unused by vars plugins, but it's here in case the instance cache expands to other plugin types.
             # We get here if we've seen this plugin before, but it wasn't called with the resolved FQCN.
-            return get_with_context_result(*cached_load_result)
+            return get_with_context_result(*cached_result)
         redirected_names = plugin_load_context.redirect_list or []
 
         if path not in self._module_cache:
@@ -934,8 +931,10 @@ class PluginLoader:
 
         self._update_object(obj, resolved_type_name, path, redirected_names, fq_name)
         if self._plugin_instance_cache is not None and getattr(obj, 'is_stateless', False):
-            # store under both the originally requested name and the resolved FQ name
-            self._plugin_instance_cache[name] = self._plugin_instance_cache[fq_name] = (obj, plugin_load_context)
+            self._plugin_instance_cache[fq_name] = (obj, plugin_load_context)
+        elif self._plugin_instance_cache is not None:
+            # The cache doubles as the load order, so record the FQCN even if the plugin hasn't set is_stateless = True
+            self._plugin_instance_cache[fq_name] = (None, PluginLoadContext())
         return get_with_context_result(obj, plugin_load_context)
 
     def _display_plugin_load(self, class_name, name, searched_paths, path, found_in_cache=None, class_only=None):
@@ -1041,9 +1040,9 @@ class PluginLoader:
             else:
                 fqcn = f"ansible.builtin.{basename}"
 
-            if self._plugin_instance_cache is not None and fqcn in self._plugin_instance_cache:
+            if (cached_result := (self._plugin_instance_cache or {}).get(fqcn)) and cached_result[1].resolved:
                 # Here just in case, but we don't call all() multiple times for vars plugins, so this should not be used.
-                yield self._plugin_instance_cache[basename][0]
+                yield cached_result[0]
                 continue
 
             if path not in self._module_cache:
@@ -1095,9 +1094,17 @@ class PluginLoader:
 
             self._update_object(obj, basename, path, resolved=fqcn)
 
-            if self._plugin_instance_cache is not None and fqcn not in self._plugin_instance_cache:
-                # Use get_with_context to cache the plugin the first time we see it.
-                self.get_with_context(fqcn)[0]
+            if self._plugin_instance_cache is not None:
+                needs_enabled = False
+                if hasattr(obj, 'REQUIRES_ENABLED'):
+                    needs_enabled = obj.REQUIRES_ENABLED
+                elif hasattr(obj, 'REQUIRES_WHITELIST'):
+                    needs_enabled = obj.REQUIRES_WHITELIST
+                    display.deprecated("The VarsModule class variable 'REQUIRES_WHITELIST' is deprecated. "
+                                       "Use 'REQUIRES_ENABLED' instead.", version=2.18)
+                if not needs_enabled:
+                    # Use get_with_context to cache the plugin the first time we see it.
+                    self.get_with_context(fqcn)[0]
 
             yield obj
 

lib/ansible/vars/plugins.py

@@ -16,41 +16,14 @@ from ansible.utils.vars import combine_vars
 
 display = Display()
 
-cached_vars_plugin_order = None
 
-
-def _load_vars_plugins_order():
+def _prime_vars_loader():
     # find 3rd party legacy vars plugins once, and look them up by name subsequently
-    auto = []
-    for auto_run_plugin in vars_loader.all(class_only=True):
-        needs_enabled = False
-        if hasattr(auto_run_plugin, 'REQUIRES_ENABLED'):
-            needs_enabled = auto_run_plugin.REQUIRES_ENABLED
-        elif hasattr(auto_run_plugin, 'REQUIRES_WHITELIST'):
-            needs_enabled = auto_run_plugin.REQUIRES_WHITELIST
-            display.deprecated("The VarsModule class variable 'REQUIRES_WHITELIST' is deprecated. "
-                               "Use 'REQUIRES_ENABLED' instead.", version=2.18)
-        if needs_enabled:
-            continue
-        auto.append(auto_run_plugin._load_name)
-
-    # find enabled plugins once so we can look them up by resolved fqcn subsequently
-    enabled = []
+    list(vars_loader.all(class_only=True))
     for plugin_name in C.VARIABLE_PLUGINS_ENABLED:
-        if (plugin := vars_loader.get(plugin_name)) is None:
-            enabled.append(plugin_name)
-        else:
-            collection = '.' in plugin.ansible_name and not plugin.ansible_name.startswith('ansible.builtin.')
-            # Warn if a collection plugin has REQUIRES_ENABLED because it has no effect.
-            if collection and (hasattr(plugin, 'REQUIRES_ENABLED') or hasattr(plugin, 'REQUIRES_WHITELIST')):
-                display.warning(
-                    "Vars plugins in collections must be enabled to be loaded, REQUIRES_ENABLED is not supported. "
-                    "This should be removed from the plugin %s." % plugin.ansible_name
-                )
-            enabled.append(plugin.ansible_name)
-
-    global cached_vars_plugin_order
-    cached_vars_plugin_order = auto + enabled
+        if not plugin_name:
+            continue
+        vars_loader.get(plugin_name)
 
 
 def get_plugin_vars(loader, plugin, path, entities):
@@ -106,16 +79,23 @@ def _plugin_should_run(plugin, stage):
 
 
 def get_vars_from_path(loader, path, entities, stage):
-
     data = {}
+    if vars_loader._paths is None:
+        # cache has been reset, reload all()
+        _prime_vars_loader()
 
-    if cached_vars_plugin_order is None:
-        _load_vars_plugins_order()
-
-    for plugin_name in cached_vars_plugin_order:
+    for plugin_name in vars_loader._plugin_instance_cache:
         if (plugin := vars_loader.get(plugin_name)) is None:
             continue
 
+        collection = '.' in plugin.ansible_name and not plugin.ansible_name.startswith('ansible.builtin.')
+        # Warn if a collection plugin has REQUIRES_ENABLED because it has no effect.
+        if collection and (hasattr(plugin, 'REQUIRES_ENABLED') or hasattr(plugin, 'REQUIRES_WHITELIST')):
+            display.warning(
+                "Vars plugins in collections must be enabled to be loaded, REQUIRES_ENABLED is not supported. "
+                "This should be removed from the plugin %s." % plugin.ansible_name
+            )
+
         if not _plugin_should_run(plugin, stage):
             continue
 

test/integration/targets/old_style_vars_plugins/roles/a/tasks/main.yml

@@ -0,0 +1,3 @@
+- assert:
+    that:
+      - auto_role_var is defined

test/integration/targets/old_style_vars_plugins/roles/a/vars_plugins/auto_role_vars.py

@@ -0,0 +1,11 @@
+from __future__ import annotations
+
+from ansible.plugins.vars import BaseVarsPlugin
+
+
+class VarsModule(BaseVarsPlugin):
+    # Implicitly
+    # REQUIRES_ENABLED = False
+
+    def get_vars(self, loader, path, entities):
+        return {'auto_role_var': True}

test/integration/targets/old_style_vars_plugins/runme.sh

@@ -46,3 +46,5 @@ ANSIBLE_DEBUG=True ansible-playbook test_task_vars.yml > out.txt
 [ "$(grep -c "Loading VarsModule 'host_group_vars'" out.txt)" -eq 1 ]
 [ "$(grep -c "Loading VarsModule 'require_enabled'" out.txt)" -lt 3 ]
 [ "$(grep -c "Loading VarsModule 'auto_enabled'" out.txt)" -gt 50 ]
+
+ansible localhost -m include_role -a 'name=a' "$@"