Promote CSIPersistentVolume feature to GA

pkg/api/persistentvolume/BUILD

@@ -1,9 +1,6 @@
 package(default_visibility = ["//visibility:public"])
 
-load(
-    "@io_bazel_rules_go//go:def.bzl",
-    "go_library",
-)
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
 
 go_library(
     name = "go_default_library",
@@ -28,3 +25,16 @@ filegroup(
     srcs = [":package-srcs"],
     tags = ["automanaged"],
 )
+
+go_test(
+    name = "go_default_test",
+    srcs = ["util_test.go"],
+    embed = [":go_default_library"],
+    deps = [
+        "//pkg/apis/core:go_default_library",
+        "//pkg/features:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/util/diff:go_default_library",
+        "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library",
+        "//staging/src/k8s.io/apiserver/pkg/util/feature/testing:go_default_library",
+    ],
+)

pkg/api/persistentvolume/util.go

@@ -22,10 +22,22 @@ import (
 	"k8s.io/kubernetes/pkg/features"
 )
 
-// DropDisabledAlphaFields removes disabled fields from the pv spec.
+// DropDisabledFields removes disabled fields from the pv spec.
 // This should be called from PrepareForCreate/PrepareForUpdate for all resources containing a pv spec.
-func DropDisabledAlphaFields(pvSpec *api.PersistentVolumeSpec) {
+func DropDisabledFields(pvSpec *api.PersistentVolumeSpec, oldPVSpec *api.PersistentVolumeSpec) {
 	if !utilfeature.DefaultFeatureGate.Enabled(features.BlockVolume) {
+		// TODO(liggitt): change this to only drop pvSpec.VolumeMode if (oldPVSpec == nil || oldPVSpec.VolumeMode == nil)
+		// Requires more coordinated changes to validation
 		pvSpec.VolumeMode = nil
+		if oldPVSpec != nil {
+			oldPVSpec.VolumeMode = nil
+		}
+	}
+
+	if !utilfeature.DefaultFeatureGate.Enabled(features.CSIPersistentVolume) {
+		// if this is a new PV, or the old PV didn't already have the CSI field, clear it
+		if oldPVSpec == nil || oldPVSpec.PersistentVolumeSource.CSI == nil {
+			pvSpec.PersistentVolumeSource.CSI = nil
+		}
 	}
 }

pkg/api/persistentvolume/util_test.go

@@ -0,0 +1,155 @@
+/*
+Copyright 2018 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package persistentvolume
+
+import (
+	"reflect"
+	"testing"
+
+	"k8s.io/apimachinery/pkg/util/diff"
+	utilfeature "k8s.io/apiserver/pkg/util/feature"
+	utilfeaturetesting "k8s.io/apiserver/pkg/util/feature/testing"
+	api "k8s.io/kubernetes/pkg/apis/core"
+	"k8s.io/kubernetes/pkg/features"
+)
+
+func TestDropDisabledFields(t *testing.T) {
+	specWithCSI := func() *api.PersistentVolumeSpec {
+		return &api.PersistentVolumeSpec{PersistentVolumeSource: api.PersistentVolumeSource{CSI: &api.CSIPersistentVolumeSource{}}}
+	}
+	specWithoutCSI := func() *api.PersistentVolumeSpec {
+		return &api.PersistentVolumeSpec{PersistentVolumeSource: api.PersistentVolumeSource{CSI: nil}}
+	}
+	specWithMode := func(mode *api.PersistentVolumeMode) *api.PersistentVolumeSpec {
+		return &api.PersistentVolumeSpec{VolumeMode: mode}
+	}
+
+	modeBlock := api.PersistentVolumeBlock
+
+	tests := map[string]struct {
+		oldSpec       *api.PersistentVolumeSpec
+		newSpec       *api.PersistentVolumeSpec
+		expectOldSpec *api.PersistentVolumeSpec
+		expectNewSpec *api.PersistentVolumeSpec
+		csiEnabled    bool
+		blockEnabled  bool
+	}{
+		"disabled csi clears new": {
+			csiEnabled:    false,
+			newSpec:       specWithCSI(),
+			expectNewSpec: specWithoutCSI(),
+			oldSpec:       nil,
+			expectOldSpec: nil,
+		},
+		"disabled csi clears update when old pv did not use csi": {
+			csiEnabled:    false,
+			newSpec:       specWithCSI(),
+			expectNewSpec: specWithoutCSI(),
+			oldSpec:       specWithoutCSI(),
+			expectOldSpec: specWithoutCSI(),
+		},
+		"disabled csi preserves update when old pv did use csi": {
+			csiEnabled:    false,
+			newSpec:       specWithCSI(),
+			expectNewSpec: specWithCSI(),
+			oldSpec:       specWithCSI(),
+			expectOldSpec: specWithCSI(),
+		},
+
+		"enabled csi preserves new": {
+			csiEnabled:    true,
+			newSpec:       specWithCSI(),
+			expectNewSpec: specWithCSI(),
+			oldSpec:       nil,
+			expectOldSpec: nil,
+		},
+		"enabled csi preserves update when old pv did not use csi": {
+			csiEnabled:    true,
+			newSpec:       specWithCSI(),
+			expectNewSpec: specWithCSI(),
+			oldSpec:       specWithoutCSI(),
+			expectOldSpec: specWithoutCSI(),
+		},
+		"enabled csi preserves update when old pv did use csi": {
+			csiEnabled:    true,
+			newSpec:       specWithCSI(),
+			expectNewSpec: specWithCSI(),
+			oldSpec:       specWithCSI(),
+			expectOldSpec: specWithCSI(),
+		},
+
+		"disabled block clears new": {
+			blockEnabled:  false,
+			newSpec:       specWithMode(&modeBlock),
+			expectNewSpec: specWithMode(nil),
+			oldSpec:       nil,
+			expectOldSpec: nil,
+		},
+		"disabled block clears update when old pv did not use block": {
+			blockEnabled:  false,
+			newSpec:       specWithMode(&modeBlock),
+			expectNewSpec: specWithMode(nil),
+			oldSpec:       specWithMode(nil),
+			expectOldSpec: specWithMode(nil),
+		},
+		// TODO: consider changing this case to preserve
+		"disabled block clears old and new on update when old pv did use block": {
+			blockEnabled:  false,
+			newSpec:       specWithMode(&modeBlock),
+			expectNewSpec: specWithMode(nil),
+			oldSpec:       specWithMode(&modeBlock),
+			expectOldSpec: specWithMode(nil),
+		},
+
+		"enabled block preserves new": {
+			blockEnabled:  true,
+			newSpec:       specWithMode(&modeBlock),
+			expectNewSpec: specWithMode(&modeBlock),
+			oldSpec:       nil,
+			expectOldSpec: nil,
+		},
+		"enabled block preserves update when old pv did not use block": {
+			blockEnabled:  true,
+			newSpec:       specWithMode(&modeBlock),
+			expectNewSpec: specWithMode(&modeBlock),
+			oldSpec:       specWithMode(nil),
+			expectOldSpec: specWithMode(nil),
+		},
+		"enabled block preserves update when old pv did use block": {
+			blockEnabled:  true,
+			newSpec:       specWithMode(&modeBlock),
+			expectNewSpec: specWithMode(&modeBlock),
+			oldSpec:       specWithMode(&modeBlock),
+			expectOldSpec: specWithMode(&modeBlock),
+		},
+	}
+
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.CSIPersistentVolume, tc.csiEnabled)()
+			defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.BlockVolume, tc.blockEnabled)()
+
+			DropDisabledFields(tc.newSpec, tc.oldSpec)
+			if !reflect.DeepEqual(tc.newSpec, tc.expectNewSpec) {
+				t.Error(diff.ObjectReflectDiff(tc.newSpec, tc.expectNewSpec))
+			}
+			if !reflect.DeepEqual(tc.oldSpec, tc.expectOldSpec) {
+				t.Error(diff.ObjectReflectDiff(tc.oldSpec, tc.expectOldSpec))
+			}
+		})
+	}
+}

pkg/api/v1/persistentvolume/BUILD

@@ -5,11 +5,7 @@ go_library(
     srcs = ["util.go"],
     importpath = "k8s.io/kubernetes/pkg/api/v1/persistentvolume",
     visibility = ["//visibility:public"],
-    deps = [
-        "//pkg/features:go_default_library",
-        "//staging/src/k8s.io/api/core/v1:go_default_library",
-        "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library",
-    ],
+    deps = ["//staging/src/k8s.io/api/core/v1:go_default_library"],
 )
 
 go_test(
@@ -18,11 +14,9 @@ go_test(
     embed = [":go_default_library"],
     deps = [
         "//pkg/apis/core:go_default_library",
-        "//pkg/features:go_default_library",
         "//staging/src/k8s.io/api/core/v1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library",
-        "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library",
     ],
 )
 

pkg/api/v1/persistentvolume/util.go

@@ -18,8 +18,6 @@ package persistentvolume
 
 import (
 	corev1 "k8s.io/api/core/v1"
-	utilfeature "k8s.io/apiserver/pkg/util/feature"
-	"k8s.io/kubernetes/pkg/features"
 )
 
 func getClaimRefNamespace(pv *corev1.PersistentVolume) string {
@@ -134,11 +132,3 @@ func VisitPVSecretNames(pv *corev1.PersistentVolume, visitor Visitor) bool {
 	}
 	return true
 }
-
-// DropDisabledAlphaFields removes disabled fields from the pv spec.
-// This should be called from PrepareForCreate/PrepareForUpdate for all resources containing a pv spec.
-func DropDisabledAlphaFields(pvSpec *corev1.PersistentVolumeSpec) {
-	if !utilfeature.DefaultFeatureGate.Enabled(features.BlockVolume) {
-		pvSpec.VolumeMode = nil
-	}
-}

pkg/api/v1/persistentvolume/util_test.go

@@ -25,9 +25,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/validation/field"
-	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	api "k8s.io/kubernetes/pkg/apis/core"
-	"k8s.io/kubernetes/pkg/features"
 )
 
 func TestPVSecrets(t *testing.T) {
@@ -272,56 +270,3 @@ func newHostPathType(pathType string) *corev1.HostPathType {
 	*hostPathType = corev1.HostPathType(pathType)
 	return hostPathType
 }
-
-func TestDropAlphaPVVolumeMode(t *testing.T) {
-	vmode := corev1.PersistentVolumeFilesystem
-
-	// PersistentVolume with VolumeMode set
-	pv := corev1.PersistentVolume{
-		Spec: corev1.PersistentVolumeSpec{
-			AccessModes: []corev1.PersistentVolumeAccessMode{corev1.ReadWriteOnce},
-			PersistentVolumeSource: corev1.PersistentVolumeSource{
-				HostPath: &corev1.HostPathVolumeSource{
-					Path: "/foo",
-					Type: newHostPathType(string(corev1.HostPathDirectory)),
-				},
-			},
-			StorageClassName: "test-storage-class",
-			VolumeMode:       &vmode,
-		},
-	}
-
-	// Enable alpha feature BlockVolume
-	err1 := utilfeature.DefaultFeatureGate.Set("BlockVolume=true")
-	if err1 != nil {
-		t.Fatalf("Failed to enable feature gate for BlockVolume: %v", err1)
-	}
-
-	// now test dropping the fields - should not be dropped
-	DropDisabledAlphaFields(&pv.Spec)
-
-	// check to make sure VolumeDevices is still present
-	// if featureset is set to true
-	if utilfeature.DefaultFeatureGate.Enabled(features.BlockVolume) {
-		if pv.Spec.VolumeMode == nil {
-			t.Error("VolumeMode in pv.Spec should not have been dropped based on feature-gate")
-		}
-	}
-
-	// Disable alpha feature BlockVolume
-	err := utilfeature.DefaultFeatureGate.Set("BlockVolume=false")
-	if err != nil {
-		t.Fatalf("Failed to disable feature gate for BlockVolume: %v", err)
-	}
-
-	// now test dropping the fields
-	DropDisabledAlphaFields(&pv.Spec)
-
-	// check to make sure VolumeDevices is nil
-	// if featureset is set to false
-	if !utilfeature.DefaultFeatureGate.Enabled(features.BlockVolume) {
-		if pv.Spec.VolumeMode != nil {
-			t.Error("DropDisabledAlphaFields VolumeMode for pv.Spec failed")
-		}
-	}
-}

pkg/apis/core/types.go

@@ -229,7 +229,7 @@ type PersistentVolumeSource struct {
 	// More info: https://releases.k8s.io/HEAD/examples/volumes/storageos/README.md
 	// +optional
 	StorageOS *StorageOSPersistentVolumeSource
-	// CSI (Container Storage Interface) represents storage that handled by an external CSI driver (Beta feature).
+	// CSI (Container Storage Interface) represents storage that handled by an external CSI driver.
 	// +optional
 	CSI *CSIPersistentVolumeSource
 }
@@ -1547,7 +1547,7 @@ type LocalVolumeSource struct {
 	FSType *string
 }
 
-// Represents storage that is managed by an external CSI volume driver (Beta feature)
+// Represents storage that is managed by an external CSI volume driver.
 type CSIPersistentVolumeSource struct {
 	// Driver is the name of the driver to use for this volume.
 	// Required.

pkg/apis/core/validation/validation.go

@@ -1443,24 +1443,27 @@ func validateStorageOSPersistentVolumeSource(storageos *core.StorageOSPersistent
 	return allErrs
 }
 
-func validateCSIPersistentVolumeSource(csi *core.CSIPersistentVolumeSource, fldPath *field.Path) field.ErrorList {
+func ValidateCSIDriverName(driverName string, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
 
-	if !utilfeature.DefaultFeatureGate.Enabled(features.CSIPersistentVolume) {
-		allErrs = append(allErrs, field.Forbidden(fldPath, "CSIPersistentVolume disabled by feature-gate"))
+	if len(driverName) == 0 {
+		allErrs = append(allErrs, field.Required(fldPath, ""))
 	}
 
-	if len(csi.Driver) == 0 {
-		allErrs = append(allErrs, field.Required(fldPath.Child("driver"), ""))
+	if len(driverName) > 63 {
+		allErrs = append(allErrs, field.TooLong(fldPath, driverName, 63))
 	}
 
-	if len(csi.Driver) > 63 {
-		allErrs = append(allErrs, field.TooLong(fldPath.Child("driver"), csi.Driver, 63))
+	if !csiDriverNameRexp.MatchString(driverName) {
+		allErrs = append(allErrs, field.Invalid(fldPath, driverName, validation.RegexError(csiDriverNameRexpErrMsg, csiDriverNameRexpFmt, "csi-hostpath")))
 	}
+	return allErrs
+}
 
-	if !csiDriverNameRexp.MatchString(csi.Driver) {
-		allErrs = append(allErrs, field.Invalid(fldPath.Child("driver"), csi.Driver, validation.RegexError(csiDriverNameRexpErrMsg, csiDriverNameRexpFmt, "csi-hostpath")))
-	}
+func validateCSIPersistentVolumeSource(csi *core.CSIPersistentVolumeSource, fldPath *field.Path) field.ErrorList {
+	allErrs := field.ErrorList{}
+
+	allErrs = append(allErrs, ValidateCSIDriverName(csi.Driver, fldPath.Child("driver"))...)
 
 	if len(csi.VolumeHandle) == 0 {
 		allErrs = append(allErrs, field.Required(fldPath.Child("volumeHandle"), ""))