From fe1c355fa96626a188adb5980e9b763da1e9dcff Mon Sep 17 00:00:00 2001 From: Grant Griffiths Date: Wed, 4 Mar 2020 20:25:31 -0800 Subject: [PATCH] Add more sidecar tests and cleanup sidecar code Signed-off-by: Grant Griffiths --- pkg/sidecar-controller/content_create_test.go | 174 ++++++++++++++++-- pkg/sidecar-controller/framework_test.go | 56 ++++-- pkg/sidecar-controller/snapshot_controller.go | 17 +- .../snapshot_delete_test.go | 10 +- 4 files changed, 210 insertions(+), 47 deletions(-) diff --git a/pkg/sidecar-controller/content_create_test.go b/pkg/sidecar-controller/content_create_test.go index 7358c693a..5185b5ed2 100644 --- a/pkg/sidecar-controller/content_create_test.go +++ b/pkg/sidecar-controller/content_create_test.go @@ -17,32 +17,168 @@ limitations under the License. package sidecar_controller import ( + "errors" "testing" "time" + + crdv1 "github.com/kubernetes-csi/external-snapshotter/v2/pkg/apis/volumesnapshot/v1beta1" + "github.com/kubernetes-csi/external-snapshotter/v2/pkg/utils" + v1 "k8s.io/api/core/v1" ) func TestSyncContent(t *testing.T) { - var tests []controllerTest - - tests = append(tests, controllerTest{ - name: "Basic content update ready to use", - initialContents: newContentArrayWithReadyToUse("content1-1", "snapuid1-1", "snap1-1", "sid1-1", defaultClass, "", "volume-handle-1-1", retainPolicy, nil, &defaultSize, &False, true), - expectedContents: newContentArrayWithReadyToUse("content1-1", "snapuid1-1", "snap1-1", "sid1-1", defaultClass, "", "volume-handle-1-1", retainPolicy, nil, &defaultSize, &True, true), - expectedEvents: noevents, - expectedCreateCalls: []createCall{ - { - volumeHandle: "volume-handle-1-1", - snapshotName: "snapshot-snapuid1-1", - driverName: mockDriverName, - snapshotId: "snapuid1-1", - creationTime: timeNow, - readyToUse: true, + + tests := []controllerTest{ + { + name: "1-1: Basic content update ready to use", + initialContents: newContentArrayWithReadyToUse("content1-1", "snapuid1-1", "snap1-1", "sid1-1", defaultClass, "", "volume-handle-1-1", retainPolicy, nil, &defaultSize, &False, true), + expectedContents: newContentArrayWithReadyToUse("content1-1", "snapuid1-1", "snap1-1", "sid1-1", defaultClass, "", "volume-handle-1-1", retainPolicy, nil, &defaultSize, &True, true), + expectedEvents: noevents, + expectedCreateCalls: []createCall{ + { + volumeHandle: "volume-handle-1-1", + snapshotName: "snapshot-snapuid1-1", + driverName: mockDriverName, + snapshotId: "snapuid1-1", + creationTime: timeNow, + readyToUse: true, + }, + }, + expectedListCalls: []listCall{{"sid1-1", map[string]string{}, true, time.Now(), 1, nil}}, + errors: noerrors, + test: testSyncContent, + }, + { + name: "1-2: Basic sync content create snapshot", + initialContents: withContentStatus(newContentArray("content1-2", "snapuid1-2", "snap1-2", "sid1-2", defaultClass, "", "volume-handle-1-2", retainPolicy, nil, &defaultSize, true), + nil), + expectedContents: withContentStatus(newContentArray("content1-2", "snapuid1-2", "snap1-2", "sid1-2", defaultClass, "", "volume-handle-1-2", retainPolicy, nil, &defaultSize, true), + &crdv1.VolumeSnapshotContentStatus{SnapshotHandle: toStringPointer("snapuid1-2"), RestoreSize: &defaultSize, ReadyToUse: &True}), + expectedEvents: noevents, + expectedCreateCalls: []createCall{ + { + volumeHandle: "volume-handle-1-2", + snapshotName: "snapshot-snapuid1-2", + driverName: mockDriverName, + snapshotId: "snapuid1-2", + creationTime: timeNow, + readyToUse: true, + size: defaultSize, + }, + }, + expectedListCalls: []listCall{{"sid1-2", map[string]string{}, true, time.Now(), 1, nil}}, + errors: noerrors, + test: testSyncContent, + }, + { + 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{ + utils.AnnDeletionSecretRefName: "", + utils.AnnDeletionSecretRefNamespace: "", + }), + expectedContents: withContentAnnotations(withContentStatus(newContentArray("content1-3", "snapuid1-3", "snap1-3", "sid1-3", invalidSecretClass, "", "volume-handle-1-3", retainPolicy, nil, &defaultSize, true), + &crdv1.VolumeSnapshotContentStatus{ + 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\""), + }), map[string]string{ + utils.AnnDeletionSecretRefName: "", + utils.AnnDeletionSecretRefNamespace: "", + }), initialSecrets: []*v1.Secret{}, // no initial secret created + expectedEvents: []string{"Warning SnapshotCreationFailed"}, + errors: noerrors, + test: testSyncContent, + }, + { + name: "1-4: Basic sync content create snapshot with valid secret", + initialContents: withContentAnnotations(withContentStatus(newContentArray("content1-4", "snapuid1-4", "snap1-4", "sid1-4", validSecretClass, "", "volume-handle-1-4", retainPolicy, nil, &defaultSize, true), + nil), map[string]string{ + utils.AnnDeletionSecretRefName: "secret", + utils.AnnDeletionSecretRefNamespace: "default", + }), + expectedContents: withContentAnnotations(withContentStatus(newContentArray("content1-4", "snapuid1-4", "snap1-4", "sid1-4", validSecretClass, "", "volume-handle-1-4", retainPolicy, nil, &defaultSize, true), + &crdv1.VolumeSnapshotContentStatus{ + SnapshotHandle: toStringPointer("snapuid1-4"), + RestoreSize: &defaultSize, + ReadyToUse: &True, + Error: nil, + }), map[string]string{ + utils.AnnDeletionSecretRefName: "secret", + utils.AnnDeletionSecretRefNamespace: "default", + }), + expectedCreateCalls: []createCall{ + { + volumeHandle: "volume-handle-1-4", + snapshotName: "snapshot-snapuid1-4", + parameters: class5Parameters, + secrets: map[string]string{ + "foo": "bar", + }, + driverName: mockDriverName, + snapshotId: "snapuid1-4", + creationTime: timeNow, + readyToUse: true, + size: defaultSize, + }, + }, + initialSecrets: []*v1.Secret{secret()}, + expectedEvents: noevents, + errors: noerrors, + test: testSyncContent, + }, + { + 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{ + utils.AnnDeletionSecretRefName: "secret", + utils.AnnDeletionSecretRefNamespace: "default", + }), + expectedContents: withContentAnnotations(withContentStatus(newContentArray("content1-5", "snapuid1-5", "snap1-5", "sid1-5", invalidSecretClass, "", "volume-handle-1-5", retainPolicy, nil, &defaultSize, true), + &crdv1.VolumeSnapshotContentStatus{ + 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\\\"\""), + }), map[string]string{ + utils.AnnDeletionSecretRefName: "secret", + utils.AnnDeletionSecretRefNamespace: "default", + }), initialSecrets: []*v1.Secret{}, // no initial secret created + expectedEvents: []string{"Warning SnapshotCreationFailed"}, + errors: []reactorError{ + // Inject error to the first client.VolumesnapshotV1beta1().VolumeSnapshots().Update call. + // All other calls will succeed. + {"get", "secrets", errors.New("mock secrets error")}, + }, + test: testSyncContent, + }, + { + name: "1-6: Basic content update ready to use bad snapshot class", + initialContents: newContentArrayWithReadyToUse("content1-6", "snapuid1-6", "snap1-6", "sid1-6", "bad-class", "", "volume-handle-1-6", retainPolicy, nil, &defaultSize, &False, true), + expectedContents: withContentStatus(newContentArray("content1-6", "snapuid1-6", "snap1-6", "sid1-6", "bad-class", "", "volume-handle-1-6", retainPolicy, nil, &defaultSize, true), + &crdv1.VolumeSnapshotContentStatus{ + SnapshotHandle: toStringPointer("sid1-6"), + RestoreSize: &defaultSize, + ReadyToUse: &False, + Error: newSnapshotError("Failed to check and update snapshot content: failed to get input parameters to create snapshot content1-6: \"failed to retrieve snapshot class bad-class from the informer: \\\"volumesnapshotclass.snapshot.storage.k8s.io \\\\\\\"bad-class\\\\\\\" not found\\\"\""), + }), + expectedEvents: []string{"Warning SnapshotContentCheckandUpdateFailed"}, + expectedCreateCalls: []createCall{ + { + volumeHandle: "volume-handle-1-6", + snapshotName: "snapshot-snapuid1-6", + driverName: mockDriverName, + snapshotId: "snapuid1-6", + creationTime: timeNow, + readyToUse: true, + }, }, + expectedListCalls: []listCall{{"sid1-6", map[string]string{}, true, time.Now(), 1, nil}}, + errors: noerrors, + test: testSyncContent, }, - expectedListCalls: []listCall{{"sid1-1", map[string]string{}, true, time.Now(), 1, nil}}, - errors: noerrors, - test: testSyncContent, - }) + } runSyncContentTests(t, tests, snapshotClasses) } diff --git a/pkg/sidecar-controller/framework_test.go b/pkg/sidecar-controller/framework_test.go index d59f0f4a6..264b989ac 100644 --- a/pkg/sidecar-controller/framework_test.go +++ b/pkg/sidecar-controller/framework_test.go @@ -123,6 +123,7 @@ var noerrors = []reactorError{} // the list. type snapshotReactor struct { secrets map[string]*v1.Secret + snapshotClasses map[string]*crdv1.VolumeSnapshotClass contents map[string]*crdv1.VolumeSnapshotContent changedObjects []interface{} changedSinceLastSync int @@ -283,7 +284,12 @@ func (r *snapshotReactor) checkContents(expectedContents []*crdv1.VolumeSnapshot v := v.DeepCopy() v.ResourceVersion = "" v.Spec.VolumeSnapshotRef.ResourceVersion = "" - v.Status.CreationTime = nil + if v.Status != nil { + v.Status.CreationTime = nil + } + if v.Status.Error != nil { + v.Status.Error.Time = &metav1.Time{} + } expectedMap[v.Name] = v } for _, v := range r.contents { @@ -292,7 +298,13 @@ func (r *snapshotReactor) checkContents(expectedContents []*crdv1.VolumeSnapshot v := v.DeepCopy() v.ResourceVersion = "" v.Spec.VolumeSnapshotRef.ResourceVersion = "" - v.Status.CreationTime = nil + if v.Status != nil { + v.Status.CreationTime = nil + if v.Status.Error != nil { + v.Status.Error.Time = &metav1.Time{} + } + } + gotMap[v.Name] = v } if !reflect.DeepEqual(expectedMap, gotMap) { @@ -487,6 +499,7 @@ func (r *snapshotReactor) modifyContentEvent(content *crdv1.VolumeSnapshotConten func newSnapshotReactor(kubeClient *kubefake.Clientset, client *fake.Clientset, ctrl *csiSnapshotSideCarController, fakeVolumeWatch, fakeClaimWatch *watch.FakeWatcher, errors []reactorError) *snapshotReactor { reactor := &snapshotReactor{ secrets: make(map[string]*v1.Secret), + snapshotClasses: make(map[string]*crdv1.VolumeSnapshotClass), contents: make(map[string]*crdv1.VolumeSnapshotContent), ctrl: ctrl, fakeContentWatch: fakeVolumeWatch, @@ -497,6 +510,7 @@ func newSnapshotReactor(kubeClient *kubefake.Clientset, client *fake.Clientset, client.AddReactor("update", "volumesnapshotcontents", reactor.React) client.AddReactor("get", "volumesnapshotcontents", reactor.React) client.AddReactor("delete", "volumesnapshotcontents", reactor.React) + kubeClient.AddReactor("get", "secrets", reactor.React) return reactor } @@ -615,16 +629,6 @@ func newContentArrayWithReadyToUse(contentName, boundToSnapshotUID, boundToSnaps } } -func newContentWithUnmatchDriverArray(contentName, boundToSnapshotUID, boundToSnapshotName, snapshotHandle, snapshotClassName, desiredSnapshotHandle, volumeHandle string, - deletionPolicy crdv1.DeletionPolicy, size, creationTime *int64, - withFinalizer bool) []*crdv1.VolumeSnapshotContent { - content := newContent(contentName, boundToSnapshotUID, boundToSnapshotName, snapshotHandle, snapshotClassName, desiredSnapshotHandle, volumeHandle, deletionPolicy, size, creationTime, withFinalizer, nil) - content.Spec.Driver = "fake" - return []*crdv1.VolumeSnapshotContent{ - content, - } -} - func newContentArrayWithDeletionTimestamp(contentName, boundToSnapshotUID, boundToSnapshotName, snapshotHandle, snapshotClassName, desiredSnapshotHandle, volumeHandle string, deletionPolicy crdv1.DeletionPolicy, size, creationTime *int64, withFinalizer bool, deletionTime *metav1.Time) []*crdv1.VolumeSnapshotContent { @@ -633,6 +637,22 @@ func newContentArrayWithDeletionTimestamp(contentName, boundToSnapshotUID, bound } } +func withContentStatus(content []*crdv1.VolumeSnapshotContent, status *crdv1.VolumeSnapshotContentStatus) []*crdv1.VolumeSnapshotContent { + for i := range content { + content[i].Status = status + } + + return content +} + +func withContentAnnotations(content []*crdv1.VolumeSnapshotContent, annotations map[string]string) []*crdv1.VolumeSnapshotContent { + for i := range content { + content[i].ObjectMeta.Annotations = annotations + } + + return content +} + func testSyncContent(ctrl *csiSnapshotSideCarController, reactor *snapshotReactor, test controllerTest) error { return ctrl.syncContent(test.initialContents[0]) } @@ -742,6 +762,7 @@ func runSyncContentTests(t *testing.T, tests []controllerTest, snapshotClasses [ indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{}) for _, class := range snapshotClasses { indexer.Add(class) + reactor.snapshotClasses[class.Name] = class } ctrl.classLister = storagelisters.NewVolumeSnapshotClassLister(indexer) @@ -875,7 +896,7 @@ func (f *fakeSnapshotter) CreateSnapshot(ctx context.Context, snapshotName strin err = fmt.Errorf("unexpected create snapshot call") } - if !reflect.DeepEqual(call.secrets, snapshotterCredentials) { + if !reflect.DeepEqual(call.secrets, snapshotterCredentials) && !(len(call.secrets) == 0 && len(snapshotterCredentials) == 0) { f.t.Errorf("Wrong CSI Create Snapshot call: snapshotName=%s, volumeHandle=%s, expected secrets %+v, got %+v", snapshotName, volumeHandle, call.secrets, snapshotterCredentials) err = fmt.Errorf("unexpected create snapshot call") } @@ -937,3 +958,12 @@ func (f *fakeSnapshotter) GetSnapshotStatus(ctx context.Context, snapshotID stri return call.readyToUse, call.createTime, call.size, call.err } + +func newSnapshotError(message string) *crdv1.VolumeSnapshotError { + return &crdv1.VolumeSnapshotError{ + Time: &metav1.Time{}, + Message: &message, + } +} + +func toStringPointer(str string) *string { return &str } diff --git a/pkg/sidecar-controller/snapshot_controller.go b/pkg/sidecar-controller/snapshot_controller.go index bf02e70ba..8c2963b68 100644 --- a/pkg/sidecar-controller/snapshot_controller.go +++ b/pkg/sidecar-controller/snapshot_controller.go @@ -55,7 +55,6 @@ const controllerUpdateFailMsg = "snapshot controller failed to update" func (ctrl *csiSnapshotSideCarController) syncContent(content *crdv1.VolumeSnapshotContent) error { klog.V(5).Infof("synchronizing VolumeSnapshotContent[%s]", content.Name) - var err error if ctrl.shouldDelete(content) { klog.V(4).Infof("VolumeSnapshotContent[%s]: the policy is %s", content.Name, content.Spec.DeletionPolicy) if content.Spec.DeletionPolicy == crdv1.VolumeSnapshotContentDelete && @@ -75,10 +74,7 @@ func (ctrl *csiSnapshotSideCarController) syncContent(content *crdv1.VolumeSnaps } else { if content.Spec.Source.VolumeHandle != nil && content.Status == nil { klog.V(5).Infof("syncContent: Call CreateSnapshot for content %s", content.Name) - if err = ctrl.createSnapshot(content); err != nil { - ctrl.updateContentErrorStatusWithEvent(content, v1.EventTypeWarning, "SnapshotCreationFailed", fmt.Sprintf("Failed to create snapshot with error %v", err)) - return err - } + ctrl.createSnapshot(content) } else { // Skip checkandUpdateContentStatus() if ReadyToUse is // already true. We don't want to keep calling CreateSnapshot @@ -87,10 +83,7 @@ func (ctrl *csiSnapshotSideCarController) syncContent(content *crdv1.VolumeSnaps if content.Status != nil && content.Status.ReadyToUse != nil && *content.Status.ReadyToUse == true { return nil } - if err = ctrl.checkandUpdateContentStatus(content); err != nil { - ctrl.updateContentErrorStatusWithEvent(content, v1.EventTypeWarning, "SnapshotContentStatusUpdateFailed", fmt.Sprintf("Failed to update snapshot content status with error %v", err)) - return err - } + ctrl.checkandUpdateContentStatus(content) } } return nil @@ -129,7 +122,7 @@ func (ctrl *csiSnapshotSideCarController) storeContentUpdate(content interface{} } // createSnapshot starts new asynchronous operation to create snapshot -func (ctrl *csiSnapshotSideCarController) createSnapshot(content *crdv1.VolumeSnapshotContent) error { +func (ctrl *csiSnapshotSideCarController) createSnapshot(content *crdv1.VolumeSnapshotContent) { klog.V(5).Infof("createSnapshot for content [%s]: started", content.Name) opName := fmt.Sprintf("create-%s", content.Name) ctrl.scheduleOperation(opName, func() error { @@ -147,10 +140,9 @@ func (ctrl *csiSnapshotSideCarController) createSnapshot(content *crdv1.VolumeSn } return nil }) - return nil } -func (ctrl *csiSnapshotSideCarController) checkandUpdateContentStatus(content *crdv1.VolumeSnapshotContent) error { +func (ctrl *csiSnapshotSideCarController) checkandUpdateContentStatus(content *crdv1.VolumeSnapshotContent) { klog.V(5).Infof("checkandUpdateContentStatus[%s] started", content.Name) opName := fmt.Sprintf("check-%s", content.Name) ctrl.scheduleOperation(opName, func() error { @@ -168,7 +160,6 @@ func (ctrl *csiSnapshotSideCarController) checkandUpdateContentStatus(content *c return nil }) - return nil } // updateContentStatusWithEvent saves new content.Status to API server and emits diff --git a/pkg/sidecar-controller/snapshot_delete_test.go b/pkg/sidecar-controller/snapshot_delete_test.go index 5c4310371..ae25a8bb8 100644 --- a/pkg/sidecar-controller/snapshot_delete_test.go +++ b/pkg/sidecar-controller/snapshot_delete_test.go @@ -68,6 +68,11 @@ var class6Parameters = map[string]string{ utils.PrefixedSnapshotterListSecretNamespaceKey: "default", } +var class7Annotations = map[string]string{ + utils.AnnDeletionSecretRefName: "secret-x", + utils.AnnDeletionSecretRefNamespace: "default-x", +} + var snapshotClasses = []*crdv1.VolumeSnapshotClass{ { TypeMeta: metav1.TypeMeta{ @@ -107,10 +112,11 @@ var snapshotClasses = []*crdv1.VolumeSnapshotClass{ Kind: "VolumeSnapshotClass", }, ObjectMeta: metav1.ObjectMeta{ - Name: invalidSecretClass, + Name: invalidSecretClass, + Annotations: class7Annotations, }, Driver: mockDriverName, - Parameters: class3Parameters, + Parameters: class2Parameters, DeletionPolicy: crdv1.VolumeSnapshotContentDelete, }, {