Skip to content

Commit c826510

Browse files
amacaskillamacaskill
committed
add create regional clone from zonal disk and improve tests
1 parent 46c7c75 commit c826510

File tree

3 files changed

+146
-49
lines changed

3 files changed

+146
-49
lines changed

‎pkg/gce-pd-csi-driver/controller.go

+20-2
Original file line numberDiff line numberDiff line change
@@ -239,10 +239,28 @@ func (gceCS *GCEControllerServer) CreateVolume(ctx context.Context, req *csi.Cre
239239
return nil, status.Error(codes.Internal, fmt.Sprintf("CreateVolume unknown get disk error when validating: %v", err))
240240
}
241241
}
242-
// Verify the zone, region, and disk type of the clone must be the same as that of the source disk.
243-
if err := gce.ValidateDiskParameters(diskFromSourceVolume, params); err != nil {
242+
243+
// Verify the replication-type, disk type, and encryption key of the clone are the same as that of the source disk.
244+
if diskFromSourceVolume.GetPDType() != params.DiskType || !kmsKeyEqual(diskFromSourceVolume.GetKMSKeyName(), params.DiskEncryptionKMSKey) {
244245
return nil, status.Errorf(codes.InvalidArgument, `CreateVolume source volume parameters do not match CreateVolumeRequest Parameters: %v`, err)
245246
}
247+
248+
// For zonal disk clones of a zonal disk, verify the zone is the same as that of the source disk.
249+
if params.ReplicationType == replicationTypeNone && (diskFromSourceVolume.LocationType() == meta.Regional || sourceVolKey.Zone != volKey.Zone) {
250+
return nil, status.Errorf(codes.InvalidArgument, `CreateVolume source volume zone %s does not match CreateVolume disk zone %s: %v`, sourceVolKey.Zone, volKey.Zone, err)
251+
}
252+
253+
if params.ReplicationType == replicationTypeNone {
254+
// For regional disk clones of a regional disk, verify the region is the same as that of the source disk.
255+
if diskFromSourceVolume.LocationType() == meta.Regional && sourceVolKey.Region != volKey.Region {
256+
return nil, status.Errorf(codes.InvalidArgument, `CreateVolume source volume region %s does not match CreateVolume disk region %s: %v`, sourceVolKey.Region, volKey.Region, err)
257+
}
258+
// For regional disk clones of a zonal disk, verify one of the replica zones matches the source disk zone.
259+
if diskFromSourceVolume.LocationType() == meta.Zonal && !contains(zones, sourceVolKey.Zone) {
260+
return nil, status.Errorf(codes.InvalidArgument, `CreateVolume source volume zone %s does not match one of the replica zones %v of the CreateVolume disk region %s: %v`, sourceVolKey.Zone, zones, volKey.Region, err)
261+
}
262+
}
263+
246264
// Verify the source disk is ready.
247265
ready, err := isDiskReady(diskFromSourceVolume)
248266
if err != nil {

‎pkg/gce-pd-csi-driver/controller_test.go

+95-46
Original file line numberDiff line numberDiff line change
@@ -898,23 +898,28 @@ func TestCreateVolumeWithVolumeSourceFromSnapshot(t *testing.T) {
898898
}
899899

900900
func TestCreateVolumeWithVolumeSourceFromVolume(t *testing.T) {
901+
901902
testSourceVolumeName := "test-volume-source-name"
902903
testZonalVolumeSourceID := fmt.Sprintf("projects/%s/zones/%s/disks/%s", project, zone, testSourceVolumeName)
903904
testRegionalVolumeSourceID := fmt.Sprintf("projects/%s/regions/%s/disks/%s", project, region, testSourceVolumeName)
904-
testVolumeSourceIDDifferentZone := fmt.Sprintf("projects/%s/zones/%s/disks/%s", project, "different-zone", testSourceVolumeName)
905+
testSecondZonalVolumeSourceID := fmt.Sprintf("projects/%s/zones/%s/disks/%s", project, "different-zone1", testSourceVolumeName)
906+
zonalParams := map[string]string{
907+
common.ParameterKeyType: "test-type", common.ParameterKeyReplicationType: replicationTypeNone,
908+
}
909+
regionalParams := map[string]string{
910+
common.ParameterKeyType: "test-type", common.ParameterKeyReplicationType: replicationTypeRegionalPD,
911+
}
905912
topology := &csi.TopologyRequirement{
906913
Requisite: []*csi.Topology{
907914
{
908-
Segments: map[string]string{common.TopologyKeyZone: region + "-b"},
915+
Segments: map[string]string{common.TopologyKeyZone: zone},
909916
},
910917
{
911-
Segments: map[string]string{common.TopologyKeyZone: region + "-c"},
918+
Segments: map[string]string{common.TopologyKeyZone: secondZone},
912919
},
913920
},
914921
}
915-
regionalParams := map[string]string{
916-
common.ParameterKeyType: "test-type", common.ParameterKeyReplicationType: "regional-pd",
917-
}
922+
918923
// Define test cases
919924
testCases := []struct {
920925
name string
@@ -923,67 +928,115 @@ func TestCreateVolumeWithVolumeSourceFromVolume(t *testing.T) {
923928
sourceVolumeID string
924929
reqParameters map[string]string
925930
sourceReqParameters map[string]string
926-
topology *csi.TopologyRequirement
931+
sourceTopology *csi.TopologyRequirement
932+
requestTopology *csi.TopologyRequirement
927933
}{
928934
{
929-
name: "success with data source of zonal volume type",
935+
name: "success zonal disk clone of zonal source disk",
930936
volumeOnCloud: true,
931937
sourceVolumeID: testZonalVolumeSourceID,
932-
reqParameters: stdParams,
933-
sourceReqParameters: stdParams,
938+
reqParameters: zonalParams,
939+
sourceReqParameters: zonalParams,
940+
sourceTopology: topology,
941+
requestTopology: topology,
934942
},
935943
{
936-
name: "success with data source of regional volume type",
944+
name: "success regional disk clone of regional source disk",
937945
volumeOnCloud: true,
938946
sourceVolumeID: testRegionalVolumeSourceID,
939947
reqParameters: regionalParams,
940948
sourceReqParameters: regionalParams,
941-
topology: topology,
949+
sourceTopology: topology,
950+
requestTopology: topology,
942951
},
943952
{
944-
name: "fail with with data source of replication-type different from CreateVolumeRequest",
953+
name: "success regional disk clone of zonal data source",
945954
volumeOnCloud: true,
946-
expErrCode: codes.InvalidArgument,
947-
sourceVolumeID: testZonalVolumeSourceID,
948-
reqParameters: stdParams,
949-
sourceReqParameters: regionalParams,
950-
topology: topology,
951-
},
952-
{
953-
name: "fail with data source of zonal volume type that doesn't exist",
954-
volumeOnCloud: false,
955-
expErrCode: codes.NotFound,
956955
sourceVolumeID: testZonalVolumeSourceID,
957-
reqParameters: stdParams,
958-
sourceReqParameters: stdParams,
956+
reqParameters: regionalParams,
957+
sourceReqParameters: zonalParams,
958+
sourceTopology: topology,
959+
requestTopology: topology,
959960
},
960961
{
961-
name: "fail with data source of zonal volume type with invalid volume id format",
962-
volumeOnCloud: false,
962+
name: "fail regional disk clone with no matching replica zone of zonal data source",
963+
volumeOnCloud: true,
963964
expErrCode: codes.InvalidArgument,
964-
sourceVolumeID: testZonalVolumeSourceID + "invalid/format",
965-
reqParameters: stdParams,
966-
sourceReqParameters: stdParams,
965+
sourceVolumeID: testZonalVolumeSourceID,
966+
reqParameters: regionalParams,
967+
sourceReqParameters: zonalParams,
968+
sourceTopology: topology,
969+
requestTopology: &csi.TopologyRequirement{
970+
Requisite: []*csi.Topology{
971+
{
972+
Segments: map[string]string{common.TopologyKeyZone: "different-zone1"},
973+
},
974+
{
975+
Segments: map[string]string{common.TopologyKeyZone: "different-zone2"},
976+
},
977+
},
978+
},
967979
},
968980
{
969-
name: "fail with data source of zonal volume type with invalid disk parameters",
981+
name: "fail zonal disk clone with different disk type",
970982
volumeOnCloud: true,
971983
expErrCode: codes.InvalidArgument,
972-
sourceVolumeID: testVolumeSourceIDDifferentZone,
973-
reqParameters: stdParams,
984+
sourceVolumeID: testZonalVolumeSourceID,
985+
reqParameters: zonalParams,
974986
sourceReqParameters: map[string]string{
975987
common.ParameterKeyType: "different-type",
976988
},
989+
sourceTopology: topology,
990+
requestTopology: topology,
991+
},
992+
{
993+
name: "fail zonal disk clone with different zone",
994+
volumeOnCloud: true,
995+
expErrCode: codes.InvalidArgument,
996+
sourceVolumeID: testSecondZonalVolumeSourceID,
997+
reqParameters: zonalParams,
998+
sourceReqParameters: zonalParams,
999+
sourceTopology: &csi.TopologyRequirement{
1000+
Requisite: []*csi.Topology{
1001+
{
1002+
Segments: map[string]string{common.TopologyKeyZone: "different-zone1"},
1003+
},
1004+
{
1005+
Segments: map[string]string{common.TopologyKeyZone: "different-zone2"},
1006+
},
1007+
},
1008+
},
1009+
requestTopology: topology,
9771010
},
9781011
{
979-
name: "fail with data source of zonal volume type with invalid replication type",
1012+
name: "fail zonal disk clone of regional data source",
9801013
volumeOnCloud: true,
9811014
expErrCode: codes.InvalidArgument,
1015+
sourceVolumeID: testRegionalVolumeSourceID,
1016+
reqParameters: zonalParams,
1017+
sourceReqParameters: regionalParams,
1018+
sourceTopology: topology,
1019+
requestTopology: topology,
1020+
},
1021+
1022+
{
1023+
name: "fail zonal source disk does not exist",
1024+
volumeOnCloud: false,
1025+
expErrCode: codes.NotFound,
9821026
sourceVolumeID: testZonalVolumeSourceID,
983-
reqParameters: regionalParams,
1027+
reqParameters: stdParams,
9841028
sourceReqParameters: stdParams,
1029+
requestTopology: topology,
9851030
},
986-
}
1031+
{
1032+
name: "fail invalid source disk volume id format",
1033+
volumeOnCloud: false,
1034+
expErrCode: codes.InvalidArgument,
1035+
sourceVolumeID: testZonalVolumeSourceID + "/invalid/format",
1036+
reqParameters: stdParams,
1037+
sourceReqParameters: stdParams,
1038+
requestTopology: topology,
1039+
}}
9871040

9881041
for _, tc := range testCases {
9891042
t.Logf("test case: %s", tc.name)
@@ -1001,18 +1054,15 @@ func TestCreateVolumeWithVolumeSourceFromVolume(t *testing.T) {
10011054
},
10021055
},
10031056
},
1057+
AccessibilityRequirements: tc.requestTopology,
10041058
}
10051059

10061060
sourceVolumeRequest := &csi.CreateVolumeRequest{
1007-
Name: testSourceVolumeName,
1008-
CapacityRange: stdCapRange,
1009-
VolumeCapabilities: stdVolCaps,
1010-
Parameters: tc.sourceReqParameters,
1011-
}
1012-
1013-
if tc.topology != nil {
1014-
// req.AccessibilityRequirements = tc.topology
1015-
sourceVolumeRequest.AccessibilityRequirements = tc.topology
1061+
Name: testSourceVolumeName,
1062+
CapacityRange: stdCapRange,
1063+
VolumeCapabilities: stdVolCaps,
1064+
Parameters: tc.sourceReqParameters,
1065+
AccessibilityRequirements: tc.sourceTopology,
10161066
}
10171067

10181068
if tc.volumeOnCloud {
@@ -1045,7 +1095,6 @@ func TestCreateVolumeWithVolumeSourceFromVolume(t *testing.T) {
10451095

10461096
// Make sure the response has the source volume.
10471097
sourceVolume := resp.GetVolume()
1048-
t.Logf("response has source volume: %v ", sourceVolume)
10491098
if sourceVolume.ContentSource == nil || sourceVolume.ContentSource.Type == nil ||
10501099
sourceVolume.ContentSource.GetVolume() == nil || sourceVolume.ContentSource.GetVolume().VolumeId == "" {
10511100
t.Fatalf("Expected volume content source to have volume ID, got none")

‎pkg/gce-pd-csi-driver/utils.go

+31-1
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package gceGCEDriver
1919
import (
2020
"errors"
2121
"fmt"
22+
"strings"
2223

2324
"context"
2425

@@ -28,7 +29,8 @@ import (
2829
)
2930

3031
const (
31-
fsTypeXFS = "xfs"
32+
fsTypeXFS = "xfs"
33+
cryptoKeyVerDelimiter = "/cryptoKeyVersions"
3234
)
3335

3436
var ProbeCSIFullMethod = "/csi.v1.Identity/Probe"
@@ -199,3 +201,31 @@ func collectMountOptions(fsType string, mntFlags []string) []string {
199201
}
200202
return options
201203
}
204+
205+
// kmsKeyEqual returns true if fetchedKMSKey and storageClassKMSKey refer to the same key.
206+
// fetchedKMSKey - key returned by the server
207+
// example: projects/{0}/locations/{1}/keyRings/{2}/cryptoKeys/{3}/cryptoKeyVersions/{4}
208+
// storageClassKMSKey - key as provided by the client
209+
// example: projects/{0}/locations/{1}/keyRings/{2}/cryptoKeys/{3}
210+
// cryptoKeyVersions should be disregarded if the rest of the key is identical.
211+
func kmsKeyEqual(fetchedKMSKey, storageClassKMSKey string) bool {
212+
return removeCryptoKeyVersion(fetchedKMSKey) == removeCryptoKeyVersion(storageClassKMSKey)
213+
}
214+
215+
func removeCryptoKeyVersion(kmsKey string) string {
216+
i := strings.LastIndex(kmsKey, cryptoKeyVerDelimiter)
217+
if i > 0 {
218+
return kmsKey[:i]
219+
}
220+
return kmsKey
221+
}
222+
223+
func contains(s []string, str string) bool {
224+
for _, v := range s {
225+
if v == str {
226+
return true
227+
}
228+
}
229+
230+
return false
231+
}

0 commit comments

Comments
 (0)