cleanup(scripts): cleanup systemd unit in DEB + RPM packages

[happy-dude]

What type of PR is this?

/kind cleanup

Any specific area of the project related to this PR?

/area build

What this PR does / why we need it:
This PR builds off the work introduced in #1448, which converted Falco's initscripts to a systemd unit.

Currently, when removing, reinstalling, and/or upgrading the Falco package via DEB or RPM, the systemd unit file can be left in an unhygienic state and the user needs to perform a sudo systemctl daemon-reload.

This change re-introduces the falco service cleanup that the DEB and RPM packages perform for it's systemd unit when (re)installing and removing the packages.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

• This change is in WIP at the moment, as the RPM-equivalent changes have not been committed

Does this PR introduce a user-facing change?: Yes

update(scripts/debian): falco.service systemd unit is now cleaned-up during (re)install and removal via the DEB and RPM packages

[poiana]

Welcome @happy-dude! It looks like this is your first PR to falcosecurity/falco 🎉

[happy-dude]

Hey team, submitting my first PR to Falco.

I didn't file an issue tied to this PR primarily since I discovered this while trying to incorporate Falco into my company's build pipeline. The systemd service unit got into a weird state after installing various builds from source and these changes should help maintain/clean that up.

The changes to the DEB postinst/prerm/postrm scripts were primarily generated by debhelper. I believe a long-term fix would be to have debhelper generate + validate the debian contents, but that introduces another dependency and requires a few changes to the CPack config.

Additionally, there is not debhelper equivalent for Fedora / RPM as far as I know. I'm pretty confident that these can be done by translating what debhelper generated to their systemctl equivalents, but I'll need to build a RPM package to test on a Fedora VM. I can get to setting all that up sometime later this/next week.

Please let me know any of your thoughts or concerns and I'll do my best to address them!

[jasondellaluce]

Hi @happy-dude, thank you for the contribution! As first steps, I'd ask you to un-draft your PR and to sign-off your commit, so that all the checks can proceed.

[happy-dude]

Thanks Jason, I've

• changed the status of the PR from draft to ready for review
• amended my commit with the signed-off message

Appreciate the guidance!

[jasondellaluce]

/milestone 0.33.0

[jasondellaluce]

Isn't this the same if statement as above? Can we merge them?

[happy-dude]

I don't think this debhelper-generated block is functionally the same as the configure block that already exists;

I did a little reading here to find out more about how these maintainer-script conditions are called: https://www.debian.org/doc/debian-policy/ch-maintainerscripts.html#summary-of-ways-maintainer-scripts-are-called

• configure: all files and dependencies unpacked and configured
• abort-upgrade: all files unpacked, dependencies configured in a unexpected state, attempt to correct or configure again later -- I believe I have ran into situations like these when apt-get states that a package was not installed properly and continues to (attempt to) install it during subsequent apt-get runs. This has happened to with falco itself, although I'm cannot recall how I ended up in that situation (perhaps through installing the upstream package, my own compiled-from-source-package, and playing around with the systemd unit definition at various locations)
• abort-deconfigure: package still needs configuring, attempt to correct or configure later
• abort-remove: if package is an unexpected configured state, abort removal and keep it "installed"

Because of this, I don't think this is equivalent to the code above.
For the code above that is associated with installing/adding the kernel module to DKMS, it makes sense to associate these steps for after the falco package has been installed and configured properly. (The other states implies that there are problems during the installation/configuration of the package.)

Other than that, visually, I wanted to keep the auto-generated debhelper contents separate.

[jasondellaluce]

I'm not super familiar with how systemd works, but this looks like a very valuable improvement! Looking forward to hear other reviewers' opinions too.

[happy-dude]

Apologies for the delay in working on the RPM-equivalent series of changes.

I'll review https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_systemd to see if there are any tidbits; thus far, it seems like calling systemctl equivalents are sufficient.

[FedeDP]

Overall, this is a very nice improvement! Tiny yet super useful!
Thanks for the contribution, i really like it!

[leogr]

It should be ready for review! I'd like some extra testing from the Fedora/RHEL side of things from folks who run those distros regularly.

@dwindsor IIRC you are an RHEL user, right? 😸

[happy-dude]

A few of the situations I'd like to make sure are in an okay systemd situation:

• upgrade falco from old version to new version -- is service enabled/started and if the service is already running, does it refresh to the new version (condrestart) ?
• when uninstalling the new version, the falco service is "masked" -- does the service get "unmasked" when installing again?

Thanks for testing 🙏

[Andreagit97]

ei @happy-dude thank you for this! I will take a look in the next few days 🚀

[leogr]

Closing and reopening to trigger the CI

/close

[poiana]

@leogr: Closed this PR.

In response to this:

Closing and reopening to trigger the CI

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[leogr]

/reopen

[poiana]

@leogr: Reopened this PR.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[leogr]

/help

[FedeDP]

@happy-dude So, i finally made some tests:

Old behavior

Install:

• falco.service is not started neither enabled

Uninstall:

• falco.service is not masked neither daemon-reload is called, so that the systemd unit remains in a cached state
• if falco is uninstalled while falco is running, rmmod fails

New behavior

Install:

• falco.service is gracefully started and enabled, so that falco is immediately running (if either a driver could be downloaded from download.falco.org, or kernel-headers are installed and it is able to auto-build the driver)

Uninstall:

• falco.service unit is masked and daemon-reload is called; the systemd state is consistent
• if falco is uninstalled while falco is running, rmmod fails

All in all, this is a nice improvement!
Do you thing we can do anything about:

if falco is uninstalled while falco is running, rmmod fails

? Like, can we kill Falco (or stop the systemd unit) in the pre-rm scripts?
Thanks!

EDIT: uh, it seems you are already doing that:

if [ -d /run/systemd/system ] && [ "$1" = remove ]; then
        deb-systemd-invoke stop 'falco.service' >/dev/null || true
fi

and

if [ -d /run/systemd/system ] && [ $1 -eq 0 ]; then
        # stop falco service before uninstall
        /usr/bin/systemctl --system stop 'falco.service' >/dev/null || true
fi

weird, it has no effect :/
Manually stopping the unit works though. Perhaps the preuninstall scripts are run too late? It would be really strange.

EDIT2: @happy-dude i see: i think we should move the above checks/actions before calling /usr/bin/falco-driver-loader --clean :)

[happy-dude]

Thanks @FedeDP for the feedback! The made the changes accordingly and hope everything works as expected.

[FedeDP]

Thanks @happy-dude for the quick response even if we took this long to review :) i think this is now ready!
We are doing a little more tests but we expect this to get merged soon :) jit for Falco 0.33 😆

[FedeDP]

/approve

[poiana]

LGTM label has been added.

Git tree hash: 41a9557294ffa793e0ec462d0837281c83002429

[jasondellaluce]

/approve

[poiana]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, Happy-Dude, jasondellaluce

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [FedeDP,jasondellaluce]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment