Fixed parsing CDROM apt sources (#62475)

* Fixed parsing CDROM apt sources

Fixes #62474

Signed-off-by: Pedro Algarvio <palgarvio@vmware.com>

* Deprecate `salt.modules.aptpkg.expand_repo_def()`

Fixes #62485

Signed-off-by: Pedro Algarvio <palgarvio@vmware.com>

* Update test_aptpkg.py

Need to import os

Signed-off-by: Pedro Algarvio <palgarvio@vmware.com>
Co-authored-by: Gareth J. Greenaway <gareth@saltstack.com>

changelog/62474.fixed

@@ -0,0 +1 @@
+Fixed parsing CDROM apt sources

changelog/62485.deprecated

@@ -0,0 +1 @@
+The `expand_repo_def` function in `salt.modules.aptpkg` is now deprecated. It's only used in `salt.states.pkgrepo` and it has no use of being exposed to the CLI.

salt/modules/aptpkg.py

@@ -47,6 +47,7 @@ from salt.exceptions import (
     SaltInvocationError,
 )
 from salt.modules.cmdmod import _parse_env
+from salt.utils.versions import warn_until_date
 
 log = logging.getLogger(__name__)
 
@@ -146,7 +147,15 @@ def _invalid(line):
         comment = line[idx + 1 :]
         line = line[:idx]
 
-    repo_line = line.strip().split()
+    cdrom_match = re.match(r"(.*)(cdrom:.*/)(.*)", line.strip())
+    if cdrom_match:
+        repo_line = (
+            [p.strip() for p in cdrom_match.group(1).split()]
+            + [cdrom_match.group(2).strip()]
+            + [p.strip() for p in cdrom_match.group(3).split()]
+        )
+    else:
+        repo_line = line.strip().split()
     if (
         not repo_line
         or repo_line[0] not in ["deb", "deb-src", "rpm", "rpm-src"]
@@ -1719,7 +1728,7 @@ def _get_opts(line):
     """
     Return all opts in [] for a repo line
     """
-    get_opts = re.search(r"\[.*\]", line)
+    get_opts = re.search(r"\[(.*=.*)\]", line)
     ret = {
         "arch": {"full": "", "value": "", "index": 0},
         "signedby": {"full": "", "value": "", "index": 0},
@@ -1851,7 +1860,6 @@ def list_repo_pkgs(*args, **kwargs):  # pylint: disable=unused-import
 
     ret = {}
     pkg_name = None
-    skip_pkg = False
     new_pkg = re.compile("^Package: (.+)")
     for line in salt.utils.itertools.split(out["stdout"], "\n"):
         if not line.strip():
@@ -3009,7 +3017,7 @@ def file_dict(*packages, **kwargs):
     return __salt__["lowpkg.file_dict"](*packages)
 
 
-def expand_repo_def(**kwargs):
+def _expand_repo_def(os_name, lsb_distrib_codename=None, **kwargs):
     """
     Take a repository definition and expand it to the full pkg repository dict
     that can be used for comparison.  This is a helper function to make
@@ -3023,8 +3031,8 @@ def expand_repo_def(**kwargs):
 
     sanitized = {}
     repo = kwargs["repo"]
-    if repo.startswith("ppa:") and __grains__["os"] in ("Ubuntu", "Mint", "neon"):
+    if repo.startswith("ppa:") and os_name in ("Ubuntu", "Mint", "neon"):
-        dist = __grains__["lsb_distrib_codename"]
+        dist = lsb_distrib_codename
         owner_name, ppa_name = repo[4:].split("/", 1)
         if "ppa_auth" in kwargs:
             auth_info = "{}@".format(kwargs["ppa_auth"])
@@ -3104,6 +3112,32 @@ def expand_repo_def(**kwargs):
     return sanitized
 
 
+def expand_repo_def(**kwargs):
+    """
+    Take a repository definition and expand it to the full pkg repository dict
+    that can be used for comparison.  This is a helper function to make
+    the Debian/Ubuntu apt sources sane for comparison in the pkgrepo states.
+
+    This is designed to be called from pkgrepo states and will have little use
+    being called on the CLI.
+
+    CLI Examples:
+
+    .. code-block:: bash
+
+        NOT USABLE IN THE CLI
+    """
+    warn_until_date(
+        "20240101",
+        "The pkg.expand_repo_def function is deprecated and set for removal "
+        "after {date}. This is only unsed internally by the apt pkg state "
+        "module. If that's not the case, please file an new issue requesting "
+        "the removal of this deprecation warning",
+        stacklevel=3,
+    )
+    return _expand_repo_def(**kwargs)
+
+
 def _parse_selections(dpkgselection):
     """
     Parses the format from ``dpkg --get-selections`` and return a format that

salt/states/pkgrepo.py

@@ -446,8 +446,18 @@ def managed(name, ppa=None, copr=None, aptkey=True, **kwargs):
     # out of the state itself and into a module that it makes more sense
     # to use. Most package providers will simply return the data provided
     # it doesn't require any "specialized" data massaging.
-    if "pkg.expand_repo_def" in __salt__:
+    if __grains__.get("os_family") == "Debian":
-        sanitizedkwargs = __salt__["pkg.expand_repo_def"](repo=repo, **kwargs)
+        from salt.modules.aptpkg import _expand_repo_def
+
+        os_name = __grains__["os"]
+        lsb_distrib_codename = __grains__["lsb_distrib_codename"]
+
+        sanitizedkwargs = _expand_repo_def(
+            os_name=os_name,
+            lsb_distrib_codename=lsb_distrib_codename,
+            repo=repo,
+            **kwargs
+        )
     else:
         sanitizedkwargs = kwargs
 

tests/pytests/functional/modules/test_aptpkg.py

@@ -1,3 +1,4 @@
+import os
 import pathlib
 import shutil
 
@@ -202,12 +203,19 @@ def test_del_repo(revert_repo_file):
     assert "Repo {} doesn't exist".format(test_repo) in exc.value.message
 
 
-def test_expand_repo_def():
+@pytest.mark.skipif(
+    not os.path.isfile("/etc/apt/sources.list"), reason="Missing /etc/apt/sources.list"
+)
+def test__expand_repo_def(grains):
     """
-    Test aptpkg.expand_repo_def when the repo exists.
+    Test aptpkg._expand_repo_def when the repo exists.
     """
     test_repo, comps = get_current_repo()
-    ret = aptpkg.expand_repo_def(repo=test_repo)
+    ret = aptpkg._expand_repo_def(
+        os_name=grains["os"],
+        lsb_distrib_codename=grains.get("lsb_distrib_codename"),
+        repo=test_repo,
+    )
     for key in [
         "comps",
         "dist",

tests/pytests/functional/states/pkgrepo/test_debian.py

@@ -70,6 +70,24 @@ def test_adding_repo_file_arch(pkgrepo, tmp_path):
         )
 
 
+@pytest.mark.requires_salt_states("pkgrepo.managed")
+def test_adding_repo_file_cdrom(pkgrepo, tmp_path):
+    """
+    test adding a repo file using pkgrepo.managed
+    The issue is that CDROM installs often have [] in the line, and we
+    should still add the repo even though it's not setting arch(for example)
+    """
+    repo_file = str(tmp_path / "cdrom.list")
+    repo_content = "deb cdrom:[Debian GNU/Linux 11.4.0 _Bullseye_ - Official amd64 NETINST 20220709-10:31]/ stable main"
+    pkgrepo.managed(name=repo_content, file=repo_file, clean_file=True)
+    with salt.utils.files.fopen(repo_file, "r") as fp:
+        file_content = fp.read()
+        assert (
+            file_content.strip()
+            == "deb cdrom:[Debian GNU/Linux 11.4.0 _Bullseye_ - Official amd64 NETINST 20220709-10:31]/ stable main"
+        )
+
+
 def system_aptsources_ids(value):
     return "{}(aptsources.sourceslist)".format(value.title())
 

tests/pytests/unit/modules/test_aptpkg.py

@@ -853,8 +853,9 @@ def test__skip_source():
     assert ret is False
 
 
-def test__parse_source():
+@pytest.mark.parametrize(
-    cases = (
+    "case",
+    (
         {"ok": False, "line": "", "invalid": True, "disabled": False},
         {"ok": False, "line": "#", "invalid": True, "disabled": True},
         {"ok": False, "line": "##", "invalid": True, "disabled": True},
@@ -881,19 +882,31 @@ def test__parse_source():
             "invalid": False,
             "disabled": False,
         },
-    )
+        {
+            "ok": True,
+            "line": (
+                "# deb cdrom:[Debian GNU/Linux 11.4.0 _Bullseye_ - Official amd64 NETINST 20220709-10:31]/ bullseye main\n"
+                "\n"
+                "deb http://httpredir.debian.org/debian bullseye main\n"
+                "deb-src http://httpredir.debian.org/debian bullseye main\n"
+            ),
+            "invalid": False,
+            "disabled": True,
+        },
+    ),
+)
+def test__parse_source(case):
     with patch.dict("sys.modules", {"aptsources.sourceslist": None}):
         importlib.reload(aptpkg)
         NoAptSourceEntry = aptpkg.SourceEntry
     importlib.reload(aptpkg)
 
-    for case in cases:
+    source = NoAptSourceEntry(case["line"])
-        source = NoAptSourceEntry(case["line"])
+    ok = source._parse_sources(case["line"])
-        ok = source._parse_sources(case["line"])
 
-        assert ok is case["ok"]
+    assert ok is case["ok"]
-        assert source.invalid is case["invalid"]
+    assert source.invalid is case["invalid"]
-        assert source.disabled is case["disabled"]
+    assert source.disabled is case["disabled"]
 
 
 def test_normalize_name():
@@ -949,21 +962,17 @@ def test_list_repos():
                 assert repos[source_uri][0]["uri"][-1] == "/"
 
 
-@pytest.mark.skipif(
+def test__expand_repo_def():
-    HAS_APTSOURCES is False, reason="The 'aptsources' library is missing."
-)
-def test_expand_repo_def():
     """
-    Checks results from expand_repo_def
+    Checks results from _expand_repo_def
     """
-    source_type = "deb"
-    source_uri = "http://cdn-aws.deb.debian.org/debian/"
-    source_line = "deb http://cdn-aws.deb.debian.org/debian/ stretch main\n"
     source_file = "/etc/apt/sources.list"
 
     # Valid source
     repo = "deb http://cdn-aws.deb.debian.org/debian/ stretch main\n"
-    sanitized = aptpkg.expand_repo_def(repo=repo, file=source_file)
+    sanitized = aptpkg._expand_repo_def(
+        os_name="debian", lsb_distrib_codename="stretch", repo=repo, file=source_file
+    )
 
     assert isinstance(sanitized, dict)
     assert "uri" in sanitized
@@ -973,8 +982,51 @@ def test_expand_repo_def():
 
     # Pass the architecture and make sure it is added the the line attribute
     repo = "deb http://cdn-aws.deb.debian.org/debian/ stretch main\n"
-    sanitized = aptpkg.expand_repo_def(
+    sanitized = aptpkg._expand_repo_def(
-        repo=repo, file=source_file, architectures="amd64"
+        os_name="debian",
+        lsb_distrib_codename="stretch",
+        repo=repo,
+        file=source_file,
+        architectures="amd64",
+    )
+
+    # Make sure line is in the dict
+    assert isinstance(sanitized, dict)
+    assert "line" in sanitized
+
+    # Make sure the architecture is in line
+    assert (
+        sanitized["line"]
+        == "deb [arch=amd64] http://cdn-aws.deb.debian.org/debian/ stretch main"
+    )
+
+
+def test__expand_repo_def_cdrom():
+    """
+    Checks results from _expand_repo_def
+    """
+    source_file = "/etc/apt/sources.list"
+
+    # Valid source
+    repo = "# deb cdrom:[Debian GNU/Linux 11.4.0 _Bullseye_ - Official amd64 NETINST 20220709-10:31]/ bullseye main\n"
+    sanitized = aptpkg._expand_repo_def(
+        os_name="debian", lsb_distrib_codename="bullseye", repo=repo, file=source_file
+    )
+
+    assert isinstance(sanitized, dict)
+    assert "uri" in sanitized
+
+    # Make sure last character in of the URI is still a /
+    assert sanitized["uri"][-1] == "/"
+
+    # Pass the architecture and make sure it is added the the line attribute
+    repo = "deb http://cdn-aws.deb.debian.org/debian/ stretch main\n"
+    sanitized = aptpkg._expand_repo_def(
+        os_name="debian",
+        lsb_distrib_codename="stretch",
+        repo=repo,
+        file=source_file,
+        architectures="amd64",
     )
 
     # Make sure line is in the dict