Skip to content

fix: plumb mtls endpoint to TransportChannelProvider #3673

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

Merged
merged 12 commits into from
Mar 19, 2025
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ public final class InstantiatingGrpcChannelProvider implements TransportChannelP
private final HeaderProvider headerProvider;
private final boolean useS2A;
private final String endpoint;
private final String mtlsEndpoint;
// TODO: remove. envProvider currently provides DirectPath environment variable, and is only used
// during initial rollout for DirectPath. This provider will be removed once the DirectPath
// environment is not used.
Expand Down Expand Up @@ -179,6 +180,7 @@ private InstantiatingGrpcChannelProvider(Builder builder) {
this.headerProvider = builder.headerProvider;
this.useS2A = builder.useS2A;
this.endpoint = builder.endpoint;
this.mtlsEndpoint = builder.mtlsEndpoint;
this.allowedHardBoundTokenTypes = builder.allowedHardBoundTokenTypes;
this.mtlsProvider = builder.mtlsProvider;
this.s2aConfigProvider = builder.s2aConfigProvider;
Expand Down Expand Up @@ -258,6 +260,12 @@ public boolean needsEndpoint() {
return endpoint == null;
}

@InternalApi("This public method is used by Gax to help configure the MTLS endpoint for S2A")
@Override
public boolean needsMtlsEndpoint() {
return mtlsEndpoint == null;
}

/**
* Specify the endpoint the channel should connect to.
*
Expand All @@ -272,6 +280,22 @@ public TransportChannelProvider withEndpoint(String endpoint) {
return toBuilder().setEndpoint(endpoint).build();
}

/**
* Specify the mTLS endpoint the channel should connect to when using S2A.
*
* <p>The value of {@code mtlsEndpoint} must be of the form {@code host:port}.
*
* @param mtlsEndpoint The mtTLS endpoint to connect to
* @return A new {@link InstantiatingGrpcChannelProvider} with the specified mTLS endpoint
* configured
*/
@InternalApi("This public method is used by Gax to help configure the MTLS endpoint for S2A")
@Override
public TransportChannelProvider withMtlsEndpoint(String mtlsEndpoint) {
validateEndpoint(mtlsEndpoint);
return toBuilder().setMtlsEndpoint(mtlsEndpoint).build();
}
Comment on lines +293 to +297
Copy link
Contributor

@lqiu96 lqiu96 Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 7e36d1a. Would you prefer making the change in EndpointContext in this PR, or a follow up?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
* Specify whether or not to use S2A.
*
Expand Down Expand Up @@ -666,7 +690,9 @@ private ManagedChannel createSingleChannel() throws IOException {
channelCredentials =
CompositeChannelCredentials.create(channelCredentials, mtlsS2ACallCredentials);
}
builder = Grpc.newChannelBuilder(endpoint, channelCredentials);
// Connect to the MTLS endpoint when using S2A because S2A is used to perform an MTLS
// handshake.
builder = Grpc.newChannelBuilder(mtlsEndpoint, channelCredentials);
} else {
// Use default if we cannot initialize channel credentials via DCA or S2A.
builder = ManagedChannelBuilder.forAddress(serviceAddress, port);
Expand Down Expand Up @@ -819,6 +845,7 @@ public static final class Builder {
private Executor executor;
private HeaderProvider headerProvider;
private String endpoint;
private String mtlsEndpoint;
private boolean useS2A;
private EnvironmentProvider envProvider;
private SecureSessionAgent s2aConfigProvider = SecureSessionAgent.create();
Expand Down Expand Up @@ -926,6 +953,14 @@ public Builder setEndpoint(String endpoint) {
return this;
}

/** Sets the mTLS Endpoint used to reach the service, eg "localhost:8080". */
@InternalApi("This public method is used by Gax to help configure the MTLS endpoint for S2A")
public Builder setMtlsEndpoint(String mtlsEndpoint) {
validateEndpoint(mtlsEndpoint);
this.mtlsEndpoint = mtlsEndpoint;
return this;
}

Builder setUseS2A(boolean useS2A) {
this.useS2A = useS2A;
return this;
Expand Down
10 changes: 10 additions & 0 deletions gax-java/gax/clirr-ignored-differences.xml
Original file line number Diff line number Diff line change
Expand Up @@ -118,4 +118,14 @@
<className>com/google/api/gax/rpc/TransportChannelProvider</className>
<method>* withUseS2A(*)</method>
</difference>
<difference>
<differenceType>7012</differenceType>
<className>com/google/api/gax/rpc/TransportChannelProvider</className>
<method>* withMtlsEndpoint(*)</method>
</difference>
<difference>
<differenceType>7012</differenceType>
<className>com/google/api/gax/rpc/TransportChannelProvider</className>
<method>* needsMtlsEndpoint()</method>
</difference>
</differences>
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,10 @@ public static ClientContext create(StubSettings settings) throws IOException {
transportChannelProvider = transportChannelProvider.withEndpoint(endpoint);
}
transportChannelProvider = transportChannelProvider.withUseS2A(endpointContext.useS2A());
if (transportChannelProvider.needsMtlsEndpoint()) {
transportChannelProvider =
transportChannelProvider.withMtlsEndpoint(endpointContext.mtlsEndpoint());
}
TransportChannel transportChannel = transportChannelProvider.getTransportChannel();

ApiCallContext defaultCallContext =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -303,13 +303,6 @@ private String determineEndpoint() throws IOException {
"mTLS is not supported in any universe other than googleapis.com");
}

// Check if Experimental S2A feature enabled. When feature is non-experimental, remove this
// check from this function, and plumb MTLS endpoint to channel creation logic separately.
// Note that mTLS via S2A is an independent feature from mTLS via DCA (for which endpoint
// determined by {@code mtlsEndpointResolver} above).
if (shouldUseS2A()) {
return mtlsEndpoint();
}
return endpoint;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,20 @@ public interface TransportChannelProvider {
*/
TransportChannelProvider withEndpoint(String endpoint);

/** True for gRPC transport provider which has no mtlsEndpoint set. */
default boolean needsMtlsEndpoint() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return false;
}

/**
* Sets the mTLS endpoint to use when constructing a new {@link TransportChannel} using S2A.
*
* <p>This method should only be called if {@link #needsMtlsEndpoint()} returns true.
*/
default TransportChannelProvider withMtlsEndpoint(String mtlsEndpoint) {
return this;
}

/** Sets whether to use S2A when constructing a new {@link TransportChannel}. */
@BetaApi(
"The S2A feature is not stable yet and may change in the future. https://github.com/grpc/grpc-java/issues/11533.")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ void endpointContextBuild_multipleUniverseDomainConfigurations_clientSettingsHas
}

@Test
void endpointContextBuild_shouldUseS2A_mtlsEndpoint() throws IOException {
void endpointContextBuild_shouldUseS2A_tlsEndpoint() throws IOException {
EnvironmentProvider envProvider = Mockito.mock(EnvironmentProvider.class);
Mockito.when(envProvider.getenv(EndpointContext.S2A_ENV_ENABLE_USE_S2A)).thenReturn("true");
defaultEndpointContextBuilder =
Expand All @@ -385,7 +385,7 @@ void endpointContextBuild_shouldUseS2A_mtlsEndpoint() throws IOException {
.setUsingGDCH(false);
EndpointContext endpointContext = defaultEndpointContextBuilder.build();
Truth.assertThat(defaultEndpointContextBuilder.shouldUseS2A()).isTrue();
Truth.assertThat(endpointContext.resolvedEndpoint()).isEqualTo(DEFAULT_MTLS_ENDPOINT);
Truth.assertThat(endpointContext.resolvedEndpoint()).isEqualTo(DEFAULT_ENDPOINT);
}

@Test
Expand Down
Loading