Merge pull request #1860 from kube-logging/fix/prom-rules-override

fix: prometheus rule overrides swaps whole list

pkg/resources/fluentbit/prometheusrules.go

@@ -50,7 +50,7 @@ func (r *Reconciler) prometheusRules() (runtime.Object, reconciler.DesiredState,
 		rules := builtInRules
 		if r.fluentbitSpec.Metrics.PrometheusRulesOverride != nil {
 			for _, o := range r.fluentbitSpec.Metrics.PrometheusRulesOverride {
-				rules = o.ListOverride(builtInRules)
+				rules = o.ListOverride(rules)
 			}
 		}
 		state = reconciler.StatePresent

pkg/resources/fluentd/prometheusrules.go

@@ -137,7 +137,7 @@ func (r *Reconciler) prometheusRules() (runtime.Object, reconciler.DesiredState,
 		rules := builtInRules
 		if r.fluentdSpec.Metrics.PrometheusRulesOverride != nil {
 			for _, o := range r.fluentdSpec.Metrics.PrometheusRulesOverride {
-				rules = o.ListOverride(builtInRules)
+				rules = o.ListOverride(rules)
 			}
 		}
 		obj.Spec.Groups = []v1.RuleGroup{

pkg/resources/syslogng/prometheusrules.go

@@ -140,10 +140,9 @@ func (r *Reconciler) prometheusRules() (runtime.Object, reconciler.DesiredState,
 		rules := builtInRules
 		if r.syslogNGSpec.Metrics.PrometheusRulesOverride != nil {
 			for _, o := range r.syslogNGSpec.Metrics.PrometheusRulesOverride {
-				rules = o.ListOverride(builtInRules)
+				rules = o.ListOverride(rules)
 			}
 		}
-
 		obj.Spec.Groups = []v1.RuleGroup{
 			{
 				Name:  ruleGroupName,

pkg/sdk/logging/api/v1beta1/common_types.go

@@ -98,34 +98,38 @@ type PrometheusRulesOverride struct {
 	Annotations map[string]string `json:"annotations,omitempty"`
 }
 
-func (o PrometheusRulesOverride) ListOverride(l []v1.Rule) []v1.Rule {
-	var rules []v1.Rule
-	for _, i := range l {
-		rules = append(rules, *(o.Override(&i)))
+func (o PrometheusRulesOverride) ListOverride(listOfRules []v1.Rule) []v1.Rule {
+	var updatedRules []v1.Rule
+	for _, rule := range listOfRules {
+		if (o.Record != "" && o.Record == rule.Record) || (o.Alert != "" && o.Alert == rule.Alert) {
+			updatedRule := o.Override(&rule)
+			updatedRules = append(updatedRules, *updatedRule)
+		} else {
+			updatedRules = append(updatedRules, rule)
+		}
 	}
-	return rules
+
+	return updatedRules
 }
 
-func (o PrometheusRulesOverride) Override(r *v1.Rule) *v1.Rule {
-	mergedRule := r.DeepCopy()
-	if (o.Record != "" && o.Record == r.Record) || (o.Alert != "" && o.Alert == r.Alert) {
-		if o.Expr != nil {
-			mergedRule.Expr = *o.Expr
-		}
-		if o.For != nil {
-			mergedRule.For = o.For
-		}
-		if o.KeepFiringFor != nil {
-			mergedRule.KeepFiringFor = o.KeepFiringFor
-		}
-		if o.Labels != nil {
-			mergedRule.Labels = o.Labels
-		}
-		if o.Annotations != nil {
-			mergedRule.Annotations = o.Annotations
-		}
+func (o PrometheusRulesOverride) Override(rule *v1.Rule) *v1.Rule {
+	updatedRule := rule.DeepCopy()
+	if o.Expr != nil {
+		updatedRule.Expr = *o.Expr
+	}
+	if o.For != nil {
+		updatedRule.For = o.For
+	}
+	if o.KeepFiringFor != nil {
+		updatedRule.KeepFiringFor = o.KeepFiringFor
+	}
+	if o.Labels != nil {
+		updatedRule.Labels = o.Labels
+	}
+	if o.Annotations != nil {
+		updatedRule.Annotations = o.Annotations
 	}
-	return mergedRule
+	return updatedRule
 }
 
 // BufferMetrics defines the service monitor endpoints

pkg/sdk/logging/api/v1beta1/commont_types_test.go

@@ -27,113 +27,160 @@ func intstrRef(val string) *intstr.IntOrString {
 	return &x
 }
 
-var overrideTests = []struct {
-	name     string
-	rule     v1.Rule
-	override PrometheusRulesOverride
-	expected v1.Rule
-}{
-	{
-		name: "SeverityOverride",
-		rule: v1.Rule{
-			Alert:  "TestAlert",
-			Labels: map[string]string{"severity": "critical"},
-		},
-		override: PrometheusRulesOverride{
-			Alert:  "TestAlert",
-			Labels: map[string]string{"severity": "none"},
-		},
-		expected: v1.Rule{
-			Alert:  "TestAlert",
-			Labels: map[string]string{"severity": "none"},
-		},
-	},
-	{
-		name: "OverrideAlert2Mismatch",
-		rule: v1.Rule{
-			Alert:  "TestAlert",
-			Labels: map[string]string{"severity": "critical"},
-		},
-		override: PrometheusRulesOverride{
-			Alert:  "TestAlert2",
-			Labels: map[string]string{"severity": "none"},
-		},
-		expected: v1.Rule{
-			Alert:  "TestAlert",
-			Labels: map[string]string{"severity": "critical"},
-		},
-	},
-	{
-		name: "OverrideExpr",
-		rule: v1.Rule{
-			Alert:  "TestAlert",
-			Labels: map[string]string{"severity": "critical"},
-			Expr:   intstr.FromString("up > 0"),
-		},
-		override: PrometheusRulesOverride{
-			Alert: "TestAlert",
-			Expr:  intstrRef("up > 1"),
-		},
-		expected: v1.Rule{
-			Alert:  "TestAlert",
-			Labels: map[string]string{"severity": "critical"},
-			Expr:   intstr.FromString("up > 1"),
-		},
-	},
-}
-
-var overrideListTests = []struct {
-	name          string
-	rules         []v1.Rule
-	override      PrometheusRulesOverride
-	expectedRules []v1.Rule
-}{
-	{
-		name: "Alert2CriticalToNone",
-		rules: []v1.Rule{
-			{
+func TestMerge(t *testing.T) {
+	tests := []struct {
+		name     string
+		rule     v1.Rule
+		override PrometheusRulesOverride
+		expected v1.Rule
+	}{
+		{
+			name: "SeverityOverride",
+			rule: v1.Rule{
 				Alert:  "TestAlert",
 				Labels: map[string]string{"severity": "critical"},
 			},
-			{
-				Alert:  "TestAlert2",
-				Labels: map[string]string{"severity": "critical"},
+			override: PrometheusRulesOverride{
+				Alert:  "TestAlert",
+				Labels: map[string]string{"severity": "none"},
+			},
+			expected: v1.Rule{
+				Alert:  "TestAlert",
+				Labels: map[string]string{"severity": "none"},
 			},
 		},
-		override: PrometheusRulesOverride{
-			Alert:  "TestAlert2",
-			Labels: map[string]string{"severity": "none"},
-		},
-		expectedRules: []v1.Rule{
-			{
+		{
+			name: "OverrideExpr",
+			rule: v1.Rule{
 				Alert:  "TestAlert",
 				Labels: map[string]string{"severity": "critical"},
+				Expr:   intstr.FromString("up > 0"),
 			},
-			{
-				Alert:  "TestAlert2",
-				Labels: map[string]string{"severity": "none"},
+			override: PrometheusRulesOverride{
+				Alert: "TestAlert",
+				Expr:  intstrRef("up > 1"),
+			},
+			expected: v1.Rule{
+				Alert:  "TestAlert",
+				Labels: map[string]string{"severity": "critical"},
+				Expr:   intstr.FromString("up > 1"),
 			},
 		},
-	},
-}
+	}
 
-func TestMerge(t *testing.T) {
-	for _, tt := range overrideTests {
-		t.Run(tt.name, func(t *testing.T) {
-			actual := *(tt.override.Override(&tt.rule))
-			if !reflect.DeepEqual(actual, tt.expected) {
-				t.Fatalf("%v is not equal to %v", actual, tt.expected)
+	for _, tt := range tests {
+		ttp := tt
+		t.Run(ttp.name, func(t *testing.T) {
+			actual := *(ttp.override.Override(&ttp.rule))
+			if !reflect.DeepEqual(actual, ttp.expected) {
+				t.Fatalf("expected: %v, got: %v", ttp.expected, actual)
 			}
 		})
 	}
 }
 
 func TestListMerge(t *testing.T) {
-	for _, tt := range overrideListTests {
-		t.Run(tt.name, func(t *testing.T) {
-			actual := tt.override.ListOverride(tt.rules)
-			if !reflect.DeepEqual(actual, tt.expectedRules) {
-				t.Fatalf("%v is not equal to %v", actual, tt.expectedRules)
+	tests := []struct {
+		name          string
+		rules         []v1.Rule
+		overrides     []PrometheusRulesOverride
+		expectedRules []v1.Rule
+	}{
+		{
+			name: "Alert2CriticalToNone",
+			rules: []v1.Rule{
+				{
+					Alert:  "TestAlert",
+					Labels: map[string]string{"severity": "critical"},
+				},
+				{
+					Alert:  "TestAlert2",
+					Labels: map[string]string{"severity": "critical"},
+				},
+			},
+			overrides: []PrometheusRulesOverride{
+				{
+					Alert:  "TestAlert2",
+					Labels: map[string]string{"severity": "none"},
+				},
+			},
+			expectedRules: []v1.Rule{
+				{
+					Alert:  "TestAlert",
+					Labels: map[string]string{"severity": "critical"},
+				},
+				{
+					Alert:  "TestAlert2",
+					Labels: map[string]string{"severity": "none"},
+				},
+			},
+		},
+		{
+			name: "OverrideAlert2Mismatch",
+			rules: []v1.Rule{
+				{
+					Alert:  "TestAlert",
+					Labels: map[string]string{"severity": "critical"},
+				},
+			},
+			overrides: []PrometheusRulesOverride{
+				{
+					Alert:  "TestAlert2",
+					Labels: map[string]string{"severity": "none"},
+				},
+			},
+			expectedRules: []v1.Rule{
+				{
+					Alert:  "TestAlert",
+					Labels: map[string]string{"severity": "critical"},
+				},
+			},
+		},
+		{
+			name: "MultipleOverridesAppliedCorrectly",
+			rules: []v1.Rule{
+				{
+					Alert: "FluentdRetry",
+					Expr:  intstr.FromString("increase(fluentd_status_retry_count[10m]) > 5"),
+				},
+				{
+					Alert: "FluentdOutputError",
+					Expr:  intstr.FromString("increase(fluentd_output_status_num_errors[10m]) > 2"),
+				},
+			},
+			overrides: []PrometheusRulesOverride{
+				{
+					Alert: "FluentdRetry",
+					Expr:  intstrRef("increase(fluentd_status_retry_count[10m]) > 10"),
+				},
+				{
+					Alert: "FluentdOutputError",
+					Expr:  intstrRef("increase(fluentd_output_status_num_errors[10m]) > 5"),
+				},
+			},
+			expectedRules: []v1.Rule{
+				{
+					Alert: "FluentdRetry",
+					Expr:  intstr.FromString("increase(fluentd_status_retry_count[10m]) > 10"),
+				},
+				{
+					Alert: "FluentdOutputError",
+					Expr:  intstr.FromString("increase(fluentd_output_status_num_errors[10m]) > 5"),
+				},
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		ttp := tt
+		t.Run(ttp.name, func(t *testing.T) {
+			actual := ttp.rules
+			for _, o := range ttp.overrides {
+				actual = o.ListOverride(actual)
+			}
+
+			if !reflect.DeepEqual(actual, ttp.expectedRules) {
+				t.Fatalf("expected: %v, got: %v", ttp.expectedRules, actual)
 			}
 		})
 	}