diff --git a/pkg/controller/framework_test.go b/pkg/controller/framework_test.go index b3c11530..e789a2f8 100644 --- a/pkg/controller/framework_test.go +++ b/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) - reactor.contents[content.Name] = content + if ctrl.isDriverMatch(test.initialContents[0]) { + ctrl.contentStore.Add(content) + reactor.contents[content.Name] = content + } } for _, claim := range test.initialClaims { reactor.claims[claim.Name] = claim diff --git a/pkg/controller/snapshot_controller.go b/pkg/controller/snapshot_controller.go index fdb32a30..1efb4eb8 100644 --- a/pkg/controller/snapshot_controller.go +++ b/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{ - Kind: "VolumeSnapshot", - Namespace: snapshot.Namespace, - Name: snapshot.Name, - UID: snapshot.UID, - APIVersion: "snapshot.storage.k8s.io/v1alpha1", - }, + VolumeSnapshotRef: snapshotRef, PersistentVolumeRef: volumeRef, VolumeSnapshotSource: crdv1.VolumeSnapshotSource{ CSI: &crdv1.CSIVolumeSnapshotSource{ diff --git a/pkg/controller/snapshot_controller_base.go b/pkg/controller/snapshot_controller_base.go index a5d34452..245c6079 100644 --- a/pkg/controller/snapshot_controller_base.go +++ b/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 - snapshotClassName := content.Spec.VolumeSnapshotClassName - if snapshotClassName != nil { - if snapshotClass, err := ctrl.classLister.Get(*snapshotClassName); err == nil { - if snapshotClass.Snapshotter != ctrl.snapshotterName { - return false - } - } + if ctrl.isDriverMatch(content) { + ctrl.updateContent(content) } - - // 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. diff --git a/pkg/controller/snapshot_delete_test.go b/pkg/controller/snapshot_delete_test.go index 652c6d0f..33e60ab1 100644 --- a/pkg/controller/snapshot_delete_test.go +++ b/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) } diff --git a/pkg/controller/snapshot_ready_test.go b/pkg/controller/snapshot_ready_test.go index c949d7aa..93aeaa96 100644 --- a/pkg/controller/snapshot_ready_test.go +++ b/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)