Improve testing for Windows SSH and other connection plugins (#83834)

Expands the test matrix used for testing on Windows to cover the three
connection plugins we support for all the tasks. This change also
changes how raw commands are run over SSH to avoid starting a
`powershell.exe` process that was uneeded in the majority of cases used
in Ansible. This simplifies our code a bit more by removing extra
Windows specific actions in the ssh plugin and improves the efficiency
when running tasks.

.azure-pipelines/azure-pipelines.yml

@@ -68,9 +68,11 @@ stages:
           nameFormat: Server {0}
           testFormat: windows/{0}/1
           targets:
-            - test: 2016
-            - test: 2019
-            - test: 2022
+            - test: 2016/winrm/http
+            - test: 2019/winrm/https
+            - test: 2022/winrm/https
+            - test: 2022/psrp/http
+            - test: 2022/ssh/key
   - stage: Remote
     dependsOn: []
     jobs:
@@ -181,9 +183,11 @@ stages:
           nameFormat: Server {0}
           testFormat: i/windows/{0}
           targets:
-            - test: 2016
-            - test: 2019
-            - test: 2022
+            - test: 2016/winrm/http
+            - test: 2019/winrm/https
+            - test: 2022/winrm/https
+            - test: 2022/psrp/http
+            - test: 2022/ssh/key
   - stage: Incidental
     dependsOn: []
     jobs:

.azure-pipelines/commands/incidental/windows.sh

@@ -6,6 +6,8 @@ declare -a args
 IFS='/:' read -ra args <<< "$1"
 
 version="${args[1]}"
+connection="${args[2]}"
+connection_setting="${args[3]}"
 
 target="shippable/windows/incidental/"
 
@@ -26,11 +28,7 @@ if [ -s /tmp/windows.txt ] || [ "${CHANGED:+$CHANGED}" == "" ]; then
     echo "Detected changes requiring integration tests specific to Windows:"
     cat /tmp/windows.txt
 
-    echo "Running Windows integration tests for multiple versions concurrently."
-
-    platforms=(
-        --windows "${version}"
-    )
+    echo "Running Windows integration tests for the version ${version}."
 else
     echo "No changes requiring integration tests specific to Windows were detected."
     echo "Running Windows integration tests for a single version only: ${single_version}"
@@ -39,14 +37,10 @@ else
         echo "Skipping this job since it is for: ${version}"
         exit 0
     fi
-
-    platforms=(
-        --windows "${version}"
-    )
 fi
 
 # shellcheck disable=SC2086
 ansible-test windows-integration --color -v --retry-on-error "${target}" ${COVERAGE:+"$COVERAGE"} ${CHANGED:+"$CHANGED"} ${UNSTABLE:+"$UNSTABLE"} \
-    "${platforms[@]}" \
-    --docker default --python "${python_default}" \
-    --remote-terminate always --remote-stage "${stage}" --remote-provider "${provider}"
+    --controller "docker:default,python=${python_default}" \
+    --target "remote:windows/${version},connection=${connection}+${connection_setting},provider=${provider}" \
+    --remote-terminate always --remote-stage "${stage}"

.azure-pipelines/commands/windows.sh

@@ -6,7 +6,9 @@ declare -a args
 IFS='/:' read -ra args <<< "$1"
 
 version="${args[1]}"
-group="${args[2]}"
+connection="${args[2]}"
+connection_setting="${args[3]}"
+group="${args[4]}"
 
 target="shippable/windows/group${group}/"
 
@@ -31,11 +33,7 @@ if [ -s /tmp/windows.txt ] || [ "${CHANGED:+$CHANGED}" == "" ]; then
     echo "Detected changes requiring integration tests specific to Windows:"
     cat /tmp/windows.txt
 
-    echo "Running Windows integration tests for multiple versions concurrently."
-
-    platforms=(
-        --windows "${version}"
-    )
+    echo "Running Windows integration tests for the version ${version}."
 else
     echo "No changes requiring integration tests specific to Windows were detected."
     echo "Running Windows integration tests for a single version only: ${single_version}"
@@ -44,17 +42,13 @@ else
         echo "Skipping this job since it is for: ${version}"
         exit 0
     fi
-
-    platforms=(
-        --windows "${version}"
-    )
 fi
 
-for version in "${python_versions[@]}"; do
+for py_version in "${python_versions[@]}"; do
     changed_all_target="all"
     changed_all_mode="default"
 
-    if [ "${version}" == "${python_default}" ]; then
+    if [ "${py_version}" == "${python_default}" ]; then
         # smoketest tests
         if [ "${CHANGED}" ]; then
             # with change detection enabled run tests for anything changed
@@ -80,7 +74,7 @@ for version in "${python_versions[@]}"; do
     fi
 
     # terminate remote instances on the final python version tested
-    if [ "${version}" = "${python_versions[-1]}" ]; then
+    if [ "${py_version}" = "${python_versions[-1]}" ]; then
         terminate="always"
     else
         terminate="never"
@@ -88,7 +82,8 @@ for version in "${python_versions[@]}"; do
 
     # shellcheck disable=SC2086
     ansible-test windows-integration --color -v --retry-on-error "${ci}" ${COVERAGE:+"$COVERAGE"} ${CHANGED:+"$CHANGED"} ${UNSTABLE:+"$UNSTABLE"} \
-        "${platforms[@]}" --changed-all-target "${changed_all_target}" --changed-all-mode "${changed_all_mode}" \
-        --docker default --python "${version}" \
-        --remote-terminate "${terminate}" --remote-stage "${stage}" --remote-provider "${provider}"
+        --changed-all-target "${changed_all_target}" --changed-all-mode "${changed_all_mode}" \
+        --controller "docker:default,python=${py_version}" \
+        --target "remote:windows/${version},connection=${connection}+${connection_setting},provider=${provider}" \
+        --remote-terminate "${terminate}" --remote-stage "${stage}"
 done

changelogs/fragments/ssh-windows.yml

@@ -0,0 +1,13 @@
+breaking_changes:
+- >-
+  Stopped wrapping all commands sent over SSH on a Windows target with a
+  ``powershell.exe`` executable. This results in one less process being started
+  on each command for Windows to improve efficiency, simplify the code, and
+  make ``raw`` an actual raw command run with the default shell configured on
+  the Windows sshd settings. This should have no affect on most tasks except
+  for ``raw`` which now is not guaranteed to always be running in a PowerShell
+  shell and from having the console output codepage set to UTF-8. To avoid this
+  issue either swap to using ``ansible.windows.win_command``,
+  ``ansible.windows.win_shell``, ``ansible.windows.win_powershell`` or manually
+  wrap the raw command with the shell commands needed to set the output console
+  encoding.

lib/ansible/executor/powershell/become_wrapper.ps1

@@ -116,12 +116,11 @@ Write-AnsibleLog "INFO - parsed become input, user: '$username', type: '$logon_t
 # set to Stop and cannot be changed. Also need to split the payload from the wrapper to prevent potentially
 # sensitive content from being logged by the scriptblock logger.
 $bootstrap_wrapper = {
-    &chcp.com 65001 > $null
-    $exec_wrapper_str = [System.Console]::In.ReadToEnd()
-    $split_parts = $exec_wrapper_str.Split(@("`0`0`0`0"), 2, [StringSplitOptions]::RemoveEmptyEntries)
+    [Console]::InputEncoding = [Console]::OutputEncoding = New-Object System.Text.UTF8Encoding
+    $ew = [System.Console]::In.ReadToEnd()
+    $split_parts = $ew.Split(@("`0`0`0`0"), 2, [StringSplitOptions]::RemoveEmptyEntries)
     Set-Variable -Name json_raw -Value $split_parts[1]
-    $exec_wrapper = [ScriptBlock]::Create($split_parts[0])
-    &$exec_wrapper
+    &([ScriptBlock]::Create($split_parts[0]))
 }
 $exec_command = [System.Convert]::ToBase64String([System.Text.Encoding]::Unicode.GetBytes($bootstrap_wrapper.ToString()))
 $lp_command_line = "powershell.exe -NonInteractive -NoProfile -ExecutionPolicy Bypass -EncodedCommand $exec_command"

lib/ansible/executor/powershell/bootstrap_wrapper.ps1

@@ -1,4 +1,4 @@
-&chcp.com 65001 > $null
+try { [Console]::InputEncoding = [Console]::OutputEncoding = New-Object System.Text.UTF8Encoding } catch { $null = $_ }
 
 if ($PSVersionTable.PSVersion -lt [Version]"3.0") {
     '{"failed":true,"msg":"Ansible requires PowerShell v3.0 or newer"}'
@@ -9,5 +9,4 @@ $exec_wrapper_str = $input | Out-String
 $split_parts = $exec_wrapper_str.Split(@("`0`0`0`0"), 2, [StringSplitOptions]::RemoveEmptyEntries)
 If (-not $split_parts.Length -eq 2) { throw "invalid payload" }
 Set-Variable -Name json_raw -Value $split_parts[1]
-$exec_wrapper = [ScriptBlock]::Create($split_parts[0])
-&$exec_wrapper
+& ([ScriptBlock]::Create($split_parts[0]))

lib/ansible/plugins/connection/ssh.py

@@ -1306,14 +1306,6 @@ class Connection(ConnectionBase):
             # prompt that will not occur
             sudoable = False
 
-            # Make sure our first command is to set the console encoding to
-            # utf-8, this must be done via chcp to get utf-8 (65001)
-            # union-attr ignores rely on internal powershell shell plugin details,
-            # this should be fixed at a future point in time.
-            cmd_parts = ["chcp.com", "65001", self._shell._SHELL_REDIRECT_ALLNULL, self._shell._SHELL_AND]  # type: ignore[union-attr]
-            cmd_parts.extend(self._shell._encode_script(cmd, as_list=True, strict_mode=False, preserve_rc=False))  # type: ignore[union-attr]
-            cmd = ' '.join(cmd_parts)
-
         # we can only use tty when we are not pipelining the modules. piping
         # data into /usr/bin/python inside a tty automatically invokes the
         # python interactive-mode but the modules are not compatible with the

lib/ansible/plugins/shell/powershell.py

@@ -100,6 +100,8 @@ class ShellModule(ShellBase):
     # Family of shells this has.  Must match the filename without extension
     SHELL_FAMILY = 'powershell'
 
+    # We try catch as some connection plugins don't have a console (PSRP).
+    _CONSOLE_ENCODING = "try { [Console]::OutputEncoding = New-Object System.Text.UTF8Encoding } catch {}"
     _SHELL_REDIRECT_ALLNULL = '> $null'
     _SHELL_AND = ';'
 
@@ -157,13 +159,14 @@ class ShellModule(ShellBase):
         if not basefile:
             basefile = self.__class__._generate_temp_dir_name()
         basefile = self._escape(self._unquote(basefile))
-        basetmpdir = tmpdir if tmpdir else self.get_option('remote_tmp')
+        basetmpdir = self._escape(tmpdir if tmpdir else self.get_option('remote_tmp'))
 
-        script = '''
-        $tmp_path = [System.Environment]::ExpandEnvironmentVariables('%s')
-        $tmp = New-Item -Type Directory -Path $tmp_path -Name '%s'
+        script = f'''
+        {self._CONSOLE_ENCODING}
+        $tmp_path = [System.Environment]::ExpandEnvironmentVariables('{basetmpdir}')
+        $tmp = New-Item -Type Directory -Path $tmp_path -Name '{basefile}'
         Write-Output -InputObject $tmp.FullName
-        ''' % (basetmpdir, basefile)
+        '''
         return self._encode_script(script.strip())
 
     def expand_user(self, user_home_path, username=''):
@@ -177,7 +180,7 @@ class ShellModule(ShellBase):
             script = "Write-Output ((Get-Location).Path + '%s')" % self._escape(user_home_path[1:])
         else:
             script = "Write-Output '%s'" % self._escape(user_home_path)
-        return self._encode_script(script)
+        return self._encode_script(f"{self._CONSOLE_ENCODING}; {script}")
 
     def exists(self, path):
         path = self._escape(self._unquote(path))

test/integration/targets/connection/test_connection.yml

@@ -17,6 +17,7 @@
                 plugin_type="connection",
             ) == "scp"
         }}
+      echo_string: 汉语
 
   - name: set test filename
     set_fact:
@@ -25,12 +26,19 @@
   ### raw with unicode arg and output
 
   - name: raw with unicode arg and output
-    raw: echo 汉语
+    raw: "{{ echo_commands[action_prefix ~ ansible_connection ~ '_' ~ (ansible_shell_type|default(''))] | default(echo_commands['default']) }}"
+    vars:
+      # Windows over SSH does not have a way to set the console codepage to allow UTF-8. We need to
+      # wrap the commands we send to the remote host to get it working.
+      echo_commands:
+        win_ssh_powershell: '[Console]::OutputEncoding = [System.Text.Encoding]::UTF8; echo {{ echo_string }}'
+        win_ssh_cmd: 'powershell.exe -Command "[Console]::OutputEncoding = [System.Text.Encoding]::UTF8; echo {{ echo_string }}"'
+        default: echo {{ echo_string }}
     register: command
   - name: check output of raw with unicode arg and output
     assert:
       that:
-      - "'汉语' in command.stdout"
+      - echo_string in command.stdout
       - command is changed # as of 2.2, raw should default to changed: true for consistency w/ shell/command/script modules
 
   ### copy local file with unicode filename and content

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

@@ -25,7 +25,7 @@
     that:
       - "getmac_result.rc == 0"
       - "getmac_result.stdout"
-      - "not getmac_result.stderr"
+      - (ansible_connection == 'ssh') | ternary(getmac_result.stderr is defined, not getmac_result.stderr)
       - "getmac_result is not failed"
       - "getmac_result is changed"
 
@@ -39,7 +39,7 @@
       - "ipconfig_result.rc == 0"
       - "ipconfig_result.stdout"
       - "'Physical Address' in ipconfig_result.stdout"
-      - "not ipconfig_result.stderr"
+      - (ansible_connection == 'ssh') | ternary(ipconfig_result.stderr is defined, not ipconfig_result.stderr)
       - "ipconfig_result is not failed"
       - "ipconfig_result is changed"
 
@@ -80,7 +80,7 @@
     that:
       - "sleep_command.rc == 0"
       - "not sleep_command.stdout"
-      - "not sleep_command.stderr"
+      - (ansible_connection == 'ssh') | ternary(sleep_command.stderr is defined, not sleep_command.stderr)
       - "sleep_command is not failed"
       - "sleep_command is changed"
 
@@ -93,11 +93,14 @@
     that:
       - "raw_result.stdout_lines[0] == 'wwe=raw'"
 
+# ssh cannot pre-set the codepage so we need to do it in the command.
 - name: unicode tests for winrm
   when: ansible_connection != 'psrp'  # Write-Host does not work over PSRP
   block:
   - name: run a raw command with unicode chars and quoted args (from https://github.com/ansible/ansible-modules-core/issues/1929)
-    raw: Write-Host --% icacls D:\somedir\ /grant "! ЗАО. Руководство":F
+    raw: |
+      {{ (ansible_connection == 'ssh') | ternary("[Console]::OutputEncoding = [System.Text.Encoding]::UTF8", "") }}
+      Write-Host --% icacls D:\somedir\ /grant "! ЗАО. Руководство":F
     register: raw_result2
 
   - name: make sure raw passes command as-is and doesn't split/rejoin args

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

@@ -38,7 +38,7 @@
       - "test_script_result.rc == 0"
       - "test_script_result.stdout"
       - "'Woohoo' in test_script_result.stdout"
-      - "not test_script_result.stderr"
+      - (ansible_connection == 'ssh') | ternary(test_script_result.stderr is defined, not test_script_result.stderr)
       - "test_script_result is not failed"
       - "test_script_result is changed"
 
@@ -54,7 +54,7 @@
       - "test_script_with_args_result.stdout_lines[0] == '/this'"
       - "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"
+      - (ansible_connection == 'ssh') | ternary(test_script_with_args_result.stderr is defined, not test_script_with_args_result.stderr)
       - "test_script_with_args_result is not failed"
       - "test_script_with_args_result is changed"
 
@@ -71,7 +71,7 @@
   assert:
     that:
     - test_script_with_large_args_result.rc == 0
-    - not test_script_with_large_args_result.stderr
+    - (ansible_connection == 'ssh') | ternary(test_script_with_large_args_result.stderr is defined, not test_script_with_large_args_result.stderr)
     - test_script_with_large_args_result is not failed
     - test_script_with_large_args_result is changed
 
@@ -99,7 +99,7 @@
       - "test_script_with_splatting_result.stdout_lines[0] == 'this'"
       - "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"
+      - (ansible_connection == 'ssh') | ternary(test_script_with_splatting_result.stderr is defined, not test_script_with_splatting_result.stderr)
       - "test_script_with_splatting_result is not failed"
       - "test_script_with_splatting_result is changed"
 
@@ -115,7 +115,7 @@
       - "test_script_with_splatting2_result.stdout_lines[0] == 'THIS'"
       - "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"
+      - (ansible_connection == 'ssh') | ternary(test_script_with_splatting2_result.stderr is defined, not test_script_with_splatting2_result.stderr)
       - "test_script_with_splatting2_result is not failed"
       - "test_script_with_splatting2_result is changed"
 
@@ -148,7 +148,7 @@
     that:
       - "test_script_creates_file_result.rc == 0"
       - "not test_script_creates_file_result.stdout"
-      - "not test_script_creates_file_result.stderr"
+      - (ansible_connection == 'ssh') | ternary(test_script_creates_file_result.stderr is defined, not test_script_creates_file_result.stderr)
       - "test_script_creates_file_result is not failed"
       - "test_script_creates_file_result is changed"
 
@@ -176,7 +176,7 @@
     that:
       - "test_script_removes_file_result.rc == 0"
       - "not test_script_removes_file_result.stdout"
-      - "not test_script_removes_file_result.stderr"
+      - (ansible_connection == 'ssh') | ternary(test_script_removes_file_result.stderr is defined, not test_script_removes_file_result.stderr)
       - "test_script_removes_file_result is not failed"
       - "test_script_removes_file_result is changed"
 
@@ -205,7 +205,7 @@
         - "test_batch_result.rc == 0"
         - "test_batch_result.stdout"
         - "'batch' in test_batch_result.stdout"
-        - "not test_batch_result.stderr"
+        - (ansible_connection == 'ssh') | ternary(test_batch_result.stderr is defined, not test_batch_result.stderr)
         - "test_batch_result is not failed"
         - "test_batch_result is changed"
 
@@ -219,7 +219,7 @@
         - "test_cmd_result.rc == 0"
         - "test_cmd_result.stdout"
         - "'cmd extension' in test_cmd_result.stdout"
-        - "not test_cmd_result.stderr"
+        - (ansible_connection == 'ssh') | ternary(test_cmd_result.stderr is defined, not test_cmd_result.stderr)
         - "test_cmd_result is not failed"
         - "test_cmd_result is changed"
 

test/integration/targets/windows-minimal/aliases

@@ -1,4 +1,5 @@
 shippable/windows/group1
 shippable/windows/minimal
 shippable/windows/smoketest
+needs/target/setup_remote_tmp_dir
 windows

test/integration/targets/windows-minimal/meta/main.yml

@@ -0,0 +1,2 @@
+dependencies:
+  - setup_remote_tmp_dir

test/integration/targets/windows-minimal/tasks/main.yml

@@ -85,3 +85,35 @@
   ansible.windows.win_shell: echo "name=foo"
   register: win_shell_collection_res
   failed_when: win_shell_collection_res.stdout | trim != 'name=foo'
+
+- name: set ping data fact
+  set_fact:
+    # FUTURE: Fix psrp so it can handle non-ASCII chars in a non-pipeline scenario
+    ping_data: '{{ (ansible_connection == "psrp") | ternary("test", "汉语") }}'
+
+- name: run module with pipelining disabled
+  ansible.builtin.command:
+    cmd: >-
+      ansible windows
+      -m ansible.windows.win_ping
+      -a 'data={{ ping_data }}'
+      -i {{ '-i '.join(ansible_inventory_sources) }}
+      {{ '' if not ansible_verbosity else '-' ~ ('v' * ansible_verbosity) }}
+      -e ansible_remote_tmp='{{ remote_tmp_dir | regex_replace('\\', '\\\\') }}'
+  register: pipeline_disabled_res
+  delegate_to: localhost
+  environment:
+    ANSIBLE_KEEP_REMOTE_FILES: 'true'
+    ANSIBLE_NOCOLOR: 'true'
+    ANSIBLE_FORCE_COLOR: 'false'
+
+- name: view temp files
+  ansible.windows.win_shell: (Get-Item '{{ remote_tmp_dir }}\ansible-tmp-*\*').Name
+  register: pipeline_disabled_files
+
+- name: assert run module with pipelining disabled
+  assert:
+    that:
+    - >-
+      pipeline_disabled_res.stdout is search('\"ping\": \"' ~ ping_data ~ '\"')
+    - pipeline_disabled_files.stdout_lines == ["AnsiballZ_win_ping.ps1"]