diff --git a/changelogs/unreleased/6335-skriss-small.md b/changelogs/unreleased/6335-skriss-small.md new file mode 100644 index 00000000000..35dfdfe90d8 --- /dev/null +++ b/changelogs/unreleased/6335-skriss-small.md @@ -0,0 +1 @@ +Gateway API: add support for HTTPRoute's Timeouts.BackendRequest field. \ No newline at end of file diff --git a/internal/dag/builder_test.go b/internal/dag/builder_test.go index ffeced315ff..7001251da91 100644 --- 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 index 58c4479b589..83759524dd1 100644 --- 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 index 4272f49b29e..7ed92689c56 100644 --- 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 index 361bab69311..86bf2238fc0 100644 --- 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 index f8b54d995b6..bdf77b7bc2a 100755 --- 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