tools: Dynamically manage motd/issue symlinks in package scripts #15784

martinpitt · 2021-05-04T07:52:07Z

Users commonly want to just remove /etc/motd.d/cockpit or
/etc/issue.d/cockpit.issue, instead of symlinking them to /dev/null.
However, rpms' %config handling does not respect that, and brings back
the files on upgrade. This also raises an alarm in rpm --verify.

Likewise, dpkg's conffile logic only handles plain file content changes
or removals, not symlinks.

Stop packaging the symlinks, and create/remove them in the package
scrips instead.

https://bugzilla.redhat.com/show_bug.cgi?id=1876848

martinpitt · 2021-05-04T09:25:02Z

Oops, on RHEL subscription-manager-cockpit depends on cockpit-ws. It should not do that (I'll file a PR), but we can simply work around that in our tests.

marusak · 2021-05-05T06:35:27Z

test/verify/check-connection

+            if m.image.startswith("debian") or m.image.startswith("ubuntu"):
+                m.execute("dpkg --install /var/tmp/build-results/cockpit-ws_*.deb")
+            else:
+                # FIXME: current package complains about mode change on /run/cockpit and selinux


What is it complaining about? Is this related to these changes?

No, unrelated:

# rpm -V cockpit-ws .M....... g /run/cockpit .M....... g /var/lib/selinux/targeted/active/modules/200/cockpit

The first one has been there forever (/run/cockpit is just a %ghost), and I figure the other one was introduced with adopting our SELinux policy. We should fix these, of course, but for now this makes sure that we don't introduce other complains than 'M'ode changes.

@lnussel : You added this %ghost %dir /run/cockpit in PR #13826. What was the purpose of that? It breaks rpm -V validation, and this should really not be a directory that rpm should be concerned about. It's managed by systemd and cockpit's units entirely, not by rpm. Would it be ok to drop that again?

Well you could remove it, it's not mandatory. But shouldn't hurt either. For now it mostly serves the purpose you see there, ie rpm -V. Why is there a disagreement about the mode of the directory?

I have no idea.. On the actual system it's just standard root:root 0755. But as the rpm does not actually ship that dir, and %ghost %dir does not declare any explicit mode, I'm not sure what the expected mode even is.. But maybe it's not really about the mode, but it's complaining about the "g"?

g %ghost file (i.e. the file contents are not included in the package payload).

then it seems pretty stupid to complain about this, because not shipping a file in the package is exactly what %ghost is for..

The rpm does include a mode for the ghost file as it's actually created in %install. So from what I can tell it looks like doing the right thing here.

# rpm -qvl cockpit-ws|grep /run/cockpit$ drwxr-xr-x 2 root root 0 Apr 22 06:57 /run/cockpit # rpm -Vv cockpit-ws|grep /run/cockpit$ ......... g /run/cockpit # chmod 700 /run/cockpit # rpm -V cockpit-ws .M....... g /run/cockpit # rpm --restore cockpit-ws # l -d /run/cockpit drwxr-xr-x 2 root root 100 7. Mai 05:33 /run/cockpit/ # rpm -V cockpit #

I just checked the .spec again, and indeed that bit happens only on SUSE:

# need this in SUSE as post build checks dislike stale symlinks install -m 644 -D /dev/null %{buildroot}/run/cockpit/motd

So I suppose I'll conditionalize the %ghost as well, as the Fedora/RHEL packages don't ship bits in /run (and they really really shouldn't! Packaging /run makes me cringe. Are you sure you want that in OpenSUSE?)

I believe this OpenSUSE-specific install hack should be obsolete since commit 5705709 . I'll send a PR after landing my rpm -V fix, and ask @lnussel for confirmation.

%ghost in /run doesn't really package anything :-) Anyway feel free to remove, I have to maintain some openSUSE specific patches anyway (license, permission macro etc).
Installing links to /run in /etc makes me cringe btw :-) agetty, login, pam etc do support reading motd and issue from /run. Looks like just openssh doesn't.

Right, but it keeps information about the expected modes/properties of the ghosted file/dir. Full ack about the installing links to /etc.. We still have some distros to support which don't yet support reading from /run/motd.d/ directly (that's fixed in most recent PAM, though). One of these days this will go away.

marusak

Thanks!

martinpitt · 2021-05-05T08:07:17Z

@KKoukiou Do you still want to review this one also? (happy to explain packaging details, if you want to learn)

KKoukiou · 2021-05-05T08:08:31Z

@KKoukiou Do you still want to review this one also? (happy to explain packaging details, if you want to learn)

Yes, will do it right now. Matej was very fast :)

martinpitt · 2021-05-05T08:18:47Z

I sent candlepin/subscription-manager#2617 to drop the cockpit-ws dependency from subscription-manager-cockpit.

KKoukiou · 2021-05-05T08:19:36Z

tools/cockpit.spec

@@ -551,6 +559,11 @@ if %{_sbindir}/selinuxenabled 2>/dev/null; then
 fi
 %endif
 %systemd_postun_with_restart cockpit.socket cockpit.service
+# clean up dynamic motd/issue symlinks on removal


Is this really needed? I read in http://ftp.rpm.org/max-rpm/s1-rpm-inside-files-list-directives.html that ghost files are also removed when the package is.

Excellent point -- I'll test this! (easy enough with the integration tests now). If it does, I'll drop that bit. Setting needswork for now.

Users commonly want to just remove /etc/motd.d/cockpit or /etc/issue.d/cockpit.issue, instead of symlinking them to /dev/null. However, rpms' %config handling does not respect that, and brings back the files on upgrade. This also raises an alarm in `rpm --verify`. Likewise, dpkg's conffile logic only handles plain file content changes or removals, not symlinks. Stop packaging the symlinks, and create/remove them in the package scrips instead. (Removal with rpm is automatic due to the `%ghost`). https://bugzilla.redhat.com/show_bug.cgi?id=1876848

martinpitt · 2021-05-05T14:45:46Z

Nice! I locally tested this on Fedora 34, and it does clean up the links on removal without the %postrm stuff. Pushed, let's validate it everywhere.

martinpitt · 2021-05-05T16:27:27Z

unrelated flake; retrying, I'll look at that separately tomorrow.

martinpitt · 2021-05-05T17:44:43Z

Doesn't want to budge -- I suppose this is related to switching TEST_OS_DEFAULT to f34 today, but firefox test is still f33? So check-shell-host-switching does not disable preloads on the auxiliary machines any more, which changed the timing.

martinpitt · 2021-05-05T17:53:25Z

I added some more fixes which will hopefully fix the above failure.

Add the missing "playground" disabling.

Many test cases in this class remove/edit/disconnect remote machines, which can cause unexpected messages due to asynchronous disconnects from preloads (which are in use on OSes other than TEST_OS_DEFAULT). testEdit() forgot to call allow_restart_journal_messages(). Just move them into setUp().

martinpitt · 2021-05-06T05:43:53Z

Whee, seems I tamed check-shell-host-switching at last 🥳

marusak

Good call with removing of ghosts files, thanks :)

KKoukiou self-requested a review May 4, 2021 07:54

martinpitt force-pushed the rpm-motd-symlinks branch from 7dba87c to f3e9f86 Compare May 4, 2021 09:25

martinpitt requested a review from marusak May 5, 2021 06:04

marusak reviewed May 5, 2021

View reviewed changes

marusak previously approved these changes May 5, 2021

View reviewed changes

martinpitt added the release-blocker Targetted for next release label May 5, 2021

KKoukiou reviewed May 5, 2021

View reviewed changes

martinpitt added the needswork label May 5, 2021

marusak mentioned this pull request May 5, 2021

Activate the web console login message does not disappear #15789

Closed

martinpitt removed the needswork label May 5, 2021

martinpitt dismissed marusak’s stale review via e597589 May 5, 2021 14:44

martinpitt force-pushed the rpm-motd-symlinks branch from f3e9f86 to e597589 Compare May 5, 2021 14:44

martinpitt requested a review from KKoukiou May 5, 2021 15:48

KKoukiou previously approved these changes May 5, 2021

View reviewed changes

martinpitt dismissed KKoukiou’s stale review via 9861d18 May 5, 2021 17:53

martinpitt added 2 commits May 5, 2021 20:04

test: Factorize preload disabling in TestHostSwitching

46a7e02

Add the missing "playground" disabling.

martinpitt force-pushed the rpm-motd-symlinks branch from 9861d18 to 1490321 Compare May 5, 2021 18:04

martinpitt requested review from marusak and KKoukiou May 6, 2021 05:43

marusak approved these changes May 6, 2021

View reviewed changes

martinpitt merged commit 25608ab into cockpit-project:master May 6, 2021

martinpitt deleted the rpm-motd-symlinks branch May 6, 2021 06:12

This was referenced May 10, 2021

Fix rpm validation errors #15807

Merged

tools: Drop obsolete OpenSUSE specific /run ghosts #15817

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools: Dynamically manage motd/issue symlinks in package scripts #15784

tools: Dynamically manage motd/issue symlinks in package scripts #15784

martinpitt commented May 4, 2021

martinpitt commented May 4, 2021

marusak May 5, 2021

martinpitt May 5, 2021

martinpitt May 5, 2021

lnussel May 6, 2021 •

edited

martinpitt May 6, 2021

lnussel May 7, 2021

martinpitt May 10, 2021

martinpitt May 10, 2021

lnussel May 10, 2021

martinpitt May 10, 2021

marusak left a comment

martinpitt commented May 5, 2021

KKoukiou commented May 5, 2021

martinpitt commented May 5, 2021

KKoukiou May 5, 2021

martinpitt May 5, 2021

martinpitt commented May 5, 2021

martinpitt commented May 5, 2021

martinpitt commented May 5, 2021

martinpitt commented May 5, 2021

martinpitt commented May 6, 2021

marusak left a comment

tools: Dynamically manage motd/issue symlinks in package scripts #15784

tools: Dynamically manage motd/issue symlinks in package scripts #15784

Conversation

martinpitt commented May 4, 2021

martinpitt commented May 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lnussel May 6, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marusak left a comment

Choose a reason for hiding this comment

martinpitt commented May 5, 2021

KKoukiou commented May 5, 2021

martinpitt commented May 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpitt commented May 5, 2021

martinpitt commented May 5, 2021

martinpitt commented May 5, 2021

martinpitt commented May 5, 2021

martinpitt commented May 6, 2021

marusak left a comment

Choose a reason for hiding this comment

lnussel May 6, 2021 •

edited