-
Notifications
You must be signed in to change notification settings - Fork 113
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
Add a default profile option for profileBindings #1869
Conversation
Hi @CoreyCook8. 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/test-infra repository. |
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.
Thank you for the contribution! One general suggestion and I assume we need a new e2e test for this.
profileRef: | ||
kind: SeccompProfile | ||
name: profile-complain | ||
image: * |
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.
Hm, how about allowing regular expressions here in general? This would end-users allow to select a registry for example.
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.
I had considered that, the implementation for the container storage is a sync.Map which I think would be difficult to work with a regular expression. I also didn't want to add too much complexity here. So I figured start with this case since it is simple rather than introducing a lot of complexity to the solution.
I can work on the E2E test!
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.
Sounds good to me!
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1869 +/- ##
==========================================
+ Coverage 45.21% 45.50% +0.28%
==========================================
Files 79 79
Lines 7714 7782 +68
==========================================
+ Hits 3488 3541 +53
- Misses 4090 4099 +9
- Partials 136 142 +6 |
/ok-to-test |
e5f673b
to
ee77c83
Compare
/retest |
1 similar comment
/retest |
Why do you tell me I have these powers and then not listen to me!! 😭 |
Is something actually breaking here? Or is it just a flakey test? |
Bump on this, is there any work / Updates I need to add to this? Or is it just the test being flakey? |
The following tests are failing right now:
I assume it's related to this PR since we only test SELinux on Fedora. The profile binding e2e test seems cricital:
May I ask you to check what's wrong here? |
@CoreyCook8 may I ask you to rebase please? |
/lgtm |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR introduces an "Enforce for all pods" option for ProfileBinding.
Container profile binding will have precedence here, but this will allow developers to create a default seccomp or selinux profile to be defaulted to all pods in a namespace.
If there exists a default profile binding and there has been no match between a container and profile binding for a given pod, the default profile will be applied to the pod level securityContext.
Which issue(s) this PR fixes:
Fixes #1868
Does this PR have test?
Special notes for your reviewer:
Does this PR introduce a user-facing change?