Alerting: Don't stop the migration when alert rule tags are invalid (#51253) (#51339)

* 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 90646e7f41)

pkg/services/ngalert/CHANGELOG.md

@@ -45,6 +45,16 @@ 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
   - `grafana_alerting_ticker_last_consumed_tick_timestamp_seconds`
   - `grafana_alerting_ticker_next_tick_timestamp_seconds`
@@ -60,10 +70,11 @@ Scopes must have an order to ensure consistency and ease of search, this helps u
 - [BUGFIX] RBAC: replace create\update\delete actions for notification policies by alert.notifications:write #49185
 - [BUGFIX] Fix access to alerts for Viewer role with editor permissions in folder #49270
 - [ENHANCEMENT] Scheduler: ticker to support stopping #48142
+- [ENHANCEMENT] Migration: Don't stop the migration when failing to parse alert rule tags #51253
 
 ## 8.5.5
 
-- [BUGFIX] Alerting: Remove double quotes from double quoted matchers #xxxx
+- [BUGFIX] Alerting: Remove double quotes from double quoted matchers #50038
 
 ## 8.5.3
 

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)

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)
+		})
+	}
+}

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"`
 }