Fix salt-ssh stacktrace when retcode is not an integer

changelog/64575.fixed.md

@@ -0,0 +1 @@
+Fixed salt-ssh stacktrace when retcode is not an integer

salt/client/ssh/__init__.py

@@ -556,6 +556,11 @@ class SSH(MultiprocessingStateMixin):
         )
         ret = {"id": single.id}
         stdout, stderr, retcode = single.run()
+        try:
+            retcode = int(retcode)
+        except (TypeError, ValueError):
+            log.warning(f"Got an invalid retcode for host '{host}': '{retcode}'")
+            retcode = 1
         # This job is done, yield
         try:
             data = salt.utils.json.find_json(stdout)
@@ -563,7 +568,14 @@ class SSH(MultiprocessingStateMixin):
                 ret["ret"] = data["local"]
                 try:
                     # Ensure a reported local retcode is kept
-                    retcode = data["local"]["retcode"]
+                    remote_retcode = data["local"]["retcode"]
+                    try:
+                        retcode = int(remote_retcode)
+                    except (TypeError, ValueError):
+                        log.warning(
+                            f"Host '{host}' reported an invalid retcode: '{remote_retcode}'"
+                        )
+                        retcode = max(retcode, 1)
                 except (KeyError, TypeError):
                     pass
             else:
@@ -816,6 +828,9 @@ class SSH(MultiprocessingStateMixin):
         final_exit = 0
         for ret, retcode in self.handle_ssh():
             host = next(iter(ret))
+            if not isinstance(retcode, int):
+                log.warning(f"Host '{host}' returned an invalid retcode: {retcode}")
+                retcode = 1
             final_exit = max(final_exit, retcode)
 
             self.cache_job(jid, host, ret[host], fun)

tests/pytests/unit/client/ssh/test_ssh.py

@@ -3,7 +3,7 @@ import pytest
 import salt.client.ssh.client
 import salt.utils.msgpack
 from salt.client import ssh
-from tests.support.mock import MagicMock, patch
+from tests.support.mock import MagicMock, Mock, patch
 
 pytestmark = [
     pytest.mark.skip_if_binaries_missing("ssh", "ssh-keygen", check_all=True),
@@ -449,3 +449,60 @@ def test_key_deploy_no_permission_denied(tmp_path, opts):
     ret = client.key_deploy(host, ssh_ret)
     assert ret == ssh_ret
     assert mock_key_run.call_count == 0
+
+
+@pytest.mark.parametrize("retcode,expected", [("null", None), ('"foo"', "foo")])
+def test_handle_routine_remote_invalid_retcode(opts, target, retcode, expected, caplog):
+    """
+    Ensure that if a remote returns an invalid retcode as part of the return dict,
+    the final exit code is still an integer and set to 1 at least.
+    """
+    single_ret = (f'{{"local": {{"retcode": {retcode}, "return": "foo"}}}}', "", 0)
+    opts["tgt"] = "localhost"
+    single = MagicMock(spec=ssh.Single)
+    single.id = "localhost"
+    single.run.return_value = single_ret
+    que = Mock()
+
+    with patch("salt.roster.get_roster_file", MagicMock(return_value="")), patch(
+        "salt.client.ssh.Single", autospec=True, return_value=single
+    ):
+        client = ssh.SSH(opts)
+        client.handle_routine(que, opts, "localhost", target)
+    que.put.assert_called_once_with(
+        ({"id": "localhost", "ret": {"retcode": expected, "return": "foo"}}, 1)
+    )
+    assert f"Host 'localhost' reported an invalid retcode: '{expected}'" in caplog.text
+
+
+def test_handle_routine_single_run_invalid_retcode(opts, target, caplog):
+    """
+    Ensure that if Single.run() call returns an invalid retcode,
+    the final exit code is still an integer and set to 1 at least.
+    """
+    single_ret = ("", "Something went seriously wrong", None)
+    opts["tgt"] = "localhost"
+    single = MagicMock(spec=ssh.Single)
+    single.id = "localhost"
+    single.run.return_value = single_ret
+    que = Mock()
+
+    with patch("salt.roster.get_roster_file", MagicMock(return_value="")), patch(
+        "salt.client.ssh.Single", autospec=True, return_value=single
+    ):
+        client = ssh.SSH(opts)
+        client.handle_routine(que, opts, "localhost", target)
+    que.put.assert_called_once_with(
+        (
+            {
+                "id": "localhost",
+                "ret": {
+                    "stdout": "",
+                    "stderr": "Something went seriously wrong",
+                    "retcode": 1,
+                },
+            },
+            1,
+        )
+    )
+    assert "Got an invalid retcode for host 'localhost': 'None'" in caplog.text