try to load unqualified plugins from whitelist (#70086)

* try to load unqualified plugins from whitelist

* necessary for backcompat loading of unqualified collectionized callback plugins redirected from <= 2.9 core
* also added de-duping from actual loaded name

* add tests

* add warning test

* group test script entries by topic

* shorten warning text grep because wrapping is dumb

* fix adhoc callback loading behavior

* collections pass over whitelist wasn't respecting `_run_additional_callbacks`
* adds regression tests for same

* avoid `grep -L` in tests since it breaks the world

lib/ansible/config/ansible_builtin_runtime.yml

@@ -8290,6 +8290,13 @@ plugin_routing:
       redirect: ansible.posix.timer
     foreman:
       redirect: theforeman.foreman.foreman
+    # 'collections' integration test entries, do not remove
+    formerly_core_callback:
+      redirect: testns.testcoll.usercallback
+    formerly_core_removed_callback:
+      redirect: testns.testcoll.removedcallback
+    formerly_core_missing_callback:
+      redirect: bogusns.boguscoll.boguscallback
   doc_fragments:
     a10:
       redirect: community.network.a10

lib/ansible/executor/task_queue_manager.py

@@ -138,6 +138,9 @@ class TaskQueueManager:
         else:
             raise AnsibleError("callback must be an instance of CallbackBase or the name of a callback plugin")
 
+        loaded_callbacks = set()
+
+        # first, load callbacks in the core distribution and configured callback paths
         for callback_plugin in callback_loader.all(class_only=True):
             callback_type = getattr(callback_plugin, 'CALLBACK_TYPE', '')
             callback_needs_whitelist = getattr(callback_plugin, 'CALLBACK_NEEDS_WHITELIST', False)
@@ -150,23 +153,34 @@ class TaskQueueManager:
             elif callback_name == 'tree' and self._run_tree:
                 # special case for ansible cli option
                 pass
+            # eg, ad-hoc doesn't allow non-default callbacks
             elif not self._run_additional_callbacks or (callback_needs_whitelist and (
                     C.DEFAULT_CALLBACK_WHITELIST is None or callback_name not in C.DEFAULT_CALLBACK_WHITELIST)):
                 # 2.x plugins shipped with ansible should require whitelisting, older or non shipped should load automatically
                 continue
 
             callback_obj = callback_plugin()
+            loaded_callbacks.add(callback_name)  # mark as loaded so we skip in second pass
             callback_obj.set_options()
             self._callback_plugins.append(callback_obj)
 
-        for callback_plugin_name in (c for c in C.DEFAULT_CALLBACK_WHITELIST if AnsibleCollectionRef.is_valid_fqcr(c)):
-            # TODO: need to extend/duplicate the stdout callback check here (and possible move this ahead of the old way
-            callback_obj = callback_loader.get(callback_plugin_name)
-            if callback_obj:
-                callback_obj.set_options()
-                self._callback_plugins.append(callback_obj)
-            else:
-                display.warning("Skipping '%s', unable to load or use as a callback" % callback_plugin_name)
+        # eg, ad-hoc doesn't allow non-default callbacks
+        if self._run_additional_callbacks:
+            # Second pass over everything in the whitelist we haven't already loaded, try to explicitly load. This will catch
+            # collection-hosted callbacks, as well as formerly-core callbacks that have been redirected to collections.
+            for callback_plugin_name in (c for c in C.DEFAULT_CALLBACK_WHITELIST if c not in loaded_callbacks):
+                # TODO: need to extend/duplicate the stdout callback check here (and possible move this ahead of the old way
+                callback_obj, plugin_load_context = callback_loader.get_with_context(callback_plugin_name)
+                if callback_obj:
+                    loaded_as_name = callback_obj._redirected_names[-1]
+                    if loaded_as_name in loaded_callbacks:
+                        display.warning("Skipping callback '%s', already loaded as '%s'." % (callback_plugin_name, loaded_as_name))
+                        continue
+                    loaded_callbacks.add(loaded_as_name)
+                    callback_obj.set_options()
+                    self._callback_plugins.append(callback_obj)
+                else:
+                    display.warning("Skipping '%s', unable to load or use as a callback" % callback_plugin_name)
 
         self._callbacks_loaded = True
 

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

@@ -2,6 +2,10 @@ plugin_routing:
   action:
     uses_redirected_action:
       redirect: testns.testcoll.subclassed_normal
+  callback:
+    removedcallback:
+      tombstone:
+        removal_date: '2020-01-01'
   connection:
     redirected_local:
       redirect: ansible.builtin.local

test/integration/targets/collections/noop.yml

@@ -0,0 +1,4 @@
+- hosts: localhost
+  gather_facts: no
+  tasks:
+    - debug:

test/integration/targets/collections/runme.sh

@@ -7,14 +7,31 @@ export ANSIBLE_GATHERING=explicit
 export ANSIBLE_GATHER_SUBSET=minimal
 export ANSIBLE_HOST_PATTERN_MISMATCH=error
 
-
 # FUTURE: just use INVENTORY_PATH as-is once ansible-test sets the right dir
 ipath=../../$(basename "${INVENTORY_PATH:-../../inventory}")
 export INVENTORY_PATH="$ipath"
 
-# test callback
-ANSIBLE_CALLBACK_WHITELIST=testns.testcoll.usercallback ansible localhost -m ping | grep "usercallback says ok"
+echo "--- validating callbacks"
+# validate FQ callbacks in ansible-playbook
+ANSIBLE_CALLBACK_WHITELIST=testns.testcoll.usercallback ansible-playbook noop.yml | grep "usercallback says ok"
+# use adhoc for the rest of these tests, must force it to load other callbacks
+export ANSIBLE_LOAD_CALLBACK_PLUGINS=1
+# validate redirected callback
+ANSIBLE_CALLBACK_WHITELIST=formerly_core_callback ansible localhost -m debug 2>&1 | grep -- "usercallback says ok"
+# validate missing redirected callback
+ANSIBLE_CALLBACK_WHITELIST=formerly_core_missing_callback ansible localhost -m debug 2>&1 | grep -- "Skipping 'formerly_core_missing_callback'"
+# validate redirected + removed callback (fatal)
+ANSIBLE_CALLBACK_WHITELIST=formerly_core_removed_callback ansible localhost -m debug 2>&1 | grep -- "testns.testcoll.removedcallback has been removed"
+# validate warning on logical duplicate (FQ + unqualified that are the same)
+ANSIBLE_CALLBACK_WHITELIST=testns.testcoll.usercallback,formerly_core_callback ansible localhost -m debug 2>&1 | grep -- "Skipping callback 'formerly_core_callback'"
+# ensure non existing callback does not crash ansible
+ANSIBLE_CALLBACK_WHITELIST=charlie.gomez.notme ansible localhost -m debug 2>&1 | grep -- "Skipping 'charlie.gomez.notme'"
+unset ANSIBLE_LOAD_CALLBACK_PLUGINS
+# adhoc normally shouldn't load non-default plugins- let's be sure
+output=$(ANSIBLE_CALLBACK_WHITELIST=testns.testcoll.usercallback ansible localhost -m debug)
+if [[ "${output}" =~ "usercallback says ok" ]]; then echo fail; exit 1; fi
 
+echo "--- validating docs"
 # test documentation
 ansible-doc testns.testcoll.testmodule -vvv | grep -- "- normal_doc_frag"
 # same with symlink
@@ -23,14 +40,16 @@ ansible-doc testns.testcoll2.testmodule2 -vvv | grep "Test module"
 # now test we can list with symlink
 ansible-doc -l -vvv| grep "testns.testcoll2.testmodule2"
 
-# test adhoc default collection resolution (use unqualified collection module with playbook dir under its collection)
-echo "testing adhoc default collection support with explicit playbook dir"
-ANSIBLE_PLAYBOOK_DIR=./collection_root_user/ansible_collections/testns/testcoll ansible localhost -m testmodule
-
 echo "testing bad doc_fragments (expected ERROR message follows)"
 # test documentation failure
 ansible-doc testns.testcoll.testmodule_bad_docfrags -vvv 2>&1 | grep -- "unknown doc_fragment"
 
+echo "--- validating default collection"
+# test adhoc default collection resolution (use unqualified collection module with playbook dir under its collection)
+
+echo "testing adhoc default collection support with explicit playbook dir"
+ANSIBLE_PLAYBOOK_DIR=./collection_root_user/ansible_collections/testns/testcoll ansible localhost -m testmodule
+
 # we need multiple plays, and conditional import_playbook is noisy and causes problems, so choose here which one to use...
 if [[ ${INVENTORY_PATH} == *.winrm ]]; then
   export TEST_PLAYBOOK=windows.yml
@@ -41,6 +60,7 @@ else
   ansible-playbook -i "${INVENTORY_PATH}" collection_root_user/ansible_collections/testns/testcoll/playbooks/default_collection_playbook.yml "$@"
 fi
 
+echo "--- validating collections support in playbooks/roles"
 # run test playbooks
 ansible-playbook -i "${INVENTORY_PATH}" -v "${TEST_PLAYBOOK}" "$@"
 
@@ -48,6 +68,7 @@ if [[ ${INVENTORY_PATH} != *.winrm ]]; then
 	ansible-playbook -i "${INVENTORY_PATH}" -v invocation_tests.yml "$@"
 fi
 
+echo "--- validating inventory"
 # test collection inventories
 ansible-playbook inventory_test.yml -i a.statichost.yml -i redirected.statichost.yml "$@"
 
@@ -85,5 +106,3 @@ fi
 
 ./vars_plugin_tests.sh
 
-# ensure non existing callback does not crash ansible
-ANSIBLE_CALLBACK_WHITELIST=charlie.gomez.notme ansible -m ping localhost