authprovider: support registrytoken field in docker auth config - #2868
Conversation
| } | ||
|
|
||
| func (ap *authProvider) FetchToken(ctx context.Context, req *auth.FetchTokenRequest) (rr *auth.FetchTokenResponse, err error) { | ||
| ac, err := ap.getAuthConfig(req.Host) |
There was a problem hiding this comment.
Calling authConfig isn't really free because this should be the call that calls credentials helpers. Eg. for gcloud credential helper iirc this call takes hundreds of milliseconds. So we should avoid the cases where this gets called multiple times (atm seems to be called again on line 76).
There was a problem hiding this comment.
I've introduced an authConfigCache field to avoid the redundant (possibly expensive) calls to GetAuthConfig.
There was a problem hiding this comment.
Isn't it simpler if credentials() just takes *types.AuthConfig as parameter or do you see that it already gets called too much?
There was a problem hiding this comment.
I had thought about doing that, and it certainly would be simpler. I can't say under what circumstances my change would be more performant.
However, what might be a real improvement here would be to call GetAuthConfigs() to retrieve auth sections for all repos and set that as authConfigCache. That way, even if a solve is interacting with multiple registries, the retrieval of auth configuration only happens once. This is from my understanding of how the solver sessions work, however, which is very naive at the moment.
What do you think? Shall I expand the change to cache all auth config sections or simplify the single host case to use a parameter?
There was a problem hiding this comment.
Looking again, I do think that the cache map is better than adding a parameter to credentials(). The latter has four different callers (Credentials, GetTokenAuthority/VerifyTokenAuthority(by way of getAuthorityKey), and FetchToken). FWICT without the cache each entry point would result in a call to GetAuthConfig before the result is passed to credentials().
There was a problem hiding this comment.
However, what might be a real improvement here would be to call
GetAuthConfigs()to retrieve auth sections for all repos and set that asauthConfigCache. That way, even if a solve is interacting with multiple registries, the retrieval of auth configuration only happens once. This is from my understanding of how the solver sessions work, however, which is very naive at the moment.
I'm also second guessing this approach. I see now that GetAuthConfigs() does not use the credential store at all. Strange.
The Docker CLI supports a field called "registrytoken" within the auth config that may contain a previously resolved bearer token. If a value is present, it is used verbatim and OAuth token retrieval using username/password or identity token is skipped. Support this same functionality in the buildkit client by checking for this field's value prior to credential based auth in `FetchToken`. If a value is set for `AuthConfig.RegistryToken`, short circuit and return the token value as is. This feature helps to support registry setups that integrate with third party auth systems such as GitLab's JWT OmniAuth provider. Signed-off-by: Dan Duvall <dduvall@wikimedia.org>
Signed-off-by: Dan Duvall <dduvall@wikimedia.org>
0c94720 to
ed05457
Compare
| } | ||
|
|
||
| func (ap *authProvider) getAuthConfig(host string) (*types.AuthConfig, error) { | ||
| if _, exists := ap.authConfigCache[host]; !exists { |
There was a problem hiding this comment.
This map access is not safe as read and write could be happening at the same time.
There was a problem hiding this comment.
Ah, I mistakenly thought that so long as only one thread was writing at a time it would be safe. I will move the mutex.
There was a problem hiding this comment.
Fixed in my follow up.
Signed-off-by: Dan Duvall <dduvall@wikimedia.org>
2d9012d to
c99cb34
Compare
|
Thanks for the approval, @tonistiigi ! Am i right in seeing that "TestIntegration/TestRelativeWorkDir/worker=containerd-snapshotter-stargz" is the only failure? I'm not sure why that would be. I'm looking on my phone's browser however and could easily be missing something. I'll take another look later today |
|
FWIW I haven't been able to reproduce the test failure locally. $ docker buildx build --target integration-tests --output type=docker,name=buildkit-tests --build-arg BUILDKIT_CONTEXT_KEEP_GIT_DIR=1 https://github.com/moby/buildkit.git#refs/pull/2868/merge --progress=plain
$ docker create -v /root/.cache -v /root/.cache/registry -v /go/pkg/mod --name buildkit-test-cache alpine
$ docker run -it -v /tmp -e TEST_DOCKERD -e SKIP_INTEGRATION_TESTS --privileged buildkit-tests go test -v -run TestClientGatewayIntegration/TestClientGatewaySlowCacheExecError/worker=containerd-snapshotter-stargz ./client
TEST_DOCKERD -e SKIP_INTEGRATION_TESTS --privileged buildkit-tests go test -v -run TestClientGatewayIntegration/TestClientGatewaySlowCacheExecError/worker=containerd-snapshotter-stargz ./client
=== RUN TestClientGatewayIntegration
time="2022-05-22T20:49:15Z" level=info msg="trying next host - response was http.StatusNotFound" host="localhost:40205"
run.go:246: copied docker.io/amd64/busybox:latest to local mirror localhost:40205/library/busybox:latest
=== RUN TestClientGatewayIntegration/TestClientGatewaySlowCacheExecError/worker=containerd-snapshotter-stargz
=== PAUSE TestClientGatewayIntegration/TestClientGatewaySlowCacheExecError/worker=containerd-snapshotter-stargz
time="2022-05-22T20:49:16Z" level=info msg="trying next host - response was http.StatusNotFound" host="localhost:38917"
=== CONT TestClientGatewayIntegration
run.go:246: copied docker.io/amd64/busybox:latest to local mirror localhost:38917/library/busybox:latest
time="2022-05-22T20:49:17Z" level=info msg="trying next host - response was http.StatusNotFound" host="localhost:34549"
run.go:246: copied docker.io/amd64/busybox:latest to local mirror localhost:34549/library/busybox:latest
=== CONT TestClientGatewayIntegration/TestClientGatewaySlowCacheExecError/worker=containerd-snapshotter-stargz
--- PASS: TestClientGatewayIntegration (3.25s)
--- PASS: TestClientGatewayIntegration/TestClientGatewaySlowCacheExecError/worker=containerd-snapshotter-stargz (2.65s)
PASS
ok |
|
Let me know if there's more I can do to be of help. |
|
@ktock Any idea what might be going on here? I've restarted the CI and it fails again on that stargz test. Can't see how it could be related to this change. |
|
Nice |
|
Green now so merging. Let's keep an eye on that stargz test if it needs some updates. |
vendor: docker, docker/cli v28.0.0-rc.1
2 participants
The Docker CLI supports a field called "registrytoken" within the auth
config that may contain a previously resolved bearer token. If a
value is present, it is used verbatim and OAuth token retrieval using
username/password or identity token is skipped.
Support this same functionality in the buildkit client by checking for
this field's value prior to credential based auth in
FetchToken. If avalue is set for
AuthConfig.RegistryToken, short circuit and returnthe token value as is.
This feature helps to support registry setups that integrate with third
party auth systems such as GitLab's JWT OmniAuth provider.