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.

Sign up for GitHub

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

Jump to bottom

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 •
edited by dfawley
Loading

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.

@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Jan 11, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@codecov Codecov
Copy link

codecov bot commented Jan 11, 2024 •
edited
Loading

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.

@dfawley dfawley assigned and1truong and unassigned dfawley Jan 25, 2024
@github-actions GitHub Actions
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.

@ginayeh ginayeh assigned dfawley and unassigned and1truong Feb 8, 2024
@ginayeh ginayeh modified the milestones: 1.62 Release, 1.63 Release Feb 8, 2024
dfawley
dfawley reviewed Feb 15, 2024
View reviewed changes
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
@dfawley dfawley assigned and1truong and unassigned dfawley Feb 15, 2024
@dfawley dfawley mentioned this pull request Feb 15, 2024
Merged
@github-actions GitHub Actions
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

@arvindbr8 arvindbr8 requested a review from dfawley February 29, 2024 19:29
@arvindbr8 arvindbr8 assigned dfawley and unassigned and1truong Feb 29, 2024
dfawley
dfawley approved these changes Mar 1, 2024
View reviewed changes
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
@dfawley dfawley requested a review from arvindbr8 March 1, 2024 00:18
@dfawley dfawley assigned and1truong and unassigned dfawley Mar 1, 2024
@dfawley dfawley assigned arvindbr8 and unassigned and1truong Mar 1, 2024
arvindbr8
arvindbr8 requested changes Mar 5, 2024
View reviewed changes
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
@and1truong and1truong requested a review from arvindbr8 March 5, 2024 23:20
@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.
Reviewers

@dfawley dfawley dfawley approved these changes

@arvindbr8 arvindbr8 arvindbr8 approved these changes

Assignees

@and1truong and1truong

Labels
Type: Bug
Projects
None yet
Milestone
1.63 Release
Development

Successfully merging this pull request may close these issues.
5 participants
@and1truong @dfawley @arvindbr8 @ginayeh @zasweq