feat: added new validator hasValidSourceTenants (#39)

* feat: added new validator hasValidSourceTenants Signed-off-by: Martin Chodur <[email protected]> * fix: human readable validations Signed-off-by: Martin Chodur <[email protected]> * chore: changelog Signed-off-by: Martin Chodur <[email protected]> --------- Signed-off-by: Martin Chodur <[email protected]>
FUSAKLA · Dec 6, 2023 · 329def6 · 329def6
1 parent 7d2edd1
commit 329def6
Show file tree

Hide file tree

Showing 11 changed files with 71 additions and 13 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -13,6 +13,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Added examples of the human-readable validation descriptions to the examples dir.
 - Refactored the validation so it can use also group to validate the context of the rule.
 - Improve linting and fix all the linting issues.
+- Added new validator `hasValidSourceTenants`, see its [docs](docs/validations.md#hasvalidsourcetenants).
+- Improved wording in the human readable validation output.
 
 ## [v2.6.0] - 2023-12-06
 - Added new validator `expressionWithNoMetricName`, see its [docs](docs/validations.md#expressionwithnometricname). Thanks @tizki !

diff --git a/docs/validations.md b/docs/validations.md
@@ -34,6 +34,7 @@
     - [`forIsNotLongerThan`](#forisnotlongerthan)
   - [Others](#others)
     - [`hasSourceTenantsForMetrics`](#hassourcetenantsformetrics)
+    - [`hasValidSourceTenants`](#hasvalidsourcetenants)
 
 ## Labels
 
@@ -303,3 +304,12 @@ params:
     # Example:
     # k8s: "kube_.*|container_.*"
 ```
+
+### `hasValidSourceTenants`
+
+Fails if the rule group has other than than the configured source tenants.
+
+```yaml
+params:
+  allowedSourceTenants: [ "foo", "bar" ]
+```
diff --git a/examples/human_readable.html b/examples/human_readable.html
@@ -37,12 +37,13 @@ <h2><a href="#check-prometheus-limitations">check-prometheus-limitations</a></h2
 
   <h2><a href="#check-source-tenants">check-source-tenants</a></h2>
 	  <ul>
-		<li>All rules verifies if the rule group, the rule belongs to, has the required source_tenants configured, according to the mapping of metric names to tenants: <code>k8s</code>:<code>^container_.*|kube_.*$</code></li>
+		<li>All rules rule group, the rule belongs to, has the required <code>source_tenants</code> configured, according to the mapping of metric names to tenants: <code>k8s</code>:<code>^container_.*|kube_.*$</code></li>
+		<li>All rules rule group, the rule belongs to, does not have other <code>source_tenants</code> than: <code>tenant1</code>, <code>tenant2</code>, <code>k8s</code></li>
 	  </ul>
 
   <h2><a href="#check-metric-name">check-metric-name</a></h2>
 	  <ul>
-		<li>Alert expression with no metric name</li>
+		<li>Alert expression uses metric name in selectors</li>
 	  </ul>
 
   <h2><a href="#another-checks">another-checks</a></h2>

diff --git a/examples/human_readable.md b/examples/human_readable.md
@@ -26,10 +26,11 @@ Validation rules:
     - All rules does not use any of the `cluster`,`locality`,`prometheus-type`,`replica` labels is in its expression
 
   check-source-tenants
-    - All rules verifies if the rule group, the rule belongs to, has the required source_tenants configured, according to the mapping of metric names to tenants: `k8s`:`^container_.*|kube_.*$`
+    - All rules rule group, the rule belongs to, has the required `source_tenants` configured, according to the mapping of metric names to tenants: `k8s`:`^container_.*|kube_.*$`
+    - All rules rule group, the rule belongs to, does not have other `source_tenants` than: `tenant1`, `tenant2`, `k8s`
 
   check-metric-name
-    - Alert expression with no metric name
+    - Alert expression uses metric name in selectors
 
   another-checks
     - All rules labels does not have empty values

diff --git a/examples/validation.yaml b/examples/validation.yaml
@@ -77,6 +77,9 @@ validationRules:
         params:
           sourceTenants:
             k8s: "container_.*|kube_.*"
+      - type: hasAllowedSourceTenants
+        params:
+          allowedSourceTenants: ["tenant1", "tenant2", "k8s"]
 
   - name: check-metric-name
     scope: Alert

diff --git a/pkg/validator/alert.go b/pkg/validator/alert.go
@@ -28,7 +28,7 @@ type forIsNotLongerThan struct {
 }
 
 func (h forIsNotLongerThan) String() string {
-	return fmt.Sprintf("alert `for` is not longer than `%s`", h.limit)
+	return fmt.Sprintf("`for` is not longer than `%s`", h.limit)
 }
 
 func (h forIsNotLongerThan) Validate(_ unmarshaler.RuleGroup, rule rulefmt.Rule, _ *prometheus.Client) []error {

diff --git a/pkg/validator/annotations.go b/pkg/validator/annotations.go
@@ -269,7 +269,7 @@ type annotationIsValidPromQL struct {
 }
 
 func (h annotationIsValidPromQL) String() string {
-	return fmt.Sprintf("Annotation `%s` is a valid PromQL expression", h.annotation)
+	return fmt.Sprintf("annotation `%s` is a valid PromQL expression", h.annotation)
 }
 
 func (h annotationIsValidPromQL) Validate(_ unmarshaler.RuleGroup, rule rulefmt.Rule, _ *prometheus.Client) []error {
@@ -294,7 +294,7 @@ func newValidateAnnotationTemplates(paramsConfig yaml.Node) (Validator, error) {
 type validateAnnotationTemplates struct{}
 
 func (h validateAnnotationTemplates) String() string {
-	return "Annotations are valid templates"
+	return "annotations are valid templates"
 }
 
 func (h validateAnnotationTemplates) Validate(_ unmarshaler.RuleGroup, rule rulefmt.Rule, _ *prometheus.Client) []error {

diff --git a/pkg/validator/config.go b/pkg/validator/config.go
@@ -38,6 +38,7 @@ var registeredValidators = map[string]validatorCreator{
 	"expressionSelectorsMatchesAnything":   newExpressionSelectorsMatchesAnything,
 	"expressionWithNoMetricName":           newExpressionWithNoMetricName,
 	"hasSourceTenantsForMetrics":           newHasSourceTenantsForMetrics,
+	"hasAllowedSourceTenants":              newHasAllowedSourceTenants,
 }
 
 func NewFromConfig(config config.ValidatorConfig) (Validator, error) {

diff --git a/pkg/validator/expression.go b/pkg/validator/expression.go
@@ -121,7 +121,7 @@ type expressionDoesNotUseRangeShorterThan struct {
 }
 
 func (h expressionDoesNotUseRangeShorterThan) String() string {
-	return fmt.Sprintf("expr does not use range selector shorter than `%s`", h.limit)
+	return fmt.Sprintf("expression does not use range selector shorter than `%s`", h.limit)
 }
 
 func (h expressionDoesNotUseRangeShorterThan) Validate(_ unmarshaler.RuleGroup, rule rulefmt.Rule, _ *prometheus.Client) []error {
@@ -153,7 +153,7 @@ func newExpressionDoesNotUseIrate(_ yaml.Node) (Validator, error) {
 type expressionDoesNotUseIrate struct{}
 
 func (h expressionDoesNotUseIrate) String() string {
-	return "expr does not use irate"
+	return "expression does not use irate"
 }
 
 func (h expressionDoesNotUseIrate) Validate(_ unmarshaler.RuleGroup, rule rulefmt.Rule, _ *prometheus.Client) []error {
@@ -190,7 +190,7 @@ type validFunctionsOnCounters struct {
 }
 
 func (h validFunctionsOnCounters) String() string {
-	msg := "functions `rate` and `increase` used only on metrics with the `_total` suffix"
+	msg := "uses functions `rate` and `increase` only on metrics with the `_total` suffix"
 	if h.allowHistograms {
 		msg += " (metrics ending with _count are exceptions since those are used by histograms)"
 	}
@@ -234,7 +234,7 @@ func newRateBeforeAggregation(_ yaml.Node) (Validator, error) {
 type rateBeforeAggregation struct{}
 
 func (h rateBeforeAggregation) String() string {
-	return "never use aggregation functions before the `rate` or `increase` functions, see https://www.robustperception.io/rate-then-sum-never-sum-then-rate"
+	return "does not use aggregation functions before the `rate` or `increase` functions, see https://www.robustperception.io/rate-then-sum-never-sum-then-rate"
 }
 
 func (h rateBeforeAggregation) Validate(_ unmarshaler.RuleGroup, rule rulefmt.Rule, _ *prometheus.Client) []error {
@@ -399,7 +399,7 @@ func newExpressionWithNoMetricName(paramsConfig yaml.Node) (Validator, error) {
 type expressionWithNoMetricName struct{}
 
 func (e expressionWithNoMetricName) String() string {
-	return "expression with no metric name"
+	return "expression uses metric name in selectors"
 }
 
 func (e expressionWithNoMetricName) Validate(_ unmarshaler.RuleGroup, rule rulefmt.Rule, _ *prometheus.Client) []error {

diff --git a/pkg/validator/others.go b/pkg/validator/others.go
@@ -42,7 +42,7 @@ func (h hasSourceTenantsForMetrics) String() string {
 	for tenant, metricsRegexp := range h.sourceTenants {
 		tenantStrings = append(tenantStrings, fmt.Sprintf("`%s`:`%s`", tenant, metricsRegexp.String()))
 	}
-	return fmt.Sprintf("verifies if the rule group, the rule belongs to, has the required source_tenants configured, according to the mapping of metric names to tenants: %s", strings.Join(tenantStrings, ", "))
+	return fmt.Sprintf("rule group, the rule belongs to, has the required `source_tenants` configured, according to the mapping of metric names to tenants: %s", strings.Join(tenantStrings, ", "))
 }
 
 func (h hasSourceTenantsForMetrics) Validate(group unmarshaler.RuleGroup, rule rulefmt.Rule, _ *prometheus.Client) []error {
@@ -61,3 +61,35 @@ func (h hasSourceTenantsForMetrics) Validate(group unmarshaler.RuleGroup, rule r
 	}
 	return errs
 }
+
+// TODO this validation should happen just once per rule group, but for simplicity it is done per rule leading to multiple errors for the same rule group.
+func newHasAllowedSourceTenants(paramsConfig yaml.Node) (Validator, error) {
+	params := struct {
+		AllowedSourceTenants []string `yaml:"allowedSourceTenants"`
+	}{}
+	if err := paramsConfig.Decode(&params); err != nil {
+		return nil, err
+	}
+	return &hasAllowedSourceTenants{allowedSourceTenants: params.AllowedSourceTenants}, nil
+}
+
+type hasAllowedSourceTenants struct {
+	allowedSourceTenants []string
+}
+
+func (h hasAllowedSourceTenants) String() string {
+	return fmt.Sprintf("rule group, the rule belongs to, does not have other `source_tenants` than: `%s`", strings.Join(h.allowedSourceTenants, "`, `"))
+}
+
+func (h hasAllowedSourceTenants) Validate(group unmarshaler.RuleGroup, _ rulefmt.Rule, _ *prometheus.Client) []error {
+	var invalidTenants []string
+	for _, tenant := range group.SourceTenants {
+		if !slices.Contains(h.allowedSourceTenants, tenant) {
+			invalidTenants = append(invalidTenants, tenant)
+		}
+	}
+	if len(invalidTenants) == 0 {
+		return []error{}
+	}
+	return []error{fmt.Errorf("invalid source_tenants: `%s`", strings.Join(invalidTenants, "`,`"))}
+}
diff --git a/pkg/validator/validator_test.go b/pkg/validator/validator_test.go
@@ -199,6 +199,14 @@ var testCases = []struct {
 	{name: "usesMetricWithSourceTenantAndGroupDoesNotHaveSourceTenant", validator: hasSourceTenantsForMetrics{sourceTenants: map[string]*regexp.Regexp{"tenant1": regexp.MustCompile(`^teanant1_metric$`)}}, group: unmarshaler.RuleGroup{SourceTenants: []string{"tenant2"}}, rule: rulefmt.Rule{Expr: `teanant1_metric{foo="bar"}`}, expectedErrors: 1},
 	{name: "usesMetricWithSourceTenantAndGroupHasMultipleSourceTenants", validator: hasSourceTenantsForMetrics{sourceTenants: map[string]*regexp.Regexp{"tenant1": regexp.MustCompile(`^teanant1_metric$`)}}, group: unmarshaler.RuleGroup{SourceTenants: []string{"tenant2", "tenant1"}}, rule: rulefmt.Rule{Expr: `teanant1_metric{foo="bar"}`}, expectedErrors: 0},
 	{name: "usesMetricWithSourceTenantAndGroupHasMultipleSourceTenantsAndOneIsMissing", validator: hasSourceTenantsForMetrics{sourceTenants: map[string]*regexp.Regexp{"tenant1": regexp.MustCompile(`^teanant1_metric$`)}}, group: unmarshaler.RuleGroup{SourceTenants: []string{"tenant2", "tenant3"}}, rule: rulefmt.Rule{Expr: `teanant1_metric{foo="bar"}`}, expectedErrors: 1},
+
+	// hasAllowedSourceTenants
+	{name: "emptyAllowedSourceTenantsAndGroupSourceTenants", validator: hasAllowedSourceTenants{allowedSourceTenants: []string{}}, group: unmarshaler.RuleGroup{SourceTenants: []string{}}, rule: rulefmt.Rule{Expr: `up{foo="bar"}`}, expectedErrors: 0},
+	{name: "emptyAllowedSourceTenantsAndGroupSourceTenantsWithOneTenant", validator: hasAllowedSourceTenants{allowedSourceTenants: []string{}}, group: unmarshaler.RuleGroup{SourceTenants: []string{"tenant1"}}, rule: rulefmt.Rule{Expr: `up{foo="bar"}`}, expectedErrors: 1},
+	{name: "emptyAllowedSourceTenantsAndGroupSourceTenantsWithMultipleTenants", validator: hasAllowedSourceTenants{allowedSourceTenants: []string{}}, group: unmarshaler.RuleGroup{SourceTenants: []string{"tenant1", "tenant2"}}, rule: rulefmt.Rule{Expr: `up{foo="bar"}`}, expectedErrors: 1},
+	{name: "allowedSourceTenantsAndGroupSourceTenantsWithOneTenant", validator: hasAllowedSourceTenants{allowedSourceTenants: []string{"tenant1"}}, group: unmarshaler.RuleGroup{SourceTenants: []string{"tenant1"}}, rule: rulefmt.Rule{Expr: `up{foo="bar"}`}, expectedErrors: 0},
+	{name: "allowedSourceTenantsAndGroupSourceTenantsWithMultipleTenants", validator: hasAllowedSourceTenants{allowedSourceTenants: []string{"tenant1", "tenant2"}}, group: unmarshaler.RuleGroup{SourceTenants: []string{"tenant1", "tenant2"}}, rule: rulefmt.Rule{Expr: `up{foo="bar"}`}, expectedErrors: 0},
+	{name: "allowedSourceTenantsAndGroupSourceTenantsWithMultipleTenantsAndOneIsMissing", validator: hasAllowedSourceTenants{allowedSourceTenants: []string{"tenant1", "tenant2"}}, group: unmarshaler.RuleGroup{SourceTenants: []string{"tenant1", "tenant3"}}, rule: rulefmt.Rule{Expr: `up{foo="bar"}`}, expectedErrors: 1},
 }
 
 func Test(t *testing.T) {