-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Implement GcpServiceAccountIdentityCredentials #11607
base: master
Are you sure you want to change the base?
Conversation
private CallCredentials getCallCredentials() throws Exception { | ||
ComputeEngineCredentials credentials = ComputeEngineCredentials.create(); | ||
IdTokenCredentials idTokenCredentials = IdTokenCredentials.newBuilder() | ||
.setIdTokenProvider(credentials) |
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.
nit: Just inline ComputeEngineCredentials.create() instead of using a local variable.
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.
Okay, I'll do that in the follow up commit.
ComputeEngineCredentials credentials = ComputeEngineCredentials.create(); | ||
IdTokenCredentials idTokenCredentials = IdTokenCredentials.newBuilder() | ||
.setIdTokenProvider(credentials) | ||
.setTargetAudience(audience) |
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.
Write unit test to extract and assert the audience using tokenCredential.getIdToken().getJsonWebSignature().getPayload().getAudienceAsList() as given in the usage examples in https://cloud.google.com/java/docs/reference/google-auth-library/latest/com.google.auth.oauth2.IdTokenCredentials.
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.
Sure, just wanted to confirm this implementation before spending time on unit tests.
* <p>This class is intended for use on GCE instances. It will not work | ||
* in other environments. | ||
*/ | ||
public class GcpServiceAccountIdentityCredentials extends CallCredentials { |
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.
Don't make any new public APIs. You want to add code to xds and just use it in your filter.
GcpServiceAccountIdentityCallCredentials from the gRFC already exists in Java. That's what your getCallCredentials()
function creates. You absolutely wouldn't want to create that every RPC.
We will use custom CallCredentials
in Java in order to read the authority
and then select the correct CallCredentials
, but that's going to look different from this.
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.
GcpServiceAccountIdentityCallCredentials from the gRFC already exists in Java.
I cannot find it. Where is it?
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.
GcpServiceAccountIdentityCallCredentials from the gRFC already exists in Java.
I cannot find it. Where is it?
The next sentence answered that:
That's what your getCallCredentials() function creates.
C doesn't have an auth library to use, unlike Java and Go. So in C they will need to make a new class. In Java, the functionality is already available.
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.
I took another look at the gRFC and it looks like we don't actually need custom CallCredentials
. (I was probably thinking of an alternative design that had been discussed; it wouldn't depend on A74.) The filter will have the data available already; once A74 is done, the cluster config will be in CallOptions, like XdsNameResolver.CLUSTER_SELECTION_KEY
is today.
@Override | ||
public void applyRequestMetadata(RequestInfo requestInfo, | ||
Executor appExecutor, MetadataApplier applier) { | ||
appExecutor.execute(() -> { |
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.
The executor here is for blocking operations (like I/O). We shouldn't need to use it directly. Call getCallCredentials()
directly in applyRequestMetadata()
Note: The Google Auth Library have built-in mechanisms to handle clock skew in ComputeEngineCredentials.java.
GoogleCredentials
manages token refreshing, validity and parsing of the JWT token internally.