From 5a0b35ca107abf1b0cbdbdecd7b42a1fc5940bc0 Mon Sep 17 00:00:00 2001 From: Edgar Lee Date: Wed, 13 Jul 2016 16:41:51 -0700 Subject: [PATCH 1/4] Fix storage drivers dropping non EOF errors when listing repositories This fixes errors other than io.EOF from being dropped when a storage driver lists repositories. For example, filesystem driver may point to a missing directory and errors, which then gets subsequently dropped. Signed-off-by: Edgar Lee --- registry/storage/catalog.go | 10 ++++---- registry/storage/catalog_test.go | 40 ++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 5 deletions(-) diff --git a/registry/storage/catalog.go b/registry/storage/catalog.go index 3b13b7ad1..51f316917 100644 --- a/registry/storage/catalog.go +++ b/registry/storage/catalog.go @@ -25,12 +25,12 @@ func (reg *registry) Repositories(ctx context.Context, repos []string, last stri return 0, errors.New("no space in slice") } - root, err := pathFor(repositoriesRootPathSpec{}) - if err != nil { - return 0, err + root, errVal := pathFor(repositoriesRootPathSpec{}) + if errVal != nil { + return 0, errVal } - err = Walk(ctx, reg.blobStore.driver, root, func(fileInfo driver.FileInfo) error { + errVal = Walk(ctx, reg.blobStore.driver, root, func(fileInfo driver.FileInfo) error { filePath := fileInfo.Path() // lop the base path off @@ -58,7 +58,7 @@ func (reg *registry) Repositories(ctx context.Context, repos []string, last stri n = copy(repos, foundRepos) // Signal that we have no more entries by setting EOF - if len(foundRepos) <= len(repos) && err != ErrFinishedWalk { + if len(foundRepos) <= len(repos) && (errVal == nil || errVal == ErrSkipDir) { errVal = io.EOF } diff --git a/registry/storage/catalog_test.go b/registry/storage/catalog_test.go index eb062c5b7..40fa909d7 100644 --- a/registry/storage/catalog_test.go +++ b/registry/storage/catalog_test.go @@ -1,6 +1,7 @@ package storage import ( + "fmt" "io" "testing" @@ -123,3 +124,42 @@ func testEq(a, b []string, size int) bool { } return true } + +func setupBadWalkEnv(t *testing.T) *setupEnv { + d := newBadListDriver() + ctx := context.Background() + registry, err := NewRegistry(ctx, d, BlobDescriptorCacheProvider(memory.NewInMemoryBlobDescriptorCacheProvider()), EnableRedirect) + if err != nil { + t.Fatalf("error creating registry: %v", err) + } + + return &setupEnv{ + ctx: ctx, + driver: d, + registry: registry, + } +} + +type badListDriver struct { + driver.StorageDriver +} + +var _ driver.StorageDriver = &badListDriver{} + +func newBadListDriver() *badListDriver { + return &badListDriver{StorageDriver: inmemory.New()} +} + +func (d *badListDriver) List(ctx context.Context, path string) ([]string, error) { + return nil, fmt.Errorf("List error") +} + +func TestCatalogWalkError(t *testing.T) { + env := setupBadWalkEnv(t) + p := make([]string, 1) + + _, err := env.registry.Repositories(env.ctx, p, "") + if err == io.EOF { + t.Errorf("Expected catalog driver list error") + } +} From aeb9a29499ed2f685ff9bd68c0849611b5d0c087 Mon Sep 17 00:00:00 2001 From: Edgar Lee Date: Thu, 14 Jul 2016 13:24:16 -0700 Subject: [PATCH 2/4] Handle new errors returned from catalog repository listing Signed-off-by: Edgar Lee --- registry/handlers/catalog.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/registry/handlers/catalog.go b/registry/handlers/catalog.go index 6ec1fe550..083ebfd08 100644 --- a/registry/handlers/catalog.go +++ b/registry/handlers/catalog.go @@ -6,9 +6,12 @@ import ( "io" "net/http" "net/url" + "reflect" "strconv" "github.com/docker/distribution/registry/api/errcode" + "github.com/docker/distribution/registry/storage" + "github.com/docker/distribution/registry/storage/driver" "github.com/gorilla/handlers" ) @@ -45,9 +48,10 @@ func (ch *catalogHandler) GetCatalog(w http.ResponseWriter, r *http.Request) { repos := make([]string, maxEntries) filled, err := ch.App.registry.Repositories(ch.Context, repos, lastEntry) - if err == io.EOF { + + if err == io.EOF || reflect.TypeOf(err) == reflect.TypeOf(driver.PathNotFoundError{}) { moreEntries = false - } else if err != nil { + } else if err != nil && err != storage.ErrFinishedWalk { ch.Errors = append(ch.Errors, errcode.ErrorCodeUnknown.WithDetail(err)) return } From 3bfd03cbe62b4cb76e36617593561c735f273fa3 Mon Sep 17 00:00:00 2001 From: Edgar Lee Date: Thu, 14 Jul 2016 13:28:08 -0700 Subject: [PATCH 3/4] Refactor errVal named parameter for catalog repositories to err Signed-off-by: Edgar Lee --- registry/storage/catalog.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/registry/storage/catalog.go b/registry/storage/catalog.go index 51f316917..aec5f2e66 100644 --- a/registry/storage/catalog.go +++ b/registry/storage/catalog.go @@ -18,19 +18,19 @@ var ErrFinishedWalk = errors.New("finished walk") // Returns a list, or partial list, of repositories in the registry. // Because it's a quite expensive operation, it should only be used when building up // an initial set of repositories. -func (reg *registry) Repositories(ctx context.Context, repos []string, last string) (n int, errVal error) { +func (reg *registry) Repositories(ctx context.Context, repos []string, last string) (n int, err error) { var foundRepos []string if len(repos) == 0 { return 0, errors.New("no space in slice") } - root, errVal := pathFor(repositoriesRootPathSpec{}) - if errVal != nil { - return 0, errVal + root, err := pathFor(repositoriesRootPathSpec{}) + if err != nil { + return 0, err } - errVal = Walk(ctx, reg.blobStore.driver, root, func(fileInfo driver.FileInfo) error { + err = Walk(ctx, reg.blobStore.driver, root, func(fileInfo driver.FileInfo) error { filePath := fileInfo.Path() // lop the base path off @@ -58,11 +58,11 @@ func (reg *registry) Repositories(ctx context.Context, repos []string, last stri n = copy(repos, foundRepos) // Signal that we have no more entries by setting EOF - if len(foundRepos) <= len(repos) && (errVal == nil || errVal == ErrSkipDir) { - errVal = io.EOF + if len(foundRepos) <= len(repos) && (err == nil || err == ErrSkipDir) { + err = io.EOF } - return n, errVal + return n, err } // Enumerate applies ingester to each repository From a82f661ef07059084adfae541333eafa2ba0388f Mon Sep 17 00:00:00 2001 From: Edgar Lee Date: Thu, 14 Jul 2016 15:03:18 -0700 Subject: [PATCH 4/4] Use typecast over reflect for error type checking Signed-off-by: Edgar Lee --- registry/handlers/catalog.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/registry/handlers/catalog.go b/registry/handlers/catalog.go index 083ebfd08..4e95bfb08 100644 --- a/registry/handlers/catalog.go +++ b/registry/handlers/catalog.go @@ -6,7 +6,6 @@ import ( "io" "net/http" "net/url" - "reflect" "strconv" "github.com/docker/distribution/registry/api/errcode" @@ -48,8 +47,9 @@ func (ch *catalogHandler) GetCatalog(w http.ResponseWriter, r *http.Request) { repos := make([]string, maxEntries) filled, err := ch.App.registry.Repositories(ch.Context, repos, lastEntry) + _, pathNotFound := err.(driver.PathNotFoundError) - if err == io.EOF || reflect.TypeOf(err) == reflect.TypeOf(driver.PathNotFoundError{}) { + if err == io.EOF || pathNotFound { moreEntries = false } else if err != nil && err != storage.ErrFinishedWalk { ch.Errors = append(ch.Errors, errcode.ErrorCodeUnknown.WithDetail(err))