Merge pull request #1526 from amacaskill/createVolumeStoragePools

Validate storage pools in CreateVolume

Author: k8s-ci-robot

pkg/common/parameters.go

@@ -97,7 +97,7 @@ type DiskParameters struct {
 	ForceAttach bool
 	// Values: {[]string}
 	// Default: ""
-	StoragePools []string
+	StoragePools []StoragePool
 }
 
 // SnapshotParameters contains normalized and defaulted parameters for snapshots
@@ -109,6 +109,13 @@ type SnapshotParameters struct {
 	Labels           map[string]string
 }
 
+type StoragePool struct {
+	Project      string
+	Zone         string
+	Name         string
+	ResourceName string
+}
+
 // ExtractAndDefaultParameters will take the relevant parameters from a map and
 // put them into a well defined struct making sure to default unspecified fields.
 // extraVolumeLabels are added as labels; if there are also labels specified in

pkg/common/parameters_test.go

@@ -19,6 +19,8 @@ package common
 import (
 	"reflect"
 	"testing"
+
+	"github.com/google/go-cmp/cmp"
 )
 
 func TestExtractAndDefaultParameters(t *testing.T) {
@@ -199,6 +201,67 @@ func TestExtractAndDefaultParameters(t *testing.T) {
 				Labels:          map[string]string{},
 			},
 		},
+		{
+			name:       "storage pool parameters",
+			parameters: map[string]string{ParameterKeyType: "hyperdisk-balanced", ParameterKeyStoragePools: "projects/my-project/zones/us-central1-a/storagePools/storagePool-1,projects/my-project/zones/us-central1-b/storagePools/storagePool-2"},
+			labels:     map[string]string{},
+			expectParams: DiskParameters{
+				DiskType:        "hyperdisk-balanced",
+				ReplicationType: "none",
+				Tags:            map[string]string{},
+				Labels:          map[string]string{},
+				StoragePools: []StoragePool{
+					{
+						Project:      "my-project",
+						Zone:         "us-central1-a",
+						Name:         "storagePool-1",
+						ResourceName: "projects/my-project/zones/us-central1-a/storagePools/storagePool-1",
+					},
+					{
+						Project:      "my-project",
+						Zone:         "us-central1-b",
+						Name:         "storagePool-2",
+						ResourceName: "projects/my-project/zones/us-central1-b/storagePools/storagePool-2",
+					},
+				},
+			},
+		},
+		{
+			name:       "invalid storage pool parameters, starts with /projects instead of projects",
+			parameters: map[string]string{ParameterKeyType: "hyperdisk-balanced", ParameterKeyStoragePools: "/projects/my-project/zones/us-central1-a/storagePools/storagePool-1"},
+			labels:     map[string]string{},
+			expectErr:  true,
+		},
+		{
+			name:       "invalid storage pool parameters, missing projects",
+			parameters: map[string]string{ParameterKeyType: "hyperdisk-balanced", ParameterKeyStoragePools: "zones/us-central1-a/storagePools/storagePool-1"},
+			labels:     map[string]string{},
+			expectErr:  true,
+		},
+		{
+			name:       "invalid storage pool parameters, missing zones",
+			parameters: map[string]string{ParameterKeyType: "hyperdisk-balanced", ParameterKeyStoragePools: "projects/my-project/storagePools/storagePool-1"},
+			labels:     map[string]string{},
+			expectErr:  true,
+		},
+		{
+			name:       "invalid storage pool parameters, duplicate projects",
+			parameters: map[string]string{ParameterKeyType: "hyperdisk-balanced", ParameterKeyStoragePools: "projects/my-project/projects/my-project/storagePools/storagePool-1"},
+			labels:     map[string]string{},
+			expectErr:  true,
+		},
+		{
+			name:       "invalid storage pool parameters, duplicate zones",
+			parameters: map[string]string{ParameterKeyType: "hyperdisk-balanced", ParameterKeyStoragePools: "zones/us-central1-a/zones/us-central1-a/storagePools/storagePool-1"},
+			labels:     map[string]string{},
+			expectErr:  true,
+		},
+		{
+			name:       "invalid storage pool parameters, duplicate storagePools",
+			parameters: map[string]string{ParameterKeyType: "hyperdisk-balanced", ParameterKeyStoragePools: "projects/my-project/storagePools/us-central1-a/storagePools/storagePool-1"},
+			labels:     map[string]string{},
+			expectErr:  true,
+		},
 	}
 
 	for _, tc := range tests {
@@ -211,8 +274,8 @@ func TestExtractAndDefaultParameters(t *testing.T) {
 				return
 			}
 
-			if !reflect.DeepEqual(p, tc.expectParams) {
-				t.Errorf("ExtractAndDefaultParameters(%+v) = %v; expected params: %v", tc.parameters, p, tc.expectParams)
+			if diff := cmp.Diff(p, tc.expectParams); diff != "" {
+				t.Errorf("ExtractAndDefaultParameters(%+v): -want, +got \n%s", tc.parameters, diff)
 			}
 		})
 	}

pkg/common/utils.go

@@ -80,6 +80,8 @@ var (
 	//   zones/zone/machineTypes/machine-type
 	machineTypeRegex = regexp.MustCompile(machineTypePattern)
 
+	storagePoolFieldsRegex = regexp.MustCompile(`^projects/([^/]+)/zones/([^/]+)/storagePools/([^/]+)$`)
+
 	// userErrorCodeMap tells how API error types are translated to error codes.
 	userErrorCodeMap = map[int]codes.Code{
 		http.StatusForbidden:       codes.PermissionDenied,
@@ -393,15 +395,72 @@ func isValidDiskEncryptionKmsKey(DiskEncryptionKmsKey string) bool {
 	return kmsKeyPattern.MatchString(DiskEncryptionKmsKey)
 }
 
-// TODO(amacaskill): Implement this function.
 // 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) ([]string, error) {
-	return nil, status.Errorf(codes.Unimplemented, "")
+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
 }
 
-// TODO(amacaskill): Implement this function.
-func StoragePoolInZone(storagePools []string, zone string) string {
-	return ""
+// 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...)
+	set2 := sets.NewString(slice2...)
+	spZonesNotInReq := set1.Difference(set2)
+	if spZonesNotInReq.Len() != 0 {
+		return false
+	}
+	return true
 }