Skip to content
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

feat: add support for new setAllowHardBoundTokens field. #3467

Merged
merged 13 commits into from
Jan 22, 2025
Merged

feat: add support for new setAllowHardBoundTokens field. #3467

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
09eb2b2
Add support for new setAllowHardBoundTokens field.
rmehta19 Dec 14, 2024
db51f4e
Merge branch 'main' into add-setting
rmehta19 Jan 7, 2025
fd1015e
Merge branch 'main' into add-setting
rmehta19 Jan 9, 2025
960084d
Clarify variable name in InstantiatingGrpcChannelProvider.
rmehta19 Jan 9, 2025
ddb3b79
Merge branch 'main' into add-setting
rmehta19 Jan 15, 2025
60dafd0
add setter for allowed hard bound token types to only the GRPC channe…
rmehta19 Jan 16, 2025
e193cc9
List not ArrayList, no setter, InternalApi.
rmehta19 Jan 16, 2025
0e40f22
add enum + javadocs.
rmehta19 Jan 16, 2025
6a27af6
Merge branch 'main' into add-setting
rmehta19 Jan 17, 2025
591ef68
mark enum as internal only.
rmehta19 Jan 21, 2025
90a32af
improve javadoc for setter.
rmehta19 Jan 21, 2025
a7f8384
add to existing unit test.
rmehta19 Jan 21, 2025
4fe44cb
Merge branch 'main' into add-setting
rmehta19 Jan 21, 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
14 changes: 14 additions & 0 deletions ...java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@
import java.nio.charset.StandardCharsets;
import java.security.GeneralSecurityException;
import java.security.KeyStore;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -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;
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

LGTM. Thanks.

@VisibleForTesting final Map<String, String> headersWithDuplicatesRemoved = new HashMap<>();

@Nullable
Expand All @@ -136,6 +138,7 @@ private InstantiatingGrpcChannelProvider(Builder builder) {
this.executor = builder.executor;
this.headerProvider = builder.headerProvider;
this.endpoint = builder.endpoint;
this.allowedValues = builder.allowedValues;
this.mtlsProvider = builder.mtlsProvider;
this.envProvider = builder.envProvider;
this.interceptorProvider = builder.interceptorProvider;
Expand Down Expand Up @@ -225,6 +228,11 @@ public TransportChannelProvider withEndpoint(String endpoint) {
return toBuilder().setEndpoint(endpoint).build();
}

@Override
public TransportChannelProvider setAllowHardBoundTokens(ArrayList<String> allowedValues) {
blakeli0 marked this conversation as resolved.
Show resolved Hide resolved
return toBuilder().setAllowHardBoundTokens(allowedValues).build();
}

/** @deprecated Please modify pool settings via {@link #toBuilder()} */
@Deprecated
@Override
Expand Down Expand Up @@ -620,6 +628,7 @@ public static final class Builder {
@Nullable private Boolean attemptDirectPathXds;
@Nullable private Boolean allowNonDefaultServiceAccount;
@Nullable private ImmutableMap<String, ?> directPathServiceConfig;
@Nullable private ArrayList<String> allowedValues;

private Builder() {
processorCount = Runtime.getRuntime().availableProcessors();
Expand Down Expand Up @@ -700,6 +709,11 @@ public Builder setEndpoint(String endpoint) {
return this;
}

public Builder setAllowHardBoundTokens(ArrayList<String> allowedValues) {
this.allowedValues = allowedValues;
return this;
}

@VisibleForTesting
Builder setMtlsProvider(MtlsProvider mtlsProvider) {
this.mtlsProvider = mtlsProvider;
Expand Down
7 changes: 7 additions & 0 deletions gax-java/gax-grpc/src/test/java/com/google/api/gax/grpc/testing/LocalChannelProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
import io.grpc.MethodDescriptor;
import io.grpc.inprocess.InProcessChannelBuilder;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.concurrent.CopyOnWriteArrayList;
Expand Down Expand Up @@ -106,6 +107,12 @@ public TransportChannelProvider withEndpoint(String endpoint) {
throw new UnsupportedOperationException("LocalChannelProvider doesn't need an endpoint");
}

@Override
public TransportChannelProvider setAllowHardBoundTokens(ArrayList<String> allowedValues) {
throw new UnsupportedOperationException(
"LocalChannelProvider doesn't support hard-bound tokens");
}

@Override
@BetaApi("The surface for customizing pool size is not stable yet and may change in the future.")
public boolean acceptsPoolSize() {
Expand Down
7 changes: 7 additions & 0 deletions ...pjson/src/main/java/com/google/api/gax/httpjson/InstantiatingHttpJsonChannelProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import java.io.IOException;
import java.security.GeneralSecurityException;
import java.security.KeyStore;
import java.util.ArrayList;
import java.util.Map;
import java.util.concurrent.Executor;
import java.util.concurrent.ScheduledExecutorService;
Expand Down Expand Up @@ -139,6 +140,12 @@ public TransportChannelProvider withPoolSize(int size) {
"InstantiatingHttpJsonChannelProvider doesn't allow pool size customization");
}

@Override
public TransportChannelProvider setAllowHardBoundTokens(ArrayList<String> allowedValues) {
throw new UnsupportedOperationException(
"InstantiatingHttpJsonChannelProvider doesn't support hard-bound tokens");
}

@Override
public String getTransportName() {
return HttpJsonTransportChannel.getHttpJsonTransportName();
Expand Down
7 changes: 7 additions & 0 deletions gax-java/gax/src/main/java/com/google/api/gax/rpc/FixedTransportChannelProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import com.google.auth.Credentials;
import com.google.common.base.Preconditions;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Map;
import java.util.concurrent.Executor;
import java.util.concurrent.ScheduledExecutorService;
Expand Down Expand Up @@ -68,6 +69,12 @@ public FixedTransportChannelProvider withExecutor(Executor executor) {
"FixedTransportChannelProvider doesn't need an executor");
}

@Override
public TransportChannelProvider setAllowHardBoundTokens(ArrayList<String> allowedValues) {
throw new UnsupportedOperationException(
"FixedTransportChannelProvider doesn't support hard-bound tokens");
}

@Override
public boolean needsHeaders() {
return false;
Expand Down
4 changes: 4 additions & 0 deletions gax-java/gax/src/main/java/com/google/api/gax/rpc/TransportChannelProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import com.google.api.core.InternalExtensionOnly;
import com.google.auth.Credentials;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Map;
import java.util.concurrent.Executor;
import java.util.concurrent.ScheduledExecutorService;
Expand Down Expand Up @@ -97,6 +98,9 @@ public interface TransportChannelProvider {
*/
TransportChannelProvider withEndpoint(String endpoint);

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

Thanks for the details! It makes perfect sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks all for the discussion! Done in 60dafd0


/**
* Reports whether this provider allows pool size customization.
*
Expand Down
12 changes: 12 additions & 0 deletions gax-java/gax/src/test/java/com/google/api/gax/rpc/ClientContextTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
import com.google.common.truth.Truth;
import java.io.IOException;
import java.net.URI;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -200,6 +201,17 @@ public boolean acceptsPoolSize() {
return false;
}

@Override
public TransportChannelProvider setAllowHardBoundTokens(ArrayList<String> allowedValues) {
return new FakeTransportProvider(
this.transport,
this.executor,
this.shouldAutoClose,
this.headers,
this.credentials,
this.endpoint);
}

@Override
public TransportChannelProvider withPoolSize(int size) {
throw new UnsupportedOperationException(
Expand Down
Loading