Skip to content

Commit 1aceb4b

Browse files
julianKatzjulianKatz
committed
Only add disk support topologies if StorageClass parameter is true
1 parent 55839ca commit 1aceb4b

File tree

5 files changed

+130
-40
lines changed

5 files changed

+130
-40
lines changed

‎cmd/gce-pd-csi-driver/main.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ var (
9494

9595
extraTagsStr = flag.String("extra-tags", "", "Extra tags to attach to each Compute Disk, Image, Snapshot created. It is a comma separated list of parent id, key and value like '<parent_id1>/<tag_key1>/<tag_value1>,...,<parent_idN>/<tag_keyN>/<tag_valueN>'. parent_id is the Organization or the Project ID or Project name where the tag key and the tag value resources exist. A maximum of 50 tags bindings is allowed for a resource. See https://cloud.google.com/resource-manager/docs/tags/tags-overview, https://cloud.google.com/resource-manager/docs/tags/tags-creating-and-managing for details")
9696

97-
diskTopology = flag.Bool("disk-topology", false, "If set to true, the driver will add a disk-type.gke.io/[some-disk-type] topology label to the Topologies returned in CreateVolumeResponse.")
97+
diskTopology = flag.Bool("disk-topology", false, "If set to true, the driver will add a disk-type.gke.io/[disk-type] topology label when the StorageClass has the use-allowed-disk-topology parameter set to true. That topology label is included in the Topologies returned in CreateVolumeResponse. This flag is disabled by default.")
9898

9999
version string
100100
)

‎pkg/common/parameters.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ const (
3636
ParameterAvailabilityClass = "availability-class"
3737
ParameterKeyEnableConfidentialCompute = "enable-confidential-storage"
3838
ParameterKeyStoragePools = "storage-pools"
39+
ParameterKeyUseAllowedDiskTopology = "use-allowed-disk-topology"
3940

4041
// Parameters for Data Cache
4142
ParameterKeyDataCacheSize = "data-cache-size"
@@ -132,6 +133,9 @@ type DiskParameters struct {
132133
// Values: READ_WRITE_SINGLE, READ_ONLY_MANY, READ_WRITE_MANY
133134
// Default: READ_WRITE_SINGLE
134135
AccessMode string
136+
// Values {}
137+
// Default: false
138+
UseAllowedDiskTopology bool
135139
}
136140

137141
func (dp *DiskParameters) IsRegional() bool {
@@ -160,6 +164,7 @@ type ParameterProcessor struct {
160164
EnableStoragePools bool
161165
EnableMultiZone bool
162166
EnableHdHA bool
167+
EnableDiskTopology bool
163168
}
164169

165170
type ModifyVolumeParameters struct {
@@ -319,6 +324,17 @@ func (pp *ParameterProcessor) ExtractAndDefaultParameters(parameters map[string]
319324
if v != "" {
320325
p.AccessMode = v
321326
}
327+
case ParameterKeyUseAllowedDiskTopology:
328+
if !pp.EnableDiskTopology {
329+
return p, d, fmt.Errorf("parameters contains invalid option %q when disk topology is not enabled", ParameterKeyUseAllowedDiskTopology)
330+
}
331+
332+
paramUseAllowedDiskTopology, err := ConvertStringToBool(v)
333+
if err != nil {
334+
return p, d, fmt.Errorf("failed to convert %s parameter with value %q to bool: %w", ParameterKeyUseAllowedDiskTopology, v, err)
335+
}
336+
337+
p.UseAllowedDiskTopology = paramUseAllowedDiskTopology
322338
default:
323339
return p, d, fmt.Errorf("parameters contains invalid option %q", k)
324340
}

‎pkg/common/parameters_test.go

Lines changed: 34 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ func TestExtractAndDefaultParameters(t *testing.T) {
3232
enableDataCache bool
3333
enableMultiZone bool
3434
enableHdHA bool
35+
enableDiskTopology bool
3536
extraTags map[string]string
3637
expectParams DiskParameters
3738
expectDataCacheParams DataCacheParameters
@@ -42,12 +43,13 @@ func TestExtractAndDefaultParameters(t *testing.T) {
4243
parameters: map[string]string{},
4344
labels: map[string]string{},
4445
expectParams: DiskParameters{
45-
DiskType: "pd-standard",
46-
ReplicationType: "none",
47-
DiskEncryptionKMSKey: "",
48-
Tags: map[string]string{},
49-
Labels: map[string]string{},
50-
ResourceTags: map[string]string{},
46+
DiskType: "pd-standard",
47+
ReplicationType: "none",
48+
DiskEncryptionKMSKey: "",
49+
Tags: map[string]string{},
50+
Labels: map[string]string{},
51+
ResourceTags: map[string]string{},
52+
UseAllowedDiskTopology: false,
5153
},
5254
},
5355
{
@@ -464,6 +466,31 @@ func TestExtractAndDefaultParameters(t *testing.T) {
464466
Labels: map[string]string{},
465467
},
466468
},
469+
{
470+
name: "useAllowedDiskTopology specified, disk topology feature disabled",
471+
parameters: map[string]string{ParameterKeyUseAllowedDiskTopology: "foo-bar"},
472+
expectErr: true,
473+
},
474+
{
475+
name: "useAllowedDiskTopology specified, wrong type",
476+
parameters: map[string]string{ParameterKeyUseAllowedDiskTopology: "123"},
477+
enableDiskTopology: true,
478+
expectErr: true,
479+
},
480+
{
481+
name: "useAllowedDiskTopology specified as valid true boolean",
482+
parameters: map[string]string{ParameterKeyUseAllowedDiskTopology: "true"},
483+
enableDiskTopology: true,
484+
expectParams: DiskParameters{
485+
DiskType: "pd-standard",
486+
ReplicationType: "none",
487+
DiskEncryptionKMSKey: "",
488+
Tags: map[string]string{},
489+
Labels: map[string]string{},
490+
ResourceTags: map[string]string{},
491+
UseAllowedDiskTopology: true,
492+
},
493+
},
467494
}
468495

469496
for _, tc := range tests {
@@ -473,6 +500,7 @@ func TestExtractAndDefaultParameters(t *testing.T) {
473500
EnableStoragePools: tc.enableStoragePools,
474501
EnableMultiZone: tc.enableMultiZone,
475502
EnableHdHA: tc.enableHdHA,
503+
EnableDiskTopology: tc.enableDiskTopology,
476504
}
477505
p, d, err := pp.ExtractAndDefaultParameters(tc.parameters, tc.labels, tc.enableDataCache, tc.extraTags)
478506
if gotErr := err != nil; gotErr != tc.expectErr {

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

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1339,6 +1339,7 @@ func (gceCS *GCEControllerServer) parameterProcessor() *common.ParameterProcesso
13391339
EnableStoragePools: gceCS.enableStoragePools,
13401340
EnableMultiZone: gceCS.multiZoneVolumeHandleConfig.Enable,
13411341
EnableHdHA: gceCS.enableHdHA,
1342+
EnableDiskTopology: gceCS.EnableDiskTopology,
13421343
}
13431344
}
13441345

@@ -2418,7 +2419,11 @@ func (gceCS *GCEControllerServer) generateCreateVolumeResponseWithVolumeId(disk
24182419
},
24192420
}
24202421

2421-
if gceCS.EnableDiskTopology {
2422+
// The addition of the disk type label requires both the feature to be
2423+
// enabled on the PDCSI binary via the `--disk-topology=true` flag AND
2424+
// the StorageClass to have the `use-allowed-disk-topology` parameter
2425+
// set to `true`.
2426+
if gceCS.EnableDiskTopology && params.UseAllowedDiskTopology {
24222427
top.Segments[common.DiskTypeLabelKey(params.DiskType)] = "true"
24232428
}
24242429

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

Lines changed: 73 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -1349,13 +1349,43 @@ func TestCreateVolumeArguments(t *testing.T) {
13491349
},
13501350
// Disk Topology Enabled tests
13511351
{
1352-
name: "success with disk topology enabled",
1352+
name: "success with disk topology enabled and StorageClass useAllowedDiskTopology false",
13531353
req: &csi.CreateVolumeRequest{
13541354
Name: "test-name",
13551355
CapacityRange: stdCapRange,
13561356
VolumeCapabilities: stdVolCaps,
1357-
Parameters: stdParams,
1357+
Parameters: mergeParameters(
1358+
stdParams,
1359+
map[string]string{common.ParameterKeyUseAllowedDiskTopology: "false"},
1360+
),
13581361
},
1362+
enableDiskTopology: true,
1363+
expVol: &csi.Volume{
1364+
CapacityBytes: common.GbToBytes(20),
1365+
VolumeId: testVolumeID,
1366+
VolumeContext: nil,
1367+
AccessibleTopology: []*csi.Topology{
1368+
{
1369+
Segments: map[string]string{
1370+
common.TopologyKeyZone: zone,
1371+
// Disk not type not included since useAllowedDiskTopology is false
1372+
},
1373+
},
1374+
},
1375+
},
1376+
},
1377+
{
1378+
name: "success with disk topology enabled and StorageClass useAllowedDiskTopology true",
1379+
req: &csi.CreateVolumeRequest{
1380+
Name: "test-name",
1381+
CapacityRange: stdCapRange,
1382+
VolumeCapabilities: stdVolCaps,
1383+
Parameters: mergeParameters(
1384+
stdParams,
1385+
map[string]string{common.ParameterKeyUseAllowedDiskTopology: "true"},
1386+
),
1387+
},
1388+
enableDiskTopology: true,
13591389
expVol: &csi.Volume{
13601390
CapacityBytes: common.GbToBytes(20),
13611391
VolumeId: testVolumeID,
@@ -1370,46 +1400,46 @@ func TestCreateVolumeArguments(t *testing.T) {
13701400
},
13711401
},
13721402
},
1373-
enableDiskTopology: true,
13741403
},
13751404
}
13761405

13771406
// Run test cases
13781407
for _, tc := range testCases {
1379-
t.Logf("test case: %s", tc.name)
1380-
1381-
// Setup new driver each time so no interference
1382-
args := &GCEControllerServerArgs{EnableDiskTopology: tc.enableDiskTopology}
1383-
gceDriver := initGCEDriver(t, nil, args)
1384-
gceDriver.cs.enableStoragePools = tc.enableStoragePools
1408+
t.Run(tc.name, func(t *testing.T) {
1409+
// Setup new driver each time so no interference
1410+
args := &GCEControllerServerArgs{EnableDiskTopology: tc.enableDiskTopology}
1411+
gceDriver := initGCEDriver(t, nil, args)
1412+
gceDriver.cs.enableStoragePools = tc.enableStoragePools
13851413

1386-
// Start Test
1387-
resp, err := gceDriver.cs.CreateVolume(context.Background(), tc.req)
1388-
if err != nil {
1389-
serverError, ok := status.FromError(err)
1390-
if !ok {
1391-
t.Fatalf("Could not get error status code from err: %v", serverError)
1414+
// Start Test
1415+
resp, err := gceDriver.cs.CreateVolume(context.Background(), tc.req)
1416+
if err != nil {
1417+
serverError, ok := status.FromError(err)
1418+
if !ok {
1419+
t.Fatalf("Could not get error status code from err: %v", serverError)
1420+
}
1421+
if serverError.Code() != tc.expErrCode {
1422+
t.Fatalf("Expected error code: %v, got: %v. err : %v", tc.expErrCode, serverError.Code(), err)
1423+
}
1424+
return
13921425
}
1393-
if serverError.Code() != tc.expErrCode {
1394-
t.Fatalf("Expected error code: %v, got: %v. err : %v", tc.expErrCode, serverError.Code(), err)
1426+
1427+
t.Logf("ErrorCode: %v", err)
1428+
if tc.expErrCode != codes.OK {
1429+
t.Fatalf("Expected error: %v, got no error", tc.expErrCode)
13951430
}
1396-
continue
1397-
}
1398-
t.Logf("ErroCode: %v", err)
1399-
if tc.expErrCode != codes.OK {
1400-
t.Fatalf("Expected error: %v, got no error", tc.expErrCode)
1401-
}
14021431

1403-
// Make sure responses match
1404-
vol := resp.GetVolume()
1405-
if vol == nil {
1406-
// If one is nil but not both
1407-
t.Fatalf("Expected volume %v, got nil volume", tc.expVol)
1408-
}
1432+
// Make sure responses match
1433+
vol := resp.GetVolume()
1434+
if vol == nil {
1435+
// If one is nil but not both
1436+
t.Fatalf("Expected volume %v, got nil volume", tc.expVol)
1437+
}
14091438

1410-
if diff := cmp.Diff(vol, tc.expVol, protocmp.Transform()); diff != "" {
1411-
t.Errorf("unexpected diff (-vol, +expVol): \n%s", diff)
1412-
}
1439+
if diff := cmp.Diff(vol, tc.expVol, protocmp.Transform()); diff != "" {
1440+
t.Errorf("unexpected diff (-vol, +expVol): \n%s", diff)
1441+
}
1442+
})
14131443
}
14141444
}
14151445

@@ -5818,3 +5848,14 @@ func googleapiErrContainsReason(err *googleapi.Error, reason string) bool {
58185848
}
58195849
return false
58205850
}
5851+
5852+
func mergeParameters(a map[string]string, b map[string]string) map[string]string {
5853+
merged := make(map[string]string)
5854+
for k, v := range a {
5855+
merged[k] = v
5856+
}
5857+
for k, v := range b {
5858+
merged[k] = v
5859+
}
5860+
return merged
5861+
}

0 commit comments

Comments
 (0)