From 4a31b1528e9f76af6af1a3f6753e5d2019973fc8 Mon Sep 17 00:00:00 2001
From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com>
Date: Tue, 22 Dec 2020 20:29:00 +0200
Subject: [PATCH] fix(kumactl) backoff durations validation (#1352) (#1353)

There were two bugs, one was happening when there was only seconds in
timeout durations provided (`1s`) as the validator was not checking if
seconds are provided and assuming that if nanoseconds field is empty, it
means the value is equal 0 and then rejecting valid values.

The second bug was that we were not validating `maxInterval` values of
the backOff properties

Signed-off-by: Bart Smykla <bartek@smykla.com>
(cherry picked from commit a7c1f39946d8192756b7868168fdb34fb8748744)

Co-authored-by: Bart Smykla <bartek@smykla.com>
---
 .../resources/apis/mesh/retry_validator.go    | 21 ++++--
 .../apis/mesh/retry_validator_test.go         | 74 +++++++++++++++++++
 2 files changed, 88 insertions(+), 7 deletions(-)

diff --git a/pkg/core/resources/apis/mesh/retry_validator.go b/pkg/core/resources/apis/mesh/retry_validator.go
index bb34a39b02ce..44cbe0cd6015 100644
--- a/pkg/core/resources/apis/mesh/retry_validator.go
+++ b/pkg/core/resources/apis/mesh/retry_validator.go
@@ -107,7 +107,7 @@ func validateDuration_GreaterThan0(
 	path validators.PathBuilder,
 	duration *duration.Duration,
 ) (err validators.ValidationError) {
-	if duration.Nanos == 0 {
+	if duration.Seconds == 0 && duration.Nanos == 0 {
 		err.AddViolationAt(path, HasToBeGreaterThan0Violation)
 	}
 
@@ -121,7 +121,7 @@ func validateDuration_GreaterThan0OrNil(
 		return
 	}
 
-	if duration.Nanos == 0 {
+	if duration.Seconds == 0 && duration.Nanos == 0 {
 		err.AddViolationAt(path, WhenDefinedHasToBeGreaterThan0Violation)
 	}
 
@@ -141,11 +141,18 @@ func validateConfProtocolBackOff(
 			path.Field("baseInterval"),
 			HasToBeDefinedViolation,
 		)
-	} else if conf.BaseInterval.Nanos == 0 {
-		err.Add(validateDuration_GreaterThan0(
-			path.Field("baseInterval"),
-			conf.BaseInterval,
-		))
+	} else {
+		if conf.BaseInterval.Seconds == 0 && conf.BaseInterval.Nanos == 0 {
+			err.Add(validateDuration_GreaterThan0(
+				path.Field("baseInterval"),
+				conf.BaseInterval,
+			))
+		} else {
+			err.Add(validateDuration_GreaterThan0OrNil(
+				path.Field("maxInterval"),
+				conf.MaxInterval,
+			))
+		}
 	}
 
 	return
diff --git a/pkg/core/resources/apis/mesh/retry_validator_test.go b/pkg/core/resources/apis/mesh/retry_validator_test.go
index 7d3f512355ba..5b2e9320296c 100644
--- a/pkg/core/resources/apis/mesh/retry_validator_test.go
+++ b/pkg/core/resources/apis/mesh/retry_validator_test.go
@@ -16,6 +16,11 @@ var _ = Describe("Retry", func() {
 			retry    string
 			expected string
 		}
+
+		type testCaseWithNoErrors struct {
+			retry string
+		}
+
 		DescribeTable("should validate all fields and return as much individual errors as possible",
 			func(given testCase) {
 				// setup
@@ -233,6 +238,27 @@ var _ = Describe("Retry", func() {
                 violations:
                 - field: conf.grpc.backOff.baseInterval
                   message: has to be greater than 0
+`,
+			}),
+			Entry("conf.grpc.backOff.maxInterval equal 0s", testCase{
+				retry: `
+                sources:
+                - match:
+                    kuma.io/service: web
+                    region: eu
+                destinations:
+                - match:
+                    kuma.io/service: backend
+                conf:
+                    grpc:
+                        backOff:
+                            baseInterval: 20ms
+                            maxInterval: 0s
+`,
+				expected: `
+                violations:
+                - field: conf.grpc.backOff.maxInterval
+                  message: has to be greater than 0 when defined
 `,
 			}),
 			Entry("empty conf.tcp", testCase{
@@ -251,6 +277,54 @@ var _ = Describe("Retry", func() {
                 violations:
                 - field: conf.tcp.maxConnectAttempts
                   message: has to be greater than 0
+`,
+			}),
+		)
+
+		DescribeTable("should validate all fields and return no errors if all are valid",
+			func(given testCaseWithNoErrors) {
+				// setup
+				retry := NewRetryResource()
+
+				// when
+				err := util_proto.FromYAML([]byte(given.retry), retry.Spec)
+				// then
+				Expect(err).ToNot(HaveOccurred())
+
+				// when
+				verr := retry.Validate()
+
+				// then
+				Expect(verr).To(BeNil())
+			},
+			Entry("all protocols configuration provided", testCaseWithNoErrors{
+				retry: `
+                sources:
+                - match:
+                    kuma.io/service: web
+                    region: eu
+                destinations:
+                - match:
+                    kuma.io/service: backend
+                conf:
+                    http:
+                        numRetries: 3
+                        perTryTimeout: 200ms
+                        backOff:
+                            baseInterval: 30ms
+                            maxInterval: 1.2s
+                        retriableStatusCodes: [501, 502]
+                    grpc:
+                        numRetries: 3
+                        perTryTimeout: 200ms
+                        backOff:
+                            baseInterval: 58ms
+                            maxInterval: 1s
+                        retryOn:
+                        - cancelled
+                        - unavailable
+                    tcp:
+                        maxConnectAttempts: 5
 `,
 			}),
 		)