Fix state.apply ignoring saltenv when autoloading dynamic modules

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 ``- <saltenv>: <sls>`` 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.

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
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~

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
+            # ``- <saltenv>: <sls>`` 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()

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):

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 == []

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