From f78d81e78a20fda60efb4a29848f77d99dd0513b Mon Sep 17 00:00:00 2001 From: James Hewitt Date: Fri, 12 Aug 2022 16:03:19 +0100 Subject: [PATCH 1/5] Remove test as S3 does not support empty directories Signed-off-by: James Hewitt --- registry/storage/driver/s3-aws/s3_test.go | 33 ----------------------- 1 file changed, 33 deletions(-) diff --git a/registry/storage/driver/s3-aws/s3_test.go b/registry/storage/driver/s3-aws/s3_test.go index 0c79a009..e4fe1793 100644 --- a/registry/storage/driver/s3-aws/s3_test.go +++ b/registry/storage/driver/s3-aws/s3_test.go @@ -205,39 +205,6 @@ func TestEmptyRootList(t *testing.T) { } } -// TestWalkEmptySubDirectory assures we list an empty sub directory only once when walking -// through its parent directory. -func TestWalkEmptySubDirectory(t *testing.T) { - if skipS3() != "" { - t.Skip(skipS3()) - } - - drv, err := s3DriverConstructor("", s3.StorageClassStandard) - if err != nil { - t.Fatalf("unexpected error creating rooted driver: %v", err) - } - - // create an empty sub directory. - s3driver := drv.StorageDriver.(*driver) - if _, err := s3driver.S3.PutObject(&s3.PutObjectInput{ - Bucket: aws.String(os.Getenv("S3_BUCKET")), - Key: aws.String("/testdir/emptydir/"), - }); err != nil { - t.Fatalf("error creating empty directory: %s", err) - } - - bucketFiles := []string{} - s3driver.Walk(context.Background(), "/testdir", func(fileInfo storagedriver.FileInfo) error { - bucketFiles = append(bucketFiles, fileInfo.Path()) - return nil - }) - - expected := []string{"/testdir/emptydir"} - if !reflect.DeepEqual(bucketFiles, expected) { - t.Errorf("expecting files %+v, found %+v instead", expected, bucketFiles) - } -} - func TestStorageClass(t *testing.T) { if skipS3() != "" { t.Skip(skipS3()) From 2d316a12d36f2c11f64acc55b64408f4c26347c5 Mon Sep 17 00:00:00 2001 From: James Hewitt Date: Fri, 12 Aug 2022 16:04:05 +0100 Subject: [PATCH 2/5] We don't use gocheck in these tests Signed-off-by: James Hewitt --- registry/storage/driver/s3-aws/s3_test.go | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/registry/storage/driver/s3-aws/s3_test.go b/registry/storage/driver/s3-aws/s3_test.go index e4fe1793..487ba652 100644 --- a/registry/storage/driver/s3-aws/s3_test.go +++ b/registry/storage/driver/s3-aws/s3_test.go @@ -13,8 +13,6 @@ import ( "strings" "testing" - "gopkg.in/check.v1" - "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/s3" @@ -23,13 +21,8 @@ import ( "github.com/distribution/distribution/v3/registry/storage/driver/testsuites" ) -// Hook up gocheck into the "go test" runner. -func Test(t *testing.T) { check.TestingT(t) } - -var ( - s3DriverConstructor func(rootDirectory, storageClass string) (*Driver, error) - skipS3 func() string -) +var s3DriverConstructor func(rootDirectory, storageClass string) (*Driver, error) +var skipS3 func() string func init() { var ( From 6ceb904c3ef529b13e6e9276977cff84be4a3b87 Mon Sep 17 00:00:00 2001 From: James Hewitt Date: Fri, 12 Aug 2022 16:04:52 +0100 Subject: [PATCH 3/5] Don't check returned storage class if we use NONE If we haven't set a storage class there's no point in checking the storage class applied to the object - s3 will choose one. Signed-off-by: James Hewitt --- registry/storage/driver/s3-aws/s3_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/registry/storage/driver/s3-aws/s3_test.go b/registry/storage/driver/s3-aws/s3_test.go index 487ba652..cff8b1be 100644 --- a/registry/storage/driver/s3-aws/s3_test.go +++ b/registry/storage/driver/s3-aws/s3_test.go @@ -229,7 +229,9 @@ func TestStorageClass(t *testing.T) { } defer resp.Body.Close() // Amazon only populates this header value for non-standard storage classes - if storageClass == s3.StorageClassStandard && resp.StorageClass != nil { + if storageClass == noStorageClass { + // We haven't specified a storage class so we can't confirm what it is + } else if storageClass == s3.StorageClassStandard && resp.StorageClass != nil { t.Fatalf( "unexpected response storage class for file with storage class %v: %v", storageClass, From f7bdd9127bc0afc71d7cc0facd440d8c81276232 Mon Sep 17 00:00:00 2001 From: James Hewitt Date: Fri, 12 Aug 2022 16:06:03 +0100 Subject: [PATCH 4/5] Don't test the OUTPOSTS storage class This test will only work on an s3 bucket on an s3 outpost. Most developers won't have access to one of these. Signed-off-by: James Hewitt --- registry/storage/driver/s3-aws/s3_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/registry/storage/driver/s3-aws/s3_test.go b/registry/storage/driver/s3-aws/s3_test.go index cff8b1be..08792f7f 100644 --- a/registry/storage/driver/s3-aws/s3_test.go +++ b/registry/storage/driver/s3-aws/s3_test.go @@ -213,6 +213,11 @@ func TestStorageClass(t *testing.T) { t.Fatalf("unexpected error creating driver with storage class %v: %v", storageClass, err) } + // Can only test outposts if using s3 outposts + if storageClass == s3.StorageClassOutposts { + continue + } + err = s3Driver.PutContent(ctx, filename, contents) if err != nil { t.Fatalf("unexpected error creating content with storage class %v: %v", storageClass, err) From 7622d0a4534886072b17f58754cd1d1924a22d19 Mon Sep 17 00:00:00 2001 From: James Hewitt Date: Fri, 12 Aug 2022 16:08:40 +0100 Subject: [PATCH 5/5] Don't return the from of a walk Other storage drivers will only return children and below, s3 should do the same. The only reason it was returning was because of the addition of a / to ensure we treat the from as a directory. Signed-off-by: James Hewitt --- registry/storage/driver/s3-aws/s3.go | 28 +++++++++++------------ registry/storage/driver/s3-aws/s3_test.go | 1 - 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/registry/storage/driver/s3-aws/s3.go b/registry/storage/driver/s3-aws/s3.go index 8753b345..b83756a7 100644 --- a/registry/storage/driver/s3-aws/s3.go +++ b/registry/storage/driver/s3-aws/s3.go @@ -1039,18 +1039,8 @@ func (d *driver) URLFor(ctx context.Context, path string, options map[string]int // Walk traverses a filesystem defined within driver, starting // from the given path, calling f on each file func (d *driver) Walk(ctx context.Context, from string, f storagedriver.WalkFn) error { - path := from - if !strings.HasSuffix(path, "/") { - path = path + "/" - } - - prefix := "" - if d.s3Path("") == "" { - prefix = "/" - } - var objectCount int64 - if err := d.doWalk(ctx, &objectCount, d.s3Path(path), prefix, f); err != nil { + if err := d.doWalk(ctx, &objectCount, from, f); err != nil { return err } @@ -1062,7 +1052,7 @@ func (d *driver) Walk(ctx context.Context, from string, f storagedriver.WalkFn) return nil } -func (d *driver) doWalk(parentCtx context.Context, objectCount *int64, path, prefix string, f storagedriver.WalkFn) error { +func (d *driver) doWalk(parentCtx context.Context, objectCount *int64, from string, f storagedriver.WalkFn) error { var ( retError error // the most recent directory walked for de-duping @@ -1070,11 +1060,21 @@ func (d *driver) doWalk(parentCtx context.Context, objectCount *int64, path, pre // the most recent skip directory to avoid walking over undesirable files prevSkipDir string ) - prevDir = strings.Replace(path, d.s3Path(""), prefix, 1) + prevDir = from + + path := from + if !strings.HasSuffix(path, "/") { + path = path + "/" + } + + prefix := "" + if d.s3Path("") == "" { + prefix = "/" + } listObjectsInput := &s3.ListObjectsV2Input{ Bucket: aws.String(d.Bucket), - Prefix: aws.String(path), + Prefix: aws.String(d.s3Path(path)), MaxKeys: aws.Int64(listMax), } diff --git a/registry/storage/driver/s3-aws/s3_test.go b/registry/storage/driver/s3-aws/s3_test.go index 08792f7f..0330eafc 100644 --- a/registry/storage/driver/s3-aws/s3_test.go +++ b/registry/storage/driver/s3-aws/s3_test.go @@ -604,7 +604,6 @@ func TestWalk(t *testing.T) { name: "from folder", fn: func(fileInfo storagedriver.FileInfo) error { return nil }, expected: []string{ - "/folder1", "/folder1/file1", }, from: "/folder1",