-
Notifications
You must be signed in to change notification settings - Fork 40k
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
Kubelet CRI: move seccomp from annotations to security context #46332
Conversation
/assign @yujuhong @timstclair |
/sig node |
cc/ @kubernetes/sig-node-pr-reviews |
I don't think the discussion on #36997 about whether to pass the path, unstructured profile data, or structured profile data, was ever resolved. /cc @jessfraz @jonboulle |
@timstclair: GitHub didn't allow me to request PR reviews from the following users: jonboulle. Note that only kubernetes members can review this PR, and authors cannot review their own PRs. In response to this:
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. I understand the commands that are listed here. |
Oh crap I really wanted us to have our own seccomp spec when it moved to security context. Can we do CRI without this? |
I really think this needs a larger discussion, passing the path was only supposed to be for annotations till we spec-ed out the actual profile that would live in k8s |
@jessfraz - note that this is only for the CRI, not the external user-facing apis. But still, I agree this needs more discussion. |
This is for cleaning up CRI ( the final step for #39130). It only changes the relative path to full path for clear. Seccomp spec should be addressed at #39128, which is not the current PR's scope. @timstclair @jessfraz for seccomp spec, please discuss at #39128. |
ping @timstclair @yujuhong |
This seems fine, I understand much more the semantics of how this works after #47019 :) seems like we can easily switch it in the future |
I am ok with this being the interim solution while we figuring out how/whether to spec out the format in the API. @timstclair WDYT? @feiskyer rebasing? |
// * unconfined: unconfined profile, ie, no seccomp sandboxing | ||
// * localhost/<profile-name>: the profile installed on the node. | ||
// <profile-name> is the full path of the profile. | ||
string seccomp_profile = 10; |
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.
Let's name this seccomp_profile_path
, since we may want to add a structured seccomp profile to the message eventually.
Yeah, I'm fine with it. |
@timstclair @yujuhong Thanks. Will make a rebase today. |
@timstclair @yujuhong Rebased. PTAL. |
/test pull-kubernetes-e2e-kops-aws |
/test pull-kubernetes-e2e-kops-aws |
ping @timstclair @yujuhong |
// Seccomp profile for the container, candidate values are: | ||
// * runtime/default: the default profile for the container runtime | ||
// * unconfined: unconfined profile, ie, no seccomp sandboxing | ||
// * localhost/<profile-name>: the profile installed on the node. |
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.
s/<profile-name>/<full-path-to-profile>
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.
@yujuhong Updated. PTAL.
@@ -1350,6 +1330,12 @@ type LinuxContainerSecurityContext struct { | |||
// (localhost) by name. The possible profile names are detailed at | |||
// http://wiki.apparmor.net/index.php/AppArmor_Core_Policy_Reference | |||
ApparmorProfile string `protobuf:"bytes,9,opt,name=apparmor_profile,json=apparmorProfile,proto3" json:"apparmor_profile,omitempty"` | |||
// Seccomp profile for the container, candidate values are: | |||
// * runtime/default: the default profile for the container runtime |
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.
We may want to define these values in the API too, but since this field is just to help with the transition, this is ok.
/retest |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: feiskyer, yujuhong Associated issue: 39130 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
Automatic merge from submit-queue |
Automatic merge from submit-queue (batch tested with PRs 48981, 47316, 49180) Add seccomp profile in sandbox security context **What this PR does / why we need it**: PR #46332 adds seccomp profile to container security context, but not sandbox. This PR adds seccomp profile in sandbox security context. Without this, we couldn't honour "seccomp.security.alpha.kubernetes.io/pod" for sandbox. **Which issue this PR fixes** fixes #49179. **Special notes for your reviewer**: **Release note**: ```release-note NONE ``` /cc @yujuhong
Automatic merge from submit-queue Support seccomp profile from container's security context **What this PR does / why we need it**: Support seccomp profile from container's security context, followup of #46332. **Which issue this PR fixes** fixes #46332. **Special notes for your reviewer**: ~~Depends on #49179. (already merged)~~ **Release note**: ```release-note NONE ```
What this PR does / why we need it:
This is the final step for #39130, which moves seccomp from annotations to linux container security context. And it also fixes #36997 by set the full seccomp profile path for node-installed profiles.
Note it doesn't include spec the seccomp profile format, which should be addressed at #39128. And a following PR is required for implementing in kuberuntime and dockershim.
Which issue this PR fixes
Fixes #39130
Fixes #36997
Special notes for your reviewer:
Release note: