From 05a258e711f82ff2671bdcfccbbc5bdb1ea4465b Mon Sep 17 00:00:00 2001 From: Collin Shoop Date: Wed, 11 Aug 2021 12:16:43 -0400 Subject: [PATCH 1/6] storagedriver/s3: Added Delete tests to s3_test (cherry picked from commit 1e3b6b67a8e6d7f01307518370f0731212935d05) Signed-off-by: Collin Shoop --- registry/storage/driver/s3-aws/s3_test.go | 273 +++++++++++++++++++++- 1 file changed, 272 insertions(+), 1 deletion(-) diff --git a/registry/storage/driver/s3-aws/s3_test.go b/registry/storage/driver/s3-aws/s3_test.go index 30bd4c97e..eb1bbdfdb 100644 --- a/registry/storage/driver/s3-aws/s3_test.go +++ b/registry/storage/driver/s3-aws/s3_test.go @@ -2,11 +2,12 @@ package s3 import ( "bytes" + "fmt" "io/ioutil" "math/rand" "os" - "reflect" "strconv" + "strings" "testing" "gopkg.in/check.v1" @@ -271,6 +272,276 @@ func TestStorageClass(t *testing.T) { } +func TestPopulate(t *testing.T) { + + if skipS3() != "" { + t.Skip(skipS3()) + } + + rootDir, err := ioutil.TempDir("", "driver-") + if err != nil { + t.Fatalf("unexpected error creating temporary directory: %v", err) + } + defer os.Remove(rootDir) + + driver, err := s3DriverConstructor(rootDir, s3.StorageClassStandard) + if err != nil { + t.Fatalf("unexpected error creating driver with standard storage: %v", err) + } + + var objs = []string{ + "/file1", + "/file1-2", + "/file1/2", + "/folder1/file1", + "/folder2/file1", + "/folder3/file1", + "/folder3/subfolder1/subfolder1/file1", + "/folder3/subfolder2/subfolder1/file1", + "/folder4/file1", + "/folder1-v2/file1", + "/folder1-v2/subfolder1/file1", + } + + init := func() []string { + // init file structure matching objs above + var created []string + for _, path := range objs { + err := driver.PutContent(context.Background(), path, []byte("content "+path)) + if err != nil { + fmt.Printf("unable to init file %s: %s\n", path, err) + continue + } + created = append(created, path) + } + return created + } + init() +} + +func TestDelete(t *testing.T) { + if skipS3() != "" { + t.Skip(skipS3()) + } + + rootDir, err := ioutil.TempDir("", "driver-") + if err != nil { + t.Fatalf("unexpected error creating temporary directory: %v", err) + } + defer os.Remove(rootDir) + + driver, err := s3DriverConstructor(rootDir, s3.StorageClassStandard) + if err != nil { + t.Fatalf("unexpected error creating driver with standard storage: %v", err) + } + + var objs = []string{ + "/file1", + "/file1-2", + "/file1/2", + "/folder1/file1", + "/folder2/file1", + "/folder3/file1", + "/folder3/subfolder1/subfolder1/file1", + "/folder3/subfolder2/subfolder1/file1", + "/folder4/file1", + "/folder1-v2/file1", + "/folder1-v2/subfolder1/file1", + } + // objects to skip auto-created test case + var skipCase = map[string]bool{ + // special case where deleting "/file1" also deletes "/file1/2" is tested explicitly + "/file1": true, + } + + type errFn func(error) bool + type testCase struct { + name string + delete string + expected []string + // error validation function + err errFn + } + + errPathNotFound := func(err error) bool { + if err == nil { + return false + } + switch err.(type) { + case storagedriver.PathNotFoundError: + return true + } + return false + } + errInvalidPath := func(err error) bool { + if err == nil { + return false + } + switch err.(type) { + case storagedriver.InvalidPathError: + return true + } + return false + } + + tcs := []testCase{ + { + // special case where a given path is a file and has subpaths + name: "delete file1", + delete: "/file1", + expected: []string{ + "/file1", + "/file1/2", + }, + }, + { + name: "delete folder1", + delete: "/folder1", + expected: []string{ + "/folder1/file1", + }, + }, + { + name: "delete folder2", + delete: "/folder2", + expected: []string{ + "/folder2/file1", + }, + }, + { + name: "delete folder3", + delete: "/folder3", + expected: []string{ + "/folder3/file1", + "/folder3/subfolder1/subfolder1/file1", + "/folder3/subfolder2/subfolder1/file1", + }, + }, + { + name: "delete path that doesn't exist", + delete: "/path/does/not/exist", + expected: []string{}, + err: errPathNotFound, + }, + { + name: "delete path invalid: trailing slash", + delete: "/path/is/invalid/", + expected: []string{}, + err: errInvalidPath, + }, + { + name: "delete path invalid: trailing special character", + delete: "/path/is/invalid*", + expected: []string{}, + err: errInvalidPath, + }, + } + + // init a test case for each file + for _, path := range objs { + if skipCase[path] { + continue + } + tcs = append(tcs, testCase{ + name: fmt.Sprintf("delete path:'%s'", path), + delete: path, + expected: []string{path}, + }) + } + + init := func() []string { + // init file structure matching objs + var created []string + for _, path := range objs { + err := driver.PutContent(context.Background(), path, []byte("content "+path)) + if err != nil { + fmt.Printf("unable to init file %s: %s\n", path, err) + continue + } + created = append(created, path) + } + return created + } + + cleanup := func(objs []string) { + var lastErr error + for _, path := range objs { + err := driver.Delete(context.Background(), path) + if err != nil { + switch err.(type) { + case storagedriver.PathNotFoundError: + continue + } + lastErr = err + } + } + if lastErr != nil { + t.Fatalf("cleanup failed: %s", lastErr) + } + } + + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + objs := init() + defer cleanup(objs) + + err := driver.Delete(context.Background(), tc.delete) + + if tc.err != nil { + if err == nil { + t.Fatalf("expected error") + } + if !tc.err(err) { + t.Fatalf("error does not match expected: %s", err) + } + } + if tc.err == nil && err != nil { + t.Fatalf("unexpected error: %s", err) + } + + var issues []string + + // validate all files expected to be deleted are deleted + // and all files not marked for deletion still remain + expected := tc.expected + isExpected := func(path string) bool { + for _, epath := range expected { + if epath == path { + return true + } + } + return false + } + for _, path := range objs { + stat, err := driver.Stat(context.Background(), path) + if err != nil { + switch err.(type) { + case storagedriver.PathNotFoundError: + if !isExpected(path) { + issues = append(issues, fmt.Sprintf("unexpected path was deleted: %s", path)) + } + // path was deleted & was supposed to be + continue + } + t.Fatalf("stat: %s", err) + } + if stat.IsDir() { + // for special cases where an object path has subpaths (eg /file1) + // once /file1 is deleted it's now a directory according to stat + continue + } + if isExpected(path) { + issues = append(issues, fmt.Sprintf("expected path was not deleted: %s", path)) + } + } + + if len(issues) > 0 { + t.Fatalf(strings.Join(issues, "; ")) + } + }) + } +} + func TestOverThousandBlobs(t *testing.T) { if skipS3() != "" { t.Skip(skipS3()) From 03f9eb3a18bb57bb0041c11f1f6c2835d06b6ac2 Mon Sep 17 00:00:00 2001 From: Collin Shoop Date: Wed, 11 Aug 2021 12:17:24 -0400 Subject: [PATCH 2/6] storagedriver/s3: Fixed a Delete noop edgecase Delete was not working when the subpath immediately followed the given path started with an ascii lower than "/" such as dash "-" and underscore "_" and requests no files to be deleted. (cherry picked from commit 5d8fa0ce94b68cce70237805db92cdd8d40de282) Signed-off-by: Collin Shoop --- registry/storage/driver/s3-aws/s3.go | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/registry/storage/driver/s3-aws/s3.go b/registry/storage/driver/s3-aws/s3.go index 6ced69fcc..0fb895f68 100644 --- a/registry/storage/driver/s3-aws/s3.go +++ b/registry/storage/driver/s3-aws/s3.go @@ -806,7 +806,21 @@ func min(a, b int) int { // We must be careful since S3 does not guarantee read after delete consistency func (d *driver) Delete(ctx context.Context, path string) error { s3Objects := make([]*s3.ObjectIdentifier, 0, listMax) - s3Path := d.s3Path(path) + + // manually add the given path if it's a file + stat, err := d.Stat(ctx, path) + if err != nil { + return err + } + if stat != nil && !stat.IsDir() { + path := d.s3Path(path) + s3Objects = append(s3Objects, &s3.ObjectIdentifier{ + Key: &path, + }) + } + + // list objects under the given path as a subpath (suffix with slash "/") + s3Path := d.s3Path(path) + "/" listObjectsInput := &s3.ListObjectsInput{ Bucket: aws.String(d.Bucket), Prefix: aws.String(s3Path), @@ -820,14 +834,10 @@ ListLoop: // if there were no more results to return after the first call, resp.IsTruncated would have been false // and the loop would be exited without recalling ListObjects if err != nil || len(resp.Contents) == 0 { - return storagedriver.PathNotFoundError{Path: path} + break ListLoop } for _, key := range resp.Contents { - // Stop if we encounter a key that is not a subpath (so that deleting "/a" does not delete "/ab"). - if len(*key.Key) > len(s3Path) && (*key.Key)[len(s3Path)] != '/' { - break ListLoop - } s3Objects = append(s3Objects, &s3.ObjectIdentifier{ Key: key.Key, }) @@ -843,8 +853,12 @@ ListLoop: } } - // need to chunk objects into groups of 1000 per s3 restrictions total := len(s3Objects) + if total == 0 { + return storagedriver.PathNotFoundError{Path: path} + } + + // need to chunk objects into groups of 1000 per s3 restrictions for i := 0; i < total; i += 1000 { _, err := d.S3.DeleteObjects(&s3.DeleteObjectsInput{ Bucket: aws.String(d.Bucket), From 6da7217b99e64022cecfd003bc8e0e7c2531c057 Mon Sep 17 00:00:00 2001 From: Collin Shoop Date: Wed, 11 Aug 2021 13:00:06 -0400 Subject: [PATCH 3/6] storagedriver/s3: Optimize s3 Delete test cleanup. (cherry picked from commit e4af4dc3a6da6da724e7cff18cf5b6da6ef2a3fd) Signed-off-by: Collin Shoop --- registry/storage/driver/s3-aws/s3_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/registry/storage/driver/s3-aws/s3_test.go b/registry/storage/driver/s3-aws/s3_test.go index eb1bbdfdb..2e9b55553 100644 --- a/registry/storage/driver/s3-aws/s3_test.go +++ b/registry/storage/driver/s3-aws/s3_test.go @@ -479,11 +479,11 @@ func TestDelete(t *testing.T) { t.Fatalf("cleanup failed: %s", lastErr) } } + defer cleanup(objs) for _, tc := range tcs { t.Run(tc.name, func(t *testing.T) { objs := init() - defer cleanup(objs) err := driver.Delete(context.Background(), tc.delete) From dc5b77101d8f62271b0bab0e254a39b29dcb8373 Mon Sep 17 00:00:00 2001 From: Collin Shoop Date: Wed, 11 Aug 2021 14:44:43 -0400 Subject: [PATCH 4/6] storagedriver/s3: Cleaning up tests (cherry picked from commit 483ba26165ca66bcf18a1eaadf41ebe4d3bd5f85) Signed-off-by: Collin Shoop --- registry/storage/driver/s3-aws/s3_test.go | 42 +++++++++++------------ 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/registry/storage/driver/s3-aws/s3_test.go b/registry/storage/driver/s3-aws/s3_test.go index 2e9b55553..dc5510e2a 100644 --- a/registry/storage/driver/s3-aws/s3_test.go +++ b/registry/storage/driver/s3-aws/s3_test.go @@ -335,25 +335,6 @@ func TestDelete(t *testing.T) { t.Fatalf("unexpected error creating driver with standard storage: %v", err) } - var objs = []string{ - "/file1", - "/file1-2", - "/file1/2", - "/folder1/file1", - "/folder2/file1", - "/folder3/file1", - "/folder3/subfolder1/subfolder1/file1", - "/folder3/subfolder2/subfolder1/file1", - "/folder4/file1", - "/folder1-v2/file1", - "/folder1-v2/subfolder1/file1", - } - // objects to skip auto-created test case - var skipCase = map[string]bool{ - // special case where deleting "/file1" also deletes "/file1/2" is tested explicitly - "/file1": true, - } - type errFn func(error) bool type testCase struct { name string @@ -384,6 +365,20 @@ func TestDelete(t *testing.T) { return false } + var objs = []string{ + "/file1", + "/file1-2", + "/file1/2", + "/folder1/file1", + "/folder2/file1", + "/folder3/file1", + "/folder3/subfolder1/subfolder1/file1", + "/folder3/subfolder2/subfolder1/file1", + "/folder4/file1", + "/folder1-v2/file1", + "/folder1-v2/subfolder1/file1", + } + tcs := []testCase{ { // special case where a given path is a file and has subpaths @@ -437,7 +432,12 @@ func TestDelete(t *testing.T) { }, } - // init a test case for each file + // objects to skip auto-created test case + var skipCase = map[string]bool{ + // special case where deleting "/file1" also deletes "/file1/2" is tested explicitly + "/file1": true, + } + // create a test case for each file for _, path := range objs { if skipCase[path] { continue @@ -536,7 +536,7 @@ func TestDelete(t *testing.T) { } if len(issues) > 0 { - t.Fatalf(strings.Join(issues, "; ")) + t.Fatalf(strings.Join(issues, "; \n\t")) } }) } From e625bc7160c0969156b2e8d31cfb7b1f747034df Mon Sep 17 00:00:00 2001 From: Collin Shoop Date: Wed, 11 Aug 2021 16:18:14 -0400 Subject: [PATCH 5/6] storagedriver/s3: Removed temporary S3 test (cherry picked from commit ce80e98cea1d15aa2a2ab931c8b9a1161fc6e218) Signed-off-by: Collin Shoop --- registry/storage/driver/s3-aws/s3_test.go | 47 ----------------------- 1 file changed, 47 deletions(-) diff --git a/registry/storage/driver/s3-aws/s3_test.go b/registry/storage/driver/s3-aws/s3_test.go index dc5510e2a..7e987a2f3 100644 --- a/registry/storage/driver/s3-aws/s3_test.go +++ b/registry/storage/driver/s3-aws/s3_test.go @@ -272,53 +272,6 @@ func TestStorageClass(t *testing.T) { } -func TestPopulate(t *testing.T) { - - if skipS3() != "" { - t.Skip(skipS3()) - } - - rootDir, err := ioutil.TempDir("", "driver-") - if err != nil { - t.Fatalf("unexpected error creating temporary directory: %v", err) - } - defer os.Remove(rootDir) - - driver, err := s3DriverConstructor(rootDir, s3.StorageClassStandard) - if err != nil { - t.Fatalf("unexpected error creating driver with standard storage: %v", err) - } - - var objs = []string{ - "/file1", - "/file1-2", - "/file1/2", - "/folder1/file1", - "/folder2/file1", - "/folder3/file1", - "/folder3/subfolder1/subfolder1/file1", - "/folder3/subfolder2/subfolder1/file1", - "/folder4/file1", - "/folder1-v2/file1", - "/folder1-v2/subfolder1/file1", - } - - init := func() []string { - // init file structure matching objs above - var created []string - for _, path := range objs { - err := driver.PutContent(context.Background(), path, []byte("content "+path)) - if err != nil { - fmt.Printf("unable to init file %s: %s\n", path, err) - continue - } - created = append(created, path) - } - return created - } - init() -} - func TestDelete(t *testing.T) { if skipS3() != "" { t.Skip(skipS3()) From 9e873f31ec46f975a9170df9e59f1a4c03dbfb5b Mon Sep 17 00:00:00 2001 From: Collin Shoop Date: Thu, 12 Aug 2021 10:54:11 -0400 Subject: [PATCH 6/6] storagedriver/s3: Adding back missing import. Signed-off-by: Collin Shoop --- registry/storage/driver/s3-aws/s3_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/registry/storage/driver/s3-aws/s3_test.go b/registry/storage/driver/s3-aws/s3_test.go index 7e987a2f3..f0b257ca4 100644 --- a/registry/storage/driver/s3-aws/s3_test.go +++ b/registry/storage/driver/s3-aws/s3_test.go @@ -6,6 +6,7 @@ import ( "io/ioutil" "math/rand" "os" + "reflect" "strconv" "strings" "testing"