AppArmor PodSecurityPolicy support #30183
Conversation
k8s-github-robot
added
size/XL
|
|
||
| profile := apparmor.GetProfileName(pod, container.Name) | ||
| if profile == "" { | ||
| if len(s.allowedProfiles) > 0 { |
There was a problem hiding this comment.
Shouldn't one be able to allow ""?
There was a problem hiding this comment.
What's the use case for that? Would the empty string profile just mean that running without an AppArmor profile would be allowed? That would probably be valuable, but I'd prefer to have an explicit parameter for it (e.g. nil) or a separate annotation altogether. My concern is that a trailing comma shouldn't accidentally enable running unconfined.
There was a problem hiding this comment.
I was thinking that later the annotation will be turned into a string field. Then "" means that some default profile is selected, i.e. runtime/default. That's how we do it in seccomp. If you somehow translate runtime/default in the PSP into "" being allowed as a pod value, that's fine. Then you don't have the trailing comma issue.
There was a problem hiding this comment.
I was thinking of it as no annotation = zero value, annotation = explicitly set value. By this reasoning I don't think the empty string value needs to be allowed.
|
Looks pretty good. Please recheck the logic around empty string profiles. There should be a way to allow them with a PSP, next to other non-empty profiles. |
| return nil | ||
| } | ||
|
|
||
| apparmor.SetProfileName(pod, container.Name, s.allowedProfiles[0]) |
There was a problem hiding this comment.
Generally I've tried to keep these methods just returning the value of what is generated and not actually setting anything. Then the provider applies the values to the copy of the SecurityContext, or in this case annotations. I'm also leaning in favor of changing the provider interface to return a set of annotations that I mentioned over email since it lets us keep the assumption that the provider does not actually change the pod and it is up to the caller to decide if it should apply the generated values.
k8s-github-robot
added
the
needs-rebase
timstclair
force-pushed
the
aa-psp
branch
2 times, most recently
from
5ce6d85 to
b8a4285
Compare
k8s-github-robot
removed
the
needs-rebase
timstclair
added
release-note-none
timstclair
changed the title
|
Rebased on to #30257 & updated to match new interface. |
timstclair
force-pushed
the
aa-psp
branch
4 times, most recently
from
54c8d10 to
c411087
Compare
k8s-github-robot
added
size/XXL
|
@dchen1107 - This is ready for review. It's the last piece of AppArmor implementation for v1.4. It's a bit messy right now since it includes to dependent PRs, so you can just review the most recent commit: c2cda9f |
|
Fixed unit test & validation. Squashed so the commit can still be reviewed separately from the dependent ones. |
k8s-github-robot
added
the
needs-rebase
|
I only reviewed commits/69efe20d6ab571703796474e6161eff9cd728416 since others are included through different PRs. LGTM overall, but looks like we have to wait for other prs merged first. |
k8s-github-robot
added
size/XL
k8s-github-robot
added
the
needs-rebase
|
Rebased now that the dependencies are merged. This should be ready for final review. |
k8s-github-robot
added
size/L
|
GCE e2e build/test passed for commit 293770e. |
|
LGTM |
dchen1107
added
the
lgtm
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e build/test passed for commit 293770e. |
|
Automatic merge from submit-queue |
Implements the AppArmor PodSecurityPolicy support based on the alpha API proposed here
This implementation deviates from the original proposal in one way: it adds a separate option for specifying a default profile:
This has several advantages over the original proposal:
The E2E cluster does not currently enable the PodSecurityPolicy, so I will submit E2E tests in a separate PR.
/cc @dchen1107 @pweil- @sttts @jfrazelle @Amey-D
This change is