Repository: envoyproxy/gateway
Pull request #4450 (2024-10-16 00:39:20)

[release/v1.1] fix: don't lose timeout settings that originate from the route when t…
fix https://github.com/envoyproxy/gateway/issues/3251
…ranslating the backend traffic policy (#4095) (#85)

* Don't lose timeout settings that originate from the route when translating the backend traffic policy



* Improve the readability of the timeout processing code.



* Removed some logically unreachable code.



---------


(cherry picked from commit 262e0466f14dace834f1b0d712c2492c27e9bb03)

**What type of PR is this?**
<!--
Your PR title should be descriptive, and generally start with type that contains a subsystem name with `()` if necessary 
and summary followed by a colon. format `chore/docs/api/feat/fix/refactor/style/test: summary`.
Examples:
* "docs: fix grammar error"
* "feat(translator): add new feature"
* "fix: fix xx bug"
* "chore: change ci & build tools etc"
* "api: add xxx fields in ClientTrafficPolicy"
-->

<!--
NOTE: If your PR contains any API changes (changes under `/api`), we recommend you to separate these API changes into
a new PR, and we will review the API part first. It will save you lots of implementation time if the API get accepted.
-->

**What this PR does / why we need it**:

**Which issue(s) this PR fixes**:
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
-->
Fixes #


codecov[bot]: ## [Codecov](https://app.codecov.io/gh/envoyproxy/gateway/pull/4450?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=envoyproxy) Report
Attention: Patch coverage is `93.18182%` with `3 lines` in your changes missing coverage. Please review.
> Project coverage is 67.37%. Comparing base [(`70b1697`)](https://app.codecov.io/gh/envoyproxy/gateway/commit/70b1697e34e3573fb0c6749f7f80ddbc78ee8d33?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=envoyproxy) to head [(`cb4b3c9`)](https://app.codecov.io/gh/envoyproxy/gateway/commit/cb4b3c9ef7ea957d4f7d3529264c7bfc90be1cc3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=envoyproxy).

| [Files with missing lines](https://app.codecov.io/gh/envoyproxy/gateway/pull/4450?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=envoyproxy) | Patch % | Lines |
|---|---|---|
| [internal/gatewayapi/backendtrafficpolicy.go](https://app.codecov.io/gh/envoyproxy/gateway/pull/4450?src=pr&el=tree&filepath=internal%2Fgatewayapi%2Fbackendtrafficpolicy.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=envoyproxy#diff-aW50ZXJuYWwvZ2F0ZXdheWFwaS9iYWNrZW5kdHJhZmZpY3BvbGljeS5nbw==) | 91.89% | [2 Missing and 1 partial :warning: ](https://app.codecov.io/gh/envoyproxy/gateway/pull/4450?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=envoyproxy) |

<details><summary>Additional details and impacted files</summary>


```diff
@@               Coverage Diff                @@
##           release/v1.1    #4450      +/-   ##
================================================
+ Coverage         67.34%   67.37%   +0.03%     
================================================
  Files               183      183              
  Lines             22521    22519       -2     
================================================
+ Hits              15166    15173       +7     
+ Misses             6265     6259       -6     
+ Partials           1090     1087       -3     
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/envoyproxy/gateway/pull/4450?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=envoyproxy).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=envoyproxy).


Patch:
diff --git a/internal/gatewayapi/backendtrafficpolicy.go b/internal/gatewayapi/backendtrafficpolicy.go
index ee47b7601..9d4088ef3 100644
--- a/internal/gatewayapi/backendtrafficpolicy.go
+++ b/internal/gatewayapi/backendtrafficpolicy.go
@@ -361,7 +361,7 @@ func (t *Translator) translateBackendTrafficPolicyForRoute(policy *egv1a1.Backen
 		rt = t.buildRetry(policy)
 	}
 	if policy.Spec.Timeout != nil {
-		if to, err = t.buildTimeout(policy, nil); err != nil {
+		if to, err = t.buildTimeout(*policy, nil); err != nil {
 			err = perr.WithMessage(err, "Timeout")
 			errs = errors.Join(errs, err)
 		}
@@ -411,6 +411,11 @@ func (t *Translator) translateBackendTrafficPolicyForRoute(policy *egv1a1.Backen
 
 		for _, http := range x.HTTP {
 			for _, r := range http.Routes {
+				// Some timeout setting originate from the route.
+				if localTo, err := t.buildTimeout(*policy, r.Traffic); err == nil {
+					to = localTo
+				}
+
 				// Apply if there is a match
 				if strings.HasPrefix(r.Name, prefix) {
 					r.Traffic = &ir.TrafficFeatures{
@@ -423,18 +428,12 @@ func (t *Translator) translateBackendTrafficPolicyForRoute(policy *egv1a1.Backen
 						TCPKeepalive:      ka,
 						Retry:             rt,
 						BackendConnection: bc,
+						Timeout:           to,
 					}
 
 					// Update the Host field in HealthCheck, now that we have access to the Route Hostname.
 					r.Traffic.HealthCheck.SetHTTPHostIfAbsent(r.Hostname)
 
-					// Some timeout setting originate from the route.
-					if policy.Spec.Timeout != nil {
-						if to, err = t.buildTimeout(policy, r); err == nil {
-							r.Traffic.Timeout = to
-						}
-					}
-
 					if policy.Spec.UseClientProtocol != nil {
 						r.UseClientProtocol = policy.Spec.UseClientProtocol
 					}
@@ -498,7 +497,7 @@ func (t *Translator) translateBackendTrafficPolicyForGateway(policy *egv1a1.Back
 		rt = t.buildRetry(policy)
 	}
 	if policy.Spec.Timeout != nil {
-		if ct, err = t.buildTimeout(policy, nil); err != nil {
+		if ct, err = t.buildTimeout(*policy, nil); err != nil {
 			err = perr.WithMessage(err, "Timeout")
 			errs = errors.Join(errs, err)
 		}
@@ -596,10 +595,8 @@ func (t *Translator) translateBackendTrafficPolicyForGateway(policy *egv1a1.Back
 			// Update the Host field in HealthCheck, now that we have access to the Route Hostname.
 			r.Traffic.HealthCheck.SetHTTPHostIfAbsent(r.Hostname)
 
-			if policy.Spec.Timeout != nil {
-				if ct, err = t.buildTimeout(policy, r); err == nil {
-					r.Traffic.Timeout = ct
-				}
+			if ct, err = t.buildTimeout(*policy, r.Traffic); err == nil {
+				r.Traffic.Timeout = ct
 			}
 
 			if policy.Spec.UseClientProtocol != nil {
@@ -1069,23 +1066,26 @@ func (t *Translator) buildCircuitBreaker(policy *egv1a1.BackendTrafficPolicy) (*
 	return cb, nil
 }
 
-func (t *Translator) buildTimeout(policy *egv1a1.BackendTrafficPolicy, r *ir.HTTPRoute) (*ir.Timeout, error) {
+func (t *Translator) buildTimeout(policy egv1a1.BackendTrafficPolicy, traffic *ir.TrafficFeatures) (*ir.Timeout, error) {
+	if policy.Spec.Timeout == nil {
+		if traffic != nil {
+			// Don't lose any existing timeout definitions.
+			return mergeTimeoutSettings(nil, traffic.Timeout), nil
+		}
+		return nil, nil
+	}
 	var (
-		tto  *ir.TCPTimeout
-		hto  *ir.HTTPTimeout
-		terr bool
 		errs error
+		to   = &ir.Timeout{}
+		pto  = policy.Spec.Timeout
 	)
 
-	pto := policy.Spec.Timeout
-
 	if pto.TCP != nil && pto.TCP.ConnectTimeout != nil {
 		d, err := time.ParseDuration(string(*pto.TCP.ConnectTimeout))
 		if err != nil {
-			terr = true
 			errs = errors.Join(errs, fmt.Errorf("invalid ConnectTimeout value %s", *pto.TCP.ConnectTimeout))
 		} else {
-			tto = &ir.TCPTimeout{
+			to.TCP = &ir.TCPTimeout{
 				ConnectTimeout: ptr.To(metav1.Duration{Duration: d}),
 			}
 		}
@@ -1098,7 +1098,6 @@ func (t *Translator) buildTimeout(policy *egv1a1.BackendTrafficPolicy, r *ir.HTT
 		if pto.HTTP.ConnectionIdleTimeout != nil {
 			d, err := time.ParseDuration(string(*pto.HTTP.ConnectionIdleTimeout))
 			if err != nil {
-				terr = true
 				errs = errors.Join(errs, fmt.Errorf("invalid ConnectionIdleTimeout value %s", *pto.HTTP.ConnectionIdleTimeout))
 			} else {
 				cit = ptr.To(metav1.Duration{Duration: d})
@@ -1108,51 +1107,51 @@ func (t *Translator) buildTimeout(policy *egv1a1.BackendTrafficPolicy, r *ir.HTT
 		if pto.HTTP.MaxConnectionDuration != nil {
 			d, err := time.ParseDuration(string(*pto.HTTP.MaxConnectionDuration))
 			if err != nil {
-				terr = true
 				errs = errors.Join(errs, fmt.Errorf("invalid MaxConnectionDuration value %s", *pto.HTTP.MaxConnectionDuration))
 			} else {
 				mcd = ptr.To(metav1.Duration{Duration: d})
 			}
 		}
 
-		hto = &ir.HTTPTimeout{
+		to.HTTP = &ir.HTTPTimeout{
 			ConnectionIdleTimeout: cit,
 			MaxConnectionDuration: mcd,
 		}
 	}
 
 	// http request timeout is translated during the gateway-api route resource translation
-	// merge route timeout setting with backendtrafficpolicy timeout settings
-	if terr {
-		if r != nil && r.Traffic != nil && r.Traffic.Timeout != nil {
-			return r.Traffic.Timeout.DeepCopy(), errs
-		}
-	} else {
-		// http request timeout is translated during the gateway-api route resource translation
-		// merge route timeout setting with backendtrafficpolicy timeout settings
-		if r != nil &&
-			r.Traffic != nil &&
-			r.Traffic.Timeout != nil &&
-			r.Traffic.Timeout.HTTP != nil &&
-			r.Traffic.Timeout.HTTP.RequestTimeout != nil {
-			if hto == nil {
-				hto = &ir.HTTPTimeout{
-					RequestTimeout: r.Traffic.Timeout.HTTP.RequestTimeout,
-				}
-			} else {
-				hto.RequestTimeout = r.Traffic.Timeout.HTTP.RequestTimeout
-			}
-		}
-
-		if hto != nil || tto != nil {
-			return &ir.Timeout{
-				TCP:  tto,
-				HTTP: hto,
-			}, nil
-		}
+	// merge route timeout setting with backendtrafficpolicy timeout settings.
+	// Merging is done after the clustersettings definitions are translated so that
+	// clustersettings will override previous settings.
+	if traffic != nil {
+		to = mergeTimeoutSettings(to, traffic.Timeout)
 	}
+	return to, errs
+}
 
-	return nil, errs
+// merge secondary into main if both are not nil, otherwise return the
+// one that is not nil. If both are nil, returns nil
+func mergeTimeoutSettings(main, secondary *ir.Timeout) *ir.Timeout {
+	switch {
+	case main == nil && secondary == nil:
+		return nil
+	case main == nil:
+		return secondary.DeepCopy()
+	case secondary == nil:
+		return main
+	default: // Neither main nor secondary are nil here
+		if secondary.HTTP != nil {
+			setIfNil(&main.HTTP, &ir.HTTPTimeout{})
+			setIfNil(&main.HTTP.RequestTimeout, secondary.HTTP.RequestTimeout)
+			setIfNil(&main.HTTP.ConnectionIdleTimeout, secondary.HTTP.ConnectionIdleTimeout)
+			setIfNil(&main.HTTP.MaxConnectionDuration, secondary.HTTP.MaxConnectionDuration)
+		}
+		if secondary.TCP != nil {
+			setIfNil(&main.TCP, &ir.TCPTimeout{})
+			setIfNil(&main.TCP.ConnectTimeout, secondary.TCP.ConnectTimeout)
+		}
+		return main
+	}
 }
 
 func int64ToUint32(in int64) (uint32, bool) {
diff --git a/internal/gatewayapi/helpers.go b/internal/gatewayapi/helpers.go
index a6e13720e..52df40f47 100644
--- a/internal/gatewayapi/helpers.go
+++ b/internal/gatewayapi/helpers.go
@@ -580,3 +580,10 @@ func getPolicyTargetRefs[T client.Object](policy egv1a1.PolicyTargetReferences,
 
 	return ret
 }
+
+// Sets *target to value if and only if *target is nil
+func setIfNil[T any](target **T, value *T) {
+	if *target == nil {
+		*target = value
+	}
+}
diff --git a/internal/gatewayapi/route.go b/internal/gatewayapi/route.go
index b113411b3..ce7fff56a 100644
--- a/internal/gatewayapi/route.go
+++ b/internal/gatewayapi/route.go
@@ -255,16 +255,9 @@ func (t *Translator) processHTTPRouteRules(httpRoute *HTTPRouteContext, parentRe
 	return routeRoutes, nil
 }
 
-func processTimeout(irRoute *ir.HTTPRoute, rule gwapiv1.HTTPRouteRule) {
+func processRouteTimeout(irRoute *ir.HTTPRoute, rule gwapiv1.HTTPRouteRule) {
 	if rule.Timeouts != nil {
-		var rto *ir.Timeout
-
-		// Timeout is translated from multiple resources and may already be partially set
-		if irRoute.Traffic != nil && irRoute.Traffic.Timeout != nil {
-			rto = irRoute.Traffic.Timeout.DeepCopy()
-		} else {
-			rto = &ir.Timeout{}
-		}
+		rto := &ir.Timeout{}
 
 		if rule.Timeouts.Request != nil {
 			d, err := time.ParseDuration(string(*rule.Timeouts.Request))
@@ -309,7 +302,7 @@ func (t *Translator) processHTTPRouteRule(httpRoute *HTTPRouteContext, ruleIdx i
 		irRoute := &ir.HTTPRoute{
 			Name: irRouteName(httpRoute, ruleIdx, -1),
 		}
-		processTimeout(irRoute, rule)
+		processRouteTimeout(irRoute, rule)
 		applyHTTPFiltersContextToIRRoute(httpFiltersContext, irRoute)
 		ruleRoutes = append(ruleRoutes, irRoute)
 	}
@@ -321,7 +314,7 @@ func (t *Translator) processHTTPRouteRule(httpRoute *HTTPRouteContext, ruleIdx i
 		irRoute := &ir.HTTPRoute{
 			Name: irRouteName(httpRoute, ruleIdx, matchIdx),
 		}
-		processTimeout(irRoute, rule)
+		processRouteTimeout(irRoute, rule)
 
 		if match.Path != nil {
 			switch PathMatchTypeDerefOr(match.Path.Type, gwapiv1.PathMatchPathPrefix) {


Test patch:
diff --git a/internal/gatewayapi/testdata/backendtrafficpolicy-with-httproute-timeout.in.yaml b/internal/gatewayapi/testdata/backendtrafficpolicy-with-httproute-timeout.in.yaml
new file mode 100644
index 000000000..e26f10c35
--- /dev/null
+++ b/internal/gatewayapi/testdata/backendtrafficpolicy-with-httproute-timeout.in.yaml
@@ -0,0 +1,50 @@
+gateways:
+- apiVersion: gateway.networking.k8s.io/v1
+  kind: Gateway
+  metadata:
+    namespace: envoy-gateway
+    name: gateway-1
+  spec:
+    gatewayClassName: envoy-gateway-class
+    listeners:
+    - name: http
+      protocol: HTTP
+      port: 80
+      allowedRoutes:
+        namespaces:
+          from: All
+httpRoutes:
+- apiVersion: gateway.networking.k8s.io/v1
+  kind: HTTPRoute
+  metadata:
+    namespace: default
+    name: httproute-1
+  spec:
+    hostnames:
+    - gateway.envoyproxy.io
+    parentRefs:
+    - namespace: envoy-gateway
+      name: gateway-1
+      sectionName: http
+    rules:
+    - matches:
+      - path:
+          value: "/"
+      backendRefs:
+      - name: service-1
+        port: 8080
+      timeouts:
+        request: 130s
+backendTrafficPolicies:
+- apiVersion: gateway.envoyproxy.io/v1alpha1
+  kind: BackendTrafficPolicy
+  metadata:
+    namespace: default
+    name: policy-for-http-route-1
+  spec:
+    targetRef:
+      group: gateway.networking.k8s.io
+      kind: HTTPRoute
+      name: httproute-1
+    useClientProtocol: true
+
diff --git a/internal/gatewayapi/testdata/backendtrafficpolicy-with-httproute-timeout.out.yaml b/internal/gatewayapi/testdata/backendtrafficpolicy-with-httproute-timeout.out.yaml
new file mode 100644
index 000000000..245739ca2
--- /dev/null
+++ b/internal/gatewayapi/testdata/backendtrafficpolicy-with-httproute-timeout.out.yaml
@@ -0,0 +1,172 @@
+backendTrafficPolicies:
+- apiVersion: gateway.envoyproxy.io/v1alpha1
+  kind: BackendTrafficPolicy
+  metadata:
+    creationTimestamp: null
+    name: policy-for-http-route-1
+    namespace: default
+  spec:
+    targetRef:
+      group: gateway.networking.k8s.io
+      kind: HTTPRoute
+      name: httproute-1
+    useClientProtocol: true
+  status:
+    ancestors:
+    - ancestorRef:
+        group: gateway.networking.k8s.io
+        kind: Gateway
+        name: gateway-1
+        namespace: envoy-gateway
+        sectionName: http
+      conditions:
+      - lastTransitionTime: null
+        message: Policy has been accepted.
+        reason: Accepted
+        status: "True"
+        type: Accepted
+      controllerName: gateway.envoyproxy.io/gatewayclass-controller
+gateways:
+- apiVersion: gateway.networking.k8s.io/v1
+  kind: Gateway
+  metadata:
+    creationTimestamp: null
+    name: gateway-1
+    namespace: envoy-gateway
+  spec:
+    gatewayClassName: envoy-gateway-class
+    listeners:
+    - allowedRoutes:
+        namespaces:
+          from: All
+      name: http
+      port: 80
+      protocol: HTTP
+  status:
+    listeners:
+    - attachedRoutes: 1
+      conditions:
+      - lastTransitionTime: null
+        message: Sending translated listener configuration to the data plane
+        reason: Programmed
+        status: "True"
+        type: Programmed
+      - lastTransitionTime: null
+        message: Listener has been successfully translated
+        reason: Accepted
+        status: "True"
+        type: Accepted
+      - lastTransitionTime: null
+        message: Listener references have been resolved
+        reason: ResolvedRefs
+        status: "True"
+        type: ResolvedRefs
+      name: http
+      supportedKinds:
+      - group: gateway.networking.k8s.io
+        kind: HTTPRoute
+      - group: gateway.networking.k8s.io
+        kind: GRPCRoute
+httpRoutes:
+- apiVersion: gateway.networking.k8s.io/v1
+  kind: HTTPRoute
+  metadata:
+    creationTimestamp: null
+    name: httproute-1
+    namespace: default
+  spec:
+    hostnames:
+    - gateway.envoyproxy.io
+    parentRefs:
+    - name: gateway-1
+      namespace: envoy-gateway
+      sectionName: http
+    rules:
+    - backendRefs:
+      - name: service-1
+        port: 8080
+      matches:
+      - path:
+          value: /
+      timeouts:
+        request: 130s
+  status:
+    parents:
+    - conditions:
+      - lastTransitionTime: null
+        message: Route is accepted
+        reason: Accepted
+        status: "True"
+        type: Accepted
+      - lastTransitionTime: null
+        message: Resolved all the Object references for the Route
+        reason: ResolvedRefs
+        status: "True"
+        type: ResolvedRefs
+      controllerName: gateway.envoyproxy.io/gatewayclass-controller
+      parentRef:
+        name: gateway-1
+        namespace: envoy-gateway
+        sectionName: http
+infraIR:
+  envoy-gateway/gateway-1:
+    proxy:
+      listeners:
+      - address: null
+        name: envoy-gateway/gateway-1/http
+        ports:
+        - containerPort: 10080
+          name: http-80
+          protocol: HTTP
+          servicePort: 80
+      metadata:
+        labels:
+          gateway.envoyproxy.io/owning-gateway-name: gateway-1
+          gateway.envoyproxy.io/owning-gateway-namespace: envoy-gateway
+      name: envoy-gateway/gateway-1
+xdsIR:
+  envoy-gateway/gateway-1:
+    accessLog:
+      text:
+      - path: /dev/stdout
+    http:
+    - address: 0.0.0.0
+      hostnames:
+      - '*'
+      isHTTP2: false
+      metadata:
+        kind: Gateway
+        name: gateway-1
+        namespace: envoy-gateway
+        sectionName: http
+      name: envoy-gateway/gateway-1/http
+      path:
+        escapedSlashesAction: UnescapeAndRedirect
+        mergeSlashes: true
+      port: 10080
+      routes:
+      - destination:
+          name: httproute/default/httproute-1/rule/0
+          settings:
+          - addressType: IP
+            endpoints:
+            - host: 7.7.7.7
+              port: 8080
+            protocol: HTTP
+            weight: 1
+        hostname: gateway.envoyproxy.io
+        isHTTP2: false
+        metadata:
+          kind: HTTPRoute
+          name: httproute-1
+          namespace: default
+        name: httproute/default/httproute-1/rule/0/match/0/gateway_envoyproxy_io
+        pathMatch:
+          distinct: false
+          name: ""
+          prefix: /
+        traffic:
+          timeout:
+            http:
+              requestTimeout: 2m10s
+        useClientProtocol: true
diff --git a/internal/gatewayapi/testdata/httproute-and-backendtrafficpolicy-with-timeout.out.yaml b/internal/gatewayapi/testdata/httproute-and-backendtrafficpolicy-with-timeout.out.yaml
index 885217164..04843eba9 100644
--- a/internal/gatewayapi/testdata/httproute-and-backendtrafficpolicy-with-timeout.out.yaml
+++ b/internal/gatewayapi/testdata/httproute-and-backendtrafficpolicy-with-timeout.out.yaml
@@ -336,5 +336,6 @@ xdsIR:
           timeout:
             http:
               maxConnectionDuration: 22s
+              requestTimeout: 1s
             tcp:
               connectTimeout: 20s