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

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)

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
 

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__ = '<ansible_synthetic_collection_package>'
@@ -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):

test/integration/targets/collections_plugin_namespace/aliases

@@ -0,0 +1,2 @@
+shippable/posix/group1
+skip/python2.6

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

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__]

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__]

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,
+        }

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

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 "$@"

test/integration/targets/collections_plugin_namespace/test.yml

@@ -0,0 +1,3 @@
+- hosts: testhost
+  roles:
+    - my_ns.my_col.test

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