From e43ddd516deb0ed720e9d607df7a4c60205ad9b3 Mon Sep 17 00:00:00 2001 From: Matthew Jacobson Date: Thu, 29 Aug 2024 13:00:55 -0400 Subject: [PATCH] Alerting: Ensure k8s receiver API create/update will never store nil settings (#92701) Ensure Create/Update will never store nil Settings --- .../ngalert/notifier/legacy_storage/compat.go | 16 +++-- .../ngalert/notifier/receiver_svc_test.go | 62 ++++++++++++++++++- 2 files changed, 69 insertions(+), 9 deletions(-) diff --git a/pkg/services/ngalert/notifier/legacy_storage/compat.go b/pkg/services/ngalert/notifier/legacy_storage/compat.go index 38cef8552b7..8cd0084471a 100644 --- a/pkg/services/ngalert/notifier/legacy_storage/compat.go +++ b/pkg/services/ngalert/notifier/legacy_storage/compat.go @@ -32,13 +32,17 @@ func IntegrationToPostableGrafanaReceiver(integration *models.Integration) (*api SecureSettings: maps.Clone(integration.SecureSettings), } - if len(integration.Settings) > 0 { - jsonBytes, err := json.Marshal(integration.Settings) - if err != nil { - return nil, err - } - postable.Settings = jsonBytes + // Alertmanager will fail validation with nil Settings , so ensure we always have at least an empty map. + settings := integration.Settings + if settings == nil { + settings = make(map[string]any) } + + jsonBytes, err := json.Marshal(settings) + if err != nil { + return nil, err + } + postable.Settings = jsonBytes return postable, nil } diff --git a/pkg/services/ngalert/notifier/receiver_svc_test.go b/pkg/services/ngalert/notifier/receiver_svc_test.go index bf23b9e9dcd..b2ba2cca5c9 100644 --- a/pkg/services/ngalert/notifier/receiver_svc_test.go +++ b/pkg/services/ngalert/notifier/receiver_svc_test.go @@ -7,6 +7,7 @@ import ( "strings" "testing" + "github.com/prometheus/alertmanager/config" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -342,6 +343,7 @@ func TestReceiverService_Create(t *testing.T) { slackIntegration := models.IntegrationGen(models.IntegrationMuts.WithName("test receiver"), models.IntegrationMuts.WithValidConfig("slack"))() emailIntegration := models.IntegrationGen(models.IntegrationMuts.WithName("test receiver"), models.IntegrationMuts.WithValidConfig("email"))() + lineIntegration := models.IntegrationGen(models.IntegrationMuts.WithName("test receiver"), models.IntegrationMuts.WithValidConfig("line"))() baseReceiver := models.ReceiverGen(models.ReceiverMuts.WithName("test receiver"), models.ReceiverMuts.WithIntegrations(slackIntegration))() for _, tc := range []struct { @@ -349,6 +351,7 @@ func TestReceiverService_Create(t *testing.T) { user identity.Requester receiver models.Receiver expectedCreate models.Receiver + expectedStored *definitions.PostableApiReceiver expectedErr error expectedProvenances map[string]models.Provenance }{ @@ -414,6 +417,49 @@ func TestReceiverService_Create(t *testing.T) { receiver: models.CopyReceiverWith(baseReceiver, models.ReceiverMuts.WithInvalidIntegration("slack")), expectedErr: legacy_storage.ErrReceiverInvalid, }, + { + name: "create integration with no normal settings should not store nil settings", + user: writer, + receiver: models.CopyReceiverWith(baseReceiver, models.ReceiverMuts.WithIntegrations( + models.CopyIntegrationWith(lineIntegration, + models.IntegrationMuts.WithSettings( + map[string]any{ // Line is valid with only the single secure field "token", so Settings will be empty when saving. + "token": "secret", + }, + )), + )), + expectedCreate: models.CopyReceiverWith(baseReceiver, models.ReceiverMuts.WithIntegrations( + models.CopyIntegrationWith(lineIntegration, + models.IntegrationMuts.WithSettings( + map[string]any{}, // Empty settings, not nil. + ), + models.IntegrationMuts.WithSecureSettings( + map[string]string{ + "token": "c2VjcmV0", // base64 encoded "secret". + }, + ), + ), + )), + expectedStored: &definitions.PostableApiReceiver{ + Receiver: config.Receiver{ + Name: lineIntegration.Name, + }, + PostableGrafanaReceivers: definitions.PostableGrafanaReceivers{ + GrafanaManagedReceivers: []*definitions.PostableGrafanaReceiver{ + { + UID: lineIntegration.UID, + Name: lineIntegration.Name, + Type: lineIntegration.Config.Type, + DisableResolveMessage: lineIntegration.DisableResolveMessage, + Settings: definitions.RawMessage(`{}`), // Empty settings, not nil. + SecureSettings: map[string]string{ + "token": "c2VjcmV0", // base64 encoded "secret". + }, + }, + }, + }, + }, + }, } { t.Run(tc.name, func(t *testing.T) { sut := createReceiverServiceSut(t, &secretsService) @@ -462,9 +508,19 @@ func TestReceiverService_Create(t *testing.T) { decrypted.Version = tc.expectedCreate.Version // Version is calculated before decryption. assert.Equal(t, decrypted, *stored) - provenances, err := sut.provisioningStore.GetProvenances(context.Background(), tc.user.GetOrgID(), (&definitions.EmbeddedContactPoint{}).ResourceType()) - require.NoError(t, err) - assert.Equal(t, tc.expectedProvenances, provenances) + if tc.expectedProvenances != nil { + provenances, err := sut.provisioningStore.GetProvenances(context.Background(), tc.user.GetOrgID(), (&definitions.EmbeddedContactPoint{}).ResourceType()) + require.NoError(t, err) + assert.Equal(t, tc.expectedProvenances, provenances) + } + + if tc.expectedStored != nil { + revision, err := sut.cfgStore.Get(context.Background(), writer.GetOrgID()) + require.NoError(t, err) + rcv, err := revision.GetReceiver(legacy_storage.NameToUid(tc.expectedStored.Name)) + require.NoError(t, err) + assert.Equal(t, tc.expectedStored, rcv) + } }) } }