Skip to content

Commit 9c74cce

Browse files
julianKatzjulianKatz
committed
Only add disk support topologies if StorageClass parameter is true
1 parent 68021f0 commit 9c74cce

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
@@ -1350,13 +1350,43 @@ func TestCreateVolumeArguments(t *testing.T) {
13501350
},
13511351
// Disk Topology Enabled tests
13521352
{
1353-
name: "success with disk topology enabled",
1353+
name: "success with disk topology enabled and StorageClass useAllowedDiskTopology false",
13541354
req: &csi.CreateVolumeRequest{
13551355
Name: "test-name",
13561356
CapacityRange: stdCapRange,
13571357
VolumeCapabilities: stdVolCaps,
1358-
Parameters: stdParams,
1358+
Parameters: mergeParameters(
1359+
stdParams,
1360+
map[string]string{common.ParameterKeyUseAllowedDiskTopology: "false"},
1361+
),
13591362
},
1363+
enableDiskTopology: true,
1364+
expVol: &csi.Volume{
1365+
CapacityBytes: common.GbToBytes(20),
1366+
VolumeId: testVolumeID,
1367+
VolumeContext: nil,
1368+
AccessibleTopology: []*csi.Topology{
1369+
{
1370+
Segments: map[string]string{
1371+
common.TopologyKeyZone: zone,
1372+
// Disk not type not included since useAllowedDiskTopology is false
1373+
},
1374+
},
1375+
},
1376+
},
1377+
},
1378+
{
1379+
name: "success with disk topology enabled and StorageClass useAllowedDiskTopology true",
1380+
req: &csi.CreateVolumeRequest{
1381+
Name: "test-name",
1382+
CapacityRange: stdCapRange,
1383+
VolumeCapabilities: stdVolCaps,
1384+
Parameters: mergeParameters(
1385+
stdParams,
1386+
map[string]string{common.ParameterKeyUseAllowedDiskTopology: "true"},
1387+
),
1388+
},
1389+
enableDiskTopology: true,
13601390
expVol: &csi.Volume{
13611391
CapacityBytes: common.GbToBytes(20),
13621392
VolumeId: testVolumeID,
@@ -1371,46 +1401,46 @@ func TestCreateVolumeArguments(t *testing.T) {
13711401
},
13721402
},
13731403
},
1374-
enableDiskTopology: true,
13751404
},
13761405
}
13771406

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

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

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

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

@@ -5815,3 +5845,14 @@ func googleapiErrContainsReason(err *googleapi.Error, reason string) bool {
58155845
}
58165846
return false
58175847
}
5848+
5849+
func mergeParameters(a map[string]string, b map[string]string) map[string]string {
5850+
merged := make(map[string]string)
5851+
for k, v := range a {
5852+
merged[k] = v
5853+
}
5854+
for k, v := range b {
5855+
merged[k] = v
5856+
}
5857+
return merged
5858+
}

0 commit comments

Comments
 (0)