diff --git a/test/s3/versioning/s3_directory_marker_delete_test.go b/test/s3/versioning/s3_directory_marker_delete_test.go index 0b93976ca..c5b9dd530 100644 --- a/test/s3/versioning/s3_directory_marker_delete_test.go +++ b/test/s3/versioning/s3_directory_marker_delete_test.go @@ -2,6 +2,7 @@ package s3api import ( "context" + "io" "testing" "github.com/aws/aws-sdk-go-v2/aws" @@ -12,8 +13,8 @@ import ( ) // TestDeletedDirectoryMarkerDisappears covers the rclone directory_markers flow: a key -// created with PutObject on "m2/" is deleted, and every current-version surface has to -// agree it is gone even though the filer directory that carries it survives. +// created with PutObject on "m2/" is deleted, and stops being a key everywhere. The +// directory that carried it goes with it once nothing is left underneath. func TestDeletedDirectoryMarkerDisappears(t *testing.T) { client := getS3Client(t) bucketName := getNewBucketName() @@ -24,7 +25,6 @@ func TestDeletedDirectoryMarkerDisappears(t *testing.T) { putObject(t, client, bucketName, "m2/", "") putObject(t, client, bucketName, "m2/f.txt", "hi") - assert.Equal(t, []string{"m2/", "m2/f.txt"}, listKeys(t, client, bucketName, "")) deleteKey(t, client, bucketName, "m2/f.txt") @@ -32,39 +32,22 @@ func TestDeletedDirectoryMarkerDisappears(t *testing.T) { assert.Empty(t, listKeys(t, client, bucketName, ""), "the deleted marker is not a key") assert.Empty(t, listPrefixes(t, client, bucketName, ""), "and it names no prefix") - assert.Empty(t, listKeys(t, client, bucketName, "m2/"), "prefix=m2/ answers empty") - _, err := client.HeadObject(context.TODO(), &s3.HeadObjectInput{ - Bucket: aws.String(bucketName), - Key: aws.String("m2/"), - }) - require.Error(t, err, "HEAD on a deleted directory marker must not answer 200") - _, err = client.GetObject(context.TODO(), &s3.GetObjectInput{ - Bucket: aws.String(bucketName), - Key: aws.String("m2/"), - }) - requireAPIError(t, err, "NoSuchKey") - - // The key appears once in the version listing, as its delete marker. + // The file's own version history is untouched by deleting the directory key. versions, err := client.ListObjectVersions(context.TODO(), &s3.ListObjectVersionsInput{ Bucket: aws.String(bucketName), - Prefix: aws.String("m2/"), }) require.NoError(t, err) - latest := 0 for _, v := range versions.Versions { - if *v.Key == "m2/" && *v.IsLatest { - latest++ - } + assert.NotEqual(t, "m2/", *v.Key, "a directory marker is not a versioned object") } for _, m := range versions.DeleteMarkers { - if *m.Key == "m2/" && *m.IsLatest { - latest++ - } + assert.NotEqual(t, "m2/", *m.Key, "deleting one writes no delete marker") } - assert.Equal(t, 1, latest, "exactly one version of m2/ is the latest") + assert.Len(t, versions.Versions, 1, "m2/f.txt keeps its version") + assert.Len(t, versions.DeleteMarkers, 1, "and its delete marker") - // Re-creating the marker retires the delete marker and keeps the history. + // Re-creating the marker brings the key back. putObject(t, client, bucketName, "m2/", "") assert.Equal(t, []string{"m2/"}, listKeys(t, client, bucketName, "")) _, err = client.HeadObject(context.TODO(), &s3.HeadObjectInput{ @@ -72,18 +55,6 @@ func TestDeletedDirectoryMarkerDisappears(t *testing.T) { Key: aws.String("m2/"), }) require.NoError(t, err) - - versions, err = client.ListObjectVersions(context.TODO(), &s3.ListObjectVersionsInput{ - Bucket: aws.String(bucketName), - Prefix: aws.String("m2/"), - }) - require.NoError(t, err) - assert.NotEmpty(t, versions.DeleteMarkers, "the delete marker stays in the history") - for _, m := range versions.DeleteMarkers { - if *m.Key == "m2/" { - assert.False(t, *m.IsLatest, "the delete marker is no longer current") - } - } } // TestDeletedDirectoryMarkerKeepsItsSubtree pins the boundary: deleting the key "m2/" @@ -101,29 +72,102 @@ func TestDeletedDirectoryMarkerKeepsItsSubtree(t *testing.T) { deleteKey(t, client, bucketName, "m2/") - assert.Equal(t, []string{"m2/keep.txt"}, listKeys(t, client, bucketName, "")) + assert.Equal(t, []string{"m2/keep.txt"}, listKeys(t, client, bucketName, ""), "the marker key is gone") assert.Equal(t, []string{"m2/"}, listPrefixes(t, client, bucketName, ""), - "a live child keeps the prefix even though the marker key is gone") + "a live child keeps the prefix") assert.Equal(t, []string{"m2/keep.txt"}, listKeys(t, client, bucketName, "m2/")) + + // The surviving object is still readable through the prefix that no longer has a key. + got, err := client.GetObject(context.TODO(), &s3.GetObjectInput{ + Bucket: aws.String(bucketName), + Key: aws.String("m2/keep.txt"), + }) + require.NoError(t, err) + require.NoError(t, got.Body.Close()) } -// TestDirectoryMarkerSurvivesWithoutVersioning guards the unversioned path, which has no -// history to consult and must keep answering as before. -func TestDirectoryMarkerSurvivesWithoutVersioning(t *testing.T) { +// TestDeletedDirectoryMarkerIsGoneFromReads checks the read surfaces once nothing is +// left under the deleted key: the directory goes with it, so the path is not there. +func TestDeletedDirectoryMarkerIsGoneFromReads(t *testing.T) { client := getS3Client(t) bucketName := getNewBucketName() createBucket(t, client, bucketName) defer deleteBucket(t, client, bucketName) + enableVersioning(t, client, bucketName) putObject(t, client, bucketName, "m2/", "") - assert.Equal(t, []string{"m2/"}, listKeys(t, client, bucketName, "")) + deleteKey(t, client, bucketName, "m2/") _, err := client.HeadObject(context.TODO(), &s3.HeadObjectInput{ Bucket: aws.String(bucketName), Key: aws.String("m2/"), }) - require.NoError(t, err) + require.Error(t, err, "HEAD on a deleted directory marker must not answer 200") + _, err = client.GetObject(context.TODO(), &s3.GetObjectInput{ + Bucket: aws.String(bucketName), + Key: aws.String("m2/"), + }) + requireAPIError(t, err, "NoSuchKey") + assert.Empty(t, listKeys(t, client, bucketName, "m2/")) +} + +// TestDirectoryMarkerDeleteMatchesUnversioned pins the point of the change: a directory +// marker is deleted the same way whether or not the bucket is versioned. +func TestDirectoryMarkerDeleteMatchesUnversioned(t *testing.T) { + client := getS3Client(t) + + for _, versioned := range []bool{false, true} { + bucketName := getNewBucketName() + createBucket(t, client, bucketName) + if versioned { + enableVersioning(t, client, bucketName) + } + + putObject(t, client, bucketName, "m/", "") + putObject(t, client, bucketName, "m/child.txt", "x") + deleteKey(t, client, bucketName, "m/") + + assert.Equal(t, []string{"m/child.txt"}, listKeys(t, client, bucketName, ""), + "versioned=%v: the marker key is gone, the child stays", versioned) + assert.Equal(t, []string{"m/"}, listPrefixes(t, client, bucketName, ""), + "versioned=%v: the prefix survives its live child", versioned) + + deleteBucket(t, client, bucketName) + } +} + +// TestDeleteDirectoryMarkerSparesAPromotedFile guards the sharp edge of storing a key +// on a directory entry: writing under an existing object turns that object's entry into +// a directory while it keeps its data, so "m2/" and "m2" end up on the same entry. They +// are still different keys, and deleting one must not destroy the other. +func TestDeleteDirectoryMarkerSparesAPromotedFile(t *testing.T) { + client := getS3Client(t) + + for _, versioned := range []bool{false, true} { + bucketName := getNewBucketName() + createBucket(t, client, bucketName) + + putObject(t, client, bucketName, "m2", "important data") + if versioned { + enableVersioning(t, client, bucketName) + } + putObject(t, client, bucketName, "m2/child.txt", "child") + + deleteKey(t, client, bucketName, "m2/") + + got, err := client.GetObject(context.TODO(), &s3.GetObjectInput{ + Bucket: aws.String(bucketName), + Key: aws.String("m2"), + }) + require.NoError(t, err, "versioned=%v: deleting m2/ must not delete m2", versioned) + body, err := io.ReadAll(got.Body) + require.NoError(t, err) + require.NoError(t, got.Body.Close()) + assert.Equal(t, "important data", string(body), "versioned=%v", versioned) + + deleteBucket(t, client, bucketName) + } } func listObjects(t *testing.T, client *s3.Client, bucketName, prefix, delimiter string) *s3.ListObjectsV2Output { diff --git a/weed/s3api/s3api_directory_marker.go b/weed/s3api/s3api_directory_marker.go new file mode 100644 index 000000000..b70955b76 --- /dev/null +++ b/weed/s3api/s3api_directory_marker.go @@ -0,0 +1,69 @@ +package s3api + +import ( + "errors" + "strings" + + "github.com/seaweedfs/seaweedfs/weed/glog" + "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/seaweedfs/seaweedfs/weed/util" +) + +// An explicit directory marker - the key "dir/" created by PutObject on a +// trailing-slash key - is stored as the filer directory itself rather than as an +// object beside it. That makes it a poor fit for versioning: a delete marker would +// have to replace an entry that other keys live under, and the history would have to +// sit inside the directory it describes, where a listing keeps meeting it. +// +// So the key is not versioned. Deleting it does what deleting it in an unversioned +// bucket already does: the directory is removed when nothing is left under it, and +// demoted to a plain directory when children remain. Listings need no version lookup +// to tell what a directory stands for, and a bucket made of directory markers costs +// the same to list versioned as unversioned. + +// deleteDirectoryMarker removes the key "/". Callers hold the object write lock, +// so the entry this decides about cannot change between the read and the delete. +func (s3a *S3ApiServer) deleteDirectoryMarker(bucket, object string) s3err.ErrorCode { + markerDir := s3a.bucketDir(bucket) + "/" + strings.TrimSuffix(strings.TrimPrefix(object, "/"), "/") + dir, name := util.FullPath(markerDir).DirAndName() + + entry, err := s3a.getEntry(dir, name) + switch { + case errors.Is(err, filer_pb.ErrNotFound): + return s3err.ErrNone // deleting a key that is not there is a success + case err != nil: + // The entry may be a file a child write promoted to a directory, whose data + // belongs to the key without the trailing slash. Deleting without knowing + // would destroy it, so fail and leave the retry to the client. + glog.Errorf("deleteDirectoryMarker: cannot read %s/%s: %v", bucket, object, err) + return s3err.ErrInternalError + case len(entry.GetChunks()) > 0 || entry.IsInRemoteOnly(): + // A promoted file, not a marker: "dir/" does not name its data. + glog.V(2).Infof("deleteDirectoryMarker: %s/%s holds uploaded data, leaving it alone", bucket, object) + return s3err.ErrNone + } + + // Drop a history an older build recorded for this key. Nothing writes one now, and + // leaving it behind keeps reporting the key in ListObjectVersions, so a history we + // cannot read or remove fails the delete rather than half finishing it. + switch _, historyErr := s3a.getEntry(markerDir, s3_constants.VersionsFolder); { + case historyErr == nil: + if rmErr := s3a.rm(markerDir, s3_constants.VersionsFolder, true, true); rmErr != nil { + glog.Errorf("deleteDirectoryMarker: failed to remove stale history of %s/%s: %v", bucket, object, rmErr) + return s3err.ErrInternalError + } + case !errors.Is(historyErr, filer_pb.ErrNotFound): + glog.Errorf("deleteDirectoryMarker: cannot read stale history of %s/%s: %v", bucket, object, historyErr) + return s3err.ErrInternalError + } + + if err := s3a.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error { + return s3a.deleteUnversionedObjectWithClient(client, bucket, object, false) + }); err != nil { + glog.Errorf("deleteDirectoryMarker: failed to delete %s/%s: %v", bucket, object, err) + return s3err.ErrInternalError + } + return s3err.ErrNone +} diff --git a/weed/s3api/s3api_object_handlers.go b/weed/s3api/s3api_object_handlers.go index 1795ba205..dee0fb0cb 100644 --- a/weed/s3api/s3api_object_handlers.go +++ b/weed/s3api/s3api_object_handlers.go @@ -463,85 +463,9 @@ func (s3a *S3ApiServer) serveDirectoryContent(w http.ResponseWriter, r *http.Req } } -// handleVersionedDirectoryObjectRequest answers GET/HEAD for the key "/" out of -// its version history, which for a directory marker lives inside the directory the key -// names. Without a history the key is whatever the directory entry says, which is the -// unversioned path the caller falls through to. -func (s3a *S3ApiServer) handleVersionedDirectoryObjectRequest(w http.ResponseWriter, r *http.Request, bucket, object, handlerName string) bool { - if !strings.HasSuffix(object, "/") { - return false - } - // A lookup that fails for any reason other than "not there" leaves the key's state - // unknown, and falling through would serve a directory whose current version may be - // a delete marker. Only a confirmed absence takes the unversioned path. - versioningConfigured, err := s3a.isVersioningConfigured(bucket) - if err != nil && !errors.Is(err, filer_pb.ErrNotFound) { - glog.Errorf("%s: versioning state of %s unknown: %v", handlerName, bucket, err) - s3err.WriteErrorResponse(w, r, s3err.ErrInternalError) - return true - } - if !versioningConfigured { - return false - } - - if versionId := r.URL.Query().Get("versionId"); versionId != "" { - entry, err := s3a.getSpecificObjectVersion(bucket, object, versionId) - if err != nil { - glog.V(2).Infof("%s: no version %s of directory object %s/%s: %v", handlerName, versionId, bucket, object, err) - s3err.WriteErrorResponse(w, r, s3err.ErrNoSuchVersion) - return true - } - w.Header().Set("x-amz-version-id", versionId) - if isDeleteMarkerEntry(entry) { - w.Header().Set(s3_constants.AmzDeleteMarker, "true") - s3err.WriteErrorResponse(w, r, s3err.ErrMethodNotAllowed) - return true - } - s3a.serveDirectoryContent(w, r, entry) - return true - } - - normalizedObject := s3_constants.NormalizeObjectKey(object) - if _, historyErr := s3a.getEntry(s3a.bucketDir(bucket), normalizedObject+s3_constants.VersionsFolder); historyErr != nil { - if !errors.Is(historyErr, filer_pb.ErrNotFound) { - glog.Errorf("%s: version history of %s/%s unknown: %v", handlerName, bucket, object, historyErr) - s3err.WriteErrorResponse(w, r, s3err.ErrInternalError) - return true - } - return false - } - // A failure here is reported the way the regular-object GET path reports it. - entry, err := s3a.getLatestObjectVersion(bucket, object) - if err != nil { - glog.V(2).Infof("%s: no current version of directory object %s/%s: %v", handlerName, bucket, object, err) - s3err.WriteErrorResponse(w, r, s3err.ErrNoSuchKey) - return true - } - versionId := string(entry.Extended[s3_constants.ExtVersionIdKey]) - if versionId == "" { - versionId = "null" - } - w.Header().Set("x-amz-version-id", versionId) - if isDeleteMarkerEntry(entry) { - w.Header().Set(s3_constants.AmzDeleteMarker, "true") - s3err.WriteErrorResponse(w, r, s3err.ErrNoSuchKey) - return true - } - s3a.serveDirectoryContent(w, r, entry) - return true -} - -func isDeleteMarkerEntry(entry *filer_pb.Entry) bool { - return entry != nil && string(entry.Extended[s3_constants.ExtDeleteMarkerKey]) == "true" -} - // handleDirectoryObjectRequest is a helper function that handles directory object requests // for both GET and HEAD operations, eliminating code duplication func (s3a *S3ApiServer) handleDirectoryObjectRequest(w http.ResponseWriter, r *http.Request, bucket, object, handlerName string) bool { - if s3a.handleVersionedDirectoryObjectRequest(w, r, bucket, object, handlerName) { - return true - } - // Check if this is a directory object and handle it directly if dirEntry, isDirectoryObject, err := s3a.checkDirectoryObject(bucket, object); err != nil { glog.Errorf("%s: error checking directory object %s/%s: %v", handlerName, bucket, object, err) diff --git a/weed/s3api/s3api_object_handlers_delete.go b/weed/s3api/s3api_object_handlers_delete.go index 0363ea015..826f37199 100644 --- a/weed/s3api/s3api_object_handlers_delete.go +++ b/weed/s3api/s3api_object_handlers_delete.go @@ -122,6 +122,13 @@ func (s3a *S3ApiServer) checkDeleteIfMatch(bucket, object, versionId, versioning func (s3a *S3ApiServer) deleteVersionedObject(r *http.Request, bucket, object, versionId, versioningState string) (deleteMutationResult, s3err.ErrorCode) { var result deleteMutationResult + // The key "dir/" is the filer directory itself, which a delete marker cannot stand + // in for without hiding the children underneath it. It is not a versioned object, + // so it is deleted the way an unversioned bucket deletes it. + if versionId == "" && strings.HasSuffix(object, "/") { + return result, s3a.deleteDirectoryMarker(bucket, object) + } + switch { case versionId != "": versionEntry, versionLookupErr := s3a.getSpecificObjectVersion(bucket, object, versionId) @@ -230,10 +237,21 @@ func (s3a *S3ApiServer) DeleteObjectHandler(w http.ResponseWriter, r *http.Reque var deleteResult deleteMutationResult var deleteCode s3err.ErrorCode + // A trailing-slash key is a directory marker in every bucket, versioned or not, and + // is deleted the same way: the raw delete below cannot handle a directory that still + // has children, and versioning has nothing to add to a key that is not an object. + deleteHandled := false + if versionId == "" && strings.HasSuffix(object, "/") { + deleteCode, deleteHandled = s3a.withObjectWriteLock(bucket, object, func() s3err.ErrorCode { + return s3a.checkDeleteIfMatch(bucket, object, versionId, versioningState, r.Header.Get(s3_constants.IfMatch), s3err.ErrPreconditionFailed) + }, func() s3err.ErrorCode { + return s3a.deleteDirectoryMarker(bucket, object) + }), true + } + // Fast path: route the delete to the owner filer under its per-path lock; // routedObjectOwner excludes versioned/object-lock buckets. - deleteHandled := false - if !versioningConfigured { + if !deleteHandled && !versioningConfigured { if cond, condOk := buildDeleteCondition(r); condOk { if owner, ownerOk := s3a.routedObjectOwner(bucket, object); ownerOk { resp, err := s3a.routedDelete(owner, bucket, object, cond) @@ -468,6 +486,10 @@ func (s3a *S3ApiServer) DeleteMultipleObjectsHandler(w http.ResponseWriter, r *h return s3err.ErrAccessDenied } + if strings.HasSuffix(object.Key, "/") { + return s3a.deleteDirectoryMarker(bucket, object.Key) + } + if err := s3a.deleteUnversionedObjectWithClient(client, bucket, object.Key, false); err != nil { glog.Errorf("DeleteMultipleObjectsHandler: failed to delete %s/%s: %v", bucket, object.Key, err) return s3err.ErrInternalError diff --git a/weed/s3api/s3api_object_handlers_list.go b/weed/s3api/s3api_object_handlers_list.go index 835e92ca7..11d8c9fa1 100644 --- a/weed/s3api/s3api_object_handlers_list.go +++ b/weed/s3api/s3api_object_handlers_list.go @@ -752,7 +752,7 @@ func (s3a *S3ApiServer) doListFilerEntries(ctx context.Context, client filer_pb. if cursor.prefixEndsOnDelimiter { cursor.prefixEndsOnDelimiter = false } - isKeyObject := s3a.isLiveDirectoryKeyObject(ctx, client, entry, dir+"/"+entry.Name, cursor) + isKeyObject := entry.IsDirectoryKeyObject() if isKeyObject { // Directory key objects (created via PutObject with trailing "/") // must appear as regular keys in recursive listing mode. @@ -792,7 +792,7 @@ func (s3a *S3ApiServer) doListFilerEntries(ctx context.Context, client filer_pb. return } // println("doListFilerEntries2 nextMarker", nextMarker) - } else if s3a.isLiveDirectoryKeyObject(ctx, client, entry, dir+"/"+entry.Name, cursor) || !s3a.dirHoldsOnlyHiddenEntries(ctx, client, bucket, dir+"/"+entry.Name, cursor) { + } else if entry.IsDirectoryKeyObject() || !s3a.dirHoldsOnlyHiddenEntries(ctx, client, bucket, dir+"/"+entry.Name, cursor) { eachEntryFn(dir, entry) } } else { @@ -812,55 +812,6 @@ func (s3a *S3ApiServer) doListFilerEntries(ctx context.Context, client filer_pb. } } -// isLiveDirectoryKeyObject reports whether entry still stands for the key "/", -// and strips entry in place when it does not. A directory marker deleted in a versioned -// bucket keeps its payload on disk — that payload is the key's null version — so the -// copy this listing holds is demoted instead, which is what stops the callback further -// down from reporting the entry as a key while the directory still serves as a prefix. -func (s3a *S3ApiServer) isLiveDirectoryKeyObject(ctx context.Context, client filer_pb.SeaweedFilerClient, entry *filer_pb.Entry, dirPath string, cursor *ListingCursor) bool { - if !entry.IsDirectoryKeyObject() { - return false - } - if !cursor.hideDeletedPrefixes || !directoryMarkerIsDeleted(ctx, client, dirPath) { - return true - } - clearDirectoryMarkerMetadata(entry) - return false -} - -// directoryMarkerIsDeleted reports whether the key "/" currently resolves to a -// delete marker. An explicit directory marker is the filer directory itself, so its -// version history is the container's own .versions entry, whose current-version stamp -// every pointer flip maintains — one lookup, no version scan. An unstamped history -// leaves the key visible, as it was before this check existed. -func directoryMarkerIsDeleted(ctx context.Context, client filer_pb.SeaweedFilerClient, dirPath string) bool { - // The answer comes off the first entry, so cancel on return rather than draining. - ctx, cancel := context.WithCancel(ctx) - defer cancel() - - stream, err := client.ListEntries(ctx, &filer_pb.ListEntriesRequest{ - Directory: dirPath, - Prefix: s3_constants.VersionsFolder, - Limit: 1, - }) - if err != nil { - if !errors.Is(err, filer_pb.ErrNotFound) { - glog.V(1).Infof("directoryMarkerIsDeleted %s: %v", dirPath, err) - } - return false - } - for { - resp, recvErr := stream.Recv() - if recvErr != nil { - return false - } - if resp.Entry == nil || resp.Entry.Name != s3_constants.VersionsFolder { - continue - } - return string(resp.Entry.Extended[s3_constants.ExtLatestVersionIsDeleteMarker]) == "true" - } -} - // hiddenProbePageSize is the window one probe request asks the filer for, and // hiddenProbeBudget caps how many entries a single list request may look at while // deciding which directories still stand for a prefix. @@ -947,7 +898,7 @@ func (s3a *S3ApiServer) dirHoldsOnlyHiddenEntries(ctx context.Context, client fi } return false } - if s3a.isLiveDirectoryKeyObject(ctx, client, entry, dir+"/"+entry.Name, cursor) { + if entry.IsDirectoryKeyObject() { return false } if !s3a.dirHoldsOnlyHiddenEntries(ctx, client, bucket, dir+"/"+entry.Name, cursor) { diff --git a/weed/s3api/s3api_object_handlers_list_directory_marker_test.go b/weed/s3api/s3api_object_handlers_list_directory_marker_test.go index ac20388c7..fe5ab2c7e 100644 --- a/weed/s3api/s3api_object_handlers_list_directory_marker_test.go +++ b/weed/s3api/s3api_object_handlers_list_directory_marker_test.go @@ -1,17 +1,18 @@ package s3api import ( - "strconv" + "context" "testing" - "time" "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" "github.com/stretchr/testify/assert" + grpc "google.golang.org/grpc" ) // directoryMarker builds the filer directory a PutObject on a trailing-slash key -// leaves behind. +// leaves behind. Deleting the key strips this metadata back off, so a deleted marker +// is just newDir. func directoryMarker(name string) *filer_pb.Entry { entry := newDir(name) entry.Attributes.Mime = s3_constants.FolderMimeType @@ -19,69 +20,47 @@ func directoryMarker(name string) *filer_pb.Entry { return entry } -// ownVersionsDir builds the history of the key "/", which lives inside the -// directory it names rather than beside it. -func ownVersionsDir(deleted bool) *filer_pb.Entry { - now := time.Now().Unix() - extended := map[string][]byte{ - s3_constants.ExtLatestVersionIdKey: []byte("v-dir"), - s3_constants.ExtLatestVersionMtimeKey: []byte(strconv.FormatInt(now, 10)), - s3_constants.ExtLatestVersionSizeKey: []byte("0"), - s3_constants.ExtLatestVersionETagKey: []byte(`"d41d8cd98f00b204e9800998ecf8427e"`), - s3_constants.ExtLatestVersionIsDeleteMarker: []byte(strconv.FormatBool(deleted)), - } - return &filer_pb.Entry{ - Name: s3_constants.VersionsFolder, - IsDirectory: true, - Attributes: &filer_pb.FuseAttributes{Mtime: now}, - Extended: extended, - } -} - -// TestDeletedDirectoryMarkerIsNotListed covers the reported flow: PutObject on "m2/", -// DELETE on "m2/" writes a delete marker into m2/.versions, and the key must stop -// being reported even though the filer directory that carries it survives. +// TestDeletedDirectoryMarkerIsNotListed covers the reported flow. The delete demotes the +// directory that carried the key, so what is left lists as the plain directory it is. func TestDeletedDirectoryMarkerIsNotListed(t *testing.T) { client := &testFilerClient{ entriesByDir: map[string][]*filer_pb.Entry{ - "/buckets/test": {directoryMarker("m2")}, - "/buckets/test/m2": {ownVersionsDir(true)}, + "/buckets/test": {newDir("m2")}, + "/buckets/test/m2": {deleteMarkedVersionsDir("f.txt")}, }, } seen := listedNames(t, client, listDirectoryRequest{dir: "/buckets/test", delimiter: "/", bucket: "test"}, &ListingCursor{maxKeys: 1000, hideDeletedPrefixes: true}) - assert.Empty(t, seen, "a delete-marked directory marker is not a key and names no prefix") + assert.Empty(t, seen, "nothing under it is a key, so it names no prefix either") seen = listedNames(t, client, listDirectoryRequest{dir: "/buckets/test", bucket: "test"}, &ListingCursor{maxKeys: 1000, hideDeletedPrefixes: true}) assert.Empty(t, seen, "the flat listing must not report it either") } -// TestLiveDirectoryMarkerIsListed pins the other half: a marker whose current version -// is real is a key, and the listing reports it from the directory that carries it. +// TestLiveDirectoryMarkerIsListed pins the other half: the directory entry carries the +// key, so the listing reports it straight off that entry. func TestLiveDirectoryMarkerIsListed(t *testing.T) { client := &testFilerClient{ entriesByDir: map[string][]*filer_pb.Entry{ "/buckets/test": {directoryMarker("m2")}, - "/buckets/test/m2": {ownVersionsDir(false)}, + "/buckets/test/m2": {}, }, } seen := listedNames(t, client, listDirectoryRequest{dir: "/buckets/test", delimiter: "/", bucket: "test"}, &ListingCursor{maxKeys: 1000, hideDeletedPrefixes: true}) assert.Equal(t, []string{"m2"}, seen) - // A live history must not also surface as a phantom key named after its container. seen = listedNames(t, client, listDirectoryRequest{dir: "/buckets/test", bucket: "test"}, &ListingCursor{maxKeys: 1000, hideDeletedPrefixes: true}) assert.Equal(t, []string{"m2"}, seen) } -// TestDeletedDirectoryMarkerKeepsLiveChildren guards the boundary between the two -// questions a directory answers: deleting the key "m2/" says nothing about the objects -// below it, which keep the prefix alive. +// TestDeletedDirectoryMarkerKeepsLiveChildren guards the boundary: deleting the key +// "m2/" says nothing about the objects below it, which keep the prefix alive. func TestDeletedDirectoryMarkerKeepsLiveChildren(t *testing.T) { client := &testFilerClient{ entriesByDir: map[string][]*filer_pb.Entry{ - "/buckets/test": {directoryMarker("m2")}, - "/buckets/test/m2": {ownVersionsDir(true), liveVersionsDir("keep.txt")}, + "/buckets/test": {newDir("m2")}, + "/buckets/test/m2": {liveVersionsDir("keep.txt")}, }, } @@ -92,31 +71,55 @@ func TestDeletedDirectoryMarkerKeepsLiveChildren(t *testing.T) { assert.Equal(t, []string{"keep.txt"}, seen, "only the surviving child is a key") } -// TestDeletedDirectoryMarkerIsNotADirectoryProbe checks the trailing-slash probe: the -// key was deleted, so prefix=m2/ answers empty instead of resurfacing the marker. -func TestDeletedDirectoryMarkerIsNotADirectoryProbe(t *testing.T) { +// ownVersionsDir builds a history recorded inside a directory by an older build, which +// described the key "/" rather than any child of it. +func ownVersionsDir() *filer_pb.Entry { + entry := deleteMarkedVersionsDir("") + entry.Name = s3_constants.VersionsFolder + return entry +} + +// TestStaleOwnVersionsIsNotAKey pins the guard for buckets written before directory +// markers stopped being versioned: a history left inside a directory describes that +// directory, and must not surface as a key named after it. +func TestStaleOwnVersionsIsNotAKey(t *testing.T) { client := &testFilerClient{ entriesByDir: map[string][]*filer_pb.Entry{ "/buckets/test": {directoryMarker("m2")}, - "/buckets/test/m2": {ownVersionsDir(true)}, + "/buckets/test/m2": {ownVersionsDir()}, }, } - seen := listedNames(t, client, listDirectoryRequest{dir: "/buckets/test", prefix: "m2", delimiter: "/", bucket: "test"}, - &ListingCursor{maxKeys: 1000, prefixEndsOnDelimiter: true, hideDeletedPrefixes: true}) - assert.Empty(t, seen) + seen := listedNames(t, client, listDirectoryRequest{dir: "/buckets/test", bucket: "test"}, &ListingCursor{maxKeys: 1000, hideDeletedPrefixes: true}) + assert.Equal(t, []string{"m2"}, seen, "the marker is the only key; no phantom m2/m2") } -// TestUnversionedBucketKeepsItsDirectoryMarkers pins the gate: without versioning there -// is no history to consult and no lookup to pay for. -func TestUnversionedBucketKeepsItsDirectoryMarkers(t *testing.T) { - client := &testFilerClient{ +// countingFilerClient records how many ListEntries round trips a listing spends. +type countingFilerClient struct { + *testFilerClient + calls int +} + +func (c *countingFilerClient) ListEntries(ctx context.Context, in *filer_pb.ListEntriesRequest, opts ...grpc.CallOption) (grpc.ServerStreamingClient[filer_pb.ListEntriesResponse], error) { + c.calls++ + return c.testFilerClient.ListEntries(ctx, in, opts...) +} + +// TestDirectoryMarkersCostNoLookups is why the delete is recorded on the directory +// entry rather than in a history a listing has to go and read. Buckets written by tools +// that keep a marker per directory are made of these, so a round trip per marker is the +// whole listing cost. +func TestDirectoryMarkersCostNoLookups(t *testing.T) { + client := &countingFilerClient{testFilerClient: &testFilerClient{ entriesByDir: map[string][]*filer_pb.Entry{ - "/buckets/test": {directoryMarker("m2")}, - "/buckets/test/m2": {ownVersionsDir(true)}, + "/buckets/test": {directoryMarker("d1"), directoryMarker("d2"), directoryMarker("d3")}, + "/buckets/test/d1": {}, + "/buckets/test/d2": {}, + "/buckets/test/d3": {}, }, - } + }} - seen := listedNames(t, client, listDirectoryRequest{dir: "/buckets/test", delimiter: "/", bucket: "test"}, &ListingCursor{maxKeys: 1000}) - assert.Equal(t, []string{"m2"}, seen) + seen := listedNames(t, client, listDirectoryRequest{dir: "/buckets/test", delimiter: "/", bucket: "test"}, &ListingCursor{maxKeys: 1000, hideDeletedPrefixes: true}) + assert.Equal(t, []string{"d1", "d2", "d3"}, seen) + assert.Equal(t, 1, client.calls, "listing directory markers must not cost a round trip per marker") } diff --git a/weed/s3api/s3api_object_handlers_put.go b/weed/s3api/s3api_object_handlers_put.go index ddea6b826..5055f61b6 100644 --- a/weed/s3api/s3api_object_handlers_put.go +++ b/weed/s3api/s3api_object_handlers_put.go @@ -189,24 +189,6 @@ func (s3a *S3ApiServer) PutObjectHandler(w http.ResponseWriter, r *http.Request) s3err.WriteErrorResponse(w, r, filerErrorToS3Error(err)) return } - // A directory marker is stored as the directory itself, so it is the key's null - // version. Re-creating one after a delete has to retire that delete marker, - // otherwise the key stays invisible to every versioned read. Reporting the PUT - // as successful before that is known to have happened would hand the client a - // marker it cannot see, so an unreadable versioning state fails the request. - state, stateErr := s3a.getVersioningState(bucket) - if stateErr != nil && !errors.Is(stateErr, filer_pb.ErrNotFound) { - glog.Errorf("PutObjectHandler: versioning state of %s unknown: %v", bucket, stateErr) - s3err.WriteErrorResponse(w, r, s3err.ErrInternalError) - return - } - if state != "" { - if err := s3a.restoreNullVersion(bucket, strings.TrimPrefix(object, "/")); err != nil { - glog.Errorf("PutObjectHandler: failed to restore directory marker %s/%s: %v", bucket, object, err) - s3err.WriteErrorResponse(w, r, s3err.ErrInternalError) - return - } - } setEtag(w, dirEtag) } else { // Get detailed versioning state for the bucket @@ -1401,22 +1383,6 @@ func (s3a *S3ApiServer) putSuspendedVersioningObject(r *http.Request, bucket, ob return etag, s3err.ErrNone, sseMetadata } -// restoreNullVersion makes the object at the regular path the current version again: -// it drops a stale null version from .versions and clears the latest-version pointer, -// which is the recorded way of saying "the null object is current". The clearing helper -// is named for suspended versioning but does exactly this, and an enabled bucket needs -// the same thing when a directory marker is re-created over its delete marker. -func (s3a *S3ApiServer) restoreNullVersion(bucket, object string) error { - if _, err := s3a.getEntry(s3a.bucketDir(bucket), object+s3_constants.VersionsFolder); err != nil { - if errors.Is(err, filer_pb.ErrNotFound) { - return nil // no history, so the object at the regular path is already current - } - return fmt.Errorf("read version history of %s/%s: %w", bucket, object, err) - } - s3a.removeNullVersionFile(bucket, object) - return s3a.updateIsLatestFlagsForSuspendedVersioning(bucket, object) -} - // removeNullVersionFile deletes the "null" version file from an object's .versions // directory, leaving real versions alone. Best-effort: a leftover null version is // superseded by the object at the regular path on the next read. diff --git a/weed/s3api/s3api_object_versioning.go b/weed/s3api/s3api_object_versioning.go index 2843e614d..21d7194ca 100644 --- a/weed/s3api/s3api_object_versioning.go +++ b/weed/s3api/s3api_object_versioning.go @@ -649,9 +649,8 @@ func (vc *versionCollector) processVersionsDirectory(entryPath string) error { return nil } -// processExplicitDirectory handles an explicit S3 directory object. containerPath is -// the directory's own filer path, which is where the key's version history lives. -func (vc *versionCollector) processExplicitDirectory(entryPath, containerPath string, entry *filer_pb.Entry) error { +// processExplicitDirectory handles an explicit S3 directory object +func (vc *versionCollector) processExplicitDirectory(entryPath string, entry *filer_pb.Entry) { directoryKey := entryPath if !strings.HasSuffix(directoryKey, "/") { directoryKey += "/" @@ -663,33 +662,18 @@ func (vc *versionCollector) processExplicitDirectory(entryPath, containerPath st // this mirrors ListObjectsV2 and AWS, and stops clients like Veeam that // reject unexpected keys in a listing from aborting. if !strings.HasPrefix(directoryKey, vc.prefix) { - return nil + return } // Skip directories at or before keyMarker if vc.keyMarker != "" && directoryKey <= vc.keyMarker { - return nil - } - - // The directory entry is this key's null version. A history under it names the - // current version, so the null is only latest when there is no pointer to follow. - // A history we cannot read leaves that unknown, and claiming latest would hide a - // current delete marker, so the listing fails instead of guessing. - isLatest := true - versionsEntry, err := vc.s3a.getEntry(containerPath, s3_constants.VersionsFolder) - switch { - case err == nil: - if _, hasPointer := versionsEntry.Extended[s3_constants.ExtLatestVersionIdKey]; hasPointer { - isLatest = false - } - case !errors.Is(err, filer_pb.ErrNotFound): - return fmt.Errorf("read version history of %s: %w", containerPath, err) + return } versionEntry := &VersionEntry{ Key: directoryKey, VersionId: "null", - IsLatest: isLatest, + IsLatest: true, LastModified: time.Unix(entry.Attributes.Mtime, 0), ETag: "\"d41d8cd98f00b204e9800998ecf8427e\"", // Empty content ETag Size: 0, @@ -697,7 +681,6 @@ func (vc *versionCollector) processExplicitDirectory(entryPath, containerPath st StorageClass: StorageClass(vc.s3a.getStorageClassFromExtended(entry.Extended)), } *vc.allVersions = append(*vc.allVersions, versionEntry) - return nil } // processRegularFile handles a regular file entry (pre-versioning or suspended-versioning object) @@ -887,9 +870,7 @@ func (vc *versionCollector) processDirectory(currentPath, entryPath string, entr // an SDK PutObject of "dir/" carries a default Content-Type, so the two // listings must agree on what counts as a directory key. if entry.IsDirectoryKeyObject() { - if err := vc.processExplicitDirectory(entryPath, path.Join(currentPath, entry.Name), entry); err != nil { - return err - } + vc.processExplicitDirectory(entryPath, entry) } // Skip entire subdirectory if all keys within it are before the keyMarker.