various mod_args fixes (#60290)

* various mod_args fixes

* filter task keywords when parsing actions from task_ds- prevents repeatedly banging on the pluginloader for things we know aren't modules/actions
* clean up module/action error messaging. Death to `no action in task!`- actually list the candidate modules/actions from the task if present.

* remove shadowed_module test

* previous discussion was that this behavior isn't worth the complexity or performance costs in mod_args

* fix/add test, remove module shadow logic

* address review feedback

changelogs/fragments/mod_args_tweaks.yml

@@ -0,0 +1,2 @@
+minor_changes:
+- changed task module/action parsing to report more helpful errors

lib/ansible/parsing/mod_args.py

@@ -115,6 +115,15 @@ class ModuleArgsParser:
             raise AnsibleAssertionError("the type of 'task_ds' should be a dict, but is a %s" % type(task_ds))
         self._task_ds = task_ds
         self._collection_list = collection_list
+        # delayed local imports to prevent circular import
+        from ansible.playbook.task import Task
+        from ansible.playbook.handler import Handler
+        # store the valid Task/Handler attrs for quick access
+        self._task_attrs = set(Task._valid_attrs.keys())
+        self._task_attrs.update(set(Handler._valid_attrs.keys()))
+        # HACK: why is static not a FieldAttribute on task with a post-validate to bomb if not include/import?
+        self._task_attrs.add('static')
+        self._task_attrs = frozenset(self._task_attrs)
 
     def _split_module_string(self, module_string):
         '''
@@ -286,8 +295,11 @@ class ModuleArgsParser:
 
         # module: <stuff> is the more new-style invocation
 
-        # walk the input dictionary to see we recognize a module name
+        # filter out task attributes so we're only querying unrecognized keys as actions/modules
-        for (item, value) in iteritems(self._task_ds):
+        non_task_ds = dict((k, v) for k, v in iteritems(self._task_ds) if (k not in self._task_attrs) and (not k.startswith('with_')))
+
+        # walk the filtered input dictionary to see if we recognize a module name
+        for item, value in iteritems(non_task_ds):
             if item in BUILTIN_TASKS or action_loader.has_plugin(item, collection_list=self._collection_list) or \
                     module_loader.has_plugin(item, collection_list=self._collection_list):
                 # finding more than one module name is a problem
@@ -299,14 +311,13 @@ class ModuleArgsParser:
 
         # if we didn't see any module in the task at all, it's not a task really
         if action is None:
-            if 'ping' not in module_loader:
+            if non_task_ds:  # there was one non-task action, but we couldn't find it
-                raise AnsibleParserError("The requested action was not found in configured module paths. "
+                bad_action = list(non_task_ds.keys())[0]
-                                         "Additionally, core modules are missing. If this is a checkout, "
+                raise AnsibleParserError("couldn't resolve module/action '{0}'. This often indicates a "
-                                         "run 'git pull --rebase' to correct this problem.",
+                                         "misspelling, missing collection, or incorrect module path.".format(bad_action),
                                          obj=self._task_ds)
-
             else:
-                raise AnsibleParserError("no action detected in task. This often indicates a misspelled module name, or incorrect module path.",
+                raise AnsibleParserError("no module/action detected in task.",
                                          obj=self._task_ds)
         elif args.get('_raw_params', '') != '' and action not in RAW_PARAM_MODULES:
             templar = Templar(loader=None)

lib/ansible/plugins/loader.py

@@ -375,7 +375,7 @@ class PluginLoader:
 
         return to_text(found_files[0])
 
-    def _find_plugin(self, name, mod_type='', ignore_deprecated=False, check_aliases=False, collection_list=None):
+    def find_plugin(self, name, mod_type='', ignore_deprecated=False, check_aliases=False, collection_list=None):
         ''' Find a plugin named name '''
 
         global _PLUGIN_FILTERS
@@ -498,20 +498,6 @@ class PluginLoader:
 
         return None
 
-    def find_plugin(self, name, mod_type='', ignore_deprecated=False, check_aliases=False, collection_list=None):
-        ''' Find a plugin named name '''
-
-        # Import here to avoid circular import
-        from ansible.vars.reserved import is_reserved_name
-
-        plugin = self._find_plugin(name, mod_type=mod_type, ignore_deprecated=ignore_deprecated, check_aliases=check_aliases, collection_list=collection_list)
-        if plugin and self.package == 'ansible.modules' and name not in ('gather_facts',) and is_reserved_name(name):
-            raise AnsibleError(
-                'Module "%s" shadows the name of a reserved keyword. Please rename or remove this module. Found at %s' % (name, plugin)
-            )
-
-        return plugin
-
     def has_plugin(self, name, collection_list=None):
         ''' Checks if a plugin named name exists '''
 

test/integration/targets/shadowed_module/aliases

@@ -1 +0,0 @@
-shippable/posix/group3

test/integration/targets/shadowed_module/playbook.yml

@@ -1,6 +0,0 @@
----
-- hosts: localhost
-  gather_facts: false
-  tasks:
-    - command: whoami
-      tags: foo

test/integration/targets/shadowed_module/playbook_lookup.yml

@@ -1,6 +0,0 @@
----
-- hosts: localhost
-  gather_facts: false
-  tasks:
-    - debug:
-        msg: "{{ lookup('vars', 'inventory_hostname') }}"

test/integration/targets/shadowed_module/runme.sh

@@ -1,14 +0,0 @@
-#!/usr/bin/env bash
-
-set -ux
-
-OUT=$(ansible-playbook playbook.yml -i inventory -e @../../integration_config.yml "$@" 2>&1 | grep 'ERROR! Module "tags" shadows the name of a reserved keyword.')
-
-if [[ -z "$OUT" ]]; then
-    echo "Fake tags module did not cause error"
-    exit 1
-fi
-
-# This playbook calls a lookup which shadows a keyword.
-# This is an ok situation, and should not error
-ansible-playbook playbook_lookup.yml -i ../../inventory -e @../../integration_config.yml "$@"

test/units/parsing/test_mod_args.py

@@ -6,6 +6,7 @@ from __future__ import absolute_import, division, print_function
 __metaclass__ = type
 
 import pytest
+import re
 
 from ansible.errors import AnsibleParserError
 from ansible.parsing.mod_args import ModuleArgsParser
@@ -124,5 +125,13 @@ class TestModArgsDwim:
             m.parse()
 
         assert err.value.args[0].startswith("conflicting action statements: ")
-        conflicts = set(err.value.args[0][len("conflicting action statements: "):].split(', '))
+        actions = set(re.search(r'(\w+), (\w+)', err.value.args[0]).groups())
-        assert conflicts == set(('ping', 'shell'))
+        assert actions == set(['ping', 'shell'])
+
+    def test_bogus_action(self):
+        args_dict = {'bogusaction': {}}
+        m = ModuleArgsParser(args_dict)
+        with pytest.raises(AnsibleParserError) as err:
+            m.parse()
+
+        assert err.value.args[0].startswith("couldn't resolve module/action 'bogusaction'")

test/units/playbook/test_helpers.py

@@ -108,7 +108,7 @@ class TestLoadListOfTasks(unittest.TestCase, MixinForMocks):
     def test_empty_task(self):
         ds = [{}]
         self.assertRaisesRegexp(errors.AnsibleParserError,
-                                "no action detected in task. This often indicates a misspelled module name, or incorrect module path",
+                                "no module/action detected in task",
                                 helpers.load_list_of_tasks,
                                 ds, play=self.mock_play,
                                 variable_manager=self.mock_variable_manager, loader=self.fake_loader)
@@ -116,7 +116,7 @@ class TestLoadListOfTasks(unittest.TestCase, MixinForMocks):
     def test_empty_task_use_handlers(self):
         ds = [{}]
         self.assertRaisesRegexp(errors.AnsibleParserError,
-                                "no action detected in task. This often indicates a misspelled module name, or incorrect module path",
+                                "no module/action detected in task.",
                                 helpers.load_list_of_tasks,
                                 ds,
                                 use_handlers=True,
@@ -384,7 +384,7 @@ class TestLoadListOfBlocks(unittest.TestCase, MixinForMocks):
         ds = [{}]
         mock_play = MagicMock(name='MockPlay')
         self.assertRaisesRegexp(errors.AnsibleParserError,
-                                "no action detected in task. This often indicates a misspelled module name, or incorrect module path",
+                                "no module/action detected in task",
                                 helpers.load_list_of_blocks,
                                 ds, mock_play,
                                 parent_block=None,