Add client implementation of distribution interface #387

dmcgowan · 2015-04-17T20:35:53Z

Adds functionality to create a Repository client which connects to a remote endpoint.

A few functions are yet to be implemented.

Authorizer is an interface to allow for passing different credential types but abstracting that from the rest of the client code. It may make sense to split this out to somewhere else. To follow the client logic today, getting an Authorizer will ping a registry endpoint to get the auth challenge list, then the implementation of Authorizer will take care of either attaching the credentials directly or hitting a token server.

As part of this change, I also want to get rid of the old client interface as well as break apart the repository file into multiple files.

tiborvass · 2015-05-05T21:09:39Z

registry/client/repository.go

+)
+
+// NewRepositoryClient creates a new Repository for the given repository name and endpoint
+func NewRepositoryClient(ctx context.Context, name string, endpoint *RepositoryEndpoint) (distribution.Repository, error) {


@dmcgowan I guess this could be called NewRepository since it's returning a new repository. wdyt?

stevvooe · 2015-05-07T21:23:39Z

registry/client/authchallenge.go

+	Scheme     string
+	Parameters map[string]string
+}
+


There should be a companion function "ParseAuthChallenge" that will parse the header if this is an exported type.

Not sure why this is even exposed, I will trim down some as this shouldn't be needed outside this package.

dmcgowan · 2015-05-08T23:39:14Z

Made updates as suggested. Can likely move Transport and Authorizer into a subpackage to focus the client package interface more. The interface is relatively light except for all the Error types introduced.

RichardScothern · 2015-05-11T17:47:39Z

registry/client/layer.go

+	n, err = rd.Read(p)
+	hl.offset += int64(n)
+
+	// Simulate io.EOR error if we reach filesize.


stevvooe · 2015-05-11T21:17:12Z

@dmcgowan Here's a good test for Authorizor and Transport: can they be used without the rest of the implementation to correctly communicate with a docker endpoint?

dmcgowan · 2015-05-11T22:12:43Z

@stevvooe the Transport and Authorizer interface, yes it could. The individual implementations for AuthenticationHandler could also be used generically. However the implementation of Authorizer which uses the handlers is not generic as it expects to get a set of authentication challenges when hitting the "v2" endpoint.

dmcgowan · 2015-05-11T23:54:45Z

Removed this WIP flag, this PR is ready for more intensive review. There are a few TODOs still in the code that I do not think are merge blockers.

tiborvass · 2015-05-13T23:11:56Z

registry/client/layer.go

+		return ioutil.NopCloser(bytes.NewReader([]byte{})), nil
+	}
+
+	blobURL, err := hl.ub.BuildBlobURL(hl.name, hl.digest)


@dmcgowan @stevvooe

It would be so much better to have an exported constructor that takes a static url string and an http client to make it very generic so that I can reuse it:

func NewHttpReadSeeker(client *http.Client, url string) io.ReadSeeker { return *httpLayer{...} }

you shouldn't need to embed *layers

Starting rebase now onto the refactor branch, will re-address your comment after.

What do you see as the generic use case for httpLayer?

dmcgowan · 2015-05-14T16:56:55Z

@stevvooe simplified interface down to func NewRepository(ctx context.Context, name, endpoint string, transport http.RoundTripper) (distribution.Repository, error)

The custom transport, modifier interface, and authentication handlers are now completely separated and should likely be moved into a sub package.

noxiouz · 2015-05-14T17:06:37Z

registry/client/repository.go

+	if err != nil {
+		return nil, err
+	}
+


defer resp.Body.Close()

stevvooe · 2015-05-20T01:03:41Z

registry/client/repository.go

+}
+
+type blobStatter struct {
+	*repository


While embedding was heavily used in the storage package, I found it to be more clear to outline the actual dependencies of the blob statter. It makes it easier to identify bad patterns, especially in an evolving code base.

This depends on ub and client.

Update comments and TODOs Fix switch style Updated parse http response to take in reader Add Cancel implementation Update blobstore variable name Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)

Each type no longer requires holding a reference to repository. Added implementation for signatures get. Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)

stevvooe · 2015-05-20T05:44:37Z

I'm going to give my LGTM on this.

There may be some tweaks we want to do but this is good enough for what is required now.

tiborvass · 2015-05-20T14:09:18Z

registry/client/repository.go

+		}
+
+		return tagsResponse.Tags, nil
+	case http.StatusNotFound:


@dmcgowan isn't a 401 expected? currently 401 goes through handleErrorResponse which returns an "unexpected" http error. It breaks downloading private images without being logged in.

Good point, when nginx is serving up the basic auth it will likely get caught as "unexpected". I think the proper solution will be to just handle 401 in the handleErrorResponse function.

Add check for unauthorized error code and explicitly set the error code if the content could not be parsed. Updated repository test for unauthorized tests and nit feedback. Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)

The transport package no longer requires importing distribution for the ReadSeekCloser, instead declares its own. Added comments on the Authenication handler in session. Added todo on http seek reader to highlight its lack of belonging to the client transport. Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)

dmcgowan · 2015-05-20T17:25:08Z

Added a few more commits to address comments and 401 errors.

I plan to have follow up commits to address

token logic refactor to registry/auth/token
ping attempt on redirected endpoints

Changes behavior so ping doesn't happen if /v2/ is anywhere in a request path, but instead only at the beginning. This fixes attempts to ping on redirected URLs. Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)

dmcgowan · 2015-05-20T20:56:05Z

Pushed a fix to the issue I described, with a comment highlighting its current behavior and ideal behavior

Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)

tiborvass · 2015-05-20T22:45:09Z

LGTM

stevvooe · 2015-05-20T22:54:34Z

LGTM

Add client implementation of distribution interface

RichardScothern · 2015-05-20T22:55:40Z

LGTM

On Wed, May 20, 2015 at 3:54 PM, Stephen Day notifications@github.com
wrote:

LGTM

—
Reply to this email directly or view it on GitHub
#387 (comment).

Add client implementation of distribution interface

dmcgowan added the in progress label Apr 17, 2015

stevvooe added the enhancement label Apr 27, 2015

stevvooe added this to the Registry/2.1 milestone Apr 27, 2015

dmcgowan mentioned this pull request Apr 27, 2015

Distribution client/engine integration #447

Closed

11 tasks

dmcgowan force-pushed the registry-client-update branch from 2138c9a to 2a8589b Compare April 28, 2015 01:25

RichardScothern mentioned this pull request Apr 28, 2015

Registry V2 mirror proposal #459

Closed

12 tasks

dmcgowan force-pushed the registry-client-update branch 2 times, most recently from 754ee85 to 2334dbb Compare May 4, 2015 23:52

tiborvass reviewed May 5, 2015
View reviewed changes

dmcgowan force-pushed the registry-client-update branch from 2334dbb to fce3441 Compare May 6, 2015 18:14

GordonTheTurtle added the dco/no label May 7, 2015

dmcgowan force-pushed the registry-client-update branch from ed1204b to 729e78f Compare May 7, 2015 18:28

GordonTheTurtle removed the dco/no label May 7, 2015

dmcgowan force-pushed the registry-client-update branch from 729e78f to da4a817 Compare May 7, 2015 20:17

stevvooe reviewed May 7, 2015
View reviewed changes

dmcgowan mentioned this pull request May 8, 2015

Clarify requirement of Docker-Content-Digest #505

Closed

dmcgowan force-pushed the registry-client-update branch from 3756058 to 2c81c10 Compare May 8, 2015 23:29

RichardScothern reviewed May 11, 2015
View reviewed changes

dmcgowan force-pushed the registry-client-update branch from 420b186 to 550eec8 Compare May 11, 2015 22:10

dmcgowan force-pushed the registry-client-update branch from 550eec8 to a571047 Compare May 11, 2015 23:43

dmcgowan changed the title ~~WIP: Add client implementation of distribution interface~~ Add client implementation of distribution interface May 11, 2015

tiborvass reviewed May 13, 2015
View reviewed changes

noxiouz reviewed May 14, 2015
View reviewed changes

registry/client/repository.go

if err != nil {

return nil, err

}

Copy link

Contributor

noxiouz May 14, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defer resp.Body.Close()

stevvooe reviewed May 20, 2015
View reviewed changes

dmcgowan added 2 commits May 19, 2015 19:18

Feedback update

a3276fc

Update comments and TODOs Fix switch style Updated parse http response to take in reader Add Cancel implementation Update blobstore variable name Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)

Break down type dependencies

13894e8

Each type no longer requires holding a reference to repository. Added implementation for signatures get. Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)

dmcgowan force-pushed the registry-client-update branch from d805e3a to 13894e8 Compare May 20, 2015 02:56

tiborvass reviewed May 20, 2015
View reviewed changes

dmcgowan added 2 commits May 20, 2015 10:05

Add unauthorized error check

572ff64

Add check for unauthorized error code and explicitly set the error code if the content could not be parsed. Updated repository test for unauthorized tests and nit feedback. Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)

Only do auth checks for endpoints starting with v2

49369ff

Changes behavior so ping doesn't happen if /v2/ is anywhere in a request path, but instead only at the beginning. This fixes attempts to ping on redirected URLs. Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)

Remove error message shortening

68c1cea

Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)

stevvooe added a commit that referenced this pull request May 20, 2015

Merge pull request #387 from dmcgowan/registry-client-update

ea54a55

Add client implementation of distribution interface

stevvooe merged commit ea54a55 into distribution:master May 20, 2015

stevvooe removed the in progress label May 20, 2015

ncdc mentioned this pull request May 21, 2015

Image name/tag resolution preprocessing pass kubernetes/kubernetes#1697

Open

xiekeyang mentioned this pull request May 26, 2015

Client Package: question about working mode #561

Closed

thaJeztah mentioned this pull request Nov 2, 2020

The push command sends a POST request with an empty Content-Type header moby/moby#41621

Open

thaJeztah pushed a commit to thaJeztah/distribution that referenced this pull request Apr 22, 2021

Merge pull request distribution#387 from dmcgowan/registry-client-update

1038073

Add client implementation of distribution interface

thaJeztah pushed a commit to thaJeztah/distribution that referenced this pull request Jan 19, 2022

Merge pull request distribution#387 from dmcgowan/registry-client-update

e2596ca

Add client implementation of distribution interface

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add client implementation of distribution interface #387

Add client implementation of distribution interface #387

dmcgowan commented Apr 17, 2015

tiborvass May 5, 2015

dmcgowan May 5, 2015

stevvooe May 7, 2015

dmcgowan May 8, 2015

dmcgowan commented May 8, 2015

RichardScothern May 11, 2015

stevvooe commented May 11, 2015

dmcgowan commented May 11, 2015

dmcgowan commented May 11, 2015

tiborvass May 13, 2015

dmcgowan May 14, 2015

dmcgowan commented May 14, 2015

noxiouz May 14, 2015

stevvooe May 20, 2015

dmcgowan May 20, 2015

stevvooe commented May 20, 2015

tiborvass May 20, 2015

dmcgowan May 20, 2015

dmcgowan commented May 20, 2015

dmcgowan commented May 20, 2015

tiborvass commented May 20, 2015

stevvooe commented May 20, 2015

RichardScothern commented May 20, 2015

Add client implementation of distribution interface #387

Add client implementation of distribution interface #387

Conversation

dmcgowan commented Apr 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmcgowan commented May 8, 2015

Choose a reason for hiding this comment

stevvooe commented May 11, 2015

dmcgowan commented May 11, 2015

dmcgowan commented May 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmcgowan commented May 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevvooe commented May 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmcgowan commented May 20, 2015

dmcgowan commented May 20, 2015

tiborvass commented May 20, 2015

stevvooe commented May 20, 2015

RichardScothern commented May 20, 2015