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

registry: don't call "/info" API endpoint to get default registry #2819

Merged
merged 1 commit into from
Mar 15, 2022

Conversation

thaJeztah
Copy link
Member

The CLI currenly calls the /info endpoint to get the address of the default registry to use.

This functionality was added as part of the initial Windows implementation of the engine. For legal reasons, Microsoft Windows (and thus Docker images based on Windows) were not allowed to be distributed through non-Microsoft infrastructure. As a temporary solution, a dedicated "registry-win-tp3.docker.io" registry was created to serve Windows images.

As a result, the default registry was no longer "fixed", so a helper function (ElectAuthServer) was added to allow the CLI to get the correct registry address from the daemon. (docker/docker PR's/issues 18019, 19891, 19973)

Using separate registries was not an ideal solution, and a more permanent solution was created by introducing "foreign image layers" in the distribution spec, after which the "registry-win-tp3.docker.io" ceased to exist, and removed from the engine through moby/moby#21100.

However, the ElectAuthServer was left in place, quoting from that PR;

make the client check which default registry the daemon uses is still more correct than leaving it up to the client, even if it won't technically matter after this PR. There may be some backward compatibility scenarios where ElectAuthServer [sic] is still helpful.

That comment was 5 years ago, and given that the engine and cli are released in tandem, and the default registry is not configurable, we can save the extra roundtrip to the daemon by using a fixed value.

This patch deprecates the ElectAuthServer function, and makes it return the default registry without calling (potentially expensive) /info API endpoint.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah
Copy link
Member Author

@tiborvass @tonistiigi PTAL WDYT?

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

LGTM 😻

@thaJeztah
Copy link
Member Author

There's one thing I need to check; I'm looking at changing the default in docker/engine from the old index.docker.io/v1/ (the /v1 is replaced by v2 or v1 depending on what part of Docker Hub is used, so ignore that) to the actual registry-1.docker.io/v2/. I think that should not cause a compatibility issue if a potential cli would still use index.docker.io/v1/, but I need to push a WIP branch that makes that change to double-check.

@codecov-commenter
Copy link

codecov-commenter commented Aug 12, 2021

Codecov Report

Merging #2819 (b4ca1c7) into master (3304c49) will decrease coverage by 0.03%.
The diff coverage is 22.22%.

@@            Coverage Diff             @@
##           master    #2819      +/-   ##
==========================================
- Coverage   59.05%   59.01%   -0.04%     
==========================================
  Files         284      284              
  Lines       23852    23833      -19     
==========================================
- Hits        14086    14066      -20     
- Misses       8907     8908       +1     
  Partials      859      859              

The CLI currenly calls the `/info` endpoint to get the address
of the default registry to use.

This functionality was added as part of the initial Windows implementation
of the engine. For legal reasons, Microsoft Windows (and thus Docker images
based on Windows) were not allowed to be distributed through non-Microsoft
infrastructure. As a temporary solution, a dedicated "registry-win-tp3.docker.io"
registry was created to serve Windows images.

As a result, the default registry was no longer "fixed", so a helper function
(`ElectAuthServer`) was added to allow the CLI to get the correct registry
address from the daemon. (docker/docker PR's/issues 18019, 19891, 19973)

Using separate registries was not an ideal solution, and a more permanent
solution was created by introducing "foreign image layers" in the distribution
spec, after which the "registry-win-tp3.docker.io" ceased to exist, and
removed from the engine through docker/docker PR 21100.

However, the `ElectAuthServer` was left in place, quoting from that PR;

> make the client check which default registry the daemon uses is still
> more correct than leaving it up to the client, even if it won't technically
> matter after this PR. There may be some backward compatibility scenarios
> where `ElectAuthServer` [sic] is still helpful.

That comment was 5 years ago, and given that the engine and cli are
released in tandem, and the default registry is not configurable, we
can save the extra roundtrip to the daemon by using a fixed value.

This patch deprecates the `ElectAuthServer` function, and makes it
return the default registry without calling (potentially expensie)
`/info` API endpoint.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Copy link
Member Author

Thx! Okay let's get this one in 👍

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.

5 participants