s3: keep the list marker exclusive for versioned objects (#10496)

* s3: keep the list marker exclusive for versioned objects

A versioned object lives in a "<key>.versions" directory, so the entry name
never matched the marker and start-after/marker returned the marker key itself.

* s3: match the list marker against the raw entry name too

A backend that echoes the marker it was given returns the ".versions" directory
name, which no longer matched once the comparison used the object name alone.
Cover both, and unit test each half.

test/s3/versioning/s3_versioning_list_marker_test.go

@@ -0,0 +1,62 @@
+package s3api
+
+import (
+	"context"
+	"testing"
+
+	"github.com/aws/aws-sdk-go-v2/aws"
+	"github.com/aws/aws-sdk-go-v2/service/s3"
+	"github.com/aws/aws-sdk-go-v2/service/s3/types"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// TestVersionedListMarkerIsExclusive covers start-after and marker on a versioned bucket,
+// where an object is stored in a "<key>.versions" directory and so never matches the
+// marker by entry name.
+func TestVersionedListMarkerIsExclusive(t *testing.T) {
+	client := getS3Client(t)
+	bucketName := getNewBucketName()
+
+	createBucket(t, client, bucketName)
+	defer deleteBucket(t, client, bucketName)
+	enableVersioning(t, client, bucketName)
+
+	for _, key := range []string{"file-0", "file-1", "file-2", "logs/file-0", "logs/file-1", "logs/file-2"} {
+		putObject(t, client, bucketName, key, "content")
+	}
+
+	listV2 := func(prefix, startAfter string) []string {
+		t.Helper()
+		resp, err := client.ListObjectsV2(context.TODO(), &s3.ListObjectsV2Input{
+			Bucket:     aws.String(bucketName),
+			Prefix:     aws.String(prefix),
+			StartAfter: aws.String(startAfter),
+		})
+		require.NoError(t, err)
+		return contentKeys(resp.Contents)
+	}
+	listV1 := func(prefix, marker string) []string {
+		t.Helper()
+		resp, err := client.ListObjects(context.TODO(), &s3.ListObjectsInput{
+			Bucket: aws.String(bucketName),
+			Prefix: aws.String(prefix),
+			Marker: aws.String(marker),
+		})
+		require.NoError(t, err)
+		return contentKeys(resp.Contents)
+	}
+
+	assert.Equal(t, []string{"file-2"}, listV2("file-", "file-1"))
+	assert.Equal(t, []string{"file-2"}, listV1("file-", "file-1"))
+	assert.Equal(t, []string{"logs/file-2"}, listV2("logs/", "logs/file-1"))
+	assert.Equal(t, []string{"logs/file-2"}, listV1("logs/", "logs/file-1"))
+}
+
+func contentKeys(contents []types.Object) []string {
+	keys := make([]string, 0, len(contents))
+	for _, c := range contents {
+		keys = append(keys, *c.Key)
+	}
+	return keys
+}

weed/s3api/s3api_object_handlers_list.go

@@ -678,7 +678,13 @@ func (s3a *S3ApiServer) doListFilerEntries(ctx context.Context, client filer_pb.
 			// listFilerEntries always calls doListFilerEntries with inclusiveStartFrom=false
 			// (S3 marker semantics are exclusive), but keep the guard explicit to preserve
 			// behavior if inclusive callers are introduced in the future.
-			if !inclusiveStartFrom && marker != "" && entry.Name == marker {
+			// A versioned object lives in a "<key>.versions" directory, so the marker also
+			// has to be matched against the object name that directory stands for.
+			markerName := entry.Name
+			if entry.IsDirectory {
+				markerName = strings.TrimSuffix(markerName, s3_constants.VersionsFolder)
+			}
+			if !inclusiveStartFrom && marker != "" && (entry.Name == marker || markerName == marker) {
 				continue
 			}
 

weed/s3api/s3api_object_handlers_list_versioned_test.go

@@ -684,3 +684,52 @@ func TestProcessDirectorySkipsBeforeMarker(t *testing.T) {
 		})
 	}
 }
+
+// TestVersionedListSkipsMarkerObject covers the exclusive marker on a versioned bucket,
+// where the marker names an object but the entry is its ".versions" directory.
+func TestVersionedListSkipsMarkerObject(t *testing.T) {
+	s3a := &S3ApiServer{option: &S3ApiServerOption{BucketsPath: "/buckets"}}
+	client := &testFilerClient{
+		entriesByDir: map[string][]*filer_pb.Entry{
+			"/buckets/test-bucket": {
+				liveVersionsDir("file-1"),
+				liveVersionsDir("file-2"),
+			},
+		},
+	}
+
+	cursor := &ListingCursor{maxKeys: 1000}
+	var seen []string
+	_, err := s3a.doListFilerEntries(context.Background(), client, listDirectoryRequest{dir: "/buckets/test-bucket", marker: "file-1", bucket: "test-bucket"}, cursor, func(dir string, entry *filer_pb.Entry) {
+		seen = append(seen, entry.Name)
+		cursor.maxKeys--
+	})
+
+	assert.NoError(t, err)
+	assert.Equal(t, []string{"file-2"}, seen, "the marker object should not be returned")
+}
+
+// TestVersionedListSkipsEchoedVersionsMarker covers a backend that echoes the marker it
+// was given, when that marker is a ".versions" directory name.
+func TestVersionedListSkipsEchoedVersionsMarker(t *testing.T) {
+	s3a := &S3ApiServer{option: &S3ApiServerOption{BucketsPath: "/buckets"}}
+	client := &markerEchoFilerClient{
+		entriesByDir: map[string][]*filer_pb.Entry{
+			"/buckets/test-bucket": {
+				liveVersionsDir("file-1"),
+				liveVersionsDir("file-2"),
+			},
+		},
+		returnFollowing: true,
+	}
+
+	cursor := &ListingCursor{maxKeys: 1000}
+	var seen []string
+	_, err := s3a.doListFilerEntries(context.Background(), client, listDirectoryRequest{dir: "/buckets/test-bucket", marker: "file-1" + s3_constants.VersionsFolder, bucket: "test-bucket"}, cursor, func(dir string, entry *filer_pb.Entry) {
+		seen = append(seen, entry.Name)
+		cursor.maxKeys--
+	})
+
+	assert.NoError(t, err)
+	assert.Equal(t, []string{"file-2"}, seen, "the echoed marker entry should not be returned")
+}