fix(policy): improve validator messages, allow string failoverthresho…

…ld (#8929) * fix(policies): improve validator messages, allow string failoverthreshold - MeshFaultInjection were not using SI notation for bandwidth (using m for Mega) - Make error messages more coherent (use always "must" never "has to") - When intOrString is a not a number don't validate ranges too - MeshLocalityAwareLoadBalancing failoverthreshold was just ignoring string values Signed-off-by: Charly Molter <[email protected]>
kumahq · Jan 19, 2024 · e2692fc · e2692fc
1 parent dd29be8
commit e2692fc
Show file tree

Hide file tree

Showing 28 changed files with 98 additions and 114 deletions.
diff --git a/UPGRADE.md b/UPGRADE.md
@@ -38,6 +38,12 @@ To ensure a smooth transition to Kuma 2.6.0, carefully review your existing conf
 The postgres driver `postgres` (lib/pq) is deprecated and will be removed in the future.
 Please migrate to the new postgres driver `pgx` by setting `DriverName=pgx` configuration option or `KUMA_STORE_POSTGRES_DRIVER_NAME=pgx` env variable.
 
+### Make format SI valid for bandwidth in MeshFaultInjection policy
+
+Prior to this upgrade `mbps` and `gbps` were used for units for parameter `conf.responseBandwidth.percentage`.
+These are not valid units according to the [International System of Units](https://en.wikipedia.org/wiki/International_System_of_Units) they are respectively corrected to `Gbps` and `Mbps` if using
+these invalid units convert them into `kbps` prior to upgrade to avoid invalid format.
+
 ## Upgrade to `2.5.x`
 
 ### Transparent-proxy and CNI v1 removal

diff --git a/app/kumactl/cmd/install/testdata/install-control-plane.defaults.golden.yaml b/app/kumactl/cmd/install/testdata/install-control-plane.defaults.golden.yaml
@@ -312,7 +312,7 @@ spec:
                                 properties:
                                   limit:
                                     description: |-
-                                      Limit is represented by value measure in gbps, mbps, kbps or bps, e.g.
+                                      Limit is represented by value measure in Gbps, Mbps, kbps, e.g.
                                       10kbps
                                     type: string
                                   percentage:
@@ -488,7 +488,7 @@ spec:
                                 properties:
                                   limit:
                                     description: |-
-                                      Limit is represented by value measure in gbps, mbps, kbps or bps, e.g.
+                                      Limit is represented by value measure in Gbps, Mbps, kbps, e.g.
                                       10kbps
                                     type: string
                                   percentage:

diff --git a/app/kumactl/cmd/install/testdata/install-control-plane.gateway-api-present-not-enabled.yaml b/app/kumactl/cmd/install/testdata/install-control-plane.gateway-api-present-not-enabled.yaml
@@ -312,7 +312,7 @@ spec:
                                 properties:
                                   limit:
                                     description: |-
-                                      Limit is represented by value measure in gbps, mbps, kbps or bps, e.g.
+                                      Limit is represented by value measure in Gbps, Mbps, kbps, e.g.
                                       10kbps
                                     type: string
                                   percentage:
@@ -488,7 +488,7 @@ spec:
                                 properties:
                                   limit:
                                     description: |-
-                                      Limit is represented by value measure in gbps, mbps, kbps or bps, e.g.
+                                      Limit is represented by value measure in Gbps, Mbps, kbps, e.g.
                                       10kbps
                                     type: string
                                   percentage:

diff --git a/app/kumactl/cmd/install/testdata/install-control-plane.gateway-api-present.yaml b/app/kumactl/cmd/install/testdata/install-control-plane.gateway-api-present.yaml
@@ -312,7 +312,7 @@ spec:
                                 properties:
                                   limit:
                                     description: |-
-                                      Limit is represented by value measure in gbps, mbps, kbps or bps, e.g.
+                                      Limit is represented by value measure in Gbps, Mbps, kbps, e.g.
                                       10kbps
                                     type: string
                                   percentage:
@@ -488,7 +488,7 @@ spec:
                                 properties:
                                   limit:
                                     description: |-
-                                      Limit is represented by value measure in gbps, mbps, kbps or bps, e.g.
+                                      Limit is represented by value measure in Gbps, Mbps, kbps, e.g.
                                       10kbps
                                     type: string
                                   percentage:

diff --git a/app/kumactl/cmd/install/testdata/install-control-plane.with-helm-set.yaml b/app/kumactl/cmd/install/testdata/install-control-plane.with-helm-set.yaml
@@ -332,7 +332,7 @@ spec:
                                 properties:
                                   limit:
                                     description: |-
-                                      Limit is represented by value measure in gbps, mbps, kbps or bps, e.g.
+                                      Limit is represented by value measure in Gbps, Mbps, kbps, e.g.
                                       10kbps
                                     type: string
                                   percentage:
@@ -508,7 +508,7 @@ spec:
                                 properties:
                                   limit:
                                     description: |-
-                                      Limit is represented by value measure in gbps, mbps, kbps or bps, e.g.
+                                      Limit is represented by value measure in Gbps, Mbps, kbps, e.g.
                                       10kbps
                                     type: string
                                   percentage:

diff --git a/app/kumactl/cmd/install/testdata/install-crds.all.golden.yaml b/app/kumactl/cmd/install/testdata/install-crds.all.golden.yaml
@@ -1786,7 +1786,7 @@ spec:
                                 properties:
                                   limit:
                                     description: |-
-                                      Limit is represented by value measure in gbps, mbps, kbps or bps, e.g.
+                                      Limit is represented by value measure in Gbps, Mbps, kbps, e.g.
                                       10kbps
                                     type: string
                                   percentage:
@@ -1962,7 +1962,7 @@ spec:
                                 properties:
                                   limit:
                                     description: |-
-                                      Limit is represented by value measure in gbps, mbps, kbps or bps, e.g.
+                                      Limit is represented by value measure in Gbps, Mbps, kbps, e.g.
                                       10kbps
                                     type: string
                                   percentage:

diff --git a/app/kumactl/cmd/install/testdata/install-crds.experimental-gatewayapi.golden.yaml b/app/kumactl/cmd/install/testdata/install-crds.experimental-gatewayapi.golden.yaml
@@ -1786,7 +1786,7 @@ spec:
                                 properties:
                                   limit:
                                     description: |-
-                                      Limit is represented by value measure in gbps, mbps, kbps or bps, e.g.
+                                      Limit is represented by value measure in Gbps, Mbps, kbps, e.g.
                                       10kbps
                                     type: string
                                   percentage:
@@ -1962,7 +1962,7 @@ spec:
                                 properties:
                                   limit:
                                     description: |-
-                                      Limit is represented by value measure in gbps, mbps, kbps or bps, e.g.
+                                      Limit is represented by value measure in Gbps, Mbps, kbps, e.g.
                                       10kbps
                                     type: string
                                   percentage:

diff --git a/deployments/charts/kuma/crds/kuma.io_meshfaultinjections.yaml b/deployments/charts/kuma/crds/kuma.io_meshfaultinjections.yaml
@@ -115,7 +115,7 @@ spec:
                                 properties:
                                   limit:
                                     description: |-
-                                      Limit is represented by value measure in gbps, mbps, kbps or bps, e.g.
+                                      Limit is represented by value measure in Gbps, Mbps, kbps, e.g.
                                       10kbps
                                     type: string
                                   percentage:
@@ -291,7 +291,7 @@ spec:
                                 properties:
                                   limit:
                                     description: |-
-                                      Limit is represented by value measure in gbps, mbps, kbps or bps, e.g.
+                                      Limit is represented by value measure in Gbps, Mbps, kbps, e.g.
                                       10kbps
                                     type: string
                                   percentage:

diff --git a/docs/generated/openapi.yaml b/docs/generated/openapi.yaml
@@ -3524,7 +3524,7 @@ components:
                                 limit:
                                   description: >-
                                     Limit is represented by value measure in
-                                    gbps, mbps, kbps or bps, e.g.
+                                    Gbps, Mbps, kbps, e.g.
 
                                     10kbps
                                   type: string
@@ -3742,7 +3742,7 @@ components:
                                 limit:
                                   description: >-
                                     Limit is represented by value measure in
-                                    gbps, mbps, kbps or bps, e.g.
+                                    Gbps, Mbps, kbps, e.g.
 
                                     10kbps
                                   type: string

diff --git a/docs/generated/raw/crds/kuma.io_meshfaultinjections.yaml b/docs/generated/raw/crds/kuma.io_meshfaultinjections.yaml
@@ -115,7 +115,7 @@ spec:
                                 properties:
                                   limit:
                                     description: |-
-                                      Limit is represented by value measure in gbps, mbps, kbps or bps, e.g.
+                                      Limit is represented by value measure in Gbps, Mbps, kbps, e.g.
                                       10kbps
                                     type: string
                                   percentage:
@@ -291,7 +291,7 @@ spec:
                                 properties:
                                   limit:
                                     description: |-
-                                      Limit is represented by value measure in gbps, mbps, kbps or bps, e.g.
+                                      Limit is represented by value measure in Gbps, Mbps, kbps, e.g.
                                       10kbps
                                     type: string
                                   percentage:

diff --git a/pkg/core/validators/common_validators.go b/pkg/core/validators/common_validators.go
@@ -92,7 +92,7 @@ func ValidateIntPercentageOrNil(path PathBuilder, percentage *int32) ValidationE
 	return err
 }
 
-func ValidatePercentage(path PathBuilder, percentage *intstr.IntOrString) ValidationError {
+func ValidatePercentage(path PathBuilder, percentage *intstr.IntOrString, allow0 bool) ValidationError {
 	var verr ValidationError
 	if percentage == nil {
 		verr.AddViolationAt(path, MustBeDefined)
@@ -102,10 +102,14 @@ func ValidatePercentage(path PathBuilder, percentage *intstr.IntOrString) Valida
 	dec, err := common_api.NewDecimalFromIntOrString(*percentage)
 	if err != nil {
 		verr.AddViolationAt(path, StringHasToBeValidNumber)
+		// Return early to avoid spam of similar errors
+		return verr
 	}
 
 	if dec.LessThan(decimal.Zero) || dec.GreaterThan(decimal.NewFromInt(100)) {
 		verr.AddViolationAt(path, HasToBeInPercentageRange)
+	} else if !allow0 && dec.IsZero() {
+		verr.AddViolationAt(path, HasToBeGreaterThanZero)
 	}
 	return verr
 }
@@ -119,31 +123,14 @@ func ValidatePercentageOrNil(path PathBuilder, percentage *intstr.IntOrString) V
 	dec, err := common_api.NewDecimalFromIntOrString(*percentage)
 	if err != nil {
 		verr.AddViolationAt(path, StringHasToBeValidNumber)
+		return verr
 	}
-
 	if dec.LessThan(decimal.Zero) || dec.GreaterThan(decimal.NewFromInt(100)) {
 		verr.AddViolationAt(path, HasToBeInPercentageRange)
 	}
 	return verr
 }
 
-func ValidateIntOrStringGreaterThan(path PathBuilder, number *intstr.IntOrString, minValue int) ValidationError {
-	var verr ValidationError
-	if number == nil {
-		return verr
-	}
-
-	dec, err := common_api.NewDecimalFromIntOrString(*number)
-	if err != nil {
-		verr.AddViolationAt(path, StringHasToBeValidNumber)
-	}
-	if dec.LessThanOrEqual(decimal.NewFromInt(int64(minValue))) {
-		verr.AddViolationAt(path, fmt.Sprintf("%s: %d", HasToBeGreaterThan, minValue))
-	}
-
-	return verr
-}
-
 func ValidateIntOrStringGreaterOrEqualThan(path PathBuilder, number *intstr.IntOrString, minValue int) ValidationError {
 	var verr ValidationError
 	if number == nil {
@@ -159,23 +146,6 @@ func ValidateIntOrStringGreaterOrEqualThan(path PathBuilder, number *intstr.IntO
 	return verr
 }
 
-func ValidateIntOrStringLessThan(path PathBuilder, number *intstr.IntOrString, maxValue int) ValidationError {
-	var verr ValidationError
-	if number == nil {
-		return verr
-	}
-
-	dec, err := common_api.NewDecimalFromIntOrString(*number)
-	if err != nil {
-		verr.AddViolationAt(path, StringHasToBeValidNumber)
-	}
-	if dec.GreaterThanOrEqual(decimal.NewFromInt(int64(maxValue))) {
-		verr.AddViolationAt(path, fmt.Sprintf("%s: %d", HasToBeLessThan, maxValue))
-	}
-
-	return verr
-}
-
 func ValidateUInt32PercentageOrNil(path PathBuilder, percentage *uint32) ValidationError {
 	var err ValidationError
 	if percentage == nil {
@@ -215,7 +185,7 @@ func ValidatePathOrNil(path PathBuilder, filePath *string) ValidationError {
 func ValidateStatusCode(path PathBuilder, status int32) ValidationError {
 	var err ValidationError
 	if status < 100 || status >= 600 {
-		err.AddViolationAt(path, "must be in range [100, 600)")
+		err.AddViolationAt(path, fmt.Sprintf(HasToBeInRangeFormat, 100, 599))
 	}
 
 	return err
@@ -253,14 +223,16 @@ func ValidateIntegerGreaterThan(path PathBuilder, value uint32, minValue uint32)
 	return err
 }
 
+var BandwidthRegex = regexp.MustCompile(`(\d*)\s?([GMk]?bps)`)
+
 func ValidateBandwidth(path PathBuilder, value string) ValidationError {
 	var err ValidationError
 	if value == "" {
 		err.AddViolationAt(path, MustBeDefined)
 		return err
 	}
-	if matched, _ := regexp.MatchString(`\d*\s?[gmk]bps`, value); !matched {
-		err.AddViolationAt(path, "has to be in kbps/mbps/gbps units")
+	if matched := BandwidthRegex.MatchString(value); !matched {
+		err.AddViolationAt(path, MustHaveBPSUnit)
 	}
 	return err
 }

diff --git a/pkg/core/validators/messages.go b/pkg/core/validators/messages.go
@@ -18,10 +18,15 @@ const (
 	MustBeDefinedAndGreaterThanZero   = "must be defined and greater than zero"
 	WhenDefinedHasToBeNonNegative     = "must not be negative when defined"
 	WhenDefinedHasToBeGreaterThanZero = "must be greater than zero when defined"
-	HasToBeInPercentageRange          = "has to be in [0.0 - 100.0] range"
-	HasToBeInUintPercentageRange      = "has to be in [0 - 100] range"
-	WhenDefinedHasToBeValidPath       = "has to be a valid path when defined"
-	StringHasToBeValidNumber          = "string has to be a valid number"
+	HasToBeInRangeFormat              = "must be in inclusive range [%v, %v]"
+	WhenDefinedHasToBeValidPath       = "must be a valid path when defined"
+	StringHasToBeValidNumber          = "string must be a valid number"
+	MustHaveBPSUnit                   = "must be in kbps/Mbps/Gbps units"
+)
+
+var (
+	HasToBeInPercentageRange     = fmt.Sprintf(HasToBeInRangeFormat, "0.0", "100.0")
+	HasToBeInUintPercentageRange = fmt.Sprintf(HasToBeInRangeFormat, 0, 100)
 )
 
 func MustHaveOnlyOne(entity string, allowedValues ...string) string {

diff --git a/pkg/plugins/policies/meshcircuitbreaker/api/v1alpha1/validator_test.go b/pkg/plugins/policies/meshcircuitbreaker/api/v1alpha1/validator_test.go
@@ -414,9 +414,9 @@ to:
 				expected: `
 violations:
   - field: spec.to[0].default.outlierDetection.maxEjectionPercent
-    message: has to be in [0 - 100] range
+    message: must be in inclusive range [0, 100]
   - field: spec.to[0].default.outlierDetection.detectors.failurePercentage.threshold
-    message: has to be in [0 - 100] range`,
+    message: must be in inclusive range [0, 100]`,
 			}),
 			Entry("detectors are not defined", testCase{
 				inputYaml: `

diff --git a/pkg/plugins/policies/meshfaultinjection/api/v1alpha1/meshfaultinjection.go b/pkg/plugins/policies/meshfaultinjection/api/v1alpha1/meshfaultinjection.go
@@ -75,7 +75,7 @@ type DelayConf struct {
 }
 
 type ResponseBandwidthConf struct {
-	// Limit is represented by value measure in gbps, mbps, kbps or bps, e.g.
+	// Limit is represented by value measure in Gbps, Mbps, kbps, e.g.
 	// 10kbps
 	Limit string `json:"limit"`
 	// Percentage of requests on which response bandwidth limit will be

diff --git a/pkg/plugins/policies/meshfaultinjection/api/v1alpha1/schema.yaml b/pkg/plugins/policies/meshfaultinjection/api/v1alpha1/schema.yaml
@@ -76,7 +76,7 @@ properties:
                         properties:
                           limit:
                             description: |-
-                              Limit is represented by value measure in gbps, mbps, kbps or bps, e.g.
+                              Limit is represented by value measure in Gbps, Mbps, kbps, e.g.
                               10kbps
                             type: string
                           percentage:
@@ -244,7 +244,7 @@ properties:
                         properties:
                           limit:
                             description: |-
-                              Limit is represented by value measure in gbps, mbps, kbps or bps, e.g.
+                              Limit is represented by value measure in Gbps, Mbps, kbps, e.g.
                               10kbps
                             type: string
                           percentage:

diff --git a/pkg/plugins/policies/meshfaultinjection/api/v1alpha1/validator.go b/pkg/plugins/policies/meshfaultinjection/api/v1alpha1/validator.go
@@ -80,17 +80,17 @@ func validateDefault(path validators.PathBuilder, conf Conf) validators.Validati
 		if fault.Abort != nil {
 			path := path.Field("abort").Index(idx)
 			verr.Add(validators.ValidateStatusCode(path.Field("httpStatus"), fault.Abort.HttpStatus))
-			verr.Add(validators.ValidatePercentage(path.Field("percentage"), &fault.Abort.Percentage))
+			verr.Add(validators.ValidatePercentage(path.Field("percentage"), &fault.Abort.Percentage, true))
 		}
 		if fault.Delay != nil {
 			path := path.Field("delay").Index(idx)
 			verr.Add(validators.ValidateDurationNotNegative(path.Field("value"), &fault.Delay.Value))
-			verr.Add(validators.ValidatePercentage(path.Field("percentage"), &fault.Delay.Percentage))
+			verr.Add(validators.ValidatePercentage(path.Field("percentage"), &fault.Delay.Percentage, true))
 		}
 		if fault.ResponseBandwidth != nil {
 			path := path.Field("responseBandwidth").Index(idx)
 			verr.Add(validators.ValidateBandwidth(path.Field("responseBandwidth"), fault.ResponseBandwidth.Limit))
-			verr.Add(validators.ValidatePercentage(path.Field("percentage"), &fault.ResponseBandwidth.Percentage))
+			verr.Add(validators.ValidatePercentage(path.Field("percentage"), &fault.ResponseBandwidth.Percentage, true))
 		}
 	}
 	return verr