diff --git a/changelog/52452.fixed.md b/changelog/52452.fixed.md new file mode 100644 index 000000000000..4b09aedca676 --- /dev/null +++ b/changelog/52452.fixed.md @@ -0,0 +1 @@ +Fixed salt-ssh continues state/pillar rendering with incorrect data when an exception is raised by a module on the target diff --git a/salt/client/ssh/__init__.py b/salt/client/ssh/__init__.py index 0d9ac9509b64..208edd591883 100644 --- a/salt/client/ssh/__init__.py +++ b/salt/client/ssh/__init__.py @@ -546,29 +546,37 @@ def handle_routine(self, que, opts, host, target, mine=False): **target, ) ret = {"id": single.id} - stdout, stderr, retcode = single.run() - # This job is done, yield + stdout = stderr = "" + retcode = 0 try: - data = salt.utils.json.find_json(stdout) - if len(data) < 2 and "local" in data: - ret["ret"] = data["local"] - try: - # Ensure a reported local retcode is kept - retcode = data["local"]["retcode"] - except (KeyError, TypeError): - pass - else: - ret["ret"] = { - "stdout": stdout, - "stderr": stderr, - "retcode": retcode, - } - except Exception: # pylint: disable=broad-except + stdout, stderr, retcode = single.run() + ret["ret"] = salt.client.ssh.wrapper.parse_ret(stdout, stderr, retcode) + except ( + salt.client.ssh.wrapper.SSHPermissionDeniedError, + salt.client.ssh.wrapper.SSHCommandExecutionError, + ) as err: + ret["ret"] = err.to_ret() + # All caught errors always indicate the retcode is/should be > 0 + retcode = max(retcode, err.retcode, 1) + except salt.client.ssh.wrapper.SSHException as err: + ret["ret"] = err.to_ret() + if not self.opts.get("raw_shell"): + # We only expect valid JSON output from Salt + retcode = max(retcode, err.retcode, 1) + ret["ret"].pop("_error", None) + except Exception as err: # pylint: disable=broad-except + log.error( + f"Error while parsing the command output: {err}", + exc_info_on_loglevel=logging.DEBUG, + ) ret["ret"] = { + "_error": f"Internal error while parsing the command output: {err}", "stdout": stdout, "stderr": stderr, "retcode": retcode, + "data": None, } + retcode = max(retcode, 1) que.put((ret, retcode)) def handle_ssh(self, mine=False): @@ -716,7 +724,7 @@ def run_iter(self, mine=False, jid=None): self.cache_job(jid, host, ret[host], fun) if self.event: id_, data = next(iter(ret.items())) - if isinstance(data, str): + if not isinstance(data, dict): data = {"return": data} if "id" not in data: data["id"] = id_ @@ -830,7 +838,7 @@ def run(self, jid=None): salt.output.display_output(p_data, outputter, self.opts) if self.event: id_, data = next(iter(ret.items())) - if isinstance(data, str): + if not isinstance(data, dict): data = {"return": data} if "id" not in data: data["id"] = id_ @@ -1056,7 +1064,8 @@ def run(self, deploy_attempted=False): Returns tuple of (stdout, stderr, retcode) """ - stdout = stderr = retcode = None + stdout = stderr = "" + retcode = 0 if self.ssh_pre_flight: if not self.opts.get("ssh_run_pre_flight", False) and self.check_thin_dir(): @@ -1133,11 +1142,6 @@ def run_wfunc(self): ) opts_pkg = pre_wrapper["test.opts_pkg"]() # pylint: disable=E1102 - if "_error" in opts_pkg: - # Refresh failed - retcode = opts_pkg["retcode"] - ret = salt.utils.json.dumps({"local": opts_pkg}) - return ret, retcode opts_pkg["file_roots"] = self.opts["file_roots"] opts_pkg["pillar_roots"] = self.opts["pillar_roots"] @@ -1262,14 +1266,18 @@ def run_wfunc(self): result = wrapper[mine_fun](*self.args, **self.kwargs) else: result = self.wfuncs[self.fun](*self.args, **self.kwargs) + except salt.client.ssh.wrapper.SSHException: + # SSHExceptions indicating remote command failure or + # parsing issues are handled centrally in SSH.handle_routine + raise except TypeError as exc: - result = f"TypeError encountered executing {self.fun}: {exc}" + result = {"local": f"TypeError encountered executing {self.fun}: {exc}"} log.error(result, exc_info_on_loglevel=logging.DEBUG) retcode = 1 except Exception as exc: # pylint: disable=broad-except - result = "An Exception occurred while executing {}: {}".format( - self.fun, exc - ) + result = { + "local": f"An Exception occurred while executing {self.fun}: {exc}" + } log.error(result, exc_info_on_loglevel=logging.DEBUG) retcode = 1 @@ -1280,6 +1288,10 @@ def run_wfunc(self): # emit (as seen in ssh_py_shim.py) if isinstance(result, dict) and "local" in result: ret = salt.utils.json.dumps({"local": result["local"]}) + elif self.context.get("retcode"): + # The wrapped command failed, the usual behavior is that + # the return is dumped as-is without declaring it as a result. + ret = salt.utils.json.dumps({"local": result}) else: ret = salt.utils.json.dumps({"local": {"return": result}}) return ret, retcode diff --git a/salt/client/ssh/wrapper/__init__.py b/salt/client/ssh/wrapper/__init__.py index 42bdc624f212..ce94b20f6cf0 100644 --- a/salt/client/ssh/wrapper/__init__.py +++ b/salt/client/ssh/wrapper/__init__.py @@ -12,6 +12,86 @@ import salt.loader import salt.utils.data import salt.utils.json +from salt.defaults import NOT_SET +from salt.exceptions import CommandExecutionError, SaltException + + +class SSHException(SaltException): + """ + Indicates general command failure via salt-ssh. + """ + + _error = "" + + def __init__( + self, stdout, stderr, retcode, result=NOT_SET, data=None, *args, **kwargs + ): + super().__init__(stderr, *args, **kwargs) + self.stdout = stdout + self.stderr = stderr + self.result = result + self.data = data + self.retcode = retcode + if args: + self._error = args.pop(0) + + def to_ret(self): + ret = { + "stdout": self.stdout, + "stderr": self.stderr, + "retcode": self.retcode, + "data": self.data, + } + if self._error: + ret["_error"] = self._error + if self.result is not NOT_SET: + ret["return"] = self.result + return ret + + +class SSHCommandExecutionError(SSHException, CommandExecutionError): + """ + Thrown whenever a non-zero exit code is returned. + This was introduced to make the salt-ssh FunctionWrapper behave + more like the usual one, in particular to force template rendering + to stop when a function call results in an exception. + """ + + _error = "The command resulted in a non-zero exit code" + + def to_ret(self): + if self.data and "local" in self.data: + # Wrapped commands that indicate a non-zero retcode + return self.data["local"] + elif self.stderr: + # Remote executions that indicate a non-zero retcode + return self.stderr + return super().to_ret() + + +class SSHPermissionDeniedError(SSHException): + """ + Thrown when "Permission denied" is found in stderr + """ + + _error = "Permission Denied" + + +class SSHReturnDecodeError(SSHException): + """ + Thrown when JSON-decoding stdout fails and the retcode is 0 otherwise + """ + + _error = "Failed to return clean data" + + +class SSHMalformedReturnError(SSHException): + """ + Thrown when a decoded return dict is not formed as + {"local": {"return": ...}} + """ + + _error = "Return dict was malformed" class FunctionWrapper: @@ -119,26 +199,7 @@ def caller(*args, **kwargs): **self.kwargs ) stdout, stderr, retcode = single.cmd_block() - if stderr.count("Permission Denied"): - return { - "_error": "Permission Denied", - "stdout": stdout, - "stderr": stderr, - "retcode": retcode, - } - try: - ret = salt.utils.json.loads(stdout) - if len(ret) < 2 and "local" in ret: - ret = ret["local"] - ret = ret.get("return", {}) - except ValueError: - ret = { - "_error": "Failed to return clean data", - "stderr": stderr, - "stdout": stdout, - "retcode": retcode, - } - return ret + return parse_ret(stdout, stderr, retcode, result_only=True) return caller @@ -176,3 +237,61 @@ def get(self, cmd, default): return self[cmd] else: return default + + +def parse_ret(stdout, stderr, retcode, result_only=False): + """ + Parse the output of a remote or local command and return its + result. Raise exceptions if the command has a non-zero exitcode + or its output is not valid JSON or is not in the expected format, + usually ``{"local": {"return": value}}`` (+ optional keys in the "local" dict). + """ + if retcode and stderr.count("Permission Denied"): + raise SSHPermissionDeniedError(stdout=stdout, stderr=stderr, retcode=retcode) + + result = NOT_SET + error = None + data = None + local_data = None + + try: + data = salt.utils.json.loads(stdout) + if len(data) < 2 and "local" in data: + try: + result = data["local"] + try: + # Ensure a reported local retcode is kept (at least) + retcode = max(retcode, result["retcode"]) + except (KeyError, TypeError): + pass + if not isinstance(data["local"], dict): + # When a command has failed, the return is dumped as-is + # without declaring it as a result, usually a string or list. + error = SSHCommandExecutionError + elif result_only: + result = result["return"] + except KeyError: + error = SSHMalformedReturnError + else: + error = SSHMalformedReturnError + + except ValueError: + # No valid JSON output was found + error = SSHReturnDecodeError + if retcode: + raise SSHCommandExecutionError( + stdout=stdout, + stderr=stderr, + retcode=retcode, + result=result, + data=local_data if local_data is not None else data, + ) + if error is not None: + raise error( + stdout=stdout, + stderr=stderr, + retcode=retcode, + result=result, + data=local_data if local_data is not None else data, + ) + return result