Support seccomp profile from container's security context #49178

feiskyer · 2017-07-19T05:49:03Z

What this PR does / why we need it:

Support seccomp profile from container's security context, followup of #46332.

Which issue this PR fixes

Special notes for your reviewer:

~~Depends on #49179. (already merged)~~

Release note:

NONE

feiskyer · 2017-07-19T07:24:57Z

/unassign

feiskyer · 2017-07-19T07:25:37Z

/assign @yujuhong

feiskyer · 2017-07-20T01:31:54Z

@yujuhong Rebased. PTAL.

feiskyer · 2017-07-20T09:10:06Z

/test pull-kubernetes-e2e-gce-etcd3

feiskyer · 2017-08-09T01:13:42Z

ping @yujuhong @euank

Random-Liu · 2017-08-10T21:54:28Z

cmd/kubelet/app/server.go

@@ -983,7 +983,7 @@ func RunDockershim(c *componentconfig.KubeletConfiguration, r *options.Container
 		SupportedPortForwardProtocols:   streaming.DefaultConfig.SupportedPortForwardProtocols,
 	}

-	ds, err := dockershim.NewDockerService(dockerClient, c.SeccompProfileRoot, r.PodSandboxImage,
+	ds, err := dockershim.NewDockerService(dockerClient, r.PodSandboxImage,


nit: Could you rearrange this a little bit? Since this line get shorter now.

Random-Liu · 2017-08-10T22:00:26Z

pkg/kubelet/kuberuntime/helpers.go

@@ -255,3 +256,30 @@ func getSysctlsFromAnnotations(annotations map[string]string) (map[string]string

 	return sysctls, nil
 }
+
+// getSeccompProfileFromAnnotations gets seccomp profile from annotations.
+// It gets pod's profile if containerName is null.


s/null/empty.

Random-Liu · 2017-08-10T22:29:48Z

pkg/kubelet/kubelet.go

@@ -572,7 +572,7 @@ func NewMainKubelet(kubeCfg *componentconfig.KubeletConfiguration, kubeDeps *Dep
 		case kubetypes.DockerContainerRuntime:
 			// Create and start the CRI shim running as a grpc server.
 			streamingConfig := getStreamingConfig(kubeCfg, kubeDeps)
-			ds, err := dockershim.NewDockerService(kubeDeps.DockerClient, kubeCfg.SeccompProfileRoot, crOptions.PodSandboxImage,
+			ds, err := dockershim.NewDockerService(kubeDeps.DockerClient, crOptions.PodSandboxImage,


Random-Liu · 2017-08-10T23:06:26Z

pkg/kubelet/dockershim/helpers_linux_test.go

-		expectedOpts: []string{`seccomp={"foo":"bar"}`},
-		expectErr:    false,
+		msg:            "Seccomp localhost/test profile",
+		seccompProfile: "localhost/fixtures/seccomp/test",


At least add comment to explain that we are abusing localhost/ for test now, because fixtures/seccomp/test is not an absolute path.

Random-Liu · 2017-08-10T23:12:29Z

pkg/kubelet/dockershim/helpers_linux.go


 	return []dockerOpt{{"seccomp", b.String(), msg}}, nil
 }

-// getSeccompSecurityOpts gets container seccomp options from container and sandbox
-// config, currently from sandbox annotations.
+// getSeccompSecurityOpts gets container seccomp options from container security context.


from container seccomp profile?

Random-Liu · 2017-08-10T23:17:33Z

pkg/kubelet/kuberuntime/helpers.go

+
+	if strings.HasPrefix(profile, "localhost/") {
+		name := strings.TrimPrefix(profile, "localhost/")
+		fname := filepath.Join(m.seccompProfileRoot, filepath.FromSlash(name))


Should add localhost/.

Random-Liu · 2017-08-10T23:25:59Z

@feiskyer Several questions:

runtime/default is not passed to runtime. We are passing docker/default instead.
It's not defined that what empty seccomp profile means in CRI, we should explicitly define that means unconfined.

As for 1), one option is to convert docker/default to runtime/default in kuberuntime.

However, I discussed this with @yujuhong today, we think that user may really be expecting "Docker" default. We should not silently convert docker/default to runtime/default, and let runtime apply its own default, which may be different from docker's.

Ideally, we should have kubelet default, and pass this default to runtime. However, that requires Kubernetes api change.

So for now, we can only keep passing docker/default, dockershim will implement it, other runtime should just return error to fail loudly.

fejta-bot · 2017-08-12T20:14:27Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

fejta-bot · 2017-08-12T23:02:27Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

fejta-bot · 2017-08-13T02:32:32Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

fejta-bot · 2017-08-13T05:20:28Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

feiskyer · 2017-08-13T07:49:48Z

Made a rebase to fix the pull-kubernetes-verify problem.

feiskyer · 2017-08-13T13:38:26Z

/retest

Random-Liu · 2017-08-14T01:47:35Z

/lgtm

k8s-github-robot · 2017-08-14T01:48:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Random-Liu, feiskyer

Associated issue: 46332

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~cmd/kubelet/OWNERS~~ [Random-Liu]
~~pkg/kubelet/OWNERS~~ [Random-Liu]

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 · 2017-08-14T01:48:24Z

/test all [submit-queue is verifying that this PR is safe to merge]

feiskyer · 2017-08-14T02:35:53Z

@Random-Liu Thanks.

feiskyer · 2017-08-14T02:35:56Z

/retest

k8s-github-robot · 2017-08-14T03:21:00Z

Automatic merge from submit-queue

runcom · 2017-09-11T14:08:23Z

So for now, we can only keep passing docker/default, dockershim will implement it, other runtime should just return error to fail loudly.

@feiskyer @Random-Liu this is not correct btw, CRI-O has its own seccomp profile which can actually be used. CRI-O isn't going to error out, but calling docker/default in the CRI (which should be abstract) is totally wrong to me. Could you please revert this? or document "every runtime can decide to honor the profile passed"?

/cc @rhatdan @mrunalp @derekwaynecarr

k8s-ci-robot · 2017-09-11T14:08:26Z

@runcom: GitHub didn't allow me to request PR reviews from the following users: rhatdan.

Note that only kubernetes members can review this PR, and authors cannot review their own PRs.

In response to this:

So for now, we can only keep passing docker/default, dockershim will implement it, other runtime should just return error to fail loudly.

@feiskyer @Random-Liu this is not correct btw, CRI-O has its own seccomp profile which can actually be used. CRI-O isn't going to error out, but calling docker/default in the CRI (which should be abstract) is totally wrong to me. Could you please revert this? or document "every runtime can decide to honor the profile passed"?

/cc @rhatdan @mrunalp @derekwaynecarr

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.

yujuhong · 2017-09-11T17:32:23Z

@runcom, kubelet passes the seccomp profile name from the Kubernetes API to the runtime directly. The exact profile name used in the pod annotation is docker/default in this case. It should not be an arbitrary runtime default in IMO. Without changes to the kubernetes API, I think the feasible solutions for cri-o right now would be 1) support a docker/default profile or 2) returns error if container demands docker/default.

yujuhong · 2017-09-11T17:45:32Z

BTW, there is ongoing work to define a default profile for kuberentes: kubernetes/community#660

k8s-ci-robot assigned feiskyer Jul 19, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 19, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Jul 19, 2017

feiskyer force-pushed the seccomp-impl branch from 55a74ce to 6bc7297 Compare July 19, 2017 06:01

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 19, 2017

feiskyer mentioned this pull request Jul 19, 2017

CRI: missing seccomp profile in sandbox security context #49179

Closed

feiskyer force-pushed the seccomp-impl branch from 6bc7297 to a081ae8 Compare July 19, 2017 07:03

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 19, 2017

feiskyer force-pushed the seccomp-impl branch from a081ae8 to 5a17150 Compare July 19, 2017 07:22

k8s-ci-robot unassigned feiskyer Jul 19, 2017

k8s-github-robot assigned euank and resouer Jul 19, 2017

feiskyer changed the title ~~WIP: Support seccomp profile from container's security context~~ Support seccomp profile from container's security context Jul 19, 2017

k8s-ci-robot assigned yujuhong Jul 19, 2017

feiskyer force-pushed the seccomp-impl branch 2 times, most recently from 944ed1a to f0b8578 Compare July 20, 2017 01:30

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 20, 2017

yujuhong mentioned this pull request Aug 8, 2017

Bot assigned the PR author to be a reviewer kubernetes/test-infra#3982

Closed

Random-Liu self-assigned this Aug 10, 2017

Random-Liu reviewed Aug 10, 2017

View reviewed changes

feiskyer added 3 commits August 13, 2017 15:42

Support seccomp profile from container's security context

f3150c9

Use seccomp from security context

bf01fa2

Rename runtime/default to docker default

c242432

feiskyer force-pushed the seccomp-impl branch from 0919eda to ea4a341 Compare August 13, 2017 07:43

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 13, 2017

run hack/update-bazel.sh

ea4a341

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 14, 2017

k8s-github-robot merged commit b32639f into kubernetes:master Aug 14, 2017

feiskyer deleted the seccomp-impl branch August 14, 2017 03:26

xiangpengzhao mentioned this pull request Aug 15, 2017

The cross-build is broken #50675

Closed

Random-Liu mentioned this pull request Aug 21, 2017

Update kubernetes containerd/cri#148

Merged

k8s-ci-robot requested review from derekwaynecarr and mrunalp September 11, 2017 14:08

runcom mentioned this pull request Sep 18, 2017

Fix Origin test and update kube to v1.7.6 cri-o/cri-o#917

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support seccomp profile from container's security context #49178

Support seccomp profile from container's security context #49178

feiskyer commented Jul 19, 2017 •

edited

Loading

feiskyer commented Jul 19, 2017

feiskyer commented Jul 19, 2017

feiskyer commented Jul 20, 2017

feiskyer commented Jul 20, 2017

feiskyer commented Aug 9, 2017

Random-Liu Aug 10, 2017

feiskyer Aug 11, 2017

Random-Liu Aug 10, 2017

feiskyer Aug 11, 2017

Random-Liu Aug 10, 2017

feiskyer Aug 11, 2017

Random-Liu Aug 10, 2017

feiskyer Aug 11, 2017

Random-Liu Aug 10, 2017

feiskyer Aug 11, 2017

Random-Liu Aug 10, 2017

Random-Liu commented Aug 10, 2017 •

edited

Loading

fejta-bot commented Aug 12, 2017

fejta-bot commented Aug 12, 2017

fejta-bot commented Aug 13, 2017

fejta-bot commented Aug 13, 2017

feiskyer commented Aug 13, 2017

feiskyer commented Aug 13, 2017

Random-Liu commented Aug 14, 2017

k8s-github-robot commented Aug 14, 2017

k8s-github-robot commented Aug 14, 2017

feiskyer commented Aug 14, 2017

feiskyer commented Aug 14, 2017

k8s-github-robot commented Aug 14, 2017

runcom commented Sep 11, 2017

k8s-ci-robot commented Sep 11, 2017

yujuhong commented Sep 11, 2017

yujuhong commented Sep 11, 2017

Support seccomp profile from container's security context #49178

Support seccomp profile from container's security context #49178

Conversation

feiskyer commented Jul 19, 2017 • edited Loading

feiskyer commented Jul 19, 2017

feiskyer commented Jul 19, 2017

feiskyer commented Jul 20, 2017

feiskyer commented Jul 20, 2017

feiskyer commented Aug 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu commented Aug 10, 2017 • edited Loading

fejta-bot commented Aug 12, 2017

fejta-bot commented Aug 12, 2017

fejta-bot commented Aug 13, 2017

fejta-bot commented Aug 13, 2017

feiskyer commented Aug 13, 2017

feiskyer commented Aug 13, 2017

Random-Liu commented Aug 14, 2017

k8s-github-robot commented Aug 14, 2017

k8s-github-robot commented Aug 14, 2017

feiskyer commented Aug 14, 2017

feiskyer commented Aug 14, 2017

k8s-github-robot commented Aug 14, 2017

runcom commented Sep 11, 2017

k8s-ci-robot commented Sep 11, 2017

yujuhong commented Sep 11, 2017

yujuhong commented Sep 11, 2017

feiskyer commented Jul 19, 2017 •

edited

Loading

Random-Liu commented Aug 10, 2017 •

edited

Loading