From 7391c1cc5220f10300b10d190f3304db746003ae Mon Sep 17 00:00:00 2001 From: Huabing Zhao Date: Wed, 16 Oct 2024 13:05:31 +0800 Subject: [PATCH] =?UTF-8?q?[release/v1.1]=20fix:=20don't=20lose=20timeout?= =?UTF-8?q?=20settings=20that=20originate=20from=20the=20route=20when=20t?= =?UTF-8?q?=E2=80=A6=20(#4450)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fix: don't lose timeout settings that originate from the route when translating the backend traffic policy (#4095) (#85) * Don't lose timeout settings that originate from the route when translating the backend traffic policy * Improve the readability of the timeout processing code. * Removed some logically unreachable code. --------- (cherry picked from commit 262e0466f14dace834f1b0d712c2492c27e9bb03) Signed-off-by: Lior Okman Signed-off-by: Huabing Zhao Co-authored-by: Lior Okman --- internal/gatewayapi/backendtrafficpolicy.go | 105 ++++++----- internal/gatewayapi/helpers.go | 7 + internal/gatewayapi/route.go | 15 +- ...afficpolicy-with-httproute-timeout.in.yaml | 50 +++++ ...fficpolicy-with-httproute-timeout.out.yaml | 172 ++++++++++++++++++ ...backendtrafficpolicy-with-timeout.out.yaml | 1 + 6 files changed, 286 insertions(+), 64 deletions(-) create mode 100644 internal/gatewayapi/testdata/backendtrafficpolicy-with-httproute-timeout.in.yaml create mode 100644 internal/gatewayapi/testdata/backendtrafficpolicy-with-httproute-timeout.out.yaml diff --git a/internal/gatewayapi/backendtrafficpolicy.go b/internal/gatewayapi/backendtrafficpolicy.go index ee47b76015c..9d4088ef3eb 100644 --- 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, nil); err != nil { err = perr.WithMessage(err, "Timeout") errs = errors.Join(errs, err) } @@ -411,6 +411,11 @@ 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 { + to = localTo + } + // Apply if there is a match if strings.HasPrefix(r.Name, prefix) { r.Traffic = &ir.TrafficFeatures{ @@ -423,18 +428,12 @@ func (t *Translator) translateBackendTrafficPolicyForRoute(policy *egv1a1.Backen TCPKeepalive: ka, Retry: rt, BackendConnection: bc, + Timeout: to, } // Update the Host field in HealthCheck, now that we have access to the Route Hostname. r.Traffic.HealthCheck.SetHTTPHostIfAbsent(r.Hostname) - // Some timeout setting originate from the route. - if policy.Spec.Timeout != nil { - if to, err = t.buildTimeout(policy, r); err == nil { - r.Traffic.Timeout = to - } - } - if policy.Spec.UseClientProtocol != nil { r.UseClientProtocol = policy.Spec.UseClientProtocol } @@ -498,7 +497,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, nil); err != nil { err = perr.WithMessage(err, "Timeout") errs = errors.Join(errs, err) } @@ -596,10 +595,8 @@ 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 policy.Spec.Timeout != nil { - if ct, err = t.buildTimeout(policy, r); err == nil { - r.Traffic.Timeout = ct - } + if ct, err = t.buildTimeout(*policy, r.Traffic); err == nil { + r.Traffic.Timeout = ct } if policy.Spec.UseClientProtocol != nil { @@ -1069,23 +1066,26 @@ func (t *Translator) buildCircuitBreaker(policy *egv1a1.BackendTrafficPolicy) (* return cb, nil } -func (t *Translator) buildTimeout(policy *egv1a1.BackendTrafficPolicy, r *ir.HTTPRoute) (*ir.Timeout, error) { +func (t *Translator) buildTimeout(policy egv1a1.BackendTrafficPolicy, traffic *ir.TrafficFeatures) (*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 ( - tto *ir.TCPTimeout - hto *ir.HTTPTimeout - terr bool errs error + to = &ir.Timeout{} + pto = policy.Spec.Timeout ) - pto := policy.Spec.Timeout - if pto.TCP != nil && pto.TCP.ConnectTimeout != nil { d, err := time.ParseDuration(string(*pto.TCP.ConnectTimeout)) if err != nil { - terr = true errs = errors.Join(errs, fmt.Errorf("invalid ConnectTimeout value %s", *pto.TCP.ConnectTimeout)) } else { - tto = &ir.TCPTimeout{ + to.TCP = &ir.TCPTimeout{ ConnectTimeout: ptr.To(metav1.Duration{Duration: d}), } } @@ -1098,7 +1098,6 @@ func (t *Translator) buildTimeout(policy *egv1a1.BackendTrafficPolicy, r *ir.HTT if pto.HTTP.ConnectionIdleTimeout != nil { d, err := time.ParseDuration(string(*pto.HTTP.ConnectionIdleTimeout)) if err != nil { - terr = true errs = errors.Join(errs, fmt.Errorf("invalid ConnectionIdleTimeout value %s", *pto.HTTP.ConnectionIdleTimeout)) } else { cit = ptr.To(metav1.Duration{Duration: d}) @@ -1108,51 +1107,51 @@ func (t *Translator) buildTimeout(policy *egv1a1.BackendTrafficPolicy, r *ir.HTT if pto.HTTP.MaxConnectionDuration != nil { d, err := time.ParseDuration(string(*pto.HTTP.MaxConnectionDuration)) if err != nil { - terr = true errs = errors.Join(errs, fmt.Errorf("invalid MaxConnectionDuration value %s", *pto.HTTP.MaxConnectionDuration)) } else { mcd = ptr.To(metav1.Duration{Duration: d}) } } - hto = &ir.HTTPTimeout{ + to.HTTP = &ir.HTTPTimeout{ ConnectionIdleTimeout: cit, MaxConnectionDuration: mcd, } } // http request timeout is translated during the gateway-api route resource translation - // merge route timeout setting with backendtrafficpolicy timeout settings - if terr { - if r != nil && r.Traffic != nil && r.Traffic.Timeout != nil { - return r.Traffic.Timeout.DeepCopy(), errs - } - } else { - // http request timeout is translated during the gateway-api route resource translation - // merge route timeout setting with backendtrafficpolicy timeout settings - if r != nil && - r.Traffic != nil && - r.Traffic.Timeout != nil && - r.Traffic.Timeout.HTTP != nil && - r.Traffic.Timeout.HTTP.RequestTimeout != nil { - if hto == nil { - hto = &ir.HTTPTimeout{ - RequestTimeout: r.Traffic.Timeout.HTTP.RequestTimeout, - } - } else { - hto.RequestTimeout = r.Traffic.Timeout.HTTP.RequestTimeout - } - } - - if hto != nil || tto != nil { - return &ir.Timeout{ - TCP: tto, - HTTP: hto, - }, nil - } + // 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 +} - return nil, 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) { diff --git a/internal/gatewayapi/helpers.go b/internal/gatewayapi/helpers.go index a6e13720e44..52df40f4736 100644 --- a/internal/gatewayapi/helpers.go +++ b/internal/gatewayapi/helpers.go @@ -580,3 +580,10 @@ 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 index b113411b3df..ce7fff56ae1 100644 --- a/internal/gatewayapi/route.go +++ b/internal/gatewayapi/route.go @@ -255,16 +255,9 @@ func (t *Translator) processHTTPRouteRules(httpRoute *HTTPRouteContext, parentRe return routeRoutes, nil } -func processTimeout(irRoute *ir.HTTPRoute, rule gwapiv1.HTTPRouteRule) { +func processRouteTimeout(irRoute *ir.HTTPRoute, rule gwapiv1.HTTPRouteRule) { if rule.Timeouts != nil { - var rto *ir.Timeout - - // Timeout is translated from multiple resources and may already be partially set - if irRoute.Traffic != nil && irRoute.Traffic.Timeout != nil { - rto = irRoute.Traffic.Timeout.DeepCopy() - } else { - rto = &ir.Timeout{} - } + rto := &ir.Timeout{} if rule.Timeouts.Request != nil { d, err := time.ParseDuration(string(*rule.Timeouts.Request)) @@ -309,7 +302,7 @@ func (t *Translator) processHTTPRouteRule(httpRoute *HTTPRouteContext, ruleIdx i irRoute := &ir.HTTPRoute{ Name: irRouteName(httpRoute, ruleIdx, -1), } - processTimeout(irRoute, rule) + processRouteTimeout(irRoute, rule) applyHTTPFiltersContextToIRRoute(httpFiltersContext, irRoute) ruleRoutes = append(ruleRoutes, irRoute) } @@ -321,7 +314,7 @@ func (t *Translator) processHTTPRouteRule(httpRoute *HTTPRouteContext, ruleIdx i irRoute := &ir.HTTPRoute{ Name: irRouteName(httpRoute, ruleIdx, matchIdx), } - processTimeout(irRoute, rule) + processRouteTimeout(irRoute, rule) if match.Path != nil { switch PathMatchTypeDerefOr(match.Path.Type, gwapiv1.PathMatchPathPrefix) { diff --git a/internal/gatewayapi/testdata/backendtrafficpolicy-with-httproute-timeout.in.yaml b/internal/gatewayapi/testdata/backendtrafficpolicy-with-httproute-timeout.in.yaml new file mode 100644 index 00000000000..e26f10c353f --- /dev/null +++ b/internal/gatewayapi/testdata/backendtrafficpolicy-with-httproute-timeout.in.yaml @@ -0,0 +1,50 @@ +gateways: +- apiVersion: gateway.networking.k8s.io/v1 + kind: Gateway + metadata: + namespace: envoy-gateway + name: gateway-1 + spec: + gatewayClassName: envoy-gateway-class + listeners: + - name: http + protocol: HTTP + port: 80 + allowedRoutes: + namespaces: + from: All +httpRoutes: +- apiVersion: gateway.networking.k8s.io/v1 + kind: HTTPRoute + metadata: + namespace: default + name: httproute-1 + 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 + metadata: + namespace: default + name: policy-for-http-route-1 + spec: + targetRef: + group: gateway.networking.k8s.io + kind: HTTPRoute + name: httproute-1 + useClientProtocol: true + diff --git a/internal/gatewayapi/testdata/backendtrafficpolicy-with-httproute-timeout.out.yaml b/internal/gatewayapi/testdata/backendtrafficpolicy-with-httproute-timeout.out.yaml new file mode 100644 index 00000000000..245739ca233 --- /dev/null +++ b/internal/gatewayapi/testdata/backendtrafficpolicy-with-httproute-timeout.out.yaml @@ -0,0 +1,172 @@ +backendTrafficPolicies: +- apiVersion: gateway.envoyproxy.io/v1alpha1 + kind: BackendTrafficPolicy + metadata: + creationTimestamp: null + name: policy-for-http-route-1 + namespace: default + spec: + targetRef: + group: gateway.networking.k8s.io + kind: HTTPRoute + name: httproute-1 + useClientProtocol: true + status: + ancestors: + - ancestorRef: + group: gateway.networking.k8s.io + kind: Gateway + name: gateway-1 + namespace: envoy-gateway + sectionName: http + conditions: + - lastTransitionTime: null + message: Policy has been accepted. + reason: Accepted + status: "True" + type: Accepted + controllerName: gateway.envoyproxy.io/gatewayclass-controller +gateways: +- apiVersion: gateway.networking.k8s.io/v1 + kind: Gateway + metadata: + creationTimestamp: null + name: gateway-1 + namespace: envoy-gateway + spec: + gatewayClassName: envoy-gateway-class + listeners: + - allowedRoutes: + namespaces: + from: All + name: http + port: 80 + protocol: HTTP + status: + listeners: + - attachedRoutes: 1 + conditions: + - lastTransitionTime: null + message: Sending translated listener configuration to the data plane + reason: Programmed + status: "True" + type: Programmed + - lastTransitionTime: null + message: Listener has been successfully translated + reason: Accepted + status: "True" + type: Accepted + - lastTransitionTime: null + message: Listener references have been resolved + reason: ResolvedRefs + status: "True" + type: ResolvedRefs + name: http + supportedKinds: + - group: gateway.networking.k8s.io + kind: HTTPRoute + - group: gateway.networking.k8s.io + kind: GRPCRoute +httpRoutes: +- apiVersion: gateway.networking.k8s.io/v1 + kind: HTTPRoute + metadata: + creationTimestamp: null + name: httproute-1 + 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: + listeners: + - address: null + name: envoy-gateway/gateway-1/http + ports: + - containerPort: 10080 + name: http-80 + protocol: HTTP + servicePort: 80 + metadata: + labels: + gateway.envoyproxy.io/owning-gateway-name: gateway-1 + gateway.envoyproxy.io/owning-gateway-namespace: envoy-gateway + name: envoy-gateway/gateway-1 +xdsIR: + envoy-gateway/gateway-1: + accessLog: + text: + - path: /dev/stdout + http: + - address: 0.0.0.0 + hostnames: + - '*' + isHTTP2: false + metadata: + kind: Gateway + name: gateway-1 + namespace: envoy-gateway + sectionName: http + name: envoy-gateway/gateway-1/http + path: + escapedSlashesAction: UnescapeAndRedirect + mergeSlashes: true + port: 10080 + routes: + - destination: + name: httproute/default/httproute-1/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-1 + namespace: default + name: httproute/default/httproute-1/rule/0/match/0/gateway_envoyproxy_io + pathMatch: + distinct: false + name: "" + prefix: / + traffic: + timeout: + http: + requestTimeout: 2m10s + useClientProtocol: true diff --git a/internal/gatewayapi/testdata/httproute-and-backendtrafficpolicy-with-timeout.out.yaml b/internal/gatewayapi/testdata/httproute-and-backendtrafficpolicy-with-timeout.out.yaml index 8852171648d..04843eba9aa 100644 --- a/internal/gatewayapi/testdata/httproute-and-backendtrafficpolicy-with-timeout.out.yaml +++ b/internal/gatewayapi/testdata/httproute-and-backendtrafficpolicy-with-timeout.out.yaml @@ -336,5 +336,6 @@ xdsIR: timeout: http: maxConnectionDuration: 22s + requestTimeout: 1s tcp: connectTimeout: 20s