diff --git a/changelog/67069.fixed.md b/changelog/67069.fixed.md new file mode 100644 index 000000000000..f50be67c59d7 --- /dev/null +++ b/changelog/67069.fixed.md @@ -0,0 +1,18 @@ +Fixed ``state.apply``/``state.highstate`` ignoring an explicitly requested +``saltenv`` when autoloading dynamic modules (``_modules``, ``_states``, +``_grains``, ...). ``BaseHighState.load_dynamic`` synced every saltenv present +in the top file matches, which can include saltenvs that came from +``master_tops`` data or from cross-saltenv ``- : `` entries in the +top file. Because all synced saltenvs are copied into the same flat +``extension_modules`` directory, the last saltenv synced won, so a +``salt '*' state.apply saltenv=qa`` could overwrite the ``qa`` copy of a custom +module with the ``base`` copy -- and leave it overwritten for subsequent runs. +A saltenv-pinned state run now syncs dynamic modules from that saltenv only. + +Relatedly, ``BaseHighState.top_matches`` now skips ``master_tops`` data for +saltenvs other than the requested one, matching how top file sections for other +saltenvs were already skipped. + +``salt.utils.extmods.sync`` now logs a warning when the same custom module name +is present in more than one of the saltenvs being synced, naming the saltenv +whose copy wins. diff --git a/doc/topics/development/modules/index.rst b/doc/topics/development/modules/index.rst index 1e4997335624..b6737e0b7634 100644 --- a/doc/topics/development/modules/index.rst +++ b/doc/topics/development/modules/index.rst @@ -67,7 +67,19 @@ dynamic modules when states are run. To disable this behavior set :conf_minion:`autoload_dynamic_modules` to ``False`` in the minion config. When dynamic modules are autoloaded via states, only the modules defined in the -same saltenvs as the states currently being run. +same saltenv as the states currently being run are synced. + +If the state run is pinned to a specific saltenv -- with the ``saltenv`` +argument, or via the :conf_minion:`saltenv` minion configuration option -- only +that saltenv is synced. Otherwise, every saltenv matched by the top file is +synced. + +.. note:: + All saltenvs are synced into the same ``extension_modules`` directory, which + is not divided per saltenv. When more than one saltenv is synced and they + contain a custom module of the same name, the saltenv synced last wins, and + its copy stays in place until something else overwrites it. Salt logs a + warning when this happens. Sync Via the saltutil Module ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/salt/state.py b/salt/state.py index da34efa01e60..b4ce31536be6 100644 --- a/salt/state.py +++ b/salt/state.py @@ -4465,6 +4465,19 @@ def _filter_matches(_match, _data, _opts): _filter_matches(match, data, self.opts["nodegroups"]) ext_matches = self._master_tops() for saltenv in ext_matches: + if self.opts["saltenv"] and saltenv != self.opts["saltenv"]: + # A saltenv was explicitly requested for this run. Top file + # sections for other saltenvs are skipped above, so master_tops + # data for other saltenvs has to be skipped too, otherwise the + # run pulls in states -- and, through load_dynamic(), custom + # modules -- from a saltenv it was told not to use. + log.debug( + "master_tops data for saltenv '%s' will be ignored, as this " + "state run is pinned to saltenv '%s'", + saltenv, + self.opts["saltenv"], + ) + continue top_file_matches = matches.get(saltenv, []) if self.opts.get("master_tops_first"): first = ext_matches[saltenv] @@ -4491,7 +4504,22 @@ def load_dynamic(self, matches): """ if not self.opts["autoload_dynamic_modules"]: return - syncd = self.state.functions["saltutil.sync_all"](list(matches), refresh=False) + if self.opts["saltenv"]: + # The state run is pinned to a single saltenv, so the dynamic + # modules have to come from that saltenv and nowhere else. + # + # ``matches`` cannot be trusted here: it can carry additional + # saltenvs picked up from master_tops data or from cross-saltenv + # ``- : `` entries in the top file. Every synced + # saltenv is copied into the same flat ``extension_modules`` + # directory, so the last saltenv synced silently wins. That let a + # ``state.apply saltenv=qa`` clobber the qa copy of a custom module + # with the ``base`` copy, and left it clobbered for later runs. + saltenvs = [self.opts["saltenv"]] + else: + saltenvs = list(matches) + log.debug("Syncing dynamic modules from saltenv(s): %s", saltenvs) + syncd = self.state.functions["saltutil.sync_all"](saltenvs, refresh=False) if syncd["grains"]: self.opts["grains"] = salt.loader.grains(self.opts) self.state.opts["pillar"] = self.state._gather_pillar() diff --git a/salt/utils/extmods.py b/salt/utils/extmods.py index f1b8a8264483..82bfd06c9a00 100644 --- a/salt/utils/extmods.py +++ b/salt/utils/extmods.py @@ -68,6 +68,11 @@ def sync( saltenv = saltenv.split(",") ret = [] remote = set() + # relpath -> saltenv it was last copied from. All saltenvs share a single + # flat destination directory, so syncing more than one saltenv means the + # last one wins for any module name they have in common. Track it so the + # collision is at least visible in the logs. + synced_from = {} source = salt.utils.url.create("_" + form) mod_dir = os.path.join(opts["extension_modules"], f"{form}") touched = False @@ -117,6 +122,20 @@ def sync( ): continue remote.add(relpath) + if synced_from.get(relpath, sub_env) != sub_env: + log.warning( + "Custom %s '%s' exists in more than one of the " + "saltenvs being synced (%s); the copy from " + "saltenv '%s' overwrites the one from saltenv " + "'%s' in %s", + form, + relname, + ", ".join(saltenv), + sub_env, + synced_from[relpath], + mod_dir, + ) + synced_from[relpath] = sub_env dest = os.path.join(mod_dir, relpath) log.info("Copying '%s' to '%s'", fn_, dest) if os.path.isfile(dest): diff --git a/tests/pytests/unit/state/test_load_dynamic.py b/tests/pytests/unit/state/test_load_dynamic.py new file mode 100644 index 000000000000..17a0d1f906c2 --- /dev/null +++ b/tests/pytests/unit/state/test_load_dynamic.py @@ -0,0 +1,158 @@ +""" +Tests for the saltenv used when a state run autoloads dynamic modules. + +``state.apply saltenv=qa`` has to sync ``_modules``/``_states``/... from the +``qa`` saltenv only. Every saltenv that gets synced lands in the same flat +``extension_modules`` directory, so any extra saltenv that sneaks into the sync +list overwrites the modules the requested saltenv just provided. +""" + +import pytest + +import salt.state +from salt.utils.datastructures import DefaultOrderedDict, HashableOrderedDict +from tests.support.mock import MagicMock + +pytestmark = [ + pytest.mark.core_test, +] + + +class MockClient: + def __init__(self, opts): + self.opts = opts + + def master_opts(self): + return self.opts + + def envs(self): + return ["base", "qa"] + + def list_states(self, saltenv): + return ["common", "foo"] + + def destroy(self): + pass + + +class MockHighState(salt.state.BaseHighState): + """ + Just enough of a HighState to drive ``top_matches``/``load_dynamic``. + """ + + def __init__(self, opts, ext_matches=None): + self.client = MockClient(opts) + self._ext_matches = ext_matches or {} + self.sync_calls = [] + super().__init__(opts) + self.matchers = {"confirm_top.confirm_top": MagicMock(return_value=True)} + self.state = MagicMock() + self.state.opts = self.opts + self.state.functions = {"saltutil.sync_all": self._sync_all} + + def _sync_all(self, saltenv=None, refresh=True, **kwargs): + self.sync_calls.append(saltenv) + return {"grains": []} + + def _master_tops(self): + return self._ext_matches + + def destroy(self): + self.client.destroy() + + +@pytest.fixture +def highstate_opts(minion_opts): + minion_opts["autoload_dynamic_modules"] = True + minion_opts["file_roots"] = {"base": [], "qa": []} + minion_opts["nodegroups"] = {} + minion_opts["id"] = "minion" + return minion_opts + + +def _top(data): + return DefaultOrderedDict(HashableOrderedDict, data) + + +def test_load_dynamic_uses_requested_saltenv_with_master_tops(highstate_opts): + """ + master_tops data for another saltenv must not drag that saltenv into the + dynamic module sync when the run is pinned to a saltenv. + """ + highstate_opts["saltenv"] = "qa" + hs = MockHighState(highstate_opts, ext_matches={"base": ["common"]}) + + hs.load_dynamic(hs.top_matches(_top({"qa": {"*": ["foo"]}}))) + + assert hs.sync_calls == [["qa"]] + + +def test_load_dynamic_uses_requested_saltenv_with_cross_saltenv_include( + highstate_opts, +): + """ + A ``- base: common`` entry in the top file must not drag ``base`` into the + dynamic module sync when the run is pinned to a saltenv. + """ + highstate_opts["saltenv"] = "qa" + hs = MockHighState(highstate_opts) + + matches = hs.top_matches(_top({"qa": {"*": ["foo", {"base": "common"}]}})) + # the cross-saltenv include is still honored for state rendering + assert "base" in matches + + hs.load_dynamic(matches) + + assert hs.sync_calls == [["qa"]] + + +def test_top_matches_ignores_master_tops_from_other_saltenvs(highstate_opts): + """ + A saltenv-pinned run only considers master_tops data for that saltenv, + matching how top file sections for other saltenvs are already skipped. + """ + highstate_opts["saltenv"] = "qa" + hs = MockHighState(highstate_opts, ext_matches={"base": ["common"], "qa": ["bar"]}) + + matches = hs.top_matches(_top({"qa": {"*": ["foo"]}})) + + assert dict(matches) == {"qa": ["foo", "bar"]} + + +def test_top_matches_keeps_master_tops_when_no_saltenv_requested(highstate_opts): + """ + Without an explicit saltenv, master_tops data for every saltenv is still + merged in. + """ + highstate_opts["saltenv"] = None + hs = MockHighState(highstate_opts, ext_matches={"base": ["common"]}) + + matches = hs.top_matches(_top({"qa": {"*": ["foo"]}})) + + assert dict(matches) == {"qa": ["foo"], "base": ["common"]} + + +def test_load_dynamic_syncs_matched_saltenvs_when_no_saltenv_requested( + highstate_opts, +): + """ + Without an explicit saltenv the matched saltenvs are synced, as before. + """ + highstate_opts["saltenv"] = None + hs = MockHighState(highstate_opts) + + hs.load_dynamic( + hs.top_matches(_top({"base": {"*": ["common"]}, "qa": {"*": ["foo"]}})) + ) + + assert hs.sync_calls == [["base", "qa"]] + + +def test_load_dynamic_noop_when_autoload_disabled(highstate_opts): + highstate_opts["saltenv"] = "qa" + highstate_opts["autoload_dynamic_modules"] = False + hs = MockHighState(highstate_opts) + + hs.load_dynamic(hs.top_matches(_top({"qa": {"*": ["foo"]}}))) + + assert hs.sync_calls == [] diff --git a/tests/pytests/unit/utils/test_extmods.py b/tests/pytests/unit/utils/test_extmods.py new file mode 100644 index 000000000000..a465105de5cc --- /dev/null +++ b/tests/pytests/unit/utils/test_extmods.py @@ -0,0 +1,112 @@ +""" +Tests for salt.utils.extmods.sync +""" + +import logging +import os + +import pytest + +import salt.utils.extmods +import salt.utils.files +from tests.support.mock import patch + +pytestmark = [ + pytest.mark.core_test, +] + + +@pytest.fixture +def cachedir(tmp_path): + path = tmp_path / "cache" + path.mkdir() + return str(path) + + +@pytest.fixture +def extension_modules(tmp_path): + return str(tmp_path / "extmods") + + +@pytest.fixture +def opts(cachedir, extension_modules): + return { + "cachedir": cachedir, + "extension_modules": extension_modules, + "extmod_whitelist": {}, + "extmod_blacklist": {}, + "clean_dynamic_modules": True, + "hash_type": "sha256", + } + + +@pytest.fixture +def fileclient(cachedir): + """ + Serve ``_modules/foo.py`` with different contents per saltenv, the way a + gitfs master with a ``qa`` branch and a ``master`` branch would. + """ + contents = {"base": "VERSION = '1.0'\n", "qa": "VERSION = '1.1'\n"} + for saltenv, body in contents.items(): + env_dir = os.path.join(cachedir, "files", saltenv, "_modules") + os.makedirs(env_dir) + with salt.utils.files.fopen( + os.path.join(env_dir, "foo.py"), "w", encoding="utf-8" + ) as fh_: + fh_.write(body) + + class FileClient: + def cache_dir(self, source, saltenv, **kwargs): + return [os.path.join(cachedir, "files", saltenv, "_modules", "foo.py")] + + def __enter__(self): + return self + + def __exit__(self, *args): + return False + + return FileClient() + + +def _synced(extension_modules): + path = os.path.join(extension_modules, "modules", "foo.py") + with salt.utils.files.fopen(path, encoding="utf-8") as fh_: + return fh_.read().strip() + + +@pytest.mark.parametrize( + "saltenv,expected", + [ + (["qa"], "VERSION = '1.1'"), + (["base"], "VERSION = '1.0'"), + ], +) +def test_sync_single_saltenv(opts, extension_modules, fileclient, saltenv, expected): + with patch("salt.fileclient.get_file_client", return_value=fileclient): + salt.utils.extmods.sync(opts, "modules", saltenv=saltenv) + assert _synced(extension_modules) == expected + + +def test_sync_multiple_saltenvs_warns_about_the_overwrite( + opts, extension_modules, fileclient, caplog +): + """ + All saltenvs share one flat ``extension_modules`` directory, so the last + saltenv synced wins for any module name they have in common. That is + long-standing behavior; make sure it is at least logged, since it is + otherwise invisible and looks like the wrong saltenv was used. + """ + with caplog.at_level(logging.WARNING, logger="salt.utils.extmods"): + with patch("salt.fileclient.get_file_client", return_value=fileclient): + salt.utils.extmods.sync(opts, "modules", saltenv=["qa", "base"]) + + assert _synced(extension_modules) == "VERSION = '1.0'" + assert "exists in more than one of the saltenvs being synced" in caplog.text + + +def test_sync_single_saltenv_does_not_warn(opts, fileclient, caplog): + with caplog.at_level(logging.WARNING, logger="salt.utils.extmods"): + with patch("salt.fileclient.get_file_client", return_value=fileclient): + salt.utils.extmods.sync(opts, "modules", saltenv=["qa"]) + + assert "exists in more than one of the saltenvs being synced" not in caplog.text