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

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

[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.

[happy-dude]

Hey team,

I just committed the RPM equivalent changes and this should be ready for review.

I tired my best to convert the deb-systemd-helper commands to straight systemctl equivalents. Additionally, I tried to incorporate RPM scriptlets where applicable. To understand the $1 value, please see the following references and table:

• https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_systemd
• https://docs.fedoraproject.org/en-US/packaging-guidelines/DefaultServices/
• https://wiki.networksecuritytoolkit.org/index.php/RPM_Quick_Reference#Systemd_Service_Control
• https://github.com/andrew-hardin/cpack-systemd-demo

	install	upgrade	uninstall
%pretrans	$1 == 1	$1 == 2	(N/A)
%pre	$1 == 1	$1 == 2	(N/A)
%post	$1 == 1	$1 == 2	(N/A)
%preun	(N/A)	$1 == 1	$1 == 0
%postun	(N/A)	$1 == 1	$1 == 0
%posttrans	$1 == 1	$1 == 1	(N/A)

I am not 100% confident that these are outright correct; I'm doing some rudimentary testing on a Fedora VM and would appreciate any extra validation and testing from fellow contributors 🙏

Let me know any questions or concerns and how I can facilitate better!

[leogr]

Hey @Happy-Dude

Is this PR still a WIP or ready to be reviewed?

[happy-dude]

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.

[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? :smile_cat:

[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 :rocket:

[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 😆

[poiana]

LGTM label has been added.

Git tree hash: 41a9557294ffa793e0ec462d0837281c83002429

[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