From 1c64dba3c9800deabb45b75bfd17b951b66e0eee Mon Sep 17 00:00:00 2001 From: Matt Clay Date: Wed, 18 Sep 2019 17:47:56 -0700 Subject: [PATCH] Fix plugin names for collection plugins. (#60317) * Fix plugin names for collection plugins. Add an integration test to verify plugin __name__ is correct for collection plugins. * Fix collection loader PEP 302 compliance. The `find_module` function now returns `None` if the module cannot be found. Previously it would return `self` for modules which did not exist. Returning a loader from `find_module` which cannot find the module will result in import errors on Python 2.x when using implicit relative imports. * add changelog * sanity/units/merge fixes --- .../collection_loader_import_fixes.yml | 2 + lib/ansible/plugins/loader.py | 34 ++++++++++------ lib/ansible/utils/collection_loader.py | 40 +++++++++++++++---- .../collections_plugin_namespace/aliases | 2 + .../my_col/plugins/filter/test_filter.py | 15 +++++++ .../my_col/plugins/lookup/lookup_name.py | 9 +++++ .../lookup/lookup_no_future_boilerplate.py | 10 +++++ .../my_ns/my_col/plugins/test/test_test.py | 13 ++++++ .../my_ns/my_col/roles/test/tasks/main.yml | 12 ++++++ .../collections_plugin_namespace/runme.sh | 5 +++ .../collections_plugin_namespace/test.yml | 3 ++ test/sanity/ignore.txt | 1 + 12 files changed, 126 insertions(+), 20 deletions(-) create mode 100644 changelogs/fragments/collection_loader_import_fixes.yml create mode 100644 test/integration/targets/collections_plugin_namespace/aliases create mode 100644 test/integration/targets/collections_plugin_namespace/collection_root/ansible_collections/my_ns/my_col/plugins/filter/test_filter.py create mode 100644 test/integration/targets/collections_plugin_namespace/collection_root/ansible_collections/my_ns/my_col/plugins/lookup/lookup_name.py create mode 100644 test/integration/targets/collections_plugin_namespace/collection_root/ansible_collections/my_ns/my_col/plugins/lookup/lookup_no_future_boilerplate.py create mode 100644 test/integration/targets/collections_plugin_namespace/collection_root/ansible_collections/my_ns/my_col/plugins/test/test_test.py create mode 100644 test/integration/targets/collections_plugin_namespace/collection_root/ansible_collections/my_ns/my_col/roles/test/tasks/main.yml create mode 100755 test/integration/targets/collections_plugin_namespace/runme.sh create mode 100644 test/integration/targets/collections_plugin_namespace/test.yml diff --git a/changelogs/fragments/collection_loader_import_fixes.yml b/changelogs/fragments/collection_loader_import_fixes.yml new file mode 100644 index 00000000000..ecdef5e293b --- /dev/null +++ b/changelogs/fragments/collection_loader_import_fixes.yml @@ -0,0 +1,2 @@ +bugfixes: + - collection loader - fixed relative imports on Python 2.7, ensure pluginloader caches use full name to prevent names from being clobbered (https://github.com/ansible/ansible/pull/60317) diff --git a/lib/ansible/plugins/loader.py b/lib/ansible/plugins/loader.py index 625563cdf23..74e7be2c492 100644 --- a/lib/ansible/plugins/loader.py +++ b/lib/ansible/plugins/loader.py @@ -322,6 +322,8 @@ class PluginLoader: acr = AnsibleCollectionRef.from_fqcr(fq_name, plugin_type) n_resource = to_native(acr.resource, errors='strict') + # we want this before the extension is added + full_name = '{0}.{1}'.format(acr.n_python_package_name, n_resource) if extension: n_resource += extension @@ -336,10 +338,10 @@ class PluginLoader: if hasattr(pkg, '__loader__') and isinstance(pkg.__loader__, AnsibleFlatMapLoader): try: file_path = pkg.__loader__.find_file(n_resource) - return to_text(file_path) + return full_name, to_text(file_path) except IOError: # this loader already takes care of extensionless files, so if we didn't find it, just bail - return None + return None, None pkg_path = os.path.dirname(pkg.__file__) @@ -347,27 +349,31 @@ class PluginLoader: # FIXME: and is file or file link or ... if os.path.exists(n_resource_path): - return to_text(n_resource_path) + return full_name, to_text(n_resource_path) # look for any matching extension in the package location (sans filter) ext_blacklist = ['.pyc', '.pyo'] found_files = [f for f in glob.iglob(os.path.join(pkg_path, n_resource) + '.*') if os.path.isfile(f) and os.path.splitext(f)[1] not in ext_blacklist] if not found_files: - return None + return None, None if len(found_files) > 1: # TODO: warn? pass - return to_text(found_files[0]) + return full_name, to_text(found_files[0]) def find_plugin(self, name, mod_type='', ignore_deprecated=False, check_aliases=False, collection_list=None): ''' Find a plugin named name ''' + return self.find_plugin_with_name(name, mod_type, ignore_deprecated, check_aliases, collection_list)[1] + + def find_plugin_with_name(self, name, mod_type='', ignore_deprecated=False, check_aliases=False, collection_list=None): + ''' Find a plugin named name ''' global _PLUGIN_FILTERS if name in _PLUGIN_FILTERS[self.package]: - return None + return None, None if mod_type: suffix = mod_type @@ -392,22 +398,23 @@ class PluginLoader: # HACK: refactor this properly if candidate_name.startswith('ansible.legacy'): # just pass the raw name to the old lookup function to check in all the usual locations + full_name = name p = self._find_plugin_legacy(name.replace('ansible.legacy.', '', 1), ignore_deprecated, check_aliases, suffix) else: - p = self._find_fq_plugin(candidate_name, suffix) + full_name, p = self._find_fq_plugin(candidate_name, suffix) if p: - return p + return full_name, p except Exception as ex: errors.append(to_native(ex)) if errors: display.debug(msg='plugin lookup for {0} failed; errors: {1}'.format(name, '; '.join(errors))) - return None + return None, None # if we got here, there's no collection list and it's not an FQ name, so do legacy lookup - return self._find_plugin_legacy(name, ignore_deprecated, check_aliases, suffix) + return name, self._find_plugin_legacy(name, ignore_deprecated, check_aliases, suffix) def _find_plugin_legacy(self, name, ignore_deprecated=False, check_aliases=False, suffix=None): @@ -501,7 +508,10 @@ class PluginLoader: def _load_module_source(self, name, path): # avoid collisions across plugins - full_name = '.'.join([self.package, name]) + if name.startswith('ansible_collections.'): + full_name = name + else: + full_name = '.'.join([self.package, name]) if full_name in sys.modules: # Avoids double loading, See https://github.com/ansible/ansible/issues/13110 @@ -534,7 +544,7 @@ class PluginLoader: collection_list = kwargs.pop('collection_list', None) if name in self.aliases: name = self.aliases[name] - path = self.find_plugin(name, collection_list=collection_list) + name, path = self.find_plugin_with_name(name, collection_list=collection_list) if path is None: return None diff --git a/lib/ansible/utils/collection_loader.py b/lib/ansible/utils/collection_loader.py index 0fc18948c92..81e3f4fac02 100644 --- a/lib/ansible/utils/collection_loader.py +++ b/lib/ansible/utils/collection_loader.py @@ -103,15 +103,29 @@ class AnsibleCollectionLoader(with_metaclass(Singleton, object)): return self._default_collection def find_module(self, fullname, path=None): - # this loader is only concerned with items under the Ansible Collections namespace hierarchy, ignore others - if fullname and fullname.split('.', 1)[0] == 'ansible_collections': + if self._find_module(fullname, path, load=False)[0]: return self return None def load_module(self, fullname): + mod = self._find_module(fullname, None, load=True)[1] + + if not mod: + raise ImportError('module {0} not found'.format(fullname)) + + return mod + + def _find_module(self, fullname, path, load): + # this loader is only concerned with items under the Ansible Collections namespace hierarchy, ignore others + if not fullname.startswith('ansible_collections.') and fullname != 'ansible_collections': + return False, None + if sys.modules.get(fullname): - return sys.modules[fullname] + if not load: + return True, None + + return True, sys.modules[fullname] newmod = None @@ -153,14 +167,21 @@ class AnsibleCollectionLoader(with_metaclass(Singleton, object)): if not map_package: raise KeyError('invalid synthetic map package definition (no target "map" defined)') + + if not load: + return True, None + mod = import_module(map_package + synpkg_remainder) sys.modules[fullname] = mod - return mod + return True, mod elif pkg_type == 'flatmap': raise NotImplementedError() elif pkg_type == 'pkg_only': + if not load: + return True, None + newmod = ModuleType(fullname) newmod.__package__ = fullname newmod.__file__ = '' @@ -170,7 +191,7 @@ class AnsibleCollectionLoader(with_metaclass(Singleton, object)): if not synpkg_def.get('allow_external_subpackages'): # if external subpackages are NOT allowed, we're done sys.modules[fullname] = newmod - return newmod + return True, newmod # if external subpackages ARE allowed, check for on-disk implementations and return a normal # package if we find one, otherwise return the one we created here @@ -196,6 +217,9 @@ class AnsibleCollectionLoader(with_metaclass(Singleton, object)): if not os.path.isfile(to_bytes(source_path)): continue + if not load: + return True, None + with open(to_bytes(source_path), 'rb') as fd: source = fd.read() @@ -227,16 +251,16 @@ class AnsibleCollectionLoader(with_metaclass(Singleton, object)): # FIXME: decide cases where we don't actually want to exec the code? exec(code_object, newmod.__dict__) - return newmod + return True, newmod # even if we didn't find one on disk, fall back to a synthetic package if we have one... if newmod: sys.modules[fullname] = newmod - return newmod + return True, newmod # FIXME: need to handle the "no dirs present" case for at least the root and synthetic internal collections like ansible.builtin - raise ImportError('module {0} not found'.format(fullname)) + return False, None @staticmethod def _extend_path_with_ns(path, ns): diff --git a/test/integration/targets/collections_plugin_namespace/aliases b/test/integration/targets/collections_plugin_namespace/aliases new file mode 100644 index 00000000000..54ea5a3b078 --- /dev/null +++ b/test/integration/targets/collections_plugin_namespace/aliases @@ -0,0 +1,2 @@ +shippable/posix/group1 +skip/python2.6 diff --git a/test/integration/targets/collections_plugin_namespace/collection_root/ansible_collections/my_ns/my_col/plugins/filter/test_filter.py b/test/integration/targets/collections_plugin_namespace/collection_root/ansible_collections/my_ns/my_col/plugins/filter/test_filter.py new file mode 100644 index 00000000000..dca094be8a4 --- /dev/null +++ b/test/integration/targets/collections_plugin_namespace/collection_root/ansible_collections/my_ns/my_col/plugins/filter/test_filter.py @@ -0,0 +1,15 @@ +from __future__ import (absolute_import, division, print_function) +__metaclass__ = type + + +def filter_name(a): + return __name__ + + +class FilterModule(object): + def filters(self): + filters = { + 'filter_name': filter_name, + } + + return filters diff --git a/test/integration/targets/collections_plugin_namespace/collection_root/ansible_collections/my_ns/my_col/plugins/lookup/lookup_name.py b/test/integration/targets/collections_plugin_namespace/collection_root/ansible_collections/my_ns/my_col/plugins/lookup/lookup_name.py new file mode 100644 index 00000000000..d0af703bbdc --- /dev/null +++ b/test/integration/targets/collections_plugin_namespace/collection_root/ansible_collections/my_ns/my_col/plugins/lookup/lookup_name.py @@ -0,0 +1,9 @@ +from __future__ import (absolute_import, division, print_function) +__metaclass__ = type + +from ansible.plugins.lookup import LookupBase + + +class LookupModule(LookupBase): + def run(self, terms, variables, **kwargs): + return [__name__] diff --git a/test/integration/targets/collections_plugin_namespace/collection_root/ansible_collections/my_ns/my_col/plugins/lookup/lookup_no_future_boilerplate.py b/test/integration/targets/collections_plugin_namespace/collection_root/ansible_collections/my_ns/my_col/plugins/lookup/lookup_no_future_boilerplate.py new file mode 100644 index 00000000000..79e80f625ae --- /dev/null +++ b/test/integration/targets/collections_plugin_namespace/collection_root/ansible_collections/my_ns/my_col/plugins/lookup/lookup_no_future_boilerplate.py @@ -0,0 +1,10 @@ +# do not add future boilerplate to this plugin +# specifically, do not add absolute_import, as the purpose of this plugin is to test implicit relative imports on Python 2.x +__metaclass__ = type + +from ansible.plugins.lookup import LookupBase + + +class LookupModule(LookupBase): + def run(self, terms, variables, **kwargs): + return [__name__] diff --git a/test/integration/targets/collections_plugin_namespace/collection_root/ansible_collections/my_ns/my_col/plugins/test/test_test.py b/test/integration/targets/collections_plugin_namespace/collection_root/ansible_collections/my_ns/my_col/plugins/test/test_test.py new file mode 100644 index 00000000000..1739072f9d9 --- /dev/null +++ b/test/integration/targets/collections_plugin_namespace/collection_root/ansible_collections/my_ns/my_col/plugins/test/test_test.py @@ -0,0 +1,13 @@ +from __future__ import (absolute_import, division, print_function) +__metaclass__ = type + + +def test_name_ok(value): + return __name__ == 'ansible_collections.my_ns.my_col.plugins.test.test_test' + + +class TestModule: + def tests(self): + return { + 'test_name_ok': test_name_ok, + } diff --git a/test/integration/targets/collections_plugin_namespace/collection_root/ansible_collections/my_ns/my_col/roles/test/tasks/main.yml b/test/integration/targets/collections_plugin_namespace/collection_root/ansible_collections/my_ns/my_col/roles/test/tasks/main.yml new file mode 100644 index 00000000000..d80f5470c69 --- /dev/null +++ b/test/integration/targets/collections_plugin_namespace/collection_root/ansible_collections/my_ns/my_col/roles/test/tasks/main.yml @@ -0,0 +1,12 @@ +- set_fact: + filter_name: "{{ 1 | my_ns.my_col.filter_name }}" + lookup_name: "{{ lookup('my_ns.my_col.lookup_name') }}" + lookup_no_future_boilerplate: "{{ lookup('my_ns.my_col.lookup_no_future_boilerplate') }}" + test_name_ok: "{{ 1 is my_ns.my_col.test_name_ok }}" + +- assert: + that: + - filter_name == 'ansible_collections.my_ns.my_col.plugins.filter.test_filter' + - lookup_name == 'ansible_collections.my_ns.my_col.plugins.lookup.lookup_name' + - lookup_no_future_boilerplate == 'ansible_collections.my_ns.my_col.plugins.lookup.lookup_no_future_boilerplate' + - test_name_ok diff --git a/test/integration/targets/collections_plugin_namespace/runme.sh b/test/integration/targets/collections_plugin_namespace/runme.sh new file mode 100755 index 00000000000..5800750358d --- /dev/null +++ b/test/integration/targets/collections_plugin_namespace/runme.sh @@ -0,0 +1,5 @@ +#!/usr/bin/env bash + +set -eux + +ANSIBLE_COLLECTIONS_PATHS="${PWD}/collection_root" ansible-playbook test.yml -i ../../inventory "$@" diff --git a/test/integration/targets/collections_plugin_namespace/test.yml b/test/integration/targets/collections_plugin_namespace/test.yml new file mode 100644 index 00000000000..d1c3f1b7a51 --- /dev/null +++ b/test/integration/targets/collections_plugin_namespace/test.yml @@ -0,0 +1,3 @@ +- hosts: testhost + roles: + - my_ns.my_col.test diff --git a/test/sanity/ignore.txt b/test/sanity/ignore.txt index cbfdc95cc68..6bd809bd3f3 100644 --- a/test/sanity/ignore.txt +++ b/test/sanity/ignore.txt @@ -5949,6 +5949,7 @@ test/integration/targets/async_fail/library/async_test.py future-import-boilerpl test/integration/targets/async_fail/library/async_test.py metaclass-boilerplate test/integration/targets/aws_lambda/files/mini_lambda.py future-import-boilerplate test/integration/targets/aws_lambda/files/mini_lambda.py metaclass-boilerplate +test/integration/targets/collections_plugin_namespace/collection_root/ansible_collections/my_ns/my_col/plugins/lookup/lookup_no_future_boilerplate.py future-import-boilerplate test/integration/targets/collections_relative_imports/collection_root/ansible_collections/my_ns/my_col/plugins/module_utils/my_util2.py pylint:relative-beyond-top-level test/integration/targets/collections_relative_imports/collection_root/ansible_collections/my_ns/my_col/plugins/module_utils/my_util3.py pylint:relative-beyond-top-level test/integration/targets/collections_relative_imports/collection_root/ansible_collections/my_ns/my_col/plugins/modules/my_module.py pylint:relative-beyond-top-level