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 all 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
8 changes: 6 additions & 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,12 @@ boolean shouldUseS2A() {
}

// If a custom endpoint is being used, skip S2A.
if (!Strings.isNullOrEmpty(clientSettingsEndpoint())
|| !Strings.isNullOrEmpty(transportChannelProviderEndpoint())) {
if ((!Strings.isNullOrEmpty(clientSettingsEndpoint())
&& !buildEndpointTemplate(serviceName(), resolvedUniverseDomain())
.contains(clientSettingsEndpoint()))
|| (!Strings.isNullOrEmpty(transportChannelProviderEndpoint())
&& !buildEndpointTemplate(serviceName(), resolvedUniverseDomain())
.contains(transportChannelProviderEndpoint()))) {
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.

Adding this "contains" logic, since buildEndpointTemplate adds 443 (https) as default port, and GrpcTransportOptions.setUpChannelProvider, for example, just sets the host.

In this way, we are sure to respect a custom endpoint set via TransportChannelProvider. For example, if client specified transportChannelProviderEndpoint as the http endpoint (default endpoint + port 80), shouldUseS2A would evaluate to false.

@lqiu96

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! I think this does resolve the above concern. Could this check for both clientSettings and transportChannelProvider just to be safe? Or is there a specific reason for only doing this for the transportchannelprovider?

I think the contains logic is fine given the concerns with port. The string contains == resolved endpoint may not be completely fool proof (I can't think of counter example off the top of my head), but I think it's safer than removing the check for the transportchannelprovider endpoint.

Hopefully this helps unblock the issue with GrpcTransportOptions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could this check for both clientSettings and transportChannelProvider just to be safe? Or is there a specific reason for only doing this for the transportchannelprovider?

I don't see an issue for doing this check for both clientSettingsEndpoint and transportChannelProviderEndpoint, the reason I initially did it for the latter was because I ran into it in the GrpcTransportOptions case :). I tested this internally and verified that doing this check on both is ok. Added this in the latest commit.

Copy link
Member

Choose a reason for hiding this comment

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

FYI, Bigtable has custom code to setEndpoint via settings which I think would be an issue: https://github.com/googleapis/java-bigtable/blob/8d3dca43224179829829bcf91972610c666b130b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubSettings.java#L807

May be an issue in the future as it auto sets a default. But you would prefer to handle those cases later on, then we can just proceed with this to unblock logging.

return false;
}

Expand Down
9 changes: 9 additions & 0 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 @@ -380,6 +380,7 @@ void endpointContextBuild_shouldUseS2A_tlsEndpoint() throws IOException {
defaultEndpointContextBuilder =
defaultEndpointContextBuilder
.setEnvProvider(envProvider)
.setResolvedUniverseDomain(Credentials.GOOGLE_DEFAULT_UNIVERSE)
.setClientSettingsEndpoint("")
.setTransportChannelProviderEndpoint("")
.setUsingGDCH(false);
Expand Down Expand Up @@ -478,6 +479,7 @@ void shouldUseS2A_envVarNotSet_returnsFalse() throws IOException {
defaultEndpointContextBuilder =
defaultEndpointContextBuilder
.setEnvProvider(envProvider)
.setResolvedUniverseDomain(Credentials.GOOGLE_DEFAULT_UNIVERSE)
.setClientSettingsEndpoint("")
.setTransportChannelProviderEndpoint("")
.setUsingGDCH(false);
Expand All @@ -491,6 +493,7 @@ void shouldUseS2A_UsingGDCH_returnsFalse() throws IOException {
defaultEndpointContextBuilder =
defaultEndpointContextBuilder
.setEnvProvider(envProvider)
.setResolvedUniverseDomain(Credentials.GOOGLE_DEFAULT_UNIVERSE)
.setClientSettingsEndpoint("")
.setTransportChannelProviderEndpoint("")
.setUsingGDCH(true);
Expand All @@ -504,6 +507,7 @@ void shouldUseS2A_customEndpointSetViaClientSettings_returnsFalse() throws IOExc
defaultEndpointContextBuilder =
defaultEndpointContextBuilder
.setEnvProvider(envProvider)
.setResolvedUniverseDomain(Credentials.GOOGLE_DEFAULT_UNIVERSE)
.setClientSettingsEndpoint("test.endpoint.com:443")
.setTransportChannelProviderEndpoint("")
.setUsingGDCH(false);
Expand All @@ -517,6 +521,7 @@ void shouldUseS2A_customEndpointSetViaTransportChannelProvider_returnsFalse() th
defaultEndpointContextBuilder =
defaultEndpointContextBuilder
.setEnvProvider(envProvider)
.setResolvedUniverseDomain(Credentials.GOOGLE_DEFAULT_UNIVERSE)
.setClientSettingsEndpoint("")
.setTransportChannelProviderEndpoint("test.endpoint.com:443")
.setUsingGDCH(false);
Expand All @@ -530,6 +535,7 @@ void shouldUseS2A_mtlsEndpointNull_returnsFalse() throws IOException {
defaultEndpointContextBuilder =
defaultEndpointContextBuilder
.setEnvProvider(envProvider)
.setResolvedUniverseDomain(Credentials.GOOGLE_DEFAULT_UNIVERSE)
.setClientSettingsEndpoint("")
.setTransportChannelProviderEndpoint("")
.setUsingGDCH(false)
Expand All @@ -544,6 +550,7 @@ void shouldUseS2A_mtlsEndpointEmpty_returnsFalse() throws IOException {
defaultEndpointContextBuilder =
defaultEndpointContextBuilder
.setEnvProvider(envProvider)
.setResolvedUniverseDomain(Credentials.GOOGLE_DEFAULT_UNIVERSE)
.setClientSettingsEndpoint("")
.setTransportChannelProviderEndpoint("")
.setMtlsEndpoint("")
Expand All @@ -558,6 +565,7 @@ void shouldUseS2A_mtlsEndpointNotGoogleDefaultUniverse_returnsFalse() throws IOE
defaultEndpointContextBuilder =
defaultEndpointContextBuilder
.setEnvProvider(envProvider)
.setResolvedUniverseDomain(Credentials.GOOGLE_DEFAULT_UNIVERSE)
.setClientSettingsEndpoint("")
.setTransportChannelProviderEndpoint("")
.setMtlsEndpoint("test.mtls.abcd.com:443")
Expand All @@ -572,6 +580,7 @@ void shouldUseS2A_success() throws IOException {
defaultEndpointContextBuilder =
defaultEndpointContextBuilder
.setEnvProvider(envProvider)
.setResolvedUniverseDomain(Credentials.GOOGLE_DEFAULT_UNIVERSE)
.setClientSettingsEndpoint("")
.setTransportChannelProviderEndpoint("")
.setUsingGDCH(false);
Expand Down
Loading