From 9f45e2e7060e3878706e70c1465f1403b6e96082 Mon Sep 17 00:00:00 2001 From: Alexander Weaver Date: Tue, 13 Sep 2022 13:53:09 -0500 Subject: [PATCH] Alerting: Fix legacy migration crash when rule name is too long (#55053) * Extract standardized UID field length to constant * Extract default length to constant * Truncate rule names that are too long * Add tests for name normalization * Fix whitespace lint error * Another linter fix * Empty commit to kick build --- .../sqlstore/migrations/ualert/alert_rule.go | 19 +++++- .../migrations/ualert/alert_rule_test.go | 46 +++++++++++++++ .../sqlstore/migrations/ualert/tables.go | 58 ++++++++++--------- 3 files changed, 94 insertions(+), 29 deletions(-) diff --git a/pkg/services/sqlstore/migrations/ualert/alert_rule.go b/pkg/services/sqlstore/migrations/ualert/alert_rule.go index 0e82c3aacc1..653f62bcf5f 100644 --- a/pkg/services/sqlstore/migrations/ualert/alert_rule.go +++ b/pkg/services/sqlstore/migrations/ualert/alert_rule.go @@ -99,7 +99,8 @@ func addMigrationInfo(da *dashAlert) (map[string]string, map[string]string) { func (m *migration) makeAlertRule(cond condition, da dashAlert, folderUID string) (*alertRule, error) { lbls, annotations := addMigrationInfo(&da) - lbls["alertname"] = da.Name + name := normalizeRuleName(da.Name) + lbls["alertname"] = name annotations["message"] = da.Message var err error @@ -110,14 +111,14 @@ func (m *migration) makeAlertRule(cond condition, da dashAlert, folderUID string ar := &alertRule{ OrgID: da.OrgId, - Title: da.Name, // TODO: Make sure all names are unique, make new name on constraint insert error. + Title: name, // TODO: Make sure all names are unique, make new name on constraint insert error. UID: util.GenerateShortUID(), Condition: cond.Condition, Data: data, IntervalSeconds: ruleAdjustInterval(da.Frequency), Version: 1, NamespaceUID: folderUID, // Folder already created, comes from env var. - RuleGroup: da.Name, + RuleGroup: name, For: duration(da.For), Updated: time.Now().UTC(), Annotations: annotations, @@ -277,3 +278,15 @@ func transExecErr(s string) (string, error) { } return "", fmt.Errorf("unrecognized Execution Error setting %v", s) } + +func normalizeRuleName(daName string) string { + // If we have to truncate, we're losing data and so there is higher risk of uniqueness conflicts. + // Append a UID to the suffix to forcibly break any collisions. + if len(daName) > DefaultFieldMaxLength { + uniq := util.GenerateShortUID() + trunc := DefaultFieldMaxLength - 1 - len(uniq) + daName = daName[:trunc] + "_" + uniq + } + + return daName +} diff --git a/pkg/services/sqlstore/migrations/ualert/alert_rule_test.go b/pkg/services/sqlstore/migrations/ualert/alert_rule_test.go index c8d8c195fc2..9d98e991966 100644 --- a/pkg/services/sqlstore/migrations/ualert/alert_rule_test.go +++ b/pkg/services/sqlstore/migrations/ualert/alert_rule_test.go @@ -2,6 +2,7 @@ package ualert import ( "encoding/json" + "strings" "testing" "github.com/grafana/grafana/pkg/components/simplejson" @@ -84,3 +85,48 @@ func TestAddMigrationInfo(t *testing.T) { }) } } + +func TestMakeAlertRule(t *testing.T) { + t.Run("when mapping rule names", func(t *testing.T) { + t.Run("leaves basic names untouched", func(t *testing.T) { + m := newTestMigration(t) + da := createTestDashAlert() + cnd := createTestDashAlertCondition() + + ar, err := m.makeAlertRule(cnd, da, "folder") + + require.NoError(t, err) + require.Equal(t, da.Name, ar.Title) + require.Equal(t, ar.Title, ar.RuleGroup) + }) + + t.Run("truncates very long names to max length", func(t *testing.T) { + m := newTestMigration(t) + da := createTestDashAlert() + da.Name = strings.Repeat("a", DefaultFieldMaxLength+1) + cnd := createTestDashAlertCondition() + + ar, err := m.makeAlertRule(cnd, da, "folder") + + require.NoError(t, err) + require.Len(t, ar.Title, DefaultFieldMaxLength) + uniq := ar.Title[len(ar.Title)-11:] + require.Regexp(t, "^_.{10}$", uniq) + require.Equal(t, ar.Title, ar.RuleGroup) + }) + }) +} + +func createTestDashAlert() dashAlert { + return dashAlert{ + Id: 1, + Name: "test", + ParsedSettings: &dashAlertSettings{}, + } +} + +func createTestDashAlertCondition() condition { + return condition{ + Condition: "A", + } +} diff --git a/pkg/services/sqlstore/migrations/ualert/tables.go b/pkg/services/sqlstore/migrations/ualert/tables.go index e0f9adbf68a..a5d51b0f50c 100644 --- a/pkg/services/sqlstore/migrations/ualert/tables.go +++ b/pkg/services/sqlstore/migrations/ualert/tables.go @@ -6,6 +6,12 @@ import ( "github.com/grafana/grafana/pkg/services/sqlstore/migrator" ) +// DefaultFieldMaxLength is the standard size for most user-settable string fields in Alerting. Use this for typical string fields, unless you have a special reason not to. +const DefaultFieldMaxLength = 190 + +// UIDMaxLength is the standard size for fields that contain UIDs. +const UIDMaxLength = 40 + // AddMigration defines database migrations. func AddTablesMigrations(mg *migrator.Migrator) { AddAlertDefinitionMigrations(mg, 60) @@ -38,13 +44,13 @@ func AddAlertDefinitionMigrations(mg *migrator.Migrator, defaultIntervalSeconds Columns: []*migrator.Column{ {Name: "id", Type: migrator.DB_BigInt, IsPrimaryKey: true, IsAutoIncrement: true}, {Name: "org_id", Type: migrator.DB_BigInt, Nullable: false}, - {Name: "title", Type: migrator.DB_NVarchar, Length: 190, Nullable: false}, - {Name: "condition", Type: migrator.DB_NVarchar, Length: 190, Nullable: false}, + {Name: "title", Type: migrator.DB_NVarchar, Length: DefaultFieldMaxLength, Nullable: false}, + {Name: "condition", Type: migrator.DB_NVarchar, Length: DefaultFieldMaxLength, Nullable: false}, {Name: "data", Type: migrator.DB_Text, Nullable: false}, {Name: "updated", Type: migrator.DB_DateTime, Nullable: false}, {Name: "interval_seconds", Type: migrator.DB_BigInt, Nullable: false, Default: fmt.Sprintf("%d", defaultIntervalSeconds)}, {Name: "version", Type: migrator.DB_Int, Nullable: false, Default: "0"}, - {Name: "uid", Type: migrator.DB_NVarchar, Length: 40, Nullable: false, Default: "0"}, + {Name: "uid", Type: migrator.DB_NVarchar, Length: UIDMaxLength, Nullable: false, Default: "0"}, }, Indices: []*migrator.Index{ {Cols: []string{"org_id", "title"}, Type: migrator.IndexType}, @@ -87,13 +93,13 @@ func AddAlertDefinitionVersionMigrations(mg *migrator.Migrator) { Columns: []*migrator.Column{ {Name: "id", Type: migrator.DB_BigInt, IsPrimaryKey: true, IsAutoIncrement: true}, {Name: "alert_definition_id", Type: migrator.DB_BigInt}, - {Name: "alert_definition_uid", Type: migrator.DB_NVarchar, Length: 40, Nullable: false, Default: "0"}, + {Name: "alert_definition_uid", Type: migrator.DB_NVarchar, Length: UIDMaxLength, Nullable: false, Default: "0"}, {Name: "parent_version", Type: migrator.DB_Int, Nullable: false}, {Name: "restored_from", Type: migrator.DB_Int, Nullable: false}, {Name: "version", Type: migrator.DB_Int, Nullable: false}, {Name: "created", Type: migrator.DB_DateTime, Nullable: false}, - {Name: "title", Type: migrator.DB_NVarchar, Length: 190, Nullable: false}, - {Name: "condition", Type: migrator.DB_NVarchar, Length: 190, Nullable: false}, + {Name: "title", Type: migrator.DB_NVarchar, Length: DefaultFieldMaxLength, Nullable: false}, + {Name: "condition", Type: migrator.DB_NVarchar, Length: DefaultFieldMaxLength, Nullable: false}, {Name: "data", Type: migrator.DB_Text, Nullable: false}, {Name: "interval_seconds", Type: migrator.DB_BigInt, Nullable: false}, }, @@ -116,10 +122,10 @@ func AlertInstanceMigration(mg *migrator.Migrator) { Name: "alert_instance", Columns: []*migrator.Column{ {Name: "def_org_id", Type: migrator.DB_BigInt, Nullable: false}, - {Name: "def_uid", Type: migrator.DB_NVarchar, Length: 40, Nullable: false, Default: "0"}, + {Name: "def_uid", Type: migrator.DB_NVarchar, Length: UIDMaxLength, Nullable: false, Default: "0"}, {Name: "labels", Type: migrator.DB_Text, Nullable: false}, - {Name: "labels_hash", Type: migrator.DB_NVarchar, Length: 190, Nullable: false}, - {Name: "current_state", Type: migrator.DB_NVarchar, Length: 190, Nullable: false}, + {Name: "labels_hash", Type: migrator.DB_NVarchar, Length: DefaultFieldMaxLength, Nullable: false}, + {Name: "current_state", Type: migrator.DB_NVarchar, Length: DefaultFieldMaxLength, Nullable: false}, {Name: "current_state_since", Type: migrator.DB_BigInt, Nullable: false}, {Name: "last_eval_time", Type: migrator.DB_BigInt, Nullable: false}, }, @@ -158,7 +164,7 @@ func AlertInstanceMigration(mg *migrator.Migrator) { mg.AddMigration("add current_reason column related to current_state", migrator.NewAddColumnMigration(alertInstance, &migrator.Column{ - Name: "current_reason", Type: migrator.DB_NVarchar, Length: 190, Nullable: true, + Name: "current_reason", Type: migrator.DB_NVarchar, Length: DefaultFieldMaxLength, Nullable: true, })) } @@ -168,16 +174,16 @@ func AddAlertRuleMigrations(mg *migrator.Migrator, defaultIntervalSeconds int64) Columns: []*migrator.Column{ {Name: "id", Type: migrator.DB_BigInt, IsPrimaryKey: true, IsAutoIncrement: true}, {Name: "org_id", Type: migrator.DB_BigInt, Nullable: false}, - {Name: "title", Type: migrator.DB_NVarchar, Length: 190, Nullable: false}, - {Name: "condition", Type: migrator.DB_NVarchar, Length: 190, Nullable: false}, + {Name: "title", Type: migrator.DB_NVarchar, Length: DefaultFieldMaxLength, Nullable: false}, + {Name: "condition", Type: migrator.DB_NVarchar, Length: DefaultFieldMaxLength, Nullable: false}, {Name: "data", Type: migrator.DB_Text, Nullable: false}, {Name: "updated", Type: migrator.DB_DateTime, Nullable: false}, {Name: "interval_seconds", Type: migrator.DB_BigInt, Nullable: false, Default: fmt.Sprintf("%d", defaultIntervalSeconds)}, {Name: "version", Type: migrator.DB_Int, Nullable: false, Default: "0"}, - {Name: "uid", Type: migrator.DB_NVarchar, Length: 40, Nullable: false, Default: "0"}, + {Name: "uid", Type: migrator.DB_NVarchar, Length: UIDMaxLength, Nullable: false, Default: "0"}, // the following fields will correspond to a dashboard (or folder) UIID - {Name: "namespace_uid", Type: migrator.DB_NVarchar, Length: 40, Nullable: false}, - {Name: "rule_group", Type: migrator.DB_NVarchar, Length: 190, Nullable: false}, + {Name: "namespace_uid", Type: migrator.DB_NVarchar, Length: UIDMaxLength, Nullable: false}, + {Name: "rule_group", Type: migrator.DB_NVarchar, Length: DefaultFieldMaxLength, Nullable: false}, {Name: "no_data_state", Type: migrator.DB_NVarchar, Length: 15, Nullable: false, Default: "'NoData'"}, {Name: "exec_err_state", Type: migrator.DB_NVarchar, Length: 15, Nullable: false, Default: "'Alerting'"}, }, @@ -259,16 +265,16 @@ func AddAlertRuleVersionMigrations(mg *migrator.Migrator) { Columns: []*migrator.Column{ {Name: "id", Type: migrator.DB_BigInt, IsPrimaryKey: true, IsAutoIncrement: true}, {Name: "rule_org_id", Type: migrator.DB_BigInt}, - {Name: "rule_uid", Type: migrator.DB_NVarchar, Length: 40, Nullable: false, Default: "0"}, + {Name: "rule_uid", Type: migrator.DB_NVarchar, Length: UIDMaxLength, Nullable: false, Default: "0"}, // the following fields will correspond to a dashboard (or folder) UID - {Name: "rule_namespace_uid", Type: migrator.DB_NVarchar, Length: 40, Nullable: false}, - {Name: "rule_group", Type: migrator.DB_NVarchar, Length: 190, Nullable: false}, + {Name: "rule_namespace_uid", Type: migrator.DB_NVarchar, Length: UIDMaxLength, Nullable: false}, + {Name: "rule_group", Type: migrator.DB_NVarchar, Length: DefaultFieldMaxLength, Nullable: false}, {Name: "parent_version", Type: migrator.DB_Int, Nullable: false}, {Name: "restored_from", Type: migrator.DB_Int, Nullable: false}, {Name: "version", Type: migrator.DB_Int, Nullable: false}, {Name: "created", Type: migrator.DB_DateTime, Nullable: false}, - {Name: "title", Type: migrator.DB_NVarchar, Length: 190, Nullable: false}, - {Name: "condition", Type: migrator.DB_NVarchar, Length: 190, Nullable: false}, + {Name: "title", Type: migrator.DB_NVarchar, Length: DefaultFieldMaxLength, Nullable: false}, + {Name: "condition", Type: migrator.DB_NVarchar, Length: DefaultFieldMaxLength, Nullable: false}, {Name: "data", Type: migrator.DB_Text, Nullable: false}, {Name: "interval_seconds", Type: migrator.DB_BigInt, Nullable: false}, {Name: "no_data_state", Type: migrator.DB_NVarchar, Length: 15, Nullable: false, Default: "'NoData'"}, @@ -368,9 +374,9 @@ func AddProvisioningMigrations(mg *migrator.Migrator) { Columns: []*migrator.Column{ {Name: "id", Type: migrator.DB_BigInt, IsPrimaryKey: true, IsAutoIncrement: true}, {Name: "org_id", Type: migrator.DB_BigInt, Nullable: false}, - {Name: "record_key", Type: migrator.DB_NVarchar, Length: 190, Nullable: false}, - {Name: "record_type", Type: migrator.DB_NVarchar, Length: 190, Nullable: false}, - {Name: "provenance", Type: migrator.DB_NVarchar, Length: 190, Nullable: false}, + {Name: "record_key", Type: migrator.DB_NVarchar, Length: DefaultFieldMaxLength, Nullable: false}, + {Name: "record_type", Type: migrator.DB_NVarchar, Length: DefaultFieldMaxLength, Nullable: false}, + {Name: "provenance", Type: migrator.DB_NVarchar, Length: DefaultFieldMaxLength, Nullable: false}, }, Indices: []*migrator.Index{ {Cols: []string{"record_type", "record_key", "org_id"}, Type: migrator.UniqueIndex}, @@ -386,9 +392,9 @@ func AddAlertImageMigrations(mg *migrator.Migrator) { Name: "alert_image", Columns: []*migrator.Column{ {Name: "id", Type: migrator.DB_BigInt, IsPrimaryKey: true, IsAutoIncrement: true}, - {Name: "token", Type: migrator.DB_NVarchar, Length: 190, Nullable: false}, - {Name: "path", Type: migrator.DB_NVarchar, Length: 190, Nullable: false}, - {Name: "url", Type: migrator.DB_NVarchar, Length: 190, Nullable: false}, + {Name: "token", Type: migrator.DB_NVarchar, Length: DefaultFieldMaxLength, Nullable: false}, + {Name: "path", Type: migrator.DB_NVarchar, Length: DefaultFieldMaxLength, Nullable: false}, + {Name: "url", Type: migrator.DB_NVarchar, Length: DefaultFieldMaxLength, Nullable: false}, {Name: "created_at", Type: migrator.DB_DateTime, Nullable: false}, {Name: "expires_at", Type: migrator.DB_DateTime, Nullable: false}, },