Merge pull request #1965 from andyzhangx/vol-cap-error-refine

cleanup: refine Volume capabilities not supported error

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 {

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:<mode:10 > ]")
 				_, err := d.ControllerPublishVolume(context.Background(), req)
 				if !reflect.DeepEqual(err, expectedErr) {
 					t.Errorf("actualErr: (%v), expectedErr: (%v)", err, expectedErr)

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 {

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()

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:<mode:10 > ]"),
 		},
 		{
 			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:<mode:10 > ]"),
 			},
 		},
 		{

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()

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 {

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:<mode:10 > ]"),
 		},
 		{
 			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:<mode:10 > ]"),
 		},
 		{
 			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,17 +1143,17 @@ 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{
 			Mode: 10,
 		},
 	}
 	caps = append(caps, &stdVolCap1)
-	if IsValidVolumeCapabilities(caps, 1) {
-		t.Errorf("Unexpected error")
+	if err := IsValidVolumeCapabilities(caps, 1); err == nil {
+		t.Errorf("Unexpected success")
 	}
 }