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

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

merged 1 commit into from Nov 19, 2020

Conversation

mohanli-ml
Copy link
Contributor

@mohanli-ml mohanli-ml commented Nov 14, 2020

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.

@mohanli-ml mohanli-ml requested review from as code owners Nov 14, 2020
@google-cla google-cla bot added the cla: yes label Nov 14, 2020
@elharo elharo changed the title fix: check Compute Engine environmentfor DirectPath fix: check Compute Engine environment for DirectPath Nov 17, 2020
resultOutStream.write(bs, 0, num);
}
String result = new String(resultOutStream.toByteArray());
return result.contains("01/01/2011") && result.contains("Google");
Copy link
Contributor

@elharo elharo Nov 18, 2020

Choose a reason for hiding this comment

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

why this date?

Copy link
Contributor Author

@mohanli-ml mohanli-ml Nov 18, 2020

Choose a reason for hiding this comment

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

Copy link
Contributor

@elharo elharo Nov 19, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@mohanli-ml mohanli-ml Nov 19, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@elharo elharo Nov 19, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@mohanli-ml mohanli-ml Nov 19, 2020

Choose a reason for hiding this comment

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

"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?

Copy link
Contributor

@elharo elharo Nov 19, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@mohanli-ml mohanli-ml Nov 19, 2020

Choose a reason for hiding this comment

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

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

@codecov
Copy link

@codecov codecov bot commented Nov 19, 2020

Codecov Report

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

Impacted file tree graph

@@             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.

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

@elharo elharo Nov 19, 2020

Choose a reason for hiding this comment

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

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

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);
Copy link
Contributor

@elharo elharo Nov 19, 2020

Choose a reason for hiding this comment

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

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

@mohanli-ml mohanli-ml requested a review from elharo Nov 19, 2020
try {
Process process = Runtime.getRuntime().exec(new String[] {"/bin/sh", "-c", cmd});
process.waitFor();
String result = CharStreams.toString(new InputStreamReader(process.getInputStream()));
Copy link
Contributor

@elharo elharo Nov 19, 2020

Choose a reason for hiding this comment

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

This needs an explicit encoding, probably UTF-8

Copy link
Contributor Author

@mohanli-ml mohanli-ml Nov 19, 2020

Choose a reason for hiding this comment

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

UTF-8 encoding added. Thanks!

elharo
elharo approved these changes Nov 19, 2020
@@ -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() {
Copy link
Contributor

@chingor13 chingor13 Nov 19, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@mohanli-ml mohanli-ml Nov 19, 2020

Choose a reason for hiding this comment

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

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

@chingor13 chingor13 merged commit 656b613 into googleapis:master Nov 19, 2020
6 of 8 checks passed
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});
Copy link

@ejona86 ejona86 Nov 23, 2020

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

@mohanli-ml mohanli-ml Nov 23, 2020

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

@ejona86 ejona86 Nov 23, 2020

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

@mohanli-ml mohanli-ml Nov 23, 2020

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.

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

@elharo elharo Mar 9, 2021

Choose a reason for hiding this comment

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

Please file an issue

Choose a reason for hiding this comment

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

Done #1323

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants