From c2bb925bc6317ff5963469db6bd01f6a6f12ded7 Mon Sep 17 00:00:00 2001 From: twangboy Date: Wed, 5 Mar 2025 13:26:33 -0700 Subject: [PATCH] Fix checking retcodes with runas on Windows --- changelog/59977.fixed.md | 2 + salt/modules/cmdmod.py | 162 ++++++++++-------- .../functional/modules/cmd/test_run_win.py | 50 ++++++ 3 files changed, 139 insertions(+), 75 deletions(-) create mode 100644 changelog/59977.fixed.md create mode 100644 tests/pytests/functional/modules/cmd/test_run_win.py diff --git a/changelog/59977.fixed.md b/changelog/59977.fixed.md new file mode 100644 index 00000000000..9069a8d621b --- /dev/null +++ b/changelog/59977.fixed.md @@ -0,0 +1,2 @@ +Fixed an issue on Windows where checking success_retcodes when using the +runas parameter would fail. Now success_retcodes are checked correctly diff --git a/salt/modules/cmdmod.py b/salt/modules/cmdmod.py index dd01fb11c87..4c7fd40d02a 100644 --- a/salt/modules/cmdmod.py +++ b/salt/modules/cmdmod.py @@ -458,8 +458,6 @@ def _run( if isinstance(cmd, (list, tuple)): cmd = " ".join(cmd) - return win_runas(cmd, runas, password, cwd) - if runas and salt.utils.platform.is_darwin(): # We need to insert the user simulation into the command itself and not # just run it from the environment on macOS as that method doesn't work @@ -492,7 +490,7 @@ def _run( # hang. runas = None - if runas: + if runas and not salt.utils.platform.is_windows(): # Save the original command before munging it try: pwd.getpwnam(runas) @@ -513,7 +511,7 @@ def _run( else: use_sudo = True - if runas or group: + if (runas or group) and not salt.utils.platform.is_windows(): try: # Getting the environment for the runas user # Use markers to thwart any stdout noise @@ -752,90 +750,104 @@ def _run( if not use_vt: # This is where the magic happens - try: + + if runas and salt.utils.platform.is_windows(): + + # We can't use TimedProc with runas on Windows if change_windows_codepage: salt.utils.win_chcp.set_codepage_id(windows_codepage) - try: - proc = salt.utils.timed_subprocess.TimedProc(cmd, **new_kwargs) - except OSError as exc: - msg = "Unable to run command '{}' with the context '{}', reason: {}".format( - cmd if output_loglevel is not None else "REDACTED", - new_kwargs, - exc, - ) - raise CommandExecutionError(msg) - try: - proc.run() - except TimedProcTimeoutError as exc: - ret["stdout"] = str(exc) - ret["stderr"] = "" - ret["retcode"] = None - ret["pid"] = proc.process.pid - # ok return code for timeouts? - ret["retcode"] = 1 - return ret - finally: + ret = win_runas(cmd, runas, password, cwd) + if change_windows_codepage: salt.utils.win_chcp.set_codepage_id(previous_windows_codepage) - if output_loglevel != "quiet" and output_encoding is not None: - log.debug( - "Decoding output from command %s using %s encoding", - cmd, - output_encoding, - ) + else: + try: + if change_windows_codepage: + salt.utils.win_chcp.set_codepage_id(windows_codepage) + try: + proc = salt.utils.timed_subprocess.TimedProc(cmd, **new_kwargs) + except OSError as exc: + msg = "Unable to run command '{}' with the context '{}', reason: {}".format( + cmd if output_loglevel is not None else "REDACTED", + new_kwargs, + exc, + ) + raise CommandExecutionError(msg) - try: - out = salt.utils.stringutils.to_unicode( - proc.stdout, encoding=output_encoding - ) - except TypeError: - # stdout is None - out = "" - except UnicodeDecodeError: - out = salt.utils.stringutils.to_unicode( - proc.stdout, encoding=output_encoding, errors="replace" - ) - if output_loglevel != "quiet": - log.error( - "Failed to decode stdout from command %s, non-decodable " - "characters have been replaced", - _log_cmd(cmd), + try: + proc.run() + except TimedProcTimeoutError as exc: + ret["stdout"] = str(exc) + ret["stderr"] = "" + ret["retcode"] = None + ret["pid"] = proc.process.pid + # ok return code for timeouts? + ret["retcode"] = 1 + return ret + finally: + if change_windows_codepage: + salt.utils.win_chcp.set_codepage_id(previous_windows_codepage) + + if output_loglevel != "quiet" and output_encoding is not None: + log.debug( + "Decoding output from command %s using %s encoding", + cmd, + output_encoding, ) - try: - err = salt.utils.stringutils.to_unicode( - proc.stderr, encoding=output_encoding - ) - except TypeError: - # stderr is None - err = "" - except UnicodeDecodeError: - err = salt.utils.stringutils.to_unicode( - proc.stderr, encoding=output_encoding, errors="replace" - ) - if output_loglevel != "quiet": - log.error( - "Failed to decode stderr from command %s, non-decodable " - "characters have been replaced", - _log_cmd(cmd), + try: + out = salt.utils.stringutils.to_unicode( + proc.stdout, encoding=output_encoding ) + except TypeError: + # stdout is None + out = "" + except UnicodeDecodeError: + out = salt.utils.stringutils.to_unicode( + proc.stdout, encoding=output_encoding, errors="replace" + ) + if output_loglevel != "quiet": + log.error( + "Failed to decode stdout from command %s, non-decodable " + "characters have been replaced", + _log_cmd(cmd), + ) + + try: + err = salt.utils.stringutils.to_unicode( + proc.stderr, encoding=output_encoding + ) + except TypeError: + # stderr is None + err = "" + except UnicodeDecodeError: + err = salt.utils.stringutils.to_unicode( + proc.stderr, encoding=output_encoding, errors="replace" + ) + if output_loglevel != "quiet": + log.error( + "Failed to decode stderr from command %s, non-decodable " + "characters have been replaced", + _log_cmd(cmd), + ) + + # Encoded commands dump CLIXML data in stderr. It's not an actual error + if encoded_cmd and "CLIXML" in err: + err = "" + if rstrip: + if out is not None: + out = out.rstrip() + if err is not None: + err = err.rstrip() + ret["pid"] = proc.process.pid + ret["retcode"] = proc.process.returncode + ret["stdout"] = out + ret["stderr"] = err - # Encoded commands dump CLIXML data in stderr. It's not an actual error - if encoded_cmd and "CLIXML" in err: - err = "" - if rstrip: - if out is not None: - out = out.rstrip() - if err is not None: - err = err.rstrip() - ret["pid"] = proc.process.pid - ret["retcode"] = proc.process.returncode if ret["retcode"] in success_retcodes: ret["retcode"] = 0 - ret["stdout"] = out - ret["stderr"] = err if any( [stdo in ret["stdout"] for stdo in success_stdout] + [stde in ret["stderr"] for stde in success_stderr] diff --git a/tests/pytests/functional/modules/cmd/test_run_win.py b/tests/pytests/functional/modules/cmd/test_run_win.py new file mode 100644 index 00000000000..cf41eb50280 --- /dev/null +++ b/tests/pytests/functional/modules/cmd/test_run_win.py @@ -0,0 +1,50 @@ +import pytest + +pytestmark = [ + pytest.mark.core_test, + pytest.mark.windows_whitelisted, +] + + +@pytest.fixture(scope="module") +def account(): + with pytest.helpers.create_account() as _account: + yield _account + + +@pytest.mark.skip_unless_on_windows(reason="Minion is not Windows") +@pytest.mark.parametrize( + "exit_code, return_code, result", + [ + (300, 0, True), + (299, 299, False), + ], +) +def test_windows_script_exitcode(modules, state_tree, exit_code, return_code, result): + ret = modules.state.single( + "cmd.run", name=f"cmd.exe /c exit {exit_code}", success_retcodes=[2, 44, 300] + ) + assert ret.result is result + assert ret.filtered["changes"]["retcode"] == return_code + + +@pytest.mark.skip_unless_on_windows(reason="Minion is not Windows") +@pytest.mark.parametrize( + "exit_code, return_code, result", + [ + (300, 0, True), + (299, 299, False), + ], +) +def test_windows_script_exitcode_runas( + modules, state_tree, exit_code, return_code, result, account +): + ret = modules.state.single( + "cmd.run", + name=f"cmd.exe /c exit {exit_code}", + success_retcodes=[2, 44, 300], + runas=account.username, + password=account.password, + ) + assert ret.result is result + assert ret.filtered["changes"]["retcode"] == return_code