From 30552679b60bf599b92da8e77355c097572b4881 Mon Sep 17 00:00:00 2001 From: Jan Safranek Date: Wed, 27 Nov 2019 14:11:06 +0100 Subject: [PATCH] UPSTREAM: 357: Fix snapshotRef checks --- pkg/controller/controller.go | 18 +++- pkg/controller/controller_test.go | 150 +++++++++++++++++++++++++----- 2 files changed, 142 insertions(+), 26 deletions(-) diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 7162a25a37..87920767fc 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -636,14 +636,26 @@ func (p *csiProvisioner) getVolumeContentSource(options controller.VolumeOptions snapContentObj, err := p.snapshotClient.VolumesnapshotV1alpha1().VolumeSnapshotContents().Get(snapshotObj.Spec.SnapshotContentName, metav1.GetOptions{}) if err != nil { - return nil, fmt.Errorf("error getting snapshot:snapshotcontent %s:%s from api server: %v", snapshotObj.Name, snapshotObj.Spec.SnapshotContentName, err) + klog.Warningf("error getting snapshotcontent %s for snapshot %s/%s from api server: %s", snapshotObj.Spec.SnapshotContentName, snapshotObj.Namespace, snapshotObj.Name, err) + return nil, fmt.Errorf("snapshot in dataSource not bound or invalid") + } + + if snapContentObj.Spec.VolumeSnapshotRef == nil { + klog.Warningf("snapshotcontent %s for snapshot %s/%s is not bound", snapshotObj.Spec.SnapshotContentName, snapshotObj.Namespace, snapshotObj.Name) + return nil, fmt.Errorf("snapshot in dataSource not bound or invalid") + } + + if snapContentObj.Spec.VolumeSnapshotRef.UID != snapshotObj.UID || snapContentObj.Spec.VolumeSnapshotRef.Namespace != snapshotObj.Namespace || snapContentObj.Spec.VolumeSnapshotRef.Name != snapshotObj.Name { + klog.Warningf("snapshotcontent %s for snapshot %s/%s is bound to a different snapshot", snapshotObj.Spec.SnapshotContentName, snapshotObj.Namespace, snapshotObj.Name) + return nil, fmt.Errorf("snapshot in dataSource not bound or invalid") } - klog.V(5).Infof("VolumeSnapshotContent %+v", snapContentObj) if snapContentObj.Spec.VolumeSnapshotSource.CSI == nil { - return nil, fmt.Errorf("error getting snapshot source from snapshot:snapshotcontent %s:%s", snapshotObj.Name, snapshotObj.Spec.SnapshotContentName) + klog.Warningf("error getting snapshot source from snapshotcontent %s for snapshot %s/%s", snapshotObj.Spec.SnapshotContentName, snapshotObj.Namespace, snapshotObj.Name) + return nil, fmt.Errorf("snapshot in dataSource not bound or invalid") } + klog.V(5).Infof("VolumeSnapshotContent %+v", snapContentObj) snapshotSource := csi.VolumeContentSource_Snapshot{ Snapshot: &csi.VolumeContentSource_SnapshotSource{ SnapshotId: snapContentObj.Spec.VolumeSnapshotSource.CSI.SnapshotHandle, diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index 4a8782a421..2b275e6e32 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -1406,7 +1406,6 @@ func newContent(name, className, snapshotHandle, volumeUID, volumeName, boundToS return &content } -// TestProvisionFromSnapshot tests create volume from snapshot func TestProvisionFromSnapshot(t *testing.T) { var apiGrp = "snapshot.storage.k8s.io" var unsupportedAPIGrp = "unsupported.group.io" @@ -1427,12 +1426,16 @@ func TestProvisionFromSnapshot(t *testing.T) { } testcases := map[string]struct { - volOpts controller.VolumeOptions - restoredVolSizeSmall bool - wrongDataSource bool - snapshotStatusReady bool - expectedPVSpec *pvSpec - expectErr bool + volOpts controller.VolumeOptions + restoredVolSizeSmall bool + wrongDataSource bool + snapshotStatusReady bool + expectedPVSpec *pvSpec + expectErr bool + expectCSICall bool + misBoundSnapshotContentUID bool + misBoundSnapshotContentNamespace bool + misBoundSnapshotContentName bool }{ "provision with volume snapshot data source": { volOpts: controller.VolumeOptions{ @@ -1444,7 +1447,7 @@ func TestProvisionFromSnapshot(t *testing.T) { Annotations: driverNameAnnotation, }, Spec: v1.PersistentVolumeClaimSpec{ - Selector: nil, + StorageClassName: &snapClassName, Resources: v1.ResourceRequirements{ Requests: v1.ResourceList{ v1.ResourceName(v1.ResourceStorage): resource.MustParse(strconv.FormatInt(requestedBytes, 10)), @@ -1461,6 +1464,7 @@ func TestProvisionFromSnapshot(t *testing.T) { Parameters: map[string]string{}, }, snapshotStatusReady: true, + expectCSICall: true, expectedPVSpec: &pvSpec{ Name: "test-testi", ReclaimPolicy: v1.PersistentVolumeReclaimDelete, @@ -1487,7 +1491,7 @@ func TestProvisionFromSnapshot(t *testing.T) { Annotations: driverNameAnnotation, }, Spec: v1.PersistentVolumeClaimSpec{ - Selector: nil, + StorageClassName: &snapClassName, Resources: v1.ResourceRequirements{ Requests: v1.ResourceList{ v1.ResourceName(v1.ResourceStorage): resource.MustParse(strconv.FormatInt(100, 10)), @@ -1516,7 +1520,7 @@ func TestProvisionFromSnapshot(t *testing.T) { Annotations: driverNameAnnotation, }, Spec: v1.PersistentVolumeClaimSpec{ - Selector: nil, + StorageClassName: &snapClassName, Resources: v1.ResourceRequirements{ Requests: v1.ResourceList{ v1.ResourceName(v1.ResourceStorage): resource.MustParse(strconv.FormatInt(requestedBytes, 10)), @@ -1544,7 +1548,7 @@ func TestProvisionFromSnapshot(t *testing.T) { Annotations: driverNameAnnotation, }, Spec: v1.PersistentVolumeClaimSpec{ - Selector: nil, + StorageClassName: &snapClassName, Resources: v1.ResourceRequirements{ Requests: v1.ResourceList{ v1.ResourceName(v1.ResourceStorage): resource.MustParse(strconv.FormatInt(requestedBytes, 10)), @@ -1572,7 +1576,7 @@ func TestProvisionFromSnapshot(t *testing.T) { Annotations: driverNameAnnotation, }, Spec: v1.PersistentVolumeClaimSpec{ - Selector: nil, + StorageClassName: &snapClassName, Resources: v1.ResourceRequirements{ Requests: v1.ResourceList{ v1.ResourceName(v1.ResourceStorage): resource.MustParse(strconv.FormatInt(requestedBytes, 10)), @@ -1600,7 +1604,7 @@ func TestProvisionFromSnapshot(t *testing.T) { Annotations: driverNameAnnotation, }, Spec: v1.PersistentVolumeClaimSpec{ - Selector: nil, + StorageClassName: &snapClassName, Resources: v1.ResourceRequirements{ Requests: v1.ResourceList{ v1.ResourceName(v1.ResourceStorage): resource.MustParse(strconv.FormatInt(100, 10)), @@ -1619,6 +1623,87 @@ func TestProvisionFromSnapshot(t *testing.T) { snapshotStatusReady: false, expectErr: true, }, + "fail snapshotContent bound to a different snapshot (by UID)": { + volOpts: controller.VolumeOptions{ + PVName: "test-name", + PVC: &v1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + UID: "testid", + }, + Spec: v1.PersistentVolumeClaimSpec{ + StorageClassName: &snapClassName, + Resources: v1.ResourceRequirements{ + Requests: v1.ResourceList{ + v1.ResourceName(v1.ResourceStorage): resource.MustParse(strconv.FormatInt(requestedBytes, 10)), + }, + }, + AccessModes: []v1.PersistentVolumeAccessMode{v1.ReadWriteOnce}, + DataSource: &v1.TypedLocalObjectReference{ + Name: snapName, + Kind: "VolumeSnapshot", + APIGroup: &apiGrp, + }, + }, + }, + }, + snapshotStatusReady: true, + expectErr: true, + misBoundSnapshotContentUID: true, + }, + "fail snapshotContent bound to a different snapshot (by namespace)": { + volOpts: controller.VolumeOptions{ + PVName: "test-name", + PVC: &v1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + UID: "testid", + }, + Spec: v1.PersistentVolumeClaimSpec{ + StorageClassName: &snapClassName, + Resources: v1.ResourceRequirements{ + Requests: v1.ResourceList{ + v1.ResourceName(v1.ResourceStorage): resource.MustParse(strconv.FormatInt(requestedBytes, 10)), + }, + }, + AccessModes: []v1.PersistentVolumeAccessMode{v1.ReadWriteOnce}, + DataSource: &v1.TypedLocalObjectReference{ + Name: snapName, + Kind: "VolumeSnapshot", + APIGroup: &apiGrp, + }, + }, + }, + }, + snapshotStatusReady: true, + expectErr: true, + misBoundSnapshotContentNamespace: true, + }, + "fail snapshotContent bound to a different snapshot (by name)": { + volOpts: controller.VolumeOptions{ + PVName: "test-name", + PVC: &v1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + UID: "testid", + }, + Spec: v1.PersistentVolumeClaimSpec{ + StorageClassName: &snapClassName, + Resources: v1.ResourceRequirements{ + Requests: v1.ResourceList{ + v1.ResourceName(v1.ResourceStorage): resource.MustParse(strconv.FormatInt(requestedBytes, 10)), + }, + }, + AccessModes: []v1.PersistentVolumeAccessMode{v1.ReadWriteOnce}, + DataSource: &v1.TypedLocalObjectReference{ + Name: snapName, + Kind: "VolumeSnapshot", + APIGroup: &apiGrp, + }, + }, + }, + }, + snapshotStatusReady: true, + expectErr: true, + misBoundSnapshotContentName: true, + }, } tmpdir := tempDir(t) @@ -1642,6 +1727,15 @@ func TestProvisionFromSnapshot(t *testing.T) { client.AddReactor("get", "volumesnapshotcontents", func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) { content := newContent("snapcontent-snapuid", snapClassName, "sid", "pv-uid", "volume", "snapuid", snapName, &requestedBytes, &timeNow) + if tc.misBoundSnapshotContentUID { + content.Spec.VolumeSnapshotRef.UID = "another-snapshot-uid" + } + if tc.misBoundSnapshotContentName { + content.Spec.VolumeSnapshotRef.Name = "another-snapshot-name" + } + if tc.misBoundSnapshotContentNamespace { + content.Spec.VolumeSnapshotRef.Namespace = "another-snapshot-namespace" + } return true, content, nil }) @@ -1661,7 +1755,15 @@ func TestProvisionFromSnapshot(t *testing.T) { // early and therefore CreateVolume is not expected to be called. // When the following if condition is met, it is a valid create volume from snapshot // operation and CreateVolume is expected to be called. - if tc.restoredVolSizeSmall == false && tc.wrongDataSource == false && tc.snapshotStatusReady { + if tc.expectCSICall { + snapshotSource := csi.VolumeContentSource_Snapshot{ + Snapshot: &csi.VolumeContentSource_SnapshotSource{ + SnapshotId: "sid", + }, + } + out.Volume.ContentSource = &csi.VolumeContentSource{ + Type: &snapshotSource, + } controllerServer.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(out, nil).Times(1) } @@ -1675,17 +1777,19 @@ func TestProvisionFromSnapshot(t *testing.T) { } if tc.expectedPVSpec != nil { - if pv.Name != tc.expectedPVSpec.Name { - t.Errorf("test %q: expected PV name: %q, got: %q", k, tc.expectedPVSpec.Name, pv.Name) - } + if pv != nil { + if pv.Name != tc.expectedPVSpec.Name { + t.Errorf("test %q: expected PV name: %q, got: %q", k, tc.expectedPVSpec.Name, pv.Name) + } - if !reflect.DeepEqual(pv.Spec.Capacity, tc.expectedPVSpec.Capacity) { - t.Errorf("test %q: expected capacity: %v, got: %v", k, tc.expectedPVSpec.Capacity, pv.Spec.Capacity) - } + if !reflect.DeepEqual(pv.Spec.Capacity, tc.expectedPVSpec.Capacity) { + t.Errorf("test %q: expected capacity: %v, got: %v", k, tc.expectedPVSpec.Capacity, pv.Spec.Capacity) + } - if tc.expectedPVSpec.CSIPVS != nil { - if !reflect.DeepEqual(pv.Spec.PersistentVolumeSource.CSI, tc.expectedPVSpec.CSIPVS) { - t.Errorf("test %q: expected PV: %v, got: %v", k, tc.expectedPVSpec.CSIPVS, pv.Spec.PersistentVolumeSource.CSI) + if tc.expectedPVSpec.CSIPVS != nil { + if !reflect.DeepEqual(pv.Spec.PersistentVolumeSource.CSI, tc.expectedPVSpec.CSIPVS) { + t.Errorf("test %q: expected PV: %v, got: %v", k, tc.expectedPVSpec.CSIPVS, pv.Spec.PersistentVolumeSource.CSI) + } } } }