make collection callbacks follow normal flow (#59932)

make collections whitelist follow normal flow

* fixes missing set_options call and adhoc and stdout processing rules
* avoid dupes
* fixed to handle redirects
* also updated tests with new and more accurate skip message
* fix callback tests for envs with cowsay installed
* lots MOAR comments on why the code is as it is, some todos to refactor in future

Co-authored-by: Matt Davis <nitzmahone@users.noreply.github.com>

changelogs/fragments/collections_cb_fix.yml

@@ -0,0 +1,2 @@
+bugfixes:
+    - Collection callbacks were ignoring options and rules for stdout and adhoc cases.

lib/ansible/executor/task_queue_manager.py

@@ -171,49 +171,70 @@ class TaskQueueManager:
         else:
             raise AnsibleError("callback must be an instance of CallbackBase or the name of a callback plugin")
 
-        loaded_callbacks = set()
+        # get all configured loadable callbacks (adjacent, builtin)
+        callback_list = list(callback_loader.all(class_only=True))
+
+        # add whitelisted callbacks that refer to collections, which might not appear in normal listing
+        for c in C.DEFAULT_CALLBACK_WHITELIST:
+            # load all, as collection ones might be using short/redirected names and not a fqcn
+            plugin = callback_loader.get(c, class_only=True)
+
+            # TODO: check if this skip is redundant, loader should handle bad file/plugin cases already
+            if plugin:
+                # avoids incorrect and dupes possible due to collections
+                if plugin not in callback_list:
+                    callback_list.append(plugin)
+            else:
+                display.warning("Skipping callback plugin '%s', unable to load" % c)
+
+        # for each callback in the list see if we should add it to 'active callbacks' used in the play
+        for callback_plugin in callback_list:
 
-        # 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)
+
+            # try to get colleciotn world name first
+            cnames = getattr(callback_plugin, '_redirected_names', [])
+            if cnames:
+                # store the name the plugin was loaded as, as that's what we'll need to compare to the configured callback list later
+                callback_name = cnames[0]
+            else:
+                # fallback to 'old loader name'
                 (callback_name, _) = os.path.splitext(os.path.basename(callback_plugin._original_path))
+
+            display.vvvvv("Attempting to use '%s' callback." % (callback_name))
             if callback_type == 'stdout':
                 # we only allow one callback of type 'stdout' to be loaded,
                 if callback_name != self._stdout_callback or stdout_callback_loaded:
+                    display.vv("Skipping callback '%s', as we already have a stdout callback." % (callback_name))
                     continue
                 stdout_callback_loaded = True
             elif callback_name == 'tree' and self._run_tree:
-                # special case for ansible cli option
+                # TODO: remove special case for tree, which is an adhoc cli option --tree
                 pass
-            # eg, ad-hoc doesn't allow non-default callbacks
             elif not self._run_additional_callbacks or (callback_needs_whitelist and (
+                # only run if not adhoc, or adhoc was specifically configured to run + check enabled list
                     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
 
+            try:
                 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)
-
-        # 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)
+                # avoid bad plugin not returning an object, only needed cause we do class_only load and bypass loader checks,
+                # really a bug in the plugin itself which we ignore as callback errors are not supposed to be fatal.
                 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)
+                    # skip initializing if we already did the work for the same plugin (even with diff names)
+                    if callback_obj not in self._callback_plugins:
                         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)
+                        display.vv("Skipping callback '%s', already loaded as '%s'." % (callback_plugin, callback_name))
+                else:
+                    display.warning("Skipping callback '%s', as it does not create a valid plugin instance." % callback_name)
+                    continue
+            except Exception as e:
+                display.warning("Skipping callback '%s', unable to load due to: %s" % (callback_name, to_native(e)))
+                continue
 
         self._callbacks_loaded = True
 
@@ -364,10 +385,7 @@ class TaskQueueManager:
 
             # a plugin can opt in to implicit tasks (such as meta). It does this
             # by declaring self.wants_implicit_tasks = True.
-            wants_implicit_tasks = getattr(
-                callback_plugin,
-                'wants_implicit_tasks',
-                False)
+            wants_implicit_tasks = getattr(callback_plugin, 'wants_implicit_tasks', False)
 
             # try to find v2 method, fallback to v1 method, ignore callback if no method found
             methods = []

test/integration/targets/callback_default/runme.sh

@@ -16,6 +16,9 @@ set -eux
 run_test() {
 	local testname=$1
 
+	# outout was recorded w/o cowsay, ensure we reproduce the same
+	export ANSIBLE_NOCOWS=1
+
 	# The shenanigans with redirection and 'tee' are to capture STDOUT and
 	# STDERR separately while still displaying both to the console
 	{ ansible-playbook -i inventory test.yml \
@@ -36,6 +39,9 @@ run_test_dryrun() {
 	# optional, pass --check to run a dry run
 	local chk=${2:-}
 
+	# outout was recorded w/o cowsay, ensure we reproduce the same
+	export ANSIBLE_NOCOWS=1
+
 	# This needed to satisfy shellcheck that can not accept unquoted variable
 	cmd="ansible-playbook -i inventory ${chk} test_dryrun.yml"
 

test/integration/targets/collections/runme.sh

@@ -18,14 +18,14 @@ ANSIBLE_CALLBACK_WHITELIST=testns.testcoll.usercallback ansible-playbook noop.ym
 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)
+## validate missing redirected callback
+ANSIBLE_CALLBACK_WHITELIST=formerly_core_missing_callback ansible localhost -m debug 2>&1 | grep -- "Skipping callback plugin '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'"
+# validate avoiding duplicate loading of callback, even if using diff names
+[ "$(ANSIBLE_CALLBACK_WHITELIST=testns.testcoll.usercallback,formerly_core_callback ansible localhost -m debug 2>&1 | grep -c 'usercallback says ok')" = "1" ]
 # 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'"
+ANSIBLE_CALLBACK_WHITELIST=charlie.gomez.notme ansible localhost -m debug 2>&1 | grep -- "Skipping callback plugin '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)