-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Add support for configuring Envoys route idle_timeout #14340
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed the changelong .txt, service-router.mdx, and envoy.mdx for docs changes.
I have one minor fix, otherwise LGTM. Approving on behalf of consul-docs.
Co-authored-by: Jeff Boruszak <[email protected]>
This pull request has been automatically flagged for inactivity because it has not been acted upon in the last 60 days. It will be closed if no new activity occurs in the next 30 days. Please feel free to re-open to resurrect the change if you feel this has happened by mistake. Thank you for your contributions. |
@blake can you remove the stale label so this doesn't get closed? 🙏 Thanks!! |
Done. I made this PR staleproof for now! |
👋 Hi. I wanted to see now that 1.14 is out if this PR could be considered? Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this @oulman. I have a few comments around doc changes and I believe this is missing a test around the local_request_timeout_ms
at the xds generation level.
Co-authored-by: Dhia Ayachi <[email protected]>
Co-authored-by: Dhia Ayachi <[email protected]>
Thanks, @dhiaayachi! Is that tests in addition to the changes here? |
The missing test would be on the rendering side, could be an addition to this test |
Ah! Thank you, @dhiaayach!i I have added the |
Hi @dhiaayachi I just checked with @oulman and he has confirmed that he has addressed your comments. Are additional changes needed here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! it would be good to have @hashicorp/consul-docs review the documentation bits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 14340.txt, service-router.mdx, and envoy.mdx again at David Yu's request.
Everything LGTM!
* Add idleTimeout Co-authored-by: Jeff Boruszak <[email protected]> Co-authored-by: Dhia Ayachi <[email protected]>
* Add idleTimeout Co-authored-by: James Oulman <[email protected]> Co-authored-by: Jeff Boruszak <[email protected]> Co-authored-by: Dhia Ayachi <[email protected]>
* Add idleTimeout Co-authored-by: Jeff Boruszak <[email protected]> Co-authored-by: Dhia Ayachi <[email protected]>
Description
This PR adds support for configuring the Envoy route level idle_timeout for service-routers and local_app. For our teams scenario, this will let us extend the timeout for long-running gRPC services so that Envoy doesn't terminate the connection prematurely.
For service-routers a
IdleTimeout
parameter has been added, and for local_app a proxy configlocal_idle_timeout_ms
parameter was created. I followed the RequestTimeout and local_request_timeout_ms definitions closely.Testing & Reproduction steps
Manual Testing Steps
service-router IdleTimeout
Created a service-router and configured the
IdleTimeout
to 60sDumped the Envoy config and verified that the
idle_timeout
configuration parameter was setlocal_idle_timeout_ms
Configure
local_idle_timeout_ms
on a service proxy.configDumped the Envoy config and verified that the
idle_timeout
configuration parameter was set on the public_listener route forlocal_app
Links
Include any links here that might be helpful for people reviewing your PR (Tickets, GH issues, API docs, external benchmarks, tools docs, etc). If there are none, feel free to delete this section.
Please be mindful not to leak any customer or confidential information. HashiCorp employees may want to use our internal URL shortener to obfuscate links.
PR Checklist