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.

Sign up for GitHub

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

Jump to bottom

falco-driver-loader improvements (custom driver name + clean previous drivers functionality) #1488

Merged
merged 6 commits into from
Mar 26, 2021
Merged

falco-driver-loader improvements (custom driver name + clean previous drivers functionality) #1488

merged 6 commits into from
Mar 26, 2021

Conversation

leodido
Copy link
Member

@leodido leodido commented Nov 16, 2020 •
edited
Loading

What type of PR is this?

/kind documentation

/kind feature

Any specific area of the project related to this PR?

NONE

What this PR does / why we need it:

It lets users specify a custom DRIVER_NAME getting it from the environment. It can be useful when the user has a Falco kernel-module with a different name (eg., falco_probe) from the current default one (ie., falco).

This way (DRIVER_NAME=falco_probe falco-driver-loader module) it will look for a kernel module (via lsmod) named falco_probe and remove it (rmmod).

Furthermore, the module should also be removed from dkms. WIP for this reason.

Last but not least, for debugging and clarity purposes, this PR make the falco-driver-loader script aware of the Falco version it has been built for.

And outputs it at:

  • runtime time
  • help/usage time

Which issue(s) this PR fixes:

Fixes #1486

Special notes for your reviewer:

/cc @leogr
/cc @fntlnz

Does this PR introduce a user-facing change?:

new: falco-driver-loader script can get a custom driver name from DRIVER_NAME env variable
new: falco-driver-loader know the Falco version

@poiana poiana requested review from fntlnz and leogr November 16, 2020 12:07
@leodido
Copy link
Member Author

leodido commented Nov 16, 2020

/milestone 0.27.0

@poiana poiana added this to the 0.27.0 milestone Nov 16, 2020
@leogr
Copy link
Member

leogr commented Nov 16, 2020 •
edited
Loading

Thank you for this PR. We definitely need that 👍

Although specifying a custom DRIVER_NAME can be useful for various reasons, I believe it's not the best way to clean up old drivers since:

  • the script will then try to insmod/modprobe with the given DRIVER_NAME
  • the script does not have action only to remove the driver

Thus, as discussed privately, I believe it worth adds just a specific cleanup command for the old falco_probe that we don't use anymore. I will investigate on that.

@leogr leogr mentioned this pull request Nov 16, 2020
Closed
@leodido
Copy link
Member Author

leodido commented Nov 16, 2020 •
edited
Loading

Yep, that's the reason this PR is WIP @leogr :)

As discussed privately, we probably need to add 3rd action for the script (eg., falco-driver-loader clean)

@leogr
Copy link
Member

leogr commented Nov 16, 2020 •
edited
Loading

Yep, that's the reason this PR is WIP @leogr :)

As discussed privately, we probably need to add 3rd action to the script (eg., falco-driver-loader clean)

That's ok for me too. But, do we still want a simple clean-up for falco_probe that runs always (I mean, before installing the kernel module)?

@leodido
Copy link
Member Author

leodido commented Nov 17, 2020 •
edited
Loading

That's up to us to decide.

My personal vote is to leave the behaviour as it currently is and just add a clean command to the script to be run for special use cases - ie., user want to remove a old/different kernel module (falco_probe.ko, even from dkms, not only rmmodding it) but then wants to install a new one (eg., falco.ko)

@leogr
Copy link
Member

leogr commented Nov 17, 2020

Ok, let's do this step-by-step.

So I can work on the clean action since I believe we all agree on that.

@poiana poiana added size/L and removed size/S labels Nov 17, 2020
@leodido
Copy link
Member Author

leodido commented Nov 17, 2020 •
edited
Loading

Yes, we all agree on that. As also discussed privately. :)

Then, we could also think about how to make the upgrade process straightforward even if the driver name changes again.
And, given that a new kernel module name change is very unlikely: if it's worth.

@leogr
Copy link
Member

leogr commented Nov 17, 2020

Yes, we all agree on that. As also discussed privately. :)

Done by aa892e4 :)
It's the first draft anyway, so PTAL.

Then, we could also think about how to make the upgrade process straightforward even if the driver name changes again.
And, given that a new kernel module name change is very unlikely: if it's worth.

👍

@fntlnz fntlnz modified the milestones: 0.27.0, 0.28.0 Jan 15, 2021
leodido and others added 6 commits March 26, 2021 11:44
@leodido @leogr
update(scripts): falco-driver-loader can now start with a custom driv…
38f2053 
…er name

Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
@leodido @leogr
docs(scripts): improve help of falco-driver-loader script
17928b3 
Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
@leodido @leogr
docs(scripts): falco-driver-loader outputs the Falco version it has b…
42aa018 
…een built for, also the driver version in use

Both in the help/usage message and at running time.

Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
@leodido @fntlnz @leogr
new(scripts): falco-driver-loader know the Falco version it has been
e054deb 
built for

Co-authored-by: Lorenzo Fontana <lo@linux.com>
Signed-off-by: Leonardo Di Donato <leodidonato@gmail.com>
@leogr
feat(scripts): --clean option for falco-driver-loader
1142014 
Signed-off-by: Leonardo Grasso <me@leonardograsso.com>
@leogr @leodido
chore(scripts): print versions at the beginning
abd040e 
Co-authored-by: Leonardo Di Donato <leodidonato@gmail.com>
Signed-off-by: Leonardo Grasso <me@leonardograsso.com>
@leogr leogr force-pushed the update/falco-driver-loader-driver-name branch from 0fb8f4a to abd040e Compare March 26, 2021 10:53
@poiana poiana added the lgtm label Mar 26, 2021
@poiana
Copy link
Contributor

poiana commented Mar 26, 2021

LGTM label has been added.

Git tree hash: c7883d2b3b5513cff0688e4579695580d2e57cb7

@leodido leodido changed the title wip: falco-driver-loader improvements falco-driver-loader improvements (custom driver name + clean previous drivers functionality) Mar 26, 2021
@poiana
Copy link
Contributor

poiana commented Mar 26, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fntlnz, leogr

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:

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

@poiana poiana merged commit ef75c63 into master Mar 26, 2021
@poiana poiana deleted the update/falco-driver-loader-driver-name branch March 26, 2021 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leogr leogr leogr approved these changes

@fntlnz fntlnz fntlnz approved these changes

Assignees

@fntlnz fntlnz

@leogr leogr

Labels
approved dco-signoff: yes kind/documentation kind/feature lgtm release-note size/L
Projects
None yet
Milestone
0.28.0
Development

Successfully merging this pull request may close these issues.

Since the Falco dirver rename, Falco is unable to unload the falco_probe module
4 participants
@leodido @leogr @poiana @fntlnz