fix: check Compute Engine environment for DirectPath #1250

mohanli-ml · 2020-11-14T03:31:17Z

Add Compute Engine environment check since DirectPath can only be used on Compute Engine. This doc is used for checking: https://docs.google.com/document/d/1xQXE27x9wTvwPsgiX9Hn0o8mcq5z3SKi-1jwscQsCAk/edit#. Relevant issue and PR: grpc/grpc-java#7604 and googleapis/java-bigtable#520.

elharo

needs tests

gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java

elharo · 2020-11-18T20:10:48Z

gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java

+          resultOutStream.write(bs, 0, num);
+        }
+        String result = new String(resultOutStream.toByteArray());
+        return result.contains("01/01/2011") && result.contains("Google");


why this date?

See this doc: https://docs.google.com/document/d/1xQXE27x9wTvwPsgiX9Hn0o8mcq5z3SKi-1jwscQsCAk/edit#heading=h.7ioa2p9bw6l1

As this is open source code, can this be made with a reference to a public doc?
Also this needs a comment in the source code explaining it, and possibly a named constant for the date.

Sorry I could not find a public doc, so I just removed the link in the code. Two constants are added though.

per doc, Virtual machines started after August 2016 have access to a more reliable "product name" string which is equal to "Google Compute Engine".

"Google Compute Engine" is for /sys/class/dmi/id/product_name, but for now we are checking /sys/class/dmi/id/bios_date and /sys/class/dmi/id/bios_vendor in the code. Do you think we also need to check /sys/class/dmi/id/product_name?

I'm just going by the doc you cited. I read it as saying one should use product name instead of bios id and vendor

I see, yeah, thanks, product name is used now.

codecov · 2020-11-19T02:16:54Z

Codecov Report

Merging #1250 (cf494ce) into master (d455da2) will decrease coverage by 0.34%.
The diff coverage is 42.85%.

@@             Coverage Diff              @@
##             master    #1250      +/-   ##
============================================
- Coverage     79.32%   78.98%   -0.35%     
  Complexity     1226     1226              
============================================
  Files           209      209              
  Lines          5344     5357      +13     
  Branches        442      446       +4     
============================================
- Hits           4239     4231       -8     
- Misses          931      948      +17     
- Partials        174      178       +4

Impacted Files	Coverage Δ	Complexity Δ
...api/gax/grpc/InstantiatingGrpcChannelProvider.java	`69.08% <42.85%> (-8.76%)`	`33.00 <1.00> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d455da2...cf494ce. Read the comment docs.

elharo · 2020-11-19T18:04:19Z

gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java

+          resultOutStream.write(bs, 0, num);
+        }
+        String result = new String(resultOutStream.toByteArray());
+        return result.contains("01/01/2011") && result.contains("Google");


per doc, Virtual machines started after August 2016 have access to a more reliable "product name" string which is equal to "Google Compute Engine".

elharo · 2020-11-19T18:04:55Z

gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java

+        Process process = Runtime.getRuntime().exec(new String[] {"/bin/sh", "-c", cmd});
+        process.waitFor();
+        String result = CharStreams.toString(new InputStreamReader(process.getInputStream()));
+        return result.contains(GCE_BIOS_DATE) && result.contains(GCE_BIOS_VENDOR);


per doc, "Virtual machines started after August 2016 have access to a more reliable "product name" string which is equal to "Google Compute Engine"."

elharo · 2020-11-19T19:58:47Z

gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java

+      try {
+        Process process = Runtime.getRuntime().exec(new String[] {"/bin/sh", "-c", cmd});
+        process.waitFor();
+        String result = CharStreams.toString(new InputStreamReader(process.getInputStream()));


This needs an explicit encoding, probably UTF-8

UTF-8 encoding added. Thanks!

chingor13 · 2020-11-19T21:16:04Z

gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java

@@ -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.
+  public static boolean isOnComputeEngine() {


Does this need to be public? If it's just for testing we can make it default visibility.

This is a good point. public has been removed. Thanks!

ejona86 · 2020-11-23T17:23:30Z

gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java

+    if ("Linux".equals(osName)) {
+      String cmd = "cat /sys/class/dmi/id/product_name";
+      try {
+        Process process = Runtime.getRuntime().exec(new String[] {"/bin/sh", "-c", cmd});


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?

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?

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.

OK, I will have another PR for this.

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

Please file an issue

sanjaypujare · 2021-03-09T01:00:37Z

gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java

+        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);


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.

mohanli-ml requested review from a team as code owners November 14, 2020 03:31

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 14, 2020

elharo reviewed Nov 14, 2020

View reviewed changes

elharo changed the title ~~fix: check Compute Engine environmentfor DirectPath~~ fix: check Compute Engine environment for DirectPath Nov 17, 2020

elharo suggested changes Nov 18, 2020

View reviewed changes

elharo reviewed Nov 19, 2020

View reviewed changes

mohanli-ml requested a review from elharo November 19, 2020 18:32

elharo reviewed Nov 19, 2020

View reviewed changes

elharo approved these changes Nov 19, 2020

View reviewed changes

chingor13 suggested changes Nov 19, 2020

View reviewed changes

fix: check Compute Engine environment for DirectPath

cf494ce

chingor13 approved these changes Nov 19, 2020

View reviewed changes

chingor13 merged commit 656b613 into googleapis:master Nov 19, 2020

ejona86 reviewed Nov 23, 2020

View reviewed changes

mohanli-ml mentioned this pull request Nov 30, 2020

feat: attempt DirectPath by default googleapis/java-bigtable#544

Merged

sanjaypujare reviewed Mar 9, 2021

View reviewed changes

sanjaypujare mentioned this pull request Mar 9, 2021

code should be updated as indicated in the PR comments #1323

Closed

igorbernstein2 mentioned this pull request Jun 29, 2021

feat: allow DirectPath by default + update integration tests googleapis/java-bigtable-hbase#3031

Merged

mohanli-ml mentioned this pull request Oct 17, 2023

feat: log DirectPath misconfiguration googleapis/sdk-platform-java#2105

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: check Compute Engine environment for DirectPath #1250

fix: check Compute Engine environment for DirectPath #1250

mohanli-ml commented Nov 14, 2020 •

edited

elharo left a comment

elharo Nov 18, 2020

mohanli-ml Nov 18, 2020

elharo Nov 19, 2020

mohanli-ml Nov 19, 2020

elharo Nov 19, 2020

mohanli-ml Nov 19, 2020

elharo Nov 19, 2020

mohanli-ml Nov 19, 2020

codecov bot commented Nov 19, 2020 •

edited

elharo Nov 19, 2020

elharo Nov 19, 2020

elharo Nov 19, 2020

mohanli-ml Nov 19, 2020

chingor13 Nov 19, 2020

mohanli-ml Nov 19, 2020

ejona86 Nov 23, 2020

mohanli-ml Nov 23, 2020

ejona86 Nov 23, 2020

mohanli-ml Nov 23, 2020

sanjaypujare Mar 9, 2021

elharo Mar 9, 2021

sanjaypujare Mar 9, 2021

sanjaypujare Mar 9, 2021

fix: check Compute Engine environment for DirectPath #1250

fix: check Compute Engine environment for DirectPath #1250

Conversation

mohanli-ml commented Nov 14, 2020 • edited

elharo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 19, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohanli-ml commented Nov 14, 2020 •

edited

codecov bot commented Nov 19, 2020 •

edited