Add proper check mode support to the script module (#31852)

* Do not run script in check mode

Fixes #30676

* Reformat script integration test

* Add integration tests for check mode of script module

* Fix name on test

* Cleanup temp file

* win_script integration test syntaxt changes

* Add check mode tests for win_script

* Use proper variable in test

* Fail if source file does not exist

* Verify script is accessible and don't copy in check mode

Use shlex to properly split shell arguments, though a path with spaces in it still needs to be quoted in the playbook.
Add note to docs describing such.
Improve error message if file is not found indicating there may be a space in the path.

* Properly encode path now that path is split using shlex

* Allow for spaces in both path and script name

* Add unicode character test to Linux script tests

* Add Linux test for space in path to script

(cherry picked from commit ea3638b580)

CHANGELOG.md

@@ -165,6 +165,7 @@ Ansible Changes By Release
   (https://github.com/ansible/ansible/pull/32762)
 * fix dci failure nxos
   (https://github.com/ansible/ansible/pull/32877)
+* Do not execute `script` tasks is check mode (https://github.com/ansible/ansible/issues/30676)
 
 <a id="2.4.1"></a>
 

lib/ansible/modules/commands/script.py

@@ -26,7 +26,7 @@ description:
 options:
   free_form:
     description:
-      - path to the local script file followed by optional arguments. There is no parameter actually named 'free form'; see the examples!
+      - Path to the local script file followed by optional arguments. There is no parameter actually named 'free form'; see the examples!
     required: true
     default: null
     aliases: []
@@ -53,6 +53,7 @@ notes:
   - The ssh connection plugin will force pseudo-tty allocation via -tt when scripts are executed. pseudo-ttys do not have a stderr channel and all
     stderr is sent to stdout. If you depend on separated stdout and stderr result keys, please switch to a copy+command set of tasks instead of using script.
   - This module is also supported for Windows targets.
+  - If the path to the local script contains spaces, it needs to be quoted.
 author:
     - Ansible Core Team
     - Michael DeHaan

lib/ansible/plugins/action/script.py

@@ -19,9 +19,10 @@ __metaclass__ = type
 
 import os
 import re
+import shlex
 
 from ansible.errors import AnsibleError
-from ansible.module_utils._text import to_native
+from ansible.module_utils._text import to_native, to_text
 from ansible.plugins.action import ActionBase
 
 
@@ -72,30 +73,46 @@ class ActionModule(ActionBase):
             if self._connection._shell.SHELL_FAMILY != 'powershell' and not chdir.startswith('/'):
                 return dict(failed=True, msg='chdir %s must be an absolute path for a Unix-aware remote node' % chdir)
 
-        # the script name is the first item in the raw params, so we split it
-        # out now so we know the file name we need to transfer to the remote,
-        # and everything else is an argument to the script which we need later
-        # to append to the remote command
-        parts = self._task.args.get('_raw_params', '').strip().split()
+        # Split out the script as the first item in raw_params using
+        # shlex.split() in order to support paths and files with spaces in the name.
+        # Any arguments passed to the script will be added back later.
+        raw_params = to_native(self._task.args.get('_raw_params', ''), errors='surrogate_or_strict')
+        parts = [to_text(s, errors='surrogate_or_strict') for s in shlex.split(raw_params.strip())]
         source = parts[0]
-        args = ' '.join(parts[1:])
 
         try:
             source = self._loader.get_real_file(self._find_needle('files', source), decrypt=self._task.args.get('decrypt', True))
         except AnsibleError as e:
             return dict(failed=True, msg=to_native(e))
 
-        # transfer the file to a remote tmp location
-        tmp_src = self._connection._shell.join_path(tmp, os.path.basename(source))
-        self._transfer_file(source, tmp_src)
+        if not self._play_context.check_mode:
+            # transfer the file to a remote tmp location
+            tmp_src = self._connection._shell.join_path(tmp, os.path.basename(source))
 
-        # set file permissions, more permissive when the copy is done as a different user
-        self._fixup_perms2((tmp, tmp_src), execute=True)
+            # Convert raw_params to text for the purpose of replacing the script since
+            # parts and tmp_src are both unicode strings and raw_params will be different
+            # depending on Python version.
+            #
+            # Once everything is encoded consistently, replace the script path on the remote
+            # system with the remainder of the raw_params. This preserves quoting in parameters
+            # that would have been removed by shlex.split().
+            target_command = to_text(raw_params).strip().replace(parts[0], tmp_src)
+
+            self._transfer_file(source, tmp_src)
+
+            # set file permissions, more permissive when the copy is done as a different user
+            self._fixup_perms2((tmp, tmp_src), execute=True)
+
+            # add preparation steps to one ssh roundtrip executing the script
+            env_dict = dict()
+            env_string = self._compute_environment_string(env_dict)
+            script_cmd = ' '.join([env_string, target_command])
+
+        if self._play_context.check_mode:
+            result['changed'] = True
+            self._remove_tmp_path(tmp)
+            return result
 
-        # add preparation steps to one ssh roundtrip executing the script
-        env_dict = dict()
-        env_string = self._compute_environment_string(env_dict)
-        script_cmd = ' '.join([env_string, tmp_src, args])
         script_cmd = self._connection._shell.wrap_for_exec(script_cmd)
 
         exec_data = None

test/integration/targets/script/files/space path/test.sh

@@ -0,0 +1,3 @@
+#!/usr/bin/env bash
+
+echo -n "Script with space in path"

test/integration/targets/script/files/test_with_args.sh

@@ -0,0 +1,5 @@
+#!/usr/bin/env bash
+
+for i in "$@"; do
+    echo "$i"
+done

test/integration/targets/script/tasks/main.yml

@@ -20,13 +20,18 @@
 ## prep
 ##
 
-- set_fact: output_dir_test={{output_dir}}/test_script
+- set_fact:
+    output_dir_test: "{{ output_dir }}/test_script"
 
 - name: make sure our testing sub-directory does not exist
-  file: path="{{ output_dir_test }}" state=absent
+  file:
+    path: "{{ output_dir_test }}"
+    state: absent
 
 - name: create our testing sub-directory
-  file: path="{{ output_dir_test }}" state=directory
+  file:
+    path: "{{ output_dir_test }}"
+    state: directory
 
 ##
 ## script
@@ -43,36 +48,101 @@
       - "script_result0.stderr == ''"
       - "script_result0.stdout == 'win'"
 
-# creates
+- name: Execute a script with a space in the path
+  script: "'space path/test.sh'"
+  register: _space_path_test
+  tags:
+    - spacepath
 
-- name: verify that afile.txt is absent
-  file: path={{output_dir_test}}/afile.txt state=absent
-
-- name: create afile.txt with create_afile.sh via command
-  script: create_afile.sh {{output_dir_test | expanduser}}/afile.txt creates={{output_dir_test | expanduser}}/afile.txt
-
-- name: verify that afile.txt is present
-  file: path={{output_dir_test}}/afile.txt state=file
-
-# removes
-
-- name: remove afile.txt with remote_afile.sh via command
-  script: remove_afile.sh {{output_dir_test | expanduser}}/afile.txt removes={{output_dir_test | expanduser}}/afile.txt
-  register: script_result1
-
-- name: verify that afile.txt is absent
-  file: path={{output_dir_test}}/afile.txt state=absent
-  register: script_result2
-
-- name: assert that the file was removed by the script
+- name: Assert that script with space in path ran successfully
   assert:
     that:
-      - "script_result1|changed"
-      - "script_result2.state == 'absent'"
+      - _space_path_test is success
+      - _space_path_test.stdout == 'Script with space in path'
+  tags:
+    - spacepath
+
+- name: Execute a script with arguments including a unicode character
+  script: test_with_args.sh -this -that -Ӧther
+  register: unicode_args
+
+- name: Assert that script with unicode character ran successfully
+  assert:
+    that:
+      - unicode_args is success
+      - unicode_args.stdout_lines[0] == '-this'
+      - unicode_args.stdout_lines[1] == '-that'
+      - unicode_args.stdout_lines[2] == '-Ӧther'
+
+# creates
+- name: verify that afile.txt is absent
+  file:
+    path: "{{ output_dir_test }}/afile.txt"
+    state: absent
+
+- name: create afile.txt with create_afile.sh via command
+  script: create_afile.sh {{ output_dir_test | expanduser }}/afile.txt
+  args:
+    creates: "{{ output_dir_test | expanduser }}/afile.txt"
+  register: _create_test1
+
+- name: Check state of created file
+  stat:
+    path: "{{ output_dir_test | expanduser }}/afile.txt"
+  register: _create_stat1
+
+- name: Run create_afile.sh again to ensure it is skipped
+  script: create_afile.sh {{ output_dir_test | expanduser }}/afile.txt
+  args:
+    creates: "{{ output_dir_test | expanduser }}/afile.txt"
+  register: _create_test2
+
+- name: Assert that script report a change, file was created, second run was skipped
+  assert:
+    that:
+      - _create_test1 | changed
+      - _create_stat1.stat.exists
+      - _create_test2 | skipped
+
+
+# removes
+- name: verify that afile.txt is present
+  file:
+    path: "{{ output_dir_test }}/afile.txt"
+    state: file
+
+- name: remove afile.txt with remote_afile.sh via command
+  script: remove_afile.sh {{ output_dir_test | expanduser }}/afile.txt
+  args:
+    removes: "{{ output_dir_test | expanduser }}/afile.txt"
+  register: _remove_test1
+
+- name: Check state of removed file
+  stat:
+    path: "{{ output_dir_test | expanduser }}/afile.txt"
+  register: _remove_stat1
+
+- name: Run remote_afile.sh again to enure it is skipped
+  script: remove_afile.sh {{ output_dir_test | expanduser }}/afile.txt
+  args:
+    removes: "{{ output_dir_test | expanduser }}/afile.txt"
+  register: _remove_test2
+
+- name: Assert that script report a change, file was removed, second run was skipped
+  assert:
+    that:
+      - _remove_test1 | changed
+      - not _remove_stat1.stat.exists
+      - _remove_test2 | skipped
+
 
 # async
-- name: test task failure with async param
+- name: verify that afile.txt is absent
+  file:
+    path: "{{ output_dir_test }}/afile.txt"
+    state: absent
 
+- name: test task failure with async param
   script: /some/script.sh
   async: 2
   ignore_errors: true
@@ -81,5 +151,73 @@
 - name: assert task with async param failed
   assert:
     that:
-      - script_result3|failed
+      - script_result3 | failed
       - script_result3.msg == "async is not supported for this task."
+
+
+# check mode
+- name: Run script to create a file in check mode
+  script: create_afile.sh {{ output_dir_test | expanduser }}/afile2.txt
+  check_mode: yes
+  register: _check_mode_test
+
+- debug:
+    var: _check_mode_test
+    verbosity: 2
+
+- name: Get state of file created by script
+  stat:
+    path: "{{ output_dir_test | expanduser }}/afile2.txt"
+  register: _afile_stat
+
+- debug:
+    var: _afile_stat
+    verbosity: 2
+
+- name: Assert that a change was reported but the script did not make changes
+  assert:
+    that:
+      - _check_mode_test | changed
+      - not _afile_stat.stat.exists
+
+- name: Run script to create a file
+  script: create_afile.sh {{ output_dir_test | expanduser }}/afile2.txt
+
+- name: Run script to create a file in check mode with 'creates' argument
+  script: create_afile.sh {{ output_dir_test | expanduser }}/afile2.txt
+  args:
+    creates: "{{ output_dir_test | expanduser }}/afile2.txt"
+  register: _check_mode_test2
+  check_mode: yes
+
+- debug:
+    var: _check_mode_test2
+    verbosity: 2
+
+- name: Assert that task was skipped and mesage was returned
+  assert:
+    that:
+      - _check_mode_test2 | skipped
+      - '_check_mode_test2.msg == "skipped, since {{ output_dir_test | expanduser }}/afile2.txt exists"'
+
+- name: Remove afile2.txt
+  file:
+    path: "{{ output_dir_test | expanduser }}/afile2.txt"
+    state: absent
+
+- name: Run script to remove a file in check mode with 'removes' argument
+  script: remove_afile.sh {{ output_dir_test | expanduser }}/afile2.txt
+  args:
+    removes: "{{ output_dir_test | expanduser }}/afile2.txt"
+  register: _check_mode_test3
+  check_mode: yes
+
+- debug:
+    var: _check_mode_test3
+    verbosity: 2
+
+- name: Assert that task was skipped and message was returned
+  assert:
+    that:
+      - _check_mode_test3 | skipped
+      - '_check_mode_test3.msg == "skipped, since {{ output_dir_test | expanduser }}/afile2.txt does not exist"'

test/integration/targets/win_script/files/space path/test_script.ps1

@@ -0,0 +1 @@
+Write-Host "Ansible supports spaces in the path to the script."

test/integration/targets/win_script/tasks/main.yml

@@ -35,8 +35,8 @@
       - "test_script_result.stdout"
       - "'Woohoo' in test_script_result.stdout"
       - "not test_script_result.stderr"
-      - "not test_script_result|failed"
-      - "test_script_result|changed"
+      - "not test_script_result | failed"
+      - "test_script_result | changed"
 
 - name: run test script that takes arguments including a unicode char
   script: test_script_with_args.ps1 /this /that /Ӧther
@@ -51,8 +51,8 @@
       - "test_script_with_args_result.stdout_lines[1] == '/that'"
       - "test_script_with_args_result.stdout_lines[2] == '/Ӧther'"
       - "not test_script_with_args_result.stderr"
-      - "not test_script_with_args_result|failed"
-      - "test_script_with_args_result|changed"
+      - "not test_script_with_args_result | failed"
+      - "test_script_with_args_result | changed"
 
 - name: run test script that takes parameters passed via splatting
   script: test_script_with_splatting.ps1 @{ This = 'this'; That = '{{ test_win_script_value }}'; Other = 'other'}
@@ -67,8 +67,8 @@
       - "test_script_with_splatting_result.stdout_lines[1] == test_win_script_value"
       - "test_script_with_splatting_result.stdout_lines[2] == 'other'"
       - "not test_script_with_splatting_result.stderr"
-      - "not test_script_with_splatting_result|failed"
-      - "test_script_with_splatting_result|changed"
+      - "not test_script_with_splatting_result | failed"
+      - "test_script_with_splatting_result | changed"
 
 - name: run test script that takes splatted parameters from a variable
   script: test_script_with_splatting.ps1 {{ test_win_script_splat }}
@@ -83,8 +83,8 @@
       - "test_script_with_splatting2_result.stdout_lines[1] == 'THAT'"
       - "test_script_with_splatting2_result.stdout_lines[2] == 'OTHER'"
       - "not test_script_with_splatting2_result.stderr"
-      - "not test_script_with_splatting2_result|failed"
-      - "test_script_with_splatting2_result|changed"
+      - "not test_script_with_splatting2_result | failed"
+      - "test_script_with_splatting2_result | changed"
 
 - name: run test script that has errors
   script: test_script_with_errors.ps1
@@ -97,15 +97,17 @@
       - "test_script_with_errors_result.rc != 0"
       - "not test_script_with_errors_result.stdout"
       - "test_script_with_errors_result.stderr"
-      - "test_script_with_errors_result|failed"
-      - "test_script_with_errors_result|changed"
+      - "test_script_with_errors_result | failed"
+      - "test_script_with_errors_result | changed"
 
 - name: cleanup test file if it exists
-  raw: Remove-Item "{{test_win_script_filename}}" -Force
+  raw: Remove-Item "{{ test_win_script_filename }}" -Force
   ignore_errors: true
 
 - name: run test script that creates a file
-  script: test_script_creates_file.ps1 "{{test_win_script_filename}}" creates="{{test_win_script_filename}}"
+  script: test_script_creates_file.ps1 {{ test_win_script_filename }}
+  args:
+    creates: "{{ test_win_script_filename }}"
   register: test_script_creates_file_result
 
 - name: check that script ran and indicated a change
@@ -114,22 +116,26 @@
       - "test_script_creates_file_result.rc == 0"
       - "not test_script_creates_file_result.stdout"
       - "not test_script_creates_file_result.stderr"
-      - "not test_script_creates_file_result|failed"
-      - "test_script_creates_file_result|changed"
+      - "not test_script_creates_file_result | failed"
+      - "test_script_creates_file_result | changed"
 
 - name: run test script that creates a file again
-  script: test_script_creates_file.ps1 "{{test_win_script_filename}}" creates="{{test_win_script_filename}}"
+  script: test_script_creates_file.ps1 {{ test_win_script_filename }}
+  args:
+    creates: "{{ test_win_script_filename }}"
   register: test_script_creates_file_again_result
 
 - name: check that the script did not run since the remote file exists
   assert:
     that:
-      - "not test_script_creates_file_again_result|failed"
-      - "not test_script_creates_file_again_result|changed"
-      - "test_script_creates_file_again_result|skipped"
+      - "not test_script_creates_file_again_result | failed"
+      - "not test_script_creates_file_again_result | changed"
+      - "test_script_creates_file_again_result | skipped"
 
 - name: run test script that removes a file
-  script: test_script_removes_file.ps1 "{{test_win_script_filename}}" removes="{{test_win_script_filename}}"
+  script: test_script_removes_file.ps1 {{ test_win_script_filename }}
+  args:
+    removes: "{{ test_win_script_filename }}"
   register: test_script_removes_file_result
 
 - name: check that the script ran since the remote file exists
@@ -138,19 +144,21 @@
       - "test_script_removes_file_result.rc == 0"
       - "not test_script_removes_file_result.stdout"
       - "not test_script_removes_file_result.stderr"
-      - "not test_script_removes_file_result|failed"
-      - "test_script_removes_file_result|changed"
+      - "not test_script_removes_file_result | failed"
+      - "test_script_removes_file_result | changed"
 
 - name: run test script that removes a file again
-  script: test_script_removes_file.ps1 "{{test_win_script_filename}}" removes="{{test_win_script_filename}}"
+  script: test_script_removes_file.ps1 {{ test_win_script_filename }}
+  args:
+    removes: "{{ test_win_script_filename }}"
   register: test_script_removes_file_again_result
 
 - name: check that the script did not run since the remote file does not exist
   assert:
     that:
-      - "not test_script_removes_file_again_result|failed"
-      - "not test_script_removes_file_again_result|changed"
-      - "test_script_removes_file_again_result|skipped"
+      - "not test_script_removes_file_again_result | failed"
+      - "not test_script_removes_file_again_result | changed"
+      - "test_script_removes_file_again_result | skipped"
 
 # TODO: these tests fail on 2008 (not even R2) with no output. It's related to the default codepage being UTF8- if we force it back to 437, it works fine.
 # Need to figure out a sane place to do that under the new exec wrapper.
@@ -165,8 +173,8 @@
 #      - "test_batch_result.stdout"
 #      - "'batch' in test_batch_result.stdout"
 #      - "not test_batch_result.stderr"
-#      - "not test_batch_result|failed"
-#      - "test_batch_result|changed"
+#      - "not test_batch_result | failed"
+#      - "test_batch_result | changed"
 
 
 #- name: run simple batch file with .cmd extension
@@ -180,8 +188,8 @@
 #      - "test_cmd_result.stdout"
 #      - "'cmd extension' in test_cmd_result.stdout"
 #      - "not test_cmd_result.stderr"
-#      - "not test_cmd_result|failed"
-#      - "test_cmd_result|changed"
+#      - "not test_cmd_result | failed"
+#      - "test_cmd_result | changed"
 
 - name: run test script that takes a boolean parameter
   script: test_script_bool.ps1 $true
@@ -205,4 +213,47 @@
 #    that:
 #    - test_script_env_result | succeeded
 #    - test_script_env_result.stdout_lines[0] == 'task'
-#
+#
+
+
+# check mode
+- name: Run test script that creates a file in check mode
+  script: test_script_creates_file.ps1 {{ test_win_script_filename }}
+  args:
+    creates: "{{ test_win_script_filename }}"
+  check_mode: yes
+  register: test_script_creates_file_check_mode
+
+- name: Get state of file created by script
+  win_stat:
+    path: "{{ test_win_script_filename }}"
+  register: create_file_stat
+
+- name: Assert that a change was reported but the script did not make changes
+  assert:
+    that:
+      - test_script_creates_file_check_mode | changed
+      - not create_file_stat.stat.exists
+
+- name: Run test script that creates a file
+  script: test_script_creates_file.ps1 {{ test_win_script_filename }}
+  args:
+    creates: "{{ test_win_script_filename }}"
+
+- name: Run test script that removes a file in check mode
+  script: test_script_removes_file.ps1 {{ test_win_script_filename }}
+  args:
+    removes: "{{ test_win_script_filename }}"
+  check_mode: yes
+  register: test_script_removes_file_check_mode
+
+- name: Get state of file removed by script
+  win_stat:
+    path: "{{ test_win_script_filename }}"
+  register: remove_file_stat
+
+- name: Assert that a change was reported but the script did not make changes
+  assert:
+    that:
+      - test_script_removes_file_check_mode | changed
+      - remove_file_stat.stat.exists