Skip to content

Conversation

@antonyj7
Copy link

@antonyj7 antonyj7 commented Sep 24, 2024

Currently, if the host is not specified, it defaults to localhost. This can lead to issues if the DNS server does not resolve localhost correctly, potentially resulting in errors such as {"code":14,"message":"dns: A record lookup error: lookup localhost: i/o timeout"}.

I believe that if the host is not provided, the system should skip DNS resolution entirely and directly use the host, which would help prevent these unintended behaviors.

RELEASE NOTES:

  • Skip DNS resolution when host not specified

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 24, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@codecov
Copy link

codecov bot commented Sep 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.14%. Comparing base (bcf9171) to head (28b4f5a).
Report is 5 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7665      +/-   ##
==========================================
+ Coverage   81.79%   82.14%   +0.34%     
==========================================
  Files         361      361              
  Lines       27821    27877      +56     
==========================================
+ Hits        22757    22900     +143     
+ Misses       3863     3812      -51     
+ Partials     1201     1165      -36     
Files with missing lines Coverage Δ
internal/resolver/dns/dns_resolver.go 90.86% <100.00%> (ø)

... and 28 files with indirect coverage changes

@antonyj7 antonyj7 marked this pull request as ready for review September 24, 2024 07:20
@antonyj7 antonyj7 changed the title Prevent DNS lookup if host not provided, default to 127.0.0.1 Transport: Prevent DNS lookup if host not provided Sep 24, 2024
@antonyj7 antonyj7 changed the title Transport: Prevent DNS lookup if host not provided Transport:Prevent DNS lookup if host not provided Sep 24, 2024
@antonyj7 antonyj7 changed the title Transport:Prevent DNS lookup if host not provided Transport: Prevent DNS lookup if host not provided Sep 24, 2024
@purnesh42H purnesh42H self-requested a review September 24, 2024 18:48
@purnesh42H
Copy link
Contributor

@antonyj7 thanks for the change. Though i think hardcoding to only 127.0.0.1 could cause problems in IPv6-preferred or IPv6-only environments. To ensure broader compatibility and future-proof the code, it would be better to handle both IPv4 (127.0.0.1) and IPv6 (::1) loopback addresses when skipping DNS resolution and that would require some more custom logic. Using localhost and relying on the system's DNS resolution to handle the appropriate mapping between IPv4 (127.0.0.1) and IPv6 (::1) might be better in most cases.

Are you facing any specific issues with using localhost? If yes, could you provide more details or logs for that?

@purnesh42H purnesh42H added Status: Requires Reporter Clarification Area: Resolvers/Balancers Includes LB policy & NR APIs, resolver/balancer/picker wrappers, LB policy impls and utilities. labels Sep 25, 2024
@antonyj7
Copy link
Author

antonyj7 commented Sep 26, 2024

localhost

Thanks for the review. We have been having issues with some of our VPN servers whereby the DNS does not resolve localhost and spits out {"code":14,"message":"dns: A record lookup error: lookup localhost: i/o timeout"} error.

However, the broader point here was - do we even have to go down the route of resolving DNS in case of localhost ?

grpc.NewClient(endpoint, opts...) fails when we use localhost:$port or :$port (ignoring host) , we have this fixed by passing 127.0.0.1:$port as endpoint

Its not a big deal for us, and we can live with this :-) but if this can be fixed in the library, then we can remove 127.0.0.1 in our code :-)

Meanwhile, I will look at handling both IPv4 and IPv6...

@antonyj7 antonyj7 closed this Sep 26, 2024
@antonyj7 antonyj7 reopened this Sep 26, 2024
@antonyj7 antonyj7 marked this pull request as draft September 26, 2024 06:17
@antonyj7
Copy link
Author

closing this, will raise another PR in case we have a generic solution to cater for IPv6 etc..

@antonyj7 antonyj7 closed this Sep 26, 2024
@purnesh42H
Copy link
Contributor

localhost

Thanks for the review. We have been having issues with some of our VPN servers whereby the DNS does not resolve localhost and spits out {"code":14,"message":"dns: A record lookup error: lookup localhost: i/o timeout"} error.

However, the broader point here was - do we even have to go down the route of resolving DNS in case of localhost ?

grpc.NewClient(endpoint, opts...) fails when we use localhost:$port or :$port (ignoring host) , we have this fixed by passing 127.0.0.1:$port as endpoint

Its not a big deal for us, and we can live with this :-) but if this can be fixed in the library, then we can remove 127.0.0.1 in our code :-)

Meanwhile, I will look at handling both IPv4 and IPv6...

Thanks @antonyj7. There is a similar issue in discussion here #7429

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 27, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Area: Resolvers/Balancers Includes LB policy & NR APIs, resolver/balancer/picker wrappers, LB policy impls and utilities. Status: Requires Reporter Clarification

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants