From 7865b198d26e3948777e1e3ee89e699075da2223 Mon Sep 17 00:00:00 2001
From: Jordan Borean <jborean93@gmail.com>
Date: Fri, 25 Aug 2023 05:24:02 +1000
Subject: [PATCH] PowerShell - Improve error checking (#80984)

Improves the error checking when running PowerShell modules using
Ansible.ModuleUtils.Legacy. It will only return an rc of 1 if both the
PowerShell module runner signalled an error occurred and those error
records were present in the output. This should reduce some false
positive errors when using the older module style.
---
 .../powershell-module-error-handling.yml      |  4 +++
 .../executor/powershell/module_wrapper.ps1    |  5 ++-
 .../action_plugins/test_rc_1.py               | 35 +++++++++++++++++++
 .../win_exec_wrapper/library/test_rc_1.ps1    | 17 +++++++++
 .../targets/win_exec_wrapper/tasks/main.yml   |  9 +++++
 5 files changed, 69 insertions(+), 1 deletion(-)
 create mode 100644 changelogs/fragments/powershell-module-error-handling.yml
 create mode 100644 test/integration/targets/win_exec_wrapper/action_plugins/test_rc_1.py
 create mode 100644 test/integration/targets/win_exec_wrapper/library/test_rc_1.ps1

diff --git a/changelogs/fragments/powershell-module-error-handling.yml b/changelogs/fragments/powershell-module-error-handling.yml
new file mode 100644
index 00000000000..cdc40e36efd
--- /dev/null
+++ b/changelogs/fragments/powershell-module-error-handling.yml
@@ -0,0 +1,4 @@
+bugfixes:
+- >-
+  powershell modules - Only set an rc of 1 if the PowerShell pipeline signaled an error occurred AND there are error
+  records present. Previously it would do so only if the error signal was present without checking the error count.
diff --git a/lib/ansible/executor/powershell/module_wrapper.ps1 b/lib/ansible/executor/powershell/module_wrapper.ps1
index 20a967731b7..1cfaf3ceae1 100644
--- a/lib/ansible/executor/powershell/module_wrapper.ps1
+++ b/lib/ansible/executor/powershell/module_wrapper.ps1
@@ -207,7 +207,10 @@ if ($null -ne $rc) {
 # with the trap handler that's now in place, this should only write to the output if
 # $ErrorActionPreference != "Stop", that's ok because this is sent to the stderr output
 # for a user to manually debug if something went horribly wrong
-if ($ps.HadErrors -or ($PSVersionTable.PSVersion.Major -lt 4 -and $ps.Streams.Error.Count -gt 0)) {
+if (
+    $ps.Streams.Error.Count -and
+    ($ps.HadErrors -or $PSVersionTable.PSVersion.Major -lt 4)
+) {
     Write-AnsibleLog "WARN - module had errors, outputting error info $ModuleName" "module_wrapper"
     # if the rc wasn't explicitly set, we return an exit code of 1
     if ($null -eq $rc) {
diff --git a/test/integration/targets/win_exec_wrapper/action_plugins/test_rc_1.py b/test/integration/targets/win_exec_wrapper/action_plugins/test_rc_1.py
new file mode 100644
index 00000000000..60cffde96bb
--- /dev/null
+++ b/test/integration/targets/win_exec_wrapper/action_plugins/test_rc_1.py
@@ -0,0 +1,35 @@
+# Copyright: (c) 2023, Ansible Project
+# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
+
+import json
+
+from ansible.plugins.action import ActionBase
+
+
+class ActionModule(ActionBase):
+
+    def run(self, tmp=None, task_vars=None):
+        super().run(tmp, task_vars)
+        del tmp
+
+        exec_command = self._connection.exec_command
+
+        def patched_exec_command(*args, **kwargs):
+            rc, stdout, stderr = exec_command(*args, **kwargs)
+
+            new_stdout = json.dumps({
+                "rc": rc,
+                "stdout": stdout.decode(),
+                "stderr": stderr.decode(),
+                "failed": False,
+                "changed": False,
+            }).encode()
+
+            return (0, new_stdout, b"")
+
+        try:
+            # This is done to capture the raw rc/stdio from the module exec
+            self._connection.exec_command = patched_exec_command
+            return self._execute_module(task_vars=task_vars)
+        finally:
+            self._connection.exec_command = exec_command
diff --git a/test/integration/targets/win_exec_wrapper/library/test_rc_1.ps1 b/test/integration/targets/win_exec_wrapper/library/test_rc_1.ps1
new file mode 100644
index 00000000000..a9879548fa9
--- /dev/null
+++ b/test/integration/targets/win_exec_wrapper/library/test_rc_1.ps1
@@ -0,0 +1,17 @@
+#!powershell
+
+# This scenario needs to use Legacy, the same HadErrors won't be set if using
+# Ansible.Basic
+#Requires -Module Ansible.ModuleUtils.Legacy
+
+# This will set `$ps.HadErrors` in the running pipeline but with no error
+# record written. We are testing that it won't set the rc to 1 for this
+# scenario.
+try {
+    Write-Error -Message err -ErrorAction Stop
+}
+catch {
+    Exit-Json @{}
+}
+
+Fail-Json @{} "This should not be reached"
diff --git a/test/integration/targets/win_exec_wrapper/tasks/main.yml b/test/integration/targets/win_exec_wrapper/tasks/main.yml
index 8fc54f7ca8f..f1342c480b1 100644
--- a/test/integration/targets/win_exec_wrapper/tasks/main.yml
+++ b/test/integration/targets/win_exec_wrapper/tasks/main.yml
@@ -272,3 +272,12 @@
   assert:
     that:
     - ps_log_count.stdout | int == 0
+
+- name: test module that sets HadErrors with no error records
+  test_rc_1:
+  register: module_had_errors
+
+- name: assert test module that sets HadErrors with no error records
+  assert:
+    that:
+    - module_had_errors.rc == 0