From d1c45321f71130547da56b8d0ef8f48407eccb6e Mon Sep 17 00:00:00 2001 From: gotjosh Date: Thu, 23 Jun 2022 18:51:22 +0100 Subject: [PATCH] Alerting: Don't stop the migration when alert rule tags are invalid (#51253) (#51341) * Alerting: Don't stop the migration when alert rule tags are invalid As we migrate we expect the `alertRuleTags` on a dashboard alert to be a JSON object. However, it seems this is not really validated by Grafana and an user can change the format to something else that the JSON parser is not able to marshal into a `map[string]string`. Let's do a bit better by "attempting" to parse the tags and if we can't we'll simple return an empty map. The data is still there so if the user wishes they can go back, fix the data and attemp the migration again. (cherry picked from commit 90646e7f414107fffc1b2fd7f0aa084d3cb43446) --- pkg/services/ngalert/CHANGELOG.md | 8 +++++ .../sqlstore/migrations/ualert/alert_rule.go | 9 +++-- .../migrations/ualert/alert_rule_test.go | 34 +++++++++++++++++++ .../sqlstore/migrations/ualert/dash_alert.go | 4 +-- 4 files changed, 50 insertions(+), 5 deletions(-) diff --git a/pkg/services/ngalert/CHANGELOG.md b/pkg/services/ngalert/CHANGELOG.md index 83c5c9dd0d5..aa88a962395 100644 --- a/pkg/services/ngalert/CHANGELOG.md +++ b/pkg/services/ngalert/CHANGELOG.md @@ -45,6 +45,14 @@ Scopes must have an order to ensure consistency and ease of search, this helps u ## Grafana Alerting - main / unreleased +- [CHANGE] Rule API to reject request to update rules that affects provisioned rules #50835 +- [FEATURE] Add first Grafana reserved label, grafana_folder is created during runtime and stores an alert's folder/namespace title #50262 +- [FEATURE] use optimistic lock by version field when updating alert rules #50274 +- [BUGFIX] State manager to use tick time to determine stale states #50991 +- [ENHANCEMENT] Scheduler: Drop ticks if rule evaluation is too slow and adds a metric grafana_alerting_schedule_rule_evaluations_missed_total to track missed evaluations per rule #48885 +- [ENHANCEMENT] Ticker to tick at predictable time #50197 +- [ENHANCEMENT] Migration: Don't stop the migration when failing to parse alert rule tags #51253 + ## 9.0.0 - [ENHANCEMENT] Scheduler: Ticker expose new metrics. In legacy, metrics are prefixed with `legacy_` #47828, #48190 diff --git a/pkg/services/sqlstore/migrations/ualert/alert_rule.go b/pkg/services/sqlstore/migrations/ualert/alert_rule.go index cefa976130a..a0471c5ca6e 100644 --- a/pkg/services/sqlstore/migrations/ualert/alert_rule.go +++ b/pkg/services/sqlstore/migrations/ualert/alert_rule.go @@ -5,6 +5,7 @@ import ( "fmt" "time" + "github.com/grafana/grafana/pkg/components/simplejson" "github.com/grafana/grafana/pkg/expr" legacymodels "github.com/grafana/grafana/pkg/models" ngmodels "github.com/grafana/grafana/pkg/services/ngalert/models" @@ -77,9 +78,11 @@ func (a *alertRule) makeVersion() *alertRuleVersion { } func addMigrationInfo(da *dashAlert) (map[string]string, map[string]string) { - lbls := da.ParsedSettings.AlertRuleTags - if lbls == nil { - lbls = make(map[string]string) + tagsMap := simplejson.NewFromAny(da.ParsedSettings.AlertRuleTags).MustMap() + lbls := make(map[string]string, len(tagsMap)) + + for k, v := range tagsMap { + lbls[k] = simplejson.NewFromAny(v).MustString() } annotations := make(map[string]string, 3) diff --git a/pkg/services/sqlstore/migrations/ualert/alert_rule_test.go b/pkg/services/sqlstore/migrations/ualert/alert_rule_test.go index b3d16922a1e..c8d8c195fc2 100644 --- a/pkg/services/sqlstore/migrations/ualert/alert_rule_test.go +++ b/pkg/services/sqlstore/migrations/ualert/alert_rule_test.go @@ -1,6 +1,7 @@ package ualert import ( + "encoding/json" "testing" "github.com/grafana/grafana/pkg/components/simplejson" @@ -50,3 +51,36 @@ func TestMigrateAlertRuleQueries(t *testing.T) { }) } } + +func TestAddMigrationInfo(t *testing.T) { + tt := []struct { + name string + tagsJSON string + expectedLabels map[string]string + expectedAnnotations map[string]string + }{ + { + name: "when alert rule tags are a JSON array, they're ignored.", + tagsJSON: `{ "alertRuleTags": ["one", "two", "three", "four"] }`, + expectedLabels: map[string]string{}, + expectedAnnotations: map[string]string{"__alertId__": "0", "__dashboardUid__": "", "__panelId__": "0"}, + }, + { + name: "when alert rule tags are a JSON object", + tagsJSON: `{ "alertRuleTags": { "key": "value", "key2": "value2" } }`, + expectedLabels: map[string]string{"key": "value", "key2": "value2"}, + expectedAnnotations: map[string]string{"__alertId__": "0", "__dashboardUid__": "", "__panelId__": "0"}, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + var settings dashAlertSettings + require.NoError(t, json.Unmarshal([]byte(tc.tagsJSON), &settings)) + + labels, annotations := addMigrationInfo(&dashAlert{ParsedSettings: &settings}) + require.Equal(t, tc.expectedLabels, labels) + require.Equal(t, tc.expectedAnnotations, annotations) + }) + } +} diff --git a/pkg/services/sqlstore/migrations/ualert/dash_alert.go b/pkg/services/sqlstore/migrations/ualert/dash_alert.go index f527744b31c..f6276507e33 100644 --- a/pkg/services/sqlstore/migrations/ualert/dash_alert.go +++ b/pkg/services/sqlstore/migrations/ualert/dash_alert.go @@ -40,7 +40,7 @@ WHERE org_id IN (SELECT id from org) AND dashboard_id IN (SELECT id from dashboard) ` -// slurpDashAlerts loads all alerts from the alert database table into the +// slurpDashAlerts loads all alerts from the alert database table into // the dashAlert type. If there are alerts that belong to either organization or dashboard that does not exist, those alerts will not be returned/ // Additionally it unmarshals the json settings for the alert into the // ParsedSettings property of the dash alert. @@ -68,7 +68,7 @@ type dashAlertSettings struct { NoDataState string `json:"noDataState"` ExecutionErrorState string `json:"executionErrorState"` Conditions []dashAlertCondition `json:"conditions"` - AlertRuleTags map[string]string `json:"alertRuleTags"` + AlertRuleTags interface{} `json:"alertRuleTags"` Notifications []dashAlertNot `json:"notifications"` }