Allow specifying explicit kernel release and version for falco-driver-loader
#2728
Conversation
Signed-off-by: Johanan Liebermann <jliebermann@microsoft.com>
|
Welcome @johananl! It looks like this is your first PR to falcosecurity/falco 🎉 |
|
/milestone 0.36.0 |
|
LGTM label has been added. Git tree hash: 15c8df01fe85054ab9832eda5ee00556b1333b84
|
| local -r kernel_version_major=$(uname -r | cut -d. -f1) | ||
| local -r kernel_version=$(uname -r | cut -d- -f1) | ||
| KERNEL_EXTRA_VERSION="-$(uname -r | cut -d- -f2)" | ||
| local -r kernel_version_major=$(echo "${DRIVER_KERNEL_RELEASE}" | cut -d. -f1) |
There was a problem hiding this comment.
Wouldn't it be better to use KERNEL_RELEASE here?
I mean, i think we should use KERNEL_RELEASE and KERNEL_VERSION everywhere, not their DRIVER_ counterparts.
There was a problem hiding this comment.
The places where I used e.g. DRIVER_KERNEL_RELEASE are places where there was an explicit call to uname -r. Note that the values of KERNEL_RELEASE and KERNEL_VERSION change throughout the script, which can be tricky to follow as there is lots of distro-specific logic and special cases.
In this case, for example, KERNEL_RELEASE may not hold the same value as uname -r at this point in the execution, because we do various assignments to this variable earlier:
falco/scripts/falco-driver-loader
Line 168 in b2ad928
falco/scripts/falco-driver-loader
Line 196 in b2ad928
This fact is likely the reason uname -r was called here instead of reusing KERNEL_RELEASE.
My idea was to keep existing functionality as is by storing the result of uname -r and uname -v once at the start of the execution (in DRIVER_KERNEL_RELEASE and DRIVER_KERNEL_VERSION, respectively), then reuse these values. This allows specifying explicit release and version values as arguments without altering the entire script.
Does that make sense? Did I miss anything?
There was a problem hiding this comment.
FWIW, I think we can make the script easier to maintain by making it more "functional" (as in functional programming) by passing explicit values to functions rather than relying on global variables.
There was a problem hiding this comment.
Oh, I see. It makes sense to me now.
This also adheres to our implicit convention that the DRIVER_ prefix represents input values (those that may be set using env vars), whereas other vars like KERNEL_RELEASE are just for internal use.
👍
Thank you!
N.B.: I've checked the whole PR. Just giving my 2 cents (specifically related to the above comment).
There was a problem hiding this comment.
Does that make sense? Did I miss anything?
I see! Thank you for the explanation. I think you are right!
Also, i share the same though about making the script more functional. At the same time, we don't probably want to spend much time on this since sooner or later we are going to revamp the script porting it to eg: a go tool (perhaps inside falcoctl?)
There was a problem hiding this comment.
Yes, that makes sense to me.
|
Forgot to mention that I tested my changes by executing the script manually in both modes (kmod and BPF) with default values as well as with explicit kernel release and version on the following distros:
If we refactor the script to be functional, we could use something like https://github.com/bats-core/bats-core to unit-test the file. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Andreagit97, FedeDP, johananl 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 |
What type of PR is this?
/kind feature
Any specific area of the project related to this PR?
What this PR does / why we need it:
This PR adds the ability to specify explicit kernel release and version when running
falco-driver-loader. This is important in cases where the Falco driver needs to be compiled for a kernel other than the currently-running kernel (such as when the kernel had been updated but not yet booted into).Which issue(s) this PR fixes:
Fixes #2658
Special notes for your reviewer:
None
Does this PR introduce a user-facing change?: