diff --git a/pkg/cloud/cloud.go b/pkg/cloud/cloud.go index e912abd8a4..f2be1197c7 100644 --- a/pkg/cloud/cloud.go +++ b/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. diff --git a/pkg/cloud/cloud_test.go b/pkg/cloud/cloud_test.go index 5cac44c3e3..2d8f6b7468 100644 --- a/pkg/cloud/cloud_test.go +++ b/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 { diff --git a/pkg/driver/controller_modify_volume.go b/pkg/driver/controller_modify_volume.go index db6be7ee7c..665d02040f 100644 --- a/pkg/driver/controller_modify_volume.go +++ b/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():