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

new(pkg): use driverkit local build processor instead of implementing drivers build #356

Merged
merged 6 commits into from
May 8, 2024

Conversation

FedeDP
Copy link
Contributor

@FedeDP FedeDP commented Nov 24, 2023

What type of PR is this?

/kind feature

Any specific area of the project related to this PR?

/area library

What this PR does / why we need it:

Driverkit v0.16.0 added a new local build processor: falcosecurity/driverkit#306.
Making use of it, we can drop all the custom logic for drivers build.

Which issue(s) this PR fixes:

Refs #327

Fixes #

Special notes for your reviewer:

TODO:

Also, most of the lines "added" are from go.sum.

@FedeDP
Copy link
Contributor Author

FedeDP commented Nov 24, 2023

Note: pulling in latest driverkit breaks build because of the usage of:

Error: ../../../go/pkg/mod/github.com/falcosecurity/driverkit@v0.16.0/cmd/local.go:90:13: undefined: unix.Utsname
Error: ../../../go/pkg/mod/github.com/falcosecurity/driverkit@v0.16.0/cmd/local.go:91:18: undefined: unix.Uname

in it for non-linux builds.
I am not sure whether to drop the dep in driverkit (enforcing user to pass kernelrelease/kernelversion even for local builds (it might make sense, after all the 2 parameteres are already mandatory for all others command) or to try something else (don't know what though!)

@FedeDP
Copy link
Contributor Author

FedeDP commented Nov 30, 2023

I am not sure whether to drop the dep in driverkit (enforcing user to pass kernelrelease/kernelversion even for local builds

Chose to follow this way: falcosecurity/driverkit#307

EDIT: bumped driverkit to that PR.

@FedeDP FedeDP force-pushed the new/use_driverkit_build branch 2 times, most recently from b47c8f2 to 770dc86 Compare November 30, 2023 10:08
@FedeDP
Copy link
Contributor Author

FedeDP commented Dec 1, 2023

/reopen
Ops :D

@poiana poiana reopened this Dec 1, 2023
@poiana
Copy link
Contributor

poiana commented Dec 1, 2023

@FedeDP: Reopened this PR.

In response to this:

/reopen
Ops :D

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
Copy link
Member

leogr commented Dec 18, 2023

/milestone v0.8.0

@poiana poiana added this to the v0.8.0 milestone Dec 18, 2023
@FedeDP FedeDP force-pushed the new/use_driverkit_build branch from 770dc86 to 4c20d62 Compare February 14, 2024 09:29
@poiana poiana added size/XXL and removed size/XL labels Feb 14, 2024
@FedeDP
Copy link
Contributor Author

FedeDP commented Feb 14, 2024

Rebased on top of master; moreover:

Only question remaining: driverkit uses log/slog package for structured logging (avail since golang 1.21) and internally logs multiple things.
This breaks our spinner behavior. Should we:

  • either disable driverkit logging (eg: by calling:
h := slog.NewTextHandler(io.Discard, &slog.HandlerOptions{})
slog.SetDefault(slog.New(h))

ie: by setting an io discarder slog as default slog instance that gets inherited by driverkit.

  • or by passing our printer.Logger.Writer and printer.Logger.Level to the slog instance, like:
h := slog.NewTextHandler(printer.Logger.Writer, &slog.HandlerOptions{
		Level: printer.Logger.Level,
		ReplaceAttr: func(groups []string, a slog.Attr) slog.Attr {
			if a.Key == slog.TimeKey {
				return slog.Attr{}
			}
			return a
		}})
slog.SetDefault(slog.New(h))

this way, the slog used by driverkit will print on our provided writer (without datetime attribute because we don't want that).

@FedeDP FedeDP force-pushed the new/use_driverkit_build branch 3 times, most recently from 2031ef5 to c7fd29a Compare February 14, 2024 10:29
@FedeDP
Copy link
Contributor Author

FedeDP commented Feb 29, 2024

Another TODO:

  • when build fails, we need some way to get the error, like accessing /var/lib/dkms... log. This is what we currently do in falcoctl main.

@FedeDP
Copy link
Contributor Author

FedeDP commented Mar 25, 2024

I will bump this once:

are merged; also i need to make a new release for driverkit once falcosecurity/driverkit#324 is merged.

@FedeDP FedeDP changed the title wip: new(pkg): use new driverkit v0.16.0 local build processor instead of implementing drivers build wip: new(pkg): use driverkit local build processor instead of implementing drivers build Mar 29, 2024
@FedeDP FedeDP force-pushed the new/use_driverkit_build branch 2 times, most recently from c705878 to 3677f09 Compare April 4, 2024 09:21
@FedeDP FedeDP force-pushed the new/use_driverkit_build branch from 3677f09 to 9f82dae Compare May 7, 2024 07:16
@poiana poiana added size/XXL and removed size/XL labels May 7, 2024
@FedeDP
Copy link
Contributor Author

FedeDP commented May 7, 2024

Bumped to driverkit v0.19.0.
There is a build issue since pkg/driver/distro imports driverkit/cmd that imports pkg/options : https://github.com/falcosecurity/driverkit/blob/master/cmd/config_options.go#L20 and that is an import cycle.
Gonna work on this asap.

@FedeDP
Copy link
Contributor Author

FedeDP commented May 7, 2024

#550 is the first step to fix the build; we will then need to port the changes to falcosecurity/driverkit#342 and tag driverkit v0.19.1. We should be finally fine then.

FedeDP added 3 commits May 7, 2024 11:26
…der logic.

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
@FedeDP FedeDP force-pushed the new/use_driverkit_build branch from 9f82dae to 8d57c21 Compare May 7, 2024 09:26
FedeDP added 2 commits May 7, 2024 11:27
This fixes the build.

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
@FedeDP FedeDP force-pushed the new/use_driverkit_build branch from 1e3fac8 to f65d4bd Compare May 8, 2024 07:04
@FedeDP
Copy link
Contributor Author

FedeDP commented May 8, 2024

Ok driverkit v0.19.1 was tagged; the CI should now be green here, and we can proceed.
@alacuku @leogr it would be great if we could merge this before Monday since otherwise i will have to fix all go.{mod,sum} conflicts 😟

@FedeDP FedeDP changed the title wip: new(pkg): use driverkit local build processor instead of implementing drivers build new(pkg): use driverkit local build processor instead of implementing drivers build May 8, 2024
Copy link
Member

@alacuku alacuku left a comment

Choose a reason for hiding this comment

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

/lgtm

I left a suggestion.

pkg/driver/distro/distro.go Outdated Show resolved Hide resolved
This was needed because of a cross-deps build issue because of versions mismatches.

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
@FedeDP FedeDP force-pushed the new/use_driverkit_build branch from 86c801d to 90c4dbf Compare May 8, 2024 13:25
Copy link
Member

@alacuku alacuku left a comment

Choose a reason for hiding this comment

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

/lgtm

@poiana
Copy link
Contributor

poiana commented May 8, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alacuku, FedeDP

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 d0523ca into main May 8, 2024
17 checks passed
@poiana poiana deleted the new/use_driverkit_build branch May 8, 2024 13:51
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.

4 participants