feat(translator): Implement BTP Timeout API #2454

guydc · 2024-01-16T23:53:21Z

What this PR does / why we need it:
Implement the BackendTrafficPolicy Timeout API

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

codecov · 2024-01-16T23:57:08Z

Codecov Report

Attention: 84 lines in your changes are missing coverage. Please review.

Comparison is base (8d19d77) 64.70% compared to head (ed5ca57) 64.61%.
Report is 1 commits behind head on main.

Files	Patch %	Lines
internal/ir/zz_generated.deepcopy.go	0.00%	61 Missing and 1 partial ⚠️
internal/gatewayapi/backendtrafficpolicy.go	86.41%	7 Missing and 4 partials ⚠️
internal/gatewayapi/route.go	57.69%	9 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2454      +/-   ##
==========================================
- Coverage   64.70%   64.61%   -0.09%     
==========================================
  Files         116      116              
  Lines       17613    17797     +184     
==========================================
+ Hits        11396    11500     +104     
- Misses       5486     5560      +74     
- Partials      731      737       +6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

arkodg · 2024-01-17T01:15:27Z

internal/gatewayapi/backendtrafficpolicy.go

+
+}
+
+func SetBackendTrafficPolicyTranslationErrorCondition(policy *egv1a1.BackendTrafficPolicy, field, errMsg string) {


can we use lower case here to make the func private - setBackend....

arkodg · 2024-01-17T01:16:51Z

internal/ir/xds.go

@@ -391,6 +391,8 @@ type HTTPRoute struct {
 	ExtensionRefs []*UnstructuredRef `json:"extensionRefs,omitempty" yaml:"extensionRefs,omitempty"`
 	// Circuit Breaker Settings
 	CircuitBreaker *CircuitBreaker `json:"circuitBreaker,omitempty" yaml:"circuitBreaker,omitempty"`
+	// Connection timeout Settings
+	BackendTimeout *Timeout `json:"backendTimeout,omitempty" yaml:"backendTimeout,omitempty"`


prefer if this was renamed to Timeout and we moved the existing Timeout field into it and renamed it to RequestTimeout under HTTP

shawnh2 · 2024-01-17T04:37:13Z

internal/xds/translator/cluster.go

+	if hasCommonHTTPOptions {
+		protocolOptions.CommonHttpProtocolOptions = &corev3.HttpProtocolOptions{
+			IdleTimeout:           durationpb.New(args.timeout.HTTP.ConnectionIdleTimeout.Duration),
+			MaxConnectionDuration: durationpb.New(args.timeout.HTTP.MaxConnectionDuration.Duration),


these two duration should be checked (whether is nil ptr) before assign values toIdleTimeout and MaxConnectionDuration, nor it will raise a nil pointer panic.

http: - name: "first-listener" #... routes: - name: "first-route" backendTimeout: tcp: connectTimeout: "31s" http: connectionIdleTimeout: "32s" #maxConnectionDuration: "33s" # paniced #...

shawnh2

lgtm, thanks

zhaohuabing

This implementation LGTM, thanks!

A minor comment on the name of Timeout structure in the API: we may want to change it to BackendTimeout because the TCP ConnectTimeout can't be reused for the future client-side Timeouts.

// BackendTimeout defines configuration for timeouts related to the backend.
type BackendTimeout struct {
	// Timeout settings for TCP.
	//
	// +optional
	TCP *TCPTimeout `json:"tcp,omitempty"`

	// Timeout settings for HTTP.
	//
	// +optional
	HTTP *HTTPTimeout `json:"http,omitempty"`
}

guydc · 2024-01-23T12:22:54Z

A minor comment on the name of Timeout structure in the API: we may want to change it to BackendTimeout because the TCP ConnectTimeout can't be reused for the future client-side Timeouts.

Can we translate this to transport socket timeout for downstream connections? If so, it can be reused...

arkodg · 2024-01-23T18:20:15Z

internal/gatewayapi/route.go

@@ -227,12 +227,20 @@ func (t *Translator) processHTTPRouteRules(httpRoute *HTTPRouteContext, parentRe

 func processTimeout(irRoute *ir.HTTPRoute, rule gwapiv1.HTTPRouteRule) {
 	if rule.Timeouts != nil {
+		var rto *ir.Timeout
+
+		if irRoute.Timeout != nil {


nit: policy is computed/translated post route translation, so we can assume its empty here

I wanted to this to be safe in case there's a re-arrangement of the computation order. If you feel that we can make this assumption, i'll update the implementation accordingly.

sure, if thats the case, can you also add a comment here, so the next dev can understand why this done this way similar to the comment you have in policy around request timeout

arkodg · 2024-01-23T19:19:09Z

internal/gatewayapi/backendtrafficpolicy.go

+		d, err := time.ParseDuration(string(*pto.TCP.ConnectTimeout))
+		if err != nil {
+			setBackendTrafficPolicyTranslationErrorCondition(policy, "TCP Timeout", fmt.Sprintf("invalid ConnectTimeout value %s", *pto.TCP.ConnectTimeout))
+			return nil


we should log error in status and continue processing instead of returning early here

arkodg · 2024-01-23T19:19:15Z

internal/gatewayapi/backendtrafficpolicy.go

+			d, err := time.ParseDuration(string(*pto.HTTP.ConnectionIdleTimeout))
+			if err != nil {
+				setBackendTrafficPolicyTranslationErrorCondition(policy, "HTTP Timeout", fmt.Sprintf("invalid ConnectionIdleTimeout value %s", *pto.HTTP.ConnectionIdleTimeout))
+				return nil


same as above

arkodg · 2024-01-23T19:19:22Z

internal/gatewayapi/backendtrafficpolicy.go

+			d, err := time.ParseDuration(string(*pto.HTTP.MaxConnectionDuration))
+			if err != nil {
+				setBackendTrafficPolicyTranslationErrorCondition(policy, "HTTP Timeout", fmt.Sprintf("invalid MaxConnectionDuration value %s", *pto.HTTP.MaxConnectionDuration))
+				return nil


same as above

arkodg · 2024-01-23T19:26:36Z

internal/xds/translator/testdata/out/xds-ir/http-route-timeout.routes.yaml

@@ -14,4 +14,3 @@
      name: first-route
      route:
        cluster: first-route-dest


what happened here, why did 5s get deleted ?

arkodg · 2024-01-23T19:28:09Z

internal/xds/translator/testdata/in/xds-ir/http-route-timeout.yaml

@@ -10,7 +10,8 @@ http:
  routes:
  - name: "first-route"
    hostname: "*"
-    timeout: 5s
+    timeout:
+      requestTimeout: 5s


test is broken, probably needs below config

timeout: http: requestTimeout: 5s

zhaohuabing · 2024-01-24T02:15:37Z

A minor comment on the name of Timeout structure in the API: we may want to change it to BackendTimeout because the TCP ConnectTimeout can't be reused for the future client-side Timeouts.

Can we translate this to transport socket timeout for downstream connections? If so, it can be reused...

Not sure about it, they look similar but not exactly the same. Are there any other possible different settings between client and backend timeouts?

@arkodg @zirain WDYT?

arkodg · 2024-01-26T02:29:34Z

A minor comment on the name of Timeout structure in the API: we may want to change it to BackendTimeout because the TCP ConnectTimeout can't be reused for the future client-side Timeouts.

Can we translate this to transport socket timeout for downstream connections? If so, it can be reused...

Not sure about it, they look similar but not exactly the same. Are there any other possible different settings between client and backend timeouts?

@arkodg @zirain WDYT?

the go struct name can be changed anytime w/o breaking YAML API compatibility so no strong comments for now from my side

guydc · 2024-01-26T15:03:24Z

Regarding E2E test for this feature: echo server does support a /delay endpoint. However, since these settings are per-connection and not per-request, timed-out connections will not terminate active delayed requests. In other words, it's difficult to demonstrate a connection timeout on a specific request.

idle timeout: "The idle timeout is defined as the period in which there are no active requests."
max connection duration: "When max_connection_duration is reached and if there are no active streams, the connection will be closed"

One way to verify timeout settings with an e2e test is to trigger some upstream connections, wait for timeouts to occur and then check the relevant envoy cluster metrics.

I propose to handle the e2e test in a dedicated issue.

arkodg · 2024-01-26T17:25:20Z

sure @guydc lets raise seperate GH issues for e2e and docs and tackle those in different PRs

arkodg

LGTM thanks !

Signed-off-by: Guy Daich <[email protected]>

guydc · 2024-01-29T15:21:37Z

/retest

arkodg

LGTM thanks !

guydc requested a review from a team as a code owner January 16, 2024 23:53

arkodg reviewed Jan 17, 2024

View reviewed changes

shawnh2 reviewed Jan 17, 2024

View reviewed changes

guydc force-pushed the impl-btp-timeout branch from 8485b19 to ab06b32 Compare January 22, 2024 23:57

shawnh2 approved these changes Jan 23, 2024

View reviewed changes

zirain requested a review from arkodg January 23, 2024 03:08

zhaohuabing reviewed Jan 23, 2024

View reviewed changes

arkodg reviewed Jan 23, 2024

View reviewed changes

guydc force-pushed the impl-btp-timeout branch from ab06b32 to f556086 Compare January 26, 2024 14:46

arkodg previously approved these changes Jan 26, 2024

View reviewed changes

guydc mentioned this pull request Jan 26, 2024

e2e: Backend Traffic Policy Timeout #2510

Open

Implement BTP Timeout API

ed5ca57

Signed-off-by: Guy Daich <[email protected]>

guydc dismissed arkodg’s stale review via ed5ca57 January 29, 2024 13:59

guydc force-pushed the impl-btp-timeout branch from f556086 to ed5ca57 Compare January 29, 2024 13:59

arkodg approved these changes Jan 29, 2024

View reviewed changes

arkodg merged commit f1a0a42 into envoyproxy:main Jan 29, 2024
19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(translator): Implement BTP Timeout API #2454

feat(translator): Implement BTP Timeout API #2454

guydc commented Jan 16, 2024

codecov bot commented Jan 16, 2024 •

edited

Loading

arkodg Jan 17, 2024

arkodg Jan 17, 2024

shawnh2 Jan 17, 2024

shawnh2 left a comment

zhaohuabing left a comment •

edited

Loading

guydc commented Jan 23, 2024

arkodg Jan 23, 2024

guydc Jan 23, 2024

arkodg Jan 23, 2024

arkodg Jan 23, 2024

arkodg Jan 23, 2024

arkodg Jan 23, 2024

arkodg Jan 23, 2024

arkodg Jan 23, 2024

zhaohuabing commented Jan 24, 2024

arkodg commented Jan 26, 2024

guydc commented Jan 26, 2024

arkodg commented Jan 26, 2024

arkodg left a comment

guydc commented Jan 29, 2024

arkodg left a comment


		}

		func SetBackendTrafficPolicyTranslationErrorCondition(policy *egv1a1.BackendTrafficPolicy, field, errMsg string) {

feat(translator): Implement BTP Timeout API #2454

feat(translator): Implement BTP Timeout API #2454

Conversation

guydc commented Jan 16, 2024

codecov bot commented Jan 16, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawnh2 left a comment

Choose a reason for hiding this comment

zhaohuabing left a comment • edited Loading

Choose a reason for hiding this comment

guydc commented Jan 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhaohuabing commented Jan 24, 2024

arkodg commented Jan 26, 2024

guydc commented Jan 26, 2024

arkodg commented Jan 26, 2024

arkodg left a comment

Choose a reason for hiding this comment

guydc commented Jan 29, 2024

arkodg left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 16, 2024 •

edited

Loading

zhaohuabing left a comment •

edited

Loading