fix: plumb mtls endpoint to TransportChannelProvider #3673
Conversation
gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
Show resolved
Hide resolved
|
@lqiu96 , please review, thanks! |
rmehta19
changed the title
|
Friendly ping @lqiu96 . Thanks! |
| @Override | ||
| public TransportChannelProvider withMtlsEndpoint(String mtlsEndpoint) { | ||
| validateEndpoint(mtlsEndpoint); | ||
| return toBuilder().setMtlsEndpoint(mtlsEndpoint).build(); | ||
| } |
There was a problem hiding this comment.
Would you be able to add @InternalApi to all the new public methods? We have a few ways to configure endpoints (i.e. ClientSettings and TransportChannelProvider for endpoint and led us to have to account for it in EndpointContext). If we add these ways for users to configure the mtls value, I think we'll need to account for it in EndpointContext as well.
Given that we can't make this package-private scope, we should let users know that they really shouldn't be tinkering with these values since they're really used to help S2A (one flow for now).
I think a small note in the comment (for this method and all the other public ones) as well warning users This public method is used by Gax to help configure S2A fallback and not intended for users or something of that variant.
There was a problem hiding this comment.
Done in 7e36d1a. Would you prefer making the change in EndpointContext in this PR, or a follow up?
There was a problem hiding this comment.
My thoughts are that we just leave EndpointContext as-is since we have publicly noted that these public methods have been marked with @InternalApi and shouldn't be set/customized by the user at all. If users do set these values via these new methods, they shouldn't expect support (as this is used to help to support S2A).
The existing way of setting the mtlsEndpoint in StubSettings is supported
gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
Show resolved
Hide resolved
|
Thanks for the review @lqiu96 ! |
|
@rmehta19 FYI I think there are lint issues and you need to run |
|
Thanks @lqiu96 ! synced to HEAD and formatted. |
|
/gcbrun |
|
@lqiu96 , resolved merge conflict, could you run gcbrun again? Thanks! |
|
/gcbrun |
|
Thanks @lqiu96 ! I see all the checks have passed, would you be able to merge once you get a chance? |
|
Adding @blakeli0 to take a quick glance as well in case there is anything missed. |
|
friendly ping, @blakeli0. Thanks! |
| @@ -97,6 +97,20 @@ public interface TransportChannelProvider { | |||
| */ | |||
| TransportChannelProvider withEndpoint(String endpoint); | |||
|
|
|||
| /** True for gRPC transport provider which has no mtlsEndpoint set. */ | |||
| default boolean needsMtlsEndpoint() { | |||
There was a problem hiding this comment.
I know we tried to avoid introducing these new public interfaces by encapsulating the logic to determine if we should use S2A in EndpointContext. Now that we need to have access to mtlsEndpoint in TransportChannelProvider anyway, maybe we can move shouldUseS2A to InstantiatingGrpcChannelProvider.java? And remove withUseS2A from this interface? This could be separate PR.
There was a problem hiding this comment.
Agreed that by plumbing the MTLS endpoint separately, we no longer need the shouldUseS2A logic in EndpointContext. I'll take care of this in a followup PR.
There was a problem hiding this comment.
@blakeli0 I started working on this and I realized that we should keep shouldUseS2A in EndpointContext. The reason being is that we want to skip S2A if using GDCH, or if the client sets a custom endpoint. This info is only available in EndpointContext.
Given this, we probably want to keep things as is. WDYT?
There was a problem hiding this comment.
if the client sets a custom endpoint
I see, yeah I think this info only exists in EndpointContext. cc: @lqiu96 in case there are more ideas.
|
Thanks for reviewing @blakeli0 ! |
This PR plumbs the MTLS endpoint (separately from the resolved endpoint) from the
EndpointContextto theInstantiatingGrpcChannelProvider.Why not just set the MTLS endpoint in
EndpointContextif S2A can be used?EndpointContext, we could fail to use S2A inInstantiatingGrpcChannelProvider(if autoconfig doesn't return an address), in which case we fall back to using a TLS connectionInstantiatingGrpcChannelProvider, and the decision to use DirectPath is made inInstantiatingGrpcChannelProvider, notEndpointContext. We may decide to use S2A inEndpointContext, but when we go to create the channel inInstantiatingGrpcChannelProvider, we may find that we should be using DirectPath, in which case we need to use the non-MTLS endpoint