diff --git a/test/s3/versioning/s3_listing_regression_test.go b/test/s3/versioning/s3_listing_regression_test.go new file mode 100644 index 0000000000..169c8511bb --- /dev/null +++ b/test/s3/versioning/s3_listing_regression_test.go @@ -0,0 +1,112 @@ +package s3api + +import ( + "bytes" + "context" + "fmt" + "sort" + "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" +) + +// Check both wire APIs with small pages. The directory walker alone does +// not exercise prefix normalization, delimiter-marker adjustment, or tokens. +func TestListingPrefixAndDelimiterPagination(t *testing.T) { + client := getS3Client(t) + ctx := context.Background() + for _, versioning := range []string{"", "Enabled", "Suspended"} { + t.Run("versioning="+versioning, func(t *testing.T) { + bucket := getNewBucketName() + createBucket(t, client, bucket) + defer deleteBucket(t, client, bucket) + if versioning != "" { + enableVersioning(t, client, bucket) + if versioning == "Suspended" { + _, err := client.PutBucketVersioning(ctx, &s3.PutBucketVersioningInput{ + Bucket: aws.String(bucket), VersioningConfiguration: &types.VersioningConfiguration{Status: types.BucketVersioningStatusSuspended}, + }) + require.NoError(t, err) + } + } + for _, key := range []string{"a", "a-c", "a.txt", "a/b", "foo_bar", "foo_bar_baz", "foo_baz"} { + _, err := client.PutObject(ctx, &s3.PutObjectInput{Bucket: aws.String(bucket), Key: aws.String(key), Body: bytes.NewReader([]byte("x"))}) + require.NoError(t, err) + } + for _, tc := range []struct { + name, prefix, delimiter, startAfter string + keys, prefixes []string + }{ + {"trailing slash", "a/", "", "", []string{"a/b"}, nil}, + {"trailing slash custom delimiter", "a/", "_", "", []string{"a/b"}, nil}, + {"custom delimiter", "foo", "_", "", nil, []string{"foo_"}}, + {"custom delimiter nested group", "foo_", "_", "", []string{"foo_bar", "foo_baz"}, []string{"foo_bar_"}}, + {"marker names directory", "a", "", "a", []string{"a-c", "a.txt", "a/b"}, nil}, + {"marker between name and slash", "a", "", "a.txt", []string{"a/b"}, nil}, + } { + for _, v2 := range []bool{false, true} { + for _, pageSize := range []int32{1, 2, 1000} { + t.Run(fmt.Sprintf("%s/v2=%t/max=%d", tc.name, v2, pageSize), func(t *testing.T) { + var keys, prefixes []string + marker, token := tc.startAfter, "" + for page := 0; ; page++ { + require.Less(t, page, 10, "listing must terminate without repeating a prefix") + var objects []types.Object + var commonPrefixes []types.CommonPrefix + var truncated bool + if v2 { + in := &s3.ListObjectsV2Input{Bucket: aws.String(bucket), Prefix: aws.String(tc.prefix), Delimiter: aws.String(tc.delimiter), MaxKeys: aws.Int32(pageSize)} + if token != "" { + in.ContinuationToken = aws.String(token) + } else if marker != "" { + in.StartAfter = aws.String(marker) + } + out, err := client.ListObjectsV2(ctx, in) + require.NoError(t, err) + objects, commonPrefixes, truncated = out.Contents, out.CommonPrefixes, aws.ToBool(out.IsTruncated) + next := aws.ToString(out.NextContinuationToken) + if truncated { + require.NotEmpty(t, next) + require.NotEqual(t, token, next, "token must advance") + } + token = next + } else { + out, err := client.ListObjects(ctx, &s3.ListObjectsInput{ + Bucket: aws.String(bucket), Prefix: aws.String(tc.prefix), Delimiter: aws.String(tc.delimiter), MaxKeys: aws.Int32(pageSize), Marker: aws.String(marker), + }) + require.NoError(t, err) + objects, commonPrefixes, truncated = out.Contents, out.CommonPrefixes, aws.ToBool(out.IsTruncated) + next := aws.ToString(out.NextMarker) + if truncated { + if next == "" && len(objects) > 0 { + next = aws.ToString(objects[len(objects)-1].Key) + } + require.Greater(t, next, marker, "marker must advance") + } + marker = next + } + for _, object := range objects { + keys = append(keys, aws.ToString(object.Key)) + } + for _, prefix := range commonPrefixes { + prefixes = append(prefixes, aws.ToString(prefix.Prefix)) + } + if !truncated { + break + } + } + sort.Strings(keys) + sort.Strings(prefixes) + assert.Equal(t, tc.keys, keys) + assert.Equal(t, tc.prefixes, prefixes) + }) + } + } + } + }) + } +} diff --git a/weed/s3api/s3api_object_handlers_list.go b/weed/s3api/s3api_object_handlers_list.go index 679c9f438a..6b049e0e70 100644 --- a/weed/s3api/s3api_object_handlers_list.go +++ b/weed/s3api/s3api_object_handlers_list.go @@ -538,6 +538,18 @@ func (s3a *S3ApiServer) listFilerEntries(ctx context.Context, req listObjectsReq if excludedKey != "" && !entry.IsDirectory && fmt.Sprintf("%s/%s", dir, entry.Name)[len(bucketPrefix):] == excludedKey { return } + // A delimiter marker excludes the entire folded prefix, not just + // the key bearing its name. This also applies to delimiters such as + // "_", whose groups are not directory blocks in the filer walk. + if delimiter != "" { + name := entry.Name + if entry.IsDirectory && !entry.IsPrefixObject() { + name += "/" + } + if folded := prefixForKey(dir, name); folded != "" && folded <= requestMarker { + return + } + } dirName, entryName, _ := entryUrlEncode(dir, entry.Name, encodingTypeUrl) if entry.IsDirectory { if originalPrefix != "" { @@ -1170,13 +1182,12 @@ func (s3a *S3ApiServer) doListFilerEntries(ctx context.Context, client filer_pb. // we must stop processing siblings to avoid matching unrelated entries like "foo1000". matchedPrefixDir := cursor.prefixEndsOnDelimiter && entry.Name == prefix && entry.IsDirectory if cursor.prefixEndsOnDelimiter { - if entry.Name == prefix && entry.IsDirectory { - if delimiter != "/" { - cursor.prefixEndsOnDelimiter = false - } - } else { + if !matchedPrefixDir { continue } + // Keep the exact-directory filter until emitDir descends into + // the deferred block. Clearing it at the name position lets + // siblings like "a.txt" leak into a listing for "a/". } if entry.IsDirectory { // glog.V(4).Infof("List Dir Entries %s, file: %s, maxKeys %d", dir, entry.Name, cursor.maxKeys) diff --git a/weed/s3api/s3api_object_handlers_list_regression_test.go b/weed/s3api/s3api_object_handlers_list_regression_test.go new file mode 100644 index 0000000000..72d4066189 --- /dev/null +++ b/weed/s3api/s3api_object_handlers_list_regression_test.go @@ -0,0 +1,125 @@ +package s3api + +import ( + "context" + "fmt" + "net/url" + "sort" + "strings" + "testing" + "time" + + "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Exercise the response builder as well as the directory walker: deferred +// directory resume markers can make the outer loop repeat prefixes. +// Derive the expected response from S3 keys, independently of filer traversal. +func TestListFilerEntriesListingRegression(t *testing.T) { + keys := []string{ + "a", "a-c", "a.txt", "a/b", "a/c/d", "a0", + "boo/bar", "boo/baz", "boo/boo", "boo/boo2", + "foo_bar", "foo_bar_baz", "foo_baz", "logs/", "quux ab", "space dir/value", "z", + } + sort.Strings(keys) + prefixObject := newDir("a") + prefixObject.MarkPrefixObject() + file := func(name string) *filer_pb.Entry { + return &filer_pb.Entry{Name: name, Attributes: &filer_pb.FuseAttributes{}} + } + filer := &fakeBucketDeleteFiler{entriesByDir: map[string][]*filer_pb.Entry{ + "/buckets/test": { + prefixObject, file("a-c"), file("a.txt"), file("a0"), newDir("boo"), + file("foo_bar"), file("foo_bar_baz"), file("foo_baz"), + directoryMarker("logs"), file("quux ab"), newDir("space dir"), file("z"), + }, + "/buckets/test/a": {file("b"), newDir("c")}, + "/buckets/test/a/c": {file("d")}, + "/buckets/test/boo": {file("bar"), file("baz"), file("boo"), file("boo2")}, + "/buckets/test/logs": {}, + "/buckets/test/space dir": {file("value")}, + }} + s3a := newFailoverTestServer(t, startFakeFiler(t, filer)) + s3a.option.BucketsPath = "/buckets" + s3a.bucketConfigCache = NewBucketConfigCache(time.Minute) + for _, versioning := range []string{"", "Enabled", "Suspended"} { + t.Run("versioning="+versioning, func(t *testing.T) { + s3a.bucketConfigCache.Set("test", &BucketConfig{Name: "test", Versioning: versioning}) + for _, encoded := range []bool{false, true} { + t.Run(fmt.Sprintf("encoded=%t", encoded), func(t *testing.T) { + testListingRegressionMatrix(t, s3a, keys, encoded) + }) + } + }) + } +} + +func testListingRegressionMatrix(t *testing.T, s3a *S3ApiServer, keys []string, encoded bool) { + t.Helper() + for _, prefix := range []string{"", "a", "a/", "a/c/", "boo", "boo/", "boo/b", "foo", "foo_", "logs/", "space dir/", "missing/"} { + for _, delimiter := range []string{"", "/", "_"} { + for _, pageSize := range []uint16{1, 2, 1000} { + t.Run(fmt.Sprintf("prefix=%s/delimiter=%s/max=%d", prefix, delimiter, pageSize), func(t *testing.T) { + var wantKeys, wantPrefixes []string + prefixes := map[string]bool{} + for _, key := range keys { + if !strings.HasPrefix(key, prefix) { + continue + } + if delimiter != "" { + if i := strings.Index(strings.TrimPrefix(key, prefix), delimiter); i >= 0 { + prefixes[key[:len(prefix)+i+len(delimiter)]] = true + continue + } + } + wantKeys = append(wantKeys, key) + } + for p := range prefixes { + wantPrefixes = append(wantPrefixes, p) + } + sort.Strings(wantKeys) + sort.Strings(wantPrefixes) + + var gotKeys, gotPrefixes []string + marker := "" + for page := 0; ; page++ { + require.Less(t, page, len(keys)+1, "listing did not terminate") + response, err := s3a.listFilerEntries(context.Background(), listObjectsRequest{ + bucket: "test", prefix: prefix, delimiter: delimiter, + marker: adjustMarkerForDelimiter(marker, prefix, delimiter), + requestMarker: marker, maxKeys: pageSize, + encodingTypeUrl: encoded, + }) + require.NoError(t, err) + decode := func(value string) string { + if !encoded { + return value + } + decoded, err := url.QueryUnescape(value) + require.NoError(t, err) + return decoded + } + for _, entry := range response.Contents { + gotKeys = append(gotKeys, decode(entry.Key)) + } + for _, entry := range response.CommonPrefixes { + gotPrefixes = append(gotPrefixes, decode(entry.Prefix)) + } + if !response.IsTruncated { + break + } + require.Greater(t, response.NextMarker, marker, "pagination must advance") + marker = response.NextMarker + } + // Sorting retains duplicates, so repeated prefixes still fail. + sort.Strings(gotKeys) + sort.Strings(gotPrefixes) + assert.Equal(t, wantKeys, gotKeys) + assert.Equal(t, wantPrefixes, gotPrefixes) + }) + } + } + } +}