-
Notifications
You must be signed in to change notification settings - Fork 18.6k
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
Client Support for Docker Registry HTTP API V2 #9784
Conversation
ab1f5de
to
d35b58a
Compare
checkout drone ;) |
return nil, fmt.Errorf("unable to decode GetV2Version JSON response: %s", err) | ||
var registry *Endpoint | ||
if r.indexEndpoint.URL.Host == IndexServerURL.Host { | ||
registry, err = NewEndpoint(REGISTRYSERVER, nil) |
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.
This makes falling back to the original URL problematic.
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.
And is probably what is breaking the tests right now. I'll try to fix.
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.
fix is over on @dmcgowan's fork
@@ -156,8 +168,13 @@ func (s *TagStore) CmdPull(job *engine.Job) engine.Status { | |||
} else if err != registry.ErrDoesNotExist { | |||
log.Errorf("Error from V2 registry: %s", err) |
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.
Do we still want to fall back in this case?
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.
We should only not falling back when verification is a strict requirement. Are there any other problems you can think of by falling back?
@@ -77,6 +79,23 @@ func main() { | |||
} | |||
protoAddrParts := strings.SplitN(flHosts[0], "://", 2) | |||
|
|||
err := os.MkdirAll(path.Dir(*flTrustKey), 0700) |
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.
this could be on one line, that's a minor nit i know... so feel free to ignore
ok i did a first round of comments but I will look over again in more detail |
Tag: tag, | ||
SchemaVersion: 1, | ||
} | ||
localRepo, exists := s.Repositories[localName] |
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.
Do we need to take a lock on Repositories
?
I just finished a first review pass, but I'll need more time to digest all these and get more familiar with this part of the code. My first "raw" impressions:
|
} | ||
setTokenAuth(req, token) | ||
res, _, err := r.doRequest(req) | ||
queryParams := url.Values{} |
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.
These parameter additions need to be additive. With this approach, any query parameters in the Location
header will be destroyed. The code should be something like this:
query := req.URL.Query()
query.Add("digest", sumType+":"+sumStr)
req.URL.RawQuery = query.Encode()
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Signed-off-by: Alexander Morozov <lk4d4@docker.com>
Signed-off-by: Alexander Morozov <lk4d4@docker.com>
Signed-off-by: Alexander Morozov <lk4d4@docker.com>
Signed-off-by: Arnaud Porterie <arnaud.porterie@docker.com>
Signed-off-by: Arnaud Porterie <arnaud.porterie@docker.com>
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
The v2 session code will no longer update the indexEndpoint value, therefore it is not necessary to save and restore the value for use with v1. Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Token cache prevents the need to get a new token for every registry interaction. Since the tokens are short lived, the cache expires after only a minute. Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Also just because this is such a large change, I have hooked up @dmcgowan's branch to run on jenkins here https://jenkins.dockerproject.com/job/Docker%20PR%20Test/ |
LGTM |
LGTM |
LGTM! @jfrazelle can we please let you proceed with the merge as we have to synchronize with Drone image update? Thanks a lot to everyone involved in reviewing this <3 |
Client Support for Docker Registry HTTP API V2
Thanks for getting this merged! |
This PR updates Docker core to support pushes and pulls of images from registries that support Docker Registry HTTP API V2, as proposed in #9015. Specifically, functionality has been added to support the V2 API while minimizing changes to the end user experience. The reasons behind this move are to improve security and access control by moving to content addressable layers and using sign-able manifests.
This PR affects the following behavior in docker core:
docker login
While the distribution side of docker core may need some refactoring work, the goal of this particular PR is to provide equivalent functionality with the new registry API. Use of a V2 registry should be transparent to the user.
This set of changes will be supported by some accompanying PR:
Overview
Images may now be pushed and pulled from registries, public and private, that implement the Docker Registry HTTP API V2. Using
registry/v2.URLBuilder
, the client code has been ported to use the URI layout described in the proposal. Definitions for routes and error codes from next generation docker registry are included. The rest of the changes are devoted to authentication handling and modifying the flow to support image push.During login, if a registry supports the V2 api, the client will use the base endpoint (“/v2/”) to solicit an authentication challenge. The challenge is then used to login to the appropriate authentication provider. The results are tested against the base endpoint to confirm the credentials are correct. After login, authorization of operations on the registry is maintained using challenges and the stored credentials. If a challenge is issued during an operation against the registry, the client attempts to re-authorize the operation, if possible.
Use of the new image format, covered in #8093, is effectively transparent. Image manifests are created and signed transparently. This paves the way for future image verification features. A new API endpoint has been added to the docker engine to get the image manifest for an image. This endpoint can be used to get the image manifest from a daemon, sign it on the client, and push the signed image. The push API endpoint has also been updated to allow a signed manifest to be sent in the post body.
Pulling images is roughly the same as before, with most modifications to this code path consisting of URL updates and authentication management. For pushing images, the POST + PUT monolithic flow is implemented as proposed.
Testing
There are numerous cases under which the new registry may be used. We suggest that reviewers do take the time to test cases that they care about directly. To ensure that this PR is somewhat ready, we have tested the following combinations against the next generation registry:
If you need help with reproducing these scenarios, please contact us directly.