diff --git a/adapter/s3.go b/adapter/s3.go index 47524d753..b28300c54 100644 --- a/adapter/s3.go +++ b/adapter/s3.go @@ -1179,16 +1179,10 @@ func (s *S3Server) deleteObject(w http.ResponseWriter, r *http.Request, bucket s cleanupManifest = nil return nil } - dispatchCtx := readTimestamp.WithDispatchVoucher(r.Context()) - _, err = kv.DispatchWithReadTimestamp(dispatchCtx, s.coordinator, &kv.OperationGroup[kv.OP]{ - IsTxn: true, - StartTS: startTS, - Elems: []*kv.Elem[kv.OP]{ - {Op: kv.Del, Key: headKey}, - }, - }) - if err != nil { - return errors.WithStack(err) + if err := s.dispatchConditionalObjectDelete( + r, readTimestamp, startTS, headKey, manifest, bucket, objectKey, + ); err != nil { + return err } cleanupManifest = manifest generation = meta.Generation @@ -2742,6 +2736,66 @@ func validateS3PutPreconditions(r *http.Request, previous *s3ObjectManifest) err return nil } +// dispatchConditionalObjectDelete evaluates If-Match against the manifest the +// surrounding transaction already loaded, then removes the head key. +// +// DELETE honours If-Match, like PUT does via validateS3PutPreconditions. +// Accepting the header and deleting anyway is worse than not supporting it: a +// caller that reads an object, decides it is reclaimable, and sends a +// conditional delete believes the condition protects it against a concurrent +// rewrite. It did not -- the object was removed whatever its current ETag -- so +// the rewrite was silently lost. +func (s *S3Server) dispatchConditionalObjectDelete( + r *http.Request, + readTimestamp kv.ReadTimestamp, + startTS uint64, + headKey []byte, + manifest *s3ObjectManifest, + bucket, objectKey string, +) error { + if err := validateS3DeletePreconditions(r, manifest); err != nil { + return newS3ResponseError(http.StatusPreconditionFailed, "PreconditionFailed", + err.Error(), bucket, objectKey) + } + dispatchCtx := readTimestamp.WithDispatchVoucher(r.Context()) + if _, err := kv.DispatchWithReadTimestamp(dispatchCtx, s.coordinator, &kv.OperationGroup[kv.OP]{ + IsTxn: true, + StartTS: startTS, + Elems: []*kv.Elem[kv.OP]{ + {Op: kv.Del, Key: headKey}, + }, + }); err != nil { + return errors.WithStack(err) + } + return nil +} + +// validateS3DeletePreconditions enforces If-Match on DELETE. +// +// Separate from validateS3PutPreconditions because the two headers mean +// different things on a delete: If-Match is a guard against removing a +// version the caller has not seen, while If-None-Match: * ("only if +// absent") is meaningless for an operation whose whole purpose is to +// remove something that exists, so it is not honoured here. +// +// previous is the manifest the surrounding transaction already loaded at +// its read timestamp, so the comparison is against the same version the +// delete will remove -- not a separately-read one that could have moved +// in between. +func validateS3DeletePreconditions(r *http.Request, previous *s3ObjectManifest) error { + if r == nil || previous == nil { + return nil + } + ifMatch := strings.TrimSpace(r.Header.Get("If-Match")) + if ifMatch == "" { + return nil + } + if strings.Trim(ifMatch, `"`) != previous.ETag { + return errors.New("etag precondition failed") + } + return nil +} + func cloneLeaderAddrMap(src map[string]string) map[string]string { if len(src) == 0 { return nil diff --git a/adapter/s3_conditional_delete_test.go b/adapter/s3_conditional_delete_test.go new file mode 100644 index 000000000..07116d82a --- /dev/null +++ b/adapter/s3_conditional_delete_test.go @@ -0,0 +1,190 @@ +package adapter + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/bootjp/elastickv/store" + "github.com/stretchr/testify/require" +) + +// Every case here uses one bucket and one object key; only the ETag and the +// presence of the header vary. +const ( + conditionalDeleteBucket = "retention" + conditionalDeleteKey = "payload.bin" +) + +// putS3TestObject stores body at the test bucket/key through the real handler +// and returns the ETag the server assigned. +func putS3TestObject(t *testing.T, server *S3Server, body string) string { + t.Helper() + + rec := httptest.NewRecorder() + req := newS3TestRequest(http.MethodPut, + "/"+conditionalDeleteBucket+"/"+conditionalDeleteKey, strings.NewReader(body)) + req.Header.Set("Content-Type", "text/plain") + server.handle(rec, req) + require.Equal(t, http.StatusOK, rec.Code, "PUT %s/%s", conditionalDeleteBucket, conditionalDeleteKey) + + etag := strings.Trim(rec.Header().Get("ETag"), `"`) + require.NotEmpty(t, etag, "the server must assign an ETag") + return etag +} + +func newConditionalDeleteS3Server(t *testing.T) *S3Server { + t.Helper() + + st := store.NewMVCCStore() + server := NewS3Server(nil, "", st, newLocalAdapterCoordinator(st), nil) + _, err := server.AdminCreateBucket(context.Background(), + fullAdminBucketsPrincipal(), conditionalDeleteBucket, s3AclPrivate) + require.NoError(t, err) + return server +} + +func deleteS3TestObject(t *testing.T, server *S3Server, key, ifMatch string) *httptest.ResponseRecorder { + t.Helper() + + rec := httptest.NewRecorder() + req := newS3TestRequest(http.MethodDelete, "/"+conditionalDeleteBucket+"/"+key, nil) + if ifMatch != "" { + req.Header.Set("If-Match", ifMatch) + } + server.handle(rec, req) + return rec +} + +func s3TestObjectExists(t *testing.T, server *S3Server, key string) bool { + t.Helper() + + rec := httptest.NewRecorder() + server.handle(rec, newS3TestRequest(http.MethodHead, "/"+conditionalDeleteBucket+"/"+key, nil)) + return rec.Code == http.StatusOK +} + +// TestS3ConditionalDeleteRefusesAStaleETag is the load-bearing case. +// +// The handler loaded the manifest -- so it had the current ETag -- and then +// dispatched kv.Del without looking at If-Match. A caller that read an object, +// decided it was reclaimable, and sent a conditional delete believed the +// condition protected it from a concurrent rewrite. It did not: the object was +// removed whatever its ETag, so the rewrite was silently lost. Snapshot-offload +// retention relies on exactly this precondition and maps the 412 to +// ErrObjectModified. +func TestS3ConditionalDeleteRefusesAStaleETag(t *testing.T) { + t.Parallel() + + server := newConditionalDeleteS3Server(t) + staleETag := putS3TestObject(t, server, "first") + + // A concurrent rewrite changes the ETag after the caller observed it. + newETag := putS3TestObject(t, server, "rewritten") + require.NotEqual(t, staleETag, newETag, "the rewrite must change the ETag") + + rec := deleteS3TestObject(t, server, conditionalDeleteKey, staleETag) + require.Equal(t, http.StatusPreconditionFailed, rec.Code, + "a delete conditioned on a superseded ETag must fail with 412") + require.Contains(t, rec.Body.String(), "PreconditionFailed") + require.True(t, s3TestObjectExists(t, server, conditionalDeleteKey), + "the rewritten object must survive a refused conditional delete") +} + +// A matching ETag must still delete, or retention could never reclaim anything. +func TestS3ConditionalDeleteAcceptsAMatchingETag(t *testing.T) { + t.Parallel() + + server := newConditionalDeleteS3Server(t) + etag := putS3TestObject(t, server, "first") + + rec := deleteS3TestObject(t, server, conditionalDeleteKey, etag) + require.Equal(t, http.StatusNoContent, rec.Code) + require.False(t, s3TestObjectExists(t, server, conditionalDeleteKey)) +} + +// Quoted ETags are the wire form AWS uses, so both spellings must match. +func TestS3ConditionalDeleteAcceptsAQuotedETag(t *testing.T) { + t.Parallel() + + server := newConditionalDeleteS3Server(t) + etag := putS3TestObject(t, server, "first") + + rec := deleteS3TestObject(t, server, conditionalDeleteKey, `"`+etag+`"`) + require.Equal(t, http.StatusNoContent, rec.Code) +} + +// An unconditional delete must be unaffected: adding the header check must not +// change the behaviour of every existing caller that does not send one. +func TestS3UnconditionalDeleteIsUnchanged(t *testing.T) { + t.Parallel() + + server := newConditionalDeleteS3Server(t) + putS3TestObject(t, server, "first") + + rec := deleteS3TestObject(t, server, conditionalDeleteKey, "") + require.Equal(t, http.StatusNoContent, rec.Code) + require.False(t, s3TestObjectExists(t, server, conditionalDeleteKey)) +} + +// Delete stays idempotent for an absent key, with or without the header: S3 +// delete is not an error on a missing object, and a precondition cannot be +// evaluated against something that does not exist. +func TestS3ConditionalDeleteOnAnAbsentKeyStaysIdempotent(t *testing.T) { + t.Parallel() + + server := newConditionalDeleteS3Server(t) + + rec := deleteS3TestObject(t, server, "never-existed", "") + require.Equal(t, http.StatusNoContent, rec.Code) + + rec = deleteS3TestObject(t, server, "never-existed", "deadbeef") + require.Equal(t, http.StatusNoContent, rec.Code) +} + +// TestValidateS3DeletePreconditions covers the predicate directly, including +// that If-None-Match is deliberately NOT honoured on a delete. +func TestValidateS3DeletePreconditions(t *testing.T) { + t.Parallel() + + previous := &s3ObjectManifest{ETag: "abc123"} + + t.Run("no header passes", func(t *testing.T) { + t.Parallel() + req := newS3TestRequest(http.MethodDelete, "/b/k", nil) + require.NoError(t, validateS3DeletePreconditions(req, previous)) + }) + + t.Run("matching etag passes", func(t *testing.T) { + t.Parallel() + req := newS3TestRequest(http.MethodDelete, "/b/k", nil) + req.Header.Set("If-Match", "abc123") + require.NoError(t, validateS3DeletePreconditions(req, previous)) + }) + + t.Run("stale etag fails", func(t *testing.T) { + t.Parallel() + req := newS3TestRequest(http.MethodDelete, "/b/k", nil) + req.Header.Set("If-Match", "stale") + require.Error(t, validateS3DeletePreconditions(req, previous)) + }) + + t.Run("If-None-Match is not honoured on delete", func(t *testing.T) { + t.Parallel() + // "only if absent" is meaningless for an operation whose purpose is + // to remove something that exists; honouring it would refuse every + // delete of a present object. + req := newS3TestRequest(http.MethodDelete, "/b/k", nil) + req.Header.Set("If-None-Match", "*") + require.NoError(t, validateS3DeletePreconditions(req, previous)) + }) + + t.Run("nil manifest passes", func(t *testing.T) { + t.Parallel() + req := newS3TestRequest(http.MethodDelete, "/b/k", nil) + req.Header.Set("If-Match", "abc123") + require.NoError(t, validateS3DeletePreconditions(req, nil)) + }) +}