From f3f47a7a732876e960d25ac784012a879ca58df6 Mon Sep 17 00:00:00 2001 From: glorv Date: Wed, 24 Feb 2021 11:33:34 +0800 Subject: [PATCH 1/3] fix s3 walk directory --- pkg/storage/s3.go | 23 +++++-------- pkg/storage/s3_test.go | 75 ++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 82 insertions(+), 16 deletions(-) diff --git a/pkg/storage/s3.go b/pkg/storage/s3.go index 3d2f6eac8..8a9cc5555 100644 --- a/pkg/storage/s3.go +++ b/pkg/storage/s3.go @@ -9,6 +9,7 @@ import ( "io" "io/ioutil" "net/url" + "path" "regexp" "strconv" "strings" @@ -279,8 +280,10 @@ func newS3Storage(backend *backup.S3, opts *ExternalStorageOptions) (*S3Storage, return nil, errors.Annotatef(berrors.ErrStorageInvalidConfig, "Bucket %s is not accessible: %v", qs.Bucket, err) } } + if len(qs.Prefix) > 0 { + qs.Prefix += "/" + } - qs.Prefix += "/" return &S3Storage{ session: ses, svc: c, @@ -378,28 +381,20 @@ func (rs *S3Storage) WalkDir(ctx context.Context, opt *WalkOption, fn func(strin if opt == nil { opt = &WalkOption{} } - - // NOTE: leave prefix empty if subDir is not set. Else, s3 will return empty result! - prefix := "" - if len(opt.SubDir) > 0 { - prefix = rs.options.Prefix + opt.SubDir - if len(prefix) > 0 && !strings.HasSuffix(prefix, "/") { - prefix += "/" - } + prefix := path.Join(rs.options.Prefix, opt.SubDir) + if len(prefix) > 0 && !strings.HasSuffix(prefix, "/") { + prefix += "/" } - maxKeys := int64(1000) if opt.ListCount > 0 { maxKeys = opt.ListCount } - req := &s3.ListObjectsInput{ Bucket: aws.String(rs.options.Bucket), + Prefix: aws.String(prefix), MaxKeys: aws.Int64(maxKeys), } - if len(prefix) > 0 { - req.Prefix = aws.String(prefix) - } + for { // FIXME: We can't use ListObjectsV2, it is not universally supported. // (Ceph RGW supported ListObjectsV2 since v15.1.0, released 2020 Jan 30th) diff --git a/pkg/storage/s3_test.go b/pkg/storage/s3_test.go index ea73f8a1e..9db2f716f 100644 --- a/pkg/storage/s3_test.go +++ b/pkg/storage/s3_test.go @@ -870,7 +870,7 @@ func (s *s3Suite) TestWalkDir(c *C) { }, nil }). After(firstCall) - s.s3.EXPECT(). + thirdCall := s.s3.EXPECT(). ListObjectsWithContext(ctx, gomock.Any()). DoAndReturn(func(_ context.Context, input *s3.ListObjectsInput) (*s3.ListObjectsOutput, error) { c.Assert(aws.StringValue(input.Marker), Equals, aws.StringValue(contents[3].Key)) @@ -881,6 +881,31 @@ func (s *s3Suite) TestWalkDir(c *C) { }, nil }). After(secondCall) + forthCall := s.s3.EXPECT(). + ListObjectsWithContext(ctx, gomock.Any()). + DoAndReturn(func(_ context.Context, input *s3.ListObjectsInput) (*s3.ListObjectsOutput, error) { + c.Assert(aws.StringValue(input.Bucket), Equals, "bucket") + c.Assert(aws.StringValue(input.Prefix), Equals, "prefix/") + c.Assert(aws.StringValue(input.Marker), Equals, "") + c.Assert(aws.Int64Value(input.MaxKeys), Equals, int64(4)) + c.Assert(aws.StringValue(input.Delimiter), Equals, "") + return &s3.ListObjectsOutput{ + IsTruncated: aws.Bool(true), + Contents: contents[:4], + }, nil + }). + After(thirdCall) + s.s3.EXPECT(). + ListObjectsWithContext(ctx, gomock.Any()). + DoAndReturn(func(_ context.Context, input *s3.ListObjectsInput) (*s3.ListObjectsOutput, error) { + c.Assert(aws.StringValue(input.Marker), Equals, aws.StringValue(contents[3].Key)) + c.Assert(aws.Int64Value(input.MaxKeys), Equals, int64(4)) + return &s3.ListObjectsOutput{ + IsTruncated: aws.Bool(false), + Contents: contents[4:], + }, nil + }). + After(forthCall) // Ensure we receive the items in order. i := 0 @@ -897,6 +922,22 @@ func (s *s3Suite) TestWalkDir(c *C) { ) c.Assert(err, IsNil) c.Assert(i, Equals, len(contents)) + + // test with empty subDir + i = 0 + err = s.storage.WalkDir( + ctx, + &WalkOption{ListCount: 4}, + func(path string, size int64) error { + comment := Commentf("index = %d", i) + c.Assert("prefix/"+path, Equals, *contents[i].Key, comment) + c.Assert(size, Equals, *contents[i].Size, comment) + i++ + return nil + }, + ) + c.Assert(err, IsNil) + c.Assert(i, Equals, len(contents)) } // TestWalkDirBucket checks WalkDir retrieves all directory content under a bucket. @@ -927,7 +968,7 @@ func (s *s3SuiteCustom) TestWalkDirWithEmptyPrefix(c *C) { Size: aws.Int64(27499), }, } - s3API.EXPECT(). + firstCall := s3API.EXPECT(). ListObjectsWithContext(ctx, gomock.Any()). DoAndReturn(func(_ context.Context, input *s3.ListObjectsInput) (*s3.ListObjectsOutput, error) { c.Assert(aws.StringValue(input.Bucket), Equals, "bucket") @@ -940,6 +981,20 @@ func (s *s3SuiteCustom) TestWalkDirWithEmptyPrefix(c *C) { Contents: contents, }, nil }) + s3API.EXPECT(). + ListObjectsWithContext(ctx, gomock.Any()). + DoAndReturn(func(_ context.Context, input *s3.ListObjectsInput) (*s3.ListObjectsOutput, error) { + c.Assert(aws.StringValue(input.Bucket), Equals, "bucket") + c.Assert(aws.StringValue(input.Prefix), Equals, "sp/") + c.Assert(aws.StringValue(input.Marker), Equals, "") + c.Assert(aws.Int64Value(input.MaxKeys), Equals, int64(2)) + c.Assert(aws.StringValue(input.Delimiter), Equals, "") + return &s3.ListObjectsOutput{ + IsTruncated: aws.Bool(false), + Contents: contents[:1], + }, nil + }). + After(firstCall) // Ensure we receive the items in order. i := 0 @@ -956,4 +1011,20 @@ func (s *s3SuiteCustom) TestWalkDirWithEmptyPrefix(c *C) { ) c.Assert(err, IsNil) c.Assert(i, Equals, len(contents)) + + // test with non-empty sub-dir + i = 0 + err = storage.WalkDir( + ctx, + &WalkOption{SubDir: "sp", ListCount: 2}, + func(path string, size int64) error { + comment := Commentf("index = %d", i) + c.Assert(path, Equals, *contents[i].Key, comment) + c.Assert(size, Equals, *contents[i].Size, comment) + i++ + return nil + }, + ) + c.Assert(err, IsNil) + c.Assert(i, Equals, 1) } From aae24bdee8b926fba071ca4276294ff2a7c829ab Mon Sep 17 00:00:00 2001 From: glorv Date: Wed, 24 Feb 2021 20:08:35 +0800 Subject: [PATCH 2/3] fix typo --- pkg/storage/s3_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/storage/s3_test.go b/pkg/storage/s3_test.go index 9db2f716f..1117306c4 100644 --- a/pkg/storage/s3_test.go +++ b/pkg/storage/s3_test.go @@ -881,7 +881,7 @@ func (s *s3Suite) TestWalkDir(c *C) { }, nil }). After(secondCall) - forthCall := s.s3.EXPECT(). + fourthCall := s.s3.EXPECT(). ListObjectsWithContext(ctx, gomock.Any()). DoAndReturn(func(_ context.Context, input *s3.ListObjectsInput) (*s3.ListObjectsOutput, error) { c.Assert(aws.StringValue(input.Bucket), Equals, "bucket") @@ -905,7 +905,7 @@ func (s *s3Suite) TestWalkDir(c *C) { Contents: contents[4:], }, nil }). - After(forthCall) + After(fourthCall) // Ensure we receive the items in order. i := 0 From c9d5a7c379ac183834fadd3e0c8e364a0546a177 Mon Sep 17 00:00:00 2001 From: glorv Date: Thu, 25 Feb 2021 15:36:26 +0800 Subject: [PATCH 3/3] also check suffix when append '/' to s3 prefix --- pkg/storage/s3.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/storage/s3.go b/pkg/storage/s3.go index 8a9cc5555..3b56da137 100644 --- a/pkg/storage/s3.go +++ b/pkg/storage/s3.go @@ -280,7 +280,7 @@ func newS3Storage(backend *backup.S3, opts *ExternalStorageOptions) (*S3Storage, return nil, errors.Annotatef(berrors.ErrStorageInvalidConfig, "Bucket %s is not accessible: %v", qs.Bucket, err) } } - if len(qs.Prefix) > 0 { + if len(qs.Prefix) > 0 && !strings.HasSuffix(qs.Prefix, "/") { qs.Prefix += "/" }