Skip to content

Allow registry_mirrors to apply to unofficial registries.#32771

Closed
Syntaxide wants to merge 1 commit intomoby:masterfrom
Syntaxide:unofficial_mirrors
Closed

Allow registry_mirrors to apply to unofficial registries.#32771
Syntaxide wants to merge 1 commit intomoby:masterfrom
Syntaxide:unofficial_mirrors

Conversation

@Syntaxide
Copy link
Copy Markdown
Contributor

@Syntaxide Syntaxide commented Apr 21, 2017

Tested with unit tests, and ran locally:
registry_mirrors="someregistry.com->index.docker.io", and docker pull
someregistry.com/library/ubuntu succeeded.

Signed-off-by: Alexander Midlash [email protected]

- What I did
Allow registry_mirrors flag to additionally set mirrors for unofficial registries.

- How I did it
For backwards compatibility, registry_mirrors entries can either be old-style ("some-host"), or new style ("original-host->new-mirror"). All mirroring information is stored in the IndexConfig, so that we can set mirrors per index.

- How to verify it
Besides unit tests, try setting registry_mirrors="somedomain.com->index.docker.io"
You can then `docker pull somedomain.com/library/ubuntu" and receive the image from the official registry.
In practice, this would be used to redirect from an organizations private registry, to per-datacenter caches.

- Description for the changelog
Allow registry_mirrors to apply to unofficial registries.

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

Comment thread registry/config.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It is a panic?
And will it be too strict?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Panic seemed reasonable to me, since mirrors are only read when the daemon starts up. The alternative seems to be failing silently, and ignoring invalid mirrors. What do you think is the best way to handle this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just to check whether LoadMirror will be called when a user reloads a daemon's config ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks like the "TestDaemonReloadMirrors " test that was removed was to allow reloading mirrors at runtime (signup)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FWIW, I don't think we panic in other cases like this; I don't think a panic is really suitable here; we validated the format, and the format was invalid, so the error should be printed, but a full dump

@thaJeztah
Copy link
Copy Markdown
Member

Looks like there's a gofmt issue;

19:21:16  - registry/config.go
19:21:16 
19:21:16 Please reformat the above files using "gofmt -s -w" and commit the result.

@thaJeztah
Copy link
Copy Markdown
Member

I'm -1 on introducing a new micro format for this feature, and would rather have we'd either use the advanced (CSV) syntax, e.g. --registry-mirror source=someregistry.com,target=index.docker.io, and use a well-defined format for the daemon.json as well, instead of trying to fit it into a string.

The CSV-syntax could easily be made backward-compatible with the "old" approach (although erroring out if the new format is used should not be a problem).

@thaJeztah thaJeztah added the status/failing-ci Indicates that the PR in its current state fails the test suite label Apr 25, 2017
@Syntaxide Syntaxide force-pushed the unofficial_mirrors branch from 26b8c14 to 98eb71b Compare April 25, 2017 23:46
Tested with unit tests, and ran locally:
registry_mirrors="someregistry.com->index.docker.io", and docker pull
someregistry.com/library/ubuntu succeeded.

Signed-off-by: Alexander Midlash <[email protected]>
@Syntaxide
Copy link
Copy Markdown
Contributor Author

@thaJeztah How would you want to specify multiple registry mirrors?

@thaJeztah
Copy link
Copy Markdown
Member

@Syntaxide the flag would be specified multiple times then;

--registry-mirror source=someregistry.com,target=index.docker.io \
--registry-mirror source=my-other-mirror.com,target=index.docker.io

@Syntaxide
Copy link
Copy Markdown
Contributor Author

@thaJeztah What do you think is the best approach for handling the json? I see the approach taken in 273eeb8 works well for the command line flags. (un)fortunately the other commit did not impact the daemon.json, so I'm not sure what the best approach here is.

It seems we'll need to add additional behavior wherever we unmarshal the daemon.json. What would you propose as the type for ServiceOptions.Mirrors?

I'm tempted to use json.RawMessage, which we can either continue to unmarshal, or just treat as the raw bytes in the case where the value isn't json. Is there a more elegant approach that I am missing?

@Syntaxide
Copy link
Copy Markdown
Contributor Author

Discussed offline. This functionality is better handled by deploying a proxy which has these forwarding rules. This does not need to be implemented in the engine.

@Syntaxide Syntaxide closed this Jun 2, 2017
@Syntaxide Syntaxide deleted the unofficial_mirrors branch June 2, 2017 17:05
@denverdino
Copy link
Copy Markdown
Contributor

@Syntaxide Can you explain more for the conclusion? I believe this is one very important missing feature in Docker. Thanks

@Syntaxide
Copy link
Copy Markdown
Contributor Author

@denverdino
The primary use for mirroring unofficial registries is that enterprises could set up local caches(per datacenter, for example) for their own private registries. However, this functionality doesn't need to be added to Docker. This behavior can be obtained by setting up an http proxy, which recognizes the private registry and forwards requests to the relevant cache instead.

@denverdino
Copy link
Copy Markdown
Contributor

@Syntaxide

We have exactly the requirement for the local cache in remote Data Center for the centralized docker registry in corp headquarter. The HTTP proxy approach is OK, but it is hard for auth and handling the private image.

Another case is user cannot access Docker images from gcr.io. The local mirror with some VPN can solve the problem.

Pls consider those requirements

Thanks a lot

@thaJeztah thaJeztah marked this as a duplicate of #34319 Jul 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

impact/cli status/failing-ci Indicates that the PR in its current state fails the test suite status/1-design-review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants