Skip to content

Commit fc7a6b5

Browse files
amacaskillamacaskill
committed
validate storage pools in CreateVolume
1 parent ee3a409 commit fc7a6b5

File tree

5 files changed

+615
-3
lines changed

5 files changed

+615
-3
lines changed

‎pkg/common/parameters_test.go

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -227,11 +227,41 @@ func TestExtractAndDefaultParameters(t *testing.T) {
227227
},
228228
},
229229
{
230-
name: "invalid storage pool parameters",
230+
name: "invalid storage pool parameters, starts with /projects instead of projects",
231+
parameters: map[string]string{ParameterKeyType: "hyperdisk-balanced", ParameterKeyStoragePools: "/projects/my-project/zones/us-central1-a/storagePools/storagePool-1"},
232+
labels: map[string]string{},
233+
expectErr: true,
234+
},
235+
{
236+
name: "invalid storage pool parameters, missing projects",
231237
parameters: map[string]string{ParameterKeyType: "hyperdisk-balanced", ParameterKeyStoragePools: "zones/us-central1-a/storagePools/storagePool-1"},
232238
labels: map[string]string{},
233239
expectErr: true,
234240
},
241+
{
242+
name: "invalid storage pool parameters, missing zones",
243+
parameters: map[string]string{ParameterKeyType: "hyperdisk-balanced", ParameterKeyStoragePools: "projects/my-project/storagePools/storagePool-1"},
244+
labels: map[string]string{},
245+
expectErr: true,
246+
},
247+
{
248+
name: "invalid storage pool parameters, duplicate projects",
249+
parameters: map[string]string{ParameterKeyType: "hyperdisk-balanced", ParameterKeyStoragePools: "projects/my-project/projects/my-project/storagePools/storagePool-1"},
250+
labels: map[string]string{},
251+
expectErr: true,
252+
},
253+
{
254+
name: "invalid storage pool parameters, duplicate zones",
255+
parameters: map[string]string{ParameterKeyType: "hyperdisk-balanced", ParameterKeyStoragePools: "zones/us-central1-a/zones/us-central1-a/storagePools/storagePool-1"},
256+
labels: map[string]string{},
257+
expectErr: true,
258+
},
259+
{
260+
name: "invalid storage pool parameters, duplicate storagePools",
261+
parameters: map[string]string{ParameterKeyType: "hyperdisk-balanced", ParameterKeyStoragePools: "projects/my-project/storagePools/us-central1-a/storagePools/storagePool-1"},
262+
labels: map[string]string{},
263+
expectErr: true,
264+
},
235265
}
236266

237267
for _, tc := range tests {

‎pkg/gce-pd-csi-driver/controller.go

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -254,6 +254,10 @@ func (gceCS *GCEControllerServer) CreateVolume(ctx context.Context, req *csi.Cre
254254
gceAPIVersion = gce.GCEAPIVersionBeta
255255
}
256256

257+
if err := validateStoragePools(req, params, gceCS.CloudProvider.GetDefaultProject()); err != nil {
258+
return nil, status.Errorf(codes.InvalidArgument, "CreateVolume failed to validate storage pools: %v", err)
259+
}
260+
257261
// Verify that the regional availability class is only used on regional disks.
258262
if params.ForceAttach && params.ReplicationType != replicationTypeRegionalPD {
259263
return nil, status.Errorf(codes.InvalidArgument, "invalid availabilty class for zonal disk")
@@ -1623,13 +1627,13 @@ func getZonesFromTopology(topList []*csi.Topology) ([]string, error) {
16231627
zones := []string{}
16241628
for _, top := range topList {
16251629
if top.GetSegments() == nil {
1626-
return nil, fmt.Errorf("preferred topologies specified but no segments")
1630+
return nil, fmt.Errorf("topologies specified but no segments")
16271631
}
16281632

16291633
// GCE PD cloud provider Create has no restrictions so just create in top preferred zone
16301634
zone, err := getZoneFromSegment(top.GetSegments())
16311635
if err != nil {
1632-
return nil, fmt.Errorf("could not get zone from preferred topology: %w", err)
1636+
return nil, fmt.Errorf("could not get zone from topology: %w", err)
16331637
}
16341638
zones = append(zones, zone)
16351639
}

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

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -877,6 +877,59 @@ func TestCreateVolumeArguments(t *testing.T) {
877877
},
878878
expErrCode: codes.InvalidArgument,
879879
},
880+
{
881+
name: "success with storage pools parameter",
882+
req: &csi.CreateVolumeRequest{
883+
Name: name,
884+
CapacityRange: stdCapRange,
885+
VolumeCapabilities: stdVolCaps,
886+
Parameters: map[string]string{"storage-pools": "projects/test-project/zones/us-central1-a/storagePools/storagePool-1", "type": "hyperdisk-balanced"},
887+
AccessibilityRequirements: &csi.TopologyRequirement{
888+
Requisite: []*csi.Topology{
889+
{
890+
Segments: map[string]string{common.TopologyKeyZone: "us-central1-a"},
891+
},
892+
},
893+
Preferred: []*csi.Topology{
894+
{
895+
Segments: map[string]string{common.TopologyKeyZone: "us-central1-a"},
896+
},
897+
},
898+
},
899+
},
900+
expVol: &csi.Volume{
901+
CapacityBytes: common.GbToBytes(20),
902+
VolumeId: "projects/test-project/zones/us-central1-a/disks/test-name",
903+
VolumeContext: nil,
904+
AccessibleTopology: []*csi.Topology{
905+
{
906+
Segments: map[string]string{common.TopologyKeyZone: "us-central1-a"},
907+
},
908+
},
909+
},
910+
},
911+
{
912+
name: "fail with invalid storage pools parameter",
913+
req: &csi.CreateVolumeRequest{
914+
Name: name,
915+
CapacityRange: stdCapRange,
916+
VolumeCapabilities: stdVolCaps,
917+
Parameters: map[string]string{"storage-pools": "zones/us-central1-a/storagePools/storagePool-1", "type": "hyperdisk-balanced"},
918+
AccessibilityRequirements: &csi.TopologyRequirement{
919+
Requisite: []*csi.Topology{
920+
{
921+
Segments: map[string]string{common.TopologyKeyZone: "us-central1-a"},
922+
},
923+
},
924+
Preferred: []*csi.Topology{
925+
{
926+
Segments: map[string]string{common.TopologyKeyZone: "us-central1-a"},
927+
},
928+
},
929+
},
930+
},
931+
expErrCode: codes.InvalidArgument,
932+
},
880933
}
881934

882935
// Run test cases
@@ -1312,6 +1365,7 @@ func TestCreateVolumeWithVolumeSourceFromVolume(t *testing.T) {
13121365
name string
13131366
volumeOnCloud bool
13141367
expErrCode codes.Code
1368+
expErrMsg string
13151369
sourceVolumeID string
13161370
reqParameters map[string]string
13171371
sourceReqParameters map[string]string
@@ -1400,6 +1454,30 @@ func TestCreateVolumeWithVolumeSourceFromVolume(t *testing.T) {
14001454
},
14011455
},
14021456
},
1457+
{
1458+
name: "fail cloning with storage pools",
1459+
volumeOnCloud: true,
1460+
sourceVolumeID: testZonalVolumeSourceID,
1461+
requestCapacityRange: stdCapRange,
1462+
sourceCapacityRange: stdCapRange,
1463+
reqParameters: map[string]string{
1464+
common.ParameterKeyType: "test-type",
1465+
common.ParameterKeyReplicationType: replicationTypeNone,
1466+
common.ParameterKeyDiskEncryptionKmsKey: "encryption-key",
1467+
common.ParameterKeyStoragePools: "projects/test-project/zones/country-region-zone/storagePools/storagePool-1",
1468+
},
1469+
sourceReqParameters: zonalParams,
1470+
sourceTopology: &csi.TopologyRequirement{
1471+
Requisite: requisiteTopology,
1472+
Preferred: prefTopology,
1473+
},
1474+
requestTopology: &csi.TopologyRequirement{
1475+
Requisite: requisiteTopology,
1476+
Preferred: prefTopology,
1477+
},
1478+
expErrCode: codes.InvalidArgument,
1479+
expErrMsg: "storage pools do not support disk clones",
1480+
},
14031481
{
14041482
name: "success zonal -> zonal cloning, req = all zones in region, pref = req w/ src zone as first element: delayed binding without allowedTopologies",
14051483
volumeOnCloud: true,
@@ -1851,6 +1929,9 @@ func TestCreateVolumeWithVolumeSourceFromVolume(t *testing.T) {
18511929
if tc.expErrCode != codes.OK {
18521930
t.Fatalf("Expected error: %v, got no error", tc.expErrCode)
18531931
}
1932+
if tc.expErrMsg != "" && tc.expErrMsg != err.Error() {
1933+
t.Fatalf("Got error: %v, expected error: %v", err.Error(), tc.expErrMsg)
1934+
}
18541935

18551936
// Make sure the response has the source volume.
18561937
respVolume := resp.GetVolume()

‎pkg/gce-pd-csi-driver/utils.go

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,10 @@ import (
2323

2424
csi "github.com/container-storage-interface/spec/lib/go/csi"
2525
"google.golang.org/grpc"
26+
"k8s.io/apimachinery/pkg/util/sets"
2627
"k8s.io/klog/v2"
28+
29+
"sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/common"
2730
)
2831

2932
const (
@@ -139,6 +142,68 @@ func validateAccessMode(am *csi.VolumeCapability_AccessMode) error {
139142
return nil
140143
}
141144

145+
func validateStoragePools(req *csi.CreateVolumeRequest, params common.DiskParameters, project string) error {
146+
storagePoolsEnabled := params.StoragePools != nil
147+
if !storagePoolsEnabled || req == nil {
148+
return nil
149+
}
150+
151+
if params.EnableConfidentialCompute {
152+
return fmt.Errorf("storage pools do not support confidential storage")
153+
}
154+
155+
if !(params.DiskType == "hyperdisk-balanced" || params.DiskType == "hyperdisk-throughput") {
156+
return fmt.Errorf("invalid disk-type: %q. storage pools only support hyperdisk-balanced or hyperdisk-throughput", params.DiskType)
157+
}
158+
159+
if params.ReplicationType == replicationTypeRegionalPD {
160+
return fmt.Errorf("storage pools do not support regional PD")
161+
}
162+
163+
if useVolumeCloning(req) {
164+
return fmt.Errorf("storage pools do not support disk clones")
165+
}
166+
167+
// Check that requisite zones matches the storage pools zones.
168+
// This means allowedTopologies was set properly by GCW.
169+
if err := validateStoragePoolZones(req, params.StoragePools); err != nil {
170+
return fmt.Errorf("failed to validate storage pools zones: %v", err)
171+
}
172+
173+
// Check that Storage Pools are in same project as the GCEClient that creates the volume.
174+
if err := validateStoragePoolProjects(project, params.StoragePools); err != nil {
175+
return fmt.Errorf("failed to validate storage pools projects: %v", err)
176+
}
177+
178+
return nil
179+
}
180+
181+
func validateStoragePoolZones(req *csi.CreateVolumeRequest, storagePools []common.StoragePool) error {
182+
storagePoolZones, err := common.StoragePoolZones(storagePools)
183+
if err != nil {
184+
return err
185+
}
186+
reqZones, err := getZonesFromTopology(req.GetAccessibilityRequirements().GetRequisite())
187+
if err != nil {
188+
return err
189+
}
190+
if !common.UnorderedSlicesEqual(storagePoolZones, reqZones) {
191+
return fmt.Errorf("requisite topologies must match storage pools zones. requisite zones: %v, storage pools zones: %v", reqZones, storagePoolZones)
192+
}
193+
return nil
194+
}
195+
196+
func validateStoragePoolProjects(project string, storagePools []common.StoragePool) error {
197+
spProjects := sets.String{}
198+
for _, sp := range storagePools {
199+
if sp.Project != project {
200+
spProjects.Insert(sp.Project)
201+
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())
202+
}
203+
}
204+
return nil
205+
}
206+
142207
func getMultiWriterFromCapability(vc *csi.VolumeCapability) (bool, error) {
143208
if vc.GetAccessMode() == nil {
144209
return false, errors.New("access mode is nil")

0 commit comments

Comments
 (0)