From 0b5bff815160b3f98c4338a393a0cb20b410c875 Mon Sep 17 00:00:00 2001 From: juliankatz Date: Mon, 6 Oct 2025 16:13:37 -0700 Subject: [PATCH 1/4] new packages pkg/parameters, pkg/convert! --- pkg/common/utils.go | 306 ----- pkg/common/utils_test.go | 1097 +---------------- pkg/convert/convert.go | 195 +++ pkg/convert/convert_test.go | 471 +++++++ pkg/gce-cloud-provider/compute/fake-gce.go | 17 +- pkg/gce-cloud-provider/compute/gce-compute.go | 35 +- pkg/gce-pd-csi-driver/controller.go | 74 +- pkg/gce-pd-csi-driver/controller_test.go | 199 +-- pkg/gce-pd-csi-driver/utils.go | 9 +- pkg/gce-pd-csi-driver/utils_test.go | 29 +- pkg/metrics/metadata.go | 4 +- pkg/{common => parameters}/parameters.go | 91 +- pkg/{common => parameters}/parameters_test.go | 2 +- pkg/parameters/utils.go | 126 ++ pkg/parameters/utils_test.go | 55 + test/e2e/tests/single_zone_e2e_test.go | 65 +- 16 files changed, 1144 insertions(+), 1631 deletions(-) create mode 100644 pkg/convert/convert.go create mode 100644 pkg/convert/convert_test.go rename pkg/{common => parameters}/parameters.go (87%) rename pkg/{common => parameters}/parameters_test.go (99%) create mode 100644 pkg/parameters/utils.go create mode 100644 pkg/parameters/utils_test.go diff --git a/pkg/common/utils.go b/pkg/common/utils.go index e58400440..c0231bd7e 100644 --- a/pkg/common/utils.go +++ b/pkg/common/utils.go @@ -34,9 +34,7 @@ import ( "google.golang.org/api/googleapi" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" - "k8s.io/apimachinery/pkg/api/resource" "k8s.io/apimachinery/pkg/util/sets" - volumehelpers "k8s.io/cloud-provider/volume/helpers" "k8s.io/klog/v2" "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/constants" ) @@ -91,15 +89,10 @@ const ( ) var ( - multiRegionalPattern = regexp.MustCompile(multiRegionalLocationFmt) - regionalPattern = regexp.MustCompile(regionalLocationFmt) - // Full or partial URL of the machine type resource, in the format: // zones/zone/machineTypes/machine-type machineTypeRegex = regexp.MustCompile(machineTypePattern) - storagePoolFieldsRegex = regexp.MustCompile(`^projects/([^/]+)/zones/([^/]+)/storagePools/([^/]+)$`) - zoneURIRegex = regexp.MustCompile(zoneURIPattern) // userErrorCodeMap tells how API error types are translated to error codes. @@ -111,13 +104,6 @@ var ( http.StatusConflict: codes.FailedPrecondition, } - validDataCacheMode = []string{constants.DataCacheModeWriteBack, constants.DataCacheModeWriteThrough} - - // Regular expressions for validating parent_id, key and value of a resource tag. - regexParent = regexp.MustCompile(`(^[1-9][0-9]{0,31}$)|(^[a-z][a-z0-9-]{4,28}[a-z0-9]$)`) - regexKey = regexp.MustCompile(`^[a-zA-Z0-9]([0-9A-Za-z_.-]{0,61}[a-zA-Z0-9])?$`) - regexValue = regexp.MustCompile(`^[a-zA-Z0-9]([0-9A-Za-z_.@%=+:,*#&()\[\]{}\-\s]{0,61}[a-zA-Z0-9])?$`) - csiRetryableErrorCodes = []codes.Code{codes.Canceled, codes.DeadlineExceeded, codes.Unavailable, codes.Aborted, codes.ResourceExhausted} ) @@ -234,212 +220,6 @@ func CreateZonalVolumeID(project, zone, name string) string { return fmt.Sprintf(volIDZonalFmt, project, zone, name) } -// ConvertLabelsStringToMap converts the labels from string to map -// example: "key1=value1,key2=value2" gets converted into {"key1": "value1", "key2": "value2"} -// See https://cloud.google.com/compute/docs/labeling-resources#label_format for details. -func ConvertLabelsStringToMap(labels string) (map[string]string, error) { - const labelsDelimiter = "," - const labelsKeyValueDelimiter = "=" - - labelsMap := make(map[string]string) - if labels == "" { - return labelsMap, nil - } - - regexKey, _ := regexp.Compile(`^\p{Ll}[\p{Ll}0-9_-]{0,62}$`) - checkLabelKeyFn := func(key string) error { - if !regexKey.MatchString(key) { - return fmt.Errorf("label value %q is invalid (should start with lowercase letter / lowercase letter, digit, _ and - chars are allowed / 1-63 characters", key) - } - return nil - } - - regexValue, _ := regexp.Compile(`^[\p{Ll}0-9_-]{0,63}$`) - checkLabelValueFn := func(value string) error { - if !regexValue.MatchString(value) { - return fmt.Errorf("label value %q is invalid (lowercase letter, digit, _ and - chars are allowed / 0-63 characters", value) - } - - return nil - } - - keyValueStrings := strings.Split(labels, labelsDelimiter) - for _, keyValue := range keyValueStrings { - keyValue := strings.Split(keyValue, labelsKeyValueDelimiter) - - if len(keyValue) != 2 { - return nil, fmt.Errorf("labels %q are invalid, correct format: 'key1=value1,key2=value2'", labels) - } - - key := strings.TrimSpace(keyValue[0]) - if err := checkLabelKeyFn(key); err != nil { - return nil, err - } - - value := strings.TrimSpace(keyValue[1]) - if err := checkLabelValueFn(value); err != nil { - return nil, err - } - - labelsMap[key] = value - } - - const maxNumberOfLabels = 64 - if len(labelsMap) > maxNumberOfLabels { - return nil, fmt.Errorf("more than %d labels is not allowed, given: %d", maxNumberOfLabels, len(labelsMap)) - } - - return labelsMap, nil -} - -// ConvertTagsStringToMap converts the tags from string to Tag slice -// example: "parent_id1/tag_key1/tag_value1,parent_id2/tag_key2/tag_value2" gets -// converted into {"parent_id1/tag_key1":"tag_value1", "parent_id2/tag_key2":"tag_value2"} -// 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 -func ConvertTagsStringToMap(tags string) (map[string]string, error) { - const tagsDelimiter = "," - const tagsParentIDKeyValueDelimiter = "/" - - tagsMap := make(map[string]string) - if tags == "" { - return nil, nil - } - - checkTagParentIDFn := func(tag, parentID string) error { - if !regexParent.MatchString(parentID) { - return fmt.Errorf("tag parent_id %q for tag %q is invalid. parent_id can have a maximum of 32 characters and cannot be empty. parent_id can be either OrganizationID or ProjectID. OrganizationID must consist of decimal numbers, and cannot have leading zeroes and ProjectID must be 6 to 30 characters in length, can only contain lowercase letters, numbers, and hyphens, and must start with a letter, and cannot end with a hyphen", parentID, tag) - } - return nil - } - - checkTagKeyFn := func(tag, key string) error { - if !regexKey.MatchString(key) { - return fmt.Errorf("tag key %q for tag %q is invalid. Tag key can have a maximum of 63 characters and cannot be empty. Tag key must begin and end with an alphanumeric character, and must contain only uppercase, lowercase alphanumeric characters, and the following special characters `._-`", key, tag) - } - return nil - } - - checkTagValueFn := func(tag, value string) error { - if !regexValue.MatchString(value) { - return fmt.Errorf("tag value %q for tag %q is invalid. Tag value can have a maximum of 63 characters and cannot be empty. Tag value must begin and end with an alphanumeric character, and must contain only uppercase, lowercase alphanumeric characters, and the following special characters `_-.@%%=+:,*#&(){}[]` and spaces", value, tag) - } - - return nil - } - - checkTagParentIDKey := sets.String{} - parentIDkeyValueStrings := strings.Split(tags, tagsDelimiter) - for _, parentIDkeyValueString := range parentIDkeyValueStrings { - parentIDKeyValue := strings.Split(parentIDkeyValueString, tagsParentIDKeyValueDelimiter) - - if len(parentIDKeyValue) != 3 { - return nil, fmt.Errorf("tag %q is invalid, correct format: 'parent_id1/key1/value1,parent_id2/key2/value2'", parentIDkeyValueString) - } - - parentID := strings.TrimSpace(parentIDKeyValue[0]) - if err := checkTagParentIDFn(parentIDkeyValueString, parentID); err != nil { - return nil, err - } - - key := strings.TrimSpace(parentIDKeyValue[1]) - if err := checkTagKeyFn(parentIDkeyValueString, key); err != nil { - return nil, err - } - - value := strings.TrimSpace(parentIDKeyValue[2]) - if err := checkTagValueFn(parentIDkeyValueString, value); err != nil { - return nil, err - } - - parentIDKeyStr := fmt.Sprintf("%s/%s", parentID, key) - if checkTagParentIDKey.Has(parentIDKeyStr) { - return nil, fmt.Errorf("tag parent_id & key combination %q exists more than once", parentIDKeyStr) - } - checkTagParentIDKey.Insert(parentIDKeyStr) - - tagsMap[parentIDKeyStr] = value - } - - // The maximum number of tags allowed per resource is 50. For more details check the following: - // https://cloud.google.com/resource-manager/docs/tags/tags-creating-and-managing#attaching - // https://cloud.google.com/resource-manager/docs/limits#tag-limits - const maxNumberOfTags = 50 - if len(tagsMap) > maxNumberOfTags { - return nil, fmt.Errorf("more than %d tags is not allowed, given: %d", maxNumberOfTags, len(tagsMap)) - } - - return tagsMap, nil -} - -// ProcessStorageLocations trims and normalizes storage location to lower letters. -func ProcessStorageLocations(storageLocations string) ([]string, error) { - normalizedLoc := strings.ToLower(strings.TrimSpace(storageLocations)) - if !multiRegionalPattern.MatchString(normalizedLoc) && !regionalPattern.MatchString(normalizedLoc) { - return []string{}, fmt.Errorf("invalid location for snapshot: %q", storageLocations) - } - return []string{normalizedLoc}, nil -} - -// ValidateSnapshotType validates the type -func ValidateSnapshotType(snapshotType string) error { - switch snapshotType { - case DiskSnapshotType, DiskImageType: - return nil - default: - return fmt.Errorf("invalid snapshot type %s", snapshotType) - } -} - -// ConvertStringToInt64 converts a string to int64 -func ConvertStringToInt64(str string) (int64, error) { - quantity, err := resource.ParseQuantity(str) - if err != nil { - return -1, err - } - return volumehelpers.RoundUpToB(quantity) -} - -// ConvertMiStringToInt64 converts a GiB string to int64 -func ConvertMiStringToInt64(str string) (int64, error) { - quantity, err := resource.ParseQuantity(str) - if err != nil { - return -1, err - } - return volumehelpers.RoundUpToMiB(quantity) -} - -// ConvertGiStringToInt64 converts a GiB string to int64 -func ConvertGiStringToInt64(str string) (int64, error) { - quantity, err := resource.ParseQuantity(str) - if err != nil { - return -1, err - } - return volumehelpers.RoundUpToGiB(quantity) -} - -// ConvertStringToBool converts a string to a boolean. -func ConvertStringToBool(str string) (bool, error) { - switch strings.ToLower(str) { - case "true": - return true, nil - case "false": - return false, nil - } - return false, fmt.Errorf("Unexpected boolean string %s", str) -} - -// ConvertStringToAvailabilityClass converts a string to an availability class string. -func ConvertStringToAvailabilityClass(str string) (string, error) { - switch strings.ToLower(str) { - case ParameterNoAvailabilityClass: - return ParameterNoAvailabilityClass, nil - case ParameterRegionalHardFailoverClass: - return ParameterRegionalHardFailoverClass, nil - } - return "", fmt.Errorf("Unexpected boolean string %s", str) -} - // ParseMachineType returns an extracted machineType from a URL, or empty if not found. // machineTypeUrl: Full or partial URL of the machine type resource, in the format: // @@ -614,12 +394,6 @@ func NewCombinedError(msg string, errs []error) error { return LoggedError(msg, errors.Join(errs...)) } -func isValidDiskEncryptionKmsKey(DiskEncryptionKmsKey string) bool { - // Validate key against default kmskey pattern - kmsKeyPattern := regexp.MustCompile("projects/[^/]+/locations/([^/]+)/keyRings/[^/]+/cryptoKeys/[^/]+") - return kmsKeyPattern.MatchString(DiskEncryptionKmsKey) -} - func ParseZoneFromURI(zoneURI string) (string, error) { zoneMatch := zoneURIRegex.FindStringSubmatch(zoneURI) if zoneMatch == nil { @@ -628,65 +402,8 @@ func ParseZoneFromURI(zoneURI string) (string, error) { return zoneMatch[1], nil } -// ParseStoragePools returns an error if none of the given storagePools -// (delimited by a comma) are in the format -// projects/project/zones/zone/storagePools/storagePool. -func ParseStoragePools(storagePools string) ([]StoragePool, error) { - spSlice := strings.Split(storagePools, ",") - parsedStoragePools := []StoragePool{} - for _, sp := range spSlice { - project, location, spName, err := fieldsFromStoragePoolResourceName(sp) - if err != nil { - return nil, err - } - spObj := StoragePool{Project: project, Zone: location, Name: spName, ResourceName: sp} - parsedStoragePools = append(parsedStoragePools, spObj) - - } - return parsedStoragePools, nil -} - -// fieldsFromResourceName returns the project, zone, and Storage Pool name from the given -// Storage Pool resource name. The resource name must be in the format -// projects/project/zones/zone/storagePools/storagePool. -// All other formats are invalid, and an error will be returned. -func fieldsFromStoragePoolResourceName(resourceName string) (project, location, spName string, err error) { - fieldMatches := storagePoolFieldsRegex.FindStringSubmatch(resourceName) - // Field matches should have 4 strings: [resourceName, project, zone, storagePool]. The first - // match is the entire string. - if len(fieldMatches) != 4 { - err := fmt.Errorf("invalid Storage Pool resource name. Got %s, expected projects/project/zones/zone/storagePools/storagePool", resourceName) - return "", "", "", err - } - project = fieldMatches[1] - location = fieldMatches[2] - spName = fieldMatches[3] - return -} - // StoragePoolZones returns the unique zones of the given storage pool resource names. // Returns an error if multiple storage pools in 1 zone are found. -func StoragePoolZones(storagePools []StoragePool) ([]string, error) { - zonesSet := sets.String{} - var zones []string - for _, sp := range storagePools { - if zonesSet.Has(sp.Zone) { - return nil, fmt.Errorf("found multiple storage pools in zone %s. Only one storage pool per zone is allowed", sp.Zone) - } - zonesSet.Insert(sp.Zone) - zones = append(zones, sp.Zone) - } - return zones, nil -} - -func StoragePoolInZone(storagePools []StoragePool, zone string) *StoragePool { - for _, pool := range storagePools { - if zone == pool.Zone { - return &pool - } - } - return nil -} func UnorderedSlicesEqual(slice1 []string, slice2 []string) bool { set1 := sets.NewString(slice1...) @@ -710,29 +427,6 @@ func VolumeIdAsMultiZone(volumeId string) (string, error) { return strings.Join(splitId, "/"), nil } -func StringInSlice(s string, list []string) bool { - for _, v := range list { - if v == s { - return true - } - } - return false -} - -func ValidateDataCacheMode(s string) error { - if StringInSlice(s, validDataCacheMode) { - return nil - } - return fmt.Errorf("invalid data-cache-mode %s. Only \"writeback\" and \"writethrough\" is a valid input", s) -} - -func ValidateNonNegativeInt(n int64) error { - if n <= 0 { - return fmt.Errorf("Input should be set to > 0, got %d", n) - } - return nil -} - // NewLimiter returns a token bucket based request rate limiter after initializing // the passed values for limit, burst (or token bucket) size. If opted for emptyBucket // all initial tokens are reserved for the first burst. diff --git a/pkg/common/utils_test.go b/pkg/common/utils_test.go index 84fb46370..e9fe0254a 100644 --- a/pkg/common/utils_test.go +++ b/pkg/common/utils_test.go @@ -32,6 +32,7 @@ import ( "google.golang.org/api/googleapi" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/parameters" ) const ( @@ -374,904 +375,6 @@ func TestKeyToVolumeID(t *testing.T) { } -func TestConvertLabelsStringToMap(t *testing.T) { - t.Run("parsing labels string into map", func(t *testing.T) { - testCases := []struct { - name string - labels string - expectedOutput map[string]string - expectedError bool - }{ - { - name: "should return empty map when labels string is empty", - labels: "", - expectedOutput: map[string]string{}, - expectedError: false, - }, - { - name: "single label string", - labels: "key=value", - expectedOutput: map[string]string{ - "key": "value", - }, - expectedError: false, - }, - { - name: "multiple label string", - labels: "key1=value1,key2=value2", - expectedOutput: map[string]string{ - "key1": "value1", - "key2": "value2", - }, - expectedError: false, - }, - { - name: "multiple labels string with whitespaces gets trimmed", - labels: "key1=value1, key2=value2", - expectedOutput: map[string]string{ - "key1": "value1", - "key2": "value2", - }, - expectedError: false, - }, - { - name: "malformed labels string (no keys and values)", - labels: ",,", - expectedOutput: nil, - expectedError: true, - }, - { - name: "malformed labels string (incorrect format)", - labels: "foo,bar", - expectedOutput: nil, - expectedError: true, - }, - { - name: "malformed labels string (missing key)", - labels: "key1=value1,=bar", - expectedOutput: nil, - expectedError: true, - }, - { - name: "malformed labels string (missing key and value)", - labels: "key1=value1,=bar,=", - expectedOutput: nil, - expectedError: true, - }, - } - - for _, tc := range testCases { - t.Logf("test case: %s", tc.name) - output, err := ConvertLabelsStringToMap(tc.labels) - if tc.expectedError && err == nil { - t.Errorf("Expected error but got none") - } - if err != nil { - if !tc.expectedError { - t.Errorf("Did not expect error but got: %v", err) - } - continue - } - - if !reflect.DeepEqual(output, tc.expectedOutput) { - t.Errorf("Got labels %v, but expected %v", output, tc.expectedOutput) - } - } - }) - - t.Run("checking google requirements", func(t *testing.T) { - testCases := []struct { - name string - labels string - expectedError bool - }{ - { - name: "64 labels at most", - labels: `k1=v,k2=v,k3=v,k4=v,k5=v,k6=v,k7=v,k8=v,k9=v,k10=v,k11=v,k12=v,k13=v,k14=v,k15=v,k16=v,k17=v,k18=v,k19=v,k20=v, - k21=v,k22=v,k23=v,k24=v,k25=v,k26=v,k27=v,k28=v,k29=v,k30=v,k31=v,k32=v,k33=v,k34=v,k35=v,k36=v,k37=v,k38=v,k39=v,k40=v, - k41=v,k42=v,k43=v,k44=v,k45=v,k46=v,k47=v,k48=v,k49=v,k50=v,k51=v,k52=v,k53=v,k54=v,k55=v,k56=v,k57=v,k58=v,k59=v,k60=v, - k61=v,k62=v,k63=v,k64=v,k65=v`, - expectedError: true, - }, - { - name: "label key must start with lowercase char (# case)", - labels: "#k=v", - expectedError: true, - }, - { - name: "label key must start with lowercase char (_ case)", - labels: "_k=v", - expectedError: true, - }, - { - name: "label key must start with lowercase char (- case)", - labels: "-k=v", - expectedError: true, - }, - { - name: "label key can only contain lowercase chars, digits, _ and -)", - labels: "k*=v", - expectedError: true, - }, - { - name: "label key may not have over 63 characters", - labels: "abcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghij1234=v", - expectedError: true, - }, - { - name: "label key cannot contain . and /", - labels: "kubernetes.io/created-for/pvc/namespace=v", - expectedError: true, - }, - { - name: "label value can only contain lowercase chars, digits, _ and -)", - labels: "k1=###", - expectedError: true, - }, - { - name: "label value may not have over 63 characters", - labels: "abcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghij1234=v", - expectedError: true, - }, - { - name: "label value cannot contain . and /", - labels: "kubernetes_io_created-for_pvc_namespace=v./", - expectedError: true, - }, - { - name: "label key can have up to 63 characters", - labels: "abcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghij123=v", - expectedError: false, - }, - { - name: "label value can have up to 63 characters", - labels: "abcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghij123=v", - expectedError: false, - }, - { - name: "label key can contain _ and -", - labels: "kubernetes_io_created-for_pvc_namespace=v", - expectedError: false, - }, - { - name: "label value can contain _ and -", - labels: "k=my_value-2", - expectedError: false, - }, - } - - for _, tc := range testCases { - t.Logf("test case: %s", tc.name) - _, err := ConvertLabelsStringToMap(tc.labels) - - if tc.expectedError && err == nil { - t.Errorf("Expected error but got none") - } - - if !tc.expectedError && err != nil { - t.Errorf("Did not expect error but got: %v", err) - } - } - }) - -} - -func TestConvertTagsStringToMap(t *testing.T) { - t.Run("parsing tags string into slice", func(t *testing.T) { - testCases := []struct { - name string - tags string - expectedOutput map[string]string - expectedError bool - }{ - { - name: "should return empty slice when tags string is empty", - tags: "", - expectedOutput: nil, - expectedError: false, - }, - { - name: "single tag string", - tags: "parent/key/value", - expectedOutput: map[string]string{"parent/key": "value"}, - expectedError: false, - }, - { - name: "multiple tag string", - tags: "parent1/key1/value1,parent2/key2/value2", - expectedOutput: map[string]string{"parent1/key1": "value1", "parent2/key2": "value2"}, - expectedError: false, - }, - { - name: "multiple tags string with whitespaces gets trimmed", - tags: "parent1/key1/value1, parent2/key2/value2", - expectedOutput: map[string]string{"parent1/key1": "value1", "parent2/key2": "value2"}, - expectedError: false, - }, - { - name: "malformed tags string (no parent_ids, keys and values)", - tags: ",,", - expectedOutput: nil, - expectedError: true, - }, - { - name: "malformed tags string (incorrect format)", - tags: "foo,bar", - expectedOutput: nil, - expectedError: true, - }, - { - name: "malformed tags string (missing parent_id)", - tags: "parent1/key1/value1,/key2/value2", - expectedOutput: nil, - expectedError: true, - }, - { - name: "malformed tags string (missing key)", - tags: "parent1//value1,parent2/key2/value2", - expectedOutput: nil, - expectedError: true, - }, - { - name: "malformed tags string (missing value)", - tags: "parent1/key1/value1,parent2/key2/", - expectedOutput: nil, - expectedError: true, - }, - { - name: "same tag parent_id, key and value string used more than once", - tags: "parent1/key1/value1,parent1/key1/value1", - expectedOutput: nil, - expectedError: true, - }, - { - name: "same tag parent_id & key string used more than once", - tags: "parent1/key1/value1,parent1/key1/value2", - expectedOutput: nil, - expectedError: true, - }, - } - - for _, tc := range testCases { - t.Logf("test case: %s", tc.name) - output, err := ConvertTagsStringToMap(tc.tags) - if tc.expectedError && err == nil { - t.Errorf("Expected error but got none") - } - - if !tc.expectedError && err != nil { - t.Errorf("Did not expect error but got: %v", err) - } - - if err == nil && !reflect.DeepEqual(output, tc.expectedOutput) { - t.Errorf("Got tags %v, but expected %v", output, tc.expectedOutput) - } - } - }) - - t.Run("checking google requirements", func(t *testing.T) { - testCases := []struct { - name string - tags string - expectedError bool - }{ - { - name: "50 tags at most", - tags: `p1/k/v,p2/k/v,p3/k/v,p4/k/v,p5/k/v,p6/k/v,p7/k/v,p8/k/v,p9/k/v,p10/k/v,p11/k/v,p12/k/v,p13/k/v,p14/k/v,p15/k/v,p16/k/v,p17/k/v, - p18/k/v,p19/k/v,p20/k/v,p21/k/v,p22/k/v,p23/k/v,p24/k/v,p25/k/v,p26/k/v,p27/k/v,p28/k/v,p29/k/v,p30/k/v,p31/k/v,p32/k/v,p33/k/v, - p34/k/v,p35/k/v,p36/k/v,p37/k/v,p38/k/v,p39/k/v,p40/k/v,p41/k/v,p42/k/v,p43/k/v,p44/k/v,p45/k/v,p46/k/v,p47/k/v,p48/k/v,p49/k/v, - p50/k/v,p51/k/v`, - expectedError: true, - }, - { - name: "tag parent_id must start with non-zero decimal when OrganizationID is used (leading zeroes case)", - tags: "01/k/v", - expectedError: true, - }, - { - name: "tag parent_id may not have more than 32 characters when OrganizationID is used", - tags: "123546789012345678901234567890123/k/v", - expectedError: true, - }, - { - name: "tag parent_id can have decimal characters when OrganizationID is used", - tags: "1234567890/k/v", - expectedError: false, - }, - { - name: "tag parent_id may not have less than 6 characters when ProjectID is used", - tags: "abcde/k/v", - expectedError: true, - }, - { - name: "tag parent_id must start with lowercase char when ProjectID is used (decimal case)", - tags: "1parent/k/v", - expectedError: true, - }, - { - name: "tag parent_id must start with lowercase char when ProjectID is used (- case)", - tags: "-parent/k/v", - expectedError: true, - }, - { - name: "tag parent_id must end with lowercase alphanumeric char when ProjectID is used (- case)", - tags: "parent-/k/v", - expectedError: true, - }, - { - name: "tag parent_id may not have more than 30 characters when ProjectID is used", - tags: "abcdefghijklmnopqrstuvwxyz12345/k/v", - expectedError: true, - }, - { - name: "tag parent_id can contain lowercase alphanumeric characters and hyphens when ProjectID is used", - tags: "parent-id-100/k/v", - expectedError: false, - }, - { - name: "tag key must start with alphanumeric char (. case)", - tags: "parent/.k/v", - expectedError: true, - }, - { - name: "tag key must start with alphanumeric char (_ case)", - tags: "parent/_k/v", - expectedError: true, - }, - { - name: "tag key must start with alphanumeric char (- case)", - tags: "parent/-k/v", - expectedError: true, - }, - { - name: "tag key can only contain uppercase, lowercase alphanumeric characters, and the following special characters '._-'", - tags: "parent/k*/v", - expectedError: true, - }, - { - name: "tag key may not have over 63 characters", - tags: "parent/abcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghij1234/v", - expectedError: true, - }, - { - name: "tag key can contain uppercase, lowercase alphanumeric characters, and the following special characters '._-'", - tags: "parent/Type_of.cloud-platform/v", - expectedError: false, - }, - { - name: "tag value must start with alphanumeric char (. case)", - tags: "parent/k/.v", - expectedError: true, - }, - { - name: "tag value must start with alphanumeric char (_ case)", - tags: "parent/k/_v", - expectedError: true, - }, - { - name: "tag value must start with alphanumeric char (- case)", - tags: "parent/k/-v", - expectedError: true, - }, - { - name: "tag value can only contain uppercase, lowercase alphanumeric characters, and the following special characters `_-.@%%=+:,*#&(){}[]` and spaces", - tags: "parent/k/v*", - expectedError: true, - }, - { - name: "tag value may not have over 63 characters", - tags: "parent/k/abcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghij1234", - expectedError: true, - }, - { - name: "tag key can contain uppercase, lowercase alphanumeric characters, and the following special characters `_-.@%%=+:,*#&(){}[]` and spaces", - tags: "parent/k/Special@value[10]{20}(30)-example", - expectedError: false, - }, - } - - for _, tc := range testCases { - t.Logf("test case: %s", tc.name) - _, err := ConvertTagsStringToMap(tc.tags) - - if tc.expectedError && err == nil { - t.Errorf("Expected error but got none") - } - - if !tc.expectedError && err != nil { - t.Errorf("Did not expect error but got: %v", err) - } - } - }) -} - -func TestSnapshotStorageLocations(t *testing.T) { - tests := []struct { - desc string - locationString string - expectedNormalizedLocations []string - expectError bool - }{ - { - "valid multi-region", - " uS ", - []string{"us"}, - false, - }, - { - "valid region", - " US-EAST1", - []string{"us-east1"}, - false, - }, - { - "valid region in large continent", - "europe-west12", - []string{"europe-west12"}, - false, - }, - { - // Zones are not valid bucket/snapshot locations. - "single zone", - "us-east1-a", - []string{}, - true, - }, - } - for _, tc := range tests { - t.Run(tc.desc, func(t *testing.T) { - normalizedLocations, err := ProcessStorageLocations(tc.locationString) - if err != nil && !tc.expectError { - t.Errorf("Got error %v processing storage locations %q; expect no error", err, tc.locationString) - } - if err == nil && tc.expectError { - t.Errorf("Got no error processing storage locations %q; expect an error", tc.locationString) - } - if err == nil && !reflect.DeepEqual(normalizedLocations, tc.expectedNormalizedLocations) { - t.Errorf("Got %v for normalized storage locations; expect %v", normalizedLocations, tc.expectedNormalizedLocations) - } - }) - } -} - -func TestConvertStringToInt64(t *testing.T) { - tests := []struct { - desc string - inputStr string - expInt64 int64 - expectError bool - }{ - { - desc: "valid number string", - inputStr: "10000", - expInt64: 10000, - expectError: false, - }, - { - desc: "test higher number", - inputStr: "15000", - expInt64: 15000, - expectError: false, - }, - { - desc: "round M to number", - inputStr: "1M", - expInt64: 1000000, - expectError: false, - }, - { - desc: "round m to number", - inputStr: "1m", - expInt64: 1, - expectError: false, - }, - { - desc: "round k to number", - inputStr: "1k", - expInt64: 1000, - expectError: false, - }, - { - desc: "invalid empty string", - inputStr: "", - expInt64: 0, - expectError: true, - }, - { - desc: "invalid string", - inputStr: "ew%65", - expInt64: 0, - expectError: true, - }, - { - desc: "invalid KiB string", - inputStr: "10KiB", - expInt64: 10000, - expectError: true, - }, - { - desc: "invalid GB string", - inputStr: "10GB", - expInt64: 0, - expectError: true, - }, - { - desc: "round Ki to number", - inputStr: "1Ki", - expInt64: 1024, - expectError: false, - }, - { - desc: "round k to number", - inputStr: "10k", - expInt64: 10000, - expectError: false, - }, - { - desc: "round Mi to number", - inputStr: "10Mi", - expInt64: 10485760, - expectError: false, - }, - { - desc: "round M to number", - inputStr: "10M", - expInt64: 10000000, - expectError: false, - }, - { - desc: "round G to number", - inputStr: "10G", - expInt64: 10000000000, - expectError: false, - }, - { - desc: "round Gi to number", - inputStr: "100Gi", - expInt64: 107374182400, - expectError: false, - }, - { - desc: "round decimal to number", - inputStr: "1.2Gi", - expInt64: 1288490189, - expectError: false, - }, - { - desc: "round big value to number", - inputStr: "8191Pi", - expInt64: 9222246136947933184, - expectError: false, - }, - } - 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) - } - }) - } -} - -func TestConvertMiStringToInt64(t *testing.T) { - tests := []struct { - desc string - inputStr string - expInt64 int64 - expectError bool - }{ - { - desc: "valid number string", - inputStr: "10000", - expInt64: 1, - expectError: false, - }, - { - desc: "round Ki to MiB", - inputStr: "1000Ki", - expInt64: 1, - expectError: false, - }, - { - desc: "round k to MiB", - inputStr: "1000k", - expInt64: 1, - expectError: false, - }, - { - desc: "round Mi to MiB", - inputStr: "1000Mi", - expInt64: 1000, - expectError: false, - }, - { - desc: "round M to MiB", - inputStr: "1000M", - expInt64: 954, - expectError: false, - }, - { - desc: "round G to MiB", - inputStr: "1000G", - expInt64: 953675, - expectError: false, - }, - { - desc: "round Gi to MiB", - inputStr: "10000Gi", - expInt64: 10240000, - expectError: false, - }, - { - desc: "round decimal to MiB", - inputStr: "1.2Gi", - expInt64: 1229, - expectError: false, - }, - { - desc: "round big value to MiB", - inputStr: "8191Pi", - expInt64: 8795019280384, - expectError: false, - }, - { - desc: "invalid empty string", - inputStr: "", - expInt64: 0, - expectError: true, - }, - { - desc: "invalid KiB string", - inputStr: "10KiB", - expInt64: 10000, - expectError: true, - }, - { - desc: "invalid GB string", - inputStr: "10GB", - expInt64: 0, - expectError: true, - }, - { - desc: "invalid string", - inputStr: "ew%65", - expInt64: 0, - expectError: true, - }, - } - for _, tc := range tests { - t.Run(tc.desc, func(t *testing.T) { - actualInt64, err := ConvertMiStringToInt64(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) - } - }) - } -} - -func TestConvertGiStringToInt64(t *testing.T) { - tests := []struct { - desc string - inputStr string - expInt64 int64 - expectError bool - }{ - { - desc: "valid number string", - inputStr: "10000", - expInt64: 1, - expectError: false, - }, - { - desc: "round Ki to GiB", - inputStr: "1000000Ki", - expInt64: 1, - expectError: false, - }, - { - desc: "round k to GiB", - inputStr: "1000000k", - expInt64: 1, - expectError: false, - }, - { - desc: "round Mi to GiB", - inputStr: "1000Mi", - expInt64: 1, - expectError: false, - }, - { - desc: "round M to GiB", - inputStr: "1000M", - expInt64: 1, - expectError: false, - }, - { - desc: "round G to GiB", - inputStr: "1000G", - expInt64: 932, - expectError: false, - }, - { - desc: "round Gi to GiB - most common case", - inputStr: "1234Gi", - expInt64: 1234, - expectError: false, - }, - { - desc: "round decimal to GiB", - inputStr: "1.2Gi", - expInt64: 2, - expectError: false, - }, - { - desc: "round big value to GiB", - inputStr: "8191Pi", - expInt64: 8588886016, - expectError: false, - }, - { - desc: "invalid empty string", - inputStr: "", - expInt64: 0, - expectError: true, - }, - { - desc: "invalid KiB string", - inputStr: "10KiB", - expInt64: 10000, - expectError: true, - }, - { - desc: "invalid GB string", - inputStr: "10GB", - expInt64: 0, - expectError: true, - }, - { - desc: "invalid string", - inputStr: "ew%65", - expInt64: 0, - expectError: true, - }, - } - for _, tc := range tests { - t.Run(tc.desc, func(t *testing.T) { - actualInt64, err := ConvertGiStringToInt64(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) - } - }) - } -} - -func TestConvertStringToBool(t *testing.T) { - tests := []struct { - desc string - inputStr string - expected bool - expectError bool - }{ - { - desc: "valid true", - inputStr: "true", - expected: true, - expectError: false, - }, - { - desc: "valid mixed case true", - inputStr: "True", - expected: true, - expectError: false, - }, - { - desc: "valid false", - inputStr: "false", - expected: false, - expectError: false, - }, - { - desc: "valid mixed case false", - inputStr: "False", - expected: false, - expectError: false, - }, - { - desc: "invalid", - inputStr: "yes", - expected: false, - expectError: true, - }, - } - for _, tc := range tests { - t.Run(tc.desc, func(t *testing.T) { - got, err := ConvertStringToBool(tc.inputStr) - if err != nil && !tc.expectError { - t.Errorf("Got error %v converting string to bool %s; expect no error", err, tc.inputStr) - } - if err == nil && tc.expectError { - t.Errorf("Got no error converting string to bool %s; expect an error", tc.inputStr) - } - if err == nil && got != tc.expected { - t.Errorf("Got %v for converting string to bool; expect %v", got, tc.expected) - } - }) - } -} - -func TestConvertStringToAvailabilityClass(t *testing.T) { - tests := []struct { - desc string - inputStr string - expected string - expectError bool - }{ - { - desc: "valid none", - inputStr: "none", - expected: ParameterNoAvailabilityClass, - expectError: false, - }, - { - desc: "valid mixed case none", - inputStr: "None", - expected: ParameterNoAvailabilityClass, - expectError: false, - }, - { - desc: "valid failover", - inputStr: "regional-hard-failover", - expected: ParameterRegionalHardFailoverClass, - expectError: false, - }, - { - desc: "valid mixed case failover", - inputStr: "Regional-Hard-Failover", - expected: ParameterRegionalHardFailoverClass, - expectError: false, - }, - { - desc: "invalid", - inputStr: "yes", - expected: "", - expectError: true, - }, - } - for _, tc := range tests { - t.Run(tc.desc, func(t *testing.T) { - got, err := ConvertStringToAvailabilityClass(tc.inputStr) - if err != nil && !tc.expectError { - t.Errorf("Got error %v converting string to availablity class %s; expect no error", err, tc.inputStr) - } - if err == nil && tc.expectError { - t.Errorf("Got no error converting string to availablity class %s; expect an error", tc.inputStr) - } - if err == nil && got != tc.expected { - t.Errorf("Got %v for converting string to availablity class; expect %v", got, tc.expected) - } - }) - } -} - func TestParseMachineType(t *testing.T) { tests := []struct { desc string @@ -1540,94 +643,16 @@ func TestIsUserMultiAttachError(t *testing.T) { } } -func TestIsValidDiskEncryptionKmsKey(t *testing.T) { - cases := []struct { - diskEncryptionKmsKey string - expectedIsValid bool - }{ - { - diskEncryptionKmsKey: "projects/my-project/locations/us-central1/keyRings/TestKeyRing/cryptoKeys/test-key", - expectedIsValid: true, - }, - { - diskEncryptionKmsKey: "projects/my-project/locations/global/keyRings/TestKeyRing/cryptoKeys/test-key", - expectedIsValid: true, - }, - { - diskEncryptionKmsKey: "projects/my-project/locations/keyRings/TestKeyRing/cryptoKeys/test-key", - expectedIsValid: false, - }, - } - for _, tc := range cases { - isValid := isValidDiskEncryptionKmsKey(tc.diskEncryptionKmsKey) - if tc.expectedIsValid != isValid { - t.Errorf("test failed: the provided key %s expected to be %v bu tgot %v", tc.diskEncryptionKmsKey, tc.expectedIsValid, isValid) - } - } -} - -func TestFieldsFromResourceName(t *testing.T) { - testcases := []struct { - name string - resourceName string - expectedProject string - expectedZone string - expectedName string - expectedErr bool - }{ - { - name: "StoragePool_WithValidResourceName_ReturnsFields", - resourceName: "projects/my-project/zones/us-central1-a/storagePools/storagePool-1", - expectedProject: "my-project", - expectedZone: "us-central1-a", - expectedName: "storagePool-1", - }, - { - name: "StoragePool_WithFullResourceURL_ReturnsError", - resourceName: "https://www.googleapis.com/compute/v1/projects/project/zones/zone/storagePools/storagePool", - expectedErr: true, - }, - { - name: "StoragePool_WithMissingProject_ReturnsError", - resourceName: "zones/us-central1-a/storagePools/storagePool-1", - expectedErr: true, - }, - { - name: "StoragePool_WithMissingZone_ReturnsError", - resourceName: "projects/my-project/storagePools/storagePool-1", - expectedErr: true, - }, - { - name: "StoragePool_WithMissingStoragePoolName_ReturnsError", - resourceName: "projects/my-project/zones/us-central1-a/storagePool-1", - expectedErr: true, - }, - } - for _, tc := range testcases { - t.Run(tc.name, func(t *testing.T) { - project, zone, name, err := fieldsFromStoragePoolResourceName(tc.resourceName) - input := fmt.Sprintf("fieldsFromStoragePoolResourceName(%q)", tc.resourceName) - gotErr := err != nil - if gotErr != tc.expectedErr { - t.Errorf("%s error presence = %v, expected error presence = %v", input, gotErr, tc.expectedErr) - } - if project != tc.expectedProject || zone != tc.expectedZone || name != tc.expectedName { - t.Errorf("%s returned {project: %q, zone: %q, name: %q}, expected {project: %q, zone: %q, name: %q}", input, project, zone, name, tc.expectedProject, tc.expectedZone, tc.expectedName) - } - }) - } -} - func TestZones(t *testing.T) { testcases := []struct { name string - storagePools []StoragePool + storagePools []parameters.StoragePool expectedZones []string expectedErr bool }{ { name: "StoragePools_WithValidResourceNames_ReturnsZones", - storagePools: []StoragePool{ + storagePools: []parameters.StoragePool{ { Project: "my-project", Zone: "us-central1-a", @@ -1645,7 +670,7 @@ func TestZones(t *testing.T) { }, { name: "StoragePools_WithDuplicateZone_ReturnsError", - storagePools: []StoragePool{ + storagePools: []parameters.StoragePool{ { Project: "my-project", Zone: "us-central1-a", @@ -1664,7 +689,7 @@ func TestZones(t *testing.T) { } for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { - zones, err := StoragePoolZones(tc.storagePools) + zones, err := parameters.StoragePoolZones(tc.storagePools) input := fmt.Sprintf("StoragePoolZones(%q)", tc.storagePools) gotErr := err != nil if gotErr != tc.expectedErr { @@ -1680,14 +705,14 @@ func TestZones(t *testing.T) { func TestStoragePoolInZone(t *testing.T) { testcases := []struct { name string - storagePools []StoragePool + storagePools []parameters.StoragePool zone string - expectedStoragePool *StoragePool + expectedStoragePool *parameters.StoragePool expectedErr bool }{ { name: "ValidStoragePools_ReturnsStoragePoolInZone", - storagePools: []StoragePool{ + storagePools: []parameters.StoragePool{ { Project: "my-project", Zone: "us-central1-a", @@ -1702,7 +727,7 @@ func TestStoragePoolInZone(t *testing.T) { }, }, zone: "us-central1-a", - expectedStoragePool: &StoragePool{ + expectedStoragePool: ¶meters.StoragePool{ Project: "my-project", Zone: "us-central1-a", Name: "storagePool-1", @@ -1711,7 +736,7 @@ func TestStoragePoolInZone(t *testing.T) { }, { name: "StoragePoolNotInZone_ReturnsNil", - storagePools: []StoragePool{ + storagePools: []parameters.StoragePool{ { Project: "my-project", Zone: "us-central1-a", @@ -1725,7 +750,7 @@ func TestStoragePoolInZone(t *testing.T) { } for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { - sp := StoragePoolInZone(tc.storagePools, tc.zone) + sp := parameters.StoragePoolInZone(tc.storagePools, tc.zone) input := fmt.Sprintf("StoragePoolInZone(%q)", tc.storagePools) if diff := cmp.Diff(tc.expectedStoragePool, sp); diff != "" { t.Errorf("%s: -want, +got \n%s", input, diff) @@ -1777,106 +802,6 @@ func TestUnorderedSlicesEqual(t *testing.T) { } } -func TestStringInSlice(t *testing.T) { - testCases := []struct { - name string - inputStr string - inputSlice []string - expectedInSlice bool - }{ - { - name: "string is in the slice", - inputStr: "in slice", - inputSlice: []string{"in slice", "other string"}, - expectedInSlice: true, - }, - { - name: "string is NOT in the slice", - inputStr: "not in slice", - inputSlice: []string{"other string"}, - }, - } - - for _, tc := range testCases { - t.Logf("test case: %s", tc.name) - actualResult := StringInSlice(tc.inputStr, tc.inputSlice) - if actualResult != tc.expectedInSlice { - t.Errorf("Expect value is %v but got %v. inputStr is %s, inputSlice is %v", tc.expectedInSlice, actualResult, tc.inputStr, tc.inputSlice) - } - } -} - -func TestValidateDataCacheMode(t *testing.T) { - testCases := []struct { - name string - inputStr string - expectError bool - }{ - { - name: "valid input - writethrough", - inputStr: "writethrough", - }, - { - name: "valid input - writeback", - inputStr: "writeback", - }, - { - name: "invalid input", - inputStr: "write-back not valid", - expectError: true, - }, - } - - for _, tc := range testCases { - t.Logf("test case: %s", tc.name) - err := ValidateDataCacheMode(tc.inputStr) - if err != nil && !tc.expectError { - t.Errorf("Got error %v validate data cache mode %s; expect no error", err, tc.inputStr) - } - - if err == nil && tc.expectError { - t.Errorf("Got no error validate data cache mode %s; expect an error", tc.inputStr) - } - } - -} - -func TestValidateNonNegativeInt(t *testing.T) { - testCases := []struct { - name string - cacheSize int64 - expectError bool - }{ - { - name: "valid input - positive cache size", - cacheSize: 100000, - }, - { - name: "invalid input - cachesize 0", - cacheSize: 0, - expectError: true, - }, - { - name: "invalid input - negative cache size", - cacheSize: -100, - expectError: true, - }, - } - - for _, tc := range testCases { - t.Logf("test case: %s", tc.name) - err := ValidateNonNegativeInt(tc.cacheSize) - if err != nil && !tc.expectError { - t.Errorf("Got error %v validate data cache mode %d; expect no error", err, tc.cacheSize) - } - - if err == nil && tc.expectError { - t.Errorf("Got no error validate data cache mode %d; expect an error", tc.cacheSize) - } - } - -} - func TestParseZoneFromURI(t *testing.T) { testcases := []struct { name string diff --git a/pkg/convert/convert.go b/pkg/convert/convert.go new file mode 100644 index 000000000..dd83c6099 --- /dev/null +++ b/pkg/convert/convert.go @@ -0,0 +1,195 @@ +package convert + +import ( + "fmt" + "regexp" + "strings" + + "k8s.io/apimachinery/pkg/api/resource" + "k8s.io/apimachinery/pkg/util/sets" + volumehelpers "k8s.io/cloud-provider/volume/helpers" +) + +var ( + + // Regular expressions for validating parent_id, key and value of a resource tag. + regexParent = regexp.MustCompile(`(^[1-9][0-9]{0,31}$)|(^[a-z][a-z0-9-]{4,28}[a-z0-9]$)`) + regexKey = regexp.MustCompile(`^[a-zA-Z0-9]([0-9A-Za-z_.-]{0,61}[a-zA-Z0-9])?$`) + regexValue = regexp.MustCompile(`^[a-zA-Z0-9]([0-9A-Za-z_.@%=+:,*#&()\[\]{}\-\s]{0,61}[a-zA-Z0-9])?$`) +) + +// ConvertStringToInt64 converts a string to int64 +func ConvertStringToInt64(str string) (int64, error) { + quantity, err := resource.ParseQuantity(str) + if err != nil { + return -1, err + } + return volumehelpers.RoundUpToB(quantity) +} + +// ConvertMiStringToInt64 converts a GiB string to int64 +func ConvertMiStringToInt64(str string) (int64, error) { + quantity, err := resource.ParseQuantity(str) + if err != nil { + return -1, err + } + return volumehelpers.RoundUpToMiB(quantity) +} + +// ConvertGiStringToInt64 converts a GiB string to int64 +func ConvertGiStringToInt64(str string) (int64, error) { + quantity, err := resource.ParseQuantity(str) + if err != nil { + return -1, err + } + return volumehelpers.RoundUpToGiB(quantity) +} + +// ConvertStringToBool converts a string to a boolean. +func ConvertStringToBool(str string) (bool, error) { + switch strings.ToLower(str) { + case "true": + return true, nil + case "false": + return false, nil + } + return false, fmt.Errorf("Unexpected boolean string %s", str) +} + +// ConvertLabelsStringToMap converts the labels from string to map +// example: "key1=value1,key2=value2" gets converted into {"key1": "value1", "key2": "value2"} +// See https://cloud.google.com/compute/docs/labeling-resources#label_format for details. +func ConvertLabelsStringToMap(labels string) (map[string]string, error) { + const labelsDelimiter = "," + const labelsKeyValueDelimiter = "=" + + labelsMap := make(map[string]string) + if labels == "" { + return labelsMap, nil + } + + regexKey, _ := regexp.Compile(`^\p{Ll}[\p{Ll}0-9_-]{0,62}$`) + checkLabelKeyFn := func(key string) error { + if !regexKey.MatchString(key) { + return fmt.Errorf("label value %q is invalid (should start with lowercase letter / lowercase letter, digit, _ and - chars are allowed / 1-63 characters", key) + } + return nil + } + + regexValue, _ := regexp.Compile(`^[\p{Ll}0-9_-]{0,63}$`) + checkLabelValueFn := func(value string) error { + if !regexValue.MatchString(value) { + return fmt.Errorf("label value %q is invalid (lowercase letter, digit, _ and - chars are allowed / 0-63 characters", value) + } + + return nil + } + + keyValueStrings := strings.Split(labels, labelsDelimiter) + for _, keyValue := range keyValueStrings { + keyValue := strings.Split(keyValue, labelsKeyValueDelimiter) + + if len(keyValue) != 2 { + return nil, fmt.Errorf("labels %q are invalid, correct format: 'key1=value1,key2=value2'", labels) + } + + key := strings.TrimSpace(keyValue[0]) + if err := checkLabelKeyFn(key); err != nil { + return nil, err + } + + value := strings.TrimSpace(keyValue[1]) + if err := checkLabelValueFn(value); err != nil { + return nil, err + } + + labelsMap[key] = value + } + + const maxNumberOfLabels = 64 + if len(labelsMap) > maxNumberOfLabels { + return nil, fmt.Errorf("more than %d labels is not allowed, given: %d", maxNumberOfLabels, len(labelsMap)) + } + + return labelsMap, nil +} + +// ConvertTagsStringToMap converts the tags from string to Tag slice +// example: "parent_id1/tag_key1/tag_value1,parent_id2/tag_key2/tag_value2" gets +// converted into {"parent_id1/tag_key1":"tag_value1", "parent_id2/tag_key2":"tag_value2"} +// 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 +func ConvertTagsStringToMap(tags string) (map[string]string, error) { + const tagsDelimiter = "," + const tagsParentIDKeyValueDelimiter = "/" + + tagsMap := make(map[string]string) + if tags == "" { + return nil, nil + } + + checkTagParentIDFn := func(tag, parentID string) error { + if !regexParent.MatchString(parentID) { + return fmt.Errorf("tag parent_id %q for tag %q is invalid. parent_id can have a maximum of 32 characters and cannot be empty. parent_id can be either OrganizationID or ProjectID. OrganizationID must consist of decimal numbers, and cannot have leading zeroes and ProjectID must be 6 to 30 characters in length, can only contain lowercase letters, numbers, and hyphens, and must start with a letter, and cannot end with a hyphen", parentID, tag) + } + return nil + } + + checkTagKeyFn := func(tag, key string) error { + if !regexKey.MatchString(key) { + return fmt.Errorf("tag key %q for tag %q is invalid. Tag key can have a maximum of 63 characters and cannot be empty. Tag key must begin and end with an alphanumeric character, and must contain only uppercase, lowercase alphanumeric characters, and the following special characters `._-`", key, tag) + } + return nil + } + + checkTagValueFn := func(tag, value string) error { + if !regexValue.MatchString(value) { + return fmt.Errorf("tag value %q for tag %q is invalid. Tag value can have a maximum of 63 characters and cannot be empty. Tag value must begin and end with an alphanumeric character, and must contain only uppercase, lowercase alphanumeric characters, and the following special characters `_-.@%%=+:,*#&(){}[]` and spaces", value, tag) + } + + return nil + } + + checkTagParentIDKey := sets.String{} + parentIDkeyValueStrings := strings.Split(tags, tagsDelimiter) + for _, parentIDkeyValueString := range parentIDkeyValueStrings { + parentIDKeyValue := strings.Split(parentIDkeyValueString, tagsParentIDKeyValueDelimiter) + + if len(parentIDKeyValue) != 3 { + return nil, fmt.Errorf("tag %q is invalid, correct format: 'parent_id1/key1/value1,parent_id2/key2/value2'", parentIDkeyValueString) + } + + parentID := strings.TrimSpace(parentIDKeyValue[0]) + if err := checkTagParentIDFn(parentIDkeyValueString, parentID); err != nil { + return nil, err + } + + key := strings.TrimSpace(parentIDKeyValue[1]) + if err := checkTagKeyFn(parentIDkeyValueString, key); err != nil { + return nil, err + } + + value := strings.TrimSpace(parentIDKeyValue[2]) + if err := checkTagValueFn(parentIDkeyValueString, value); err != nil { + return nil, err + } + + parentIDKeyStr := fmt.Sprintf("%s/%s", parentID, key) + if checkTagParentIDKey.Has(parentIDKeyStr) { + return nil, fmt.Errorf("tag parent_id & key combination %q exists more than once", parentIDKeyStr) + } + checkTagParentIDKey.Insert(parentIDKeyStr) + + tagsMap[parentIDKeyStr] = value + } + + // The maximum number of tags allowed per resource is 50. For more details check the following: + // https://cloud.google.com/resource-manager/docs/tags/tags-creating-and-managing#attaching + // https://cloud.google.com/resource-manager/docs/limits#tag-limits + const maxNumberOfTags = 50 + if len(tagsMap) > maxNumberOfTags { + return nil, fmt.Errorf("more than %d tags is not allowed, given: %d", maxNumberOfTags, len(tagsMap)) + } + + return tagsMap, nil +} diff --git a/pkg/convert/convert_test.go b/pkg/convert/convert_test.go new file mode 100644 index 000000000..9829d0102 --- /dev/null +++ b/pkg/convert/convert_test.go @@ -0,0 +1,471 @@ +package convert + +import ( + "reflect" + "testing" +) + +func TestConvertLabelsStringToMap(t *testing.T) { + t.Run("parsing labels string into map", func(t *testing.T) { + testCases := []struct { + name string + labels string + expectedOutput map[string]string + expectedError bool + }{ + { + name: "should return empty map when labels string is empty", + labels: "", + expectedOutput: map[string]string{}, + expectedError: false, + }, + { + name: "single label string", + labels: "key=value", + expectedOutput: map[string]string{ + "key": "value", + }, + expectedError: false, + }, + { + name: "multiple label string", + labels: "key1=value1,key2=value2", + expectedOutput: map[string]string{ + "key1": "value1", + "key2": "value2", + }, + expectedError: false, + }, + { + name: "multiple labels string with whitespaces gets trimmed", + labels: "key1=value1, key2=value2", + expectedOutput: map[string]string{ + "key1": "value1", + "key2": "value2", + }, + expectedError: false, + }, + { + name: "malformed labels string (no keys and values)", + labels: ",,", + expectedOutput: nil, + expectedError: true, + }, + { + name: "malformed labels string (incorrect format)", + labels: "foo,bar", + expectedOutput: nil, + expectedError: true, + }, + { + name: "malformed labels string (missing key)", + labels: "key1=value1,=bar", + expectedOutput: nil, + expectedError: true, + }, + { + name: "malformed labels string (missing key and value)", + labels: "key1=value1,=bar,=", + expectedOutput: nil, + expectedError: true, + }, + } + + for _, tc := range testCases { + t.Logf("test case: %s", tc.name) + output, err := ConvertLabelsStringToMap(tc.labels) + if tc.expectedError && err == nil { + t.Errorf("Expected error but got none") + } + if err != nil { + if !tc.expectedError { + t.Errorf("Did not expect error but got: %v", err) + } + continue + } + + if !reflect.DeepEqual(output, tc.expectedOutput) { + t.Errorf("Got labels %v, but expected %v", output, tc.expectedOutput) + } + } + }) + + t.Run("checking google requirements", func(t *testing.T) { + testCases := []struct { + name string + labels string + expectedError bool + }{ + { + name: "64 labels at most", + labels: `k1=v,k2=v,k3=v,k4=v,k5=v,k6=v,k7=v,k8=v,k9=v,k10=v,k11=v,k12=v,k13=v,k14=v,k15=v,k16=v,k17=v,k18=v,k19=v,k20=v, + k21=v,k22=v,k23=v,k24=v,k25=v,k26=v,k27=v,k28=v,k29=v,k30=v,k31=v,k32=v,k33=v,k34=v,k35=v,k36=v,k37=v,k38=v,k39=v,k40=v, + k41=v,k42=v,k43=v,k44=v,k45=v,k46=v,k47=v,k48=v,k49=v,k50=v,k51=v,k52=v,k53=v,k54=v,k55=v,k56=v,k57=v,k58=v,k59=v,k60=v, + k61=v,k62=v,k63=v,k64=v,k65=v`, + expectedError: true, + }, + { + name: "label key must start with lowercase char (# case)", + labels: "#k=v", + expectedError: true, + }, + { + name: "label key must start with lowercase char (_ case)", + labels: "_k=v", + expectedError: true, + }, + { + name: "label key must start with lowercase char (- case)", + labels: "-k=v", + expectedError: true, + }, + { + name: "label key can only contain lowercase chars, digits, _ and -)", + labels: "k*=v", + expectedError: true, + }, + { + name: "label key may not have over 63 characters", + labels: "abcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghij1234=v", + expectedError: true, + }, + { + name: "label key cannot contain . and /", + labels: "kubernetes.io/created-for/pvc/namespace=v", + expectedError: true, + }, + { + name: "label value can only contain lowercase chars, digits, _ and -)", + labels: "k1=###", + expectedError: true, + }, + { + name: "label value may not have over 63 characters", + labels: "abcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghij1234=v", + expectedError: true, + }, + { + name: "label value cannot contain . and /", + labels: "kubernetes_io_created-for_pvc_namespace=v./", + expectedError: true, + }, + { + name: "label key can have up to 63 characters", + labels: "abcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghij123=v", + expectedError: false, + }, + { + name: "label value can have up to 63 characters", + labels: "abcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghij123=v", + expectedError: false, + }, + { + name: "label key can contain _ and -", + labels: "kubernetes_io_created-for_pvc_namespace=v", + expectedError: false, + }, + { + name: "label value can contain _ and -", + labels: "k=my_value-2", + expectedError: false, + }, + } + + for _, tc := range testCases { + t.Logf("test case: %s", tc.name) + _, err := ConvertLabelsStringToMap(tc.labels) + + if tc.expectedError && err == nil { + t.Errorf("Expected error but got none") + } + + if !tc.expectedError && err != nil { + t.Errorf("Did not expect error but got: %v", err) + } + } + }) + +} + +func TestConvertTagsStringToMap(t *testing.T) { + t.Run("parsing tags string into slice", func(t *testing.T) { + testCases := []struct { + name string + tags string + expectedOutput map[string]string + expectedError bool + }{ + { + name: "should return empty slice when tags string is empty", + tags: "", + expectedOutput: nil, + expectedError: false, + }, + { + name: "single tag string", + tags: "parent/key/value", + expectedOutput: map[string]string{"parent/key": "value"}, + expectedError: false, + }, + { + name: "multiple tag string", + tags: "parent1/key1/value1,parent2/key2/value2", + expectedOutput: map[string]string{"parent1/key1": "value1", "parent2/key2": "value2"}, + expectedError: false, + }, + { + name: "multiple tags string with whitespaces gets trimmed", + tags: "parent1/key1/value1, parent2/key2/value2", + expectedOutput: map[string]string{"parent1/key1": "value1", "parent2/key2": "value2"}, + expectedError: false, + }, + { + name: "malformed tags string (no parent_ids, keys and values)", + tags: ",,", + expectedOutput: nil, + expectedError: true, + }, + { + name: "malformed tags string (incorrect format)", + tags: "foo,bar", + expectedOutput: nil, + expectedError: true, + }, + { + name: "malformed tags string (missing parent_id)", + tags: "parent1/key1/value1,/key2/value2", + expectedOutput: nil, + expectedError: true, + }, + { + name: "malformed tags string (missing key)", + tags: "parent1//value1,parent2/key2/value2", + expectedOutput: nil, + expectedError: true, + }, + { + name: "malformed tags string (missing value)", + tags: "parent1/key1/value1,parent2/key2/", + expectedOutput: nil, + expectedError: true, + }, + { + name: "same tag parent_id, key and value string used more than once", + tags: "parent1/key1/value1,parent1/key1/value1", + expectedOutput: nil, + expectedError: true, + }, + { + name: "same tag parent_id & key string used more than once", + tags: "parent1/key1/value1,parent1/key1/value2", + expectedOutput: nil, + expectedError: true, + }, + } + + for _, tc := range testCases { + t.Logf("test case: %s", tc.name) + output, err := ConvertTagsStringToMap(tc.tags) + if tc.expectedError && err == nil { + t.Errorf("Expected error but got none") + } + + if !tc.expectedError && err != nil { + t.Errorf("Did not expect error but got: %v", err) + } + + if err == nil && !reflect.DeepEqual(output, tc.expectedOutput) { + t.Errorf("Got tags %v, but expected %v", output, tc.expectedOutput) + } + } + }) + + t.Run("checking google requirements", func(t *testing.T) { + testCases := []struct { + name string + tags string + expectedError bool + }{ + { + name: "50 tags at most", + tags: `p1/k/v,p2/k/v,p3/k/v,p4/k/v,p5/k/v,p6/k/v,p7/k/v,p8/k/v,p9/k/v,p10/k/v,p11/k/v,p12/k/v,p13/k/v,p14/k/v,p15/k/v,p16/k/v,p17/k/v, + p18/k/v,p19/k/v,p20/k/v,p21/k/v,p22/k/v,p23/k/v,p24/k/v,p25/k/v,p26/k/v,p27/k/v,p28/k/v,p29/k/v,p30/k/v,p31/k/v,p32/k/v,p33/k/v, + p34/k/v,p35/k/v,p36/k/v,p37/k/v,p38/k/v,p39/k/v,p40/k/v,p41/k/v,p42/k/v,p43/k/v,p44/k/v,p45/k/v,p46/k/v,p47/k/v,p48/k/v,p49/k/v, + p50/k/v,p51/k/v`, + expectedError: true, + }, + { + name: "tag parent_id must start with non-zero decimal when OrganizationID is used (leading zeroes case)", + tags: "01/k/v", + expectedError: true, + }, + { + name: "tag parent_id may not have more than 32 characters when OrganizationID is used", + tags: "123546789012345678901234567890123/k/v", + expectedError: true, + }, + { + name: "tag parent_id can have decimal characters when OrganizationID is used", + tags: "1234567890/k/v", + expectedError: false, + }, + { + name: "tag parent_id may not have less than 6 characters when ProjectID is used", + tags: "abcde/k/v", + expectedError: true, + }, + { + name: "tag parent_id must start with lowercase char when ProjectID is used (decimal case)", + tags: "1parent/k/v", + expectedError: true, + }, + { + name: "tag parent_id must start with lowercase char when ProjectID is used (- case)", + tags: "-parent/k/v", + expectedError: true, + }, + { + name: "tag parent_id must end with lowercase alphanumeric char when ProjectID is used (- case)", + tags: "parent-/k/v", + expectedError: true, + }, + { + name: "tag parent_id may not have more than 30 characters when ProjectID is used", + tags: "abcdefghijklmnopqrstuvwxyz12345/k/v", + expectedError: true, + }, + { + name: "tag parent_id can contain lowercase alphanumeric characters and hyphens when ProjectID is used", + tags: "parent-id-100/k/v", + expectedError: false, + }, + { + name: "tag key must start with alphanumeric char (. case)", + tags: "parent/.k/v", + expectedError: true, + }, + { + name: "tag key must start with alphanumeric char (_ case)", + tags: "parent/_k/v", + expectedError: true, + }, + { + name: "tag key must start with alphanumeric char (- case)", + tags: "parent/-k/v", + expectedError: true, + }, + { + name: "tag key can only contain uppercase, lowercase alphanumeric characters, and the following special characters '._-'", + tags: "parent/k*/v", + expectedError: true, + }, + { + name: "tag key may not have over 63 characters", + tags: "parent/abcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghij1234/v", + expectedError: true, + }, + { + name: "tag key can contain uppercase, lowercase alphanumeric characters, and the following special characters '._-'", + tags: "parent/Type_of.cloud-platform/v", + expectedError: false, + }, + { + name: "tag value must start with alphanumeric char (. case)", + tags: "parent/k/.v", + expectedError: true, + }, + { + name: "tag value must start with alphanumeric char (_ case)", + tags: "parent/k/_v", + expectedError: true, + }, + { + name: "tag value must start with alphanumeric char (- case)", + tags: "parent/k/-v", + expectedError: true, + }, + { + name: "tag value can only contain uppercase, lowercase alphanumeric characters, and the following special characters `_-.@%%=+:,*#&(){}[]` and spaces", + tags: "parent/k/v*", + expectedError: true, + }, + { + name: "tag value may not have over 63 characters", + tags: "parent/k/abcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghij1234", + expectedError: true, + }, + { + name: "tag key can contain uppercase, lowercase alphanumeric characters, and the following special characters `_-.@%%=+:,*#&(){}[]` and spaces", + tags: "parent/k/Special@value[10]{20}(30)-example", + expectedError: false, + }, + } + + for _, tc := range testCases { + t.Logf("test case: %s", tc.name) + _, err := ConvertTagsStringToMap(tc.tags) + + if tc.expectedError && err == nil { + t.Errorf("Expected error but got none") + } + + if !tc.expectedError && err != nil { + t.Errorf("Did not expect error but got: %v", err) + } + } + }) +} + +func TestConvertStringToBool(t *testing.T) { + tests := []struct { + desc string + inputStr string + expected bool + expectError bool + }{ + { + desc: "valid true", + inputStr: "true", + expected: true, + expectError: false, + }, + { + desc: "valid mixed case true", + inputStr: "True", + expected: true, + expectError: false, + }, + { + desc: "valid false", + inputStr: "false", + expected: false, + expectError: false, + }, + { + desc: "valid mixed case false", + inputStr: "False", + expected: false, + expectError: false, + }, + { + desc: "invalid", + inputStr: "yes", + expected: false, + expectError: true, + }, + } + for _, tc := range tests { + t.Run(tc.desc, func(t *testing.T) { + got, err := ConvertStringToBool(tc.inputStr) + if err != nil && !tc.expectError { + t.Errorf("Got error %v converting string to bool %s; expect no error", err, tc.inputStr) + } + if err == nil && tc.expectError { + t.Errorf("Got no error converting string to bool %s; expect an error", tc.inputStr) + } + if err == nil && got != tc.expected { + t.Errorf("Got %v for converting string to bool; expect %v", got, tc.expected) + } + }) + } +} diff --git a/pkg/gce-cloud-provider/compute/fake-gce.go b/pkg/gce-cloud-provider/compute/fake-gce.go index a03f3135b..9e8b8ca53 100644 --- a/pkg/gce-cloud-provider/compute/fake-gce.go +++ b/pkg/gce-cloud-provider/compute/fake-gce.go @@ -31,6 +31,7 @@ import ( "k8s.io/klog/v2" "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/common" "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/constants" + "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/parameters" "k8s.io/apimachinery/pkg/util/sets" ) @@ -198,7 +199,7 @@ func (cloud *FakeCloudProvider) GetDisk(ctx context.Context, project string, vol return disk, nil } -func (cloud *FakeCloudProvider) InsertDisk(ctx context.Context, project string, volKey *meta.Key, params common.DiskParameters, capBytes int64, capacityRange *csi.CapacityRange, replicaZones []string, snapshotID string, volumeContentSourceVolumeID string, multiWriter bool, accessMode string) error { +func (cloud *FakeCloudProvider) InsertDisk(ctx context.Context, project string, volKey *meta.Key, params parameters.DiskParameters, capBytes int64, capacityRange *csi.CapacityRange, replicaZones []string, snapshotID string, volumeContentSourceVolumeID string, multiWriter bool, accessMode string) error { if disk, ok := cloud.disks[volKey.String()]; ok { err := ValidateExistingDisk(ctx, disk, params, int64(capacityRange.GetRequiredBytes()), @@ -230,9 +231,9 @@ func (cloud *FakeCloudProvider) InsertDisk(ctx context.Context, project string, return err } switch snapshotType { - case common.DiskSnapshotType: + case parameters.DiskSnapshotType: computeDisk.SourceSnapshotId = snapshotID - case common.DiskImageType: + case parameters.DiskImageType: computeDisk.SourceImageId = snapshotID default: return fmt.Errorf("invalid snapshot type in snapshot ID: %s", snapshotType) @@ -259,7 +260,7 @@ func (cloud *FakeCloudProvider) InsertDisk(ctx context.Context, project string, return nil } -func (cloud *FakeCloudProvider) UpdateDisk(ctx context.Context, project string, volKey *meta.Key, existingDisk *CloudDisk, params common.ModifyVolumeParameters) error { +func (cloud *FakeCloudProvider) UpdateDisk(ctx context.Context, project string, volKey *meta.Key, existingDisk *CloudDisk, params parameters.ModifyVolumeParameters) error { _, ok := cloud.disks[volKey.String()] if !ok { return notFoundError() @@ -400,7 +401,7 @@ func (cloud *FakeCloudProvider) GetSnapshot(ctx context.Context, project, snapsh return snapshot, nil } -func (cloud *FakeCloudProvider) CreateSnapshot(ctx context.Context, project string, volKey *meta.Key, snapshotName string, snapshotParams common.SnapshotParameters) (*computev1.Snapshot, error) { +func (cloud *FakeCloudProvider) CreateSnapshot(ctx context.Context, project string, volKey *meta.Key, snapshotName string, snapshotParams parameters.SnapshotParameters) (*computev1.Snapshot, error) { if snapshot, ok := cloud.snapshots[snapshotName]; ok { return snapshot, nil } @@ -481,7 +482,7 @@ func (cloud *FakeCloudProvider) GetImage(ctx context.Context, project, imageName return image, nil } -func (cloud *FakeCloudProvider) CreateImage(ctx context.Context, project string, volKey *meta.Key, imageName string, snapshotParams common.SnapshotParameters) (*computev1.Image, error) { +func (cloud *FakeCloudProvider) CreateImage(ctx context.Context, project string, volKey *meta.Key, imageName string, snapshotParams parameters.SnapshotParameters) (*computev1.Image, error) { if image, ok := cloud.images[imageName]; ok { return image, nil } @@ -584,14 +585,14 @@ type FakeBlockingCloudProvider struct { // Upon starting a CreateSnapshot, it passes a chan 'executeCreateSnapshot' into readyToExecute, then blocks on executeCreateSnapshot. // The test calling this function can block on readyToExecute to ensure that the operation has started and // allowed the CreateSnapshot to continue by passing a struct into executeCreateSnapshot. -func (cloud *FakeBlockingCloudProvider) CreateSnapshot(ctx context.Context, project string, volKey *meta.Key, snapshotName string, snapshotParams common.SnapshotParameters) (*computev1.Snapshot, error) { +func (cloud *FakeBlockingCloudProvider) CreateSnapshot(ctx context.Context, project string, volKey *meta.Key, snapshotName string, snapshotParams parameters.SnapshotParameters) (*computev1.Snapshot, error) { executeCreateSnapshot := make(chan Signal) cloud.ReadyToExecute <- executeCreateSnapshot <-executeCreateSnapshot return cloud.FakeCloudProvider.CreateSnapshot(ctx, project, volKey, snapshotName, snapshotParams) } -func (cloud *FakeBlockingCloudProvider) CreateImage(ctx context.Context, project string, volKey *meta.Key, imageName string, snapshotParams common.SnapshotParameters) (*computev1.Image, error) { +func (cloud *FakeBlockingCloudProvider) CreateImage(ctx context.Context, project string, volKey *meta.Key, imageName string, snapshotParams parameters.SnapshotParameters) (*computev1.Image, error) { executeCreateSnapshot := make(chan Signal) cloud.ReadyToExecute <- executeCreateSnapshot <-executeCreateSnapshot diff --git a/pkg/gce-cloud-provider/compute/gce-compute.go b/pkg/gce-cloud-provider/compute/gce-compute.go index f6f2ca3d3..de06d9db7 100644 --- a/pkg/gce-cloud-provider/compute/gce-compute.go +++ b/pkg/gce-cloud-provider/compute/gce-compute.go @@ -42,6 +42,7 @@ import ( "k8s.io/utils/strings/slices" "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/common" "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/constants" + "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/parameters" ) const ( @@ -101,9 +102,9 @@ type GCECompute interface { // Disk Methods GetDisk(ctx context.Context, project string, volumeKey *meta.Key) (*CloudDisk, error) RepairUnderspecifiedVolumeKey(ctx context.Context, project string, volumeKey *meta.Key) (string, *meta.Key, error) - InsertDisk(ctx context.Context, project string, volKey *meta.Key, params common.DiskParameters, capBytes int64, capacityRange *csi.CapacityRange, replicaZones []string, snapshotID string, volumeContentSourceVolumeID string, multiWriter bool, accessMode string) error + InsertDisk(ctx context.Context, project string, volKey *meta.Key, params parameters.DiskParameters, capBytes int64, capacityRange *csi.CapacityRange, replicaZones []string, snapshotID string, volumeContentSourceVolumeID string, multiWriter bool, accessMode string) error DeleteDisk(ctx context.Context, project string, volumeKey *meta.Key) error - UpdateDisk(ctx context.Context, project string, volKey *meta.Key, existingDisk *CloudDisk, params common.ModifyVolumeParameters) error + UpdateDisk(ctx context.Context, project string, volKey *meta.Key, existingDisk *CloudDisk, params parameters.ModifyVolumeParameters) error AttachDisk(ctx context.Context, project string, volKey *meta.Key, readWrite, diskType, instanceZone, instanceName string, forceAttach bool) error DetachDisk(ctx context.Context, project, deviceName, instanceZone, instanceName string) error SetDiskAccessMode(ctx context.Context, project string, volKey *meta.Key, accessMode string) error @@ -123,11 +124,11 @@ type GCECompute interface { ListZones(ctx context.Context, region string) ([]string, error) ListSnapshots(ctx context.Context, filter string) ([]*computev1.Snapshot, string, error) GetSnapshot(ctx context.Context, project, snapshotName string) (*computev1.Snapshot, error) - CreateSnapshot(ctx context.Context, project string, volKey *meta.Key, snapshotName string, snapshotParams common.SnapshotParameters) (*computev1.Snapshot, error) + CreateSnapshot(ctx context.Context, project string, volKey *meta.Key, snapshotName string, snapshotParams parameters.SnapshotParameters) (*computev1.Snapshot, error) DeleteSnapshot(ctx context.Context, project, snapshotName string) error ListImages(ctx context.Context, filter string) ([]*computev1.Image, string, error) GetImage(ctx context.Context, project, imageName string) (*computev1.Image, error) - CreateImage(ctx context.Context, project string, volKey *meta.Key, imageName string, snapshotParams common.SnapshotParameters) (*computev1.Image, error) + CreateImage(ctx context.Context, project string, volKey *meta.Key, imageName string, snapshotParams parameters.SnapshotParameters) (*computev1.Image, error) DeleteImage(ctx context.Context, project, imageName string) error } @@ -439,7 +440,7 @@ func (cloud *CloudProvider) getRegionURI(project, region string) string { region) } -func ValidateExistingDisk(ctx context.Context, resp *CloudDisk, params common.DiskParameters, reqBytes, limBytes int64, multiWriter bool, accessMode string) error { +func ValidateExistingDisk(ctx context.Context, resp *CloudDisk, params parameters.DiskParameters, reqBytes, limBytes int64, multiWriter bool, accessMode string) error { klog.V(5).Infof("Validating existing disk %v with diskType: %s, reqested bytes: %v, limit bytes: %v", resp, params.DiskType, reqBytes, limBytes) if resp == nil { return fmt.Errorf("disk does not exist") @@ -479,7 +480,7 @@ func validAccessMode(want, got string) bool { // ValidateDiskParameters takes a CloudDisk and returns true if the parameters // specified validly describe the disk provided, and false otherwise. -func ValidateDiskParameters(disk *CloudDisk, params common.DiskParameters) error { +func ValidateDiskParameters(disk *CloudDisk, params parameters.DiskParameters) error { if disk.GetPDType() != params.DiskType { return fmt.Errorf("actual pd type %s did not match the expected param %s", disk.GetPDType(), params.DiskType) } @@ -498,7 +499,7 @@ func ValidateDiskParameters(disk *CloudDisk, params common.DiskParameters) error return nil } -func (cloud *CloudProvider) InsertDisk(ctx context.Context, project string, volKey *meta.Key, params common.DiskParameters, capBytes int64, capacityRange *csi.CapacityRange, replicaZones []string, snapshotID string, volumeContentSourceVolumeID string, multiWriter bool, accessMode string) error { +func (cloud *CloudProvider) InsertDisk(ctx context.Context, project string, volKey *meta.Key, params parameters.DiskParameters, capBytes int64, capacityRange *csi.CapacityRange, replicaZones []string, snapshotID string, volumeContentSourceVolumeID string, multiWriter bool, accessMode string) error { klog.V(5).Infof("Inserting disk %v", volKey) description, err := encodeTags(params.Tags) @@ -534,7 +535,7 @@ func (cloud *CloudProvider) constructDiskToCreate( ctx context.Context, project string, volKey *meta.Key, - params common.DiskParameters, + params parameters.DiskParameters, capBytes int64, replicaZones []string, snapshotID string, @@ -569,7 +570,7 @@ func (cloud *CloudProvider) constructDiskToCreate( if volKey.Type() == meta.Regional { return nil, status.Errorf(codes.InvalidArgument, "cannot create regional disks in a Storage Pool") } - sp := common.StoragePoolInZone(params.StoragePools, volKey.Zone) + sp := parameters.StoragePoolInZone(params.StoragePools, volKey.Zone) if sp == nil { return nil, status.Errorf(codes.InvalidArgument, "cannot create disk in zone %q: no Storage Pools exist in zone", volKey.Zone) } @@ -582,9 +583,9 @@ func (cloud *CloudProvider) constructDiskToCreate( return nil, err } switch snapshotType { - case common.DiskSnapshotType: + case parameters.DiskSnapshotType: diskToCreate.SourceSnapshot = snapshotID - case common.DiskImageType: + case parameters.DiskImageType: diskToCreate.SourceImage = snapshotID default: return nil, fmt.Errorf("invalid snapshot type in snapshot ID: %s", snapshotType) @@ -620,7 +621,7 @@ func (cloud *CloudProvider) constructDiskToCreate( return diskToCreate, nil } -func (cloud *CloudProvider) processDiskAlreadyExistErr(ctx context.Context, err error, project string, volKey *meta.Key, params common.DiskParameters, capacityRange *csi.CapacityRange, multiWriter bool, accessMode string) error { +func (cloud *CloudProvider) processDiskAlreadyExistErr(ctx context.Context, err error, project string, volKey *meta.Key, params parameters.DiskParameters, capacityRange *csi.CapacityRange, multiWriter bool, accessMode string) error { if err == nil { return nil } @@ -644,7 +645,7 @@ func (cloud *CloudProvider) processDiskAlreadyExistErr(ctx context.Context, err return err } -func (cloud *CloudProvider) insertConstructedDisk(ctx context.Context, disk *computebeta.Disk, isZonal bool, project string, volKey *meta.Key, params common.DiskParameters, capacityRange *csi.CapacityRange, multiWriter bool, accessMode string) error { +func (cloud *CloudProvider) insertConstructedDisk(ctx context.Context, disk *computebeta.Disk, isZonal bool, project string, volKey *meta.Key, params parameters.DiskParameters, capacityRange *csi.CapacityRange, multiWriter bool, accessMode string) error { var ( insertOp *computebeta.Operation opName string @@ -680,7 +681,7 @@ func (cloud *CloudProvider) insertConstructedDisk(ctx context.Context, disk *com return nil } -func (cloud *CloudProvider) UpdateDisk(ctx context.Context, project string, volKey *meta.Key, existingDisk *CloudDisk, params common.ModifyVolumeParameters) error { +func (cloud *CloudProvider) UpdateDisk(ctx context.Context, project string, volKey *meta.Key, existingDisk *CloudDisk, params parameters.ModifyVolumeParameters) error { // hyperdisks are zonal disks // pd-disks do not support modification of IOPS and Throughput if volKey.Type() == meta.Regional { @@ -690,7 +691,7 @@ func (cloud *CloudProvider) UpdateDisk(ctx context.Context, project string, volK return cloud.updateZonalDisk(ctx, project, volKey, existingDisk, params) } -func (cloud *CloudProvider) updateZonalDisk(ctx context.Context, project string, volKey *meta.Key, existingDisk *CloudDisk, params common.ModifyVolumeParameters) error { +func (cloud *CloudProvider) updateZonalDisk(ctx context.Context, project string, volKey *meta.Key, existingDisk *CloudDisk, params parameters.ModifyVolumeParameters) error { specifiedIops := params.IOPS != nil && *params.IOPS != 0 specifiedThroughput := params.Throughput != nil && *params.Throughput != 0 if !specifiedIops && !specifiedThroughput { @@ -1264,7 +1265,7 @@ func (cloud *CloudProvider) DeleteSnapshot(ctx context.Context, project, snapsho return nil } -func (cloud *CloudProvider) CreateSnapshot(ctx context.Context, project string, volKey *meta.Key, snapshotName string, snapshotParams common.SnapshotParameters) (*computev1.Snapshot, error) { +func (cloud *CloudProvider) CreateSnapshot(ctx context.Context, project string, volKey *meta.Key, snapshotName string, snapshotParams parameters.SnapshotParameters) (*computev1.Snapshot, error) { klog.V(5).Infof("Creating snapshot %s for volume %v", snapshotName, volKey) description, err := encodeTags(snapshotParams.Tags) @@ -1307,7 +1308,7 @@ func (cloud *CloudProvider) CreateSnapshot(ctx context.Context, project string, return snapshot, err } -func (cloud *CloudProvider) CreateImage(ctx context.Context, project string, volKey *meta.Key, imageName string, snapshotParams common.SnapshotParameters) (*computev1.Image, error) { +func (cloud *CloudProvider) CreateImage(ctx context.Context, project string, volKey *meta.Key, imageName string, snapshotParams parameters.SnapshotParameters) (*computev1.Image, error) { klog.V(5).Infof("Creating image %s for source %v", imageName, volKey) description, err := encodeTags(snapshotParams.Tags) diff --git a/pkg/gce-pd-csi-driver/controller.go b/pkg/gce-pd-csi-driver/controller.go index 51b3e41b6..8b6126f43 100644 --- a/pkg/gce-pd-csi-driver/controller.go +++ b/pkg/gce-pd-csi-driver/controller.go @@ -40,8 +40,10 @@ import ( "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/common" "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/constants" + "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/convert" gce "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/gce-cloud-provider/compute" "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/metrics" + "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/parameters" ) type GCEControllerServer struct { @@ -242,10 +244,10 @@ var ( "nextPageToken", } listDisksFieldsWithUsers = append(listDisksFieldsWithoutUsers, "items/users") - disksWithModifiableAccessMode = []string{common.DiskTypeHdML} + disksWithModifiableAccessMode = []string{parameters.DiskTypeHdML} disksWithUnsettableAccessMode = map[string]bool{ - common.DiskTypeHdE: true, - common.DiskTypeHdT: true, + parameters.DiskTypeHdE: true, + parameters.DiskTypeHdT: true, } ) @@ -353,7 +355,7 @@ func (gceCS *GCEControllerServer) createVolumeInternal(ctx context.Context, req if !supportsIopsChange && !supportsThroughputChange { return nil, status.Errorf(codes.InvalidArgument, "Disk type %s does not support dynamic provisioning", params.DiskType) } - p, err := common.ExtractModifyVolumeParameters(mutableParams) + p, err := parameters.ExtractModifyVolumeParameters(mutableParams) if err != nil { return nil, status.Errorf(codes.InvalidArgument, "Invalid mutable parameters: %v", err) } @@ -385,12 +387,12 @@ func (gceCS *GCEControllerServer) createVolumeInternal(ctx context.Context, req // Validate VolumeContentSource is set when access mode is read only readonly, _ := getReadOnlyFromCapabilities(volumeCapabilities) - if readonly && req.GetVolumeContentSource() == nil && params.DiskType != common.DiskTypeHdML { + if readonly && req.GetVolumeContentSource() == nil && params.DiskType != parameters.DiskTypeHdML { return nil, status.Error(codes.InvalidArgument, "VolumeContentSource must be provided when AccessMode is set to read only") } - if readonly && params.DiskType == common.DiskTypeHdHA { - return nil, status.Errorf(codes.InvalidArgument, "Invalid access mode for disk type %s", common.DiskTypeHdHA) + if readonly && params.DiskType == parameters.DiskTypeHdHA { + return nil, status.Errorf(codes.InvalidArgument, "Invalid access mode for disk type %s", parameters.DiskTypeHdHA) } // Hyperdisk-throughput and hyperdisk-extreme do not support attaching to multiple VMs. @@ -431,7 +433,7 @@ func (gceCS *GCEControllerServer) getSupportedZonesForPDType(ctx context.Context return zones, nil } -func (gceCS *GCEControllerServer) getMultiZoneProvisioningZones(ctx context.Context, req *csi.CreateVolumeRequest, params common.DiskParameters) ([]string, error) { +func (gceCS *GCEControllerServer) getMultiZoneProvisioningZones(ctx context.Context, req *csi.CreateVolumeRequest, params parameters.DiskParameters) ([]string, error) { top := req.GetAccessibilityRequirements() if top == nil { return nil, status.Errorf(codes.InvalidArgument, "no topology specified") @@ -474,7 +476,7 @@ func (gceCS *GCEControllerServer) getMultiZoneProvisioningZones(ctx context.Cont return combinedZones.List(), nil } -func (gceCS *GCEControllerServer) createMultiZoneDisk(ctx context.Context, req *csi.CreateVolumeRequest, params common.DiskParameters, dataCacheParams common.DataCacheParameters, enableDataCache bool) (*csi.CreateVolumeResponse, error) { +func (gceCS *GCEControllerServer) createMultiZoneDisk(ctx context.Context, req *csi.CreateVolumeRequest, params parameters.DiskParameters, dataCacheParams parameters.DataCacheParameters, enableDataCache bool) (*csi.CreateVolumeResponse, error) { var err error // For multi-zone, we either select: // 1) The zones specified in requisite topology requirements @@ -570,7 +572,7 @@ func (gceCS *GCEControllerServer) updateAccessModeIfNecessary(ctx context.Contex return gceCS.CloudProvider.SetDiskAccessMode(ctx, project, volKey, constants.GCEReadOnlyManyAccessMode) } -func (gceCS *GCEControllerServer) createSingleDeviceDisk(ctx context.Context, req *csi.CreateVolumeRequest, params common.DiskParameters, dataCacheParams common.DataCacheParameters, enableDataCache bool) (*csi.CreateVolumeResponse, error) { +func (gceCS *GCEControllerServer) createSingleDeviceDisk(ctx context.Context, req *csi.CreateVolumeRequest, params parameters.DiskParameters, dataCacheParams parameters.DataCacheParameters, enableDataCache bool) (*csi.CreateVolumeResponse, error) { var err error var locationTopReq *locationRequirements if useVolumeCloning(req) { @@ -618,7 +620,7 @@ func (gceCS *GCEControllerServer) createSingleDeviceDisk(ctx context.Context, re // If creating an empty disk (content source nil), always create RWO disks (when supported) // This allows disks to be created as underlying RWO disks, so they can be hydrated. readonly, _ := getReadOnlyFromCapabilities(req.GetVolumeCapabilities()) - if readonly && req.GetVolumeContentSource() == nil && params.DiskType == common.DiskTypeHdML { + if readonly && req.GetVolumeContentSource() == nil && params.DiskType == parameters.DiskTypeHdML { accessMode = constants.GCEReadWriteOnceAccessMode } @@ -635,7 +637,7 @@ func (gceCS *GCEControllerServer) createSingleDeviceDisk(ctx context.Context, re return gceCS.generateCreateVolumeResponseWithVolumeId(disk, zones, params, dataCacheParams, enableDataCache, volumeID), nil } -func getAccessMode(req *csi.CreateVolumeRequest, params common.DiskParameters) (string, error) { +func getAccessMode(req *csi.CreateVolumeRequest, params parameters.DiskParameters) (string, error) { readonly, _ := getReadOnlyFromCapabilities(req.GetVolumeCapabilities()) if common.IsHyperdisk(params.DiskType) { if am, err := getHyperdiskAccessModeFromCapabilities(req.GetVolumeCapabilities()); err != nil { @@ -658,7 +660,7 @@ func getAccessMode(req *csi.CreateVolumeRequest, params common.DiskParameters) ( return "", nil } -func (gceCS *GCEControllerServer) createSingleDisk(ctx context.Context, req *csi.CreateVolumeRequest, params common.DiskParameters, volKey *meta.Key, zones []string, accessMode string) (*gce.CloudDisk, error) { +func (gceCS *GCEControllerServer) createSingleDisk(ctx context.Context, req *csi.CreateVolumeRequest, params parameters.DiskParameters, volKey *meta.Key, zones []string, accessMode string) (*gce.CloudDisk, error) { capacityRange := req.GetCapacityRange() capBytes, _ := getRequestCapacity(capacityRange) @@ -844,7 +846,7 @@ func (gceCS *GCEControllerServer) ControllerModifyVolume(ctx context.Context, re return nil, err } - volumeModifyParams, err := common.ExtractModifyVolumeParameters(req.GetMutableParameters()) + volumeModifyParams, err := parameters.ExtractModifyVolumeParameters(req.GetMutableParameters()) if err != nil { klog.Errorf("Failed to extract parameters for volume %s: %v", volumeID, err) err = status.Errorf(codes.InvalidArgument, "Invalid parameters: %v", err) @@ -1074,7 +1076,7 @@ func (gceCS *GCEControllerServer) validateMultiZoneDisk(volumeID string, disk *g return nil } -func (gceCS *GCEControllerServer) validateMultiZoneProvisioning(req *csi.CreateVolumeRequest, params common.DiskParameters) error { +func (gceCS *GCEControllerServer) validateMultiZoneProvisioning(req *csi.CreateVolumeRequest, params parameters.DiskParameters) error { if !gceCS.multiZoneVolumeHandleConfig.Enable { return nil } @@ -1087,15 +1089,15 @@ func (gceCS *GCEControllerServer) validateMultiZoneProvisioning(req *csi.CreateV // We don't have support volume cloning from an existing PVC if useVolumeCloning(req) { - return fmt.Errorf("%q parameter does not support volume cloning", common.ParameterKeyEnableMultiZoneProvisioning) + return fmt.Errorf("%q parameter does not support volume cloning", parameters.ParameterKeyEnableMultiZoneProvisioning) } if readonly, _ := getReadOnlyFromCapabilities(req.GetVolumeCapabilities()); !readonly && req.GetVolumeContentSource() != nil { - return fmt.Errorf("%q parameter does not support specifying volume content source in readwrite mode", common.ParameterKeyEnableMultiZoneProvisioning) + return fmt.Errorf("%q parameter does not support specifying volume content source in readwrite mode", parameters.ParameterKeyEnableMultiZoneProvisioning) } if !slices.Contains(gceCS.multiZoneVolumeHandleConfig.DiskTypes, params.DiskType) { - return fmt.Errorf("%q parameter with unsupported disk type: %v", common.ParameterKeyEnableMultiZoneProvisioning, params.DiskType) + return fmt.Errorf("%q parameter with unsupported disk type: %v", parameters.ParameterKeyEnableMultiZoneProvisioning, params.DiskType) } return nil @@ -1340,8 +1342,8 @@ func (gceCS *GCEControllerServer) executeControllerUnpublishVolume(ctx context.C return &csi.ControllerUnpublishVolumeResponse{}, nil, diskToUnpublish } -func (gceCS *GCEControllerServer) parameterProcessor() *common.ParameterProcessor { - return &common.ParameterProcessor{ +func (gceCS *GCEControllerServer) parameterProcessor() *parameters.ParameterProcessor { + return ¶meters.ParameterProcessor{ DriverName: gceCS.Driver.name, EnableStoragePools: gceCS.enableStoragePools, EnableMultiZone: gceCS.multiZoneVolumeHandleConfig.Enable, @@ -1628,21 +1630,21 @@ func (gceCS *GCEControllerServer) CreateSnapshot(ctx context.Context, req *csi.C return nil, common.LoggedError("CreateSnapshot, failed to getDisk: ", err) } - snapshotParams, err := common.ExtractAndDefaultSnapshotParameters(req.GetParameters(), gceCS.Driver.name, gceCS.Driver.extraTags) + snapshotParams, err := parameters.ExtractAndDefaultSnapshotParameters(req.GetParameters(), gceCS.Driver.name, gceCS.Driver.extraTags) if err != nil { return nil, status.Errorf(codes.InvalidArgument, "Invalid snapshot parameters: %v", err.Error()) } var snapshot *csi.Snapshot switch snapshotParams.SnapshotType { - case common.DiskSnapshotType: + case parameters.DiskSnapshotType: snapshot, err = gceCS.createPDSnapshot(ctx, project, volKey, req.Name, snapshotParams) if err != nil { return nil, err } - case common.DiskImageType: + case parameters.DiskImageType: if disk.LocationType() == meta.Regional { - return nil, status.Errorf(codes.InvalidArgument, "Cannot create backup type %s for regional disk %s", common.DiskImageType, disk.GetName()) + return nil, status.Errorf(codes.InvalidArgument, "Cannot create backup type %s for regional disk %s", parameters.DiskImageType, disk.GetName()) } snapshot, err = gceCS.createImage(ctx, project, volKey, req.Name, snapshotParams) if err != nil { @@ -1656,7 +1658,7 @@ func (gceCS *GCEControllerServer) CreateSnapshot(ctx context.Context, req *csi.C return &csi.CreateSnapshotResponse{Snapshot: snapshot}, nil } -func (gceCS *GCEControllerServer) createPDSnapshot(ctx context.Context, project string, volKey *meta.Key, snapshotName string, snapshotParams common.SnapshotParameters) (*csi.Snapshot, error) { +func (gceCS *GCEControllerServer) createPDSnapshot(ctx context.Context, project string, volKey *meta.Key, snapshotName string, snapshotParams parameters.SnapshotParameters) (*csi.Snapshot, error) { volumeID, err := common.KeyToVolumeID(volKey, project) if err != nil { return nil, status.Errorf(codes.InvalidArgument, "Invalid volume key: %v", volKey) @@ -1717,7 +1719,7 @@ func (gceCS *GCEControllerServer) createPDSnapshot(ctx context.Context, project }, nil } -func (gceCS *GCEControllerServer) createImage(ctx context.Context, project string, volKey *meta.Key, imageName string, snapshotParams common.SnapshotParameters) (*csi.Snapshot, error) { +func (gceCS *GCEControllerServer) createImage(ctx context.Context, project string, volKey *meta.Key, imageName string, snapshotParams parameters.SnapshotParameters) (*csi.Snapshot, error) { volumeID, err := common.KeyToVolumeID(volKey, project) if err != nil { return nil, status.Errorf(codes.InvalidArgument, "Invalid volume key: %v", volKey) @@ -1876,12 +1878,12 @@ func (gceCS *GCEControllerServer) DeleteSnapshot(ctx context.Context, req *csi.D } switch snapshotType { - case common.DiskSnapshotType: + case parameters.DiskSnapshotType: err = gceCS.CloudProvider.DeleteSnapshot(ctx, project, key) if err != nil { return nil, common.LoggedError("Failed to DeleteSnapshot: ", err) } - case common.DiskImageType: + case parameters.DiskImageType: err = gceCS.CloudProvider.DeleteImage(ctx, project, key) if err != nil { return nil, common.LoggedError("Failed to DeleteImage error: ", err) @@ -2044,7 +2046,7 @@ func (gceCS *GCEControllerServer) getSnapshotByID(ctx context.Context, snapshotI var entries []*csi.ListSnapshotsResponse_Entry switch snapshotType { - case common.DiskSnapshotType: + case parameters.DiskSnapshotType: snapshot, err := gceCS.CloudProvider.GetSnapshot(ctx, project, key) if err != nil { if gce.IsGCEError(err, "notFound") { @@ -2058,7 +2060,7 @@ func (gceCS *GCEControllerServer) getSnapshotByID(ctx context.Context, snapshotI return nil, fmt.Errorf("failed to generate snapshot entry: %w", err) } entries = []*csi.ListSnapshotsResponse_Entry{e} - case common.DiskImageType: + case parameters.DiskImageType: image, err := gceCS.CloudProvider.GetImage(ctx, project, key) if err != nil { if gce.IsGCEError(err, "notFound") { @@ -2396,7 +2398,7 @@ func getNumDefaultZonesInRegion(ctx context.Context, gceCS *GCEControllerServer, return ret, nil } -func paramsToVolumeContext(params common.DiskParameters) map[string]string { +func paramsToVolumeContext(params parameters.DiskParameters) map[string]string { context := map[string]string{} if params.ForceAttach { context[contextForceAttach] = "true" @@ -2414,7 +2416,7 @@ func extractVolumeContext(context map[string]string) (*PDCSIContext, error) { for key, val := range context { switch key { case contextForceAttach: - b, err := common.ConvertStringToBool(val) + b, err := convert.ConvertStringToBool(val) if err != nil { return nil, fmt.Errorf("bad volume context force attach: %w", err) } @@ -2424,7 +2426,7 @@ func extractVolumeContext(context map[string]string) (*PDCSIContext, error) { return info, nil } -func (gceCS *GCEControllerServer) generateCreateVolumeResponseWithVolumeId(disk *gce.CloudDisk, zones []string, params common.DiskParameters, dataCacheParams common.DataCacheParameters, enableDataCache bool, volumeId string) *csi.CreateVolumeResponse { +func (gceCS *GCEControllerServer) generateCreateVolumeResponseWithVolumeId(disk *gce.CloudDisk, zones []string, params parameters.DiskParameters, dataCacheParams parameters.DataCacheParameters, enableDataCache bool, volumeId string) *csi.CreateVolumeResponse { tops := []*csi.Topology{} for _, zone := range zones { top := &csi.Topology{ @@ -2454,7 +2456,7 @@ func (gceCS *GCEControllerServer) generateCreateVolumeResponseWithVolumeId(disk }, } // Set data cache volume context - if enableDataCache && dataCacheParams != (common.DataCacheParameters{}) { + if enableDataCache && dataCacheParams != (parameters.DataCacheParameters{}) { if createResp.Volume.VolumeContext == nil { createResp.Volume.VolumeContext = map[string]string{} } @@ -2527,7 +2529,7 @@ func getResourceId(resourceLink string) (string, error) { return strings.Join(elts[3:], "/"), nil } -func createRegionalDisk(ctx context.Context, cloudProvider gce.GCECompute, name string, zones []string, params common.DiskParameters, capacityRange *csi.CapacityRange, capBytes int64, snapshotID string, volumeContentSourceVolumeID string, multiWriter bool, accessMode string) (*gce.CloudDisk, error) { +func createRegionalDisk(ctx context.Context, cloudProvider gce.GCECompute, name string, zones []string, params parameters.DiskParameters, capacityRange *csi.CapacityRange, capBytes int64, snapshotID string, volumeContentSourceVolumeID string, multiWriter bool, accessMode string) (*gce.CloudDisk, error) { project := cloudProvider.GetDefaultProject() region, err := common.GetRegionFromZones(zones) if err != nil { @@ -2553,7 +2555,7 @@ func createRegionalDisk(ctx context.Context, cloudProvider gce.GCECompute, name return disk, nil } -func createSingleZoneDisk(ctx context.Context, cloudProvider gce.GCECompute, name string, zones []string, params common.DiskParameters, capacityRange *csi.CapacityRange, capBytes int64, snapshotID string, volumeContentSourceVolumeID string, multiWriter bool, accessMode string) (*gce.CloudDisk, error) { +func createSingleZoneDisk(ctx context.Context, cloudProvider gce.GCECompute, name string, zones []string, params parameters.DiskParameters, capacityRange *csi.CapacityRange, capBytes int64, snapshotID string, volumeContentSourceVolumeID string, multiWriter bool, accessMode string) (*gce.CloudDisk, error) { project := cloudProvider.GetDefaultProject() if len(zones) != 1 { return nil, fmt.Errorf("got wrong number of zones for zonal create volume: %v", len(zones)) diff --git a/pkg/gce-pd-csi-driver/controller_test.go b/pkg/gce-pd-csi-driver/controller_test.go index 35932fa15..e33d8e3c4 100644 --- a/pkg/gce-pd-csi-driver/controller_test.go +++ b/pkg/gce-pd-csi-driver/controller_test.go @@ -47,6 +47,7 @@ import ( "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/constants" gce "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/gce-cloud-provider/compute" gcecloudprovider "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/gce-cloud-provider/compute" + "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/parameters" ) const ( @@ -67,7 +68,7 @@ var ( RequiredBytes: common.GbToBytes(20), } stdParams = map[string]string{ - common.ParameterKeyType: stdDiskType, + parameters.ParameterKeyType: stdDiskType, } stdTopology = []*csi.Topology{ { @@ -109,7 +110,7 @@ func TestCreateSnapshotArguments(t *testing.T) { req: &csi.CreateSnapshotRequest{ Name: name, SourceVolumeId: testVolumeID, - Parameters: map[string]string{common.ParameterKeyStorageLocations: " US-WEST2"}, + Parameters: map[string]string{parameters.ParameterKeyStorageLocations: " US-WEST2"}, }, seedDisks: []*gce.CloudDisk{ createZonalCloudDisk(name), @@ -127,7 +128,7 @@ func TestCreateSnapshotArguments(t *testing.T) { req: &csi.CreateSnapshotRequest{ Name: name, SourceVolumeId: testVolumeID, - Parameters: map[string]string{common.ParameterKeyStorageLocations: " US-WEST2", common.ParameterKeySnapshotType: "images"}, + Parameters: map[string]string{parameters.ParameterKeyStorageLocations: " US-WEST2", parameters.ParameterKeySnapshotType: "images"}, }, seedDisks: []*gce.CloudDisk{ createZonalCloudDisk(name), @@ -189,7 +190,7 @@ func TestCreateSnapshotArguments(t *testing.T) { req: &csi.CreateSnapshotRequest{ Name: name, SourceVolumeId: testVolumeID, - Parameters: map[string]string{common.ParameterKeyStorageLocations: "bad-region"}, + Parameters: map[string]string{parameters.ParameterKeyStorageLocations: "bad-region"}, }, seedDisks: []*gce.CloudDisk{ createZonalCloudDisk(name), @@ -231,13 +232,13 @@ func TestCreateSnapshotArguments(t *testing.T) { req: &csi.CreateSnapshotRequest{ Name: name, SourceVolumeId: testRegionalID, - Parameters: map[string]string{common.ParameterKeyStorageLocations: " US-WEST2", common.ParameterKeySnapshotType: "images"}, + Parameters: map[string]string{parameters.ParameterKeyStorageLocations: " US-WEST2", parameters.ParameterKeySnapshotType: "images"}, }, seedDisks: []*gce.CloudDisk{ gce.CloudDiskFromV1(&compute.Disk{ Name: name, SelfLink: fmt.Sprintf("https://www.googleapis.com/compute/v1/projects/project/regions/country-region/name/%s", name), - Type: common.DiskTypeHdHA, + Type: parameters.DiskTypeHdHA, Region: "country-region", }), }, @@ -248,13 +249,13 @@ func TestCreateSnapshotArguments(t *testing.T) { req: &csi.CreateSnapshotRequest{ Name: name, SourceVolumeId: testRegionalID, - Parameters: map[string]string{common.ParameterKeyStorageLocations: " US-WEST2"}, + Parameters: map[string]string{parameters.ParameterKeyStorageLocations: " US-WEST2"}, }, seedDisks: []*gce.CloudDisk{ gce.CloudDiskFromV1(&compute.Disk{ Name: name, SelfLink: fmt.Sprintf("https://www.googleapis.com/compute/v1/projects/project/regions/country-region/name/%s", name), - Type: common.DiskTypeHdHA, + Type: parameters.DiskTypeHdHA, Region: "country-region", }), }, @@ -538,7 +539,7 @@ func TestListSnapshotsArguments(t *testing.T) { createReq := &csi.CreateSnapshotRequest{ Name: nameID, SourceVolumeId: volumeID, - Parameters: map[string]string{common.ParameterKeySnapshotType: common.DiskSnapshotType}, + Parameters: map[string]string{parameters.ParameterKeySnapshotType: parameters.DiskSnapshotType}, } _, err := gceDriver.cs.CreateSnapshot(context.Background(), createReq) if err != nil { @@ -552,7 +553,7 @@ func TestListSnapshotsArguments(t *testing.T) { createReq := &csi.CreateSnapshotRequest{ Name: nameID, SourceVolumeId: volumeID, - Parameters: map[string]string{common.ParameterKeySnapshotType: common.DiskImageType}, + Parameters: map[string]string{parameters.ParameterKeySnapshotType: parameters.DiskImageType}, } _, err := gceDriver.cs.CreateSnapshot(context.Background(), createReq) if err != nil { @@ -824,7 +825,7 @@ func TestCreateVolumeArguments(t *testing.T) { Name: name, CapacityRange: stdCapRange, VolumeCapabilities: stdVolCaps, - Parameters: map[string]string{common.ParameterKeyReplicationType: replicationTypeRegionalPD}, + Parameters: map[string]string{parameters.ParameterKeyReplicationType: replicationTypeRegionalPD}, AccessibilityRequirements: &csi.TopologyRequirement{ Preferred: []*csi.Topology{ { @@ -857,7 +858,7 @@ func TestCreateVolumeArguments(t *testing.T) { CapacityRange: stdCapRange, VolumeCapabilities: stdVolCaps, Parameters: map[string]string{ - common.ParameterKeyReplicationType: replicationTypeRegionalPD, + parameters.ParameterKeyReplicationType: replicationTypeRegionalPD, }, AccessibilityRequirements: &csi.TopologyRequirement{ Requisite: []*csi.Topology{ @@ -881,7 +882,7 @@ func TestCreateVolumeArguments(t *testing.T) { CapacityRange: stdCapRange, VolumeCapabilities: stdVolCaps, Parameters: map[string]string{ - common.ParameterKeyReplicationType: replicationTypeRegionalPD, + parameters.ParameterKeyReplicationType: replicationTypeRegionalPD, }, }, expVol: &csi.Volume{ @@ -905,7 +906,7 @@ func TestCreateVolumeArguments(t *testing.T) { Name: name, CapacityRange: stdCapRange, VolumeCapabilities: stdVolCaps, - Parameters: map[string]string{common.ParameterKeyType: common.DiskTypeHdHA}, + Parameters: map[string]string{parameters.ParameterKeyType: parameters.DiskTypeHdHA}, AccessibilityRequirements: &csi.TopologyRequirement{ Preferred: []*csi.Topology{ { @@ -938,7 +939,7 @@ func TestCreateVolumeArguments(t *testing.T) { CapacityRange: stdCapRange, VolumeCapabilities: stdVolCaps, Parameters: map[string]string{ - common.ParameterKeyType: common.DiskTypeHdHA, + parameters.ParameterKeyType: parameters.DiskTypeHdHA, }, AccessibilityRequirements: &csi.TopologyRequirement{ Requisite: []*csi.Topology{ @@ -962,7 +963,7 @@ func TestCreateVolumeArguments(t *testing.T) { CapacityRange: stdCapRange, VolumeCapabilities: stdVolCaps, Parameters: map[string]string{ - common.ParameterKeyType: common.DiskTypeHdHA, + parameters.ParameterKeyType: parameters.DiskTypeHdHA, }, }, expVol: &csi.Volume{ @@ -1026,7 +1027,7 @@ func TestCreateVolumeArguments(t *testing.T) { CapacityRange: stdCapRange, VolumeCapabilities: stdVolCaps, Parameters: map[string]string{ - common.ParameterKeyDiskEncryptionKmsKey: "projects/KMS_PROJECT_ID/locations/REGION/keyRings/KEY_RING/cryptoKeys/KEY", + parameters.ParameterKeyDiskEncryptionKmsKey: "projects/KMS_PROJECT_ID/locations/REGION/keyRings/KEY_RING/cryptoKeys/KEY", }, }, expVol: &csi.Volume{ @@ -1298,7 +1299,7 @@ func TestCreateVolumeArguments(t *testing.T) { }, }, Parameters: map[string]string{ - common.ParameterKeyType: "hyperdisk-balanced", + parameters.ParameterKeyType: "hyperdisk-balanced", }, }, expErrCode: codes.InvalidArgument, @@ -1319,7 +1320,7 @@ func TestCreateVolumeArguments(t *testing.T) { }, }, Parameters: map[string]string{ - common.ParameterKeyType: "hyperdisk-ml", + parameters.ParameterKeyType: "hyperdisk-ml", }, }, expVol: &csi.Volume{ @@ -1339,7 +1340,7 @@ func TestCreateVolumeArguments(t *testing.T) { VolumeCapabilities: stdVolCaps, Parameters: mergeParameters( stdParams, - map[string]string{common.ParameterKeyUseAllowedDiskTopology: "false"}, + map[string]string{parameters.ParameterKeyUseAllowedDiskTopology: "false"}, ), }, enableDiskTopology: true, @@ -1365,7 +1366,7 @@ func TestCreateVolumeArguments(t *testing.T) { VolumeCapabilities: stdVolCaps, Parameters: mergeParameters( stdParams, - map[string]string{common.ParameterKeyUseAllowedDiskTopology: "true"}, + map[string]string{parameters.ParameterKeyUseAllowedDiskTopology: "true"}, ), }, enableDiskTopology: true, @@ -1451,8 +1452,8 @@ func TestMultiZoneVolumeCreation(t *testing.T) { }, }, Parameters: map[string]string{ - common.ParameterKeyType: "hyperdisk-ml", - common.ParameterKeyEnableMultiZoneProvisioning: "true", + parameters.ParameterKeyType: "hyperdisk-ml", + parameters.ParameterKeyEnableMultiZoneProvisioning: "true", }, VolumeContentSource: &csi.VolumeContentSource{ Type: &csi.VolumeContentSource_Snapshot{ @@ -1492,8 +1493,8 @@ func TestMultiZoneVolumeCreation(t *testing.T) { }, }, Parameters: map[string]string{ - common.ParameterKeyType: "hyperdisk-ml", - common.ParameterKeyEnableMultiZoneProvisioning: "true", + parameters.ParameterKeyType: "hyperdisk-ml", + parameters.ParameterKeyEnableMultiZoneProvisioning: "true", }, VolumeContentSource: &csi.VolumeContentSource{ Type: &csi.VolumeContentSource_Snapshot{ @@ -1523,8 +1524,8 @@ func TestMultiZoneVolumeCreation(t *testing.T) { }, }, Parameters: map[string]string{ - common.ParameterKeyType: "hyperdisk-ml", - common.ParameterKeyEnableMultiZoneProvisioning: "true", + parameters.ParameterKeyType: "hyperdisk-ml", + parameters.ParameterKeyEnableMultiZoneProvisioning: "true", }, VolumeContentSource: &csi.VolumeContentSource{ Type: &csi.VolumeContentSource_Snapshot{ @@ -1570,8 +1571,8 @@ func TestMultiZoneVolumeCreation(t *testing.T) { }, }, Parameters: map[string]string{ - common.ParameterKeyType: "hyperdisk-ml", - common.ParameterKeyEnableMultiZoneProvisioning: "true", + parameters.ParameterKeyType: "hyperdisk-ml", + parameters.ParameterKeyEnableMultiZoneProvisioning: "true", }, AccessibilityRequirements: &csi.TopologyRequirement{ Requisite: []*csi.Topology{ @@ -1610,8 +1611,8 @@ func TestMultiZoneVolumeCreation(t *testing.T) { }, }, Parameters: map[string]string{ - common.ParameterKeyType: "hyperdisk-ml", - common.ParameterKeyEnableMultiZoneProvisioning: "true", + parameters.ParameterKeyType: "hyperdisk-ml", + parameters.ParameterKeyEnableMultiZoneProvisioning: "true", }, AccessibilityRequirements: &csi.TopologyRequirement{ Requisite: []*csi.Topology{ @@ -1647,8 +1648,8 @@ func TestMultiZoneVolumeCreation(t *testing.T) { }, }, Parameters: map[string]string{ - common.ParameterKeyType: "hyperdisk-balanced", - common.ParameterKeyEnableMultiZoneProvisioning: "true", + parameters.ParameterKeyType: "hyperdisk-balanced", + parameters.ParameterKeyEnableMultiZoneProvisioning: "true", }, AccessibilityRequirements: &csi.TopologyRequirement{ Requisite: []*csi.Topology{ @@ -1684,8 +1685,8 @@ func TestMultiZoneVolumeCreation(t *testing.T) { }, }, Parameters: map[string]string{ - common.ParameterKeyType: "hyperdisk-ml", - common.ParameterKeyEnableMultiZoneProvisioning: "true", + parameters.ParameterKeyType: "hyperdisk-ml", + parameters.ParameterKeyEnableMultiZoneProvisioning: "true", }, VolumeContentSource: &csi.VolumeContentSource{ Type: &csi.VolumeContentSource_Snapshot{ @@ -1713,8 +1714,8 @@ func TestMultiZoneVolumeCreation(t *testing.T) { }, }, Parameters: map[string]string{ - common.ParameterKeyType: "hyperdisk-ml", - common.ParameterKeyEnableMultiZoneProvisioning: "true", + parameters.ParameterKeyType: "hyperdisk-ml", + parameters.ParameterKeyEnableMultiZoneProvisioning: "true", }, VolumeContentSource: &csi.VolumeContentSource{ Type: &csi.VolumeContentSource_Snapshot{ @@ -1761,8 +1762,8 @@ func TestMultiZoneVolumeCreation(t *testing.T) { }, }, Parameters: map[string]string{ - common.ParameterKeyType: "hyperdisk-ml", - common.ParameterKeyEnableMultiZoneProvisioning: "true", + parameters.ParameterKeyType: "hyperdisk-ml", + parameters.ParameterKeyEnableMultiZoneProvisioning: "true", }, AccessibilityRequirements: &csi.TopologyRequirement{ Requisite: []*csi.Topology{ @@ -1796,11 +1797,11 @@ func TestMultiZoneVolumeCreation(t *testing.T) { gceDriver.cs.fallbackRequisiteZones = tc.fallbackZones if tc.req.VolumeContentSource.GetType() != nil { - snapshotParams, err := common.ExtractAndDefaultSnapshotParameters(nil, gceDriver.name, nil) + snapshotParams, err := parameters.ExtractAndDefaultSnapshotParameters(nil, gceDriver.name, nil) if err != nil { t.Errorf("Got error extracting snapshot parameters: %v", err) } - if snapshotParams.SnapshotType == common.DiskSnapshotType { + if snapshotParams.SnapshotType == parameters.DiskSnapshotType { fcp.CreateSnapshot(context.Background(), project, meta.ZonalKey(name, constants.MultiZoneValue), name, snapshotParams) } else { t.Fatalf("No volume source mentioned in snapshot parameters %v", snapshotParams) @@ -1928,7 +1929,7 @@ func TestCreateVolumeMultiWriterOrAccessMode(t *testing.T) { }, }, Parameters: map[string]string{ - common.ParameterKeyType: "pd-balanced", + parameters.ParameterKeyType: "pd-balanced", }, }, expMultiWriter: false, @@ -1948,7 +1949,7 @@ func TestCreateVolumeMultiWriterOrAccessMode(t *testing.T) { }, }, Parameters: map[string]string{ - common.ParameterKeyType: "pd-balanced", + parameters.ParameterKeyType: "pd-balanced", }, }, expMultiWriter: true, @@ -1968,7 +1969,7 @@ func TestCreateVolumeMultiWriterOrAccessMode(t *testing.T) { }, }, Parameters: map[string]string{ - common.ParameterKeyType: "hyperdisk-balanced", + parameters.ParameterKeyType: "hyperdisk-balanced", }, }, expAccessMode: constants.GCEReadWriteManyAccessMode, @@ -1988,7 +1989,7 @@ func TestCreateVolumeMultiWriterOrAccessMode(t *testing.T) { }, }, Parameters: map[string]string{ - common.ParameterKeyType: "hyperdisk-balanced", + parameters.ParameterKeyType: "hyperdisk-balanced", }, }, expAccessMode: constants.GCEReadWriteOnceAccessMode, @@ -2008,7 +2009,7 @@ func TestCreateVolumeMultiWriterOrAccessMode(t *testing.T) { }, }, Parameters: map[string]string{ - common.ParameterKeyType: "hyperdisk-balanced", + parameters.ParameterKeyType: "hyperdisk-balanced", }, }, expErrCode: codes.InvalidArgument, @@ -2100,7 +2101,7 @@ func (cloud *FakeCloudProviderInsertDiskErr) AddDiskForErr(volKey *meta.Key, err cloud.insertDiskErrors[volKey.String()] = err } -func (cloud *FakeCloudProviderInsertDiskErr) InsertDisk(ctx context.Context, project string, volKey *meta.Key, params common.DiskParameters, capBytes int64, capacityRange *csi.CapacityRange, replicaZones []string, snapshotID string, volumeContentSourceVolumeID string, multiWriter bool, accessMode string) error { +func (cloud *FakeCloudProviderInsertDiskErr) InsertDisk(ctx context.Context, project string, volKey *meta.Key, params parameters.DiskParameters, capBytes int64, capacityRange *csi.CapacityRange, replicaZones []string, snapshotID string, volumeContentSourceVolumeID string, multiWriter bool, accessMode string) error { if err, ok := cloud.insertDiskErrors[volKey.String()]; ok { return err } @@ -2132,8 +2133,8 @@ func TestMultiZoneVolumeCreationErrHandling(t *testing.T) { }, }, Parameters: map[string]string{ - common.ParameterKeyType: "hyperdisk-ml", - common.ParameterKeyEnableMultiZoneProvisioning: "true", + parameters.ParameterKeyType: "hyperdisk-ml", + parameters.ParameterKeyEnableMultiZoneProvisioning: "true", }, AccessibilityRequirements: &csi.TopologyRequirement{ Requisite: []*csi.Topology{ @@ -2179,8 +2180,8 @@ func TestMultiZoneVolumeCreationErrHandling(t *testing.T) { }, }, Parameters: map[string]string{ - common.ParameterKeyType: "hyperdisk-ml", - common.ParameterKeyEnableMultiZoneProvisioning: "true", + parameters.ParameterKeyType: "hyperdisk-ml", + parameters.ParameterKeyEnableMultiZoneProvisioning: "true", }, AccessibilityRequirements: &csi.TopologyRequirement{ Requisite: []*csi.Topology{ @@ -2282,9 +2283,9 @@ func TestCreateVolumeWithVolumeAttributeClassParameters(t *testing.T) { }, }, Parameters: map[string]string{ - common.ParameterKeyType: "hyperdisk-balanced", - common.ParameterKeyProvisionedIOPSOnCreate: "10000", - common.ParameterKeyProvisionedThroughputOnCreate: "500Mi", + parameters.ParameterKeyType: "hyperdisk-balanced", + parameters.ParameterKeyProvisionedIOPSOnCreate: "10000", + parameters.ParameterKeyProvisionedThroughputOnCreate: "500Mi", }, AccessibilityRequirements: &csi.TopologyRequirement{ Preferred: []*csi.Topology{ @@ -2315,9 +2316,9 @@ func TestCreateVolumeWithVolumeAttributeClassParameters(t *testing.T) { }, }, Parameters: map[string]string{ - common.ParameterKeyType: "pd-ssd", - common.ParameterKeyProvisionedIOPSOnCreate: "10000", - common.ParameterKeyProvisionedThroughputOnCreate: "500Mi", + parameters.ParameterKeyType: "pd-ssd", + parameters.ParameterKeyProvisionedIOPSOnCreate: "10000", + parameters.ParameterKeyProvisionedThroughputOnCreate: "500Mi", }, AccessibilityRequirements: &csi.TopologyRequirement{ Preferred: []*csi.Topology{ @@ -2385,7 +2386,7 @@ func TestVolumeModifyOperation(t *testing.T) { name string req *csi.ControllerModifyVolumeRequest diskType string - params *common.DiskParameters + params *parameters.DiskParameters expIops int64 expThroughput int64 expErrMessage string @@ -2397,7 +2398,7 @@ func TestVolumeModifyOperation(t *testing.T) { MutableParameters: map[string]string{"iops": "20000", "throughput": "600Mi"}, }, diskType: "hyperdisk-balanced", - params: &common.DiskParameters{ + params: ¶meters.DiskParameters{ DiskType: "hyperdisk-balanced", ProvisionedIOPSOnCreate: 10000, ProvisionedThroughputOnCreate: 500, @@ -2413,7 +2414,7 @@ func TestVolumeModifyOperation(t *testing.T) { MutableParameters: map[string]string{"iops": "0", "throughput": "0Mi"}, }, diskType: "hyperdisk-balanced", - params: &common.DiskParameters{ + params: ¶meters.DiskParameters{ DiskType: "hyperdisk-balanced", ProvisionedIOPSOnCreate: 10000, ProvisionedThroughputOnCreate: 500, @@ -2429,7 +2430,7 @@ func TestVolumeModifyOperation(t *testing.T) { MutableParameters: map[string]string{"iops": "20000", "throughput": "600Mi"}, }, diskType: "pd-ssd", - params: &common.DiskParameters{ + params: ¶meters.DiskParameters{ DiskType: "pd-ssd", }, expIops: 0, @@ -2503,7 +2504,7 @@ func (cloud *FakeCloudProviderUpdateDiskErr) AddDiskForErr(volKey *meta.Key, err cloud.updateDiskErrors[volKey.String()] = err } -func (cloud *FakeCloudProviderUpdateDiskErr) UpdateDisk(ctx context.Context, project string, volKey *meta.Key, existingDisk *gcecloudprovider.CloudDisk, params common.ModifyVolumeParameters) error { +func (cloud *FakeCloudProviderUpdateDiskErr) UpdateDisk(ctx context.Context, project string, volKey *meta.Key, existingDisk *gcecloudprovider.CloudDisk, params parameters.ModifyVolumeParameters) error { if err, ok := cloud.updateDiskErrors[volKey.String()]; ok { return err } @@ -2538,9 +2539,9 @@ func TestVolumeModifyErrorHandling(t *testing.T) { createReq: &csi.CreateVolumeRequest{ Name: name, Parameters: map[string]string{ - common.ParameterKeyType: "hyperdisk-balanced", - common.ParameterKeyProvisionedIOPSOnCreate: "3000", - common.ParameterKeyProvisionedThroughputOnCreate: "150Mi", + parameters.ParameterKeyType: "hyperdisk-balanced", + parameters.ParameterKeyProvisionedIOPSOnCreate: "3000", + parameters.ParameterKeyProvisionedThroughputOnCreate: "150Mi", }, VolumeCapabilities: stdVolCaps, AccessibilityRequirements: &csi.TopologyRequirement{ @@ -2574,9 +2575,9 @@ func TestVolumeModifyErrorHandling(t *testing.T) { createReq: &csi.CreateVolumeRequest{ Name: name, Parameters: map[string]string{ - common.ParameterKeyType: "hyperdisk-balanced", - common.ParameterKeyProvisionedIOPSOnCreate: "3000", - common.ParameterKeyProvisionedThroughputOnCreate: "150Mi", + parameters.ParameterKeyType: "hyperdisk-balanced", + parameters.ParameterKeyProvisionedIOPSOnCreate: "3000", + parameters.ParameterKeyProvisionedThroughputOnCreate: "150Mi", }, VolumeCapabilities: stdVolCaps, AccessibilityRequirements: &csi.TopologyRequirement{ @@ -3080,14 +3081,14 @@ func TestCreateVolumeWithVolumeSourceFromSnapshot(t *testing.T) { name: "success with data source of snapshot type", project: "test-project", volKey: meta.ZonalKey("my-disk", zone), - snapshotType: common.DiskSnapshotType, + snapshotType: parameters.DiskSnapshotType, snapshotOnCloud: true, }, { name: "fail with data source of snapshot type that doesn't exist", project: "test-project", volKey: meta.ZonalKey("my-disk", zone), - snapshotType: common.DiskSnapshotType, + snapshotType: parameters.DiskSnapshotType, snapshotOnCloud: false, expErrCode: codes.NotFound, }, @@ -3095,14 +3096,14 @@ func TestCreateVolumeWithVolumeSourceFromSnapshot(t *testing.T) { name: "success with data source of snapshot type", project: "test-project", volKey: meta.ZonalKey("my-disk", zone), - snapshotType: common.DiskImageType, + snapshotType: parameters.DiskImageType, snapshotOnCloud: true, }, { name: "fail with data source of snapshot type that doesn't exist", project: "test-project", volKey: meta.ZonalKey("my-disk", zone), - snapshotType: common.DiskImageType, + snapshotType: parameters.DiskImageType, snapshotOnCloud: false, expErrCode: codes.NotFound, }, @@ -3114,7 +3115,7 @@ func TestCreateVolumeWithVolumeSourceFromSnapshot(t *testing.T) { // Setup new driver each time so no interference gceDriver := initGCEDriver(t, nil, &GCEControllerServerArgs{}) - snapshotParams, err := common.ExtractAndDefaultSnapshotParameters(nil, gceDriver.name, nil) + snapshotParams, err := parameters.ExtractAndDefaultSnapshotParameters(nil, gceDriver.name, nil) if err != nil { t.Errorf("Got error extracting snapshot parameters: %v", err) } @@ -3122,12 +3123,12 @@ func TestCreateVolumeWithVolumeSourceFromSnapshot(t *testing.T) { // Start Test var snapshotID string switch tc.snapshotType { - case common.DiskSnapshotType: + case parameters.DiskSnapshotType: snapshotID = testSnapshotID if tc.snapshotOnCloud { gceDriver.cs.CloudProvider.CreateSnapshot(context.Background(), tc.project, tc.volKey, name, snapshotParams) } - case common.DiskImageType: + case parameters.DiskImageType: snapshotID = testImageID if tc.snapshotOnCloud { gceDriver.cs.CloudProvider.CreateImage(context.Background(), tc.project, tc.volKey, name, snapshotParams) @@ -3193,7 +3194,7 @@ func TestCloningLocationRequirements(t *testing.T) { sourceVolumeID: testZonalVolumeSourceID, requestCapacityRange: stdCapRange, reqParameters: map[string]string{ - common.ParameterKeyReplicationType: replicationTypeNone, + parameters.ParameterKeyReplicationType: replicationTypeNone, }, cloneIsRegional: false, expectedLocationRequirements: &locationRequirements{srcVolRegion: region, srcVolZone: zone, srcIsRegional: false, cloneIsRegional: false}, @@ -3204,7 +3205,7 @@ func TestCloningLocationRequirements(t *testing.T) { sourceVolumeID: testRegionalVolumeSourceID, requestCapacityRange: stdCapRange, reqParameters: map[string]string{ - common.ParameterKeyReplicationType: replicationTypeRegionalPD, + parameters.ParameterKeyReplicationType: replicationTypeRegionalPD, }, cloneIsRegional: true, expectedLocationRequirements: &locationRequirements{srcVolRegion: region, srcVolZone: "", srcIsRegional: true, cloneIsRegional: true}, @@ -3215,7 +3216,7 @@ func TestCloningLocationRequirements(t *testing.T) { sourceVolumeID: testZonalVolumeSourceID, requestCapacityRange: stdCapRange, reqParameters: map[string]string{ - common.ParameterKeyType: common.DiskTypeHdHA, + parameters.ParameterKeyType: parameters.DiskTypeHdHA, }, cloneIsRegional: true, expectedLocationRequirements: &locationRequirements{srcVolRegion: region, srcVolZone: zone, srcIsRegional: false, cloneIsRegional: true}, @@ -3226,7 +3227,7 @@ func TestCloningLocationRequirements(t *testing.T) { nilVolumeContentSource: true, requestCapacityRange: stdCapRange, reqParameters: map[string]string{ - common.ParameterKeyReplicationType: replicationTypeRegionalPD, + parameters.ParameterKeyReplicationType: replicationTypeRegionalPD, }, cloneIsRegional: true, expectedLocationRequirements: nil, @@ -3237,7 +3238,7 @@ func TestCloningLocationRequirements(t *testing.T) { sourceVolumeID: fmt.Sprintf("projects/%s/disks/%s", project, testSourceVolumeName), requestCapacityRange: stdCapRange, reqParameters: map[string]string{ - common.ParameterKeyReplicationType: replicationTypeNone, + parameters.ParameterKeyReplicationType: replicationTypeNone, }, cloneIsRegional: false, expectedLocationRequirements: nil, @@ -3283,12 +3284,12 @@ func TestCreateVolumeWithVolumeSourceFromVolume(t *testing.T) { testRegionalVolumeSourceID := fmt.Sprintf("projects/%s/regions/%s/disks/%s", project, region, testSourceVolumeName) testSecondZonalVolumeSourceID := fmt.Sprintf("projects/%s/zones/%s/disks/%s", project, "different-zone1", testSourceVolumeName) zonalParams := map[string]string{ - common.ParameterKeyType: stdDiskType, common.ParameterKeyReplicationType: replicationTypeNone, - common.ParameterKeyDiskEncryptionKmsKey: "encryption-key", + parameters.ParameterKeyType: stdDiskType, parameters.ParameterKeyReplicationType: replicationTypeNone, + parameters.ParameterKeyDiskEncryptionKmsKey: "encryption-key", } regionalParams := map[string]string{ - common.ParameterKeyType: stdDiskType, common.ParameterKeyReplicationType: replicationTypeRegionalPD, - common.ParameterKeyDiskEncryptionKmsKey: "encryption-key", + parameters.ParameterKeyType: stdDiskType, parameters.ParameterKeyReplicationType: replicationTypeRegionalPD, + parameters.ParameterKeyDiskEncryptionKmsKey: "encryption-key", } requisiteTopology := []*csi.Topology{ { @@ -3422,10 +3423,10 @@ func TestCreateVolumeWithVolumeSourceFromVolume(t *testing.T) { sourceCapacityRange: stdCapRange, enableStoragePools: true, reqParameters: map[string]string{ - common.ParameterKeyType: "hyperdisk-balanced", - common.ParameterKeyReplicationType: replicationTypeNone, - common.ParameterKeyDiskEncryptionKmsKey: "encryption-key", - common.ParameterKeyStoragePools: "projects/test-project/zones/country-region-zone/storagePools/storagePool-1", + parameters.ParameterKeyType: "hyperdisk-balanced", + parameters.ParameterKeyReplicationType: replicationTypeNone, + parameters.ParameterKeyDiskEncryptionKmsKey: "encryption-key", + parameters.ParameterKeyStoragePools: "projects/test-project/zones/country-region-zone/storagePools/storagePool-1", }, sourceReqParameters: zonalParams, sourceTopology: &csi.TopologyRequirement{ @@ -3712,7 +3713,7 @@ func TestCreateVolumeWithVolumeSourceFromVolume(t *testing.T) { sourceCapacityRange: stdCapRange, reqParameters: zonalParams, sourceReqParameters: map[string]string{ - common.ParameterKeyType: "different-type", + parameters.ParameterKeyType: "different-type", }, sourceTopology: &csi.TopologyRequirement{ Requisite: requisiteTopology, @@ -3732,8 +3733,8 @@ func TestCreateVolumeWithVolumeSourceFromVolume(t *testing.T) { sourceCapacityRange: stdCapRange, reqParameters: zonalParams, sourceReqParameters: map[string]string{ - common.ParameterKeyType: stdDiskType, common.ParameterKeyReplicationType: replicationTypeNone, - common.ParameterKeyDiskEncryptionKmsKey: "different-encryption-key", + parameters.ParameterKeyType: stdDiskType, parameters.ParameterKeyReplicationType: replicationTypeNone, + parameters.ParameterKeyDiskEncryptionKmsKey: "different-encryption-key", }, sourceTopology: &csi.TopologyRequirement{ Requisite: requisiteTopology, @@ -5681,9 +5682,9 @@ func TestCreateConfidentialVolume(t *testing.T) { CapacityRange: stdCapRange, VolumeCapabilities: stdVolCaps, Parameters: map[string]string{ - common.ParameterKeyEnableConfidentialCompute: "true", - common.ParameterKeyDiskEncryptionKmsKey: testDiskEncryptionKmsKey, - common.ParameterKeyType: "hyperdisk-balanced", + parameters.ParameterKeyEnableConfidentialCompute: "true", + parameters.ParameterKeyDiskEncryptionKmsKey: testDiskEncryptionKmsKey, + parameters.ParameterKeyType: "hyperdisk-balanced", }, VolumeContentSource: &csi.VolumeContentSource{ Type: &csi.VolumeContentSource_Snapshot{ @@ -5703,8 +5704,8 @@ func TestCreateConfidentialVolume(t *testing.T) { CapacityRange: stdCapRange, VolumeCapabilities: stdVolCaps, Parameters: map[string]string{ - common.ParameterKeyEnableConfidentialCompute: "false", - common.ParameterKeyType: "hyperdisk-balanced", + parameters.ParameterKeyEnableConfidentialCompute: "false", + parameters.ParameterKeyType: "hyperdisk-balanced", }, VolumeContentSource: &csi.VolumeContentSource{ Type: &csi.VolumeContentSource_Snapshot{ @@ -5726,11 +5727,11 @@ func TestCreateConfidentialVolume(t *testing.T) { gceDriver := initGCEDriverWithCloudProvider(t, fcp, &GCEControllerServerArgs{}) if tc.req.VolumeContentSource.GetType() != nil { - snapshotParams, err := common.ExtractAndDefaultSnapshotParameters(nil, gceDriver.name, nil) + snapshotParams, err := parameters.ExtractAndDefaultSnapshotParameters(nil, gceDriver.name, nil) if err != nil { t.Errorf("Got error extracting snapshot parameters: %v", err) } - if snapshotParams.SnapshotType == common.DiskSnapshotType { + if snapshotParams.SnapshotType == parameters.DiskSnapshotType { fcp.CreateSnapshot(context.Background(), project, tc.volKey, name, snapshotParams) } else { t.Fatalf("No volume source mentioned in snapshot parameters %v", snapshotParams) @@ -5753,7 +5754,7 @@ func TestCreateConfidentialVolume(t *testing.T) { if err != nil { t.Fatalf("Get Disk failed for created disk with error: %v", err) } - val, ok := tc.req.Parameters[common.ParameterKeyEnableConfidentialCompute] + val, ok := tc.req.Parameters[parameters.ParameterKeyEnableConfidentialCompute] if ok && val != strconv.FormatBool(createdDisk.GetEnableConfidentialCompute()) { t.Fatalf("Confidential disk parameter does not match with created disk: %v Got error %v", createdDisk.GetEnableConfidentialCompute(), err) } diff --git a/pkg/gce-pd-csi-driver/utils.go b/pkg/gce-pd-csi-driver/utils.go index 8c14117ec..604644d99 100644 --- a/pkg/gce-pd-csi-driver/utils.go +++ b/pkg/gce-pd-csi-driver/utils.go @@ -28,6 +28,7 @@ import ( "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/common" "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/constants" + "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/parameters" ) const ( @@ -156,7 +157,7 @@ func validateAccessMode(am *csi.VolumeCapability_AccessMode) error { return nil } -func validateStoragePools(req *csi.CreateVolumeRequest, params common.DiskParameters, project string) error { +func validateStoragePools(req *csi.CreateVolumeRequest, params parameters.DiskParameters, project string) error { storagePoolsEnabled := params.StoragePools != nil if !storagePoolsEnabled || req == nil { return nil @@ -192,8 +193,8 @@ func validateStoragePools(req *csi.CreateVolumeRequest, params common.DiskParame return nil } -func validateStoragePoolZones(req *csi.CreateVolumeRequest, storagePools []common.StoragePool) error { - storagePoolZones, err := common.StoragePoolZones(storagePools) +func validateStoragePoolZones(req *csi.CreateVolumeRequest, storagePools []parameters.StoragePool) error { + storagePoolZones, err := parameters.StoragePoolZones(storagePools) if err != nil { return err } @@ -207,7 +208,7 @@ func validateStoragePoolZones(req *csi.CreateVolumeRequest, storagePools []commo return nil } -func validateStoragePoolProjects(project string, storagePools []common.StoragePool) error { +func validateStoragePoolProjects(project string, storagePools []parameters.StoragePool) error { spProjects := sets.String{} for _, sp := range storagePools { if sp.Project != project { diff --git a/pkg/gce-pd-csi-driver/utils_test.go b/pkg/gce-pd-csi-driver/utils_test.go index 92fa034b9..5472a7f8a 100644 --- a/pkg/gce-pd-csi-driver/utils_test.go +++ b/pkg/gce-pd-csi-driver/utils_test.go @@ -27,6 +27,7 @@ import ( "github.com/google/go-cmp/cmp" "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/common" "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/constants" + "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/parameters" ) var ( @@ -332,7 +333,7 @@ func TestValidateStoragePools(t *testing.T) { }, params: common.DiskParameters{ DiskType: "hyperdisk-balanced", - StoragePools: []common.StoragePool{ + StoragePools: []parameters.StoragePool{ { Project: "test-project", Zone: "us-central1-a", @@ -358,7 +359,7 @@ func TestValidateStoragePools(t *testing.T) { }, params: common.DiskParameters{ DiskType: "pd-balanced", - StoragePools: []common.StoragePool{ + StoragePools: []parameters.StoragePool{ { Project: "test-project", Zone: "us-central1-a", @@ -384,7 +385,7 @@ func TestValidateStoragePools(t *testing.T) { params: common.DiskParameters{ DiskType: "hyperdisk-balanced", ReplicationType: "regional-pd", - StoragePools: []common.StoragePool{ + StoragePools: []parameters.StoragePool{ { Project: "test-project", Zone: "us-central1-a", @@ -409,7 +410,7 @@ func TestValidateStoragePools(t *testing.T) { }, params: common.DiskParameters{ DiskType: "hyperdisk-balanced-high-availability", - StoragePools: []common.StoragePool{ + StoragePools: []parameters.StoragePool{ { Project: "test-project", Zone: "us-central1-a", @@ -435,7 +436,7 @@ func TestValidateStoragePools(t *testing.T) { }, params: common.DiskParameters{ DiskType: "hyperdisk-balanced-high-availability", - StoragePools: []common.StoragePool{ + StoragePools: []parameters.StoragePool{ { Project: "test-project", Zone: "us-central1-a", @@ -467,7 +468,7 @@ func TestValidateStoragePools(t *testing.T) { }, params: common.DiskParameters{ DiskType: "hyperdisk-balanced", - StoragePools: []common.StoragePool{ + StoragePools: []parameters.StoragePool{ { Project: "test-project", Zone: "us-central1-a", @@ -507,7 +508,7 @@ func TestValidateStoragePools(t *testing.T) { }, params: common.DiskParameters{ DiskType: "hyperdisk-balanced", - StoragePools: []common.StoragePool{ + StoragePools: []parameters.StoragePool{ { Project: "test-project", Zone: "us-central1-a", @@ -550,7 +551,7 @@ func TestValidateStoragePools(t *testing.T) { }, params: common.DiskParameters{ DiskType: "hyperdisk-balanced", - StoragePools: []common.StoragePool{ + StoragePools: []parameters.StoragePool{ { Project: "test-project", Zone: "us-central1-a", @@ -593,7 +594,7 @@ func TestValidateStoragePools(t *testing.T) { }, params: common.DiskParameters{ DiskType: "hyperdisk-balanced", - StoragePools: []common.StoragePool{ + StoragePools: []parameters.StoragePool{ { Project: "test-project", Zone: "us-central1-a", @@ -634,7 +635,7 @@ func TestValidateStoragePoolZones(t *testing.T) { testCases := []struct { name string req *csi.CreateVolumeRequest - storagePools []common.StoragePool + storagePools []parameters.StoragePool expErr error }{ { @@ -654,7 +655,7 @@ func TestValidateStoragePoolZones(t *testing.T) { }, }, }, - storagePools: []common.StoragePool{ + storagePools: []parameters.StoragePool{ { Project: "test-project", Zone: "us-central1-a", @@ -683,7 +684,7 @@ func TestValidateStoragePoolZones(t *testing.T) { }, }, }, - storagePools: []common.StoragePool{ + storagePools: []parameters.StoragePool{ { Project: "test-project", Zone: "us-central1-a", @@ -710,7 +711,7 @@ func TestValidateStoragePoolZones(t *testing.T) { }, }, }, - storagePools: []common.StoragePool{ + storagePools: []parameters.StoragePool{ { Project: "test-project", Zone: "us-central1-b", @@ -743,7 +744,7 @@ func TestValidateStoragePoolZones(t *testing.T) { }, }, }, - storagePools: []common.StoragePool{ + storagePools: []parameters.StoragePool{ { Project: "test-project", Zone: "us-central1-a", diff --git a/pkg/metrics/metadata.go b/pkg/metrics/metadata.go index a2e41e81a..1dd6eead9 100644 --- a/pkg/metrics/metadata.go +++ b/pkg/metrics/metadata.go @@ -4,8 +4,8 @@ import ( "context" "strconv" - "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/common" gce "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/gce-cloud-provider/compute" + "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/parameters" ) const ( @@ -35,7 +35,7 @@ func MetadataFromContext(ctx context.Context) *RequestMetadata { return requestMetadata } -func UpdateRequestMetadataFromParams(ctx context.Context, params common.DiskParameters) { +func UpdateRequestMetadataFromParams(ctx context.Context, params parameters.DiskParameters) { metadata := MetadataFromContext(ctx) if metadata != nil { metadata.diskType = params.DiskType diff --git a/pkg/common/parameters.go b/pkg/parameters/parameters.go similarity index 87% rename from pkg/common/parameters.go rename to pkg/parameters/parameters.go index 422d07259..2c92bc29b 100644 --- a/pkg/common/parameters.go +++ b/pkg/parameters/parameters.go @@ -14,15 +14,17 @@ See the License for the specific language governing permissions and limitations under the License. */ -package common +package parameters import ( "fmt" "strconv" "strings" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/klog/v2" "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/constants" + "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/convert" ) const ( @@ -51,14 +53,8 @@ const ( ParameterKeyStorageLocations = "storage-locations" ParameterKeySnapshotType = "snapshot-type" ParameterKeyImageFamily = "image-family" - DiskSnapshotType = "snapshots" - DiskImageType = "images" replicationTypeNone = "none" - // Parameters for AvailabilityClass - ParameterNoAvailabilityClass = "none" - ParameterRegionalHardFailoverClass = "regional-hard-failover" - // Keys for PV and PVC parameters as reported by external-provisioner ParameterKeyPVCName = "csi.storage.k8s.io/pvc/name" ParameterKeyPVCNamespace = "csi.storage.k8s.io/pvc/namespace" @@ -75,6 +71,10 @@ const ( ParameterKeyVolumeSnapshotNamespace = "csi.storage.k8s.io/volumesnapshot/namespace" ParameterKeyVolumeSnapshotContentName = "csi.storage.k8s.io/volumesnapshotcontent/name" + // Parameters for AvailabilityClass + ParameterNoAvailabilityClass = "none" + ParameterRegionalHardFailoverClass = "regional-hard-failover" + // Keys for tags to put in the provisioned snapshot description tagKeyCreatedForSnapshotName = "kubernetes.io/created-for/volumesnapshot/name" tagKeyCreatedForSnapshotNamespace = "kubernetes.io/created-for/volumesnapshot/namespace" @@ -85,8 +85,19 @@ const ( DiskTypeHdT = "hyperdisk-throughput" DiskTypeHdE = "hyperdisk-extreme" DiskTypeHdML = "hyperdisk-ml" + + // Parameters for VolumeSnapshotClass + DiskSnapshotType = "snapshots" + DiskImageType = "images" ) +type StoragePool struct { + Project string + Zone string + Name string + ResourceName string +} + type DataCacheParameters struct { // Values: {string} in int64 form // Default: "" @@ -155,13 +166,6 @@ type SnapshotParameters struct { ResourceTags map[string]string } -type StoragePool struct { - Project string - Zone string - Name string - ResourceName string -} - type ParameterProcessor struct { DriverName string EnableStoragePools bool @@ -231,7 +235,7 @@ func (pp *ParameterProcessor) ExtractAndDefaultParameters(parameters map[string] case ParameterKeyPVName: p.Tags[tagKeyCreatedForVolumeName] = v case ParameterKeyLabels: - paramLabels, err := ConvertLabelsStringToMap(v) + paramLabels, err := convert.ConvertLabelsStringToMap(v) if err != nil { return p, d, fmt.Errorf("parameters contain invalid labels parameter: %w", err) } @@ -240,13 +244,13 @@ func (pp *ParameterProcessor) ExtractAndDefaultParameters(parameters map[string] p.Labels[labelKey] = labelValue } case ParameterKeyProvisionedIOPSOnCreate: - paramProvisionedIOPSOnCreate, err := ConvertStringToInt64(v) + paramProvisionedIOPSOnCreate, err := convert.ConvertStringToInt64(v) if err != nil { return p, d, fmt.Errorf("parameters contain invalid provisionedIOPSOnCreate parameter: %w", err) } p.ProvisionedIOPSOnCreate = paramProvisionedIOPSOnCreate case ParameterKeyProvisionedThroughputOnCreate: - paramProvisionedThroughputOnCreate, err := ConvertMiStringToInt64(v) + paramProvisionedThroughputOnCreate, err := convert.ConvertMiStringToInt64(v) if err != nil { return p, d, fmt.Errorf("parameters contain invalid provisionedThroughputOnCreate parameter: %w", err) } @@ -255,7 +259,7 @@ func (pp *ParameterProcessor) ExtractAndDefaultParameters(parameters map[string] } p.ProvisionedThroughputOnCreate = paramProvisionedThroughputOnCreate case ParameterAvailabilityClass: - paramAvailabilityClass, err := ConvertStringToAvailabilityClass(v) + paramAvailabilityClass, err := convertStringToAvailabilityClass(v) if err != nil { return p, d, fmt.Errorf("parameters contain invalid availability class parameter: %w", err) } @@ -263,7 +267,7 @@ func (pp *ParameterProcessor) ExtractAndDefaultParameters(parameters map[string] p.ForceAttach = true } case ParameterKeyEnableConfidentialCompute: - paramEnableConfidentialCompute, err := ConvertStringToBool(v) + paramEnableConfidentialCompute, err := convert.ConvertStringToBool(v) if err != nil { return p, d, fmt.Errorf("parameters contain invalid value for enable-confidential-storage parameter: %w", err) } @@ -290,7 +294,7 @@ func (pp *ParameterProcessor) ExtractAndDefaultParameters(parameters map[string] return p, d, fmt.Errorf("data caching enabled: %v; parameters contains invalid option %q", enableDataCache, ParameterKeyDataCacheSize) } - paramDataCacheSize, err := ConvertGiStringToInt64(v) + paramDataCacheSize, err := convert.ConvertGiStringToInt64(v) if err != nil { return p, d, fmt.Errorf("parameters contain invalid dataCacheSize parameter: %w", err) } @@ -314,7 +318,7 @@ func (pp *ParameterProcessor) ExtractAndDefaultParameters(parameters map[string] if !pp.EnableMultiZone { return p, d, fmt.Errorf("parameters contains invalid option %q", ParameterKeyEnableMultiZoneProvisioning) } - paramEnableMultiZoneProvisioning, err := ConvertStringToBool(v) + paramEnableMultiZoneProvisioning, err := convert.ConvertStringToBool(v) if err != nil { return p, d, fmt.Errorf("parameters contain invalid value for %s parameter: %w", ParameterKeyEnableMultiZoneProvisioning, err) } @@ -333,7 +337,7 @@ func (pp *ParameterProcessor) ExtractAndDefaultParameters(parameters map[string] continue } - paramUseAllowedDiskTopology, err := ConvertStringToBool(v) + paramUseAllowedDiskTopology, err := convert.ConvertStringToBool(v) if err != nil { klog.Warningf("failed to convert %s parameter with value %q to bool: %v", ParameterKeyUseAllowedDiskTopology, v, err) continue @@ -386,7 +390,7 @@ func ExtractAndDefaultSnapshotParameters(parameters map[string]string, driverNam case ParameterKeyVolumeSnapshotContentName: p.Tags[tagKeyCreatedForSnapshotContentName] = v case ParameterKeyLabels: - paramLabels, err := ConvertLabelsStringToMap(v) + paramLabels, err := convert.ConvertLabelsStringToMap(v) if err != nil { return p, fmt.Errorf("parameters contain invalid labels parameter: %w", err) } @@ -409,7 +413,7 @@ func ExtractAndDefaultSnapshotParameters(parameters map[string]string, driverNam } func extractResourceTagsParameter(tagsString string, resourceTags map[string]string) error { - paramResourceTags, err := ConvertTagsStringToMap(tagsString) + paramResourceTags, err := convert.ConvertTagsStringToMap(tagsString) if err != nil { return fmt.Errorf("parameters contain invalid %s parameter: %w", ParameterKeyResourceTags, err) } @@ -427,13 +431,13 @@ func ExtractModifyVolumeParameters(parameters map[string]string) (ModifyVolumePa for key, value := range parameters { switch strings.ToLower(key) { case "iops": - iops, err := ConvertStringToInt64(value) + iops, err := convert.ConvertStringToInt64(value) if err != nil { return ModifyVolumeParameters{}, fmt.Errorf("parameters contain invalid iops parameter: %w", err) } modifyVolumeParams.IOPS = &iops case "throughput": - throughput, err := ConvertMiStringToInt64(value) + throughput, err := convert.ConvertMiStringToInt64(value) if err != nil { return ModifyVolumeParameters{}, fmt.Errorf("parameters contain invalid throughput parameter: %w", err) } @@ -444,3 +448,38 @@ func ExtractModifyVolumeParameters(parameters map[string]string) (ModifyVolumePa } return modifyVolumeParams, nil } + +// ConvertStringToAvailabilityClass converts a string to an availability class string. +func convertStringToAvailabilityClass(str string) (string, error) { + switch strings.ToLower(str) { + case ParameterNoAvailabilityClass: + return ParameterNoAvailabilityClass, nil + case ParameterRegionalHardFailoverClass: + return ParameterRegionalHardFailoverClass, nil + } + return "", fmt.Errorf("Unexpected boolean string %s", str) +} + +// StoragePoolZones returns the zones from the given storage pools. +func StoragePoolZones(storagePools []StoragePool) ([]string, error) { + zonesSet := sets.String{} + var zones []string + for _, sp := range storagePools { + if zonesSet.Has(sp.Zone) { + return nil, fmt.Errorf("found multiple storage pools in zone %s. Only one storage pool per zone is allowed", sp.Zone) + } + zonesSet.Insert(sp.Zone) + zones = append(zones, sp.Zone) + } + return zones, nil +} + +// StoragePoolInZone returns the storage pool in the given zone. +func StoragePoolInZone(storagePools []StoragePool, zone string) *StoragePool { + for _, pool := range storagePools { + if zone == pool.Zone { + return &pool + } + } + return nil +} diff --git a/pkg/common/parameters_test.go b/pkg/parameters/parameters_test.go similarity index 99% rename from pkg/common/parameters_test.go rename to pkg/parameters/parameters_test.go index fded9b40c..f84614a22 100644 --- a/pkg/common/parameters_test.go +++ b/pkg/parameters/parameters_test.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package common +package parameters import ( "reflect" diff --git a/pkg/parameters/utils.go b/pkg/parameters/utils.go new file mode 100644 index 000000000..2eaa0c03f --- /dev/null +++ b/pkg/parameters/utils.go @@ -0,0 +1,126 @@ +/* +Copyright 2018 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package parameters + +import ( + "fmt" + "regexp" + "strings" + + "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/constants" +) + +const ( + // Snapshot storage location format + // Reference: https://cloud.google.com/storage/docs/locations + // Example: us + multiRegionalLocationFmt = "^[a-z]+$" + // Example: us-east1 + regionalLocationFmt = "^[a-z]+-[a-z]+[0-9]{1,2}$" +) + +var ( + multiRegionalPattern = regexp.MustCompile(multiRegionalLocationFmt) + regionalPattern = regexp.MustCompile(regionalLocationFmt) + + validDataCacheMode = []string{constants.DataCacheModeWriteBack, constants.DataCacheModeWriteThrough} + storagePoolFieldsRegex = regexp.MustCompile(`^projects/([^/]+)/zones/([^/]+)/storagePools/([^/]+)$`) +) + +// ProcessStorageLocations trims and normalizes storage location to lower letters. +func ProcessStorageLocations(storageLocations string) ([]string, error) { + normalizedLoc := strings.ToLower(strings.TrimSpace(storageLocations)) + if !multiRegionalPattern.MatchString(normalizedLoc) && !regionalPattern.MatchString(normalizedLoc) { + return []string{}, fmt.Errorf("invalid location for snapshot: %q", storageLocations) + } + return []string{normalizedLoc}, nil +} + +// ValidateSnapshotType validates the type +func ValidateSnapshotType(snapshotType string) error { + switch snapshotType { + case DiskSnapshotType, DiskImageType: + return nil + default: + return fmt.Errorf("invalid snapshot type %s", snapshotType) + } +} + +// ParseStoragePools returns an error if none of the given storagePools +// (delimited by a comma) are in the format +// projects/project/zones/zone/storagePools/storagePool. +func ParseStoragePools(storagePools string) ([]StoragePool, error) { + spSlice := strings.Split(storagePools, ",") + parsedStoragePools := []StoragePool{} + for _, sp := range spSlice { + project, location, spName, err := fieldsFromStoragePoolResourceName(sp) + if err != nil { + return nil, err + } + spObj := StoragePool{Project: project, Zone: location, Name: spName, ResourceName: sp} + parsedStoragePools = append(parsedStoragePools, spObj) + + } + return parsedStoragePools, nil +} + +// fieldsFromResourceName returns the project, zone, and Storage Pool name from the given +// Storage Pool resource name. The resource name must be in the format +// projects/project/zones/zone/storagePools/storagePool. +// All other formats are invalid, and an error will be returned. +func fieldsFromStoragePoolResourceName(resourceName string) (project, location, spName string, err error) { + fieldMatches := storagePoolFieldsRegex.FindStringSubmatch(resourceName) + // Field matches should have 4 strings: [resourceName, project, zone, storagePool]. The first + // match is the entire string. + if len(fieldMatches) != 4 { + err := fmt.Errorf("invalid Storage Pool resource name. Got %s, expected projects/project/zones/zone/storagePools/storagePool", resourceName) + return "", "", "", err + } + project = fieldMatches[1] + location = fieldMatches[2] + spName = fieldMatches[3] + return +} + +func ValidateDataCacheMode(s string) error { + if StringInSlice(s, validDataCacheMode) { + return nil + } + return fmt.Errorf("invalid data-cache-mode %s. Only \"writeback\" and \"writethrough\" is a valid input", s) +} + +func ValidateNonNegativeInt(n int64) error { + if n <= 0 { + return fmt.Errorf("Input should be set to > 0, got %d", n) + } + return nil +} + +func StringInSlice(s string, list []string) bool { + for _, v := range list { + if v == s { + return true + } + } + return false +} + +func isValidDiskEncryptionKmsKey(DiskEncryptionKmsKey string) bool { + // Validate key against default kmskey pattern + kmsKeyPattern := regexp.MustCompile("projects/[^/]+/locations/([^/]+)/keyRings/[^/]+/cryptoKeys/[^/]+") + return kmsKeyPattern.MatchString(DiskEncryptionKmsKey) +} diff --git a/pkg/parameters/utils_test.go b/pkg/parameters/utils_test.go new file mode 100644 index 000000000..3c410d996 --- /dev/null +++ b/pkg/parameters/utils_test.go @@ -0,0 +1,55 @@ +package parameters + +import ( + "reflect" + "testing" +) + +func TestSnapshotStorageLocations(t *testing.T) { + tests := []struct { + desc string + locationString string + expectedNormalizedLocations []string + expectError bool + }{ + { + "valid multi-region", + " uS ", + []string{"us"}, + false, + }, + { + "valid region", + " US-EAST1", + []string{"us-east1"}, + false, + }, + { + "valid region in large continent", + "europe-west12", + []string{"europe-west12"}, + false, + }, + { + // Zones are not valid bucket/snapshot locations. + "single zone", + "us-east1-a", + []string{}, + true, + }, + } + for _, tc := range tests { + t.Run(tc.desc, func(t *testing.T) { + normalizedLocations, err := ProcessStorageLocations(tc.locationString) + if err != nil && !tc.expectError { + t.Errorf("Got error %v processing storage locations %q; expect no error", err, tc.locationString) + } + if err == nil && tc.expectError { + t.Errorf("Got no error processing storage locations %q; expect an error", tc.locationString) + } + if err == nil && !reflect.DeepEqual(normalizedLocations, tc.expectedNormalizedLocations) { + t.Errorf("Got %v for normalized storage locations; expect %v", normalizedLocations, tc.expectedNormalizedLocations) + } + }) + } +} diff --git a/test/e2e/tests/single_zone_e2e_test.go b/test/e2e/tests/single_zone_e2e_test.go index 392c3879d..5ac324b9b 100644 --- a/test/e2e/tests/single_zone_e2e_test.go +++ b/test/e2e/tests/single_zone_e2e_test.go @@ -31,6 +31,7 @@ import ( "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/constants" "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/deviceutils" gce "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/gce-cloud-provider/compute" + "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/parameters" testutils "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/test/e2e/utils" "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/test/remote" @@ -432,7 +433,7 @@ var _ = Describe("GCE PD CSI Driver", func() { // Create Disk volName := testNamePrefix + string(uuid.NewUUID()) volume, err := controllerClient.CreateVolume(volName, map[string]string{ - common.ParameterKeyReplicationType: "regional-pd", + parameters.ParameterKeyReplicationType: "regional-pd", }, defaultRepdSizeGb, nil, nil) Expect(err).To(BeNil(), "CreateVolume failed with error: %v", err) @@ -514,7 +515,7 @@ var _ = Describe("GCE PD CSI Driver", func() { // Create Disk diskParams := map[string]string{ - common.ParameterKeyType: diskType, + parameters.ParameterKeyType: diskType, } volName := testNamePrefix + string(uuid.NewUUID()) @@ -558,7 +559,7 @@ var _ = Describe("GCE PD CSI Driver", func() { disk := typeToDisk[diskType] volName := testNamePrefix + string(uuid.NewUUID()) params := merge(disk.params, map[string]string{ - common.ParameterKeyLabels: "key1=value1,key2=value2", + parameters.ParameterKeyLabels: "key1=value1,key2=value2", }) diskSize := defaultSizeGb @@ -685,7 +686,7 @@ var _ = Describe("GCE PD CSI Driver", func() { disk := typeToDisk[diskType] volName := testNamePrefix + string(uuid.NewUUID()) params := merge(disk.params, map[string]string{ - common.ParameterKeyDiskEncryptionKmsKey: key.Name, + parameters.ParameterKeyDiskEncryptionKmsKey: key.Name, }) topology := &csi.TopologyRequirement{ Requisite: []*csi.Topology{ @@ -815,7 +816,7 @@ var _ = Describe("GCE PD CSI Driver", func() { // Create Disk volName := testNamePrefix + string(uuid.NewUUID()) volume, err := controllerClient.CreateVolume(volName, map[string]string{ - common.ParameterKeyReplicationType: "regional-pd", + parameters.ParameterKeyReplicationType: "regional-pd", }, defaultRepdSizeGb, nil, nil) Expect(err).To(BeNil(), "CreateVolume failed with error: %v", err) @@ -1029,9 +1030,9 @@ var _ = Describe("GCE PD CSI Driver", func() { disk := typeToDisk[diskType] volName := testNamePrefix + string(uuid.NewUUID()) params := merge(disk.params, map[string]string{ - common.ParameterKeyPVCName: "test-pvc", - common.ParameterKeyPVCNamespace: "test-pvc-namespace", - common.ParameterKeyPVName: "test-pv-name", + parameters.ParameterKeyPVCName: "test-pvc", + parameters.ParameterKeyPVCNamespace: "test-pvc-namespace", + parameters.ParameterKeyPVName: "test-pv-name", }) volume, err := controllerClient.CreateVolume(volName, params, diskSize, nil /* topReq */, nil) Expect(err).To(BeNil(), "CreateVolume failed with error: %v", err) @@ -1077,10 +1078,10 @@ var _ = Describe("GCE PD CSI Driver", func() { snapshotLocation := z[:len(z)-2] snapshotParams := map[string]string{ - common.ParameterKeyStorageLocations: snapshotLocation, - common.ParameterKeyVolumeSnapshotName: "test-volumesnapshot-name", - common.ParameterKeyVolumeSnapshotNamespace: "test-volumesnapshot-namespace", - common.ParameterKeyVolumeSnapshotContentName: "test-volumesnapshotcontent-name", + parameters.ParameterKeyStorageLocations: snapshotLocation, + parameters.ParameterKeyVolumeSnapshotName: "test-volumesnapshot-name", + parameters.ParameterKeyVolumeSnapshotNamespace: "test-volumesnapshot-namespace", + parameters.ParameterKeyVolumeSnapshotContentName: "test-volumesnapshotcontent-name", } snapshotID, err := client.CreateSnapshot(snapshotName, volID, snapshotParams) Expect(err).To(BeNil(), "CreateSnapshot failed with error: %v", err) @@ -1138,7 +1139,7 @@ var _ = Describe("GCE PD CSI Driver", func() { snapshotName := testNamePrefix + string(uuid.NewUUID()) testImageFamily := "test-family" - snapshotParams := map[string]string{common.ParameterKeySnapshotType: common.DiskImageType, common.ParameterKeyImageFamily: testImageFamily} + snapshotParams := map[string]string{parameters.ParameterKeySnapshotType: parameters.DiskImageType, parameters.ParameterKeyImageFamily: testImageFamily} snapshotID, err := client.CreateSnapshot(snapshotName, volID, snapshotParams) Expect(err).To(BeNil(), "CreateSnapshot failed with error: %v", err) @@ -1162,7 +1163,7 @@ var _ = Describe("GCE PD CSI Driver", func() { Expect(err).To(BeNil(), "Could not get snapshot from cloud directly") _, snapshotType, _, err := common.SnapshotIDToProjectKey(cleanSelfLink(snapshot.SelfLink)) Expect(err).To(BeNil(), "Failed to parse snapshot ID") - Expect(snapshotType).To(Equal(common.DiskImageType), "Expected images type in snapshot ID") + Expect(snapshotType).To(Equal(parameters.DiskImageType), "Expected images type in snapshot ID") snapshots, err := client.ListSnapshots() Expect(err).To(BeNil(), "Could not list snapshots") @@ -1202,7 +1203,7 @@ var _ = Describe("GCE PD CSI Driver", func() { // Create Disk volName := testNamePrefix + string(uuid.NewUUID()) volume, err := controllerClient.CreateVolume(volName, map[string]string{ - common.ParameterKeyReplicationType: "none", + parameters.ParameterKeyReplicationType: "none", }, defaultSizeGb, &csi.TopologyRequirement{ Requisite: []*csi.Topology{ @@ -1258,12 +1259,12 @@ var _ = Describe("GCE PD CSI Driver", func() { // Create Source Disk srcVolName := testNamePrefix + string(uuid.NewUUID()) srcVolume, err := controllerClient.CreateVolume(srcVolName, map[string]string{ - common.ParameterKeyReplicationType: "none", + parameters.ParameterKeyReplicationType: "none", }, defaultRepdSizeGb, nil, nil) // Create Disk volName := testNamePrefix + string(uuid.NewUUID()) volume, err := controllerClient.CreateVolume(volName, map[string]string{ - common.ParameterKeyReplicationType: "regional-pd", + parameters.ParameterKeyReplicationType: "regional-pd", }, defaultRepdSizeGb, nil, &csi.VolumeContentSource{ Type: &csi.VolumeContentSource_Volume{ @@ -1323,12 +1324,12 @@ var _ = Describe("GCE PD CSI Driver", func() { // Create Source Disk srcVolName := testNamePrefix + string(uuid.NewUUID()) srcVolume, err := controllerClient.CreateVolume(srcVolName, map[string]string{ - common.ParameterKeyReplicationType: "regional-pd", + parameters.ParameterKeyReplicationType: "regional-pd", }, defaultRepdSizeGb, nil, nil) // Create Disk volName := testNamePrefix + string(uuid.NewUUID()) volume, err := controllerClient.CreateVolume(volName, map[string]string{ - common.ParameterKeyReplicationType: "regional-pd", + parameters.ParameterKeyReplicationType: "regional-pd", }, defaultRepdSizeGb, nil, &csi.VolumeContentSource{ Type: &csi.VolumeContentSource_Volume{ @@ -1862,7 +1863,7 @@ func createAndValidateUniqueZonalMultiWriterDisk(client *remote.CsiClient, proje // Create Disk disk := typeToDisk[diskType] - disk.params[common.ParameterAccessMode] = "READ_WRITE_MANY" + disk.params[parameters.ParameterAccessMode] = "READ_WRITE_MANY" volName := testNamePrefix + string(uuid.NewUUID()) volume, err := client.CreateVolumeWithCaps(volName, disk.params, defaultMwSizeGb, &csi.TopologyRequirement{ @@ -1969,7 +1970,7 @@ type disk struct { var typeToDisk = map[string]*disk{ standardDiskType: { params: map[string]string{ - common.ParameterKeyType: standardDiskType, + parameters.ParameterKeyType: standardDiskType, }, validate: func(disk *compute.Disk) { Expect(disk.Type).To(ContainSubstring(standardDiskType)) @@ -1977,8 +1978,8 @@ var typeToDisk = map[string]*disk{ }, extremeDiskType: { params: map[string]string{ - common.ParameterKeyType: extremeDiskType, - common.ParameterKeyProvisionedIOPSOnCreate: provisionedIOPSOnCreate, + parameters.ParameterKeyType: extremeDiskType, + parameters.ParameterKeyProvisionedIOPSOnCreate: provisionedIOPSOnCreate, }, validate: func(disk *compute.Disk) { Expect(disk.Type).To(ContainSubstring(extremeDiskType)) @@ -1987,9 +1988,9 @@ var typeToDisk = map[string]*disk{ }, hdbDiskType: { params: map[string]string{ - common.ParameterKeyType: hdbDiskType, - common.ParameterKeyProvisionedIOPSOnCreate: provisionedIOPSOnCreateHdb, - common.ParameterKeyProvisionedThroughputOnCreate: provisionedThroughputOnCreateHdb, + parameters.ParameterKeyType: hdbDiskType, + parameters.ParameterKeyProvisionedIOPSOnCreate: provisionedIOPSOnCreateHdb, + parameters.ParameterKeyProvisionedThroughputOnCreate: provisionedThroughputOnCreateHdb, }, validate: func(disk *compute.Disk) { Expect(disk.Type).To(ContainSubstring(hdbDiskType)) @@ -1999,8 +2000,8 @@ var typeToDisk = map[string]*disk{ }, hdxDiskType: { params: map[string]string{ - common.ParameterKeyType: hdxDiskType, - common.ParameterKeyProvisionedIOPSOnCreate: provisionedIOPSOnCreateHdx, + parameters.ParameterKeyType: hdxDiskType, + parameters.ParameterKeyProvisionedIOPSOnCreate: provisionedIOPSOnCreateHdx, }, validate: func(disk *compute.Disk) { Expect(disk.Type).To(ContainSubstring(hdxDiskType)) @@ -2009,8 +2010,8 @@ var typeToDisk = map[string]*disk{ }, hdtDiskType: { params: map[string]string{ - common.ParameterKeyType: hdtDiskType, - common.ParameterKeyProvisionedThroughputOnCreate: provisionedThroughputOnCreate, + parameters.ParameterKeyType: hdtDiskType, + parameters.ParameterKeyProvisionedThroughputOnCreate: provisionedThroughputOnCreate, }, validate: func(disk *compute.Disk) { Expect(disk.Type).To(ContainSubstring(hdtDiskType)) @@ -2019,7 +2020,7 @@ var typeToDisk = map[string]*disk{ }, ssdDiskType: { params: map[string]string{ - common.ParameterKeyType: ssdDiskType, + parameters.ParameterKeyType: ssdDiskType, }, validate: func(disk *compute.Disk) { Expect(disk.Type).To(ContainSubstring(ssdDiskType)) @@ -2027,7 +2028,7 @@ var typeToDisk = map[string]*disk{ }, "hyperdisk-ml": { params: map[string]string{ - common.ParameterKeyType: "hyperdisk-ml", + parameters.ParameterKeyType: "hyperdisk-ml", }, validate: func(disk *compute.Disk) { Expect(disk.Type).To(ContainSubstring("hyperdisk-ml")) From 634368571c1d57253679e4919474fd02b7b29491 Mon Sep 17 00:00:00 2001 From: juliankatz Date: Tue, 7 Oct 2025 13:28:37 -0700 Subject: [PATCH 2/4] Fix some more broken references --- cmd/gce-pd-csi-driver/main.go | 6 +-- .../compute/gce-compute_test.go | 5 ++- pkg/gce-pd-csi-driver/server_test.go | 4 +- pkg/gce-pd-csi-driver/utils_test.go | 25 ++++++----- test/e2e/tests/multi_zone_e2e_test.go | 43 ++++++++++--------- test/sanity/sanity_test.go | 7 +-- 6 files changed, 46 insertions(+), 44 deletions(-) diff --git a/cmd/gce-pd-csi-driver/main.go b/cmd/gce-pd-csi-driver/main.go index ef839e4e6..911b6df5a 100644 --- a/cmd/gce-pd-csi-driver/main.go +++ b/cmd/gce-pd-csi-driver/main.go @@ -29,8 +29,8 @@ import ( "k8s.io/klog/v2" "k8s.io/utils/strings/slices" - "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/common" "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/constants" + "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/convert" "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/deviceutils" gce "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/gce-cloud-provider/compute" metadataservice "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/gce-cloud-provider/metadata" @@ -181,7 +181,7 @@ func handle() { if len(*extraVolumeLabelsStr) > 0 && !*runControllerService { klog.Fatalf("Extra volume labels provided but not running controller") } - extraVolumeLabels, err := common.ConvertLabelsStringToMap(*extraVolumeLabelsStr) + extraVolumeLabels, err := convert.ConvertLabelsStringToMap(*extraVolumeLabelsStr) if err != nil { klog.Fatalf("Bad extra volume labels: %v", err.Error()) } @@ -189,7 +189,7 @@ func handle() { if len(*extraTagsStr) > 0 && !*runControllerService { klog.Fatalf("Extra tags provided but not running controller") } - extraTags, err := common.ConvertTagsStringToMap(*extraTagsStr) + extraTags, err := convert.ConvertTagsStringToMap(*extraTagsStr) if err != nil { klog.Fatalf("Bad extra tags: %v", err.Error()) } diff --git a/pkg/gce-cloud-provider/compute/gce-compute_test.go b/pkg/gce-cloud-provider/compute/gce-compute_test.go index a997ba10c..ebfef1c60 100644 --- a/pkg/gce-cloud-provider/compute/gce-compute_test.go +++ b/pkg/gce-cloud-provider/compute/gce-compute_test.go @@ -23,6 +23,7 @@ import ( "google.golang.org/grpc/codes" "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/common" "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/constants" + "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/parameters" ) func TestValidateDiskParameters(t *testing.T) { @@ -87,7 +88,7 @@ func TestValidateDiskParameters(t *testing.T) { Kind: "compute#disk", }) - storageClassParams := common.DiskParameters{ + storageClassParams := parameters.DiskParameters{ DiskType: "pd-standard", ReplicationType: "none", DiskEncryptionKMSKey: tc.storageClassKMSKey, @@ -242,7 +243,7 @@ func TestValidateExistingDisk(t *testing.T) { d.Zone = "zone" // Bootstrap params. We don't care about these as they are already tested in previous unit test. - params := common.DiskParameters{ + params := parameters.DiskParameters{ DiskType: tc.diskType, } diff --git a/pkg/gce-pd-csi-driver/server_test.go b/pkg/gce-pd-csi-driver/server_test.go index 9b6bcf420..9a645c4cb 100644 --- a/pkg/gce-pd-csi-driver/server_test.go +++ b/pkg/gce-pd-csi-driver/server_test.go @@ -25,9 +25,9 @@ import ( "google.golang.org/grpc" csi "github.com/container-storage-interface/spec/lib/go/csi" - "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/common" gce "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/gce-cloud-provider/compute" "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/metrics" + "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/parameters" ) func createSocketFile() (string, func(), error) { @@ -104,7 +104,7 @@ func TestServerCreateVolumeMetric(t *testing.T) { CapacityRange: stdCapRange, VolumeCapabilities: stdVolCaps, Parameters: map[string]string{ - common.ParameterKeyType: "pd-balanced", + parameters.ParameterKeyType: "pd-balanced", }, } resp, err := controllerClient.CreateVolume(context.Background(), req) diff --git a/pkg/gce-pd-csi-driver/utils_test.go b/pkg/gce-pd-csi-driver/utils_test.go index 5472a7f8a..60bdd8511 100644 --- a/pkg/gce-pd-csi-driver/utils_test.go +++ b/pkg/gce-pd-csi-driver/utils_test.go @@ -25,7 +25,6 @@ import ( csi "github.com/container-storage-interface/spec/lib/go/csi" "github.com/google/go-cmp/cmp" - "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/common" "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/constants" "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/parameters" ) @@ -303,7 +302,7 @@ func TestValidateStoragePools(t *testing.T) { testCases := []struct { name string req *csi.CreateVolumeRequest - params common.DiskParameters + params parameters.DiskParameters project string expErr error enableHdHA bool @@ -313,7 +312,7 @@ func TestValidateStoragePools(t *testing.T) { req: &csi.CreateVolumeRequest{ Name: "test-name", }, - params: common.DiskParameters{ + params: parameters.DiskParameters{ DiskType: "hyperdisk-balanced", }, expErr: nil, @@ -321,7 +320,7 @@ func TestValidateStoragePools(t *testing.T) { { name: "success with nil CreateVolumeReq", req: nil, - params: common.DiskParameters{ + params: parameters.DiskParameters{ DiskType: "hyperdisk-balanced", }, expErr: nil, @@ -331,7 +330,7 @@ func TestValidateStoragePools(t *testing.T) { req: &csi.CreateVolumeRequest{ Name: "test-name", }, - params: common.DiskParameters{ + params: parameters.DiskParameters{ DiskType: "hyperdisk-balanced", StoragePools: []parameters.StoragePool{ { @@ -357,7 +356,7 @@ func TestValidateStoragePools(t *testing.T) { req: &csi.CreateVolumeRequest{ Name: "test-name", }, - params: common.DiskParameters{ + params: parameters.DiskParameters{ DiskType: "pd-balanced", StoragePools: []parameters.StoragePool{ { @@ -382,7 +381,7 @@ func TestValidateStoragePools(t *testing.T) { req: &csi.CreateVolumeRequest{ Name: "test-name", }, - params: common.DiskParameters{ + params: parameters.DiskParameters{ DiskType: "hyperdisk-balanced", ReplicationType: "regional-pd", StoragePools: []parameters.StoragePool{ @@ -408,7 +407,7 @@ func TestValidateStoragePools(t *testing.T) { req: &csi.CreateVolumeRequest{ Name: "test-name", }, - params: common.DiskParameters{ + params: parameters.DiskParameters{ DiskType: "hyperdisk-balanced-high-availability", StoragePools: []parameters.StoragePool{ { @@ -434,7 +433,7 @@ func TestValidateStoragePools(t *testing.T) { req: &csi.CreateVolumeRequest{ Name: "test-name", }, - params: common.DiskParameters{ + params: parameters.DiskParameters{ DiskType: "hyperdisk-balanced-high-availability", StoragePools: []parameters.StoragePool{ { @@ -466,7 +465,7 @@ func TestValidateStoragePools(t *testing.T) { }, }, }, - params: common.DiskParameters{ + params: parameters.DiskParameters{ DiskType: "hyperdisk-balanced", StoragePools: []parameters.StoragePool{ { @@ -506,7 +505,7 @@ func TestValidateStoragePools(t *testing.T) { }, }, }, - params: common.DiskParameters{ + params: parameters.DiskParameters{ DiskType: "hyperdisk-balanced", StoragePools: []parameters.StoragePool{ { @@ -549,7 +548,7 @@ func TestValidateStoragePools(t *testing.T) { }, }, }, - params: common.DiskParameters{ + params: parameters.DiskParameters{ DiskType: "hyperdisk-balanced", StoragePools: []parameters.StoragePool{ { @@ -592,7 +591,7 @@ func TestValidateStoragePools(t *testing.T) { }, }, }, - params: common.DiskParameters{ + params: parameters.DiskParameters{ DiskType: "hyperdisk-balanced", StoragePools: []parameters.StoragePool{ { diff --git a/test/e2e/tests/multi_zone_e2e_test.go b/test/e2e/tests/multi_zone_e2e_test.go index 933f535d3..7e0b0d0b2 100644 --- a/test/e2e/tests/multi_zone_e2e_test.go +++ b/test/e2e/tests/multi_zone_e2e_test.go @@ -33,6 +33,7 @@ import ( "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/common" "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/constants" gce "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/gce-cloud-provider/compute" + "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/parameters" testutils "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/test/e2e/utils" remote "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/test/remote" ) @@ -234,8 +235,8 @@ var _ = Describe("GCE PD CSI Driver Multi-Zone", func() { // Create multi-zone Disk resp, err := controllerClient.CreateVolumeWithCaps(volName, map[string]string{ - common.ParameterKeyEnableMultiZoneProvisioning: "true", - common.ParameterKeyType: "hyperdisk-ml", + parameters.ParameterKeyEnableMultiZoneProvisioning: "true", + parameters.ParameterKeyType: "hyperdisk-ml", }, defaultHdmlSizeGb, &csi.TopologyRequirement{ Requisite: []*csi.Topology{ @@ -356,8 +357,8 @@ var _ = Describe("GCE PD CSI Driver Multi-Zone", func() { // Create multi-zone Disk volName := testNamePrefix + string(uuid.NewUUID()) _, err = controllerClient.CreateVolumeWithCaps(volName, map[string]string{ - common.ParameterKeyEnableMultiZoneProvisioning: "true", - common.ParameterKeyType: "hyperdisk-ml", + parameters.ParameterKeyEnableMultiZoneProvisioning: "true", + parameters.ParameterKeyType: "hyperdisk-ml", }, defaultHdmlSizeGb, &csi.TopologyRequirement{ Requisite: []*csi.Topology{ @@ -504,8 +505,8 @@ var _ = Describe("GCE PD CSI Driver Multi-Zone", func() { // Create multi-zone Disk volName := testNamePrefix + string(uuid.NewUUID()) _, err = controllerClient.CreateVolumeWithCaps(volName, map[string]string{ - common.ParameterKeyEnableMultiZoneProvisioning: "true", - common.ParameterKeyType: "hyperdisk-ml", + parameters.ParameterKeyEnableMultiZoneProvisioning: "true", + parameters.ParameterKeyType: "hyperdisk-ml", }, defaultHdmlSizeGb, &csi.TopologyRequirement{}, []*csi.VolumeCapability{ @@ -648,8 +649,8 @@ var _ = Describe("GCE PD CSI Driver Multi-Zone", func() { volName := testNamePrefix + string(uuid.NewUUID()) _, err = controllerClient.CreateVolumeWithCaps(volName, map[string]string{ - common.ParameterKeyEnableMultiZoneProvisioning: "true", - common.ParameterKeyType: "hyperdisk-ml", + parameters.ParameterKeyEnableMultiZoneProvisioning: "true", + parameters.ParameterKeyType: "hyperdisk-ml", }, defaultHdmlSizeGb, &csi.TopologyRequirement{ Requisite: []*csi.Topology{ @@ -751,8 +752,8 @@ var _ = Describe("GCE PD CSI Driver Multi-Zone", func() { // Create Disk volName := testNamePrefix + string(uuid.NewUUID()) _, err := controllerClient.CreateVolumeWithCaps(volName, map[string]string{ - common.ParameterKeyEnableMultiZoneProvisioning: "true", - common.ParameterKeyType: "hyperdisk-ml", + parameters.ParameterKeyEnableMultiZoneProvisioning: "true", + parameters.ParameterKeyType: "hyperdisk-ml", }, defaultHdmlSizeGb, &csi.TopologyRequirement{ Requisite: []*csi.Topology{ @@ -867,8 +868,8 @@ var _ = Describe("GCE PD CSI Driver Multi-Zone", func() { // Create Disk volName := testNamePrefix + string(uuid.NewUUID()) _, err := controllerClient.CreateVolumeWithCaps(volName, map[string]string{ - common.ParameterKeyEnableMultiZoneProvisioning: "true", - common.ParameterKeyType: "hyperdisk-ml", + parameters.ParameterKeyEnableMultiZoneProvisioning: "true", + parameters.ParameterKeyType: "hyperdisk-ml", }, defaultHdmlSizeGb, &csi.TopologyRequirement{ Requisite: []*csi.Topology{ @@ -977,7 +978,7 @@ var _ = Describe("GCE PD CSI Driver Multi-Zone", func() { // Create Disk volName := testNamePrefix + string(uuid.NewUUID()) _, err := controllerClient.CreateVolumeWithCaps(volName, map[string]string{ - common.ParameterKeyType: "hyperdisk-ml", + parameters.ParameterKeyType: "hyperdisk-ml", }, defaultHdmlSizeGb, &csi.TopologyRequirement{ Requisite: []*csi.Topology{ @@ -1067,7 +1068,7 @@ var _ = Describe("GCE PD CSI Driver Multi-Zone", func() { // Create Disk volName := testNamePrefix + string(uuid.NewUUID()) volume, err := controllerClient.CreateVolume(volName, map[string]string{ - common.ParameterKeyReplicationType: "regional-pd", + parameters.ParameterKeyReplicationType: "regional-pd", }, defaultRepdSizeGb, &csi.TopologyRequirement{ Requisite: []*csi.Topology{ { @@ -1150,8 +1151,8 @@ var _ = Describe("GCE PD CSI Driver Multi-Zone", func() { // Create Disk volName := testNamePrefix + string(uuid.NewUUID()) volume, err := controllerClient.CreateVolume(volName, map[string]string{ - common.ParameterKeyReplicationType: "regional-pd", - common.ParameterAvailabilityClass: common.ParameterRegionalHardFailoverClass, + parameters.ParameterKeyReplicationType: "regional-pd", + parameters.ParameterAvailabilityClass: parameters.ParameterRegionalHardFailoverClass, }, defaultRepdSizeGb, &csi.TopologyRequirement{ Requisite: []*csi.Topology{ { @@ -1268,9 +1269,9 @@ var _ = Describe("GCE PD CSI Driver Multi-Zone", func() { volName := testNamePrefix + string(uuid.NewUUID()) wantIOPs, wantThroughput := int64(7000), int64(250) volume, err := controllerClient.CreateVolume(volName, map[string]string{ - common.ParameterKeyType: common.DiskTypeHdHA, - common.ParameterKeyProvisionedIOPSOnCreate: strconv.FormatInt(wantIOPs, 10), - common.ParameterKeyProvisionedThroughputOnCreate: strconv.FormatInt(wantThroughput, 10) + "Mi", + parameters.ParameterKeyType: parameters.DiskTypeHdHA, + parameters.ParameterKeyProvisionedIOPSOnCreate: strconv.FormatInt(wantIOPs, 10), + parameters.ParameterKeyProvisionedThroughputOnCreate: strconv.FormatInt(wantThroughput, 10) + "Mi", }, defaultRepdSizeGb, &csi.TopologyRequirement{ Requisite: []*csi.Topology{ { @@ -1351,8 +1352,8 @@ var _ = Describe("GCE PD CSI Driver Multi-Zone", func() { // Create Disk volName := testNamePrefix + string(uuid.NewUUID()) volume, err := controllerClient.CreateVolume(volName, map[string]string{ - common.ParameterKeyType: common.DiskTypeHdHA, - common.ParameterAvailabilityClass: common.ParameterRegionalHardFailoverClass, + parameters.ParameterKeyType: parameters.DiskTypeHdHA, + parameters.ParameterAvailabilityClass: parameters.ParameterRegionalHardFailoverClass, }, defaultRepdSizeGb, &csi.TopologyRequirement{ Requisite: []*csi.Topology{ { diff --git a/test/sanity/sanity_test.go b/test/sanity/sanity_test.go index 5ce72493c..ae0460a79 100644 --- a/test/sanity/sanity_test.go +++ b/test/sanity/sanity_test.go @@ -35,6 +35,7 @@ import ( driver "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/gce-pd-csi-driver" "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/linkcache" mountmanager "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/mount-manager" + "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/parameters" ) func TestSanity(t *testing.T) { @@ -128,9 +129,9 @@ func TestSanity(t *testing.T) { IDGen: newPDIDGenerator(project, zone), TestVolumeSize: common.GbToBytes(200), TestVolumeParameters: map[string]string{ - common.ParameterKeyType: "hyperdisk-balanced", - common.ParameterKeyProvisionedIOPSOnCreate: "3000", - common.ParameterKeyProvisionedThroughputOnCreate: "150Mi", + parameters.ParameterKeyType: "hyperdisk-balanced", + parameters.ParameterKeyProvisionedIOPSOnCreate: "3000", + parameters.ParameterKeyProvisionedThroughputOnCreate: "150Mi", }, TestVolumeMutableParameters: map[string]string{"iops": "3013", "throughput": "151"}, } From ffc2cae5054ff5619395f105ab3875a6481cf770 Mon Sep 17 00:00:00 2001 From: juliankatz Date: Tue, 7 Oct 2025 13:46:15 -0700 Subject: [PATCH 3/4] Add back some tests that were accidentally removed --- pkg/convert/convert_test.go | 330 +++++++++++++++++++++++++++++++++++ pkg/parameters/parameters.go | 3 +- pkg/parameters/utils_test.go | 179 +++++++++++++++++++ 3 files changed, 511 insertions(+), 1 deletion(-) diff --git a/pkg/convert/convert_test.go b/pkg/convert/convert_test.go index 9829d0102..a4c929934 100644 --- a/pkg/convert/convert_test.go +++ b/pkg/convert/convert_test.go @@ -469,3 +469,333 @@ func TestConvertStringToBool(t *testing.T) { }) } } + +func TestConvertStringToInt64(t *testing.T) { + tests := []struct { + desc string + inputStr string + expInt64 int64 + expectError bool + }{ + { + desc: "valid number string", + inputStr: "10000", + expInt64: 10000, + expectError: false, + }, + { + desc: "test higher number", + inputStr: "15000", + expInt64: 15000, + expectError: false, + }, + { + desc: "round M to number", + inputStr: "1M", + expInt64: 1000000, + expectError: false, + }, + { + desc: "round m to number", + inputStr: "1m", + expInt64: 1, + expectError: false, + }, + { + desc: "round k to number", + inputStr: "1k", + expInt64: 1000, + expectError: false, + }, + { + desc: "invalid empty string", + inputStr: "", + expInt64: 0, + expectError: true, + }, + { + desc: "invalid string", + inputStr: "ew%65", + expInt64: 0, + expectError: true, + }, + { + desc: "invalid KiB string", + inputStr: "10KiB", + expInt64: 10000, + expectError: true, + }, + { + desc: "invalid GB string", + inputStr: "10GB", + expInt64: 0, + expectError: true, + }, + { + desc: "round Ki to number", + inputStr: "1Ki", + expInt64: 1024, + expectError: false, + }, + { + desc: "round k to number", + inputStr: "10k", + expInt64: 10000, + expectError: false, + }, + { + desc: "round Mi to number", + inputStr: "10Mi", + expInt64: 10485760, + expectError: false, + }, + { + desc: "round M to number", + inputStr: "10M", + expInt64: 10000000, + expectError: false, + }, + { + desc: "round G to number", + inputStr: "10G", + expInt64: 10000000000, + expectError: false, + }, + { + desc: "round Gi to number", + inputStr: "100Gi", + expInt64: 107374182400, + expectError: false, + }, + { + desc: "round decimal to number", + inputStr: "1.2Gi", + expInt64: 1288490189, + expectError: false, + }, + { + desc: "round big value to number", + inputStr: "8191Pi", + expInt64: 9222246136947933184, + expectError: false, + }, + } + 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) + } + }) + } +} + +func TestConvertMiStringToInt64(t *testing.T) { + tests := []struct { + desc string + inputStr string + expInt64 int64 + expectError bool + }{ + { + desc: "valid number string", + inputStr: "10000", + expInt64: 1, + expectError: false, + }, + { + desc: "round Ki to MiB", + inputStr: "1000Ki", + expInt64: 1, + expectError: false, + }, + { + desc: "round k to MiB", + inputStr: "1000k", + expInt64: 1, + expectError: false, + }, + { + desc: "round Mi to MiB", + inputStr: "1000Mi", + expInt64: 1000, + expectError: false, + }, + { + desc: "round M to MiB", + inputStr: "1000M", + expInt64: 954, + expectError: false, + }, + { + desc: "round G to MiB", + inputStr: "1000G", + expInt64: 953675, + expectError: false, + }, + { + desc: "round Gi to MiB", + inputStr: "10000Gi", + expInt64: 10240000, + expectError: false, + }, + { + desc: "round decimal to MiB", + inputStr: "1.2Gi", + expInt64: 1229, + expectError: false, + }, + { + desc: "round big value to MiB", + inputStr: "8191Pi", + expInt64: 8795019280384, + expectError: false, + }, + { + desc: "invalid empty string", + inputStr: "", + expInt64: 0, + expectError: true, + }, + { + desc: "invalid KiB string", + inputStr: "10KiB", + expInt64: 10000, + expectError: true, + }, + { + desc: "invalid GB string", + inputStr: "10GB", + expInt64: 0, + expectError: true, + }, + { + desc: "invalid string", + inputStr: "ew%65", + expInt64: 0, + expectError: true, + }, + } + for _, tc := range tests { + t.Run(tc.desc, func(t *testing.T) { + actualInt64, err := ConvertMiStringToInt64(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) + } + }) + } +} + +func TestConvertGiStringToInt64(t *testing.T) { + tests := []struct { + desc string + inputStr string + expInt64 int64 + expectError bool + }{ + { + desc: "valid number string", + inputStr: "10000", + expInt64: 1, + expectError: false, + }, + { + desc: "round Ki to GiB", + inputStr: "1000000Ki", + expInt64: 1, + expectError: false, + }, + { + desc: "round k to GiB", + inputStr: "1000000k", + expInt64: 1, + expectError: false, + }, + { + desc: "round Mi to GiB", + inputStr: "1000Mi", + expInt64: 1, + expectError: false, + }, + { + desc: "round M to GiB", + inputStr: "1000M", + expInt64: 1, + expectError: false, + }, + { + desc: "round G to GiB", + inputStr: "1000G", + expInt64: 932, + expectError: false, + }, + { + desc: "round Gi to GiB - most common case", + inputStr: "1234Gi", + expInt64: 1234, + expectError: false, + }, + { + desc: "round decimal to GiB", + inputStr: "1.2Gi", + expInt64: 2, + expectError: false, + }, + { + desc: "round big value to GiB", + inputStr: "8191Pi", + expInt64: 8588886016, + expectError: false, + }, + { + desc: "invalid empty string", + inputStr: "", + expInt64: 0, + expectError: true, + }, + { + desc: "invalid KiB string", + inputStr: "10KiB", + expInt64: 10000, + expectError: true, + }, + { + desc: "invalid GB string", + inputStr: "10GB", + expInt64: 0, + expectError: true, + }, + { + desc: "invalid string", + inputStr: "ew%65", + expInt64: 0, + expectError: true, + }, + } + for _, tc := range tests { + t.Run(tc.desc, func(t *testing.T) { + actualInt64, err := ConvertGiStringToInt64(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) + } + }) + } +} diff --git a/pkg/parameters/parameters.go b/pkg/parameters/parameters.go index 2c92bc29b..90a8c62b7 100644 --- a/pkg/parameters/parameters.go +++ b/pkg/parameters/parameters.go @@ -460,7 +460,8 @@ func convertStringToAvailabilityClass(str string) (string, error) { return "", fmt.Errorf("Unexpected boolean string %s", str) } -// StoragePoolZones returns the zones from the given storage pools. +// StoragePoolZones returns the unique zones of the given storage pool resource names. +// Returns an error if multiple storage pools in 1 zone are found. func StoragePoolZones(storagePools []StoragePool) ([]string, error) { zonesSet := sets.String{} var zones []string diff --git a/pkg/parameters/utils_test.go b/pkg/parameters/utils_test.go index 3c410d996..38d1ac57a 100644 --- a/pkg/parameters/utils_test.go +++ b/pkg/parameters/utils_test.go @@ -1,6 +1,7 @@ package parameters import ( + "fmt" "reflect" "testing" ) @@ -53,3 +54,181 @@ func TestSnapshotStorageLocations(t *testing.T) { }) } } + +func TestStringInSlice(t *testing.T) { + testCases := []struct { + name string + inputStr string + inputSlice []string + expectedInSlice bool + }{ + { + name: "string is in the slice", + inputStr: "in slice", + inputSlice: []string{"in slice", "other string"}, + expectedInSlice: true, + }, + { + name: "string is NOT in the slice", + inputStr: "not in slice", + inputSlice: []string{"other string"}, + }, + } + + for _, tc := range testCases { + t.Logf("test case: %s", tc.name) + actualResult := StringInSlice(tc.inputStr, tc.inputSlice) + if actualResult != tc.expectedInSlice { + t.Errorf("Expect value is %v but got %v. inputStr is %s, inputSlice is %v", tc.expectedInSlice, actualResult, tc.inputStr, tc.inputSlice) + } + } +} + +func TestValidateDataCacheMode(t *testing.T) { + testCases := []struct { + name string + inputStr string + expectError bool + }{ + { + name: "valid input - writethrough", + inputStr: "writethrough", + }, + { + name: "valid input - writeback", + inputStr: "writeback", + }, + { + name: "invalid input", + inputStr: "write-back not valid", + expectError: true, + }, + } + + for _, tc := range testCases { + t.Logf("test case: %s", tc.name) + err := ValidateDataCacheMode(tc.inputStr) + if err != nil && !tc.expectError { + t.Errorf("Got error %v validate data cache mode %s; expect no error", err, tc.inputStr) + } + + if err == nil && tc.expectError { + t.Errorf("Got no error validate data cache mode %s; expect an error", tc.inputStr) + } + } + +} + +func TestValidateNonNegativeInt(t *testing.T) { + testCases := []struct { + name string + cacheSize int64 + expectError bool + }{ + { + name: "valid input - positive cache size", + cacheSize: 100000, + }, + { + name: "invalid input - cachesize 0", + cacheSize: 0, + expectError: true, + }, + { + name: "invalid input - negative cache size", + cacheSize: -100, + expectError: true, + }, + } + + for _, tc := range testCases { + t.Logf("test case: %s", tc.name) + err := ValidateNonNegativeInt(tc.cacheSize) + if err != nil && !tc.expectError { + t.Errorf("Got error %v validate data cache mode %d; expect no error", err, tc.cacheSize) + } + + if err == nil && tc.expectError { + t.Errorf("Got no error validate data cache mode %d; expect an error", tc.cacheSize) + } + } + +} + +func TestIsValidDiskEncryptionKmsKey(t *testing.T) { + cases := []struct { + diskEncryptionKmsKey string + expectedIsValid bool + }{ + { + diskEncryptionKmsKey: "projects/my-project/locations/us-central1/keyRings/TestKeyRing/cryptoKeys/test-key", + expectedIsValid: true, + }, + { + diskEncryptionKmsKey: "projects/my-project/locations/global/keyRings/TestKeyRing/cryptoKeys/test-key", + expectedIsValid: true, + }, + { + diskEncryptionKmsKey: "projects/my-project/locations/keyRings/TestKeyRing/cryptoKeys/test-key", + expectedIsValid: false, + }, + } + for _, tc := range cases { + isValid := isValidDiskEncryptionKmsKey(tc.diskEncryptionKmsKey) + if tc.expectedIsValid != isValid { + t.Errorf("test failed: the provided key %s expected to be %v bu tgot %v", tc.diskEncryptionKmsKey, tc.expectedIsValid, isValid) + } + } +} + +func TestFieldsFromResourceName(t *testing.T) { + testcases := []struct { + name string + resourceName string + expectedProject string + expectedZone string + expectedName string + expectedErr bool + }{ + { + name: "StoragePool_WithValidResourceName_ReturnsFields", + resourceName: "projects/my-project/zones/us-central1-a/storagePools/storagePool-1", + expectedProject: "my-project", + expectedZone: "us-central1-a", + expectedName: "storagePool-1", + }, + { + name: "StoragePool_WithFullResourceURL_ReturnsError", + resourceName: "https://www.googleapis.com/compute/v1/projects/project/zones/zone/storagePools/storagePool", + expectedErr: true, + }, + { + name: "StoragePool_WithMissingProject_ReturnsError", + resourceName: "zones/us-central1-a/storagePools/storagePool-1", + expectedErr: true, + }, + { + name: "StoragePool_WithMissingZone_ReturnsError", + resourceName: "projects/my-project/storagePools/storagePool-1", + expectedErr: true, + }, + { + name: "StoragePool_WithMissingStoragePoolName_ReturnsError", + resourceName: "projects/my-project/zones/us-central1-a/storagePool-1", + expectedErr: true, + }, + } + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + project, zone, name, err := fieldsFromStoragePoolResourceName(tc.resourceName) + input := fmt.Sprintf("fieldsFromStoragePoolResourceName(%q)", tc.resourceName) + gotErr := err != nil + if gotErr != tc.expectedErr { + t.Errorf("%s error presence = %v, expected error presence = %v", input, gotErr, tc.expectedErr) + } + if project != tc.expectedProject || zone != tc.expectedZone || name != tc.expectedName { + t.Errorf("%s returned {project: %q, zone: %q, name: %q}, expected {project: %q, zone: %q, name: %q}", input, project, zone, name, tc.expectedProject, tc.expectedZone, tc.expectedName) + } + }) + } +} From b3393ff75d957e28e990bea46a4e26b4071b4419 Mon Sep 17 00:00:00 2001 From: juliankatz Date: Tue, 7 Oct 2025 13:59:34 -0700 Subject: [PATCH 4/4] Match unexported function in comment --- pkg/common/utils.go | 3 --- pkg/parameters/parameters.go | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/pkg/common/utils.go b/pkg/common/utils.go index c0231bd7e..a72f6bc47 100644 --- a/pkg/common/utils.go +++ b/pkg/common/utils.go @@ -402,9 +402,6 @@ func ParseZoneFromURI(zoneURI string) (string, error) { return zoneMatch[1], nil } -// StoragePoolZones returns the unique zones of the given storage pool resource names. -// Returns an error if multiple storage pools in 1 zone are found. - func UnorderedSlicesEqual(slice1 []string, slice2 []string) bool { set1 := sets.NewString(slice1...) set2 := sets.NewString(slice2...) diff --git a/pkg/parameters/parameters.go b/pkg/parameters/parameters.go index 90a8c62b7..124886849 100644 --- a/pkg/parameters/parameters.go +++ b/pkg/parameters/parameters.go @@ -449,7 +449,7 @@ func ExtractModifyVolumeParameters(parameters map[string]string) (ModifyVolumePa return modifyVolumeParams, nil } -// ConvertStringToAvailabilityClass converts a string to an availability class string. +// convertStringToAvailabilityClass converts a string to an availability class string. func convertStringToAvailabilityClass(str string) (string, error) { switch strings.ToLower(str) { case ParameterNoAvailabilityClass: