From 805debfb8f3d6aebe79e9874d66c98c45be314ef Mon Sep 17 00:00:00 2001 From: Steve Kriss Date: Tue, 5 Dec 2023 13:47:01 -0700 Subject: [PATCH] only support request timeout for now Signed-off-by: Steve Kriss --- internal/dag/builder_test.go | 93 +++++++++------------------- internal/dag/gatewayapi_processor.go | 57 +++++++---------- internal/dag/status_test.go | 14 ++--- 3 files changed, 56 insertions(+), 108 deletions(-) diff --git a/internal/dag/builder_test.go b/internal/dag/builder_test.go index f6161c9a0c3..fb4b435059c 100644 --- a/internal/dag/builder_test.go +++ b/internal/dag/builder_test.go @@ -4159,55 +4159,50 @@ func TestDAGInsertGatewayAPI(t *testing.T) { ), }, - "Route rule with timeouts": { + "HTTPRoute rule with request timeout": { gatewayclass: validClass, gateway: gatewayHTTPAllNamespaces, objs: []any{ kuardService, - makeHTTPRoute("5s", "5s"), + makeHTTPRoute("5s", ""), }, want: listeners( &Listener{ Name: "http-80", VirtualHosts: virtualhosts( - virtualhost("test.projectcontour.io", timeoutsHTTPProute("/", makeHTTPRouteTimeouts("5s", "5s"), service(kuardService))), + virtualhost("test.projectcontour.io", + &Route{ + PathMatchCondition: prefixString("/"), + Clusters: clustersWeight(service(kuardService)), + TimeoutPolicy: RouteTimeoutPolicy{ + ResponseTimeout: timeout.DurationSetting(5 * time.Second), + }, + }, + ), ), }, ), }, - "Route rule with timeouts request only": { + "HTTPRoute rule with request and backendRequest timeout": { gatewayclass: validClass, gateway: gatewayHTTPAllNamespaces, objs: []any{ kuardService, - makeHTTPRoute("5s", ""), + makeHTTPRoute("5s", "5s"), }, - want: listeners( - &Listener{ - Name: "http-80", - VirtualHosts: virtualhosts( - virtualhost("test.projectcontour.io", timeoutsHTTPProute("/", makeHTTPRouteTimeouts("5s", ""), service(kuardService))), - ), - }, - ), + want: listeners(), }, - "Route rule with timeouts backendRequest only": { + + "HTTPRoute rule with backendRequest timeout only": { gatewayclass: validClass, gateway: gatewayHTTPAllNamespaces, objs: []any{ kuardService, makeHTTPRoute("", "5s"), }, - want: listeners( - &Listener{ - Name: "http-80", - VirtualHosts: virtualhosts( - virtualhost("test.projectcontour.io", timeoutsHTTPProute("/", makeHTTPRouteTimeouts("", ""), service(kuardService))), - ), - }, - ), + want: listeners(), }, - "Route rule with timeouts request invalid": { + "HTTPRoute rule with invalid request timeout": { gatewayclass: validClass, gateway: gatewayHTTPAllNamespaces, objs: []any{ @@ -4216,7 +4211,6 @@ func TestDAGInsertGatewayAPI(t *testing.T) { }, want: listeners(), }, - // END "different weights for multiple forwardTos": { gatewayclass: validClass, @@ -15994,44 +15988,6 @@ func prefixroute(prefix string, first *Service, rest ...*Service) *Route { } } -func httpRouteTimeoutToPolicy(routeTimeout *gatewayapi_v1.HTTPRouteTimeouts) (rtp *RouteTimeoutPolicy, rp *RetryPolicy, err error) { - rtp = &RouteTimeoutPolicy{} - if routeTimeout.Request != nil { - reqTimeout, err2 := timeout.Parse(string(*routeTimeout.Request)) - if err2 != nil { - return nil, nil, fmt.Errorf("HTTPRoute.Spec.Rules.Timeouts: Invalid value for Request is specified") - } - rtp.ResponseTimeout = reqTimeout - } - - br := &contour_api_v1.RetryPolicy{} - if routeTimeout.BackendRequest != nil { - br.PerTryTimeout = string(*routeTimeout.BackendRequest) - } - - rp = retryPolicy(br) - if rp.PerTryTimeout.Duration() > rtp.ResponseTimeout.Duration() { - rp.PerTryTimeout = rtp.ResponseTimeout - } - - rp.RetryOn = "" - return - -} - -func timeoutsHTTPProute(prefix string, timeouts *gatewayapi_v1.HTTPRouteTimeouts, first *Service, rest ...*Service) *Route { - services := append([]*Service{first}, rest...) - - rtp, rp, _ := httpRouteTimeoutToPolicy(timeouts) - - return &Route{ - PathMatchCondition: prefixString(prefix), - Clusters: clustersWeight(services...), - TimeoutPolicy: *rtp, - RetryPolicy: rp, - } -} - func prefixrouteHTTPRoute(prefix string, first *Service, rest ...*Service) *Route { services := append([]*Service{first}, rest...) return &Route{ @@ -16371,11 +16327,18 @@ func withMirror(r *Route, mirrors []*Service, weight int64) *Route { } func makeHTTPRouteTimeouts(request, backendRequest string) *gatewayapi_v1.HTTPRouteTimeouts { - return &gatewayapi_v1.HTTPRouteTimeouts{ - Request: ref.To(gatewayapi_v1.Duration(request)), - BackendRequest: ref.To(gatewayapi_v1.Duration(backendRequest)), + httpRouteTimeouts := &gatewayapi_v1.HTTPRouteTimeouts{} + + if request != "" { + httpRouteTimeouts.Request = ref.To(gatewayapi_v1.Duration(request)) } + if backendRequest != "" { + httpRouteTimeouts.BackendRequest = ref.To(gatewayapi_v1.Duration(backendRequest)) + } + + return httpRouteTimeouts } + func makeHTTPRoute(request, backendRequest string) *gatewayapi_v1beta1.HTTPRoute { return &gatewayapi_v1beta1.HTTPRoute{ ObjectMeta: metav1.ObjectMeta{ diff --git a/internal/dag/gatewayapi_processor.go b/internal/dag/gatewayapi_processor.go index bc3f9e8ba1e..9aa722374dc 100644 --- a/internal/dag/gatewayapi_processor.go +++ b/internal/dag/gatewayapi_processor.go @@ -14,17 +14,18 @@ package dag import ( + "errors" "fmt" "net/http" "regexp" "strings" "time" - contour_api_v1 "github.com/projectcontour/contour/apis/projectcontour/v1" "github.com/projectcontour/contour/internal/gatewayapi" "github.com/projectcontour/contour/internal/k8s" "github.com/projectcontour/contour/internal/ref" "github.com/projectcontour/contour/internal/status" + "github.com/projectcontour/contour/internal/timeout" "github.com/sirupsen/logrus" v1 "k8s.io/api/core/v1" @@ -1107,36 +1108,27 @@ func (p *GatewayAPIProcessor) resolveRouteRefs(route any, routeAccessor *status. // *NOTE: the Gateway API does not yet support retries, so do we. // please refer to https://github.com/projectcontour/contour/pull/5940#discussion_r1385198654 // and related discussions for more information. for more information. -func parseHTTPRouteTimeouts(timeout *gatewayapi_v1.HTTPRouteTimeouts) (*RouteTimeoutPolicy, *RetryPolicy, error) { - if timeout == nil || (timeout.BackendRequest == nil && timeout.Request == nil) { - return nil, nil, nil +func parseHTTPRouteTimeouts(httpRouteTimeouts *gatewayapi_v1.HTTPRouteTimeouts) (*RouteTimeoutPolicy, error) { + if httpRouteTimeouts == nil { + return nil, nil } - - tr := &contour_api_v1.TimeoutPolicy{} - if timeout.Request != nil { - tr.Response = string(*timeout.Request) - } - - rtp, _, err := timeoutPolicy(tr, 0) - if err != nil { - return nil, nil, fmt.Errorf("HTTPRoute.Spec.Rules.Timeouts: Invalid value for Request is specified") + if httpRouteTimeouts.BackendRequest != nil { + return nil, errors.New("HTTPRoute.Spec.Rules.Timeouts.BackendRequest is not supported") } - - br := &contour_api_v1.RetryPolicy{} - if timeout.BackendRequest != nil { - br.PerTryTimeout = string(*timeout.BackendRequest) + if httpRouteTimeouts.Request == nil { + return nil, nil } - rp := retryPolicy(br) - if rp.PerTryTimeout.Duration() > rtp.ResponseTimeout.Duration() { - rp.PerTryTimeout = rtp.ResponseTimeout + requestTimeout, err := timeout.Parse(string(*httpRouteTimeouts.Request)) + if err != nil { + return nil, fmt.Errorf("invalid HTTPRoute.Spec.Rules.Timeouts.Request: %v", err) } - // retry disabled - rp.RetryOn = "" - return &rtp, rp, nil - + return &RouteTimeoutPolicy{ + ResponseTimeout: requestTimeout, + }, nil } + func (p *GatewayAPIProcessor) computeHTTPRouteForListener(route *gatewayapi_v1beta1.HTTPRoute, routeAccessor *status.RouteParentStatusUpdate, listener *listenerInfo, hosts sets.Set[string]) bool { var programmed bool for ruleIndex, rule := range route.Spec.Rules { @@ -1179,11 +1171,9 @@ func (p *GatewayAPIProcessor) computeHTTPRouteForListener(route *gatewayapi_v1be // Process rule-level filters. var ( - err error - redirect *Redirect - urlRewriteHostname string - - retryPolicy *RetryPolicy + err error + redirect *Redirect + urlRewriteHostname string mirrorPolicies []*MirrorPolicy requestHeaderPolicy *HeadersPolicy responseHeaderPolicy *HeadersPolicy @@ -1191,7 +1181,7 @@ func (p *GatewayAPIProcessor) computeHTTPRouteForListener(route *gatewayapi_v1be requestTimeoutPolicy *RouteTimeoutPolicy ) - requestTimeoutPolicy, retryPolicy, err = parseHTTPRouteTimeouts(rule.Timeouts) + requestTimeoutPolicy, err = parseHTTPRouteTimeouts(rule.Timeouts) if err != nil { routeAccessor.AddCondition(gatewayapi_v1beta1.RouteConditionAccepted, metav1.ConditionFalse, gatewayapi_v1beta1.RouteReasonUnsupportedValue, err.Error()) continue @@ -1414,8 +1404,7 @@ func (p *GatewayAPIProcessor) computeHTTPRouteForListener(route *gatewayapi_v1be responseHeaderPolicy, mirrorPolicies, pathRewritePolicy, - requestTimeoutPolicy, - retryPolicy) + requestTimeoutPolicy) } // Add each route to the relevant vhost(s)/svhosts(s). @@ -1556,13 +1545,11 @@ func (p *GatewayAPIProcessor) computeGRPCRouteForListener(route *gatewayapi_v1al KindGRPCRoute, route.Namespace, route.Name, - requestHeaderPolicy, responseHeaderPolicy, mirrorPolicies, nil, nil, - nil, ) // Add each route to the relevant vhost(s)/svhosts(s). @@ -2146,7 +2133,6 @@ func (p *GatewayAPIProcessor) clusterRoutes( mirrorPolicies []*MirrorPolicy, pathRewritePolicy *PathRewritePolicy, timeoutPolicy *RouteTimeoutPolicy, - retryPolicy *RetryPolicy, ) []*Route { var routes []*Route @@ -2170,7 +2156,6 @@ func (p *GatewayAPIProcessor) clusterRoutes( MirrorPolicies: mirrorPolicies, Priority: priority, PathRewritePolicy: pathRewritePolicy, - RetryPolicy: retryPolicy, } if timeoutPolicy != nil { route.TimeoutPolicy = *timeoutPolicy diff --git a/internal/dag/status_test.go b/internal/dag/status_test.go index 6c7ea8da92e..9c3c84392f4 100644 --- a/internal/dag/status_test.go +++ b/internal/dag/status_test.go @@ -9222,7 +9222,7 @@ func TestGatewayAPIHTTPRouteDAGStatus(t *testing.T) { wantGatewayStatusUpdate: validGatewayStatusUpdate("http", gatewayapi_v1.HTTPProtocolType, 1), }) - run(t, "route rule with timeouts", testcase{ + run(t, "route rule with timeouts.request and timeouts.backendRequest specified", testcase{ objs: []any{ kuardService, &gatewayapi_v1beta1.HTTPRoute{ @@ -9256,15 +9256,15 @@ func TestGatewayAPIHTTPRouteDAGStatus(t *testing.T) { ParentRef: gatewayapi.GatewayParentRef("projectcontour", "contour"), Conditions: []metav1.Condition{ routeResolvedRefsCondition(), - routeAcceptedHTTPRouteCondition(), + routeAcceptedFalse(gatewayapi_v1.RouteReasonUnsupportedValue, "HTTPRoute.Spec.Rules.Timeouts.BackendRequest is not supported"), }, }, }, }}, - wantGatewayStatusUpdate: validGatewayStatusUpdate("http", gatewayapi_v1.HTTPProtocolType, 1), + wantGatewayStatusUpdate: validGatewayStatusUpdate("http", gatewayapi_v1.HTTPProtocolType, 0), }) - run(t, "route rule with timeouts.request empty", testcase{ + run(t, "route rule with only timeouts.backendRequest specified", testcase{ objs: []any{ kuardService, &gatewayapi_v1beta1.HTTPRoute{ @@ -9297,12 +9297,12 @@ func TestGatewayAPIHTTPRouteDAGStatus(t *testing.T) { ParentRef: gatewayapi.GatewayParentRef("projectcontour", "contour"), Conditions: []metav1.Condition{ routeResolvedRefsCondition(), - routeAcceptedHTTPRouteCondition(), + routeAcceptedFalse(gatewayapi_v1.RouteReasonUnsupportedValue, "HTTPRoute.Spec.Rules.Timeouts.BackendRequest is not supported"), }, }, }, }}, - wantGatewayStatusUpdate: validGatewayStatusUpdate("http", gatewayapi_v1.HTTPProtocolType, 1), + wantGatewayStatusUpdate: validGatewayStatusUpdate("http", gatewayapi_v1.HTTPProtocolType, 0), }) run(t, "timeouts with invalid request for httproute", testcase{ @@ -9335,7 +9335,7 @@ func TestGatewayAPIHTTPRouteDAGStatus(t *testing.T) { ParentRef: gatewayapi.GatewayParentRef("projectcontour", "contour"), Conditions: []metav1.Condition{ routeResolvedRefsCondition(), - routeAcceptedFalse(gatewayapi_v1.RouteReasonUnsupportedValue, "HTTPRoute.Spec.Rules.Timeouts: Invalid value for Request is specified"), + routeAcceptedFalse(gatewayapi_v1.RouteReasonUnsupportedValue, "invalid HTTPRoute.Spec.Rules.Timeouts.Request: unable to parse timeout string \"invalid\": time: invalid duration \"invalid\""), }, }, },