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

feat: add google-c2p dependence to DirectPath #1521

Merged
merged 11 commits into from Oct 24, 2021
Merged

feat: add google-c2p dependence to DirectPath #1521

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
119b6d1
feat: add google-c2p dependence to DirectPath
mohanli-ml Oct 14, 2021
ce1227a
feat: add google-c2p dependence to DirectPath
mohanli-ml Oct 15, 2021
d8188d3
feat: add google-c2p dependence to DirectPath
mohanli-ml Oct 15, 2021
8bfcd10
feat: add google-c2p dependence to DirectPath
mohanli-ml Oct 19, 2021
77e81d7
feat: add google-c2p dependence to DirectPath
mohanli-ml Oct 19, 2021
70f3af2
Merge branch 'main' into add-google-c2p
mohanli-ml Oct 19, 2021
90ed61e
Merge branch 'main' into add-google-c2p
chanseokoh Oct 20, 2021
baaff18
Simplify code
chanseokoh Oct 20, 2021
db57e67
Simplify
chanseokoh Oct 20, 2021
1c288e0
feat: add google-c2p dependence to DirectPath
mohanli-ml Oct 20, 2021
c863608
feat: add google-c2p dependence to DirectPath
mohanli-ml Oct 21, 2021
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
1 change: 1 addition & 0 deletions build.gradle
View file
Open in desktop
Expand Up @@ -40,6 +40,7 @@ ext {
'maven.io_grpc_grpc_protobuf': "io.grpc:grpc-protobuf:${libraries['version.io_grpc']}",
'maven.io_grpc_grpc_netty_shaded': "io.grpc:grpc-netty-shaded:${libraries['version.io_grpc']}",
'maven.io_grpc_grpc_alts': "io.grpc:grpc-alts:${libraries['version.io_grpc']}",
'maven.io_grpc_grpc_xds': "io.grpc:grpc-xds:${libraries['version.io_grpc']}",
Copy link
Contributor

Choose a reason for hiding this comment

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

I just noticed that this should be added to dependencies.properties, not here, so that both Bazel and Gradle resolve and use the dependency.

However, the Bazel build didn't break, meaning that this is unnecessary. I also just compiled the project with Gradle manually without this, and it compiled successfully. In fact, I don't see any new class being used in the code below. Obviously, this dependency is not required for compilation. Should we remove it?

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 just noticed that this should be added to dependencies.properties, not here, so that both Bazel and Gradle resolve and use the dependency.

Can you give me an example about how to add the dependency to dependencies.properties? And should I also remove change in both gax-grpc/build.gradle?

However, the Bazel build didn't break, meaning that this is unnecessary. I also just compiled the project with Gradle manually without this, and it compiled successfully. In fact, I don't see any new class being used in the code below. Obviously, this dependency is not required for compilation. Should we remove it?

Compilation can succeed because as you said, no new class is added. However, this dependency is actually registering google-c2p resolver to the build binary, otherwise gRPC can not recognize the google-c2p:/// URI scheme and we will get error like Java.lang.IllegalArgumentException: cannot find a NameResolver for google-c2p:///localhost. So we should keep the dependency.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. What you are saying sounds like the dependency is required to exist in the runtime classpath of the user consuming the gax-java library.

Since it's not required for compiling the project, let's have this as a Gradle-only dependency. I'm pretty new to this repo, so I don't know whether the Bazel build will also need this for something. However, gax-java libraries are released from Gradle, at least they will be okay. If there comes an issue on the Bazel side later, we'll take care of it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, thanks!

'maven.com_google_protobuf': "com.google.protobuf:protobuf-java:${libraries['version.com_google_protobuf']}",
'maven.com_google_protobuf_java_util': "com.google.protobuf:protobuf-java-util:${libraries['version.com_google_protobuf']}")
}
Expand Down
3 changes: 2 additions & 1 deletion gax-grpc/build.gradle
View file
Open in desktop
Expand Up @@ -18,7 +18,8 @@ dependencies {
libraries['maven.io_grpc_grpc_auth'],
libraries['maven.io_grpc_grpc_netty_shaded'],
libraries['maven.io_grpc_grpc_protobuf'],
libraries['maven.io_grpc_grpc_stub'])
libraries['maven.io_grpc_grpc_stub'],
libraries['maven.io_grpc_grpc_xds'])
Copy link
Contributor

Choose a reason for hiding this comment

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

cc @ejona86 will adding grpc-xds explode dependencies and the size of gax-grpc library?

Copy link

Choose a reason for hiding this comment

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

Yes, but that is the design. With added risk, it would be possible to delay adding this as a dependency until users are expected to actually use it. But this would eventually need to happen.

With some effort, the C2P-enabled services could add this dependency instead of gax. So only those services using it would pay the price.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should not add this per my comment above?

Copy link
Contributor

Choose a reason for hiding this comment

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

Per my new comment, I think this should be a runtime classpath. Note that you added this to implementation on line 15. Assuming that it's a runtime dependency, you need to do

runtimeOnly libraries['maven.io_grpc_grpc_xds']

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


compileOnly libraries['maven.com_google_auto_value_auto_value']

Expand Down
37 changes: 31 additions & 6 deletions gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
View file
Open in desktop
Expand Up @@ -83,6 +83,7 @@ public final class InstantiatingGrpcChannelProvider implements TransportChannelP
static final String DIRECT_PATH_ENV_VAR = "GOOGLE_CLOUD_ENABLE_DIRECT_PATH";
private static final String DIRECT_PATH_ENV_DISABLE_DIRECT_PATH =
"GOOGLE_CLOUD_DISABLE_DIRECT_PATH";
private static final String DIRECT_PATH_ENV_ENABLE_XDS = "GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS";
static final long DIRECT_PATH_KEEP_ALIVE_TIME_SECONDS = 3600;
static final long DIRECT_PATH_KEEP_ALIVE_TIMEOUT_SECONDS = 20;
// reduce the thundering herd problem of too many channels trying to (re)connect at the same time
Expand All @@ -108,6 +109,7 @@ public final class InstantiatingGrpcChannelProvider implements TransportChannelP
@Nullable private final Boolean allowNonDefaultServiceAccount;
@VisibleForTesting final ImmutableMap<String, ?> directPathServiceConfig;
@Nullable private final MtlsProvider mtlsProvider;
@Nullable @VisibleForTesting private String activeEndpoint;

@Nullable
private final ApiFunction<ManagedChannelBuilder, ManagedChannelBuilder> channelConfigurator;
Expand Down Expand Up @@ -135,6 +137,7 @@ private InstantiatingGrpcChannelProvider(Builder builder) {
builder.directPathServiceConfig == null
? getDefaultDirectPathServiceConfig()
: builder.directPathServiceConfig;
this.activeEndpoint = builder.endpoint;
}

/**
Expand Down Expand Up @@ -330,16 +333,24 @@ private ManagedChannel createSingleChannel() throws IOException, GeneralSecurity

ManagedChannelBuilder<?> builder;

// TODO(weiranf): Add API in ComputeEngineCredentials to check default service account.
// Check DirectPath traffic.
boolean isDirectPathXdsEnabled = false;
if (isDirectPathEnabled(serviceAddress)
&& isNonDefaultServiceAccountAllowed()
&& isOnComputeEngine()) {
builder = ComputeEngineChannelBuilder.forAddress(serviceAddress, port);
isDirectPathXdsEnabled = Boolean.parseBoolean(envProvider.getenv(DIRECT_PATH_ENV_ENABLE_XDS));
if (isDirectPathXdsEnabled) {
// google-c2p resolver target must not have a port number
activeEndpoint = "google-c2p:///" + serviceAddress;
builder = ComputeEngineChannelBuilder.forTarget(activeEndpoint);
} else {
builder = ComputeEngineChannelBuilder.forAddress(serviceAddress, port);
builder.defaultServiceConfig(directPathServiceConfig);
}
// Set default keepAliveTime and keepAliveTimeout when directpath environment is enabled.
// Will be overridden by user defined values if any.
builder.keepAliveTime(DIRECT_PATH_KEEP_ALIVE_TIME_SECONDS, TimeUnit.SECONDS);
builder.keepAliveTimeout(DIRECT_PATH_KEEP_ALIVE_TIMEOUT_SECONDS, TimeUnit.SECONDS);
builder.defaultServiceConfig(directPathServiceConfig);
} else {
ChannelCredentials channelCredentials = createMtlsChannelCredentials();
if (channelCredentials != null) {
Expand All @@ -348,10 +359,13 @@ && isOnComputeEngine()) {
builder = ManagedChannelBuilder.forAddress(serviceAddress, port);
}
}
// google-c2p resolver requires service config lookup
if (!isDirectPathXdsEnabled) {
// See https://github.com/googleapis/gapic-generator/issues/2816
builder.disableServiceConfigLookUp();
}
builder =
builder
// See https://github.com/googleapis/gapic-generator/issues/2816
.disableServiceConfigLookUp()
.intercept(new GrpcChannelUUIDInterceptor())
.intercept(headerInterceptor)
.intercept(metadataHandlerInterceptor)
Expand Down Expand Up @@ -387,11 +401,16 @@ && isOnComputeEngine()) {
return managedChannel;
}

/** The endpoint to be used for the channel. */
/** The endpoint passed to the builder, which must have a port. */
public String getEndpoint() {
return endpoint;
}

/** The actual endpoint to be used for building a channel. */
public String getActiveEndpoint() {
return activeEndpoint;
}

/** The time without read activity before sending a keepalive ping. */
public Duration getKeepAliveTime() {
return keepAliveTime;
Expand Down Expand Up @@ -524,6 +543,12 @@ Builder setMtlsProvider(MtlsProvider mtlsProvider) {
return this;
}

@VisibleForTesting
Builder setEnvProvider(EnvironmentProvider envProvider) {
this.envProvider = envProvider;
return this;
}

/**
* Sets the GrpcInterceptorProvider for this TransportChannelProvider.
*
Expand Down
40 changes: 40 additions & 0 deletions gax-grpc/src/test/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProviderTest.java
View file
Open in desktop
Expand Up @@ -67,6 +67,23 @@

@RunWith(JUnit4.class)
public class InstantiatingGrpcChannelProviderTest extends AbstractMtlsTransportChannelTest {
static class TestEnvironmentProvider
implements InstantiatingGrpcChannelProvider.EnvironmentProvider {
private final String isDirectPathXdsEnabled;

TestEnvironmentProvider(String isDirectPathXdsEnabled) {
this.isDirectPathXdsEnabled = isDirectPathXdsEnabled;
}

@Override
public String getenv(String env) {
if (env.equals("GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS")) {
return isDirectPathXdsEnabled;
}
return System.getenv(env);
}
}

@Test
public void testEndpoint() {
String endpoint = "localhost:8080";
Expand Down Expand Up @@ -390,6 +407,29 @@ public ManagedChannelBuilder apply(ManagedChannelBuilder channelBuilder) {
provider.getTransportChannel().shutdownNow();
}

@Test
public void testWithDirectPathXds() throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for trying. I saw this test is failing, and I think I know why. In order to test the section of the interest, we should satisfy the following constraints:

    if (isDirectPathEnabled(serviceAddress)
        && isNonDefaultServiceAccountAllowed()
        && isOnComputeEngine()) {

The problem is that, isOnComputeEngine() is not controllable. Certainly, this class is not very testable. For making this testable, we should further refactor the class so that the information about whether it's on GCE is given from another provider (like an env provider).

But at this point, I don't want to make your life too difficult. Let's forget adding this and the setEnvProvider/activeEndpoint stuff to maintain simple code. Sorry, I appreciate your time you spent to explore this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, thanks! I will remove the unit test then.

ScheduledExecutorService executor = new ScheduledThreadPoolExecutor(1);
executor.shutdown();

InstantiatingGrpcChannelProvider grpcChannelProvider =
InstantiatingGrpcChannelProvider.newBuilder()
.setAttemptDirectPath(true)
.setEnvProvider(new TestEnvironmentProvider(/*isDirectPathXdsEnabled = */ "true"))
.build();

TransportChannelProvider transportChannelProvider =
grpcChannelProvider
.withHeaders(Collections.<String, String>emptyMap())
.withExecutor((Executor) executor)
.withEndpoint("localhost:8080")
.withCredentials(ComputeEngineCredentials.create());

transportChannelProvider.getTransportChannel().shutdownNow();

assertEquals(grpcChannelProvider.getActiveEndpoint(), "google-c2p:///localhost");
}

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