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

rmehta19 · 2025-05-06T22:28:48Z

Certain handwritten client libraries set this field to the (TLS) endpoint via GrpcTransportOptions.setUpChannelProvider.

This isn't really a custom endpoint, so we should be able to use S2A when this is set.

Example with Cloud Logging: https://github.com/googleapis/java-logging/blob/644ebd756d44ffc02b58aa30d9a07c8afe700981/google-cloud-logging/src/main/java/com/google/cloud/logging/spi/v2/GrpcLoggingRpc.java#L137

rmehta19 · 2025-05-07T23:14:51Z

@lqiu96 , would you be able to review this? Thanks!

lqiu96 · 2025-05-08T18:16:06Z

Certain handwritten client libraries set this field to the (TLS) endpoint via GrpcTransportOptions.setUpChannelProvider.

I see, I wasn't aware of this use case. However, if a user does create their own TransportChannelProvider with a custom endpoint, then it wouldn't skip S2A.

Can you remind me again what the original rationale was for skipping a custom endpoint?

rmehta19 · 2025-05-12T17:16:35Z

Can you remind me again what the original rationale was for skipping a custom endpoint?

When a user is setting a custom endpoint, we can't be sure this is a MTLS endpoint, therefore we can't use S2A. We only want to use S2A when we can use the MTLS endpoint.

However, if a user does create their own TransportChannelProvider with a custom endpoint, then it wouldn't skip S2A.

This is true, if a hw client library does this (say they set it to something foo.xyz.com) then S2A wouldn't get skipped, and S2A would be used to connect to MTLS endpoint of api (foo.mtls.googleapis.com) which isn't our desired behavior (we want to respect custom set endpoint and skip S2A in this case). Perhaps we could add some extra field (GAXProvidedEndpoint) to the TransportChannelProvider interface and GrpcTransportOptions.setUpChannelProvider can set that field instead? Then we could keep the S2A logic in EndpointContext as is. WDYT @lqiu96?

rmehta19 · 2025-05-19T16:34:36Z

Friendly ping on this @lqiu96. Thank you!

lqiu96 · 2025-05-28T20:44:23Z

gax-java/gax/src/main/java/com/google/api/gax/rpc/EndpointContext.java

@@ -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())) {


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

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())

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 ?

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.

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

rmehta19 · 2025-05-29T21:02:31Z

gax-java/gax/src/main/java/com/google/api/gax/rpc/EndpointContext.java

-          || !Strings.isNullOrEmpty(transportChannelProviderEndpoint())) {
+          || (!Strings.isNullOrEmpty(transportChannelProviderEndpoint())
+              && !buildEndpointTemplate(serviceName(), resolvedUniverseDomain())
+                  .contains(transportChannelProviderEndpoint()))) {


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

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

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.

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.

lqiu96

Code LGTM, added a small comment if you could take a look.

I think it should be fine, but we can try to address any additional things after more testing.

rmehta19 · 2025-05-30T16:16:08Z

Code LGTM, added a small comment if you could take a look.

I think it should be fine, but we can try to address any additional things after more testing.

Thanks @lqiu96 ! Addressed your latest comment. PTAL when you get a chance.

lqiu96 · 2025-05-30T17:37:09Z

/gcbrun

lqiu96 · 2025-05-30T17:38:38Z

Re-running the failed showcase test. Not sure if it's flaky or not and we can see after this run.

Also, can you update the title to follow conventioncommits with the update changes? i.e. fix: S2A- Check if a default endpoint has been set or however you see fit. Thanks!

rmehta19 · 2025-05-30T23:35:47Z

Re-running the failed showcase test. Not sure if it's flaky or not and we can see after this run.

Also, can you update the title to follow conventioncommits with the update changes? i.e. fix: S2A- Check if a default endpoint has been set or however you see fit. Thanks!

Done, thanks @lqiu96 !

lqiu96 · 2025-06-02T14:29:19Z

Seeing some odd showcase errors. Can you try merging the latest into the branch and we can see if that resolves the CI issues?

rmehta19 · 2025-06-02T16:05:22Z

Seeing some odd showcase errors. Can you try merging the latest into the branch and we can see if that resolves the CI issues?

Merged the latest changes from main @lqiu96

lqiu96 · 2025-06-02T17:08:09Z

/gcbrun

lqiu96 · 2025-06-02T17:13:42Z

Seeing some odd showcase errors. Can you try merging the latest into the branch and we can see if that resolves the CI issues?

Merged the latest changes from main @lqiu96

Thanks! Running the graalvm checks and should auto-merge after success

Certain handwritten client libraries set this field to the (TLS) endpoint via [`GrpcTransportOptions.setUpChannelProvider`](https://github.com/googleapis/sdk-platform-java/blob/2a41393c0e148e96fd3a0a8e1505e0827aa4fd3a/java-core/google-cloud-core-grpc/src/main/java/com/google/cloud/grpc/GrpcTransportOptions.java#L165). This isn't really a custom endpoint, so we should be able to use S2A when this is set. Example with Cloud Logging: https://github.com/googleapis/java-logging/blob/644ebd756d44ffc02b58aa30d9a07c8afe700981/google-cloud-logging/src/main/java/com/google/cloud/logging/spi/v2/GrpcLoggingRpc.java#L137

🤖 I have created a release *beep* *boop* --- <details><summary>2.59.1</summary> ## [2.59.1](v2.59.0...v2.59.1) (2025-06-12) ### Bug Fixes * S2A- Check if a default endpoint has been set ([#3784](#3784)) ([5b2ab82](5b2ab82)) ### Dependencies * update google auth library dependencies to v1.37.0 ([#3830](#3830)) ([8bf9d3c](8bf9d3c)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

don't check transport channel provider endpoint.

09b024e

product-auto-label bot added the size: s Pull request size is small. label May 6, 2025

merge upstream.

c4ed18e

Merge branch 'main' into dont-check-transport-channel-endpoint

4bf3ade

lqiu96 reviewed May 28, 2025

View reviewed changes

rmehta19 and others added 3 commits May 29, 2025 09:50

check transportchannelprovider endpoint not the default endpoint.

da85450

Merge branch 'main' into dont-check-transport-channel-endpoint

be79698

update tests.

6ad5b8e

rmehta19 force-pushed the dont-check-transport-channel-endpoint branch from 981454b to 6ad5b8e Compare May 29, 2025 17:29

rmehta19 added 3 commits May 29, 2025 11:10

fix tests.

5617650

fix if condition.

6fb54f9

fix if.

4bba85b

rmehta19 commented May 29, 2025

View reviewed changes

lqiu96 previously approved these changes May 29, 2025

View reviewed changes

check if clientSettingsEndpoint is default endpoint

5fafdaf

rmehta19 dismissed lqiu96’s stale review via 5fafdaf May 30, 2025 16:11

Merge branch 'main' into dont-check-transport-channel-endpoint

604fe07

rmehta19 changed the title ~~s2a fix: don't check transport channel provider endpoint.~~ fix: S2A- Check if a default endpoint has been set May 30, 2025

lqiu96 approved these changes Jun 2, 2025

View reviewed changes

Merge branch 'main' into dont-check-transport-channel-endpoint

9b52a4e

lqiu96 enabled auto-merge (squash) June 2, 2025 17:11

lqiu96 merged commit 5b2ab82 into googleapis:main Jun 2, 2025
51 of 53 checks passed

release-please bot mentioned this pull request Jun 2, 2025

chore(main): release 2.59.1 #3824

Merged

	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);

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

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

Uh oh!

Conversation

rmehta19 commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rmehta19 commented May 7, 2025

Uh oh!

lqiu96 commented May 8, 2025

Uh oh!

rmehta19 commented May 12, 2025

Uh oh!

rmehta19 commented May 19, 2025

Uh oh!

lqiu96 May 28, 2025

Choose a reason for hiding this comment

Uh oh!

lqiu96 May 28, 2025

Choose a reason for hiding this comment

Uh oh!

rmehta19 May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lqiu96 May 28, 2025

Choose a reason for hiding this comment

Uh oh!

rmehta19 May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmehta19 May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lqiu96 May 29, 2025

Choose a reason for hiding this comment

Uh oh!

rmehta19 May 30, 2025

Choose a reason for hiding this comment

Uh oh!

lqiu96 May 30, 2025

Choose a reason for hiding this comment

Uh oh!

lqiu96 left a comment

Choose a reason for hiding this comment

Uh oh!

rmehta19 commented May 30, 2025

Uh oh!

lqiu96 commented May 30, 2025

Uh oh!

lqiu96 commented May 30, 2025

Uh oh!

rmehta19 commented May 30, 2025

Uh oh!

lqiu96 commented Jun 2, 2025

Uh oh!

rmehta19 commented Jun 2, 2025

Uh oh!

lqiu96 commented Jun 2, 2025

Uh oh!

lqiu96 commented Jun 2, 2025

Uh oh!

Uh oh!

Uh oh!

rmehta19 commented May 6, 2025 •

edited

Loading

rmehta19 May 28, 2025 •

edited

Loading

rmehta19 May 29, 2025 •

edited

Loading

rmehta19 May 29, 2025 •

edited

Loading