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

Allow specifying explicit kernel release and version for falco-driver-loader #2728

Merged
merged 1 commit into from
Aug 24, 2023
Merged

Allow specifying explicit kernel release and version for falco-driver-loader #2728

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 16 additions & 11 deletions scripts/falco-driver-loader
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ get_target_id() {
then
ARCH_extra="-${BASH_REMATCH[1]}${BASH_REMATCH[2]}"
fi
if [[ $(uname -v) =~ ([0-9]+\.[0-9]+\.[0-9]+\-[0-9]+) ]];
if [[ ${DRIVER_KERNEL_VERSION} =~ ([0-9]+\.[0-9]+\.[0-9]+\-[0-9]+) ]];
then
KERNEL_RELEASE="${BASH_REMATCH[1]}${ARCH_extra}"
fi
Expand All @@ -187,9 +187,9 @@ get_target_id() {
# from the following `uname -v` result
# `#26~22.04.1-Ubuntu SMP Mon Apr 24 01:58:15 UTC 2023`
# we obtain the kernelversion`26~22.04.1`
if [[ $(uname -v) =~ (^\#[0-9]+\~[^-]*-Ubuntu .*$) ]];
if [[ ${DRIVER_KERNEL_VERSION} =~ (^\#[0-9]+\~[^-]*-Ubuntu .*$) ]];
then
KERNEL_VERSION=$(uname -v | sed 's/#\([^-\\ ]*\).*/\1/g')
KERNEL_VERSION=$(echo "${DRIVER_KERNEL_VERSION}" | sed 's/#\([^-\\ ]*\).*/\1/g')
fi
;;
("flatcar")
Expand Down Expand Up @@ -258,7 +258,7 @@ flatcar_relocate_tools() {

load_kernel_module_compile() {
# Skip dkms on UEK hosts because it will always fail
if [[ $(uname -r) == *uek* ]]; then
if [[ ${DRIVER_KERNEL_RELEASE} == *uek* ]]; then
>&2 echo "Skipping because the dkms install always fail (on UEK hosts)"
return
fi
Expand All @@ -269,7 +269,7 @@ load_kernel_module_compile() {
fi

if [ "${TARGET_ID}" == "flatcar" ]; then
KERNEL_RELEASE=$(uname -r)
KERNEL_RELEASE=${DRIVER_KERNEL_RELEASE}
echo "* Flatcar detected (version ${VERSION_ID}); relocating kernel tools"
flatcar_relocate_tools
fi
Expand Down Expand Up @@ -522,7 +522,7 @@ load_bpf_probe_compile() {
MINIKUBE_VERSION="$(cat "${HOST_ROOT}/etc/VERSION")"
echo "* Minikube detected (${MINIKUBE_VERSION}), using linux kernel sources for minikube kernel"
local kernel_version
kernel_version=$(uname -r)
kernel_version=${DRIVER_KERNEL_RELEASE}
local -r kernel_version_major=$(echo "${kernel_version}" | cut -d. -f1)
local -r kernel_version_minor=$(echo "${kernel_version}" | cut -d. -f2)
local -r kernel_version_patch=$(echo "${kernel_version}" | cut -d. -f3)
Expand All @@ -535,9 +535,9 @@ load_bpf_probe_compile() {
fi

if [ -n "${BPF_USE_LOCAL_KERNEL_SOURCES}" ]; then
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

Copy link
Contributor Author

@johananl johananl Aug 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

KERNEL_RELEASE="${BASH_REMATCH[1]}${ARCH_extra}"

falco/scripts/falco-driver-loader

Line 196 in b2ad928

KERNEL_RELEASE="${VERSION_ID}"

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?

Copy link
Contributor Author

@johananl johananl Aug 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that makes sense to me.

local -r kernel_version=$(echo "${DRIVER_KERNEL_RELEASE}" | cut -d- -f1)
KERNEL_EXTRA_VERSION="-$(echo "${DRIVER_KERNEL_RELEASE}" | cut -d- -f2)"

echo "* Using downloaded kernel sources for kernel version ${kernel_version}..."

Expand Down Expand Up @@ -667,6 +667,8 @@ print_usage() {
echo " DRIVER_NAME specify a different name for the driver"
echo " DRIVER_INSECURE_DOWNLOAD whether you want to allow insecure downloads or not"
echo " DRIVER_CURL_OPTIONS specify additional options to be passed to curl command used to download Falco drivers"
echo " DRIVER_KERNEL_RELEASE specify the kernel release for which to download/build the driver in the same format used by 'uname -r' (e.g. '6.1.0-10-cloud-amd64')"
echo " DRIVER_KERNEL_VERSION specify the kernel version for which to download/build the driver in the same format used by 'uname -v' (e.g. '#1 SMP PREEMPT_DYNAMIC Debian 6.1.38-2 (2023-07-27)')"
echo ""
echo "Versions:"
echo " Falco version ${FALCO_VERSION}"
Expand All @@ -676,13 +678,16 @@ print_usage() {

ARCH=$(uname -m)

KERNEL_RELEASE=$(uname -r)
DRIVER_KERNEL_RELEASE=${DRIVER_KERNEL_RELEASE:-$(uname -r)}
KERNEL_RELEASE=${DRIVER_KERNEL_RELEASE}

if ! hash sed > /dev/null 2>&1; then
>&2 echo "This program requires sed"
exit 1
fi
KERNEL_VERSION=$(uname -v | sed 's/#\([[:digit:]]\+\).*/\1/')

DRIVER_KERNEL_VERSION=${DRIVER_KERNEL_VERSION:-$(uname -v)}
KERNEL_VERSION=$(echo "${DRIVER_KERNEL_VERSION}" | sed 's/#\([[:digit:]]\+\).*/\1/')

DRIVERS_REPO=${DRIVERS_REPO:-"@DRIVERS_REPO@"}

Expand Down