From e89daea26489c891f9c9de07324e0d570894f44e Mon Sep 17 00:00:00 2001
From: jeanluc <lkubb@protonmail.com>
Date: Fri, 23 Jun 2023 13:01:06 +0200
Subject: [PATCH] Add tests for issues #64531 and #52452

---
 .../test_retcode_render_module_exception.py   |  67 +++++++
 ...test_retcode_state_run_remote_exception.py |  91 +++++++++
 tests/pytests/integration/ssh/test_deploy.py  | 175 ++++++++++++++++++
 tests/pytests/integration/ssh/test_mine.py    |  35 ++++
 4 files changed, 368 insertions(+)
 create mode 100644 tests/pytests/integration/ssh/state/test_retcode_render_module_exception.py
 create mode 100644 tests/pytests/integration/ssh/state/test_retcode_state_run_remote_exception.py

diff --git a/tests/pytests/integration/ssh/state/test_retcode_render_module_exception.py b/tests/pytests/integration/ssh/state/test_retcode_render_module_exception.py
new file mode 100644
index 000000000000..7125485a69b0
--- /dev/null
+++ b/tests/pytests/integration/ssh/state/test_retcode_render_module_exception.py
@@ -0,0 +1,67 @@
+"""
+Verify salt-ssh stops state execution and fails with a retcode > 0
+when a state rendering fails because an execution module throws an exception.
+"""
+
+import pytest
+
+from salt.defaults.exitcodes import EX_AGGREGATE
+
+pytestmark = [
+    pytest.mark.skip_on_windows(reason="salt-ssh not available on Windows"),
+    pytest.mark.slow_test,
+]
+
+
+@pytest.fixture(scope="module", autouse=True)
+def state_tree_render_module_exception(base_env_state_tree_root_dir):
+    top_file = """
+    base:
+      'localhost':
+        - fail_module_exception
+      '127.0.0.1':
+        - fail_module_exception
+    """
+    state_file = r"""
+    This should fail being rendered:
+      test.show_notification:
+        - text: {{ salt["disk.usage"]("c") | yaml_dquote }}
+    """
+    top_tempfile = pytest.helpers.temp_file(
+        "top.sls", top_file, base_env_state_tree_root_dir
+    )
+    state_tempfile = pytest.helpers.temp_file(
+        "fail_module_exception.sls", state_file, base_env_state_tree_root_dir
+    )
+    with top_tempfile, state_tempfile:
+        yield
+
+
+@pytest.mark.parametrize(
+    "args,retcode",
+    (
+        (("state.sls", "fail_module_exception"), EX_AGGREGATE),
+        (("state.highstate",), EX_AGGREGATE),
+        (("state.sls_id", "foo", "fail_module_exception"), EX_AGGREGATE),
+        (("state.show_sls", "fail_module_exception"), EX_AGGREGATE),
+        (("state.show_low_sls", "fail_module_exception"), EX_AGGREGATE),
+        (("state.show_highstate",), EX_AGGREGATE),
+        # state.show_lowstate exits with 0 for non-ssh as well
+        (("state.show_lowstate",), 0),
+        (("state.top", "top.sls"), EX_AGGREGATE),
+    ),
+)
+def test_it(salt_ssh_cli, args, retcode):
+    ret = salt_ssh_cli.run(*args)
+
+    assert ret.returncode == retcode
+    assert isinstance(ret.data, list)
+    assert ret.data
+    assert isinstance(ret.data[0], str)
+    # While these three should usually follow each other, there
+    # can be warnings in between that would break such a logic.
+    assert "Rendering SLS 'base:fail_module_exception' failed:" in ret.data[0]
+    assert "Problem running salt function in Jinja template:" in ret.data[0]
+    assert (
+        "Error running 'disk.usage': Invalid flag passed to disk.usage" in ret.data[0]
+    )
diff --git a/tests/pytests/integration/ssh/state/test_retcode_state_run_remote_exception.py b/tests/pytests/integration/ssh/state/test_retcode_state_run_remote_exception.py
new file mode 100644
index 000000000000..60621a2ffe0d
--- /dev/null
+++ b/tests/pytests/integration/ssh/state/test_retcode_state_run_remote_exception.py
@@ -0,0 +1,91 @@
+"""
+Verify salt-ssh does not report success when it cannot parse
+the return value.
+"""
+
+import pytest
+
+from salt.defaults.exitcodes import EX_AGGREGATE
+
+pytestmark = [
+    pytest.mark.skip_on_windows(reason="salt-ssh not available on Windows"),
+    pytest.mark.slow_test,
+]
+
+
+@pytest.fixture(scope="module")
+def state_tree_remote_exception_mod(salt_run_cli, base_env_state_tree_root_dir):
+    module_contents = r"""
+import os
+import sys
+
+
+def __virtual__():
+    return "whoops"
+
+
+def do_stuff(name):
+    print("Hi there, nice that you're trying to make me do stuff.", file=sys.stderr)
+    print("Traceback (most recent call last):", file=sys.stderr)
+    print('  File "/dont/treat/me/like/that.py" in buzz_off', file=sys.stderr)
+    print("ComplianceError: 'Outlaw detected'", file=sys.stderr)
+    sys.stderr.flush()
+    os._exit(1)
+"""
+    module_dir = base_env_state_tree_root_dir / "_states"
+    module_tempfile = pytest.helpers.temp_file("whoops.py", module_contents, module_dir)
+    try:
+        with module_tempfile:
+            ret = salt_run_cli.run("saltutil.sync_states")
+            assert ret.returncode == 0
+            assert "states.whoops" in ret.data
+            yield
+    finally:
+        ret = salt_run_cli.run("saltutil.sync_states")
+        assert ret.returncode == 0
+
+
+@pytest.fixture(scope="module", autouse=True)
+def state_tree_remote_exception(
+    base_env_state_tree_root_dir, state_tree_remote_exception_mod
+):
+    top_file = """
+    base:
+      'localhost':
+        - remote_stacktrace
+      '127.0.0.1':
+        - remote_stacktrace
+    """
+    state_file = r"""
+    This should be detected as failure:
+      whoops.do_stuff
+    """
+    top_tempfile = pytest.helpers.temp_file(
+        "top.sls", top_file, base_env_state_tree_root_dir
+    )
+    state_tempfile = pytest.helpers.temp_file(
+        "remote_stacktrace.sls", state_file, base_env_state_tree_root_dir
+    )
+    with top_tempfile, state_tempfile:
+        yield
+
+
+@pytest.mark.slow_test
+@pytest.mark.usefixtures("state_tree_remote_exception")
+@pytest.mark.parametrize(
+    "args",
+    (
+        ("state.sls", "remote_stacktrace"),
+        ("state.highstate",),
+        ("state.sls_id", "This should be detected as failure", "remote_stacktrace"),
+        ("state.top", "top.sls"),
+        ("state.single", "whoops.do_stuff", "now"),
+    ),
+)
+def test_it(salt_ssh_cli, args):
+    ret = salt_ssh_cli.run(*args)
+
+    assert ret.returncode == EX_AGGREGATE
+    assert ret.data
+    assert isinstance(ret.data, str)
+    assert "ComplianceError: 'Outlaw detected'" in ret.data
diff --git a/tests/pytests/integration/ssh/test_deploy.py b/tests/pytests/integration/ssh/test_deploy.py
index c95bb40a3110..6e55bc252a0f 100644
--- a/tests/pytests/integration/ssh/test_deploy.py
+++ b/tests/pytests/integration/ssh/test_deploy.py
@@ -28,6 +28,119 @@ def thin_dir(salt_ssh_cli):
         shutil.rmtree(thin_dir_path, ignore_errors=True)
 
 
+@pytest.fixture(scope="module")
+def invalid_json_exe_mod(salt_run_cli, base_env_state_tree_root_dir):
+    module_contents = r"""
+import os
+import sys
+
+
+def __virtual__():
+    return "whoops"
+
+
+def test():
+    data = '{\n  "local": {\n    "whoops": "hrhrhr"\n  }\n}'
+    ctr = 0
+    for line in data.splitlines():
+        sys.stdout.write(line)
+        if ctr == 3:
+            print("Warning: Chaos is not a letter")
+        ctr += 1
+    sys.stdout.flush()
+    os._exit(0)
+"""
+    module_dir = base_env_state_tree_root_dir / "_modules"
+    module_tempfile = pytest.helpers.temp_file("whoops.py", module_contents, module_dir)
+    try:
+        with module_tempfile:
+            ret = salt_run_cli.run("saltutil.sync_modules")
+            assert ret.returncode == 0
+            assert "modules.whoops" in ret.data
+            yield
+    finally:
+        ret = salt_run_cli.run("saltutil.sync_modules")
+        assert ret.returncode == 0
+
+
+@pytest.fixture(scope="module")
+def invalid_return_exe_mod(salt_run_cli, base_env_state_tree_root_dir):
+    module_contents = r"""
+import json
+import os
+import sys
+
+
+def __virtual__():
+    return "whoopsiedoodle"
+
+
+def test(wrapped=True):
+    data = "Chaos is a ladder though"
+    if wrapped:
+        data = {"local": {"no_return_key_present": data}}
+    else:
+        data = {"no_local_key_present": data}
+
+    print(json.dumps(data))
+    sys.stdout.flush()
+    os._exit(0)
+"""
+    module_dir = base_env_state_tree_root_dir / "_modules"
+    module_tempfile = pytest.helpers.temp_file(
+        "whoopsiedoodle.py", module_contents, module_dir
+    )
+    try:
+        with module_tempfile:
+            ret = salt_run_cli.run("saltutil.sync_modules")
+            assert ret.returncode == 0
+            assert "modules.whoopsiedoodle" in ret.data
+            yield
+    finally:
+        ret = salt_run_cli.run("saltutil.sync_modules")
+        assert ret.returncode == 0
+
+
+@pytest.fixture(scope="module")
+def remote_exception_wrap_mod(salt_master):
+    module_contents = r"""
+def __virtual__():
+    return "check_exception"
+
+
+def failure():
+    # This should raise an exception
+    ret = __salt__["disk.usage"]("c")
+    return f"Probably got garbage: {ret}"
+"""
+    module_dir = pathlib.Path(salt_master.config["extension_modules"]) / "wrapper"
+    module_tempfile = pytest.helpers.temp_file(
+        "check_exception.py", module_contents, module_dir
+    )
+    with module_tempfile:
+        yield
+
+
+@pytest.fixture(scope="module")
+def remote_parsing_failure_wrap_mod(salt_master, invalid_json_exe_mod):
+    module_contents = r"""
+def __virtual__():
+    return "check_parsing"
+
+
+def failure(mod):
+    # This should raise an exception
+    ret = __salt__[f"{mod}.test"]()
+    return f"Probably got garbage: {ret}"
+"""
+    module_dir = pathlib.Path(salt_master.config["extension_modules"]) / "wrapper"
+    module_tempfile = pytest.helpers.temp_file(
+        "check_parsing.py", module_contents, module_dir
+    )
+    with module_tempfile:
+        yield
+
+
 def test_ping(salt_ssh_cli):
     """
     Test a simple ping
@@ -122,3 +235,65 @@ def test_retcode_exe_run_exception(salt_ssh_cli):
     assert isinstance(ret.data, dict)
     assert ret.data["stderr"].endswith("Exception: hehehe")
     assert ret.data["retcode"] == 1
+
+
+@pytest.mark.usefixtures("invalid_json_exe_mod")
+def test_retcode_json_decode_error(salt_ssh_cli):
+    """
+    Verify salt-ssh exits with a non-zero exit code when
+    it cannot decode the output of a command.
+    """
+    ret = salt_ssh_cli.run("whoops.test")
+    assert ret.returncode == EX_AGGREGATE
+    assert isinstance(ret.data, dict)
+    assert (
+        ret.data["stdout"]
+        == '{  "local": {    "whoops": "hrhrhr"  }Warning: Chaos is not a letter\n}'
+    )
+    assert ret.data["retcode"] == 0
+
+
+@pytest.mark.usefixtures("invalid_return_exe_mod")
+def test_retcode_invalid_return(salt_ssh_cli):
+    """
+    Verify salt-ssh exits with a non-zero exit code when
+    the decoded command output is invalid.
+    """
+    ret = salt_ssh_cli.run("whoopsiedoodle.test", "false")
+    assert ret.returncode == EX_AGGREGATE
+    assert isinstance(ret.data, dict)
+    assert ret.data["stdout"] == '{"no_local_key_present": "Chaos is a ladder though"}'
+    assert ret.data["retcode"] == 0
+
+
+@pytest.mark.usefixtures("remote_exception_wrap_mod")
+def test_wrapper_unwrapped_command_exception(salt_ssh_cli):
+    """
+    Verify salt-ssh does not return unexpected exception output to wrapper modules.
+    """
+    ret = salt_ssh_cli.run("check_exception.failure")
+    assert ret.data
+    assert "Probably got garbage" not in ret.data
+    assert ret.returncode == EX_AGGREGATE
+
+
+@pytest.mark.usefixtures("remote_parsing_failure_wrap_mod", "invalid_json_exe_mod")
+def test_wrapper_unwrapped_command_parsing_failure(salt_ssh_cli):
+    """
+    Verify salt-ssh does not return unexpected unparsable output to wrapper modules.
+    """
+    ret = salt_ssh_cli.run("check_parsing.failure", "whoops")
+    assert ret.data
+    assert "Probably got garbage" not in ret.data
+    assert ret.returncode == EX_AGGREGATE
+
+
+@pytest.mark.usefixtures("remote_parsing_failure_wrap_mod", "invalid_return_exe_mod")
+def test_wrapper_unwrapped_command_invalid_return(salt_ssh_cli):
+    """
+    Verify salt-ssh does not return unexpected unparsable output to wrapper modules.
+    """
+    ret = salt_ssh_cli.run("check_parsing.failure", "whoopsiedoodle")
+    assert ret.data
+    assert "Probably got garbage" not in ret.data
+    assert ret.returncode == EX_AGGREGATE
diff --git a/tests/pytests/integration/ssh/test_mine.py b/tests/pytests/integration/ssh/test_mine.py
index a39760b7652c..77a0572dbbbd 100644
--- a/tests/pytests/integration/ssh/test_mine.py
+++ b/tests/pytests/integration/ssh/test_mine.py
@@ -8,6 +8,31 @@
 ]
 
 
+@pytest.fixture(scope="module", autouse=True)
+def pillar_tree(base_env_pillar_tree_root_dir):
+    top_file = """
+    base:
+      'localhost':
+        - mine
+      '127.0.0.1':
+        - mine
+    """
+    mine_pillar_file = """
+    mine_functions:
+      disk.usage:
+        - c
+    """
+    top_tempfile = pytest.helpers.temp_file(
+        "top.sls", top_file, base_env_pillar_tree_root_dir
+    )
+    mine_tempfile = pytest.helpers.temp_file(
+        "mine.sls", mine_pillar_file, base_env_pillar_tree_root_dir
+    )
+
+    with top_tempfile, mine_tempfile:
+        yield
+
+
 @pytest.fixture(autouse=True)
 def thin_dir(salt_ssh_cli):
     try:
@@ -54,3 +79,13 @@ def test_mine_get(salt_ssh_cli, salt_minion, tgts):
     assert set(ret.data) == exp
     for id_ in exp:
         assert ret.data[id_] is True
+
+
+def test_ssh_mine_get_error(salt_ssh_cli):
+    """
+    Test that a mine function returning an error is not
+    included in the output.
+    """
+    ret = salt_ssh_cli.run("mine.get", "localhost", "disk.usage")
+    assert ret.returncode == 0
+    assert not ret.data