Test framework improvements, add TODO test for conflict error.

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

go.mod

@@ -7,6 +7,7 @@ require (
 	github.com/evanphx/json-patch v5.9.0+incompatible
 	github.com/fsnotify/fsnotify v1.7.0
 	github.com/golang/mock v1.6.0
+	github.com/google/go-cmp v0.6.0
 	github.com/google/gofuzz v1.2.0
 	github.com/kubernetes-csi/csi-lib-utils v0.17.0
 	github.com/kubernetes-csi/csi-test/v5 v5.2.0
@@ -40,7 +41,6 @@ require (
 	github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect
 	github.com/golang/protobuf v1.5.3 // indirect
 	github.com/google/gnostic-models v0.6.8 // indirect
-	github.com/google/go-cmp v0.6.0 // indirect
 	github.com/google/uuid v1.6.0 // indirect
 	github.com/imdario/mergo v0.3.13 // indirect
 	github.com/inconshreveable/mousetrap v1.1.0 // indirect

pkg/common-controller/framework_test.go

@@ -271,6 +271,8 @@ func (r *snapshotReactor) React(action core.Action) (handled bool, ret runtime.O
 		// Check and bump object version
 		storedSnapshotContent, found := r.contents[action.GetName()]
 		if found {
+			// don't modify existing object
+			storedSnapshotContent = storedSnapshotContent.DeepCopy()
 			// Apply patch
 			storedSnapshotBytes, err := json.Marshal(storedSnapshotContent)
 			if err != nil {
@@ -335,6 +337,8 @@ func (r *snapshotReactor) React(action core.Action) (handled bool, ret runtime.O
 		// Check and bump object version
 		storedSnapshot, found := r.snapshots[action.GetName()]
 		if found {
+			// don't modify existing object
+			storedSnapshot = storedSnapshot.DeepCopy()
 			// Apply patch
 			storedSnapshotBytes, err := json.Marshal(storedSnapshot)
 			if err != nil {
@@ -349,7 +353,8 @@ func (r *snapshotReactor) React(action core.Action) (handled bool, ret runtime.O
 			if err != nil {
 				return true, nil, err
 			}
-
+			// following unmarshal removes the time millisecond precision which was present in the original object
+			// make sure time used in tests are in seconds precision
 			err = json.Unmarshal(modified, storedSnapshot)
 			if err != nil {
 				return true, nil, err
@@ -457,6 +462,44 @@ func (r *snapshotReactor) React(action core.Action) (handled bool, ret runtime.O
 		klog.V(4).Infof("saved updated claim %s", claim.Name)
 		return true, claim, nil
 
+	case action.Matches("patch", "persistentvolumeclaims"):
+		claim := &v1.PersistentVolumeClaim{}
+		action := action.(core.PatchAction)
+
+		// Check and bump object version
+		storedClaim, found := r.claims[action.GetName()]
+		if found {
+			// don't modify existing object
+			storedClaim = storedClaim.DeepCopy()
+			// Apply patch
+			storedClaimBytes, err := json.Marshal(storedClaim)
+			if err != nil {
+				return true, nil, err
+			}
+			claimPatch, err := jsonpatch.DecodePatch(action.GetPatch())
+			if err != nil {
+				return true, nil, err
+			}
+			modified, err := claimPatch.Apply(storedClaimBytes)
+			if err != nil {
+				return true, nil, err
+			}
+			err = json.Unmarshal(modified, claim)
+			if err != nil {
+				return true, nil, err
+			}
+			storedVer, _ := strconv.Atoi(claim.ResourceVersion)
+			claim.ResourceVersion = strconv.Itoa(storedVer + 1)
+		} else {
+			return true, nil, fmt.Errorf("cannot update claim %s: claim not found", action.GetName())
+		}
+		// Store the updated object to appropriate places.
+		r.claims[claim.Name] = claim
+		r.changedObjects = append(r.changedObjects, claim)
+		r.changedSinceLastSync++
+		klog.V(4).Infof("saved updated claim %s", claim.Name)
+		return
+
 	case action.Matches("get", "secrets"):
 		name := action.(core.GetAction).GetName()
 		secret, found := r.secrets[name]
@@ -811,6 +854,7 @@ func newSnapshotReactor(kubeClient *kubefake.Clientset, client *fake.Clientset,
 	client.AddReactor("delete", "volumesnapshotclasses", reactor.React)
 	kubeClient.AddReactor("get", "persistentvolumeclaims", reactor.React)
 	kubeClient.AddReactor("update", "persistentvolumeclaims", reactor.React)
+	kubeClient.AddReactor("patch", "persistentvolumeclaims", reactor.React)
 	kubeClient.AddReactor("get", "persistentvolumes", reactor.React)
 	kubeClient.AddReactor("get", "secrets", reactor.React)
 
@@ -1261,7 +1305,6 @@ func testAddPVCFinalizer(ctrl *csiSnapshotCommonController, reactor *snapshotRea
 func testRemovePVCFinalizer(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error {
 	return ctrl.checkandRemovePVCFinalizer(test.initialSnapshots[0], false)
 }
-
 func testAddSnapshotFinalizer(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error {
 	return ctrl.addSnapshotFinalizer(test.initialSnapshots[0], true, true)
 }
@@ -1274,6 +1317,10 @@ func testRemoveSnapshotFinalizer(ctrl *csiSnapshotCommonController, reactor *sna
 	return ctrl.removeSnapshotFinalizer(test.initialSnapshots[0], true, true, false)
 }
 
+func testRemoveSnapshotFinalizerAfterUpdateConflict(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error {
+	return ctrl.removeSnapshotFinalizer(test.initialSnapshots[0], true, true, false)
+}
+
 func testUpdateSnapshotClass(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error {
 	snap, err := ctrl.checkAndUpdateSnapshotClass(test.initialSnapshots[0])
 	// syncSnapshotByKey expects that checkAndUpdateSnapshotClass always returns a snapshot
@@ -1425,7 +1472,6 @@ func runFinalizerTests(t *testing.T, tests []controllerTest, snapshotClasses []*
 	snapshotscheme.AddToScheme(scheme.Scheme)
 	for _, test := range tests {
 		klog.V(4).Infof("starting test %q", test.name)
-
 		// Initialize the controller
 		kubeClient := &kubefake.Clientset{}
 		client := &fake.Clientset{}
@@ -1468,7 +1514,7 @@ func runFinalizerTests(t *testing.T, tests []controllerTest, snapshotClasses []*
 
 		// Run the tested functions
 		err = test.test(ctrl, reactor, test)
-		if err != nil {
+		if test.expectSuccess && err != nil {
 			t.Errorf("Test %q failed: %v", test.name, err)
 		}
 

pkg/common-controller/groupsnapshot_controller_helper_test.go

@@ -0,0 +1,90 @@
+/*
+Copyright 2023 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 common_controller
+
+import (
+	"context"
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	crdv1alpha1 "github.com/kubernetes-csi/external-snapshotter/client/v7/apis/volumegroupsnapshot/v1alpha1"
+	"github.com/kubernetes-csi/external-snapshotter/client/v7/clientset/versioned/fake"
+	"github.com/kubernetes-csi/external-snapshotter/v7/pkg/utils"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/tools/cache"
+)
+
+func Test_csiSnapshotCommonController_removeGroupSnapshotFinalizer(t *testing.T) {
+	type args struct {
+		groupSnapshot        *crdv1alpha1.VolumeGroupSnapshot
+		removeBoundFinalizer bool
+	}
+	tests := []struct {
+		name               string
+		args               args
+		wantErr            bool
+		expectedFinalizers []string
+	}{
+		{
+			name: "Test removeGroupSnapshotFinalizer",
+			args: args{
+				removeBoundFinalizer: true,
+				groupSnapshot: &crdv1alpha1.VolumeGroupSnapshot{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:       "test-group-snapshot",
+						Finalizers: []string{utils.VolumeGroupSnapshotBoundFinalizer},
+					},
+				},
+			},
+		},
+		{
+			name: "Test removeGroupSnapshotFinalizer and not something else",
+			args: args{
+				removeBoundFinalizer: true,
+				groupSnapshot: &crdv1alpha1.VolumeGroupSnapshot{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:       "test-group-snapshot",
+						Finalizers: []string{"somethingElse", utils.VolumeGroupSnapshotBoundFinalizer},
+					},
+				},
+			},
+			expectedFinalizers: []string{"somethingElse"},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ctrl := &csiSnapshotCommonController{
+				clientset:          fake.NewSimpleClientset(tt.args.groupSnapshot),
+				groupSnapshotStore: cache.NewStore(cache.DeletionHandlingMetaNamespaceKeyFunc),
+			}
+			if err := ctrl.removeGroupSnapshotFinalizer(tt.args.groupSnapshot, tt.args.removeBoundFinalizer); (err != nil) != tt.wantErr {
+				t.Errorf("csiSnapshotCommonController.removeGroupSnapshotFinalizer() error = %v, wantErr %v", err, tt.wantErr)
+			}
+			vgs, err := ctrl.clientset.GroupsnapshotV1alpha1().VolumeGroupSnapshots(tt.args.groupSnapshot.Namespace).Get(context.TODO(), tt.args.groupSnapshot.Name, metav1.GetOptions{})
+			if err != nil {
+				t.Errorf("Error getting volume group snapshot: %v", err)
+			}
+			if tt.expectedFinalizers == nil && vgs.Finalizers != nil {
+				tt.expectedFinalizers = []string{} // if expectedFinalizers is nil, then it should be an empty slice so that cmp.Diff does not panic
+			}
+			if vgs.Finalizers != nil && cmp.Diff(vgs.Finalizers, tt.expectedFinalizers) != "" {
+				t.Errorf("Finalizers not expected: %v", cmp.Diff(vgs.Finalizers, tt.expectedFinalizers))
+			}
+
+		})
+	}
+}

pkg/common-controller/snapshot_delete_test.go

@@ -19,6 +19,7 @@ package common_controller
 import (
 	"errors"
 	"testing"
+	"time"
 
 	crdv1 "github.com/kubernetes-csi/external-snapshotter/client/v7/apis/volumesnapshot/v1"
 	"github.com/kubernetes-csi/external-snapshotter/v7/pkg/utils"
@@ -49,7 +50,12 @@ var class5Parameters = map[string]string{
 	utils.PrefixedSnapshotterSecretNamespaceKey: "default",
 }
 
-var timeNowMetav1 = metav1.Now()
+var timeNowMetav1 = func() metav1.Time {
+	// json.unmarshal does not restore millisecond precision
+	// so we need to round the time to seconds
+	timeNow := timeNow.Round(time.Second)
+	return metav1.NewTime(timeNow)
+}()
 
 var (
 	content31 = "content3-1"

pkg/common-controller/snapshot_finalizer_test.go

@@ -17,9 +17,12 @@ limitations under the License.
 package common_controller
 
 import (
+	"testing"
+
+	crdv1 "github.com/kubernetes-csi/external-snapshotter/client/v7/apis/volumesnapshot/v1"
 	"github.com/kubernetes-csi/external-snapshotter/v7/pkg/utils"
 	v1 "k8s.io/api/core/v1"
-	"testing"
+	"k8s.io/apimachinery/pkg/api/errors"
 )
 
 // Test single call to ensurePVCFinalizer, checkandRemovePVCFinalizer, addSnapshotFinalizer, removeSnapshotFinalizer
@@ -82,6 +85,38 @@ func TestSnapshotFinalizer(t *testing.T) {
 			test:             testRemoveSnapshotFinalizer,
 			expectSuccess:    true,
 		},
+		// TODO: Handle conflict errors, currently failing
+		// {
+		// 	name:             "2-4 - successful remove Snapshot finalizer after update conflict",
+		// 	initialSnapshots: newSnapshotArray("snap2-4", "snapuid2-4", "claim2-4", "", classSilver, "", &False, nil, nil, nil, false, true, nil),
+		// 	initialClaims:    newClaimArray("claim2-4", "pvc-uid2-4", "1Gi", "volume2-4", v1.ClaimBound, &classEmpty),
+		// 	test:             testRemoveSnapshotFinalizerAfterUpdateConflict,
+		// 	expectSuccess:    true,
+		// 	errors: []reactorError{
+		// 		{"update", "volumesnapshots", errors.NewConflict(crdv1.Resource("volumesnapshots"), "snap2-4", nil)},
+		// 	},
+		// },
+		{
+			name:             "2-5 - unsuccessful remove Snapshot finalizer after update non-conflict error",
+			initialSnapshots: newSnapshotArray("snap2-5", "snapuid2-5", "claim2-5", "", classSilver, "", &False, nil, nil, nil, false, true, nil),
+			initialClaims:    newClaimArray("claim2-5", "pvc-uid2-5", "1Gi", "volume2-5", v1.ClaimBound, &classEmpty),
+			test:             testRemoveSnapshotFinalizerAfterUpdateConflict,
+			expectSuccess:    false,
+			errors: []reactorError{
+				{"update", "volumesnapshots", errors.NewForbidden(crdv1.Resource("volumesnapshots"), "snap2-5", nil)},
+			},
+		},
+		{
+			name:             "2-6 - unsuccessful remove Snapshot finalizer after update conflict and get fails",
+			initialSnapshots: newSnapshotArray("snap2-6", "snapuid2-6", "claim2-6", "", classSilver, "", &False, nil, nil, nil, false, true, nil),
+			initialClaims:    newClaimArray("claim2-6", "pvc-uid2-6", "1Gi", "volume2-6", v1.ClaimBound, &classEmpty),
+			test:             testRemoveSnapshotFinalizerAfterUpdateConflict,
+			expectSuccess:    false,
+			errors: []reactorError{
+				{"update", "volumesnapshots", errors.NewConflict(crdv1.Resource("volumesnapshots"), "snap2-6", nil)},
+				{"get", "volumesnapshots", errors.NewServerTimeout(crdv1.Resource("volumesnapshots"), "get", 10)},
+			},
+		},
 	}
 	runFinalizerTests(t, tests, snapshotClasses)
 }

pkg/common-controller/snapshot_update_test.go

@@ -117,9 +117,10 @@ func TestSync(t *testing.T) {
 			initialVolumes:    newVolumeArray("volume2-8", "pv-uid2-8", "pv-handle2-8", "1Gi", "pvc-uid2-8", "claim2-8", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
 			initialSecrets:    []*v1.Secret{secret()},
 			errors: []reactorError{
-				// Inject error to the first client.VolumesnapshotV1().VolumeSnapshots().Update call.
+				// Inject error to the first client.VolumesnapshotV1().VolumeSnapshots().Update and .Patch call.
 				// All other calls will succeed.
 				{"update", "volumesnapshots", errors.New("mock update error")},
+				{"patch", "volumesnapshots", errors.New("mock update error")},
 			},
 			test: testSyncSnapshotError,
 		},

pkg/utils/util.go

@@ -20,6 +20,7 @@ import (
 	"context"
 	"fmt"
 	"os"
+	"slices"
 	"strconv"
 	"strings"
 	"time"
@@ -173,28 +174,21 @@ func MapContainsKey(m map[string]string, s string) bool {
 
 // ContainsString checks if a given slice of strings contains the provided string.
 func ContainsString(slice []string, s string) bool {
-	for _, item := range slice {
-		if item == s {
-			return true
-		}
-	}
-	return false
+	return slices.Contains(slice, s)
 }
 
 // RemoveString returns a newly created []string that contains all items from slice that
 // are not equal to s.
 func RemoveString(slice []string, s string) []string {
-	newSlice := make([]string, 0)
-	for _, item := range slice {
-		if item == s {
-			continue
-		}
-		newSlice = append(newSlice, item)
-	}
+	return RemoveStrings(slice, s)
+}
+
+func RemoveStrings(slice []string, removes ...string) []string {
+	newSlice := slices.DeleteFunc(slice, func(remove string) bool {
+		return slices.Contains(removes, remove)
+	})
 	if len(newSlice) == 0 {
-		// Sanitize for unit tests so we don't need to distinguish empty array
-		// and nil.
-		newSlice = nil
+		return nil
 	}
 	return newSlice
 }