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

ephemeral mode check #80568

Merged
merged 6 commits into from Aug 26, 2019

Conversation

@pohly
Copy link
Contributor

commented Jul 25, 2019

What type of PR is this?
/kind api-change

What this PR does / why we need it:

This makes it safer to use ephemeral inline volumes (an alpha feature in 1.15) because it prevents accidental usage of a normal CSI driver for an ephemeral inline volume. The whitelisting of CSI drivers via a pod security policy would have the same effect, but might not be active in a cluster.

Special notes for your reviewer:

Which issue(s) this PR fixes:
Fixes: #79624
Related-to: #79983, which has been merged

Does this PR introduce a user-facing change?:

a CSI driver that supports ephemeral inline volumes must explicitly declare that by providing a CSIDriver object where the new "mode" field is set to "ephemeral" or "persistent+ephemeral"

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/blob/master/keps/sig-storage/20190122-csi-inline-volumes.md#driver-mode
@pohly

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

/priority important-soon
/cc @vladimirvivien

}
driver := getTestCSIDriver(testDriver, nil, nil, &driverMode)
fakeClient := fakeclient.NewSimpleClientset(driver)
plug, tmpDir := newTestPlugin(t, fakeClient)
defer os.RemoveAll(tmpDir)

This comment has been minimized.

Copy link
@cwdsuzhou

cwdsuzhou Jul 25, 2019

Member

since we use defer in foreach,using t.Run is better

This comment has been minimized.

Copy link
@pohly

pohly Jul 25, 2019

Author Contributor

Right, good catch. Other tests already did that and I missed that this one doesn't. Will fix it.

This comment has been minimized.

Copy link
@pohly

pohly Jul 26, 2019

Author Contributor

Actually there was a t.Run, I just moved the code into the wrong spot. Fixed.

@pohly

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

Before actually asking for an API review, I need to rebase and split the commit into smaller ones (just the API change, generated files, additional code and tests).

Right now I am just trying to find out what I might have missed (like updating some more generated files) and why the roundtrip test is failing.

@pohly pohly force-pushed the pohly:ephemeral-mode-check branch from df562ba to 4de69dc Jul 25, 2019

@vladimirvivien
Copy link
Member

left a comment

Looking good, left comments for some minor issues.

// supportsVolumeMode looks up CSIDriver object associated with the
// driver name to determine if the driver supports the given volume
// mode. An error indicates that it isn't supported and explains why.
func (p *csiPlugin) supportsVolumeMode(driver string, volumeMode csiVolumeMode) error {

This comment has been minimized.

Copy link
@vladimirvivien

vladimirvivien Jul 25, 2019

Member

This probably would be better as a isVolumeSuppoerted(driver, mode) (bool, error) Returns true if supported, false if not, and error if something went wrong while attempting to check.

This comment has been minimized.

Copy link
@pohly

pohly Jul 25, 2019

Author Contributor

I had that initially, but then there's no way to convey to the caller what the reason is when the mode isn't supported.

These two error message would have to be removed:

case isDefault:
// Using the same error message might be confusing because 'which only supports "persistent"' would
// be wrong for a driver that merely doesn't specify that it supports ephemeral.
return fmt.Errorf("volume mode %q not supported by driver %s, no CSIDriver object or no Mode specified", volumeMode, driver)
default:
return fmt.Errorf("volume mode %q not supported by driver %s which only supports %q", volumeMode, driver, driverMode)

}

// lookupDriverMode looks up CSIDriver object associated with the driver name and
// returns what it finds there or the "persistent" default.

This comment has been minimized.

Copy link
@vladimirvivien

vladimirvivien Jul 25, 2019

Member

Please clarify in comment what the bool return param is for is for.

This comment has been minimized.

Copy link
@pohly

pohly Jul 25, 2019

Author Contributor

Will do. It tells the caller whether the returned string was explicitly set or is the default value. That's relevant when it comes to producing a good error message.

Would it be better to return the empty string for "not set" and drop the bool?

This comment has been minimized.

Copy link
@pohly

pohly Jul 26, 2019

Author Contributor

Would it be better to return the empty string for "not set" and drop the bool?

I changed it to that. @vladimirvivien: okay?

if csiDriver.Spec.Mode == nil {
return storage.PersistentDriverMode, true, nil
}
return *csiDriver.Spec.Mode, false, nil

This comment has been minimized.

Copy link
@vladimirvivien

vladimirvivien Jul 25, 2019

Member

Should true be returned here since driver mode was set ?

return "", false, err
}
if csiDriver.Spec.Mode == nil {
return storage.PersistentDriverMode, true, nil

This comment has been minimized.

Copy link
@vladimirvivien

vladimirvivien Jul 25, 2019

Member

Should false be returned since driver mode is not set. the boolean needs to be clarified I suppose.

@pohly pohly force-pushed the pohly:ephemeral-mode-check branch from 4de69dc to 1a14c40 Jul 25, 2019

@msau42

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

/assign @msau42 @jsafrane

// - "persistent+ephemeral": both modes supported
// More modes may be added in the future.
// +optional
Mode *CSIDriverMode

This comment has been minimized.

Copy link
@msau42

This comment has been minimized.

Copy link
@pohly

pohly Jul 25, 2019

Author Contributor

Agreed, will add this. I just wanted to fix the roundtrip error first before working on making the API nicer to use.

This comment has been minimized.

Copy link
@pohly

pohly Jul 26, 2019

Author Contributor

Done.

@@ -47,4 +47,8 @@ func SetDefaults_CSIDriver(obj *storagev1beta1.CSIDriver) {
obj.Spec.PodInfoOnMount = new(bool)
*(obj.Spec.PodInfoOnMount) = false
}
if obj.Spec.Mode == nil {

This comment has been minimized.

Copy link
@msau42

msau42 Jul 25, 2019

Member

I think this needs to be feature gated

This comment has been minimized.

Copy link
@msau42

msau42 Jul 25, 2019

Member

We also need to drop the alpha field in PrepareForCreate, PrepareForUpdate calls in https://github.com/kubernetes/kubernetes/blob/master/pkg/registry/storage/csidriver/strategy.go. Take a look at this example

This comment has been minimized.

Copy link
@pohly

pohly Jul 25, 2019

Author Contributor

Okay. But will this help with the roundtrip problem?

This comment has been minimized.

Copy link
@pohly

pohly Jul 25, 2019

Author Contributor

And which part of that example are the hand-written files? They all come from bb45b8e#diff-da9973f9d53ca756ac31a053eb90161c, which includes everything in a single commit.

This comment has been minimized.

Copy link
@msau42

msau42 Jul 25, 2019

Member

the entire strategy.go is a handwritten file

This comment has been minimized.

Copy link
@msau42

msau42 Jul 25, 2019

Member

To fix the roundtripping test, I think we need to modify the fuzzer

This comment has been minimized.

Copy link
@pohly

pohly Jul 26, 2019

Author Contributor

I added the feature gate check to defaults.go and strategy.go. With the feature disabled, the roundtrip test passes, but of course breaks again when enabling it by default.

To fix the roundtripping test, I think we need to modify the fuzzer

Yes, that did the trick.

I'm going to squash the following change into the commit:

diff --git a/pkg/apis/storage/fuzzer/fuzzer.go b/pkg/apis/storage/fuzzer/fuzzer.go
index 2081ffe846..c205f2c4f7 100644
--- a/pkg/apis/storage/fuzzer/fuzzer.go
+++ b/pkg/apis/storage/fuzzer/fuzzer.go
@@ -46,6 +46,10 @@ var Funcs = func(codecs runtimeserializer.CodecFactory) []interface{} {
                                obj.Spec.PodInfoOnMount = new(bool)
                                *(obj.Spec.PodInfoOnMount) = false
                        }
+                       if obj.Spec.Mode == nil {
+                               obj.Spec.Mode = new(storage.CSIDriverMode)
+                               *obj.Spec.Mode = storage.PersistentDriverMode
+                       }
                },
        }
 }
diff --git a/pkg/apis/storage/v1beta1/defaults.go b/pkg/apis/storage/v1beta1/defaults.go
index 05bc6dbb3e..75acec10f3 100644
--- a/pkg/apis/storage/v1beta1/defaults.go
+++ b/pkg/apis/storage/v1beta1/defaults.go
@@ -20,6 +20,8 @@ import (
        "k8s.io/api/core/v1"
        storagev1beta1 "k8s.io/api/storage/v1beta1"
        "k8s.io/apimachinery/pkg/runtime"
+       utilfeature "k8s.io/apiserver/pkg/util/feature"
+       "k8s.io/kubernetes/pkg/features"
 )
 
 func addDefaultingFuncs(scheme *runtime.Scheme) error {
@@ -47,7 +49,7 @@ func SetDefaults_CSIDriver(obj *storagev1beta1.CSIDriver) {
                obj.Spec.PodInfoOnMount = new(bool)
                *(obj.Spec.PodInfoOnMount) = false
        }
-       if obj.Spec.Mode == nil {
+       if obj.Spec.Mode == nil && utilfeature.DefaultFeatureGate.Enabled(features.CSIInlineVolume) {
                obj.Spec.Mode = new(storagev1beta1.CSIDriverMode)
                *(obj.Spec.Mode) = storagev1beta1.PersistentDriverMode
        }
diff --git a/pkg/registry/storage/csidriver/strategy.go b/pkg/registry/storage/csidriver/strategy.go
index 1f534b2e76..1b31a7b23a 100644
--- a/pkg/registry/storage/csidriver/strategy.go
+++ b/pkg/registry/storage/csidriver/strategy.go
@@ -22,9 +22,11 @@ import (
        "k8s.io/apimachinery/pkg/runtime"
        "k8s.io/apimachinery/pkg/util/validation/field"
        "k8s.io/apiserver/pkg/storage/names"
+       utilfeature "k8s.io/apiserver/pkg/util/feature"
        "k8s.io/kubernetes/pkg/api/legacyscheme"
        "k8s.io/kubernetes/pkg/apis/storage"
        "k8s.io/kubernetes/pkg/apis/storage/validation"
+       "k8s.io/kubernetes/pkg/features"
 )
 
 // csiDriverStrategy implements behavior for CSIDriver objects
@@ -41,8 +43,12 @@ func (csiDriverStrategy) NamespaceScoped() bool {
        return false
 }
 
-// ResetBeforeCreate clears the Status field which is not allowed to be set by end users on creation.
+// PrepareForCreate clears the Mode field if the corresponding feature is disabled.
 func (csiDriverStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) {
+       if !utilfeature.DefaultFeatureGate.Enabled(features.CSIInlineVolume) {
+               csiDriver := obj.(*storage.CSIDriver)
+               csiDriver.Spec.Mode = nil
+       }
 }
 
 func (csiDriverStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList {
@@ -62,8 +68,12 @@ func (csiDriverStrategy) AllowCreateOnUpdate() bool {
        return false
 }
 
-// PrepareForUpdate sets the Status fields which is not allowed to be set by an end user updating a CSIDriver
+// PrepareForUpdate clears the Mode field if the corresponding feature is disabled.
 func (csiDriverStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) {
+       if !utilfeature.DefaultFeatureGate.Enabled(features.CSIInlineVolume) {
+               newCSIDriver := obj.(*storage.CSIDriver)
+               newCSIDriver.Spec.Mode = nil
+       }
 }
 
 func (csiDriverStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList {

@pohly pohly force-pushed the pohly:ephemeral-mode-check branch from 1a14c40 to 16aa09b Jul 26, 2019

@msau42

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 14, 2019

@pohly

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

@smarterclayton: ping - is the API okay?

@smarterclayton

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

Need to remove the validation gating as Jordan described.

@kfox1111

This comment has been minimized.

Copy link

commented Aug 21, 2019

Is this the last thing needed to get ephemeral to beta?

@msau42

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

@kfox1111 feature gate needs to be flipped + docs

@pohly pohly force-pushed the pohly:ephemeral-mode-check branch from eef8a70 to 8270fe8 Aug 22, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label Aug 22, 2019

@pohly

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

/retest

@msau42

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 22, 2019

@pohly

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

@smarterclayton: sorry for the delay, I am on vacation and missed the new comments last week. The PR should now be as discussed.

@smarterclayton

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

/approve

API change for alpha gated feature in 1.16 approved.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: msau42, pohly, smarterclayton

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

pohly added a commit to pohly/kubernetes that referenced this pull request Aug 26, 2019
storage: enhance test for ValidateCSIDriverUpdate
This revised test changes one field at a time in various ways. This
ensures that one failure reason doesn't mask the other.

An incorrect comment also gets fixed.

Suggested in kubernetes#80568 (review).

@k8s-ci-robot k8s-ci-robot merged commit 087aafc into kubernetes:master Aug 26, 2019

23 checks passed

cla/linuxfoundation pohly 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 Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Job succeeded.
Details
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

@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 26, 2019

pohly added a commit to pohly/kubernetes that referenced this pull request Aug 26, 2019
storage: enhance test for ValidateCSIDriverUpdate
This revised test changes one field at a time in various ways. This
ensures that one failure reason doesn't mask the other.

An incorrect comment also gets fixed.

Suggested in kubernetes#80568 (review).
pohly added a commit to pohly/kubernetes that referenced this pull request Aug 26, 2019
e2e storage: enhance test for ValidateCSIDriverUpdate
This revised test changes one field at a time in various ways. This
ensures that one failure reason doesn't mask the other.

An incorrect comment also gets fixed.

Suggested in kubernetes#80568 (review).
pohly added a commit to pohly/kubernetes that referenced this pull request Aug 26, 2019
e2e storage: enhance test for ValidateCSIDriverUpdate
This revised test changes one field at a time in various ways. This
ensures that one failure reason doesn't mask the other.

An incorrect comment also gets fixed.

Suggested in kubernetes#80568 (review).
pohly added a commit to pohly/kubernetes that referenced this pull request Aug 26, 2019
storage: enhance test for ValidateCSIDriverUpdate
This revised test changes one field at a time in various ways. This
ensures that one failure reason doesn't mask the other.

An incorrect comment also gets fixed.

Suggested in kubernetes#80568 (review).
pohly added a commit to pohly/kubernetes that referenced this pull request Aug 26, 2019
storage: enhance test for ValidateCSIDriverUpdate
This revised test changes one field at a time in various ways. This
ensures that one failure reason doesn't mask the other.

An incorrect comment also gets fixed.

Suggested in kubernetes#80568 (review).

@liggitt liggitt moved this from Assigned to API review completed, 1.16 in API Reviews Aug 29, 2019

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.