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

Implement private registry mirror support #34319

Open
wants to merge 1 commit into
base: master
from

Conversation

@vrothberg

vrothberg commented Jul 31, 2017

Add private-registry mirror support

Add support for mirroring private registries. The daemon.json config
can now be configured as exemplified below:

{
"registries": [
        {
	"Prefix": "docker.io/alpine",
	"Mirrors": [
		{
			"URL": "http://local-alpine-mirror.lan",
		}
	]
        },
        {
        "Prefix": "registry.suse.com",
        "Mirrors": [
                {
                        "URL": "https://remote.suse.mirror.com"
                }
        ]
        },
        {
        "Prefix": "http://insecure.registry.org:5000"
        }
],
"registry-mirrors": ["https://deprecated-mirror.com"]
}

With the new semantics, a mirror will be selected as an endpoint if the
specified prefix matches the prefix of the requested resource (e.g., an
image reference). In the upper example, "local-alpine-mirror" will only
serve as a mirror for docker.io if the requested resource matches the
"alpine" prefix, such as "alpine:latest" or "alpine-foo/bar".

Furthermore, private registries can now be mirrored as well. In the
example above, "remote.suse.mirror.com" will serve as a mirror for all
requests to "registry.suse.com". Notice that if no http{s,} scheme is
specified, the URI will always default to https without fallback to
http. An insecure registry can now be specified by adding the "http://"
scheme to the corresponding prefix.

Note that the configuration is sanity checked, so that a given mirror
can serve multiple prefixes if they all point to the same registry,
while a registry cannot simultaneously serve as a mirror. The daemon
will warn in case the URI schemes of a registry and one of its mirrors
do not correspond.

This change deprecates the "insecure-regestries" and "registry-mirrors"
options, while the "insecure-registries" cannot be used simultaneously
with the new "registries", which doesn't allow a fallback from https to
http for security reasons.

Signed-off-by: Flavio Castelli fcastelli@suse.com
Signed-off-by: Valentin Rothberg vrothberg@suse.com

@vrothberg

This comment has been minimized.

Show comment
Hide comment
@vrothberg

vrothberg commented Jul 31, 2017

Linking a related issue: docker/distribution#1431

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jul 31, 2017

Member

Looks like a duplicate of #32771, #21245, #19009, and #18915

Following earlier discussions / PR's on this subject, this needs a proper design first (see #19009 (comment)); I think this PR needs agreement on a design for this on the related issue: #18818

Member

thaJeztah commented Jul 31, 2017

Looks like a duplicate of #32771, #21245, #19009, and #18915

Following earlier discussions / PR's on this subject, this needs a proper design first (see #19009 (comment)); I think this PR needs agreement on a design for this on the related issue: #18818

@thaJeztah thaJeztah marked this as a duplicate of #21245 Jul 31, 2017

@thaJeztah thaJeztah marked this as a duplicate of #32771 Jul 31, 2017

@vrothberg

This comment has been minimized.

Show comment
Hide comment
@vrothberg

vrothberg Aug 1, 2017

Hi @thaJeztah,

thanks for your comment.

Following earlier discussions / PR's on this subject, this needs a proper design first (see docker#19009 (comment)); I think this PR needs agreement on a design for this on the related issue: docker#18818

Shall I use this PR as a design proposal for #18818? We read through all related PRs before coming up with this solution, and believe that this PR matches the requirements. The most important part is that images can't be messed up as a mirror can't be used for more than one registry.

vrothberg commented Aug 1, 2017

Hi @thaJeztah,

thanks for your comment.

Following earlier discussions / PR's on this subject, this needs a proper design first (see docker#19009 (comment)); I think this PR needs agreement on a design for this on the related issue: docker#18818

Shall I use this PR as a design proposal for #18818? We read through all related PRs before coming up with this solution, and believe that this PR matches the requirements. The most important part is that images can't be messed up as a mirror can't be used for more than one registry.

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Aug 3, 2017

Contributor

CI failure is caused by one of the test vectors not being updated with the new private-registry-mirrors member.

09:36:45 ----------------------------------------------------------------------
09:36:45 FAIL: docker_cli_events_unix_test.go:390: DockerDaemonSuite.TestDaemonEvents
09:36:45 
09:36:45 [dd85c3b245ad7] waiting for daemon to start
09:36:45 [dd85c3b245ad7] daemon started
09:36:45 
09:36:45 docker_cli_events_unix_test.go:431:
09:36:45     c.Assert(out, checker.Contains, fmt.Sprintf("daemon reload %s (allow-nondistributable-artifacts=[], cluster-advertise=, cluster-store=, cluster-store-opts={}, debug=true, default-runtime=runc, default-shm-size=67108864, insecure-registries=[], labels=[\"bar=foo\"], live-restore=false, max-concurrent-downloads=1, max-concurrent-uploads=5, name=%s, registry-mirrors=[], runtimes=runc:{docker-runc []}, shutdown-timeout=10)", daemonID, daemonName))
09:36:45 ... obtained string = "2017-07-31T09:36:42.951676711Z daemon reload 2JW5:JJ5Z:E3AL:VASX:7RZB:CVIG:YWKL:URZX:TD6V:2RXB:7OMD:BXWL (allow-nondistributable-artifacts=[], cluster-advertise=, cluster-store=, cluster-store-opts={}, debug=true, default-runtime=runc, default-shm-size=67108864, insecure-registries=[], labels=[\"bar=foo\"], live-restore=false, max-concurrent-downloads=1, max-concurrent-uploads=5, name=02f072db8516, private-registry-mirrors={}, registry-mirrors=[], runtimes=runc:{docker-runc []}, shutdown-timeout=10)\n"
09:36:45 ... substring string = "daemon reload 2JW5:JJ5Z:E3AL:VASX:7RZB:CVIG:YWKL:URZX:TD6V:2RXB:7OMD:BXWL (allow-nondistributable-artifacts=[], cluster-advertise=, cluster-store=, cluster-store-opts={}, debug=true, default-runtime=runc, default-shm-size=67108864, insecure-registries=[], labels=[\"bar=foo\"], live-restore=false, max-concurrent-downloads=1, max-concurrent-uploads=5, name=02f072db8516, registry-mirrors=[], runtimes=runc:{docker-runc []}, shutdown-timeout=10)"
Contributor

cyphar commented Aug 3, 2017

CI failure is caused by one of the test vectors not being updated with the new private-registry-mirrors member.

09:36:45 ----------------------------------------------------------------------
09:36:45 FAIL: docker_cli_events_unix_test.go:390: DockerDaemonSuite.TestDaemonEvents
09:36:45 
09:36:45 [dd85c3b245ad7] waiting for daemon to start
09:36:45 [dd85c3b245ad7] daemon started
09:36:45 
09:36:45 docker_cli_events_unix_test.go:431:
09:36:45     c.Assert(out, checker.Contains, fmt.Sprintf("daemon reload %s (allow-nondistributable-artifacts=[], cluster-advertise=, cluster-store=, cluster-store-opts={}, debug=true, default-runtime=runc, default-shm-size=67108864, insecure-registries=[], labels=[\"bar=foo\"], live-restore=false, max-concurrent-downloads=1, max-concurrent-uploads=5, name=%s, registry-mirrors=[], runtimes=runc:{docker-runc []}, shutdown-timeout=10)", daemonID, daemonName))
09:36:45 ... obtained string = "2017-07-31T09:36:42.951676711Z daemon reload 2JW5:JJ5Z:E3AL:VASX:7RZB:CVIG:YWKL:URZX:TD6V:2RXB:7OMD:BXWL (allow-nondistributable-artifacts=[], cluster-advertise=, cluster-store=, cluster-store-opts={}, debug=true, default-runtime=runc, default-shm-size=67108864, insecure-registries=[], labels=[\"bar=foo\"], live-restore=false, max-concurrent-downloads=1, max-concurrent-uploads=5, name=02f072db8516, private-registry-mirrors={}, registry-mirrors=[], runtimes=runc:{docker-runc []}, shutdown-timeout=10)\n"
09:36:45 ... substring string = "daemon reload 2JW5:JJ5Z:E3AL:VASX:7RZB:CVIG:YWKL:URZX:TD6V:2RXB:7OMD:BXWL (allow-nondistributable-artifacts=[], cluster-advertise=, cluster-store=, cluster-store-opts={}, debug=true, default-runtime=runc, default-shm-size=67108864, insecure-registries=[], labels=[\"bar=foo\"], live-restore=false, max-concurrent-downloads=1, max-concurrent-uploads=5, name=02f072db8516, registry-mirrors=[], runtimes=runc:{docker-runc []}, shutdown-timeout=10)"
@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Aug 8, 2017

Contributor

@vrothberg Thanks for the great PR!

@thaJeztah As far as I remember, we still had concerns for this model due trust about mirrors vs origin, but I am not sure where we are on this today. Let's make sure the right people are involved to make a decision here.

@vrothberg Any reason we just don't accept these in the --registry-mirrors flag? What is the reasoning behind the separate flag?

Contributor

stevvooe commented Aug 8, 2017

@vrothberg Thanks for the great PR!

@thaJeztah As far as I remember, we still had concerns for this model due trust about mirrors vs origin, but I am not sure where we are on this today. Let's make sure the right people are involved to make a decision here.

@vrothberg Any reason we just don't accept these in the --registry-mirrors flag? What is the reasoning behind the separate flag?

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Aug 8, 2017

Contributor

@vikstrous PTAL

Contributor

stevvooe commented Aug 8, 2017

@vikstrous PTAL

@vrothberg

This comment has been minimized.

Show comment
Hide comment
@vrothberg

vrothberg Aug 8, 2017

@stevvooe Thanks a lot for your comments.

@vrothberg Any reason we just don't accept these in the --registry-mirrors flag? What is the reasoning behind the separate flag?

We figured it's more explicit and expressive to configure. But if you wish, we can change that and split private registry and mirror, for instance, with an '@' in --registry-mirror.

vrothberg commented Aug 8, 2017

@stevvooe Thanks a lot for your comments.

@vrothberg Any reason we just don't accept these in the --registry-mirrors flag? What is the reasoning behind the separate flag?

We figured it's more explicit and expressive to configure. But if you wish, we can change that and split private registry and mirror, for instance, with an '@' in --registry-mirror.

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Aug 8, 2017

Contributor

@vrothberg That is my question: is there a difference in behavior or policy, other than that these registries may need separate credentials?

Contributor

stevvooe commented Aug 8, 2017

@vrothberg That is my question: is there a difference in behavior or policy, other than that these registries may need separate credentials?

@vrothberg

This comment has been minimized.

Show comment
Hide comment
@vrothberg

vrothberg Aug 9, 2017

@vrothberg That is my question: is there a difference in behavior or policy, other than that these registries may need separate credentials?

@stevvooe No, to my knowledge, there is no difference. From a mirror's perspective, it's just serving to a non-default and non-official registry URL.

vrothberg commented Aug 9, 2017

@vrothberg That is my question: is there a difference in behavior or policy, other than that these registries may need separate credentials?

@stevvooe No, to my knowledge, there is no difference. From a mirror's perspective, it's just serving to a non-default and non-official registry URL.

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Aug 22, 2017

Contributor

@vrothberg So, do you think a separate flag is really necessary?

Contributor

stevvooe commented Aug 22, 2017

@vrothberg So, do you think a separate flag is really necessary?

@vrothberg

This comment has been minimized.

Show comment
Hide comment
@vrothberg

vrothberg Aug 23, 2017

@vrothberg So, do you think a separate flag is really necessary?

@stevvooe No, not from a technical point of view. I prefer a separate flag because it is more explicit, but that's just matter of taste.

Do you want me to change the commit to use the --registry-mirror flag instead and split host and proxy with, for instance, an @?

vrothberg commented Aug 23, 2017

@vrothberg So, do you think a separate flag is really necessary?

@stevvooe No, not from a technical point of view. I prefer a separate flag because it is more explicit, but that's just matter of taste.

Do you want me to change the commit to use the --registry-mirror flag instead and split host and proxy with, for instance, an @?

@vrothberg

This comment has been minimized.

Show comment
Hide comment
@vrothberg

vrothberg Sep 1, 2017

@stevvooe, @thaJeztah I'd love to push this PR forward. As far as I can see, the remaining open question is whether a dedicated flag for private-registry mirrors is really needed or not. There have been some concerns if it's actually possible to find a separator that wouldn't break a valid URL [1], which is indeed really tricky. However, using @ works as the daemon doesn't accept mirrors with http basic auth [2]. Keeping this in mind, shall I update this PR and remove the dedicated flag?

There is another open PR #34495 fixing the unfortunate fact that the daemon currently ignores invalid input, but there should be no conflict as soon as the error propagation and input validation work as expected.

[1] https://tools.ietf.org/html/rfc3986
[2] https://github.com/moby/moby/blob/master/registry/config.go#L329

vrothberg commented Sep 1, 2017

@stevvooe, @thaJeztah I'd love to push this PR forward. As far as I can see, the remaining open question is whether a dedicated flag for private-registry mirrors is really needed or not. There have been some concerns if it's actually possible to find a separator that wouldn't break a valid URL [1], which is indeed really tricky. However, using @ works as the daemon doesn't accept mirrors with http basic auth [2]. Keeping this in mind, shall I update this PR and remove the dedicated flag?

There is another open PR #34495 fixing the unfortunate fact that the daemon currently ignores invalid input, but there should be no conflict as soon as the error propagation and input validation work as expected.

[1] https://tools.ietf.org/html/rfc3986
[2] https://github.com/moby/moby/blob/master/registry/config.go#L329

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Sep 1, 2017

Contributor

@vrothberg I just LGTM'd #34495.

Do you have an example of the flag format?

@vikstrous Does this interfere with any other plans in this area?

Contributor

stevvooe commented Sep 1, 2017

@vrothberg I just LGTM'd #34495.

Do you have an example of the flag format?

@vikstrous Does this interfere with any other plans in this area?

@vrothberg

This comment has been minimized.

Show comment
Hide comment
@vrothberg

vrothberg Sep 4, 2017

@vrothberg I just LGTM'd #34495.
Do you have an example of the flag format?

Thanks a lot, @stevvooe! In case we merge the semantics of private-registry mirrors with the --registry-mirror flag, we may split the mirror and the registry via mirror@registry. Currently, this should work as basic authentication (e.g., username:password@www.example.com) is not accepted (which is a good thing as passwords may be stored in clear text in the config.json). So if we go this path and decide to support basic authentication in the future, we will have a conflict with the mirror@registry syntax. Also if a user tries to use basic authentication now, it will be tricky to notify that this is not supported as we have to figure out if the specified string looks like registry@mirror or username:password@mirror.

So to the more I think about it, the safer I feel using a dedicated flag/config option for private-registry mirrors.

vrothberg commented Sep 4, 2017

@vrothberg I just LGTM'd #34495.
Do you have an example of the flag format?

Thanks a lot, @stevvooe! In case we merge the semantics of private-registry mirrors with the --registry-mirror flag, we may split the mirror and the registry via mirror@registry. Currently, this should work as basic authentication (e.g., username:password@www.example.com) is not accepted (which is a good thing as passwords may be stored in clear text in the config.json). So if we go this path and decide to support basic authentication in the future, we will have a conflict with the mirror@registry syntax. Also if a user tries to use basic authentication now, it will be tricky to notify that this is not supported as we have to figure out if the specified string looks like registry@mirror or username:password@mirror.

So to the more I think about it, the safer I feel using a dedicated flag/config option for private-registry mirrors.

@vikstrous

This comment has been minimized.

Show comment
Hide comment
@vikstrous

vikstrous Sep 4, 2017

Contributor

There is one use case I was hoping to be able to solve that this proposal doesn't address. I think it would be valuable to be able to support mirroring hub within a sub-namespace on another registry. That way you can have a hub mirror AND local images without namespace conflicts. For, example, I would like to be able to pull someone/nginx and have that effectively rewritten to registry.example.com/hubmirror/someone/nginx so that I can configure registry.example.com as a pull-through cache for hub as well as a regular registry.

Contributor

vikstrous commented Sep 4, 2017

There is one use case I was hoping to be able to solve that this proposal doesn't address. I think it would be valuable to be able to support mirroring hub within a sub-namespace on another registry. That way you can have a hub mirror AND local images without namespace conflicts. For, example, I would like to be able to pull someone/nginx and have that effectively rewritten to registry.example.com/hubmirror/someone/nginx so that I can configure registry.example.com as a pull-through cache for hub as well as a regular registry.

@flavio

This comment has been minimized.

Show comment
Hide comment
@flavio

flavio Sep 5, 2017

Contributor

I'm against using separators because the final solution looks like a hack. I really appreciate the usage of a dictionary because it's more explicit.

A new flag was introduced to:

  • be able to use a dictionary
  • avoid breakage of the cli for existing users (do not force the existing flag to use a dictionary)
  • make the distinction between the central hub and third party registries explicit
Contributor

flavio commented Sep 5, 2017

I'm against using separators because the final solution looks like a hack. I really appreciate the usage of a dictionary because it's more explicit.

A new flag was introduced to:

  • be able to use a dictionary
  • avoid breakage of the cli for existing users (do not force the existing flag to use a dictionary)
  • make the distinction between the central hub and third party registries explicit
@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Sep 5, 2017

Contributor

@flavio @vrothberg Thanks for making the need for a new flag much clearer. I agree that this is fundamentally different than the existing registry-mirrors flag.

In practice, we are mapping a namespace, which was traditionally a registry host, to a registry host. Let's come up with at least a syntax/data structure that represents this problem.

Do we have a set of example use cases that we could build a table around?

Contributor

stevvooe commented Sep 5, 2017

@flavio @vrothberg Thanks for making the need for a new flag much clearer. I agree that this is fundamentally different than the existing registry-mirrors flag.

In practice, we are mapping a namespace, which was traditionally a registry host, to a registry host. Let's come up with at least a syntax/data structure that represents this problem.

Do we have a set of example use cases that we could build a table around?

@flavio

This comment has been minimized.

Show comment
Hide comment
@flavio

flavio Sep 7, 2017

Contributor

@vrothberg and I are working on a concrete example with config files and such. We will come back with it by the beginning of next week.

Contributor

flavio commented Sep 7, 2017

@vrothberg and I are working on a concrete example with config files and such. We will come back with it by the beginning of next week.

@alexkli

This comment has been minimized.

Show comment
Hide comment
@alexkli

alexkli Jul 25, 2018

Have you considered doing @scopes as implemented by NPM for a similar problem? IIUC the discussed approach above, the scope could be the only "prefix" option... This might simplify things a bit, but still cover the needs of enterprises with custom registries.

The @ would clearly separate a scope (@mycompany/image) from a regular existing org (ubuntu/image). A client can then configure the registry to be used for a certain @scope, for example say that anything with @mycompany is to be fetched from dockerregistry.mycompany.com instead of the public docker hub. That should allow a simple docker pull @mycompany/image and would allow a company to change its local registries around and support for caching registries without impacting references to these image names. Because @ opens a new yet unused namespace (AFAIU), it avoids breaking existing image namespaces.

alexkli commented Jul 25, 2018

Have you considered doing @scopes as implemented by NPM for a similar problem? IIUC the discussed approach above, the scope could be the only "prefix" option... This might simplify things a bit, but still cover the needs of enterprises with custom registries.

The @ would clearly separate a scope (@mycompany/image) from a regular existing org (ubuntu/image). A client can then configure the registry to be used for a certain @scope, for example say that anything with @mycompany is to be fetched from dockerregistry.mycompany.com instead of the public docker hub. That should allow a simple docker pull @mycompany/image and would allow a company to change its local registries around and support for caching registries without impacting references to these image names. Because @ opens a new yet unused namespace (AFAIU), it avoids breaking existing image namespaces.

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Aug 4, 2018

Contributor

@alexkli The purpose of this PR is to allow third-party registries to act as mirrors for the main registry -- so having a separate namespace doesn't really help this issue.

Contributor

cyphar commented Aug 4, 2018

@alexkli The purpose of this PR is to allow third-party registries to act as mirrors for the main registry -- so having a separate namespace doesn't really help this issue.

@alexkli

This comment has been minimized.

Show comment
Hide comment
@alexkli

alexkli Aug 4, 2018

@cyphar It‘s not just about mirroring & caching. It‘s also about mapping certain „prefixes“ to different registries - that‘s right in the original description.

In any case, it would be really helpful to have a model where one can have multiple registries, and docker would select the right registry based on a prefix or „scope“ in the tag, without having the URL in the tag name. To allow switching the registry or mirror hostname, move images to a different registry, mirror the public one to enable enterprise restrictions etc without having to change the name of images in all your source or config files of an application.

alexkli commented Aug 4, 2018

@cyphar It‘s not just about mirroring & caching. It‘s also about mapping certain „prefixes“ to different registries - that‘s right in the original description.

In any case, it would be really helpful to have a model where one can have multiple registries, and docker would select the right registry based on a prefix or „scope“ in the tag, without having the URL in the tag name. To allow switching the registry or mirror hostname, move images to a different registry, mirror the public one to enable enterprise restrictions etc without having to change the name of images in all your source or config files of an application.

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Aug 6, 2018

Contributor

@alexkli The original description was edited after some of the above discussion -- "prefixes" is a term that came up during that discussion. But on your actual point -- I agree that the current way images are referenced is quite frustrating (having the hostname in the URI serves as a fairly simple "security feature" where conflicts are handled by scoping via hostname).

I don't think that proposing to change how images are referenced to another format (even if that format is arguably better) is going to help this PRs chances of being merged. In fact I would argue it would make it even harder, because there would be several orders of magnitude more bike-shedding. (Not to mention that changing it wouldn't help immediately because you would have to migrate everyone to the new format before you could actually use it for mirroring -- it's much simpler to just have a way of remapping the existing format.)

Contributor

cyphar commented Aug 6, 2018

@alexkli The original description was edited after some of the above discussion -- "prefixes" is a term that came up during that discussion. But on your actual point -- I agree that the current way images are referenced is quite frustrating (having the hostname in the URI serves as a fairly simple "security feature" where conflicts are handled by scoping via hostname).

I don't think that proposing to change how images are referenced to another format (even if that format is arguably better) is going to help this PRs chances of being merged. In fact I would argue it would make it even harder, because there would be several orders of magnitude more bike-shedding. (Not to mention that changing it wouldn't help immediately because you would have to migrate everyone to the new format before you could actually use it for mirroring -- it's much simpler to just have a way of remapping the existing format.)

@alexkli

This comment has been minimized.

Show comment
Hide comment
@alexkli

alexkli Aug 6, 2018

The beauty of a new namespace such as @ prefixed scopes is that it does not affect existing naming. Yes, users would have to switch to this model for their non-public images. But arguably they have to change these names sometimes already, when they move to a different registry for example.

However, my understanding is that the core problem of the above is it would introduce changes to the resolution of existing names, which might have unwanted consequences. No form of prefix variations would solve this core issue...

Just my 2 cents.

alexkli commented Aug 6, 2018

The beauty of a new namespace such as @ prefixed scopes is that it does not affect existing naming. Yes, users would have to switch to this model for their non-public images. But arguably they have to change these names sometimes already, when they move to a different registry for example.

However, my understanding is that the core problem of the above is it would introduce changes to the resolution of existing names, which might have unwanted consequences. No form of prefix variations would solve this core issue...

Just my 2 cents.

@bodgit

This comment has been minimized.

Show comment
Hide comment
@bodgit

bodgit Aug 6, 2018

The amount of bikeshedding going on here is silly. You have someone clearly willing to contribute this feature which a lot of people want (myself included) and you can't make your mind up what you want. Please can you sort this out so we can finally get a Docker that is vaguely useful when it isn't directly attached to the Internet because at the moment, it's terrible.

bodgit commented Aug 6, 2018

The amount of bikeshedding going on here is silly. You have someone clearly willing to contribute this feature which a lot of people want (myself included) and you can't make your mind up what you want. Please can you sort this out so we can finally get a Docker that is vaguely useful when it isn't directly attached to the Internet because at the moment, it's terrible.

@vrothberg

This comment has been minimized.

Show comment
Hide comment
@vrothberg

vrothberg Aug 6, 2018

The amount of bikeshedding going on here is silly. You have someone clearly willing to contribute this feature which a lot of people want (myself included) and you can't make your mind up what you want. Please can you sort this out so we can finally get a Docker that is vaguely useful when it isn't directly attached to the Internet because at the moment, it's terrible.

I agree with @bodgit and would love to have a clear public statement from the maintainers if this pull request is still desired or not, with a clear commitment to provide answers for the remaining open questions regarding the design (Cc: @stevvooe @thaJeztah @vdemeester @dnephin). See #34319 (comment).

The pull request has been opened more than a year ago and went through several iterations of reviews, refinements but also entire changes to the design - all of which increased the overall quality of the proposed changes. I understand that the request for the dramatic design change makes sense and that more pairs of eyes had to be involved, but please provide the missing pieces or make your decision on this PR. I do not know how to escalate this topic any further in a civilzed way and I cannot stress out enough how badly this PR has been dealt with.

vrothberg commented Aug 6, 2018

The amount of bikeshedding going on here is silly. You have someone clearly willing to contribute this feature which a lot of people want (myself included) and you can't make your mind up what you want. Please can you sort this out so we can finally get a Docker that is vaguely useful when it isn't directly attached to the Internet because at the moment, it's terrible.

I agree with @bodgit and would love to have a clear public statement from the maintainers if this pull request is still desired or not, with a clear commitment to provide answers for the remaining open questions regarding the design (Cc: @stevvooe @thaJeztah @vdemeester @dnephin). See #34319 (comment).

The pull request has been opened more than a year ago and went through several iterations of reviews, refinements but also entire changes to the design - all of which increased the overall quality of the proposed changes. I understand that the request for the dramatic design change makes sense and that more pairs of eyes had to be involved, but please provide the missing pieces or make your decision on this PR. I do not know how to escalate this topic any further in a civilzed way and I cannot stress out enough how badly this PR has been dealt with.

cyphar added a commit to SUSE/docker-ce that referenced this pull request Aug 22, 2018

Add private-registry mirror support
NOTE: This is a backport/downstream patch of the upstream pull-request
      for Moby, which is still subject to changes.  Please visit
      moby/moby#34319 for the current status.

Add support for mirroring private registries.  The daemon.json config
can now be configured as exemplified below:

```json
{
"registries": [
        {
        "Prefix": "docker.io/library/alpine",
        "Mirrors": [
                {
                        "URL": "http://local-alpine-mirror.lan"
                }
        ]
        },
        {
        "Prefix": "registry.suse.com",
        "Mirrors": [
                {
                        "URL": "https://remote.suse.mirror.com"
                }
        ]
        },
        {
        "Prefix": "http://insecure.registry.org:5000"
        }
],
"registry-mirrors": ["https://deprecated-mirror.com"]
}
```

With the new semantics, a mirror will be selected as an endpoint if the
specified prefix matches the prefix of the requested resource (e.g., an
image reference).  In the upper example, "local-alpine-mirror" will only
serve as a mirror for docker.io if the requested resource matches the
"alpine" prefix, such as "alpine:latest" or "alpine-foo/bar".

Furthermore, private registries can now be mirrored as well.  In the
example above, "remote.suse.mirror.com" will serve as a mirror for all
requests to "registry.suse.com".  Notice that if no http{s,} scheme is
specified, the URI will always default to https without fallback to
http.  An insecure registry can now be specified by adding the "http://"
scheme to the corresponding prefix.

Note that the configuration is sanity checked, so that a given mirror
can serve multiple prefixes if they all point to the same registry,
while a registry cannot simultaneously serve as a mirror.  The daemon
will warn in case the URI schemes of a registry and one of its mirrors
do not correspond.

This change deprecates the "insecure-regestries" and "registry-mirrors"
options, while the "insecure-registries" cannot be used simultaneously
with the new "registries", which doesn't allow a fallback from https to
http for security reasons.

Signed-off-by: Flavio Castelli <fcastelli@suse.com>
Signed-off-by: Valentin Rothberg <vrothberg@suse.com>
@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Aug 24, 2018

Contributor

@vrothberg Sorry this has dropped a little bit. I left docker and haven't been looking at PRs.

To be clear, this is absolutely desired but it is extremely hard to get right. Getting it wrong will have a broad impact on users and someone needs to speak for the silent majority that will have to deal with a broken design (not that this one is broken).

Make sure you address the open items. There are several, such as the use of the RegURL type that make things a little more complicated than necessary.

For this to work, you need to have a prefix and a url+params configuration. It looks like this:

type Registry struct {
    URL string
    // We have a struct so we can add future parameters.
}

type Registries struct { // there might be a better name for this.
    Prefix string
    Push []Registry // restrict to 1 for now, allow multi-push later
    Pull []Registry
}

type RegistryConfig []Registries // or something like this

The above should provide enough flexibility to allow parameterization at the registry level if options around mapping the image names are required. This does require you to fill in some gaps for mapping the exact image to each registry, but that can be modified with fields on Registry and Registries. @thaJeztah suggested an elegant solution in #34319 (comment).

Hopefully, this can get things going. I have too many github notifications to work through, so if you can hit me up on slack or twitter, I'll try to be more responsive.

Contributor

stevvooe commented Aug 24, 2018

@vrothberg Sorry this has dropped a little bit. I left docker and haven't been looking at PRs.

To be clear, this is absolutely desired but it is extremely hard to get right. Getting it wrong will have a broad impact on users and someone needs to speak for the silent majority that will have to deal with a broken design (not that this one is broken).

Make sure you address the open items. There are several, such as the use of the RegURL type that make things a little more complicated than necessary.

For this to work, you need to have a prefix and a url+params configuration. It looks like this:

type Registry struct {
    URL string
    // We have a struct so we can add future parameters.
}

type Registries struct { // there might be a better name for this.
    Prefix string
    Push []Registry // restrict to 1 for now, allow multi-push later
    Pull []Registry
}

type RegistryConfig []Registries // or something like this

The above should provide enough flexibility to allow parameterization at the registry level if options around mapping the image names are required. This does require you to fill in some gaps for mapping the exact image to each registry, but that can be modified with fields on Registry and Registries. @thaJeztah suggested an elegant solution in #34319 (comment).

Hopefully, this can get things going. I have too many github notifications to work through, so if you can hit me up on slack or twitter, I'll try to be more responsive.

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Aug 26, 2018

Contributor

@stevvooe

Make sure you address the open items. There are several,

There are several open items that the Docker maintainers have yet to respond to (for several months now). This is what I believe most people watching this PR are waiting for -- there's no point in implementing a design that has not been agreed upon (and has significant open issues that have been unanswered and significantly affect the implementation).

such as the use of the RegURL type that make things a little more complicated than necessary.

At the moment (given the design is not agreed upon) this is an implementation detail, which is not significant compared to the current concern that prefix stripping is not actually included in the design -- even though it is what the majority of users actually want from this PR.

Contributor

cyphar commented Aug 26, 2018

@stevvooe

Make sure you address the open items. There are several,

There are several open items that the Docker maintainers have yet to respond to (for several months now). This is what I believe most people watching this PR are waiting for -- there's no point in implementing a design that has not been agreed upon (and has significant open issues that have been unanswered and significantly affect the implementation).

such as the use of the RegURL type that make things a little more complicated than necessary.

At the moment (given the design is not agreed upon) this is an implementation detail, which is not significant compared to the current concern that prefix stripping is not actually included in the design -- even though it is what the majority of users actually want from this PR.

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Aug 27, 2018

Contributor

@cyphar what are the open items you want addressed?

Contributor

stevvooe commented Aug 27, 2018

@cyphar what are the open items you want addressed?

@dmcgowan

This comment has been minimized.

Show comment
Hide comment
@dmcgowan

dmcgowan Aug 27, 2018

Member

I am OK clarifying the design to have prefix stripping, we just need to make sure we cover the case for no path being given. Originally I wasn't a huge fan because it would require users creating configurations with namespace components to be aware of the /v2/ path in registry urls, but its not a big deal. So...

{
    "registries": [
        {
          "prefix": "docker.io/",
          "pull": [
            {"url": "http://localhost:5000"}, // Will be used as `http://localhost:5000/v2/`
            {"url": "https://registry-1.docker.io"} // Will be used as `https://registry-1.docker.io/v2/`
          ],
          "push": [
            {"url": "https://registry-1.docker.io"} // Will be used as `https://registry-1.docker.io/v2/`
          ]
        },
    "registries": [
        {
          "prefix": "docker.io/library/",
          "pull": [
            {"url": "http://localhost:5000/v2/library"},
            {"url": "https://registry-1.docker.io/v2/library"}
          ],
          "push": [
            {"url": "https://registry-1.docker.io/v2/"} 
          ]
        },
    ]
}

@cyphar if you have any other concerns over lack of clarity, feel free to list them out so they can be addresses. From my perspective the design is decided on but we can discuss any details where there is still some disagreement or ambiguity.

Member

dmcgowan commented Aug 27, 2018

I am OK clarifying the design to have prefix stripping, we just need to make sure we cover the case for no path being given. Originally I wasn't a huge fan because it would require users creating configurations with namespace components to be aware of the /v2/ path in registry urls, but its not a big deal. So...

{
    "registries": [
        {
          "prefix": "docker.io/",
          "pull": [
            {"url": "http://localhost:5000"}, // Will be used as `http://localhost:5000/v2/`
            {"url": "https://registry-1.docker.io"} // Will be used as `https://registry-1.docker.io/v2/`
          ],
          "push": [
            {"url": "https://registry-1.docker.io"} // Will be used as `https://registry-1.docker.io/v2/`
          ]
        },
    "registries": [
        {
          "prefix": "docker.io/library/",
          "pull": [
            {"url": "http://localhost:5000/v2/library"},
            {"url": "https://registry-1.docker.io/v2/library"}
          ],
          "push": [
            {"url": "https://registry-1.docker.io/v2/"} 
          ]
        },
    ]
}

@cyphar if you have any other concerns over lack of clarity, feel free to list them out so they can be addresses. From my perspective the design is decided on but we can discuss any details where there is still some disagreement or ambiguity.

@vrothberg

This comment has been minimized.

Show comment
Hide comment
@vrothberg

vrothberg Aug 28, 2018

@stevvooe, @dmcgowan, thanks for your comments. There are still some details left I find worth discussing. Please drop an ACK or a NACK + explanation to the following points (also, feel free to comment and clarify):

  • stripping: the entire prefix will be stripped from an image reference, while the remainder gets appended to the specified push/pull URL. If a user wants to translate namespaces, the prefix must contain the corresponding URI path plus the /v2 substring.

  • matching: a Prefix can be specified only once and the longest Prefix wins during endpoint lookup. Namespace translation requires to match only at path boundaries (i.e., /).

    • An image reference has more boundaries than the path, such as the tag boundary (:) and the digest boundary (@) but we do not support those.
    • A Prefix and a URL is expected to end either with something different than @ or :; if it doesn't end with / it will be appended.
    • The above points imply that a Prefix can not be a fully qualified image reference.
  • push/pull endpoints: an endpoint can be specified multiple times as a push as well as a pull endpoint and may also serve as a "prefix". This way, we do not restrict users and give them full flexibility at the cost that they need to make sure the config is sane. We avoid "mirror loops" by performing only one endpoint lookup. We do not perform additional endpoint lookups for mirrors, but only once for a given image reference based on the set of Prefixes.

  • Insecure registry: a registry can be marked as insecure by specifying the http URI scheme in the corresponding prefix (e.g., "Prefix": "http://insecure.registry.org:5000"). Unless specified, a Prefix will default to the https scheme.

  • URI schemes: we expect a specified URL to either start with the http or the https URI scheme. URL will be renamed to Address to support other transports in the future (e.g. Unix sockets), see comment https://github.com/moby/moby/pull/34319/files#r168667767 (Cc: @AkihiroSuda). Future PRs must make sure, that input validation is transport-specific.

  • InsecureSkipVerify: prefixes and mirrors have a dedicated field InsecureSkipVerify to support skipping TLS verification as suggested by @thaJeztah, see comment https://github.com/moby/moby/pull/34319/files#r168667767:

    • If InsecureSkipVerify is true, TLS accepts any certificate presented by the server and any host name in that certificate. In this mode, TLS is susceptible to man-in-the-middle attacks. This should be used only for testing.
    • The daemon will warn when different security settings or URI schemes are used but will not reject them as there are usecases that require such a setup.

That's all I can think of at the moment. Once we're set on the above points, I will update the code.

Heads up: I'll be on my honeymoon the last three weeks of September, but I will make sure to push things forward as much as possible until then.

vrothberg commented Aug 28, 2018

@stevvooe, @dmcgowan, thanks for your comments. There are still some details left I find worth discussing. Please drop an ACK or a NACK + explanation to the following points (also, feel free to comment and clarify):

  • stripping: the entire prefix will be stripped from an image reference, while the remainder gets appended to the specified push/pull URL. If a user wants to translate namespaces, the prefix must contain the corresponding URI path plus the /v2 substring.

  • matching: a Prefix can be specified only once and the longest Prefix wins during endpoint lookup. Namespace translation requires to match only at path boundaries (i.e., /).

    • An image reference has more boundaries than the path, such as the tag boundary (:) and the digest boundary (@) but we do not support those.
    • A Prefix and a URL is expected to end either with something different than @ or :; if it doesn't end with / it will be appended.
    • The above points imply that a Prefix can not be a fully qualified image reference.
  • push/pull endpoints: an endpoint can be specified multiple times as a push as well as a pull endpoint and may also serve as a "prefix". This way, we do not restrict users and give them full flexibility at the cost that they need to make sure the config is sane. We avoid "mirror loops" by performing only one endpoint lookup. We do not perform additional endpoint lookups for mirrors, but only once for a given image reference based on the set of Prefixes.

  • Insecure registry: a registry can be marked as insecure by specifying the http URI scheme in the corresponding prefix (e.g., "Prefix": "http://insecure.registry.org:5000"). Unless specified, a Prefix will default to the https scheme.

  • URI schemes: we expect a specified URL to either start with the http or the https URI scheme. URL will be renamed to Address to support other transports in the future (e.g. Unix sockets), see comment https://github.com/moby/moby/pull/34319/files#r168667767 (Cc: @AkihiroSuda). Future PRs must make sure, that input validation is transport-specific.

  • InsecureSkipVerify: prefixes and mirrors have a dedicated field InsecureSkipVerify to support skipping TLS verification as suggested by @thaJeztah, see comment https://github.com/moby/moby/pull/34319/files#r168667767:

    • If InsecureSkipVerify is true, TLS accepts any certificate presented by the server and any host name in that certificate. In this mode, TLS is susceptible to man-in-the-middle attacks. This should be used only for testing.
    • The daemon will warn when different security settings or URI schemes are used but will not reject them as there are usecases that require such a setup.

That's all I can think of at the moment. Once we're set on the above points, I will update the code.

Heads up: I'll be on my honeymoon the last three weeks of September, but I will make sure to push things forward as much as possible until then.

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Aug 29, 2018

Contributor

Thanks for listing these out!

The details look close but I'll clarify a bit. The goal of this config is to declare a mapping of a canonical reference to a URL. Thus, we aren't doing a namespace translation, but rather a Reference to URL lookup and translation.

  • stripping: the entire prefix will be stripped from an image reference, while the remainder gets appended to the specified push/pull URL. If a user wants to translate namespaces, the prefix must contain the corresponding URI path plus the /v2 substring.

The prefix should be stripped from the resulting url to get the path in the registry. The input to this function is the reference and the output is the url. Example:

prefix := "docker.io/library"
reference := "docker.io/library/ubuntu:latest"
base := "registry-1.docker.io/v2" // we can have logic to add "v2/" to urls without paths

remainder := strip(reference, prefix) // remainder := "ubuntu", note we drop the tag
url := join(base, remainder)

Hopefully, the above psuedocode clarifies the process.

  • matching: a Prefix can be specified only once and the longest Prefix wins during endpoint lookup. Namespace translation requires to match only at path boundaries (i.e., /).
  • An image reference has more boundaries than the path, such as the tag boundary (:) and the digest boundary (@) but we do not support those.
  • A Prefix and a URL is expected to end either with something different than @ or :; if it doesn't end with / it will be appended.

For now, restrict it to longest match, anchored to the start. The match is of the "prefix" against the image reference host and path component. @ and : are lopped off for this process. Example:

reference := "docker.io/library/ubuntu:latest"
p := prepare("docker.io/library/ubuntu:latest") // -> docker.io/library/ubuntu, lops the tag/digest
match("docker.io/library", p) // -> true
  • The above points imply that a Prefix can not be a fully qualified image reference.

ACK!

  • push/pull endpoints: an endpoint can be specified multiple times as a push as well as a pull endpoint and may also serve as a "prefix". This way, we do not restrict users and give them full flexibility at the cost that they need to make sure the config is sane. We avoid "mirror loops" by performing only one endpoint lookup. We do not perform additional endpoint lookups for mirrors, but only once for a given image reference based on the set of Prefixes.

ACK!

For push, limit it to the first endpoint matched. For pull, fallback until it is found, in order of match appearance. If either of these adds a large amount of code, defer to another PR, as I don't think its super important for getting this feature in.

  • Insecure registry: a registry can be marked as insecure by specifying the http URI scheme in the corresponding prefix (e.g., "Prefix": "http://insecure.registry.org:5000"). Unless specified, a Prefix will default to the https scheme.

ACK!

  • URI schemes: we expect a specified URL to either start with the http or the https URI scheme. URL will be renamed to Address to support other transports in the future (e.g. Unix sockets), see comment https://github.com/moby/moby/pull/34319/files#r168667767 (Cc: @AkihiroSuda). Future PRs must make sure, that input validation is transport-specific.

ACK!

Note that we should autofill /v2 if no path is specified.

-> InsecureSkipVerify: prefixes and mirrors have a dedicated field InsecureSkipVerify to support skipping TLS verification as suggested by @thaJeztah, see comment https://github.com/moby/moby/pull/34319/files#r168667767:

  • If InsecureSkipVerify is true, TLS accepts any certificate presented by the server and any host name in that certificate. In this mode, TLS is susceptible to man-in-the-middle attacks. This should be used only for testing.
  • The daemon will warn when different security settings or URI schemes are used but will not reject them as there are usecases that require such a setup.

ACK!

I am also fine with allowing this configuration per registry:

type Registry struct {
    URL string
    InsecureSkipVerify bool
}

Let me know if you have questions!

Contributor

stevvooe commented Aug 29, 2018

Thanks for listing these out!

The details look close but I'll clarify a bit. The goal of this config is to declare a mapping of a canonical reference to a URL. Thus, we aren't doing a namespace translation, but rather a Reference to URL lookup and translation.

  • stripping: the entire prefix will be stripped from an image reference, while the remainder gets appended to the specified push/pull URL. If a user wants to translate namespaces, the prefix must contain the corresponding URI path plus the /v2 substring.

The prefix should be stripped from the resulting url to get the path in the registry. The input to this function is the reference and the output is the url. Example:

prefix := "docker.io/library"
reference := "docker.io/library/ubuntu:latest"
base := "registry-1.docker.io/v2" // we can have logic to add "v2/" to urls without paths

remainder := strip(reference, prefix) // remainder := "ubuntu", note we drop the tag
url := join(base, remainder)

Hopefully, the above psuedocode clarifies the process.

  • matching: a Prefix can be specified only once and the longest Prefix wins during endpoint lookup. Namespace translation requires to match only at path boundaries (i.e., /).
  • An image reference has more boundaries than the path, such as the tag boundary (:) and the digest boundary (@) but we do not support those.
  • A Prefix and a URL is expected to end either with something different than @ or :; if it doesn't end with / it will be appended.

For now, restrict it to longest match, anchored to the start. The match is of the "prefix" against the image reference host and path component. @ and : are lopped off for this process. Example:

reference := "docker.io/library/ubuntu:latest"
p := prepare("docker.io/library/ubuntu:latest") // -> docker.io/library/ubuntu, lops the tag/digest
match("docker.io/library", p) // -> true
  • The above points imply that a Prefix can not be a fully qualified image reference.

ACK!

  • push/pull endpoints: an endpoint can be specified multiple times as a push as well as a pull endpoint and may also serve as a "prefix". This way, we do not restrict users and give them full flexibility at the cost that they need to make sure the config is sane. We avoid "mirror loops" by performing only one endpoint lookup. We do not perform additional endpoint lookups for mirrors, but only once for a given image reference based on the set of Prefixes.

ACK!

For push, limit it to the first endpoint matched. For pull, fallback until it is found, in order of match appearance. If either of these adds a large amount of code, defer to another PR, as I don't think its super important for getting this feature in.

  • Insecure registry: a registry can be marked as insecure by specifying the http URI scheme in the corresponding prefix (e.g., "Prefix": "http://insecure.registry.org:5000"). Unless specified, a Prefix will default to the https scheme.

ACK!

  • URI schemes: we expect a specified URL to either start with the http or the https URI scheme. URL will be renamed to Address to support other transports in the future (e.g. Unix sockets), see comment https://github.com/moby/moby/pull/34319/files#r168667767 (Cc: @AkihiroSuda). Future PRs must make sure, that input validation is transport-specific.

ACK!

Note that we should autofill /v2 if no path is specified.

-> InsecureSkipVerify: prefixes and mirrors have a dedicated field InsecureSkipVerify to support skipping TLS verification as suggested by @thaJeztah, see comment https://github.com/moby/moby/pull/34319/files#r168667767:

  • If InsecureSkipVerify is true, TLS accepts any certificate presented by the server and any host name in that certificate. In this mode, TLS is susceptible to man-in-the-middle attacks. This should be used only for testing.
  • The daemon will warn when different security settings or URI schemes are used but will not reject them as there are usecases that require such a setup.

ACK!

I am also fine with allowing this configuration per registry:

type Registry struct {
    URL string
    InsecureSkipVerify bool
}

Let me know if you have questions!

@vrothberg

This comment has been minimized.

Show comment
Hide comment
@vrothberg

vrothberg Aug 29, 2018

@stevvooe, thanks for the quick response. I am having a good feeling, and just some small follow-up questions that you can find below.

prefix := "docker.io/library"
reference := "docker.io/library/ubuntu:latest"
base := "registry-1.docker.io/v2" // we can have logic to add "v2/" to urls without paths

remainder := strip(reference, prefix) // remainder := "ubuntu", note we drop the tag
url := join(base, remainder)

Can you elaborate on why the tag is dropped?

matching: [...]

For now, restrict it to longest match, anchored to the start. The match is of the "prefix" against the image reference host and path component. @ and : are lopped off for this process.

reference := "docker.io/library/ubuntu:latest"
p := prepare("docker.io/library/ubuntu:latest") // -> docker.io/library/ubuntu, lops the tag/digest
match("docker.io/library", p) // -> true

Same here: Why is tag/digest dropped from the reference?

push/pull endpoints:
[...]
For push, limit it to the first endpoint matched. For pull, fallback until it is found, in order of match appearance. If either of these adds a large amount of code, defer to another PR, as I don't think its super important for getting this feature in.

Thanks for bringing that up. I missed that in the list:

  • pulling: The logic for pulling is already there. We'll pull from the slice of endpoints until a successful pull.
  • pushing: If we limit it to the first endpoint, then there can be at most one push endpoint for now. Is that correct?

I support the idea of deferring some things to another PR, especially the push semantics can be extended to meet more usecases.

Note that we should autofill /v2 if no path is specified.

The registry has a Prefix option that allows a given registry to run at a different root (e.g., "my-registry.com:5000/some/path/root"). Autofilling would fail here. Is this something we want to support or do we expect a registry to always run at the default root?

I am also fine with allowing this configuration per registry:

type Registry struct {
    URL string
    InsecureSkipVerify bool
}

Great! I think we need the same for type Mirror struct as well. @thaJeztah will like that.

vrothberg commented Aug 29, 2018

@stevvooe, thanks for the quick response. I am having a good feeling, and just some small follow-up questions that you can find below.

prefix := "docker.io/library"
reference := "docker.io/library/ubuntu:latest"
base := "registry-1.docker.io/v2" // we can have logic to add "v2/" to urls without paths

remainder := strip(reference, prefix) // remainder := "ubuntu", note we drop the tag
url := join(base, remainder)

Can you elaborate on why the tag is dropped?

matching: [...]

For now, restrict it to longest match, anchored to the start. The match is of the "prefix" against the image reference host and path component. @ and : are lopped off for this process.

reference := "docker.io/library/ubuntu:latest"
p := prepare("docker.io/library/ubuntu:latest") // -> docker.io/library/ubuntu, lops the tag/digest
match("docker.io/library", p) // -> true

Same here: Why is tag/digest dropped from the reference?

push/pull endpoints:
[...]
For push, limit it to the first endpoint matched. For pull, fallback until it is found, in order of match appearance. If either of these adds a large amount of code, defer to another PR, as I don't think its super important for getting this feature in.

Thanks for bringing that up. I missed that in the list:

  • pulling: The logic for pulling is already there. We'll pull from the slice of endpoints until a successful pull.
  • pushing: If we limit it to the first endpoint, then there can be at most one push endpoint for now. Is that correct?

I support the idea of deferring some things to another PR, especially the push semantics can be extended to meet more usecases.

Note that we should autofill /v2 if no path is specified.

The registry has a Prefix option that allows a given registry to run at a different root (e.g., "my-registry.com:5000/some/path/root"). Autofilling would fail here. Is this something we want to support or do we expect a registry to always run at the default root?

I am also fine with allowing this configuration per registry:

type Registry struct {
    URL string
    InsecureSkipVerify bool
}

Great! I think we need the same for type Mirror struct as well. @thaJeztah will like that.

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Aug 30, 2018

Contributor

@vrothberg

Can you elaborate on why the tag is dropped?

We are not assembling a new reference, we are assembling a base URL for the repository in the registry 😄. The semantics of addressing tags and manifests are then API operations, mounted at that base path. The tag or digest gets re-added when you operate on the url in the rest of the pull process.

pushing: If we limit it to the first endpoint, then there can be at most one push endpoint for now. Is that correct?

Yes. If you think you can get this feature into this PR, great, but I don't think we should block on this feature. The future goal here is to support "broadcast mirroring"

Autofilling would fail here. Is this something we want to support or do we expect a registry to always run at the default root?

If you autofill only when there is no path component, this works fine. For example, if you put http://localhost:5000/my-prefix/v2, there should be no autofill. This will allow for registry configurations that don't run at root (has been a part of the API for a long time, but never supported in docker).

In practice, most registry configurations will be https://<host> and will just get autofilled. I think we should also tolerate /, unless there is a good reason not to. We can also introduce a parameter to control this behavior, but I think these behaviors will cover most use cases for now.

Great! I think we need the same for type Mirror struct as well. @thaJeztah will like that.

Wouldn't a mirror just be another item in the list?

Contributor

stevvooe commented Aug 30, 2018

@vrothberg

Can you elaborate on why the tag is dropped?

We are not assembling a new reference, we are assembling a base URL for the repository in the registry 😄. The semantics of addressing tags and manifests are then API operations, mounted at that base path. The tag or digest gets re-added when you operate on the url in the rest of the pull process.

pushing: If we limit it to the first endpoint, then there can be at most one push endpoint for now. Is that correct?

Yes. If you think you can get this feature into this PR, great, but I don't think we should block on this feature. The future goal here is to support "broadcast mirroring"

Autofilling would fail here. Is this something we want to support or do we expect a registry to always run at the default root?

If you autofill only when there is no path component, this works fine. For example, if you put http://localhost:5000/my-prefix/v2, there should be no autofill. This will allow for registry configurations that don't run at root (has been a part of the API for a long time, but never supported in docker).

In practice, most registry configurations will be https://<host> and will just get autofilled. I think we should also tolerate /, unless there is a good reason not to. We can also introduce a parameter to control this behavior, but I think these behaviors will cover most use cases for now.

Great! I think we need the same for type Mirror struct as well. @thaJeztah will like that.

Wouldn't a mirror just be another item in the list?

@vrothberg

This comment has been minimized.

Show comment
Hide comment
@vrothberg

vrothberg Aug 30, 2018

We are not assembling a new reference, we are assembling a base URL for the repository in the registry smile. The semantics of addressing tags and manifests are then API operations, mounted at that base path. The tag or digest gets re-added when you operate on the url in the rest of the pull process.

👍 that's right, thanks for the clarification.

Wouldn't a mirror just be another item in the list?

I would have a dedicated type for mirrors and have a slice of mirrors as a field within a registry. This allows a fast lookup for push and pull mirrors. It makes things also easier when adding support for Unix sockets for mirrors (Cc @AkihiroSuda).

vrothberg commented Aug 30, 2018

We are not assembling a new reference, we are assembling a base URL for the repository in the registry smile. The semantics of addressing tags and manifests are then API operations, mounted at that base path. The tag or digest gets re-added when you operate on the url in the rest of the pull process.

👍 that's right, thanks for the clarification.

Wouldn't a mirror just be another item in the list?

I would have a dedicated type for mirrors and have a slice of mirrors as a field within a registry. This allows a fast lookup for push and pull mirrors. It makes things also easier when adding support for Unix sockets for mirrors (Cc @AkihiroSuda).

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Aug 30, 2018

Contributor

I would have a dedicated type for mirrors and have a slice of mirrors as a field within a registry. This allows a fast lookup for push and pull mirrors. It makes things also easier when adding support for Unix sockets for mirrors (Cc @AkihiroSuda).

What is the difference between a mirror and a registry in the push/pull list? They are the same.

Contributor

stevvooe commented Aug 30, 2018

I would have a dedicated type for mirrors and have a slice of mirrors as a field within a registry. This allows a fast lookup for push and pull mirrors. It makes things also easier when adding support for Unix sockets for mirrors (Cc @AkihiroSuda).

What is the difference between a mirror and a registry in the push/pull list? They are the same.

@vrothberg

This comment has been minimized.

Show comment
Hide comment
@vrothberg

vrothberg Aug 31, 2018

What is the difference between a mirror and a registry in the push/pull list? They are the same.

My assumption was that the URL -> Address change to support unix sockets should only apply to mirrors. But that doesn't seem to be the case, so there is no difference which I support 100 percent.

vrothberg commented Aug 31, 2018

What is the difference between a mirror and a registry in the push/pull list? They are the same.

My assumption was that the URL -> Address change to support unix sockets should only apply to mirrors. But that doesn't seem to be the case, so there is no difference which I support 100 percent.

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Aug 31, 2018

Contributor

@vrothberg I think we can use the addition of a separate field for that use case.

Contributor

stevvooe commented Aug 31, 2018

@vrothberg I think we can use the addition of a separate field for that use case.

@vrothberg

This comment has been minimized.

Show comment
Hide comment
@vrothberg

vrothberg Sep 6, 2018

@stevvooe two additional issues came up while working on the PR.

Official namespace

As @thaJeztah has pointed out in #34319 (comment), the daemon is mixing hostnames and image pointing to the Docker hub in way that docker pull registry-1.docker.io/library/foo yields the same image as docker pull docker.io/library/foo and docker pull foo. Is that something we should "fix" to allow only "docker.io" as a valid namespace and not translate "registry-1.docker.io" to "docker.io"?

Authentication to pull/push registries

Let's consider a very simple example:

  • prefix = "https://docker.io/library"
  • pull = ["https://pull-registry.com/docker.io/library"] with pull-registry.com/docker.io/library requiring authentication

In this scenario, the authentication will not work as the client will pass the AuthConfig for "docker.io" but the daemon will use the specified pull registry first and tries to pull from "pull-registry.com/docker.io/library" where the AuthConfig won't work.

All credentials are stored on the client side which makes sense for security reasons, and passing the AuthConfig based on the initial image name/reference is baked into the entire API but that doesn't allow us to cover the above use case.

@stevvooe what's your take on this? Currently, I am torn between not supporting authentication for push and pull registries, and extending the config to allow specifying credentials (or sth similar), e.g.:

"pull": [
{
    "address": "https://pull-reg-with-auth.com:5000/",
    "user": "xxxx",
    "password": "xxxxxx"
}
]

vrothberg commented Sep 6, 2018

@stevvooe two additional issues came up while working on the PR.

Official namespace

As @thaJeztah has pointed out in #34319 (comment), the daemon is mixing hostnames and image pointing to the Docker hub in way that docker pull registry-1.docker.io/library/foo yields the same image as docker pull docker.io/library/foo and docker pull foo. Is that something we should "fix" to allow only "docker.io" as a valid namespace and not translate "registry-1.docker.io" to "docker.io"?

Authentication to pull/push registries

Let's consider a very simple example:

  • prefix = "https://docker.io/library"
  • pull = ["https://pull-registry.com/docker.io/library"] with pull-registry.com/docker.io/library requiring authentication

In this scenario, the authentication will not work as the client will pass the AuthConfig for "docker.io" but the daemon will use the specified pull registry first and tries to pull from "pull-registry.com/docker.io/library" where the AuthConfig won't work.

All credentials are stored on the client side which makes sense for security reasons, and passing the AuthConfig based on the initial image name/reference is baked into the entire API but that doesn't allow us to cover the above use case.

@stevvooe what's your take on this? Currently, I am torn between not supporting authentication for push and pull registries, and extending the config to allow specifying credentials (or sth similar), e.g.:

"pull": [
{
    "address": "https://pull-reg-with-auth.com:5000/",
    "user": "xxxx",
    "password": "xxxxxx"
}
]
@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Sep 11, 2018

Contributor

@vrothberg Per our conversation, I need to clarify that the prefix is not a url, but an image reference prefix. The determination of insecure/secure should be done with the scheme of the URL in the Registry struct, not the prefix used to match it.

As far as the propagation of auth credentials is concerned, I don't have a solid suggestion there. The existing behavior of AuthConfig needs to be preserved, but newer clients are going to have to implement a protocol to resolve the required authentication before starting the operation. I haven't thought this aspect through, in detail, but this needs to be explored before moving forward.

Contributor

stevvooe commented Sep 11, 2018

@vrothberg Per our conversation, I need to clarify that the prefix is not a url, but an image reference prefix. The determination of insecure/secure should be done with the scheme of the URL in the Registry struct, not the prefix used to match it.

As far as the propagation of auth credentials is concerned, I don't have a solid suggestion there. The existing behavior of AuthConfig needs to be preserved, but newer clients are going to have to implement a protocol to resolve the required authentication before starting the operation. I haven't thought this aspect through, in detail, but this needs to be explored before moving forward.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Sep 13, 2018

Member

Discussing this in the maintainers meeting with @tiborvass @tonistiigi @dmcgowan @cpuguy83;

  • (if this was not commented yet (sorry it's a lengthy discussion, so possibly missed it)); we're good with doing the prefix-stripping 😄
  • We think it's good to reduce the "magic" handling of URL's to a minimum. So not automatically append /v2 (even if no path is in the configured registry URL) - use the URL as-is. This keeps the way open to support registries that don't use a /v2 in the URL (which is not a requirement according to the specs). It's also easier to add this later than to remove it
  • Don't change authentication; authentication is used for the registry where push/pull is done. Keep the current mechanisms to obtain the correct authentication information. In future we should look at better ways to query / send authentication
Member

thaJeztah commented Sep 13, 2018

Discussing this in the maintainers meeting with @tiborvass @tonistiigi @dmcgowan @cpuguy83;

  • (if this was not commented yet (sorry it's a lengthy discussion, so possibly missed it)); we're good with doing the prefix-stripping 😄
  • We think it's good to reduce the "magic" handling of URL's to a minimum. So not automatically append /v2 (even if no path is in the configured registry URL) - use the URL as-is. This keeps the way open to support registries that don't use a /v2 in the URL (which is not a requirement according to the specs). It's also easier to add this later than to remove it
  • Don't change authentication; authentication is used for the registry where push/pull is done. Keep the current mechanisms to obtain the correct authentication information. In future we should look at better ways to query / send authentication
@vrothberg

This comment has been minimized.

Show comment
Hide comment
@vrothberg

vrothberg Sep 15, 2018

  • Don't change authentication; authentication is used for the registry where push/pull is done. Keep the current mechanisms to obtain the correct authentication information. In future we should look at better ways to query / send authentication

@thaJeztah, thanks for the update. Does this imply not adding user/password to a push/pull registry in the config? I am currently on the road, but I already had something working as follows:

{ ...
"pull": [
{
"address": "https://pull-mirror.com:5000",
"user": "xxx",
"password": "yyy",
}
]}

Supporting that was rather straight forward and did not require very intrusive changes (Cc @stevvooe ). Having credentials in clear text is not ideal, but it seems to be the only backwards compatible solution at the moment. In the future, we can work on a client-side solution but that seems like a rather long journey given the API needs to change (likely also the CRI for K8s etc.).

vrothberg commented Sep 15, 2018

  • Don't change authentication; authentication is used for the registry where push/pull is done. Keep the current mechanisms to obtain the correct authentication information. In future we should look at better ways to query / send authentication

@thaJeztah, thanks for the update. Does this imply not adding user/password to a push/pull registry in the config? I am currently on the road, but I already had something working as follows:

{ ...
"pull": [
{
"address": "https://pull-mirror.com:5000",
"user": "xxx",
"password": "yyy",
}
]}

Supporting that was rather straight forward and did not require very intrusive changes (Cc @stevvooe ). Having credentials in clear text is not ideal, but it seems to be the only backwards compatible solution at the moment. In the future, we can work on a client-side solution but that seems like a rather long journey given the API needs to change (likely also the CRI for K8s etc.).

@markgalpin

This comment has been minimized.

Show comment
Hide comment
@markgalpin

markgalpin Sep 16, 2018

@vrothberg Personally, I would prefer that you perform 'docker login' to the relevant private registry with its URL as usual (i.e. docker login private.registry.com:81) and then the mirror config automatically selects the correct location when it rewrites the address. In an ideal world, docker login will now take full URLs like the mirrors do. This does mean that the mirror isn't entirely 'hidden' from the user, but I think that's okay.
I suppose we could have it automatically rewrite so when you did 'docker login' it automatically rewrote the mirrored URL based on the rules you configure. But that seems like it could get tricky, and very confusing in the creds file (definitely the authentication file should store creds based on the REAL url to the mirror, not the one the user types)
This way we have the existing secret handling protection and other mechanisms we're currently used to. I don't view plaintext credentials as a very good choice.

markgalpin commented Sep 16, 2018

@vrothberg Personally, I would prefer that you perform 'docker login' to the relevant private registry with its URL as usual (i.e. docker login private.registry.com:81) and then the mirror config automatically selects the correct location when it rewrites the address. In an ideal world, docker login will now take full URLs like the mirrors do. This does mean that the mirror isn't entirely 'hidden' from the user, but I think that's okay.
I suppose we could have it automatically rewrite so when you did 'docker login' it automatically rewrote the mirrored URL based on the rules you configure. But that seems like it could get tricky, and very confusing in the creds file (definitely the authentication file should store creds based on the REAL url to the mirror, not the one the user types)
This way we have the existing secret handling protection and other mechanisms we're currently used to. I don't view plaintext credentials as a very good choice.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Sep 16, 2018

Contributor

The main point is we want to keep credential handling out of this PR because it is a very different thing to tackle and we don't want to tack on even more mess to the existing a auth system in Docker.

I think also for this particular PR, if the client needs to be updated at all then that should be split up into a separate discussion. So far this is just changing how the daemon implementation if pull works.
Anything else is effectively a protocol change (protocol as in how the client and daemon interact) which requires further discussion...though we'd definitely like to see some improvements here.

Contributor

cpuguy83 commented Sep 16, 2018

The main point is we want to keep credential handling out of this PR because it is a very different thing to tackle and we don't want to tack on even more mess to the existing a auth system in Docker.

I think also for this particular PR, if the client needs to be updated at all then that should be split up into a separate discussion. So far this is just changing how the daemon implementation if pull works.
Anything else is effectively a protocol change (protocol as in how the client and daemon interact) which requires further discussion...though we'd definitely like to see some improvements here.

@vrothberg

This comment has been minimized.

Show comment
Hide comment
@vrothberg

vrothberg Sep 18, 2018

@vrothberg Personally, I would prefer that you perform 'docker login' to the relevant private registry with its URL as usual (i.e. docker login private.registry.com:81) and then the mirror config automatically selects the correct location when it rewrites the address. In an ideal world, docker login will now take full URLs like the mirrors do. This does mean that the mirror isn't entirely 'hidden' from the user, but I think that's okay.

@markgalpin I agree, that would the best solution. The problem is that this does not work with the current API as the clients don't know that the image is being pulled from a different registry with potentially different creds. Changing the API and all clients looks like a rather long adventure, and I would really like to have the authentication work for this specific use case. A daemon-only config option is transparent to the clients (no change required) and can be deprecated once the API and the clients are updated. A very common use case now is Kubernetes, which requires additional chanages to dockershim until all users could benefit from the feature.

The main point is we want to keep credential handling out of this PR because it is a very different thing to tackle and we don't want to tack on even more mess to the existing an auth system in Docker.

@cpuguy83, I fear that it will end up for a downstream patch on our side as customers have this requirement, but the decision is up to you (the maintainers). Specifying the creds in the config is optional and it allows to support the use case of a repository with authentication while not changing any client (e.g., docker cli, docker shim, etc.) or the protocol. However, I am absolutely on your side that it should be handled by clients and that's the way to go in the future. Once that is working, we could deprecate the creds in the config (or ignore them with a big warning).

@cpuguy83 Is a temporary server-side config solution acceptable to you? Sorry in case we're turning in circles, but I just want to be sure.

vrothberg commented Sep 18, 2018

@vrothberg Personally, I would prefer that you perform 'docker login' to the relevant private registry with its URL as usual (i.e. docker login private.registry.com:81) and then the mirror config automatically selects the correct location when it rewrites the address. In an ideal world, docker login will now take full URLs like the mirrors do. This does mean that the mirror isn't entirely 'hidden' from the user, but I think that's okay.

@markgalpin I agree, that would the best solution. The problem is that this does not work with the current API as the clients don't know that the image is being pulled from a different registry with potentially different creds. Changing the API and all clients looks like a rather long adventure, and I would really like to have the authentication work for this specific use case. A daemon-only config option is transparent to the clients (no change required) and can be deprecated once the API and the clients are updated. A very common use case now is Kubernetes, which requires additional chanages to dockershim until all users could benefit from the feature.

The main point is we want to keep credential handling out of this PR because it is a very different thing to tackle and we don't want to tack on even more mess to the existing an auth system in Docker.

@cpuguy83, I fear that it will end up for a downstream patch on our side as customers have this requirement, but the decision is up to you (the maintainers). Specifying the creds in the config is optional and it allows to support the use case of a repository with authentication while not changing any client (e.g., docker cli, docker shim, etc.) or the protocol. However, I am absolutely on your side that it should be handled by clients and that's the way to go in the future. Once that is working, we could deprecate the creds in the config (or ignore them with a big warning).

@cpuguy83 Is a temporary server-side config solution acceptable to you? Sorry in case we're turning in circles, but I just want to be sure.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Sep 18, 2018

Contributor

I don't think we want to store creds in plain text in the config file.

Contributor

cpuguy83 commented Sep 18, 2018

I don't think we want to store creds in plain text in the config file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment