Repository: google/go-containerregistry
Pull request #607 (2019-11-10 12:19:22)

Use Link header for pagination
* Adds support for pagination on /v2/repo/tags/list
* Apply same logic for catalog /v2/_catalog

Instead of setting the url parameters `n` and `last` manually the logic only relies on the `Link` header as described in https://docs.docker.com/registry/spec/api/#pagination.

#549 

codecov-io: # [Codecov](https://codecov.io/gh/google/go-containerregistry/pull/607?src=pr&el=h1) Report
> Merging [#607](https://codecov.io/gh/google/go-containerregistry/pull/607?src=pr&el=desc) into [master](https://codecov.io/gh/google/go-containerregistry/commit/79629ba8e9a15229f6c404644ce37ddce30414a7?src=pr&el=desc) will **decrease** coverage by `0.55%`.
> The diff coverage is `22.85%`.

[![Impacted file tree graph](https://codecov.io/gh/google/go-containerregistry/pull/607/graphs/tree.svg?width=650&token=wG7iiDCdZ4&height=150&src=pr)](https://codecov.io/gh/google/go-containerregistry/pull/607?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #607      +/-   ##
==========================================
- Coverage   72.85%   72.29%   -0.56%     
==========================================
  Files         102      102              
  Lines        4487     4534      +47     
==========================================
+ Hits         3269     3278       +9     
- Misses        807      843      +36     
- Partials      411      413       +2
```


| [Impacted Files](https://codecov.io/gh/google/go-containerregistry/pull/607?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/v1/remote/catalog.go](https://codecov.io/gh/google/go-containerregistry/pull/607/diff?src=pr&el=tree#diff-cGtnL3YxL3JlbW90ZS9jYXRhbG9nLmdv) | `28.81% <0%> (-39.19%)` | :arrow_down: |
| [pkg/crane/catalog.go](https://codecov.io/gh/google/go-containerregistry/pull/607/diff?src=pr&el=tree#diff-cGtnL2NyYW5lL2NhdGFsb2cuZ28=) | `0% <0%> (ø)` | :arrow_up: |
| [pkg/v1/remote/list.go](https://codecov.io/gh/google/go-containerregistry/pull/607/diff?src=pr&el=tree#diff-cGtnL3YxL3JlbW90ZS9saXN0Lmdv) | `51.06% <45.71%> (-14.16%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/google/go-containerregistry/pull/607?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/google/go-containerregistry/pull/607?src=pr&el=footer). Last update [79629ba...e8e774a](https://codecov.io/gh/google/go-containerregistry/pull/607?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

jonjohnsonjr: Thanks for the PR!

I'm not sure that we want exactly this signature for `remote.Catalog`, since it ends up buffering all the results before returning to the caller, see the discussion from this PR: https://github.com/google/go-containerregistry/pull/548

I think we'll want to do something with channels or maybe even a callback function instead, but it's unclear to me what the best approach is. I'm leaning towards a signature like this:

```go
func Catalog(ctx context.Context, target name.Registry, options ...Option) (<-chan string, <-chan error)
```

Some benefits:
1. You can cancel the catalog iteration via the context.
2. You get streaming results, so you don't have to wait for everything to buffer.
3. You can ignore individual errors and keep going instead of failing all or nothing.
dsbrng25b: I added support for passing a context.Context to the list actions.
Before it works correctly the retry mechanism in the RoundTripper has to be changed/fixed because currently it does not respect the context of the http.Request.

Further I set the page size (query parameter `n`) to 10'000. This greatly improves the performance if you list catalog which contains a lot of images. I tested this with the `registry:2` with over 6000 images. If you don't set a page size the registry responds with 100 images per page and it takes over 5 seconds to return all images. With `n` set to 10000 it takes less than 0.3s.
```
$ time ./crane catalog registry.local | wc -l    
6023    
    
real    0m5.315s    
user    0m0.103s    
sys     0m0.015s    
$ git checkout pagination                        
Switched to branch 'pagination'    
$ go build                       
$ time ./crane catalog registry.local | wc -l                       
6023    
    
real    0m0.247s    
user    0m0.070s    
sys     0m0.032s 
```

Concerning the return value I would stay with `([]string, error)`.  In my opinion you either want the whole list or nothing. Also I can't think of a use case where the return time is very important. Or what use cases do you think of where you need to have control over these things?
I would publish Catalog() and List() with ([]string, error) and if people need more control over it introduce further functions later.
dsbrng25b: I changed the IsTemporary function to always return false if the error is a DeadlineExceeded or a Canceled error. Now the context should work correctly in List and Catalog.
jonjohnsonjr: > In my opinion you either want the whole list or nothing. Also I can't think of a use case where the return time is very important. Or what use cases do you think of where you need to have control over these things?

Specifically for me, I can't use catalog with this interface, so it will be hard to convince me otherwise :) This is partially why we're [removing catalog](https://github.com/opencontainers/distribution-spec/pull/69) from the OCI spec, but to explain:

All registry operations are scoped to a namespace except for catalog. Catalog is specifically `<host>/v2/_catalog`. This is important for later.

There are two major ways most people segment registries into namespaces: by domain or by path. DockerHub and GCR both use path, whereas ACR and ECR (I think) segment by domain. For example, I have a registry `gcr.io/jonjohnson`, but for ACR it would be something like `jonjohnson.azurecr.io`.

For ACR, I agree with you, `GET jonjohnson.azurecr.io/v2/_catalog` would return everything in that single registry, and partial results don't make any sense.

For GCR, things get a bit tricky. The namespace here (jonjohnson) actually maps to a GCP project. Ideally, a catalog operation would be something like `GET gcr.io/v2/jonjohnson/_catalog`, but that's _not_ how catalog works. You have to `GET gcr.io/v2/_catalog`, which presents a problem. My project ID is dropped from the path, so there's no way map back to my registry.

How does GCR solve this? They list every project I'm a member of, and return results for all of them. This presents some problems:

1. As a google employee, I'm a member of a lot of projects. Trying to finish a catalog operation would probably take me hours. I actually can't control which projects I'm a part of, because others can add me to their projects without my knowledge or consent.
2. For a large majority of those projects, I don't have read access to GCR, so when I try to actually list repositories in that project, it will fail.

I can mostly work around this by using service accounts that only have access to certain projects, but folks could still add that service account to their project and break my catalog listing without my knowledge :( so... being able to ignore errors here is really important for me, and anyone else that uses GCR.

That being said... I do agree with your point that returning ([]string, error) is much easier to use. My hope here was that pkg/v1/remote would expose as much useful complexity as we could (within reason), whereas pkg/crane would be a really simple to use package that covers most use cases. If you needed more than that, you'd drop down into remote and deal with the unwieldy API of reality.

> Further I set the page size (query parameter n) to 10'000. This greatly improves the performance if you list catalog which contains a lot of images.

👍 

> I changed the IsTemporary function to always return false if the error is a DeadlineExceeded or a Canceled error. Now the context should work correctly in List and Catalog.

`context.DeadlineExceeded` makes sense but I don't think `context.Canceled` implements `Temporary`, does it?
dsbrng25b: My main goal was to fix the List function since it did not return all tags. At the same time I changed Catalog that it uses the same logic as List. I see that there are some difficulties with _catalog on GCR or in general on container registries with a lot of images, but I don't think that these problems can be fixed by revealing more implementation details. Most likely you have to use a solution which is specific to your container registry? Or which specific details would you like to control Catalog?

> `context.DeadlineExceeded` makes sense but I don't think `context.Canceled` implements `Temporary`, does it?
I'm not sure what you mean? Neither does implement Temporary but if it is a DeadlineExceeded or a Canceled error it is never temporary. Once a context gets canceled or it times out it stays that way.


dsbrng25b: @jonjohnsonjr How can we get ahead here? I would like to see the fixed List function merged if possible. I could remove the changes in the catalog part that  it can be discussed later or even removed in a v2 since _catalog will be removed from the oci spec either way. What do you think?
jonjohnsonjr: >I'm not sure what you mean? Neither does implement Temporary but if it is a DeadlineExceeded or a Canceled error it is never temporary.

`context.DeadlineExceeded` implements `Temporary` [here](https://github.com/golang/go/blob/0ad368675bae1e3228c9146e092cd00cfb29ac27/src/context/context.go#L167), which means it's necessary to ignore it (assuming we don't want to retry it, which I'm not so sure about), but `context.Canceled` [doesn't (currently) implement `Temporary`](https://github.com/golang/go/blob/0ad368675bae1e3228c9146e092cd00cfb29ac27/src/context/context.go#L156-L157), so we don't need to specifically check it, since it will fail [this typecheck](https://github.com/google/go-containerregistry/blob/d448c55e6b46c1d58522a2e4ef0d5a4efda9f28c/pkg/internal/retry/retry.go#L35-L37).
jonjohnsonjr: Sorry for the delay, I missed that comment while OOO and this PR got lost in a nearly infinite sea of tabs.
dsbrng25b: Thank you for reviewing.
I rebased the branch, fixed the typos, removed context.Canceled from the IsTemporary check and added a test for the retry transport.

Patch:
diff --git a/pkg/crane/catalog.go b/pkg/crane/catalog.go
index 6df30459..c08cb094 100644
--- a/pkg/crane/catalog.go
+++ b/pkg/crane/catalog.go
@@ -15,6 +15,8 @@
 package crane
 
 import (
+	"context"
+
 	"github.com/google/go-containerregistry/pkg/authn"
 	"github.com/google/go-containerregistry/pkg/name"
 	"github.com/google/go-containerregistry/pkg/v1/remote"
@@ -27,23 +29,5 @@ func Catalog(src string) (res []string, err error) {
 		return nil, err
 	}
 
-	n := 100
-	last := ""
-	for {
-		page, err := remote.CatalogPage(reg, last, n, remote.WithAuthFromKeychain(authn.DefaultKeychain))
-		if err != nil {
-			return nil, err
-		}
-
-		if len(page) > 0 {
-			last = page[len(page)-1]
-			res = append(res, page...)
-		}
-
-		if len(page) < n {
-			break
-		}
-	}
-
-	return res, nil
+	return remote.Catalog(context.TODO(), reg, remote.WithAuthFromKeychain(authn.DefaultKeychain))
 }
diff --git a/pkg/internal/retry/retry.go b/pkg/internal/retry/retry.go
index b4151088..542ba49b 100644
--- a/pkg/internal/retry/retry.go
+++ b/pkg/internal/retry/retry.go
@@ -17,6 +17,7 @@
 package retry
 
 import (
+	"context"
 	"fmt"
 
 	"github.com/google/go-containerregistry/pkg/internal/retry/wait"
@@ -35,6 +36,9 @@ type temporary interface {
 
 // IsTemporary returns true if err implements Temporary() and it returns true.
 func IsTemporary(err error) bool {
+	if err == context.DeadlineExceeded {
+		return false
+	}
 	if te, ok := err.(temporary); ok && te.Temporary() {
 		return true
 	}
diff --git a/pkg/v1/remote/catalog.go b/pkg/v1/remote/catalog.go
index da9c7353..0923a9de 100644
--- a/pkg/v1/remote/catalog.go
+++ b/pkg/v1/remote/catalog.go
@@ -15,6 +15,7 @@
 package remote
 
 import (
+	"context"
 	"encoding/json"
 	"fmt"
 	"net/http"
@@ -68,3 +69,74 @@ func CatalogPage(target name.Registry, last string, n int, options ...Option) ([
 
 	return parsed.Repos, nil
 }
+
+// Catalog calls /_catalog, returning the list of repositories on the registry.
+func Catalog(ctx context.Context, target name.Registry, options ...Option) ([]string, error) {
+	o, err := makeOptions(target, options...)
+	if err != nil {
+		return nil, err
+	}
+
+	scopes := []string{target.Scope(transport.PullScope)}
+	tr, err := transport.New(target, o.auth, o.transport, scopes)
+	if err != nil {
+		return nil, err
+	}
+
+	uri := &url.URL{
+		Scheme:   target.Scheme(),
+		Host:     target.RegistryStr(),
+		Path:     "/v2/_catalog",
+		RawQuery: "n=10000",
+	}
+
+	client := http.Client{Transport: tr}
+
+	var (
+		parsed   catalog
+		repoList []string
+	)
+
+	// get responses until there is no next page
+	for {
+		select {
+		case <-ctx.Done():
+			return nil, ctx.Err()
+		default:
+		}
+
+		req, err := http.NewRequest("GET", uri.String(), nil)
+		if err != nil {
+			return nil, err
+		}
+		req = req.WithContext(ctx)
+
+		resp, err := client.Do(req)
+		if err != nil {
+			return nil, err
+		}
+
+		if err := transport.CheckError(resp, http.StatusOK); err != nil {
+			return nil, err
+		}
+
+		if err := json.NewDecoder(resp.Body).Decode(&parsed); err != nil {
+			return nil, err
+		}
+		if err := resp.Body.Close(); err != nil {
+			return nil, err
+		}
+
+		repoList = append(repoList, parsed.Repos...)
+
+		uri, err = getNextPageURL(resp)
+		if err != nil {
+			return nil, err
+		}
+		// no next page
+		if uri == nil {
+			break
+		}
+	}
+	return repoList, nil
+}
diff --git a/pkg/v1/remote/list.go b/pkg/v1/remote/list.go
index bf50a797..32b2a8ff 100644
--- a/pkg/v1/remote/list.go
+++ b/pkg/v1/remote/list.go
@@ -15,10 +15,12 @@
 package remote
 
 import (
+	"context"
 	"encoding/json"
 	"fmt"
 	"net/http"
 	"net/url"
+	"strings"
 
 	"github.com/google/go-containerregistry/pkg/name"
 	"github.com/google/go-containerregistry/pkg/v1/remote/transport"
@@ -29,9 +31,14 @@ type tags struct {
 	Tags []string `json:"tags"`
 }
 
-// List calls /tags/list for the given repository, returning the list of tags
-// in the "tags" property.
+// List wraps ListWithContext using the background context.
 func List(repo name.Repository, options ...Option) ([]string, error) {
+	return ListWithContext(context.Background(), repo, options...)
+}
+
+// ListWithContext calls /tags/list for the given repository, returning the list of tags
+// in the "tags" property.
+func ListWithContext(ctx context.Context, repo name.Repository, options ...Option) ([]string, error) {
 	o, err := makeOptions(repo, options...)
 	if err != nil {
 		return nil, err
@@ -42,27 +49,86 @@ func List(repo name.Repository, options ...Option) ([]string, error) {
 		return nil, err
 	}
 
-	uri := url.URL{
-		Scheme: repo.Registry.Scheme(),
-		Host:   repo.Registry.RegistryStr(),
-		Path:   fmt.Sprintf("/v2/%s/tags/list", repo.RepositoryStr()),
+	uri := &url.URL{
+		Scheme:   repo.Registry.Scheme(),
+		Host:     repo.Registry.RegistryStr(),
+		Path:     fmt.Sprintf("/v2/%s/tags/list", repo.RepositoryStr()),
+		RawQuery: "n=10000",
 	}
 
 	client := http.Client{Transport: tr}
-	resp, err := client.Get(uri.String())
-	if err != nil {
-		return nil, err
+	tagList := []string{}
+	parsed := tags{}
+
+	// get responses until there is no next page
+	for {
+		select {
+		case <-ctx.Done():
+			return nil, ctx.Err()
+		default:
+		}
+
+		req, err := http.NewRequest("GET", uri.String(), nil)
+		if err != nil {
+			return nil, err
+		}
+		req = req.WithContext(ctx)
+
+		resp, err := client.Do(req)
+		if err != nil {
+			return nil, err
+		}
+
+		if err := transport.CheckError(resp, http.StatusOK); err != nil {
+			return nil, err
+		}
+
+		if err := json.NewDecoder(resp.Body).Decode(&parsed); err != nil {
+			return nil, err
+		}
+
+		if err := resp.Body.Close(); err != nil {
+			return nil, err
+		}
+
+		tagList = append(tagList, parsed.Tags...)
+
+		uri, err = getNextPageURL(resp)
+		if err != nil {
+			return nil, err
+		}
+		// no next page
+		if uri == nil {
+			break
+		}
 	}
-	defer resp.Body.Close()
 
-	if err := transport.CheckError(resp, http.StatusOK); err != nil {
-		return nil, err
+	return tagList, nil
+}
+
+// getNextPageURL checks if there is a Link header in a http.Response which
+// contains a link to the next page. If yes it returns the url.URL of the next
+// page otherwise it returns nil.
+func getNextPageURL(resp *http.Response) (*url.URL, error) {
+	link := resp.Header.Get("Link")
+	if link == "" {
+		return nil, nil
 	}
 
-	parsed := tags{}
-	if err := json.NewDecoder(resp.Body).Decode(&parsed); err != nil {
-		return nil, err
+	if link[0] != '<' {
+		return nil, fmt.Errorf("failed to parse link header: missing '<' in: %s", link)
+	}
+
+	end := strings.Index(link, ">")
+	if end == -1 {
+		return nil, fmt.Errorf("failed to parse link header: missing '>' in: %s", link)
 	}
+	link = link[1:end]
 
-	return parsed.Tags, nil
+	linkURL, err := url.Parse(link)
+	if err != nil {
+		return nil, err
+	}
+	linkURL = resp.Request.URL.ResolveReference(linkURL)
+	return linkURL, nil
 }


Test patch:
diff --git a/pkg/v1/remote/transport/retry_test.go b/pkg/v1/remote/transport/retry_test.go
index 4b946f5e..91cc02a8 100644
--- a/pkg/v1/remote/transport/retry_test.go
+++ b/pkg/v1/remote/transport/retry_test.go
@@ -15,8 +15,11 @@
 package transport
 
 import (
+	"context"
 	"net/http"
+	"net/http/httptest"
 	"testing"
+	"time"
 
 	"github.com/google/go-containerregistry/pkg/internal/retry"
 )
@@ -92,3 +95,37 @@ func TestRetryDefaults(t *testing.T) {
 		t.Fatal("default predicate not set")
 	}
 }
+
+func TestTimeoutContext(t *testing.T) {
+	tr := NewRetry(http.DefaultTransport)
+
+	slowServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		// hanging request
+		time.Sleep(time.Second * 1)
+	}))
+	defer func() { go func() { slowServer.Close() }() }()
+
+	ctx, cancel := context.WithDeadline(context.Background(), time.Now().Add(time.Millisecond*20))
+	defer cancel()
+	req, err := http.NewRequest("GET", slowServer.URL, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	req = req.WithContext(ctx)
+
+	result := make(chan error)
+
+	go func() {
+		_, err := tr.RoundTrip(req)
+		result <- err
+	}()
+
+	select {
+	case err := <-result:
+		if err != context.DeadlineExceeded {
+			t.Fatalf("got: %v, want: %v", err, context.DeadlineExceeded)
+		}
+	case <-time.After(time.Millisecond * 100):
+		t.Fatalf("deadline was not recognized by transport")
+	}
+}