diff --git a/deploy/kubernetes/csi-snapshotter/rbac-csi-snapshotter.yaml b/deploy/kubernetes/csi-snapshotter/rbac-csi-snapshotter.yaml index 9ca4e00a..db4aefe8 100644 --- a/deploy/kubernetes/csi-snapshotter/rbac-csi-snapshotter.yaml +++ b/deploy/kubernetes/csi-snapshotter/rbac-csi-snapshotter.yaml @@ -35,10 +35,10 @@ 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"] + verbs: ["update", "patch"] --- kind: ClusterRoleBinding 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/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..78860c07 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,45 @@ 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"): + 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) + } else { + return true, nil, fmt.Errorf("cannot update snapshot %s: snapshot not found", action.GetName()) + } + + // Store the updated object to appropriate places. + 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 + case action.Matches("get", "volumesnapshotcontents"): name := action.(core.GetAction).GetName() content, found := r.contents[name] @@ -437,6 +517,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 +795,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) @@ -1169,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) } @@ -1426,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 } @@ -1435,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 48786f84..689e845b 100644 --- a/pkg/common-controller/snapshot_controller.go +++ b/pkg/common-controller/snapshot_controller.go @@ -809,10 +809,25 @@ func (ctrl *csiSnapshotCommonController) updateSnapshotErrorStatusWithEvent(snap // addContentFinalizer adds a Finalizer for VolumeSnapshotContent. func (ctrl *csiSnapshotCommonController) addContentFinalizer(content *crdv1.VolumeSnapshotContent) 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{}) + 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()) } @@ -983,15 +998,26 @@ 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 + + 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 } @@ -1392,24 +1418,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 + + // 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 { + 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", + Path: "/metadata/finalizers/-", + Value: utils.VolumeSnapshotAsSourceFinalizer, + }) + } + if addBoundFinalizer { + patches = append(patches, utils.PatchOp{ + Op: "add", + Path: "/metadata/finalizers/-", + Value: utils.VolumeSnapshotBoundFinalizer, + }) + } + + 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 } @@ -1489,14 +1546,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 { 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, 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/pkg/sidecar-controller/snapshot_controller.go b/pkg/sidecar-controller/snapshot_controller.go index ec0c6deb..313f173d 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) @@ -576,16 +583,28 @@ 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" - 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: patchedAnnotations, + }) + + 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 = updatedContent + content = patchedContent _, err = ctrl.storeContentUpdate(content) if err != nil { 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 +} 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