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

Add a ProcMount option to the SecurityContext & AllowedProcMountTypes to PodSecurityPolicy #64283

Merged
merged 8 commits into from Aug 31, 2018

Conversation

@jessfraz
Copy link
Contributor

jessfraz commented May 24, 2018

So there is a bit of a chicken and egg problem here in that the CRI runtimes will need to implement this for there to be any sort of e2e testing.

What this PR does / why we need it: This PR implements design proposal kubernetes/community#1934. This adds a ProcMount option to the SecurityContext and AllowedProcMountTypes to PodSecurityPolicy

Relies on google/cadvisor#1967

Release note:

ProcMount added to SecurityContext and AllowedProcMounts added to PodSecurityPolicy to allow paths in the container's /proc to not be masked.

cc @Random-Liu @mrunalp

@@ -271,6 +271,21 @@ func (s *simpleProvider) ValidateContainerSecurityContext(pod *api.Pod, containe
allErrs = append(allErrs, field.Invalid(fldPath.Child("privileged"), *privileged, "Privileged containers are not allowed"))
}

procMount := sc.ProcMount()
allowedProcMounts := s.psp.Spec.AllowedProcMountTypes
if allowedProcMounts == nil {

This comment has been minimized.

Copy link
@dims

dims May 24, 2018

Member

Check for empty as well? (in addition to nil)

This comment has been minimized.

Copy link
@annismckenzie

annismckenzie May 24, 2018

So a length check against 0 would catch both.

@dims

This comment has been minimized.

Copy link
Member

dims commented May 24, 2018

@jessfraz we need a Feature for this right? (entry in kube_features.go)

@jessfraz

This comment has been minimized.

Copy link
Contributor Author

jessfraz commented May 24, 2018

Oh thanks for reminding me I'll do that

@@ -586,6 +586,12 @@ message LinuxContainerSecurityContext {
// no_new_privs defines if the flag for no_new_privs should be set on the
// container.
bool no_new_privs = 11;
// masked_paths is a slice of paths that should be masked by the container
// runtime, this can be passed directly to the OCI spec.
repeated string masked_paths = 13;

This comment has been minimized.

Copy link
@AkihiroSuda

AkihiroSuda May 25, 2018

Contributor

nit: 13 -> 12?

This comment has been minimized.

Copy link
@jessfraz

jessfraz May 25, 2018

Author Contributor

run_as_group is 12 its just out of order, it got me on the compile :D

This comment has been minimized.

Copy link
@dims

dims May 25, 2018

Member

y, i scratched my head for a few mins and realized that

This comment has been minimized.

Copy link
@runcom

runcom Oct 1, 2018

Member

is there any guidance for runtimes on how to handle these two fields? containerd/cri@3e4cec8#diff-c656bacd6cb0fe9a7f64814b01256decR358 cri-containerd seems to handle it that way but I believe there must be guidance in the api itself, @Random-Liu @jessfraz

@jessfraz jessfraz force-pushed the jessfraz:ProcMountType branch from e195cbc to fa808ad May 31, 2018
@jessfraz jessfraz force-pushed the jessfraz:ProcMountType branch from fa808ad to b719f09 Jun 1, 2018
@jessfraz jessfraz force-pushed the jessfraz:ProcMountType branch from b719f09 to 58e9a12 Jun 1, 2018
@dims

This comment has been minimized.

Copy link
Member

dims commented Jun 9, 2018

/unassign
/uncc

@jessfraz jessfraz added this to the v1.12 milestone Jun 14, 2018
@jessfraz jessfraz force-pushed the jessfraz:ProcMountType branch from 58e9a12 to 947e1ba Jun 14, 2018
@jessfraz jessfraz force-pushed the jessfraz:ProcMountType branch 2 times, most recently from ce9482c to 605cddf Jun 14, 2018
@jessfraz jessfraz force-pushed the jessfraz:ProcMountType branch from ce8b5d6 to 1a4cf7a Aug 30, 2018
Copy link
Member

feiskyer left a comment

LGTM

@dims

This comment has been minimized.

Copy link
Member

dims commented Aug 31, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 31, 2018
@smarterclayton

This comment has been minimized.

Copy link
Contributor

smarterclayton commented Aug 31, 2018

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Aug 31, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, jessfraz, smarterclayton, tpepper

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jessfraz

This comment has been minimized.

Copy link
Contributor Author

jessfraz commented Aug 31, 2018

/test pull-kubernetes-e2e-kops-aws

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Aug 31, 2018

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

@jessfraz

This comment has been minimized.

Copy link
Contributor Author

jessfraz commented Aug 31, 2018

/test pull-kubernetes-e2e-kops-aws

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Aug 31, 2018

Automatic merge from submit-queue (batch tested with PRs 64283, 67910, 67803, 68100). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md.

@k8s-github-robot k8s-github-robot merged commit 39004e8 into kubernetes:master Aug 31, 2018
15 of 18 checks passed
15 of 18 checks passed
Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-kops-aws
Details
pull-kubernetes-e2e-kops-aws Job triggered.
Details
pull-kubernetes-local-e2e-containerized Job triggered.
Details
cla/linuxfoundation jessfraz authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
@jessfraz jessfraz deleted the jessfraz:ProcMountType branch Aug 31, 2018
@derekwaynecarr

This comment has been minimized.

Copy link
Member

derekwaynecarr commented Sep 1, 2018

Can someone confirm this does not break support for docker 1.13?

@annismckenzie

This comment has been minimized.

Copy link

annismckenzie commented Sep 1, 2018

It doesn't – as long as your pod specs don't request/use the feature. According to https://github.com/kubernetes/kubernetes/pull/64283/files#diff-9a9f71a82ddbbcd0627de3aea90c6649R262 it'll be available starting with Docker API version 1.38 (which isn't even in a released version yet – see here). At that point in the code you also see that it'll fail to admit pods that ask for the feature with Docker not being at that version.

@dims

This comment has been minimized.

Copy link
Member

dims commented Sep 1, 2018

@derekwaynecarr as i mentioned earlier in this PR - one of the side results of the docker/docker update in this PR is that we switch from docker api version 1.31 to 1.38 if we want to keep the max to the earlier 1.31 docker API version then we need something like:
#66511

@jessfraz

This comment has been minimized.

Copy link
Contributor Author

jessfraz commented Sep 2, 2018

@yujuhong

This comment has been minimized.

Copy link
Member

yujuhong commented Sep 5, 2018

I know the PR is already in, but I'd rather not add more Docker API checking logic in kubelet itself (with dockershim being the exception). Can we move that logic out of kubelet, and just let dockershim return an error when trying to start the pod? The pod will be pending but it's no worse than what this PR currently does IMO.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Sep 9, 2018

@jessfraz: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-local-e2e-containerized 1a4cf7a link /test pull-kubernetes-local-e2e-containerized

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.