feat(MeshRetry): allow setting numRetries to 0 to disable retries (#1…

…0250) Currently if you have any retry policy inherited from anywhere you wouldn't be able to remove the retries in a more specific policy. This enables users to set `{grpc,http}.numRetries = 0` in a more specific policy and remove the retry filter completely. It also disallows `tcp.maxConnectAttempt = 0` which would create an invalid envoy configuration. fix #10248 Signed-off-by: Charly Molter <charly.molter@konghq.com>
kumahq · May 22, 2024 · e0221c6 · e0221c6
1 parent 9970499
commit e0221c6
Show file tree

Hide file tree

Showing 7 changed files with 149 additions and 9 deletions.
diff --git a/UPGRADE.md b/UPGRADE.md
@@ -8,6 +8,13 @@ does not have any particular instructions.
 
 ## Upgrade to `2.8.x`
 
+### MeshRetry tcp.MaxConnectAttempt
+
+With [#10250](https://github.com/kumahq/kuma/pull/10250) `MeshRetry` policies with `spec.tcp.MaxConnectAttempt=0` will be rejected.
+Prior to 2.8.x these were semantically valid but would create invalid Envoy configuration and would cause issues on the dataplane.
+Now this is rejected sooner to avoid service disruption.
+
+
 ## Upgrade to `2.7.x`
 
 ### MeshMetric and cluster stats merging

diff --git a/pkg/plugins/policies/meshretry/api/v1alpha1/validator.go b/pkg/plugins/policies/meshretry/api/v1alpha1/validator.go
@@ -89,6 +89,8 @@ func validateTCP(tcp *TCP) validators.ValidationError {
 	path := validators.RootedAt("tcp")
 	if tcp.MaxConnectAttempt == nil {
 		verr.AddViolationAt(path, validators.MustNotBeEmpty)
+	} else if *tcp.MaxConnectAttempt == 0 {
+		verr.AddViolationAt(path.Field("maxConnectAttempt"), validators.HasToBeGreaterThanZero)
 	}
 	return verr
 }

diff --git a/pkg/plugins/policies/meshretry/api/v1alpha1/validator_test.go b/pkg/plugins/policies/meshretry/api/v1alpha1/validator_test.go
@@ -281,6 +281,22 @@ violations:
     message: must not be empty
   - field: spec.to[0].default.conf.grpc
     message: must not be empty`,
+			}),
+			Entry("0 for tcp.maxConnectAttempt", testCase{
+				inputYaml: `
+targetRef:
+  kind: Mesh
+to:
+  - targetRef:
+      kind: Mesh
+    default:
+      tcp:
+        maxConnectAttempt: 0
+`,
+				expected: `
+violations:
+  - field: spec.to[0].default.conf.tcp.maxConnectAttempt
+    message: must be greater than 0`,
 			}),
 			Entry("empty http.backOff", testCase{
 				inputYaml: `

diff --git a/pkg/plugins/policies/meshretry/plugin/v1alpha1/plugin_test.go b/pkg/plugins/policies/meshretry/plugin/v1alpha1/plugin_test.go
@@ -179,6 +179,27 @@ var _ = Describe("MeshRetry", func() {
 			},
 			goldenFilePrefix: "http",
 		}),
+		Entry("http retry 0 numRetries", testCase{
+			resources: []core_xds.Resource{{
+				Name:     "outbound",
+				Origin:   generator.OriginOutbound,
+				Resource: httpListenerWithSimpleRoute(10001),
+			}},
+			toRules: core_rules.ToRules{
+				Rules: []*core_rules.Rule{
+					{
+						Subset: core_rules.Subset{},
+						Conf: api.Conf{
+							HTTP: &api.HTTP{
+								NumRetries:    pointer.To[uint32](0),
+								PerTryTimeout: test.ParseDuration("2s"),
+							},
+						},
+					},
+				},
+			},
+			goldenFilePrefix: "http_0_numretries",
+		}),
 		Entry("grpc retry", testCase{
 			resources: []core_xds.Resource{{
 				Name:     "outbound",
@@ -227,6 +248,30 @@ var _ = Describe("MeshRetry", func() {
 			},
 			goldenFilePrefix: "grpc",
 		}),
+		Entry("grpc retry 0 numRetries", testCase{
+			resources: []core_xds.Resource{{
+				Name:     "outbound",
+				Origin:   generator.OriginOutbound,
+				Resource: httpListenerWithSimpleRoute(10002),
+			}},
+			toRules: core_rules.ToRules{
+				Rules: []*core_rules.Rule{
+					{
+						Subset: core_rules.Subset{core_rules.Tag{
+							Key:   mesh_proto.ServiceTag,
+							Value: "grpc-service",
+						}},
+						Conf: api.Conf{
+							GRPC: &api.GRPC{
+								NumRetries:    pointer.To[uint32](0),
+								PerTryTimeout: test.ParseDuration("12s"),
+							},
+						},
+					},
+				},
+			},
+			goldenFilePrefix: "grpc_0_numretries",
+		}),
 		Entry("tcp retry", testCase{
 			resources: []core_xds.Resource{{
 				Name:     "outbound",

diff --git a/...ugins/policies/meshretry/plugin/v1alpha1/testdata/grpc_0_numretries.listeners.golden.yaml b/...ugins/policies/meshretry/plugin/v1alpha1/testdata/grpc_0_numretries.listeners.golden.yaml
@@ -0,0 +1,34 @@
+address:
+  socketAddress:
+    address: 127.0.0.1
+    portValue: 10002
+filterChains:
+- filters:
+  - name: envoy.filters.network.http_connection_manager
+    typedConfig:
+      '@type': type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager
+      httpFilters:
+      - name: envoy.filters.http.router
+        typedConfig:
+          '@type': type.googleapis.com/envoy.extensions.filters.http.router.v3.Router
+      routeConfig:
+        name: outbound:backend
+        requestHeadersToAdd:
+        - header:
+            key: x-kuma-tags
+            value: '&kuma.io/service=web&'
+        validateClusters: false
+        virtualHosts:
+        - domains:
+          - '*'
+          name: backend
+          routes:
+          - match:
+              prefix: /
+            name: 9Zuf5Tg79OuZcQITwBbQykxAk2u4fRKrwYn3//AL4Yo=
+            route:
+              cluster: backend
+              timeout: 0s
+      statPrefix: outbound_127_0_0_1_10002
+name: outbound:127.0.0.1:10002
+trafficDirection: OUTBOUND
diff --git a/...ugins/policies/meshretry/plugin/v1alpha1/testdata/http_0_numretries.listeners.golden.yaml b/...ugins/policies/meshretry/plugin/v1alpha1/testdata/http_0_numretries.listeners.golden.yaml
@@ -0,0 +1,34 @@
+address:
+  socketAddress:
+    address: 127.0.0.1
+    portValue: 10001
+filterChains:
+- filters:
+  - name: envoy.filters.network.http_connection_manager
+    typedConfig:
+      '@type': type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager
+      httpFilters:
+      - name: envoy.filters.http.router
+        typedConfig:
+          '@type': type.googleapis.com/envoy.extensions.filters.http.router.v3.Router
+      routeConfig:
+        name: outbound:backend
+        requestHeadersToAdd:
+        - header:
+            key: x-kuma-tags
+            value: '&kuma.io/service=web&'
+        validateClusters: false
+        virtualHosts:
+        - domains:
+          - '*'
+          name: backend
+          routes:
+          - match:
+              prefix: /
+            name: 9Zuf5Tg79OuZcQITwBbQykxAk2u4fRKrwYn3//AL4Yo=
+            route:
+              cluster: backend
+              timeout: 0s
+      statPrefix: outbound_127_0_0_1_10001
+name: outbound:127.0.0.1:10001
+trafficDirection: OUTBOUND
diff --git a/pkg/plugins/policies/meshretry/plugin/xds/configurer.go b/pkg/plugins/policies/meshretry/plugin/xds/configurer.go
@@ -96,13 +96,9 @@ func (c *Configurer) getRouteRetryConfig(conf *api.Conf) (*envoy_route.RetryPoli
 	}
 	switch c.Protocol {
 	case "http":
-		policy, err := genHttpRetryPolicy(conf.HTTP)
-		if err != nil {
-			return nil, err
-		}
-		return policy, nil
+		return genHttpRetryPolicy(conf.HTTP)
 	case "grpc":
-		return genGrpcRetryPolicy(conf.GRPC), nil
+		return genGrpcRetryPolicy(conf.GRPC)
 	default:
 		return nil, nil
 	}
@@ -123,9 +119,9 @@ func GrpcRetryOn(conf *[]api.GRPCRetryOn) string {
 	return strings.Join(retryOn, ",")
 }
 
-func genGrpcRetryPolicy(conf *api.GRPC) *envoy_route.RetryPolicy {
+func genGrpcRetryPolicy(conf *api.GRPC) (*envoy_route.RetryPolicy, error) {
 	if conf == nil {
-		return nil
+		return nil, nil
 	}
 
 	policy := envoy_route.RetryPolicy{
@@ -137,6 +133,9 @@ func genGrpcRetryPolicy(conf *api.GRPC) *envoy_route.RetryPolicy {
 	}
 
 	if conf.NumRetries != nil {
+		if *conf.NumRetries == 0 { // If numRetries is 0 just don't configure retries
+			return nil, nil
+		}
 		policy.NumRetries = util_proto.UInt32(*conf.NumRetries)
 	}
 
@@ -160,7 +159,7 @@ func genGrpcRetryPolicy(conf *api.GRPC) *envoy_route.RetryPolicy {
 		policy.RateLimitedRetryBackOff = configureRateLimitedRetryBackOff(conf.RateLimitedBackOff)
 	}
 
-	return &policy
+	return &policy, nil
 }
 
 func genHttpRetryPolicy(conf *api.HTTP) (*envoy_route.RetryPolicy, error) {
@@ -177,6 +176,9 @@ func genHttpRetryPolicy(conf *api.HTTP) (*envoy_route.RetryPolicy, error) {
 	}
 
 	if conf.NumRetries != nil {
+		if *conf.NumRetries == 0 { // If numRetries is 0 just don't configure retries
+			return nil, nil
+		}
 		policy.NumRetries = util_proto.UInt32(*conf.NumRetries)
 	}