Merge pull request #173 from jsafrane/fix-remove-finalizers

Fix removal of PV protection finalizer

controller/controller.go

@@ -54,7 +54,7 @@ import (
 	"k8s.io/client-go/tools/record"
 	ref "k8s.io/client-go/tools/reference"
 	"k8s.io/client-go/util/workqueue"
-	"k8s.io/klog/v2"
+	klog "k8s.io/klog/v2"
 	"sigs.k8s.io/sig-storage-lib-external-provisioner/v10/controller/metrics"
 	"sigs.k8s.io/sig-storage-lib-external-provisioner/v10/util"
 )
@@ -1588,7 +1588,6 @@ func (ctrl *ProvisionController) deleteVolumeOperation(ctx context.Context, volu
 				return fmt.Errorf("expected volume but got %+v", volumeObj)
 			}
 			finalizers, modified := removeFinalizer(newVolume.ObjectMeta.Finalizers, finalizerPV)
-
 			// Only update the finalizers if we actually removed something
 			if modified {
 				if _, err = ctrl.patchPersistentVolumeWithFinalizers(ctx, newVolume, finalizers); err != nil {
@@ -1607,23 +1606,21 @@ func (ctrl *ProvisionController) deleteVolumeOperation(ctx context.Context, volu
 	return nil
 }
 
-// removeFinalizer removes finalizer from slice, returns slice and whether modified.
+// removeFinalizer removes finalizer from slice, returns the new slice and whether modified.
+// It does not modify the original slice.
 func removeFinalizer(finalizers []string, finalizerToRemove string) ([]string, bool) {
-	for i, finalizer := range finalizers {
-		if finalizer == finalizerToRemove {
-			finalizers = append(finalizers[:i], finalizers[i+1:]...)
-			if len(finalizers) == 0 {
-				finalizers = nil
-			}
-			return finalizers, true
+	ret := make([]string, 0, len(finalizers))
+	for _, finalizer := range finalizers {
+		if finalizer != finalizerToRemove {
+			ret = append(ret, finalizer)
 		}
 	}
 
-	if len(finalizers) == 0 {
-		finalizers = nil
+	if len(ret) == 0 {
+		ret = nil
 	}
 
-	return finalizers, false
+	return ret, len(ret) != len(finalizers)
 }
 
 // addFinalizer adds finalizer to slice, returns slice and whether modified.

controller/controller_test.go

@@ -1406,13 +1406,22 @@ func TestRemoveFinalizer(t *testing.T) {
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
 
+			var originalFinalizers []string
+			if test.finalizers != nil {
+				originalFinalizers = make([]string, len(test.finalizers))
+				copy(originalFinalizers, test.finalizers)
+			}
+
 			modifiedFinalizers, modified := removeFinalizer(test.finalizers, test.finalizerToRemove)
 			if test.expectedModified != modified {
 				t.Errorf("expected modified %v but got %v\n", test.expectedModified, modified)
 			}
 			if !reflect.DeepEqual(test.expectedFinalizers, modifiedFinalizers) {
 				t.Errorf("expected finalizers %v but got %v\n", test.expectedFinalizers, modifiedFinalizers)
 			}
+			if !reflect.DeepEqual(test.finalizers, originalFinalizers) {
+				t.Errorf("removeFinalizer() has modified the source finalizers %+v to %+v", originalFinalizers, test.finalizers)
+			}
 		})
 	}
 }