diff --git a/changelog/69806.fixed.md b/changelog/69806.fixed.md new file mode 100644 index 000000000000..07aa1fd1309b --- /dev/null +++ b/changelog/69806.fixed.md @@ -0,0 +1 @@ +Fix loader race that could randomly mark OS-specific virtual modules (e.g. ``postgres``) as unavailable when a sibling implementation (e.g. ``deb_postgres``) was evaluated first and poisoned the shared ``__virtualname__`` in the missing-modules cache. diff --git a/salt/loader/lazy.py b/salt/loader/lazy.py index 677371f6734a..372b375776d1 100644 --- a/salt/loader/lazy.py +++ b/salt/loader/lazy.py @@ -310,6 +310,13 @@ def __init__( # names of modules that we don't have (errors, __virtual__, etc.) self.missing_modules = {} # mapping of name -> error + # mapping of __virtualname__ -> list of error reasons from every file + # that claimed the virtualname and whose __virtual__() returned False. + # Kept separate from missing_modules so a failed sibling (e.g. + # deb_postgres) does not poison the shared virtualname (e.g. postgres) + # and prevent the real module from loading. Consulted by + # missing_fun_string() to surface every failure reason. + self.missing_virtualnames = {} self.loaded_modules = set() self.loaded_files = set() # TODO: just remove them from file_mapping? self.static_modules = static_modules if static_modules else [] @@ -370,6 +377,8 @@ def destroy(self): self.loaded_modules.clear() if hasattr(self, "missing_modules"): self.missing_modules.clear() + if hasattr(self, "missing_virtualnames"): + self.missing_virtualnames.clear() def clean_modules(self): """ @@ -491,18 +500,34 @@ def missing_fun_string(self, function_name): mod_name = function_name.split(".")[0] if mod_name in self.loaded_modules: return f"'{function_name}' is not available." - else: - try: - reason = self.missing_modules[mod_name] - except KeyError: - return f"'{function_name}' is not available." - else: - if reason is not None: - return "'{}' __virtual__ returned False: {}".format( - mod_name, reason - ) - else: - return f"'{mod_name}' __virtual__ returned False" + + # Collect reasons from missing_modules (keyed by file basename) and + # from missing_virtualnames (keyed by shared __virtualname__). The + # latter lets us surface every failure reason when multiple files + # collide on a single virtualname (e.g. x509 and x509_v2). + reasons = [] + seen = set() + primary = self.missing_modules.get(mod_name, KeyError) + if primary is not KeyError and primary is not None: + reason_str = str(primary) + if reason_str not in seen: + seen.add(reason_str) + reasons.append(reason_str) + for reason in self.missing_virtualnames.get(mod_name, ()): + if reason is None: + continue + reason_str = str(reason) + if reason_str not in seen: + seen.add(reason_str) + reasons.append(reason_str) + + if reasons: + return "'{}' __virtual__ returned False: {}".format( + mod_name, "; ".join(reasons) + ) + if mod_name in self.missing_modules or mod_name in self.missing_virtualnames: + return f"'{mod_name}' __virtual__ returned False" + return f"'{function_name}' is not available." def _refresh_file_mapping(self): """ @@ -670,6 +695,7 @@ def clear(self): super().clear() # clear the lazy loader self.loaded_files = set() self.missing_modules = {} + self.missing_virtualnames = {} self.loaded_modules = set() # if we have been loaded before, lets clear the file mapping since # we obviously want a re-do @@ -1041,26 +1067,28 @@ def _load_module(self, name): # if _process_virtual returned a non-True value then we are # supposed to not process this module if virtual_ret is not True: - # Always record the per-file reason; `name` is unique. + # Record the failure under both the file path (`name`) + # and the file basename (`module_name`). We intentionally + # do NOT record it under __virtualname__ in + # missing_modules: a sibling file failing (e.g. + # deb_postgres on RHEL) must never poison the shared + # virtualname (e.g. postgres) and block the real module + # from loading (issue #69806). self.missing_modules[name] = virtual_err - # The virtualname (module_name) can collide when multiple - # files declare the same __virtualname__ (e.g. x509 and - # x509_v2 both use "x509"). If we've already recorded a - # reason for this virtualname, append the new one so the - # user sees every failure, not just the first. if module_name not in self.missing_modules: self.missing_modules[module_name] = virtual_err - elif virtual_err is not None: - existing = self.missing_modules[module_name] - if existing is None: - self.missing_modules[module_name] = virtual_err - else: - existing_str = str(existing) - new_str = str(virtual_err) - if new_str and new_str not in existing_str.split("; "): - self.missing_modules[module_name] = ( - f"{existing_str}; {new_str}" - ) + # For error-message quality (issue #68625), track every + # failure reason for a shared __virtualname__ in a + # separate structure that missing_fun_string() consults. + virtualname = getattr(mod, "__virtualname__", None) + if ( + isinstance(virtualname, str) + and virtualname + and virtualname != module_name + ): + reasons = self.missing_virtualnames.setdefault(virtualname, []) + if virtual_err not in reasons: + reasons.append(virtual_err) return False else: virtual_aliases = () @@ -1309,16 +1337,18 @@ def _process_virtual(self, mod, module_name, virtual_func="__virtual__"): module_name, ) - # If the module explicitly declares __virtualname__, report - # the failure under that name so the caller can detect - # collisions with other modules claiming the same name. - if ( - hasattr(mod, "__virtualname__") - and isinstance(virtualname, str) - and virtualname - ): - module_name = virtualname - + # NOTE: Do NOT reassign ``module_name`` to the module's + # __virtualname__ on failure here. Doing so caused the + # caller to poison ``missing_modules[virtualname]`` (issue + # #69806): when a sibling module (e.g. deb_postgres on a + # non-Debian host) failed its __virtual__ check first, the + # real module claiming the same virtualname (postgres) was + # skipped by ``_load()`` because that virtualname was + # already marked missing. The caller now tracks failure + # reasons per-__virtualname__ separately (see + # ``missing_virtualnames``) so error surfacing for + # collisions (issue #68625) is preserved without the + # poisoning race. return (False, module_name, error_reason, virtual_aliases) # At this point, __virtual__ did not return a diff --git a/tests/pytests/unit/loader/test_lazy.py b/tests/pytests/unit/loader/test_lazy.py index cf4cebd3a64c..20d377fea2d9 100644 --- a/tests/pytests/unit/loader/test_lazy.py +++ b/tests/pytests/unit/loader/test_lazy.py @@ -208,11 +208,74 @@ def expires(*args, **kwargs): with pytest.raises(KeyError): _ = loader["x509.expires"] + # The primary failure (x509.py has file basename == virtualname) is + # recorded in missing_modules; the sibling x509_v2 failure is tracked + # separately in missing_virtualnames to avoid poisoning the shared + # virtualname for unrelated modules (see #69806). reason = loader.missing_modules.get("x509") assert reason is not None - assert "Superseded, using x509_v2" in reason - assert "Could not load cryptography" in reason + assert "Superseded, using x509_v2" in str(reason) + + extra = loader.missing_virtualnames.get("x509", []) + assert any("Could not load cryptography" in str(r) for r in extra) msg = loader.missing_fun_string("x509.expires") assert "Superseded, using x509_v2" in msg assert "Could not load cryptography" in msg + + +def test_virtualname_sibling_failure_does_not_poison_real_module(tmp_path): + """ + A sibling module whose __virtual__() returns False must not poison the + shared __virtualname__ in missing_modules and block the real module + from loading. + + Regression test for #69806: on non-Debian OSes, deb_postgres.py's + __virtual__() returns False and (under the buggy code) recorded the + failure under missing_modules["postgres"] via its __virtualname__. On + the next _load("postgres.foo") call the loader early-returned because + "postgres" was already marked missing and the real postgres.py module + was never loaded. + """ + (tmp_path / "deb_postgres.py").write_text( + textwrap.dedent( + """ + __virtualname__ = "postgres" + + def __virtual__(): + return (False, "Not a Debian host") + + def user_create(*args, **kwargs): + return True + """ + ) + ) + (tmp_path / "postgres.py").write_text( + textwrap.dedent( + """ + __virtualname__ = "postgres" + + def __virtual__(): + return True + + def user_create(*args, **kwargs): + return "real-postgres" + """ + ) + ) + + opts = {"optimization_order": [0, 1, 2]} + loader = salt.loader.lazy.LazyLoader([str(tmp_path)], opts) + + # Force the failing sibling to be processed first so we hit the race the + # bug produced (non-deterministic directory ordering in the wild). + loader._load_module("deb_postgres") + + # The failing sibling must be recorded under its own basename, NOT + # under the shared virtualname. + assert "deb_postgres" in loader.missing_modules + assert "postgres" not in loader.missing_modules + + # The real postgres.py must still load and provide postgres.user_create. + fun = loader["postgres.user_create"] + assert fun() == "real-postgres"