diff --git a/changelog/68208.fixed.md b/changelog/68208.fixed.md new file mode 100644 index 00000000000..46441c2ca2f --- /dev/null +++ b/changelog/68208.fixed.md @@ -0,0 +1,9 @@ +Fixed ``pkgrepo.managed`` honouring ``clean_file: True`` when the desired +repo line is already present in the managed file alongside unrelated stale +lines. Previously the state returned "already configured" and silently +skipped both the file truncation and the re-write, leaving the stale +entries (for example an obsolete ``bullseye-backports`` line in a file +managed for ``bookworm-backports``) in place. The clean + reconfigure +path now runs whenever the managed file contains any non-comment, +non-blank content other than the desired repo line; when the file already +contains only the desired line the state remains idempotent. diff --git a/salt/states/pkgrepo.py b/salt/states/pkgrepo.py index 5c3664da35a..9388236d3c1 100644 --- a/salt/states/pkgrepo.py +++ b/salt/states/pkgrepo.py @@ -117,12 +117,14 @@ - aptkey: False """ +import os import sys import salt.utils.data import salt.utils.files import salt.utils.pkg.deb import salt.utils.pkg.rpm +import salt.utils.stringutils import salt.utils.versions from salt.exceptions import CommandExecutionError, SaltInvocationError from salt.state import STATE_INTERNAL_KEYWORDS as _STATE_INTERNAL_KEYWORDS @@ -479,7 +481,38 @@ def managed(name, ppa=None, copr=None, aptkey=True, **kwargs): else: sanitizedkwargs = kwargs - if pre: + # Issue #68208: when ``clean_file`` is requested the user expects the + # managed file to be truncated and re-populated with *only* the desired + # repo line. If the file currently contains any other non-blank, non- + # comment line the state is out-of-sync even though ``pkg.get_repo`` + # found a matching definition; force the clean + reconfigure path in + # that case. When the file is already exactly the desired line (the + # common steady-state case) the "already configured" short-circuit + # below is still hit and ``test=True`` correctly reports no changes. + clean_file_needs_reconfigure = False + if ( + pre + and kwargs.get("clean_file", False) + and kwargs.get("file") + and os.path.isfile(kwargs["file"]) + ): + try: + with salt.utils.files.fopen(kwargs["file"], "r") as _cf: + _stale = [ + _line + for _line in ( + salt.utils.stringutils.to_unicode(_raw).strip() for _raw in _cf + ) + if _line and not _line.startswith("#") and _line != name.strip() + ] + if _stale: + clean_file_needs_reconfigure = True + except OSError: + # If we cannot read the file, fall through to the normal flow; + # downstream code will surface the real error. + pass + + if pre and not clean_file_needs_reconfigure: # 22412: Remove file attribute in case same repo is set up multiple times but with different files pre.pop("file", None) sanitizedkwargs.pop("file", None) diff --git a/tests/pytests/unit/states/test_pkgrepo.py b/tests/pytests/unit/states/test_pkgrepo.py index bbae18b050d..e63bb201d46 100644 --- a/tests/pytests/unit/states/test_pkgrepo.py +++ b/tests/pytests/unit/states/test_pkgrepo.py @@ -5,6 +5,7 @@ import pytest import salt.states.pkgrepo as pkgrepo +import salt.utils.files from tests.support.mock import MagicMock, patch @@ -14,6 +15,7 @@ def configure_loader_modules(): pkgrepo: { "__opts__": {"test": True}, "__grains__": {"os": "", "os_family": ""}, + "__env__": "base", } } @@ -92,3 +94,110 @@ def test_managed_insecure_key(): ret["comment"] == "Cannot have 'key_url' using http with 'allow_insecure_key' set to True" ) + + +def test_managed_clean_file_with_matching_existing_repo_68208(tmp_path): + """ + Regression test for #68208. + + When pkgrepo.managed is called with ``clean_file: True`` and the desired + repo line already exists in the file *alongside other stale lines*, + the state must still empty the file and reconfigure the repo. Prior to + the fix it would return ``already configured`` and silently skip the + clean+reconfigure, leaving the stale lines in place. + """ + backports_file = tmp_path / "backports.list" + desired_line = "deb http://deb.debian.org/debian bookworm-backports main" + stale_line = "deb http://deb.debian.org/debian bullseye-backports main" + backports_file.write_text(stale_line + "\n" + desired_line + "\n") + + kwargs = { + "name": desired_line, + "disabled": False, + "file": str(backports_file), + "clean_file": True, + } + # pkg.get_repo returns the matching definition because the desired + # line is already present (along with an unrelated stale entry). + get_repo = MagicMock(return_value=kwargs.copy()) + mod_repo = MagicMock(return_value=None) + + # We want salt.utils.files.fopen to behave normally for the file-read + # (probing for stale content) but be observable for the "w" truncation + # call. Use a wrapper that delegates to the real fopen. + real_fopen = salt.utils.files.fopen + fopen_calls = [] + + def _track_fopen(*args, **kw): + fopen_calls.append((args, kw)) + return real_fopen(*args, **kw) + + with patch.dict( + pkgrepo.__salt__, + {"pkg.get_repo": get_repo, "pkg.mod_repo": mod_repo}, + ), patch.dict(pkgrepo.__opts__, {"test": False}), patch.dict( + pkgrepo.__grains__, + {"os": "Debian", "os_family": "Debian", "oscodename": "bookworm"}, + ), patch( + "salt.modules.aptpkg._expand_repo_def", + MagicMock(side_effect=lambda os_name, os_codename, repo, **kw: kw), + ), patch( + "salt.utils.files.fopen", side_effect=_track_fopen + ), patch( + "salt.utils.path.which", MagicMock(return_value=None) + ): + ret = pkgrepo.managed(**kwargs) + + # The state must NOT short-circuit with "already configured" when + # clean_file is requested and there is stale content in the file. + assert ret["comment"] != ( + "Package repo '{}' already configured".format(kwargs["name"]) + ) + # The file must have been opened for writing (truncation) before + # pkg.mod_repo is invoked. + assert ((str(backports_file), "w"), {}) in fopen_calls + # pkg.mod_repo must be called so the repo line is re-written to the + # now-empty file. + assert mod_repo.called + + +def test_managed_clean_file_with_only_desired_line_no_changes_68208(tmp_path): + """ + Companion to #68208 regression. When ``clean_file: True`` is set and + the managed file already contains exactly (and only) the desired repo + line, the state must still short-circuit to "already configured" so + repeated runs are idempotent. Without this, the fix for #68208 would + falsely report a change on every run. + """ + backports_file = tmp_path / "backports.list" + desired_line = "deb http://deb.debian.org/debian bookworm-backports main" + backports_file.write_text(desired_line + "\n") + + kwargs = { + "name": desired_line, + "disabled": False, + "file": str(backports_file), + "clean_file": True, + } + get_repo = MagicMock(return_value=kwargs.copy()) + mod_repo = MagicMock(return_value=None) + + with patch.dict( + pkgrepo.__salt__, + {"pkg.get_repo": get_repo, "pkg.mod_repo": mod_repo}, + ), patch.dict(pkgrepo.__opts__, {"test": False}), patch.dict( + pkgrepo.__grains__, + {"os": "Debian", "os_family": "Debian", "oscodename": "bookworm"}, + ), patch( + "salt.modules.aptpkg._expand_repo_def", + MagicMock(side_effect=lambda os_name, os_codename, repo, **kw: kw), + ), patch( + "salt.utils.path.which", MagicMock(return_value=None) + ): + ret = pkgrepo.managed(**kwargs) + + assert ret["comment"] == ( + "Package repo '{}' already configured".format(kwargs["name"]) + ) + assert ret["changes"] == {} + assert not mod_repo.called