fix: Configure idle timeout when timeout is set on HTTPRoute#2708
fix: Configure idle timeout when timeout is set on HTTPRoute#2708arkodg merged 3 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: David Alger <davidmalger@gmail.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2708 +/- ##
==========================================
+ Coverage 63.00% 63.03% +0.02%
==========================================
Files 122 122
Lines 19770 19786 +16
==========================================
+ Hits 12457 12473 +16
Misses 6506 6506
Partials 807 807 ☔ View full report in Codecov by Sentry. |
| } | ||
| } | ||
|
|
||
| func idleTimeout(httpRoute *ir.HTTPRoute) *durationpb.Duration { |
There was a problem hiding this comment.
I'd vote for below logic until we expose this as an API field
idleTimeout = 1hr
if idleTimout < requestTimeout {
idleTimout = requestTimeout
}
There was a problem hiding this comment.
I'm ok with this, with the caveat that it should still be set to zero so the request timeout will function per GEP-1742:
A zero-valued timeout ("0s") MUST be interpreted as disabling the timeout
ref: https://gateway-api.sigs.k8s.io/geps/gep-1742/#timeout-values
This should work per envoy docs:
value of 0 will completely disable the route’s idle timeout, even if a connection manager stream idle timeout is configured
Signed-off-by: David Alger <davidmalger@gmail.com>
| name: first-route | ||
| route: | ||
| cluster: first-route-dest | ||
| idleTimeout: 3600s |
There was a problem hiding this comment.
nit: would be ideal if this wasn't set
There was a problem hiding this comment.
Two choices:
a) 1hr default set on the route only when the request timeout is passed on the route (as currently implemented)
b) Set stream_idle_timeout in the connection manager to 1hr (vs implicit default of 300s), only configuring per-route idle timeout if >1hr or 0
thoughts?
|
/retest |
|
/retest |
What type of PR is this?
Fixes bug where default
stream_idle_timeoutin connection manager interferes with request timeouts set on HTTPRoute when set greater than 5 minutes preventing high request timeouts from being used without patching connection manager settings.What this PR does:
If an HTTP request
timeoutis configured on the route and is non-zero, theidle_timeoutwill be set to the greater of 1hr or the configured request timeout. If thetimeouton the HTTPRoute is set to0theidle_timeoutis also set to0disabling both timeouts on the route.How to reproduce:
curlnoting the 504 response code at the 5 minute mark