From a36d974942d00d8a4452adac1c50c785d70d9046 Mon Sep 17 00:00:00 2001 From: Ben McClelland Date: Tue, 13 Aug 2024 10:52:47 -0700 Subject: [PATCH 1/2] fix: copy-object with replace metadata-directive In copy-object, if the source and destination are the same then X-Amz-Metadata-Directive must be set to "REPLACE" in order to use this api call to update the metadata of the object in place. The default X-Amz-Metadata-Directive is "COPY" if not specified. "COPY" is only valid if source and destination are not the same object. When "REPLACE" selected, metadata does not have to differ for the call to be successful. The "REPLACE" always sets the incoming metadata (even if empty or the same as the source). Fixes #734 --- backend/posix/posix.go | 73 ++++++++++++++------------------ s3api/controllers/base.go | 18 ++++++++ s3err/s3err.go | 6 +++ tests/integration/group-tests.go | 2 + tests/integration/tests.go | 24 +++++++++++ 5 files changed, 82 insertions(+), 41 deletions(-) diff --git a/backend/posix/posix.go b/backend/posix/posix.go index 2143506f..25c30b70 100644 --- a/backend/posix/posix.go +++ b/backend/posix/posix.go @@ -805,20 +805,6 @@ func (p *Posix) loadUserMetaData(bucket, object string, m map[string]string) (st return contentType, contentEncoding } -func compareUserMetadata(meta1, meta2 map[string]string) bool { - if len(meta1) != len(meta2) { - return false - } - - for key, val := range meta1 { - if meta2[key] != val { - return false - } - } - - return true -} - func isValidMeta(val string) bool { if strings.HasPrefix(val, metaHdr) { return true @@ -2012,42 +1998,47 @@ func (p *Posix) CopyObject(ctx context.Context, input *s3.CopyObjectInput) (*s3. return nil, s3err.GetAPIError(s3err.ErrNoSuchKey) } - meta := make(map[string]string) - p.loadUserMetaData(srcBucket, srcObject, meta) + mdmap := make(map[string]string) + p.loadUserMetaData(srcBucket, srcObject, mdmap) + + var etag string dstObjdPath := filepath.Join(dstBucket, dstObject) if dstObjdPath == objPath { - if compareUserMetadata(meta, input.Metadata) { + if input.MetadataDirective == types.MetadataDirectiveCopy { return &s3.CopyObjectOutput{}, s3err.GetAPIError(s3err.ErrInvalidCopyDest) - } else { - for key := range meta { - err := p.meta.DeleteAttribute(dstBucket, dstObject, key) - if err != nil { - return nil, fmt.Errorf("delete user metadata: %w", err) - } + } + + for key := range mdmap { + err := p.meta.DeleteAttribute(dstBucket, dstObject, key) + if err != nil && !errors.Is(err, meta.ErrNoSuchKey) { + return nil, fmt.Errorf("delete user metadata: %w", err) } - for k, v := range input.Metadata { - err := p.meta.StoreAttribute(dstBucket, dstObject, - fmt.Sprintf("%v.%v", metaHdr, k), []byte(v)) - if err != nil { - return nil, fmt.Errorf("set user attr %q: %w", k, err) - } + } + for k, v := range input.Metadata { + err := p.meta.StoreAttribute(dstBucket, dstObject, + fmt.Sprintf("%v.%v", metaHdr, k), []byte(v)) + if err != nil { + return nil, fmt.Errorf("set user attr %q: %w", k, err) } } - } - contentLength := fi.Size() + b, _ := p.meta.RetrieveAttribute(dstBucket, dstObject, etagkey) + etag = string(b) + } else { + contentLength := fi.Size() - etag, err := p.PutObject(ctx, - &s3.PutObjectInput{ - Bucket: &dstBucket, - Key: &dstObject, - Body: f, - ContentLength: &contentLength, - Metadata: meta, - }) - if err != nil { - return nil, err + etag, err = p.PutObject(ctx, + &s3.PutObjectInput{ + Bucket: &dstBucket, + Key: &dstObject, + Body: f, + ContentLength: &contentLength, + Metadata: input.Metadata, + }) + if err != nil { + return nil, err + } } fi, err = os.Stat(dstObjdPath) diff --git a/s3api/controllers/base.go b/s3api/controllers/base.go index b90f2a70..a08f043d 100644 --- a/s3api/controllers/base.go +++ b/s3api/controllers/base.go @@ -1533,6 +1533,7 @@ func (c S3ApiController) PutActions(ctx *fiber.Ctx) error { copySrcModifSince := ctx.Get("X-Amz-Copy-Source-If-Modified-Since") copySrcUnmodifSince := ctx.Get("X-Amz-Copy-Source-If-Unmodified-Since") copySrcRange := ctx.Get("X-Amz-Copy-Source-Range") + directive := ctx.Get("X-Amz-Metadata-Directive") // Permission headers acl := ctx.Get("X-Amz-Acl") @@ -2054,6 +2055,22 @@ func (c S3ApiController) PutActions(ctx *fiber.Ctx) error { metadata := utils.GetUserMetaData(&ctx.Request().Header) + if directive != "" && directive != "COPY" && directive != "REPLACE" { + return SendXMLResponse(ctx, nil, + s3err.GetAPIError(s3err.ErrInvalidMetadataDirective), + &MetaOpts{ + Logger: c.logger, + MetricsMng: c.mm, + Action: metrics.ActionCopyObject, + BucketOwner: parsedAcl.Owner, + }) + } + + metaDirective := types.MetadataDirectiveCopy + if directive == "REPLACE" { + metaDirective = types.MetadataDirectiveReplace + } + res, err := c.be.CopyObject(ctx.Context(), &s3.CopyObjectInput{ Bucket: &bucket, @@ -2065,6 +2082,7 @@ func (c S3ApiController) PutActions(ctx *fiber.Ctx) error { CopySourceIfUnmodifiedSince: umtime, ExpectedBucketOwner: &acct.Access, Metadata: metadata, + MetadataDirective: metaDirective, StorageClass: types.StorageClass(storageClass), }) if err == nil { diff --git a/s3err/s3err.go b/s3err/s3err.go index 9b4b2629..8433ca94 100644 --- a/s3err/s3err.go +++ b/s3err/s3err.go @@ -130,6 +130,7 @@ const ( ErrMalformedACL ErrUnexpectedContent ErrMissingSecurityHeader + ErrInvalidMetadataDirective // Non-AWS errors ErrExistingObjectIsDirectory @@ -514,6 +515,11 @@ var errorCodeResponse = map[ErrorCode]APIError{ Description: "Your request was missing a required header", HTTPStatusCode: http.StatusNotFound, }, + ErrInvalidMetadataDirective: { + Code: "InvalidArgument", + Description: "Unknown metadata directive.", + HTTPStatusCode: http.StatusBadRequest, + }, // non aws errors ErrExistingObjectIsDirectory: { diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index f0692d4a..f8ee0853 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -199,6 +199,7 @@ func TestCopyObject(s *S3Conf) { CopyObject_non_existing_dst_bucket(s) CopyObject_not_owned_source_bucket(s) CopyObject_copy_to_itself(s) + CopyObject_copy_to_itself_invalid_directive(s) CopyObject_to_itself_with_new_metadata(s) CopyObject_CopySource_starting_with_slash(s) CopyObject_non_existing_dir_object(s) @@ -620,6 +621,7 @@ func GetIntTests() IntTests { "CopyObject_non_existing_dst_bucket": CopyObject_non_existing_dst_bucket, "CopyObject_not_owned_source_bucket": CopyObject_not_owned_source_bucket, "CopyObject_copy_to_itself": CopyObject_copy_to_itself, + "CopyObject_copy_to_itself_invalid_directive": CopyObject_copy_to_itself_invalid_directive, "CopyObject_to_itself_with_new_metadata": CopyObject_to_itself_with_new_metadata, "CopyObject_CopySource_starting_with_slash": CopyObject_CopySource_starting_with_slash, "CopyObject_non_existing_dir_object": CopyObject_non_existing_dir_object, diff --git a/tests/integration/tests.go b/tests/integration/tests.go index a4c1ab9d..0f5d7614 100644 --- a/tests/integration/tests.go +++ b/tests/integration/tests.go @@ -4268,6 +4268,29 @@ func CopyObject_copy_to_itself(s *S3Conf) error { }) } +func CopyObject_copy_to_itself_invalid_directive(s *S3Conf) error { + testName := "CopyObject_copy_to_itself_invalid_directive" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + obj := "my-obj" + err := putObjects(s3client, []string{obj}, bucket) + if err != nil { + return err + } + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.CopyObject(ctx, &s3.CopyObjectInput{ + Bucket: &bucket, + Key: &obj, + CopySource: getPtr(fmt.Sprintf("%v/%v", bucket, obj)), + MetadataDirective: types.MetadataDirective("invalid"), + }) + cancel() + if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrInvalidMetadataDirective)); err != nil { + return err + } + return nil + }) +} + func CopyObject_to_itself_with_new_metadata(s *S3Conf) error { testName := "CopyObject_to_itself_with_new_metadata" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { @@ -4284,6 +4307,7 @@ func CopyObject_to_itself_with_new_metadata(s *S3Conf) error { Metadata: map[string]string{ "Hello": "World", }, + MetadataDirective: types.MetadataDirectiveReplace, }) cancel() if err != nil { From 54490f55ccdce1ee4a33b9d08999d286d96c9436 Mon Sep 17 00:00:00 2001 From: Ben McClelland Date: Tue, 13 Aug 2024 11:09:11 -0700 Subject: [PATCH 2/2] chore: cleanup staticcheck errors --- backend/s3proxy/s3.go | 2 +- s3api/controllers/base.go | 2 +- tests/integration/utils.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/backend/s3proxy/s3.go b/backend/s3proxy/s3.go index f2e2cdf7..9a9533c1 100644 --- a/backend/s3proxy/s3.go +++ b/backend/s3proxy/s3.go @@ -654,7 +654,7 @@ func (s *S3Proxy) ChangeBucketOwner(ctx context.Context, bucket string, acl []by return err } defer resp.Body.Close() - return fmt.Errorf(string(body)) + return fmt.Errorf("%v", string(body)) } return nil diff --git a/s3api/controllers/base.go b/s3api/controllers/base.go index a08f043d..cfda9724 100644 --- a/s3api/controllers/base.go +++ b/s3api/controllers/base.go @@ -2801,7 +2801,7 @@ func (c S3ApiController) CreateActions(ctx *fiber.Ctx) error { if ctx.Request().URI().QueryArgs().Has("restore") { var restoreRequest types.RestoreRequest if err := xml.Unmarshal(ctx.Body(), &restoreRequest); err != nil { - if !errors.Is(io.EOF, err) { + if !errors.Is(err, io.EOF) { return SendResponse(ctx, s3err.GetAPIError(s3err.ErrMalformedXML), &MetaOpts{ Logger: c.logger, diff --git a/tests/integration/utils.go b/tests/integration/utils.go index 785ee88b..a4d3d9d1 100644 --- a/tests/integration/utils.go +++ b/tests/integration/utils.go @@ -613,7 +613,7 @@ func changeBucketsOwner(s *S3Conf, buckets []string, owner string) error { return err } if !strings.Contains(string(out), "Bucket owner has been updated successfully") { - return fmt.Errorf(string(out)) + return fmt.Errorf("%v", string(out)) } }