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_<id> 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 <chris.lu@gmail.com>
Co-authored-by: chrislusf <chrislusf@users.noreply.github.com>

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
 }

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"))
+		})
+	}
+}

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_<id> 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
 			}
 		}

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

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