-
Notifications
You must be signed in to change notification settings - Fork 172
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(driver): add sanity check for kmod and ebpf configure systems #2283
base: master
Are you sure you want to change the base?
Conversation
Perf diff from master - unit tests
Heap diff from master - unit tests
Heap diff from master - scap file
Benchmarks diff from master
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2283 +/- ##
=======================================
Coverage 75.32% 75.32%
=======================================
Files 280 280
Lines 34556 34556
Branches 5902 5902
=======================================
Hits 26031 26031
Misses 8525 8525
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: i think we should enforce the sanity
check to be first one, right?
Also, i'd expect to see a line like:
[configure-bpf] Build system is sane
in the CI output (eg: https://github.com/falcosecurity/libs/actions/runs/13372344552/job/37343720272?pr=2283) but that's not the case.
Uhm, yes that was my original idea but then I forgot about it. It's not extremely important but it makes sense.
You're right, the code is broken in its current state (just a misplaced |
Signed-off-by: Gerlando Falauto <[email protected]>
6a530fb
to
5156cd8
Compare
Yay much better now :D |
Signed-off-by: Gerlando Falauto <[email protected]>
5156cd8
to
8a62188
Compare
Done! |
/milestone next-driver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
LGTM label has been added. Git tree hash: 6ec67338dd861b918cfe12af21691446393825c1
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: FedeDP, iurly 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?
/area driver-kmod
/area driver-bpf
Does this PR require a change in the driver versions?
What this PR does / why we need it:
Both kmod and legacy eBPF have a configure system to evaluate whether the kernel enables certain features introduced by a certain kernel release, mainly to catch RedHat backporting cutting edge features on “their stable” kernels.
The approach consists in trying to build a stub module and if it goes through, claim the feature is supported. However, the build might fail for completely unrelated reasons, leading to a wrong assumption which will then cause the actual build to fail with cryptic compile errors.
This PR introduces a one-code-fits-all configure check named __SANITY which does essentially nothing and is therefore guaranteed to compile on every kernel, provided the build environment is sane. When this check fails, we let the whole build fail fast.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: