internal/dag: basic Gateway API Timeouts.BackendRequest impl (#6335)

Implements the Gateway API BackendRequest timeout in a way that satisfies current conformance. Since retries do not exist yet in Gateway API, BackendRequest is functionally equivalent to Request timeout, but according to the API spec must be less than/equal to Request timeout if both are specified. Signed-off-by: Steve Kriss <stephen.kriss@gmail.com>
projectcontour · Apr 12, 2024 · add0746 · add0746
1 parent 818ca2d
commit add0746
Show file tree

Hide file tree

Showing 6 changed files with 89 additions and 32 deletions.
diff --git a/changelogs/unreleased/6335-skriss-small.md b/changelogs/unreleased/6335-skriss-small.md
@@ -0,0 +1 @@
+Gateway API: add support for HTTPRoute's Timeouts.BackendRequest field.
diff --git a/internal/dag/builder_test.go b/internal/dag/builder_test.go
@@ -3398,9 +3398,24 @@ func TestDAGInsertGatewayAPI(t *testing.T) {
 			gateway:      gatewayHTTPAllNamespaces,
 			objs: []any{
 				kuardService,
-				makeHTTPRouteWithTimeouts("5s", "5s"),
+				makeHTTPRouteWithTimeouts("5s", "4s"),
 			},
-			want: listeners(),
+			want: listeners(
+				&Listener{
+					Name: "http-80",
+					VirtualHosts: virtualhosts(
+						virtualhost("test.projectcontour.io",
+							&Route{
+								PathMatchCondition: prefixString("/"),
+								Clusters:           clustersWeight(service(kuardService)),
+								TimeoutPolicy: RouteTimeoutPolicy{
+									ResponseTimeout: timeout.DurationSetting(4 * time.Second),
+								},
+							},
+						),
+					),
+				},
+			),
 		},
 
 		"HTTPRoute rule with backendRequest timeout only": {
@@ -3410,7 +3425,22 @@ func TestDAGInsertGatewayAPI(t *testing.T) {
 				kuardService,
 				makeHTTPRouteWithTimeouts("", "5s"),
 			},
-			want: listeners(),
+			want: listeners(
+				&Listener{
+					Name: "http-80",
+					VirtualHosts: virtualhosts(
+						virtualhost("test.projectcontour.io",
+							&Route{
+								PathMatchCondition: prefixString("/"),
+								Clusters:           clustersWeight(service(kuardService)),
+								TimeoutPolicy: RouteTimeoutPolicy{
+									ResponseTimeout: timeout.DurationSetting(5 * time.Second),
+								},
+							},
+						),
+					),
+				},
+			),
 		},
 		"HTTPRoute rule with invalid request timeout": {
 			gatewayclass: validClass,
@@ -3421,6 +3451,25 @@ func TestDAGInsertGatewayAPI(t *testing.T) {
 			},
 			want: listeners(),
 		},
+		"HTTPRoute rule with invalid backend request timeout": {
+			gatewayclass: validClass,
+			gateway:      gatewayHTTPAllNamespaces,
+			objs: []any{
+				kuardService,
+				makeHTTPRouteWithTimeouts("5s", "invalid"),
+			},
+			want: listeners(),
+		},
+
+		"HTTPRoute rule with backend request timeout greater than request timeout": {
+			gatewayclass: validClass,
+			gateway:      gatewayHTTPAllNamespaces,
+			objs: []any{
+				kuardService,
+				makeHTTPRouteWithTimeouts("5s", "6s"),
+			},
+			want: listeners(),
+		},
 
 		"HTTPRoute with BackendTLSPolicy": {
 			gatewayclass: validClass,

diff --git a/internal/dag/gatewayapi_processor.go b/internal/dag/gatewayapi_processor.go
@@ -14,7 +14,6 @@
 package dag
 
 import (
-	"errors"
 	"fmt"
 	"net/http"
 	"regexp"
@@ -1117,28 +1116,42 @@ func (p *GatewayAPIProcessor) resolveRouteRefs(route any, routeAccessor *status.
 }
 
 func parseHTTPRouteTimeouts(httpRouteTimeouts *gatewayapi_v1.HTTPRouteTimeouts) (*RouteTimeoutPolicy, error) {
-	if httpRouteTimeouts == nil {
+	if httpRouteTimeouts == nil || (httpRouteTimeouts.Request == nil && httpRouteTimeouts.BackendRequest == nil) {
 		return nil, nil
 	}
 
-	// Since Gateway API doesn't yet support retries, this timeout setting
-	// is functionally equivalent to httpRouteTimeouts.Request, so we're
-	// not implementing it for now. Once retries are added to Gateway API,
-	// support for backend request timeouts can be added.
-	if httpRouteTimeouts.BackendRequest != nil {
-		return nil, errors.New("HTTPRoute.Spec.Rules.Timeouts.BackendRequest is not supported, use HTTPRoute.Spec.Rules.Timeouts.Request instead")
-	}
-	if httpRouteTimeouts.Request == nil {
-		return nil, nil
+	var responseTimeout timeout.Setting
+
+	if httpRouteTimeouts.Request != nil {
+		requestTimeout, err := timeout.Parse(string(*httpRouteTimeouts.Request))
+		if err != nil {
+			return nil, fmt.Errorf("invalid HTTPRoute.Spec.Rules.Timeouts.Request: %v", err)
+		}
+
+		responseTimeout = requestTimeout
 	}
 
-	requestTimeout, err := timeout.Parse(string(*httpRouteTimeouts.Request))
-	if err != nil {
-		return nil, fmt.Errorf("invalid HTTPRoute.Spec.Rules.Timeouts.Request: %v", err)
+	// Note, since retries are not yet implemented in Gateway API, the backend
+	// request timeout is functionally equivalent to the request timeout for now.
+	// The API spec requires that it be less than/equal to the request timeout if
+	// both are specified. This implementation will change when retries are implemented.
+	if httpRouteTimeouts.BackendRequest != nil {
+		backendRequestTimeout, err := timeout.Parse(string(*httpRouteTimeouts.BackendRequest))
+		if err != nil {
+			return nil, fmt.Errorf("invalid HTTPRoute.Spec.Rules.Timeouts.BackendRequest: %v", err)
+		}
+
+		// If Timeouts.Request was specified, then Timeouts.BackendRequest must be
+		// less than/equal to it.
+		if responseTimeout.Duration() > 0 && backendRequestTimeout.Duration() > responseTimeout.Duration() {
+			return nil, fmt.Errorf("HTTPRoute.Spec.Rules.Timeouts.BackendRequest must be less than/equal to HTTPRoute.Spec.Rules.Timeouts.Request when both are specified")
+		}
+
+		responseTimeout = backendRequestTimeout
 	}
 
 	return &RouteTimeoutPolicy{
-		ResponseTimeout: requestTimeout,
+		ResponseTimeout: responseTimeout,
 	}, nil
 }
 

diff --git a/internal/dag/status_test.go b/internal/dag/status_test.go
@@ -9441,7 +9441,7 @@ func TestGatewayAPIHTTPRouteDAGStatus(t *testing.T) {
 		},
 	})
 
-	run(t, "route rule with timeouts.request and timeouts.backendRequest specified", testcase{
+	run(t, "route rule with timeouts.backendRequest greater than timeouts.request", testcase{
 		objs: []any{
 			kuardService,
 			&gatewayapi_v1.HTTPRoute{
@@ -9462,7 +9462,7 @@ func TestGatewayAPIHTTPRouteDAGStatus(t *testing.T) {
 							BackendRefs: gatewayapi.HTTPBackendRef("kuard", 8080, 1),
 							Timeouts: &gatewayapi_v1.HTTPRouteTimeouts{
 								Request:        ptr.To(gatewayapi_v1.Duration("30s")),
-								BackendRequest: ptr.To(gatewayapi_v1.Duration("30s")),
+								BackendRequest: ptr.To(gatewayapi_v1.Duration("60s")),
 							},
 						},
 					},
@@ -9477,7 +9477,7 @@ func TestGatewayAPIHTTPRouteDAGStatus(t *testing.T) {
 					ParentRef: gatewayapi.GatewayParentRef("projectcontour", "contour"),
 					Conditions: []meta_v1.Condition{
 						routeResolvedRefsCondition(),
-						routeAcceptedFalse(gatewayapi_v1.RouteReasonUnsupportedValue, "HTTPRoute.Spec.Rules.Timeouts.BackendRequest is not supported, use HTTPRoute.Spec.Rules.Timeouts.Request instead"),
+						routeAcceptedFalse(gatewayapi_v1.RouteReasonUnsupportedValue, "HTTPRoute.Spec.Rules.Timeouts.BackendRequest must be less than/equal to HTTPRoute.Spec.Rules.Timeouts.Request when both are specified"),
 					},
 				},
 			},
@@ -9486,7 +9486,7 @@ func TestGatewayAPIHTTPRouteDAGStatus(t *testing.T) {
 		wantGatewayStatusUpdate: validGatewayStatusUpdate("http", gatewayapi_v1.HTTPProtocolType, 1),
 	})
 
-	run(t, "route rule with only timeouts.backendRequest specified", testcase{
+	run(t, "route rule with invalid timeouts.backendRequest specified", testcase{
 		objs: []any{
 			kuardService,
 			&gatewayapi_v1.HTTPRoute{
@@ -9506,7 +9506,7 @@ func TestGatewayAPIHTTPRouteDAGStatus(t *testing.T) {
 							Matches:     gatewayapi.HTTPRouteMatch(gatewayapi_v1.PathMatchPathPrefix, "/"),
 							BackendRefs: gatewayapi.HTTPBackendRef("kuard", 8080, 1),
 							Timeouts: &gatewayapi_v1.HTTPRouteTimeouts{
-								BackendRequest: ptr.To(gatewayapi_v1.Duration("30s")),
+								BackendRequest: ptr.To(gatewayapi_v1.Duration("invalid")),
 							},
 						},
 					},
@@ -9520,7 +9520,7 @@ func TestGatewayAPIHTTPRouteDAGStatus(t *testing.T) {
 					ParentRef: gatewayapi.GatewayParentRef("projectcontour", "contour"),
 					Conditions: []meta_v1.Condition{
 						routeResolvedRefsCondition(),
-						routeAcceptedFalse(gatewayapi_v1.RouteReasonUnsupportedValue, "HTTPRoute.Spec.Rules.Timeouts.BackendRequest is not supported, use HTTPRoute.Spec.Rules.Timeouts.Request instead"),
+						routeAcceptedFalse(gatewayapi_v1.RouteReasonUnsupportedValue, "invalid HTTPRoute.Spec.Rules.Timeouts.BackendRequest: unable to parse timeout string \"invalid\": time: invalid duration \"invalid\""),
 					},
 				},
 			},

diff --git a/test/conformance/gatewayapi/gateway_conformance_test.go b/test/conformance/gatewayapi/gateway_conformance_test.go
@@ -74,12 +74,6 @@ func TestGatewayConformance(t *testing.T) {
 			// See: https://github.com/envoyproxy/envoy/issues/17318
 			tests.HTTPRouteRedirectPortAndScheme.ShortName,
 
-			// Not implemented yet since it's functionally equivalent
-			// to Timeouts.Request, to be enabled once Gateway API
-			// supports retries.
-			// See: https://github.com/projectcontour/contour/issues/6000
-			tests.HTTPRouteTimeoutBackendRequest.ShortName,
-
 			// Contour supports the positive-case functionality,
 			// but there are some negative cases that aren't fully
 			// implemented plus complications with the test setup itself.

diff --git a/test/scripts/run-gateway-conformance.sh b/test/scripts/run-gateway-conformance.sh
@@ -58,7 +58,7 @@ GO_MOD_GATEWAY_API_VERSION=$(grep "sigs.k8s.io/gateway-api" go.mod | awk '{print
 
 if [ "$GATEWAY_API_VERSION" = "$GO_MOD_GATEWAY_API_VERSION" ]; then
   go test -timeout=40m -tags conformance ./test/conformance/gatewayapi --gateway-class=contour
-else 
+else
   cd $(mktemp -d)
   git clone https://github.com/kubernetes-sigs/gateway-api
   cd gateway-api
@@ -67,5 +67,5 @@ else
   # test/conformance/gatewayapi/gateway_conformance_test.go.
   go test -timeout=40m ./conformance -run TestConformance -gateway-class=contour -all-features \
     -exempt-features=Mesh \
-    -skip-tests=HTTPRouteRedirectPortAndScheme,HTTPRouteTimeoutBackendRequest,GatewayStaticAddresses
+    -skip-tests=HTTPRouteRedirectPortAndScheme,GatewayStaticAddresses
 fi