From 90f0bdb015ec827bb0fc8dc7087e8f4e2c1f3432 Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Thu, 11 Jun 2026 17:38:26 -0700 Subject: [PATCH 1/2] Fix pkgrepo.managed to honour clean_file when repo line already present MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pkgrepo.managed compared the desired repo definition against the result of pkg.get_repo and, when they matched, short-circuited with "already configured" — even when the caller passed clean_file: True. That left any unrelated stale entries in the managed file untouched, with no reported change. The user's bookworm-backports state that also still listed bullseye-backports is the canonical example. Skip the short-circuit comparison when clean_file is requested so the truncate + reconfigure path always runs in that case. The post-mod_repo change-diff loop still reports the change correctly. Fixes #68208 --- changelog/68208.fixed.md | 7 ++++ salt/states/pkgrepo.py | 7 +++- tests/pytests/unit/states/test_pkgrepo.py | 50 +++++++++++++++++++++++ 3 files changed, 63 insertions(+), 1 deletion(-) create mode 100644 changelog/68208.fixed.md diff --git a/changelog/68208.fixed.md b/changelog/68208.fixed.md new file mode 100644 index 000000000000..9776f9f1abde --- /dev/null +++ b/changelog/68208.fixed.md @@ -0,0 +1,7 @@ +Fixed ``pkgrepo.managed`` honouring ``clean_file: True`` when the desired +repo line is already present in the file. Previously the state returned +"already configured" and silently skipped both the file truncation and the +re-write, leaving any unrelated stale entries (e.g. an obsolete +``bullseye-backports`` line in a file managed for ``bookworm-backports``) +in place. The clean + reconfigure path now always runs when +``clean_file: True`` is requested. diff --git a/salt/states/pkgrepo.py b/salt/states/pkgrepo.py index 5c3664da35af..77f4dec3488e 100644 --- a/salt/states/pkgrepo.py +++ b/salt/states/pkgrepo.py @@ -479,7 +479,12 @@ def managed(name, ppa=None, copr=None, aptkey=True, **kwargs): else: sanitizedkwargs = kwargs - if pre: + # When ``clean_file`` is requested the user wants the file emptied and + # re-populated regardless of whether ``pkg.get_repo`` already finds the + # desired line in it (the file may also contain stale lines that must be + # removed). Skip the short-circuit "already configured" comparison in + # that case so the clean + reconfigure path always runs. (Issue #68208) + if pre and not kwargs.get("clean_file", False): # 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 bbae18b050dc..b71b5c572c31 100644 --- a/tests/pytests/unit/states/test_pkgrepo.py +++ b/tests/pytests/unit/states/test_pkgrepo.py @@ -14,6 +14,7 @@ def configure_loader_modules(): pkgrepo: { "__opts__": {"test": True}, "__grains__": {"os": "", "os_family": ""}, + "__env__": "base", } } @@ -92,3 +93,52 @@ 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(): + """ + 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. + """ + kwargs = { + "name": "deb http://deb.debian.org/debian bookworm-backports main", + "disabled": False, + "file": "/etc/apt/sources.list.d/backports.list", + "clean_file": True, + } + # pkg.get_repo returns the same line because it is already present in + # the file (along with unrelated stale entries the user wants removed). + 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.files.fopen", MagicMock() + ) as fopen_mock, 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 -- it has to clean the file and reconfigure. + 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. + fopen_mock.assert_any_call(kwargs["file"], "w") + # pkg.mod_repo must be called so the repo line is re-written to the + # now-empty file. + assert mod_repo.called From 9b3634d22d0d3646f674a650706f9bc132448f69 Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Thu, 11 Jun 2026 21:25:56 -0700 Subject: [PATCH 2/2] Only force clean_file reconfigure when file has stale content The first cut for #68208 unconditionally skipped the pre/sanitized-kwargs short-circuit when clean_file was set. That broke the idempotency contract that the test_adding_repo_file_signedby[Without(aptsources.sourceslist)] functional test exercises: after a successful first run the file already contains exactly the desired line, and a subsequent test=True run should report no changes -- but the unconditional skip caused the test=True path to emit a spurious "file" diff (the file kwarg was no longer popped), producing changes != {}. Narrow the condition: only force the clean + reconfigure path when the managed file currently holds any non-blank, non-comment line other than the desired one. The exact-match-only case still short-circuits to "already configured", so repeated runs stay idempotent. Adds a companion regression test (the all-desired-line file remains no-op) alongside the original #68208 regression test (a file with stale lines triggers truncate + mod_repo). --- changelog/68208.fixed.md | 14 ++-- salt/states/pkgrepo.py | 40 ++++++++++-- tests/pytests/unit/states/test_pkgrepo.py | 79 ++++++++++++++++++++--- 3 files changed, 111 insertions(+), 22 deletions(-) diff --git a/changelog/68208.fixed.md b/changelog/68208.fixed.md index 9776f9f1abde..46441c2ca2fb 100644 --- a/changelog/68208.fixed.md +++ b/changelog/68208.fixed.md @@ -1,7 +1,9 @@ Fixed ``pkgrepo.managed`` honouring ``clean_file: True`` when the desired -repo line is already present in the file. Previously the state returned -"already configured" and silently skipped both the file truncation and the -re-write, leaving any unrelated stale entries (e.g. an obsolete -``bullseye-backports`` line in a file managed for ``bookworm-backports``) -in place. The clean + reconfigure path now always runs when -``clean_file: True`` is requested. +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 77f4dec3488e..9388236d3c10 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,12 +481,38 @@ def managed(name, ppa=None, copr=None, aptkey=True, **kwargs): else: sanitizedkwargs = kwargs - # When ``clean_file`` is requested the user wants the file emptied and - # re-populated regardless of whether ``pkg.get_repo`` already finds the - # desired line in it (the file may also contain stale lines that must be - # removed). Skip the short-circuit "already configured" comparison in - # that case so the clean + reconfigure path always runs. (Issue #68208) - if pre and not kwargs.get("clean_file", False): + # 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 b71b5c572c31..e63bb201d461 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 @@ -95,26 +96,42 @@ def test_managed_insecure_key(): ) -def test_managed_clean_file_with_matching_existing_repo_68208(): +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), + 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": "deb http://deb.debian.org/debian bookworm-backports main", + "name": desired_line, "disabled": False, - "file": "/etc/apt/sources.list.d/backports.list", + "file": str(backports_file), "clean_file": True, } - # pkg.get_repo returns the same line because it is already present in - # the file (along with unrelated stale entries the user wants removed). + # 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}, @@ -125,20 +142,62 @@ def test_managed_clean_file_with_matching_existing_repo_68208(): "salt.modules.aptpkg._expand_repo_def", MagicMock(side_effect=lambda os_name, os_codename, repo, **kw: kw), ), patch( - "salt.utils.files.fopen", MagicMock() - ) as fopen_mock, 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 -- it has to clean the file and reconfigure. + # 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. - fopen_mock.assert_any_call(kwargs["file"], "w") + 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