Allow PSP's to specify a whitelist of allowed paths for host volume #50212

jhorwit2 · 2017-08-06T19:53:03Z

What this PR does / why we need it:

Reverts the revert for the allowed host path feature that was moved from v1.7 to v1.8. This PR also changes the api as discussed in #47811.

Original pr: #43946
revert: #47851

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #29326

Special notes for your reviewer:

cc @liggitt @ericchiang @php-coder

It seems the api build process has changed. Hopefully I did it right 👼 .

Release note:

Allow PSP's to specify a whitelist of allowed paths for host volume

k8s-ci-robot · 2017-08-06T19:53:11Z

Hi @jhorwit2. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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.

sttts · 2017-08-07T07:16:48Z

/unassign
/assign @derekwaynecarr

php-coder · 2017-08-07T09:39:56Z

CC @pweil-

php-coder · 2017-08-07T09:45:48Z

pkg/security/podsecuritypolicy/provider.go

+				if !psputil.PSPAllowsHostVolumePath(s.psp, v.HostPath.Path) {
+					allErrs = append(allErrs, field.Invalid(
+						field.NewPath("spec", "volumes").Index(i), string(fsType),
+						fmt.Sprintf("host path %s is not allowed to be used. allowed host paths", v.HostPath.Path)))


"host path %s is not allowed to be used. allowed host paths" -> "host path %s is not allowed to be used"

php-coder · 2017-08-07T09:48:29Z

pkg/security/podsecuritypolicy/provider.go

+				continue
+			}
+
+			if fsType == extensions.HostPath {


Looks like this check will be triggered only when volumes not equal to *. This is incorrect.

Good catch! I updated the PR.

php-coder · 2017-08-07T09:52:50Z

pkg/security/podsecuritypolicy/util/util_test.go

+	for k, v := range tests {
+		allows := PSPAllowsHostVolumePath(v.psp, v.path)
+		if v.allows != allows {
+			t.Errorf("%s expected PSPAllowsHostVolumePath to return %t but got %t", k, v.allows, allows)


s/PSPAllowsHostVolumePath//

php-coder · 2017-08-07T09:55:21Z

pkg/apis/extensions/types.go

+}
+
+// AllowedHostPath defines the host volume conditions that will be enabled by a policy
+// for pods to use. It requires the path to be defined.


"It requires the path to be defined." Code for validating this scenario is missing.

php-coder · 2017-08-07T09:58:38Z

pkg/apis/extensions/types.go

+// for pods to use. It requires the path to be defined.
+type AllowedHostPath struct {
+	// Path is the path prefix that the host volume must match
+	Path string


It's confusing me that we use path anywhere but we mean "path prefix" really.

@jhorwit2 @pweil- @liggitt Maybe we should rename the fields? For example, it can be named AllowedHostPathPrefixes.

could name the field PathPrefix, the parent should remain AllowedHostPath

php-coder · 2017-08-07T10:01:15Z

pkg/security/podsecuritypolicy/util/util.go

+
+// PSPAllowsHostVolumePath is a utility for checking if a PSP allows the host volume path.
+// This only checks the path. You should still check to make sure the host volume fs type is allowed.
+func PSPAllowsHostVolumePath(psp *extensions.PodSecurityPolicy, hostPath string) bool {


Is it mandatory to use PSP in the function name? I don't see a reason for that. psputil.AllowsHostVolumePath() looks good to me either.

jhorwit2 · 2017-08-11T00:45:58Z

@liggitt @php-coder i've addressed all the major concerns. Could someone please enable the test suite for me 👼

ericchiang · 2017-08-11T18:15:49Z

/ok-to-test

jhorwit2 · 2017-08-11T20:20:29Z

/test pull-kubernetes-e2e-gce-etcd3

…

On Aug 11, 2017, at 4:06 PM, k8s-ci-robot ***@***.***> wrote: /test pull-kubernetes-e2e-gce-etcd3

jhorwit2 · 2017-08-12T14:09:43Z

Is there anyone else that should review this? Tests all look good.

jhorwit2 · 2017-08-24T23:33:47Z

@bgrant0607 @liggitt it should all be good now!

removed files not supposed to be there

jhorwit2 · 2017-08-26T11:25:56Z

/retest

…

On Aug 25, 2017, at 11:28 PM, k8s-ci-robot ***@***.***> wrote: @jhorwit2: The following tests failed, say /retest to rerun them all: Test name Commit Details Rerun command pull-kubernetes-e2e-gce-bazel 6ec738a link /test pull-kubernetes-e2e-gce-bazel pull-kubernetes-e2e-gce-etcd3 6ec738a link /test pull-kubernetes-e2e-gce-etcd3 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. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

smarterclayton · 2017-08-26T23:54:20Z

Spawned #51422 for the checker

jhorwit2 · 2017-08-27T02:36:42Z

Is there anyone else that should review this? I was told we wanted this in for 1.8 so i just want to make sure we meet code freeze.

liggitt · 2017-08-27T02:37:46Z

/lgtm

jhorwit2 · 2017-08-30T13:06:30Z

@smarterclayton @ericchiang @liggitt do we still plan to make this before code freeze?

liggitt · 2017-08-30T13:19:20Z

adding @smarterclayton for docs/ pkg/ approval

liggitt · 2017-08-30T13:19:47Z

/test pull-kubernetes-bazel-build
/test pull-kubernetes-bazel-test

smarterclayton · 2017-08-30T19:33:13Z

/approve

based on the history and comments in this thread and previous approval in last release

k8s-github-robot · 2017-08-30T19:34:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jhorwit2, liggitt, smarterclayton

Associated issue: 29326

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

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

~~api/OWNERS~~ [liggitt,smarterclayton]
~~docs/OWNERS~~ [smarterclayton]
~~pkg/OWNERS~~ [smarterclayton]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt,smarterclayton]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

jhorwit2 · 2017-08-31T18:23:07Z

/test pull-kubernetes-e2e-gce-etcd3
/test pull-kubernetes-e2e-kops-aws

k8s-github-robot · 2017-09-01T04:09:12Z

Automatic merge from submit-queue (batch tested with PRs 50719, 51216, 50212, 51408, 51381)

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 6, 2017

k8s-github-robot assigned sttts and jianhuiz Aug 6, 2017

jhorwit2 mentioned this pull request Aug 6, 2017

change psp AllowedHostPath api to be more extensible #47811

Closed

k8s-ci-robot assigned derekwaynecarr and unassigned sttts Aug 7, 2017

php-coder reviewed Aug 7, 2017

View reviewed changes

jhorwit2 force-pushed the jah/psp-hostpath branch 3 times, most recently from d7ec40a to 2c3925b Compare August 11, 2017 00:44

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 11, 2017

jhorwit2 force-pushed the jah/psp-hostpath branch from 1da54eb to 340123e Compare August 11, 2017 21:43

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 16, 2017

jhorwit2 force-pushed the jah/psp-hostpath branch from 340123e to 9396113 Compare August 16, 2017 12:28

jhorwit2 force-pushed the jah/psp-hostpath branch from 4d05eef to c298282 Compare August 24, 2017 11:53

liggitt assigned liggitt and ericchiang and unassigned jianhuiz and derekwaynecarr Aug 25, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 25, 2017

jhorwit2 force-pushed the jah/psp-hostpath branch from c298282 to f8c43dd Compare August 26, 2017 01:32

jhorwit2 added 2 commits August 25, 2017 21:35

Allow PSP's to specify a whitelist of allowed paths for host volume

fab6044

removed files not supposed to be there

generated files

6ec738a

jhorwit2 force-pushed the jah/psp-hostpath branch from f8c43dd to 6ec738a Compare August 26, 2017 01:39

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 26, 2017

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

liggitt assigned smarterclayton Aug 30, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 30, 2017

liggitt mentioned this pull request Aug 30, 2017

validate allowedHostPaths in podsecuritypolicy #46956

Closed

k8s-github-robot merged commit 3937783 into kubernetes:master Sep 1, 2017

php-coder mentioned this pull request Sep 1, 2017

Revert "add host paths to psp (#3971)" kubernetes/website#4420

Merged

tengqm mentioned this pull request Nov 6, 2017

AllowedHostPaths PSP policy not documented. kubernetes/website#6205

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow PSP's to specify a whitelist of allowed paths for host volume #50212

Allow PSP's to specify a whitelist of allowed paths for host volume #50212

jhorwit2 commented Aug 6, 2017

k8s-ci-robot commented Aug 6, 2017

sttts commented Aug 7, 2017

php-coder commented Aug 7, 2017

php-coder Aug 7, 2017

php-coder Aug 7, 2017

jhorwit2 Aug 7, 2017

php-coder Aug 7, 2017

php-coder Aug 7, 2017

jhorwit2 Aug 7, 2017

php-coder Aug 7, 2017

liggitt Aug 7, 2017

jhorwit2 Aug 11, 2017

php-coder Aug 7, 2017

jhorwit2 commented Aug 11, 2017

ericchiang commented Aug 11, 2017

jhorwit2 commented Aug 11, 2017 via email

jhorwit2 commented Aug 12, 2017

jhorwit2 commented Aug 24, 2017

jhorwit2 commented Aug 26, 2017 via email

smarterclayton commented Aug 26, 2017

jhorwit2 commented Aug 27, 2017

liggitt commented Aug 27, 2017

jhorwit2 commented Aug 30, 2017

liggitt commented Aug 30, 2017 •

edited

Loading

liggitt commented Aug 30, 2017

smarterclayton commented Aug 30, 2017

k8s-github-robot commented Aug 30, 2017

jhorwit2 commented Aug 31, 2017

k8s-github-robot commented Sep 1, 2017

Allow PSP's to specify a whitelist of allowed paths for host volume #50212

Allow PSP's to specify a whitelist of allowed paths for host volume #50212

Conversation

jhorwit2 commented Aug 6, 2017

k8s-ci-robot commented Aug 6, 2017

sttts commented Aug 7, 2017

php-coder commented Aug 7, 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

jhorwit2 commented Aug 11, 2017

ericchiang commented Aug 11, 2017

jhorwit2 commented Aug 11, 2017 via email

jhorwit2 commented Aug 12, 2017

jhorwit2 commented Aug 24, 2017

jhorwit2 commented Aug 26, 2017 via email

smarterclayton commented Aug 26, 2017

jhorwit2 commented Aug 27, 2017

liggitt commented Aug 27, 2017

jhorwit2 commented Aug 30, 2017

liggitt commented Aug 30, 2017 • edited Loading

liggitt commented Aug 30, 2017

smarterclayton commented Aug 30, 2017

k8s-github-robot commented Aug 30, 2017

jhorwit2 commented Aug 31, 2017

k8s-github-robot commented Sep 1, 2017

liggitt commented Aug 30, 2017 •

edited

Loading