fix: check Compute Engine environment for DirectPath by mohanli-ml · Pull Request #1250 · googleapis/gax-java

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

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

+          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

+          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

+        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

+      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


+  // 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

+    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

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

Conversation

mohanli-ml commented Nov 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elharo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Nov 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mohanli-ml commented Nov 14, 2020 •

edited

Loading

codecov Bot commented Nov 19, 2020 •

edited

Loading