From 258578766b43edd042c3d6e6ad4154ad796fe244 Mon Sep 17 00:00:00 2001 From: Dimitris Sotirakis Date: Mon, 12 Apr 2021 15:53:51 +0300 Subject: [PATCH] Alerting: Add checks for non supported units - disable defaulting to seconds (#32477) * Add error handling for unknown units * Fix test cases * Add case for empty string * Changed tests from convey to testify * Fix lints * Move regex vars * Add regex as ng-patterns on alert_tab.html * Update public/app/features/alerting/partials/alert_tab.html Co-authored-by: Marcus Efraimsson * Update public/app/features/alerting/partials/alert_tab.html Co-authored-by: Marcus Efraimsson * Make zero and empty string not throw errors * Updated validation error comments * Frequency should allow zero or empty strings * use checkFrequency instead of ng-pattern checkFrequency is not triggered if ng-pattern is defined. * Extract getForValue func - add tests * Fix linting Co-authored-by: Marcus Efraimsson --- pkg/services/alerting/extractor.go | 11 +- pkg/services/alerting/extractor_test.go | 508 ++++++++---------- pkg/services/alerting/rule.go | 65 ++- pkg/services/alerting/rule_test.go | 200 +++---- public/app/features/alerting/AlertTabCtrl.ts | 8 +- .../features/alerting/partials/alert_tab.html | 1 + 6 files changed, 395 insertions(+), 398 deletions(-) diff --git a/pkg/services/alerting/extractor.go b/pkg/services/alerting/extractor.go index 129187a0363..c02f5358815 100644 --- a/pkg/services/alerting/extractor.go +++ b/pkg/services/alerting/extractor.go @@ -4,7 +4,6 @@ import ( "encoding/json" "errors" "fmt" - "time" "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/components/simplejson" @@ -109,12 +108,10 @@ func (e *DashAlertExtractor) getAlertFromPanels(jsonWithPanels *simplejson.Json, } rawFor := jsonAlert.Get("for").MustString() - var forValue time.Duration - if rawFor != "" { - forValue, err = time.ParseDuration(rawFor) - if err != nil { - return nil, ValidationError{Reason: "Could not parse for"} - } + + forValue, err := getForValue(rawFor) + if err != nil { + return nil, err } alert := &models.Alert{ diff --git a/pkg/services/alerting/extractor_test.go b/pkg/services/alerting/extractor_test.go index 7e36faa6bed..eb4cf503a9b 100644 --- a/pkg/services/alerting/extractor_test.go +++ b/pkg/services/alerting/extractor_test.go @@ -9,293 +9,239 @@ import ( "github.com/grafana/grafana/pkg/components/simplejson" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/sqlstore" - . "github.com/smartystreets/goconvey/convey" + "github.com/stretchr/testify/require" ) func TestAlertRuleExtraction(t *testing.T) { - Convey("Parsing alert rules from dashboard json", t, func() { - RegisterCondition("query", func(model *simplejson.Json, index int) (Condition, error) { - return &FakeCondition{}, nil - }) - - // mock data - defaultDs := &models.DataSource{Id: 12, OrgId: 1, Name: "I am default", IsDefault: true} - graphite2Ds := &models.DataSource{Id: 15, OrgId: 1, Name: "graphite2"} - influxDBDs := &models.DataSource{Id: 16, OrgId: 1, Name: "InfluxDB"} - prom := &models.DataSource{Id: 17, OrgId: 1, Name: "Prometheus"} - - bus.AddHandler("test", func(query *models.GetDefaultDataSourceQuery) error { + RegisterCondition("query", func(model *simplejson.Json, index int) (Condition, error) { + return &FakeCondition{}, nil + }) + + // mock data + defaultDs := &models.DataSource{Id: 12, OrgId: 1, Name: "I am default", IsDefault: true} + graphite2Ds := &models.DataSource{Id: 15, OrgId: 1, Name: "graphite2"} + influxDBDs := &models.DataSource{Id: 16, OrgId: 1, Name: "InfluxDB"} + prom := &models.DataSource{Id: 17, OrgId: 1, Name: "Prometheus"} + + bus.AddHandler("test", func(query *models.GetDefaultDataSourceQuery) error { + query.Result = defaultDs + return nil + }) + + bus.AddHandler("test", func(query *models.GetDataSourceQuery) error { + if query.Name == defaultDs.Name { query.Result = defaultDs - return nil - }) - - bus.AddHandler("test", func(query *models.GetDataSourceQuery) error { - if query.Name == defaultDs.Name { - query.Result = defaultDs - } - if query.Name == graphite2Ds.Name { - query.Result = graphite2Ds - } - if query.Name == influxDBDs.Name { - query.Result = influxDBDs - } - if query.Name == prom.Name { - query.Result = prom - } - - return nil - }) - - json, err := ioutil.ReadFile("./testdata/graphite-alert.json") - So(err, ShouldBeNil) - - Convey("Extractor should not modify the original json", func() { - dashJSON, err := simplejson.NewJson(json) - So(err, ShouldBeNil) - - dash := models.NewDashboardFromJson(dashJSON) - - getTarget := func(j *simplejson.Json) string { - rowObj := j.Get("rows").MustArray()[0] - row := simplejson.NewFromAny(rowObj) - panelObj := row.Get("panels").MustArray()[0] - panel := simplejson.NewFromAny(panelObj) - conditionObj := panel.Get("alert").Get("conditions").MustArray()[0] - condition := simplejson.NewFromAny(conditionObj) - return condition.Get("query").Get("model").Get("target").MustString() - } - - Convey("Dashboard json rows.panels.alert.query.model.target should be empty", func() { - So(getTarget(dashJSON), ShouldEqual, "") - }) - - extractor := NewDashAlertExtractor(dash, 1, nil) - _, _ = extractor.GetAlerts() - - Convey("Dashboard json should not be updated after extracting rules", func() { - So(getTarget(dashJSON), ShouldEqual, "") - }) - }) - - Convey("Parsing and validating dashboard containing graphite alerts", func() { - dashJSON, err := simplejson.NewJson(json) - So(err, ShouldBeNil) - - dash := models.NewDashboardFromJson(dashJSON) - extractor := NewDashAlertExtractor(dash, 1, nil) - - alerts, err := extractor.GetAlerts() - - Convey("Get rules without error", func() { - So(err, ShouldBeNil) - }) - - Convey("all properties have been set", func() { - So(len(alerts), ShouldEqual, 2) - - for _, v := range alerts { - So(v.DashboardId, ShouldEqual, 57) - So(v.Name, ShouldNotBeEmpty) - So(v.Message, ShouldNotBeEmpty) - - settings := simplejson.NewFromAny(v.Settings) - So(settings.Get("interval").MustString(""), ShouldEqual, "") - } - - Convey("should extract handler property", func() { - So(alerts[0].Handler, ShouldEqual, 1) - So(alerts[1].Handler, ShouldEqual, 0) - }) - - Convey("should extract frequency in seconds", func() { - So(alerts[0].Frequency, ShouldEqual, 60) - So(alerts[1].Frequency, ShouldEqual, 60) - }) - - Convey("should extract panel idc", func() { - So(alerts[0].PanelId, ShouldEqual, 3) - So(alerts[1].PanelId, ShouldEqual, 4) - }) - - Convey("should extract for param", func() { - So(alerts[0].For, ShouldEqual, time.Minute*2) - So(alerts[1].For, ShouldEqual, time.Duration(0)) - }) - - Convey("should extract name and desc", func() { - So(alerts[0].Name, ShouldEqual, "name1") - So(alerts[0].Message, ShouldEqual, "desc1") - So(alerts[1].Name, ShouldEqual, "name2") - So(alerts[1].Message, ShouldEqual, "desc2") - }) - - Convey("should set datasourceId", func() { - condition := simplejson.NewFromAny(alerts[0].Settings.Get("conditions").MustArray()[0]) - query := condition.Get("query") - So(query.Get("datasourceId").MustInt64(), ShouldEqual, 12) - }) - - Convey("should copy query model to condition", func() { - condition := simplejson.NewFromAny(alerts[0].Settings.Get("conditions").MustArray()[0]) - model := condition.Get("query").Get("model") - So(model.Get("target").MustString(), ShouldEqual, "aliasByNode(statsd.fakesite.counters.session_start.desktop.count, 4)") - }) - }) - }) - - Convey("Panels missing id should return error", func() { - panelWithoutID, err := ioutil.ReadFile("./testdata/panels-missing-id.json") - So(err, ShouldBeNil) - - dashJSON, err := simplejson.NewJson(panelWithoutID) - So(err, ShouldBeNil) - dash := models.NewDashboardFromJson(dashJSON) - extractor := NewDashAlertExtractor(dash, 1, nil) - - _, err = extractor.GetAlerts() - - Convey("panels without Id should return error", func() { - So(err, ShouldNotBeNil) - }) - }) - - Convey("Panel with id set to zero should return error", func() { - panelWithIDZero, err := ioutil.ReadFile("./testdata/panel-with-id-0.json") - So(err, ShouldBeNil) - - dashJSON, err := simplejson.NewJson(panelWithIDZero) - So(err, ShouldBeNil) - dash := models.NewDashboardFromJson(dashJSON) - extractor := NewDashAlertExtractor(dash, 1, nil) - - _, err = extractor.GetAlerts() - - Convey("panel with id 0 should return error", func() { - So(err, ShouldNotBeNil) - }) - }) - - Convey("Panel does not have datasource configured, use the default datasource", func() { - panelWithoutSpecifiedDatasource, err := ioutil.ReadFile("./testdata/panel-without-specified-datasource.json") - So(err, ShouldBeNil) - - dashJSON, err := simplejson.NewJson(panelWithoutSpecifiedDatasource) - So(err, ShouldBeNil) - dash := models.NewDashboardFromJson(dashJSON) - extractor := NewDashAlertExtractor(dash, 1, nil) - - alerts, err := extractor.GetAlerts() - - Convey("Get rules without error", func() { - So(err, ShouldBeNil) - }) - - Convey("Use default datasource", func() { - condition := simplejson.NewFromAny(alerts[0].Settings.Get("conditions").MustArray()[0]) - query := condition.Get("query") - So(query.Get("datasourceId").MustInt64(), ShouldEqual, 12) - }) - }) - - Convey("Parse alerts from dashboard without rows", func() { - json, err := ioutil.ReadFile("./testdata/v5-dashboard.json") - So(err, ShouldBeNil) - - dashJSON, err := simplejson.NewJson(json) - So(err, ShouldBeNil) - dash := models.NewDashboardFromJson(dashJSON) - extractor := NewDashAlertExtractor(dash, 1, nil) - - alerts, err := extractor.GetAlerts() - - Convey("Get rules without error", func() { - So(err, ShouldBeNil) - }) - - Convey("Should have 2 alert rule", func() { - So(len(alerts), ShouldEqual, 2) - }) - }) - - Convey("Alert notifications are in DB", func() { - sqlstore.InitTestDB(t) - firstNotification := models.CreateAlertNotificationCommand{Uid: "notifier1", OrgId: 1, Name: "1"} - err = sqlstore.CreateAlertNotificationCommand(&firstNotification) - So(err, ShouldBeNil) - secondNotification := models.CreateAlertNotificationCommand{Uid: "notifier2", OrgId: 1, Name: "2"} - err = sqlstore.CreateAlertNotificationCommand(&secondNotification) - So(err, ShouldBeNil) - - Convey("Parse and validate dashboard containing influxdb alert", func() { - json, err := ioutil.ReadFile("./testdata/influxdb-alert.json") - So(err, ShouldBeNil) - - dashJSON, err := simplejson.NewJson(json) - So(err, ShouldBeNil) - dash := models.NewDashboardFromJson(dashJSON) - extractor := NewDashAlertExtractor(dash, 1, nil) - - alerts, err := extractor.GetAlerts() - - Convey("Get rules without error", func() { - So(err, ShouldBeNil) - }) - - Convey("should be able to read interval", func() { - So(len(alerts), ShouldEqual, 1) - - for _, alert := range alerts { - So(alert.DashboardId, ShouldEqual, 4) + } + if query.Name == graphite2Ds.Name { + query.Result = graphite2Ds + } + if query.Name == influxDBDs.Name { + query.Result = influxDBDs + } + if query.Name == prom.Name { + query.Result = prom + } + + return nil + }) + + json, err := ioutil.ReadFile("./testdata/graphite-alert.json") + require.Nil(t, err) + + t.Run("Parsing alert rules from dashboard json", func(t *testing.T) { + dashJSON, err := simplejson.NewJson(json) + require.Nil(t, err) + + dash := models.NewDashboardFromJson(dashJSON) + + getTarget := func(j *simplejson.Json) string { + rowObj := j.Get("rows").MustArray()[0] + row := simplejson.NewFromAny(rowObj) + panelObj := row.Get("panels").MustArray()[0] + panel := simplejson.NewFromAny(panelObj) + conditionObj := panel.Get("alert").Get("conditions").MustArray()[0] + condition := simplejson.NewFromAny(conditionObj) + return condition.Get("query").Get("model").Get("target").MustString() + } + + require.Equal(t, getTarget(dashJSON), "") + + extractor := NewDashAlertExtractor(dash, 1, nil) + _, _ = extractor.GetAlerts() + + require.Equal(t, getTarget(dashJSON), "") + }) + + t.Run("Parsing and validating dashboard containing graphite alerts", func(t *testing.T) { + dashJSON, err := simplejson.NewJson(json) + require.Nil(t, err) + + dash := models.NewDashboardFromJson(dashJSON) + extractor := NewDashAlertExtractor(dash, 1, nil) + + alerts, err := extractor.GetAlerts() + + require.Nil(t, err) + + require.Len(t, alerts, 2) + + for _, v := range alerts { + require.EqualValues(t, v.DashboardId, 57) + require.NotEmpty(t, v.Name) + require.NotEmpty(t, v.Message) + + settings := simplejson.NewFromAny(v.Settings) + require.Equal(t, settings.Get("interval").MustString(""), "") + } + + require.EqualValues(t, alerts[0].Handler, 1) + require.EqualValues(t, alerts[1].Handler, 0) + + require.EqualValues(t, alerts[0].Frequency, 60) + require.EqualValues(t, alerts[1].Frequency, 60) + + require.EqualValues(t, alerts[0].PanelId, 3) + require.EqualValues(t, alerts[1].PanelId, 4) + + require.Equal(t, alerts[0].For, time.Minute*2) + require.Equal(t, alerts[1].For, time.Duration(0)) + + require.Equal(t, alerts[0].Name, "name1") + require.Equal(t, alerts[0].Message, "desc1") + require.Equal(t, alerts[1].Name, "name2") + require.Equal(t, alerts[1].Message, "desc2") + + condition := simplejson.NewFromAny(alerts[0].Settings.Get("conditions").MustArray()[0]) + query := condition.Get("query") + require.EqualValues(t, query.Get("datasourceId").MustInt64(), 12) + + condition = simplejson.NewFromAny(alerts[0].Settings.Get("conditions").MustArray()[0]) + model := condition.Get("query").Get("model") + require.Equal(t, model.Get("target").MustString(), "aliasByNode(statsd.fakesite.counters.session_start.desktop.count, 4)") + }) + + t.Run("Panels missing id should return error", func(t *testing.T) { + panelWithoutID, err := ioutil.ReadFile("./testdata/panels-missing-id.json") + require.Nil(t, err) + + dashJSON, err := simplejson.NewJson(panelWithoutID) + require.Nil(t, err) + dash := models.NewDashboardFromJson(dashJSON) + extractor := NewDashAlertExtractor(dash, 1, nil) + + _, err = extractor.GetAlerts() + + require.NotNil(t, err) + }) + + t.Run("Panels missing id should return error", func(t *testing.T) { + panelWithIDZero, err := ioutil.ReadFile("./testdata/panel-with-id-0.json") + require.Nil(t, err) + + dashJSON, err := simplejson.NewJson(panelWithIDZero) + require.Nil(t, err) + dash := models.NewDashboardFromJson(dashJSON) + extractor := NewDashAlertExtractor(dash, 1, nil) + + _, err = extractor.GetAlerts() + + require.NotNil(t, err) + }) + + t.Run("Panel does not have datasource configured, use the default datasource", func(t *testing.T) { + panelWithoutSpecifiedDatasource, err := ioutil.ReadFile("./testdata/panel-without-specified-datasource.json") + require.Nil(t, err) + + dashJSON, err := simplejson.NewJson(panelWithoutSpecifiedDatasource) + require.Nil(t, err) + dash := models.NewDashboardFromJson(dashJSON) + extractor := NewDashAlertExtractor(dash, 1, nil) + + alerts, err := extractor.GetAlerts() + require.Nil(t, err) + + condition := simplejson.NewFromAny(alerts[0].Settings.Get("conditions").MustArray()[0]) + query := condition.Get("query") + require.EqualValues(t, query.Get("datasourceId").MustInt64(), 12) + }) + + t.Run("Parse alerts from dashboard without rows", func(t *testing.T) { + json, err := ioutil.ReadFile("./testdata/v5-dashboard.json") + require.Nil(t, err) + + dashJSON, err := simplejson.NewJson(json) + require.Nil(t, err) + dash := models.NewDashboardFromJson(dashJSON) + extractor := NewDashAlertExtractor(dash, 1, nil) + + alerts, err := extractor.GetAlerts() + require.Nil(t, err) + + require.Len(t, alerts, 2) + }) + + t.Run("Alert notifications are in DB", func(t *testing.T) { + sqlstore.InitTestDB(t) + firstNotification := models.CreateAlertNotificationCommand{Uid: "notifier1", OrgId: 1, Name: "1"} + err = sqlstore.CreateAlertNotificationCommand(&firstNotification) + require.Nil(t, err) + secondNotification := models.CreateAlertNotificationCommand{Uid: "notifier2", OrgId: 1, Name: "2"} + err = sqlstore.CreateAlertNotificationCommand(&secondNotification) + require.Nil(t, err) + + json, err := ioutil.ReadFile("./testdata/influxdb-alert.json") + require.Nil(t, err) + + dashJSON, err := simplejson.NewJson(json) + require.Nil(t, err) + dash := models.NewDashboardFromJson(dashJSON) + extractor := NewDashAlertExtractor(dash, 1, nil) + + alerts, err := extractor.GetAlerts() + require.Nil(t, err) + + require.Len(t, alerts, 1) + + for _, alert := range alerts { + require.EqualValues(t, alert.DashboardId, 4) + + conditions := alert.Settings.Get("conditions").MustArray() + cond := simplejson.NewFromAny(conditions[0]) + + require.Equal(t, cond.Get("query").Get("model").Get("interval").MustString(), ">10s") + } + }) + + t.Run("Should be able to extract collapsed panels", func(t *testing.T) { + json, err := ioutil.ReadFile("./testdata/collapsed-panels.json") + require.Nil(t, err) + + dashJSON, err := simplejson.NewJson(json) + require.Nil(t, err) + + dash := models.NewDashboardFromJson(dashJSON) + extractor := NewDashAlertExtractor(dash, 1, nil) + + alerts, err := extractor.GetAlerts() + require.Nil(t, err) + + require.Len(t, alerts, 4) + }) + + t.Run("Parse and validate dashboard without id and containing an alert", func(t *testing.T) { + json, err := ioutil.ReadFile("./testdata/dash-without-id.json") + require.Nil(t, err) + + dashJSON, err := simplejson.NewJson(json) + require.Nil(t, err) + dash := models.NewDashboardFromJson(dashJSON) + extractor := NewDashAlertExtractor(dash, 1, nil) - conditions := alert.Settings.Get("conditions").MustArray() - cond := simplejson.NewFromAny(conditions[0]) - - So(cond.Get("query").Get("model").Get("interval").MustString(), ShouldEqual, ">10s") - } - }) - }) - - Convey("Should be able to extract collapsed panels", func() { - json, err := ioutil.ReadFile("./testdata/collapsed-panels.json") - So(err, ShouldBeNil) + err = extractor.ValidateAlerts() - dashJSON, err := simplejson.NewJson(json) - So(err, ShouldBeNil) - - dash := models.NewDashboardFromJson(dashJSON) - extractor := NewDashAlertExtractor(dash, 1, nil) - - alerts, err := extractor.GetAlerts() + require.Nil(t, err) - Convey("Get rules without error", func() { - So(err, ShouldBeNil) - }) - - Convey("should be able to extract collapsed alerts", func() { - So(len(alerts), ShouldEqual, 4) - }) - }) - - Convey("Parse and validate dashboard without id and containing an alert", func() { - json, err := ioutil.ReadFile("./testdata/dash-without-id.json") - So(err, ShouldBeNil) - - dashJSON, err := simplejson.NewJson(json) - So(err, ShouldBeNil) - dash := models.NewDashboardFromJson(dashJSON) - extractor := NewDashAlertExtractor(dash, 1, nil) - - err = extractor.ValidateAlerts() - - Convey("Should validate without error", func() { - So(err, ShouldBeNil) - }) - - Convey("Should fail on save", func() { - _, err := extractor.GetAlerts() - So(err.Error(), ShouldEqual, "alert validation error: Panel id is not correct, alertName=Influxdb, panelId=1") - }) - }) - }) + _, err = extractor.GetAlerts() + require.Equal(t, err.Error(), "alert validation error: Panel id is not correct, alertName=Influxdb, panelId=1") }) } diff --git a/pkg/services/alerting/rule.go b/pkg/services/alerting/rule.go index 6e9b1060e56..a1030eff8aa 100644 --- a/pkg/services/alerting/rule.go +++ b/pkg/services/alerting/rule.go @@ -3,6 +3,7 @@ package alerting import ( "errors" "fmt" + "reflect" "regexp" "strconv" "time" @@ -12,12 +13,28 @@ import ( "github.com/grafana/grafana/pkg/models" ) +var unitMultiplier = map[string]int{ + "s": 1, + "m": 60, + "h": 3600, + "d": 86400, +} + +var ( + valueFormatRegex = regexp.MustCompile(`^\d+`) + isDigitRegex = regexp.MustCompile(`^[0-9]+$`) + unitFormatRegex = regexp.MustCompile(`[a-z]+`) +) + var ( // ErrFrequencyCannotBeZeroOrLess frequency cannot be below zero ErrFrequencyCannotBeZeroOrLess = errors.New(`"evaluate every" cannot be zero or below`) // ErrFrequencyCouldNotBeParsed frequency cannot be parsed ErrFrequencyCouldNotBeParsed = errors.New(`"evaluate every" field could not be parsed`) + + // ErrWrongUnitFormat wrong unit format + ErrWrongUnitFormat = fmt.Errorf(`time unit not supported. supported units: %s`, reflect.ValueOf(unitMultiplier).MapKeys()) ) // Rule is the in-memory version of an alert rule. @@ -72,20 +89,18 @@ func (e ValidationError) Error() string { return fmt.Sprintf("alert validation error: %s", extraInfo) } -var ( - valueFormatRegex = regexp.MustCompile(`^\d+`) - unitFormatRegex = regexp.MustCompile(`\w{1}$`) -) +func getTimeDurationStringToSeconds(str string) (int64, error) { + // Check if frequency lacks unit + if isDigitRegex.MatchString(str) || str == "" { + return 0, ErrFrequencyCouldNotBeParsed + } -var unitMultiplier = map[string]int{ - "s": 1, - "m": 60, - "h": 3600, - "d": 86400, -} + unit := unitFormatRegex.FindAllString(str, 1)[0] + if _, ok := unitMultiplier[unit]; !ok { + return 0, ErrWrongUnitFormat + } -func getTimeDurationStringToSeconds(str string) (int64, error) { - multiplier := 1 + multiplier := unitMultiplier[unit] matches := valueFormatRegex.FindAllString(str, 1) @@ -102,13 +117,29 @@ func getTimeDurationStringToSeconds(str string) (int64, error) { return 0, ErrFrequencyCannotBeZeroOrLess } - unit := unitFormatRegex.FindAllString(str, 1)[0] + return int64(value * multiplier), nil +} - if val, ok := unitMultiplier[unit]; ok { - multiplier = val - } +func getForValue(rawFor string) (time.Duration, error) { + var forValue time.Duration + var err error - return int64(value * multiplier), nil + if rawFor != "" { + if rawFor != "0" { + strings := unitFormatRegex.FindAllString(rawFor, 1) + if strings == nil { + return 0, ValidationError{Reason: fmt.Sprintf("no specified unit, error: %s", ErrWrongUnitFormat.Error())} + } + if _, ok := unitMultiplier[strings[0]]; !ok { + return 0, ValidationError{Reason: fmt.Sprintf("could not parse for field, error: %s", ErrWrongUnitFormat.Error())} + } + } + forValue, err = time.ParseDuration(rawFor) + if err != nil { + return 0, ValidationError{Reason: "Could not parse for field"} + } + } + return forValue, nil } // NewRuleFromDBAlert maps a db version of diff --git a/pkg/services/alerting/rule_test.go b/pkg/services/alerting/rule_test.go index d7f2881daeb..b73a76f7858 100644 --- a/pkg/services/alerting/rule_test.go +++ b/pkg/services/alerting/rule_test.go @@ -1,13 +1,14 @@ package alerting import ( + "fmt" "testing" + "time" "github.com/grafana/grafana/pkg/components/simplejson" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/plugins" "github.com/grafana/grafana/pkg/services/sqlstore" - . "github.com/smartystreets/goconvey/convey" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -28,11 +29,12 @@ func TestAlertRuleFrequencyParsing(t *testing.T) { {input: "10m", result: 600}, {input: "1h", result: 3600}, {input: "1d", result: 86400}, - {input: "1o", result: 1}, + {input: "1o", err: ErrWrongUnitFormat}, {input: "0s", err: ErrFrequencyCannotBeZeroOrLess}, {input: "0m", err: ErrFrequencyCannotBeZeroOrLess}, {input: "0h", err: ErrFrequencyCannotBeZeroOrLess}, - {input: "0", err: ErrFrequencyCannotBeZeroOrLess}, + {input: "0", err: ErrFrequencyCouldNotBeParsed}, + {input: "", err: ErrFrequencyCouldNotBeParsed}, {input: "-1s", err: ErrFrequencyCouldNotBeParsed}, } @@ -49,28 +51,52 @@ func TestAlertRuleFrequencyParsing(t *testing.T) { } } -func TestAlertRuleModel(t *testing.T) { - sqlstore.InitTestDB(t) - Convey("Testing alert rule", t, func() { - RegisterCondition("test", func(model *simplejson.Json, index int) (Condition, error) { - return &FakeCondition{}, nil - }) +func TestAlertRuleForParsing(t *testing.T) { + tcs := []struct { + input string + err error + result time.Duration + }{ + {input: "10s", result: time.Duration(10000000000)}, + {input: "10m", result: time.Duration(600000000000)}, + {input: "1h", result: time.Duration(3600000000000)}, + {input: "1o", err: fmt.Errorf("alert validation error: could not parse for field, error: %s", ErrWrongUnitFormat)}, + {input: "1", err: fmt.Errorf("alert validation error: no specified unit, error: %s", ErrWrongUnitFormat)}, + {input: "0s", result: time.Duration(0)}, + {input: "0m", result: time.Duration(0)}, + {input: "0h", result: time.Duration(0)}, + {input: "0", result: time.Duration(0)}, + {input: "", result: time.Duration(0)}, + } - Convey("should return err for empty string", func() { - _, err := getTimeDurationStringToSeconds("") - So(err, ShouldNotBeNil) + for _, tc := range tcs { + t.Run(tc.input, func(t *testing.T) { + r, err := getForValue(tc.input) + if tc.err == nil { + require.NoError(t, err) + } else { + require.EqualError(t, err, tc.err.Error()) + } + assert.Equal(t, tc.result, r) }) + } +} + +func TestAlertRuleModel(t *testing.T) { + sqlstore.InitTestDB(t) + RegisterCondition("test", func(model *simplejson.Json, index int) (Condition, error) { + return &FakeCondition{}, nil + }) - Convey("can construct alert rule model", func() { - firstNotification := models.CreateAlertNotificationCommand{Uid: "notifier1", OrgId: 1, Name: "1"} - err := sqlstore.CreateAlertNotificationCommand(&firstNotification) - So(err, ShouldBeNil) - secondNotification := models.CreateAlertNotificationCommand{Uid: "notifier2", OrgId: 1, Name: "2"} - err = sqlstore.CreateAlertNotificationCommand(&secondNotification) - So(err, ShouldBeNil) + firstNotification := models.CreateAlertNotificationCommand{Uid: "notifier1", OrgId: 1, Name: "1"} + err := sqlstore.CreateAlertNotificationCommand(&firstNotification) + require.Nil(t, err) + secondNotification := models.CreateAlertNotificationCommand{Uid: "notifier2", OrgId: 1, Name: "2"} + err = sqlstore.CreateAlertNotificationCommand(&secondNotification) + require.Nil(t, err) - Convey("with notification id and uid", func() { - json := ` + t.Run("Testing alert rule with notification id and uid", func(t *testing.T) { + json := ` { "name": "name2", "description": "desc2", @@ -91,33 +117,30 @@ func TestAlertRuleModel(t *testing.T) { } ` - alertJSON, jsonErr := simplejson.NewJson([]byte(json)) - So(jsonErr, ShouldBeNil) + alertJSON, jsonErr := simplejson.NewJson([]byte(json)) + require.Nil(t, jsonErr) - alert := &models.Alert{ - Id: 1, - OrgId: 1, - DashboardId: 1, - PanelId: 1, + alert := &models.Alert{ + Id: 1, + OrgId: 1, + DashboardId: 1, + PanelId: 1, - Settings: alertJSON, - } + Settings: alertJSON, + } - alertRule, err := NewRuleFromDBAlert(alert, false) - So(err, ShouldBeNil) + alertRule, err := NewRuleFromDBAlert(alert, false) + require.Nil(t, err) - So(len(alertRule.Conditions), ShouldEqual, 1) - So(len(alertRule.Notifications), ShouldEqual, 2) + require.Len(t, alertRule.Conditions, 1) + require.Len(t, alertRule.Notifications, 2) - Convey("Can read Id and Uid notifications (translate Id to Uid)", func() { - So(alertRule.Notifications, ShouldContain, "notifier2") - So(alertRule.Notifications, ShouldContain, "notifier1") - }) - }) - }) + require.Contains(t, alertRule.Notifications, "notifier2") + require.Contains(t, alertRule.Notifications, "notifier1") + }) - Convey("with non existing notification id", func() { - json := ` + t.Run("Testing alert rule with non existing notification id", func(t *testing.T) { + json := ` { "name": "name3", "description": "desc3", @@ -133,28 +156,26 @@ func TestAlertRuleModel(t *testing.T) { } ` - alertJSON, jsonErr := simplejson.NewJson([]byte(json)) - So(jsonErr, ShouldBeNil) + alertJSON, jsonErr := simplejson.NewJson([]byte(json)) + require.Nil(t, jsonErr) - alert := &models.Alert{ - Id: 1, - OrgId: 1, - DashboardId: 1, - PanelId: 1, + alert := &models.Alert{ + Id: 1, + OrgId: 1, + DashboardId: 1, + PanelId: 1, - Settings: alertJSON, - } + Settings: alertJSON, + } - alertRule, err := NewRuleFromDBAlert(alert, false) - Convey("swallows the error", func() { - So(err, ShouldBeNil) - So(alertRule.Notifications, ShouldNotContain, "999") - So(alertRule.Notifications, ShouldContain, "notifier2") - }) - }) + alertRule, err := NewRuleFromDBAlert(alert, false) + require.Nil(t, err) + require.NotContains(t, alertRule.Notifications, "999") + require.Contains(t, alertRule.Notifications, "notifier2") + }) - Convey("can construct alert rule model with invalid frequency", func() { - json := ` + t.Run("Testing alert rule which can construct alert rule model with invalid frequency", func(t *testing.T) { + json := ` { "name": "name2", "description": "desc2", @@ -165,26 +186,26 @@ func TestAlertRuleModel(t *testing.T) { "notifications": [] }` - alertJSON, jsonErr := simplejson.NewJson([]byte(json)) - So(jsonErr, ShouldBeNil) + alertJSON, jsonErr := simplejson.NewJson([]byte(json)) + require.Nil(t, jsonErr) - alert := &models.Alert{ - Id: 1, - OrgId: 1, - DashboardId: 1, - PanelId: 1, - Frequency: 0, + alert := &models.Alert{ + Id: 1, + OrgId: 1, + DashboardId: 1, + PanelId: 1, + Frequency: 0, - Settings: alertJSON, - } + Settings: alertJSON, + } - alertRule, err := NewRuleFromDBAlert(alert, false) - So(err, ShouldBeNil) - So(alertRule.Frequency, ShouldEqual, 60) - }) + alertRule, err := NewRuleFromDBAlert(alert, false) + require.Nil(t, err) + require.EqualValues(t, alertRule.Frequency, 60) + }) - Convey("raise error in case of missing notification id and uid", func() { - json := ` + t.Run("Testing alert rule which will raise error in case of missing notification id and uid", func(t *testing.T) { + json := ` { "name": "name2", "description": "desc2", @@ -203,22 +224,21 @@ func TestAlertRuleModel(t *testing.T) { } ` - alertJSON, jsonErr := simplejson.NewJson([]byte(json)) - So(jsonErr, ShouldBeNil) + alertJSON, jsonErr := simplejson.NewJson([]byte(json)) + require.Nil(t, jsonErr) - alert := &models.Alert{ - Id: 1, - OrgId: 1, - DashboardId: 1, - PanelId: 1, - Frequency: 0, + alert := &models.Alert{ + Id: 1, + OrgId: 1, + DashboardId: 1, + PanelId: 1, + Frequency: 0, - Settings: alertJSON, - } + Settings: alertJSON, + } - _, err := NewRuleFromDBAlert(alert, false) - So(err, ShouldNotBeNil) - So(err.Error(), ShouldEqual, "alert validation error: Neither id nor uid is specified in 'notifications' block, type assertion to string failed AlertId: 1 PanelId: 1 DashboardId: 1") - }) + _, err := NewRuleFromDBAlert(alert, false) + require.NotNil(t, err) + require.EqualValues(t, err.Error(), "alert validation error: Neither id nor uid is specified in 'notifications' block, type assertion to string failed AlertId: 1 PanelId: 1 DashboardId: 1") }) } diff --git a/public/app/features/alerting/AlertTabCtrl.ts b/public/app/features/alerting/AlertTabCtrl.ts index 6122b23c5f2..0cd209d1473 100644 --- a/public/app/features/alerting/AlertTabCtrl.ts +++ b/public/app/features/alerting/AlertTabCtrl.ts @@ -264,12 +264,14 @@ export class AlertTabCtrl { } checkFrequency() { - if (!this.alert.frequency) { + this.frequencyWarning = ''; + + if (!(this.alert.frequency || '').match(/^\d+([dhms])$/)) { + this.frequencyWarning = + 'Invalid frequency, has to be numeric followed by one of the following units: "d, h, m, s"'; return; } - this.frequencyWarning = ''; - try { const frequencySecs = rangeUtil.intervalToSeconds(this.alert.frequency); if (frequencySecs < this.alertingMinIntervalSecs) { diff --git a/public/app/features/alerting/partials/alert_tab.html b/public/app/features/alerting/partials/alert_tab.html index 1a232d9c27c..6a4aceff323 100644 --- a/public/app/features/alerting/partials/alert_tab.html +++ b/public/app/features/alerting/partials/alert_tab.html @@ -30,6 +30,7 @@ ng-model="ctrl.alert.for" spellcheck="false" placeholder="5m" + ng-pattern="/(^\d+([dhms])$)|(0)|(^$)/" /> If an alert rule has a configured and the query violates the configured threshold, then it goes from OK