[RHELC-1434] Use public CDN instead of paywalled CDN and FTP #1123

bocekm · 2024-03-01T22:34:27Z

The convert2rhel repos moved from https://cdn.redhat.com/ to https://cdn-public.redhat.com/.
The convert2rhel repofiles moved from https://ftp.redhat.com/ to https://cdn-public.redhat.com/.

The https://cdn-public.redhat.com/ does not use a self-signed SSL cert as the https://cdn.redhat.com/ does. So we can drop handling the redhat-uep.pem certificate.

Jira Issues: RHELC-1434

Checklist

PR has been tested manually in a VM (either author or reviewer)
Jira issue has been made public if possible
[RHELC-] is part of the PR title
GitHub label has been added to help with Release notes
PR title explains the change from the user's point of view
Code and tests are documented properly
The commits are squashed to as few commits as possible (without losing data)
When merged: Jira issue has been updated to Release Pending if relevant

Depends on

[RHELC-601] Install subscription-manager using package manager #1164

has-bot · 2024-03-01T22:34:40Z

/packit test --labels tier0

Comment generated by an automation.

Log | Bot Usage

codecov · 2024-03-01T23:01:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.52%. Comparing base (3f6ce10) to head (00d0e4a).

❗ Current head 00d0e4a differs from pull request most recent head 886e206

Please upload reports for the commit 886e206 to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1123      +/-   ##
==========================================
- Coverage   95.74%   95.52%   -0.22%     
==========================================
  Files          55       54       -1     
  Lines        4752     4720      -32     
  Branches      834      832       -2     
==========================================
- Hits         4550     4509      -41     
- Misses        116      127      +11     
+ Partials       86       84       -2

Flag	Coverage Δ
centos-linux-7	`90.67% <100.00%> (-0.26%)`	⬇️
centos-linux-8	`91.61% <100.00%> (-0.25%)`	⬇️
centos-linux-9	`91.67% <100.00%> (-0.25%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

.pre-commit-config.yaml

convert2rhel/actions/system_checks/convert2rhel_latest.py

bocekm · 2024-03-05T22:16:01Z

I've updated the PR to download the official convert2rhel repofile instead of generating it. I'll be adding unit tests later on.

convert2rhel/actions/system_checks/convert2rhel_latest.py

bocekm · 2024-05-20T22:00:30Z

The PR is ready for review and will be ready for testing after #1164 is merged (it introduces repo.download_repofile and repo.write_temporary_repofile).

bookwar · 2024-05-21T09:15:17Z

/packit test --labels tier0

bocekm · 2024-05-21T09:43:45Z

convert2rhel/actions/system_checks/convert2rhel_latest.py

-        # Note: This is safe because we're creating in utils.TMP_DIR which is hardcoded to
-        # /var/lib/convert2rhel which does not have any world-writable directory components.
-        files.mkdir_p(repo_dir)
-
-        try:
-            raw_output_convert2rhel_versions, return_code = utils.run_subprocess(cmd, print_output=False)
-        finally:
-            shutil.rmtree(repo_dir)


FYI for reviewers: No need to create and then remove a temporary directory here - its creation is handled by repo.write_temporary_repofile() and its deletion by utils.remove_tmp_dir().

bocekm · 2024-05-21T09:48:06Z

convert2rhel/actions/system_checks/convert2rhel_latest.py

+        if system_info.version.major not in C2R_REPOFILE_URLS:
+            self.add_message(
+                level="WARNING",
+                id="CONVERT2RHEL_LATEST_CHECK_UNEXPECTED_SYS_VERSION",
+                title="Did not perform convert2rhel latest version check",
+                description="Checking whether the installed convert2rhel package is of the latest available version was"
+                " skipped due to an unexpected system version",
+                diagnosis="Expected system versions: %s. Detected major version: %s"
+                % (", ".join(str(x) for x in C2R_REPOFILE_URLS), system_info.version.major),
+            )
+            return None


FYI for reviewers: This is likely too much of a defensive programming. I can't foresee a scenario in which a customer would hit this. If anyone thinks this should better be removed, I can certainly do that.

I think, we halt the execution way before reaching any of this in case of the user having a system version that does not correspond to this, right?

I don't mind having this defensive code, so I would leave it here

Yes, if the system major version is anything other than 7 or 8, convert2rhel stops almost immediately for not having a distro config available.
When we start testing and shipping convert2rhel for RHEL 9, we will need to add a repofile to the C2R_REPOFILE_URLS mapping https://github.com/oamg/convert2rhel/pull/1123/files#diff-7e310f649f5ad232ee8a3ce27055e178d7303046cf546ef0dc6f4ea208db8589R32. So this report message will likely appear during development and will prompt us to add the RHEL 9 URL.
I'm going to keep the code then.

bocekm · 2024-05-21T13:30:14Z

tests/integration/tier0/non-destructive/subscription-manager/test_sub_man_rollback.py

-@pytest.fixture(scope="function")
-def convert2rhel_repo(shell):
-    assert shell("rpm -qi subscription-manager").returncode == 0
-    # The following repository requires the redhat-uep.pem certificate.
-    # convert2rhel will try accessing the repo and during the test we run the convert2rhel twice
-    # making sure that the rollback of the first run correctly reinstalled the certificate
-    # when installing subscription-manager-rhsm-certificates.
-    c2r_repo = "/etc/yum.repos.d/convert2rhel.repo"
-
-    assert (
-        shell(
-            f"curl -o {c2r_repo} https://cdn-public.redhat.com/content/public/repofiles/convert2rhel-for-rhel-8-x86_64.repo"
-        ).returncode
-        == 0
-    )
-    assert os.path.exists(c2r_repo) is True
-
-    yield
-
-    assert shell(f"rm -f {c2r_repo}")
-    assert os.path.exists(c2r_repo) is False


FYI for reviewers: This fixture was needed just for one of the cases tested by the below test_sub_man_rollback test. This case is no longer relevant because the convert2rhel repository no longer requires the /etc/rhsm/ca/redhat-uep.pem certificate to be in place.

bocekm · 2024-05-22T13:22:18Z

I'm working on the codecov failure/finding.

Review addressed

pr-watson

Looks good just added a comment to add a "." to the end of the description for CONVERT2RHEL_LATEST_CHECK_UNEXPECTED_SYS_VERSION. There's also no unit test for this Warning but I think it can be added later

convert2rhel/actions/system_checks/convert2rhel_latest.py

r0x0d · 2024-05-22T17:05:08Z

/packit test --labels tier0

r0x0d · 2024-05-22T18:10:59Z

/packit test --labels tier0

bocekm · 2024-05-23T11:43:36Z

convert2rhel/actions/system_checks/convert2rhel_latest.py

-            "--disablerepo=*",
-            "--enablerepo=convert2rhel",


FYI for reviewers: The temp directory used for the convert2rhel repofile is created only for the
repofile and nothing else. Because we tell repoquery to look only at that
directory we don't need to specify a repoid to use - repoquery will simply use
the repoid in the downloaded repofile.

r0x0d · 2024-05-23T12:15:44Z

/packit test --labels tier0

hosekadam · 2024-05-23T13:51:52Z

convert2rhel/repo.py

+            id_="CREATE_TMP_DIR_FOR_REPOFILES_FAILED",
+            title="Failed to create a temporary directory",
+            description="Failed to create a temporary directory for storing a repository file under %s.\n"
+            "Reason: %s" % (TMP_DIR, str(err)),


Suggested change

"Reason: %s" % (TMP_DIR, str(err)),

diagnosis="Reason: %s" % (TMP_DIR, str(err)),

Shouldn't this be diagnosis? (and for the exception above too)

Good point. I've read the notification too late. It's merged now but you can raise that under https://issues.redhat.com/browse/RHELC-1598.

The convert2rhel repos moved from https://cdn.redhat.com/ to https://cdn-public.redhat.com/. The convert2rhel repofiles moved from https://ftp.redhat.com/ to https://cdn-public.redhat.com/. The https://cdn-public.redhat.com/ does not use a self-signed SSL cert as the https://cdn.redhat.com/ does. So we can drop handling the redhat-uep.pem certificate.

It's safer to use the official convert2rhel repofile that is to be used by customers instead of generating it as that way any changes done it don't need further convert2rhel code updates.

Co-authored-by: Preston Watson <prwatson@redhat.com>

The temp directory used for the convert2rhel repofile is created only for the repofile and nothing else. Because we tell repoquery to look only at that directory we don't need to specify a repoid to use - repoquery will simply use the repoid in the downloaded repofile.

To better fit what the real meaning of the messages and the purpose of a method. More specifically: - the write_temporary_repofile() function is not just for storing a convert2rhel repofil - the _get_convert2rhel_repofile_path() method does more than just getting a path; its main purpose is downloading a repofile

bocekm · 2024-05-23T14:16:17Z

Rebased to include a fix to a regression introduced in #1238.

bocekm · 2024-05-23T14:16:23Z

/packit test --labels tier0

r0x0d · 2024-05-23T14:27:02Z

/packit test --labels tier0

r0x0d · 2024-05-23T18:38:05Z

/packit test --labels tier0

danmyway

QE Ack

bocekm added the tests/tier0 PR ready to run the essential test suit. Equivalent to `/packit test --labels tier0`. label Mar 1, 2024

bocekm requested a review from danmyway March 1, 2024 22:34

bocekm force-pushed the use-public-cdn-instead-of-ftp branch from 370bfa5 to eb95f7d Compare March 1, 2024 22:59

r0x0d force-pushed the use-public-cdn-instead-of-ftp branch from eb95f7d to 429ff7b Compare March 4, 2024 12:16

Venefilyn previously requested changes Mar 5, 2024

View reviewed changes

.pre-commit-config.yaml Outdated Show resolved Hide resolved

convert2rhel/actions/system_checks/convert2rhel_latest.py Outdated Show resolved Hide resolved

bocekm force-pushed the use-public-cdn-instead-of-ftp branch from 429ff7b to fb4bc8f Compare March 5, 2024 20:45

github-advanced-security bot found potential problems Mar 5, 2024

View reviewed changes

convert2rhel/actions/system_checks/convert2rhel_latest.py Fixed Show fixed Hide fixed

bocekm marked this pull request as draft March 5, 2024 22:14

r0x0d reviewed Apr 17, 2024

View reviewed changes

convert2rhel/actions/system_checks/convert2rhel_latest.py Outdated Show resolved Hide resolved

bocekm force-pushed the use-public-cdn-instead-of-ftp branch 2 times, most recently from 846ce59 to e00f418 Compare May 20, 2024 11:35

bocekm marked this pull request as ready for review May 20, 2024 11:35

bocekm force-pushed the use-public-cdn-instead-of-ftp branch 4 times, most recently from 3f783af to 115278f Compare May 20, 2024 22:00

bocekm force-pushed the use-public-cdn-instead-of-ftp branch 3 times, most recently from fe9f4ff to 9faaaf0 Compare May 21, 2024 13:24

bocekm commented May 21, 2024

View reviewed changes

bocekm added the kind/feature New feature or request label May 22, 2024

bocekm force-pushed the use-public-cdn-instead-of-ftp branch 2 times, most recently from 487763d to 1284e29 Compare May 22, 2024 09:20

bocekm force-pushed the use-public-cdn-instead-of-ftp branch from 1284e29 to 85e86d9 Compare May 22, 2024 14:29

pr-watson approved these changes May 22, 2024

View reviewed changes

convert2rhel/actions/system_checks/convert2rhel_latest.py Outdated Show resolved Hide resolved

convert2rhel/actions/system_checks/convert2rhel_latest.py Outdated Show resolved Hide resolved

r0x0d force-pushed the use-public-cdn-instead-of-ftp branch from 85e86d9 to b4bb914 Compare May 22, 2024 17:51

bocekm commented May 23, 2024

View reviewed changes

hosekadam reviewed May 23, 2024

View reviewed changes

bocekm and others added 5 commits May 23, 2024 16:15

Download c2r repofile instead of generating it

31f5447

It's safer to use the official convert2rhel repofile that is to be used by customers instead of generating it as that way any changes done it don't need further convert2rhel code updates.

Apply suggestions from code review

d344dc0

Co-authored-by: Preston Watson <prwatson@redhat.com>

bocekm force-pushed the use-public-cdn-instead-of-ftp branch from 00d0e4a to 886e206 Compare May 23, 2024 14:15

danmyway approved these changes Jun 12, 2024

View reviewed changes

Venefilyn merged commit fb9b4e0 into oamg:main Jun 12, 2024
32 of 33 checks passed

This was referenced Aug 21, 2024

Bump to version 2.1.0 #1352

Merged

Bump to version 2.1.0 #1356

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RHELC-1434] Use public CDN instead of paywalled CDN and FTP #1123

[RHELC-1434] Use public CDN instead of paywalled CDN and FTP #1123

bocekm commented Mar 1, 2024 •

edited by Venefilyn

Loading

has-bot commented Mar 1, 2024

codecov bot commented Mar 1, 2024 •

edited

Loading

bocekm commented Mar 5, 2024

bocekm commented May 20, 2024

bookwar commented May 21, 2024

bocekm May 21, 2024

bocekm May 21, 2024

r0x0d May 21, 2024

bocekm May 21, 2024

bocekm May 21, 2024

bocekm commented May 22, 2024

pr-watson left a comment

r0x0d commented May 22, 2024

r0x0d commented May 22, 2024

bocekm May 23, 2024

r0x0d commented May 23, 2024

hosekadam May 23, 2024 •

edited

Loading

bocekm Jun 12, 2024

bocekm commented May 23, 2024

bocekm commented May 23, 2024

r0x0d commented May 23, 2024

r0x0d commented May 23, 2024

danmyway left a comment

	"Reason: %s" % (TMP_DIR, str(err)),
	diagnosis="Reason: %s" % (TMP_DIR, str(err)),

[RHELC-1434] Use public CDN instead of paywalled CDN and FTP #1123

[RHELC-1434] Use public CDN instead of paywalled CDN and FTP #1123

Conversation

bocekm commented Mar 1, 2024 • edited by Venefilyn Loading

Depends on

has-bot commented Mar 1, 2024

codecov bot commented Mar 1, 2024 • edited Loading

Codecov Report

bocekm commented Mar 5, 2024

bocekm commented May 20, 2024

bookwar commented May 21, 2024

bocekm May 21, 2024

Choose a reason for hiding this comment

bocekm May 21, 2024

Choose a reason for hiding this comment

r0x0d May 21, 2024

Choose a reason for hiding this comment

bocekm May 21, 2024

Choose a reason for hiding this comment

bocekm May 21, 2024

Choose a reason for hiding this comment

bocekm commented May 22, 2024

pr-watson left a comment

Choose a reason for hiding this comment

r0x0d commented May 22, 2024

r0x0d commented May 22, 2024

bocekm May 23, 2024

Choose a reason for hiding this comment

r0x0d commented May 23, 2024

hosekadam May 23, 2024 • edited Loading

Choose a reason for hiding this comment

bocekm Jun 12, 2024

Choose a reason for hiding this comment

bocekm commented May 23, 2024

bocekm commented May 23, 2024

r0x0d commented May 23, 2024

r0x0d commented May 23, 2024

danmyway left a comment

Choose a reason for hiding this comment

bocekm commented Mar 1, 2024 •

edited by Venefilyn

Loading

codecov bot commented Mar 1, 2024 •

edited

Loading

hosekadam May 23, 2024 •

edited

Loading