diff --git a/conf/provisioning/alerting/sample.yaml b/conf/provisioning/alerting/sample.yaml index b6a58e392be..ec41552e531 100644 --- a/conf/provisioning/alerting/sample.yaml +++ b/conf/provisioning/alerting/sample.yaml @@ -13,7 +13,7 @@ apiVersion: 1 # interval: 60s # # list of rules that are part of the rule group # rules: -# # unique identifier for the rule +# # unique identifier for the rule. Should not exceed 40 symbols. Only letters, numbers, - (hyphen), and _ (underscore) allowed. # - uid: my_id_1 # # title of the rule, will be displayed in the UI # title: my_first_rule @@ -82,7 +82,7 @@ apiVersion: 1 # # name of the contact point # name: cp_1 # receivers: -# # unique identifier for the receiver +# # unique identifier for the receiver. Should not exceed 40 symbols. Only letters, numbers, - (hyphen), and _ (underscore) allowed. # - uid: first_uid # # type of the receiver # type: prometheus-alertmanager diff --git a/docs/sources/alerting/set-up/provision-alerting-resources/file-provisioning/index.md b/docs/sources/alerting/set-up/provision-alerting-resources/file-provisioning/index.md index a699192b949..9e47b176a4a 100644 --- a/docs/sources/alerting/set-up/provision-alerting-resources/file-provisioning/index.md +++ b/docs/sources/alerting/set-up/provision-alerting-resources/file-provisioning/index.md @@ -65,7 +65,7 @@ groups: interval: 60s # list of rules that are part of the rule group rules: - # unique identifier for the rule + # unique identifier for the rule. Should not exceed 40 symbols. Only letters, numbers, - (hyphen), and _ (underscore) allowed. - uid: my_id_1 # title of the rule that will be displayed in the UI title: my_first_rule @@ -158,7 +158,7 @@ contactPoints: # name of the contact point name: cp_1 receivers: - # unique identifier for the receiver + # unique identifier for the receiver. Should not exceed 40 symbols. Only letters, numbers, - (hyphen), and _ (underscore) allowed. - uid: first_uid # type of the receiver type: prometheus-alertmanager diff --git a/pkg/services/ngalert/api/api_provisioning_test.go b/pkg/services/ngalert/api/api_provisioning_test.go index d856bc805e7..245e80ba0f6 100644 --- a/pkg/services/ngalert/api/api_provisioning_test.go +++ b/pkg/services/ngalert/api/api_provisioning_test.go @@ -33,6 +33,7 @@ import ( secrets_fakes "github.com/grafana/grafana/pkg/services/secrets/fakes" "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/setting" + "github.com/grafana/grafana/pkg/util" "github.com/grafana/grafana/pkg/web" ) @@ -283,7 +284,7 @@ func TestProvisioningApi(t *testing.T) { t.Run("PUT sets expected fields with no provenance", func(t *testing.T) { sut := createProvisioningSrvSut(t) - uid := t.Name() + uid := util.GenerateShortUID() rule := createTestAlertRule("rule", 1) rule.UID = uid insertRuleInOrg(t, sut, rule, 3) diff --git a/pkg/services/ngalert/api/tooling/api.json b/pkg/services/ngalert/api/tooling/api.json index c84c77b6129..b83aac29945 100644 --- a/pkg/services/ngalert/api/tooling/api.json +++ b/pkg/services/ngalert/api/tooling/api.json @@ -2663,6 +2663,9 @@ "type": "string" }, "uid": { + "maxLength": 40, + "minLength": 1, + "pattern": "^[a-zA-Z0-9-_]+$", "type": "string" }, "updated": { diff --git a/pkg/services/ngalert/api/tooling/definitions/provisioning_alert_rules.go b/pkg/services/ngalert/api/tooling/definitions/provisioning_alert_rules.go index 71d2835ea00..efcf09d233b 100644 --- a/pkg/services/ngalert/api/tooling/definitions/provisioning_alert_rules.go +++ b/pkg/services/ngalert/api/tooling/definitions/provisioning_alert_rules.go @@ -108,7 +108,11 @@ type AlertRuleHeaders struct { type ProvisionedAlertRules []ProvisionedAlertRule type ProvisionedAlertRule struct { - ID int64 `json:"id"` + ID int64 `json:"id"` + // required: false + // minLength: 1 + // maxLength: 40 + // pattern: ^[a-zA-Z0-9-_]+$ UID string `json:"uid"` // required: true OrgID int64 `json:"orgID"` diff --git a/pkg/services/ngalert/api/tooling/definitions/provisioning_contactpoints.go b/pkg/services/ngalert/api/tooling/definitions/provisioning_contactpoints.go index ab00b540925..6f9d02354e3 100644 --- a/pkg/services/ngalert/api/tooling/definitions/provisioning_contactpoints.go +++ b/pkg/services/ngalert/api/tooling/definitions/provisioning_contactpoints.go @@ -81,6 +81,10 @@ type ContactPoints []EmbeddedContactPoint type EmbeddedContactPoint struct { // UID is the unique identifier of the contact point. The UID can be // set by the user. + // required: false + // minLength: 1 + // maxLength: 40 + // pattern: ^[a-zA-Z0-9\-\_]+$ // example: my_external_reference UID string `json:"uid"` // Name is used as grouping key in the UI. Contact points with the diff --git a/pkg/services/ngalert/api/tooling/post.json b/pkg/services/ngalert/api/tooling/post.json index c5fff6f18c1..cf7813b0e6b 100644 --- a/pkg/services/ngalert/api/tooling/post.json +++ b/pkg/services/ngalert/api/tooling/post.json @@ -764,6 +764,9 @@ "uid": { "description": "UID is the unique identifier of the contact point. The UID can be\nset by the user.", "example": "my_external_reference", + "maxLength": 40, + "minLength": 1, + "pattern": "^[a-zA-Z0-9\\-\\_]+$", "type": "string" } }, @@ -2663,6 +2666,9 @@ "type": "string" }, "uid": { + "maxLength": 40, + "minLength": 1, + "pattern": "^[a-zA-Z0-9-_]+$", "type": "string" }, "updated": { diff --git a/pkg/services/ngalert/api/tooling/spec.json b/pkg/services/ngalert/api/tooling/spec.json index 3010a0b1153..8b1bd684db7 100644 --- a/pkg/services/ngalert/api/tooling/spec.json +++ b/pkg/services/ngalert/api/tooling/spec.json @@ -3625,6 +3625,9 @@ "uid": { "description": "UID is the unique identifier of the contact point. The UID can be\nset by the user.", "type": "string", + "maxLength": 40, + "minLength": 1, + "pattern": "^[a-zA-Z0-9\\-\\_]+$", "example": "my_external_reference" } } @@ -5534,7 +5537,10 @@ "example": "Always firing" }, "uid": { - "type": "string" + "type": "string", + "maxLength": 40, + "minLength": 1, + "pattern": "^[a-zA-Z0-9-_]+$" }, "updated": { "type": "string", diff --git a/pkg/services/ngalert/provisioning/alert_rules.go b/pkg/services/ngalert/provisioning/alert_rules.go index 524a0a0bc0e..0e6c2bb6e9f 100644 --- a/pkg/services/ngalert/provisioning/alert_rules.go +++ b/pkg/services/ngalert/provisioning/alert_rules.go @@ -112,6 +112,8 @@ func (service *AlertRuleService) GetAlertRuleWithFolderTitle(ctx context.Context func (service *AlertRuleService) CreateAlertRule(ctx context.Context, rule models.AlertRule, provenance models.Provenance, userID int64) (models.AlertRule, error) { if rule.UID == "" { rule.UID = util.GenerateShortUID() + } else if err := util.ValidateUID(rule.UID); err != nil { + return models.AlertRule{}, errors.Join(models.ErrAlertRuleFailedValidation, fmt.Errorf("cannot create rule with UID '%s': %w", rule.UID, err)) } interval, err := service.ruleStore.GetRuleGroupInterval(ctx, rule.OrgID, rule.NamespaceUID, rule.RuleGroup) // if the alert group does not exists we just use the default interval diff --git a/pkg/services/ngalert/provisioning/alert_rules_test.go b/pkg/services/ngalert/provisioning/alert_rules_test.go index 67f8d731512..be924adef91 100644 --- a/pkg/services/ngalert/provisioning/alert_rules_test.go +++ b/pkg/services/ngalert/provisioning/alert_rules_test.go @@ -4,12 +4,15 @@ import ( "context" "encoding/json" "strconv" + "strings" "testing" "time" - "github.com/grafana/grafana/pkg/expr" "github.com/stretchr/testify/require" + "github.com/grafana/grafana/pkg/expr" + "github.com/grafana/grafana/pkg/util" + "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/ngalert/models" @@ -21,21 +24,6 @@ func TestAlertRuleService(t *testing.T) { ruleService := createAlertRuleService(t) var orgID int64 = 1 - t.Run("alert rule creation should return the created id", func(t *testing.T) { - rule, err := ruleService.CreateAlertRule(context.Background(), dummyRule("test#1", orgID), models.ProvenanceNone, 0) - require.NoError(t, err) - require.NotEqual(t, 0, rule.ID, "expected to get the created id and not the zero value") - }) - - t.Run("alert rule creation should set the right provenance", func(t *testing.T) { - rule, err := ruleService.CreateAlertRule(context.Background(), dummyRule("test#2", orgID), models.ProvenanceAPI, 0) - require.NoError(t, err) - - _, provenance, err := ruleService.GetAlertRule(context.Background(), orgID, rule.UID) - require.NoError(t, err) - require.Equal(t, models.ProvenanceAPI, provenance) - }) - t.Run("group creation should set the right provenance", func(t *testing.T) { group := createDummyGroup("group-test-1", orgID) err := ruleService.ReplaceRuleGroup(context.Background(), orgID, group, 0, models.ProvenanceAPI) @@ -531,6 +519,45 @@ func TestAlertRuleService(t *testing.T) { }) } +func TestCreateAlertRule(t *testing.T) { + ruleService := createAlertRuleService(t) + var orgID int64 = 1 + + t.Run("should return the created id", func(t *testing.T) { + rule, err := ruleService.CreateAlertRule(context.Background(), dummyRule("test#1", orgID), models.ProvenanceNone, 0) + require.NoError(t, err) + require.NotEqual(t, 0, rule.ID, "expected to get the created id and not the zero value") + }) + + t.Run("should set the right provenance", func(t *testing.T) { + rule, err := ruleService.CreateAlertRule(context.Background(), dummyRule("test#2", orgID), models.ProvenanceAPI, 0) + require.NoError(t, err) + + _, provenance, err := ruleService.GetAlertRule(context.Background(), orgID, rule.UID) + require.NoError(t, err) + require.Equal(t, models.ProvenanceAPI, provenance) + }) + + t.Run("when UID is specified", func(t *testing.T) { + t.Run("return error if it is not valid UID", func(t *testing.T) { + rule := dummyRule("test#3", orgID) + rule.UID = strings.Repeat("1", util.MaxUIDLength+1) + rule, err := ruleService.CreateAlertRule(context.Background(), rule, models.ProvenanceNone, 0) + require.ErrorIs(t, err, models.ErrAlertRuleFailedValidation) + }) + t.Run("should create a new rule with this UID", func(t *testing.T) { + rule := dummyRule("test#3", orgID) + uid := util.GenerateShortUID() + rule.UID = uid + created, err := ruleService.CreateAlertRule(context.Background(), rule, models.ProvenanceNone, 0) + require.NoError(t, err) + require.Equal(t, uid, created.UID) + _, _, err = ruleService.GetAlertRule(context.Background(), orgID, uid) + require.NoError(t, err) + }) + }) +} + func createAlertRuleService(t *testing.T) AlertRuleService { t.Helper() sqlStore := db.InitTestDB(t) diff --git a/pkg/services/ngalert/provisioning/contactpoints.go b/pkg/services/ngalert/provisioning/contactpoints.go index eeb2537596a..010b530e565 100644 --- a/pkg/services/ngalert/provisioning/contactpoints.go +++ b/pkg/services/ngalert/provisioning/contactpoints.go @@ -4,6 +4,7 @@ import ( "context" "encoding/base64" "encoding/json" + "errors" "fmt" "sort" "strings" @@ -190,6 +191,8 @@ func (ecp *ContactPointService) CreateContactPoint(ctx context.Context, orgID in if contactPoint.UID == "" { contactPoint.UID = util.GenerateShortUID() + } else if err := util.ValidateUID(contactPoint.UID); err != nil { + return apimodels.EmbeddedContactPoint{}, errors.Join(ErrValidation, fmt.Errorf("cannot create contact point with UID '%s': %w", contactPoint.UID, err)) } jsonData, err := contactPoint.Settings.MarshalJSON() diff --git a/pkg/services/ngalert/provisioning/contactpoints_test.go b/pkg/services/ngalert/provisioning/contactpoints_test.go index 745a83855f1..7385d9c053b 100644 --- a/pkg/services/ngalert/provisioning/contactpoints_test.go +++ b/pkg/services/ngalert/provisioning/contactpoints_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "strings" "testing" "github.com/prometheus/alertmanager/config" @@ -23,6 +24,7 @@ import ( "github.com/grafana/grafana/pkg/services/secrets/manager" "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/setting" + "github.com/grafana/grafana/pkg/util" ) func TestContactPointService(t *testing.T) { @@ -84,6 +86,16 @@ func TestContactPointService(t *testing.T) { require.Equal(t, customUID, cps[1].UID) }) + t.Run("it's not possible to use invalid UID", func(t *testing.T) { + customUID := strings.Repeat("1", util.MaxUIDLength+1) + sut := createContactPointServiceSut(t, secretsService) + newCp := createTestContactPoint() + newCp.UID = customUID + + _, err := sut.CreateContactPoint(context.Background(), 1, newCp, models.ProvenanceAPI) + require.ErrorIs(t, err, ErrValidation) + }) + t.Run("it's not possible to use the same uid twice", func(t *testing.T) { customUID := "1337" sut := createContactPointServiceSut(t, secretsService) diff --git a/pkg/util/shortid_generator.go b/pkg/util/shortid_generator.go index 03107c70fac..eb841b7bba0 100644 --- a/pkg/util/shortid_generator.go +++ b/pkg/util/shortid_generator.go @@ -1,6 +1,8 @@ package util import ( + "errors" + "fmt" "math/rand" "regexp" "sync" @@ -9,10 +11,18 @@ import ( "github.com/google/uuid" ) +const MaxUIDLength = 40 + var uidrand = rand.New(rand.NewSource(time.Now().UnixNano())) var alphaRunes = []rune("abcdefghijklmnopqrstuvwxyz") var hexLetters = []rune("abcdef") +var ( + ErrUIDTooLong = fmt.Errorf("UID is longer than %d symbols", MaxUIDLength) + ErrUIDFormatInvalid = errors.New("invalid format of UID. Only letters, numbers, '-' and '_' are allowed") + ErrUIDEmpty = fmt.Errorf("UID is empty") +) + // We want to protect our number generator as they are not thread safe. Not using // the mutex could result in panics in certain cases where UIDs would be generated // at the same time. @@ -29,7 +39,7 @@ func IsValidShortUID(uid string) bool { // IsShortUIDTooLong checks if short unique identifier is too long func IsShortUIDTooLong(uid string) bool { - return len(uid) > 40 + return len(uid) > MaxUIDLength } // GenerateShortUID will generate a UUID that can also be a k8s name @@ -51,3 +61,17 @@ func GenerateShortUID() string { } return uuid } + +// ValidateUID checks the format and length of the string and returns error if it does not pass the condition +func ValidateUID(uid string) error { + if len(uid) == 0 { + return ErrUIDEmpty + } + if IsShortUIDTooLong(uid) { + return ErrUIDTooLong + } + if !IsValidShortUID(uid) { + return ErrUIDFormatInvalid + } + return nil +} diff --git a/pkg/util/shortid_generator_test.go b/pkg/util/shortid_generator_test.go index fe7ad937e1c..010219244ae 100644 --- a/pkg/util/shortid_generator_test.go +++ b/pkg/util/shortid_generator_test.go @@ -4,6 +4,7 @@ import ( "sync" "testing" + "cuelang.org/go/pkg/strings" "github.com/google/uuid" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/util/validation" @@ -84,3 +85,47 @@ func TestIsShortUIDTooLong(t *testing.T) { }) } } + +func TestValidateUID(t *testing.T) { + var tests = []struct { + name string + uid string + expected error + }{ + { + name: "no error when string is of correct length", + uid: "f8cc010c-ee72-4681-89d2-d46e1bd47d33", + expected: nil, + }, + { + name: "error when string is empty", + uid: "", + expected: ErrUIDEmpty, + }, + { + name: "error when string is too long", + uid: strings.Repeat("1", MaxUIDLength+1), + expected: ErrUIDTooLong, + }, + { + name: "error when string has invalid characters", + uid: "f8cc010c.ee72.4681;89d2+d46e1bd47d33", + expected: ErrUIDFormatInvalid, + }, + { + name: "error when string has only whitespaces", + uid: " ", + expected: ErrUIDFormatInvalid, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ValidateUID(tt.uid) + if tt.expected == nil { + require.NoError(t, err) + } else { + require.ErrorIs(t, err, tt.expected) + } + }) + } +}