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

DNS resolving with timeout #6917

Merged
merged 6 commits into from
Mar 5, 2024
Merged

DNS resolving with timeout #6917

merged 6 commits into from
Mar 5, 2024

Conversation

and1truong
Copy link
Contributor

@and1truong and1truong commented Jan 11, 2024

Currently, the DNS resolver does not have a timeout mechanism for DNS resolution. In some edge cases, where the DNS server is slow to respond, this can lead to stale records being used, resulting in failed connections or other issues.

This pull request adds a configurable timeout for DNS resolution, which can be set by the user to a value that suits their needs. The default value is set to 5 seconds, which should be sufficient for most cases. If the DNS server does not respond within the specified time, the resolver will cancel the request and move on to the next available server.

Please review and let me know if you have any questions or concerns.

Thanks!

RELEASE NOTES:

  • resolver/dns: add SetResolvingTimeout to allow configuring the DNS resolver's timeout globally.

Copy link

linux-foundation-easycla bot commented Jan 11, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link

codecov bot commented Jan 11, 2024

Codecov Report

Merging #6917 (5e45d0d) into master (03e76b3) will decrease coverage by 1.32%.
Report is 27 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6917      +/-   ##
==========================================
- Coverage   83.70%   82.38%   -1.32%     
==========================================
  Files         288      296       +8     
  Lines       30921    31468     +547     
==========================================
+ Hits        25883    25926      +43     
- Misses       3976     4475     +499     
- Partials     1062     1067       +5     
Files Coverage Δ
internal/resolver/dns/dns_resolver.go 89.31% <100.00%> (+0.08%) ⬆️
resolver/dns/dns_resolver.go 50.00% <100.00%> (+50.00%) ⬆️

... and 37 files with indirect coverage changes

@zasweq zasweq added this to the 1.62 Release milestone Jan 23, 2024
@dfawley dfawley self-requested a review January 24, 2024 18:14
@dfawley dfawley self-assigned this Jan 24, 2024
@dfawley
Copy link
Member

dfawley commented Jan 25, 2024

Hi @and1truong,

Such a wide-sweeping change would require cross-language agreement for adding this.

Instead, maybe you can add a global in the resolver/dns package to control this (which would be set globally at initialization time)? This is equivalent to what gRPC-Java has for this kind of thing.

Copy link

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

@github-actions github-actions bot added stale and removed stale labels Jan 31, 2024
@and1truong
Copy link
Contributor Author

Thanks for comment. I will start working on this next week.

internal/resolver/dns/dns_resolver.go Outdated Show resolved Hide resolved
internal/resolver/dns/dns_resolver.go Outdated Show resolved Hide resolved
resolver/dns/dns_resolver.go Outdated Show resolved Hide resolved
resolver/dns/dns_resolver.go Outdated Show resolved Hide resolved
resolver/dns/dns_resolver.go Show resolved Hide resolved
Copy link

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

@github-actions github-actions bot added stale and removed stale labels Feb 21, 2024
@arvindbr8
Copy link
Member

@and1truong -- let us know if this is ready for another review

@and1truong
Copy link
Contributor Author

@arvindbr8 I pushed the changes recommended by @dfawley.

@arvindbr8
Copy link
Member

okay, I'll request @dfawley to take another look at this

Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

LGTM except for the last few small things. Thank you for the PR!

internal/resolver/dns/dns_resolver_test.go Outdated Show resolved Hide resolved
internal/resolver/dns/dns_resolver_test.go Outdated Show resolved Hide resolved
internal/resolver/dns/fake_net_resolver_test.go Outdated Show resolved Hide resolved
Copy link
Member

@arvindbr8 arvindbr8 left a comment

Choose a reason for hiding this comment

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

Few comments, mostly formatting.

internal/resolver/dns/dns_resolver.go Show resolved Hide resolved
internal/resolver/dns/dns_resolver_test.go Show resolved Hide resolved
internal/resolver/dns/dns_resolver_test.go Outdated Show resolved Hide resolved
internal/resolver/dns/dns_resolver_test.go Outdated Show resolved Hide resolved
resolver/dns/dns_resolver.go Outdated Show resolved Hide resolved
@arvindbr8 arvindbr8 assigned and1truong and unassigned arvindbr8 Mar 5, 2024
@arvindbr8
Copy link
Member

Thanks for the PR @and1truong. We look forward to seeing your next PR.

@arvindbr8 arvindbr8 merged commit f7c5e6a into grpc:master Mar 5, 2024
14 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants