Action Plugin argspec validation (#77013)

changelogs/fragments/action-plugin-arg-spec.yml

@@ -0,0 +1,2 @@
+minor_changes:
+- Action Plugins - Add helper method for argument spec validation, and extend to pause and async_wrapper

lib/ansible/plugins/action/__init__.py

@@ -23,6 +23,8 @@ from ansible import constants as C
 from ansible.errors import AnsibleError, AnsibleConnectionFailure, AnsibleActionSkip, AnsibleActionFail, AnsibleAuthenticationFailure
 from ansible.executor.module_common import modify_module
 from ansible.executor.interpreter_discovery import discover_interpreter, InterpreterDiscoveryRequiredError
+from ansible.module_utils.common.arg_spec import ArgumentSpecValidator
+from ansible.module_utils.errors import UnsupportedError
 from ansible.module_utils.json_utils import _filter_non_json_lines
 from ansible.module_utils.six import binary_type, string_types, text_type
 from ansible.module_utils._text import to_bytes, to_native, to_text
@@ -118,6 +120,56 @@ class ActionBase(ABC):
 
         return result
 
+    def validate_argument_spec(self, argument_spec=None,
+                               mutually_exclusive=None,
+                               required_together=None,
+                               required_one_of=None,
+                               required_if=None,
+                               required_by=None,
+                               ):
+        """Validate an argument spec against the task args
+
+        This will return a tuple of (ValidationResult, dict) where the dict
+        is the validated, coerced, and normalized task args.
+
+        Be cautious when directly passing ``new_module_args`` directly to a
+        module invocation, as it will contain the defaults, and not only
+        the args supplied from the task. If you do this, the module
+        should not define ``mututally_exclusive`` or similar.
+
+        This code is roughly copied from the ``validate_argument_spec``
+        action plugin for use by other action plugins.
+        """
+
+        new_module_args = self._task.args.copy()
+
+        validator = ArgumentSpecValidator(
+            argument_spec,
+            mutually_exclusive=mutually_exclusive,
+            required_together=required_together,
+            required_one_of=required_one_of,
+            required_if=required_if,
+            required_by=required_by,
+        )
+        validation_result = validator.validate(new_module_args)
+
+        new_module_args.update(validation_result.validated_parameters)
+
+        try:
+            error = validation_result.errors[0]
+        except IndexError:
+            error = None
+
+        # Fail for validation errors, even in check mode
+        if error:
+            msg = validation_result.errors.msg
+            if isinstance(error, UnsupportedError):
+                msg = f"Unsupported parameters for ({self._load_name}) module: {msg}"
+
+            raise AnsibleActionFail(msg)
+
+        return validation_result, new_module_args
+
     def cleanup(self, force=False):
         """Method to perform a clean up at the end of an action plugin execution
 

lib/ansible/plugins/action/async_status.py

@@ -11,8 +11,6 @@ from ansible.utils.vars import merge_hash
 
 class ActionModule(ActionBase):
 
-    _VALID_ARGS = frozenset(('jid', 'mode'))
-
     def _get_async_dir(self):
 
         # async directory based on the shell option
@@ -24,18 +22,20 @@ class ActionModule(ActionBase):
 
         results = super(ActionModule, self).run(tmp, task_vars)
 
+        validation_result, new_module_args = self.validate_argument_spec(
+            argument_spec={
+                'jid': {'type': 'str', 'required': True},
+                'mode': {'type': 'str', 'choices': ['status', 'cleanup'], 'default': 'status'},
+            },
+        )
+
         # initialize response
         results['started'] = results['finished'] = 0
         results['stdout'] = results['stderr'] = ''
         results['stdout_lines'] = results['stderr_lines'] = []
 
-        # read params
-        try:
-            jid = self._task.args["jid"]
-        except KeyError:
-            raise AnsibleActionFail("jid is required", result=results)
-
-        mode = self._task.args.get("mode", "status")
+        jid = new_module_args["jid"]
+        mode = new_module_args["mode"]
 
         results['ansible_job_id'] = jid
         async_dir = self._get_async_dir()
@@ -47,7 +47,7 @@ class ActionModule(ActionBase):
             results['results_file'] = log_path
             results['started'] = 1
 
-        module_args = dict(jid=jid, mode=mode, _async_dir=async_dir)
-        results = merge_hash(results, self._execute_module(module_name='ansible.legacy.async_status', task_vars=task_vars, module_args=module_args))
+        new_module_args['_async_dir'] = async_dir
+        results = merge_hash(results, self._execute_module(module_name='ansible.legacy.async_status', task_vars=task_vars, module_args=new_module_args))
 
         return results

lib/ansible/plugins/action/pause.py

@@ -88,7 +88,6 @@ class ActionModule(ActionBase):
     ''' pauses execution for a length or time, or until input is received '''
 
     BYPASS_HOST_LOOP = True
-    _VALID_ARGS = frozenset(('echo', 'minutes', 'prompt', 'seconds'))
 
     def run(self, tmp=None, task_vars=None):
         ''' run the pause action module '''
@@ -98,6 +97,18 @@ class ActionModule(ActionBase):
         result = super(ActionModule, self).run(tmp, task_vars)
         del tmp  # tmp no longer has any effect
 
+        validation_result, new_module_args = self.validate_argument_spec(
+            argument_spec={
+                'echo': {'type': 'bool', 'default': True},
+                'minutes': {'type': int},  # Don't break backwards compat, allow floats, by using int callable
+                'seconds': {'type': int},  # Don't break backwards compat, allow floats, by using int callable
+                'prompt': {'type': 'str'},
+            },
+            mutually_exclusive=(
+                ('minutes', 'seconds'),
+            ),
+        )
+
         duration_unit = 'minutes'
         prompt = None
         seconds = None
@@ -114,41 +125,22 @@ class ActionModule(ActionBase):
             echo=echo
         ))
 
-        # Should keystrokes be echoed to stdout?
-        if 'echo' in self._task.args:
-            try:
-                echo = boolean(self._task.args['echo'])
-            except TypeError as e:
-                result['failed'] = True
-                result['msg'] = to_native(e)
-                return result
-
-            # Add a note saying the output is hidden if echo is disabled
-            if not echo:
-                echo_prompt = ' (output is hidden)'
-
-        # Is 'prompt' a key in 'args'?
-        if 'prompt' in self._task.args:
-            prompt = "[%s]\n%s%s:" % (self._task.get_name().strip(), self._task.args['prompt'], echo_prompt)
+        echo = new_module_args['echo']
+        # Add a note saying the output is hidden if echo is disabled
+        if not echo:
+            echo_prompt = ' (output is hidden)'
+
+        if new_module_args['prompt']:
+            prompt = "[%s]\n%s%s:" % (self._task.get_name().strip(), new_module_args['prompt'], echo_prompt)
         else:
             # If no custom prompt is specified, set a default prompt
             prompt = "[%s]\n%s%s:" % (self._task.get_name().strip(), 'Press enter to continue, Ctrl+C to interrupt', echo_prompt)
 
-        # Are 'minutes' or 'seconds' keys that exist in 'args'?
-        if 'minutes' in self._task.args or 'seconds' in self._task.args:
-            try:
-                if 'minutes' in self._task.args:
-                    # The time() command operates in seconds so we need to
-                    # recalculate for minutes=X values.
-                    seconds = int(self._task.args['minutes']) * 60
-                else:
-                    seconds = int(self._task.args['seconds'])
-                    duration_unit = 'seconds'
-
-            except ValueError as e:
-                result['failed'] = True
-                result['msg'] = u"non-integer value given for prompt duration:\n%s" % to_text(e)
-                return result
+        if new_module_args['minutes'] is not None:
+            seconds = new_module_args['minutes'] * 60
+        elif new_module_args['seconds'] is not None:
+            seconds = new_module_args['seconds']
+            duration_unit = 'seconds'
 
         ########################################################################
         # Begin the hard work!
@@ -173,7 +165,7 @@ class ActionModule(ActionBase):
                 display.display("(ctrl+C then 'C' = continue early, ctrl+C then 'A' = abort)\r"),
 
                 # show the prompt specified in the task
-                if 'prompt' in self._task.args:
+                if new_module_args['prompt']:
                     display.display(prompt)
 
             else:

test/integration/targets/pause/test-pause.yml

@@ -13,7 +13,7 @@
     - assert:
         that:
           - result is failed
-          - "'non-integer' in result.msg"
+          - "'unable to convert to int' in result.msg"
 
     - name: non-boolean for echo (EXPECTED FAILURE)
       pause:
@@ -26,7 +26,8 @@
           - result is failed
           - "'not a valid boolean' in result.msg"
 
-    - pause:
+    - name: Less than 1
+      pause:
         seconds: 0.1
       register: results
 
@@ -34,7 +35,8 @@
         that:
           - results.stdout is search('Paused for \d+\.\d+ seconds')
 
-    - pause:
+    - name: 1 second
+      pause:
         seconds: 1
       register: results
 
@@ -42,10 +44,29 @@
         that:
           - results.stdout is search('Paused for \d+\.\d+ seconds')
 
-    - pause:
+    - name: 1 minute
+      pause:
         minutes: 1
       register: results
 
     - assert:
         that:
           - results.stdout is search('Paused for \d+\.\d+ minutes')
+
+    - name: minutes and seconds
+      pause:
+        minutes: 1
+        seconds: 1
+      register: exclusive
+      ignore_errors: yes
+
+    - name: invalid arg
+      pause:
+        foo: bar
+      register: invalid
+      ignore_errors: yes
+
+    - assert:
+        that:
+          - '"parameters are mutually exclusive: minutes|seconds" in exclusive.msg'
+          - '"Unsupported parameters for (pause) module: foo." in invalid.msg'