From 515380c349fb40936a997c9d1e9de9d0dc77b034 Mon Sep 17 00:00:00 2001 From: Jay Hemnani <193022578+jayhemnani9910@users.noreply.github.com> Date: Mon, 26 Oct 2026 00:32:19 +0530 Subject: [PATCH] s3: answer GET and HEAD of a delete marker as S3 does (#12051) * s3: answer GET and HEAD of a delete marker as S3 does A delete marker as the current version now returns 404 NoSuchKey with x-amz-delete-marker: true and the marker's x-amz-version-id. Asking for the marker by versionId returns 405 MethodNotAllowed with the same headers and Last-Modified, instead of 404 NoSuchKey. * s3: check the tag-conditioned read grant before answering a delete marker The marker branch returned before the entry-based policy recheck the rest of the read path performs, so an s3:ExistingObjectTag-conditioned Deny did not apply to the delete-marker response and its version headers. * s3: send Allow: DELETE with the delete-marker 405 RFC 9110 15.5.6 requires a 405 answer to list the methods the resource supports; S3 sends allow: DELETE for a delete marker named by versionId. * s3: keep delete-marker headers on conditional-read misses If-Match or If-Unmodified-Since against a current delete marker short- circuited to a bare NoSuchKey. Carry the resolved marker through the conditional result so the 404 keeps x-amz-delete-marker and x-amz-version-id, matching the unconditional read. * s3: exercise delete-marker responses through GET and HEAD handlers * s3: read marker version id from the entry before the caller's guess A delete marker written outside the normal PUT path can miss the version-id attribute while the v_ file name still names it; the new x-amz-version-id header would then report "null". Reuse versionIdFromEntry so both the handler and conditional-miss paths emit the real id. * s3: answer 405 for versionId=null on a suspended-versioning marker The null marker lives in .versions while getSpecificObjectVersion only checks the plain path, so GET/HEAD ?versionId=null answered NoSuchVersion. On that miss, look up the null marker and answer it like any other marker named by versionId. * s3: propagate null-marker lookup failures instead of answering 404 A transient filer error reading .versions/v_null was swallowed as "no marker", so GET/HEAD answered NoSuchVersion for a live read failure. NotFound and absent entries still mean no marker; anything else is InternalError now. --------- Co-authored-by: Chris Lu Co-authored-by: chrislusf --- weed/s3api/custom_types.go | 7 +- ...3api_object_delete_marker_response_test.go | 226 ++++++++++++++++++ weed/s3api/s3api_object_handlers.go | 85 ++++++- weed/s3api/s3api_object_handlers_put.go | 6 +- weed/s3api/s3api_object_versioning.go | 20 ++ 5 files changed, 333 insertions(+), 11 deletions(-) create mode 100644 weed/s3api/s3api_object_delete_marker_response_test.go diff --git a/weed/s3api/custom_types.go b/weed/s3api/custom_types.go index 0e147dced1..a3d139fddb 100644 --- a/weed/s3api/custom_types.go +++ b/weed/s3api/custom_types.go @@ -7,7 +7,8 @@ import ( // ConditionalHeaderResult holds the result of conditional header checking type ConditionalHeaderResult struct { - ErrorCode s3err.ErrorCode - ETag string // ETag of the object (for 304 responses) - Entry *filer_pb.Entry // Entry fetched during conditional check (nil if not fetched or object doesn't exist) + ErrorCode s3err.ErrorCode + ETag string // ETag of the object (for 304 responses) + Entry *filer_pb.Entry // Entry fetched during conditional check (nil if not fetched or object doesn't exist) + DeleteMarker *filer_pb.Entry // Set when the resolved entry is a delete marker, so a missing-key answer can still carry its headers } diff --git a/weed/s3api/s3api_object_delete_marker_response_test.go b/weed/s3api/s3api_object_delete_marker_response_test.go new file mode 100644 index 0000000000..f96989f0f7 --- /dev/null +++ b/weed/s3api/s3api_object_delete_marker_response_test.go @@ -0,0 +1,226 @@ +package s3api + +import ( + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/gorilla/mux" + "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3err" + "github.com/stretchr/testify/require" +) + +func TestWriteDeleteMarkerResponse(t *testing.T) { + s3a := &S3ApiServer{} + entry := &filer_pb.Entry{ + Name: "v_marker", + Attributes: &filer_pb.FuseAttributes{Mtime: 784111777}, + Extended: map[string][]byte{s3_constants.ExtDeleteMarkerKey: []byte("true")}, + } + for _, tc := range []struct { + name, requestedVersionId string + status int + lastModified string + allow string + }{ + {"current version", "", http.StatusNotFound, "", ""}, + {"marker by versionId", "marker", http.StatusMethodNotAllowed, "Sun, 06 Nov 1994 08:49:37 GMT", "DELETE"}, + } { + for _, method := range []string{http.MethodGet, http.MethodHead} { + w := httptest.NewRecorder() + s3a.writeDeleteMarkerResponse(w, httptest.NewRequest(method, "/bucket/object", nil), "bucket", "object", "TestHandler", entry, "marker", tc.requestedVersionId) + if w.Code != tc.status { + t.Errorf("%s %s: status %d, want %d", tc.name, method, w.Code, tc.status) + } + if got := w.Header().Get(s3_constants.AmzDeleteMarker); got != "true" { + t.Errorf("%s %s: %s = %q, want true", tc.name, method, s3_constants.AmzDeleteMarker, got) + } + if got := w.Header().Get("x-amz-version-id"); got != "marker" { + t.Errorf("%s %s: x-amz-version-id = %q, want marker", tc.name, method, got) + } + if got := w.Header().Get("Last-Modified"); got != tc.lastModified { + t.Errorf("%s %s: Last-Modified = %q, want %q", tc.name, method, got, tc.lastModified) + } + if got := w.Header().Get("Allow"); got != tc.allow { + t.Errorf("%s %s: Allow = %q, want %q", tc.name, method, got, tc.allow) + } + } + } +} + +func TestWriteDeleteMarkerResponseAnonymous(t *testing.T) { + s3a, _ := newAnonymousReadTestServer(t) + require.NoError(t, s3a.policyEngine.engine.SetBucketPolicy("b", `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Principal":"*","Action":"s3:ListBucket","Resource":"arn:aws:s3:::b"}]}`)) + entry := anonymousReadEntry(anonymousReadGrant("READ")) + entry.Extended[s3_constants.ExtDeleteMarkerKey] = []byte("true") + for _, query := range []string{"", "?versionId=m"} { + r := mux.SetURLVars(httptest.NewRequest(http.MethodGet, "/b/o"+query, nil), map[string]string{"bucket": "b", "object": "o"}) + r, deferred := s3a.deferAnonymousObjectRead(r, s3_constants.ACTION_READ, "b", "o") + require.True(t, deferred) + w := httptest.NewRecorder() + s3a.writeDeleteMarkerResponse(w, r, "b", "o", "TestHandler", entry, "m", r.URL.Query().Get("versionId")) + require.Equal(t, http.StatusNotFound, w.Code, query) + require.Empty(t, w.Header().Get(s3_constants.AmzDeleteMarker), query) + require.Empty(t, w.Header().Get("x-amz-version-id"), query) + } +} + +func TestWriteConditionalCheckErrorDeleteMarker(t *testing.T) { + s3a := &S3ApiServer{} + marker := &filer_pb.Entry{ + Name: "v_marker", + Extended: map[string][]byte{s3_constants.ExtDeleteMarkerKey: []byte("true"), s3_constants.ExtVersionIdKey: []byte("marker")}, + } + for _, method := range []string{http.MethodGet, http.MethodHead} { + w := httptest.NewRecorder() + s3a.writeConditionalCheckError(w, httptest.NewRequest(method, "/bucket/object", nil), "bucket", "object", "TestHandler", ConditionalHeaderResult{ErrorCode: s3err.ErrNoSuchKey, DeleteMarker: marker}) + require.Equal(t, http.StatusNotFound, w.Code, method) + require.Equal(t, "true", w.Header().Get(s3_constants.AmzDeleteMarker), method) + require.Equal(t, "marker", w.Header().Get("x-amz-version-id"), method) + } + + w := httptest.NewRecorder() + s3a.writeConditionalCheckError(w, httptest.NewRequest(http.MethodGet, "/bucket/object", nil), "bucket", "object", "TestHandler", ConditionalHeaderResult{ErrorCode: s3err.ErrNoSuchKey}) + require.Equal(t, http.StatusNotFound, w.Code) + require.Empty(t, w.Header().Get(s3_constants.AmzDeleteMarker)) +} + +func TestWriteConditionalCheckErrorDeleteMarkerVersionFromName(t *testing.T) { + s3a := &S3ApiServer{} + marker := &filer_pb.Entry{ + Name: "v_named", + Extended: map[string][]byte{s3_constants.ExtDeleteMarkerKey: []byte("true")}, + } + w := httptest.NewRecorder() + s3a.writeConditionalCheckError(w, httptest.NewRequest(http.MethodGet, "/bucket/object", nil), "bucket", "object", "TestHandler", ConditionalHeaderResult{ErrorCode: s3err.ErrNoSuchKey, DeleteMarker: marker}) + require.Equal(t, http.StatusNotFound, w.Code) + require.Equal(t, "named", w.Header().Get("x-amz-version-id")) +} + +func newDeleteMarkerTestServer(t *testing.T) *S3ApiServer { + t.Helper() + f := &anonymousReadFiler{entries: map[string]*filer_pb.Entry{ + "/buckets/b/o.versions": { + Name: "o.versions", + IsDirectory: true, + Attributes: &filer_pb.FuseAttributes{Mtime: 784111700}, + Extended: map[string][]byte{ + s3_constants.ExtLatestVersionIdKey: []byte("marker"), + s3_constants.ExtLatestVersionFileNameKey: []byte("v_marker"), + }, + }, + "/buckets/b/o.versions/v_marker": { + Name: "v_marker", + Attributes: &filer_pb.FuseAttributes{Mtime: 784111777}, + Extended: map[string][]byte{ + s3_constants.ExtDeleteMarkerKey: []byte("true"), + s3_constants.ExtVersionIdKey: []byte("marker"), + }, + }, + }} + s3a := newPutTestServer(t, startFakeFiler(t, f)) + s3a.bucketConfigCache = NewBucketConfigCache(time.Minute) + s3a.bucketConfigCache.Set("b", &BucketConfig{Name: "b", Versioning: s3_constants.VersioningEnabled}) + return s3a +} + +func TestGetHeadDeleteMarker(t *testing.T) { + for _, tc := range []struct { + name, query string + status int + lastModified, allow string + }{ + {"current marker", "", http.StatusNotFound, "", ""}, + {"marker by versionId", "?versionId=marker", http.StatusMethodNotAllowed, "Sun, 06 Nov 1994 08:49:37 GMT", "DELETE"}, + } { + for _, method := range []string{http.MethodGet, http.MethodHead} { + t.Run(tc.name+"/"+method, func(t *testing.T) { + s3a := newDeleteMarkerTestServer(t) + r := mux.SetURLVars(httptest.NewRequest(method, "/b/o"+tc.query, nil), map[string]string{"bucket": "b", "object": "o"}) + w := httptest.NewRecorder() + handler := s3a.GetObjectHandler + if method == http.MethodHead { + handler = s3a.HeadObjectHandler + } + handler(w, r) + require.Equal(t, tc.status, w.Code) + require.Equal(t, "true", w.Header().Get(s3_constants.AmzDeleteMarker)) + require.Equal(t, "marker", w.Header().Get("x-amz-version-id")) + require.Equal(t, tc.lastModified, w.Header().Get("Last-Modified")) + require.Equal(t, tc.allow, w.Header().Get("Allow")) + }) + } + } +} + +func TestGetHeadNullDeleteMarker(t *testing.T) { + f := &anonymousReadFiler{entries: map[string]*filer_pb.Entry{ + "/buckets/b/o.versions": { + Name: "o.versions", + IsDirectory: true, + Attributes: &filer_pb.FuseAttributes{Mtime: 784111700}, + Extended: map[string][]byte{ + s3_constants.ExtLatestVersionIdKey: []byte("null"), + s3_constants.ExtLatestVersionFileNameKey: []byte("v_null"), + }, + }, + "/buckets/b/o.versions/v_null": { + Name: "v_null", + Attributes: &filer_pb.FuseAttributes{Mtime: 784111777}, + Extended: map[string][]byte{ + s3_constants.ExtDeleteMarkerKey: []byte("true"), + s3_constants.ExtVersionIdKey: []byte("null"), + }, + }, + }} + for _, method := range []string{http.MethodGet, http.MethodHead} { + t.Run(method, func(t *testing.T) { + s3a := newPutTestServer(t, startFakeFiler(t, f)) + s3a.bucketConfigCache = NewBucketConfigCache(time.Minute) + s3a.bucketConfigCache.Set("b", &BucketConfig{Name: "b", Versioning: s3_constants.VersioningSuspended}) + r := mux.SetURLVars(httptest.NewRequest(method, "/b/o?versionId=null", nil), map[string]string{"bucket": "b", "object": "o"}) + w := httptest.NewRecorder() + handler := s3a.GetObjectHandler + if method == http.MethodHead { + handler = s3a.HeadObjectHandler + } + handler(w, r) + require.Equal(t, http.StatusMethodNotAllowed, w.Code) + require.Equal(t, "true", w.Header().Get(s3_constants.AmzDeleteMarker)) + require.Equal(t, "null", w.Header().Get("x-amz-version-id")) + require.Equal(t, "DELETE", w.Header().Get("Allow")) + }) + } +} + +func TestGetHeadNullVersionAbsent(t *testing.T) { + s3a := newDeleteMarkerTestServer(t) + r := mux.SetURLVars(httptest.NewRequest(http.MethodGet, "/b/o?versionId=null", nil), map[string]string{"bucket": "b", "object": "o"}) + w := httptest.NewRecorder() + s3a.GetObjectHandler(w, r) + require.Equal(t, http.StatusNotFound, w.Code) + require.Empty(t, w.Header().Get(s3_constants.AmzDeleteMarker)) +} + +func TestGetHeadDeleteMarkerConditionalMiss(t *testing.T) { + for _, method := range []string{http.MethodGet, http.MethodHead} { + t.Run(method, func(t *testing.T) { + s3a := newDeleteMarkerTestServer(t) + r := httptest.NewRequest(method, "/b/o", nil) + r.Header.Set("If-Match", `"no-match"`) + r = mux.SetURLVars(r, map[string]string{"bucket": "b", "object": "o"}) + w := httptest.NewRecorder() + handler := s3a.GetObjectHandler + if method == http.MethodHead { + handler = s3a.HeadObjectHandler + } + handler(w, r) + require.Equal(t, http.StatusNotFound, w.Code) + require.Equal(t, "true", w.Header().Get(s3_constants.AmzDeleteMarker)) + require.Equal(t, "marker", w.Header().Get("x-amz-version-id")) + }) + } +} diff --git a/weed/s3api/s3api_object_handlers.go b/weed/s3api/s3api_object_handlers.go index 4c16300cd5..ea962a5fb2 100644 --- a/weed/s3api/s3api_object_handlers.go +++ b/weed/s3api/s3api_object_handlers.go @@ -790,16 +790,63 @@ func (s3a *S3ApiServer) processConditionalHeaders(w http.ResponseWriter, r *http w.Header().Set("ETag", result.ETag) } - if result.ErrorCode == s3err.ErrNoSuchKey || result.ErrorCode == s3err.ErrNoSuchBucket { - s3a.writeAnonymousAwareNotFound(w, r, bucket, result.ErrorCode) - } else { - s3err.WriteErrorResponse(w, r, result.ErrorCode) - } + s3a.writeConditionalCheckError(w, r, bucket, object, handlerName, result) return result, true // request handled } return result, false // request not handled } +// writeConditionalCheckError answers a failed conditional-header check. A +// missing-key answer still carries the delete marker's headers when the +// condition failed against one, so a resuming client can tell a deleted key +// from one that never existed. +func (s3a *S3ApiServer) writeConditionalCheckError(w http.ResponseWriter, r *http.Request, bucket, object, handlerName string, result ConditionalHeaderResult) { + if result.ErrorCode == s3err.ErrNoSuchKey && result.DeleteMarker != nil { + s3a.writeDeleteMarkerResponse(w, r, bucket, object, handlerName, result.DeleteMarker, "", "") + return + } + if result.ErrorCode == s3err.ErrNoSuchKey || result.ErrorCode == s3err.ErrNoSuchBucket { + s3a.writeAnonymousAwareNotFound(w, r, bucket, result.ErrorCode) + return + } + s3err.WriteErrorResponse(w, r, result.ErrorCode) +} + +// writeDeleteMarkerResponse answers a GET or HEAD that resolved to a delete +// marker as S3 does: x-amz-delete-marker and the marker's version id, with +// 404 NoSuchKey for the current version and 405 MethodNotAllowed when the +// marker itself was requested by versionId. Anonymous reads never get a +// delete marker (authorizeAnonymousObjectRead), so they keep the plain miss. +func (s3a *S3ApiServer) writeDeleteMarkerResponse(w http.ResponseWriter, r *http.Request, bucket, object, handlerName string, entry *filer_pb.Entry, markerVersionId, requestedVersionId string) { + if isAnonymousObjectRead(r) { + s3a.writeAnonymousAwareNotFound(w, r, bucket, s3err.ErrNoSuchKey) + return + } + if errCode := s3a.recheckPolicyWithObjectEntry(r, bucket, object, string(s3_constants.ACTION_READ), entry.Extended, handlerName); errCode != s3err.ErrNone { + s3err.WriteErrorResponse(w, r, errCode) + return + } + // The entry's own version id wins: markers written outside the normal PUT + // path may lack the attribute yet still carry it in the v_ file name. + if versionId := versionIdFromEntry(entry); versionId != "" { + markerVersionId = versionId + } + if markerVersionId == "" { + markerVersionId = "null" + } + w.Header().Set(s3_constants.AmzDeleteMarker, "true") + w.Header().Set("x-amz-version-id", markerVersionId) + if requestedVersionId == "" { + s3err.WriteErrorResponse(w, r, s3err.ErrNoSuchKey) + return + } + w.Header().Set("Allow", "DELETE") + if entry.Attributes != nil { + w.Header().Set("Last-Modified", time.Unix(entry.Attributes.Mtime, 0).UTC().Format(http.TimeFormat)) + } + s3err.WriteErrorResponse(w, r, s3err.ErrMethodNotAllowed) +} + func (s3a *S3ApiServer) GetObjectHandler(w http.ResponseWriter, r *http.Request) { bucket, object := s3_constants.GetBucketAndObject(r) @@ -882,6 +929,18 @@ func (s3a *S3ApiServer) GetObjectHandler(w http.ResponseWriter, r *http.Request) glog.V(3).Infof("GetObject: requesting specific version %s for %s/%s", versionId, bucket, object) entry, err = s3a.getSpecificObjectVersion(bucket, object, versionId) if err != nil { + if versionId == "null" && errors.Is(err, filer_pb.ErrNotFound) { + marker, markerErr := s3a.nullDeleteMarkerEntry(bucket, object) + if markerErr != nil { + glog.Errorf("GetObject: failed to look up null delete marker for %s/%s: %v", bucket, object, markerErr) + s3err.WriteErrorResponse(w, r, s3err.ErrInternalError) + return + } + if marker != nil { + s3a.writeDeleteMarkerResponse(w, r, bucket, object, "GetObjectHandler", marker, "null", versionId) + return + } + } glog.Errorf("Failed to get specific version %s: %v", versionId, err) if errors.Is(err, filer_pb.ErrNotFound) { s3a.writeAnonymousAwareNotFound(w, r, bucket, s3err.ErrNoSuchVersion) @@ -954,7 +1013,7 @@ func (s3a *S3ApiServer) GetObjectHandler(w http.ResponseWriter, r *http.Request) // Check if this is a delete marker if entry.Extended != nil { if deleteMarker, exists := entry.Extended[s3_constants.ExtDeleteMarkerKey]; exists && string(deleteMarker) == "true" { - s3a.writeAnonymousAwareNotFound(w, r, bucket, s3err.ErrNoSuchKey) + s3a.writeDeleteMarkerResponse(w, r, bucket, object, "GetObjectHandler", entry, targetVersionId, versionId) return } } @@ -2565,6 +2624,18 @@ func (s3a *S3ApiServer) HeadObjectHandler(w http.ResponseWriter, r *http.Request glog.V(2).Infof("HeadObject: requesting specific version %s for %s/%s", versionId, bucket, object) entry, err = s3a.getSpecificObjectVersion(bucket, object, versionId) if err != nil { + if versionId == "null" && errors.Is(err, filer_pb.ErrNotFound) { + marker, markerErr := s3a.nullDeleteMarkerEntry(bucket, object) + if markerErr != nil { + glog.Errorf("HeadObject: failed to look up null delete marker for %s/%s: %v", bucket, object, markerErr) + s3err.WriteErrorResponse(w, r, s3err.ErrInternalError) + return + } + if marker != nil { + s3a.writeDeleteMarkerResponse(w, r, bucket, object, "HeadObjectHandler", marker, "null", versionId) + return + } + } glog.Errorf("Failed to get specific version %s for %s/%s: %v", versionId, bucket, object, err) if errors.Is(err, filer_pb.ErrNotFound) { s3a.writeAnonymousAwareNotFound(w, r, bucket, s3err.ErrNoSuchVersion) @@ -2637,7 +2708,7 @@ func (s3a *S3ApiServer) HeadObjectHandler(w http.ResponseWriter, r *http.Request // Check if this is a delete marker if entry.Extended != nil { if deleteMarker, exists := entry.Extended[s3_constants.ExtDeleteMarkerKey]; exists && string(deleteMarker) == "true" { - s3a.writeAnonymousAwareNotFound(w, r, bucket, s3err.ErrNoSuchKey) + s3a.writeDeleteMarkerResponse(w, r, bucket, object, "HeadObjectHandler", entry, targetVersionId, versionId) return } } diff --git a/weed/s3api/s3api_object_handlers_put.go b/weed/s3api/s3api_object_handlers_put.go index aa6682712a..d8bdc0df7f 100644 --- a/weed/s3api/s3api_object_handlers_put.go +++ b/weed/s3api/s3api_object_handlers_put.go @@ -3022,7 +3022,11 @@ func (s3a *S3ApiServer) checkConditionalHeadersForReads(r *http.Request, bucket, if versionId != "" && normalizeConditionalTargetEntry(entry) == nil { return ConditionalHeaderResult{ErrorCode: s3err.ErrNone, Entry: nil} } - return s3a.validateConditionalHeadersForReads(r, headers, entry, bucket, object) + result := s3a.validateConditionalHeadersForReads(r, headers, entry, bucket, object) + if entry != nil && normalizeConditionalTargetEntry(entry) == nil { + result.DeleteMarker = entry + } + return result } // lookupVolumeJwt mints a fresh volume-server JWT for an existing file id, so a diff --git a/weed/s3api/s3api_object_versioning.go b/weed/s3api/s3api_object_versioning.go index 0e4cc950f3..8b9c4209cf 100644 --- a/weed/s3api/s3api_object_versioning.go +++ b/weed/s3api/s3api_object_versioning.go @@ -314,6 +314,26 @@ func (s3a *S3ApiServer) createNullDeleteMarker(bucket, object string) error { return nil } +// nullDeleteMarkerEntry returns the suspended-versioning "null" delete marker +// stored under .versions, or nil when the object has none. +// getSpecificObjectVersion only checks the plain path for "null", so callers +// that must answer a marker named by versionId consult this on a miss. Lookup +// failures other than not found propagate: a masked read error would answer +// a live filer hiccup as a deleted version. +func (s3a *S3ApiServer) nullDeleteMarkerEntry(bucket, object string) (*filer_pb.Entry, error) { + entry, err := s3a.getEntry(s3a.getVersionedObjectDir(bucket, s3_constants.NormalizeObjectKey(object)), s3a.getVersionFileName("null")) + if err != nil { + if errors.Is(err, filer_pb.ErrNotFound) { + return nil, nil + } + return nil, err + } + if entry == nil || string(entry.Extended[s3_constants.ExtDeleteMarkerKey]) != "true" { + return nil, nil + } + return entry, nil +} + // versionListItem represents an item in the unified version/prefix list type versionListItem struct { key string