From 3c06882dc73a036e936eda3f181fd99b3cbdf361 Mon Sep 17 00:00:00 2001 From: Marek Czernek Date: Thu, 18 Apr 2024 10:24:40 +0200 Subject: [PATCH 01/13] Allow explicit reboot on transactional minions --- salt/modules/transactional_update.py | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/salt/modules/transactional_update.py b/salt/modules/transactional_update.py index d6915475f5..4f108e53b9 100644 --- a/salt/modules/transactional_update.py +++ b/salt/modules/transactional_update.py @@ -985,13 +985,30 @@ def call(function, *args, **kwargs): else: return local except ValueError: - return {"result": False, "retcode": 1, "comment": ret_stdout} + local = {"result": False, "retcode": 1, "comment": ret_stdout} + return local finally: # Check if reboot is needed - if activate_transaction and pending_transaction(): + if (activate_transaction and pending_transaction()) or _user_specified_reboot( + local + ): reboot() +def _user_specified_reboot(local): + if not isinstance(local, dict): + # Skip if execution is not state/highstate + return False + + explicit_reboot_cmds = ["reboot", "init 6", "shutdown -r", "shutdown --reboot"] + names = [] + for _, value in local.items(): + if isinstance(value, dict) and "name" in value: + names.append(value["name"]) + # Partial match reboot_cmds to names, so that e.g. "reboot" matches "system.reboot" + return any([cmd in name for cmd in explicit_reboot_cmds for name in names]) + + def apply_(mods=None, **kwargs): """Apply an state inside a transaction. From c0e081e933259f1aa4e39f91d4eada35e53fec2d Mon Sep 17 00:00:00 2001 From: Marek Czernek Date: Thu, 18 Apr 2024 13:52:22 +0200 Subject: [PATCH 02/13] Implement feedback in t_u module --- salt/modules/transactional_update.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/salt/modules/transactional_update.py b/salt/modules/transactional_update.py index 4f108e53b9..07c19d8d9c 100644 --- a/salt/modules/transactional_update.py +++ b/salt/modules/transactional_update.py @@ -989,8 +989,8 @@ def call(function, *args, **kwargs): return local finally: # Check if reboot is needed - if (activate_transaction and pending_transaction()) or _user_specified_reboot( - local + if (activate_transaction and pending_transaction()) or ( + not kwargs.get("test", False) and _user_specified_reboot(local) ): reboot() @@ -1000,13 +1000,13 @@ def _user_specified_reboot(local): # Skip if execution is not state/highstate return False - explicit_reboot_cmds = ["reboot", "init 6", "shutdown -r", "shutdown --reboot"] - names = [] + explicit_reboot_cmds = set(["reboot", "system.reboot"]) + names = set() for _, value in local.items(): if isinstance(value, dict) and "name" in value: - names.append(value["name"]) - # Partial match reboot_cmds to names, so that e.g. "reboot" matches "system.reboot" - return any([cmd in name for cmd in explicit_reboot_cmds for name in names]) + names.add(value["name"]) + + return bool(explicit_reboot_cmds.intersection(names)) def apply_(mods=None, **kwargs): From d4642efeef61ca68caeaf0586cb1e3243e8a5fc7 Mon Sep 17 00:00:00 2001 From: Marek Czernek Date: Thu, 18 Apr 2024 13:52:32 +0200 Subject: [PATCH 03/13] Add tests --- .../unit/modules/test_transactional_update.py | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/tests/pytests/unit/modules/test_transactional_update.py b/tests/pytests/unit/modules/test_transactional_update.py index dbd72fd74b..bfd137563f 100644 --- a/tests/pytests/unit/modules/test_transactional_update.py +++ b/tests/pytests/unit/modules/test_transactional_update.py @@ -501,6 +501,57 @@ def test_call_success_parameters(): ) +def test_call_success_explicit_reboot(): + """Test transactional_update.call executes reboot when user specifies 'reboot' in sls""" + return_json = {'local': { 'cmd_|-reboot_test_|-reboot_|-run': {'name': 'reboot', 'changes': {}, 'result': True}}} + utils_mock = { + "json.find_json": MagicMock(return_value=return_json), + } + salt_mock = { + "cmd.run_all": MagicMock(return_value={"retcode": 0, "stdout": ""}), + } + reboot_mock = MagicMock() + with patch.dict(tu.__utils__, utils_mock), patch.dict(tu.__salt__, salt_mock), patch( + "salt.modules.transactional_update.reboot", reboot_mock + ): + tu.call("module.function", key="value") + reboot_mock.assert_called_once() + + +def test_call_success_explicit_reboot_test(): + """Test transactional_update.call does NOT execute reboot when user specifies 'reboot' in sls in test mode""" + return_json = {'local': { 'cmd_|-reboot_test_|-reboot_|-run': {'name': 'reboot', 'changes': {}, 'result': True}}} + utils_mock = { + "json.find_json": MagicMock(return_value=return_json), + } + salt_mock = { + "cmd.run_all": MagicMock(return_value={"retcode": 0, "stdout": ""}), + } + reboot_mock = MagicMock() + with patch.dict(tu.__utils__, utils_mock), patch.dict(tu.__salt__, salt_mock), patch( + "salt.modules.transactional_update.reboot", reboot_mock + ): + tu.call("module.function", test="True") + assert not reboot_mock.called + + +def test_call_fail_explicit_reboot(): + """Test transactional_update.call does NOT execute reboot when the word 'reboot' appears in sls""" + return_json = {'local': { 'cmd_|-reboot_test_|-reboot_|-run': {'name': 'service reboot', 'changes': {}, 'result': True}}} + utils_mock = { + "json.find_json": MagicMock(return_value=return_json), + } + salt_mock = { + "cmd.run_all": MagicMock(return_value={"retcode": 0, "stdout": ""}), + } + reboot_mock = MagicMock() + with patch.dict(tu.__utils__, utils_mock), patch.dict(tu.__salt__, salt_mock), patch( + "salt.modules.transactional_update.reboot", reboot_mock + ): + tu.call("module.function", test="True") + assert not reboot_mock.called + + def test_sls(): """Test transactional_update.sls""" salt_mock = { From 89609c83082562b48ff91ee721a913a59351e733 Mon Sep 17 00:00:00 2001 From: Marek Czernek Date: Thu, 18 Apr 2024 13:52:51 +0200 Subject: [PATCH 04/13] Apply Black to test_transactional_update.py --- .../unit/modules/test_transactional_update.py | 56 ++++++++++++++----- 1 file changed, 41 insertions(+), 15 deletions(-) diff --git a/tests/pytests/unit/modules/test_transactional_update.py b/tests/pytests/unit/modules/test_transactional_update.py index bfd137563f..1783f2628c 100644 --- a/tests/pytests/unit/modules/test_transactional_update.py +++ b/tests/pytests/unit/modules/test_transactional_update.py @@ -178,9 +178,11 @@ def test_commands_with_global_params(): "--non-interactive", "--drop-if-no-change", "--no-selfupdate", - cmd.replace("_", ".") - if cmd.startswith("grub") - else cmd.replace("_", "-"), + ( + cmd.replace("_", ".") + if cmd.startswith("grub") + else cmd.replace("_", "-") + ), ] ) @@ -503,7 +505,15 @@ def test_call_success_parameters(): def test_call_success_explicit_reboot(): """Test transactional_update.call executes reboot when user specifies 'reboot' in sls""" - return_json = {'local': { 'cmd_|-reboot_test_|-reboot_|-run': {'name': 'reboot', 'changes': {}, 'result': True}}} + return_json = { + "local": { + "cmd_|-reboot_test_|-reboot_|-run": { + "name": "reboot", + "changes": {}, + "result": True, + } + } + } utils_mock = { "json.find_json": MagicMock(return_value=return_json), } @@ -511,16 +521,24 @@ def test_call_success_explicit_reboot(): "cmd.run_all": MagicMock(return_value={"retcode": 0, "stdout": ""}), } reboot_mock = MagicMock() - with patch.dict(tu.__utils__, utils_mock), patch.dict(tu.__salt__, salt_mock), patch( - "salt.modules.transactional_update.reboot", reboot_mock - ): + with patch.dict(tu.__utils__, utils_mock), patch.dict( + tu.__salt__, salt_mock + ), patch("salt.modules.transactional_update.reboot", reboot_mock): tu.call("module.function", key="value") reboot_mock.assert_called_once() def test_call_success_explicit_reboot_test(): """Test transactional_update.call does NOT execute reboot when user specifies 'reboot' in sls in test mode""" - return_json = {'local': { 'cmd_|-reboot_test_|-reboot_|-run': {'name': 'reboot', 'changes': {}, 'result': True}}} + return_json = { + "local": { + "cmd_|-reboot_test_|-reboot_|-run": { + "name": "reboot", + "changes": {}, + "result": True, + } + } + } utils_mock = { "json.find_json": MagicMock(return_value=return_json), } @@ -528,16 +546,24 @@ def test_call_success_explicit_reboot_test(): "cmd.run_all": MagicMock(return_value={"retcode": 0, "stdout": ""}), } reboot_mock = MagicMock() - with patch.dict(tu.__utils__, utils_mock), patch.dict(tu.__salt__, salt_mock), patch( - "salt.modules.transactional_update.reboot", reboot_mock - ): + with patch.dict(tu.__utils__, utils_mock), patch.dict( + tu.__salt__, salt_mock + ), patch("salt.modules.transactional_update.reboot", reboot_mock): tu.call("module.function", test="True") assert not reboot_mock.called def test_call_fail_explicit_reboot(): """Test transactional_update.call does NOT execute reboot when the word 'reboot' appears in sls""" - return_json = {'local': { 'cmd_|-reboot_test_|-reboot_|-run': {'name': 'service reboot', 'changes': {}, 'result': True}}} + return_json = { + "local": { + "cmd_|-reboot_test_|-reboot_|-run": { + "name": "service reboot", + "changes": {}, + "result": True, + } + } + } utils_mock = { "json.find_json": MagicMock(return_value=return_json), } @@ -545,9 +571,9 @@ def test_call_fail_explicit_reboot(): "cmd.run_all": MagicMock(return_value={"retcode": 0, "stdout": ""}), } reboot_mock = MagicMock() - with patch.dict(tu.__utils__, utils_mock), patch.dict(tu.__salt__, salt_mock), patch( - "salt.modules.transactional_update.reboot", reboot_mock - ): + with patch.dict(tu.__utils__, utils_mock), patch.dict( + tu.__salt__, salt_mock + ), patch("salt.modules.transactional_update.reboot", reboot_mock): tu.call("module.function", test="True") assert not reboot_mock.called From 7a7f7a1be091adcd7f42b7934632456694723bb0 Mon Sep 17 00:00:00 2001 From: Marek Czernek Date: Thu, 18 Apr 2024 14:54:45 +0200 Subject: [PATCH 05/13] Execute function only in sls --- salt/modules/transactional_update.py | 8 +++++--- tests/pytests/unit/modules/test_transactional_update.py | 6 +++--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/salt/modules/transactional_update.py b/salt/modules/transactional_update.py index 07c19d8d9c..20cafda46c 100644 --- a/salt/modules/transactional_update.py +++ b/salt/modules/transactional_update.py @@ -990,14 +990,16 @@ def call(function, *args, **kwargs): finally: # Check if reboot is needed if (activate_transaction and pending_transaction()) or ( - not kwargs.get("test", False) and _user_specified_reboot(local) + not kwargs.get("test", False) and _user_specified_reboot(local, function) ): reboot() -def _user_specified_reboot(local): +def _user_specified_reboot(local, function): + if function != "state.highstate" and function != "state.sls": + return False + if not isinstance(local, dict): - # Skip if execution is not state/highstate return False explicit_reboot_cmds = set(["reboot", "system.reboot"]) diff --git a/tests/pytests/unit/modules/test_transactional_update.py b/tests/pytests/unit/modules/test_transactional_update.py index 1783f2628c..8e5b9b996b 100644 --- a/tests/pytests/unit/modules/test_transactional_update.py +++ b/tests/pytests/unit/modules/test_transactional_update.py @@ -524,7 +524,7 @@ def test_call_success_explicit_reboot(): with patch.dict(tu.__utils__, utils_mock), patch.dict( tu.__salt__, salt_mock ), patch("salt.modules.transactional_update.reboot", reboot_mock): - tu.call("module.function", key="value") + tu.call("state.sls", key="value") reboot_mock.assert_called_once() @@ -549,7 +549,7 @@ def test_call_success_explicit_reboot_test(): with patch.dict(tu.__utils__, utils_mock), patch.dict( tu.__salt__, salt_mock ), patch("salt.modules.transactional_update.reboot", reboot_mock): - tu.call("module.function", test="True") + tu.call("state.sls", test="True") assert not reboot_mock.called @@ -574,7 +574,7 @@ def test_call_fail_explicit_reboot(): with patch.dict(tu.__utils__, utils_mock), patch.dict( tu.__salt__, salt_mock ), patch("salt.modules.transactional_update.reboot", reboot_mock): - tu.call("module.function", test="True") + tu.call("state.sls", test="True") assert not reboot_mock.called From a63e2a3693b8e09799d0b26fe08e050ad911593d Mon Sep 17 00:00:00 2001 From: Marek Czernek Date: Thu, 18 Apr 2024 15:17:51 +0200 Subject: [PATCH 06/13] Add test for non-sls functions --- .../unit/modules/test_transactional_update.py | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tests/pytests/unit/modules/test_transactional_update.py b/tests/pytests/unit/modules/test_transactional_update.py index 8e5b9b996b..b6d4ede9f2 100644 --- a/tests/pytests/unit/modules/test_transactional_update.py +++ b/tests/pytests/unit/modules/test_transactional_update.py @@ -578,6 +578,34 @@ def test_call_fail_explicit_reboot(): assert not reboot_mock.called +def test_call_fail_explicit_reboot_non_sls(): + """ + Test transactional_update.call does NOT execute reboot when the word 'reboot' + appears in sls executed with function other than state.sls + """ + return_json = { + "local": { + "cmd_|-reboot_test_|-reboot_|-run": { + "name": "reboot", + "changes": {}, + "result": True, + } + } + } + utils_mock = { + "json.find_json": MagicMock(return_value=return_json), + } + salt_mock = { + "cmd.run_all": MagicMock(return_value={"retcode": 0, "stdout": ""}), + } + reboot_mock = MagicMock() + with patch.dict(tu.__utils__, utils_mock), patch.dict( + tu.__salt__, salt_mock + ), patch("salt.modules.transactional_update.reboot", reboot_mock): + tu.call("state.test") + assert not reboot_mock.called + + def test_sls(): """Test transactional_update.sls""" salt_mock = { From 7bb6f29ab8abd4ef03af37171341e1d828f54d68 Mon Sep 17 00:00:00 2001 From: Marek Czernek Date: Thu, 18 Apr 2024 15:20:20 +0200 Subject: [PATCH 07/13] Rename test --- tests/pytests/unit/modules/test_transactional_update.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/pytests/unit/modules/test_transactional_update.py b/tests/pytests/unit/modules/test_transactional_update.py index b6d4ede9f2..0bd63fb472 100644 --- a/tests/pytests/unit/modules/test_transactional_update.py +++ b/tests/pytests/unit/modules/test_transactional_update.py @@ -528,7 +528,7 @@ def test_call_success_explicit_reboot(): reboot_mock.assert_called_once() -def test_call_success_explicit_reboot_test(): +def test_call_fail_explicit_reboot_test(): """Test transactional_update.call does NOT execute reboot when user specifies 'reboot' in sls in test mode""" return_json = { "local": { From cf97edd4387dfe76127fb35c90f85711099c8067 Mon Sep 17 00:00:00 2001 From: Marek Czernek Date: Mon, 22 Apr 2024 13:13:40 +0200 Subject: [PATCH 08/13] Ensure that only cmd or module get parsed --- salt/modules/transactional_update.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/salt/modules/transactional_update.py b/salt/modules/transactional_update.py index 20cafda46c..836ab4447e 100644 --- a/salt/modules/transactional_update.py +++ b/salt/modules/transactional_update.py @@ -1003,8 +1003,16 @@ def _user_specified_reboot(local, function): return False explicit_reboot_cmds = set(["reboot", "system.reboot"]) + explicit_reboot_modules = ["cmd_", "module_"] names = set() - for _, value in local.items(): + for key, value in local.items(): + if not isinstance(key, str): + continue + + module = key.split("|")[0] + if module not in explicit_reboot_modules: + continue + if isinstance(value, dict) and "name" in value: names.add(value["name"]) From cf48e168f6284550e09d27f97a71891bfe39092b Mon Sep 17 00:00:00 2001 From: Marek Czernek Date: Mon, 22 Apr 2024 13:16:51 +0200 Subject: [PATCH 09/13] Add test --- .../unit/modules/test_transactional_update.py | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tests/pytests/unit/modules/test_transactional_update.py b/tests/pytests/unit/modules/test_transactional_update.py index 0bd63fb472..c7cd65c27b 100644 --- a/tests/pytests/unit/modules/test_transactional_update.py +++ b/tests/pytests/unit/modules/test_transactional_update.py @@ -606,6 +606,34 @@ def test_call_fail_explicit_reboot_non_sls(): assert not reboot_mock.called +def test_call_fail_explicit_reboot_non_cmd(): + """ + Test transactional_update.call does NOT execute reboot when the word 'reboot' + appears in sls executed with module other than `cmd` or `module.include` + """ + return_json = { + "local": { + "test_|-reboot_test_|-reboot_|-run": { + "name": "reboot", + "changes": {}, + "result": True, + } + } + } + utils_mock = { + "json.find_json": MagicMock(return_value=return_json), + } + salt_mock = { + "cmd.run_all": MagicMock(return_value={"retcode": 0, "stdout": ""}), + } + reboot_mock = MagicMock() + with patch.dict(tu.__utils__, utils_mock), patch.dict( + tu.__salt__, salt_mock + ), patch("salt.modules.transactional_update.reboot", reboot_mock): + tu.call("state.test") + assert not reboot_mock.called + + def test_sls(): """Test transactional_update.sls""" salt_mock = { From 43acc0f62fc6761e123761eb89b798a8bc754d74 Mon Sep 17 00:00:00 2001 From: Marek Czernek Date: Mon, 22 Apr 2024 13:23:44 +0200 Subject: [PATCH 10/13] Rename vars for better readability --- salt/modules/transactional_update.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/salt/modules/transactional_update.py b/salt/modules/transactional_update.py index 836ab4447e..f5fdc7d978 100644 --- a/salt/modules/transactional_update.py +++ b/salt/modules/transactional_update.py @@ -1005,16 +1005,16 @@ def _user_specified_reboot(local, function): explicit_reboot_cmds = set(["reboot", "system.reboot"]) explicit_reboot_modules = ["cmd_", "module_"] names = set() - for key, value in local.items(): - if not isinstance(key, str): + for full_module_name, module_result in local.items(): + if not isinstance(full_module_name, str): continue - module = key.split("|")[0] + module = full_module_name.split("|")[0] if module not in explicit_reboot_modules: continue - if isinstance(value, dict) and "name" in value: - names.add(value["name"]) + if isinstance(module_result, dict) and "name" in module_result: + names.add(module_result["name"]) return bool(explicit_reboot_cmds.intersection(names)) From 8ec03a2c2b22a56f266c08d1bb7a808ac2811dae Mon Sep 17 00:00:00 2001 From: Marek Czernek Date: Tue, 23 Apr 2024 09:23:54 +0200 Subject: [PATCH 11/13] Fix test function --- tests/pytests/unit/modules/test_transactional_update.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/pytests/unit/modules/test_transactional_update.py b/tests/pytests/unit/modules/test_transactional_update.py index c7cd65c27b..402841d8f2 100644 --- a/tests/pytests/unit/modules/test_transactional_update.py +++ b/tests/pytests/unit/modules/test_transactional_update.py @@ -630,7 +630,7 @@ def test_call_fail_explicit_reboot_non_cmd(): with patch.dict(tu.__utils__, utils_mock), patch.dict( tu.__salt__, salt_mock ), patch("salt.modules.transactional_update.reboot", reboot_mock): - tu.call("state.test") + tu.call("state.sls") assert not reboot_mock.called From 86d6235df59936c038df608030a715901897b85b Mon Sep 17 00:00:00 2001 From: Marek Czernek Date: Wed, 24 Apr 2024 11:49:03 +0200 Subject: [PATCH 12/13] Fix separator and naming --- salt/modules/transactional_update.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/salt/modules/transactional_update.py b/salt/modules/transactional_update.py index f5fdc7d978..c03dff78e0 100644 --- a/salt/modules/transactional_update.py +++ b/salt/modules/transactional_update.py @@ -1003,18 +1003,18 @@ def _user_specified_reboot(local, function): return False explicit_reboot_cmds = set(["reboot", "system.reboot"]) - explicit_reboot_modules = ["cmd_", "module_"] + explicit_reboot_modules = ["cmd", "module"] names = set() - for full_module_name, module_result in local.items(): - if not isinstance(full_module_name, str): + for execution_id, execution_result in local.items(): + if not isinstance(execution_id, str): continue - module = full_module_name.split("|")[0] + module = execution_id.split("_|-")[0] if module not in explicit_reboot_modules: continue - if isinstance(module_result, dict) and "name" in module_result: - names.add(module_result["name"]) + if isinstance(execution_result, dict) and "name" in execution_result: + names.add(execution_result["name"]) return bool(explicit_reboot_cmds.intersection(names)) From 862216d23346ea647c38a6766be42a35c5a6a9ee Mon Sep 17 00:00:00 2001 From: Marek Czernek Date: Wed, 24 Apr 2024 13:38:29 +0200 Subject: [PATCH 13/13] Add all functions from t-u executor --- salt/modules/transactional_update.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/salt/modules/transactional_update.py b/salt/modules/transactional_update.py index c03dff78e0..9f2dea7344 100644 --- a/salt/modules/transactional_update.py +++ b/salt/modules/transactional_update.py @@ -283,6 +283,7 @@ import salt.client.ssh.state import salt.client.ssh.wrapper.state import salt.exceptions +import salt.executors.transactional_update import salt.utils.args from salt.modules.state import _check_queue, _prior_running_states, _wait, running @@ -996,14 +997,15 @@ def call(function, *args, **kwargs): def _user_specified_reboot(local, function): - if function != "state.highstate" and function != "state.sls": + explicit_reboot_cmds = set(["reboot", "system.reboot"]) + explicit_reboot_modules = ["cmd", "module"] + + if function not in salt.executors.transactional_update.DELEGATION_MAP.keys(): return False if not isinstance(local, dict): return False - explicit_reboot_cmds = set(["reboot", "system.reboot"]) - explicit_reboot_modules = ["cmd", "module"] names = set() for execution_id, execution_result in local.items(): if not isinstance(execution_id, str):