Merge pull request #825 from pyrra-dev/validation-latency

kubernetes/api: Add validation for latency indicators
pyrra-dev · Jul 22, 2023 · 1f288e9 · 1f288e9
2 parents 16e9227 + e18ad63
commit 1f288e9
Show file tree

Hide file tree

Showing 2 changed files with 155 additions and 0 deletions.
diff --git a/kubernetes/api/v1alpha1/servicelevelobjective_types.go b/kubernetes/api/v1alpha1/servicelevelobjective_types.go
@@ -246,6 +246,57 @@ func (in *ServiceLevelObjective) validate() (admission.Warnings, error) {
 		}
 	}
 
+	if in.Spec.ServiceLevelIndicator.Latency != nil {
+		latency := in.Spec.ServiceLevelIndicator.Latency
+		if latency.Total.Metric == "" {
+			return warnings, fmt.Errorf("latency total metric must be set")
+		}
+		if latency.Success.Metric == "" {
+			return warnings, fmt.Errorf("latency success metric must be set")
+		}
+		if latency.Success.Metric == latency.Total.Metric {
+			warnings = append(warnings, "latency success metric should be different from latency total metric")
+		}
+
+		parsedTotal, err := parser.ParseExpr(latency.Total.Metric)
+		if err != nil {
+			return warnings, fmt.Errorf("failed to parse latency total metric: %w", err)
+		}
+		parsedSuccess, err := parser.ParseExpr(latency.Success.Metric)
+		if err != nil {
+			return warnings, fmt.Errorf("failed to parse latency success metric: %w", err)
+		}
+
+		switch parsedTotal.Type() {
+		case parser.ValueTypeVector:
+			v := parsedTotal.(*parser.VectorSelector)
+			if !strings.HasSuffix(v.Name, "_count") {
+				warnings = append(warnings, "latency total metric should usually be a histogram count")
+			}
+		}
+		switch parsedSuccess.Type() {
+		case parser.ValueTypeVector:
+			v := parsedSuccess.(*parser.VectorSelector)
+			var bucketFound bool
+			for _, matcher := range v.LabelMatchers {
+				if matcher.Name == labels.BucketLabel {
+					if _, err := strconv.ParseFloat(matcher.Value, 64); err != nil {
+						return warnings, fmt.Errorf("latency success metric must contain a le label matcher with a float value: %w", err)
+					}
+					bucketFound = true
+					break
+				}
+			}
+			if !bucketFound {
+				return warnings, fmt.Errorf("latency success metric must contain a le label matcher")
+			}
+
+			if !strings.HasSuffix(v.Name, "_bucket") {
+				warnings = append(warnings, "latency success metric should usually be a histogram bucket")
+			}
+		}
+	}
+
 	return warnings, nil
 }
 

diff --git a/kubernetes/api/v1alpha1/servicelevelobjective_types_test.go b/kubernetes/api/v1alpha1/servicelevelobjective_types_test.go
@@ -8,6 +8,7 @@ import (
 	"github.com/prometheus/prometheus/model/labels"
 	"github.com/stretchr/testify/require"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 	"sigs.k8s.io/yaml"
 
 	"github.com/pyrra-dev/pyrra/kubernetes/api/v1alpha1"
@@ -460,4 +461,107 @@ func TestServiceLevelObjective_Validate(t *testing.T) {
 			require.Nil(t, warn)
 		})
 	})
+
+	t.Run("latency", func(t *testing.T) {
+		latency := func() *v1alpha1.ServiceLevelObjective {
+			return &v1alpha1.ServiceLevelObjective{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "name",
+					Namespace: "namespace",
+				},
+				Spec: v1alpha1.ServiceLevelObjectiveSpec{
+					Target: "99",
+					Window: "2w",
+					ServiceLevelIndicator: v1alpha1.ServiceLevelIndicator{
+						Latency: &v1alpha1.LatencyIndicator{
+							Success:  v1alpha1.Query{Metric: `foo_bucket{foo="bar",le="1"}`},
+							Total:    v1alpha1.Query{Metric: `foo_count{foo="bar"}`},
+							Grouping: nil,
+						},
+					},
+				},
+			}
+		}
+
+		warn, err := latency().ValidateCreate()
+		require.NoError(t, err)
+		require.Nil(t, warn)
+
+		t.Run("empty", func(t *testing.T) {
+			latency := latency()
+			latency.Spec.ServiceLevelIndicator.Latency.Success.Metric = ""
+			latency.Spec.ServiceLevelIndicator.Latency.Total.Metric = ""
+			warn, err := latency.ValidateCreate()
+			require.EqualError(t, err, "latency total metric must be set")
+			require.Nil(t, warn)
+
+			latency.Spec.ServiceLevelIndicator.Latency.Success.Metric = ""
+			latency.Spec.ServiceLevelIndicator.Latency.Total.Metric = "foo"
+			warn, err = latency.ValidateCreate()
+			require.EqualError(t, err, "latency success metric must be set")
+			require.Nil(t, warn)
+
+			latency.Spec.ServiceLevelIndicator.Latency.Success.Metric = "foo"
+			latency.Spec.ServiceLevelIndicator.Latency.Total.Metric = ""
+			warn, err = latency.ValidateCreate()
+			require.EqualError(t, err, "latency total metric must be set")
+			require.Nil(t, warn)
+		})
+
+		t.Run("equal", func(t *testing.T) {
+			latency := latency()
+			latency.Spec.ServiceLevelIndicator.Latency.Success.Metric = "foo"
+			latency.Spec.ServiceLevelIndicator.Latency.Total.Metric = "foo"
+			warn, err := latency.ValidateCreate()
+			require.NotNil(t, warn)
+			require.Equal(t, "latency success metric should be different from latency total metric", warn[0])
+			require.Error(t, err)
+		})
+
+		t.Run("warnings", func(t *testing.T) {
+			latency := latency()
+			latency.Spec.ServiceLevelIndicator.Latency.Success.Metric = `foo{le="1"}`
+			latency.Spec.ServiceLevelIndicator.Latency.Total.Metric = "bar"
+			warn, err = latency.ValidateCreate()
+			require.NoError(t, err)
+			require.Equal(t,
+				admission.Warnings{
+					"latency total metric should usually be a histogram count",
+					"latency success metric should usually be a histogram bucket",
+				},
+				warn)
+		})
+
+		t.Run("invalidMetric", func(t *testing.T) {
+			latency := latency()
+			latency.Spec.ServiceLevelIndicator.Latency.Total.Metric = "foo{"
+			warn, err := latency.ValidateCreate()
+			require.EqualError(t, err, "failed to parse latency total metric: 1:5: parse error: unexpected end of input inside braces")
+			require.Nil(t, warn)
+
+			latency.Spec.ServiceLevelIndicator.Latency.Total.Metric = "foo}"
+			warn, err = latency.ValidateCreate()
+			require.EqualError(t, err, "failed to parse latency total metric: 1:4: parse error: unexpected character: '}'")
+			require.Nil(t, warn)
+
+			latency.Spec.ServiceLevelIndicator.Latency.Total.Metric = "$$$"
+			warn, err = latency.ValidateCreate()
+			require.EqualError(t, err, "failed to parse latency total metric: 1:1: parse error: unexpected character: '$'")
+			require.Nil(t, warn)
+
+			latency.Spec.ServiceLevelIndicator.Latency.Total.Metric = `foo{foo="bar'}`
+			warn, err = latency.ValidateCreate()
+			require.EqualError(t, err, "failed to parse latency total metric: 1:9: parse error: unterminated quoted string")
+			require.Nil(t, warn)
+
+			latency.Spec.ServiceLevelIndicator.Latency.Total.Metric = `foo{foo="bar"}`
+			latency.Spec.ServiceLevelIndicator.Latency.Success.Metric = `foo{foo="baz"}`
+			_, err = latency.ValidateCreate()
+			require.EqualError(t, err, "latency success metric must contain a le label matcher")
+
+			latency.Spec.ServiceLevelIndicator.Latency.Success.Metric = `foo{le="foo"}`
+			_, err = latency.ValidateCreate()
+			require.EqualError(t, err, `latency success metric must contain a le label matcher with a float value: strconv.ParseFloat: parsing "foo": invalid syntax`)
+		})
+	})
 }