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

dfp: Add a feature to disable DNS query timeout #38067

Merged
merged 30 commits into from
Jan 22, 2025
Merged

dfp: Add a feature to disable DNS query timeout #38067

merged 30 commits into from
Jan 22, 2025

Conversation

fredyw
Copy link
Member

@fredyw fredyw commented Jan 16, 2025 •
edited
Loading

This PR adds a feature to disable the Envoy DNS query timeout and use the underlying DNS implementation timeout by setting DnsCacheConfig.dns_query_timeout to 0.

Risk Level: low
Testing: unit and integration test
Docs Changes: inline
Release Notes: inline
Platform Specific Features: dfp

fredyw added 2 commits January 16, 2025 19:00
@fredyw
Add a knob to disable DFP resolve timeout
7383b4a 
Signed-off-by: Fredy Wijaya <[email protected]>
@fredyw
Fix typo
63fb430 
Signed-off-by: Fredy Wijaya <[email protected]>
@repokitteh-read-only RepoKitteh - Read Only
Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #38067 was opened by fredyw.

see: more, trace.

@fredyw
Fix more typo
ca9620f 
Signed-off-by: Fredy Wijaya <[email protected]>
@fredyw
Copy link
Member Author

fredyw commented Jan 16, 2025

/retest

fredyw added 8 commits January 16, 2025 21:54
@fredyw
Merge branch 'main' into disable_resolve_timeout
dab7eb6 
Signed-off-by: Fredy Wijaya <[email protected]>
@fredyw
Debugging
ba3b940 
Signed-off-by: Fredy Wijaya <[email protected]>
@fredyw
Limit num retries
bfed955 
Signed-off-by: Fredy Wijaya <[email protected]>
@fredyw
Debug
a813e98 
Signed-off-by: Fredy Wijaya <[email protected]>
@fredyw
More debugging
6cdd83d 
Signed-off-by: Fredy Wijaya <[email protected]>
@fredyw
Debug
3213d1a 
Signed-off-by: Fredy Wijaya <[email protected]>
@fredyw
Test
5054584 
Signed-off-by: Fredy Wijaya <[email protected]>
@fredyw
Update TestResolver to respect number of retries
b6f820c 
Signed-off-by: Fredy Wijaya <[email protected]>
@fredyw fredyw marked this pull request as ready for review January 17, 2025 01:20
@fredyw
Copy link
Member Author

fredyw commented Jan 17, 2025

/assign @alyssawilk

@fredyw
Resolve comments
e7631ff 
Signed-off-by: Fredy Wijaya <[email protected]>
@repokitteh-read-only RepoKitteh - Read Only
Copy link

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #38067 was synchronize by fredyw.

see: more, trace.

@fredyw fredyw changed the title dfp: Add a knob to disable resolve timeout dfp: Add a knob to disable DNS resolve timeout Jan 21, 2025
@fredyw
Remove EM changes
209ebd3 
Signed-off-by: Fredy Wijaya <[email protected]>
@fredyw fredyw changed the title dfp: Add a knob to disable DNS resolve timeout dfp: Add a feature to disable DNS resolve timeout Jan 21, 2025
@fredyw fredyw changed the title dfp: Add a feature to disable DNS resolve timeout dfp: Add a feature to disable DNS query timeout Jan 21, 2025
fredyw added 6 commits January 21, 2025 18:11
@fredyw
Address comments
91a5ec5 
Signed-off-by: Fredy Wijaya <[email protected]>
@fredyw
Update changelogs
52369f8 
Signed-off-by: Fredy Wijaya <[email protected]>
@fredyw
Remove runtime guard
41506e1 
Signed-off-by: Fredy Wijaya <[email protected]>
@fredyw
Update EM
3c90b12 
Signed-off-by: Fredy Wijaya <[email protected]>
@fredyw
Let's not update the default value first
08a3ef5 
Signed-off-by: Fredy Wijaya <[email protected]>
@fredyw
Set a default value
2f4316b 
Signed-off-by: Fredy Wijaya <[email protected]>
fredyw added 2 commits January 21, 2025 21:18
@fredyw
Use reloadable flag
53ac807 
Signed-off-by: Fredy Wijaya <[email protected]>
@fredyw
Don't test per request
79d494d 
Signed-off-by: Fredy Wijaya <[email protected]>
fredyw added 3 commits January 21, 2025 21:52
@fredyw
Remove flag
79cda3a 
Signed-off-by: Fredy Wijaya <[email protected]>
@fredyw
Remove runtime guard
21c51d0 
Signed-off-by: Fredy Wijaya <[email protected]>
@fredyw
Update proto comment
8b5c5ba 
Signed-off-by: Fredy Wijaya <[email protected]>
adisuissa
adisuissa reviewed Jan 22, 2025
View reviewed changes
Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

/lgtm api

changelogs/current.yaml Outdated Show resolved Hide resolved
alyssawilk
alyssawilk previously approved these changes Jan 22, 2025
View reviewed changes
@fredyw
Update changelogs
b0a9698 
Signed-off-by: Fredy Wijaya <[email protected]>
@fredyw
Typo
07503c4 
Signed-off-by: Fredy Wijaya <[email protected]>
@fredyw
Copy link
Member Author

fredyw commented Jan 22, 2025

/retest

@alyssawilk alyssawilk enabled auto-merge (squash) January 22, 2025 17:28
@alyssawilk alyssawilk merged commit e3246a5 into envoyproxy:main Jan 22, 2025
25 checks passed
@fredyw fredyw deleted the disable_resolve_timeout branch January 22, 2025 17:36
bazmurphy pushed a commit to bazmurphy/envoy that referenced this pull request Jan 29, 2025
@fredyw @bazmurphy
dfp: Add a feature to disable DNS query timeout (envoyproxy#38067)
b577d36 
This PR adds a feature to disable the Envoy DNS query timeout and use
the underlying DNS implementation timeout by setting
`DnsCacheConfig.dns_query_timeout` to 0.

Risk Level: low
Testing: unit and integration test
Docs Changes: inline
Release Notes: inline
Platform Specific Features: dfp

---------

Signed-off-by: Fredy Wijaya <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adisuissa adisuissa adisuissa left review comments

@alyssawilk alyssawilk alyssawilk approved these changes

@RyanTheOptimist RyanTheOptimist Awaiting requested review from RyanTheOptimist

@abeyad abeyad Awaiting requested review from abeyad

@mattklein123 mattklein123 Awaiting requested review from mattklein123 mattklein123 is a code owner

Assignees

@adisuissa adisuissa

@alyssawilk alyssawilk

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fredyw @adisuissa @alyssawilk