Skip to content
Merged
Changes from 9 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -126,16 +126,32 @@ public final class InstantiatingGrpcChannelProvider implements TransportChannelP
@Nullable private final Boolean allowNonDefaultServiceAccount;
@VisibleForTesting final ImmutableMap<String, ?> directPathServiceConfig;
@Nullable private final MtlsProvider mtlsProvider;
@Nullable private final List<HardBoundTokenTypes> allowedHardBoundTokenTypes;
@VisibleForTesting final Map<String, String> headersWithDuplicatesRemoved = new HashMap<>();

@Nullable
private final ApiFunction<ManagedChannelBuilder, ManagedChannelBuilder> channelConfigurator;

/*
* Experimental feature
*
* <p>{@link HardBoundTokenTypes} specifies if hard bound tokens should be used if DirectPath
* or S2A is used to estabilsh a connection to Google APIs.
*
*/
public enum HardBoundTokenTypes {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to also mark this as internal?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed that this should be marked as Internal Api, since this is intended to be set by client libraries. Done in 591ef68

This is being marked as experimental for now, since we are in progress of adding the related logic (e.g #3548, #3572) and then piloting, as discussed in the internal doc + chat. When the feature is non-experimental, the field (allowedHardBoundTokenTypes) will be set for all gapics to include both (MTLS_S2A and ALTS), however handwritten libraries will continue to set this field (allowedHardBoundTokenTypes) themselves in their handwritten layer (e.g. GCS). Additionally, when it is non-experimental, gapics + handwritten libraries will have the option to override the default value of the allowedHardBoundTokenTypes. I think the enum helps to proves clarity on the options.

// Use ALTS bound tokens when using DirectPath
ALTS,
// Use MTLS bound tokens when using S2A
MTLS_S2A
}

private InstantiatingGrpcChannelProvider(Builder builder) {
this.processorCount = builder.processorCount;
this.executor = builder.executor;
this.headerProvider = builder.headerProvider;
this.endpoint = builder.endpoint;
this.allowedHardBoundTokenTypes = builder.allowedHardBoundTokenTypes;
this.mtlsProvider = builder.mtlsProvider;
this.envProvider = builder.envProvider;
this.interceptorProvider = builder.interceptorProvider;
Expand Down Expand Up @@ -620,6 +636,7 @@ public static final class Builder {
@Nullable private Boolean attemptDirectPathXds;
@Nullable private Boolean allowNonDefaultServiceAccount;
@Nullable private ImmutableMap<String, ?> directPathServiceConfig;
@Nullable private List<HardBoundTokenTypes> allowedHardBoundTokenTypes;

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

/*
* Sets the allowed hard bound token types for this TransportChannelProvider.
*
* <p>This is optional; if it is not provided, bearer tokens will be used.
*
* <p>Examples:
*
* <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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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! Looking at this again agreed that the behavior of setting the field to different values should be paired with the logic. Updated the javadoc to specify what the field means. Also made the enum javadoc a bit more specific.

90a32af

*/
@InternalApi
public Builder setAllowHardBoundTokenTypes(List<HardBoundTokenTypes> allowedValues) {
this.allowedHardBoundTokenTypes = allowedValues;
return this;
}

@VisibleForTesting
Builder setMtlsProvider(MtlsProvider mtlsProvider) {
this.mtlsProvider = mtlsProvider;
Expand Down
Loading