Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug 1774304: UPSTREAM: 357: 4.2: Fix snapshotRef checks #23

Merged
merged 1 commit into from
Dec 2, 2019
Merged

Bug 1774304: UPSTREAM: 357: 4.2: Fix snapshotRef checks #23

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions pkg/controller/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
150 changes: 127 additions & 23 deletions pkg/controller/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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{
Expand All @@ -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)),
Expand All @@ -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,
Expand All @@ -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)),
Expand Down Expand Up @@ -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)),
Expand Down Expand Up @@ -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)),
Expand Down Expand Up @@ -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)),
Expand Down Expand Up @@ -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)),
Expand All @@ -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)
Expand All @@ -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
})

Expand All @@ -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)
}

Expand All @@ -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)
}
}
}
}
Expand Down