From 29c6aae2fcffa92a693ef3278b94e97fff1ebbd3 Mon Sep 17 00:00:00 2001 From: Matt Davis Date: Tue, 16 Jun 2020 17:17:38 -0700 Subject: [PATCH] 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 --- .../config/ansible_builtin_runtime.yml | 7 ++++ lib/ansible/executor/task_queue_manager.py | 30 +++++++++++---- .../testns/testcoll/meta/runtime.yml | 4 ++ test/integration/targets/collections/noop.yml | 4 ++ test/integration/targets/collections/runme.sh | 37 ++++++++++++++----- 5 files changed, 65 insertions(+), 17 deletions(-) create mode 100644 test/integration/targets/collections/noop.yml diff --git a/lib/ansible/config/ansible_builtin_runtime.yml b/lib/ansible/config/ansible_builtin_runtime.yml index b412518ab19..e7d35fb3d87 100644 --- a/lib/ansible/config/ansible_builtin_runtime.yml +++ b/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 diff --git a/lib/ansible/executor/task_queue_manager.py b/lib/ansible/executor/task_queue_manager.py index cd6c396860c..e7277c9183c 100644 --- a/lib/ansible/executor/task_queue_manager.py +++ b/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 diff --git a/test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/meta/runtime.yml b/test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/meta/runtime.yml index cb21fee66fa..879d1a03626 100644 --- a/test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/meta/runtime.yml +++ b/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 diff --git a/test/integration/targets/collections/noop.yml b/test/integration/targets/collections/noop.yml new file mode 100644 index 00000000000..81c6e473827 --- /dev/null +++ b/test/integration/targets/collections/noop.yml @@ -0,0 +1,4 @@ +- hosts: localhost + gather_facts: no + tasks: + - debug: diff --git a/test/integration/targets/collections/runme.sh b/test/integration/targets/collections/runme.sh index 15cd43f2a01..d830bf48b9e 100755 --- a/test/integration/targets/collections/runme.sh +++ b/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