Fix vars plugin code and documentation mismatch (#78562)

* Add a warning for collections that are attempting to be autoloaded to no effect
* Deprecate REQUIRES_WHITELIST and add support for REQUIRES_ENABLED so the docs are accurate
* Fix documentation
* add more vars plugin tests
* Simplify code and add a FIXME for another bug
* fix precedence
* Make setting the class attr at all a warning, even if it's True
* Add fun parsing for _load_name
* include _load_name in messages

changelogs/fragments/78562-deprecate-vars-plugin-attr.yml

@@ -0,0 +1,2 @@
+deprecated_features:
+  - vars plugins - determining whether or not to run ansible.legacy vars plugins with the class attribute REQUIRES_WHITELIST is deprecated, set REQUIRES_ENABLED instead.

docs/docsite/rst/dev_guide/developing_collections_structure.rst

@@ -94,7 +94,9 @@ plugins directory
 
 Add a 'per plugin type' specific subdirectory here, including ``module_utils`` which is usable not only by modules, but by most plugins by using their FQCN. This is a way to distribute modules, lookups, filters, and so on without having to import a role in every play.
 
-Vars plugins are supported in collections as long as they require being explicitly enabled (using ``REQUIRES_ENABLED``) and they are included using their fully qualified collection name. See :ref:`enable_vars` and :ref:`developing_vars_plugins` for details. Cache plugins may be used in collections for fact caching, but are not supported for inventory plugins.
+Vars plugins in collections are not loaded automatically, and always require being explicitly enabled by using their fully qualified collection name. See :ref:`enable_vars` for details.
+
+Cache plugins in collections may be used for fact caching, but are not supported for inventory plugins.
 
 .. _collection_module_utils:
 

docs/docsite/rst/dev_guide/developing_plugins.rst

@@ -501,7 +501,9 @@ This ``get_vars`` method just needs to return a dictionary structure with the va
 
 Since Ansible version 2.4, vars plugins only execute as needed when preparing to execute a task. This avoids the costly 'always execute' behavior that occurred during inventory construction in older versions of Ansible. Since Ansible version 2.10, vars plugin execution can be toggled by the user to run when preparing to execute a task or after importing an inventory source.
 
-You can create vars plugins that are not enabled by default using the class variable ``REQUIRES_ENABLED``. If your vars plugin resides in a collection, it cannot be enabled by default. You must use ``REQUIRES_ENABLED`` in all collections-based vars plugins. To require users to enable your plugin, set the class variable ``REQUIRES_ENABLED``:
+The user must explicitly enable vars plugins that reside in a collection. See :ref:`enable_vars` for details.
+
+Legacy vars plugins are always loaded and run by default. You can prevent them from automatically running by setting ``REQUIRES_ENABLED`` to True.
 
 .. code-block:: python
 

lib/ansible/plugins/vars/host_group_vars.py

@@ -67,7 +67,7 @@ FOUND = {}  # type: dict[str, list[str]]
 
 class VarsModule(BaseVarsPlugin):
 
-    REQUIRES_WHITELIST = True
+    REQUIRES_ENABLED = True
 
     def get_vars(self, loader, path, entities, cache=True):
         ''' parses the inventory file '''

lib/ansible/vars/plugins.py

@@ -54,9 +54,44 @@ def get_vars_from_path(loader, path, entities, stage):
                 vars_plugin_list.append(vars_plugin)
 
     for plugin in vars_plugin_list:
-        if plugin._load_name not in C.VARIABLE_PLUGINS_ENABLED and getattr(plugin, 'REQUIRES_WHITELIST', False):
-            # 2.x plugins shipped with ansible should require enabling, older or non shipped should load automatically
-            continue
+        # legacy plugins always run by default, but they can set REQUIRES_ENABLED=True to opt out.
+
+        # The name in config corresponds to the following _load_name:
+        #   - legacy_plugin == legacy_plugin
+        #   - ansible.legacy.legacy_plugin == legacy_plugin
+        #   - builtin_plugin == builtin_plugin
+        #   - ansible.builtin.builtin_plugin == ansible_collections.ansible.builtin.plugins.vars.builtin_plugin
+        builtin_or_legacy = '.' not in plugin._load_name or plugin._load_name.startswith('ansible_collections.ansible.builtin.')
+
+        # builtin is supposed to have REQUIRES_ENABLED=True, the following is for legacy plugins...
+        needs_enabled = not builtin_or_legacy
+        if hasattr(plugin, 'REQUIRES_ENABLED'):
+            needs_enabled = plugin.REQUIRES_ENABLED
+        elif hasattr(plugin, 'REQUIRES_WHITELIST'):
+            display.deprecated("The VarsModule class variable 'REQUIRES_WHITELIST' is deprecated. "
+                               "Use 'REQUIRES_ENABLED' instead.", version=2.18)
+            needs_enabled = plugin.REQUIRES_WHITELIST
+
+        # A collection plugin was enabled to get to this point because vars_loader.all() does not include collection plugins.
+        # Warn if a collection plugin has REQUIRES_ENABLED because it has no effect.
+        if not builtin_or_legacy 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._load_name  # FIXME: display ns.coll.resource instead of _load_name
+            )
+        elif builtin_or_legacy and plugin._load_name not in C.VARIABLE_PLUGINS_ENABLED and needs_enabled:
+            # Maybe it was enabled by FQCN.
+            is_builtin = plugin._load_name == 'ansible_collections.ansible.builtin.plugins.vars.host_group_vars'
+            if is_builtin:
+                fqcn_builtin = 'ansible.builtin.host_group_vars'
+                fqcn_legacy = 'ansible.legacy.host_group_vars'
+                if fqcn_builtin not in C.VARIABLE_PLUGINS_ENABLED and fqcn_legacy not in C.VARIABLE_PLUGINS_ENABLED:
+                    continue
+            else:
+                # legacy plugin
+                fqcn_legacy = 'ansible.legacy.%s' % plugin._load_name
+                if fqcn_legacy not in C.VARIABLE_PLUGINS_ENABLED:
+                    continue
 
         has_stage = hasattr(plugin, 'get_option') and plugin.has_option('stage')
 

test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/plugins/vars/custom_vars.py

@@ -39,6 +39,10 @@ from ansible.plugins.vars import BaseVarsPlugin
 
 class VarsModule(BaseVarsPlugin):
 
+    # Vars plugins in collections are only loaded when they are enabled by the user.
+    # If a vars plugin sets REQUIRES_ENABLED = False, a warning should occur (assuming it is loaded).
+    REQUIRES_ENABLED = False
+
     def get_vars(self, loader, path, entities, cache=True):
         super(VarsModule, self).get_vars(loader, path, entities)
         return {'collection': 'collection_root_user'}

test/integration/targets/collections/vars_plugin_tests.sh

@@ -10,19 +10,21 @@ export ANSIBLE_RUN_VARS_PLUGINS=start
 # Test vars plugin in a playbook-adjacent collection
 export ANSIBLE_VARS_ENABLED=testns.content_adj.custom_adj_vars
 
-ansible-inventory -i a.statichost.yml --list --playbook-dir=./ | tee out.txt
+ansible-inventory -i a.statichost.yml --list --playbook-dir=./ 2>&1 | tee out.txt
 
 grep '"collection": "adjacent"' out.txt
 grep '"adj_var": "value"' out.txt
+grep -v "REQUIRES_ENABLED is not supported" out.txt
 
 # Test vars plugin in a collection path
 export ANSIBLE_VARS_ENABLED=testns.testcoll.custom_vars
 export ANSIBLE_COLLECTIONS_PATH=$PWD/collection_root_user:$PWD/collection_root_sys
 
-ansible-inventory -i a.statichost.yml --list --playbook-dir=./ | tee out.txt
+ansible-inventory -i a.statichost.yml --list --playbook-dir=./ 2>&1 | tee out.txt
 
 grep '"collection": "collection_root_user"' out.txt
 grep -v '"adj_var": "value"' out.txt
+grep "REQUIRES_ENABLED is not supported" out.txt
 
 # Test enabled vars plugins order reflects the order in which variables are merged
 export ANSIBLE_VARS_ENABLED=testns.content_adj.custom_adj_vars,testns.testcoll.custom_vars

test/integration/targets/old_style_vars_plugins/aliases

@@ -0,0 +1,2 @@
+shippable/posix/group3
+context/controller

test/integration/targets/old_style_vars_plugins/deprecation_warning/vars.py

@@ -0,0 +1,8 @@
+from ansible.plugins.vars import BaseVarsPlugin
+
+
+class VarsModule(BaseVarsPlugin):
+    REQUIRES_WHITELIST = False
+
+    def get_vars(self, loader, path, entities):
+        return {}

test/integration/targets/old_style_vars_plugins/runme.sh

@@ -0,0 +1,20 @@
+#!/usr/bin/env bash
+
+set -eux
+
+export ANSIBLE_VARS_PLUGINS=./vars_plugins
+
+# Test vars plugin without REQUIRES_ENABLED class attr and vars plugin with REQUIRES_ENABLED = False run by default
+[ "$(ansible-inventory -i localhost, --list --yaml all "$@" | grep -Ec '(implicitly|explicitly)_auto_enabled')" = "2" ]
+
+# Test vars plugin with REQUIRES_ENABLED=True only runs when enabled
+[ "$(ansible-inventory -i localhost, --list --yaml all "$@" | grep -Ec 'require_enabled')" = "0" ]
+export ANSIBLE_VARS_ENABLED=require_enabled
+[ "$(ansible-inventory -i localhost, --list --yaml all "$@" | grep -c 'require_enabled')" = "1" ]
+
+# Test the deprecated class attribute
+export ANSIBLE_VARS_PLUGINS=./deprecation_warning
+WARNING="The VarsModule class variable 'REQUIRES_WHITELIST' is deprecated. Use 'REQUIRES_ENABLED' instead."
+ANSIBLE_DEPRECATION_WARNINGS=True ANSIBLE_NOCOLOR=True ANSIBLE_FORCE_COLOR=False \
+	ansible-inventory -i localhost, --list all 2> err.txt
+ansible localhost -m debug -a "msg={{ lookup('file', 'err.txt') | regex_replace('\n', '') }}" | grep "$WARNING"

test/integration/targets/old_style_vars_plugins/vars_plugins/auto_enabled.py

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

test/integration/targets/old_style_vars_plugins/vars_plugins/implicitly_auto_enabled.py

@@ -0,0 +1,7 @@
+from ansible.plugins.vars import BaseVarsPlugin
+
+
+class VarsModule(BaseVarsPlugin):
+
+    def get_vars(self, loader, path, entities):
+        return {'implicitly_auto_enabled': True}

test/integration/targets/old_style_vars_plugins/vars_plugins/require_enabled.py

@@ -0,0 +1,8 @@
+from ansible.plugins.vars import BaseVarsPlugin
+
+
+class VarsModule(BaseVarsPlugin):
+    REQUIRES_ENABLED = True
+
+    def get_vars(self, loader, path, entities):
+        return {'require_enabled': True}