From 3b9f0e2c720f82a78bac93b8b24a45574d06e466 Mon Sep 17 00:00:00 2001 From: Grant Griffiths Date: Mon, 27 Sep 2021 15:15:38 -0700 Subject: [PATCH] Fix unit tests and don't patch with empty finalizers Signed-off-by: Grant Griffiths --- go.mod | 1 + pkg/common-controller/framework_test.go | 94 ++++++++++++++++++- pkg/common-controller/snapshot_controller.go | 34 +++---- pkg/common-controller/snapshot_update_test.go | 4 +- pkg/sidecar-controller/content_create_test.go | 12 +-- pkg/sidecar-controller/framework_test.go | 43 +++++++++ vendor/modules.txt | 1 + 7 files changed, 162 insertions(+), 27 deletions(-) diff --git a/go.mod b/go.mod index 467d61e4..44fb15cb 100644 --- a/go.mod +++ b/go.mod @@ -4,6 +4,7 @@ go 1.16 require ( github.com/container-storage-interface/spec v1.5.0 + github.com/evanphx/json-patch v4.11.0+incompatible github.com/fsnotify/fsnotify v1.4.9 github.com/golang/mock v1.4.4 github.com/golang/protobuf v1.5.2 diff --git a/pkg/common-controller/framework_test.go b/pkg/common-controller/framework_test.go index c0093f00..120ce9b5 100644 --- a/pkg/common-controller/framework_test.go +++ b/pkg/common-controller/framework_test.go @@ -17,6 +17,7 @@ limitations under the License. package common_controller import ( + "encoding/json" "errors" "fmt" "net/http" @@ -29,8 +30,7 @@ import ( "testing" "time" - "k8s.io/client-go/util/workqueue" - + jsonpatch "github.com/evanphx/json-patch" crdv1 "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" clientset "github.com/kubernetes-csi/external-snapshotter/client/v4/clientset/versioned" "github.com/kubernetes-csi/external-snapshotter/client/v4/clientset/versioned/fake" @@ -55,6 +55,7 @@ import ( core "k8s.io/client-go/testing" "k8s.io/client-go/tools/cache" "k8s.io/client-go/tools/record" + "k8s.io/client-go/util/workqueue" klog "k8s.io/klog/v2" ) @@ -258,6 +259,46 @@ func (r *snapshotReactor) React(action core.Action) (handled bool, ret runtime.O klog.V(4).Infof("saved updated content %s", content.Name) return true, content, nil + case action.Matches("patch", "volumesnapshotcontents"): + content := &crdv1.VolumeSnapshotContent{} + action := action.(core.PatchAction) + + // Check and bump object version + storedSnapshotContent, found := r.contents[action.GetName()] + if found { + // Apply patch + storedSnapshotBytes, err := json.Marshal(storedSnapshotContent) + if err != nil { + return true, nil, err + } + contentPatch, err := jsonpatch.DecodePatch(action.GetPatch()) + if err != nil { + return true, nil, err + } + + modified, err := contentPatch.Apply(storedSnapshotBytes) + if err != nil { + return true, nil, err + } + + err = json.Unmarshal(modified, content) + if err != nil { + return true, nil, err + } + + storedVer, _ := strconv.Atoi(content.ResourceVersion) + content.ResourceVersion = strconv.Itoa(storedVer + 1) + } else { + return true, nil, fmt.Errorf("cannot update snapshot content %s: snapshot content not found", action.GetName()) + } + + // Store the updated object to appropriate places. + r.contents[content.Name] = content + r.changedObjects = append(r.changedObjects, content) + r.changedSinceLastSync++ + klog.V(4).Infof("saved updated content %s", content.Name) + return true, content, nil + case action.Matches("update", "volumesnapshots"): obj := action.(core.UpdateAction).GetObject() snapshot := obj.(*crdv1.VolumeSnapshot) @@ -284,6 +325,52 @@ func (r *snapshotReactor) React(action core.Action) (handled bool, ret runtime.O klog.V(4).Infof("saved updated snapshot %s", snapshot.Name) 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 { + // Apply patch + storedSnapshotBytes, err := json.Marshal(storedSnapshot) + if err != nil { + return true, nil, err + } + snapPatch, err := jsonpatch.DecodePatch(action.GetPatch()) + if err != nil { + return true, nil, err + } + + modified, err := snapPatch.Apply(storedSnapshotBytes) + if err != nil { + return true, nil, err + } + + err = json.Unmarshal(modified, storedSnapshot) + if err != nil { + return true, nil, err + } + + storedVer, _ := strconv.Atoi(storedSnapshot.ResourceVersion) + storedSnapshot.ResourceVersion = strconv.Itoa(storedVer + 1) + + // // If we were updating annotations and the new annotations are nil, leave as empty. + // // This seems to be the behavior for merge-patching nil & empty annotations + // if !reflect.DeepEqual(storedSnapshotContent.Annotations, content.Annotations) && content.Annotations == nil { + // content.Annotations = make(map[string]string) + // } + } else { + return true, nil, fmt.Errorf("cannot update snapshot %s: snapshot not found", action.GetName()) + } + + // Store the updated object to appropriate places. + r.snapshots[snapshot.Name] = storedSnapshot + r.changedObjects = append(r.changedObjects, storedSnapshot) + r.changedSinceLastSync++ + klog.V(4).Infof("saved updated snapshot %s", storedSnapshot.Name) + return true, storedSnapshot, nil + case action.Matches("get", "volumesnapshotcontents"): name := action.(core.GetAction).GetName() content, found := r.contents[name] @@ -437,6 +524,7 @@ func (r *snapshotReactor) checkContents(expectedContents []*crdv1.VolumeSnapshot } gotMap[v.Name] = v } + if !reflect.DeepEqual(expectedMap, gotMap) { // Print ugly but useful diff of expected and received objects for // easier debugging. @@ -714,6 +802,8 @@ func newSnapshotReactor(kubeClient *kubefake.Clientset, client *fake.Clientset, client.AddReactor("create", "volumesnapshotcontents", reactor.React) client.AddReactor("update", "volumesnapshotcontents", reactor.React) client.AddReactor("update", "volumesnapshots", reactor.React) + client.AddReactor("patch", "volumesnapshotcontents", reactor.React) + client.AddReactor("patch", "volumesnapshots", reactor.React) client.AddReactor("update", "volumesnapshotclasses", reactor.React) client.AddReactor("get", "volumesnapshotcontents", reactor.React) client.AddReactor("get", "volumesnapshots", reactor.React) diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go index ee7d6a3d..9bd0ef8b 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/pkg/common-controller/snapshot_controller.go @@ -809,13 +809,24 @@ func (ctrl *csiSnapshotCommonController) updateSnapshotErrorStatusWithEvent(snap // addContentFinalizer adds a Finalizer for VolumeSnapshotContent. func (ctrl *csiSnapshotCommonController) addContentFinalizer(content *crdv1.VolumeSnapshotContent) error { - var patches []utils.PatchOp - patches = append(patches, utils.PatchOp{ - Op: "add", - Path: "/metadata/finalizers/-", - Value: utils.VolumeSnapshotContentFinalizer, - }) + var patches []utils.PatchOp + if len(content.Finalizers) > 0 { + // Add to the end of the finalizers if we have any other finalizers + patches = append(patches, utils.PatchOp{ + Op: "add", + Path: "/metadata/finalizers/-", + Value: utils.VolumeSnapshotContentFinalizer, + }) + + } else { + // 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}, + }) + } newContent, err := utils.PatchVolumeSnapshotContent(content, patches, ctrl.clientset) if err != nil { return newControllerUpdateError(content.Name, err.Error()) @@ -987,17 +998,6 @@ func (ctrl *csiSnapshotCommonController) checkandBindSnapshotContent(snapshot *c } else if content.Spec.VolumeSnapshotRef.UID != "" && content.Spec.VolumeSnapshotClassName != nil { return content, nil } - // contentClone := content.DeepCopy() - // contentClone.Spec.VolumeSnapshotRef.UID = snapshot.UID - // if snapshot.Spec.VolumeSnapshotClassName != nil { - // className := *(snapshot.Spec.VolumeSnapshotClassName) - // contentClone.Spec.VolumeSnapshotClassName = &className - // } - // newContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{}) - // if err != nil { - // klog.V(4).Infof("updating VolumeSnapshotContent[%s] error status failed %v", contentClone.Name, err) - // return content, err - // } patches := []utils.PatchOp{ { diff --git a/pkg/common-controller/snapshot_update_test.go b/pkg/common-controller/snapshot_update_test.go index 864fe81c..494082b8 100644 --- a/pkg/common-controller/snapshot_update_test.go +++ b/pkg/common-controller/snapshot_update_test.go @@ -162,7 +162,7 @@ func TestSync(t *testing.T) { expectedSnapshots: newSnapshotArray("snap2-12", "snapuid2-12", "", "content2-12", validSecretClass, "content2-12", &False, nil, nil, newVolumeError("Snapshot failed to bind VolumeSnapshotContent, mock update error"), false, true, nil), errors: []reactorError{ // Inject error to the forth client.VolumesnapshotV1().VolumeSnapshots().Update call. - {"update", "volumesnapshotcontents", errors.New("mock update error")}, + {"patch", "volumesnapshotcontents", errors.New("mock update error")}, }, test: testSyncSnapshot, }, @@ -312,7 +312,7 @@ func TestSync(t *testing.T) { initialSecrets: []*v1.Secret{secret()}, errors: []reactorError{ // Inject error to the forth client.VolumesnapshotV1().VolumeSnapshots().Update call. - {"update", "volumesnapshotcontents", errors.New("mock update error")}, + {"patch", "volumesnapshotcontents", errors.New("mock update error")}, }, expectSuccess: false, test: testSyncContentError, diff --git a/pkg/sidecar-controller/content_create_test.go b/pkg/sidecar-controller/content_create_test.go index a980e7b6..af6ea04d 100644 --- a/pkg/sidecar-controller/content_create_test.go +++ b/pkg/sidecar-controller/content_create_test.go @@ -85,7 +85,7 @@ func TestSyncContent(t *testing.T) { { name: "1-3: Basic sync content create snapshot with non-existent secret", initialContents: withContentAnnotations(withContentStatus(newContentArray("content1-3", "snapuid1-3", "snap1-3", "sid1-3", invalidSecretClass, "", "volume-handle-1-3", retainPolicy, nil, &defaultSize, true), - nil), map[string]string{ + &crdv1.VolumeSnapshotContentStatus{}), map[string]string{ utils.AnnDeletionSecretRefName: "", utils.AnnDeletionSecretRefNamespace: "", }), @@ -94,12 +94,12 @@ func TestSyncContent(t *testing.T) { SnapshotHandle: nil, RestoreSize: nil, ReadyToUse: &False, - Error: newSnapshotError("Failed to create snapshot: failed to get input parameters to create snapshot for content content1-3: \"cannot retrieve secrets for snapshot content \\\"content1-3\\\", err: secret name or namespace not specified\""), + Error: newSnapshotError("Failed to check and update snapshot content: failed to get input parameters to create snapshot for content content1-3: \"cannot retrieve secrets for snapshot content \\\"content1-3\\\", err: secret name or namespace not specified\""), }), map[string]string{ utils.AnnDeletionSecretRefName: "", utils.AnnDeletionSecretRefNamespace: "", }), initialSecrets: []*v1.Secret{}, // no initial secret created - expectedEvents: []string{"Warning SnapshotCreationFailed"}, + expectedEvents: []string{"Warning SnapshotContentCheckandUpdateFailed"}, errors: noerrors, test: testSyncContent, }, @@ -149,7 +149,7 @@ func TestSyncContent(t *testing.T) { { name: "1-5: Basic sync content create snapshot with failed secret call", initialContents: withContentAnnotations(withContentStatus(newContentArray("content1-5", "snapuid1-5", "snap1-5", "sid1-5", invalidSecretClass, "", "volume-handle-1-5", retainPolicy, nil, &defaultSize, true), - nil), map[string]string{ + &crdv1.VolumeSnapshotContentStatus{}), map[string]string{ utils.AnnDeletionSecretRefName: "secret", utils.AnnDeletionSecretRefNamespace: "default", }), @@ -158,12 +158,12 @@ func TestSyncContent(t *testing.T) { SnapshotHandle: nil, RestoreSize: nil, ReadyToUse: &False, - Error: newSnapshotError("Failed to create snapshot: failed to get input parameters to create snapshot for content content1-5: \"cannot get credentials for snapshot content \\\"content1-5\\\"\""), + Error: newSnapshotError(`Failed to check and update snapshot content: failed to get input parameters to create snapshot for content content1-5: "cannot get credentials for snapshot content \"content1-5\""`), }), map[string]string{ utils.AnnDeletionSecretRefName: "secret", utils.AnnDeletionSecretRefNamespace: "default", }), initialSecrets: []*v1.Secret{}, // no initial secret created - expectedEvents: []string{"Warning SnapshotCreationFailed"}, + expectedEvents: []string{"Warning SnapshotContentCheckandUpdateFailed"}, errors: []reactorError{ // Inject error to the first client.VolumesnapshotV1().VolumeSnapshots().Update call. // All other calls will succeed. diff --git a/pkg/sidecar-controller/framework_test.go b/pkg/sidecar-controller/framework_test.go index 078cfd0a..6a15b21a 100644 --- a/pkg/sidecar-controller/framework_test.go +++ b/pkg/sidecar-controller/framework_test.go @@ -15,6 +15,7 @@ package sidecar_controller import ( "context" + "encoding/json" "errors" "fmt" "reflect" @@ -25,6 +26,7 @@ import ( "testing" "time" + jsonpatch "github.com/evanphx/json-patch" crdv1 "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" clientset "github.com/kubernetes-csi/external-snapshotter/client/v4/clientset/versioned" "github.com/kubernetes-csi/external-snapshotter/client/v4/clientset/versioned/fake" @@ -215,6 +217,46 @@ func (r *snapshotReactor) React(action core.Action) (handled bool, ret runtime.O klog.V(4).Infof("saved updated content %s", content.Name) return true, content, nil + case action.Matches("patch", "volumesnapshotcontents"): + content := &crdv1.VolumeSnapshotContent{} + action := action.(core.PatchAction) + + // Check and bump object version + storedSnapshotContent, found := r.contents[action.GetName()] + if found { + // Apply patch + storedSnapshotBytes, err := json.Marshal(storedSnapshotContent) + if err != nil { + return true, nil, err + } + contentPatch, err := jsonpatch.DecodePatch(action.GetPatch()) + if err != nil { + return true, nil, err + } + + modified, err := contentPatch.Apply(storedSnapshotBytes) + if err != nil { + return true, nil, err + } + + err = json.Unmarshal(modified, content) + if err != nil { + return true, nil, err + } + + storedVer, _ := strconv.Atoi(content.ResourceVersion) + content.ResourceVersion = strconv.Itoa(storedVer + 1) + } else { + return true, nil, fmt.Errorf("cannot update snapshot content %s: snapshot content not found", action.GetName()) + } + + // Store the updated object to appropriate places. + r.contents[content.Name] = content + r.changedObjects = append(r.changedObjects, content) + r.changedSinceLastSync++ + klog.V(4).Infof("saved updated content %s", content.Name) + return true, content, nil + case action.Matches("get", "volumesnapshotcontents"): name := action.(core.GetAction).GetName() content, found := r.contents[name] @@ -488,6 +530,7 @@ func newSnapshotReactor(kubeClient *kubefake.Clientset, client *fake.Clientset, client.AddReactor("create", "volumesnapshotcontents", reactor.React) client.AddReactor("update", "volumesnapshotcontents", reactor.React) + client.AddReactor("patch", "volumesnapshotcontents", reactor.React) client.AddReactor("get", "volumesnapshotcontents", reactor.React) client.AddReactor("delete", "volumesnapshotcontents", reactor.React) kubeClient.AddReactor("get", "secrets", reactor.React) diff --git a/vendor/modules.txt b/vendor/modules.txt index 541ae632..c4f646d9 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -10,6 +10,7 @@ github.com/container-storage-interface/spec/lib/go/csi # github.com/davecgh/go-spew v1.1.1 github.com/davecgh/go-spew/spew # github.com/evanphx/json-patch v4.11.0+incompatible +## explicit github.com/evanphx/json-patch # github.com/fsnotify/fsnotify v1.4.9 ## explicit