-
Notifications
You must be signed in to change notification settings - Fork 107
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
Refactor BPF recorder to enable simultaneous profile recording #2260
Conversation
Hi @mhils. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
/ok-to-test |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2260 +/- ##
==========================================
- Coverage 45.50% 41.82% -3.68%
==========================================
Files 79 109 +30
Lines 7782 15534 +7752
==========================================
+ Hits 3541 6497 +2956
- Misses 4099 8561 +4462
- Partials 142 476 +334 |
9873650
to
6b36a61
Compare
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.
Thanks, this is a great refactoring! I didn't spot any issues. 😄
@mhils I think there are some lint error to fix. |
see kubernetes-sigs#2260 for details
see kubernetes-sigs#2260 for details
see kubernetes-sigs#2260 for details
Thank you for the super quick review, @ccojocar - you folks are fantastic! 😃 🍰 CI has unfortunately uncovered a new issue I wasn't aware of before: Modern kernels all have BPF LSM support, but Debian/Ubuntu has not enabled it by default (see lockc-project/lockc#159 and https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2054810). This means the AppArmor recorder won't work on these systems properly as these hooks are silently ignored (Exhibit A: I've updated |
see kubernetes-sigs#2260 for details
see kubernetes-sigs#2260 for details
Isn't possible to enable it into the integration tests for Ubuntu? Does it require to recompile the kernel with the LSM hook flag enabled? Not sure what kernel version is used by the integration tests, but it would be great to have a test for this to avoid future breakage. Maybe you can deploy a dedicated VM image. |
The profile merging test seems to fail now:
|
My understanding is that it requires a recent Kernel (which we have in CI), but also boot-time configuration which is not set on the GitHub-hosted runners. The libbpf folks are running custom kernels via QEMU to work around this. That would be an alternative, but also comes with quite a lot of CI infrastructure that we would need to maintained (they have their own repo just for the GitHub Actions - https://github.com/libbpf/ci/tree/main). I have updated Will take a look at the remaining CI failures later today. :) |
see kubernetes-sigs#2260 for details
CI failures fixed in 8d28fc6. 😃 |
see kubernetes-sigs#2260 for details
…o record both simultaneously
see kubernetes-sigs#2260 for details
I have GHA tests passing for the same code at mhils#4, so I assume the previous failures were just flaky tests. :) |
7fa5b22
to
15b5954
Compare
We use the In case the e2e tests are taking too long, we can think to phase out the flatcar e2e test. I am not sure how much is used these days. I would add this e2e test as a follow up pull request. I guess you can follow the same approach we have for fedora.. @saschagrunert What do you think? |
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.
Thanks for this great contribution!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ccojocar, mhils, saschagrunert 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 bug
/kind cleanup
/kind feature
What this PR does / why we need it:
This PR:
bpfrecorder_seccomp.go
andbpfrecorder_apparmor.go
respectively.bpf_d_path()
to get absolute file paths in all cases. It also fixes tracking of child-child-processes. This makes the AppArmor recorder work flawlessly with several real-world workloads.--type all
for the recorder to simultaneously record AppArmor and Seccomp profiles.Which issue(s) this PR fixes:
Fixes #2255
Does this PR have test?
Yes.
Special notes for your reviewer:
The good news is that we remove significantly more lines than we add, the bad news is that this PR is still quite large. This thing emerged from a rather iterative integration hell, let me know if we need to split it up further. :)
Does this PR introduce a user-facing change?