Stop ErrFinishedWalk from escaping from Repositories walk

Signed-off-by: Edgar Lee <edgar.lee@docker.com>

registry/handlers/catalog.go

@@ -9,6 +9,7 @@ import (
 	"strconv"
 
 	"github.com/docker/distribution/registry/api/errcode"
+	"github.com/docker/distribution/registry/storage/driver"
 	"github.com/gorilla/handlers"
 )
 
@@ -45,7 +46,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)
-	if err == io.EOF {
+	_, pathNotFound := err.(driver.PathNotFoundError)
+
+	if err == io.EOF || pathNotFound {
 		moreEntries = false
 	} else if err != nil {
 		ch.Errors = append(ch.Errors, errcode.ErrorCodeUnknown.WithDetail(err))

registry/storage/catalog.go

@@ -10,15 +10,14 @@ import (
 	"github.com/docker/distribution/registry/storage/driver"
 )
 
-// ErrFinishedWalk is used when the called walk function no longer wants
-// to accept any more values.  This is used for pagination when the
-// required number of repos have been found.
-var ErrFinishedWalk = errors.New("finished walk")
+// errFinishedWalk signals an early exit to the walk when the current query
+// is satisfied.
+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 {
@@ -30,26 +29,22 @@ func (reg *registry) Repositories(ctx context.Context, repos []string, last stri
 		return 0, err
 	}
 
+	// errFinishedWalk signals an early exit to the walk when the current query
+	// is satisfied.
+	errFinishedWalk := errors.New("finished walk")
+
 	err = Walk(ctx, reg.blobStore.driver, root, func(fileInfo driver.FileInfo) error {
-		filePath := fileInfo.Path()
-
-		// lop the base path off
-		repoPath := filePath[len(root)+1:]
-
-		_, file := path.Split(repoPath)
-		if file == "_layers" {
-			repoPath = strings.TrimSuffix(repoPath, "/_layers")
-			if repoPath > last {
-				foundRepos = append(foundRepos, repoPath)
-			}
-			return ErrSkipDir
-		} else if strings.HasPrefix(file, "_") {
-			return ErrSkipDir
+		err := handleRepository(fileInfo, root, last, func(repoPath string) error {
+			foundRepos = append(foundRepos, repoPath)
+			return nil
+		})
+		if err != nil {
+			return err
 		}
 
 		// if we've filled our array, no need to walk any further
 		if len(foundRepos) == len(repos) {
-			return ErrFinishedWalk
+			return errFinishedWalk
 		}
 
 		return nil
@@ -57,41 +52,102 @@ 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 {
-		errVal = io.EOF
+	switch err {
+	case nil:
+		// nil means that we completed walk and didn't fill buffer. No more
+		// records are available.
+		err = io.EOF
+	case errFinishedWalk:
+		// more records are available.
+		err = nil
 	}
 
-	return n, errVal
+	return n, err
 }
 
 // Enumerate applies ingester to each repository
 func (reg *registry) Enumerate(ctx context.Context, ingester func(string) error) error {
-	repoNameBuffer := make([]string, 100)
-	var last string
-	for {
-		n, err := reg.Repositories(ctx, repoNameBuffer, last)
-		if err != nil && err != io.EOF {
-			return err
+	root, err := pathFor(repositoriesRootPathSpec{})
+	if err != nil {
+		return err
+	}
+
+	err = Walk(ctx, reg.blobStore.driver, root, func(fileInfo driver.FileInfo) error {
+		return handleRepository(fileInfo, root, "", ingester)
+	})
+
+	return err
+}
+
+// lessPath returns true if one path a is less than path b.
+//
+// A component-wise comparison is done, rather than the lexical comparison of
+// strings.
+func lessPath(a, b string) bool {
+	// we provide this behavior by making separator always sort first.
+	return compareReplaceInline(a, b, '/', '\x00') < 0
+}
+
+// compareReplaceInline modifies runtime.cmpstring to replace old with new
+// during a byte-wise comparison.
+func compareReplaceInline(s1, s2 string, old, new byte) int {
+	l := len(s1)
+	if len(s2) < l {
+		l = len(s2)
+	}
+
+	for i := 0; i < l; i++ {
+		c1, c2 := s1[i], s2[i]
+		if c1 == old {
+			c1 = new
 		}
 
-		if n == 0 {
-			break
+		if c2 == old {
+			c2 = new
 		}
 
-		last = repoNameBuffer[n-1]
-		for i := 0; i < n; i++ {
-			repoName := repoNameBuffer[i]
-			err = ingester(repoName)
-			if err != nil {
+		if c1 < c2 {
+			return -1
+		}
+
+		if c1 > c2 {
+			return +1
+		}
+	}
+
+	if len(s1) < len(s2) {
+		return -1
+	}
+
+	if len(s1) > len(s2) {
+		return +1
+	}
+
+	return 0
+}
+
+// handleRepository calls function fn with a repository path if fileInfo
+// has a path of a repository under root and that it is lexographically
+// after last. Otherwise, it will return ErrSkipDir. This should be used
+// with Walk to do handling with repositories in a storage.
+func handleRepository(fileInfo driver.FileInfo, root, last string, fn func(repoPath string) error) error {
+	filePath := fileInfo.Path()
+
+	// lop the base path off
+	repo := filePath[len(root)+1:]
+
+	_, file := path.Split(repo)
+	if file == "_layers" {
+		repo = strings.TrimSuffix(repo, "/_layers")
+		if lessPath(last, repo) {
+			if err := fn(repo); err != nil {
 				return err
 			}
 		}
-
-		if err == io.EOF {
-			break
-		}
+		return ErrSkipDir
+	} else if strings.HasPrefix(file, "_") {
+		return ErrSkipDir
 	}
-	return nil
 
+	return nil
 }

registry/storage/catalog_test.go

@@ -1,6 +1,7 @@
 package storage
 
 import (
+	"fmt"
 	"io"
 	"testing"
 
@@ -112,7 +113,28 @@ func TestCatalogInParts(t *testing.T) {
 	if !testEq(p, env.expected[chunkLen*2:chunkLen*3-1], numFilled) {
 		t.Errorf("Expected catalog third chunk err")
 	}
+}
 
+func TestCatalogEnumerate(t *testing.T) {
+	env := setupFS(t)
+
+	var repos []string
+	repositoryEnumerator := env.registry.(distribution.RepositoryEnumerator)
+	err := repositoryEnumerator.Enumerate(env.ctx, func(repoName string) error {
+		repos = append(repos, repoName)
+		return nil
+	})
+	if err != nil {
+		t.Errorf("Expected catalog enumerate err")
+	}
+
+	if len(repos) != len(env.expected) {
+		t.Errorf("Expected catalog enumerate doesn't have correct number of values")
+	}
+
+	if !testEq(repos, env.expected, len(env.expected)) {
+		t.Errorf("Expected catalog enumerate not over all values")
+	}
 }
 
 func testEq(a, b []string, size int) bool {
@@ -123,3 +145,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")
+	}
+}