From 0b0f89c4f6bd90c711e830d8416a6c0e2dbd4859 Mon Sep 17 00:00:00 2001 From: andyzhangx Date: Fri, 15 Sep 2023 13:54:17 +0000 Subject: [PATCH] cleanup: refine Volume capabilities not supported error --- pkg/azuredisk/controllerserver.go | 12 ++++++------ pkg/azuredisk/controllerserver_test.go | 4 ++-- pkg/azuredisk/controllerserver_v2.go | 12 ++++++------ pkg/azuredisk/nodeserver.go | 8 ++++---- pkg/azuredisk/nodeserver_test.go | 4 ++-- pkg/azuredisk/nodeserver_v2.go | 8 ++++---- pkg/azureutils/azure_disk_utils.go | 20 +++++++++++-------- pkg/azureutils/azure_disk_utils_test.go | 26 ++++++++++++------------- 8 files changed, 49 insertions(+), 45 deletions(-) diff --git a/pkg/azuredisk/controllerserver.go b/pkg/azuredisk/controllerserver.go index 9994cae02b..3a41072eab 100644 --- a/pkg/azuredisk/controllerserver.go +++ b/pkg/azuredisk/controllerserver.go @@ -79,8 +79,8 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) return nil, status.Error(codes.InvalidArgument, "CreateVolume Volume capabilities must be provided") } - if !azureutils.IsValidVolumeCapabilities(volCaps, diskParams.MaxShares) { - return nil, status.Error(codes.InvalidArgument, "Volume capability not supported") + if err := azureutils.IsValidVolumeCapabilities(volCaps, diskParams.MaxShares); err != nil { + return nil, status.Error(codes.InvalidArgument, err.Error()) } isAdvancedPerfProfile := strings.EqualFold(diskParams.PerfProfile, consts.PerfProfileAdvanced) // If perfProfile is set to advanced and no/invalid device settings are provided, fail the request @@ -362,8 +362,8 @@ func (d *Driver) ControllerPublishVolume(ctx context.Context, req *csi.Controlle return nil, status.Error(codes.InvalidArgument, "MaxShares value not supported") } - if !azureutils.IsValidVolumeCapabilities(caps, maxShares) { - return nil, status.Error(codes.InvalidArgument, "Volume capability not supported") + if err := azureutils.IsValidVolumeCapabilities(caps, maxShares); err != nil { + return nil, status.Error(codes.InvalidArgument, err.Error()) } disk, err := d.checkDiskExists(ctx, diskURI) @@ -524,8 +524,8 @@ func (d *Driver) ValidateVolumeCapabilities(ctx context.Context, req *csi.Valida return nil, status.Error(codes.InvalidArgument, "MaxShares value not supported") } - if !azureutils.IsValidVolumeCapabilities(volumeCapabilities, maxShares) { - return &csi.ValidateVolumeCapabilitiesResponse{Message: "VolumeCapabilities are invalid"}, nil + if err := azureutils.IsValidVolumeCapabilities(volumeCapabilities, maxShares); err != nil { + return &csi.ValidateVolumeCapabilitiesResponse{Message: err.Error()}, nil } if _, err := d.checkDiskExists(ctx, diskURI); err != nil { diff --git a/pkg/azuredisk/controllerserver_test.go b/pkg/azuredisk/controllerserver_test.go index f6868ffcc0..690cc5e6aa 100644 --- a/pkg/azuredisk/controllerserver_test.go +++ b/pkg/azuredisk/controllerserver_test.go @@ -217,7 +217,7 @@ func TestCreateVolume(t *testing.T) { Parameters: mp, } _, err := d.CreateVolume(context.Background(), req) - expectedErr := status.Error(codes.InvalidArgument, "Volume capability not supported") + expectedErr := status.Error(codes.InvalidArgument, "mountVolume is not supported for access mode: MULTI_NODE_MULTI_WRITER") if !reflect.DeepEqual(err, expectedErr) { t.Errorf("actualErr: (%v), expectedErr: (%v)", err, expectedErr) } @@ -635,7 +635,7 @@ func TestControllerPublishVolume(t *testing.T) { VolumeId: "vol_1", VolumeCapability: volumeCapWrong, } - expectedErr := status.Error(codes.InvalidArgument, "Volume capability not supported") + expectedErr := status.Error(codes.InvalidArgument, "invalid access mode: [mount:<> access_mode: ]") _, err := d.ControllerPublishVolume(context.Background(), req) if !reflect.DeepEqual(err, expectedErr) { t.Errorf("actualErr: (%v), expectedErr: (%v)", err, expectedErr) diff --git a/pkg/azuredisk/controllerserver_v2.go b/pkg/azuredisk/controllerserver_v2.go index 3f87ec5940..43add6a7ea 100644 --- a/pkg/azuredisk/controllerserver_v2.go +++ b/pkg/azuredisk/controllerserver_v2.go @@ -70,8 +70,8 @@ func (d *DriverV2) CreateVolume(ctx context.Context, req *csi.CreateVolumeReques return nil, status.Error(codes.InvalidArgument, "CreateVolume Volume capabilities must be provided") } - if !azureutils.IsValidVolumeCapabilities(volCaps, diskParams.MaxShares) { - return nil, status.Error(codes.InvalidArgument, "Volume capability not supported") + if err := azureutils.IsValidVolumeCapabilities(volCaps, diskParams.MaxShares); err != nil { + return nil, status.Error(codes.InvalidArgument, err.Error()) } isAdvancedPerfProfile := strings.EqualFold(diskParams.PerfProfile, consts.PerfProfileAdvanced) // If perfProfile is set to advanced and no/invalid device settings are provided, fail the request @@ -309,8 +309,8 @@ func (d *DriverV2) ControllerPublishVolume(ctx context.Context, req *csi.Control return nil, status.Error(codes.InvalidArgument, "MaxShares value not supported") } - if !azureutils.IsValidVolumeCapabilities(caps, maxShares) { - return nil, status.Error(codes.InvalidArgument, "Volume capability not supported") + if err := azureutils.IsValidVolumeCapabilities(caps, maxShares); err != nil { + return nil, status.Error(codes.InvalidArgument, err.Error()) } disk, err := d.checkDiskExists(ctx, diskURI) @@ -454,8 +454,8 @@ func (d *DriverV2) ValidateVolumeCapabilities(ctx context.Context, req *csi.Vali return nil, status.Error(codes.InvalidArgument, "MaxShares value not supported") } - if !azureutils.IsValidVolumeCapabilities(volumeCapabilities, maxShares) { - return &csi.ValidateVolumeCapabilitiesResponse{Message: "VolumeCapabilities are invalid"}, nil + if err := azureutils.IsValidVolumeCapabilities(volumeCapabilities, maxShares); err != nil { + return &csi.ValidateVolumeCapabilitiesResponse{Message: err.Error()}, nil } if _, err := d.checkDiskExists(ctx, diskURI); err != nil { diff --git a/pkg/azuredisk/nodeserver.go b/pkg/azuredisk/nodeserver.go index 4b2dac61d4..b0e1d56d03 100644 --- a/pkg/azuredisk/nodeserver.go +++ b/pkg/azuredisk/nodeserver.go @@ -80,8 +80,8 @@ func (d *Driver) NodeStageVolume(ctx context.Context, req *csi.NodeStageVolumeRe return nil, status.Error(codes.InvalidArgument, "MaxShares value not supported") } - if !azureutils.IsValidVolumeCapabilities([]*csi.VolumeCapability{volumeCapability}, maxShares) { - return nil, status.Error(codes.InvalidArgument, "Volume capability not supported") + if err := azureutils.IsValidVolumeCapabilities([]*csi.VolumeCapability{volumeCapability}, maxShares); err != nil { + return nil, status.Error(codes.InvalidArgument, err.Error()) } if acquired := d.volumeLocks.TryAcquire(diskURI); !acquired { @@ -226,8 +226,8 @@ func (d *Driver) NodePublishVolume(ctx context.Context, req *csi.NodePublishVolu return nil, status.Error(codes.InvalidArgument, "MaxShares value not supported") } - if !azureutils.IsValidVolumeCapabilities([]*csi.VolumeCapability{volumeCapability}, maxShares) { - return nil, status.Error(codes.InvalidArgument, "Volume capability not supported") + if err := azureutils.IsValidVolumeCapabilities([]*csi.VolumeCapability{volumeCapability}, maxShares); err != nil { + return nil, status.Error(codes.InvalidArgument, err.Error()) } source := req.GetStagingTargetPath() diff --git a/pkg/azuredisk/nodeserver_test.go b/pkg/azuredisk/nodeserver_test.go index 987a4c4e34..53b173488f 100644 --- a/pkg/azuredisk/nodeserver_test.go +++ b/pkg/azuredisk/nodeserver_test.go @@ -464,7 +464,7 @@ func TestNodeStageVolume(t *testing.T) { { desc: "Volume capabilities not supported", req: csi.NodeStageVolumeRequest{VolumeId: "vol_1", StagingTargetPath: sourceTest, VolumeCapability: &csi.VolumeCapability{AccessMode: &volumeCapWrong}}, - expectedErr: status.Error(codes.InvalidArgument, "Volume capability not supported"), + expectedErr: status.Error(codes.InvalidArgument, "invalid access mode: [access_mode: ]"), }, { desc: "MaxShares value not supported", @@ -819,7 +819,7 @@ func TestNodePublishVolume(t *testing.T) { req: csi.NodePublishVolumeRequest{VolumeCapability: &csi.VolumeCapability{AccessMode: &volumeCapWrong, AccessType: stdVolCapBlock}, VolumeId: "vol_1"}, expectedErr: testutil.TestError{ - DefaultError: status.Error(codes.InvalidArgument, "Volume capability not supported"), + DefaultError: status.Error(codes.InvalidArgument, "invalid access mode: [block:<> access_mode: ]"), }, }, { diff --git a/pkg/azuredisk/nodeserver_v2.go b/pkg/azuredisk/nodeserver_v2.go index e2e60d05b4..d35a1ffd2b 100644 --- a/pkg/azuredisk/nodeserver_v2.go +++ b/pkg/azuredisk/nodeserver_v2.go @@ -67,8 +67,8 @@ func (d *DriverV2) NodeStageVolume(ctx context.Context, req *csi.NodeStageVolume return nil, status.Error(codes.InvalidArgument, "MaxShares value not supported") } - if !azureutils.IsValidVolumeCapabilities([]*csi.VolumeCapability{volumeCapability}, maxShares) { - return nil, status.Error(codes.InvalidArgument, "Volume capability not supported") + if err := azureutils.IsValidVolumeCapabilities([]*csi.VolumeCapability{volumeCapability}, maxShares); err != nil { + return nil, status.Error(codes.InvalidArgument, err.Error()) } if acquired := d.volumeLocks.TryAcquire(diskURI); !acquired { @@ -213,8 +213,8 @@ func (d *DriverV2) NodePublishVolume(ctx context.Context, req *csi.NodePublishVo return nil, status.Error(codes.InvalidArgument, "MaxShares value not supported") } - if !azureutils.IsValidVolumeCapabilities([]*csi.VolumeCapability{volumeCapability}, maxShares) { - return nil, status.Error(codes.InvalidArgument, "Volume capability not supported") + if err := azureutils.IsValidVolumeCapabilities([]*csi.VolumeCapability{volumeCapability}, maxShares); err != nil { + return nil, status.Error(codes.InvalidArgument, err.Error()) } source := req.GetStagingTargetPath() diff --git a/pkg/azureutils/azure_disk_utils.go b/pkg/azureutils/azure_disk_utils.go index f326c69f7e..d16834353c 100644 --- a/pkg/azureutils/azure_disk_utils.go +++ b/pkg/azureutils/azure_disk_utils.go @@ -454,31 +454,35 @@ func IsValidDiskURI(diskURI string) error { return nil } -func IsValidVolumeCapabilities(volCaps []*csi.VolumeCapability, maxShares int) bool { +// IsValidVolumeCapabilities checks whether the volume capabilities are valid +func IsValidVolumeCapabilities(volCaps []*csi.VolumeCapability, maxShares int) error { if ok := IsValidAccessModes(volCaps); !ok { - return false + return fmt.Errorf("invalid access mode: %v", volCaps) } for _, c := range volCaps { blockVolume := c.GetBlock() mountVolume := c.GetMount() accessMode := c.GetAccessMode().GetMode() - if (blockVolume == nil && mountVolume == nil) || - (blockVolume != nil && mountVolume != nil) { - return false + if blockVolume == nil && mountVolume == nil { + return fmt.Errorf("blockVolume and mountVolume are both nil") + } + + if blockVolume != nil && mountVolume != nil { + return fmt.Errorf("blockVolume and mountVolume are both not nil") } if mountVolume != nil && (accessMode == csi.VolumeCapability_AccessMode_MULTI_NODE_MULTI_WRITER || accessMode == csi.VolumeCapability_AccessMode_MULTI_NODE_READER_ONLY || accessMode == csi.VolumeCapability_AccessMode_MULTI_NODE_SINGLE_WRITER) { - return false + return fmt.Errorf("mountVolume is not supported for access mode: %s", accessMode.String()) } if maxShares < 2 && (accessMode == csi.VolumeCapability_AccessMode_MULTI_NODE_MULTI_WRITER || accessMode == csi.VolumeCapability_AccessMode_MULTI_NODE_READER_ONLY || accessMode == csi.VolumeCapability_AccessMode_MULTI_NODE_SINGLE_WRITER) { - return false + return fmt.Errorf("access mode: %s is not supported for non-shared disk", accessMode.String()) } } - return true + return nil } func IsValidAccessModes(volCaps []*csi.VolumeCapability) bool { diff --git a/pkg/azureutils/azure_disk_utils_test.go b/pkg/azureutils/azure_disk_utils_test.go index 9e51136ede..0b3a1dbb78 100644 --- a/pkg/azureutils/azure_disk_utils_test.go +++ b/pkg/azureutils/azure_disk_utils_test.go @@ -1006,7 +1006,7 @@ func TestIsValidVolumeCapabilities(t *testing.T) { description string volCaps []*csi.VolumeCapability maxShares int - expectedResult bool + expectedResult error }{ { description: "[Success] Returns true for valid mount capabilities", @@ -1021,7 +1021,7 @@ func TestIsValidVolumeCapabilities(t *testing.T) { }, }, maxShares: 1, - expectedResult: true, + expectedResult: nil, }, { description: "[Failure] Returns false for unsupported mount access mode", @@ -1036,7 +1036,7 @@ func TestIsValidVolumeCapabilities(t *testing.T) { }, }, maxShares: 2, - expectedResult: false, + expectedResult: fmt.Errorf("mountVolume is not supported for access mode: MULTI_NODE_MULTI_WRITER"), }, { description: "[Failure] Returns false for invalid mount access mode", @@ -1051,7 +1051,7 @@ func TestIsValidVolumeCapabilities(t *testing.T) { }, }, maxShares: 1, - expectedResult: false, + expectedResult: fmt.Errorf("invalid access mode: [mount:<> access_mode: ]"), }, { description: "[Success] Returns true for valid block capabilities", @@ -1066,7 +1066,7 @@ func TestIsValidVolumeCapabilities(t *testing.T) { }, }, maxShares: 1, - expectedResult: true, + expectedResult: nil, }, { description: "[Success] Returns true for shared block access mode", @@ -1081,7 +1081,7 @@ func TestIsValidVolumeCapabilities(t *testing.T) { }, }, maxShares: 2, - expectedResult: true, + expectedResult: nil, }, { description: "[Failure] Returns false for unsupported mount access mode", @@ -1096,7 +1096,7 @@ func TestIsValidVolumeCapabilities(t *testing.T) { }, }, maxShares: 1, - expectedResult: false, + expectedResult: fmt.Errorf("access mode: MULTI_NODE_MULTI_WRITER is not supported for non-shared disk"), }, { description: "[Failure] Returns false for invalid block access mode", @@ -1111,7 +1111,7 @@ func TestIsValidVolumeCapabilities(t *testing.T) { }, }, maxShares: 1, - expectedResult: false, + expectedResult: fmt.Errorf("invalid access mode: [block:<> access_mode: ]"), }, { description: "[Failure] Returns false for empty volume capability", @@ -1122,7 +1122,7 @@ func TestIsValidVolumeCapabilities(t *testing.T) { }, }, maxShares: 1, - expectedResult: false, + expectedResult: fmt.Errorf("invalid access mode: []"), }, } @@ -1143,8 +1143,8 @@ func TestIsValidVolumeCapabilities(t *testing.T) { }, } caps = append(caps, &stdVolCap) - if !IsValidVolumeCapabilities(caps, 1) { - t.Errorf("Unexpected error") + if err := IsValidVolumeCapabilities(caps, 1); err != nil { + t.Errorf("Unexpected error: %v", err) } stdVolCap1 := csi.VolumeCapability{ AccessMode: &csi.VolumeCapability_AccessMode{ @@ -1152,8 +1152,8 @@ func TestIsValidVolumeCapabilities(t *testing.T) { }, } caps = append(caps, &stdVolCap1) - if IsValidVolumeCapabilities(caps, 1) { - t.Errorf("Unexpected error") + if err := IsValidVolumeCapabilities(caps, 1); err == nil { + t.Errorf("Unexpected success") } }