Merge pull request #32 from jingxu97/contentdefaultclass

Handle the CSI driver in VolumeSnapshotContent does not match case

pkg/controller/framework_test.go

@@ -34,6 +34,7 @@ import (
 	crdv1 "github.com/kubernetes-csi/external-snapshotter/pkg/apis/volumesnapshot/v1alpha1"
 	clientset "github.com/kubernetes-csi/external-snapshotter/pkg/client/clientset/versioned"
 	"github.com/kubernetes-csi/external-snapshotter/pkg/client/clientset/versioned/fake"
+	snapshotscheme "github.com/kubernetes-csi/external-snapshotter/pkg/client/clientset/versioned/scheme"
 	informers "github.com/kubernetes-csi/external-snapshotter/pkg/client/informers/externalversions"
 	storagelisters "github.com/kubernetes-csi/external-snapshotter/pkg/client/listers/volumesnapshot/v1alpha1"
 	"k8s.io/api/core/v1"
@@ -48,6 +49,7 @@ import (
 	"k8s.io/apimachinery/pkg/watch"
 	"k8s.io/client-go/kubernetes"
 	kubefake "k8s.io/client-go/kubernetes/fake"
+	"k8s.io/client-go/kubernetes/scheme"
 	core "k8s.io/client-go/testing"
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/client-go/tools/record"
@@ -786,6 +788,14 @@ func newContentArray(name, className, snapshotHandle, volumeUID, volumeName, bou
 	}
 }
 
+func newContentWithUnmatchDriverArray(name, className, snapshotHandle, volumeUID, volumeName, boundToSnapshotUID, boundToSnapshotName string, size *int64, creationTime *int64) []*crdv1.VolumeSnapshotContent {
+	content := newContent(name, className, snapshotHandle, volumeUID, volumeName, boundToSnapshotUID, boundToSnapshotName, size, creationTime)
+	content.Spec.VolumeSnapshotSource.CSI.Driver = "fake"
+	return []*crdv1.VolumeSnapshotContent{
+		content,
+	}
+}
+
 func newSnapshot(name, className, boundToContent, snapshotUID, claimName string, ready bool, err *storagev1beta1.VolumeError, creationTime *metav1.Time, size *resource.Quantity) *crdv1.VolumeSnapshot {
 	snapshot := crdv1.VolumeSnapshot{
 		ObjectMeta: metav1.ObjectMeta{
@@ -1005,6 +1015,7 @@ func evaluateTestResults(ctrl *csiSnapshotController, reactor *snapshotReactor,
 //    controllerTest.testCall *once*.
 // 3. Compare resulting contents and snapshots with expected contents and snapshots.
 func runSyncTests(t *testing.T, tests []controllerTest, snapshotClasses []*crdv1.VolumeSnapshotClass) {
+	snapshotscheme.AddToScheme(scheme.Scheme)
 	for _, test := range tests {
 		glog.V(4).Infof("starting test %q", test.name)
 
@@ -1023,8 +1034,10 @@ func runSyncTests(t *testing.T, tests []controllerTest, snapshotClasses []*crdv1
 			reactor.snapshots[snapshot.Name] = snapshot
 		}
 		for _, content := range test.initialContents {
-			ctrl.contentStore.Add(content)
+			if ctrl.isDriverMatch(test.initialContents[0]) {
-			reactor.contents[content.Name] = content
+				ctrl.contentStore.Add(content)
+				reactor.contents[content.Name] = content
+			}
 		}
 		for _, claim := range test.initialClaims {
 			reactor.claims[claim.Name] = claim

pkg/controller/snapshot_controller.go

@@ -85,49 +85,49 @@ const IsDefaultSnapshotClassAnnotation = "snapshot.storage.kubernetes.io/is-defa
 func (ctrl *csiSnapshotController) syncContent(content *crdv1.VolumeSnapshotContent) error {
 	glog.V(5).Infof("synchronizing VolumeSnapshotContent[%s]", content.Name)
 
-	// VolumeSnapshotContent is not bound to any VolumeSnapshot, this case rare and we just return err
+	// VolumeSnapshotContent is not bound to any VolumeSnapshot, in this case we just return err
 	if content.Spec.VolumeSnapshotRef == nil {
 		// content is not bound
 		glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: VolumeSnapshotContent is not bound to any VolumeSnapshot", content.Name)
 		ctrl.eventRecorder.Event(content, v1.EventTypeWarning, "SnapshotContentNotBound", "VolumeSnapshotContent is not bound to any VolumeSnapshot")
 		return fmt.Errorf("volumeSnapshotContent %s is not bound to any VolumeSnapshot", content.Name)
-	} else {
-		glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: content is bound to snapshot %s", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
-		// The VolumeSnapshotContent is reserved for a VolumeSnapshot;
-		// that VolumeSnapshot has not yet been bound to this VolumeSnapshotContent; the VolumeSnapshot sync will handle it.
-		if content.Spec.VolumeSnapshotRef.UID == "" {
-			glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: VolumeSnapshotContent is pre-bound to VolumeSnapshot %s", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
-			return nil
-		}
-		// Get the VolumeSnapshot by _name_
-		var snapshot *crdv1.VolumeSnapshot
-		snapshotName := snapshotRefKey(content.Spec.VolumeSnapshotRef)
-		obj, found, err := ctrl.snapshotStore.GetByKey(snapshotName)
-		if err != nil {
-			return err
-		}
-		if !found {
-			glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: snapshot %s not found", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
-			// Fall through with snapshot = nil
-		} else {
-			var ok bool
-			snapshot, ok = obj.(*crdv1.VolumeSnapshot)
-			if !ok {
-				return fmt.Errorf("cannot convert object from snapshot cache to snapshot %q!?: %#v", content.Name, obj)
-			}
-			glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: snapshot %s found", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
-		}
-		if snapshot != nil && snapshot.UID != content.Spec.VolumeSnapshotRef.UID {
-			// The snapshot that the content was pointing to was deleted, and another
-			// with the same name created.
-			glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: content %s has different UID, the old one must have been deleted", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
-			// Treat the volume as bound to a missing claim.
-			snapshot = nil
-		}
-		if snapshot == nil {
-			ctrl.deleteSnapshotContent(content)
-		}
 	}
+	glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: content is bound to snapshot %s", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
+	// The VolumeSnapshotContent is reserved for a VolumeSnapshot;
+	// that VolumeSnapshot has not yet been bound to this VolumeSnapshotContent; the VolumeSnapshot sync will handle it.
+	if content.Spec.VolumeSnapshotRef.UID == "" {
+		glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: VolumeSnapshotContent is pre-bound to VolumeSnapshot %s", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
+		return nil
+	}
+	// Get the VolumeSnapshot by _name_
+	var snapshot *crdv1.VolumeSnapshot
+	snapshotName := snapshotRefKey(content.Spec.VolumeSnapshotRef)
+	obj, found, err := ctrl.snapshotStore.GetByKey(snapshotName)
+	if err != nil {
+		return err
+	}
+	if !found {
+		glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: snapshot %s not found", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
+		// Fall through with snapshot = nil
+	} else {
+		var ok bool
+		snapshot, ok = obj.(*crdv1.VolumeSnapshot)
+		if !ok {
+			return fmt.Errorf("cannot convert object from snapshot cache to snapshot %q!?: %#v", content.Name, obj)
+		}
+		glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: snapshot %s found", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
+	}
+	if snapshot != nil && snapshot.UID != content.Spec.VolumeSnapshotRef.UID {
+		// The snapshot that the content was pointing to was deleted, and another
+		// with the same name created.
+		glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: content %s has different UID, the old one must have been deleted", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
+		// Treat the content as bound to a missing snapshot.
+		snapshot = nil
+	}
+	if snapshot == nil {
+		ctrl.deleteSnapshotContent(content)
+	}
+
 	return nil
 }
 
@@ -506,19 +506,20 @@ func (ctrl *csiSnapshotController) createSnapshotOperation(snapshot *crdv1.Volum
 	}
 	// Create VolumeSnapshotContent in the database
 	volumeRef, err := ref.GetReference(scheme.Scheme, volume)
+	if err != nil {
+		return nil, err
+	}
+	snapshotRef, err := ref.GetReference(scheme.Scheme, snapshot)
+	if err != nil {
+		return nil, err
+	}
 
 	snapshotContent := &crdv1.VolumeSnapshotContent{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: contentName,
 		},
 		Spec: crdv1.VolumeSnapshotContentSpec{
-			VolumeSnapshotRef: &v1.ObjectReference{
+			VolumeSnapshotRef:   snapshotRef,
-				Kind:       "VolumeSnapshot",
-				Namespace:  snapshot.Namespace,
-				Name:       snapshot.Name,
-				UID:        snapshot.UID,
-				APIVersion: "snapshot.storage.k8s.io/v1alpha1",
-			},
 			PersistentVolumeRef: volumeRef,
 			VolumeSnapshotSource: crdv1.VolumeSnapshotSource{
 				CSI: &crdv1.CSIVolumeSnapshotSource{

pkg/controller/snapshot_controller_base.go

@@ -271,20 +271,12 @@ func (ctrl *csiSnapshotController) contentWorker() {
 			return false
 		}
 		content, err := ctrl.contentLister.Get(name)
+		// The content still exists in informer cache, the event must have
+		// been add/update/sync
 		if err == nil {
-			// Skip update if content is for another CSI driver
+			if ctrl.isDriverMatch(content) {
-			snapshotClassName := content.Spec.VolumeSnapshotClassName
+				ctrl.updateContent(content)
-			if snapshotClassName != nil {
-				if snapshotClass, err := ctrl.classLister.Get(*snapshotClassName); err == nil {
-					if snapshotClass.Snapshotter != ctrl.snapshotterName {
-						return false
-					}
-				}
 			}
-
-			// The content still exists in informer cache, the event must have
-			// been add/update/sync
-			ctrl.updateContent(content)
 			return false
 		}
 		if !errors.IsNotFound(err) {
@@ -322,6 +314,27 @@ func (ctrl *csiSnapshotController) contentWorker() {
 	}
 }
 
+// verify whether the driver specified in VolumeSnapshotContent matches the controller's driver name
+func (ctrl *csiSnapshotController) isDriverMatch(content *crdv1.VolumeSnapshotContent) bool {
+	if content.Spec.VolumeSnapshotSource.CSI == nil {
+		// Skip this snapshot content if it not a CSI snapshot
+		return false
+	}
+	if content.Spec.VolumeSnapshotSource.CSI.Driver != ctrl.snapshotterName {
+		// Skip this snapshot content if the driver does not match
+		return false
+	}
+	snapshotClassName := content.Spec.VolumeSnapshotClassName
+	if snapshotClassName != nil {
+		if snapshotClass, err := ctrl.classLister.Get(*snapshotClassName); err == nil {
+			if snapshotClass.Snapshotter != ctrl.snapshotterName {
+				return false
+			}
+		}
+	}
+	return true
+}
+
 // checkAndUpdateSnapshotClass gets the VolumeSnapshotClass from VolumeSnapshot. If it is not set,
 // gets it from default VolumeSnapshotClass and sets it. It also detects if snapshotter in the
 // VolumeSnapshotClass is the same as the snapshotter in external controller.

pkg/controller/snapshot_delete_test.go

@@ -118,7 +118,7 @@ var snapshotClasses = []*crdv1.VolumeSnapshotClass{
 func TestDeleteSync(t *testing.T) {
 	tests := []controllerTest{
 		{
-			name:                "1-1 - successful delete with empty snapshot class",
+			name:                "1-1 - content with empty snapshot class is deleted if it is bound to a non-exist snapshot and also has a snapshot uid specified",
 			initialContents:     newContentArray("content1-1", classEmpty, "sid1-1", "vuid1-1", "volume1-1", "snapuid1-1", "snap1-1", nil, nil),
 			expectedContents:    nocontents,
 			initialSnapshots:    nosnapshots,
@@ -128,6 +128,17 @@ func TestDeleteSync(t *testing.T) {
 			expectedDeleteCalls: []deleteCall{{"sid1-1", nil, nil}},
 			test:                testSyncContent,
 		},
+		{
+			name:                "2-1 - content with empty snapshot class will not be deleted if it is bound to a non-exist snapshot but it does not have a snapshot uid specified",
+			initialContents:     newContentArray("content2-1", classEmpty, "sid2-1", "vuid2-1", "volume2-1", "", "snap2-1", nil, nil),
+			expectedContents:    newContentArray("content2-1", classEmpty, "sid2-1", "vuid2-1", "volume2-1", "", "snap2-1", nil, nil),
+			initialSnapshots:    nosnapshots,
+			expectedSnapshots:   nosnapshots,
+			expectedEvents:      noevents,
+			errors:              noerrors,
+			expectedDeleteCalls: []deleteCall{{"sid2-1", nil, nil}},
+			test:                testSyncContent,
+		},
 		{
 			name:                "1-2 - successful delete with snapshot class that has empty secret parameter",
 			initialContents:     newContentArray("content1-2", emptySecretClass, "sid1-2", "vuid1-2", "volume1-2", "snapuid1-2", "snap1-2", nil, nil),
@@ -222,6 +233,40 @@ func TestDeleteSync(t *testing.T) {
 				reactor.lock.Unlock()
 			}),
 		},
+		{
+			name:              "1-9 - content will not be deleted if it is bound to a snapshot correctly, snapshot uid is specified",
+			initialContents:   newContentArray("content1-9", validSecretClass, "sid1-9", "vuid1-9", "volume1-9", "snapuid1-9", "snap1-9", nil, nil),
+			expectedContents:  newContentArray("content1-9", validSecretClass, "sid1-9", "vuid1-9", "volume1-9", "snapuid1-9", "snap1-9", nil, nil),
+			initialSnapshots:  newSnapshotArray("snap1-9", validSecretClass, "content1-9", "snapuid1-9", "claim1-9", false, nil, nil, nil),
+			expectedSnapshots: newSnapshotArray("snap1-9", validSecretClass, "content1-9", "snapuid1-9", "claim1-9", false, nil, nil, nil),
+			expectedEvents:    noevents,
+			initialSecrets:    []*v1.Secret{secret()},
+			errors:            noerrors,
+			test:              testSyncContent,
+		},
+		{
+			name:                "1-10 - should delete content which is bound to a snapshot incorrectly",
+			initialContents:     newContentArray("content1-10", validSecretClass, "sid1-10", "vuid1-10", "volume1-10", "snapuid1-10-x", "snap1-10", nil, nil),
+			expectedContents:    nocontents,
+			initialSnapshots:    newSnapshotArray("snap1-10", validSecretClass, "content1-10", "snapuid1-10", "claim1-10", false, nil, nil, nil),
+			expectedSnapshots:   newSnapshotArray("snap1-10", validSecretClass, "content1-10", "snapuid1-10", "claim1-10", false, nil, nil, nil),
+			expectedEvents:      noevents,
+			initialSecrets:      []*v1.Secret{secret()},
+			errors:              noerrors,
+			expectedDeleteCalls: []deleteCall{{"sid1-10", map[string]string{"foo": "bar"}, nil}},
+			test:                testSyncContent,
+		},
+		{
+			name:              "1-11 - content will not be deleted if it is bound to a snapshot correctly, snapsht uid is not specified",
+			initialContents:   newContentArray("content1-11", validSecretClass, "sid1-11", "vuid1-11", "volume1-11", "", "snap1-11", nil, nil),
+			expectedContents:  newContentArray("content1-11", validSecretClass, "sid1-11", "vuid1-11", "volume1-11", "", "snap1-11", nil, nil),
+			initialSnapshots:  newSnapshotArray("snap1-11", validSecretClass, "content1-11", "snapuid1-11", "claim1-11", false, nil, nil, nil),
+			expectedSnapshots: newSnapshotArray("snap1-11", validSecretClass, "content1-11", "snapuid1-11", "claim1-11", false, nil, nil, nil),
+			expectedEvents:    noevents,
+			initialSecrets:    []*v1.Secret{secret()},
+			errors:            noerrors,
+			test:              testSyncContent,
+		},
 	}
 	runSyncTests(t, tests, snapshotClasses)
 }

pkg/controller/snapshot_ready_test.go

@@ -232,6 +232,26 @@ func TestSync(t *testing.T) {
 			errors:            noerrors,
 			test:              testSyncSnapshot,
 		},
+		{
+			name:              "3-5 - snapshot bound to content in which the driver does not match",
+			initialContents:   newContentWithUnmatchDriverArray("content3-5", validSecretClass, "sid3-5", "vuid3-5", "volume3-5", "", "snap3-5", nil, nil),
+			expectedContents:  nocontents,
+			initialSnapshots:  newSnapshotArray("snap3-5", validSecretClass, "content3-5", "snapuid3-5", "claim3-5", false, nil, nil, nil),
+			expectedSnapshots: newSnapshotArray("snap3-5", validSecretClass, "content3-5", "snapuid3-5", "claim3-5", false, newVolumeError("VolumeSnapshotContent is missing"), nil, nil),
+			expectedEvents:    []string{"Warning SnapshotContentMissing"},
+			errors:            noerrors,
+			test:              testSyncSnapshotError,
+		},
+		{
+			name:              "3-6 - snapshot bound to content in which the snapshot uid does not match",
+			initialContents:   newContentArray("content3-4", validSecretClass, "sid3-4", "vuid3-4", "volume3-4", "snapuid3-4-x", "snap3-6", nil, nil),
+			expectedContents:  newContentArray("content3-4", validSecretClass, "sid3-4", "vuid3-4", "volume3-4", "snapuid3-4-x", "snap3-6", nil, nil),
+			initialSnapshots:  newSnapshotArray("snap3-5", validSecretClass, "content3-5", "snapuid3-5", "claim3-5", false, nil, nil, nil),
+			expectedSnapshots: newSnapshotArray("snap3-5", validSecretClass, "content3-5", "snapuid3-5", "claim3-5", false, newVolumeError("VolumeSnapshotContent is missing"), nil, nil),
+			expectedEvents:    []string{"Warning SnapshotContentMissing"},
+			errors:            noerrors,
+			test:              testSyncSnapshotError,
+		},
 	}
 
 	runSyncTests(t, tests, snapshotClasses)