Add more sidecar tests and cleanup sidecar code

Signed-off-by: Grant Griffiths <grant@portworx.com>

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{
+	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),
+			name:             "1-1: Basic content update ready to use",
-		expectedContents: newContentArrayWithReadyToUse("content1-1", "snapuid1-1", "snap1-1", "sid1-1", defaultClass, "", "volume-handle-1-1", retainPolicy, nil, &defaultSize, &True, true),
+			initialContents:  newContentArrayWithReadyToUse("content1-1", "snapuid1-1", "snap1-1", "sid1-1", defaultClass, "", "volume-handle-1-1", retainPolicy, nil, &defaultSize, &False, true),
-		expectedEvents:   noevents,
+			expectedContents: newContentArrayWithReadyToUse("content1-1", "snapuid1-1", "snap1-1", "sid1-1", defaultClass, "", "volume-handle-1-1", retainPolicy, nil, &defaultSize, &True, true),
-		expectedCreateCalls: []createCall{
+			expectedEvents:   noevents,
-			{
+			expectedCreateCalls: []createCall{
-				volumeHandle: "volume-handle-1-1",
+				{
-				snapshotName: "snapshot-snapuid1-1",
+					volumeHandle: "volume-handle-1-1",
-				driverName:   mockDriverName,
+					snapshotName: "snapshot-snapuid1-1",
-				snapshotId:   "snapuid1-1",
+					driverName:   mockDriverName,
-				creationTime: timeNow,
+					snapshotId:   "snapuid1-1",
-				readyToUse:   true,
+					creationTime: timeNow,
+					readyToUse:   true,
+				},
 			},
+			expectedListCalls: []listCall{{"sid1-1", 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,
+			name: "1-2: Basic sync content create snapshot",
-		test:              testSyncContent,
+			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,
+		},
+	}
 
 	runSyncContentTests(t, tests, snapshotClasses)
 }

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 }

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.createSnapshot(content)
-				ctrl.updateContentErrorStatusWithEvent(content, v1.EventTypeWarning, "SnapshotCreationFailed", fmt.Sprintf("Failed to create snapshot with error %v", err))
-				return err
-			}
 		} 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.checkandUpdateContentStatus(content)
-				ctrl.updateContentErrorStatusWithEvent(content, v1.EventTypeWarning, "SnapshotContentStatusUpdateFailed", fmt.Sprintf("Failed to update snapshot content status with error %v", err))
-				return err
-			}
 		}
 	}
 	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

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,
 	},
 	{