Tolerate existing CSI fields in PVs when the feature is disabled

pkg/api/persistentvolume/BUILD

 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

+/*
+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/validation/validation.go

@@ -1463,10 +1463,6 @@ func ValidateCSIDriverName(driverName string, fldPath *field.Path) field.ErrorLi
 func validateCSIPersistentVolumeSource(csi *core.CSIPersistentVolumeSource, 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"))
-	}
-
 	allErrs = append(allErrs, ValidateCSIDriverName(csi.Driver, fldPath.Child("driver"))...)
 
 	if len(csi.VolumeHandle) == 0 {

pkg/registry/core/persistentvolume/strategy.go

@@ -53,7 +53,7 @@ func (persistentvolumeStrategy) PrepareForCreate(ctx context.Context, obj runtim
 	pv := obj.(*api.PersistentVolume)
 	pv.Status = api.PersistentVolumeStatus{}
 
-	pvutil.DropDisabledAlphaFields(&pv.Spec)
+	pvutil.DropDisabledFields(&pv.Spec, nil)
 }
 
 func (persistentvolumeStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList {
@@ -76,8 +76,7 @@ func (persistentvolumeStrategy) PrepareForUpdate(ctx context.Context, obj, old r
 	oldPv := old.(*api.PersistentVolume)
 	newPv.Status = oldPv.Status
 
-	pvutil.DropDisabledAlphaFields(&newPv.Spec)
-	pvutil.DropDisabledAlphaFields(&oldPv.Spec)
+	pvutil.DropDisabledFields(&newPv.Spec, &oldPv.Spec)
 }
 
 func (persistentvolumeStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList {