From 7f15a9fed48afaeb62b0fd9f47bea56ccbc76d24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguy=E1=BB=85n=20L=E1=BB=99c=20Ph=C3=BAc?= <112325531+phucnguyen261199@users.noreply.github.com> Date: Sat, 6 Jun 2026 02:41:18 +0700 Subject: [PATCH] fix(s3api): standardize ETag calculation in copy handlers (#9829) * fix(s3api): standardize ETag calculation across S3 API handlers * s3: make copyEntryETag nil-safe --------- Co-authored-by: Chris Lu --- weed/s3api/s3api_object_handlers_copy.go | 46 ++++--------- .../s3api_object_handlers_copy_etag_test.go | 65 +++++++++++++++++++ 2 files changed, 77 insertions(+), 34 deletions(-) create mode 100644 weed/s3api/s3api_object_handlers_copy_etag_test.go diff --git a/weed/s3api/s3api_object_handlers_copy.go b/weed/s3api/s3api_object_handlers_copy.go index 512bfa95b..2286f6171 100644 --- a/weed/s3api/s3api_object_handlers_copy.go +++ b/weed/s3api/s3api_object_handlers_copy.go @@ -444,8 +444,11 @@ func cloneProtoEntry(entry *filer_pb.Entry) *filer_pb.Entry { return proto.Clone(entry).(*filer_pb.Entry) } -func copyEntryETag(fullPath util.FullPath, entry *filer_pb.Entry) string { - if entry != nil && entry.Extended != nil { +func copyEntryETag(entry *filer_pb.Entry) string { + if entry == nil { + return "" + } + if entry.Extended != nil { if etag, ok := entry.Extended[s3_constants.ExtETagKey]; ok && len(etag) > 0 { return string(etag) } @@ -461,11 +464,10 @@ func copyEntryETag(fullPath util.FullPath, entry *filer_pb.Entry) string { } } return filer.ETagEntry(&filer.Entry{ - FullPath: fullPath, - Attr: attr, - Chunks: entry.Chunks, - Content: entry.Content, - Remote: entry.RemoteEntry, + Attr: attr, + Chunks: entry.Chunks, + Content: entry.Content, + Remote: entry.RemoteEntry, }) } @@ -494,7 +496,7 @@ func (s3a *S3ApiServer) finalizeCopyDestination(dstBucket, dstObject, dstVersion dstEntry.Extended = make(map[string][]byte) } - etag = copyEntryETag(dstPath, dstEntry) + etag = copyEntryETag(dstEntry) switch dstVersioningState { case s3_constants.VersioningEnabled: @@ -987,19 +989,7 @@ func (s3a *S3ApiServer) CopyObjectPartHandler(w http.ResponseWriter, r *http.Req } // Calculate ETag for the part - partPath := util.FullPath(uploadDir + "/" + partName) - filerEntry := &filer.Entry{ - FullPath: partPath, - Attr: filer.Attr{ - FileSize: dstEntry.Attributes.FileSize, - Mtime: time.Unix(dstEntry.Attributes.Mtime, 0), - Crtime: time.Unix(dstEntry.Attributes.Crtime, 0), - Mime: dstEntry.Attributes.Mime, - }, - Chunks: dstEntry.Chunks, - } - - etag := filer.ETagEntry(filerEntry) + etag := copyEntryETag(dstEntry) setEtag(w, etag) response := CopyPartResult{ @@ -1429,19 +1419,7 @@ func (s3a *S3ApiServer) copyChunksForRange(entry *filer_pb.Entry, startOffset, e // validateConditionalCopyHeaders validates the conditional copy headers against the source entry func (s3a *S3ApiServer) validateConditionalCopyHeaders(r *http.Request, entry *filer_pb.Entry) s3err.ErrorCode { - // Calculate ETag for the source entry. ETagEntry derives the tag from the - // chunks/Md5/remote-etag only, so no path is needed here. - filerEntry := &filer.Entry{ - Attr: filer.Attr{ - FileSize: entry.Attributes.FileSize, - Mtime: time.Unix(entry.Attributes.Mtime, 0), - Crtime: time.Unix(entry.Attributes.Crtime, 0), - Mime: entry.Attributes.Mime, - Md5: entry.Attributes.Md5, - }, - Chunks: entry.Chunks, - } - sourceETag := filer.ETagEntry(filerEntry) + sourceETag := copyEntryETag(entry) // Check X-Amz-Copy-Source-If-Match if ifMatch := r.Header.Get(s3_constants.AmzCopySourceIfMatch); ifMatch != "" { diff --git a/weed/s3api/s3api_object_handlers_copy_etag_test.go b/weed/s3api/s3api_object_handlers_copy_etag_test.go new file mode 100644 index 000000000..11036cd3e --- /dev/null +++ b/weed/s3api/s3api_object_handlers_copy_etag_test.go @@ -0,0 +1,65 @@ +package s3api + +import ( + "net/http/httptest" + "strings" + "testing" + + "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3err" +) + +func TestCopyEntryETagPrefersStoredExtendedETag(t *testing.T) { + storedETag := "11111111111111111111111111111111-2" + entry := newCopyETagTestEntry(t, storedETag, "22222222222222222222222222222222") + + if got := copyEntryETag(entry); got != storedETag { + t.Fatalf("copyEntryETag() = %q, want stored extended ETag %q", got, storedETag) + } +} + +func TestCopyEntryETagFallsBackToFilerETag(t *testing.T) { + computedETag := "33333333333333333333333333333333" + entry := newCopyETagTestEntry(t, "", computedETag) + + if got := strings.Trim(copyEntryETag(entry), `"`); got != computedETag { + t.Fatalf("copyEntryETag() = %q, want fallback filer ETag %q", got, computedETag) + } +} + +func TestValidateConditionalCopyHeadersUsesStoredExtendedETag(t *testing.T) { + storedETag := "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-2" + entry := newCopyETagTestEntry(t, storedETag, "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb") + s3a := &S3ApiServer{} + + matchReq := httptest.NewRequest("PUT", "/dst", nil) + matchReq.Header.Set(s3_constants.AmzCopySourceIfMatch, `"`+storedETag+`"`) + if got := s3a.validateConditionalCopyHeaders(matchReq, entry); got != s3err.ErrNone { + t.Fatalf("validateConditionalCopyHeaders(If-Match stored ETag) = %v, want %v", got, s3err.ErrNone) + } + + noneMatchReq := httptest.NewRequest("PUT", "/dst", nil) + noneMatchReq.Header.Set(s3_constants.AmzCopySourceIfNoneMatch, storedETag) + if got := s3a.validateConditionalCopyHeaders(noneMatchReq, entry); got != s3err.ErrPreconditionFailed { + t.Fatalf("validateConditionalCopyHeaders(If-None-Match stored ETag) = %v, want %v", got, s3err.ErrPreconditionFailed) + } +} + +func newCopyETagTestEntry(t *testing.T, storedETag, computedETag string) *filer_pb.Entry { + t.Helper() + + entry := &filer_pb.Entry{ + Name: "object", + Attributes: &filer_pb.FuseAttributes{ + FileSize: 5, + Md5: mustDecodeHexETagForTest(t, computedETag), + }, + } + if storedETag != "" { + entry.Extended = map[string][]byte{ + s3_constants.ExtETagKey: []byte(storedETag), + } + } + return entry +}