From 820c33841434513dae2359ec4e923a2f878b3abd Mon Sep 17 00:00:00 2001 From: William Wernert Date: Mon, 21 Apr 2025 11:15:09 -0400 Subject: [PATCH] Alerting: Ensure field validators return the proper type (#104050) * Ensure field validators return the proper type This ensures correct error propagation through services up to the API layer. * Move error wrapping up to call site --- pkg/services/ngalert/models/alert_rule.go | 8 +- .../ngalert/models/alert_rule_test.go | 221 +++++++++++------- 2 files changed, 138 insertions(+), 91 deletions(-) diff --git a/pkg/services/ngalert/models/alert_rule.go b/pkg/services/ngalert/models/alert_rule.go index 6e02e1bef96..fb5ecad3970 100644 --- a/pkg/services/ngalert/models/alert_rule.go +++ b/pkg/services/ngalert/models/alert_rule.go @@ -642,7 +642,7 @@ func (alertRule *AlertRule) ValidateAlertRule(cfg setting.UnifiedAlertingSetting err = validateAlertRuleFields(alertRule) } if err != nil { - return err + return fmt.Errorf("%w: %s", ErrAlertRuleFailedValidation, err) } if alertRule.For < 0 { @@ -682,7 +682,7 @@ func validateAlertRuleFields(rule *AlertRule) error { } if rule.MissingSeriesEvalsToResolve != nil && *rule.MissingSeriesEvalsToResolve <= 0 { - return fmt.Errorf("%w: field `missing_series_evals_to_resolve` must be greater than 0", ErrAlertRuleFailedValidation) + return errors.New("field `missing_series_evals_to_resolve` must be greater than 0") } return nil @@ -691,10 +691,10 @@ func validateAlertRuleFields(rule *AlertRule) error { func validateRecordingRuleFields(rule *AlertRule) error { metricName := prommodels.LabelValue(rule.Record.Metric) if !metricName.IsValid() { - return fmt.Errorf("%w: %s", ErrAlertRuleFailedValidation, "metric name for recording rule must be a valid utf8 string") + return errors.New("metric name for recording rule must be a valid utf8 string") } if !prommodels.IsValidMetricName(metricName) { - return fmt.Errorf("%w: %s", ErrAlertRuleFailedValidation, "metric name for recording rule must be a valid Prometheus metric name") + return errors.New("metric name for recording rule must be a valid Prometheus metric name") } ClearRecordingRuleIgnoredFields(rule) diff --git a/pkg/services/ngalert/models/alert_rule_test.go b/pkg/services/ngalert/models/alert_rule_test.go index c9608663b26..5c702ddedec 100644 --- a/pkg/services/ngalert/models/alert_rule_test.go +++ b/pkg/services/ngalert/models/alert_rule_test.go @@ -1052,45 +1052,143 @@ func TestGeneratorFillsAllFields(t *testing.T) { } func TestValidateAlertRule(t *testing.T) { - testCases := []struct { - name string - keepFiringFor time.Duration - expectedErr error - }{ - { - name: "should accept zero keep firing for", - keepFiringFor: 0, - expectedErr: nil, - }, - { - name: "should accept positive keep firing for", - keepFiringFor: 1 * time.Minute, - expectedErr: nil, - }, - { - name: "should reject negative keep firing for", - keepFiringFor: -1 * time.Minute, - expectedErr: fmt.Errorf("%w: field `keep_firing_for` cannot be negative", ErrAlertRuleFailedValidation), - }, - } + t.Run("keepFiringFor", func(t *testing.T) { + testCases := []struct { + name string + keepFiringFor time.Duration + expectedErr error + }{ + { + name: "should accept zero keep firing for", + keepFiringFor: 0, + expectedErr: nil, + }, + { + name: "should accept positive keep firing for", + keepFiringFor: 1 * time.Minute, + expectedErr: nil, + }, + { + name: "should reject negative keep firing for", + keepFiringFor: -1 * time.Minute, + expectedErr: fmt.Errorf("%w: field `keep_firing_for` cannot be negative", ErrAlertRuleFailedValidation), + }, + } - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - rule := RuleGen.With( - RuleGen.WithKeepFiringFor(tc.keepFiringFor), - RuleGen.WithIntervalSeconds(10), - ).GenerateRef() + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + rule := RuleGen.With( + RuleGen.WithKeepFiringFor(tc.keepFiringFor), + RuleGen.WithIntervalSeconds(10), + ).GenerateRef() - err := rule.ValidateAlertRule(setting.UnifiedAlertingSettings{BaseInterval: 10 * time.Second}) + err := rule.ValidateAlertRule(setting.UnifiedAlertingSettings{BaseInterval: 10 * time.Second}) - if tc.expectedErr == nil { - require.NoError(t, err) - } else { - require.Error(t, err) - require.Equal(t, tc.expectedErr.Error(), err.Error()) - } - }) - } + if tc.expectedErr == nil { + require.NoError(t, err) + } else { + require.Error(t, err) + require.Equal(t, tc.expectedErr.Error(), err.Error()) + } + }) + } + }) + + t.Run("missingSeriesEvalsToResolve", func(t *testing.T) { + testCases := []struct { + name string + missingSeriesEvalsToResolve *int + expectedErrorContains string + }{ + { + name: "should allow nil value", + missingSeriesEvalsToResolve: nil, + }, + { + name: "should reject negative value", + missingSeriesEvalsToResolve: util.Pointer(-1), + expectedErrorContains: "field `missing_series_evals_to_resolve` must be greater than 0", + }, + { + name: "should reject 0", + missingSeriesEvalsToResolve: util.Pointer(0), + expectedErrorContains: "field `missing_series_evals_to_resolve` must be greater than 0", + }, + { + name: "should accept positive value", + missingSeriesEvalsToResolve: util.Pointer(2), + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + baseIntervalSeconds := int64(10) + cfg := setting.UnifiedAlertingSettings{ + BaseInterval: time.Duration(baseIntervalSeconds) * time.Second, + } + + rule := RuleGen.With( + RuleMuts.WithIntervalSeconds(baseIntervalSeconds * 2), + ).Generate() + rule.MissingSeriesEvalsToResolve = tc.missingSeriesEvalsToResolve + + err := rule.ValidateAlertRule(cfg) + + if tc.expectedErrorContains != "" { + require.Error(t, err) + require.ErrorIs(t, err, ErrAlertRuleFailedValidation) + require.Contains(t, err.Error(), tc.expectedErrorContains) + } else { + require.NoError(t, err) + } + }) + } + }) + + t.Run("ExecErrState & NoDataState", func(t *testing.T) { + testCases := []struct { + name string + execErrState string + noDataState string + error bool + }{ + { + name: "invalid error state", + execErrState: "invalid", + error: true, + }, + { + name: "invalid no data state", + noDataState: "invalid", + error: true, + }, + { + name: "valid states", + error: false, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + rule := RuleGen.With( + RuleMuts.WithIntervalSeconds(10), + ).Generate() + if tc.execErrState != "" { + rule.ExecErrState = ExecutionErrorState(tc.execErrState) + } + if tc.noDataState != "" { + rule.NoDataState = NoDataState(tc.noDataState) + } + + err := rule.ValidateAlertRule(setting.UnifiedAlertingSettings{BaseInterval: 10 * time.Second}) + if tc.error { + require.Error(t, err) + require.ErrorIs(t, err, ErrAlertRuleFailedValidation) + } else { + require.NoError(t, err) + } + }) + } + }) } func TestAlertRule_PrometheusRuleDefinition(t *testing.T) { @@ -1159,54 +1257,3 @@ func TestAlertRule_PrometheusRuleDefinition(t *testing.T) { }) } } - -func TestMissingSeriesEvalsToResolveValidation(t *testing.T) { - testCases := []struct { - name string - missingSeriesEvalsToResolve *int - expectedErrorContains string - }{ - { - name: "should allow nil value", - missingSeriesEvalsToResolve: nil, - }, - { - name: "should reject negative value", - missingSeriesEvalsToResolve: util.Pointer(-1), - expectedErrorContains: "field `missing_series_evals_to_resolve` must be greater than 0", - }, - { - name: "should reject 0", - missingSeriesEvalsToResolve: util.Pointer(0), - expectedErrorContains: "field `missing_series_evals_to_resolve` must be greater than 0", - }, - { - name: "should accept positive value", - missingSeriesEvalsToResolve: util.Pointer(2), - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - baseIntervalSeconds := int64(10) - cfg := setting.UnifiedAlertingSettings{ - BaseInterval: time.Duration(baseIntervalSeconds) * time.Second, - } - - rule := RuleGen.With( - RuleMuts.WithIntervalSeconds(baseIntervalSeconds * 2), - ).Generate() - rule.MissingSeriesEvalsToResolve = tc.missingSeriesEvalsToResolve - - err := rule.ValidateAlertRule(cfg) - - if tc.expectedErrorContains != "" { - require.Error(t, err) - require.ErrorIs(t, err, ErrAlertRuleFailedValidation) - require.Contains(t, err.Error(), tc.expectedErrorContains) - } else { - require.NoError(t, err) - } - }) - } -}