From 9c04aaae6252db3f7ec1d71bf071becb9dac743e Mon Sep 17 00:00:00 2001 From: Arko Dasgupta Date: Thu, 30 Jan 2025 14:15:31 -0800 Subject: [PATCH] fail validation if baseInterval is 0s (#5176) * fail validation if baseInterval is 0s Fixes: https://github.com/envoyproxy/gateway/issues/5147 Signed-off-by: Arko Dasgupta * more validations Signed-off-by: Arko Dasgupta --------- Signed-off-by: Arko Dasgupta --- internal/gatewayapi/backendtrafficpolicy.go | 14 +- internal/gatewayapi/clustersettings.go | 26 ++- .../backendtrafficpolicy-with-retries.in.yaml | 69 ++++++- ...backendtrafficpolicy-with-retries.out.yaml | 185 +++++++++++++++++- 4 files changed, 280 insertions(+), 14 deletions(-) diff --git a/internal/gatewayapi/backendtrafficpolicy.go b/internal/gatewayapi/backendtrafficpolicy.go index 3bdc92d4728..16d00920c58 100644 --- a/internal/gatewayapi/backendtrafficpolicy.go +++ b/internal/gatewayapi/backendtrafficpolicy.go @@ -333,9 +333,12 @@ func (t *Translator) translateBackendTrafficPolicyForRoute( err = perr.WithMessage(err, "TCPKeepalive") errs = errors.Join(errs, err) } - if policy.Spec.Retry != nil { - rt = buildRetry(policy.Spec.Retry) + + if rt, err = buildRetry(policy.Spec.Retry); err != nil { + err = perr.WithMessage(err, "Retry") + errs = errors.Join(errs, err) } + if to, err = buildClusterSettingsTimeout(policy.Spec.ClusterSettings); err != nil { err = perr.WithMessage(err, "Timeout") errs = errors.Join(errs, err) @@ -484,9 +487,12 @@ func (t *Translator) translateBackendTrafficPolicyForGateway( err = perr.WithMessage(err, "TCPKeepalive") errs = errors.Join(errs, err) } - if policy.Spec.Retry != nil { - rt = buildRetry(policy.Spec.Retry) + + if rt, err = buildRetry(policy.Spec.Retry); err != nil { + err = perr.WithMessage(err, "Retry") + errs = errors.Join(errs, err) } + if ct, err = buildClusterSettingsTimeout(policy.Spec.ClusterSettings); err != nil { err = perr.WithMessage(err, "Timeout") errs = errors.Join(errs, err) diff --git a/internal/gatewayapi/clustersettings.go b/internal/gatewayapi/clustersettings.go index 5706504f42b..76d3e1aeaff 100644 --- a/internal/gatewayapi/clustersettings.go +++ b/internal/gatewayapi/clustersettings.go @@ -72,7 +72,10 @@ func translateTrafficFeatures(policy *egv1a1.ClusterSettings) (*ir.TrafficFeatur ret.HTTP2 = h2 } - ret.Retry = buildRetry(policy.Retry) + var err error + if ret.Retry, err = buildRetry(policy.Retry); err != nil { + return nil, err + } // If nothing was set in any of the above calls, return nil instead of an empty // container @@ -477,9 +480,9 @@ func translateDNS(policy egv1a1.ClusterSettings) *ir.DNS { } } -func buildRetry(r *egv1a1.Retry) *ir.Retry { +func buildRetry(r *egv1a1.Retry) (*ir.Retry, error) { if r == nil { - return nil + return nil, nil } rt := &ir.Retry{} @@ -518,13 +521,22 @@ func buildRetry(r *egv1a1.Retry) *ir.Retry { if r.PerRetry.BackOff != nil { if r.PerRetry.BackOff.MaxInterval != nil || r.PerRetry.BackOff.BaseInterval != nil { bop := &ir.BackOffPolicy{} + if r.PerRetry.BackOff.BaseInterval != nil { + bop.BaseInterval = r.PerRetry.BackOff.BaseInterval + if bop.BaseInterval.Duration == 0 { + return nil, fmt.Errorf("baseInterval cannot be set to 0s") + } + } if r.PerRetry.BackOff.MaxInterval != nil { bop.MaxInterval = r.PerRetry.BackOff.MaxInterval + if bop.MaxInterval.Duration == 0 { + return nil, fmt.Errorf("maxInterval cannot be set to 0s") + } + if bop.BaseInterval != nil && bop.BaseInterval.Duration > bop.MaxInterval.Duration { + return nil, fmt.Errorf("maxInterval cannot be less than baseInterval") + } } - if r.PerRetry.BackOff.BaseInterval != nil { - bop.BaseInterval = r.PerRetry.BackOff.BaseInterval - } pr.BackOff = bop bpr = true } @@ -535,5 +547,5 @@ func buildRetry(r *egv1a1.Retry) *ir.Retry { } } - return rt + return rt, nil } diff --git a/internal/gatewayapi/testdata/backendtrafficpolicy-with-retries.in.yaml b/internal/gatewayapi/testdata/backendtrafficpolicy-with-retries.in.yaml index be4649ab425..bf2e264bf27 100644 --- a/internal/gatewayapi/testdata/backendtrafficpolicy-with-retries.in.yaml +++ b/internal/gatewayapi/testdata/backendtrafficpolicy-with-retries.in.yaml @@ -62,6 +62,44 @@ httpRoutes: backendRefs: - name: service-1 port: 8080 +- apiVersion: gateway.networking.k8s.io/v1 + kind: HTTPRoute + metadata: + namespace: default + name: httproute-2 + spec: + hostnames: + - gateway.envoyproxy.io + parentRefs: + - namespace: envoy-gateway + name: gateway-2 + sectionName: http + rules: + - matches: + - path: + value: "/route2" + backendRefs: + - name: service-1 + port: 8080 +- apiVersion: gateway.networking.k8s.io/v1 + kind: HTTPRoute + metadata: + namespace: default + name: httproute-3 + spec: + hostnames: + - gateway.envoyproxy.io + parentRefs: + - namespace: envoy-gateway + name: gateway-2 + sectionName: http + rules: + - matches: + - path: + value: "/route3" + backendRefs: + - name: service-1 + port: 8080 backendTrafficPolicies: - apiVersion: gateway.envoyproxy.io/v1alpha1 kind: BackendTrafficPolicy @@ -86,7 +124,7 @@ backendTrafficPolicies: kind: BackendTrafficPolicy metadata: namespace: default - name: policy-for-route + name: policy-for-route-1 spec: targetRef: group: gateway.networking.k8s.io @@ -106,3 +144,32 @@ backendTrafficPolicies: backoff: baseInterval: 100ms maxInterval: 10s +- apiVersion: gateway.envoyproxy.io/v1alpha1 + kind: BackendTrafficPolicy + metadata: + namespace: default + name: policy-for-route-2 + spec: + targetRef: + group: gateway.networking.k8s.io + kind: HTTPRoute + name: httproute-2 + retry: + perRetry: + backoff: + baseInterval: 0s +- apiVersion: gateway.envoyproxy.io/v1alpha1 + kind: BackendTrafficPolicy + metadata: + namespace: default + name: policy-for-route-3 + spec: + targetRef: + group: gateway.networking.k8s.io + kind: HTTPRoute + name: httproute-3 + retry: + perRetry: + backoff: + baseInterval: 2s + maxInterval: 1s diff --git a/internal/gatewayapi/testdata/backendtrafficpolicy-with-retries.out.yaml b/internal/gatewayapi/testdata/backendtrafficpolicy-with-retries.out.yaml index 40ae88b602d..2e3dd5bbbfc 100644 --- a/internal/gatewayapi/testdata/backendtrafficpolicy-with-retries.out.yaml +++ b/internal/gatewayapi/testdata/backendtrafficpolicy-with-retries.out.yaml @@ -3,7 +3,7 @@ backendTrafficPolicies: kind: BackendTrafficPolicy metadata: creationTimestamp: null - name: policy-for-route + name: policy-for-route-1 namespace: default spec: retry: @@ -39,6 +39,67 @@ backendTrafficPolicies: status: "True" type: Accepted controllerName: gateway.envoyproxy.io/gatewayclass-controller +- apiVersion: gateway.envoyproxy.io/v1alpha1 + kind: BackendTrafficPolicy + metadata: + creationTimestamp: null + name: policy-for-route-2 + namespace: default + spec: + retry: + perRetry: + backOff: + baseInterval: 0s + targetRef: + group: gateway.networking.k8s.io + kind: HTTPRoute + name: httproute-2 + status: + ancestors: + - ancestorRef: + group: gateway.networking.k8s.io + kind: Gateway + name: gateway-2 + namespace: envoy-gateway + sectionName: http + conditions: + - lastTransitionTime: null + message: 'Retry: baseInterval cannot be set to 0s.' + reason: Invalid + status: "False" + type: Accepted + controllerName: gateway.envoyproxy.io/gatewayclass-controller +- apiVersion: gateway.envoyproxy.io/v1alpha1 + kind: BackendTrafficPolicy + metadata: + creationTimestamp: null + name: policy-for-route-3 + namespace: default + spec: + retry: + perRetry: + backOff: + baseInterval: 2s + maxInterval: 1s + targetRef: + group: gateway.networking.k8s.io + kind: HTTPRoute + name: httproute-3 + status: + ancestors: + - ancestorRef: + group: gateway.networking.k8s.io + kind: Gateway + name: gateway-2 + namespace: envoy-gateway + sectionName: http + conditions: + - lastTransitionTime: null + message: 'Retry: maxInterval cannot be less than baseInterval.' + reason: Invalid + status: "False" + type: Accepted + controllerName: gateway.envoyproxy.io/gatewayclass-controller - apiVersion: gateway.envoyproxy.io/v1alpha1 kind: BackendTrafficPolicy metadata: @@ -131,7 +192,7 @@ gateways: protocol: HTTP status: listeners: - - attachedRoutes: 1 + - attachedRoutes: 3 conditions: - lastTransitionTime: null message: Sending translated listener configuration to the data plane @@ -227,6 +288,82 @@ httpRoutes: name: gateway-2 namespace: envoy-gateway sectionName: http +- apiVersion: gateway.networking.k8s.io/v1 + kind: HTTPRoute + metadata: + creationTimestamp: null + name: httproute-2 + namespace: default + spec: + hostnames: + - gateway.envoyproxy.io + parentRefs: + - name: gateway-2 + namespace: envoy-gateway + sectionName: http + rules: + - backendRefs: + - name: service-1 + port: 8080 + matches: + - path: + value: /route2 + 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-2 + namespace: envoy-gateway + sectionName: http +- apiVersion: gateway.networking.k8s.io/v1 + kind: HTTPRoute + metadata: + creationTimestamp: null + name: httproute-3 + namespace: default + spec: + hostnames: + - gateway.envoyproxy.io + parentRefs: + - name: gateway-2 + namespace: envoy-gateway + sectionName: http + rules: + - backendRefs: + - name: service-1 + port: 8080 + matches: + - path: + value: /route3 + 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-2 + namespace: envoy-gateway + sectionName: http infraIR: envoy-gateway/gateway-1: proxy: @@ -325,6 +462,50 @@ xdsIR: mergeSlashes: true port: 10080 routes: + - destination: + name: httproute/default/httproute-2/rule/0 + settings: + - addressType: IP + endpoints: + - host: 7.7.7.7 + port: 8080 + protocol: HTTP + weight: 1 + directResponse: + statusCode: 500 + hostname: gateway.envoyproxy.io + isHTTP2: false + metadata: + kind: HTTPRoute + name: httproute-2 + namespace: default + name: httproute/default/httproute-2/rule/0/match/0/gateway_envoyproxy_io + pathMatch: + distinct: false + name: "" + prefix: /route2 + - destination: + name: httproute/default/httproute-3/rule/0 + settings: + - addressType: IP + endpoints: + - host: 7.7.7.7 + port: 8080 + protocol: HTTP + weight: 1 + directResponse: + statusCode: 500 + hostname: gateway.envoyproxy.io + isHTTP2: false + metadata: + kind: HTTPRoute + name: httproute-3 + namespace: default + name: httproute/default/httproute-3/rule/0/match/0/gateway_envoyproxy_io + pathMatch: + distinct: false + name: "" + prefix: /route3 - destination: name: httproute/default/httproute-1/rule/0 settings: