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

fix: check Compute Engine environment for DirectPath #1250

Merged
merged 1 commit into from Nov 19, 2020
Merged

fix: check Compute Engine environment for DirectPath #1250

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 27 additions & 1 deletion gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
View file
Open in desktop
Expand Up @@ -44,10 +44,12 @@
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.io.CharStreams;
import io.grpc.ManagedChannel;
import io.grpc.ManagedChannelBuilder;
import io.grpc.alts.ComputeEngineChannelBuilder;
import java.io.IOException;
import java.io.InputStreamReader;
import java.util.Map;
import java.util.concurrent.Executor;
import java.util.concurrent.ScheduledExecutorService;
Expand All @@ -74,6 +76,8 @@ public final class InstantiatingGrpcChannelProvider implements TransportChannelP
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
static final int MAX_POOL_SIZE = 1000;
static final String GCE_PRODUCTION_NAME_PRIOR_2016 = "Google";
static final String GCE_PRODUCTION_NAME_AFTER_2016 = "Google Compute Engine";

private final int processorCount;
private final Executor executor;
Expand Down Expand Up @@ -234,6 +238,26 @@ private boolean isDirectPathEnabled(String serviceAddress) {
return false;
}

// DirectPath should only be used on Compute Engine.
// Notice Windows is supported for now.
static boolean isOnComputeEngine() {
String osName = System.getProperty("os.name");
if ("Linux".equals(osName)) {
mohanli-ml marked this conversation as resolved.
Show resolved Hide resolved
mohanli-ml marked this conversation as resolved.
Show resolved Hide resolved
String cmd = "cat /sys/class/dmi/id/product_name";
try {
Process process = Runtime.getRuntime().exec(new String[] {"/bin/sh", "-c", cmd});
Copy link

Choose a reason for hiding this comment

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

What is going on here? Why is this forking a process, to call a shell to then run the cat command to read a file? That seems unnecessary on two levels. Why not use a FileInputStream? Is there something I'm missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey Eric, yes FileInputStream can definitely work. Sorry I am not a java expert and I directly follow the doc to use a shall command. Do you want me to make the change?

Copy link

Choose a reason for hiding this comment

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

I'm not a gax-java maintaner, so "I have no power here." But it seems like a good idea to fix it, as it reduces the chances of something going wrong and makes it easier to debug. For example, if there's permission problems the error will be more obvious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I will have another PR for this.

Copy link

Choose a reason for hiding this comment

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

I think we should fix this. Process/exec on "bin/sh" seems completely unnecessary

Copy link
Contributor

Choose a reason for hiding this comment

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

Please file an issue

Copy link

Choose a reason for hiding this comment

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

Done #1323

process.waitFor();
String result =
CharStreams.toString(new InputStreamReader(process.getInputStream(), "UTF-8"));
return result.contains(GCE_PRODUCTION_NAME_PRIOR_2016)
|| result.contains(GCE_PRODUCTION_NAME_AFTER_2016);
Copy link

Choose a reason for hiding this comment

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

So couple of comments:

  • if the result contains something like Google App Engine then this will return true because GCE_PRODUCTION_NAME_PRIOR_2016 = "Google" even though this is App Engine and not GCE.

  • Secondly I did a test on a pod in GKE and

cat /sys/class/dmi/id/product_name
Google Compute Engine

So even on GKE it returns "Google Compute Engine" so it is quite possible even on GAE it returns the same string.

} catch (IOException | InterruptedException e) {
return false;
}
}
return false;
}

private ManagedChannel createSingleChannel() throws IOException {
GrpcHeaderInterceptor headerInterceptor =
new GrpcHeaderInterceptor(headerProvider.getHeaders());
Expand All @@ -250,7 +274,9 @@ private ManagedChannel createSingleChannel() throws IOException {
ManagedChannelBuilder builder;

// TODO(weiranf): Add API in ComputeEngineCredentials to check default service account.
if (isDirectPathEnabled(serviceAddress) && credentials instanceof ComputeEngineCredentials) {
if (isDirectPathEnabled(serviceAddress)
&& credentials instanceof ComputeEngineCredentials
&& isOnComputeEngine()) {
builder = ComputeEngineChannelBuilder.forAddress(serviceAddress, port);
// Set default keepAliveTime and keepAliveTimeout when directpath environment is enabled.
// Will be overridden by user defined values if any.
Expand Down
12 changes: 10 additions & 2 deletions gax-grpc/src/test/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProviderTest.java
View file
Open in desktop
Expand Up @@ -219,7 +219,11 @@ public void testWithGCECredentials() throws IOException {
ApiFunction<ManagedChannelBuilder, ManagedChannelBuilder> channelConfigurator =
new ApiFunction<ManagedChannelBuilder, ManagedChannelBuilder>() {
public ManagedChannelBuilder apply(ManagedChannelBuilder channelBuilder) {
assertThat(channelBuilder instanceof ComputeEngineChannelBuilder).isTrue();
if (InstantiatingGrpcChannelProvider.isOnComputeEngine()) {
assertThat(channelBuilder instanceof ComputeEngineChannelBuilder).isTrue();
} else {
assertThat(channelBuilder instanceof ComputeEngineChannelBuilder).isFalse();
}
return channelBuilder;
}
};
Expand All @@ -234,7 +238,11 @@ public ManagedChannelBuilder apply(ManagedChannelBuilder channelBuilder) {
.withEndpoint("localhost:8080");

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

provider.getTransportChannel().shutdownNow();
Expand Down