diff --git a/pkg/api/persistentvolume/BUILD b/pkg/api/persistentvolume/BUILD index c4743ccc70..7983e32031 100644 --- a/pkg/api/persistentvolume/BUILD +++ b/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", + ], +) diff --git a/pkg/api/persistentvolume/util.go b/pkg/api/persistentvolume/util.go index 3b28f207a1..298a38250f 100644 --- a/pkg/api/persistentvolume/util.go +++ b/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 + } } } diff --git a/pkg/api/persistentvolume/util_test.go b/pkg/api/persistentvolume/util_test.go new file mode 100644 index 0000000000..6ea33b5a38 --- /dev/null +++ b/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)) + } + }) + } +} diff --git a/pkg/api/v1/persistentvolume/BUILD b/pkg/api/v1/persistentvolume/BUILD index 0a5cbd81f5..06cd006971 100644 --- a/pkg/api/v1/persistentvolume/BUILD +++ b/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", ], ) diff --git a/pkg/api/v1/persistentvolume/util.go b/pkg/api/v1/persistentvolume/util.go index 063c3ff8e7..e43ae58d2e 100644 --- a/pkg/api/v1/persistentvolume/util.go +++ b/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 - } -} diff --git a/pkg/api/v1/persistentvolume/util_test.go b/pkg/api/v1/persistentvolume/util_test.go index 56473d37bc..62e60cc6e9 100644 --- a/pkg/api/v1/persistentvolume/util_test.go +++ b/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") - } - } -} diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 007a4e8e53..6f0302c37f 100644 --- a/pkg/apis/core/validation/validation.go +++ b/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 { diff --git a/pkg/registry/core/persistentvolume/strategy.go b/pkg/registry/core/persistentvolume/strategy.go index eb172251a8..312fa8cfb4 100644 --- a/pkg/registry/core/persistentvolume/strategy.go +++ b/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 {