Skip to content
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

Merged
merged 2 commits into from
Jul 17, 2017

Conversation

feiskyer
Copy link
Member

@feiskyer feiskyer commented May 24, 2017

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:

Kubelet CRI: move seccomp from annotations to security context.

@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels May 24, 2017
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 24, 2017
@feiskyer
Copy link
Member Author

/assign @yujuhong @timstclair

@feiskyer
Copy link
Member Author

/sig node

@k8s-ci-robot k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label May 24, 2017
@feiskyer
Copy link
Member Author

cc/ @kubernetes/sig-node-pr-reviews

@timstclair
Copy link

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

@k8s-ci-robot k8s-ci-robot requested a review from jessfraz May 24, 2017 21:20
@k8s-ci-robot
Copy link
Contributor

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

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

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.

@jessfraz
Copy link
Contributor

jessfraz commented May 24, 2017

Oh crap I really wanted us to have our own seccomp spec when it moved to security context. Can we do CRI without this?

@jessfraz
Copy link
Contributor

jessfraz commented May 24, 2017

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
These profiles should not live on the node on disk, it will not scale well

@timstclair
Copy link

@jessfraz - note that this is only for the CRI, not the external user-facing apis. But still, I agree this needs more discussion.

@feiskyer
Copy link
Member Author

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.

@feiskyer
Copy link
Member Author

ping @timstclair @yujuhong

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 31, 2017
@jessfraz
Copy link
Contributor

jessfraz commented Jun 6, 2017

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

@yujuhong
Copy link
Contributor

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;
Copy link
Member

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.

@tallclair
Copy link
Member

Yeah, I'm fine with it.

@feiskyer
Copy link
Member Author

@timstclair @yujuhong Thanks. Will make a rebase today.

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 11, 2017
@feiskyer
Copy link
Member Author

@timstclair @yujuhong Rebased. PTAL.

@feiskyer
Copy link
Member Author

/test pull-kubernetes-e2e-kops-aws

@fejta
Copy link
Contributor

fejta commented Jul 11, 2017

/test pull-kubernetes-e2e-kops-aws

@feiskyer
Copy link
Member Author

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.
Copy link
Contributor

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>

Copy link
Member Author

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
Copy link
Contributor

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.

@feiskyer
Copy link
Member Author

/retest

@yujuhong
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 17, 2017
@k8s-github-robot
Copy link

[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 /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 17, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 10dc1aa into kubernetes:master Jul 17, 2017
@feiskyer feiskyer deleted the Seccomp branch July 17, 2017 21:48
k8s-github-robot pushed a commit that referenced this pull request Jul 19, 2017
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
k8s-github-robot pushed a commit that referenced this pull request Aug 14, 2017
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
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/node Categorizes an issue or PR as relevant to SIG Node. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CRI: using typed and structured fields instead of annotations CRI: pass full seccomp profile to runtime
9 participants