From a0f175c7a5d571145f707815d6e7ca971d979b50 Mon Sep 17 00:00:00 2001 From: David Parrott Date: Wed, 19 May 2021 09:05:32 -0700 Subject: [PATCH] also don't allow negative intervalseconds (#34319) --- pkg/services/ngalert/schedule/schedule_test.go | 18 ++++++++---------- pkg/services/ngalert/store/alert_rule.go | 4 ++-- .../api/alerting/api_alertmanager_test.go | 2 +- 3 files changed, 11 insertions(+), 13 deletions(-) diff --git a/pkg/services/ngalert/schedule/schedule_test.go b/pkg/services/ngalert/schedule/schedule_test.go index 7a6be93c55c..374435f4fb4 100644 --- a/pkg/services/ngalert/schedule/schedule_test.go +++ b/pkg/services/ngalert/schedule/schedule_test.go @@ -129,8 +129,6 @@ func TestAlertingTicker(t *testing.T) { t.Cleanup(registry.ClearOverrides) alerts := make([]*models.AlertRule, 0) - // create alert rule with zero interval (should never run) - alerts = append(alerts, tests.CreateTestAlertRule(t, dbstore, 0)) // create alert rule with one second interval alerts = append(alerts, tests.CreateTestAlertRule(t, dbstore, 1)) @@ -166,7 +164,7 @@ func TestAlertingTicker(t *testing.T) { }() runtime.Gosched() - expectedAlertRulesEvaluated := []models.AlertRuleKey{alerts[1].GetKey()} + expectedAlertRulesEvaluated := []models.AlertRuleKey{alerts[0].GetKey()} t.Run(fmt.Sprintf("on 1st tick alert rules: %s should be evaluated", concatenate(expectedAlertRulesEvaluated)), func(t *testing.T) { tick := advanceClock(t, mockedClock) assertEvalRun(t, evalAppliedCh, tick, expectedAlertRulesEvaluated...) @@ -174,10 +172,10 @@ func TestAlertingTicker(t *testing.T) { // change alert rule interval to three seconds var threeSecInterval int64 = 3 - alerts[0] = tests.UpdateTestAlertRuleIntervalSeconds(t, dbstore, alerts[0], threeSecInterval) - t.Logf("alert rule: %v interval reset to: %d", alerts[0].GetKey(), threeSecInterval) + alerts = append(alerts, tests.CreateTestAlertRule(t, dbstore, threeSecInterval)) + t.Logf("alert rule: %v added with interval: %d", alerts[1].GetKey(), threeSecInterval) - expectedAlertRulesEvaluated = []models.AlertRuleKey{alerts[1].GetKey()} + expectedAlertRulesEvaluated = []models.AlertRuleKey{alerts[0].GetKey()} t.Run(fmt.Sprintf("on 2nd tick alert rule: %s should be evaluated", concatenate(expectedAlertRulesEvaluated)), func(t *testing.T) { tick := advanceClock(t, mockedClock) assertEvalRun(t, evalAppliedCh, tick, expectedAlertRulesEvaluated...) @@ -189,13 +187,13 @@ func TestAlertingTicker(t *testing.T) { assertEvalRun(t, evalAppliedCh, tick, expectedAlertRulesEvaluated...) }) - expectedAlertRulesEvaluated = []models.AlertRuleKey{alerts[1].GetKey()} + expectedAlertRulesEvaluated = []models.AlertRuleKey{alerts[0].GetKey()} t.Run(fmt.Sprintf("on 4th tick alert rules: %s should be evaluated", concatenate(expectedAlertRulesEvaluated)), func(t *testing.T) { tick := advanceClock(t, mockedClock) assertEvalRun(t, evalAppliedCh, tick, expectedAlertRulesEvaluated...) }) - err := dbstore.DeleteAlertRuleByUID(alerts[1].OrgID, alerts[1].UID) + err := dbstore.DeleteAlertRuleByUID(alerts[0].OrgID, alerts[0].UID) require.NoError(t, err) t.Logf("alert rule: %v deleted", alerts[1].GetKey()) @@ -204,12 +202,12 @@ func TestAlertingTicker(t *testing.T) { tick := advanceClock(t, mockedClock) assertEvalRun(t, evalAppliedCh, tick, expectedAlertRulesEvaluated...) }) - expectedAlertRulesStopped := []models.AlertRuleKey{alerts[1].GetKey()} + expectedAlertRulesStopped := []models.AlertRuleKey{alerts[0].GetKey()} t.Run(fmt.Sprintf("on 5th tick alert rules: %s should be stopped", concatenate(expectedAlertRulesStopped)), func(t *testing.T) { assertStopRun(t, stopAppliedCh, expectedAlertRulesStopped...) }) - expectedAlertRulesEvaluated = []models.AlertRuleKey{alerts[0].GetKey()} + expectedAlertRulesEvaluated = []models.AlertRuleKey{alerts[1].GetKey()} t.Run(fmt.Sprintf("on 6th tick alert rules: %s should be evaluated", concatenate(expectedAlertRulesEvaluated)), func(t *testing.T) { tick := advanceClock(t, mockedClock) assertEvalRun(t, evalAppliedCh, tick, expectedAlertRulesEvaluated...) diff --git a/pkg/services/ngalert/store/alert_rule.go b/pkg/services/ngalert/store/alert_rule.go index ad89a459cd6..0d118902d3e 100644 --- a/pkg/services/ngalert/store/alert_rule.go +++ b/pkg/services/ngalert/store/alert_rule.go @@ -441,8 +441,8 @@ func (st DBstore) validateAlertRule(alertRule ngmodels.AlertRule) error { return fmt.Errorf("%w: title is empty", ngmodels.ErrAlertRuleFailedValidation) } - if alertRule.IntervalSeconds%int64(st.BaseInterval.Seconds()) != 0 { - return fmt.Errorf("%w: interval (%v) should be divided exactly by scheduler interval: %v", ngmodels.ErrAlertRuleFailedValidation, time.Duration(alertRule.IntervalSeconds)*time.Second, st.BaseInterval) + if alertRule.IntervalSeconds%int64(st.BaseInterval.Seconds()) != 0 || alertRule.IntervalSeconds <= 0 { + return fmt.Errorf("%w: interval (%v) should be non-zero and divided exactly by scheduler interval: %v", ngmodels.ErrAlertRuleFailedValidation, time.Duration(alertRule.IntervalSeconds)*time.Second, st.BaseInterval) } // enfore max name length in SQLite diff --git a/pkg/tests/api/alerting/api_alertmanager_test.go b/pkg/tests/api/alerting/api_alertmanager_test.go index a8e57ed91b3..cbf99ee12c9 100644 --- a/pkg/tests/api/alerting/api_alertmanager_test.go +++ b/pkg/tests/api/alerting/api_alertmanager_test.go @@ -633,7 +633,7 @@ func TestAlertRuleCRUD(t *testing.T) { }, }, }, - expectedResponse: `{"error":"invalid alert rule: interval (1s) should be divided exactly by scheduler interval: 10s", "message":"failed to update rule group"}`, + expectedResponse: `{"error":"invalid alert rule: interval (1s) should be non-zero and divided exactly by scheduler interval: 10s", "message":"failed to update rule group"}`, }, { desc: "alert rule with unknown datasource",