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

Drop trailing dot from HOST_ADDRESS for Istio compatibility #210

Closed
wants to merge 1 commit into from
Closed

Drop trailing dot from HOST_ADDRESS for Istio compatibility #210

wants to merge 1 commit into from

Conversation

kalyanac
Copy link

@kalyanac kalyanac commented Jun 6, 2019

Istio on GKE currently does not support unambiguous FQDNs, (FQDN with a trailing dot). 14405 adds this support, possibly in 1.2.x, but it is not yet available on GKE. GKE is still using 1.1 in alpha.

With the trailing dot in GCP metadata server name, looks ups for GCP metadata will fail with 404. Node.js profiling agent will not work.

log messages:

@google-cloud/profiler Failed to create profile, waiting 31.4s to try again: Error: Unexpected error determining execution environment: Unsuccessful response status code. Request failed with status code 404
@google-cloud/profiler Failed to create profile, waiting 2m 35.3s to try again: Error: Unexpected error determining execution environment: Unsuccessful response status code. Request failed with status code 404
The benchmark is running.
The benchmark is running.
The benchmark is running.
@google-cloud/profiler Failed to create profile, waiting 4m 32.9s to try again: Error: Unexpected error determining execution environment: Unsuccessful response status code. Request failed with status code 404
The benchmark is running.

Istio proxy messages:

textPayload:  "[2019-06-06T22:07:25.773Z] "GET /computeMetadata/v1/instanceHTTP/1.1" 404 NR 0 0 0 - "-" "node-fetch/1.0 (+https://github.com/bitinn/node-fetch)" "5c8d5908-d809-445e-bd46-ad3e9cc2e9f2" "metadata.google.internal." "-" - - 169.254.169.254:80 10.28.4.11:58610"  

This patch can be rolled back when GKE starts using Istio version that supports unambiguous FQDNs

Fixes #<issue_number_goes_here> (it's a good idea to open an issue first for discussion)

  • Tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Istio on GKE currently does not support unambiguous FQDNs, (FQDN with a trailing dot). [14405](istio/istio#14405) adds this support, possibly in 1.2.x, but it is not yet available on GKE. GKE is still using 1.1 in alpha. 

With the trailing dot in GCP metadata server name, looks ups for GCP metadata will fail with 404. Node.js profiling agent will not work. 

log messages:
```
@google-cloud/profiler Failed to create profile, waiting 31.4s to try again: Error: Unexpected error determining execution environment: Unsuccessful response status code. Request failed with status code 404
@google-cloud/profiler Failed to create profile, waiting 2m 35.3s to try again: Error: Unexpected error determining execution environment: Unsuccessful response status code. Request failed with status code 404
The benchmark is running.
The benchmark is running.
The benchmark is running.
@google-cloud/profiler Failed to create profile, waiting 4m 32.9s to try again: Error: Unexpected error determining execution environment: Unsuccessful response status code. Request failed with status code 404
The benchmark is running.
```

Istio proxy messages:
```
textPayload:  "[2019-06-06T22:07:25.773Z] "GET /computeMetadata/v1/instanceHTTP/1.1" 404 NR 0 0 0 - "-" "node-fetch/1.0 (+https://github.com/bitinn/node-fetch)" "5c8d5908-d809-445e-bd46-ad3e9cc2e9f2" "metadata.google.internal." "-" - - 169.254.169.254:80 10.28.4.11:58610"  
```

This patch can be rolled back when GKE starts using Istio version that supports unambiguous FQDNs
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 6, 2019
@codecov
Copy link

codecov bot commented Jun 6, 2019

Codecov Report

Merging #210 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #210   +/-   ##
=======================================
  Coverage   95.65%   95.65%           
=======================================
  Files           1        1           
  Lines          46       46           
  Branches       10       10           
=======================================
  Hits           44       44           
  Misses          1        1           
  Partials        1        1
Impacted Files Coverage Δ
src/index.ts 95.65% <100%> (ø) ⬆️

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 3c22806...a23088d. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Jun 6, 2019

Codecov Report

Merging #210 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #210   +/-   ##
=======================================
  Coverage   95.65%   95.65%           
=======================================
  Files           1        1           
  Lines          46       46           
  Branches       10       10           
=======================================
  Hits           44       44           
  Misses          1        1           
  Partials        1        1
Impacted Files Coverage Δ
src/index.ts 95.65% <100%> (ø) ⬆️

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 3c22806...a23088d. Read the comment docs.

@theacodes
Copy link

Hi @kalyanac. Unfortunately, this isn't that simple. See #117 for more details, but the short story is that not having the FQDN leads to an excessive amount of DNS queries.

We're discussing the option of using the IP address directly and never using the DNS name for the metadata service which could reduce latency and improve reliability here. If you'd like to get involved with that you can reach out to @JustinBeckwith.

@theacodes theacodes closed this Jun 6, 2019
@kalyanac
Copy link
Author

kalyanac commented Jun 6, 2019

understood, can we change it to use the well known IP to prevent look ups? This is a blocker for Node.js profiling agent.

@theacodes
Copy link

We want to but unfortunately we need to confirm with GCE, GKE, GAE, and GCF teams first. @JustinBeckwith is working on this.

@kalyanac
Copy link
Author

kalyanac commented Jun 6, 2019

Thanks for the info. Is there a tracking bug (internal or external) for that?

@kalyanac kalyanac deleted the patch-1 branch June 6, 2019 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants