Revert patch for empty finalizers

Signed-off-by: Grant Griffiths <ggriffiths@purestorage.com>
kubernetes-csi · Sep 30, 2021 · 0c60d1e · 0c60d1e
1 parent d14e2ee
commit 0c60d1e
Show file tree

Hide file tree

Showing 3 changed files with 45 additions and 20 deletions.
diff --git a/pkg/common-controller/framework_test.go b/pkg/common-controller/framework_test.go
@@ -326,9 +326,7 @@ func (r *snapshotReactor) React(action core.Action) (handled bool, ret runtime.O
 		return true, snapshot, nil
 
 	case action.Matches("patch", "volumesnapshots"):
-		snapshot := &crdv1.VolumeSnapshot{}
 		action := action.(core.PatchAction)
-
 		// Check and bump object version
 		storedSnapshot, found := r.snapshots[action.GetName()]
 		if found {
@@ -359,9 +357,10 @@ func (r *snapshotReactor) React(action core.Action) (handled bool, ret runtime.O
 		}
 
 		// Store the updated object to appropriate places.
-		r.snapshots[snapshot.Name] = storedSnapshot
+		r.snapshots[storedSnapshot.Name] = storedSnapshot
 		r.changedObjects = append(r.changedObjects, storedSnapshot)
 		r.changedSinceLastSync++
+
 		klog.V(4).Infof("saved updated snapshot %s", storedSnapshot.Name)
 		return true, storedSnapshot, nil
 
@@ -1253,6 +1252,10 @@ func testAddSnapshotFinalizer(ctrl *csiSnapshotCommonController, reactor *snapsh
 	return ctrl.addSnapshotFinalizer(test.initialSnapshots[0], true, true)
 }
 
+func testAddSingleSnapshotFinalizer(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error {
+	return ctrl.addSnapshotFinalizer(test.initialSnapshots[0], false, true)
+}
+
 func testRemoveSnapshotFinalizer(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error {
 	return ctrl.removeSnapshotFinalizer(test.initialSnapshots[0], true, true)
 }
@@ -1510,24 +1513,29 @@ func evaluateFinalizerTests(ctrl *csiSnapshotCommonController, reactor *snapshot
 		if funcName == "testAddSnapshotFinalizer" {
 			for _, snapshot := range reactor.snapshots {
 				if test.initialSnapshots[0].Name == snapshot.Name {
-					if !utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) && utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) &&
-						!utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) && utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) {
+					if !utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) &&
+						utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) &&
+						!utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) &&
+						utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) {
 						klog.V(4).Infof("test %q succeeded. Finalizers are added to snapshot %s", test.name, snapshot.Name)
 						bHasSnapshotFinalizer = true
 					}
 					break
 				}
 			}
 			if test.expectSuccess && !bHasSnapshotFinalizer {
-				t.Errorf("Test %q: failed to add finalizer to Snapshot %s", test.name, test.initialSnapshots[0].Name)
+				t.Errorf("Test %q: failed to add finalizer to Snapshot %s. Finalizers: %s", test.name, test.initialSnapshots[0].Name, test.initialSnapshots[0].GetFinalizers())
 			}
 		}
 		bHasSnapshotFinalizer = true
 		if funcName == "testRemoveSnapshotFinalizer" {
 			for _, snapshot := range reactor.snapshots {
 				if test.initialSnapshots[0].Name == snapshot.Name {
-					if utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) && !utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) &&
-						utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) && !utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) {
+					if utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) &&
+						!utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) &&
+						utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) &&
+						!utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) {
+
 						klog.V(4).Infof("test %q succeeded. SnapshotFinalizer is removed from Snapshot %s", test.name, snapshot.Name)
 						bHasSnapshotFinalizer = false
 					}

diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go
@@ -1421,16 +1421,24 @@ func (ctrl *csiSnapshotCommonController) addSnapshotFinalizer(snapshot *crdv1.Vo
 	var updatedSnapshot *crdv1.VolumeSnapshot
 	var err error
 
-	var patches []utils.PatchOp
+	// Must perform an update if no finalizers exist
 	if len(snapshot.ObjectMeta.Finalizers) == 0 {
-		// Replace finalizers with new array if there are no other finalizers
-		patches = append(patches, utils.PatchOp{
-			Op:    "add",
-			Path:  "/metadata/finalizers",
-			Value: []string{utils.VolumeSnapshotContentFinalizer},
-		})
+		snapshotClone := snapshot.DeepCopy()
+		if addSourceFinalizer {
+			snapshotClone.ObjectMeta.Finalizers = append(snapshotClone.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer)
+		}
+		if addBoundFinalizer {
+			snapshotClone.ObjectMeta.Finalizers = append(snapshotClone.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer)
+		}
+		updatedSnapshot, err = ctrl.clientset.SnapshotV1().VolumeSnapshots(snapshotClone.Namespace).Update(context.TODO(), snapshotClone, metav1.UpdateOptions{})
+		if err != nil {
+			return newControllerUpdateError(utils.SnapshotKey(snapshot), err.Error())
+		}
 	} else {
 		// Otherwise, perform a patch
+		var patches []utils.PatchOp
+
+		// If finalizers exist already, add new ones to the end of the array
 		if addSourceFinalizer {
 			patches = append(patches, utils.PatchOp{
 				Op:    "add",
@@ -1445,10 +1453,11 @@ func (ctrl *csiSnapshotCommonController) addSnapshotFinalizer(snapshot *crdv1.Vo
 				Value: utils.VolumeSnapshotBoundFinalizer,
 			})
 		}
-	}
-	updatedSnapshot, err = utils.PatchVolumeSnapshot(snapshot, patches, ctrl.clientset)
-	if err != nil {
-		return newControllerUpdateError(utils.SnapshotKey(snapshot), err.Error())
+
+		updatedSnapshot, err = utils.PatchVolumeSnapshot(snapshot, patches, ctrl.clientset)
+		if err != nil {
+			return newControllerUpdateError(utils.SnapshotKey(snapshot), err.Error())
+		}
 	}
 
 	_, err = ctrl.storeSnapshotUpdate(updatedSnapshot)

diff --git a/pkg/common-controller/snapshot_finalizer_test.go b/pkg/common-controller/snapshot_finalizer_test.go
@@ -19,6 +19,7 @@ package common_controller
 import (
 	"testing"
 
+	"github.com/kubernetes-csi/external-snapshotter/v4/pkg/utils"
 	v1 "k8s.io/api/core/v1"
 )
 
@@ -70,7 +71,14 @@ func TestSnapshotFinalizer(t *testing.T) {
 			expectSuccess:    true,
 		},
 		{
-			name:             "2-1 - successful remove Snapshot finalizer",
+			name:             "2-2 - successful add single Snapshot finalizer with patch",
+			initialSnapshots: withSnapshotFinalizers(newSnapshotArray("snap6-2", "snapuid6-2", "claim6-2", "", classSilver, "", &False, nil, nil, nil, false, false, nil), utils.VolumeSnapshotBoundFinalizer),
+			initialClaims:    newClaimArray("claim6-2", "pvc-uid6-2", "1Gi", "volume6-2", v1.ClaimBound, &classEmpty),
+			test:             testAddSingleSnapshotFinalizer,
+			expectSuccess:    true,
+		},
+		{
+			name:             "2-3 - successful remove Snapshot finalizer",
 			initialSnapshots: newSnapshotArray("snap6-2", "snapuid6-2", "claim6-2", "", classSilver, "", &False, nil, nil, nil, false, true, nil),
 			initialClaims:    newClaimArray("claim6-2", "pvc-uid6-2", "1Gi", "volume6-2", v1.ClaimBound, &classEmpty),
 			test:             testRemoveSnapshotFinalizer,