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.

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

Update login to use token handling code from distribution #20832

Merged
merged 1 commit into from
Mar 3, 2016

Conversation

aaronlehmann
Copy link
Contributor

This PR lays the groundwork for a future PR to add OAuth registry authentication (see distribution/distribution#1418 and distribution/distribution#1475). Before this can happen, docker login needs to use the token handling code in docker/distribution instead of using a separate implementation.

The main purpose of this PR is to make docker login use token handling logic defined in docker/distribution, like push and pull already do. However, there's a fair amount of cleanup here as well. The idea is to simplify the login code and clean up the technical debt here so the upcoming PR to add OAuth will be a relatively simple change that can be easily reviewed.

The main changes here are:

  • Unify v2 registry "ping" logic between login and push/pull
  • Change LookupPullEndpoints to take a hostname instead parsing a full image reference to extract the hostname
  • Rename Endpoint to V1Endpoint to make clear it is only used in v1 flows
  • Remove getToken function that duplicates token handling code in the github.com/docker/distribution/registry/client/auth auth package. Use the vendored code instead.

This was originally written by @dmcgowan. I am submitting the PR on his behalf (with a few minor edits of my own), since he is away for the next few days.

Further differentiate the APIEndpoint used with V2 with the endpoint type which is only used for v1 registry interactions
Rename Endpoint to V1Endpoint and remove version ambiguity
Use distribution token handler for login

Signed-off-by: Derek McGowan <[email protected]>
Signed-off-by: Aaron Lehmann <[email protected]>
@icecrime
Copy link
Contributor

icecrime commented Mar 1, 2016

Experimental failure looks like a flaky test (cc @thaJeztah: can you please make sure we track that one?)

12:43:59 FAIL: docker_cli_pull_test.go:75: DockerHubPullSuite.TestPullFromCentralRegistryImplicitRefParts
12:43:59 
12:43:59 docker_cli_pull_test.go:106:
12:43:59     c.Assert(out, checker.Contains, "Image is up to date for hello-world:latest")
12:43:59 ... obtained string = "" +
12:43:59 ...     "Using default tag: latest\n" +
12:43:59 ...     "latest: Pulling from library/hello-world\n" +
12:43:59 ...     "03f4658f8b78: Pulling fs layer\n" +
12:43:59 ...     "a3ed95caeb02: Pulling fs layer\n" +
12:43:59 ...     "a3ed95caeb02: Verifying Checksum\n" +
12:43:59 ...     "a3ed95caeb02: Download complete\n" +
12:43:59 ...     "03f4658f8b78: Verifying Checksum\n" +
12:43:59 ...     "03f4658f8b78: Download complete\n" +
12:43:59 ...     "03f4658f8b78: Pull complete\n" +
12:43:59 ...     "a3ed95caeb02: Pull complete\n" +
12:43:59 ...     "Digest: sha256:8be990ef2aeb16dbcb9271ddfe2610fa6658d13f6dfb8bc72074cc1ca36966a7\n" +
12:43:59 ...     "Status: Downloaded newer image for hello-world:latest\n"
12:43:59 ... substring string = "Image is up to date for hello-world:latest"

@aaronlehmann
Copy link
Contributor Author

Interesting. I saw the same failure on janky in #20831. I don't remember seeing this often in the past. I wonder if something in the tests has changed recently in a way that made this more flaky.

@thaJeztah
Copy link
Member

@icecrime I reopened #17214

@aaronlehmann possibly because someone changed the test in #20411 😇 😉

@aaronlehmann
Copy link
Contributor Author

I think the fix in #20411 was not harmful, but it was insufficient. I'll open a PR that might help.

@thaJeztah
Copy link
Member

@aaronlehmann I know, just teasing 👍

@calavera
Copy link
Contributor

calavera commented Mar 2, 2016

LGTM

@RichardScothern
Copy link
Contributor

LGTM 🎉

@thaJeztah
Copy link
Member

LGTM

thaJeztah added a commit that referenced this pull request Mar 3, 2016
Update login to use token handling code from distribution
@thaJeztah thaJeztah merged commit 17156ba into moby:master Mar 3, 2016
@aaronlehmann aaronlehmann deleted the login-endpoint-refactor branch March 3, 2016 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants