From add074674f2aa49f05736c620450e0bf09ca6d1b Mon Sep 17 00:00:00 2001 From: Steve Kriss Date: Fri, 12 Apr 2024 10:51:47 -0600 Subject: [PATCH] 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 --- changelogs/unreleased/6335-skriss-small.md | 1 + internal/dag/builder_test.go | 55 ++++++++++++++++++- internal/dag/gatewayapi_processor.go | 43 ++++++++++----- internal/dag/status_test.go | 12 ++-- .../gatewayapi/gateway_conformance_test.go | 6 -- test/scripts/run-gateway-conformance.sh | 4 +- 6 files changed, 89 insertions(+), 32 deletions(-) create mode 100644 changelogs/unreleased/6335-skriss-small.md 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