Skip to content
This repository has been archived by the owner on Sep 26, 2023. It is now read-only.

feat: add an option to enable DirectPath xDS #1968

Closed
wants to merge 3 commits into from
Closed

feat: add an option to enable DirectPath xDS #1968

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ce3530e
Add an option to enable DirectPath xDS
mohanli-ml Apr 12, 2023
f3973f0
feat: add an option to enable DirectPath xDS
mohanli-ml Apr 12, 2023
fb378ee
feat: add an option to enable DirectPath xDS
mohanli-ml Apr 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 26 additions & 1 deletion 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 @@ -109,6 +109,7 @@ public final class InstantiatingGrpcChannelProvider implements TransportChannelP
@Nullable private final Credentials credentials;
@Nullable private final ChannelPrimer channelPrimer;
@Nullable private final Boolean attemptDirectPath;
@Nullable private final Boolean useDirectPathXds;
@Nullable private final Boolean allowNonDefaultServiceAccount;
@VisibleForTesting final ImmutableMap<String, ?> directPathServiceConfig;
@Nullable private final MtlsProvider mtlsProvider;
Expand All @@ -134,6 +135,7 @@ private InstantiatingGrpcChannelProvider(Builder builder) {
this.credentials = builder.credentials;
this.channelPrimer = builder.channelPrimer;
this.attemptDirectPath = builder.attemptDirectPath;
this.useDirectPathXds = builder.useDirectPathXds;
this.allowNonDefaultServiceAccount = builder.allowNonDefaultServiceAccount;
this.directPathServiceConfig =
builder.directPathServiceConfig == null
Expand Down Expand Up @@ -262,6 +264,20 @@ private boolean isDirectPathEnabled(String serviceAddress) {
return false;
}

private boolean isDirectPathXdsUsed() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we test this behavior in unit tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

// Method 1: Enable DirectPath xDS by env.
String directPathXdsEnv = envProvider.getenv(DIRECT_PATH_ENV_ENABLE_XDS);
boolean isDirectPathXdsEnv = Boolean.parseBoolean(directPathXdsEnv);
if (isDirectPathXdsEnv) {
return true;
}
// Method 2: Enable DirectPath xDS by option.
if (Boolean.TRUE.equals(useDirectPathXds)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually an explicitly set value has higher priority than environment variable, otherwise the users of this public field could be confused why their explicitly set value does not work.
That being said, this class and the whole DirectPath feature is supposed to be use by internal users only, so I'm fine with it if it's a requirement that env takes precedence over options, at least we should document this behavior in Javadoc.

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 reversed the order and check the option first.

return true;
}
return false;
}

private boolean isNonDefaultServiceAccountAllowed() {
if (allowNonDefaultServiceAccount != null && allowNonDefaultServiceAccount) {
return true;
Expand Down Expand Up @@ -324,7 +340,7 @@ && isOnComputeEngine()) {
CallCredentials callCreds = MoreCallCredentials.from(credentials);
ChannelCredentials channelCreds =
GoogleDefaultChannelCredentials.newBuilder().callCredentials(callCreds).build();
isDirectPathXdsEnabled = Boolean.parseBoolean(envProvider.getenv(DIRECT_PATH_ENV_ENABLE_XDS));
isDirectPathXdsEnabled = isDirectPathXdsUsed();
if (isDirectPathXdsEnabled) {
// google-c2p: CloudToProd(C2P) Directpath. This scheme is defined in
// io.grpc.googleapis.GoogleCloudToProdNameResolverProvider.
Expand Down Expand Up @@ -450,6 +466,7 @@ public static final class Builder {
@Nullable private ChannelPrimer channelPrimer;
private ChannelPoolSettings channelPoolSettings;
@Nullable private Boolean attemptDirectPath;
@Nullable private Boolean useDirectPathXds;
@Nullable private Boolean allowNonDefaultServiceAccount;
@Nullable private ImmutableMap<String, ?> directPathServiceConfig;

Expand All @@ -476,6 +493,7 @@ private Builder(InstantiatingGrpcChannelProvider provider) {
this.channelPrimer = provider.channelPrimer;
this.channelPoolSettings = provider.channelPoolSettings;
this.attemptDirectPath = provider.attemptDirectPath;
this.useDirectPathXds = provider.useDirectPathXds;
this.allowNonDefaultServiceAccount = provider.allowNonDefaultServiceAccount;
this.directPathServiceConfig = provider.directPathServiceConfig;
this.mtlsProvider = provider.mtlsProvider;
Expand Down Expand Up @@ -684,6 +702,13 @@ public Builder setAllowNonDefaultServiceAccount(boolean allowNonDefaultServiceAc
return this;
}

/** Use DirectPath xDS. Only valid if DirectPath is attempted. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Document that an environment variable could override this option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new API is used to enable DirectPath xDS, but not disable. So I removed the bool parameter. In this way, the environment can not override the option.

@InternalApi("For internal use by google-cloud-java clients only")
public Builder setUseDirectPathXds(boolean useDirectPathXds) {
this.useDirectPathXds = useDirectPathXds;
return this;
}

/**
* Sets a service config for direct path. If direct path is not enabled, the provided service
* config will be ignored.
Expand Down