From cfef212b554ae5e67261b481c4ff404cefea4e07 Mon Sep 17 00:00:00 2001 From: Drew Sirenko <68304519+AndrewSirenko@users.noreply.github.com> Date: Mon, 4 Mar 2024 16:55:23 -0500 Subject: [PATCH] Return ErrInvalidArgument in cloud upon EC2 ModifyVolume --- pkg/cloud/cloud.go | 25 ++++++++++++- pkg/cloud/cloud_test.go | 52 +++++++++++++++++++++++++- pkg/driver/controller_modify_volume.go | 6 ++- 3 files changed, 80 insertions(+), 3 deletions(-) diff --git a/pkg/cloud/cloud.go b/pkg/cloud/cloud.go index e912abd8a4..1ad12ba0e1 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 isAWSErrorModifyVolumeInvalidParameter(err) { + // Wrap the many possible EC2 errors with cloud error, but pass on actual EC2 error to operator + 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,22 @@ func isAWSErrorBlockDeviceInUse(err error) bool { return false } +// isAWSErrorModifyVolumeInvalidParameter returns a boolean indicating whether the +// given error is caused by invalid parameters in a ModifyVolume request +func isAWSErrorModifyVolumeInvalidParameter(err error) bool { + if isAWSError(err, "InvalidParameterValue") { + return true + } + if isAWSError(err, "InvalidParameterCombination") { + return true + } + if isAWSError(err, "UnknownVolumeType") { + return true + } + + 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..44889223e2 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 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():