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.

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

fix(pkg/driver): fixup kernel headers download #499

Merged
merged 2 commits into from
Apr 3, 2024

Conversation

FedeDP
Copy link
Contributor

@FedeDP FedeDP commented Mar 29, 2024

What type of PR is this?

/kind bug
/kind cleanup

Any specific area of the project related to this PR?

/area library

What this PR does / why we need it:

This PR fixes the templating of kernel headers download script from driverkit, that needs a fixed up kernelversion (only ubuntu uses it in driverkit, and it must be actually fixed, so that eg: #26~22.04.1 Ubuntu... becomes 26~22.04.1).

Moreover, it bumps driverkit to what will become v0.18.2 that contains a fix for new KERNELDIR mechanism (falcosecurity/driverkit#330).
wip until driverkit v0.18.2 is released.
Already in falcoctl main!

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

@FedeDP
Copy link
Contributor Author

FedeDP commented Mar 29, 2024

/milestone v0.8.0

@poiana poiana added this to the v0.8.0 milestone Mar 29, 2024
@poiana poiana added the size/L label Mar 29, 2024
out, err := exec.Command("bash", "-c", script).Output() //nolint:gosec // false positive
path := strings.TrimSuffix(string(out), "\n")
var path string
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -208,7 +220,12 @@ func Build(ctx context.Context,
// try to load kernel headers urls from driverkit.
if _, found := env[drivertype.KernelDirEnv]; !found {
printer.Logger.Debug("Trying to automatically fetch kernel headers.")
kernelHeadersPath, cleaner, err := loadKernelHeadersFromDk(d.String(), kr)
// KernelVersion needs to be fixed up; it is only used by driverkit Ubuntu builder
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@FedeDP
Copy link
Contributor Author

FedeDP commented Mar 29, 2024

Bumped to driverkit v0.18.2.

@FedeDP FedeDP changed the title wip: fix(pkg/driver): fixup kernel headers download fix(pkg/driver): fixup kernel headers download Mar 29, 2024
// KernelVersion needs to be fixed up; it is only used by driverkit Ubuntu builder
// and we must ensure that it is correctly set.
fixedKr := d.FixupKernel(kr)
kVerFixedKr := kr
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't overwrite kr here, instead use a copy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While this is a bit ugly rn, it will be much better once #356 lands.

…kit kernel download script template.

Signed-off-by: Federico Di Pierro <[email protected]>
@FedeDP FedeDP force-pushed the fix/fixup_kernel_headers_download branch from ed53932 to 70289f4 Compare April 2, 2024 07:11
@poiana poiana added size/S and removed size/L labels Apr 2, 2024
@poiana poiana added the lgtm label Apr 3, 2024
@poiana
Copy link
Contributor

poiana commented Apr 3, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, 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 0296002 into main Apr 3, 2024
17 checks passed
@poiana poiana deleted the fix/fixup_kernel_headers_download branch April 3, 2024 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants