From 405ae2bffdb2c7ec5b42f0462d1b607a9cb74747 Mon Sep 17 00:00:00 2001 From: Predrag Janosevic Date: Tue, 22 Oct 2024 12:03:38 +0000 Subject: [PATCH] storage: handle errors in batch objects delete action (#627) Batch objects delete API method from `aws-sdk-go-v2` will not return error as second argument if individual objects failed to be deleted. Instead it will return both list of deleted objects and list of errors in result structure. This prints errors (if any) after the list of successfully deleted files. --- CHANGELOG.md | 6 +++++ cmd/storage_delete.go | 12 ++++++++- pkg/storage/sos/object.go | 16 +++++++++++- pkg/storage/sos/object_test.go | 47 ++++++++++++++++++++++++++++++++++ 4 files changed, 79 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bab0358b6..63090a197 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +### Bug fixes + +- storage: handle errors in batch objects delete action #627 + ## 1.80.0 ### Features diff --git a/cmd/storage_delete.go b/cmd/storage_delete.go index 467993b17..75a937dac 100644 --- a/cmd/storage_delete.go +++ b/cmd/storage_delete.go @@ -2,9 +2,11 @@ package cmd import ( "fmt" + "os" "strings" "github.com/aws/aws-sdk-go-v2/aws" + "github.com/hashicorp/go-multierror" "github.com/spf13/cobra" "github.com/exoscale/cli/pkg/storage/sos" @@ -86,7 +88,15 @@ argument with "/": deleted, err := storage.DeleteObjects(gContext, bucket, prefix, recursive) if err != nil { - return fmt.Errorf("unable to delete objects: %w", err) + if merr, ok := err.(*multierror.Error); ok { + // Error in individual files, print to stderr & continue + for _, e := range merr.Errors { + fmt.Fprintln(os.Stderr, e) + } + } else { + // Global error, exit + return fmt.Errorf("unable to delete objects: %w", err) + } } if verbose { diff --git a/pkg/storage/sos/object.go b/pkg/storage/sos/object.go index 8e6f88cac..0e1d799d3 100644 --- a/pkg/storage/sos/object.go +++ b/pkg/storage/sos/object.go @@ -21,6 +21,7 @@ import ( "github.com/aws/aws-sdk-go-v2/service/s3" "github.com/aws/aws-sdk-go-v2/service/s3/types" "github.com/dustin/go-humanize" + "github.com/hashicorp/go-multierror" "github.com/vbauerster/mpb/v4" "github.com/vbauerster/mpb/v4/decor" @@ -45,6 +46,7 @@ func (c *Client) DeleteObjects(ctx context.Context, bucket, prefix string, recur // precaution we're batching deletes. maxKeys := 1000 deleted := make([]types.DeletedObject, 0) + errs := &multierror.Error{} for i := 0; i < len(deleteList); i += maxKeys { j := i + maxKeys @@ -61,9 +63,21 @@ func (c *Client) DeleteObjects(ctx context.Context, bucket, prefix string, recur } deleted = append(deleted, res.Deleted...) + for _, err := range res.Errors { + var e error + switch { + case err.Message != nil: + e = errors.New(*err.Message) + case err.Code != nil: + e = errors.New(*err.Code) + default: + e = fmt.Errorf("undefined error") + } + errs = multierror.Append(errs, e) + } } - return deleted, nil + return deleted, errs.ErrorOrNil() } func (c *Client) GenPresignedURL(ctx context.Context, method, bucket, key string, expires time.Duration) (string, error) { diff --git a/pkg/storage/sos/object_test.go b/pkg/storage/sos/object_test.go index 7c360ab2e..a4156b067 100644 --- a/pkg/storage/sos/object_test.go +++ b/pkg/storage/sos/object_test.go @@ -15,6 +15,7 @@ import ( s3manager "github.com/aws/aws-sdk-go-v2/feature/s3/manager" "github.com/aws/aws-sdk-go-v2/service/s3" "github.com/aws/aws-sdk-go-v2/service/s3/types" + "github.com/hashicorp/go-multierror" "github.com/stretchr/testify/assert" "github.com/exoscale/cli/pkg/storage/sos" @@ -103,6 +104,7 @@ func TestDeleteObjects(t *testing.T) { commonPrefix := "myobjects/" objectKeys := []string{commonPrefix + "object1", commonPrefix + "object2", commonPrefix + "object3"} + // Happy path nCalls := 0 expectedDeleteInput := &s3.DeleteObjectsInput{ Bucket: &bucket, @@ -149,6 +151,7 @@ func TestDeleteObjects(t *testing.T) { assert.Equal(t, objectKeys[i], *key.Key) } + // General error client = sos.Client{ S3Client: &MockS3API{ mockDeleteObjects: func(ctx context.Context, params *s3.DeleteObjectsInput, optFns ...func(*s3.Options)) (*s3.DeleteObjectsOutput, error) { @@ -169,6 +172,50 @@ func TestDeleteObjects(t *testing.T) { _, err = client.DeleteObjects(context.Background(), bucket, commonPrefix, false) assert.Error(t, err) + + // Individual error in batch delete + client = sos.Client{ + S3Client: &MockS3API{ + mockDeleteObjects: func(ctx context.Context, params *s3.DeleteObjectsInput, optFns ...func(*s3.Options)) (*s3.DeleteObjectsOutput, error) { + nCalls++ + assert.Equal(t, expectedDeleteInput, params) + return &s3.DeleteObjectsOutput{ + Deleted: []types.DeletedObject{ + {Key: aws.String(objectKeys[0])}, + {Key: aws.String(objectKeys[2])}, + }, + Errors: []types.Error{ + { + Code: aws.String("AccessDenied"), + Key: aws.String("1"), + Message: aws.String("Access Denied"), + VersionId: aws.String("1"), + }, + }, + }, nil + }, + mockListObjectsV2: func(ctx context.Context, params *s3.ListObjectsV2Input, optFns ...func(*s3.Options)) (*s3.ListObjectsV2Output, error) { + return &s3.ListObjectsV2Output{ + IsTruncated: false, + Contents: []types.Object{ + {Key: aws.String(objectKeys[0])}, + {Key: aws.String(objectKeys[1])}, + {Key: aws.String(objectKeys[2])}, + }, + }, nil + }, + }, + } + deleted, err = client.DeleteObjects(context.Background(), bucket, commonPrefix, false) + + assert.Equal(t, 2, len(deleted)) + assert.Error(t, err) + if merr, ok := err.(*multierror.Error); ok { + assert.Equal(t, 1, len(merr.Errors)) + assert.Equal(t, "Access Denied", merr.Errors[0].Error()) + } else { + assert.NoError(t, err) + } } type MockUploader struct {