From 7f13acd57f279d8cd5ccad15f3c904dc3ebe0a81 Mon Sep 17 00:00:00 2001 From: jeanluc Date: Wed, 21 Jun 2023 13:40:20 +0200 Subject: [PATCH 1/4] Add tests for salt-ssh `state.*` exitcodes --- tests/pytests/integration/ssh/test_state.py | 341 ++++++++++++++++++++ 1 file changed, 341 insertions(+) diff --git a/tests/pytests/integration/ssh/test_state.py b/tests/pytests/integration/ssh/test_state.py index 9d3a38d2c9f5..5f9bfb45e9f4 100644 --- a/tests/pytests/integration/ssh/test_state.py +++ b/tests/pytests/integration/ssh/test_state.py @@ -2,6 +2,8 @@ import pytest +from salt.defaults.exitcodes import EX_AGGREGATE + pytestmark = [ pytest.mark.skip_on_windows(reason="salt-ssh not available on Windows"), ] @@ -75,6 +77,129 @@ def state_tree_dir(base_env_state_tree_root_dir): yield +@pytest.fixture(scope="class") +def state_tree_render_fail(base_env_state_tree_root_dir): + top_file = """ + base: + 'localhost': + - fail_render + '127.0.0.1': + - fail_render + """ + state_file = r""" + abc var is not defined {{ abc }}: + test.nop + """ + top_tempfile = pytest.helpers.temp_file( + "top.sls", top_file, base_env_state_tree_root_dir + ) + state_tempfile = pytest.helpers.temp_file( + "fail_render.sls", state_file, base_env_state_tree_root_dir + ) + with top_tempfile, state_tempfile: + yield + + +@pytest.fixture(scope="class") +def state_tree_req_fail(base_env_state_tree_root_dir): + top_file = """ + base: + 'localhost': + - fail_req + '127.0.0.1': + - fail_req + """ + state_file = """ + This has an invalid requisite: + test.nop: + - name: foo + - require_in: + - file.managed: invalid_requisite + """ + top_tempfile = pytest.helpers.temp_file( + "top.sls", top_file, base_env_state_tree_root_dir + ) + state_tempfile = pytest.helpers.temp_file( + "fail_req.sls", state_file, base_env_state_tree_root_dir + ) + with top_tempfile, state_tempfile: + yield + + +@pytest.fixture(scope="class") +def state_tree_structure_fail(base_env_state_tree_root_dir): + top_file = """ + base: + 'localhost': + - fail_structure + '127.0.0.1': + - fail_structure + """ + state_file = """ + extend: + Some file state: + file: + - name: /tmp/bar + - contents: bar + """ + top_tempfile = pytest.helpers.temp_file( + "top.sls", top_file, base_env_state_tree_root_dir + ) + state_tempfile = pytest.helpers.temp_file( + "fail_structure.sls", state_file, base_env_state_tree_root_dir + ) + with top_tempfile, state_tempfile: + yield + + +@pytest.fixture(scope="class") +def state_tree_run_fail(base_env_state_tree_root_dir): + top_file = """ + base: + 'localhost': + - fail_run + '127.0.0.1': + - fail_run + """ + state_file = """ + This file state fails: + file.managed: + - name: /tmp/non/ex/is/tent + - makedirs: false + - contents: foo + """ + top_tempfile = pytest.helpers.temp_file( + "top.sls", top_file, base_env_state_tree_root_dir + ) + state_tempfile = pytest.helpers.temp_file( + "fail_run.sls", state_file, base_env_state_tree_root_dir + ) + with top_tempfile, state_tempfile: + yield + + +@pytest.fixture(scope="class") +def pillar_tree_render_fail(base_env_pillar_tree_root_dir): + top_file = """ + base: + 'localhost': + - fail_render + '127.0.0.1': + - fail_render + """ + pillar_file = r""" + not_defined: {{ abc }} + """ + top_tempfile = pytest.helpers.temp_file( + "top.sls", top_file, base_env_pillar_tree_root_dir + ) + pillar_tempfile = pytest.helpers.temp_file( + "fail_render.sls", pillar_file, base_env_pillar_tree_root_dir + ) + with top_tempfile, pillar_tempfile: + yield + + @pytest.mark.slow_test def test_state_with_import(salt_ssh_cli, state_tree): """ @@ -220,3 +345,219 @@ def test_state_high(salt_ssh_cli): ]["stdout"] == "blah" ) + + +@pytest.mark.slow_test +@pytest.mark.usefixtures("state_tree_render_fail") +class TestRenderExceptionRetcode: + """ + Verify salt-ssh fails with a retcode > 0 when a state rendering fails. + """ + + def test_retcode_state_sls_render_exception(self, salt_ssh_cli): + ret = salt_ssh_cli.run("state.sls", "fail_render") + self._assert_ret(ret, EX_AGGREGATE) + + def test_retcode_state_highstate_render_exception(self, salt_ssh_cli): + ret = salt_ssh_cli.run("state.highstate") + self._assert_ret(ret, EX_AGGREGATE) + + def test_retcode_state_sls_id_render_exception(self, salt_ssh_cli): + ret = salt_ssh_cli.run("state.sls_id", "foo", "fail_render") + self._assert_ret(ret, EX_AGGREGATE) + + def test_retcode_state_show_sls_render_exception(self, salt_ssh_cli): + ret = salt_ssh_cli.run("state.show_sls", "fail_render") + self._assert_ret(ret, EX_AGGREGATE) + + def test_retcode_state_show_low_sls_render_exception(self, salt_ssh_cli): + ret = salt_ssh_cli.run("state.show_low_sls", "fail_render") + self._assert_ret(ret, EX_AGGREGATE) + + def test_retcode_state_show_highstate_render_exception(self, salt_ssh_cli): + ret = salt_ssh_cli.run("state.show_highstate") + self._assert_ret(ret, EX_AGGREGATE) + + def test_retcode_state_show_lowstate_render_exception(self, salt_ssh_cli): + ret = salt_ssh_cli.run("state.show_lowstate") + # state.show_lowstate exits with 0 for non-ssh as well + self._assert_ret(ret, 0) + + def test_retcode_state_top_render_exception(self, salt_ssh_cli): + ret = salt_ssh_cli.run("state.top", "top.sls") + self._assert_ret(ret, EX_AGGREGATE) + + def test_retcode_state_single_render_exception(self, salt_ssh_cli): + ret = salt_ssh_cli.run("state.single", "file") + assert ret.returncode == EX_AGGREGATE + assert isinstance(ret.data, str) + assert "single() missing 1 required positional argument" in ret.data + + def _assert_ret(self, ret, retcode): + assert ret.returncode == retcode + assert isinstance(ret.data, list) + assert ret.data + assert isinstance(ret.data[0], str) + assert ret.data[0].startswith( + "Rendering SLS 'base:fail_render' failed: Jinja variable 'abc' is undefined;" + ) + + +@pytest.mark.slow_test +@pytest.mark.usefixtures("pillar_tree_render_fail") +class TestPillarRenderExceptionRetcode: + """ + Verify salt-ssh fails with a retcode > 0 when a pillar rendering fails. + """ + + def test_retcode_state_sls_pillar_render_exception(self, salt_ssh_cli): + ret = salt_ssh_cli.run("state.sls", "basic") + self._assert_ret(ret) + + def test_retcode_state_highstate_pillar_render_exception(self, salt_ssh_cli): + ret = salt_ssh_cli.run("state.highstate") + self._assert_ret(ret) + + def test_retcode_state_sls_id_pillar_render_exception(self, salt_ssh_cli): + ret = salt_ssh_cli.run("state.sls_id", "foo", "basic") + self._assert_ret(ret) + + def test_retcode_state_show_sls_pillar_render_exception(self, salt_ssh_cli): + ret = salt_ssh_cli.run("state.show_sls", "basic") + self._assert_ret(ret) + + def test_retcode_state_show_low_sls_pillar_render_exception(self, salt_ssh_cli): + ret = salt_ssh_cli.run("state.show_low_sls", "basic") + self._assert_ret(ret) + + def test_retcode_state_show_highstate_pillar_render_exception(self, salt_ssh_cli): + ret = salt_ssh_cli.run("state.show_highstate") + self._assert_ret(ret) + + def test_retcode_state_show_lowstate_pillar_render_exception(self, salt_ssh_cli): + ret = salt_ssh_cli.run("state.show_lowstate") + self._assert_ret(ret) + + def test_retcode_state_top_pillar_render_exception(self, salt_ssh_cli): + ret = salt_ssh_cli.run("state.top", "top.sls") + self._assert_ret(ret) + + def _assert_ret(self, ret): + assert ret.returncode == EX_AGGREGATE + assert isinstance(ret.data, list) + assert ret.data + assert isinstance(ret.data[0], str) + assert ret.data[0] == "Pillar failed to render with the following messages:" + assert ret.data[1].startswith("Rendering SLS 'fail_render' failed.") + + +@pytest.mark.slow_test +@pytest.mark.usefixtures("state_tree_req_fail") +class TestStateReqFailRetcode: + """ + Verify salt-ssh fails with a retcode > 0 when a highstate verification fails. + ``state.show_highstate`` does not validate this. + """ + + def test_retcode_state_sls_invalid_requisite(self, salt_ssh_cli): + ret = salt_ssh_cli.run("state.sls", "fail_req") + self._assert_ret(ret, EX_AGGREGATE) + + def test_retcode_state_highstate_invalid_requisite(self, salt_ssh_cli): + ret = salt_ssh_cli.run("state.highstate") + self._assert_ret(ret, EX_AGGREGATE) + + def test_retcode_state_show_sls_invalid_requisite(self, salt_ssh_cli): + ret = salt_ssh_cli.run("state.show_sls", "fail_req") + self._assert_ret(ret, EX_AGGREGATE) + + def test_retcode_state_show_low_sls_invalid_requisite(self, salt_ssh_cli): + ret = salt_ssh_cli.run("state.show_low_sls", "fail_req") + self._assert_ret(ret, EX_AGGREGATE) + + def test_retcode_state_show_lowstate_invalid_requisite(self, salt_ssh_cli): + ret = salt_ssh_cli.run("state.show_lowstate") + # state.show_lowstate exits with 0 for non-ssh as well + self._assert_ret(ret, 0) + + def test_retcode_state_top_invalid_requisite(self, salt_ssh_cli): + ret = salt_ssh_cli.run("state.top", "top.sls") + self._assert_ret(ret, EX_AGGREGATE) + + def _assert_ret(self, ret, retcode): + assert ret.returncode == retcode + assert isinstance(ret.data, list) + assert ret.data + assert isinstance(ret.data[0], str) + assert ret.data[0].startswith( + "Invalid requisite in require: file.managed for invalid_requisite" + ) + + +@pytest.mark.slow_test +@pytest.mark.usefixtures("state_tree_structure_fail") +class TestStateStructureFailRetcode: + """ + Verify salt-ssh fails with a retcode > 0 when a highstate verification fails. + This targets another step of the verification. + ``state.sls_id`` does not seem to support extends. + ``state.show_highstate`` does not validate this. + """ + + def test_retcode_state_sls_invalid_structure(self, salt_ssh_cli): + ret = salt_ssh_cli.run("state.sls", "fail_structure") + self._assert_ret(ret, EX_AGGREGATE) + + def test_retcode_state_highstate_invalid_structure(self, salt_ssh_cli): + ret = salt_ssh_cli.run("state.highstate") + self._assert_ret(ret, EX_AGGREGATE) + + def test_retcode_state_show_sls_invalid_structure(self, salt_ssh_cli): + ret = salt_ssh_cli.run("state.show_sls", "fail_structure") + self._assert_ret(ret, EX_AGGREGATE) + + def test_retcode_state_show_low_sls_invalid_structure(self, salt_ssh_cli): + ret = salt_ssh_cli.run("state.show_low_sls", "fail_structure") + self._assert_ret(ret, EX_AGGREGATE) + + def test_retcode_state_show_lowstate_invalid_structure(self, salt_ssh_cli): + ret = salt_ssh_cli.run("state.show_lowstate") + # state.show_lowstate exits with 0 for non-ssh as well + self._assert_ret(ret, 0) + + def test_retcode_state_top_invalid_structure(self, salt_ssh_cli): + ret = salt_ssh_cli.run("state.top", "top.sls") + self._assert_ret(ret, EX_AGGREGATE) + + def _assert_ret(self, ret, retcode): + assert ret.returncode == retcode + assert isinstance(ret.data, list) + assert ret.data + assert isinstance(ret.data[0], str) + assert ret.data[0].startswith( + "Cannot extend ID 'Some file state' in 'base:fail_structure" + ) + + +@pytest.mark.slow_test +@pytest.mark.usefixtures("state_tree_run_fail") +class TestStateRunFailRetcode: + """ + Verify salt-ssh passes on a failing retcode from state execution. + """ + + def test_retcode_state_sls_run_fail(self, salt_ssh_cli): + ret = salt_ssh_cli.run("state.sls", "fail_run") + assert ret.returncode == EX_AGGREGATE + + def test_retcode_state_highstate_run_fail(self, salt_ssh_cli): + ret = salt_ssh_cli.run("state.highstate") + assert ret.returncode == EX_AGGREGATE + + def test_retcode_state_sls_id_render_exception(self, salt_ssh_cli): + ret = salt_ssh_cli.run("state.sls_id", "This file state fails", "fail_run") + assert ret.returncode == EX_AGGREGATE + + def test_retcode_state_top_run_fail(self, salt_ssh_cli): + ret = salt_ssh_cli.run("state.top", "top.sls") + assert ret.returncode == EX_AGGREGATE From 1154b1725b921a299b4c7652f397f8d76fd4c46c Mon Sep 17 00:00:00 2001 From: jeanluc Date: Wed, 21 Jun 2023 15:38:32 +0200 Subject: [PATCH 2/4] Fix salt-ssh state.* commands retcode for render fail --- changelog/64514.fixed.md | 1 + salt/client/ssh/__init__.py | 40 ++++++++++-------- salt/client/ssh/wrapper/state.py | 70 ++++++++++++++++++++++++++++---- 3 files changed, 84 insertions(+), 27 deletions(-) create mode 100644 changelog/64514.fixed.md diff --git a/changelog/64514.fixed.md b/changelog/64514.fixed.md new file mode 100644 index 000000000000..b84fb366bf14 --- /dev/null +++ b/changelog/64514.fixed.md @@ -0,0 +1 @@ +Fixed salt-ssh state.* commands returning retcode 0 when state/pillar rendering fails diff --git a/salt/client/ssh/__init__.py b/salt/client/ssh/__init__.py index 88365a60994d..4a1e785e6b66 100644 --- a/salt/client/ssh/__init__.py +++ b/salt/client/ssh/__init__.py @@ -552,6 +552,11 @@ def handle_routine(self, que, opts, host, target, mine=False): 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, @@ -564,7 +569,7 @@ def handle_routine(self, que, opts, host, target, mine=False): "stderr": stderr, "retcode": retcode, } - que.put(ret) + que.put((ret, retcode)) def handle_ssh(self, mine=False): """ @@ -608,7 +613,7 @@ def handle_ssh(self, mine=False): "fun": "", "id": host, } - yield {host: no_ret} + yield {host: no_ret}, 1 continue args = ( que, @@ -622,11 +627,12 @@ def handle_ssh(self, mine=False): running[host] = {"thread": routine} continue ret = {} + retcode = 0 try: - ret = que.get(False) + ret, retcode = que.get(False) if "id" in ret: returned.add(ret["id"]) - yield {ret["id"]: ret["ret"]} + yield {ret["id"]: ret["ret"]}, retcode except queue.Empty: pass for host in running: @@ -636,10 +642,10 @@ def handle_ssh(self, mine=False): # last checked try: while True: - ret = que.get(False) + ret, retcode = que.get(False) if "id" in ret: returned.add(ret["id"]) - yield {ret["id"]: ret["ret"]} + yield {ret["id"]: ret["ret"]}, retcode except queue.Empty: pass @@ -650,7 +656,7 @@ def handle_ssh(self, mine=False): ) ret = {"id": host, "ret": error} log.error(error) - yield {ret["id"]: ret["ret"]} + yield {ret["id"]: ret["ret"]}, 1 running[host]["thread"].join() rets.add(host) for host in rets: @@ -705,8 +711,8 @@ def run_iter(self, mine=False, jid=None): jid, job_load ) - for ret in self.handle_ssh(mine=mine): - host = next(iter(ret.keys())) + for ret, _ in self.handle_ssh(mine=mine): + host = next(iter(ret)) self.cache_job(jid, host, ret[host], fun) if self.event: id_, data = next(iter(ret.items())) @@ -799,15 +805,9 @@ def run(self, jid=None): sret = {} outputter = self.opts.get("output", "nested") final_exit = 0 - for ret in self.handle_ssh(): - host = next(iter(ret.keys())) - if isinstance(ret[host], dict): - host_ret = ret[host].get("retcode", 0) - if host_ret != 0: - final_exit = 1 - else: - # Error on host - final_exit = 1 + for ret, retcode in self.handle_ssh(): + host = next(iter(ret)) + final_exit = max(final_exit, retcode) self.cache_job(jid, host, ret[host], fun) ret = self.key_deploy(host, ret) @@ -1274,6 +1274,10 @@ def run_wfunc(self): ) log.error(result, exc_info_on_loglevel=logging.DEBUG) retcode = 1 + + # Ensure retcode from wrappers is respected, especially state render exceptions + retcode = max(retcode, self.context.get("retcode", 0)) + # Mimic the json data-structure that "salt-call --local" will # emit (as seen in ssh_py_shim.py) if isinstance(result, dict) and "local" in result: diff --git a/salt/client/ssh/wrapper/state.py b/salt/client/ssh/wrapper/state.py index 002853972ab6..0a1d5bdf5f9b 100644 --- a/salt/client/ssh/wrapper/state.py +++ b/salt/client/ssh/wrapper/state.py @@ -8,6 +8,7 @@ import salt.client.ssh.shell import salt.client.ssh.state +import salt.defaults.exitcodes import salt.loader import salt.minion import salt.roster @@ -84,14 +85,14 @@ def _set_retcode(ret, highstate=None): """ # Set default retcode to 0 - __context__["retcode"] = 0 + __context__["retcode"] = salt.defaults.exitcodes.EX_OK if isinstance(ret, list): - __context__["retcode"] = 1 + __context__["retcode"] = salt.defaults.exitcodes.EX_STATE_COMPILER_ERROR return if not salt.utils.state.check_result(ret, highstate=highstate): - __context__["retcode"] = 2 + __context__["retcode"] = salt.defaults.exitcodes.EX_STATE_FAILURE def _check_pillar(kwargs, pillar=None): @@ -182,6 +183,11 @@ def sls(mods, saltenv="base", test=None, exclude=None, **kwargs): __context__["fileclient"], context=__context__.value(), ) as st_: + if not _check_pillar(kwargs, st_.opts["pillar"]): + __context__["retcode"] = salt.defaults.exitcodes.EX_PILLAR_FAILURE + err = ["Pillar failed to render with the following messages:"] + err += st_.opts["pillar"]["_errors"] + return err st_.push_active() mods = _parse_mods(mods) high_data, errors = st_.render_highstate( @@ -198,12 +204,14 @@ def sls(mods, saltenv="base", test=None, exclude=None, **kwargs): errors += ext_errors errors += st_.state.verify_high(high_data) if errors: + __context__["retcode"] = salt.defaults.exitcodes.EX_STATE_COMPILER_ERROR return errors high_data, req_in_errors = st_.state.requisite_in(high_data) errors += req_in_errors high_data = st_.state.apply_exclude(high_data) # Verify that the high data is structurally sound if errors: + __context__["retcode"] = salt.defaults.exitcodes.EX_STATE_COMPILER_ERROR return errors # Compile and verify the raw chunks chunks = st_.state.compile_high_data(high_data) @@ -316,7 +324,7 @@ def _check_queue(queue, kwargs): else: conflict = running(concurrent=kwargs.get("concurrent", False)) if conflict: - __context__["retcode"] = 1 + __context__["retcode"] = salt.defaults.exitcodes.EX_STATE_COMPILER_ERROR return conflict @@ -681,6 +689,11 @@ def highstate(test=None, **kwargs): __context__["fileclient"], context=__context__.value(), ) as st_: + if not _check_pillar(kwargs, st_.opts["pillar"]): + __context__["retcode"] = salt.defaults.exitcodes.EX_PILLAR_FAILURE + err = ["Pillar failed to render with the following messages:"] + err += st_.opts["pillar"]["_errors"] + return err st_.push_active() chunks = st_.compile_low_chunks(context=__context__.value()) file_refs = salt.client.ssh.state.lowstate_file_refs( @@ -692,7 +705,7 @@ def highstate(test=None, **kwargs): # Check for errors for chunk in chunks: if not isinstance(chunk, dict): - __context__["retcode"] = 1 + __context__["retcode"] = salt.defaults.exitcodes.EX_STATE_COMPILER_ERROR return chunks roster = salt.roster.Roster(opts, opts.get("roster", "flat")) @@ -766,9 +779,19 @@ def top(topfn, test=None, **kwargs): __context__["fileclient"], context=__context__.value(), ) as st_: + if not _check_pillar(kwargs, st_.opts["pillar"]): + __context__["retcode"] = salt.defaults.exitcodes.EX_PILLAR_FAILURE + err = ["Pillar failed to render with the following messages:"] + err += st_.opts["pillar"]["_errors"] + return err st_.opts["state_top"] = os.path.join("salt://", topfn) st_.push_active() chunks = st_.compile_low_chunks(context=__context__.value()) + # Check for errors + for chunk in chunks: + if not isinstance(chunk, dict): + __context__["retcode"] = salt.defaults.exitcodes.EX_STATE_COMPILER_ERROR + return chunks file_refs = salt.client.ssh.state.lowstate_file_refs( chunks, _merge_extra_filerefs( @@ -839,8 +862,17 @@ def show_highstate(**kwargs): __context__["fileclient"], context=__context__.value(), ) as st_: + if not _check_pillar(kwargs, st_.opts["pillar"]): + __context__["retcode"] = salt.defaults.exitcodes.EX_PILLAR_FAILURE + err = ["Pillar failed to render with the following messages:"] + err += st_.opts["pillar"]["_errors"] + return err st_.push_active() chunks = st_.compile_highstate(context=__context__.value()) + # Check for errors + if not isinstance(chunks, dict): + __context__["retcode"] = salt.defaults.exitcodes.EX_STATE_COMPILER_ERROR + return chunks _cleanup_slsmod_high_data(chunks) return chunks @@ -864,6 +896,11 @@ def show_lowstate(**kwargs): __context__["fileclient"], context=__context__.value(), ) as st_: + if not _check_pillar(kwargs, st_.opts["pillar"]): + __context__["retcode"] = salt.defaults.exitcodes.EX_PILLAR_FAILURE + err = ["Pillar failed to render with the following messages:"] + err += st_.opts["pillar"]["_errors"] + return err st_.push_active() chunks = st_.compile_low_chunks(context=__context__.value()) _cleanup_slsmod_low_data(chunks) @@ -925,7 +962,7 @@ def sls_id(id_, mods, test=None, queue=False, **kwargs): ) as st_: if not _check_pillar(kwargs, st_.opts["pillar"]): - __context__["retcode"] = 5 + __context__["retcode"] = salt.defaults.exitcodes.EX_PILLAR_FAILURE err = ["Pillar failed to render with the following messages:"] err += __pillar__["_errors"] return err @@ -943,7 +980,7 @@ def sls_id(id_, mods, test=None, queue=False, **kwargs): # but it is required to get the unit tests to pass. errors.extend(req_in_errors) if errors: - __context__["retcode"] = 1 + __context__["retcode"] = salt.defaults.exitcodes.EX_STATE_COMPILER_ERROR return errors chunks = st_.state.compile_high_data(high_) chunk = [x for x in chunks if x.get("__id__", "") == id_] @@ -988,6 +1025,11 @@ def show_sls(mods, saltenv="base", test=None, **kwargs): __context__["fileclient"], context=__context__.value(), ) as st_: + if not _check_pillar(kwargs, st_.opts["pillar"]): + __context__["retcode"] = salt.defaults.exitcodes.EX_PILLAR_FAILURE + err = ["Pillar failed to render with the following messages:"] + err += st_.opts["pillar"]["_errors"] + return err st_.push_active() mods = _parse_mods(mods) high_data, errors = st_.render_highstate( @@ -997,12 +1039,14 @@ def show_sls(mods, saltenv="base", test=None, **kwargs): errors += ext_errors errors += st_.state.verify_high(high_data) if errors: + __context__["retcode"] = salt.defaults.exitcodes.EX_STATE_COMPILER_ERROR return errors high_data, req_in_errors = st_.state.requisite_in(high_data) errors += req_in_errors high_data = st_.state.apply_exclude(high_data) # Verify that the high data is structurally sound if errors: + __context__["retcode"] = salt.defaults.exitcodes.EX_STATE_COMPILER_ERROR return errors _cleanup_slsmod_high_data(high_data) return high_data @@ -1036,6 +1080,11 @@ def show_low_sls(mods, saltenv="base", test=None, **kwargs): __context__["fileclient"], context=__context__.value(), ) as st_: + if not _check_pillar(kwargs, st_.opts["pillar"]): + __context__["retcode"] = salt.defaults.exitcodes.EX_PILLAR_FAILURE + err = ["Pillar failed to render with the following messages:"] + err += st_.opts["pillar"]["_errors"] + return err st_.push_active() mods = _parse_mods(mods) high_data, errors = st_.render_highstate( @@ -1045,12 +1094,14 @@ def show_low_sls(mods, saltenv="base", test=None, **kwargs): errors += ext_errors errors += st_.state.verify_high(high_data) if errors: + __context__["retcode"] = salt.defaults.exitcodes.EX_STATE_COMPILER_ERROR return errors high_data, req_in_errors = st_.state.requisite_in(high_data) errors += req_in_errors high_data = st_.state.apply_exclude(high_data) # Verify that the high data is structurally sound if errors: + __context__["retcode"] = salt.defaults.exitcodes.EX_STATE_COMPILER_ERROR return errors ret = st_.state.compile_high_data(high_data) _cleanup_slsmod_low_data(ret) @@ -1080,6 +1131,7 @@ def show_top(**kwargs): errors = [] errors += st_.verify_tops(top_data) if errors: + __context__["retcode"] = salt.defaults.exitcodes.EX_STATE_COMPILER_ERROR return errors matches = st_.top_matches(top_data) return matches @@ -1110,7 +1162,7 @@ def single(fun, name, test=None, **kwargs): # state.fun -> [state, fun] comps = fun.split(".") if len(comps) < 2: - __context__["retcode"] = 1 + __context__["retcode"] = salt.defaults.exitcodes.EX_STATE_COMPILER_ERROR return "Invalid function passed" # Create the low chunk, using kwargs as a base @@ -1133,7 +1185,7 @@ def single(fun, name, test=None, **kwargs): # Verify the low chunk err = st_.verify_data(kwargs) if err: - __context__["retcode"] = 1 + __context__["retcode"] = salt.defaults.exitcodes.EX_STATE_COMPILER_ERROR return err # Must be a list of low-chunks From 199c284826fad8e602d51f23d253781c6d22cee1 Mon Sep 17 00:00:00 2001 From: jeanluc Date: Wed, 21 Jun 2023 15:40:54 +0200 Subject: [PATCH 3/4] Add misc tests for retcode passthrough --- tests/pytests/integration/ssh/test_deploy.py | 24 ++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/pytests/integration/ssh/test_deploy.py b/tests/pytests/integration/ssh/test_deploy.py index dac28f84f373..d70f8fce23be 100644 --- a/tests/pytests/integration/ssh/test_deploy.py +++ b/tests/pytests/integration/ssh/test_deploy.py @@ -9,6 +9,7 @@ import salt.utils.files import salt.utils.yaml +from salt.defaults.exitcodes import EX_AGGREGATE pytestmark = [ pytest.mark.slow_test, @@ -100,3 +101,26 @@ def test_tty(salt_ssh_cli, tmp_path, salt_ssh_roster_file): ret = salt_ssh_cli.run("--roster-file={}".format(roster_file), "test.ping") assert ret.returncode == 0 assert ret.data is True + + +def test_retcode_exe_run_fail(salt_ssh_cli): + """ + Verify salt-ssh passes through the retcode it receives. + """ + ret = salt_ssh_cli.run("file.touch", "/tmp/non/ex/is/tent") + assert ret.returncode == EX_AGGREGATE + assert isinstance(ret.data, dict) + assert ret.data["stderr"] == "Error running 'file.touch': No such file or directory" + assert ret.data["retcode"] == 1 + + +def test_retcode_exe_run_exception(salt_ssh_cli): + """ + Verify salt-ssh passes through the retcode it receives + when an exception is thrown. (Ref #50727) + """ + ret = salt_ssh_cli.run("salttest.jinja_error") + assert ret.returncode == EX_AGGREGATE + assert isinstance(ret.data, dict) + assert ret.data["stderr"].endswith("Exception: hehehe") + assert ret.data["retcode"] == 1 From 7ee2340be3831f2add6980c3e2e0bf7813f7b15e Mon Sep 17 00:00:00 2001 From: jeanluc Date: Wed, 21 Jun 2023 15:41:52 +0200 Subject: [PATCH 4/4] run pre-commit upgrade code for Py3.8+ --- salt/client/ssh/__init__.py | 50 +++++++++----------- salt/client/ssh/wrapper/state.py | 18 +++---- tests/pytests/integration/ssh/test_deploy.py | 8 ++-- 3 files changed, 34 insertions(+), 42 deletions(-) diff --git a/salt/client/ssh/__init__.py b/salt/client/ssh/__init__.py index 4a1e785e6b66..0d9ac9509b64 100644 --- a/salt/client/ssh/__init__.py +++ b/salt/client/ssh/__init__.py @@ -304,7 +304,7 @@ def __init__(self, opts): } if self.opts.get("rand_thin_dir"): self.defaults["thin_dir"] = os.path.join( - "/var/tmp", ".{}".format(uuid.uuid4().hex[:6]) + "/var/tmp", f".{uuid.uuid4().hex[:6]}" ) self.opts["ssh_wipe"] = "True" self.returners = salt.loader.returners(self.opts, {}) @@ -454,9 +454,9 @@ def get_pubkey(self): priv = self.opts.get( "ssh_priv", os.path.join(self.opts["pki_dir"], "ssh", "salt-ssh.rsa") ) - pub = "{}.pub".format(priv) + pub = f"{priv}.pub" with salt.utils.files.fopen(pub, "r") as fp_: - return "{} rsa root@master".format(fp_.read().split()[1]) + return f"{fp_.read().split()[1]} rsa root@master" def key_deploy(self, host, ret): """ @@ -500,7 +500,7 @@ def _key_deploy_run(self, host, target, re_run=True): mods=self.mods, fsclient=self.fsclient, thin=self.thin, - **target + **target, ) if salt.utils.path.which("ssh-copy-id"): # we have ssh-copy-id, use it! @@ -516,7 +516,7 @@ def _key_deploy_run(self, host, target, re_run=True): mods=self.mods, fsclient=self.fsclient, thin=self.thin, - **target + **target, ) stdout, stderr, retcode = single.cmd_block() try: @@ -543,7 +543,7 @@ def handle_routine(self, que, opts, host, target, mine=False): fsclient=self.fsclient, thin=self.thin, mine=mine, - **target + **target, ) ret = {"id": single.id} stdout, stderr, retcode = single.run() @@ -798,7 +798,7 @@ def run(self, jid=None): ) if self.opts.get("verbose"): - msg = "Executing job with jid {}".format(jid) + msg = f"Executing job with jid {jid}" print(msg) print("-" * len(msg) + "\n") print("") @@ -883,7 +883,7 @@ def __init__( remote_port_forwards=None, winrm=False, ssh_options=None, - **kwargs + **kwargs, ): # Get mine setting and mine_functions if defined in kwargs (from roster) self.mine = mine @@ -1017,9 +1017,7 @@ def check_thin_dir(self): """ check if the thindir exists on the remote machine """ - stdout, stderr, retcode = self.shell.exec_cmd( - "test -d {}".format(self.thin_dir) - ) + stdout, stderr, retcode = self.shell.exec_cmd(f"test -d {self.thin_dir}") if retcode != 0: return False return True @@ -1131,7 +1129,7 @@ def run_wfunc(self): self.id, fsclient=self.fsclient, minion_opts=self.minion_opts, - **self.target + **self.target, ) opts_pkg = pre_wrapper["test.opts_pkg"]() # pylint: disable=E1102 @@ -1217,7 +1215,7 @@ def run_wfunc(self): self.id, fsclient=self.fsclient, minion_opts=self.minion_opts, - **self.target + **self.target, ) wrapper.fsclient.opts["cachedir"] = opts["cachedir"] self.wfuncs = salt.loader.ssh_wrapper(opts, wrapper, self.context) @@ -1265,7 +1263,7 @@ def run_wfunc(self): else: result = self.wfuncs[self.fun](*self.args, **self.kwargs) except TypeError as exc: - result = "TypeError encountered executing {}: {}".format(self.fun, exc) + result = 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 @@ -1292,7 +1290,7 @@ def _cmd_str(self): """ if self.target.get("sudo"): sudo = ( - "sudo -p '{}'".format(salt.client.ssh.shell.SUDO_PROMPT) + f"sudo -p '{salt.client.ssh.shell.SUDO_PROMPT}'" if self.target.get("passwd") else "sudo" ) @@ -1364,20 +1362,18 @@ def execute_script(self, script, extension="py", pre_dir="", script_args=None): script_args = shlex.split(str(script_args)) args = " {}".format(" ".join([shlex.quote(str(el)) for el in script_args])) if extension == "ps1": - ret = self.shell.exec_cmd('"powershell {}"'.format(script)) + ret = self.shell.exec_cmd(f'"powershell {script}"') else: if not self.winrm: - ret = self.shell.exec_cmd( - "/bin/sh '{}{}'{}".format(pre_dir, script, args) - ) + ret = self.shell.exec_cmd(f"/bin/sh '{pre_dir}{script}'{args}") else: ret = saltwinshell.call_python(self, script) # Remove file from target system if not self.winrm: - self.shell.exec_cmd("rm '{}{}'".format(pre_dir, script)) + self.shell.exec_cmd(f"rm '{pre_dir}{script}'") else: - self.shell.exec_cmd("del {}".format(script)) + self.shell.exec_cmd(f"del {script}") return ret @@ -1465,7 +1461,7 @@ def cmd_block(self, is_retry=False): while re.search(RSTR_RE, stderr): stderr = re.split(RSTR_RE, stderr, 1)[1].strip() else: - return "ERROR: {}".format(error), stderr, retcode + return f"ERROR: {error}", stderr, retcode # FIXME: this discards output from ssh_shim if the shim succeeds. It should # always save the shim output regardless of shim success or failure. @@ -1525,7 +1521,7 @@ def cmd_block(self, is_retry=False): # If RSTR is not seen in both stdout and stderr then there # was a thin deployment problem. return ( - "ERROR: Failure deploying ext_mods: {}".format(stdout), + f"ERROR: Failure deploying ext_mods: {stdout}", stderr, retcode, ) @@ -1693,7 +1689,7 @@ def mod_data(fsclient): files = fsclient.file_list(env) for ref in sync_refs: mods_data = {} - pref = "_{}".format(ref) + pref = f"_{ref}" for fn_ in sorted(files): if fn_.startswith(pref): if fn_.endswith((".py", ".so", ".pyx")): @@ -1715,9 +1711,7 @@ def mod_data(fsclient): ver_base = salt.utils.stringutils.to_bytes(ver_base) ver = hashlib.sha1(ver_base).hexdigest() - ext_tar_path = os.path.join( - fsclient.opts["cachedir"], "ext_mods.{}.tgz".format(ver) - ) + ext_tar_path = os.path.join(fsclient.opts["cachedir"], f"ext_mods.{ver}.tgz") mods = {"version": ver, "file": ext_tar_path} if os.path.isfile(ext_tar_path): return mods @@ -1766,7 +1760,7 @@ def _convert_args(args): for key in list(arg.keys()): if key == "__kwarg__": continue - converted.append("{}={}".format(key, arg[key])) + converted.append(f"{key}={arg[key]}") else: converted.append(arg) return converted diff --git a/salt/client/ssh/wrapper/state.py b/salt/client/ssh/wrapper/state.py index 0a1d5bdf5f9b..353d8a0e03eb 100644 --- a/salt/client/ssh/wrapper/state.py +++ b/salt/client/ssh/wrapper/state.py @@ -55,7 +55,7 @@ def _ssh_state(chunks, st_kwargs, kwargs, test=False): cmd, fsclient=__context__["fileclient"], minion_opts=__salt__.minion_opts, - **st_kwargs + **st_kwargs, ) single.shell.send(trans_tar, "{}/salt_state.tgz".format(__opts__["thin_dir"])) stdout, stderr, _ = single.cmd_block() @@ -244,7 +244,7 @@ def sls(mods, saltenv="base", test=None, exclude=None, **kwargs): cmd, fsclient=__context__["fileclient"], minion_opts=__salt__.minion_opts, - **st_kwargs + **st_kwargs, ) single.shell.send(trans_tar, "{}/salt_state.tgz".format(opts["thin_dir"])) stdout, stderr, _ = single.cmd_block() @@ -392,7 +392,7 @@ def low(data, **kwargs): cmd, fsclient=__context__["fileclient"], minion_opts=__salt__.minion_opts, - **st_kwargs + **st_kwargs, ) single.shell.send(trans_tar, "{}/salt_state.tgz".format(__opts__["thin_dir"])) stdout, stderr, _ = single.cmd_block() @@ -482,7 +482,7 @@ def high(data, **kwargs): cmd, fsclient=__context__["fileclient"], minion_opts=__salt__.minion_opts, - **st_kwargs + **st_kwargs, ) single.shell.send(trans_tar, "{}/salt_state.tgz".format(opts["thin_dir"])) stdout, stderr, _ = single.cmd_block() @@ -558,7 +558,7 @@ def request(mods=None, **kwargs): try: if salt.utils.platform.is_windows(): # Make sure cache file isn't read-only - __salt__["cmd.run"]('attrib -R "{}"'.format(notify_path)) + __salt__["cmd.run"](f'attrib -R "{notify_path}"') with salt.utils.files.fopen(notify_path, "w+b") as fp_: salt.payload.dump(req, fp_) except OSError: @@ -622,7 +622,7 @@ def clear_request(name=None): try: if salt.utils.platform.is_windows(): # Make sure cache file isn't read-only - __salt__["cmd.run"]('attrib -R "{}"'.format(notify_path)) + __salt__["cmd.run"](f'attrib -R "{notify_path}"') with salt.utils.files.fopen(notify_path, "w+b") as fp_: salt.payload.dump(req, fp_) except OSError: @@ -730,7 +730,7 @@ def highstate(test=None, **kwargs): cmd, fsclient=__context__["fileclient"], minion_opts=__salt__.minion_opts, - **st_kwargs + **st_kwargs, ) single.shell.send(trans_tar, "{}/salt_state.tgz".format(opts["thin_dir"])) stdout, stderr, _ = single.cmd_block() @@ -821,7 +821,7 @@ def top(topfn, test=None, **kwargs): cmd, fsclient=__context__["fileclient"], minion_opts=__salt__.minion_opts, - **st_kwargs + **st_kwargs, ) single.shell.send(trans_tar, "{}/salt_state.tgz".format(opts["thin_dir"])) stdout, stderr, _ = single.cmd_block() @@ -1227,7 +1227,7 @@ def single(fun, name, test=None, **kwargs): cmd, fsclient=__context__["fileclient"], minion_opts=__salt__.minion_opts, - **st_kwargs + **st_kwargs, ) # Copy the tar down diff --git a/tests/pytests/integration/ssh/test_deploy.py b/tests/pytests/integration/ssh/test_deploy.py index d70f8fce23be..8512813f6e60 100644 --- a/tests/pytests/integration/ssh/test_deploy.py +++ b/tests/pytests/integration/ssh/test_deploy.py @@ -75,15 +75,13 @@ def test_set_path(salt_ssh_cli, tmp_path, salt_ssh_roster_file): roster_data = salt.utils.yaml.safe_load(rfh) roster_data["localhost"].update( { - "set_path": "$PATH:/usr/local/bin/:{}".format(path), + "set_path": f"$PATH:/usr/local/bin/:{path}", } ) with salt.utils.files.fopen(roster_file, "w") as wfh: salt.utils.yaml.safe_dump(roster_data, wfh) - ret = salt_ssh_cli.run( - "--roster-file={}".format(roster_file), "environ.get", "PATH" - ) + ret = salt_ssh_cli.run(f"--roster-file={roster_file}", "environ.get", "PATH") assert ret.returncode == 0 assert path in ret.data @@ -98,7 +96,7 @@ def test_tty(salt_ssh_cli, tmp_path, salt_ssh_roster_file): roster_data["localhost"].update({"tty": True}) with salt.utils.files.fopen(roster_file, "w") as wfh: salt.utils.yaml.safe_dump(roster_data, wfh) - ret = salt_ssh_cli.run("--roster-file={}".format(roster_file), "test.ping") + ret = salt_ssh_cli.run(f"--roster-file={roster_file}", "test.ping") assert ret.returncode == 0 assert ret.data is True