diff --git a/pkg/azuredisk/controllerserver.go b/pkg/azuredisk/controllerserver.go index 1de3528351..06591338fd 100644 --- a/pkg/azuredisk/controllerserver.go +++ b/pkg/azuredisk/controllerserver.go @@ -80,8 +80,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 @@ -356,8 +356,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) @@ -518,8 +518,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 17a9022080..2ceb1955d8 100644 --- a/pkg/azuredisk/controllerserver_test.go +++ b/pkg/azuredisk/controllerserver_test.go @@ -219,7 +219,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) } @@ -637,7 +637,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 0fe4dc1dc7..cf76a7143f 100644 --- a/pkg/azuredisk/controllerserver_v2.go +++ b/pkg/azuredisk/controllerserver_v2.go @@ -71,8 +71,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 @@ -303,8 +303,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) @@ -448,8 +448,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 49949024ce..44364aa4a3 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 4ab5c7937d..7b753d0141 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 bc9a375e6d..ce41bb9bd9 100644 --- a/pkg/azuredisk/nodeserver_v2.go +++ b/pkg/azuredisk/nodeserver_v2.go @@ -68,8 +68,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 { @@ -214,8 +214,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 a641081e6c..eb7a2c1182 100644 --- a/pkg/azureutils/azure_disk_utils.go +++ b/pkg/azureutils/azure_disk_utils.go @@ -447,31 +447,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 2827a4d2ca..6999cf7e5f 100644 --- a/pkg/azureutils/azure_disk_utils_test.go +++ b/pkg/azureutils/azure_disk_utils_test.go @@ -1007,7 +1007,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", @@ -1022,7 +1022,7 @@ func TestIsValidVolumeCapabilities(t *testing.T) { }, }, maxShares: 1, - expectedResult: true, + expectedResult: nil, }, { description: "[Failure] Returns false for unsupported mount access mode", @@ -1037,7 +1037,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", @@ -1052,7 +1052,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", @@ -1067,7 +1067,7 @@ func TestIsValidVolumeCapabilities(t *testing.T) { }, }, maxShares: 1, - expectedResult: true, + expectedResult: nil, }, { description: "[Success] Returns true for shared block access mode", @@ -1082,7 +1082,7 @@ func TestIsValidVolumeCapabilities(t *testing.T) { }, }, maxShares: 2, - expectedResult: true, + expectedResult: nil, }, { description: "[Failure] Returns false for unsupported mount access mode", @@ -1097,7 +1097,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", @@ -1112,7 +1112,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", @@ -1123,7 +1123,7 @@ func TestIsValidVolumeCapabilities(t *testing.T) { }, }, maxShares: 1, - expectedResult: false, + expectedResult: fmt.Errorf("invalid access mode: []"), }, } @@ -1144,8 +1144,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{ @@ -1153,8 +1153,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") } }