From 8063643b4cec51a72377da5f3fa354d3ff9e737a Mon Sep 17 00:00:00 2001 From: Sloane Hertel <19572925+s-hertel@users.noreply.github.com> Date: Mon, 7 Mar 2022 15:39:56 -0500 Subject: Fix collection redirects for filter and test plugins (#77210) * Fix collection redirects for jinja2 filters/tests * Handle recursive redirects Co-authored-by: Matt Martz --- .../77210-fix-collection-filter-test-redirects.yml | 2 + lib/ansible/template/__init__.py | 89 +++++++++++++--------- .../testns/testredirect/meta/runtime.yml | 17 +++++ test/integration/targets/collections/runme.sh | 10 +++ .../targets/collections/test_collection_meta.yml | 19 +++++ 5 files changed, 101 insertions(+), 36 deletions(-) create mode 100644 changelogs/fragments/77210-fix-collection-filter-test-redirects.yml diff --git a/changelogs/fragments/77210-fix-collection-filter-test-redirects.yml b/changelogs/fragments/77210-fix-collection-filter-test-redirects.yml new file mode 100644 index 0000000000..2175696796 --- /dev/null +++ b/changelogs/fragments/77210-fix-collection-filter-test-redirects.yml @@ -0,0 +1,2 @@ +bugfixes: + - Fix collection filter/test plugin redirects (https://github.com/ansible/ansible/issues/77192). diff --git a/lib/ansible/template/__init__.py b/lib/ansible/template/__init__.py index 894418ef52..3df7c5025e 100644 --- a/lib/ansible/template/__init__.py +++ b/lib/ansible/template/__init__.py @@ -455,6 +455,7 @@ class JinjaPluginIntercept(MutableMapping): # FUTURE: we can cache FQ filter/test calls for the entire duration of a run, since a given collection's impl's # aren't supposed to change during a run def __getitem__(self, key): + original_key = key self._load_ansible_plugins() try: @@ -469,56 +470,61 @@ class JinjaPluginIntercept(MutableMapping): if func: return func - # didn't find it in the pre-built Jinja env, assume it's a former builtin and follow the normal routing path - leaf_key = key - key = 'ansible.builtin.' + key - else: - leaf_key = key.split('.')[-1] + key, leaf_key = get_fqcr_and_name(key) + seen = set() + + while True: + if key in seen: + raise TemplateSyntaxError( + 'recursive collection redirect found for %r' % original_key, + 0 + ) + seen.add(key) - acr = AnsibleCollectionRef.try_parse_fqcr(key, self._dirname) + acr = AnsibleCollectionRef.try_parse_fqcr(key, self._dirname) - if not acr: - raise KeyError('invalid plugin name: {0}'.format(key)) + if not acr: + raise KeyError('invalid plugin name: {0}'.format(key)) - ts = _get_collection_metadata(acr.collection) + ts = _get_collection_metadata(acr.collection) - # TODO: implement support for collection-backed redirect (currently only builtin) - # TODO: implement cycle detection (unified across collection redir as well) + # TODO: implement cycle detection (unified across collection redir as well) - routing_entry = ts.get('plugin_routing', {}).get(self._dirname, {}).get(leaf_key, {}) + routing_entry = ts.get('plugin_routing', {}).get(self._dirname, {}).get(leaf_key, {}) - deprecation_entry = routing_entry.get('deprecation') - if deprecation_entry: - warning_text = deprecation_entry.get('warning_text') - removal_date = deprecation_entry.get('removal_date') - removal_version = deprecation_entry.get('removal_version') + deprecation_entry = routing_entry.get('deprecation') + if deprecation_entry: + warning_text = deprecation_entry.get('warning_text') + removal_date = deprecation_entry.get('removal_date') + removal_version = deprecation_entry.get('removal_version') - if not warning_text: - warning_text = '{0} "{1}" is deprecated'.format(self._dirname, key) + if not warning_text: + warning_text = '{0} "{1}" is deprecated'.format(self._dirname, key) - display.deprecated(warning_text, version=removal_version, date=removal_date, collection_name=acr.collection) + display.deprecated(warning_text, version=removal_version, date=removal_date, collection_name=acr.collection) - tombstone_entry = routing_entry.get('tombstone') + tombstone_entry = routing_entry.get('tombstone') - if tombstone_entry: - warning_text = tombstone_entry.get('warning_text') - removal_date = tombstone_entry.get('removal_date') - removal_version = tombstone_entry.get('removal_version') + if tombstone_entry: + warning_text = tombstone_entry.get('warning_text') + removal_date = tombstone_entry.get('removal_date') + removal_version = tombstone_entry.get('removal_version') - if not warning_text: - warning_text = '{0} "{1}" has been removed'.format(self._dirname, key) + if not warning_text: + warning_text = '{0} "{1}" has been removed'.format(self._dirname, key) - exc_msg = display.get_deprecation_message(warning_text, version=removal_version, date=removal_date, - collection_name=acr.collection, removed=True) + exc_msg = display.get_deprecation_message(warning_text, version=removal_version, date=removal_date, + collection_name=acr.collection, removed=True) - raise AnsiblePluginRemovedError(exc_msg) + raise AnsiblePluginRemovedError(exc_msg) - redirect_fqcr = routing_entry.get('redirect', None) - if redirect_fqcr: - acr = AnsibleCollectionRef.from_fqcr(ref=redirect_fqcr, ref_type=self._dirname) - display.vvv('redirecting {0} {1} to {2}.{3}'.format(self._dirname, key, acr.collection, acr.resource)) - key = redirect_fqcr - # TODO: handle recursive forwarding (not necessary for builtin, but definitely for further collection redirs) + redirect = routing_entry.get('redirect', None) + if redirect: + next_key, leaf_key = get_fqcr_and_name(redirect, collection=acr.collection) + display.vvv('redirecting (type: {0}) {1}.{2} to {3}'.format(self._dirname, acr.collection, acr.resource, next_key)) + key = next_key + else: + break func = self._collection_jinja_func_cache.get(key) @@ -593,6 +599,17 @@ class JinjaPluginIntercept(MutableMapping): return len(self._delegatee) +def get_fqcr_and_name(resource, collection='ansible.builtin'): + if '.' not in resource: + name = resource + fqcr = collection + '.' + resource + else: + name = resource.split('.')[-1] + fqcr = resource + + return fqcr, name + + @_unroll_iterator def _ansible_finalize(thing): """A custom finalize function for jinja2, which prevents None from being diff --git a/test/integration/targets/collections/collection_root_user/ansible_collections/testns/testredirect/meta/runtime.yml b/test/integration/targets/collections/collection_root_user/ansible_collections/testns/testredirect/meta/runtime.yml index da8e4901ce..2dcf456e17 100644 --- a/test/integration/targets/collections/collection_root_user/ansible_collections/testns/testredirect/meta/runtime.yml +++ b/test/integration/targets/collections/collection_root_user/ansible_collections/testns/testredirect/meta/runtime.yml @@ -2,3 +2,20 @@ plugin_routing: modules: ping: redirect: testns.testcoll.ping + filter: + multi_redirect_filter: + redirect: testns.testredirect.redirect_filter1 + deprecation: + warning_text: deprecation1 + redirect_filter1: + redirect: redirect_filter2 + deprecation: + warning_text: deprecation2 + redirect_filter2: + redirect: testns.testcoll.testfilter + deprecation: + warning_text: deprecation3 + dead_end: + redirect: bad_redirect + recursive_redirect: + redirect: recursive_redirect diff --git a/test/integration/targets/collections/runme.sh b/test/integration/targets/collections/runme.sh index d9d8a31af5..5f11abebae 100755 --- a/test/integration/targets/collections/runme.sh +++ b/test/integration/targets/collections/runme.sh @@ -65,6 +65,16 @@ else ansible-playbook -i "${INVENTORY_PATH}" collection_root_user/ansible_collections/testns/testcoll/playbooks/default_collection_playbook.yml "$@" fi +# test redirects and warnings for filter redirects +echo "testing redirect and deprecation display" +ANSIBLE_DEPRECATION_WARNINGS=yes ansible localhost -m debug -a msg='{{ "data" | testns.testredirect.multi_redirect_filter }}' -vvvvv 2>&1 | tee out.txt +cat out.txt + +test "$(grep out.txt -ce 'deprecation1' -ce 'deprecation2' -ce 'deprecation3')" == 3 +grep out.txt -e 'redirecting (type: filter) testns.testredirect.multi_redirect_filter to testns.testredirect.redirect_filter1' +grep out.txt -e 'redirecting (type: filter) testns.testredirect.redirect_filter1 to testns.testredirect.redirect_filter2' +grep out.txt -e 'redirecting (type: filter) testns.testredirect.redirect_filter2 to testns.testcoll.testfilter' + echo "--- validating collections support in playbooks/roles" # run test playbooks ansible-playbook -i "${INVENTORY_PATH}" -v "${TEST_PLAYBOOK}" "$@" diff --git a/test/integration/targets/collections/test_collection_meta.yml b/test/integration/targets/collections/test_collection_meta.yml index 22a00b2197..8e611500d3 100644 --- a/test/integration/targets/collections/test_collection_meta.yml +++ b/test/integration/targets/collections/test_collection_meta.yml @@ -21,6 +21,25 @@ # redirect filter - assert: that: ('yes' | formerly_core_filter) == True + # redirect filter (multiple levels) + - assert: + that: ('data' | testns.testredirect.multi_redirect_filter) == 'data_via_testfilter_from_userdir' + # invalid filter redirect + - debug: msg="{{ 'data' | testns.testredirect.dead_end }}" + ignore_errors: yes + register: redirect_failure + - assert: + that: + - redirect_failure is failed + - '"No filter named ''testns.testredirect.dead_end''" in redirect_failure.msg' + # recursive filter redirect + - debug: msg="{{ 'data' | testns.testredirect.recursive_redirect }}" + ignore_errors: yes + register: redirect_failure + - assert: + that: + - redirect_failure is failed + - '"recursive collection redirect found for ''testns.testredirect.recursive_redirect''" in redirect_failure.msg' # legacy filter should mask redirected - assert: that: ('' | formerly_core_masked_filter) == 'hello from overridden formerly_core_masked_filter' -- cgit v1.2.1