From 04848c755aaa121f696f7fa553bfcfe5864914bc Mon Sep 17 00:00:00 2001 From: Max Jonas Werner Date: Wed, 22 Jul 2020 09:30:23 +0200 Subject: [PATCH] chore: remove unused parameter from util funcs; add unit test Added the unit tests from https://github.com/kubernetes/kubernetes/blob/master/pkg/util/slice/slice_test.go (minus modifier tests) so we can be sure to not change behaviour. --- pkg/common-controller/framework_test.go | 12 ++--- pkg/common-controller/snapshot_controller.go | 12 ++--- pkg/sidecar-controller/snapshot_controller.go | 4 +- pkg/utils/util.go | 21 +++----- pkg/utils/util_test.go | 52 +++++++++++++++++++ 5 files changed, 73 insertions(+), 28 deletions(-) diff --git a/pkg/common-controller/framework_test.go b/pkg/common-controller/framework_test.go index 77629dad..f0004964 100644 --- a/pkg/common-controller/framework_test.go +++ b/pkg/common-controller/framework_test.go @@ -1320,7 +1320,7 @@ func evaluateFinalizerTests(ctrl *csiSnapshotCommonController, reactor *snapshot if funcName == "testAddPVCFinalizer" { for _, pvc := range reactor.claims { if test.initialClaims[0].Name == pvc.Name { - if !utils.ContainsString(test.initialClaims[0].ObjectMeta.Finalizers, utils.PVCFinalizer, nil) && utils.ContainsString(pvc.ObjectMeta.Finalizers, utils.PVCFinalizer, nil) { + if !utils.ContainsString(test.initialClaims[0].ObjectMeta.Finalizers, utils.PVCFinalizer) && utils.ContainsString(pvc.ObjectMeta.Finalizers, utils.PVCFinalizer) { klog.V(4).Infof("test %q succeeded. PVCFinalizer is added to PVC %s", test.name, pvc.Name) bHasPVCFinalizer = true } @@ -1335,7 +1335,7 @@ func evaluateFinalizerTests(ctrl *csiSnapshotCommonController, reactor *snapshot if funcName == "testRemovePVCFinalizer" { for _, pvc := range reactor.claims { if test.initialClaims[0].Name == pvc.Name { - if utils.ContainsString(test.initialClaims[0].ObjectMeta.Finalizers, utils.PVCFinalizer, nil) && !utils.ContainsString(pvc.ObjectMeta.Finalizers, utils.PVCFinalizer, nil) { + if utils.ContainsString(test.initialClaims[0].ObjectMeta.Finalizers, utils.PVCFinalizer) && !utils.ContainsString(pvc.ObjectMeta.Finalizers, utils.PVCFinalizer) { klog.V(4).Infof("test %q succeeded. PVCFinalizer is removed from PVC %s", test.name, pvc.Name) bHasPVCFinalizer = false } @@ -1350,8 +1350,8 @@ 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, nil) && utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer, nil) && - !utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer, nil) && utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer, nil) { + 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 } @@ -1366,8 +1366,8 @@ func evaluateFinalizerTests(ctrl *csiSnapshotCommonController, reactor *snapshot 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, nil) && !utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer, nil) && - utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer, nil) && !utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer, nil) { + 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 index d5945a2e..e5fab006 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/pkg/common-controller/snapshot_controller.go @@ -222,7 +222,7 @@ func (ctrl *csiSnapshotCommonController) isPVCwithFinalizerInUseByCurrentSnapsho } // Check if there is a Finalizer on PVC. If not, return false - if !utils.ContainsString(pvc.ObjectMeta.Finalizers, utils.PVCFinalizer, nil) { + if !utils.ContainsString(pvc.ObjectMeta.Finalizers, utils.PVCFinalizer) { return false } @@ -820,7 +820,7 @@ func (ctrl *csiSnapshotCommonController) ensurePVCFinalizer(snapshot *crdv1.Volu } // If PVC is not being deleted and PVCFinalizer is not added yet, the PVCFinalizer should be added. - if pvc.ObjectMeta.DeletionTimestamp == nil && !utils.ContainsString(pvc.ObjectMeta.Finalizers, utils.PVCFinalizer, nil) { + if pvc.ObjectMeta.DeletionTimestamp == nil && !utils.ContainsString(pvc.ObjectMeta.Finalizers, utils.PVCFinalizer) { // Add the finalizer pvcClone := pvc.DeepCopy() pvcClone.ObjectMeta.Finalizers = append(pvcClone.ObjectMeta.Finalizers, utils.PVCFinalizer) @@ -841,7 +841,7 @@ func (ctrl *csiSnapshotCommonController) removePVCFinalizer(pvc *v1.PersistentVo // TODO(xyang): We get PVC from informer but it may be outdated // Should get it from API server directly before removing finalizer pvcClone := pvc.DeepCopy() - pvcClone.ObjectMeta.Finalizers = utils.RemoveString(pvcClone.ObjectMeta.Finalizers, utils.PVCFinalizer, nil) + pvcClone.ObjectMeta.Finalizers = utils.RemoveString(pvcClone.ObjectMeta.Finalizers, utils.PVCFinalizer) _, err := ctrl.client.CoreV1().PersistentVolumeClaims(pvcClone.Namespace).Update(context.TODO(), pvcClone, metav1.UpdateOptions{}) if err != nil { @@ -897,7 +897,7 @@ func (ctrl *csiSnapshotCommonController) checkandRemovePVCFinalizer(snapshot *cr klog.V(5).Infof("checkandRemovePVCFinalizer for snapshot [%s]: snapshot status [%#v]", snapshot.Name, snapshot.Status) // Check if there is a Finalizer on PVC to be removed - if utils.ContainsString(pvc.ObjectMeta.Finalizers, utils.PVCFinalizer, nil) { + if utils.ContainsString(pvc.ObjectMeta.Finalizers, utils.PVCFinalizer) { // There is a Finalizer on PVC. Check if PVC is used // and remove finalizer if it's not used. inUse := ctrl.isPVCBeingUsed(pvc, snapshot) @@ -1294,10 +1294,10 @@ func (ctrl *csiSnapshotCommonController) removeSnapshotFinalizer(snapshot *crdv1 snapshotClone := snapshot.DeepCopy() if removeSourceFinalizer { - snapshotClone.ObjectMeta.Finalizers = utils.RemoveString(snapshotClone.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer, nil) + snapshotClone.ObjectMeta.Finalizers = utils.RemoveString(snapshotClone.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) } if removeBoundFinalizer { - snapshotClone.ObjectMeta.Finalizers = utils.RemoveString(snapshotClone.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer, nil) + snapshotClone.ObjectMeta.Finalizers = utils.RemoveString(snapshotClone.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) } _, err := ctrl.clientset.SnapshotV1beta1().VolumeSnapshots(snapshotClone.Namespace).Update(context.TODO(), snapshotClone, metav1.UpdateOptions{}) if err != nil { diff --git a/pkg/sidecar-controller/snapshot_controller.go b/pkg/sidecar-controller/snapshot_controller.go index 7f2bbcf9..36af723e 100644 --- a/pkg/sidecar-controller/snapshot_controller.go +++ b/pkg/sidecar-controller/snapshot_controller.go @@ -505,12 +505,12 @@ func (ctrl *csiSnapshotSideCarController) GetCredentialsFromAnnotation(content * // removeContentFinalizer removes the VolumeSnapshotContentFinalizer from a // content if there exists one. func (ctrl csiSnapshotSideCarController) removeContentFinalizer(content *crdv1.VolumeSnapshotContent) error { - if !utils.ContainsString(content.ObjectMeta.Finalizers, utils.VolumeSnapshotContentFinalizer, nil) { + if !utils.ContainsString(content.ObjectMeta.Finalizers, utils.VolumeSnapshotContentFinalizer) { // the finalizer does not exit, return directly return nil } contentClone := content.DeepCopy() - contentClone.ObjectMeta.Finalizers = utils.RemoveString(contentClone.ObjectMeta.Finalizers, utils.VolumeSnapshotContentFinalizer, nil) + contentClone.ObjectMeta.Finalizers = utils.RemoveString(contentClone.ObjectMeta.Finalizers, utils.VolumeSnapshotContentFinalizer) _, err := ctrl.clientset.SnapshotV1beta1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{}) if err != nil { diff --git a/pkg/utils/util.go b/pkg/utils/util.go index dccbc1ff..df1cdcf0 100644 --- a/pkg/utils/util.go +++ b/pkg/utils/util.go @@ -109,30 +109,23 @@ var SnapshotterListSecretParams = secretParamsMap{ } // ContainsString checks if a given slice of strings contains the provided string. -// If a modifier func is provided, it is called with the slice item before the comparation. -func ContainsString(slice []string, s string, modifier func(s string) string) bool { +func ContainsString(slice []string, s string) bool { for _, item := range slice { if item == s { return true } - if modifier != nil && modifier(item) == s { - return true - } } return false } // RemoveString returns a newly created []string that contains all items from slice that -// are not equal to s and modifier(s) in case modifier func is provided. -func RemoveString(slice []string, s string, modifier func(s string) string) []string { +// are not equal to s. +func RemoveString(slice []string, s string) []string { newSlice := make([]string, 0) for _, item := range slice { if item == s { continue } - if modifier != nil && modifier(item) == s { - continue - } newSlice = append(newSlice, item) } if len(newSlice) == 0 { @@ -371,23 +364,23 @@ func NoResyncPeriodFunc() time.Duration { // NeedToAddContentFinalizer checks if a Finalizer needs to be added for the volume snapshot content. func NeedToAddContentFinalizer(content *crdv1.VolumeSnapshotContent) bool { - return content.ObjectMeta.DeletionTimestamp == nil && !ContainsString(content.ObjectMeta.Finalizers, VolumeSnapshotContentFinalizer, nil) + return content.ObjectMeta.DeletionTimestamp == nil && !ContainsString(content.ObjectMeta.Finalizers, VolumeSnapshotContentFinalizer) } // IsSnapshotDeletionCandidate checks if a volume snapshot deletionTimestamp // is set and any finalizer is on the snapshot. func IsSnapshotDeletionCandidate(snapshot *crdv1.VolumeSnapshot) bool { - return snapshot.ObjectMeta.DeletionTimestamp != nil && (ContainsString(snapshot.ObjectMeta.Finalizers, VolumeSnapshotAsSourceFinalizer, nil) || ContainsString(snapshot.ObjectMeta.Finalizers, VolumeSnapshotBoundFinalizer, nil)) + return snapshot.ObjectMeta.DeletionTimestamp != nil && (ContainsString(snapshot.ObjectMeta.Finalizers, VolumeSnapshotAsSourceFinalizer) || ContainsString(snapshot.ObjectMeta.Finalizers, VolumeSnapshotBoundFinalizer)) } // NeedToAddSnapshotAsSourceFinalizer checks if a Finalizer needs to be added for the volume snapshot as a source for PVC. func NeedToAddSnapshotAsSourceFinalizer(snapshot *crdv1.VolumeSnapshot) bool { - return snapshot.ObjectMeta.DeletionTimestamp == nil && !ContainsString(snapshot.ObjectMeta.Finalizers, VolumeSnapshotAsSourceFinalizer, nil) + return snapshot.ObjectMeta.DeletionTimestamp == nil && !ContainsString(snapshot.ObjectMeta.Finalizers, VolumeSnapshotAsSourceFinalizer) } // NeedToAddSnapshotBoundFinalizer checks if a Finalizer needs to be added for the bound volume snapshot. func NeedToAddSnapshotBoundFinalizer(snapshot *crdv1.VolumeSnapshot) bool { - return snapshot.ObjectMeta.DeletionTimestamp == nil && !ContainsString(snapshot.ObjectMeta.Finalizers, VolumeSnapshotBoundFinalizer, nil) && IsBoundVolumeSnapshotContentNameSet(snapshot) + return snapshot.ObjectMeta.DeletionTimestamp == nil && !ContainsString(snapshot.ObjectMeta.Finalizers, VolumeSnapshotBoundFinalizer) && IsBoundVolumeSnapshotContentNameSet(snapshot) } func deprecationWarning(deprecatedParam, newParam, removalVersion string) string { diff --git a/pkg/utils/util_test.go b/pkg/utils/util_test.go index d3a37191..84ba5f05 100644 --- a/pkg/utils/util_test.go +++ b/pkg/utils/util_test.go @@ -25,6 +25,58 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) +func TestContainsString(t *testing.T) { + src := []string{"aa", "bb", "cc"} + if !ContainsString(src, "bb") { + t.Errorf("ContainsString didn't find the string as expected") + } +} + +func TestRemoveString(t *testing.T) { + tests := []struct { + testName string + input []string + remove string + want []string + }{ + { + testName: "Nil input slice", + input: nil, + remove: "", + want: nil, + }, + { + testName: "Slice doesn't contain the string", + input: []string{"a", "ab", "cdef"}, + remove: "NotPresentInSlice", + want: []string{"a", "ab", "cdef"}, + }, + { + testName: "All strings removed, result is nil", + input: []string{"a"}, + remove: "a", + want: nil, + }, + { + testName: "One string removed", + input: []string{"a", "ab", "cdef"}, + remove: "ab", + want: []string{"a", "cdef"}, + }, + { + testName: "All(three) strings removed", + input: []string{"ab", "a", "ab", "cdef", "ab"}, + remove: "ab", + want: []string{"a", "cdef"}, + }, + } + for _, tt := range tests { + if got := RemoveString(tt.input, tt.remove); !reflect.DeepEqual(got, tt.want) { + t.Errorf("%v: RemoveString(%v, %q) = %v WANT %v", tt.testName, tt.input, tt.remove, got, tt.want) + } + } +} + func TestGetSecretReference(t *testing.T) { testcases := map[string]struct { secretParams secretParamsMap