fix: Gateway-target BTP ignored when route timeout defined (envoyprox…

…y#4860) * fix: Gateway-target BTP ignored when route timeout defined Signed-off-by: Guy Daich <[email protected]> * fix gen, add note Signed-off-by: Guy Daich <[email protected]> --------- Signed-off-by: Guy Daich <[email protected]> (cherry picked from commit e6fce34) Signed-off-by: Guy Daich <[email protected]>
guydc · Dec 11, 2024 · 6a2a1a0 · 6a2a1a0
1 parent 52672de
commit 6a2a1a0
Show file tree

Hide file tree

Showing 16 changed files with 382 additions and 96 deletions.
diff --git 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); err != nil {
 			err = perr.WithMessage(err, "Timeout")
 			errs = errors.Join(errs, err)
 		}
@@ -411,8 +411,7 @@ 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 {
+				if localTo, err := t.buildTimeout(*policy); err == nil {
 					to = localTo
 				}
 
@@ -497,7 +496,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); err != nil {
 			err = perr.WithMessage(err, "Timeout")
 			errs = errors.Join(errs, err)
 		}
@@ -595,7 +594,7 @@ 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 ct, err = t.buildTimeout(*policy, r.Traffic); err == nil {
+			if ct, err = t.buildTimeout(*policy); err == nil {
 				r.Traffic.Timeout = ct
 			}
 
@@ -1066,12 +1065,8 @@ func (t *Translator) buildCircuitBreaker(policy *egv1a1.BackendTrafficPolicy) (*
 	return cb, nil
 }
 
-func (t *Translator) buildTimeout(policy egv1a1.BackendTrafficPolicy, traffic *ir.TrafficFeatures) (*ir.Timeout, error) {
+func (t *Translator) buildTimeout(policy egv1a1.BackendTrafficPolicy) (*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 (
@@ -1119,41 +1114,9 @@ func (t *Translator) buildTimeout(policy egv1a1.BackendTrafficPolicy, traffic *i
 		}
 	}
 
-	// http request timeout is translated during the gateway-api route resource translation
-	// 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
 }
 
-// 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) {
 	if in >= 0 && in <= math.MaxUint32 {
 		return uint32(in), true

diff --git a/internal/gatewayapi/helpers.go b/internal/gatewayapi/helpers.go
@@ -580,10 +580,3 @@ 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
@@ -257,14 +257,13 @@ func (t *Translator) processHTTPRouteRules(httpRoute *HTTPRouteContext, parentRe
 
 func processRouteTimeout(irRoute *ir.HTTPRoute, rule gwapiv1.HTTPRouteRule) {
 	if rule.Timeouts != nil {
-		rto := &ir.Timeout{}
 
 		if rule.Timeouts.Request != nil {
 			d, err := time.ParseDuration(string(*rule.Timeouts.Request))
 			if err != nil {
 				d, _ = time.ParseDuration(HTTPRequestTimeout)
 			}
-			setRequestTimeout(rto, metav1.Duration{Duration: d})
+			irRoute.Timeout = ptr.To(metav1.Duration{Duration: d})
 		}
 
 		// Also set the IR Route Timeout to the backend request timeout
@@ -274,23 +273,8 @@ func processRouteTimeout(irRoute *ir.HTTPRoute, rule gwapiv1.HTTPRouteRule) {
 			if err != nil {
 				d, _ = time.ParseDuration(HTTPRequestTimeout)
 			}
-			setRequestTimeout(rto, metav1.Duration{Duration: d})
+			irRoute.Timeout = ptr.To(metav1.Duration{Duration: d})
 		}
-
-		irRoute.Traffic = &ir.TrafficFeatures{
-			Timeout: rto,
-		}
-	}
-}
-
-func setRequestTimeout(irTimeout *ir.Timeout, d metav1.Duration) {
-	switch {
-	case irTimeout.HTTP == nil:
-		irTimeout.HTTP = &ir.HTTPTimeout{
-			RequestTimeout: ptr.To(d),
-		}
-	default:
-		irTimeout.HTTP.RequestTimeout = ptr.To(d)
 	}
 }
 
@@ -692,11 +676,11 @@ func (t *Translator) processHTTPRouteParentRefListener(route RouteContext, route
 					Mirrors:               routeRoute.Mirrors,
 					ExtensionRefs:         routeRoute.ExtensionRefs,
 					IsHTTP2:               routeRoute.IsHTTP2,
+					Timeout:               routeRoute.Timeout,
 				}
 				if routeRoute.Traffic != nil {
 					hostRoute.Traffic = &ir.TrafficFeatures{
-						Timeout: routeRoute.Traffic.Timeout,
-						Retry:   routeRoute.Traffic.Retry,
+						Retry: routeRoute.Traffic.Retry,
 					}
 				}
 				perHostRoutes = append(perHostRoutes, hostRoute)

diff --git a/internal/gatewayapi/testdata/backendtrafficpolicy-with-httproute-timeout.in.yaml b/internal/gatewayapi/testdata/backendtrafficpolicy-with-httproute-timeout.in.yaml
@@ -35,6 +35,27 @@ httpRoutes:
         port: 8080
       timeouts:
         request: 130s
+- apiVersion: gateway.networking.k8s.io/v1
+  kind: HTTPRoute
+  metadata:
+    namespace: default
+    name: httproute-2
+  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
@@ -47,4 +68,20 @@ backendTrafficPolicies:
       kind: HTTPRoute
       name: httproute-1
     useClientProtocol: true
-
+- apiVersion: gateway.envoyproxy.io/v1alpha1
+  kind: BackendTrafficPolicy
+  metadata:
+    namespace: envoy-gateway
+    name: policy-for-gateway
+  spec:
+    targetRef:
+      group: gateway.networking.k8s.io
+      kind: Gateway
+      name: gateway-1
+    useClientProtocol: true
+    loadBalancer:
+      type: ConsistentHash
+      consistentHash:
+        type: Cookie
+        cookie:
+          name: "test"
diff --git a/internal/gatewayapi/testdata/backendtrafficpolicy-with-httproute-timeout.out.yaml b/internal/gatewayapi/testdata/backendtrafficpolicy-with-httproute-timeout.out.yaml
@@ -26,6 +26,44 @@ backendTrafficPolicies:
         status: "True"
         type: Accepted
       controllerName: gateway.envoyproxy.io/gatewayclass-controller
+- apiVersion: gateway.envoyproxy.io/v1alpha1
+  kind: BackendTrafficPolicy
+  metadata:
+    creationTimestamp: null
+    name: policy-for-gateway
+    namespace: envoy-gateway
+  spec:
+    loadBalancer:
+      consistentHash:
+        cookie:
+          name: test
+        type: Cookie
+      type: ConsistentHash
+    targetRef:
+      group: gateway.networking.k8s.io
+      kind: Gateway
+      name: gateway-1
+    useClientProtocol: true
+  status:
+    ancestors:
+    - ancestorRef:
+        group: gateway.networking.k8s.io
+        kind: Gateway
+        name: gateway-1
+        namespace: envoy-gateway
+      conditions:
+      - lastTransitionTime: null
+        message: Policy has been accepted.
+        reason: Accepted
+        status: "True"
+        type: Accepted
+      - lastTransitionTime: null
+        message: 'This policy is being overridden by other backendTrafficPolicies
+          for these routes: [default/httproute-1]'
+        reason: Overridden
+        status: "True"
+        type: Overridden
+      controllerName: gateway.envoyproxy.io/gatewayclass-controller
 gateways:
 - apiVersion: gateway.networking.k8s.io/v1
   kind: Gateway
@@ -44,7 +82,7 @@ gateways:
       protocol: HTTP
   status:
     listeners:
-    - attachedRoutes: 1
+    - attachedRoutes: 2
       conditions:
       - lastTransitionTime: null
         message: Sending translated listener configuration to the data plane
@@ -108,6 +146,46 @@ httpRoutes:
         name: gateway-1
         namespace: envoy-gateway
         sectionName: http
+- apiVersion: gateway.networking.k8s.io/v1
+  kind: HTTPRoute
+  metadata:
+    creationTimestamp: null
+    name: httproute-2
+    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:
@@ -165,8 +243,33 @@ xdsIR:
           distinct: false
           name: ""
           prefix: /
+        timeout: 2m10s
+        traffic: {}
+        useClientProtocol: true
+      - destination:
+          name: httproute/default/httproute-2/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-2
+          namespace: default
+        name: httproute/default/httproute-2/rule/0/match/0/gateway_envoyproxy_io
+        pathMatch:
+          distinct: false
+          name: ""
+          prefix: /
+        timeout: 2m10s
         traffic:
-          timeout:
-            http:
-              requestTimeout: 2m10s
+          loadBalancer:
+            consistentHash:
+              cookie:
+                name: test
         useClientProtocol: true
diff --git a/internal/gatewayapi/testdata/httproute-and-backendtrafficpolicy-with-timeout-error.out.yaml b/internal/gatewayapi/testdata/httproute-and-backendtrafficpolicy-with-timeout-error.out.yaml
@@ -169,7 +169,4 @@ xdsIR:
           distinct: false
           name: ""
           prefix: /
-        traffic:
-          timeout:
-            http:
-              requestTimeout: 1s
+        timeout: 1s
diff --git a/internal/gatewayapi/testdata/httproute-and-backendtrafficpolicy-with-timeout.out.yaml b/internal/gatewayapi/testdata/httproute-and-backendtrafficpolicy-with-timeout.out.yaml
@@ -332,10 +332,10 @@ xdsIR:
           distinct: false
           name: ""
           prefix: /
+        timeout: 1s
         traffic:
           timeout:
             http:
               maxConnectionDuration: 22s
-              requestTimeout: 1s
             tcp:
               connectTimeout: 20s
diff --git a/internal/gatewayapi/testdata/httproute-backend-request-timeout.out.yaml b/internal/gatewayapi/testdata/httproute-backend-request-timeout.out.yaml
@@ -135,7 +135,4 @@ xdsIR:
           distinct: false
           name: ""
           prefix: /
-        traffic:
-          timeout:
-            http:
-              requestTimeout: 1s
+        timeout: 1s
diff --git a/internal/gatewayapi/testdata/httproute-request-timeout.out.yaml b/internal/gatewayapi/testdata/httproute-request-timeout.out.yaml
@@ -135,7 +135,4 @@ xdsIR:
           distinct: false
           name: ""
           prefix: /
-        traffic:
-          timeout:
-            http:
-              requestTimeout: 5s
+        timeout: 5s
diff --git a/internal/ir/xds.go b/internal/ir/xds.go
@@ -563,6 +563,8 @@ type HTTPRoute struct {
 	UseClientProtocol *bool `json:"useClientProtocol,omitempty" yaml:"useClientProtocol,omitempty"`
 	// Metadata is used to enrich envoy route metadata with user and provider-specific information
 	Metadata *ResourceMetadata `json:"metadata,omitempty" yaml:"metadata,omitempty"`
+	// Timeout is the time until which entire response is received from the upstream.
+	Timeout *metav1.Duration `json:"timeout,omitempty" yaml:"timeout,omitempty"`
 }
 
 // TrafficFeatures holds the information associated with the Backend Traffic Policy.