Skip to content

Commit c401b23

Browse files
jingxu97jingxu97
committed
Handle the CSI driver in VolumeSnapshotContent does not match case
In VolumeSnapshotContent, if the CSI driver does not match the plugin's, the controller should skip this content instead of always processing it. This PR also add a few tests related to snapshot and content static binding. During binding, if content specify its bound snapshot uid and it does not match the snapshot's uid, the content object and also the physical snapshot will be deleted. In this case, the controller will treat the content as an orphan content because its snapshot object does not exist (deleted) any more.
1 parent 5ed076f commit c401b23

File tree

5 files changed

+149
-57
lines changed

5 files changed

+149
-57
lines changed

‎pkg/controller/framework_test.go‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ import (
3434
crdv1 "github.com/kubernetes-csi/external-snapshotter/pkg/apis/volumesnapshot/v1alpha1"
3535
clientset "github.com/kubernetes-csi/external-snapshotter/pkg/client/clientset/versioned"
3636
"github.com/kubernetes-csi/external-snapshotter/pkg/client/clientset/versioned/fake"
37+
snapshotscheme "github.com/kubernetes-csi/external-snapshotter/pkg/client/clientset/versioned/scheme"
3738
informers "github.com/kubernetes-csi/external-snapshotter/pkg/client/informers/externalversions"
3839
storagelisters "github.com/kubernetes-csi/external-snapshotter/pkg/client/listers/volumesnapshot/v1alpha1"
3940
"k8s.io/api/core/v1"
@@ -48,6 +49,7 @@ import (
4849
"k8s.io/apimachinery/pkg/watch"
4950
"k8s.io/client-go/kubernetes"
5051
kubefake "k8s.io/client-go/kubernetes/fake"
52+
"k8s.io/client-go/kubernetes/scheme"
5153
core "k8s.io/client-go/testing"
5254
"k8s.io/client-go/tools/cache"
5355
"k8s.io/client-go/tools/record"
@@ -786,6 +788,14 @@ func newContentArray(name, className, snapshotHandle, volumeUID, volumeName, bou
786788
}
787789
}
788790

791+
func newContentWithUnmatchDriverArray(name, className, snapshotHandle, volumeUID, volumeName, boundToSnapshotUID, boundToSnapshotName string, size *int64, creationTime *int64) []*crdv1.VolumeSnapshotContent {
792+
content := newContent(name, className, snapshotHandle, volumeUID, volumeName, boundToSnapshotUID, boundToSnapshotName, size, creationTime)
793+
content.Spec.VolumeSnapshotSource.CSI.Driver = "fake"
794+
return []*crdv1.VolumeSnapshotContent{
795+
content,
796+
}
797+
}
798+
789799
func newSnapshot(name, className, boundToContent, snapshotUID, claimName string, ready bool, err *storagev1beta1.VolumeError, creationTime *metav1.Time, size *resource.Quantity) *crdv1.VolumeSnapshot {
790800
snapshot := crdv1.VolumeSnapshot{
791801
ObjectMeta: metav1.ObjectMeta{
@@ -1005,6 +1015,7 @@ func evaluateTestResults(ctrl *csiSnapshotController, reactor *snapshotReactor,
10051015
// controllerTest.testCall *once*.
10061016
// 3. Compare resulting contents and snapshots with expected contents and snapshots.
10071017
func runSyncTests(t *testing.T, tests []controllerTest, snapshotClasses []*crdv1.VolumeSnapshotClass) {
1018+
snapshotscheme.AddToScheme(scheme.Scheme)
10081019
for _, test := range tests {
10091020
glog.V(4).Infof("starting test %q", test.name)
10101021

@@ -1023,8 +1034,10 @@ func runSyncTests(t *testing.T, tests []controllerTest, snapshotClasses []*crdv1
10231034
reactor.snapshots[snapshot.Name] = snapshot
10241035
}
10251036
for _, content := range test.initialContents {
1026-
ctrl.contentStore.Add(content)
1027-
reactor.contents[content.Name] = content
1037+
if ctrl.isDriverMatch(test.initialContents[0]) {
1038+
ctrl.contentStore.Add(content)
1039+
reactor.contents[content.Name] = content
1040+
}
10281041
}
10291042
for _, claim := range test.initialClaims {
10301043
reactor.claims[claim.Name] = claim

‎pkg/controller/snapshot_controller.go‎

Lines changed: 43 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -85,49 +85,49 @@ const IsDefaultSnapshotClassAnnotation = "snapshot.storage.kubernetes.io/is-defa
8585
func (ctrl *csiSnapshotController) syncContent(content *crdv1.VolumeSnapshotContent) error {
8686
glog.V(5).Infof("synchronizing VolumeSnapshotContent[%s]", content.Name)
8787

88-
// VolumeSnapshotContent is not bound to any VolumeSnapshot, this case rare and we just return err
88+
// VolumeSnapshotContent is not bound to any VolumeSnapshot, in this case we just return err
8989
if content.Spec.VolumeSnapshotRef == nil {
9090
// content is not bound
9191
glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: VolumeSnapshotContent is not bound to any VolumeSnapshot", content.Name)
9292
ctrl.eventRecorder.Event(content, v1.EventTypeWarning, "SnapshotContentNotBound", "VolumeSnapshotContent is not bound to any VolumeSnapshot")
9393
return fmt.Errorf("volumeSnapshotContent %s is not bound to any VolumeSnapshot", content.Name)
94+
}
95+
glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: content is bound to snapshot %s", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
96+
// The VolumeSnapshotContent is reserved for a VolumeSnapshot;
97+
// that VolumeSnapshot has not yet been bound to this VolumeSnapshotContent; the VolumeSnapshot sync will handle it.
98+
if content.Spec.VolumeSnapshotRef.UID == "" {
99+
glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: VolumeSnapshotContent is pre-bound to VolumeSnapshot %s", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
100+
return nil
101+
}
102+
// Get the VolumeSnapshot by _name_
103+
var snapshot *crdv1.VolumeSnapshot
104+
snapshotName := snapshotRefKey(content.Spec.VolumeSnapshotRef)
105+
obj, found, err := ctrl.snapshotStore.GetByKey(snapshotName)
106+
if err != nil {
107+
return err
108+
}
109+
if !found {
110+
glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: snapshot %s not found", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
111+
// Fall through with snapshot = nil
94112
} else {
95-
glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: content is bound to snapshot %s", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
96-
// The VolumeSnapshotContent is reserved for a VolumeSnapshot;
97-
// that VolumeSnapshot has not yet been bound to this VolumeSnapshotContent; the VolumeSnapshot sync will handle it.
98-
if content.Spec.VolumeSnapshotRef.UID == "" {
99-
glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: VolumeSnapshotContent is pre-bound to VolumeSnapshot %s", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
100-
return nil
101-
}
102-
// Get the VolumeSnapshot by _name_
103-
var snapshot *crdv1.VolumeSnapshot
104-
snapshotName := snapshotRefKey(content.Spec.VolumeSnapshotRef)
105-
obj, found, err := ctrl.snapshotStore.GetByKey(snapshotName)
106-
if err != nil {
107-
return err
108-
}
109-
if !found {
110-
glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: snapshot %s not found", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
111-
// Fall through with snapshot = nil
112-
} else {
113-
var ok bool
114-
snapshot, ok = obj.(*crdv1.VolumeSnapshot)
115-
if !ok {
116-
return fmt.Errorf("cannot convert object from snapshot cache to snapshot %q!?: %#v", content.Name, obj)
117-
}
118-
glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: snapshot %s found", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
119-
}
120-
if snapshot != nil && snapshot.UID != content.Spec.VolumeSnapshotRef.UID {
121-
// The snapshot that the content was pointing to was deleted, and another
122-
// with the same name created.
123-
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))
124-
// Treat the volume as bound to a missing claim.
125-
snapshot = nil
126-
}
127-
if snapshot == nil {
128-
ctrl.deleteSnapshotContent(content)
113+
var ok bool
114+
snapshot, ok = obj.(*crdv1.VolumeSnapshot)
115+
if !ok {
116+
return fmt.Errorf("cannot convert object from snapshot cache to snapshot %q!?: %#v", content.Name, obj)
129117
}
118+
glog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: snapshot %s found", content.Name, snapshotRefKey(content.Spec.VolumeSnapshotRef))
130119
}
120+
if snapshot != nil && snapshot.UID != content.Spec.VolumeSnapshotRef.UID {
121+
// The snapshot that the content was pointing to was deleted, and another
122+
// with the same name created.
123+
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))
124+
// Treat the content as bound to a missing snapshot.
125+
snapshot = nil
126+
}
127+
if snapshot == nil {
128+
ctrl.deleteSnapshotContent(content)
129+
}
130+
131131
return nil
132132
}
133133

@@ -506,19 +506,20 @@ func (ctrl *csiSnapshotController) createSnapshotOperation(snapshot *crdv1.Volum
506506
}
507507
// Create VolumeSnapshotContent in the database
508508
volumeRef, err := ref.GetReference(scheme.Scheme, volume)
509+
if err != nil {
510+
return nil, err
511+
}
512+
snapshotRef, err := ref.GetReference(scheme.Scheme, snapshot)
513+
if err != nil {
514+
return nil, err
515+
}
509516

510517
snapshotContent := &crdv1.VolumeSnapshotContent{
511518
ObjectMeta: metav1.ObjectMeta{
512519
Name: contentName,
513520
},
514521
Spec: crdv1.VolumeSnapshotContentSpec{
515-
VolumeSnapshotRef: &v1.ObjectReference{
516-
Kind: "VolumeSnapshot",
517-
Namespace: snapshot.Namespace,
518-
Name: snapshot.Name,
519-
UID: snapshot.UID,
520-
APIVersion: "snapshot.storage.k8s.io/v1alpha1",
521-
},
522+
VolumeSnapshotRef: snapshotRef,
522523
PersistentVolumeRef: volumeRef,
523524
VolumeSnapshotSource: crdv1.VolumeSnapshotSource{
524525
CSI: &crdv1.CSIVolumeSnapshotSource{

‎pkg/controller/snapshot_controller_base.go‎

Lines changed: 25 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -271,20 +271,12 @@ func (ctrl *csiSnapshotController) contentWorker() {
271271
return false
272272
}
273273
content, err := ctrl.contentLister.Get(name)
274+
// The content still exists in informer cache, the event must have
275+
// been add/update/sync
274276
if err == nil {
275-
// Skip update if content is for another CSI driver
276-
snapshotClassName := content.Spec.VolumeSnapshotClassName
277-
if snapshotClassName != nil {
278-
if snapshotClass, err := ctrl.classLister.Get(*snapshotClassName); err == nil {
279-
if snapshotClass.Snapshotter != ctrl.snapshotterName {
280-
return false
281-
}
282-
}
277+
if ctrl.isDriverMatch(content) {
278+
ctrl.updateContent(content)
283279
}
284-
285-
// The content still exists in informer cache, the event must have
286-
// been add/update/sync
287-
ctrl.updateContent(content)
288280
return false
289281
}
290282
if !errors.IsNotFound(err) {
@@ -322,6 +314,27 @@ func (ctrl *csiSnapshotController) contentWorker() {
322314
}
323315
}
324316

317+
// verify whether the driver specified in VolumeSnapshotContent matches the controller's driver name
318+
func (ctrl *csiSnapshotController) isDriverMatch(content *crdv1.VolumeSnapshotContent) bool {
319+
if content.Spec.VolumeSnapshotSource.CSI == nil {
320+
// Skip this snapshot content if it not a CSI snapshot
321+
return false
322+
}
323+
if content.Spec.VolumeSnapshotSource.CSI.Driver != ctrl.snapshotterName {
324+
// Skip this snapshot content if the driver does not match
325+
return false
326+
}
327+
snapshotClassName := content.Spec.VolumeSnapshotClassName
328+
if snapshotClassName != nil {
329+
if snapshotClass, err := ctrl.classLister.Get(*snapshotClassName); err == nil {
330+
if snapshotClass.Snapshotter != ctrl.snapshotterName {
331+
return false
332+
}
333+
}
334+
}
335+
return true
336+
}
337+
325338
// checkAndUpdateSnapshotClass gets the VolumeSnapshotClass from VolumeSnapshot. If it is not set,
326339
// gets it from default VolumeSnapshotClass and sets it. It also detects if snapshotter in the
327340
// VolumeSnapshotClass is the same as the snapshotter in external controller.

‎pkg/controller/snapshot_delete_test.go‎

Lines changed: 46 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ var snapshotClasses = []*crdv1.VolumeSnapshotClass{
118118
func TestDeleteSync(t *testing.T) {
119119
tests := []controllerTest{
120120
{
121-
name: "1-1 - successful delete with empty snapshot class",
121+
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",
122122
initialContents: newContentArray("content1-1", classEmpty, "sid1-1", "vuid1-1", "volume1-1", "snapuid1-1", "snap1-1", nil, nil),
123123
expectedContents: nocontents,
124124
initialSnapshots: nosnapshots,
@@ -128,6 +128,17 @@ func TestDeleteSync(t *testing.T) {
128128
expectedDeleteCalls: []deleteCall{{"sid1-1", nil, nil}},
129129
test: testSyncContent,
130130
},
131+
{
132+
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",
133+
initialContents: newContentArray("content2-1", classEmpty, "sid2-1", "vuid2-1", "volume2-1", "", "snap2-1", nil, nil),
134+
expectedContents: newContentArray("content2-1", classEmpty, "sid2-1", "vuid2-1", "volume2-1", "", "snap2-1", nil, nil),
135+
initialSnapshots: nosnapshots,
136+
expectedSnapshots: nosnapshots,
137+
expectedEvents: noevents,
138+
errors: noerrors,
139+
expectedDeleteCalls: []deleteCall{{"sid2-1", nil, nil}},
140+
test: testSyncContent,
141+
},
131142
{
132143
name: "1-2 - successful delete with snapshot class that has empty secret parameter",
133144
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) {
222233
reactor.lock.Unlock()
223234
}),
224235
},
236+
{
237+
name: "1-9 - content will not be deleted if it is bound to a snapshot correctly, snapshot uid is specified",
238+
initialContents: newContentArray("content1-9", validSecretClass, "sid1-9", "vuid1-9", "volume1-9", "snapuid1-9", "snap1-9", nil, nil),
239+
expectedContents: newContentArray("content1-9", validSecretClass, "sid1-9", "vuid1-9", "volume1-9", "snapuid1-9", "snap1-9", nil, nil),
240+
initialSnapshots: newSnapshotArray("snap1-9", validSecretClass, "content1-9", "snapuid1-9", "claim1-9", false, nil, nil, nil),
241+
expectedSnapshots: newSnapshotArray("snap1-9", validSecretClass, "content1-9", "snapuid1-9", "claim1-9", false, nil, nil, nil),
242+
expectedEvents: noevents,
243+
initialSecrets: []*v1.Secret{secret()},
244+
errors: noerrors,
245+
test: testSyncContent,
246+
},
247+
{
248+
name: "1-10 - should delete content which is bound to a snapshot incorrectly",
249+
initialContents: newContentArray("content1-10", validSecretClass, "sid1-10", "vuid1-10", "volume1-10", "snapuid1-10-x", "snap1-10", nil, nil),
250+
expectedContents: nocontents,
251+
initialSnapshots: newSnapshotArray("snap1-10", validSecretClass, "content1-10", "snapuid1-10", "claim1-10", false, nil, nil, nil),
252+
expectedSnapshots: newSnapshotArray("snap1-10", validSecretClass, "content1-10", "snapuid1-10", "claim1-10", false, nil, nil, nil),
253+
expectedEvents: noevents,
254+
initialSecrets: []*v1.Secret{secret()},
255+
errors: noerrors,
256+
expectedDeleteCalls: []deleteCall{{"sid1-10", map[string]string{"foo": "bar"}, nil}},
257+
test: testSyncContent,
258+
},
259+
{
260+
name: "1-11 - content will not be deleted if it is bound to a snapshot correctly, snapsht uid is not specified",
261+
initialContents: newContentArray("content1-11", validSecretClass, "sid1-11", "vuid1-11", "volume1-11", "", "snap1-11", nil, nil),
262+
expectedContents: newContentArray("content1-11", validSecretClass, "sid1-11", "vuid1-11", "volume1-11", "", "snap1-11", nil, nil),
263+
initialSnapshots: newSnapshotArray("snap1-11", validSecretClass, "content1-11", "snapuid1-11", "claim1-11", false, nil, nil, nil),
264+
expectedSnapshots: newSnapshotArray("snap1-11", validSecretClass, "content1-11", "snapuid1-11", "claim1-11", false, nil, nil, nil),
265+
expectedEvents: noevents,
266+
initialSecrets: []*v1.Secret{secret()},
267+
errors: noerrors,
268+
test: testSyncContent,
269+
},
225270
}
226271
runSyncTests(t, tests, snapshotClasses)
227272
}

‎pkg/controller/snapshot_ready_test.go‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -232,6 +232,26 @@ func TestSync(t *testing.T) {
232232
errors: noerrors,
233233
test: testSyncSnapshot,
234234
},
235+
{
236+
name: "3-5 - snapshot bound to content in which the driver does not match",
237+
initialContents: newContentWithUnmatchDriverArray("content3-5", validSecretClass, "sid3-5", "vuid3-5", "volume3-5", "", "snap3-5", nil, nil),
238+
expectedContents: nocontents,
239+
initialSnapshots: newSnapshotArray("snap3-5", validSecretClass, "content3-5", "snapuid3-5", "claim3-5", false, nil, nil, nil),
240+
expectedSnapshots: newSnapshotArray("snap3-5", validSecretClass, "content3-5", "snapuid3-5", "claim3-5", false, newVolumeError("VolumeSnapshotContent is missing"), nil, nil),
241+
expectedEvents: []string{"Warning SnapshotContentMissing"},
242+
errors: noerrors,
243+
test: testSyncSnapshotError,
244+
},
245+
{
246+
name: "3-6 - snapshot bound to content in which the snapshot uid does not match",
247+
initialContents: newContentArray("content3-4", validSecretClass, "sid3-4", "vuid3-4", "volume3-4", "snapuid3-4-x", "snap3-6", nil, nil),
248+
expectedContents: newContentArray("content3-4", validSecretClass, "sid3-4", "vuid3-4", "volume3-4", "snapuid3-4-x", "snap3-6", nil, nil),
249+
initialSnapshots: newSnapshotArray("snap3-5", validSecretClass, "content3-5", "snapuid3-5", "claim3-5", false, nil, nil, nil),
250+
expectedSnapshots: newSnapshotArray("snap3-5", validSecretClass, "content3-5", "snapuid3-5", "claim3-5", false, newVolumeError("VolumeSnapshotContent is missing"), nil, nil),
251+
expectedEvents: []string{"Warning SnapshotContentMissing"},
252+
errors: noerrors,
253+
test: testSyncSnapshotError,
254+
},
235255
}
236256

237257
runSyncTests(t, tests, snapshotClasses)

0 commit comments

Comments
 (0)