Skip to content

fix: S2A- Check if a default endpoint has been set #3784

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.

Sign up for GitHub

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
Jun 2, 2025
Merged

fix: S2A- Check if a default endpoint has been set #3784

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
09b024e
don't check transport channel provider endpoint.
rmehta19 May 6, 2025
c4ed18e
merge upstream.
rmehta19 May 7, 2025
4bf3ade
Merge branch 'main' into dont-check-transport-channel-endpoint
rmehta19 May 28, 2025
da85450
check transportchannelprovider endpoint not the default endpoint.
rmehta19 May 29, 2025
be79698
Merge branch 'main' into dont-check-transport-channel-endpoint
rmehta19 May 29, 2025
6ad5b8e
update tests.
rmehta19 May 29, 2025
5617650
fix tests.
rmehta19 May 29, 2025
6fb54f9
fix if condition.
rmehta19 May 29, 2025
4bba85b
fix if.
rmehta19 May 29, 2025
5fafdaf
check if clientSettingsEndpoint is default endpoint
rmehta19 May 30, 2025
604fe07
Merge branch 'main' into dont-check-transport-channel-endpoint
rmehta19 May 30, 2025
9b52a4e
Merge branch 'main' into dont-check-transport-channel-endpoint
rmehta19 Jun 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions gax-java/gax/src/main/java/com/google/api/gax/rpc/EndpointContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,8 +327,7 @@ boolean shouldUseS2A() {
}

// If a custom endpoint is being used, skip S2A.
if (!Strings.isNullOrEmpty(clientSettingsEndpoint())
|| !Strings.isNullOrEmpty(transportChannelProviderEndpoint())) {
if (!Strings.isNullOrEmpty(clientSettingsEndpoint())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm worried that this may only end up potentially resolving the issue for the GrpcTransportOptions.setUpChannelProvider case. IIRC, resolveEndpoint()'s logic still revolves around if either clientSettings' endpoint or transportChannelProvider's endpoint is set, and if so, then that's the resolved endpoint.

I'm worried that it would end up with s2a being enabled with the resolved endpoint still being the one configured in GrpcTransportOptions.setUpChannelProvider

Copy link
Contributor

Choose a reason for hiding this comment

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

Gax and the generator no longer provide a default non-mtls endpoint for the client libraries. The "default" endpoint that is intended to be used is constructed inside the EndpointContext. If you want to check, you can theoretically just check if clientSettingsEndpoint or transportChannelEndpoint == buildEndpointTemplate(serviceName(), resolvedUniverseDomain())

Copy link
Contributor Author

@rmehta19 rmehta19 May 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm worried that it would end up with s2a being enabled with the resolved endpoint still being the one configured in GrpcTransportOptions.setUpChannelProvider

I believe our logic in InstantiatingGrpcChannelProvider prevents this from happening. If S2A is enabled, we use the mtlsEndpoint (that is plumbed separately from the endpoint; endpoint == resolvedEndpoint()):

sdk-platform-java/gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java

Lines 686 to 699 in 49a7ae5

if (useS2A) {
channelCredentials = createS2ASecuredChannelCredentials();
}
if (channelCredentials != null) {
// Create the channel using S2A-secured channel credentials.
if (mtlsS2ACallCredentials != null) {
// Set {@code mtlsS2ACallCredentials} to be per-RPC call credentials,
// which will be used to fetch MTLS_S2A hard bound tokens from the metdata server.
channelCredentials =
CompositeChannelCredentials.create(channelCredentials, mtlsS2ACallCredentials);
}
// Connect to the MTLS endpoint when using S2A because S2A is used to perform an MTLS
// handshake.
builder = Grpc.newChannelBuilder(mtlsEndpoint, channelCredentials);

WDYT @lqiu96 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right, there is the custom logic for S2A. What about the case if a user has a custom TransportChannelProvider with a non-mtls endpoint configured? IIUC above, the intention would be not use S2A, but our checks in EndpointContext would have shouldUseS2A as true.

Copy link
Contributor Author

@rmehta19 rmehta19 May 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Oh right, there is the custom logic for S2A. What about the case if a user has a custom TransportChannelProvider with a non-mtls endpoint configured? IIUC above, the intention would be not use S2A, but our checks in EndpointContext would have shouldUseS2A as true.

It is correct that if the user is intentionally setting a custom endpoint via their own TransportChannelProvider, that we want to skip S2A. With the latest changes, if they are setting the custom endpoint to be the same as the default non-mtls endpoint, we would end up using S2A. I believe this is ok, as my understanding is that if the user is setting an endpoint, it is usually something different than the default non-mtls endpoint.

Gax and the generator no longer provide a default non-mtls endpoint for the client libraries. The "default" endpoint that is intended to be used is constructed inside the EndpointContext. If you want to check, you can theoretically just check if clientSettingsEndpoint or transportChannelEndpoint == buildEndpointTemplate(serviceName(), resolvedUniverseDomain())

Apologies, I missed this comment of yours. Thanks for bringing this up! I tested this out internally and this works. The latest commit contains this logic, WDYT @lqiu96

return false;
}

Expand Down
21 changes: 0 additions & 21 deletions gax-java/gax/src/test/java/com/google/api/gax/rpc/EndpointContextTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,6 @@ void endpointContextBuild_shouldUseS2A_tlsEndpoint() throws IOException {
defaultEndpointContextBuilder
.setEnvProvider(envProvider)
.setClientSettingsEndpoint("")
.setTransportChannelProviderEndpoint("")
.setUsingGDCH(false);
EndpointContext endpointContext = defaultEndpointContextBuilder.build();
Truth.assertThat(defaultEndpointContextBuilder.shouldUseS2A()).isTrue();
Expand Down Expand Up @@ -479,7 +478,6 @@ void shouldUseS2A_envVarNotSet_returnsFalse() throws IOException {
defaultEndpointContextBuilder
.setEnvProvider(envProvider)
.setClientSettingsEndpoint("")
.setTransportChannelProviderEndpoint("")
.setUsingGDCH(false);
Truth.assertThat(defaultEndpointContextBuilder.shouldUseS2A()).isFalse();
}
Expand All @@ -492,7 +490,6 @@ void shouldUseS2A_UsingGDCH_returnsFalse() throws IOException {
defaultEndpointContextBuilder
.setEnvProvider(envProvider)
.setClientSettingsEndpoint("")
.setTransportChannelProviderEndpoint("")
.setUsingGDCH(true);
Truth.assertThat(defaultEndpointContextBuilder.shouldUseS2A()).isFalse();
}
Expand All @@ -505,20 +502,6 @@ void shouldUseS2A_customEndpointSetViaClientSettings_returnsFalse() throws IOExc
defaultEndpointContextBuilder
.setEnvProvider(envProvider)
.setClientSettingsEndpoint("test.endpoint.com:443")
.setTransportChannelProviderEndpoint("")
.setUsingGDCH(false);
Truth.assertThat(defaultEndpointContextBuilder.shouldUseS2A()).isFalse();
}

@Test
void shouldUseS2A_customEndpointSetViaTransportChannelProvider_returnsFalse() throws IOException {
EnvironmentProvider envProvider = Mockito.mock(EnvironmentProvider.class);
Mockito.when(envProvider.getenv(EndpointContext.S2A_ENV_ENABLE_USE_S2A)).thenReturn("true");
defaultEndpointContextBuilder =
defaultEndpointContextBuilder
.setEnvProvider(envProvider)
.setClientSettingsEndpoint("")
.setTransportChannelProviderEndpoint("test.endpoint.com:443")
.setUsingGDCH(false);
Truth.assertThat(defaultEndpointContextBuilder.shouldUseS2A()).isFalse();
}
Expand All @@ -531,7 +514,6 @@ void shouldUseS2A_mtlsEndpointNull_returnsFalse() throws IOException {
defaultEndpointContextBuilder
.setEnvProvider(envProvider)
.setClientSettingsEndpoint("")
.setTransportChannelProviderEndpoint("")
.setUsingGDCH(false)
.setMtlsEndpoint(null);
Truth.assertThat(defaultEndpointContextBuilder.shouldUseS2A()).isFalse();
Expand All @@ -545,7 +527,6 @@ void shouldUseS2A_mtlsEndpointEmpty_returnsFalse() throws IOException {
defaultEndpointContextBuilder
.setEnvProvider(envProvider)
.setClientSettingsEndpoint("")
.setTransportChannelProviderEndpoint("")
.setMtlsEndpoint("")
.setUsingGDCH(false);
Truth.assertThat(defaultEndpointContextBuilder.shouldUseS2A()).isFalse();
Expand All @@ -559,7 +540,6 @@ void shouldUseS2A_mtlsEndpointNotGoogleDefaultUniverse_returnsFalse() throws IOE
defaultEndpointContextBuilder
.setEnvProvider(envProvider)
.setClientSettingsEndpoint("")
.setTransportChannelProviderEndpoint("")
.setMtlsEndpoint("test.mtls.abcd.com:443")
.setUsingGDCH(false);
Truth.assertThat(defaultEndpointContextBuilder.shouldUseS2A()).isFalse();
Expand All @@ -573,7 +553,6 @@ void shouldUseS2A_success() throws IOException {
defaultEndpointContextBuilder
.setEnvProvider(envProvider)
.setClientSettingsEndpoint("")
.setTransportChannelProviderEndpoint("")
.setUsingGDCH(false);
Truth.assertThat(defaultEndpointContextBuilder.shouldUseS2A()).isTrue();
}
Expand Down
Loading