From 77a751f83b1c4e1de7853b20230d2031302a274a Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Tue, 16 Aug 2022 21:35:27 +0100 Subject: [PATCH 1/3] Fixed parsing CDROM apt sources Fixes #62474 Signed-off-by: Pedro Algarvio --- changelog/62474.fixed | 1 + salt/modules/aptpkg.py | 13 +++- .../functional/states/pkgrepo/test_debian.py | 18 +++++ tests/pytests/unit/modules/test_aptpkg.py | 71 +++++++++++++++---- 4 files changed, 85 insertions(+), 18 deletions(-) create mode 100644 changelog/62474.fixed diff --git a/changelog/62474.fixed b/changelog/62474.fixed new file mode 100644 index 000000000000..bf45b04872a0 --- /dev/null +++ b/changelog/62474.fixed @@ -0,0 +1 @@ +Fixed parsing CDROM apt sources diff --git a/salt/modules/aptpkg.py b/salt/modules/aptpkg.py index 0b94cd03708c..07698d04f282 100644 --- a/salt/modules/aptpkg.py +++ b/salt/modules/aptpkg.py @@ -146,7 +146,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"] @@ -1716,7 +1724,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}, @@ -1848,7 +1856,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(): diff --git a/tests/pytests/functional/states/pkgrepo/test_debian.py b/tests/pytests/functional/states/pkgrepo/test_debian.py index 12ab69ab6b2e..d2159be4818c 100644 --- a/tests/pytests/functional/states/pkgrepo/test_debian.py +++ b/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()) diff --git a/tests/pytests/unit/modules/test_aptpkg.py b/tests/pytests/unit/modules/test_aptpkg.py index f72f4d53b1d5..3ea1315ea094 100644 --- a/tests/pytests/unit/modules/test_aptpkg.py +++ b/tests/pytests/unit/modules/test_aptpkg.py @@ -853,8 +853,9 @@ def test__skip_source(): assert ret is False -def test__parse_source(): - cases = ( +@pytest.mark.parametrize( + "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"]) - ok = source._parse_sources(case["line"]) + source = NoAptSourceEntry(case["line"]) + ok = source._parse_sources(case["line"]) - assert ok is case["ok"] - assert source.invalid is case["invalid"] - assert source.disabled is case["disabled"] + assert ok is case["ok"] + assert source.invalid is case["invalid"] + assert source.disabled is case["disabled"] def test_normalize_name(): @@ -949,16 +962,10 @@ def test_list_repos(): assert repos[source_uri][0]["uri"][-1] == "/" -@pytest.mark.skipif( - HAS_APTSOURCES is False, reason="The 'aptsources' library is missing." -) def test_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 @@ -988,6 +995,40 @@ def test_expand_repo_def(): ) +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(repo=repo, file=source_file) + log.warning("SAN: %s", sanitized) + + 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( + 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_list_pkgs(): """ Test packages listing. From 25751724313ba256ca0ed0ba7b503051e0b2c376 Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Thu, 18 Aug 2022 10:36:36 +0100 Subject: [PATCH 2/3] Deprecate `salt.modules.aptpkg.expand_repo_def()` Fixes #62485 Signed-off-by: Pedro Algarvio --- changelog/62485.deprecated | 1 + salt/modules/aptpkg.py | 33 +++++++++++++++++-- salt/states/pkgrepo.py | 14 ++++++-- .../pytests/functional/modules/test_aptpkg.py | 10 ++++-- tests/pytests/unit/modules/test_aptpkg.py | 33 ++++++++++++------- 5 files changed, 72 insertions(+), 19 deletions(-) create mode 100644 changelog/62485.deprecated diff --git a/changelog/62485.deprecated b/changelog/62485.deprecated new file mode 100644 index 000000000000..64fdf5869416 --- /dev/null +++ b/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. diff --git a/salt/modules/aptpkg.py b/salt/modules/aptpkg.py index 07698d04f282..cb40081e3e8e 100644 --- a/salt/modules/aptpkg.py +++ b/salt/modules/aptpkg.py @@ -47,6 +47,7 @@ SaltInvocationError, ) from salt.modules.cmdmod import _parse_env +from salt.utils.versions import warn_until_date log = logging.getLogger(__name__) @@ -3013,7 +3014,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 @@ -3027,8 +3028,8 @@ def expand_repo_def(**kwargs): sanitized = {} repo = kwargs["repo"] - if repo.startswith("ppa:") and __grains__["os"] in ("Ubuntu", "Mint", "neon"): - dist = __grains__["lsb_distrib_codename"] + if repo.startswith("ppa:") and os_name in ("Ubuntu", "Mint", "neon"): + dist = lsb_distrib_codename owner_name, ppa_name = repo[4:].split("/", 1) if "ppa_auth" in kwargs: auth_info = "{}@".format(kwargs["ppa_auth"]) @@ -3108,6 +3109,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 diff --git a/salt/states/pkgrepo.py b/salt/states/pkgrepo.py index 358c92769588..84aec74681cf 100644 --- a/salt/states/pkgrepo.py +++ b/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__: - sanitizedkwargs = __salt__["pkg.expand_repo_def"](repo=repo, **kwargs) + if __grains__.get("os_family") == "Debian": + 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 diff --git a/tests/pytests/functional/modules/test_aptpkg.py b/tests/pytests/functional/modules/test_aptpkg.py index 8b1e897bc70d..35d7c8c4cd62 100644 --- a/tests/pytests/functional/modules/test_aptpkg.py +++ b/tests/pytests/functional/modules/test_aptpkg.py @@ -210,12 +210,16 @@ def test_del_repo(revert_repo_file): @pytest.mark.skipif( not os.path.isfile("/etc/apt/sources.list"), reason="Missing /etc/apt/sources.list" ) -def test_expand_repo_def(): +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", diff --git a/tests/pytests/unit/modules/test_aptpkg.py b/tests/pytests/unit/modules/test_aptpkg.py index 3ea1315ea094..326f35400f67 100644 --- a/tests/pytests/unit/modules/test_aptpkg.py +++ b/tests/pytests/unit/modules/test_aptpkg.py @@ -962,15 +962,17 @@ def test_list_repos(): assert repos[source_uri][0]["uri"][-1] == "/" -def test_expand_repo_def(): +def test__expand_repo_def(): """ - Checks results from expand_repo_def + Checks results from _expand_repo_def """ 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 @@ -980,8 +982,12 @@ 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( - repo=repo, file=source_file, architectures="amd64" + 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 @@ -995,16 +1001,17 @@ def test_expand_repo_def(): ) -def test_expand_repo_def_cdrom(): +def test__expand_repo_def_cdrom(): """ - Checks results from expand_repo_def + 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(repo=repo, file=source_file) - log.warning("SAN: %s", sanitized) + 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 @@ -1014,8 +1021,12 @@ def test_expand_repo_def_cdrom(): # 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( - repo=repo, file=source_file, architectures="amd64" + 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 From 0503ee099563c59f9b74ee2d5db27392f2d4e8af Mon Sep 17 00:00:00 2001 From: "Gareth J. Greenaway" Date: Thu, 6 Oct 2022 15:58:03 -0700 Subject: [PATCH 3/3] Update test_aptpkg.py Need to import os --- tests/pytests/functional/modules/test_aptpkg.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/pytests/functional/modules/test_aptpkg.py b/tests/pytests/functional/modules/test_aptpkg.py index acf879694048..1c4de8c1b780 100644 --- a/tests/pytests/functional/modules/test_aptpkg.py +++ b/tests/pytests/functional/modules/test_aptpkg.py @@ -1,3 +1,4 @@ +import os import pathlib import shutil