mirror of
https://github.com/seaweedfs/seaweedfs.git
synced 2026-08-31 21:31:24 +00:00
s3: keep a missing object a 404 under If-Match and If-Unmodified-Since (#10985)
* s3: keep a missing object a 404 under If-Match and If-Unmodified-Since GET and HEAD resolved the target before evaluating the conditional headers, and a missing target failed If-Match and If-Unmodified-Since outright, so absence surfaced as 412 PreconditionFailed. AWS reports the missing object instead: 404 for HeadObject, NoSuchKey for GetObject, and 412 only when a live object fails the condition. Clients cannot tell absence from a stale precondition without an extra racy HEAD, so OpenDAL disabled its four conditional stat/read capabilities against SeaweedFS. A precondition now only fails against an object that exists; a missing one -- including a latest version that is a delete marker -- returns NoSuchKey. Claude-Session: https://claude.ai/code/session_01X4kEbuwxd9DFsTnSXjfjgv * s3: evaluate a conditional read against the version the request names GET and HEAD resolved the latest version before evaluating the conditional headers, so a request carrying versionId had its If-Match compared against a different version than the one it was asking for: a live version whose ETag the client held failed once a newer version -- or a delete marker -- became the latest. resolveObjectEntry now resolves the named version on a versioned bucket, the way DELETE already does. A named version that resolves to nothing is left to the handler, which alone knows whether the bucket is versioned and so whether it owes NoSuchVersion. Claude-Session: https://claude.ai/code/session_01X4kEbuwxd9DFsTnSXjfjgv
This commit is contained in:
Chris Lu
@@ -0,0 +1,130 @@
|
||||
package s3api
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/aws/aws-sdk-go-v2/aws"
|
||||
"github.com/aws/aws-sdk-go-v2/service/s3"
|
||||
"github.com/aws/smithy-go"
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
|
||||
func requireAPIErrorCode(t *testing.T, err error, expected string) {
|
||||
t.Helper()
|
||||
require.Error(t, err)
|
||||
var apiErr smithy.APIError
|
||||
require.True(t, errors.As(err, &apiErr), "expected a smithy.APIError, got %T: %v", err, err)
|
||||
assert.Equal(t, expected, apiErr.ErrorCode())
|
||||
}
|
||||
|
||||
// TestConditionalReadsOfMissingObject verifies that a missing key stays a missing key
|
||||
// under If-Match and If-Unmodified-Since instead of surfacing as 412.
|
||||
// reproduces issue #10984
|
||||
func TestConditionalReadsOfMissingObject(t *testing.T) {
|
||||
client := getS3Client(t)
|
||||
bucketName := getNewBucketName()
|
||||
|
||||
createBucket(t, client, bucketName)
|
||||
defer deleteBucket(t, client, bucketName)
|
||||
|
||||
existing := putObject(t, client, bucketName, "etag-source", "content")
|
||||
require.NotNil(t, existing.ETag)
|
||||
future := aws.Time(time.Now().Add(24 * time.Hour))
|
||||
missing := aws.String("conditional-missing")
|
||||
|
||||
t.Run("HeadObject If-Match", func(t *testing.T) {
|
||||
_, err := client.HeadObject(context.TODO(), &s3.HeadObjectInput{
|
||||
Bucket: aws.String(bucketName), Key: missing, IfMatch: existing.ETag,
|
||||
})
|
||||
requireAPIErrorCode(t, err, "NotFound")
|
||||
})
|
||||
|
||||
t.Run("HeadObject If-Unmodified-Since", func(t *testing.T) {
|
||||
_, err := client.HeadObject(context.TODO(), &s3.HeadObjectInput{
|
||||
Bucket: aws.String(bucketName), Key: missing, IfUnmodifiedSince: future,
|
||||
})
|
||||
requireAPIErrorCode(t, err, "NotFound")
|
||||
})
|
||||
|
||||
t.Run("GetObject If-Match", func(t *testing.T) {
|
||||
_, err := client.GetObject(context.TODO(), &s3.GetObjectInput{
|
||||
Bucket: aws.String(bucketName), Key: missing, IfMatch: existing.ETag,
|
||||
})
|
||||
requireAPIErrorCode(t, err, "NoSuchKey")
|
||||
})
|
||||
|
||||
t.Run("GetObject If-Unmodified-Since", func(t *testing.T) {
|
||||
_, err := client.GetObject(context.TODO(), &s3.GetObjectInput{
|
||||
Bucket: aws.String(bucketName), Key: missing, IfUnmodifiedSince: future,
|
||||
})
|
||||
requireAPIErrorCode(t, err, "NoSuchKey")
|
||||
})
|
||||
|
||||
t.Run("GetObject stale If-Match on a live object stays 412", func(t *testing.T) {
|
||||
_, err := client.GetObject(context.TODO(), &s3.GetObjectInput{
|
||||
Bucket: aws.String(bucketName), Key: aws.String("etag-source"),
|
||||
IfMatch: aws.String(`"0000000000000000000000000000dead"`),
|
||||
})
|
||||
requireAPIErrorCode(t, err, "PreconditionFailed")
|
||||
})
|
||||
}
|
||||
|
||||
// TestConditionalReadsOfNamedVersion verifies that a conditional GET or HEAD of an
|
||||
// explicit versionId is evaluated against that version rather than the latest one,
|
||||
// including when the latest version is a delete marker.
|
||||
func TestConditionalReadsOfNamedVersion(t *testing.T) {
|
||||
client := getS3Client(t)
|
||||
bucketName := getNewBucketName()
|
||||
|
||||
createBucket(t, client, bucketName)
|
||||
defer deleteBucket(t, client, bucketName)
|
||||
enableVersioning(t, client, bucketName)
|
||||
|
||||
key := "conditional-read-version"
|
||||
v1 := putObject(t, client, bucketName, key, "content-v1")
|
||||
require.NotNil(t, v1.ETag)
|
||||
require.NotNil(t, v1.VersionId)
|
||||
v2 := putObject(t, client, bucketName, key, "content-v2")
|
||||
require.NotNil(t, v2.ETag)
|
||||
require.NotEqual(t, *v1.ETag, *v2.ETag)
|
||||
|
||||
t.Run("If-Match matches the named version, not the latest", func(t *testing.T) {
|
||||
_, err := client.GetObject(context.TODO(), &s3.GetObjectInput{
|
||||
Bucket: aws.String(bucketName), Key: aws.String(key),
|
||||
VersionId: v1.VersionId, IfMatch: v1.ETag,
|
||||
})
|
||||
require.NoError(t, err)
|
||||
})
|
||||
|
||||
t.Run("If-Match against the latest ETag fails on the named version", func(t *testing.T) {
|
||||
_, err := client.GetObject(context.TODO(), &s3.GetObjectInput{
|
||||
Bucket: aws.String(bucketName), Key: aws.String(key),
|
||||
VersionId: v1.VersionId, IfMatch: v2.ETag,
|
||||
})
|
||||
requireAPIErrorCode(t, err, "PreconditionFailed")
|
||||
})
|
||||
|
||||
_, err := client.DeleteObject(context.TODO(), &s3.DeleteObjectInput{
|
||||
Bucket: aws.String(bucketName), Key: aws.String(key),
|
||||
})
|
||||
require.NoError(t, err)
|
||||
|
||||
t.Run("named version survives a delete marker on the latest", func(t *testing.T) {
|
||||
_, err := client.HeadObject(context.TODO(), &s3.HeadObjectInput{
|
||||
Bucket: aws.String(bucketName), Key: aws.String(key),
|
||||
VersionId: v1.VersionId, IfMatch: v1.ETag,
|
||||
})
|
||||
require.NoError(t, err)
|
||||
})
|
||||
|
||||
t.Run("delete marker latest is a missing object", func(t *testing.T) {
|
||||
_, err := client.GetObject(context.TODO(), &s3.GetObjectInput{
|
||||
Bucket: aws.String(bucketName), Key: aws.String(key), IfMatch: v1.ETag,
|
||||
})
|
||||
requireAPIErrorCode(t, err, "NoSuchKey")
|
||||
})
|
||||
}
|
||||
@@ -376,7 +376,7 @@ func applyMultipartSSES3HeadersFromUploadEntry(dst *filer_pb.Entry, sses3Info *m
|
||||
}
|
||||
|
||||
func (s3a *S3ApiServer) prepareMultipartCompletionState(r *http.Request, input *s3.CompleteMultipartUploadInput, uploadDirectory, entryName, dirName string, completedPartNumbers []int, completedPartMap map[int][]string, maxPartNo int) (*multipartCompletionState, *CompleteMultipartUploadResult, s3err.ErrorCode) {
|
||||
if entry, err := s3a.resolveObjectEntry(*input.Bucket, *input.Key); err == nil && entry != nil && entry.Extended != nil {
|
||||
if entry, err := s3a.resolveObjectEntry(*input.Bucket, *input.Key, ""); err == nil && entry != nil && entry.Extended != nil {
|
||||
if uploadId, ok := entry.Extended[s3_constants.SeaweedFSUploadId]; ok && *input.UploadId == string(uploadId) {
|
||||
cleanupEntries, _, cleanupErr := s3a.list(uploadDirectory, "", "", false, s3_constants.MaxS3MultipartParts+1)
|
||||
if cleanupErr != nil && !errors.Is(cleanupErr, filer_pb.ErrNotFound) {
|
||||
|
||||
@@ -423,9 +423,10 @@ func (s3a *S3ApiServer) checkDirectoryObject(bucket, object string) (*filer_pb.E
|
||||
return dirEntry, true, nil
|
||||
}
|
||||
|
||||
// resolveObjectEntry resolves the object entry for conditional checks,
|
||||
// handling versioned buckets by resolving the latest version.
|
||||
func (s3a *S3ApiServer) resolveObjectEntry(bucket, object string) (*filer_pb.Entry, error) {
|
||||
// resolveObjectEntry resolves the object entry for conditional checks: the version the
|
||||
// request names when the bucket is versioned, otherwise the latest version. Callers
|
||||
// with no version to target pass an empty versionId.
|
||||
func (s3a *S3ApiServer) resolveObjectEntry(bucket, object, versionId string) (*filer_pb.Entry, error) {
|
||||
// Check if versioning is configured
|
||||
versioningConfigured, err := s3a.isVersioningConfigured(bucket)
|
||||
if err != nil && !errors.Is(err, filer_pb.ErrNotFound) {
|
||||
@@ -434,6 +435,9 @@ func (s3a *S3ApiServer) resolveObjectEntry(bucket, object string) (*filer_pb.Ent
|
||||
}
|
||||
|
||||
if versioningConfigured {
|
||||
if versionId != "" {
|
||||
return s3a.getSpecificObjectVersion(bucket, object, versionId)
|
||||
}
|
||||
// For versioned buckets, we must use getLatestObjectVersion to correctly
|
||||
// find the latest versioned object (in .versions/) or null version.
|
||||
// Standard getEntry would fail to find objects moved to .versions/.
|
||||
|
||||
@@ -0,0 +1,60 @@
|
||||
package s3api
|
||||
|
||||
import (
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/seaweedfs/seaweedfs/weed/pb/filer_pb"
|
||||
"github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants"
|
||||
"github.com/seaweedfs/seaweedfs/weed/s3api/s3err"
|
||||
)
|
||||
|
||||
// A precondition can only fail against an object that exists. GET/HEAD of a missing
|
||||
// key must stay a missing-key answer even when If-Match or If-Unmodified-Since is sent.
|
||||
func TestValidateConditionalHeadersForReadsMissingObject(t *testing.T) {
|
||||
s3a := &S3ApiServer{}
|
||||
|
||||
existing := &filer_pb.Entry{
|
||||
Attributes: &filer_pb.FuseAttributes{Mtime: time.Now().Unix()},
|
||||
Extended: map[string][]byte{s3_constants.ExtETagKey: []byte("d41d8cd98f00b204e9800998ecf8427e")},
|
||||
}
|
||||
deleteMarker := &filer_pb.Entry{
|
||||
Attributes: &filer_pb.FuseAttributes{Mtime: time.Now().Unix()},
|
||||
Extended: map[string][]byte{s3_constants.ExtDeleteMarkerKey: []byte("true")},
|
||||
}
|
||||
future := time.Now().Add(24 * time.Hour).UTC().Format(http.TimeFormat)
|
||||
|
||||
testCases := []struct {
|
||||
name string
|
||||
header string
|
||||
value string
|
||||
entry *filer_pb.Entry
|
||||
want s3err.ErrorCode
|
||||
}{
|
||||
{"if-match on missing object", s3_constants.IfMatch, "0000", nil, s3err.ErrNoSuchKey},
|
||||
{"if-match star on missing object", s3_constants.IfMatch, "*", nil, s3err.ErrNoSuchKey},
|
||||
{"if-unmodified-since on missing object", s3_constants.IfUnmodifiedSince, future, nil, s3err.ErrNoSuchKey},
|
||||
{"if-match on delete marker", s3_constants.IfMatch, "0000", deleteMarker, s3err.ErrNoSuchKey},
|
||||
{"if-none-match on missing object", s3_constants.IfNoneMatch, "*", nil, s3err.ErrNone},
|
||||
{"if-modified-since on missing object", s3_constants.IfModifiedSince, future, nil, s3err.ErrNone},
|
||||
{"if-match mismatch on existing object", s3_constants.IfMatch, "0000", existing, s3err.ErrPreconditionFailed},
|
||||
{"if-match hit on existing object", s3_constants.IfMatch, "d41d8cd98f00b204e9800998ecf8427e", existing, s3err.ErrNone},
|
||||
}
|
||||
|
||||
for _, tc := range testCases {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
r := httptest.NewRequest(http.MethodGet, "/bucket/object", nil)
|
||||
r.Header.Set(tc.header, tc.value)
|
||||
headers, errCode := parseConditionalHeaders(r)
|
||||
if errCode != s3err.ErrNone {
|
||||
t.Fatalf("parseConditionalHeaders: %v", errCode)
|
||||
}
|
||||
result := s3a.validateConditionalHeadersForReads(r, headers, tc.entry, "bucket", "object")
|
||||
if result.ErrorCode != tc.want {
|
||||
t.Errorf("got %v, want %v", result.ErrorCode, tc.want)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
@@ -78,7 +78,7 @@ func (s3a *S3ApiServer) resolveDeleteConditionalEntry(bucket, object, versionId,
|
||||
}
|
||||
return normalizeConditionalTargetEntry(entry), nil
|
||||
default:
|
||||
entry, err := s3a.resolveObjectEntry(bucket, normalizedObject)
|
||||
entry, err := s3a.resolveObjectEntry(bucket, normalizedObject, "")
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
@@ -2277,7 +2277,7 @@ func (s3a *S3ApiServer) checkConditionalHeaders(r *http.Request, bucket, object
|
||||
|
||||
// Use resolveObjectEntry to correctly handle versioned objects.
|
||||
// This ensures we check conditions against the LATEST version, not a null version.
|
||||
entry, err := s3a.resolveObjectEntry(bucket, object)
|
||||
entry, err := s3a.resolveObjectEntry(bucket, object, "")
|
||||
if err != nil {
|
||||
if errors.Is(err, filer_pb.ErrNotFound) || errors.Is(err, ErrDeleteMarker) {
|
||||
entry = nil
|
||||
@@ -2298,18 +2298,14 @@ func (s3a *S3ApiServer) validateConditionalHeadersForReads(r *http.Request, head
|
||||
entry = normalizeConditionalTargetEntry(entry)
|
||||
objectExists := entry != nil
|
||||
|
||||
// If object doesn't exist, fail for If-Match and If-Unmodified-Since
|
||||
// A precondition only fails against an object that exists: AWS keeps GET/HEAD of a
|
||||
// missing key a missing-key answer, so a condition never turns absence into 412.
|
||||
// If-None-Match and If-Modified-Since pass here and the handler answers 404 itself.
|
||||
if !objectExists {
|
||||
if headers.ifMatch != "" {
|
||||
glog.V(3).Infof("validateConditionalHeadersForReads: If-Match failed - object %s/%s does not exist", bucket, object)
|
||||
return ConditionalHeaderResult{ErrorCode: s3err.ErrPreconditionFailed, Entry: nil}
|
||||
if headers.ifMatch != "" || !headers.ifUnmodifiedSince.IsZero() {
|
||||
glog.V(3).Infof("validateConditionalHeadersForReads: object %s/%s does not exist", bucket, object)
|
||||
return ConditionalHeaderResult{ErrorCode: s3err.ErrNoSuchKey, Entry: nil}
|
||||
}
|
||||
if !headers.ifUnmodifiedSince.IsZero() {
|
||||
glog.V(3).Infof("validateConditionalHeadersForReads: If-Unmodified-Since failed - object %s/%s does not exist", bucket, object)
|
||||
return ConditionalHeaderResult{ErrorCode: s3err.ErrPreconditionFailed, Entry: nil}
|
||||
}
|
||||
// If-None-Match and If-Modified-Since succeed when object doesn't exist
|
||||
// No entry to return since object doesn't exist
|
||||
return ConditionalHeaderResult{ErrorCode: s3err.ErrNone, Entry: nil}
|
||||
}
|
||||
|
||||
@@ -2396,9 +2392,10 @@ func (s3a *S3ApiServer) checkConditionalHeadersForReads(r *http.Request, bucket,
|
||||
return ConditionalHeaderResult{ErrorCode: s3err.ErrNone, Entry: nil}
|
||||
}
|
||||
|
||||
// Use resolveObjectEntry to correctly handle versioned objects.
|
||||
// This ensures we check conditions against the LATEST version, not a null version.
|
||||
entry, err := s3a.resolveObjectEntry(bucket, object)
|
||||
// Use resolveObjectEntry to correctly handle versioned objects: the version the
|
||||
// request names, or the LATEST version rather than a null version.
|
||||
versionId := r.URL.Query().Get("versionId")
|
||||
entry, err := s3a.resolveObjectEntry(bucket, object, versionId)
|
||||
if err != nil {
|
||||
if errors.Is(err, filer_pb.ErrNotFound) || errors.Is(err, ErrDeleteMarker) {
|
||||
entry = nil
|
||||
@@ -2407,6 +2404,11 @@ func (s3a *S3ApiServer) checkConditionalHeadersForReads(r *http.Request, bucket,
|
||||
return ConditionalHeaderResult{ErrorCode: s3err.ErrInternalError, Entry: nil}
|
||||
}
|
||||
}
|
||||
// A named version that resolves to nothing is the handler's answer to give: only it
|
||||
// knows whether the bucket is versioned, and so whether that is NoSuchVersion.
|
||||
if versionId != "" && normalizeConditionalTargetEntry(entry) == nil {
|
||||
return ConditionalHeaderResult{ErrorCode: s3err.ErrNone, Entry: nil}
|
||||
}
|
||||
return s3a.validateConditionalHeadersForReads(r, headers, entry, bucket, object)
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user