Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: Drop cockpit-ws* groups #187

Merged
merged 1 commit into from
Dec 13, 2024
Merged

Conversation

martinpitt
Copy link
Collaborator

@martinpitt martinpitt commented Dec 12, 2024

Cockpit 330 (RHEL 9.6/10.0/Fedora 40) does not have any static system groups any more, everything is handled through DynamicUser=.

The ownership of the certificate hasn't mattered since Cockpit 257 [1], which is in RHEL 8.7, 9.0, and all current Fedora/Debian/Ubuntu OSes.

Setting a certificate group can be useful to share it with other services (like symlinking a global LetsEncrypt cert to ws-certs.d/), but this isn't what our documentation and tests do -- they produce a certificate exclusively for Cockpit.

Adjust the documentation and get_cockpit_group.yml to special-case the "cockpit-ws" group ownership for RHEL 7, similar to what other tests already do. Note that there was never any situation where the certificate needs to be owned by cockpit-wsinstance -- this was always meant to be an internal implementation detail.

[1] cockpit-project/cockpit@644116a0cd


This fixes the recent F41/C10 failures in #129. Draft because there are two issues to discuss:

  • I didn't update .README.html -- this looks autogenerated. How do I update it? -- @spetrosi : autogenerated on release

  • This won't work on RHEL/CentOS 7 (the test passes, but it doesn't check that cockpit's web server can actually read the cert). CentOS 7 is dead, so I propose to just drop that from CI. Do you still actually support RHEL 7 from main? If so, both README and the tests need to put back a conditional "if .el7 then group: cockpit-ws". Happy to do it, but is it actually worth it?

@martinpitt
Copy link
Collaborator Author

[citest]

@martinpitt
Copy link
Collaborator Author

@richm Does this install failure ring a bell? That's unrelated to the group change.

"No packages to remove for argument: cockpit-doc",

"Problem: problem with installed package\n - installed package cockpit-system-330-1.fc41.noarch requires cockpit-bridge >= 330-1.fc41, but none of the providers can be installed\n - package cockpit-system-326-1.fc41.noarch from fedora requires cockpit-bridge >= 326-1.fc41, but none of the providers can be installed\n - package cockpit-system-330-1.fc41.noarch from updates requires cockpit-bridge >= 330-1.fc41, but none of the providers can be installed\n - conflicting requests"

I don't know what this wants to tell me, but either dnf or the compose is confused here? Clearly both cockpit-system adn cockpit-bridge 330 are available.. 🤔

@spetrosi
Copy link
Collaborator

I didn't update .README.html -- this looks autogenerated. How do I update it?

Right, it is generated automatically on repository release

Does this install failure ring a bell?

Weird, it cannot remove package due to some conflicts. Maybe a bug in new Fedora?

@richm
Copy link
Contributor

richm commented Dec 12, 2024

Looks like some sort of issue with fedora-41 - new dnf - not related to the PR
@martinpitt pr lgtm - do you want to mark it as Ready for review?

@martinpitt
Copy link
Collaborator Author

@richm I think it'd be a bit premature, or at least unclean, to land it in its current form. Do you mind having a look at the RHEL 7 question in the description? Thanks!

@richm
Copy link
Contributor

richm commented Dec 12, 2024

@martinpitt We need to keep testing on el7, so the test should work on el7. Yes, centos 7 is dead, but rhel 7 is still in ELS for some time https://www.redhat.com/en/technologies/linux-platforms/enterprise-linux/rhel-7-end-of-maintenance - so either we keep testing on centos-7, or we somehow figure out how to test with an actual rhel 7.

@martinpitt
Copy link
Collaborator Author

@richm ok! I'll look into that tomorrow then

@richm
Copy link
Contributor

richm commented Dec 12, 2024

Alternate fix - #188

@martinpitt
Copy link
Collaborator Author

[citest]

@martinpitt
Copy link
Collaborator Author

@richm OK, I reworked this to keep the RHEL 7 special case (more precisely, by checking the cockpit package version -- we already do that in several other places).

@martinpitt
Copy link
Collaborator Author

It's unhappy with the version check, so I'll try to check the distribution instead.

Cockpit 330 (RHEL 9.6/10.0/Fedora 40) does not have any static system
groups any more, everything is handled through `DynamicUser=`.

The ownership of the certificate hasn't mattered since Cockpit 257 [1],
which is in RHEL 8.7, 9.0, and all current Fedora/Debian/Ubuntu OSes.

Setting a certificate group *can* be useful to share it with other
services (like symlinking a global LetsEncrypt cert to ws-certs.d/), but
this isn't what our documentation and tests do -- they produce a
certificate exclusively for Cockpit.

Adjust the documentation and get_cockpit_group.yml to special-case the
"cockpit-ws" group ownership for RHEL 7. similar to what other tests
already do. Note that there was *never* any situation where the
certificate needs to be owned by `cockpit-wsinstance` -- this was always
meant to be an internal implementation detail.

[1] cockpit-project/cockpit@644116a0cd
@martinpitt
Copy link
Collaborator Author

[citest]

@martinpitt martinpitt marked this pull request as ready for review December 13, 2024 05:43
@martinpitt martinpitt requested a review from richm December 13, 2024 05:43
@richm richm merged commit 5bd06ff into linux-system-roles:main Dec 13, 2024
17 of 18 checks passed
@martinpitt martinpitt deleted the group branch December 13, 2024 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants