use google-auth-library-oauth2-http for fetching access tokens #762
Conversation
mattnworb
force-pushed
the
mattbrown/gcp-accesstoken
branch
2 times, most recently
from
6983f14 to
21aee1d
Compare
|
@davidxia @mavenraven @gimaker @caipre one other thing I want to add here is another static method like |
|
also I think once something like |
Instead of invoking `gcloud docker` and parsing a docker config file, this adds a new ContainerRegistryAuthSupplier which programmatically fetches access tokens for a Google service or user account loaded for a credentials file. The benefit of this implementation is that any arbitrary account can be used without having to be registered/added to the `gcloud` CLI - the supplier just needs to be pointed at a file containing the credentials for the account to be used. There is also no need for gcloud to be available on the helios agent. This support should probably be pulled into a new library at some point, something like `docker-client-gcr`, so that non-GCR users of docker-client don't have to have this additional logic pulled in. For now, the Google library needed for GCR support is marked as `optional` in the pom.xml so users have to make sure to add the dependency themselves.
avoid unnecessary calls to refresh(), even if internally the method is synchronized and threadsafe.
the "auth" in this method name is the "auth" field from the docker client config file
mattnworb
force-pushed
the
mattbrown/gcp-accesstoken
branch
from
21aee1d to
9790957
Compare
Codecov Report
@@ Coverage Diff @@
## master #762 +/- ##
===========================================
- Coverage 65.89% 65.3% -0.59%
- Complexity 671 675 +4
===========================================
Files 158 156 -2
Lines 2926 2957 +31
Branches 335 340 +5
===========================================
+ Hits 1928 1931 +3
- Misses 850 877 +27
- Partials 148 149 +1 |
|
it feels so great to be able to get green status on commits without having to rerun individual travis jobs 20 times ✅ 😄 |
refactors ContainerRegistryAuthSupplier and its Builder to load the Google Application Default Credentials, in addition to the existing behavior of being able to load an account's credentials from a given inputStream. https://developers.google.com/identity/protocols/application-default-credentials
now that ContainerRegistryAuthSupplier is able to fetch access tokens for arbitrary accounts and the default application credentials, the original implementation that invokes `gcloud docker -a` and reads the configuration file seems unnecessary. In every case it would be better to use the new ContainerRegistryAuthSupplier, rather than requiring `gcloud` to be installed and having it populate a json file used by the docker CLI which we then read.
|
with the two new commits above I believe this is complete and ready for a full review |
|
🎉 |
| private final CredentialRefresher credentialRefresher; | ||
|
|
||
| @VisibleForTesting | ||
| ContainerRegistryAuthSupplier( |
There was a problem hiding this comment.
Do we need to expose this at all vs. using a Builder in the tests?
There was a problem hiding this comment.
yeah, so the tests can pass in a mock CredentialRefresher - someone using a builder will never need to do that
There was a problem hiding this comment.
I guess my question is, we also have this in the builder https://github.com/spotify/docker-client/pull/762/files/46e5fafb70d59ead941a02afb1a39284550116d1#diff-980dfc9639d40441a5814482f27ccba5R126 . Seems like we should either being using only the contructor or only the optional builder methods in the tests.
There was a problem hiding this comment.
ah yea, fair point. That withClock method doesn't need to be there
| */ | ||
| private AccessToken getAccessToken() throws DockerException { | ||
| // synchronize attempts to refresh the accessToken | ||
| synchronized (credentials) { |
There was a problem hiding this comment.
We're locking on an object that's exposed outside of ContainerRegistryAuthSupplier, which could lead to deadlock. Ideally, we don't need to need to mutate the credentials object at, and just get a new one, but that might not possible with the API.
There was a problem hiding this comment.
I don't think this will be an issue as I think when we call credentials.createScoped() we get a new instance, and only this class holds that instance.
I don't like synchronizing on what seems like a shared object either but wasn't sure what else to use here to avoid redundant calls to refresh if there are concurrent calls to authFor - would be open to any suggestions you might have.
| @@ -169,6 +169,19 @@ | |||
| <version>3.0.1</version> | |||
| <scope>provided</scope> | |||
| </dependency> | |||
| <!-- TODO we should pull out the Google Cloud support to a new library --> | |||
There was a problem hiding this comment.
@mattnworb Just curious. What did you have in mind for this?
There was a problem hiding this comment.
see these two comments:
spotify/dockerfile-maven#54 (comment)
spotify/dockerfile-maven#54 (comment)
Instead of invoking
gcloud dockerand parsing a docker config file, thisadds a new ContainerRegistryAuthSupplier which programmatically fetches
access tokens for a Google service or user account loaded for a credentials
file.
The benefit of this implementation is that any arbitrary account can be
used without having to be registered/added to the
gcloudCLI - thesupplier just needs to be pointed at a file containing the credentials for
the account to be used. There is also no need for gcloud to be available
on the helios agent.
This support should probably be pulled into a new library at some point,
something like
docker-client-gcr, so that non-GCR users ofdocker-client don't have to have this additional logic pulled in. For
now, the Google library needed for GCR support is marked as
optionalin the pom.xml so users have to make sure to add the dependency
themselves.