Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Resolve ambiguity on registry v2 ping #10227

Merged
merged 2 commits into from
Jan 21, 2015
Merged

Resolve ambiguity on registry v2 ping #10227

merged 2 commits into from
Jan 21, 2015

Conversation

jlhawn
Copy link
Contributor

@jlhawn jlhawn commented Jan 20, 2015

v2 ping now checks for a Docker-Distribution-API-Version
header that identifies the endpoint as "registry/2.0"

fixes #10226

@jessfraz jessfraz added this to the 1.5.0 milestone Jan 20, 2015
@jessfraz
Copy link
Contributor

can you add a test? we have the registry tests now

jessfraz
jessfraz reviewed Jan 20, 2015
View reviewed changes
registry/endpoint.go
// Ensure it supports the v2 Registry API.
var supportsV2 bool

for _, versionName := range resp.Header[http.CanonicalHeaderKey("Docker-Distribution-API-Version")] {
Copy link
Contributor

Choose a reason for hiding this comment

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

what if it is such an old registry it doesnt have this header, will it panic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it will range over a nil slice, which is safe, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

https://groups.google.com/forum/#!topic/golang-nuts/gVgVLQU1FFE

On Tue, Jan 20, 2015 at 3:40 PM, Josh Hawn [email protected] wrote:

In registry/endpoint.go
#10227 (comment):

@@ -227,6 +227,21 @@ func (e *Endpoint) pingV2() (RegistryInfo, error) {
}
defer resp.Body.Close()

  • // The endpoint may have multiple supported versions.
  • // Ensure it supports the v2 Registry API.
  • var supportsV2 bool
  • for _, versionName := range resp.Header[http.CanonicalHeaderKey("Docker-Distribution-API-Version")] {

it will range over a nil slice, which is safe, right?

—
Reply to this email directly or view it on GitHub
https://github.com/docker/docker/pull/10227/files#r23266959.

Copy link
Contributor

Choose a reason for hiding this comment

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

right

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think it's safe (accessing an unknown key from a map returns the default value for the mapped type, and it's safe to range over a nil slice).

Copy link
Contributor

Choose a reason for hiding this comment

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

ok cool can we still have a test 😈

not for the range thing but for the ping

Copy link
Contributor

Choose a reason for hiding this comment

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

@jlhawn This might be too late: this loop is only iterating over the instances of the Docker-Distribution-API-Version. There needs to be a split on space for this to work correctly in the future.

For example, this only supports this:

Docker-Distribution-API-Version: registry/2.0
Docker-Distribution-API-Version: registry/2.1

When it should support this:

Docker-Distribution-API-Version: registry/2.0 registry/2.1

This will keep 1.5 sufficiently future proof.

And even this:

Docker-Distribution-API-Version: registry/2.0 registry/2.1
Docker-Distribution-API-Version: registry/1.0

@tiborvass
Copy link
Contributor

I confirm this works and fixed my problem with docker login on a private registry (tested with HTTP only, not HTTPS)

But please address @jfrazelle's concern

@jlhawn jlhawn force-pushed the v1_v2_login_patch branch 4 times, most recently from 29d59d8 to 22debc6 Compare January 21, 2015 01:44
@jessfraz
Copy link
Contributor

LGTM

@jessfraz
Copy link
Contributor

tested locally with the dockerfile change

@tiborvass
Copy link
Contributor

@jlhawn @jfrazelle I still have https://gist.github.com/tiborvass/70954869930b67c6a929 locally.

@jessfraz
Copy link
Contributor

That seems unrelated to this patch? Hmmm

On Tuesday, January 20, 2015, Tibor Vass [email protected] wrote:

@jlhawn https://github.com/jlhawn @jfrazelle
https://github.com/jfrazelle I still have
https://gist.github.com/tiborvass/70954869930b67c6a929 locally.

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

@jlhawn
Copy link
Contributor Author

jlhawn commented Jan 21, 2015

@jfrazelle @tiborvass: @tianon is helping me update this so it doesn't require the libtrust changes! 👏

@jessfraz
Copy link
Contributor

/me is very excite

On Tue, Jan 20, 2015 at 7:36 PM, Josh Hawn [email protected] wrote:

@jfrazelle https://github.com/jfrazelle @tiborvass
https://github.com/tiborvass: @tianon https://github.com/tianon is
helping me update this so it doesn't require the libtrust changes! [image:
👏]

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

@jlhawn jlhawn force-pushed the v1_v2_login_patch branch from 22debc6 to 5dd7fa8 Compare January 21, 2015 03:38
@tianon
Copy link
Member

tianon commented Jan 21, 2015

Black market PR for you, sir: jlhawn#3

@tianon
Copy link
Member

tianon commented Jan 21, 2015

(for those following along at home, the Godeps in github.com/docker/distribution is new as of 8 days ago, which is why we weren't using this trick before now 🎉)

@tianon
Copy link
Member

tianon commented Jan 21, 2015

🎈 🍰 🎂

@jessfraz
Copy link
Contributor

is there a way to get rid of that merge commit? idk... but testing

@jlhawn jlhawn force-pushed the v1_v2_login_patch branch from 6d57079 to 0ff357a Compare January 21, 2015 03:51
Josh Hawn and others added 2 commits January 20, 2015 19:52
Resolve ambiguity on registry v2 ping
f46923b 
v2 ping now checks for a Docker-Distribution-API-Version
header that identifies the endpoint as "registry/2.0"

Docker-DCO-1.1-Signed-off-by: Josh Hawn <[email protected]> (github: jlhawn)
@tianon
Update Dockerfile to use Godeps for distribution
681f4d8 
Update our "registry" install to use the included Godeps libraries so that it
doesn't require anything from our current source (hence moving it up for
better caching too)

Signed-off-by: Andrew "Tianon" Page <[email protected]>
@jlhawn jlhawn force-pushed the v1_v2_login_patch branch from 0ff357a to 681f4d8 Compare January 21, 2015 03:52
@tianon
Copy link
Member

tianon commented Jan 21, 2015

Yeah a rebase will do it.

@jlhawn
Copy link
Contributor Author

jlhawn commented Jan 21, 2015

@jfrazelle I rebased the merge commit out

...

and I made @tianon's title shorter. I HAD TO >_>

@tianon
Copy link
Member

tianon commented Jan 21, 2015

ie, docker pull --rebase origin master

@tianon
Copy link
Member

tianon commented Jan 21, 2015

Muahahahaha

@jessfraz
Copy link
Contributor

LGTM

@jessfraz
Copy link
Contributor

whoa i beat drone

@jessfraz
Copy link
Contributor

which will fail anyways bc of the dockerfile

@jlhawn
Copy link
Contributor Author

jlhawn commented Jan 21, 2015

I just started my make test... I'll let you know if it goes bad

@jlhawn
Copy link
Contributor Author

jlhawn commented Jan 21, 2015

all clear for me!

@jessfraz
Copy link
Contributor

🎆

@tiborvass
Copy link
Contributor

LGTM

tiborvass added a commit that referenced this pull request Jan 21, 2015
@tiborvass
Merge pull request #10227 from jlhawn/v1_v2_login_patch
9de604a 
Resolve ambiguity on registry v2 ping
@tiborvass tiborvass merged commit 9de604a into moby:master Jan 21, 2015
@stevvooe stevvooe mentioned this pull request Jan 21, 2015
Closed
@SvenDowideit SvenDowideit mentioned this pull request Jan 27, 2015
Merged
@jlhawn jlhawn deleted the v1_v2_login_patch branch July 31, 2015 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.5.0
Development

Successfully merging this pull request may close these issues.

Registry version resolution is ambiguous if unknown version endpoint requires auth
6 participants
@jlhawn @jessfraz @tiborvass @tianon @stevvooe @icecrime