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 <lior.okman@sap.com> * Make 'gen-check' happy Signed-off-by: Lior Okman <lior.okman@sap.com> --------- Signed-off-by: Lior Okman <lior.okman@sap.com> Signed-off-by: Yael Shechter <yael.shechter@sap.com>
Between envoyproxy#2585 & envoyproxy#2581 Signed-off-by: Arko Dasgupta <arko@tetrate.io> Signed-off-by: Yael Shechter <yael.shechter@sap.com>
* feat: downstream mTLS Relates to envoyproxy#2483 Signed-off-by: Arko Dasgupta <arko@tetrate.io> * configmap provider logic Signed-off-by: Arko Dasgupta <arko@tetrate.io> * gatewayapi translation Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix charts Signed-off-by: Arko Dasgupta <arko@tetrate.io> * tests Signed-off-by: Arko Dasgupta <arko@tetrate.io> * lint Signed-off-by: Arko Dasgupta <arko@tetrate.io> --------- Signed-off-by: Arko Dasgupta <arko@tetrate.io> Signed-off-by: Yael Shechter <yael.shechter@sap.com>
Signed-off-by: Yael Shechter <yael.shechter@sap.com>
Signed-off-by: Yael Shechter <yael.shechter@sap.com>
Signed-off-by: Yael Shechter <yael.shechter@sap.com>
Signed-off-by: Yael Shechter <yael.shechter@sap.com>
Signed-off-by: Yael Shechter <yael.shechter@sap.com>
guydc
left a comment
There was a problem hiding this comment.
Aligned with previous discussion in the community meeting.
LGTM.
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 <yael.shechter@sap.com>
Signed-off-by: Yael Shechter <yael.shechter@sap.com>
Signed-off-by: Yael Shechter <yael.shechter@sap.com>
|
/retest |
Signed-off-by: Yael Shechter <yael.shechter@sap.com>
|
/retest |
|
/retest |
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