fail validation if baseInterval is 0s (envoyproxy#5176)

* fail validation if baseInterval is 0s Fixes: envoyproxy#5147 Signed-off-by: Arko Dasgupta <arko@tetrate.io> * more validations Signed-off-by: Arko Dasgupta <arko@tetrate.io> --------- Signed-off-by: Arko Dasgupta <arko@tetrate.io>
guydc · Jan 31, 2025 · 9c04aaa · 9c04aaa
1 parent cd3fe13
commit 9c04aaa
Show file tree

Hide file tree

Showing 4 changed files with 280 additions and 14 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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: