Skip to content

Commit 254e0af

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #254 from davidz627/feature/enforceAccessModes
Statically validate volume capabilities for CreateVolume, ControllerPublish, NodeStage, NodePublish
2 parents 7295d5e + 34d1b19 commit 254e0af

File tree

6 files changed

+315
-47
lines changed

6 files changed

+315
-47
lines changed

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

+9-9
Original file line numberDiff line numberDiff line change
@@ -81,12 +81,9 @@ func (gceCS *GCEControllerServer) CreateVolume(ctx context.Context, req *csi.Cre
8181
return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("CreateVolume Request Capacity is invalid: %v", err))
8282
}
8383

84-
// TODO(#94): Validate AccessModes in VolumeCapabilities
85-
for _, capability := range volumeCapabilities {
86-
if blk := capability.GetBlock(); blk != nil {
87-
// TODO(#64): Block volume support
88-
return nil, status.Error(codes.Unimplemented, fmt.Sprintf("Block volume support is not yet implemented"))
89-
}
84+
err = validateVolumeCapabilities(volumeCapabilities)
85+
if err != nil {
86+
return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("VolumeCapabilities is invalid: %v", err))
9087
}
9188

9289
// Apply Parameters (case-insensitive). We leave validation of
@@ -251,7 +248,11 @@ func (gceCS *GCEControllerServer) ControllerPublishVolume(ctx context.Context, r
251248
if err != nil {
252249
return nil, status.Error(codes.NotFound, fmt.Sprintf("Could not find volume with ID %v: %v", volumeID, err))
253250
}
254-
// TODO(#94): Check volume capability matches
251+
252+
// TODO(#253): Check volume capability matches for ALREADY_EXISTS
253+
if err = validateVolumeCapability(volumeCapability); err != nil {
254+
return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("VolumeCapabilities is invalid: %v", err))
255+
}
255256

256257
pubVolResp := &csi.ControllerPublishVolumeResponse{
257258
PublishContext: nil,
@@ -364,7 +365,6 @@ func (gceCS *GCEControllerServer) ControllerUnpublishVolume(ctx context.Context,
364365
}
365366

366367
func (gceCS *GCEControllerServer) ValidateVolumeCapabilities(ctx context.Context, req *csi.ValidateVolumeCapabilitiesRequest) (*csi.ValidateVolumeCapabilitiesResponse, error) {
367-
// TODO(#94): Factor out the volume capability functionality and use as validation in all other functions as well
368368
// TODO(#162): Implement ValidateVolumeCapabilities
369369

370370
glog.V(5).Infof("Using default ValidateVolumeCapabilities")
@@ -744,7 +744,7 @@ func diskIsAttachedAndCompatible(deviceName string, instance *compute.Instance,
744744
if disk.Mode != readWrite {
745745
return true, fmt.Errorf("disk mode does not match. Got %v. Want %v", disk.Mode, readWrite)
746746
}
747-
// TODO(#94): Check volume_capability.
747+
// TODO(#253): Check volume capability matches for ALREADY_EXISTS
748748
return true, nil
749749
}
750750
}

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

+42-28
Original file line numberDiff line numberDiff line change
@@ -46,16 +46,6 @@ const (
4646

4747
var (
4848
// Define "normal" parameters
49-
stdVolCap = []*csi.VolumeCapability{
50-
{
51-
AccessType: &csi.VolumeCapability_Mount{
52-
Mount: &csi.VolumeCapability_MountVolume{},
53-
},
54-
AccessMode: &csi.VolumeCapability_AccessMode{
55-
Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER,
56-
},
57-
},
58-
}
5949
stdCapRange = &csi.CapacityRange{
6050
RequiredBytes: common.GbToBytes(20),
6151
}
@@ -304,7 +294,6 @@ func TestListSnapshotsArguments(t *testing.T) {
304294
}
305295

306296
func TestCreateVolumeArguments(t *testing.T) {
307-
308297
// Define test cases
309298
testCases := []struct {
310299
name string
@@ -317,7 +306,7 @@ func TestCreateVolumeArguments(t *testing.T) {
317306
req: &csi.CreateVolumeRequest{
318307
Name: "test-name",
319308
CapacityRange: stdCapRange,
320-
VolumeCapabilities: stdVolCap,
309+
VolumeCapabilities: stdVolCaps,
321310
Parameters: stdParams,
322311
},
323312
expVol: &csi.Volume{
@@ -327,12 +316,37 @@ func TestCreateVolumeArguments(t *testing.T) {
327316
AccessibleTopology: stdTopology,
328317
},
329318
},
319+
{
320+
name: "success with MULTI_NODE_READER_ONLY",
321+
req: &csi.CreateVolumeRequest{
322+
Name: "test-name",
323+
CapacityRange: stdCapRange,
324+
VolumeCapabilities: createVolumeCapabilities(csi.VolumeCapability_AccessMode_MULTI_NODE_READER_ONLY),
325+
Parameters: stdParams,
326+
},
327+
expVol: &csi.Volume{
328+
CapacityBytes: common.GbToBytes(20),
329+
VolumeId: testVolumeId,
330+
VolumeContext: nil,
331+
AccessibleTopology: stdTopology,
332+
},
333+
},
334+
{
335+
name: "fail with MULTI_NODE_MULTI_WRITER capability",
336+
req: &csi.CreateVolumeRequest{
337+
Name: "test-name",
338+
CapacityRange: stdCapRange,
339+
VolumeCapabilities: createVolumeCapabilities(csi.VolumeCapability_AccessMode_MULTI_NODE_MULTI_WRITER),
340+
Parameters: stdParams,
341+
},
342+
expErrCode: codes.InvalidArgument,
343+
},
330344
{
331345
name: "fail no name",
332346
req: &csi.CreateVolumeRequest{
333347
Name: "",
334348
CapacityRange: stdCapRange,
335-
VolumeCapabilities: stdVolCap,
349+
VolumeCapabilities: stdVolCaps,
336350
Parameters: stdParams,
337351
},
338352
expErrCode: codes.InvalidArgument,
@@ -341,7 +355,7 @@ func TestCreateVolumeArguments(t *testing.T) {
341355
name: "success no capacity range",
342356
req: &csi.CreateVolumeRequest{
343357
Name: "test-name",
344-
VolumeCapabilities: stdVolCap,
358+
VolumeCapabilities: stdVolCaps,
345359
Parameters: stdParams,
346360
},
347361
expVol: &csi.Volume{
@@ -365,7 +379,7 @@ func TestCreateVolumeArguments(t *testing.T) {
365379
req: &csi.CreateVolumeRequest{
366380
Name: "test-name",
367381
CapacityRange: stdCapRange,
368-
VolumeCapabilities: stdVolCap,
382+
VolumeCapabilities: stdVolCaps,
369383
},
370384
expVol: &csi.Volume{
371385
CapacityBytes: common.GbToBytes(20),
@@ -379,7 +393,7 @@ func TestCreateVolumeArguments(t *testing.T) {
379393
req: &csi.CreateVolumeRequest{
380394
Name: "test-name",
381395
CapacityRange: stdCapRange,
382-
VolumeCapabilities: stdVolCap,
396+
VolumeCapabilities: stdVolCaps,
383397
Parameters: stdParams,
384398
Secrets: map[string]string{"key1": "this is a random", "crypto": "secret"},
385399
},
@@ -395,7 +409,7 @@ func TestCreateVolumeArguments(t *testing.T) {
395409
req: &csi.CreateVolumeRequest{
396410
Name: "test-name",
397411
CapacityRange: stdCapRange,
398-
VolumeCapabilities: stdVolCap,
412+
VolumeCapabilities: stdVolCaps,
399413
Parameters: map[string]string{"type": "test-type"},
400414
AccessibilityRequirements: &csi.TopologyRequirement{
401415
Requisite: []*csi.Topology{
@@ -421,7 +435,7 @@ func TestCreateVolumeArguments(t *testing.T) {
421435
req: &csi.CreateVolumeRequest{
422436
Name: "test-name",
423437
CapacityRange: stdCapRange,
424-
VolumeCapabilities: stdVolCap,
438+
VolumeCapabilities: stdVolCaps,
425439
Parameters: map[string]string{"type": "test-type"},
426440
AccessibilityRequirements: &csi.TopologyRequirement{
427441
Requisite: []*csi.Topology{
@@ -464,7 +478,7 @@ func TestCreateVolumeArguments(t *testing.T) {
464478
req: &csi.CreateVolumeRequest{
465479
Name: "test-name",
466480
CapacityRange: stdCapRange,
467-
VolumeCapabilities: stdVolCap,
481+
VolumeCapabilities: stdVolCaps,
468482
Parameters: stdParams,
469483
AccessibilityRequirements: &csi.TopologyRequirement{
470484
Requisite: []*csi.Topology{
@@ -481,7 +495,7 @@ func TestCreateVolumeArguments(t *testing.T) {
481495
req: &csi.CreateVolumeRequest{
482496
Name: "test-name",
483497
CapacityRange: stdCapRange,
484-
VolumeCapabilities: stdVolCap,
498+
VolumeCapabilities: stdVolCaps,
485499
Parameters: stdParams,
486500
AccessibilityRequirements: &csi.TopologyRequirement{
487501
Requisite: []*csi.Topology{
@@ -499,7 +513,7 @@ func TestCreateVolumeArguments(t *testing.T) {
499513
req: &csi.CreateVolumeRequest{
500514
Name: name,
501515
CapacityRange: stdCapRange,
502-
VolumeCapabilities: stdVolCap,
516+
VolumeCapabilities: stdVolCaps,
503517
Parameters: map[string]string{common.ParameterKeyReplicationType: replicationTypeRegionalPD},
504518
AccessibilityRequirements: &csi.TopologyRequirement{
505519
Preferred: []*csi.Topology{
@@ -531,7 +545,7 @@ func TestCreateVolumeArguments(t *testing.T) {
531545
req: &csi.CreateVolumeRequest{
532546
Name: name,
533547
CapacityRange: stdCapRange,
534-
VolumeCapabilities: stdVolCap,
548+
VolumeCapabilities: stdVolCaps,
535549
Parameters: map[string]string{
536550
common.ParameterKeyReplicationType: replicationTypeRegionalPD,
537551
},
@@ -555,7 +569,7 @@ func TestCreateVolumeArguments(t *testing.T) {
555569
req: &csi.CreateVolumeRequest{
556570
Name: name,
557571
CapacityRange: stdCapRange,
558-
VolumeCapabilities: stdVolCap,
572+
VolumeCapabilities: stdVolCaps,
559573
Parameters: map[string]string{
560574
common.ParameterKeyReplicationType: replicationTypeRegionalPD,
561575
},
@@ -579,7 +593,7 @@ func TestCreateVolumeArguments(t *testing.T) {
579593
req: &csi.CreateVolumeRequest{
580594
Name: "test-name",
581595
CapacityRange: stdCapRange,
582-
VolumeCapabilities: stdVolCap,
596+
VolumeCapabilities: stdVolCaps,
583597
VolumeContentSource: &csi.VolumeContentSource{
584598
Type: &csi.VolumeContentSource_Snapshot{
585599
Snapshot: &csi.VolumeContentSource_SnapshotSource{
@@ -618,7 +632,7 @@ func TestCreateVolumeArguments(t *testing.T) {
618632
},
619633
},
620634
},
621-
expErrCode: codes.Unimplemented,
635+
expErrCode: codes.InvalidArgument,
622636
},
623637
{
624638
name: "fail with both mount and block volume capability",
@@ -644,14 +658,14 @@ func TestCreateVolumeArguments(t *testing.T) {
644658
},
645659
},
646660
},
647-
expErrCode: codes.Unimplemented, // once block support is implemented, this error should be InvalidArgument
661+
expErrCode: codes.InvalidArgument,
648662
},
649663
{
650664
name: "success with disk encryption kms key",
651665
req: &csi.CreateVolumeRequest{
652666
Name: name,
653667
CapacityRange: stdCapRange,
654-
VolumeCapabilities: stdVolCap,
668+
VolumeCapabilities: stdVolCaps,
655669
Parameters: map[string]string{
656670
common.ParameterKeyDiskEncryptionKmsKey: "projects/KMS_PROJECT_ID/locations/REGION/keyRings/KEY_RING/cryptoKeys/KEY",
657671
},
@@ -713,7 +727,7 @@ func TestCreateVolumeRandomRequisiteTopology(t *testing.T) {
713727
req := &csi.CreateVolumeRequest{
714728
Name: "test-name",
715729
CapacityRange: stdCapRange,
716-
VolumeCapabilities: stdVolCap,
730+
VolumeCapabilities: stdVolCaps,
717731
Parameters: map[string]string{"type": "test-type"},
718732
AccessibilityRequirements: &csi.TopologyRequirement{
719733
Requisite: []*csi.Topology{

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

+11-1
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,10 @@ func (ns *GCENodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePub
7575
return nil, status.Error(codes.InvalidArgument, "NodePublishVolume Volume Capability must be provided")
7676
}
7777

78+
if err := validateVolumeCapability(volumeCapability); err != nil {
79+
return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("VolumeCapability is invalid: %v", err))
80+
}
81+
7882
notMnt, err := ns.Mounter.Interface.IsLikelyNotMountPoint(targetPath)
7983
if err != nil && !os.IsNotExist(err) {
8084
glog.Errorf("cannot validate mount point: %s %v", targetPath, err)
@@ -84,7 +88,7 @@ func (ns *GCENodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePub
8488
// TODO(#95): check if mount is compatible. Return OK if it is, or appropriate error.
8589
/*
8690
1) Target Path MUST be the vol referenced by vol ID
87-
2) VolumeCapability MUST match
91+
2) TODO(#253): Check volume capability matches for ALREADY_EXISTS
8892
3) Readonly MUST match
8993
9094
*/
@@ -175,6 +179,12 @@ func (ns *GCENodeServer) NodeStageVolume(ctx context.Context, req *csi.NodeStage
175179
return nil, status.Error(codes.InvalidArgument, "NodeStageVolume Volume Capability must be provided")
176180
}
177181

182+
if err := validateVolumeCapability(volumeCapability); err != nil {
183+
return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("VolumeCapability is invalid: %v", err))
184+
}
185+
186+
// TODO(#253): Check volume capability matches for ALREADY_EXISTS
187+
178188
volumeKey, err := common.VolumeIDToKey(volumeID)
179189
if err != nil {
180190
return nil, err

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

+28-9
Original file line numberDiff line numberDiff line change
@@ -108,16 +108,27 @@ func TestNodePublishVolume(t *testing.T) {
108108
TargetPath: defaultTargetPath,
109109
StagingTargetPath: defaultStagingPath,
110110
Readonly: false,
111-
VolumeCapability: &csi.VolumeCapability{},
111+
VolumeCapability: stdVolCap,
112112
},
113113
},
114+
{
115+
name: "Invalid request (invalid access mode)",
116+
req: &csi.NodePublishVolumeRequest{
117+
VolumeId: defaultVolumeID,
118+
TargetPath: defaultTargetPath,
119+
StagingTargetPath: defaultStagingPath,
120+
Readonly: false,
121+
VolumeCapability: createVolumeCapability(csi.VolumeCapability_AccessMode_MULTI_NODE_MULTI_WRITER),
122+
},
123+
expErrCode: codes.InvalidArgument,
124+
},
114125
{
115126
name: "Invalid request (No VolumeId)",
116127
req: &csi.NodePublishVolumeRequest{
117128
TargetPath: defaultTargetPath,
118129
StagingTargetPath: defaultStagingPath,
119130
Readonly: false,
120-
VolumeCapability: &csi.VolumeCapability{},
131+
VolumeCapability: stdVolCap,
121132
},
122133
expErrCode: codes.InvalidArgument,
123134
},
@@ -127,7 +138,7 @@ func TestNodePublishVolume(t *testing.T) {
127138
VolumeId: defaultVolumeID,
128139
StagingTargetPath: defaultStagingPath,
129140
Readonly: false,
130-
VolumeCapability: &csi.VolumeCapability{},
141+
VolumeCapability: stdVolCap,
131142
},
132143
expErrCode: codes.InvalidArgument,
133144
},
@@ -137,7 +148,7 @@ func TestNodePublishVolume(t *testing.T) {
137148
VolumeId: defaultVolumeID,
138149
TargetPath: defaultTargetPath,
139150
Readonly: false,
140-
VolumeCapability: &csi.VolumeCapability{},
151+
VolumeCapability: stdVolCap,
141152
},
142153
expErrCode: codes.InvalidArgument,
143154
},
@@ -242,22 +253,31 @@ func TestNodeStageVolume(t *testing.T) {
242253
req: &csi.NodeStageVolumeRequest{
243254
VolumeId: volumeID,
244255
StagingTargetPath: defaultStagingPath,
245-
VolumeCapability: &csi.VolumeCapability{},
256+
VolumeCapability: stdVolCap,
246257
},
247258
},
259+
{
260+
name: "Invalid request (Bad Access Mode)",
261+
req: &csi.NodeStageVolumeRequest{
262+
VolumeId: volumeID,
263+
StagingTargetPath: defaultStagingPath,
264+
VolumeCapability: createVolumeCapability(csi.VolumeCapability_AccessMode_MULTI_NODE_MULTI_WRITER),
265+
},
266+
expErrCode: codes.InvalidArgument,
267+
},
248268
{
249269
name: "Invalid request (No VolumeId)",
250270
req: &csi.NodeStageVolumeRequest{
251271
StagingTargetPath: defaultStagingPath,
252-
VolumeCapability: &csi.VolumeCapability{},
272+
VolumeCapability: stdVolCap,
253273
},
254274
expErrCode: codes.InvalidArgument,
255275
},
256276
{
257277
name: "Invalid request (No StagingTargetPath)",
258278
req: &csi.NodeStageVolumeRequest{
259279
VolumeId: volumeID,
260-
VolumeCapability: &csi.VolumeCapability{},
280+
VolumeCapability: stdVolCap,
261281
},
262282
expErrCode: codes.InvalidArgument,
263283
},
@@ -277,9 +297,8 @@ func TestNodeStageVolume(t *testing.T) {
277297
StagingTargetPath: defaultStagingPath,
278298
VolumeCapability: cap,
279299
},
280-
expErrCode: codes.Unimplemented,
300+
expErrCode: codes.InvalidArgument,
281301
},
282-
// Capability Mount. codes.Unimplemented
283302
}
284303
for _, tc := range testCases {
285304
t.Logf("Test case: %s", tc.name)

0 commit comments

Comments
 (0)