From fbb3ad5fc45a93e040d1ab90092741612d990696 Mon Sep 17 00:00:00 2001 From: bergquist Date: Thu, 10 Jan 2019 09:35:48 +0100 Subject: [PATCH] make sure frequency cannot be zero frequency set to zero causes division by zero panics in the alert schedular. closes #14810 --- pkg/services/alerting/extractor.go | 2 +- pkg/services/alerting/rule.go | 21 ++++++-- pkg/services/alerting/rule_test.go | 80 ++++++++++++++++++++++-------- 3 files changed, 79 insertions(+), 24 deletions(-) diff --git a/pkg/services/alerting/extractor.go b/pkg/services/alerting/extractor.go index e33e3dc2af3..5b911c5a9ad 100644 --- a/pkg/services/alerting/extractor.go +++ b/pkg/services/alerting/extractor.go @@ -112,7 +112,7 @@ func (e *DashAlertExtractor) getAlertFromPanels(jsonWithPanels *simplejson.Json, frequency, err := getTimeDurationStringToSeconds(jsonAlert.Get("frequency").MustString()) if err != nil { - return nil, ValidationError{Reason: "Could not parse frequency"} + return nil, ValidationError{Reason: err.Error()} } rawFor := jsonAlert.Get("for").MustString() diff --git a/pkg/services/alerting/rule.go b/pkg/services/alerting/rule.go index d2a505145ac..4423046d600 100644 --- a/pkg/services/alerting/rule.go +++ b/pkg/services/alerting/rule.go @@ -1,16 +1,21 @@ package alerting import ( + "errors" "fmt" "regexp" "strconv" "time" "github.com/grafana/grafana/pkg/components/simplejson" - m "github.com/grafana/grafana/pkg/models" ) +var ( + ErrFrequencyCannotBeZeroOrLess = errors.New(`"evaluate every" cannot be zero or below`) + ErrFrequencyCouldNotBeParsed = errors.New(`"evaluate every" field could not be parsed`) +) + type Rule struct { Id int64 OrgId int64 @@ -76,7 +81,7 @@ func getTimeDurationStringToSeconds(str string) (int64, error) { matches := ValueFormatRegex.FindAllString(str, 1) if len(matches) <= 0 { - return 0, fmt.Errorf("Frequency could not be parsed") + return 0, ErrFrequencyCouldNotBeParsed } value, err := strconv.Atoi(matches[0]) @@ -84,6 +89,10 @@ func getTimeDurationStringToSeconds(str string) (int64, error) { return 0, err } + if value == 0 { + return 0, ErrFrequencyCannotBeZeroOrLess + } + unit := UnitFormatRegex.FindAllString(str, 1)[0] if val, ok := unitMultiplier[unit]; ok { @@ -101,7 +110,6 @@ func NewRuleFromDBAlert(ruleDef *m.Alert) (*Rule, error) { model.PanelId = ruleDef.PanelId model.Name = ruleDef.Name model.Message = ruleDef.Message - model.Frequency = ruleDef.Frequency model.State = ruleDef.State model.LastStateChange = ruleDef.NewStateDate model.For = ruleDef.For @@ -109,6 +117,13 @@ func NewRuleFromDBAlert(ruleDef *m.Alert) (*Rule, error) { model.ExecutionErrorState = m.ExecutionErrorOption(ruleDef.Settings.Get("executionErrorState").MustString("alerting")) model.StateChanges = ruleDef.StateChanges + model.Frequency = ruleDef.Frequency + // frequency cannot be zero since that would not execute the alert rule. + // so we fallback to 60 seconds if `Freqency` is missing + if model.Frequency == 0 { + model.Frequency = 60 + } + for _, v := range ruleDef.Settings.Get("notifications").MustArray() { jsonModel := simplejson.NewFromAny(v) id, err := jsonModel.Get("id").Int64() diff --git a/pkg/services/alerting/rule_test.go b/pkg/services/alerting/rule_test.go index 2a9e95e5723..cf25cc118f4 100644 --- a/pkg/services/alerting/rule_test.go +++ b/pkg/services/alerting/rule_test.go @@ -14,6 +14,36 @@ func (f *FakeCondition) Eval(context *EvalContext) (*ConditionResult, error) { return &ConditionResult{}, nil } +func TestAlertRuleFrequencyParsing(t *testing.T) { + tcs := []struct { + input string + err error + result int64 + }{ + {input: "10s", result: 10}, + {input: "10m", result: 600}, + {input: "1h", result: 3600}, + {input: "1o", result: 1}, + {input: "0s", err: ErrFrequencyCannotBeZeroOrLess}, + {input: "0m", err: ErrFrequencyCannotBeZeroOrLess}, + {input: "0h", err: ErrFrequencyCannotBeZeroOrLess}, + {input: "0", err: ErrFrequencyCannotBeZeroOrLess}, + {input: "-1s", err: ErrFrequencyCouldNotBeParsed}, + } + + for _, tc := range tcs { + r, err := getTimeDurationStringToSeconds(tc.input) + if err != tc.err { + t.Errorf("expected error: '%v' got: '%v'", tc.err, err) + return + } + + if r != tc.result { + t.Errorf("expected result: %d got %d", tc.result, r) + } + } +} + func TestAlertRuleModel(t *testing.T) { Convey("Testing alert rule", t, func() { @@ -21,26 +51,6 @@ func TestAlertRuleModel(t *testing.T) { return &FakeCondition{}, nil }) - Convey("Can parse seconds", func() { - seconds, _ := getTimeDurationStringToSeconds("10s") - So(seconds, ShouldEqual, 10) - }) - - Convey("Can parse minutes", func() { - seconds, _ := getTimeDurationStringToSeconds("10m") - So(seconds, ShouldEqual, 600) - }) - - Convey("Can parse hours", func() { - seconds, _ := getTimeDurationStringToSeconds("1h") - So(seconds, ShouldEqual, 3600) - }) - - Convey("defaults to seconds", func() { - seconds, _ := getTimeDurationStringToSeconds("1o") - So(seconds, ShouldEqual, 1) - }) - Convey("should return err for empty string", func() { _, err := getTimeDurationStringToSeconds("") So(err, ShouldNotBeNil) @@ -89,5 +99,35 @@ func TestAlertRuleModel(t *testing.T) { So(len(alertRule.Notifications), ShouldEqual, 2) }) }) + + Convey("can construct alert rule model with invalid frequency", func() { + json := ` + { + "name": "name2", + "description": "desc2", + "noDataMode": "critical", + "enabled": true, + "frequency": "0s", + "conditions": [ { "type": "test", "prop": 123 } ], + "notifications": [] + }` + + alertJSON, jsonErr := simplejson.NewJson([]byte(json)) + So(jsonErr, ShouldBeNil) + + alert := &m.Alert{ + Id: 1, + OrgId: 1, + DashboardId: 1, + PanelId: 1, + Frequency: 0, + + Settings: alertJSON, + } + + alertRule, err := NewRuleFromDBAlert(alert) + So(err, ShouldBeNil) + So(alertRule.Frequency, ShouldEqual, 60) + }) }) }