module_utils fixes in collections (#55118)

* module_utils fixes in collections

* fixed Windows module_utils in collections
* fixed more Python module_utils cases (from X import module)
* "medium style" Ansiballz modules now work properly with collections (ie, non-replacer but also not using basic.py)
* added more tests
* split Windows/POSIX exec

* sanity

lib/ansible/executor/module_common.py

@@ -473,7 +473,15 @@ class ModuleDepFinder(ast.NodeVisitor):
 
         elif node.module.startswith('ansible_collections.'):
             # TODO: finish out the subpackage et al cases
-            self.submodules.add(tuple(node.module.split('.')))
+            if node.module.endswith('plugins.module_utils'):
+                # from ansible_collections.ns.coll.plugins.module_utils import MODULE [as aname] [,MODULE2] [as aname]
+                py_mod = tuple(node.module.split('.'))
+                for alias in node.names:
+                    self.submodules.add(py_mod + (alias.name,))
+            else:
+                # from ansible_collections.ns.coll.plugins.module_utils.MODULE import IDENTIFIER [as aname]
+                self.submodules.add(tuple(node.module.split('.')))
+
         self.generic_visit(node)
 
 
@@ -762,7 +770,9 @@ def _find_module_utils(module_name, b_module_data, module_path, module_args, tas
         module_style = 'new'
         module_substyle = 'python'
         b_module_data = b_module_data.replace(REPLACER, b'from ansible.module_utils.basic import *')
-    elif b'from ansible.module_utils.' in b_module_data:
+    # FUTURE: combined regex for this stuff, or a "looks like Python, let's inspect further" mechanism
+    elif b'from ansible.module_utils.' in b_module_data or b'from ansible_collections.' in b_module_data\
+            or b'import ansible_collections.' in b_module_data:
         module_style = 'new'
         module_substyle = 'python'
     elif REPLACER_WINDOWS in b_module_data:
@@ -772,6 +782,7 @@ def _find_module_utils(module_name, b_module_data, module_path, module_args, tas
     elif re.search(b'#Requires -Module', b_module_data, re.IGNORECASE) \
             or re.search(b'#Requires -Version', b_module_data, re.IGNORECASE)\
             or re.search(b'#AnsibleRequires -OSVersion', b_module_data, re.IGNORECASE) \
+            or re.search(b'#AnsibleRequires -Powershell', b_module_data, re.IGNORECASE) \
             or re.search(b'#AnsibleRequires -CSharpUtil', b_module_data, re.IGNORECASE):
         module_style = 'new'
         module_substyle = 'powershell'

lib/ansible/executor/powershell/module_manifest.py

@@ -35,8 +35,9 @@ class PSModuleDepFinder(object):
         self.os_version = None
         self.become = False
 
-        self._re_cs_module = re.compile(to_bytes(r'(?i)^using\s(Ansible\..+);$'))
-        self._re_cs_in_ps_module = re.compile(to_bytes(r'(?i)^#\s*ansiblerequires\s+-csharputil\s+(Ansible\..+)'))
+        self._re_cs_module = re.compile(to_bytes(r'(?i)^using\s((Ansible\..+)|(AnsibleCollections\.\w.+\.\w.+\w.+));\s*$'))
+        self._re_cs_in_ps_module = re.compile(to_bytes(r'(?i)^#\s*ansiblerequires\s+-csharputil\s+((Ansible\..+)|(AnsibleCollections\.\w.+\.\w.+\w.+))'))
+        self._re_coll_ps_in_ps_module = re.compile(to_bytes(r'(?i)^#\s*ansiblerequires\s+-powershell\s+((Ansible\..+)|(AnsibleCollections\.\w.+\.\w.+\w.+))'))
         self._re_module = re.compile(to_bytes(r'(?i)^#\s*requires\s+\-module(?:s?)\s*(Ansible\.ModuleUtils\..+)'))
         self._re_wrapper = re.compile(to_bytes(r'(?i)^#\s*ansiblerequires\s+-wrapper\s+(\w*)'))
         self._re_ps_version = re.compile(to_bytes(r'(?i)^#requires\s+\-version\s+([0-9]+(\.[0-9]+){0,3})$'))
@@ -55,12 +56,14 @@ class PSModuleDepFinder(object):
             checks = [
                 # PS module contains '#Requires -Module Ansible.ModuleUtils.*'
                 (self._re_module, self.ps_modules, ".psm1"),
+                # PS module contains '#AnsibleRequires -Powershell Ansible.*' (or FQ collections module_utils ref)
+                (self._re_coll_ps_in_ps_module, self.ps_modules, ".psm1"),
                 # PS module contains '#AnsibleRequires -CSharpUtil Ansible.*'
                 (self._re_cs_in_ps_module, cs_utils, ".cs"),
             ]
         else:
             checks = [
-                # CS module contains 'using Ansible.*;'
+                # CS module contains 'using Ansible.*;' or 'using AnsibleCollections.ns.coll.*;'
                 (self._re_cs_module, cs_utils, ".cs"),
             ]
 
@@ -70,7 +73,8 @@ class PSModuleDepFinder(object):
                 if match:
                     # tolerate windows line endings by stripping any remaining
                     # newline chars
-                    module_util_name = to_text(match.group(1).rstrip())
+                    module_util_name = self._normalize_mu_name(match.group(1).rstrip())
+
                     if module_util_name not in check[1].keys():
                         module_utils.add((module_util_name, check[2]))
 
@@ -156,6 +160,15 @@ class PSModuleDepFinder(object):
             if LooseVersion(new_version) > LooseVersion(existing_version):
                 setattr(self, attribute, new_version)
 
+    def _normalize_mu_name(self, mu):
+        # normalize Windows module_utils to remove 'AnsibleCollections.' prefix so the plugin loader can find them
+        mu = to_text(mu)
+
+        if not mu.startswith(u'AnsibleCollections.'):
+            return mu
+
+        return mu.replace(u'AnsibleCollections.', u'', 1)
+
 
 def _slurp(path):
     if not os.path.exists(path):

lib/ansible/plugins/loader.py

@@ -324,7 +324,7 @@ class PluginLoader:
             # only current non-class special case, module_utils don't use this loader method
             if append_plugin_type == 'library':
                 append_plugin_type = 'modules'
-            else:
+            elif append_plugin_type != 'module_utils':
                 append_plugin_type = get_plugin_class(append_plugin_type)
             package += '.plugins.{0}'.format(append_plugin_type)
 

lib/ansible/utils/collection_loader.py

@@ -30,7 +30,7 @@ _SYNTHETIC_PACKAGES = {
 }
 
 # TODO: tighten this up to subset Python identifier requirements (and however we want to restrict ns/collection names)
-_collection_qualified_re = re.compile(to_text(r'^(\w+)\.(\w+)\.(\w+)$'))
+_collection_qualified_re = re.compile(to_text(r'^(\w+)\.(\w+)\.(\w+)'))
 
 
 # FIXME: exception handling/error logging

test/integration/targets/collections/aliases

@@ -1,2 +1,5 @@
+posix
 shippable/posix/group4
+shippable/windows/group2
 skip/python2.6
+windows

test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/plugins/module_utils/AnotherCSMU.cs

@@ -0,0 +1,12 @@
+using System;
+
+namespace AnsibleCollections.testns.testcoll.AnotherCSMU
+{
+    public class AnotherThing
+    {
+        public static string CallMe()
+        {
+            return "Hello from nested user-collection-hosted AnotherCSMU";
+        }
+    }
+}

test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/plugins/module_utils/MyCSMU.cs

@@ -0,0 +1,15 @@
+using System;
+
+using AnsibleCollections.testns.testcoll.AnotherCSMU;
+
+namespace AnsibleCollections.testns.testcoll.MyCSMU
+{
+    public class CustomThing
+    {
+        public static string HelloWorld()
+        {
+            string res = AnotherThing.CallMe();
+            return String.Format("Hello from user_mu collection-hosted MyCSMU, also {0}", res);
+        }
+    }
+}

test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/plugins/module_utils/MyPSMU.psm1

@@ -0,0 +1,9 @@
+Function CallMe-FromUserPSMU {
+    <#
+    .SYNOPSIS
+    Test function
+    #>
+    return "from user_mu"
+}
+
+Export-ModuleMember -Function CallMe-FromUserPSMU

test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/plugins/module_utils/subpkg/submod.py

@@ -0,0 +1,2 @@
+def thingtocall():
+    return "thingtocall in subpkg.submod"

test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/plugins/module_utils/subpkg_with_init/__init__.py

@@ -0,0 +1,2 @@
+def thingtocall():
+    return "thingtocall in subpkg_with_init"

test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/plugins/modules/uses_base_mu_granular_nested_import.py

@@ -3,8 +3,6 @@
 import json
 import sys
 
-# FIXME: this is only required due to a bug around "new style module detection"
-from ansible.module_utils.basic import AnsibleModule
 from ansible_collections.testns.testcoll.plugins.module_utils.base import thingtocall
 
 

test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/plugins/modules/uses_leaf_mu_flat_import.py

@@ -3,8 +3,6 @@
 import json
 import sys
 
-# FIXME: this is only required due to a bug around "new style module detection"
-from ansible.module_utils.basic import AnsibleModule
 import ansible_collections.testns.testcoll.plugins.module_utils.leaf
 
 

test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/plugins/modules/uses_leaf_mu_granular_import.py

@@ -3,13 +3,11 @@
 import json
 import sys
 
-# FIXME: this is only required due to a bug around "new style module detection"
-from ansible.module_utils.basic import AnsibleModule
-from ansible_collections.testns.testcoll.plugins.module_utils.leaf import thingtocall
+from ansible_collections.testns.testcoll.plugins.module_utils.leaf import thingtocall as aliasedthing
 
 
 def main():
-    mu_result = thingtocall()
+    mu_result = aliasedthing()
     print(json.dumps(dict(changed=False, source='user', mu_result=mu_result)))
 
     sys.exit()

test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/plugins/modules/uses_leaf_mu_module_import_from.py

@@ -3,15 +3,18 @@
 import json
 import sys
 
-# FIXME: this is only required due to a bug around "new style module detection"
-from ansible.module_utils.basic import AnsibleModule
-# FIXME: this style doesn't work yet under collections
-from ansible_collections.testns.testcoll.plugins.module_utils import leaf
+from ansible_collections.testns.testcoll.plugins.module_utils import leaf, secondary
+# FIXME: these don't work yet under collections
+# from ansible_collections.testns.testcoll.plugins.module_utils.subpkg import submod
+# from ansible_collections.testns.testcoll.plugins.module_utils.subpkg_with_init import thingtocall as spwi_thingtocall
 
 
 def main():
     mu_result = leaf.thingtocall()
-    print(json.dumps(dict(changed=False, source='user', mu_result=mu_result)))
+    mu2_result = secondary.thingtocall()
+#    mu3_result = submod.thingtocall()
+#    mu4_result = spwi_thingtocall()
+    print(json.dumps(dict(changed=False, source='user', mu_result=mu_result, mu2_result=mu2_result)))
 
     sys.exit()
 

test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/plugins/modules/win_csbasic_only.ps1

@@ -0,0 +1,22 @@
+#!powershell
+
+# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
+
+#AnsibleRequires -CSharpUtil Ansible.Basic
+
+$spec = @{
+    options = @{
+        data = @{ type = "str"; default = "pong" }
+    }
+    supports_check_mode = $true
+}
+$module = [Ansible.Basic.AnsibleModule]::Create($args, $spec)
+$data = $module.Params.data
+
+if ($data -eq "crash") {
+    throw "boom"
+}
+
+$module.Result.ping = $data
+$module.Result.source = "user"
+$module.ExitJson()

test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/plugins/modules/win_selfcontained.ps1

@@ -0,0 +1,9 @@
+#!powershell
+
+$res = @{
+  changed = $false
+  source = "user"
+  msg = "hi from selfcontained.ps1"
+}
+
+ConvertTo-Json $res

test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/plugins/modules/win_selfcontained.py

@@ -0,0 +1 @@
+# docs for Windows module would go here; just ensure we don't accidentally load this instead of the .ps1

test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/plugins/modules/win_uses_coll_csmu.ps1

@@ -0,0 +1,23 @@
+#!powershell
+
+# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
+
+#AnsibleRequires -CSharpUtil Ansible.Basic
+#AnsibleRequires -CSharpUtil AnsibleCollections.testns.testcoll.MyCSMU
+
+$spec = @{
+    options = @{
+        data = @{ type = "str"; default = "called from $([AnsibleCollections.testns.testcoll.MyCSMU.CustomThing]::HelloWorld())" }
+    }
+    supports_check_mode = $true
+}
+$module = [Ansible.Basic.AnsibleModule]::Create($args, $spec)
+$data = $module.Params.data
+
+if ($data -eq "crash") {
+    throw "boom"
+}
+
+$module.Result.ping = $data
+$module.Result.source = "user"
+$module.ExitJson()

test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/plugins/modules/win_uses_coll_psmu.ps1

@@ -0,0 +1,23 @@
+#!powershell
+
+# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
+
+#AnsibleRequires -CSharpUtil Ansible.Basic
+#AnsibleRequires -Powershell AnsibleCollections.testns.testcoll.MyPSMU
+
+$spec = @{
+    options = @{
+        data = @{ type = "str"; default = "called from $(CallMe-FromUserPSMU)" }
+    }
+    supports_check_mode = $true
+}
+$module = [Ansible.Basic.AnsibleModule]::Create($args, $spec)
+$data = $module.Params.data
+
+if ($data -eq "crash") {
+    throw "boom"
+}
+
+$module.Result.ping = $data
+$module.Result.source = "user"
+$module.ExitJson()

test/integration/targets/collections/includeme.yml

@@ -0,0 +1,6 @@
+- testns.testcoll.plugin_lookup:
+  register: included_plugin_lookup_out
+
+- assert:
+    that:
+    - included_plugin_lookup_out.collection_list == ['bogus.bogus', 'ansible.legacy']

test/integration/targets/collections/posix.yml

@@ -38,11 +38,9 @@
     testns.testcoll.uses_leaf_mu_flat_import:
     register: flat_out
 
-  # FIXME: this one doesn't work yet
   # module with a full-module module_utils import using 'from' (from (this collection).module_utils import leaf)
   - name: exec module with full-module module_utils import using 'from' from this collection
     testns.testcoll.uses_leaf_mu_module_import_from:
-    ignore_errors: true
     register: from_out
 
   - assert:
@@ -54,7 +52,8 @@
       - granular_out.mu_result == 'thingtocall in leaf'
       - granular_nested_out.mu_result == 'thingtocall in base called thingtocall in secondary'
       - flat_out.mu_result == 'thingtocall in leaf'
-      - from_out is failed  # FIXME: switch back once this import is fixed --> from_out.mu_result == 'thingtocall in leaf'
+      - from_out.mu_result == 'thingtocall in leaf'
+      - from_out.mu2_result == 'thingtocall in secondary'
 
   - name: exercise filters/tests/lookups
     assert:
@@ -169,16 +168,10 @@
     systestmodule:
     register: systestmodule_out
 
-  # ensure we're looking up actions properly
-  - name: unqualified action test
-    plugin_lookup:
-    register: pluginlookup_out
-
   - assert:
       that:
       - testmodule_out.source == 'user'
       - systestmodule_out.source == 'sys'
-      - pluginlookup_out.collection_list == ['testns.testcoll', 'testns.coll_in_sys', 'testns.contentadj', 'ansible.legacy']
 
 # FIXME: this won't work until collections list gets passed through task templar
 #  - name: exercise unqualified filters/tests/lookups
@@ -257,6 +250,22 @@
       - test_role_output.msg == test_role_input
 
 
+- name: validate static task include behavior
+  hosts: testhost
+  collections:
+  - bogus.bogus
+  tasks:
+  - import_tasks: includeme.yml
+
+
+- name: validate dynamic task include behavior
+  hosts: testhost
+  collections:
+  - bogus.bogus
+  tasks:
+  - include_tasks: includeme.yml
+
+
 - name: test a collection-hosted connection plugin against a host from a collection-hosted inventory plugin
   hosts: dynamic_host_a
   vars:

test/integration/targets/collections/runme.sh

@@ -5,9 +5,14 @@ set -eux
 export ANSIBLE_COLLECTIONS_PATHS=$PWD/collection_root_user:$PWD/collection_root_sys
 export ANSIBLE_GATHERING=explicit
 export ANSIBLE_GATHER_SUBSET=minimal
+export ANSIBLE_HOST_PATTERN_MISMATCH=error
+
+# FIXME: just use INVENTORY_PATH as-is once ansible-test sets the right dir
+ipath=../../$(basename "${INVENTORY_PATH}")
+export INVENTORY_PATH="$ipath"
 
 # temporary hack to keep this test from running on Python 2.6 in CI
-if ansible-playbook -i ../../inventory pythoncheck.yml | grep UNSUPPORTEDPYTHON; then
+if ansible-playbook pythoncheck.yml | grep UNSUPPORTEDPYTHON; then
   echo skipping test for unsupported Python version...
   exit 0
 fi
@@ -15,5 +20,12 @@ fi
 # test callback
 ANSIBLE_CALLBACK_WHITELIST=testns.testcoll.usercallback ansible localhost -m ping | grep "usercallback says ok"
 
+# 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
+else
+  export TEST_PLAYBOOK=posix.yml
+fi
+
 # run test playbook
-ansible-playbook -i ../../inventory -i ./a.statichost.yml -v play.yml
+ansible-playbook -i "${INVENTORY_PATH}"  -i ./a.statichost.yml -v "${TEST_PLAYBOOK}"

test/integration/targets/collections/windows.yml

@@ -0,0 +1,20 @@
+- hosts: windows
+  tasks:
+  - testns.testcoll.win_selfcontained:
+    register: selfcontained_out
+
+  - testns.testcoll.win_csbasic_only:
+    register: csbasic_only_out
+
+  - testns.testcoll.win_uses_coll_psmu:
+    register: uses_coll_psmu
+
+  - testns.testcoll.win_uses_coll_csmu:
+    register: uses_coll_csmu
+
+  - assert:
+      that:
+      - selfcontained_out.source == 'user'
+      - csbasic_only_out.source == 'user'
+      - uses_coll_psmu.source == 'user' and 'user_mu' in uses_coll_psmu.ping
+      - uses_coll_csmu.source == 'user' and 'user_mu' in uses_coll_csmu.ping

test/sanity/code-smell/shebang.py

@@ -30,6 +30,10 @@ def main():
     }
 
     skip = set([
+        'test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/plugins/modules/win_csbasic_only.ps1',
+        'test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/plugins/modules/win_selfcontained.ps1',
+        'test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/plugins/modules/win_uses_coll_csmu.ps1',
+        'test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/plugins/modules/win_uses_coll_psmu.ps1',
         'test/integration/targets/win_module_utils/library/legacy_only_new_way_win_line_ending.ps1',
         'test/integration/targets/win_module_utils/library/legacy_only_old_way_win_line_ending.ps1',
         'test/utils/shippable/timing.py',

test/sanity/pslint/ignore.txt

@@ -151,6 +151,7 @@ lib/ansible/modules/windows/win_wait_for.ps1 PSAvoidUsingEmptyCatchBlock
 lib/ansible/modules/windows/win_wait_for.ps1 PSCustomUseLiteralPath
 lib/ansible/modules/windows/win_webpicmd.ps1 PSAvoidUsingInvokeExpression
 lib/ansible/modules/windows/win_xml.ps1 PSCustomUseLiteralPath
+test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/plugins/module_utils/MyPSMU.psm1 PSUseApprovedVerbs
 test/integration/targets/win_audit_rule/library/test_get_audit_rule.ps1 PSAvoidUsingCmdletAliases
 test/integration/targets/win_audit_rule/library/test_get_audit_rule.ps1 PSCustomUseLiteralPath
 test/integration/targets/win_chocolatey/files/tools/chocolateyUninstall.ps1 PSCustomUseLiteralPath