From ea3638b58083a54a7910e85ba22cd4d37d28735c Mon Sep 17 00:00:00 2001 From: Sam Doran Date: Mon, 13 Nov 2017 18:33:44 -0500 Subject: [PATCH] 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 --- lib/ansible/modules/commands/script.py | 3 +- lib/ansible/plugins/action/script.py | 49 +++-- .../targets/script/files/space path/test.sh | 3 + .../targets/script/files/test_with_args.sh | 5 + .../integration/targets/script/tasks/main.yml | 194 +++++++++++++++--- .../files/space path/test_script.ps1 | 1 + .../targets/win_script/tasks/main.yml | 111 +++++++--- 7 files changed, 291 insertions(+), 75 deletions(-) create mode 100755 test/integration/targets/script/files/space path/test.sh create mode 100755 test/integration/targets/script/files/test_with_args.sh create mode 100644 test/integration/targets/win_script/files/space path/test_script.ps1 diff --git a/lib/ansible/modules/commands/script.py b/lib/ansible/modules/commands/script.py index ad89d48d00..2d186a3308 100644 --- a/lib/ansible/modules/commands/script.py +++ b/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 diff --git a/lib/ansible/plugins/action/script.py b/lib/ansible/plugins/action/script.py index f37964da3c..3549e23321 100644 --- a/lib/ansible/plugins/action/script.py +++ b/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 diff --git a/test/integration/targets/script/files/space path/test.sh b/test/integration/targets/script/files/space path/test.sh new file mode 100755 index 0000000000..6f6334d71f --- /dev/null +++ b/test/integration/targets/script/files/space path/test.sh @@ -0,0 +1,3 @@ +#!/usr/bin/env bash + +echo -n "Script with space in path" \ No newline at end of file diff --git a/test/integration/targets/script/files/test_with_args.sh b/test/integration/targets/script/files/test_with_args.sh new file mode 100755 index 0000000000..13dce4f24c --- /dev/null +++ b/test/integration/targets/script/files/test_with_args.sh @@ -0,0 +1,5 @@ +#!/usr/bin/env bash + +for i in "$@"; do + echo "$i" +done \ No newline at end of file diff --git a/test/integration/targets/script/tasks/main.yml b/test/integration/targets/script/tasks/main.yml index 7b31444190..85c9a8dcbc 100644 --- a/test/integration/targets/script/tasks/main.yml +++ b/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"' diff --git a/test/integration/targets/win_script/files/space path/test_script.ps1 b/test/integration/targets/win_script/files/space path/test_script.ps1 new file mode 100644 index 0000000000..24e0ab2ed5 --- /dev/null +++ b/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." diff --git a/test/integration/targets/win_script/tasks/main.yml b/test/integration/targets/win_script/tasks/main.yml index 1a9c25924f..32b637630a 100644 --- a/test/integration/targets/win_script/tasks/main.yml +++ b/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' -# \ No newline at end of file +# + + +# 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