-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Add namespace query parameter for mirrors #3451
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
Conversation
|
Build succeeded.
|
Codecov Report
@@ Coverage Diff @@
## master #3451 +/- ##
==========================================
+ Coverage 44.11% 44.11% +<.01%
==========================================
Files 124 124
Lines 13760 13794 +34
==========================================
+ Hits 6070 6085 +15
- Misses 6759 6774 +15
- Partials 931 935 +4
Continue to review full report at Codecov.
|
Mirrored registries are design to serve content from upstreams. However, the mirror hostname will usually not match the hostname of the upstream, requiring the mirror to only use a single upstream or use its own pattern matching to determine the upstream. To solve this issue, the client will pass along the namespace which is being used for the request, allowing mirrors to easily map to multiple upstreams. This query parameter can safely be ignored if multiple upstreams are not supported. Signed-off-by: Derek McGowan <[email protected]>
7538d45 to
078c2b2
Compare
|
Build succeeded.
|
|
@dmcgowan With today's containerd, the client is configured to talk with a specific registry In this model, the mirror will get the original image ref, thus it will know whatever the upstream is, right? I guess you don't like the regexp match of |
|
The problem with In this model, the mirror will get the upstream host information via a query parameter, which can be safely ignored if the mirror does not support arbitrary or multiple upstreams. (A path component cannot be ignored 😄 ) |
@dmcgowan Hm, my bad. I didn't pay attention to the protocol before. I thought that we are just talking to different mirrors, and request However, as you said (and based on the log and code), it seems that todays implementation is Then adding |
|
@Random-Liu I see in your mirror PR on CRI you don't add the push capability to the hosts. This code would see that and add the query parameter on all requests from CRI. While it is harmless I am not sure if that is ideal |
|
@dmcgowan Kubernetes/CRI doesn't need the push capability from the client side. If we want to add push capability, I think we can only add it to the default host. |
| var q url.Values | ||
| // Parse query | ||
| if i := strings.IndexByte(r.path, '?'); i > 0 { | ||
| r.path = r.path[:i+1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should parse first and then truncate the path
|
Going to close this for now since it is not mergeable, changes need to be made for configurable and better logic for determining whether a registry is a global mirror. |
Mirrored registries are design to serve content from upstreams. However, the mirror hostname will usually not match the hostname of the upstream, requiring the mirror to only use a single upstream or use its own pattern matching to determine the upstream. To solve this issue, the client will pass along the namespace which is being used for the request, allowing mirrors to easily map to multiple upstreams. This query parameter can safely be ignored if multiple upstreams are not supported.
Marking as draft to move along conversation in opencontainers/distribution-spec#12