Add support for new setAllowHardBoundTokens field. #3467

rmehta19 · 2024-12-14T05:22:06Z

Introduce new setAllowHardBoundTokens field.

rmehta19 · 2025-01-07T19:15:22Z

@lqiu96 , @blakeli0 , @zhumin8, please review, thanks!

rmehta19 · 2025-01-08T00:11:40Z

cc: @rockspore

rockspore

Thanks for the PR, Riya! Not up to me but LGTM.

rockspore · 2025-01-08T00:16:35Z

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

@@ -126,6 +127,7 @@ public final class InstantiatingGrpcChannelProvider implements TransportChannelP
  @Nullable private final Boolean allowNonDefaultServiceAccount;
  @VisibleForTesting final ImmutableMap<String, ?> directPathServiceConfig;
  @Nullable private final MtlsProvider mtlsProvider;
+  @Nullable private final ArrayList<String> allowedValues;


Should this variable name be more specific? Reading it alone doesn't tell you where these values will be used.

Thanks for calling this out! I updated this variable name to allowedHardBoundTokenTypes, WDYT?

LGTM. Thanks.

rmehta19 · 2025-01-15T17:29:57Z

Friendly ping @lqiu96 , @blakeli0 , @zhumin8. Thanks!

blakeli0 · 2025-01-15T18:06:20Z

What is the usage of this new allowHardBoundTokens field? Do you mind providing more background info?

blakeli0 · 2025-01-15T21:47:08Z

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

@@ -97,6 +98,9 @@ public interface TransportChannelProvider {
   */
  TransportChannelProvider withEndpoint(String endpoint);

+  /** Sets the allowed hard bound token types. */
+  TransportChannelProvider setAllowHardBoundTokens(ArrayList<String> allowedValues);


I think this field is going to be only used by gRPC transport per the internal doc? And the teams that would use it are all handwritten libraries? If that's the case, I think they are likely going to initialize an InstantiatingGrpcChannelProvider in their repo directly and pass it to Gax, so can we only add this to InstantiatingGrpcChannelProvider?

I think this field is going to be only used by gRPC transport per the internal doc?

That is correct, hard bound tokens is only for gRPC.

I think they are likely going to initialize an InstantiatingGrpcChannelProvider in their repo directly and pass it to Gax, so can we only add this to InstantiatingGrpcChannelProvider?

I think this will work since I believe the libraries separate their logic for HTTP and gRPC, so they don't end up using the interface TransportChannelProvider. Looking at GCS for example, this is the case

@rockspore can you confirm that the places you want to use this setting are using the InstantiatingGrpcChannelProvider directly, not the interface TransportChannelProvider?

For Cloud Spanner, I found this so it seems to be the case too, although I don't see where it gets called by default.

So yeah it should be good if we only do this in InstantiatingGrpcChannelProvider, as long as we have a way to set these default values eventually to all auto-generated clients.

@rockspore The link you referred to is a generated default instance of InstantiatingGrpcChannelProvider, which I believe will be overridden later in their handwritten code.
Either way, they are using InstantiatingGrpcChannelProvider directly not the interface. And since it is used within InstantiatingGrpcChannelProvider only, we don't need to expose a getter for it either. Hence it should be fine if we only add it to InstantiatingGrpcChannelProvider.

Thanks for the details! It makes perfect sense.

Thanks all for the discussion! Done in 60dafd0

rmehta19 · 2025-01-15T23:12:27Z

What is the usage of this new allowHardBoundTokens field? Do you mind providing more background info?

I believe we discussed this in the chat offline. To summarize it'll be used by clients (libraries) to indicate if they want MTLS / Directpath bound tokens. Please let me know if there is anything we can make more clear.

…l provider.

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

blakeli0 · 2025-01-16T18:17:47Z

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

@@ -700,6 +708,11 @@ public Builder setEndpoint(String endpoint) {
      return this;
    }

+    public Builder setAllowHardBoundTokenTypes(ArrayList<String> allowedValues) {


Do we expect customers to set this? Or only internal teams like Spanner/Storage to set it? If it is the latter, can we mark this method as @InternalApi?

Right we don't want external customers to set it.

We only expect internal teams to set this field, marked as @InternalApi in e193cc9

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

blakeli0 · 2025-01-16T19:00:06Z

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

@@ -700,6 +703,12 @@ public Builder setEndpoint(String endpoint) {
      return this;
    }

+    @InternalApi
+    public Builder setAllowHardBoundTokenTypes(List<String> allowedValues) {


What are the possible values of allowedValues? I think there are only two possible values so far per internal doc, if that's the case, can they be Enums so it's less error prone?

Yes we can capture the allowed values in an enum. Done in 0e40f22

zhumin8

Can you please also add some test coverage for this change?

zhumin8 · 2025-01-17T19:59:43Z

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

+     * <p>allowedValues is {HardBoundTokenTypes.ALTS}: If DirectPath is used to create the channel,
+     * use hard ALTS-bound tokens for requests sent on that channel.
+     *
+     * <p>allowedValues is {HardBoundTokenTypes.MTLS_S2A}: If MTLS via S2A is used to create the
+     * channel, use hard MTLS-bound tokens for requests sent on that channel.
+     *
+     * <p>allowedValues is {HardBoundTokenTypes.ALTS, HardBoundTokenTypes.MTLS_S2A}: if DirectPath
+     * is used to create the channel, use hard ALTS-bound tokens for requests sent on that channel.
+     * If MTLS via S2A is used to create the channel, use hard MTLS-bound tokens for requests sent
+     * on that channel.


These logic is not part of this setter, I suppose is part of a subsequent PR? Perhaps move this explanation there. In the future, if this logic changes, it is easy to miss updating here.

Agree that maybe the detailed explanation can be moved to the subsequent PR. The public doc for this method should be more like "What is HardBoundTokenTypes?", not "What would happen if we set HardBoundTokenTypes to different values?". But if listing the different behavior is the best way to explain "What is HardBoundTokenTypes?", I think this is OK too.

zhumin8 · 2025-01-17T20:08:06Z

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

+   * or S2A is used to estabilsh a connection to Google APIs.
+   *
+   */
+  public enum HardBoundTokenTypes {


Does it make sense to also mark this as internal?

Agree on making this internal as well.
Thinking twice about it though, I see that it is an Experimental feature, is it that we will always set the tokens to certain values? Or it's just this feature is not stable yet, internal teams could still set this to different values? If it's the former, then we don't have to introduce another public enum since they would be obsolete soon.

Add support for new setAllowHardBoundTokens field.

09eb2b2

product-auto-label bot added the size: m Pull request size is medium. label Dec 14, 2024

Merge branch 'main' into add-setting

db51f4e

rockspore reviewed Jan 8, 2025

View reviewed changes

rmehta19 added 3 commits January 9, 2025 06:42

Merge branch 'main' into add-setting

fd1015e

Clarify variable name in InstantiatingGrpcChannelProvider.

960084d

Merge branch 'main' into add-setting

ddb3b79

blakeli0 reviewed Jan 15, 2025

View reviewed changes

add setter for allowed hard bound token types to only the GRPC channe…

60dafd0

…l provider.

product-auto-label bot added size: s Pull request size is small. and removed size: m Pull request size is medium. labels Jan 16, 2025

blakeli0 reviewed Jan 16, 2025

View reviewed changes

List not ArrayList, no setter, InternalApi.

e193cc9

product-auto-label bot added size: xs Pull request size is extra small. and removed size: s Pull request size is small. labels Jan 16, 2025

blakeli0 reviewed Jan 16, 2025

View reviewed changes

add enum + javadocs.

0e40f22

product-auto-label bot added size: s Pull request size is small. and removed size: xs Pull request size is extra small. labels Jan 16, 2025

rockspore mentioned this pull request Jan 17, 2025

Enable DirectPath bound token in InstantiatingGrpcChannelProvider #3572

Draft

Merge branch 'main' into add-setting

6a27af6

zhumin8 reviewed Jan 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for new setAllowHardBoundTokens field. #3467

Add support for new setAllowHardBoundTokens field. #3467

rmehta19 commented Dec 14, 2024 •

edited

Loading

rmehta19 commented Jan 7, 2025

rmehta19 commented Jan 8, 2025

rockspore left a comment

rockspore Jan 8, 2025

rmehta19 Jan 9, 2025

rockspore Jan 9, 2025

rmehta19 commented Jan 15, 2025

blakeli0 commented Jan 15, 2025

blakeli0 Jan 15, 2025

rmehta19 Jan 15, 2025

rockspore Jan 16, 2025

blakeli0 Jan 16, 2025

rockspore Jan 16, 2025

rmehta19 Jan 16, 2025

rmehta19 commented Jan 15, 2025

blakeli0 Jan 16, 2025

rockspore Jan 16, 2025

rmehta19 Jan 16, 2025

blakeli0 Jan 16, 2025

rmehta19 Jan 16, 2025

zhumin8 left a comment

zhumin8 Jan 17, 2025

blakeli0 Jan 17, 2025

zhumin8 Jan 17, 2025

blakeli0 Jan 17, 2025

Add support for new setAllowHardBoundTokens field. #3467

Are you sure you want to change the base?

Add support for new setAllowHardBoundTokens field. #3467

Conversation

rmehta19 commented Dec 14, 2024 • edited Loading

rmehta19 commented Jan 7, 2025

rmehta19 commented Jan 8, 2025

rockspore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmehta19 commented Jan 15, 2025

blakeli0 commented Jan 15, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmehta19 commented Jan 15, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhumin8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmehta19 commented Dec 14, 2024 •

edited

Loading