Repository: kubernetes-sigs/gcp-compute-persistent-disk-csi-driver
Pull request #1526 (2023-11-30 19:56:19)

Validate storage pools in CreateVolume
<!--  Thanks for sending a pull request!  Here are some tips for you:

1. If this is your first time, please read our contributor guidelines: https://git.k8s.io/community/contributors/guide#your-first-contribution and developer guide https://git.k8s.io/community/contributors/devel/development.md#development-guide
2. Follow the instructions for writing a release note: https://git.k8s.io/community/contributors/guide/release-notes.md
3. If the PR is unfinished, see how to mark it: https://git.k8s.io/community/contributors/guide/pull-requests.md#marking-unfinished-pull-requests
-->

**What type of PR is this?**
> Uncomment only one ` /kind <>` line, hit enter to put that in a new line, and remove leading whitespaces from that line:
>
> /kind api-change
> /kind bug
> /kind cleanup
> /kind design
> /kind documentation
> /kind failing-test

/kind feature

> /kind flake

**What this PR does / why we need it**:
This PR validates storage pools passed to the CreateVolume request in parameters. This PR contains 2 commits: 
1. Adds storage pool helper functions to common package, and change DiskParameters to use StoragePool type instead of a string.
2. Adds the validation for storage pools in CreateVolume. 

A follow up PR will add enable_storage_pool to the already existing csidriver_operation_errors metric. 

**Which issue(s) this PR fixes**:
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->
Fixes #

**Special notes for your reviewer**:

**Does this PR introduce a user-facing change?**:
<!--
If no, just write "NONE" in the release-note block below.
If yes, a release note is required:
Enter your extended release note in the block below. If the PR requires additional action from users switching to the new release, include the string "action required".
-->
```release-note
Adds validation and support for creating volumes in storage pools.
```


k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/pull/1526#" title="Author self-approved">amacaskill</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fgcp-compute-persistent-disk-csi-driver).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/blob/master/OWNERS)~~ [amacaskill]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
amacaskill: /assign @mattcary 

Patch:
diff --git a/pkg/common/parameters.go b/pkg/common/parameters.go
index a9eea224..e70639eb 100644
--- a/pkg/common/parameters.go
+++ b/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
diff --git a/pkg/common/utils.go b/pkg/common/utils.go
index 813e5838..5f35abfa 100644
--- a/pkg/common/utils.go
+++ b/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
 }
diff --git a/pkg/gce-cloud-provider/compute/gce-compute.go b/pkg/gce-cloud-provider/compute/gce-compute.go
index 61477bd7..56aa9fc3 100644
--- a/pkg/gce-cloud-provider/compute/gce-compute.go
+++ b/pkg/gce-cloud-provider/compute/gce-compute.go
@@ -454,7 +454,7 @@ func convertV1DiskToBetaDisk(v1Disk *computev1.Disk, provisionedThroughputOnCrea
 	return betaDisk
 }
 
-func convertV1DiskToAlphaDisk(v1Disk *computev1.Disk, provisionedThroughputOnCreate int64, storagePool string) *computealpha.Disk {
+func convertV1DiskToAlphaDisk(v1Disk *computev1.Disk, provisionedThroughputOnCreate int64, storagePool *common.StoragePool) *computealpha.Disk {
 	// Note: this is an incomplete list. It only includes the fields we use for disk creation.
 	alphaDisk := &computealpha.Disk{
 		Name:             v1Disk.Name,
@@ -478,8 +478,8 @@ func convertV1DiskToAlphaDisk(v1Disk *computev1.Disk, provisionedThroughputOnCre
 	if provisionedThroughputOnCreate > 0 {
 		alphaDisk.ProvisionedThroughput = provisionedThroughputOnCreate
 	}
-	if storagePool != "" {
-		alphaDisk.StoragePool = storagePool
+	if storagePool != nil {
+		alphaDisk.StoragePool = storagePool.ResourceName
 	}
 
 	return alphaDisk
@@ -669,10 +669,10 @@ func (cloud *CloudProvider) insertZonalDisk(
 		}
 	} else if gceAPIVersion == GCEAPIVersionAlpha {
 		var insertOp *computealpha.Operation
-		var storagePool string
+		var storagePool *common.StoragePool
 		if storagePoolsEnabled {
 			storagePool = common.StoragePoolInZone(params.StoragePools, diskToCreate.Zone)
-			if storagePool == "" {
+			if storagePool == nil {
 				return status.Errorf(codes.InvalidArgument, "cannot create disk in zone %q: no Storage Pools exist in zone", diskToCreate.Zone)
 			}
 		}
diff --git a/pkg/gce-pd-csi-driver/controller.go b/pkg/gce-pd-csi-driver/controller.go
index 38ba1009..5d0d4c70 100644
--- a/pkg/gce-pd-csi-driver/controller.go
+++ b/pkg/gce-pd-csi-driver/controller.go
@@ -254,6 +254,10 @@ func (gceCS *GCEControllerServer) CreateVolume(ctx context.Context, req *csi.Cre
 		gceAPIVersion = gce.GCEAPIVersionBeta
 	}
 
+	if err := validateStoragePools(req, params, gceCS.CloudProvider.GetDefaultProject()); err != nil {
+		return nil, status.Errorf(codes.InvalidArgument, "CreateVolume failed to validate storage pools: %v", err)
+	}
+
 	// Verify that the regional availability class is only used on regional disks.
 	if params.ForceAttach && params.ReplicationType != replicationTypeRegionalPD {
 		return nil, status.Errorf(codes.InvalidArgument, "invalid availabilty class for zonal disk")
@@ -1623,13 +1627,13 @@ func getZonesFromTopology(topList []*csi.Topology) ([]string, error) {
 	zones := []string{}
 	for _, top := range topList {
 		if top.GetSegments() == nil {
-			return nil, fmt.Errorf("preferred topologies specified but no segments")
+			return nil, fmt.Errorf("topologies specified but no segments")
 		}
 
 		// GCE PD cloud provider Create has no restrictions so just create in top preferred zone
 		zone, err := getZoneFromSegment(top.GetSegments())
 		if err != nil {
-			return nil, fmt.Errorf("could not get zone from preferred topology: %w", err)
+			return nil, fmt.Errorf("could not get zone from topology: %w", err)
 		}
 		zones = append(zones, zone)
 	}
diff --git a/pkg/gce-pd-csi-driver/utils.go b/pkg/gce-pd-csi-driver/utils.go
index d8ef8ce0..a33d66cc 100644
--- a/pkg/gce-pd-csi-driver/utils.go
+++ b/pkg/gce-pd-csi-driver/utils.go
@@ -23,7 +23,10 @@ import (
 
 	csi "github.com/container-storage-interface/spec/lib/go/csi"
 	"google.golang.org/grpc"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/klog/v2"
+
+	"sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/common"
 )
 
 const (
@@ -139,6 +142,68 @@ func validateAccessMode(am *csi.VolumeCapability_AccessMode) error {
 	return nil
 }
 
+func validateStoragePools(req *csi.CreateVolumeRequest, params common.DiskParameters, project string) error {
+	storagePoolsEnabled := params.StoragePools != nil
+	if !storagePoolsEnabled || req == nil {
+		return nil
+	}
+
+	if params.EnableConfidentialCompute {
+		return fmt.Errorf("storage pools do not support confidential storage")
+	}
+
+	if !(params.DiskType == "hyperdisk-balanced" || params.DiskType == "hyperdisk-throughput") {
+		return fmt.Errorf("invalid disk-type: %q. storage pools only support hyperdisk-balanced or hyperdisk-throughput", params.DiskType)
+	}
+
+	if params.ReplicationType == replicationTypeRegionalPD {
+		return fmt.Errorf("storage pools do not support regional PD")
+	}
+
+	if useVolumeCloning(req) {
+		return fmt.Errorf("storage pools do not support disk clones")
+	}
+
+	// Check that requisite zones matches the storage pools zones.
+	// This means allowedTopologies was set properly by GCW.
+	if err := validateStoragePoolZones(req, params.StoragePools); err != nil {
+		return fmt.Errorf("failed to validate storage pools zones: %v", err)
+	}
+
+	// Check that Storage Pools are in same project as the GCEClient that creates the volume.
+	if err := validateStoragePoolProjects(project, params.StoragePools); err != nil {
+		return fmt.Errorf("failed to validate storage pools projects: %v", err)
+	}
+
+	return nil
+}
+
+func validateStoragePoolZones(req *csi.CreateVolumeRequest, storagePools []common.StoragePool) error {
+	storagePoolZones, err := common.StoragePoolZones(storagePools)
+	if err != nil {
+		return err
+	}
+	reqZones, err := getZonesFromTopology(req.GetAccessibilityRequirements().GetRequisite())
+	if err != nil {
+		return err
+	}
+	if !common.UnorderedSlicesEqual(storagePoolZones, reqZones) {
+		return fmt.Errorf("requisite topologies must match storage pools zones. requisite zones: %v, storage pools zones: %v", reqZones, storagePoolZones)
+	}
+	return nil
+}
+
+func validateStoragePoolProjects(project string, storagePools []common.StoragePool) error {
+	spProjects := sets.String{}
+	for _, sp := range storagePools {
+		if sp.Project != project {
+			spProjects.Insert(sp.Project)
+			return fmt.Errorf("cross-project storage pools usage is not supported. Trying to CreateVolume in project %q with storage pools in projects %v", project, spProjects.UnsortedList())
+		}
+	}
+	return nil
+}
+
 func getMultiWriterFromCapability(vc *csi.VolumeCapability) (bool, error) {
 	if vc.GetAccessMode() == nil {
 		return false, errors.New("access mode is nil")


Test patch:
diff --git a/pkg/common/parameters_test.go b/pkg/common/parameters_test.go
index acffe5a5..98f3ff39 100644
--- a/pkg/common/parameters_test.go
+++ b/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)
 			}
 		})
 	}
diff --git a/pkg/common/utils_test.go b/pkg/common/utils_test.go
index 3f5993a0..5d7d541d 100644
--- a/pkg/common/utils_test.go
+++ b/pkg/common/utils_test.go
@@ -25,6 +25,7 @@ import (
 	"testing"
 
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta"
+	"github.com/google/go-cmp/cmp"
 	"google.golang.org/api/googleapi"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
@@ -1101,3 +1102,214 @@ func TestIsValidDiskEncryptionKmsKey(t *testing.T) {
 		}
 	}
 }
+
+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
+		expectedZones []string
+		expectedErr   bool
+	}{
+		{
+			name: "StoragePools_WithValidResourceNames_ReturnsZones",
+			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",
+				},
+			},
+			expectedZones: []string{"us-central1-a", "us-central1-b"},
+		},
+		{
+			name: "StoragePools_WithDuplicateZone_ReturnsError",
+			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-a",
+					Name:         "storagePool-2",
+					ResourceName: "projects/my-project/zones/us-central1-a/storagePools/storagePool-2",
+				},
+			},
+			expectedErr: true,
+		},
+	}
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			zones, err := StoragePoolZones(tc.storagePools)
+			input := fmt.Sprintf("StoragePoolZones(%q)", tc.storagePools)
+			gotErr := err != nil
+			if gotErr != tc.expectedErr {
+				t.Errorf("%s error presence = %v, expected error presence = %v", input, gotErr, tc.expectedErr)
+			}
+			if diff := cmp.Diff(tc.expectedZones, zones); diff != "" {
+				t.Errorf("%s: -want err, +got err\n%s", input, diff)
+			}
+		})
+	}
+}
+
+func TestStoragePoolInZone(t *testing.T) {
+	testcases := []struct {
+		name                string
+		storagePools        []StoragePool
+		zone                string
+		expectedStoragePool *StoragePool
+		expectedErr         bool
+	}{
+		{
+			name: "ValidStoragePools_ReturnsStoragePoolInZone",
+			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",
+				},
+			},
+			zone: "us-central1-a",
+			expectedStoragePool: &StoragePool{
+				Project:      "my-project",
+				Zone:         "us-central1-a",
+				Name:         "storagePool-1",
+				ResourceName: "projects/my-project/zones/us-central1-a/storagePools/storagePool-1",
+			},
+		},
+		{
+			name: "StoragePoolNotInZone_ReturnsNil",
+			storagePools: []StoragePool{
+				{
+					Project:      "my-project",
+					Zone:         "us-central1-a",
+					Name:         "storagePool-1",
+					ResourceName: "projects/my-project/zones/us-central1-a/storagePools/storagePool-1",
+				},
+			},
+			zone:                "us-central1-b",
+			expectedStoragePool: nil,
+		},
+	}
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			sp := 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)
+			}
+		})
+	}
+}
+
+func TestUnorderedSlicesEqual(t *testing.T) {
+	testcases := []struct {
+		name                string
+		slice1              []string
+		slice2              []string
+		expectedSlicesEqual bool
+	}{
+		{
+			name:                "OrderedSlicesEqual_ReturnsTrue",
+			slice1:              []string{"us-central1-a", "us-central1-b"},
+			slice2:              []string{"us-central1-a", "us-central1-b"},
+			expectedSlicesEqual: true,
+		},
+		{
+			name:                "UnorderedSlicesEqual_ReturnsTrue",
+			slice1:              []string{"us-central1-a", "us-central1-b"},
+			slice2:              []string{"us-central1-b", "us-central1-a"},
+			expectedSlicesEqual: true,
+		},
+		{
+			name:                "SlicesNotEqualSameLength_ReturnsFalse",
+			slice1:              []string{"us-central1-a", "us-central1-b"},
+			slice2:              []string{"us-central1-a", "us-central1-a"},
+			expectedSlicesEqual: false,
+		},
+		{
+			name:                "SlicesNotEqualDifferentLength_ReturnsFalse",
+			slice1:              []string{"us-central1-a"},
+			slice2:              []string{},
+			expectedSlicesEqual: false,
+		},
+	}
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			slicesEqual := UnorderedSlicesEqual(tc.slice1, tc.slice2)
+			input := fmt.Sprintf("UnorderedSlicesEqual(%v, %v)", tc.slice1, tc.slice2)
+			if diff := cmp.Diff(tc.expectedSlicesEqual, slicesEqual); diff != "" {
+				t.Errorf("%s: -want, +got \n%s", input, diff)
+			}
+		})
+	}
+}
diff --git a/pkg/gce-pd-csi-driver/controller_test.go b/pkg/gce-pd-csi-driver/controller_test.go
index 8c5e611b..7430d0ea 100644
--- a/pkg/gce-pd-csi-driver/controller_test.go
+++ b/pkg/gce-pd-csi-driver/controller_test.go
@@ -877,6 +877,59 @@ func TestCreateVolumeArguments(t *testing.T) {
 			},
 			expErrCode: codes.InvalidArgument,
 		},
+		{
+			name: "success with storage pools parameter",
+			req: &csi.CreateVolumeRequest{
+				Name:               name,
+				CapacityRange:      stdCapRange,
+				VolumeCapabilities: stdVolCaps,
+				Parameters:         map[string]string{"storage-pools": "projects/test-project/zones/us-central1-a/storagePools/storagePool-1", "type": "hyperdisk-balanced"},
+				AccessibilityRequirements: &csi.TopologyRequirement{
+					Requisite: []*csi.Topology{
+						{
+							Segments: map[string]string{common.TopologyKeyZone: "us-central1-a"},
+						},
+					},
+					Preferred: []*csi.Topology{
+						{
+							Segments: map[string]string{common.TopologyKeyZone: "us-central1-a"},
+						},
+					},
+				},
+			},
+			expVol: &csi.Volume{
+				CapacityBytes: common.GbToBytes(20),
+				VolumeId:      "projects/test-project/zones/us-central1-a/disks/test-name",
+				VolumeContext: nil,
+				AccessibleTopology: []*csi.Topology{
+					{
+						Segments: map[string]string{common.TopologyKeyZone: "us-central1-a"},
+					},
+				},
+			},
+		},
+		{
+			name: "fail with invalid storage pools parameter",
+			req: &csi.CreateVolumeRequest{
+				Name:               name,
+				CapacityRange:      stdCapRange,
+				VolumeCapabilities: stdVolCaps,
+				Parameters:         map[string]string{"storage-pools": "zones/us-central1-a/storagePools/storagePool-1", "type": "hyperdisk-balanced"},
+				AccessibilityRequirements: &csi.TopologyRequirement{
+					Requisite: []*csi.Topology{
+						{
+							Segments: map[string]string{common.TopologyKeyZone: "us-central1-a"},
+						},
+					},
+					Preferred: []*csi.Topology{
+						{
+							Segments: map[string]string{common.TopologyKeyZone: "us-central1-a"},
+						},
+					},
+				},
+			},
+			expErrCode: codes.InvalidArgument,
+		},
 	}
 
 	// Run test cases
@@ -1312,6 +1365,7 @@ func TestCreateVolumeWithVolumeSourceFromVolume(t *testing.T) {
 		name                 string
 		volumeOnCloud        bool
 		expErrCode           codes.Code
+		expErrMsg            string
 		sourceVolumeID       string
 		reqParameters        map[string]string
 		sourceReqParameters  map[string]string
@@ -1400,6 +1454,30 @@ func TestCreateVolumeWithVolumeSourceFromVolume(t *testing.T) {
 				},
 			},
 		},
+		{
+			name:                 "fail cloning with storage pools",
+			volumeOnCloud:        true,
+			sourceVolumeID:       testZonalVolumeSourceID,
+			requestCapacityRange: stdCapRange,
+			sourceCapacityRange:  stdCapRange,
+			reqParameters: map[string]string{
+				common.ParameterKeyType:                 "test-type",
+				common.ParameterKeyReplicationType:      replicationTypeNone,
+				common.ParameterKeyDiskEncryptionKmsKey: "encryption-key",
+				common.ParameterKeyStoragePools:         "projects/test-project/zones/country-region-zone/storagePools/storagePool-1",
+			},
+			sourceReqParameters: zonalParams,
+			sourceTopology: &csi.TopologyRequirement{
+				Requisite: requisiteTopology,
+				Preferred: prefTopology,
+			},
+			requestTopology: &csi.TopologyRequirement{
+				Requisite: requisiteTopology,
+				Preferred: prefTopology,
+			},
+			expErrCode: codes.InvalidArgument,
+			expErrMsg:  "storage pools do not support disk clones",
+		},
 		{
 			name:                 "success zonal -> zonal cloning, req = all zones in region, pref = req w/ src zone as first element: delayed binding without allowedTopologies",
 			volumeOnCloud:        true,
@@ -1851,6 +1929,9 @@ func TestCreateVolumeWithVolumeSourceFromVolume(t *testing.T) {
 		if tc.expErrCode != codes.OK {
 			t.Fatalf("Expected error: %v, got no error", tc.expErrCode)
 		}
+		if tc.expErrMsg != "" && tc.expErrMsg != err.Error() {
+			t.Fatalf("Got error: %v, expected error: %v", err.Error(), tc.expErrMsg)
+		}
 
 		// Make sure the response has the source volume.
 		respVolume := resp.GetVolume()
diff --git a/pkg/gce-pd-csi-driver/utils_test.go b/pkg/gce-pd-csi-driver/utils_test.go
index 2a0fd5f4..fd114fc8 100644
--- a/pkg/gce-pd-csi-driver/utils_test.go
+++ b/pkg/gce-pd-csi-driver/utils_test.go
@@ -18,9 +18,12 @@ limitations under the License.
 package gceGCEDriver
 
 import (
+	"fmt"
 	"testing"
 
 	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"
 )
 
 var (
@@ -291,3 +294,432 @@ func TestGetReadOnlyFromCapabilities(t *testing.T) {
 		}
 	}
 }
+
+func TestValidateStoragePools(t *testing.T) {
+	testCases := []struct {
+		name    string
+		req     *csi.CreateVolumeRequest
+		params  common.DiskParameters
+		project string
+		expErr  error
+	}{
+		{
+			name: "success with storage pools not enabled",
+			req: &csi.CreateVolumeRequest{
+				Name: "test-name",
+			},
+			params: common.DiskParameters{
+				DiskType: "hyperdisk-balanced",
+			},
+			expErr: nil,
+		},
+		{
+			name: "success with nil CreateVolumeReq",
+			req:  nil,
+			params: common.DiskParameters{
+				DiskType: "hyperdisk-balanced",
+			},
+			expErr: nil,
+		},
+		{
+			name: "fail storage pools with confidential storage",
+			req: &csi.CreateVolumeRequest{
+				Name: "test-name",
+			},
+			params: common.DiskParameters{
+				DiskType: "hyperdisk-balanced",
+				StoragePools: []common.StoragePool{
+					{
+						Project:      "test-project",
+						Zone:         "us-central1-a",
+						Name:         "storagePool-1",
+						ResourceName: "projects/test-project/zones/us-central1-a/storagePools/storagePool-1",
+					},
+					{
+						Project:      "test-project",
+						Zone:         "us-central1-b",
+						Name:         "storagePool-2",
+						ResourceName: "projects/test-project/zones/us-central1-a/storagePools/storagePool-1",
+					},
+				},
+				EnableConfidentialCompute: true,
+			},
+			project: "test-project",
+			expErr:  fmt.Errorf("storage pools do not support confidential storage"),
+		},
+		{
+			name: "fail storage pools with disk type other than HdB/HdT",
+			req: &csi.CreateVolumeRequest{
+				Name: "test-name",
+			},
+			params: common.DiskParameters{
+				DiskType: "pd-balanced",
+				StoragePools: []common.StoragePool{
+					{
+						Project:      "test-project",
+						Zone:         "us-central1-a",
+						Name:         "storagePool-1",
+						ResourceName: "projects/test-project/zones/us-central1-a/storagePools/storagePool-1",
+					},
+					{
+						Project:      "test-project",
+						Zone:         "us-central1-b",
+						Name:         "storagePool-2",
+						ResourceName: "projects/test-project/zones/us-central1-a/storagePools/storagePool-1",
+					},
+				},
+			},
+			project: "test-project",
+			expErr:  fmt.Errorf("invalid disk-type: \"pd-balanced\". storage pools only support hyperdisk-balanced or hyperdisk-throughput"),
+		},
+		{
+			name: "fail storage pools with regional PD",
+			req: &csi.CreateVolumeRequest{
+				Name: "test-name",
+			},
+			params: common.DiskParameters{
+				DiskType:        "hyperdisk-balanced",
+				ReplicationType: "regional-pd",
+				StoragePools: []common.StoragePool{
+					{
+						Project:      "test-project",
+						Zone:         "us-central1-a",
+						Name:         "storagePool-1",
+						ResourceName: "projects/test-project/zones/us-central1-a/storagePools/storagePool-1",
+					},
+					{
+						Project:      "test-project",
+						Zone:         "us-central1-b",
+						Name:         "storagePool-2",
+						ResourceName: "projects/test-project/zones/us-central1-a/storagePools/storagePool-1",
+					},
+				},
+			},
+			project: "test-project",
+			expErr:  fmt.Errorf("storage pools do not support regional PD"),
+		},
+		{
+			name: "fail storage pools with disk clones",
+			req: &csi.CreateVolumeRequest{
+				Name: "test-name",
+				VolumeContentSource: &csi.VolumeContentSource{
+					Type: &csi.VolumeContentSource_Volume{
+						Volume: &csi.VolumeContentSource_VolumeSource{
+							VolumeId: "projects/test-project/zones/us-central1-a/disks/disk-1",
+						},
+					},
+				},
+			},
+			params: common.DiskParameters{
+				DiskType: "hyperdisk-balanced",
+				StoragePools: []common.StoragePool{
+					{
+						Project:      "test-project",
+						Zone:         "us-central1-a",
+						Name:         "storagePool-1",
+						ResourceName: "projects/test-project/zones/us-central1-a/storagePools/storagePool-1",
+					},
+					{
+						Project:      "test-project",
+						Zone:         "us-central1-b",
+						Name:         "storagePool-2",
+						ResourceName: "projects/test-project/zones/us-central1-a/storagePools/storagePool-1",
+					},
+				},
+			},
+			project: "test-project",
+			expErr:  fmt.Errorf("storage pools do not support disk clones"),
+		},
+		{
+			name: "fail storage pools zones, requisite zones mismatch",
+			req: &csi.CreateVolumeRequest{
+				Name: "test-name",
+				AccessibilityRequirements: &csi.TopologyRequirement{
+					Requisite: []*csi.Topology{
+						{
+							Segments: map[string]string{common.TopologyKeyZone: "us-central1-a"},
+						},
+					},
+					Preferred: []*csi.Topology{
+						{
+							Segments: map[string]string{common.TopologyKeyZone: "us-central1-a"},
+						},
+						{
+							Segments: map[string]string{common.TopologyKeyZone: "us-central1-b"},
+						},
+					},
+				},
+			},
+			params: common.DiskParameters{
+				DiskType: "hyperdisk-balanced",
+				StoragePools: []common.StoragePool{
+					{
+						Project:      "test-project",
+						Zone:         "us-central1-a",
+						Name:         "storagePool-1",
+						ResourceName: "projects/test-project/zones/us-central1-a/storagePools/storagePool-1",
+					},
+					{
+						Project:      "test-project",
+						Zone:         "us-central1-b",
+						Name:         "storagePool-2",
+						ResourceName: "projects/test-project/zones/us-central1-a/storagePools/storagePool-1",
+					},
+				},
+			},
+			project: "test-project",
+			expErr:  fmt.Errorf("failed to validate storage pools zones: requisite topologies must match storage pools zones. requisite zones: [us-central1-a], storage pools zones: [us-central1-a us-central1-b]"),
+		},
+		{
+			name: "fail storage pools cross-project usage",
+			req: &csi.CreateVolumeRequest{
+				Name: "test-name",
+				AccessibilityRequirements: &csi.TopologyRequirement{
+					Requisite: []*csi.Topology{
+						{
+							Segments: map[string]string{common.TopologyKeyZone: "us-central1-a"},
+						},
+						{
+							Segments: map[string]string{common.TopologyKeyZone: "us-central1-b"},
+						},
+					},
+					Preferred: []*csi.Topology{
+						{
+							Segments: map[string]string{common.TopologyKeyZone: "us-central1-a"},
+						},
+						{
+							Segments: map[string]string{common.TopologyKeyZone: "us-central1-b"},
+						},
+					},
+				},
+			},
+			params: common.DiskParameters{
+				DiskType: "hyperdisk-balanced",
+				StoragePools: []common.StoragePool{
+					{
+						Project:      "test-project",
+						Zone:         "us-central1-a",
+						Name:         "storagePool-1",
+						ResourceName: "projects/test-project/zones/us-central1-a/storagePools/storagePool-1",
+					},
+					{
+						Project:      "test-project",
+						Zone:         "us-central1-b",
+						Name:         "storagePool-2",
+						ResourceName: "projects/test-project/zones/us-central1-a/storagePools/storagePool-1",
+					},
+				},
+			},
+			project: "other-project",
+			expErr:  fmt.Errorf("failed to validate storage pools projects: cross-project storage pools usage is not supported. Trying to CreateVolume in project \"other-project\" with storage pools in projects [test-project]"),
+		},
+		{
+			name: "success validateStoragePools",
+			req: &csi.CreateVolumeRequest{
+				Name: "test-name",
+				AccessibilityRequirements: &csi.TopologyRequirement{
+					Requisite: []*csi.Topology{
+						{
+							Segments: map[string]string{common.TopologyKeyZone: "us-central1-a"},
+						},
+						{
+							Segments: map[string]string{common.TopologyKeyZone: "us-central1-b"},
+						},
+					},
+					Preferred: []*csi.Topology{
+						{
+							Segments: map[string]string{common.TopologyKeyZone: "us-central1-a"},
+						},
+						{
+							Segments: map[string]string{common.TopologyKeyZone: "us-central1-b"},
+						},
+					},
+				},
+			},
+			params: common.DiskParameters{
+				DiskType: "hyperdisk-balanced",
+				StoragePools: []common.StoragePool{
+					{
+						Project:      "test-project",
+						Zone:         "us-central1-a",
+						Name:         "storagePool-1",
+						ResourceName: "projects/test-project/zones/us-central1-a/storagePools/storagePool-1",
+					},
+					{
+						Project:      "test-project",
+						Zone:         "us-central1-b",
+						Name:         "storagePool-2",
+						ResourceName: "projects/test-project/zones/us-central1-a/storagePools/storagePool-1",
+					},
+				},
+			},
+			project: "test-project",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Logf("Running test: %v", tc.name)
+		input := "validateStoragePools()"
+		err := validateStoragePools(tc.req, tc.params, tc.project)
+		if tc.expErr != nil && err == nil {
+			t.Fatalf("%s didn't get any error, but expected error %v", input, tc.expErr)
+		}
+		if tc.expErr == nil && err != nil {
+			t.Fatalf("%s got error %v, but didn't expect any error", input, err)
+		}
+		if err != nil && tc.expErr != nil {
+			if diff := cmp.Diff(err.Error(), tc.expErr.Error()); diff != "" {
+				t.Errorf("%s: -want, +got \n%s", input, diff)
+			}
+		}
+	}
+}
+
+func TestValidateStoragePoolZones(t *testing.T) {
+	testCases := []struct {
+		name         string
+		req          *csi.CreateVolumeRequest
+		storagePools []common.StoragePool
+		expErr       error
+	}{
+		{
+			name: "fail with 2 storage pools in 1 zone",
+			req: &csi.CreateVolumeRequest{
+				Name: "test-name",
+				AccessibilityRequirements: &csi.TopologyRequirement{
+					Requisite: []*csi.Topology{
+						{
+							Segments: map[string]string{common.TopologyKeyZone: "us-central1-a"},
+						},
+					},
+					Preferred: []*csi.Topology{
+						{
+							Segments: map[string]string{common.TopologyKeyZone: "us-central1-a"},
+						},
+					},
+				},
+			},
+			storagePools: []common.StoragePool{
+				{
+					Project:      "test-project",
+					Zone:         "us-central1-a",
+					Name:         "storagePool-1",
+					ResourceName: "projects/test-project/zones/us-central1-a/storagePools/storagePool-1",
+				},
+				{
+					Project:      "test-project",
+					Zone:         "us-central1-a",
+					Name:         "storagePool-2",
+					ResourceName: "projects/test-project/zones/us-central1-a/storagePools/storagePool-1",
+				},
+			},
+			expErr: fmt.Errorf("found multiple storage pools in zone us-central1-a. Only one storage pool per zone is allowed"),
+		},
+		{
+			name: "fail with requisite topology with no segments",
+			req: &csi.CreateVolumeRequest{
+				Name: "test-name",
+				AccessibilityRequirements: &csi.TopologyRequirement{
+					Requisite: []*csi.Topology{{}},
+					Preferred: []*csi.Topology{
+						{
+							Segments: map[string]string{common.TopologyKeyZone: "us-central1-a"},
+						},
+					},
+				},
+			},
+			storagePools: []common.StoragePool{
+				{
+					Project:      "test-project",
+					Zone:         "us-central1-a",
+					Name:         "storagePool-1",
+					ResourceName: "projects/test-project/zones/us-central1-a/storagePools/storagePool-1",
+				},
+			},
+			expErr: fmt.Errorf("topologies specified but no segments"),
+		},
+		{
+			name: "fail with requisite zones does not match storage pools zones",
+			req: &csi.CreateVolumeRequest{
+				Name: "test-name",
+				AccessibilityRequirements: &csi.TopologyRequirement{
+					Requisite: []*csi.Topology{
+						{
+							Segments: map[string]string{common.TopologyKeyZone: "us-central1-a"},
+						},
+					},
+					Preferred: []*csi.Topology{
+						{
+							Segments: map[string]string{common.TopologyKeyZone: "us-central1-a"},
+						},
+					},
+				},
+			},
+			storagePools: []common.StoragePool{
+				{
+					Project:      "test-project",
+					Zone:         "us-central1-b",
+					Name:         "storagePool-1",
+					ResourceName: "projects/test-project/zones/us-central1-b/storagePools/storagePool-1",
+				},
+			},
+			expErr: fmt.Errorf("requisite topologies must match storage pools zones. requisite zones: [us-central1-a], storage pools zones: [us-central1-b]"),
+		},
+		{
+			name: "success validateStoragePoolZones",
+			req: &csi.CreateVolumeRequest{
+				Name: "test-name",
+				AccessibilityRequirements: &csi.TopologyRequirement{
+					Requisite: []*csi.Topology{
+						{
+							Segments: map[string]string{common.TopologyKeyZone: "us-central1-a"},
+						},
+						{
+							Segments: map[string]string{common.TopologyKeyZone: "us-central1-b"},
+						},
+					},
+					Preferred: []*csi.Topology{
+						{
+							Segments: map[string]string{common.TopologyKeyZone: "us-central1-a"},
+						},
+						{
+							Segments: map[string]string{common.TopologyKeyZone: "us-central1-b"},
+						},
+					},
+				},
+			},
+			storagePools: []common.StoragePool{
+				{
+					Project:      "test-project",
+					Zone:         "us-central1-a",
+					Name:         "storagePool-1",
+					ResourceName: "projects/test-project/zones/us-central1-a/storagePools/storagePool-1",
+				},
+				{
+					Project:      "test-project",
+					Zone:         "us-central1-b",
+					Name:         "storagePool-2",
+					ResourceName: "projects/test-project/zones/us-central1-a/storagePools/storagePool-1",
+				},
+			},
+			expErr: nil,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Logf("Running test: %v", tc.name)
+		err := validateStoragePoolZones(tc.req, tc.storagePools)
+		input := "validateStoragePoolZones()"
+		if tc.expErr != nil && err == nil {
+			t.Fatalf("%s didn't get any error, but expected error %v", input, tc.expErr)
+		}
+		if tc.expErr == nil && err != nil {
+			t.Fatalf("%s got error %v, but didn't expect any error", input, err)
+		}
+		if err != nil && tc.expErr != nil {
+			if diff := cmp.Diff(err.Error(), tc.expErr.Error()); diff != "" {
+				t.Errorf("%s: -want, +got \n%s", input, diff)
+			}
+		}
+	}
+}