public/grafana
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Alerting: Prohibit receivers with empty name (#113064)

Browse Source
This commit is contained in:
Yuri Tseretyan
2025-10-29 16:30:38 -04:00
committed by GitHub
parent f2404361bf
commit a4df6c8bb9
4 changed files with 121 additions and 64 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/services/ngalert/models/receivers.go
+3
View File
@@ -130,6 +130,9 @@ func (r *Receiver) WithExistingSecureFields(existing *Receiver, integrationSecur
// Validate validates all integration settings, ensuring that the integrations are correctly configured.
func (r *Receiver) Validate(decryptFn DecryptFn) error {
var errs []error
if r.Name == "" {
errs = append(errs, fmt.Errorf("name should not be an empty string"))
}
for _, integration := range r.Integrations {
if err := integration.Validate(decryptFn); err != nil {
errs = append(errs, err)
pkg/services/ngalert/notifier/receiver_svc_test.go
+16
View File
@@ -476,6 +476,12 @@ func TestReceiverService_Create(t *testing.T) {
},
},
},
{
name: "receiver with empty name fails",
user: writer,
receiver: models.CopyReceiverWith(baseReceiver, models.ReceiverMuts.WithName("")),
expectedErr: legacy_storage.ErrReceiverInvalid,
},
} {
t.Run(tc.name, func(t *testing.T) {
sut := createReceiverServiceSut(t, &secretsService)
@@ -901,6 +907,16 @@ func TestReceiverService_UpdateReceiverName(t *testing.T) {
assert.NotNil(t, ruleStore.Calls[0].Args[4])
assert.Falsef(t, ruleStore.Calls[0].Args[5].(bool), "dryrun expected to be false")
})
t.Run("returns ErrReceiverInvalid if empty name", func(t *testing.T) {
ruleStore := &fakeAlertRuleNotificationStore{}
sut := createReceiverServiceSut(t, &secretsService)
sut.ruleNotificationsStore = ruleStore
baseReceiver.Name = ""
_, err := sut.UpdateReceiver(context.Background(), &baseReceiver, nil, writer.GetOrgID(), writer)
require.ErrorIs(t, err, legacy_storage.ErrReceiverInvalid)
})
}
func TestReceiverServiceAC_Read(t *testing.T) {
pkg/services/ngalert/provisioning/contactpoints.go
+13 -8
View File
@@ -309,13 +309,14 @@ func (ecp *ContactPointService) UpdateContactPoint(ctx context.Context, orgID in
return err
}
oldReceiverName, fullRemoval, newReceiverCreated := stitchReceiver(revision.Config, mergedReceiver)
if oldReceiverName == "" {
return fmt.Errorf("contact point with uid '%s' not found", mergedReceiver.UID)
oldReceiverNameRef, fullRemoval, newReceiverCreated := stitchReceiver(revision.Config, mergedReceiver)
if oldReceiverNameRef == nil {
return fmt.Errorf("%w: contact point with uid '%s' not found", ErrNotFound, mergedReceiver.UID)
}
oldReceiverName := *oldReceiverNameRef
err = ecp.xact.InTransaction(ctx, func(ctx context.Context) error {
if mergedReceiver.Name != oldReceiverName {
if mergedReceiver.Name != oldReceiverName && oldReceiverName != "" {
if newReceiverCreated {
// Copy receiver permissions
permissionsUpdated, err := ecp.resourcePermissions.CopyPermissions(ctx, orgID, nil, legacy_storage.NameToUid(oldReceiverName), legacy_storage.NameToUid(mergedReceiver.Name))
@@ -373,12 +374,12 @@ func (ecp *ContactPointService) DeleteContactPoint(ctx context.Context, orgID in
}
}
}
if fullRemoval && revision.ReceiverNameUsedByRoutes(name) {
if fullRemoval && name != "" && revision.ReceiverNameUsedByRoutes(name) {
return ErrContactPointReferenced.Errorf("")
}
return ecp.xact.InTransaction(ctx, func(ctx context.Context) error {
if fullRemoval {
if fullRemoval && name != "" {
used, err := ecp.notificationSettingsStore.ListNotificationSettings(ctx, models.ListNotificationSettingsQuery{OrgID: orgID, ReceiverName: name})
if err != nil {
return fmt.Errorf("failed to query alert rules for reference to the contact point '%s': %w", name, err)
@@ -443,7 +444,7 @@ func (ecp *ContactPointService) encryptValue(value string) (string, error) {
// stitchReceiver modifies a receiver, target, in an alertmanager configStore. It modifies the given configStore in-place.
// Returns true if the configStore was altered in any way, and false otherwise.
// If integration was moved to another group and it was the last in the previous group, the second parameter contains the name of the old group that is gone
func stitchReceiver(cfg *apimodels.PostableUserConfig, target *apimodels.PostableGrafanaReceiver) (oldReceiverName string, fullRemoval bool, newReceiverCreated bool) {
func stitchReceiver(cfg *apimodels.PostableUserConfig, target *apimodels.PostableGrafanaReceiver) (oldReceiverName *string, fullRemoval bool, newReceiverCreated bool) {
// Algorithm to fix up receivers. Receivers are very complex and depend heavily on internal consistency.
// All receivers in a given receiver group have the same name. We must maintain this across renames.
groupLoop:
@@ -451,7 +452,8 @@ groupLoop:
// Does the current group contain the grafana receiver we're interested in?
for i, grafanaReceiver := range receiverGroup.GrafanaManagedReceivers {
if grafanaReceiver.UID == target.UID {
oldReceiverName = receiverGroup.Name
name := receiverGroup.Name
oldReceiverName = &name
// If it's a basic field change, simply replace it. Done!
//
// NOTE:
@@ -519,6 +521,9 @@ groupLoop:
}
func ValidateContactPoint(ctx context.Context, e *apimodels.EmbeddedContactPoint, decryptFunc alertingNotify.GetDecryptedValueFn) error {
if e.Name == "" {
return errors.New("name is required")
}
iType, err := alertingNotify.IntegrationTypeFromString(e.Type)
if err != nil {
return err
pkg/services/ngalert/provisioning/contactpoints_test.go
+89 -56
View File
@@ -114,16 +114,6 @@ func TestIntegrationContactPointService(t *testing.T) {
require.Equal(t, customUID, cps[0].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, redactedUser, 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)
@@ -137,14 +127,40 @@ func TestIntegrationContactPointService(t *testing.T) {
require.Error(t, err)
})
t.Run("create rejects contact points that fail validation", func(t *testing.T) {
t.Run("create rejects invalid contact points", func(t *testing.T) {
sut := createContactPointServiceSut(t, secretsService)
newCp := createTestContactPoint()
newCp.Type = ""
testCases := []struct {
name string
cp func(*definitions.EmbeddedContactPoint)
}{
{
name: "empty type",
cp: func(cp *definitions.EmbeddedContactPoint) {
cp.Type = ""
},
},
{
name: "empty name",
cp: func(cp *definitions.EmbeddedContactPoint) {
cp.Name = ""
},
},
{
name: "invalid UID",
cp: func(cp *definitions.EmbeddedContactPoint) {
cp.UID = strings.Repeat("1", util.MaxUIDLength+1)
},
},
}
_, err := sut.CreateContactPoint(context.Background(), 1, redactedUser, newCp, models.ProvenanceAPI)
require.ErrorIs(t, err, ErrValidation)
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
newCp := createTestContactPoint()
tc.cp(&newCp)
_, err := sut.CreateContactPoint(context.Background(), 1, redactedUser, newCp, models.ProvenanceAPI)
require.ErrorIs(t, err, ErrValidation)
})
}
})
t.Run("create accepts contact point with type in different cases", func(t *testing.T) {
@@ -162,40 +178,57 @@ func TestIntegrationContactPointService(t *testing.T) {
assert.EqualValues(t, slack.Type, got[0].Type)
})
t.Run("update rejects contact points with no settings", func(t *testing.T) {
t.Run("update rejects invalid contact points", func(t *testing.T) {
testCases := []struct {
name string
cp func(*definitions.EmbeddedContactPoint)
}{
{
name: "empty type",
cp: func(cp *definitions.EmbeddedContactPoint) {
cp.Type = ""
},
},
{
name: "empty name",
cp: func(cp *definitions.EmbeddedContactPoint) {
cp.Name = ""
},
},
{
name: "nil settings",
cp: func(cp *definitions.EmbeddedContactPoint) {
cp.Settings = nil
},
},
{
name: "invalid settings after merge",
cp: func(cp *definitions.EmbeddedContactPoint) {
cp.Settings, _ = simplejson.NewJson([]byte(`{}`))
},
},
}
sut := createContactPointServiceSut(t, secretsService)
newCp := createTestContactPoint()
newCp, err := sut.CreateContactPoint(context.Background(), 1, redactedUser, newCp, models.ProvenanceAPI)
require.NoError(t, err)
newCp.Settings = nil
err = sut.UpdateContactPoint(context.Background(), 1, newCp, models.ProvenanceAPI)
require.ErrorIs(t, err, ErrValidation)
})
t.Run("update rejects contact points with no type", func(t *testing.T) {
sut := createContactPointServiceSut(t, secretsService)
newCp := createTestContactPoint()
newCp, err := sut.CreateContactPoint(context.Background(), 1, redactedUser, newCp, models.ProvenanceAPI)
require.NoError(t, err)
newCp.Type = ""
err = sut.UpdateContactPoint(context.Background(), 1, newCp, models.ProvenanceAPI)
require.ErrorIs(t, err, ErrValidation)
})
t.Run("update rejects contact points which fail validation after merging", func(t *testing.T) {
sut := createContactPointServiceSut(t, secretsService)
newCp := createTestContactPoint()
newCp, err := sut.CreateContactPoint(context.Background(), 1, redactedUser, newCp, models.ProvenanceAPI)
require.NoError(t, err)
newCp.Settings, _ = simplejson.NewJson([]byte(`{}`))
err = sut.UpdateContactPoint(context.Background(), 1, newCp, models.ProvenanceAPI)
require.ErrorIs(t, err, ErrValidation)
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
cp := definitions.EmbeddedContactPoint{
UID: newCp.UID,
Name: newCp.Name,
Type: newCp.Type,
Settings: newCp.Settings.DeepCopy(),
DisableResolveMessage: newCp.DisableResolveMessage,
Provenance: newCp.Provenance,
}
tc.cp(&cp)
err = sut.UpdateContactPoint(context.Background(), 1, cp, models.ProvenanceAPI)
require.ErrorIs(t, err, ErrValidation)
})
}
})
t.Run("update accepts contact points with type in another case", func(t *testing.T) {
@@ -592,7 +625,7 @@ func TestStitchReceivers(t *testing.T) {
initial *definitions.PostableUserConfig
new *definitions.PostableGrafanaReceiver
expCfg definitions.PostableApiAlertingConfig
expOldReceiver string
expOldReceiver *string
expCreatedReceiver bool
expFullRemoval bool
}
@@ -603,7 +636,7 @@ func TestStitchReceivers(t *testing.T) {
new: &definitions.PostableGrafanaReceiver{
UID: "does not exist",
},
expOldReceiver: "",
expOldReceiver: nil,
expCfg: createTestConfigWithReceivers().AlertmanagerConfig,
},
{
@@ -613,7 +646,7 @@ func TestStitchReceivers(t *testing.T) {
Name: "receiver-2",
Type: "teams",
},
expOldReceiver: "receiver-2",
expOldReceiver: util.Pointer("receiver-2"),
expCfg: definitions.PostableApiAlertingConfig{
Config: definitions.Config{
Route: &definitions.Route{
@@ -674,7 +707,7 @@ func TestStitchReceivers(t *testing.T) {
Name: "new-receiver",
Type: "slack",
},
expOldReceiver: "receiver-1",
expOldReceiver: util.Pointer("receiver-1"),
expCreatedReceiver: true,
expFullRemoval: true,
expCfg: definitions.PostableApiAlertingConfig{
@@ -737,7 +770,7 @@ func TestStitchReceivers(t *testing.T) {
Name: "receiver-1",
Type: "slack",
},
expOldReceiver: "receiver-2",
expOldReceiver: util.Pointer("receiver-2"),
expCreatedReceiver: false,
expCfg: definitions.PostableApiAlertingConfig{
Config: definitions.Config{
@@ -858,7 +891,7 @@ func TestStitchReceivers(t *testing.T) {
Name: "receiver-2",
Type: "slack",
},
expOldReceiver: "receiver-1",
expOldReceiver: util.Pointer("receiver-1"),
expCreatedReceiver: false,
expCfg: definitions.PostableApiAlertingConfig{
Config: definitions.Config{
@@ -1002,7 +1035,7 @@ func TestStitchReceivers(t *testing.T) {
Name: "receiver-4",
Type: "slack",
},
expOldReceiver: "receiver-1",
expOldReceiver: util.Pointer("receiver-1"),
expCreatedReceiver: false,
expCfg: definitions.PostableApiAlertingConfig{
Config: definitions.Config{
@@ -1087,7 +1120,7 @@ func TestStitchReceivers(t *testing.T) {
Name: "brand-new-group",
Type: "opsgenie",
},
expOldReceiver: "receiver-2",
expOldReceiver: util.Pointer("receiver-2"),
expCreatedReceiver: true,
expCfg: definitions.PostableApiAlertingConfig{
Config: definitions.Config{
@@ -1159,7 +1192,7 @@ func TestStitchReceivers(t *testing.T) {
Name: "brand-new-group",
Type: "opsgenie",
},
expOldReceiver: "receiver-2", // Not the inconsistent receiver-3?
expOldReceiver: util.Pointer("receiver-2"), // Not the inconsistent receiver-3?
expCreatedReceiver: true,
expCfg: definitions.PostableApiAlertingConfig{
Config: definitions.Config{
@@ -1282,7 +1315,7 @@ func TestStitchReceivers(t *testing.T) {
Name: "receiver-1",
Type: "slack",
},
expOldReceiver: "receiver-2",
expOldReceiver: util.Pointer("receiver-2"),
expCreatedReceiver: false,
expFullRemoval: true,
expCfg: definitions.PostableApiAlertingConfig{
@@ -1337,7 +1370,7 @@ func TestStitchReceivers(t *testing.T) {
}
renamedReceiver, fullRemoval, createdReceiver := stitchReceiver(cfg, c.new)
assert.Equalf(t, c.expOldReceiver, renamedReceiver, "expected old receiver to be %s, got %s", c.expOldReceiver, renamedReceiver)
assert.EqualValuesf(t, c.expOldReceiver, renamedReceiver, "expected old receiver to be %v, got %v", c.expOldReceiver, renamedReceiver)
assert.Equalf(t, c.expFullRemoval, fullRemoval, "expected full removal to be %t, got %t", c.expFullRemoval, fullRemoval)
assert.Equalf(t, c.expCreatedReceiver, createdReceiver, "expected created receiver to be %t, got %t", c.expCreatedReceiver, createdReceiver)
require.Equal(t, c.expCfg, cfg.AlertmanagerConfig)