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.

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 an option to enable DirectPath xDS #1643

Merged
merged 9 commits into from
May 1, 2023
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 attemptDirectPathXds;
@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.attemptDirectPathXds = builder.attemptDirectPathXds;
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 isDirectPathXdsEnabled() {
// Method 1: Enable DirectPath xDS by option.
if (Boolean.TRUE.equals(attemptDirectPathXds)) {
return true;
}
// Method 2: Enable DirectPath xDS by env.
String directPathXdsEnv = envProvider.getenv(DIRECT_PATH_ENV_ENABLE_XDS);
boolean isDirectPathXdsEnv = Boolean.parseBoolean(directPathXdsEnv);
if (isDirectPathXdsEnv) {
return true;
}
return false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add one more test case for the default value? Our sonar check is failing because the new code is lacking coverage, I know we cannot set environment variable in unit tests, but we should be able to test the default case.

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. PTAL. Thanks!

}

private boolean isNonDefaultServiceAccountAllowed() {
if (allowNonDefaultServiceAccount != null && allowNonDefaultServiceAccount) {
return true;
Expand Down Expand Up @@ -317,15 +333,15 @@ private ManagedChannel createSingleChannel() throws IOException {
ManagedChannelBuilder<?> builder;

// Check DirectPath traffic.
boolean isDirectPathXdsEnabled = false;
boolean useDirectPathXds = false;
if (isDirectPathEnabled(serviceAddress)
&& isNonDefaultServiceAccountAllowed()
&& isOnComputeEngine()) {
CallCredentials callCreds = MoreCallCredentials.from(credentials);
ChannelCredentials channelCreds =
GoogleDefaultChannelCredentials.newBuilder().callCredentials(callCreds).build();
isDirectPathXdsEnabled = Boolean.parseBoolean(envProvider.getenv(DIRECT_PATH_ENV_ENABLE_XDS));
if (isDirectPathXdsEnabled) {
useDirectPathXds = isDirectPathXdsEnabled();
if (useDirectPathXds) {
// google-c2p: CloudToProd(C2P) Directpath. This scheme is defined in
// io.grpc.googleapis.GoogleCloudToProdNameResolverProvider.
// This resolver target must not have a port number.
Expand All @@ -352,7 +368,7 @@ && isOnComputeEngine()) {
}
}
// google-c2p resolver requires service config lookup
if (!isDirectPathXdsEnabled) {
if (!useDirectPathXds) {
// See https://github.com/googleapis/gapic-generator/issues/2816
builder.disableServiceConfigLookUp();
}
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 attemptDirectPathXds;
@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.attemptDirectPathXds = provider.attemptDirectPathXds;
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. */
@InternalApi("For internal use by google-cloud-java clients only")
public Builder setAttemptDirectPathXds() {
this.attemptDirectPathXds = true;
return this;
}

/**
* Sets a service config for direct path. If direct path is not enabled, the provided service
* config will be ignored.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,31 @@ public void testWithGCECredentials() throws IOException {
provider.getTransportChannel().shutdownNow();
}

@Test
public void testDirectPathXds() throws IOException {
ScheduledExecutorService executor = new ScheduledThreadPoolExecutor(1);
executor.shutdown();

TransportChannelProvider provider =
InstantiatingGrpcChannelProvider.newBuilder()
.setAttemptDirectPath(true)
.setAttemptDirectPathXds()
.build()
.withExecutor((Executor) executor)
.withHeaders(Collections.<String, String>emptyMap())
.withEndpoint("localhost:8080");

assertThat(provider.needsCredentials()).isTrue();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I understand this test case, does setAttemptDirectPathXds make needsCredentials() to true indirectly?

if (InstantiatingGrpcChannelProvider.isOnComputeEngine()) {
provider = provider.withCredentials(ComputeEngineCredentials.create());
} else {
provider = provider.withCredentials(CloudShellCredentials.create(3000));
}
assertThat(provider.needsCredentials()).isFalse();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why provider.needsCredentials() is false now?


provider.getTransportChannel().shutdownNow();
}

@Test
public void testWithNonGCECredentials() throws IOException {
ScheduledExecutorService executor = new ScheduledThreadPoolExecutor(1);
Expand Down