[Policy] Consolidate check() across policies #3764

TurboTurtle · 2024-08-28T03:01:32Z

There is a common theme for new policies to subclass an existing one and just update the check() method. In almost all instances, the check() method remains almost entirely the same, simply changing a release file name or string.

Reduce repeating ourselves by consolidating this into the base LinuxPolicy.check(), and leverage per-policy class attributes os_release_file, os_release_name, and os_release_id. The first of which will enable a policy if the specified file is found. Failing that, os_release_name is used to check the NAME field in /etc/os-release, and os_release_id is used to check the ID field in the same file if the name pattern does not match (or is not set).

Note that we do not check the ID_LIKE field.

This allows a policy to be defined with as little as something like the following:

class MyPolicy(LinuxPolicy):
    distro = 'My Awesome Linux'
    vendor = 'Myself'
    os_release_file = '/etc/mylinux-release'
    os_release_name = 'MyLinux'
    os_release_id ='mynix'

Resolves: #1934

Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines

Is the commit message split over multiple lines and hard-wrapped at 72 characters?
Is the subject and message clear and concise?
Does the subject start with [plugin_name] if submitting a plugin patch or a [section_name] if part of the core sosreport code?
Does the commit contain a Signed-off-by: First Lastname [email protected]?
Are any related Issues or existing PRs properly referenced via a Closes (Issue) or Resolved (PR) line?
Are all passwords or private data gathered by this PR obfuscated?

TurboTurtle · 2024-08-28T03:02:30Z

I tested this on Fedora 39, RHEL 9, and Ubuntu 24.04 and the correct policy loaded on all of them. Additional sanity checks would be appreciated.

packit-as-a-service · 2024-08-28T03:04:27Z

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

sudo yum install -y dnf-plugins-core on RHEL 8
sudo dnf install -y dnf-plugins-core on Fedora
dnf copr enable packit/sosreport-sos-3764
And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

arif-ali · 2024-08-30T18:01:37Z

I've created a deb package specifically for this in Launchpad, to test this end-to-end on 24.04, and work as expected.

The manifist.json shows the policy Ubuntu being used.
The --upload worked as expected.

I'll create a few more packages for prior releases next week, and do further testing.

pmoravec · 2024-08-31T13:06:17Z

sos/policies/distros/debian.py

@@ -17,6 +15,7 @@ class DebianPolicy(LinuxPolicy):
    distro = "Debian"
    vendor = "the Debian project"
    vendor_urls = [('Community Website', 'https://www.debian.org/')]
+    os_release_file = '/etc/debian_version'


Shouldn't we also set os_release_name = 'Debian'? Otherwise the variable is nested from LinuxPolicy with '' value, and that can make troubles in def _check_release(content): method.

Anyway, see my more general question in PR.

In this scenario, we're only checking for the existence of the file. If that exists then the debian policy is used.

So, I think it makes sense, and we don't necessarily need the name.

Nevertheless, I can also check some debian variants too and check back here too

I think the lines below, that were added already tackle this in the same way as previously. As long as the file exists, then the Debian policy is activated.

# use the os-specific file primarily if os.path.isfile(cls.os_release_file): return True

Checking 4 releases of Debian, sid, trixie, bookworm and bullseye; they all have this file.

Alternatively, if we only want to use /etc/os-release, then we could have

os_release_id="debian" os_release_name="Debian GNU/Linux"

With the new package always going into sid and/or trixie, this will only really impact those OSs. The debian package is typically not back-ported to older releases. At least from what I can see based on my interaction these past 2 months

pmoravec · 2024-08-31T13:08:52Z

Why to distinguish distro and os_release_name, as we always check against both variables, and they are usually the same (I frown on the ambiguous Azure class :) )?

Isn't it easier to have os_release_names as a list (always), where the very first item plays the role of distro?

Upon to that, ACK from me and good idea.

pmoravec · 2024-08-31T13:37:55Z

sos/policies/distros/redhat.py

-                    if value.startswith(cls.distro):
-                        return True


We are changing this to direct values match, but that is imho correct. NAME should be "Red Hat Enterprise Linux Server".

pmoravec · 2024-08-31T13:46:27Z

sos/policies/distros/suse.py

@@ -62,6 +62,7 @@ class OpenSuSEPolicy(SuSEPolicy):
    distro = "OpenSuSE"
    vendor = "SuSE"
    vendor_urls = [('Community Website', 'https://www.opensuse.org/')]
+    os_release_file = '/etc/SUSE-brand'


Same here wrt missing os_release_name(s).

This is valid, if we're only looking for the location of the file as is, and no id/name

arif-ali · 2024-09-02T09:27:34Z

OK, this doesn't work for debian.

Ultimately, checking through the policies, it will try to check all. As the Ubuntu policy inherits the variables and such from Debian, this now suggests that the policy being used is Ubutu.

We may need to add the following to the UbuntuPolicy, at least this worked for me.

os_release_file = ''

arif-ali · 2024-09-02T10:56:21Z

I'll create a few more packages for prior releases next week, and do further testing.

Tested on focal (20.04), jammy (22.04) and noble (24.04), works as expected and policy is Ubuntu in the manifest

TurboTurtle · 2024-09-02T18:58:03Z

Why to distinguish distro and os_release_name, as we always check against both variables, and they are usually the same (I frown on the ambiguous Azure class :) )?

Isn't it easier to have os_release_names as a list (always), where the very first item plays the role of distro?

Upon to that, ACK from me and good idea.

There are a few places where we, intentionally or not, use distro as a shorthand reference for a particular distribution. We could very well drop distro I think with little churn. Let me poke around a bit more and if everything checks out like I'm thinking it will, I'll push an updated branch. Likely tomorrow as I am currently sitting in an airport.

There is a common theme for new policies to subclass an existing one and just update the `check()` method. In almost all instances, the `check()` method remains almost entirely the same, simply changing a release file name or string. Reduce repeating ourselves by consolidating this into the base `LinuxPolicy.check()`, and leverage per-policy class attributes `os_release_file`, `os_release_name`, and `os_release_id`. The first of which will enable a policy if the specified file is found. Failing that, `os_release_name` is used to check the `NAME` field in `/etc/os-release`, and `os_release_id` is used to check the `ID` field in the same file if the name pattern does not match (or is not set). Note that we do _not_ check the `ID_LIKE` field. This allows a policy to be defined with as little as something like the following: ``` class MyPolicy(LinuxPolicy): vendor = 'Myself' os_release_file = '/etc/mylinux-release' os_release_name = 'MyLinux' os_release_id ='mynix' ``` Resolves: sosreport#1934 Signed-off-by: Jake Hunsaker <[email protected]>

TurboTurtle · 2024-09-14T20:21:03Z

I've updated the branch to entirely remove distro, and replace it with references to os_release_name. I thought about allowing this to be a list, but the implementation gets murky in my opinion. The only policy that has/had more than one name in this context was AzureLinux, which carried the CBL Mariner name since that policy was actually previously renamed from the Marine policy. In this case I simply reinstated a CBL policy whose only difference is the os_release_name value.

We can certainly revisit this if we start getting a need for multiple policy names, but given the existing precedent it didn't seem necessary.

TurboTurtle · 2024-09-14T20:21:22Z

OK, this doesn't work for debian.

Ultimately, checking through the policies, it will try to check all. As the Ubuntu policy inherits the variables and such from Debian, this now suggests that the policy being used is Ubutu.

We may need to add the following to the UbuntuPolicy, at least this worked for me.
os_release_file = ''

Added in the latest push.

arif-ali · 2024-09-16T21:24:52Z

Built updated packages in the PPA again, will test in both Debian and Ubuntu this week sometime

arif-ali · 2024-09-17T18:45:36Z

Tested this on both Ubuntu and Debian, and manifest.json now looks good to me

TurboTurtle force-pushed the policy-consolidated-check branch from 325fbc7 to 9b6bff7 Compare August 28, 2024 03:03

pmoravec reviewed Aug 31, 2024

View reviewed changes

TurboTurtle force-pushed the policy-consolidated-check branch from 9b6bff7 to 8328aa5 Compare September 14, 2024 20:18

arif-ali approved these changes Sep 17, 2024

View reviewed changes

pmoravec approved these changes Sep 17, 2024

View reviewed changes

TurboTurtle merged commit a0035b6 into sosreport:main Sep 19, 2024
36 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Policy] Consolidate check() across policies #3764

[Policy] Consolidate check() across policies #3764

TurboTurtle commented Aug 28, 2024

TurboTurtle commented Aug 28, 2024

packit-as-a-service bot commented Aug 28, 2024

arif-ali commented Aug 30, 2024

pmoravec Aug 31, 2024

arif-ali Aug 31, 2024

arif-ali Sep 2, 2024

pmoravec commented Aug 31, 2024 •

edited

Loading

pmoravec Aug 31, 2024

pmoravec Aug 31, 2024

arif-ali Sep 2, 2024

arif-ali commented Sep 2, 2024

arif-ali commented Sep 2, 2024 •

edited

Loading

TurboTurtle commented Sep 2, 2024

TurboTurtle commented Sep 14, 2024

TurboTurtle commented Sep 14, 2024

arif-ali commented Sep 16, 2024

arif-ali commented Sep 17, 2024

[Policy] Consolidate check() across policies #3764

[Policy] Consolidate check() across policies #3764

Conversation

TurboTurtle commented Aug 28, 2024

TurboTurtle commented Aug 28, 2024

packit-as-a-service bot commented Aug 28, 2024

arif-ali commented Aug 30, 2024

pmoravec Aug 31, 2024

Choose a reason for hiding this comment

arif-ali Aug 31, 2024

Choose a reason for hiding this comment

arif-ali Sep 2, 2024

Choose a reason for hiding this comment

pmoravec commented Aug 31, 2024 • edited Loading

pmoravec Aug 31, 2024

Choose a reason for hiding this comment

pmoravec Aug 31, 2024

Choose a reason for hiding this comment

arif-ali Sep 2, 2024

Choose a reason for hiding this comment

arif-ali commented Sep 2, 2024

arif-ali commented Sep 2, 2024 • edited Loading

TurboTurtle commented Sep 2, 2024

TurboTurtle commented Sep 14, 2024

TurboTurtle commented Sep 14, 2024

arif-ali commented Sep 16, 2024

arif-ali commented Sep 17, 2024

pmoravec commented Aug 31, 2024 •

edited

Loading

arif-ali commented Sep 2, 2024 •

edited

Loading