From afff04176dac53214b45b152ed1c5b5cba7fe573 Mon Sep 17 00:00:00 2001 From: Grant Griffiths Date: Wed, 26 May 2021 11:56:28 -0700 Subject: [PATCH 01/10] WIP Signed-off-by: Grant Griffiths --- .../rbac-snapshot-controller.yaml | 9 ++- pkg/common-controller/snapshot_controller.go | 68 +++++++++++++++---- pkg/sidecar-controller/snapshot_controller.go | 31 +++++---- pkg/utils/patch.go | 58 ++++++++++++++++ 4 files changed, 139 insertions(+), 27 deletions(-) create mode 100644 pkg/utils/patch.go diff --git a/deploy/kubernetes/snapshot-controller/rbac-snapshot-controller.yaml b/deploy/kubernetes/snapshot-controller/rbac-snapshot-controller.yaml index a6b8c7a9..9fb7e9f3 100644 --- a/deploy/kubernetes/snapshot-controller/rbac-snapshot-controller.yaml +++ b/deploy/kubernetes/snapshot-controller/rbac-snapshot-controller.yaml @@ -34,13 +34,16 @@ rules: verbs: ["get", "list", "watch"] - apiGroups: ["snapshot.storage.k8s.io"] resources: ["volumesnapshotcontents"] - verbs: ["create", "get", "list", "watch", "update", "delete"] + verbs: ["create", "get", "list", "watch", "update", "delete", "patch"] + - apiGroups: ["snapshot.storage.k8s.io"] + resources: ["volumesnapshotcontents/status"] + verbs: ["patch"] - apiGroups: ["snapshot.storage.k8s.io"] resources: ["volumesnapshots"] - verbs: ["get", "list", "watch", "update"] + verbs: ["get", "list", "watch", "update", "patch"] - apiGroups: ["snapshot.storage.k8s.io"] resources: ["volumesnapshots/status"] - verbs: ["update"] + verbs: ["update", "patch"] --- kind: ClusterRoleBinding diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go index 48786f84..4a18a57d 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/pkg/common-controller/snapshot_controller.go @@ -809,6 +809,19 @@ func (ctrl *csiSnapshotCommonController) updateSnapshotErrorStatusWithEvent(snap // addContentFinalizer adds a Finalizer for VolumeSnapshotContent. func (ctrl *csiSnapshotCommonController) addContentFinalizer(content *crdv1.VolumeSnapshotContent) error { + // patches := []utils.PatchOp{ + // { + // Op: "add", + // Path: "/metadata/finalizers/-", + // Value: utils.VolumeSnapshotContentFinalizer, + // }, + // } + + // newContent, err := utils.PatchVolumeSnapshotContent(content, patches, ctrl.clientset) + // if err != nil { + // return newControllerUpdateError(content.Name, err.Error()) + // } + contentClone := content.DeepCopy() contentClone.ObjectMeta.Finalizers = append(contentClone.ObjectMeta.Finalizers, utils.VolumeSnapshotContentFinalizer) @@ -1392,24 +1405,55 @@ func isControllerUpdateFailError(err *crdv1.VolumeSnapshotError) bool { // addSnapshotFinalizer adds a Finalizer for VolumeSnapshot. func (ctrl *csiSnapshotCommonController) addSnapshotFinalizer(snapshot *crdv1.VolumeSnapshot, addSourceFinalizer bool, addBoundFinalizer bool) error { - 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) - } - newSnapshot, err := ctrl.clientset.SnapshotV1().VolumeSnapshots(snapshotClone.Namespace).Update(context.TODO(), snapshotClone, metav1.UpdateOptions{}) - if err != nil { - return newControllerUpdateError(utils.SnapshotKey(snapshot), err.Error()) + var updatedSnapshot *crdv1.VolumeSnapshot + var err error + + // Must perform an update if no finalizers exist + if len(snapshot.ObjectMeta.Finalizers) == 0 { + 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 addSourceFinalizer { + patches = append(patches, utils.PatchOp{ + Op: "add", + Path: "/metadata/finalizers/-", + Value: utils.VolumeSnapshotAsSourceFinalizer, + }) + } + if addBoundFinalizer { + patches = append(patches, utils.PatchOp{ + Op: "add", + Path: "/metadata/finalizers/-", + Value: utils.VolumeSnapshotBoundFinalizer, + }) + } + + klog.Infof("GGCSI - ADD SNAPSHOT FINALIZER - snapshot: %v", snapshot) + klog.Infof("GGCSI - ADD SNAPSHOT FINALIZER - patches: %v", patches) + updatedSnapshot, err = utils.PatchVolumeSnapshot(snapshot, patches, ctrl.clientset) + if err != nil { + return newControllerUpdateError(utils.SnapshotKey(snapshot), err.Error()) + } } - _, err = ctrl.storeSnapshotUpdate(newSnapshot) + _, err = ctrl.storeSnapshotUpdate(updatedSnapshot) if err != nil { klog.Errorf("failed to update snapshot store %v", err) } - klog.V(5).Infof("Added protection finalizer to volume snapshot %s", utils.SnapshotKey(newSnapshot)) + klog.V(5).Infof("Added protection finalizer to volume snapshot %s", utils.SnapshotKey(updatedSnapshot)) return nil } diff --git a/pkg/sidecar-controller/snapshot_controller.go b/pkg/sidecar-controller/snapshot_controller.go index ec0c6deb..1f930472 100644 --- a/pkg/sidecar-controller/snapshot_controller.go +++ b/pkg/sidecar-controller/snapshot_controller.go @@ -146,19 +146,26 @@ func (ctrl *csiSnapshotSideCarController) updateContentErrorStatusWithEvent(cont klog.V(4).Infof("updateContentStatusWithEvent[%s]: the same error %v is already set", content.Name, content.Status.Error) return nil } - contentClone := content.DeepCopy() - if contentClone.Status == nil { - contentClone.Status = &crdv1.VolumeSnapshotContentStatus{} - } - contentClone.Status.Error = &crdv1.VolumeSnapshotError{ - Time: &metav1.Time{ - Time: time.Now(), - }, - Message: &message, - } + ready := false - contentClone.Status.ReadyToUse = &ready - newContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().UpdateStatus(context.TODO(), contentClone, metav1.UpdateOptions{}) + patch := []utils.PatchOp{ + { + Op: "replace", + Path: "/status/error", + Value: &crdv1.VolumeSnapshotError{ + Time: &metav1.Time{ + Time: time.Now(), + }, + Message: &message, + }, + }, + { + Op: "replace", + Path: "/status/readyToUse", + Value: &ready, + }, + } + newContent, err := utils.PatchVolumeSnapshotContent(content, patch, ctrl.clientset, "status") // Emit the event even if the status update fails so that user can see the error ctrl.eventRecorder.Event(newContent, eventtype, reason, message) diff --git a/pkg/utils/patch.go b/pkg/utils/patch.go new file mode 100644 index 00000000..abd77a3d --- /dev/null +++ b/pkg/utils/patch.go @@ -0,0 +1,58 @@ +package utils + +import ( + "context" + "encoding/json" + + crdv1 "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" + clientset "github.com/kubernetes-csi/external-snapshotter/client/v4/clientset/versioned" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" +) + +// PatchOp represents a json patch operation +type PatchOp struct { + Op string `json:"op"` + Path string `json:"path"` + Value interface{} `json:"value,omitempty"` +} + +// PatchVolumeSnapshotContent patches a volume snapshot content object +func PatchVolumeSnapshotContent( + existingSnapshotContent *crdv1.VolumeSnapshotContent, + patch []PatchOp, + client clientset.Interface, + subresources ...string, +) (*crdv1.VolumeSnapshotContent, error) { + data, err := json.Marshal(patch) + if nil != err { + return existingSnapshotContent, err + } + + newSnapshotContent, err := client.SnapshotV1().VolumeSnapshotContents().Patch(context.TODO(), existingSnapshotContent.Name, types.JSONPatchType, data, metav1.PatchOptions{}, subresources...) + if err != nil { + return existingSnapshotContent, err + } + + return newSnapshotContent, nil +} + +// PatchVolumeSnapshot patches a volume snapshot object +func PatchVolumeSnapshot( + existingSnapshot *crdv1.VolumeSnapshot, + patch []PatchOp, + client clientset.Interface, + subresources ...string, +) (*crdv1.VolumeSnapshot, error) { + data, err := json.Marshal(patch) + if nil != err { + return existingSnapshot, err + } + + newSnapshot, err := client.SnapshotV1().VolumeSnapshots(existingSnapshot.Namespace).Patch(context.TODO(), existingSnapshot.Name, types.JSONPatchType, data, metav1.PatchOptions{}, subresources...) + if err != nil { + return existingSnapshot, err + } + + return newSnapshot, nil +} From 36b6b879ba35662c868ebe4a6e1d425791b0deae Mon Sep 17 00:00:00 2001 From: Grant Griffiths Date: Wed, 4 Aug 2021 07:12:33 -0700 Subject: [PATCH 02/10] add content finalizer patch Signed-off-by: Grant Griffiths --- pkg/common-controller/snapshot_controller.go | 25 ++++++-------------- 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go index 4a18a57d..de25512b 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/pkg/common-controller/snapshot_controller.go @@ -809,23 +809,14 @@ func (ctrl *csiSnapshotCommonController) updateSnapshotErrorStatusWithEvent(snap // addContentFinalizer adds a Finalizer for VolumeSnapshotContent. func (ctrl *csiSnapshotCommonController) addContentFinalizer(content *crdv1.VolumeSnapshotContent) error { - // patches := []utils.PatchOp{ - // { - // Op: "add", - // Path: "/metadata/finalizers/-", - // Value: utils.VolumeSnapshotContentFinalizer, - // }, - // } + var patches []utils.PatchOp + patches = append(patches, utils.PatchOp{ + Op: "add", + Path: "/metadata/finalizers/-", + Value: utils.VolumeSnapshotContentFinalizer, + }) - // newContent, err := utils.PatchVolumeSnapshotContent(content, patches, ctrl.clientset) - // if err != nil { - // return newControllerUpdateError(content.Name, err.Error()) - // } - - contentClone := content.DeepCopy() - contentClone.ObjectMeta.Finalizers = append(contentClone.ObjectMeta.Finalizers, utils.VolumeSnapshotContentFinalizer) - - newContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{}) + newContent, err := utils.PatchVolumeSnapshotContent(content, patches, ctrl.clientset) if err != nil { return newControllerUpdateError(content.Name, err.Error()) } @@ -1440,8 +1431,6 @@ func (ctrl *csiSnapshotCommonController) addSnapshotFinalizer(snapshot *crdv1.Vo }) } - klog.Infof("GGCSI - ADD SNAPSHOT FINALIZER - snapshot: %v", snapshot) - klog.Infof("GGCSI - ADD SNAPSHOT FINALIZER - patches: %v", patches) updatedSnapshot, err = utils.PatchVolumeSnapshot(snapshot, patches, ctrl.clientset) if err != nil { return newControllerUpdateError(utils.SnapshotKey(snapshot), err.Error()) From 2039ad91014a69257bbf978c370cea6d20b99f19 Mon Sep 17 00:00:00 2001 From: Grant Griffiths Date: Fri, 24 Sep 2021 11:11:15 -0700 Subject: [PATCH 03/10] checkandBindSnapshotContent with patch tested Signed-off-by: Grant Griffiths --- pkg/common-controller/snapshot_controller.go | 32 +++++++++++++++++--- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go index de25512b..ee7d6a3d 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/pkg/common-controller/snapshot_controller.go @@ -987,15 +987,37 @@ 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 + // 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{ + { + Op: "replace", + Path: "/spec/volumeSnapshotRef/uid", + Value: string(snapshot.UID), + }, + } if snapshot.Spec.VolumeSnapshotClassName != nil { className := *(snapshot.Spec.VolumeSnapshotClassName) - contentClone.Spec.VolumeSnapshotClassName = &className + patches = append(patches, utils.PatchOp{ + Op: "replace", + Path: "/spec/volumeSnapshotClassName", + Value: className, + }) } - newContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{}) + + newContent, err := utils.PatchVolumeSnapshotContent(content, patches, ctrl.clientset) if err != nil { - klog.V(4).Infof("updating VolumeSnapshotContent[%s] error status failed %v", contentClone.Name, err) + klog.V(4).Infof("updating VolumeSnapshotContent[%s] error status failed %v", content.Name, err) return content, err } From 3b9f0e2c720f82a78bac93b8b24a45574d06e466 Mon Sep 17 00:00:00 2001 From: Grant Griffiths Date: Mon, 27 Sep 2021 15:15:38 -0700 Subject: [PATCH 04/10] 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 From e1f82e4f1acbc5054499b11bd8f5bf9fe186c982 Mon Sep 17 00:00:00 2001 From: Grant Griffiths Date: Tue, 28 Sep 2021 15:54:15 -0700 Subject: [PATCH 05/10] Add patch for delete annotation Signed-off-by: Grant Griffiths --- pkg/common-controller/snapshot_controller.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go index 9bd0ef8b..e2f56e91 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/pkg/common-controller/snapshot_controller.go @@ -1544,14 +1544,21 @@ func (ctrl *csiSnapshotCommonController) setAnnVolumeSnapshotBeingDeleted(conten // Set AnnVolumeSnapshotBeingDeleted if it is not set yet if !metav1.HasAnnotation(content.ObjectMeta, utils.AnnVolumeSnapshotBeingDeleted) { klog.V(5).Infof("setAnnVolumeSnapshotBeingDeleted: set annotation [%s] on content [%s].", utils.AnnVolumeSnapshotBeingDeleted, content.Name) + var patches []utils.PatchOp metav1.SetMetaDataAnnotation(&content.ObjectMeta, utils.AnnVolumeSnapshotBeingDeleted, "yes") + patches = append(patches, utils.PatchOp{ + Op: "replace", + Path: "/metadata/annotations", + Value: content.ObjectMeta.GetAnnotations(), + }) - updateContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), content, metav1.UpdateOptions{}) + patchedContent, err := utils.PatchVolumeSnapshotContent(content, patches, ctrl.clientset) if err != nil { return content, newControllerUpdateError(content.Name, err.Error()) } + // update content if update is successful - content = updateContent + content = patchedContent _, err = ctrl.storeContentUpdate(content) if err != nil { From b2d17cd6bbabed5c72b3f4773d2e0fb1d87c64ba Mon Sep 17 00:00:00 2001 From: Grant Griffiths Date: Tue, 28 Sep 2021 17:40:55 -0700 Subject: [PATCH 06/10] setAnnVolumeSnapshotBeingCreated patch impl Signed-off-by: Grant Griffiths --- pkg/sidecar-controller/snapshot_controller.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/pkg/sidecar-controller/snapshot_controller.go b/pkg/sidecar-controller/snapshot_controller.go index 1f930472..0cb8d5f9 100644 --- a/pkg/sidecar-controller/snapshot_controller.go +++ b/pkg/sidecar-controller/snapshot_controller.go @@ -587,12 +587,19 @@ func (ctrl *csiSnapshotSideCarController) setAnnVolumeSnapshotBeingCreated(conte contentClone := content.DeepCopy() metav1.SetMetaDataAnnotation(&contentClone.ObjectMeta, utils.AnnVolumeSnapshotBeingCreated, "yes") - updatedContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{}) + var patches []utils.PatchOp + patches = append(patches, utils.PatchOp{ + Op: "replace", + Path: "/metadata/annotations", + Value: contentClone.ObjectMeta.GetAnnotations(), + }) + + patchedContent, err := utils.PatchVolumeSnapshotContent(contentClone, patches, ctrl.clientset) if err != nil { return content, newControllerUpdateError(content.Name, err.Error()) } // update content if update is successful - content = updatedContent + content = patchedContent _, err = ctrl.storeContentUpdate(content) if err != nil { From 2980e22122f71d6c57212582908b44e966494e2b Mon Sep 17 00:00:00 2001 From: Grant Griffiths Date: Wed, 29 Sep 2021 13:42:50 -0700 Subject: [PATCH 07/10] Allow csi-snapshotter to patch vsc Signed-off-by: Grant Griffiths --- deploy/kubernetes/csi-snapshotter/rbac-csi-snapshotter.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/deploy/kubernetes/csi-snapshotter/rbac-csi-snapshotter.yaml b/deploy/kubernetes/csi-snapshotter/rbac-csi-snapshotter.yaml index 9ca4e00a..62d1b214 100644 --- a/deploy/kubernetes/csi-snapshotter/rbac-csi-snapshotter.yaml +++ b/deploy/kubernetes/csi-snapshotter/rbac-csi-snapshotter.yaml @@ -35,7 +35,7 @@ rules: verbs: ["get", "list", "watch"] - apiGroups: ["snapshot.storage.k8s.io"] resources: ["volumesnapshotcontents"] - verbs: ["create", "get", "list", "watch", "update", "delete"] + verbs: ["create", "get", "list", "watch", "update", "delete", "patch"] - apiGroups: ["snapshot.storage.k8s.io"] resources: ["volumesnapshotcontents/status"] verbs: ["update"] From 0ccf8017800f0f6160f5003f2cf333954dfa4a91 Mon Sep 17 00:00:00 2001 From: Grant Griffiths Date: Wed, 29 Sep 2021 14:25:33 -0700 Subject: [PATCH 08/10] Add volumesnapshotcontents/status patch rbac Signed-off-by: Grant Griffiths --- deploy/kubernetes/csi-snapshotter/rbac-csi-snapshotter.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/deploy/kubernetes/csi-snapshotter/rbac-csi-snapshotter.yaml b/deploy/kubernetes/csi-snapshotter/rbac-csi-snapshotter.yaml index 62d1b214..db4aefe8 100644 --- a/deploy/kubernetes/csi-snapshotter/rbac-csi-snapshotter.yaml +++ b/deploy/kubernetes/csi-snapshotter/rbac-csi-snapshotter.yaml @@ -38,7 +38,7 @@ rules: verbs: ["create", "get", "list", "watch", "update", "delete", "patch"] - apiGroups: ["snapshot.storage.k8s.io"] resources: ["volumesnapshotcontents/status"] - verbs: ["update"] + verbs: ["update", "patch"] --- kind: ClusterRoleBinding From d14e2eea8ffbcdecda18d1cea0a9a421457b8a31 Mon Sep 17 00:00:00 2001 From: Grant Griffiths Date: Wed, 29 Sep 2021 16:23:42 -0700 Subject: [PATCH 09/10] Use patch for snapshot-controller when there are no finalizers - Also address PR feedback re: avoid a deepCopy for annotations patch Signed-off-by: Grant Griffiths --- pkg/common-controller/framework_test.go | 6 ---- pkg/common-controller/snapshot_controller.go | 30 +++++++------------ pkg/sidecar-controller/snapshot_controller.go | 13 +++++--- 3 files changed, 20 insertions(+), 29 deletions(-) diff --git a/pkg/common-controller/framework_test.go b/pkg/common-controller/framework_test.go index 120ce9b5..673946c8 100644 --- a/pkg/common-controller/framework_test.go +++ b/pkg/common-controller/framework_test.go @@ -354,12 +354,6 @@ func (r *snapshotReactor) React(action core.Action) (handled bool, ret runtime.O 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()) } diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go index e2f56e91..db346b4e 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/pkg/common-controller/snapshot_controller.go @@ -1421,23 +1421,16 @@ func (ctrl *csiSnapshotCommonController) addSnapshotFinalizer(snapshot *crdv1.Vo var updatedSnapshot *crdv1.VolumeSnapshot var err error - // Must perform an update if no finalizers exist + var patches []utils.PatchOp if len(snapshot.ObjectMeta.Finalizers) == 0 { - 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()) - } + // 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}, + }) } else { // Otherwise, perform a patch - var patches []utils.PatchOp - if addSourceFinalizer { patches = append(patches, utils.PatchOp{ Op: "add", @@ -1452,11 +1445,10 @@ 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/sidecar-controller/snapshot_controller.go b/pkg/sidecar-controller/snapshot_controller.go index 0cb8d5f9..313f173d 100644 --- a/pkg/sidecar-controller/snapshot_controller.go +++ b/pkg/sidecar-controller/snapshot_controller.go @@ -583,18 +583,23 @@ func (ctrl *csiSnapshotSideCarController) setAnnVolumeSnapshotBeingCreated(conte } // Set AnnVolumeSnapshotBeingCreated + // Combine existing annotations with the new annotations. + // If there are no existing annotations, we create a new map. klog.V(5).Infof("setAnnVolumeSnapshotBeingCreated: set annotation [%s:yes] on content [%s].", utils.AnnVolumeSnapshotBeingCreated, content.Name) - contentClone := content.DeepCopy() - metav1.SetMetaDataAnnotation(&contentClone.ObjectMeta, utils.AnnVolumeSnapshotBeingCreated, "yes") + patchedAnnotations := make(map[string]string) + for k, v := range content.GetAnnotations() { + patchedAnnotations[k] = v + } + patchedAnnotations[utils.AnnVolumeSnapshotBeingCreated] = "yes" var patches []utils.PatchOp patches = append(patches, utils.PatchOp{ Op: "replace", Path: "/metadata/annotations", - Value: contentClone.ObjectMeta.GetAnnotations(), + Value: patchedAnnotations, }) - patchedContent, err := utils.PatchVolumeSnapshotContent(contentClone, patches, ctrl.clientset) + patchedContent, err := utils.PatchVolumeSnapshotContent(content, patches, ctrl.clientset) if err != nil { return content, newControllerUpdateError(content.Name, err.Error()) } From ad7dfe60451936fe0b5fa5201c174f3ebbcf0c0c Mon Sep 17 00:00:00 2001 From: Grant Griffiths Date: Wed, 29 Sep 2021 19:22:43 -0700 Subject: [PATCH 10/10] Revert patch for empty finalizers Signed-off-by: Grant Griffiths --- pkg/common-controller/framework_test.go | 24 +++++++++----- pkg/common-controller/snapshot_controller.go | 32 ++++++++++++------- .../snapshot_finalizer_test.go | 10 +++++- 3 files changed, 46 insertions(+), 20 deletions(-) diff --git a/pkg/common-controller/framework_test.go b/pkg/common-controller/framework_test.go index 673946c8..78860c07 100644 --- 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,8 +1513,10 @@ 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 } @@ -1519,15 +1524,18 @@ func evaluateFinalizerTests(ctrl *csiSnapshotCommonController, reactor *snapshot } } 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 index db346b4e..689e845b 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/pkg/common-controller/snapshot_controller.go @@ -1421,16 +1421,25 @@ func (ctrl *csiSnapshotCommonController) addSnapshotFinalizer(snapshot *crdv1.Vo var updatedSnapshot *crdv1.VolumeSnapshot var err error - var patches []utils.PatchOp + // NOTE(ggriffiths): Must perform an update if no finalizers exist. + // Unable to find a patch that correctly updated the finalizers if none currently 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 +1454,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 index ecaf0246..8e5a3f06 100644 --- 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,