Using google-auth-library-oauth2-http to get default credentials #151
Conversation
|
Sorry, this is on my radar but I haven't had a time to dive very deep on it right now. My two current concerns are:
@meltsufin - Any concerns about this from the |
|
|
||
| /** Factory for creating {@link Credential}s for interaction with Cloud SQL Admin API. */ | ||
| public interface CredentialFactory { | ||
| /** Name of system property that can specify an alternative credential factory. */ | ||
| String CREDENTIAL_FACTORY_PROPERTY = "cloudSql.socketFactory.credentialFactory"; | ||
|
|
||
| Credential create(); | ||
| HttpRequestInitializer create(); |
There was a problem hiding this comment.
This is a breaking changing to a public interface.
I would expect a major version bump for this to permissible.
There was a problem hiding this comment.
It might break binary compatibility, but it's source compatible
There was a problem hiding this comment.
I didn't realize Credential implements HttpRequestInitializer. This is actually OK then.
| } | ||
| return credential; | ||
| return new HttpCredentialsAdapter(credentials); |
There was a problem hiding this comment.
So why not make this method return com.google.auth.Credentials instead and do the wrapping in the caller?
You would change the method signature to Credentials create() as well.
There was a problem hiding this comment.
That would be a source code breaking change, com.google.api.client.auth.oauth2.Credential implements com.google.api.client.http.HttpRequestInitializer so this way the PR is source code compatible - existing implementations of com.google.cloud.sql.CredentialFactory will not need any source code changes, only be recompiled against the new library version. com.google.auth.Credentials on the other hand is a completely separate API and would require all existing implementations to change.
There was a problem hiding this comment.
You right. Thanks for the clarification.
There was a problem hiding this comment.
Two blocking issues right now:
- Determine if it's possible to make these changes without a major version bump
- Determine if using
google-auth-library-oauth2-httpis necessary, or if it is feasible to update or replace the auth currently used by the SQLAdmin Client
|
|
||
| /** Factory for creating {@link Credential}s for interaction with Cloud SQL Admin API. */ | ||
| public interface CredentialFactory { | ||
| /** Name of system property that can specify an alternative credential factory. */ | ||
| String CREDENTIAL_FACTORY_PROPERTY = "cloudSql.socketFactory.credentialFactory"; | ||
|
|
||
| Credential create(); | ||
| HttpRequestInitializer create(); |
There was a problem hiding this comment.
I didn't realize Credential implements HttpRequestInitializer. This is actually OK then.
| @@ -17,11 +17,12 @@ | |||
| package com.google.cloud.sql; | |||
|
|
|||
| import com.google.api.client.auth.oauth2.Credential; | |||
There was a problem hiding this comment.
It's actually used in the class comment but maybe the comment should be updated? I left it as is because I figured that in most cases it would be a Credential that is returned.
| } | ||
| return credential; | ||
| return new HttpCredentialsAdapter(credentials); |
There was a problem hiding this comment.
You right. Thanks for the clarification.
This should enable use of Workload Identities on GKE.
This should enable use of Workload Identities on GKE, which requires this fix included in google-auth-library-oauth2-http 0.16.2
The solution isn't terribly beautiful but it seemed like a simple way to do it while maintaining backwards compatibility.