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

CSI ephemeral - Enforcing pod security policy AllowedCSIDriver #76915

Merged

Conversation

@vladimirvivien
Copy link
Member

commented Apr 22, 2019

What type of PR is this?
/kind bug

What this PR does / why we need it:
This PR adds code that enforces pod security policy AllowedCSIDriver to indicate which CSI drivers are allowed to run as inline ephemeral.

Which issue(s) this PR fixes:

Fixes #76620

Inline CSI ephemeral volumes can now be controlled with PodSecurityPolicy when the CSIInlineVolume alpha feature is enabled

/sig storage

@k8s-ci-robot k8s-ci-robot requested review from derekwaynecarr and lavalamp Apr 22, 2019

@vladimirvivien vladimirvivien force-pushed the vladimirvivien:csi-inline-podsec-policy branch from a2116ca to 60ce0fc May 9, 2019

@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels May 9, 2019

@vladimirvivien vladimirvivien force-pushed the vladimirvivien:csi-inline-podsec-policy branch from 60ce0fc to 045c19b May 9, 2019

@vladimirvivien vladimirvivien changed the title [WIP] CSI ephemeral - Enforcing pod security policy AllowedCSIDriver CSI ephemeral - Enforcing pod security policy AllowedCSIDriver May 9, 2019

@msau42

This comment has been minimized.

Copy link
Member

commented May 9, 2019

@tallclair

This comment has been minimized.

Copy link
Member

commented May 9, 2019

/priority important-soon
/milestone v1.15

@k8s-ci-robot k8s-ci-robot added this to the v1.15 milestone May 9, 2019

@@ -389,6 +389,14 @@ func TestValidatePodSecurityPolicy(t *testing.T) {
invalidProcMount := validPSP()
invalidProcMount.Spec.AllowedProcMountTypes = []api.ProcMountType{api.ProcMountType("bogus")}

emptyAllowedCSIDrivers := validPSP()

This comment has been minimized.

Copy link
@tallclair

tallclair May 9, 2019

Member

I think validation should only happen when the feature is enabled. @liggitt can you confirm?

This comment has been minimized.

Copy link
@vladimirvivien

vladimirvivien May 10, 2019

Author Member

Will wait for @liggitt response here.

This comment has been minimized.

Copy link
@liggitt

liggitt May 10, 2019

Member

if the feature is disabled:

  • new pods will have their CSI pod.spec.volumes[*].csi field cleared, so validation wouldn't be applicable
  • old pods without the CSI field set cannot update to set it, so validation is not applicable there either
  • old pods with the CSI field set would preserve it, so PSP validation could affect behavior there

if a user updates an existing pod (changes the image, etc) which has an inline CSI volume (which means the pod was created when the feature gate was on), and there's a PSP with CSI fields (which also must have been created when the feature gate was on), would you expect the policy to still enforce the CSI fields. I think I would.


emptyAllowedCSIDriverName := validPSP()
emptyAllowedCSIDriverName.Spec.Volumes = []policy.FSType{policy.CSI}
emptyAllowedCSIDriverName.Spec.AllowedCSIDrivers = []policy.AllowedCSIDriver{{Name: ""}}

This comment has been minimized.

Copy link
@tallclair

tallclair May 9, 2019

Member

unused? Did you mean to add these to the error cases?

This comment has been minimized.

Copy link
@vladimirvivien

vladimirvivien May 10, 2019

Author Member

Thanks, removing.

@@ -296,6 +296,28 @@ func (s *simpleProvider) ValidatePod(pod *api.Pod) field.ErrorList {
"Flexvolume driver is not allowed to be used"))
}
}

This comment has been minimized.

Copy link
@tallclair

tallclair May 9, 2019

Member

This function is getting hard to read - mind pulling the volume loop body out to a separate function?

@@ -296,6 +296,28 @@ func (s *simpleProvider) ValidatePod(pod *api.Pod) field.ErrorList {
"Flexvolume driver is not allowed to be used"))
}
}

if fsType == policy.CSI {

This comment has been minimized.

Copy link
@tallclair

tallclair May 9, 2019

Member

nit: switch on fsType

if fsType == policy.CSI {
// one or more drivers must be specified
if len(s.psp.Spec.AllowedCSIDrivers) == 0 {
allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "allowedCSIDrivers"), s.psp.Spec.AllowedCSIDrivers,

This comment has been minimized.

Copy link
@tallclair

tallclair May 9, 2019

Member

This is supposed to be validating the pod, not the PSP. Refer to the flex driver error above as an example

This comment has been minimized.

Copy link
@vladimirvivien

vladimirvivien May 15, 2019

Author Member

Got it.

@@ -296,6 +296,28 @@ func (s *simpleProvider) ValidatePod(pod *api.Pod) field.ErrorList {
"Flexvolume driver is not allowed to be used"))
}
}

if fsType == policy.CSI {

This comment has been minimized.

Copy link
@tallclair

tallclair May 9, 2019

Member

Only perform the verification if the inline csi driver feature gate is enabled

This comment has been minimized.

Copy link
@vladimirvivien

vladimirvivien May 15, 2019

Author Member

Fixed.

@vladimirvivien vladimirvivien force-pushed the vladimirvivien:csi-inline-podsec-policy branch from 045c19b to 7ba0f26 May 9, 2019

@@ -296,6 +296,28 @@ func (s *simpleProvider) ValidatePod(pod *api.Pod) field.ErrorList {
"Flexvolume driver is not allowed to be used"))
}
}

if fsType == policy.CSI {

This comment has been minimized.

Copy link
@liggitt

liggitt May 10, 2019

Member

I'm trying to remember why this was intended to behave differently than AllowedFlexVolumes (empty AllowedFlexVolumes means any flex volume is allowed).

This comment has been minimized.

Copy link
@vladimirvivien

vladimirvivien May 15, 2019

Author Member

This was a safeguard against user requesting any CSI driver to function as inline ephemeral. Based on suggestion above, it probably is best to make the CSI inline PSP work like other PSP's like FlexVolume (i.e. empty list means all driver allowed). This will work since the existence of the CSIDriver object can further restrict that feature.

This comment has been minimized.

Copy link
@vladimirvivien

vladimirvivien May 15, 2019

Author Member

Furthermore, if CSI PSP requires a non-empty allowed whitelist to work at all, then it creates a weird case where if policy.CSI is set and no volumes are provided, then it errors out. For now, I think it's better to follow how FlexVol is used .

This comment has been minimized.

Copy link
@liggitt

liggitt May 15, 2019

Member

For now, I think it's better to follow how FlexVol is used .

sounds good. can you update the API doc

@vladimirvivien vladimirvivien force-pushed the vladimirvivien:csi-inline-podsec-policy branch 2 times, most recently from f955066 to a802e07 May 10, 2019

@vladimirvivien

This comment has been minimized.

Copy link
Member Author

commented May 15, 2019

/test pull-kubernetes-e2e-gce

@vladimirvivien vladimirvivien force-pushed the vladimirvivien:csi-inline-podsec-policy branch from a802e07 to 86eabf1 May 28, 2019

@vladimirvivien vladimirvivien force-pushed the vladimirvivien:csi-inline-podsec-policy branch from 86eabf1 to 2ae2868 May 28, 2019

@fejta-bot

This comment has been minimized.

Copy link

commented May 28, 2019

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@vladimirvivien vladimirvivien force-pushed the vladimirvivien:csi-inline-podsec-policy branch from 2ae2868 to 20c0ca6 May 29, 2019

@xmudrii

This comment has been minimized.

Copy link
Member

commented May 29, 2019

Hello! I'd like just to remind that the code freeze for 1.15 is starting tomorrow EOD. Is this PR still planned for this release cycle?

@vladimirvivien

This comment has been minimized.

Copy link
Member Author

commented May 29, 2019

@xmudrii Yes.

@vladimirvivien

This comment has been minimized.

Copy link
Member Author

commented May 29, 2019

@liggitt PTAL

if !found {

case policy.CSI:
if !utilfeature.DefaultFeatureGate.Enabled(features.CSIInlineVolume) {

This comment has been minimized.

Copy link
@liggitt

liggitt May 29, 2019

Member

either drop the feature gate check here, or just skip all CSI checks if the feature is disabled (see how we deal with RunAsGroup in this file)... don't add blanket validation failures if the feature is disabled.

@vladimirvivien vladimirvivien force-pushed the vladimirvivien:csi-inline-podsec-policy branch from 20c0ca6 to 90d993c May 29, 2019

@vladimirvivien

This comment has been minimized.

Copy link
Member Author

commented May 29, 2019

@liggitt PTAL

@vladimirvivien vladimirvivien force-pushed the vladimirvivien:csi-inline-podsec-policy branch from 90d993c to d15ff34 May 29, 2019

@liggitt

This comment has been minimized.

Copy link
Member

commented May 29, 2019

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, vladimirvivien

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

@vladimirvivien

This comment has been minimized.

Copy link
Member Author

commented May 29, 2019

/test pull-kubernetes-verify

@vladimirvivien

This comment has been minimized.

Copy link
Member Author

commented May 29, 2019

/retest

@k8s-ci-robot k8s-ci-robot merged commit 3aecf43 into kubernetes:master May 30, 2019

21 checks passed

cla/linuxfoundation vladimirvivien authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps 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-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
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.