Fix pkgrepo.managed honouring clean_file when line already present (#68208)

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.

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)

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