Skip to content

Commit

Permalink
Fix snapshotRef checks
Browse files Browse the repository at this point in the history
  • Loading branch information
@jsafrane
jsafrane committed Oct 8, 2019
1 parent aa27082 commit 2bb7c70
Show file tree
Hide file tree
Showing 2 changed files with 143 additions and 27 deletions.
18 changes: 15 additions & 3 deletions pkg/controller/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -570,14 +570,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)
glog.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 {
glog.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 {
glog.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")
}
glog.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)
glog.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")
}

glog.V(5).Infof("VolumeSnapshotContent %+v", snapContentObj)
snapshotSource := csi.VolumeContentSource_Snapshot{
Snapshot: &csi.VolumeContentSource_SnapshotSource{
Id: snapContentObj.Spec.VolumeSnapshotSource.CSI.SnapshotHandle,
Expand Down
152 changes: 128 additions & 24 deletions pkg/controller/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import (
"github.com/kubernetes-csi/external-snapshotter/pkg/client/clientset/versioned/fake"
"github.com/kubernetes-incubator/external-storage/lib/controller"
"google.golang.org/grpc"
"k8s.io/api/core/v1"
v1 "k8s.io/api/core/v1"
storage "k8s.io/api/storage/v1beta1"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -1220,7 +1220,6 @@ func newContent(name, className, snapshotHandle, volumeUID, volumeName, boundToS
return &content
}

// TestProvisionFromSnapshot tests create volume from snapshot
func TestProvisionFromSnapshot(t *testing.T) {
var apiGrp string = "snapshot.storage.k8s.io"
var unsupportedAPIGrp string = "unsupported.group.io"
Expand All @@ -1241,12 +1240,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 @@ -1257,7 +1260,7 @@ func TestProvisionFromSnapshot(t *testing.T) {
UID: "testid",
},
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 @@ -1274,6 +1277,7 @@ func TestProvisionFromSnapshot(t *testing.T) {
Parameters: map[string]string{},
},
snapshotStatusReady: true,
expectCSICall: true,
expectedPVSpec: &pvSpec{
Name: "test-testi",
ReclaimPolicy: v1.PersistentVolumeReclaimDelete,
Expand All @@ -1299,7 +1303,7 @@ func TestProvisionFromSnapshot(t *testing.T) {
UID: "testid",
},
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 @@ -1327,7 +1331,7 @@ func TestProvisionFromSnapshot(t *testing.T) {
UID: "testid",
},
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 @@ -1354,7 +1358,7 @@ func TestProvisionFromSnapshot(t *testing.T) {
UID: "testid",
},
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 @@ -1381,7 +1385,7 @@ func TestProvisionFromSnapshot(t *testing.T) {
UID: "testid",
},
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 @@ -1408,7 +1412,7 @@ func TestProvisionFromSnapshot(t *testing.T) {
UID: "testid",
},
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 @@ -1427,6 +1431,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,
},
}

mockController, driver, identityServer, controllerServer, csiConn, err := createMockServer(t)
Expand All @@ -1448,6 +1533,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 @@ -1472,7 +1566,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{
Id: "sid",
},
}
out.Volume.ContentSource = &csi.VolumeContentSource{
Type: &snapshotSource,
}
controllerServer.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(out, nil).Times(1)
}

Expand All @@ -1486,17 +1588,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

0 comments on commit 2bb7c70

Please sign in to comment.