Merge pull request #1960 from AndrewSirenko/mfw2

Return ErrInvalidArgument in cloud upon EC2 ModifyVolume

pkg/cloud/cloud.go

@@ -174,6 +174,9 @@ var (
 
 	// VolumeNotBeingModified is returned if volume being described is not being modified
 	VolumeNotBeingModified = fmt.Errorf("volume is not being modified")
+
+	// ErrInvalidArgument is returned if parameters were rejected by cloud provider
+	ErrInvalidArgument = errors.New("invalid argument")
 )
 
 // Set during build time via -ldflags
@@ -653,7 +656,11 @@ func (c *cloud) ResizeOrModifyDisk(ctx context.Context, volumeID string, newSize
 
 	response, err := c.ec2.ModifyVolumeWithContext(ctx, req)
 	if err != nil {
-		return 0, fmt.Errorf("unable to modify AWS volume %q: %w", volumeID, err)
+		if isAWSErrorInvalidParameter(err) {
+			// Wrap error to preserve original message from AWS as to why this was an invalid argument
+			return 0, fmt.Errorf("%w: %w", ErrInvalidArgument, err)
+		}
+		return 0, err
 	}
 
 	// If the volume modification isn't immediately completed, wait for it to finish
@@ -1407,6 +1414,35 @@ func isAWSErrorBlockDeviceInUse(err error) bool {
 	return false
 }
 
+// isAWSErrorInvalidParameter returns a boolean indicating whether the
+// given error is caused by invalid parameters in a EC2 API request.
+func isAWSErrorInvalidParameter(err error) bool {
+	var awsErr awserr.Error
+	if errors.As(err, &awsErr) {
+		switch awsErr.Code() {
+		case "InvalidParameter":
+			return true
+		case "InvalidParameterCombination":
+			return true
+		case "InvalidParameterDependency":
+			return true
+		case "InvalidParameterValue":
+			return true
+		case "UnknownParameter":
+			return true
+		case "UnknownVolumeType":
+			return true
+		case "UnsupportedOperation":
+			return true
+		case "ValidationError":
+			return true
+		default:
+			return false
+		}
+	}
+	return false
+}
+
 // Checks for desired size on volume by also verifying volume size by describing volume.
 // This is to get around potential eventual consistency problems with describing volume modifications
 // objects and ensuring that we read two different objects to verify volume state.

pkg/cloud/cloud_test.go

@@ -1926,8 +1926,52 @@ func TestResizeOrModifyDisk(t *testing.T) {
 				AvailabilityZone: aws.String(defaultZone),
 				VolumeType:       aws.String("gp2"),
 			},
+			modifiedVolumeError: awserr.New("GenericErr", "Generic error that does not involve invalid parameters", nil),
+			expErr:              awserr.New("GenericErr", "Generic error that does not involve invalid parameters", nil),
+		},
+		{
+			name:     "failure: returned ErrInvalidArgument when ModifyVolume returned InvalidParameterCombination",
+			volumeID: "vol-test",
+			modifyDiskOptions: &ModifyDiskOptions{
+				VolumeType: "GP2",
+				IOPS:       3000,
+			},
+			existingVolume: &ec2.Volume{
+				VolumeId:         aws.String("vol-test"),
+				AvailabilityZone: aws.String(defaultZone),
+				VolumeType:       aws.String("gp2"),
+			},
 			modifiedVolumeError: awserr.New("InvalidParameterCombination", "The parameter iops is not supported for gp2 volumes", nil),
-			expErr:              awserr.New("InvalidParameterCombination", "The parameter iops is not supported for gp2 volumes", nil),
+			expErr:              ErrInvalidArgument,
+		},
+		{
+			name:     "failure: returned returned ErrInvalidArgument when ModifyVolume returned UnknownVolumeType",
+			volumeID: "vol-test",
+			modifyDiskOptions: &ModifyDiskOptions{
+				VolumeType: "GPFake",
+			},
+			existingVolume: &ec2.Volume{
+				VolumeId:         aws.String("vol-test"),
+				AvailabilityZone: aws.String(defaultZone),
+				VolumeType:       aws.String("gp2"),
+			},
+			modifiedVolumeError: awserr.New("UnknownVolumeType", "Unsupported volume type 'GPFake' for volume creation.", nil),
+			expErr:              ErrInvalidArgument,
+		},
+		{
+			name:     "failure: returned ErrInvalidArgument when ModifyVolume returned InvalidParameterValue",
+			volumeID: "vol-test",
+			modifyDiskOptions: &ModifyDiskOptions{
+				VolumeType: "GP3",
+				IOPS:       9999999,
+			},
+			existingVolume: &ec2.Volume{
+				VolumeId:         aws.String("vol-test"),
+				AvailabilityZone: aws.String(defaultZone),
+				VolumeType:       aws.String("gp2"),
+			},
+			modifiedVolumeError: awserr.New("InvalidParameterValue", "Volume iops of 9999999 is too high; maximum is x", nil),
+			expErr:              ErrInvalidArgument,
 		},
 		{
 			name:     "success: does not call ModifyVolume when no modification required",
@@ -2016,6 +2060,12 @@ func TestResizeOrModifyDisk(t *testing.T) {
 			if err != nil {
 				if tc.expErr == nil {
 					t.Fatalf("ResizeOrModifyDisk() failed: expected no error, got: %v", err)
+				} else {
+					if errors.Is(tc.expErr, ErrInvalidArgument) {
+						if !errors.Is(err, ErrInvalidArgument) {
+							t.Fatalf("ResizeOrModifyDisk() failed: expected ErrInvalidArgument, got: %v", err)
+						}
+					}
 				}
 			} else {
 				if tc.expErr != nil {

pkg/driver/controller_modify_volume.go

@@ -18,6 +18,7 @@ package driver
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"strconv"
 	"sync"
@@ -183,7 +184,7 @@ func (d *controllerService) executeModifyVolumeRequest(volumeID string, req *mod
 	defer cancel()
 	actualSizeGiB, err := d.cloud.ResizeOrModifyDisk(ctx, volumeID, req.newSize, &req.modifyDiskOptions)
 	if err != nil {
-		return 0, status.Errorf(codes.Internal, "Could not modify volume %q: %v", volumeID, err)
+		return 0, err
 	} else {
 		return actualSizeGiB, nil
 	}
@@ -272,6 +273,9 @@ func (d *controllerService) modifyVolumeWithCoalescing(ctx context.Context, volu
 	select {
 	case response := <-responseChan:
 		if response.err != nil {
+			if errors.Is(response.err, cloud.ErrInvalidArgument) {
+				return status.Errorf(codes.InvalidArgument, "Could not modify volume %q: %v", volume, response.err)
+			}
 			return status.Errorf(codes.Internal, "Could not modify volume %q: %v", volume, response.err)
 		}
 	case <-ctx.Done():