Fix a bug adding unrelated candidates to the plugin loader redirect_list (#73863)

* Fix a bug adding unrelated candidates to the plugin loader redirect_list

* Add tests for the redirect list

  * test redirect list for builtin module
  * test redirect list for redirected builtin module
  * test redirect list for collection module
  * test redirect list for redirected collection module
  * test redirect list for legacy module

* changelog

changelogs/fragments/73863-fix-plugin-redirect-list.yaml

@@ -0,0 +1,2 @@
+bugfixes:
+  - Fix adding unrelated candidate names to the plugin loader redirect list.

lib/ansible/plugins/loader.py

@@ -476,6 +476,11 @@ class PluginLoader:
             if redirect:
                 # FIXME: remove once this is covered in debug or whatever
                 display.vv("redirecting (type: {0}) {1} to {2}".format(plugin_type, fq_name, redirect))
+                # The name doing the redirection is added at the beginning of _resolve_plugin_step,
+                # but if the unqualified name is used in conjunction with the collections keyword, only
+                # the unqualified name is in the redirect list.
+                if fq_name not in plugin_load_context.redirect_list:
+                    plugin_load_context.redirect_list.append(fq_name)
                 return plugin_load_context.redirect(redirect)
                 # TODO: non-FQCN case, do we support `.` prefix for current collection, assume it with no dots, require it for subdirs in current, or ?
 
@@ -603,7 +608,12 @@ class PluginLoader:
                         # 'ansible.builtin' should be handled here. This means only internal, or builtin, paths are searched.
                         plugin_load_context = self._find_fq_plugin(candidate_name, suffix, plugin_load_context=plugin_load_context)
 
-                        if candidate_name != plugin_load_context.original_name and candidate_name not in plugin_load_context.redirect_list:
+                        # Pending redirects are added to the redirect_list at the beginning of _resolve_plugin_step.
+                        # Once redirects are resolved, ensure the final FQCN is added here.
+                        # e.g. 'ns.coll.module' is included rather than only 'module' if a collections list is provided:
+                        # - module:
+                        #   collections: ['ns.coll']
+                        if plugin_load_context.resolved and candidate_name not in plugin_load_context.redirect_list:
                             plugin_load_context.redirect_list.append(candidate_name)
 
                     if plugin_load_context.resolved or plugin_load_context.pending_redirect:  # if we got an answer or need to chase down a redirect, return

test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/plugins/action/plugin_lookup.py

@@ -15,19 +15,26 @@ class ActionModule(ActionBase):
 
         result = super(ActionModule, self).run(None, task_vars)
 
-        type = self._task.args.get('type')
+        plugin_type = self._task.args.get('type')
         name = self._task.args.get('name')
 
         result = dict(changed=False, collection_list=self._task.collections)
 
-        if all([type, name]):
+        if all([plugin_type, name]):
-            attr_name = '{0}_loader'.format(type)
+            attr_name = '{0}_loader'.format(plugin_type)
 
             typed_loader = getattr(loader, attr_name, None)
 
             if not typed_loader:
-                return (dict(failed=True, msg='invalid plugin type {0}'.format(type)))
+                return (dict(failed=True, msg='invalid plugin type {0}'.format(plugin_type)))
 
-            result['plugin_path'] = typed_loader.find_plugin(name, collection_list=self._task.collections)
+            context = typed_loader.find_plugin_with_context(name, collection_list=self._task.collections)
+
+            if not context.resolved:
+                result['plugin_path'] = None
+                result['redirect_list'] = []
+            else:
+                result['plugin_path'] = context.plugin_resolved_path
+                result['redirect_list'] = context.redirect_list
 
         return result

test/integration/targets/collections/collection_root_user/ansible_collections/testns/testredirect/meta/runtime.yml

@@ -0,0 +1,4 @@
+plugin_routing:
+  modules:
+    ping:
+      redirect: testns.testcoll.ping

test/integration/targets/collections/runme.sh

@@ -101,6 +101,9 @@ fi
 # test collection inventories
 ansible-playbook inventory_test.yml -i a.statichost.yml -i redirected.statichost.yml "$@"
 
+# test plugin loader redirect_list
+ansible-playbook test_redirect_list.yml -v "$@"
+
 # test adjacent with --playbook-dir
 export ANSIBLE_COLLECTIONS_PATH=''
 ANSIBLE_INVENTORY_ANY_UNPARSED_IS_FAILED=1 ansible-inventory --list --export --playbook-dir=. -v "$@"

test/integration/targets/collections/test_redirect_list.yml

@@ -0,0 +1,86 @@
+---
+- hosts: localhost
+  gather_facts: no
+  module_defaults:
+    testns.testcoll.plugin_lookup:
+      type: module
+  tasks:
+  - name: test builtin
+    testns.testcoll.plugin_lookup:
+      name: dnf
+    register: result
+    failed_when:
+    - result['redirect_list'] != ['dnf'] or result['plugin_path'].endswith('library/dnf.py')
+
+  - name: test builtin with collections kw
+    testns.testcoll.plugin_lookup:
+      name: dnf
+    register: result
+    failed_when:
+    - result['redirect_list'] != ['dnf'] or result['plugin_path'].endswith('library/dnf.py')
+    collections:
+      - testns.unrelatedcoll
+
+  - name: test redirected builtin
+    testns.testcoll.plugin_lookup:
+      name: formerly_core_ping
+    register: result
+    failed_when: result['redirect_list'] != expected_redirect_list
+    vars:
+      expected_redirect_list:
+        - formerly_core_ping
+        - ansible.builtin.formerly_core_ping
+        - testns.testcoll.ping
+
+  - name: test redirected builtin with collections kw
+    testns.testcoll.plugin_lookup:
+      name: formerly_core_ping
+    register: result
+    failed_when: result['redirect_list'] != expected_redirect_list
+    vars:
+      expected_redirect_list:
+        - formerly_core_ping
+        - ansible.builtin.formerly_core_ping
+        - testns.testcoll.ping
+    collections:
+      - testns.unrelatedcoll
+      - testns.testcoll
+
+  - name: test collection module with collections kw
+    testns.testcoll.plugin_lookup:
+      name: ping
+    register: result
+    failed_when: result['redirect_list'] != expected_redirect_list
+    vars:
+      expected_redirect_list:
+        - ping
+        - testns.testcoll.ping
+    collections:
+      - testns.unrelatedcoll
+      - testns.testcoll
+
+  - name: test redirected collection module with collections kw
+    testns.testcoll.plugin_lookup:
+      name: ping
+    register: result
+    failed_when: result['redirect_list'] != expected_redirect_list
+    vars:
+      expected_redirect_list:
+        - ping
+        - testns.testredirect.ping
+        - testns.testcoll.ping
+    collections:
+      - testns.unrelatedcoll
+      - testns.testredirect
+
+  - name: test legacy module with collections kw
+    testns.testcoll.plugin_lookup:
+      name: ping
+    register: result
+    failed_when:
+    - result['redirect_list'] != expected_redirect_list or not result['plugin_path'].endswith('library/ping.py')
+    vars:
+      expected_redirect_list:
+        - ping
+    collections:
+      - testns.unrelatedcoll