-
Notifications
You must be signed in to change notification settings - Fork 3.9k
[xDS] A97 - JWT token file call creds #12242
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
base: master
Are you sure you want to change the base?
Conversation
a8f76b0
to
c781460
Compare
@ejona86 Could you please review this PR when you get a chance? Thanks! |
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 shape of what I saw looked good. I didn't pay attention to any of the tests yet. I need to look through the provider plumbing more. But wanted to send the comments I have.
alts/src/main/java/io/grpc/alts/JwtTokenFileCallCredentials.java
Outdated
Show resolved
Hide resolved
alts/src/testFixtures/java/io/grpc/alts/JwtTokenFileTestUtils.java
Outdated
Show resolved
Hide resolved
xds/src/main/java/io/grpc/xds/internal/JwtTokenFileXdsCredentialsProvider.java
Outdated
Show resolved
Hide resolved
alts/src/main/java/io/grpc/alts/JwtTokenFileCallCredentials.java
Outdated
Show resolved
Hide resolved
alts/src/main/java/io/grpc/alts/JwtTokenFileCallCredentials.java
Outdated
Show resolved
Hide resolved
alts/src/main/java/io/grpc/alts/JwtTokenFileCallCredentials.java
Outdated
Show resolved
Hide resolved
c781460
to
85495b0
Compare
85495b0
to
9f97bb5
Compare
@ejona86 PR ready for second round |
@ejona86 kindly reminder about this forgotten piece :) |
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.
Just sending what I have.
We're going to want to make a clone of XdsCredentialsRegistry for CallCreds, because they are two different config namespaces, the gRFC says we'd create a new registry, and there's really no reason to provide both call creds and channel creds. However, I see that XdsCredentialsRegistry.getHardCodedClasses() should be deleted; we should just hard-code an empty list to InternalServiceProviders.loadAll()
; none of this will be used on Android.
public static File createValidJwtToken(long expTime) | ||
throws Exception { | ||
File jwtToken = File.createTempFile(new String("jwt.token"), ""); | ||
jwtToken.deleteOnExit(); |
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.
When I mentioned removing the TemporaryFolder usage I was thinking you'd pass in a File (so the caller would be responsible for temporaryFolder.newFile()
). deleteOnExit()
isn't the worst, but generally a code smell.
* limitations under the License. | ||
*/ | ||
|
||
package io.grpc.xds; |
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'd expect this to be in the same package as JwtTokenFileXdsCredentialsProvider
. I'm less concerned with which package, and just that they are co-located.
Things like this directly in io.grpc.xds
are ideally package-private. Sometimes we have to make it public, but then we'd take some steps to hide it from the javadoc and mark it as @Internal
. There's a few different options. I'm not seeing a need for anything public here (once the package is sorted out).
(I'm not claiming the xds package structure makes sense. io.grpc.xds.client
and orca
are pretty well separated, but the others are a bit haphazard.)
* the call credentials | ||
* | ||
*/ | ||
protected abstract CallCredentials newCallCredentials(Map<String, ?> jsonConfig); |
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.
Very few implementations implement this. Give this a default implementation that returns null
?
} | ||
|
||
@Override | ||
protected Object getImplSpecificConfig(Map<String, ?> serverConfig, String serverUri) |
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.
You don't need a new method.
protected Object getImplSpecificConfig(Map<String, ?> serverConfig, String serverUri)
throws XdsInitializationException {
ChannelCredentials channelCreds = getChannelCredentials(serverConfig, serverUri);
CallCredentials callCreds = getCallCredentials(serverConfig, serverUri);
if (callCreds != null) {
channelCreds = CompositeChannelCredentials.create(channelCreds, callCreds);
}
return channelCreds;
}
.build(); | ||
this.callCredentials = callCredentials; | ||
|
||
if (callCredentials != null && implSpecificConfig instanceof CompositeChannelCredentials) { |
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.
You don't need to pull the CallCreds back out. The gRPC Channel is already using them, as they were passed to newChannelBuilder()
. Any CallCredentials added later, like in XdsStreamingCall's constructor, will be combined together with the call creds passed to newChannelBuilder()
.
The callCredentials passed into this method are a bit weird. They could be passed to newChannelBuilder()
, but let's just leave them alone.
implementing gRFC A97 grpc/proposal#492