Merge pull request #84049 from jsafrane/block-feature-gate-checks

Add block feature gate checks to PV controller

pkg/controller/volume/persistentvolume/binder_test.go

@@ -19,7 +19,7 @@ package persistentvolume
 import (
 	"testing"
 
-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	storage "k8s.io/api/storage/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
@@ -621,9 +621,9 @@ func TestSyncBlockVolumeDisabled(t *testing.T) {
 			// syncVolume binds a requested block claim to a block volume
 			"14-1 - binding to volumeMode block",
 			withVolumeVolumeMode(&modeBlock, newVolumeArray("volume14-1", "10Gi", "", "", v1.VolumeAvailable, v1.PersistentVolumeReclaimRetain, classEmpty)),
-			withVolumeVolumeMode(&modeBlock, newVolumeArray("volume14-1", "10Gi", "uid14-1", "claim14-1", v1.VolumeBound, v1.PersistentVolumeReclaimRetain, classEmpty, pvutil.AnnBoundByController)),
+			withVolumeVolumeMode(&modeBlock, newVolumeArray("volume14-1", "10Gi", "", "", v1.VolumeAvailable, v1.PersistentVolumeReclaimRetain, classEmpty)),
+			withClaimVolumeMode(&modeBlock, newClaimArray("claim14-1", "uid14-1", "10Gi", "", v1.ClaimPending, nil)),
 			withClaimVolumeMode(&modeBlock, newClaimArray("claim14-1", "uid14-1", "10Gi", "", v1.ClaimPending, nil)),
-			withClaimVolumeMode(&modeBlock, newClaimArray("claim14-1", "uid14-1", "10Gi", "volume14-1", v1.ClaimBound, nil, pvutil.AnnBoundByController, pvutil.AnnBindCompleted)),
 			noevents, noerrors, testSyncClaim,
 		},
 		{
@@ -657,9 +657,9 @@ func TestSyncBlockVolumeDisabled(t *testing.T) {
 			// syncVolume binds a requested filesystem claim to an unspecified volumeMode for volume
 			"14-5 - binding different volumeModes should be ignored",
 			withVolumeVolumeMode(&modeBlock, newVolumeArray("volume14-5", "10Gi", "", "", v1.VolumeAvailable, v1.PersistentVolumeReclaimRetain, classEmpty)),
-			withVolumeVolumeMode(&modeBlock, newVolumeArray("volume14-5", "10Gi", "uid14-5", "claim14-5", v1.VolumeBound, v1.PersistentVolumeReclaimRetain, classEmpty, pvutil.AnnBoundByController)),
+			withVolumeVolumeMode(&modeBlock, newVolumeArray("volume14-5", "10Gi", "", "", v1.VolumeAvailable, v1.PersistentVolumeReclaimRetain, classEmpty)),
+			withClaimVolumeMode(&modeFile, newClaimArray("claim14-5", "uid14-5", "10Gi", "", v1.ClaimPending, nil)),
 			withClaimVolumeMode(&modeFile, newClaimArray("claim14-5", "uid14-5", "10Gi", "", v1.ClaimPending, nil)),
-			withClaimVolumeMode(&modeFile, newClaimArray("claim14-5", "uid14-5", "10Gi", "volume14-5", v1.ClaimBound, nil, pvutil.AnnBoundByController, pvutil.AnnBindCompleted)),
 			noevents, noerrors, testSyncClaim,
 		},
 	}

pkg/controller/volume/persistentvolume/index_test.go

@@ -20,7 +20,7 @@ import (
 	"sort"
 	"testing"
 
-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
@@ -1118,19 +1118,19 @@ func TestVolumeModeCheck(t *testing.T) {
 			enableBlock:        true,
 		},
 		"feature disabled - pvc block and pv filesystem": {
-			isExpectedMismatch: false,
+			isExpectedMismatch: true,
 			vol:                createVolumeModeFilesystemTestVolume(),
 			pvc:                makeVolumeModePVC("8G", &blockMode, nil),
 			enableBlock:        false,
 		},
 		"feature disabled - pvc filesystem and pv block": {
-			isExpectedMismatch: false,
+			isExpectedMismatch: true,
 			vol:                createVolumeModeBlockTestVolume(),
 			pvc:                makeVolumeModePVC("8G", &filesystemMode, nil),
 			enableBlock:        false,
 		},
 		"feature disabled - pvc block and pv block": {
-			isExpectedMismatch: false,
+			isExpectedMismatch: true,
 			vol:                createVolumeModeBlockTestVolume(),
 			pvc:                makeVolumeModePVC("8G", &blockMode, nil),
 			enableBlock:        false,
@@ -1146,10 +1146,7 @@ func TestVolumeModeCheck(t *testing.T) {
 	for name, scenario := range scenarios {
 		t.Run(name, func(t *testing.T) {
 			defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.BlockVolume, scenario.enableBlock)()
-			expectedMismatch, err := pvutil.CheckVolumeModeMismatches(&scenario.pvc.Spec, &scenario.vol.Spec)
+			expectedMismatch := pvutil.CheckVolumeModeMismatches(&scenario.pvc.Spec, &scenario.vol.Spec)
-			if err != nil {
-				t.Errorf("Unexpected failure for checkVolumeModeMismatches: %v", err)
-			}
 			// expected to match but either got an error or no returned pvmatch
 			if expectedMismatch && !scenario.isExpectedMismatch {
 				t.Errorf("Unexpected failure for scenario, expected not to mismatch on modes but did: %s", name)
@@ -1222,7 +1219,7 @@ func TestFilteringVolumeModes(t *testing.T) {
 			enableBlock:     false,
 		},
 		"2-2 feature disabled - pvc mode is block and pv mode is block - fields should be dropped by api and not analyzed with gate disabled": {
-			isExpectedMatch: true,
+			isExpectedMatch: false,
 			vol:             createTestVolOrderedIndex(createVolumeModeBlockTestVolume()),
 			pvc:             makeVolumeModePVC("8G", &blockMode, nil),
 			enableBlock:     false,

pkg/controller/volume/persistentvolume/pv_controller.go

@@ -268,11 +268,7 @@ func checkVolumeSatisfyClaim(volume *v1.PersistentVolume, claim *v1.PersistentVo
 		return fmt.Errorf("storageClassName does not match")
 	}
 
-	isMismatch, err := pvutil.CheckVolumeModeMismatches(&claim.Spec, &volume.Spec)
+	if pvutil.CheckVolumeModeMismatches(&claim.Spec, &volume.Spec) {
-	if err != nil {
-		return fmt.Errorf("error checking volumeMode: %v", err)
-	}
-	if isMismatch {
 		return fmt.Errorf("incompatible volumeMode")
 	}
 
@@ -589,7 +585,7 @@ func (ctrl *PersistentVolumeController) syncVolume(volume *v1.PersistentVolume)
 			}
 			return nil
 		} else if claim.Spec.VolumeName == "" {
-			if isMismatch, err := pvutil.CheckVolumeModeMismatches(&claim.Spec, &volume.Spec); err != nil || isMismatch {
+			if pvutil.CheckVolumeModeMismatches(&claim.Spec, &volume.Spec) {
 				// Binding for the volume won't be called in syncUnboundClaim,
 				// because findBestMatchForClaim won't return the volume due to volumeMode mismatch.
 				volumeMsg := fmt.Sprintf("Cannot bind PersistentVolume to requested PersistentVolumeClaim %q due to incompatible volumeMode.", claim.Name)

pkg/controller/volume/persistentvolume/util/util.go

@@ -19,7 +19,7 @@ package persistentvolume
 import (
 	"fmt"
 
-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	storage "k8s.io/api/storage/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/resource"
@@ -207,13 +207,8 @@ func FindMatchingVolume(
 
 		volumeQty := volume.Spec.Capacity[v1.ResourceStorage]
 
-		// check if volumeModes do not match (feature gate protected)
-		isMismatch, err := CheckVolumeModeMismatches(&claim.Spec, &volume.Spec)
-		if err != nil {
-			return nil, fmt.Errorf("error checking if volumeMode was a mismatch: %v", err)
-		}
 		// filter out mismatching volumeModes
-		if isMismatch {
+		if CheckVolumeModeMismatches(&claim.Spec, &volume.Spec) {
 			continue
 		}
 
@@ -309,9 +304,22 @@ func FindMatchingVolume(
 
 // CheckVolumeModeMismatches is a convenience method that checks volumeMode for PersistentVolume
 // and PersistentVolumeClaims
-func CheckVolumeModeMismatches(pvcSpec *v1.PersistentVolumeClaimSpec, pvSpec *v1.PersistentVolumeSpec) (bool, error) {
+func CheckVolumeModeMismatches(pvcSpec *v1.PersistentVolumeClaimSpec, pvSpec *v1.PersistentVolumeSpec) bool {
 	if !utilfeature.DefaultFeatureGate.Enabled(features.BlockVolume) {
-		return false, nil
+		if pvcSpec.VolumeMode != nil && *pvcSpec.VolumeMode == v1.PersistentVolumeBlock {
+			// Block PVC does not match anything when the feature is off. We explicitly want
+			// to prevent binding block PVC to filesystem PV.
+			// The PVC should be ignored by PV controller.
+			return true
+		}
+		if pvSpec.VolumeMode != nil && *pvSpec.VolumeMode == v1.PersistentVolumeBlock {
+			// Block PV does not match anything when the feature is off. We explicitly want
+			// to prevent binding block PV to filesystem PVC.
+			// The PV should be ignored by PV controller.
+			return true
+		}
+		// Both PV + PVC are not block.
+		return false
 	}
 
 	// In HA upgrades, we cannot guarantee that the apiserver is on a version >= controller-manager.
@@ -324,7 +332,7 @@ func CheckVolumeModeMismatches(pvcSpec *v1.PersistentVolumeClaimSpec, pvSpec *v1
 	if pvSpec.VolumeMode != nil {
 		pvVolumeMode = *pvSpec.VolumeMode
 	}
-	return requestedVolumeMode != pvVolumeMode, nil
+	return requestedVolumeMode != pvVolumeMode
 }
 
 // CheckAccessModes returns true if PV satisfies all the PVC's requested AccessModes