From 691fec5e34d18411ccf7d3f3e9ca65eb8c1cc456 Mon Sep 17 00:00:00 2001
From: Jan Safranek <jsafrane@redhat.com>
Date: Wed, 24 Jan 2018 09:48:38 +0100
Subject: [PATCH] Don't bind PVs and PVCs with different access modes.

PVC pre-bound to a PV can bind to the PV only if it has correct access mode.
Report an event if it does not and keep the PVC Pending.
---
 .../volume/persistentvolume/binder_test.go    | 26 +++++++++++++++++++
 .../volume/persistentvolume/framework_test.go |  7 +++++
 .../volume/persistentvolume/pv_controller.go  | 13 ++++++----
 3 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/pkg/controller/volume/persistentvolume/binder_test.go b/pkg/controller/volume/persistentvolume/binder_test.go
index a34b595747f..cb916c08e03 100644
--- a/pkg/controller/volume/persistentvolume/binder_test.go
+++ b/pkg/controller/volume/persistentvolume/binder_test.go
@@ -198,6 +198,32 @@ func TestSync(t *testing.T) {
 			newClaimArray("claim1-1", "uid1-1", "1Gi", "volume1-1", v1.ClaimBound, &classWait, annBoundByController, annBindCompleted),
 			noevents, noerrors, testSyncClaim,
 		},
+		{
+			// syncClaim binds pre-bound PVC only to the volume it points to,
+			// even if there is smaller volume available
+			"1-15 - successful prebound PVC",
+			[]*v1.PersistentVolume{
+				newVolume("volume1-15_1", "10Gi", "", "", v1.VolumePending, v1.PersistentVolumeReclaimRetain, classEmpty),
+				newVolume("volume1-15_2", "1Gi", "", "", v1.VolumePending, v1.PersistentVolumeReclaimRetain, classEmpty),
+			},
+			[]*v1.PersistentVolume{
+				newVolume("volume1-15_1", "10Gi", "uid1-15", "claim1-15", v1.VolumeBound, v1.PersistentVolumeReclaimRetain, classEmpty, annBoundByController),
+				newVolume("volume1-15_2", "1Gi", "", "", v1.VolumePending, v1.PersistentVolumeReclaimRetain, classEmpty),
+			},
+			newClaimArray("claim1-15", "uid1-15", "1Gi", "volume1-15_1", v1.ClaimPending, nil),
+			withExpectedCapacity("10Gi", newClaimArray("claim1-15", "uid1-15", "1Gi", "volume1-15_1", v1.ClaimBound, nil, annBindCompleted)),
+			noevents, noerrors, testSyncClaim,
+		},
+		{
+			// syncClaim does not bind pre-bound PVC to PV with different AccessMode
+			"1-16 - successful prebound PVC",
+			// PV has ReadWriteOnce
+			newVolumeArray("volume1-16", "1Gi", "", "", v1.VolumePending, v1.PersistentVolumeReclaimRetain, classEmpty),
+			newVolumeArray("volume1-16", "1Gi", "", "", v1.VolumePending, v1.PersistentVolumeReclaimRetain, classEmpty),
+			claimWithAccessMode([]v1.PersistentVolumeAccessMode{v1.ReadWriteMany}, newClaimArray("claim1-16", "uid1-16", "1Gi", "volume1-16", v1.ClaimPending, nil)),
+			claimWithAccessMode([]v1.PersistentVolumeAccessMode{v1.ReadWriteMany}, newClaimArray("claim1-16", "uid1-16", "1Gi", "volume1-16", v1.ClaimPending, nil)),
+			noevents, noerrors, testSyncClaim,
+		},
 
 		// [Unit test set 2] User asked for a specific PV.
 		// Test the binding when pv.ClaimRef is already set by controller or
diff --git a/pkg/controller/volume/persistentvolume/framework_test.go b/pkg/controller/volume/persistentvolume/framework_test.go
index 5c165abd946..7720dd56076 100644
--- a/pkg/controller/volume/persistentvolume/framework_test.go
+++ b/pkg/controller/volume/persistentvolume/framework_test.go
@@ -802,6 +802,13 @@ func claimWithAnnotation(name, value string, claims []*v1.PersistentVolumeClaim)
 	return claims
 }
 
+// claimWithAccessMode saves given access into given claims.
+// Meant to be used to compose claims specified inline in a test.
+func claimWithAccessMode(modes []v1.PersistentVolumeAccessMode, claims []*v1.PersistentVolumeClaim) []*v1.PersistentVolumeClaim {
+	claims[0].Spec.AccessModes = modes
+	return claims
+}
+
 func testSyncClaim(ctrl *PersistentVolumeController, reactor *volumeReactor, test controllerTest) error {
 	return ctrl.syncClaim(test.initialClaims[0])
 }
diff --git a/pkg/controller/volume/persistentvolume/pv_controller.go b/pkg/controller/volume/persistentvolume/pv_controller.go
index 540455ff78e..593eed1516d 100644
--- a/pkg/controller/volume/persistentvolume/pv_controller.go
+++ b/pkg/controller/volume/persistentvolume/pv_controller.go
@@ -233,11 +233,6 @@ func (ctrl *PersistentVolumeController) syncClaim(claim *v1.PersistentVolumeClai
 func checkVolumeSatisfyClaim(volume *v1.PersistentVolume, claim *v1.PersistentVolumeClaim) error {
 	requestedQty := claim.Spec.Resources.Requests[v1.ResourceName(v1.ResourceStorage)]
 	requestedSize := requestedQty.Value()
-	isMisMatch, err := checkVolumeModeMisMatches(&claim.Spec, &volume.Spec)
-	if err != nil {
-		return fmt.Errorf("error checking volumeMode: %v", err)
-	}
-
 	volumeQty := volume.Spec.Capacity[v1.ResourceStorage]
 	volumeSize := volumeQty.Value()
 	if volumeSize < requestedSize {
@@ -249,10 +244,18 @@ func checkVolumeSatisfyClaim(volume *v1.PersistentVolume, claim *v1.PersistentVo
 		return fmt.Errorf("storageClasseName does not match")
 	}
 
+	isMisMatch, err := checkVolumeModeMisMatches(&claim.Spec, &volume.Spec)
+	if err != nil {
+		return fmt.Errorf("error checking volumeMode: %v", err)
+	}
 	if isMisMatch {
 		return fmt.Errorf("incompatible volumeMode")
 	}
 
+	if !checkAccessModes(claim, volume) {
+		return fmt.Errorf("incompatible accessMode")
+	}
+
 	return nil
 }