fix loading vars_plugins in roles (#82273) (#82609)

* 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

(cherry picked from commit 13e6d8487a)

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

@@ -238,10 +238,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.subdir == 'vars_plugins' else None
-            self._plugin_instance_cache = {} if self.type == 'vars' else None
-        except ValueError:
-            self._plugin_instance_cache = None
 
         self._searched_paths = set()
 
@@ -266,7 +263,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):
@@ -872,12 +869,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:
@@ -889,10 +886,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:
@@ -935,8 +932,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[fq_name] = (obj, plugin_load_context)
-            self._plugin_instance_cache[name] = 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):
@@ -1042,9 +1041,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:
@@ -1096,7 +1095,15 @@ 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:
+            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]
 

lib/ansible/vars/plugins.py

@@ -16,41 +16,14 @@ from ansible.utils.vars import combine_vars
 
 display = Display()
 
-cached_vars_plugin_order = None
 
-
+def _prime_vars_loader():
-def _load_vars_plugins_order():
     # find 3rd party legacy vars plugins once, and look them up by name subsequently
-    auto = []
+    list(vars_loader.all(class_only=True))
-    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 = []
     for plugin_name in C.VARIABLE_PLUGINS_ENABLED:
-        if (plugin := vars_loader.get(plugin_name)) is None:
+        if not plugin_name:
-            enabled.append(plugin_name)
+            continue
-        else:
+        vars_loader.get(plugin_name)
-            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
 
 
 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:
+    for plugin_name in vars_loader._plugin_instance_cache:
-        _load_vars_plugins_order()
-
-    for plugin_name in cached_vars_plugin_order:
         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' "$@"