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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

/ok-to-test

Codecov Report

Attention: Patch coverage is 51.57068% with 185 lines in your changes are missing coverage. Please review.

Project coverage is 41.82%. Comparing base (11d77f4) to head (0706fb8).
Report is 212 commits behind head on main.

@@            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     

@mhils I think there are some lint error to fix.

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: test / e2e-spoc fails in CI). Unfortunately there are no good workarounds. We do really want bpf_d_path to get absolute paths, the previous implementation was pretty broken. But the non-LSM hooks allowlisted to use bpf_d_path are not comprehensive enough to cover our needs. So we have to use LSM hooks for recording.

I've updated AppArmorRecorder.Load to error at runtime if we detect that LSM hooks are disabled. This unfortunately means we cannot run all the spoc tests in CI currently, but I've made it so that we still cover the AppArmor parts that do not rely on file paths.

I've updated AppArmorRecorder.Load to error at runtime if we detect that LSM hooks are disabled. This unfortunately means we cannot run all the spoc tests in CI currently, but I've made it so that we still cover the AppArmor parts that do not rely on file paths.

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:

TestSuite/TestSecurityProfilesOperator/cluster-wide:_profile_merging

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.

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 test/spoc/e2e_spoc_test.go so that we always test AppArmor socket and capability recording, even if BPF LSM support is missing on the host. This ensures that the AppArmor subsystem is working, which I think should cover most things that could possibly break. I feel it's probably ok to accept the risk for now. Eventually https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2054810 should be fixed and we get this fully covered in CI again without spending any effort on it. Does that sound reasonable?

Will take a look at the remaining CI failures later today. :)

CI failures fixed in 8d28fc6. 😃

I have GHA tests passing for the same code at mhils#4, so I assume the previous failures were just flaky tests. :)

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

We use the ubuntu-22.04 as an action image and on top of it runs vagrant with different images such as ubunut, fedora and flatcar where we run the e2e tests. This provides us some flexibility since the LSM needs to be enabled on vagrant image. I think we can add a dedicated e2e job for debian with LSM enabled (LSM should be enabled by default in Debian as I understand from ubuntu bug, or a custom ubuntu image with LSM enabled.

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?

[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