Address pr feedbacks

Author: sunnylovestiramisu

README.md

@@ -64,7 +64,7 @@ See Github [Issues](https://github.com/kubernetes-sigs/gcp-compute-persistent-di
 | replication-type            | `none` OR `regional-pd`   | `none`        | Replication type allows you to choose between Zonal Persistent Disks or Regional Persistent Disks  |
 | disk-encryption-kms-key     | Fully qualified resource identifier for the key to use to encrypt new disks. | Empty string. | Encrypt disk using Customer Managed Encryption Key (CMEK). See [GKE Docs](https://cloud.google.com/kubernetes-engine/docs/how-to/using-cmek#create_a_cmek_protected_attached_disk) for details. |
 | labels                      | `key1=value1,key2=value2` |               | Labels allow you to assign custom [GCE Disk labels](https://cloud.google.com/compute/docs/labeling-resources). |
-| provisioned-iops-on-create  | string (int64 format). Values must be between 10,000 and 120,000 |               | Indicates how many IOPS to provision for the disk. See the [Extreme persistent disk documentation](https://cloud.google.com/compute/docs/disks/extreme-persistent-disk). |
+| provisioned-iops-on-create  | string (int64 format). Values typically between 10,000 and 120,000 |               | Indicates how many IOPS to provision for the disk. See the [Extreme persistent disk documentation](https://cloud.google.com/compute/docs/disks/extreme-persistent-disk) for details, including valid ranges for IOPS. |
 
 
 ### Topology

pkg/common/parameters.go

@@ -78,7 +78,7 @@ type DiskParameters struct {
 	Labels map[string]string
 	// Values: {string}
 	// Default: ""
-	ProvisionedIOPSOnCreate string
+	ProvisionedIOPSOnCreate int64
 }
 
 // SnapshotParameters contains normalized and defaulted parameters for snapshots
@@ -96,12 +96,11 @@ type SnapshotParameters struct {
 // parameters, any matching extraVolumeLabels will be overridden.
 func ExtractAndDefaultParameters(parameters map[string]string, driverName string, extraVolumeLabels map[string]string) (DiskParameters, error) {
 	p := DiskParameters{
-		DiskType:                "pd-standard",           // Default
-		ReplicationType:         replicationTypeNone,     // Default
-		DiskEncryptionKMSKey:    "",                      // Default
-		Tags:                    make(map[string]string), // Default
-		Labels:                  make(map[string]string), // Default
-		ProvisionedIOPSOnCreate: "",                      // Default
+		DiskType:             "pd-standard",           // Default
+		ReplicationType:      replicationTypeNone,     // Default
+		DiskEncryptionKMSKey: "",                      // Default
+		Tags:                 make(map[string]string), // Default
+		Labels:               make(map[string]string), // Default
 	}
 
 	for k, v := range extraVolumeLabels {
@@ -141,7 +140,11 @@ func ExtractAndDefaultParameters(parameters map[string]string, driverName string
 				p.Labels[labelKey] = labelValue
 			}
 		case ParameterKeyProvisionedIOPSOnCreate:
-			p.ProvisionedIOPSOnCreate = v
+			paramProvisionedIOPSOnCreate, err := ConvertStringToInt64(v)
+			if err != nil {
+				return p, fmt.Errorf("parameters contain invalid provisionedIOPSOnCreate parameter: %w", err)
+			}
+			p.ProvisionedIOPSOnCreate = paramProvisionedIOPSOnCreate
 		default:
 			return p, fmt.Errorf("parameters contains invalid option %q", k)
 		}

pkg/common/parameters_test.go

@@ -87,7 +87,7 @@ func TestExtractAndDefaultParameters(t *testing.T) {
 					"key1": "value1",
 					"key2": "value2",
 				},
-				ProvisionedIOPSOnCreate: "10000",
+				ProvisionedIOPSOnCreate: 10000,
 			},
 		},
 		{

pkg/common/utils.go

@@ -19,6 +19,7 @@ package common
 import (
 	"fmt"
 	"regexp"
+	"strconv"
 	"strings"
 
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta"
@@ -248,3 +249,12 @@ func ValidateSnapshotType(snapshotType string) error {
 		return fmt.Errorf("invalid snapshot type %s", snapshotType)
 	}
 }
+
+// ConvertStringToInt64 converts a string to int64
+func ConvertStringToInt64(str string) (int64, error) {
+	int64Value, err := strconv.ParseInt(str, 10, 64)
+	if err != nil {
+		return -1, fmt.Errorf("Int64 value %s is invalid", str)
+	}
+	return int64Value, nil
+}

pkg/common/utils_test.go

@@ -577,3 +577,39 @@ func TestSnapshotStorageLocations(t *testing.T) {
 		})
 	}
 }
+
+func TestConvertStringToInt64(t *testing.T) {
+	tests := []struct {
+		desc        string
+		inputStr    string
+		expInt64    int64
+		expectError bool
+	}{
+		{
+			"valid string",
+			"10000",
+			10000,
+			false,
+		},
+		{
+			"invalid string",
+			"ew%65",
+			10000,
+			true,
+		},
+	}
+	for _, tc := range tests {
+		t.Run(tc.desc, func(t *testing.T) {
+			actualInt64, err := ConvertStringToInt64(tc.inputStr)
+			if err != nil && !tc.expectError {
+				t.Errorf("Got error %v converting string to int64 %s; expect no error", err, tc.inputStr)
+			}
+			if err == nil && tc.expectError {
+				t.Errorf("Got no error converting string to int64 %s; expect an error", tc.inputStr)
+			}
+			if err == nil && actualInt64 != tc.expInt64 {
+				t.Errorf("Got %d for converting string to int64; expect %d", actualInt64, tc.expInt64)
+			}
+		})
+	}
+}

pkg/gce-cloud-provider/compute/fake-gce.go

@@ -196,13 +196,14 @@ func (cloud *FakeCloudProvider) InsertDisk(ctx context.Context, project string,
 	}
 
 	computeDisk := &computev1.Disk{
-		Name:         volKey.Name,
-		SizeGb:       common.BytesToGbRoundUp(capBytes),
-		Description:  "Disk created by GCE-PD CSI Driver",
-		Type:         cloud.GetDiskTypeURI(project, volKey, params.DiskType),
-		SourceDiskId: volumeContentSourceVolumeID,
-		Status:       cloud.mockDiskStatus,
-		Labels:       params.Labels,
+		Name:            volKey.Name,
+		SizeGb:          common.BytesToGbRoundUp(capBytes),
+		Description:     "Disk created by GCE-PD CSI Driver",
+		Type:            cloud.GetDiskTypeURI(project, volKey, params.DiskType),
+		SourceDiskId:    volumeContentSourceVolumeID,
+		Status:          cloud.mockDiskStatus,
+		Labels:          params.Labels,
+		ProvisionedIops: params.ProvisionedIOPSOnCreate,
 	}
 
 	if snapshotID != "" {

pkg/gce-cloud-provider/compute/gce-compute.go

@@ -429,11 +429,12 @@ func (cloud *CloudProvider) insertRegionalDisk(
 	}
 
 	diskToCreate := &computev1.Disk{
-		Name:        volKey.Name,
-		SizeGb:      common.BytesToGbRoundUp(capBytes),
-		Description: description,
-		Type:        cloud.GetDiskTypeURI(cloud.project, volKey, params.DiskType),
-		Labels:      params.Labels,
+		Name:            volKey.Name,
+		SizeGb:          common.BytesToGbRoundUp(capBytes),
+		Description:     description,
+		Type:            cloud.GetDiskTypeURI(cloud.project, volKey, params.DiskType),
+		Labels:          params.Labels,
+		ProvisionedIops: params.ProvisionedIOPSOnCreate,
 	}
 	if snapshotID != "" {
 		_, snapshotType, _, err := common.SnapshotIDToProjectKey(snapshotID)

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

@@ -792,6 +792,31 @@ func TestCreateVolumeArguments(t *testing.T) {
 			},
 			expErrCode: codes.InvalidArgument,
 		},
+		{
+			name: "success with provisionedIops parameter",
+			req: &csi.CreateVolumeRequest{
+				Name:               name,
+				CapacityRange:      stdCapRange,
+				VolumeCapabilities: stdVolCaps,
+				Parameters:         map[string]string{"labels": "key1=value1,key2=value2", "provisioned-iops-on-create": 10000},
+			},
+			expVol: &csi.Volume{
+				CapacityBytes:      common.GbToBytes(20),
+				VolumeId:           testVolumeID,
+				VolumeContext:      nil,
+				AccessibleTopology: stdTopology,
+			},
+		},
+		{
+			name: "fail with malformed provisionedIops parameter",
+			req: &csi.CreateVolumeRequest{
+				Name:               name,
+				CapacityRange:      stdCapRange,
+				VolumeCapabilities: stdVolCaps,
+				Parameters:         map[string]string{"labels": "key1=value1,key2=value2", "provisioned-iops-on-create": "dsfo3"},
+			},
+			expErrCode: codes.InvalidArgument,
+		},
 	}
 
 	// Run test cases