From 13e6d8487a2fb93779da64e15018ae0a0f48a2c4 Mon Sep 17 00:00:00 2001 From: Sloane Hertel <19572925+s-hertel@users.noreply.github.com> Date: Fri, 26 Jan 2024 09:01:14 -0500 Subject: [PATCH] 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 --- .../fragments/fix-vars-plugins-in-roles.yml | 2 + lib/ansible/plugins/loader.py | 39 ++++++++------ lib/ansible/vars/plugins.py | 54 ++++++------------- .../roles/a/tasks/main.yml | 3 ++ .../roles/a/vars_plugins/auto_role_vars.py | 11 ++++ .../targets/old_style_vars_plugins/runme.sh | 2 + 6 files changed, 58 insertions(+), 53 deletions(-) create mode 100644 changelogs/fragments/fix-vars-plugins-in-roles.yml create mode 100644 test/integration/targets/old_style_vars_plugins/roles/a/tasks/main.yml create mode 100644 test/integration/targets/old_style_vars_plugins/roles/a/vars_plugins/auto_role_vars.py diff --git a/changelogs/fragments/fix-vars-plugins-in-roles.yml b/changelogs/fragments/fix-vars-plugins-in-roles.yml new file mode 100644 index 00000000000..b64d586b9ef --- /dev/null +++ b/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). diff --git a/lib/ansible/plugins/loader.py b/lib/ansible/plugins/loader.py index 90ff17b198a..caab2f689cc 100644 --- a/lib/ansible/plugins/loader.py +++ b/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 diff --git a/lib/ansible/vars/plugins.py b/lib/ansible/vars/plugins.py index 37624d9de78..c2343507f28 100644 --- a/lib/ansible/vars/plugins.py +++ b/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 diff --git a/test/integration/targets/old_style_vars_plugins/roles/a/tasks/main.yml b/test/integration/targets/old_style_vars_plugins/roles/a/tasks/main.yml new file mode 100644 index 00000000000..8e0742a5a5d --- /dev/null +++ b/test/integration/targets/old_style_vars_plugins/roles/a/tasks/main.yml @@ -0,0 +1,3 @@ +- assert: + that: + - auto_role_var is defined diff --git a/test/integration/targets/old_style_vars_plugins/roles/a/vars_plugins/auto_role_vars.py b/test/integration/targets/old_style_vars_plugins/roles/a/vars_plugins/auto_role_vars.py new file mode 100644 index 00000000000..a1cd30d3b04 --- /dev/null +++ b/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} diff --git a/test/integration/targets/old_style_vars_plugins/runme.sh b/test/integration/targets/old_style_vars_plugins/runme.sh index 71275b8ac47..9f416235bb0 100755 --- a/test/integration/targets/old_style_vars_plugins/runme.sh +++ b/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' "$@"