api: Support Timeouts in ClientTrafficPolicy#2605
Conversation
…r' header by default (envoyproxy#2585) * Implement and update tests for the default header transformations. Signed-off-by: Lior Okman <[email protected]> * Make 'gen-check' happy Signed-off-by: Lior Okman <[email protected]> --------- Signed-off-by: Lior Okman <[email protected]> Signed-off-by: Yael Shechter <[email protected]>
Between envoyproxy#2585 & envoyproxy#2581 Signed-off-by: Arko Dasgupta <[email protected]> Signed-off-by: Yael Shechter <[email protected]>
* feat: downstream mTLS Relates to envoyproxy#2483 Signed-off-by: Arko Dasgupta <[email protected]> * configmap provider logic Signed-off-by: Arko Dasgupta <[email protected]> * gatewayapi translation Signed-off-by: Arko Dasgupta <[email protected]> * fix charts Signed-off-by: Arko Dasgupta <[email protected]> * tests Signed-off-by: Arko Dasgupta <[email protected]> * lint Signed-off-by: Arko Dasgupta <[email protected]> --------- Signed-off-by: Arko Dasgupta <[email protected]> Signed-off-by: Yael Shechter <[email protected]>
Signed-off-by: Yael Shechter <[email protected]>
Signed-off-by: Yael Shechter <[email protected]>
Signed-off-by: Yael Shechter <[email protected]>
Signed-off-by: Yael Shechter <[email protected]>
Signed-off-by: Yael Shechter <[email protected]>
guydc
left a comment
guydc
left a comment
There was a problem hiding this comment.
Aligned with previous discussion in the community meeting.
LGTM.
davidalger
left a comment
davidalger
left a comment
There was a problem hiding this comment.
Perhaps this wasn't meant to be a finished work when pushed, but the added fields here aren't used anywhere and don't do anything. They need translation into XDS that's added to the listener as well as test coverage.
If an example is needed, I recently made a similar addition to the ClientTrafficPolicy type that might help point you to the right places: https://github.com/envoyproxy/gateway/pull/2535/files
Pay specific attention to internal/gatewayapi/clienttrafficpolicy.go and internal/ir/xds.go for translation as well as internal/xds/translator/listener.go where it's injected into HttpConnectionManager settings.
|
@davidalger features that change APIs are usually split into two PRs, the first one to agree upon the API itself and the second one to implement the feature. For example, #2534 and #2577 , or #2487 and #2492. |
How did I miss this? 😮 Nvm me then 🙈 |
|
thanks for pointing out the ideal workflow @liorokman (GH issue -> API PR -> Implementation PR) ! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2605 +/- ##
=======================================
Coverage 63.41% 63.41%
=======================================
Files 119 119
Lines 19227 19227
=======================================
Hits 12193 12193
Misses 6222 6222
Partials 812 812 ☔ View full report in Codecov by Sentry. |
api/v1alpha1/timeout_types.go
Outdated
| // Default: 300 seconds. | ||
| // | ||
| // +optional | ||
| RequestProcessTimeout *gwapiv1.Duration `json:"requestProcessTimeout,omitempty"` |
There was a problem hiding this comment.
requestProceess doesnt convey the exact meaning of the knob imo
thoughts on processingRequestTimeout or even requestTimeout ?
There was a problem hiding this comment.
+1 for requestTimeout with a descriptive comment indicating something closer to what the docs describe:
The amount of time that Envoy will wait for the entire request to be received. The timer is activated when the request is initiated, and is disarmed when the last byte of the request is sent upstream OR when the response is initiated.
Alternately something like requestRecievedTimeout seems less ambiguous than any variation of process timeout and conveys the intent while differentiating this from the (upstream) request timeout which can be specified via timeouts.request on the HTTPRoute
There was a problem hiding this comment.
+1 for requestRecievedTimeout
There was a problem hiding this comment.
requestTimeout is slightly overloaded as gateway-api already defines a structure like timeout.request for httproutes.
I'm fine with requestRecievedTimeout or just requestTimeout if we're not concerned that this would be confusing (due to the other GW API field).
There was a problem hiding this comment.
Thanks for the feedback, I changed the field name to requestReceivedTimeout and added a more descriptive comment as @davidalger suggested.
Signed-off-by: Yael Shechter <[email protected]>
Signed-off-by: Yael Shechter <[email protected]>
Signed-off-by: Yael Shechter <[email protected]>
Signed-off-by: Yael Shechter <[email protected]>
|
/retest |
Signed-off-by: Yael Shechter <[email protected]>
|
/retest |
|
/retest |
6 participants
What this PR does / why we need it:
This PR will allow users to configure timeouts for client connections in the
ClientTrafficPolicy.This is implemented as a struct to allow future addition of related fields (such as
request_headers_timeoutandidle_timeout).Naming rationale:
requestReceivedTimeoutis the envoy'srequest_timeout. This name indicates that the timeout includes the time it took for the upstream to receive the request.Example:
Which issue(s) this PR fixes:
Fixes #2598