From 333e12ffbb8247d1bceee608a90f4f5f6fea04a9 Mon Sep 17 00:00:00 2001 From: "Gary T. Giesen" Date: Sat, 4 Jul 2026 16:25:42 -0400 Subject: [PATCH 1/2] Honour test=True in keystone_role_grant present/absent The keystone_role_grant.present and keystone_role_grant.absent states performed no __opts__['test'] handling, so a state run with test=True still called keystoneng.role_grant / role_revoke and made real changes. absent() in particular would destructively revoke a live role assignment during a test run. Both functions now short-circuit when test mode is active, returning result=None with the predicted changes and a "would be granted/revoked" comment instead of calling the execution module. Fixes #52220 --- changelog/52220.fixed.md | 1 + salt/states/keystone_role_grant.py | 14 ++++ .../unit/states/test_keystone_role_grant.py | 66 +++++++++++++++++++ 3 files changed, 81 insertions(+) create mode 100644 changelog/52220.fixed.md create mode 100644 tests/pytests/unit/states/test_keystone_role_grant.py diff --git a/changelog/52220.fixed.md b/changelog/52220.fixed.md new file mode 100644 index 000000000000..c5892932d8b7 --- /dev/null +++ b/changelog/52220.fixed.md @@ -0,0 +1 @@ +Fixed keystone_role_grant.present and keystone_role_grant.absent to honour test=True so role assignments are no longer granted or revoked in test mode diff --git a/salt/states/keystone_role_grant.py b/salt/states/keystone_role_grant.py index d908cf93af5a..b2203ab330e1 100644 --- a/salt/states/keystone_role_grant.py +++ b/salt/states/keystone_role_grant.py @@ -109,6 +109,13 @@ def present(name, auth=None, **kwargs): grants = __salt__["keystoneng.role_assignment_list"](filters=filters) if not grants: + if __opts__["test"] is True: + ret["result"] = None + for k, v in filters.items(): + ret["changes"][k] = v + ret["comment"] = "Role assignment would be granted" + return ret + __salt__["keystoneng.role_grant"](**kwargs) for k, v in filters.items(): ret["changes"][k] = v @@ -129,6 +136,13 @@ def absent(name, auth=None, **kwargs): grants = __salt__["keystoneng.role_assignment_list"](filters=filters) if grants: + if __opts__["test"] is True: + ret["result"] = None + for k, v in filters.items(): + ret["changes"][k] = v + ret["comment"] = "Role assignment would be revoked" + return ret + __salt__["keystoneng.role_revoke"](**kwargs) for k, v in filters.items(): ret["changes"][k] = v diff --git a/tests/pytests/unit/states/test_keystone_role_grant.py b/tests/pytests/unit/states/test_keystone_role_grant.py new file mode 100644 index 000000000000..1e10e0ba7aa3 --- /dev/null +++ b/tests/pytests/unit/states/test_keystone_role_grant.py @@ -0,0 +1,66 @@ +""" +Test cases for salt.states.keystone_role_grant +""" + +import pytest + +import salt.states.keystone_role_grant as keystone_role_grant +from tests.support.mock import MagicMock, patch + + +@pytest.fixture +def configure_loader_modules(): + return {keystone_role_grant: {}} + + +def _base_salt_dunder(**overrides): + role = MagicMock() + role.id = "role-id" + salt_dunder = { + "keystoneng.setup_clouds": MagicMock(), + "keystoneng.role_get": MagicMock(return_value=role), + "keystoneng.role_grant": MagicMock(), + "keystoneng.role_revoke": MagicMock(), + } + salt_dunder.update(overrides) + return salt_dunder + + +def test_present_test_mode_does_not_grant(): + """ + In test=True mode present() must not call role_grant and must + report result=None with predicted changes. + """ + salt_dunder = _base_salt_dunder() + salt_dunder["keystoneng.role_assignment_list"] = MagicMock(return_value=[]) + + with patch.dict(keystone_role_grant.__salt__, salt_dunder), patch.dict( + keystone_role_grant.__opts__, {"test": True} + ): + ret = keystone_role_grant.present("myrole") + + assert salt_dunder["keystoneng.role_grant"].call_count == 0 + assert ret["result"] is None + assert ret["changes"] == {"role": "role-id"} + assert ret["comment"] == "Role assignment would be granted" + + +def test_absent_test_mode_does_not_revoke(): + """ + In test=True mode absent() must not call role_revoke and must + report result=None with predicted changes. + """ + salt_dunder = _base_salt_dunder() + salt_dunder["keystoneng.role_assignment_list"] = MagicMock( + return_value=["existing-grant"] + ) + + with patch.dict(keystone_role_grant.__salt__, salt_dunder), patch.dict( + keystone_role_grant.__opts__, {"test": True} + ): + ret = keystone_role_grant.absent("myrole") + + assert salt_dunder["keystoneng.role_revoke"].call_count == 0 + assert ret["result"] is None + assert ret["changes"] == {"role": "role-id"} + assert ret["comment"] == "Role assignment would be revoked" From 28aab0edea842c1d6b34aae6ba5aeef6054f9757 Mon Sep 17 00:00:00 2001 From: "Gary T. Giesen" Date: Sun, 5 Jul 2026 23:20:37 -0400 Subject: [PATCH 2/2] Add direct and inverse regression tests for keystone_role_grant test mode The existing tests already call present() and absent() directly with __opts__["test"] patched to True, the exact mechanism the state compiler uses in production, so direct-altitude coverage was in place. This adds the inverse cases: test=False must still call role_grant/role_revoke with unchanged result/changes/comment (the legacy path must not be altered by the new test-mode branch), and test=True must keep reporting result=True with empty changes when the system is already in the desired state (no phantom pending changes). All four inverse tests pass with and without the source fix, guarding against overcorrection. --- .../unit/states/test_keystone_role_grant.py | 85 +++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/tests/pytests/unit/states/test_keystone_role_grant.py b/tests/pytests/unit/states/test_keystone_role_grant.py index 1e10e0ba7aa3..a9d555afc84e 100644 --- a/tests/pytests/unit/states/test_keystone_role_grant.py +++ b/tests/pytests/unit/states/test_keystone_role_grant.py @@ -64,3 +64,88 @@ def test_absent_test_mode_does_not_revoke(): assert ret["result"] is None assert ret["changes"] == {"role": "role-id"} assert ret["comment"] == "Role assignment would be revoked" + + +def test_present_real_mode_still_grants_52220(): + """ + Guards against overcorrection: with test=False (the state compiler's + default __opts__["test"] value on a real run) present() must still + call role_grant exactly as before the test-mode fix. + """ + salt_dunder = _base_salt_dunder() + salt_dunder["keystoneng.role_assignment_list"] = MagicMock(return_value=[]) + + with patch.dict(keystone_role_grant.__salt__, salt_dunder), patch.dict( + keystone_role_grant.__opts__, {"test": False} + ): + ret = keystone_role_grant.present("myrole") + + assert salt_dunder["keystoneng.role_grant"].call_count == 1 + assert ret["result"] is True + assert ret["changes"] == {"role": "role-id"} + assert ret["comment"] == "Granted role assignment" + + +def test_absent_real_mode_still_revokes_52220(): + """ + Guards against overcorrection: with test=False absent() must still + call role_revoke exactly as before the test-mode fix. + """ + salt_dunder = _base_salt_dunder() + salt_dunder["keystoneng.role_assignment_list"] = MagicMock( + return_value=["existing-grant"] + ) + + with patch.dict(keystone_role_grant.__salt__, salt_dunder), patch.dict( + keystone_role_grant.__opts__, {"test": False} + ): + ret = keystone_role_grant.absent("myrole") + + assert salt_dunder["keystoneng.role_revoke"].call_count == 1 + assert ret["result"] is True + assert ret["changes"] == {"role": "role-id"} + assert ret["comment"] == "Revoked role assignment" + + +def test_present_test_mode_no_changes_when_grant_exists_52220(): + """ + Guards against overcorrection: in test=True mode, when the role + assignment already exists, present() must keep reporting result=True + with no changes rather than a phantom pending change. + """ + salt_dunder = _base_salt_dunder() + salt_dunder["keystoneng.role_assignment_list"] = MagicMock( + return_value=["existing-grant"] + ) + + # test=True is the decisive flag; the no-grants branch must not run + with patch.dict(keystone_role_grant.__salt__, salt_dunder), patch.dict( + keystone_role_grant.__opts__, {"test": True} + ): + ret = keystone_role_grant.present("myrole") + + assert salt_dunder["keystoneng.role_grant"].call_count == 0 + assert ret["result"] is True + assert ret["changes"] == {} + assert ret["comment"] == "" + + +def test_absent_test_mode_no_changes_when_no_grant_52220(): + """ + Guards against overcorrection: in test=True mode, when no role + assignment exists, absent() must keep reporting result=True with no + changes rather than a phantom pending change. + """ + salt_dunder = _base_salt_dunder() + salt_dunder["keystoneng.role_assignment_list"] = MagicMock(return_value=[]) + + # test=True is the decisive flag; the grants-exist branch must not run + with patch.dict(keystone_role_grant.__salt__, salt_dunder), patch.dict( + keystone_role_grant.__opts__, {"test": True} + ): + ret = keystone_role_grant.absent("myrole") + + assert salt_dunder["keystoneng.role_revoke"].call_count == 0 + assert ret["result"] is True + assert ret["changes"] == {} + assert ret["comment"] == ""