api: Support Timeouts in ClientTrafficPolicy #2605

yaelSchechter · 2024-02-14T10:53:54Z

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_timeout and idle_timeout).

Naming rationale: requestReceivedTimeout is the envoy's request_timeout. This name indicates that the timeout includes the time it took for the upstream to receive the request.

Example:

spec:
  timeout:
    http:
      requestReceivedTimeout: 300s

Which issue(s) this PR fixes:
Fixes #2598

…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>

guydc

Aligned with previous discussion in the community meeting.
LGTM.

liorokman

LGTM

davidalger

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.

liorokman · 2024-02-14T15:46:43Z

@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.

davidalger · 2024-02-14T15:55:03Z

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

How did I miss this? 😮 Nvm me then 🙈

arkodg · 2024-02-14T22:06:59Z

thanks for pointing out the ideal workflow @liorokman (GH issue -> API PR -> Implementation PR) !
@davidalger thanks for proactively helping out other contributors !

codecov · 2024-02-14T22:10:33Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a5125bf) 63.41% compared to head (3fca4e7) 63.41%.

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.
📢 Have feedback on the report? Share it here.

arkodg · 2024-02-14T22:11:14Z

api/v1alpha1/timeout_types.go

+	// Default: 300 seconds.
+	//
+	// +optional
+	RequestProcessTimeout *gwapiv1.Duration `json:"requestProcessTimeout,omitempty"`


requestProceess doesnt convey the exact meaning of the knob imo
thoughts on processingRequestTimeout or even requestTimeout ?

+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

+1 for requestRecievedTimeout

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).

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>

…into main

Signed-off-by: Yael Shechter <yael.shechter@sap.com>

yaelSchechter · 2024-02-15T16:46:21Z

/retest

arkodg

LGTM thanks !

Signed-off-by: Yael Shechter <yael.shechter@sap.com>

yaelSchechter · 2024-02-16T08:19:52Z

/retest

yaelSchechter · 2024-02-18T10:06:56Z

/retest

liorokman and others added 7 commits February 14, 2024 03:26

bug: fix merge race (envoyproxy#2604)

f2309e0

Between envoyproxy#2585 & envoyproxy#2581 Signed-off-by: Arko Dasgupta <arko@tetrate.io> Signed-off-by: Yael Shechter <yael.shechter@sap.com>

add timeout to clientTrafficPolicy

3a2c79a

Signed-off-by: Yael Shechter <yael.shechter@sap.com>

fix comment

1ea8b40

Signed-off-by: Yael Shechter <yael.shechter@sap.com>

add omitempty

de0dd3c

Signed-off-by: Yael Shechter <yael.shechter@sap.com>

add cel test for coverage

371639f

Signed-off-by: Yael Shechter <yael.shechter@sap.com>

yaelSchechter requested a review from a team as a code owner February 14, 2024 10:53

yaelSchechter force-pushed the main branch from f23f8eb to de0dd3c Compare February 14, 2024 10:57

yaelSchechter and others added 2 commits February 14, 2024 12:57

Merge branch 'main' into main

5c700c0

run make commands

01e450f

Signed-off-by: Yael Shechter <yael.shechter@sap.com>

guydc approved these changes Feb 14, 2024

View reviewed changes

liorokman approved these changes Feb 14, 2024

View reviewed changes

davidalger suggested changes Feb 14, 2024

View reviewed changes

arkodg reviewed Feb 14, 2024

View reviewed changes

yaelSchechter and others added 5 commits February 15, 2024 09:33

change request timeout field name and desc

2177485

Signed-off-by: Yael Shechter <yael.shechter@sap.com>

tidy up comment

7f49ad7

Signed-off-by: Yael Shechter <yael.shechter@sap.com>

Merge branch 'main' into main

35d3b88

fix cel test

850076c

Signed-off-by: Yael Shechter <yael.shechter@sap.com>

Merge branch 'main' of https://github.com/yaelSchechter/envoy-gateway …

54a3c5e

…into main

guydc approved these changes Feb 15, 2024

View reviewed changes

fix typo

630b3af

Signed-off-by: Yael Shechter <yael.shechter@sap.com>

arkodg previously approved these changes Feb 15, 2024

View reviewed changes

run generate

298472a

Signed-off-by: Yael Shechter <yael.shechter@sap.com>

arkodg requested a review from a team February 15, 2024 23:43

yaelSchechter dismissed arkodg’s stale review via 298472a February 16, 2024 08:19

Merge branch 'main' into main

805c24c

arkodg approved these changes Feb 16, 2024

View reviewed changes

Merge branch 'main' into main

3fca4e7

zirain approved these changes Feb 18, 2024

View reviewed changes

zirain merged commit 199f50c into envoyproxy:main Feb 18, 2024
20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api: Support Timeouts in ClientTrafficPolicy #2605

api: Support Timeouts in ClientTrafficPolicy #2605

yaelSchechter commented Feb 14, 2024 •

edited

Loading

guydc left a comment

liorokman left a comment

davidalger left a comment

liorokman commented Feb 14, 2024

davidalger commented Feb 14, 2024

arkodg commented Feb 14, 2024

codecov bot commented Feb 14, 2024 •

edited

Loading

arkodg Feb 14, 2024

davidalger Feb 14, 2024

arkodg Feb 14, 2024

guydc Feb 14, 2024

yaelSchechter Feb 15, 2024 •

edited

Loading

yaelSchechter commented Feb 15, 2024

arkodg left a comment

yaelSchechter commented Feb 16, 2024

yaelSchechter commented Feb 18, 2024

api: Support Timeouts in ClientTrafficPolicy #2605

api: Support Timeouts in ClientTrafficPolicy #2605

Conversation

yaelSchechter commented Feb 14, 2024 • edited Loading

guydc left a comment

Choose a reason for hiding this comment

liorokman left a comment

Choose a reason for hiding this comment

davidalger left a comment

Choose a reason for hiding this comment

liorokman commented Feb 14, 2024

davidalger commented Feb 14, 2024

arkodg commented Feb 14, 2024

codecov bot commented Feb 14, 2024 • edited Loading

Codecov Report

arkodg Feb 14, 2024

Choose a reason for hiding this comment

davidalger Feb 14, 2024

Choose a reason for hiding this comment

arkodg Feb 14, 2024

Choose a reason for hiding this comment

guydc Feb 14, 2024

Choose a reason for hiding this comment

yaelSchechter Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

yaelSchechter commented Feb 15, 2024

arkodg left a comment

Choose a reason for hiding this comment

yaelSchechter commented Feb 16, 2024

yaelSchechter commented Feb 18, 2024

yaelSchechter commented Feb 14, 2024 •

edited

Loading

codecov bot commented Feb 14, 2024 •

edited

Loading

yaelSchechter Feb 15, 2024 •

edited

Loading