From 9945514baa31e228c8350edb8805ffa9e7344ee1 Mon Sep 17 00:00:00 2001 From: Santiago Date: Tue, 19 Dec 2023 18:41:48 +0100 Subject: [PATCH] Alerting: Validate configuration for the remote Alertmanager struct (#79691) * Alerting: Validate configuration for the remote Alertmanager struct * add TenantID to test * add OrgID to config struct in tests --- pkg/services/ngalert/ngalert.go | 2 +- pkg/services/ngalert/remote/alertmanager.go | 25 +++++++++++--- .../ngalert/remote/alertmanager_test.go | 33 +++++++++++-------- 3 files changed, 41 insertions(+), 19 deletions(-) diff --git a/pkg/services/ngalert/ngalert.go b/pkg/services/ngalert/ngalert.go index c6d2eef7120..af40e9e2f77 100644 --- a/pkg/services/ngalert/ngalert.go +++ b/pkg/services/ngalert/ngalert.go @@ -179,7 +179,7 @@ func (ng *AlertNG) init() error { externalAMCfg := remote.AlertmanagerConfig{} // We won't be handling files on disk, we can pass an empty string as workingDirPath. stateStore := notifier.NewFileStore(orgID, ng.KVStore, "") - return remote.NewAlertmanager(externalAMCfg, orgID, stateStore) + return remote.NewAlertmanager(externalAMCfg, stateStore) }) overrides = append(overrides, override) diff --git a/pkg/services/ngalert/remote/alertmanager.go b/pkg/services/ngalert/remote/alertmanager.go index a38b52625a3..9d2d35d9ea8 100644 --- a/pkg/services/ngalert/remote/alertmanager.go +++ b/pkg/services/ngalert/remote/alertmanager.go @@ -38,21 +38,36 @@ type Alertmanager struct { } type AlertmanagerConfig struct { + OrgID int64 URL string TenantID string BasicAuthPassword string } -func NewAlertmanager(cfg AlertmanagerConfig, orgID int64, store stateStore) (*Alertmanager, error) { +func (cfg *AlertmanagerConfig) Validate() error { + if cfg.OrgID == 0 { + return fmt.Errorf("orgID for remote Alertmanager not set") + } + + if cfg.TenantID == "" { + return fmt.Errorf("empty remote Alertmanager tenantID") + } + if cfg.URL == "" { - return nil, fmt.Errorf("empty remote Alertmanager URL for tenant '%s'", cfg.TenantID) + return fmt.Errorf("empty remote Alertmanager URL for tenant '%s'", cfg.TenantID) + } + return nil +} + +func NewAlertmanager(cfg AlertmanagerConfig, store stateStore) (*Alertmanager, error) { + if err := cfg.Validate(); err != nil { + return nil, err } u, err := url.Parse(cfg.URL) if err != nil { return nil, fmt.Errorf("unable to parse remote Alertmanager URL: %w", err) } - logger := log.New("ngalert.remote.alertmanager") mcCfg := &remoteClient.Config{ @@ -84,7 +99,7 @@ func NewAlertmanager(cfg AlertmanagerConfig, orgID int64, store stateStore) (*Al } s := sender.NewExternalAlertmanagerSender(sender.WithDoFunc(doFunc)) s.Run() - err = s.ApplyConfig(orgID, 0, []sender.ExternalAMcfg{{URL: cfg.URL + "/alertmanager"}}) + err = s.ApplyConfig(cfg.OrgID, 0, []sender.ExternalAMcfg{{URL: cfg.URL + "/alertmanager"}}) if err != nil { return nil, err } @@ -95,7 +110,7 @@ func NewAlertmanager(cfg AlertmanagerConfig, orgID int64, store stateStore) (*Al state: store, amClient: amc, sender: s, - orgID: orgID, + orgID: cfg.OrgID, tenantID: cfg.TenantID, url: cfg.URL, }, nil diff --git a/pkg/services/ngalert/remote/alertmanager_test.go b/pkg/services/ngalert/remote/alertmanager_test.go index 88bcf3314e4..97108208916 100644 --- a/pkg/services/ngalert/remote/alertmanager_test.go +++ b/pkg/services/ngalert/remote/alertmanager_test.go @@ -63,11 +63,12 @@ func TestNewAlertmanager(t *testing.T) { for _, test := range tests { t.Run(test.name, func(tt *testing.T) { cfg := AlertmanagerConfig{ + OrgID: test.orgID, URL: test.url, TenantID: test.tenantID, BasicAuthPassword: test.password, } - am, err := NewAlertmanager(cfg, test.orgID, nil) + am, err := NewAlertmanager(cfg, nil) if test.expErr != "" { require.EqualError(tt, err, test.expErr) return @@ -94,16 +95,18 @@ func TestApplyConfig(t *testing.T) { // A non-200 response should result in an error. server := httptest.NewServer(errorHandler) cfg := AlertmanagerConfig{ - URL: server.URL, + OrgID: 1, + TenantID: "test", + URL: server.URL, } ctx := context.Background() store := fakes.NewFakeKVStore(t) fstore := notifier.NewFileStore(1, store, "") - require.NoError(t, store.Set(ctx, 1, "alertmanager", notifier.SilencesFilename, "test")) - require.NoError(t, store.Set(ctx, 1, "alertmanager", notifier.NotificationLogFilename, "test")) + require.NoError(t, store.Set(ctx, cfg.OrgID, "alertmanager", notifier.SilencesFilename, "test")) + require.NoError(t, store.Set(ctx, cfg.OrgID, "alertmanager", notifier.NotificationLogFilename, "test")) - am, err := NewAlertmanager(cfg, 1, fstore) + am, err := NewAlertmanager(cfg, fstore) require.NoError(t, err) config := &ngmodels.AlertConfiguration{} @@ -135,6 +138,7 @@ func TestIntegrationRemoteAlertmanagerApplyConfigOnlyUploadsOnce(t *testing.T) { // ApplyConfig performs a readiness check. cfg := AlertmanagerConfig{ + OrgID: 1, URL: amURL, TenantID: tenantID, BasicAuthPassword: password, @@ -158,8 +162,8 @@ func TestIntegrationRemoteAlertmanagerApplyConfigOnlyUploadsOnce(t *testing.T) { fstore := notifier.NewFileStore(1, store, "") ctx := context.Background() - require.NoError(t, store.Set(ctx, 1, "alertmanager", notifier.SilencesFilename, base64.StdEncoding.EncodeToString(silences))) - require.NoError(t, store.Set(ctx, 1, "alertmanager", notifier.NotificationLogFilename, base64.StdEncoding.EncodeToString(nflog))) + require.NoError(t, store.Set(ctx, cfg.OrgID, "alertmanager", notifier.SilencesFilename, base64.StdEncoding.EncodeToString(silences))) + require.NoError(t, store.Set(ctx, cfg.OrgID, "alertmanager", notifier.NotificationLogFilename, base64.StdEncoding.EncodeToString(nflog))) fs := clusterpb.FullState{ Parts: []clusterpb.Part{ @@ -171,7 +175,7 @@ func TestIntegrationRemoteAlertmanagerApplyConfigOnlyUploadsOnce(t *testing.T) { require.NoError(t, err) encodedFullState := base64.StdEncoding.EncodeToString(fullState) - am, err := NewAlertmanager(cfg, 1, fstore) + am, err := NewAlertmanager(cfg, fstore) require.NoError(t, err) // We should have no configuration or state at first. @@ -209,8 +213,8 @@ func TestIntegrationRemoteAlertmanagerApplyConfigOnlyUploadsOnce(t *testing.T) { // Calling `ApplyConfig` again with a changed configuration and state yields no effect. { - require.NoError(t, store.Set(ctx, 1, "alertmanager", "silences", base64.StdEncoding.EncodeToString([]byte("abc123")))) - require.NoError(t, store.Set(ctx, 1, "alertmanager", "notifications", base64.StdEncoding.EncodeToString([]byte("abc123")))) + require.NoError(t, store.Set(ctx, cfg.OrgID, "alertmanager", "silences", base64.StdEncoding.EncodeToString([]byte("abc123")))) + require.NoError(t, store.Set(ctx, cfg.OrgID, "alertmanager", "notifications", base64.StdEncoding.EncodeToString([]byte("abc123")))) fakeConfig.ID = 30000000000000000 require.NoError(t, am.ApplyConfig(ctx, fakeConfig)) @@ -250,11 +254,12 @@ func TestIntegrationRemoteAlertmanagerSilences(t *testing.T) { password := os.Getenv("AM_PASSWORD") cfg := AlertmanagerConfig{ + OrgID: 1, URL: amURL, TenantID: tenantID, BasicAuthPassword: password, } - am, err := NewAlertmanager(cfg, 1, nil) + am, err := NewAlertmanager(cfg, nil) require.NoError(t, err) // We should have no silences at first. @@ -329,11 +334,12 @@ func TestIntegrationRemoteAlertmanagerAlerts(t *testing.T) { password := os.Getenv("AM_PASSWORD") cfg := AlertmanagerConfig{ + OrgID: 1, URL: amURL, TenantID: tenantID, BasicAuthPassword: password, } - am, err := NewAlertmanager(cfg, 1, nil) + am, err := NewAlertmanager(cfg, nil) require.NoError(t, err) // Wait until the Alertmanager is ready to send alerts. @@ -393,12 +399,13 @@ func TestIntegrationRemoteAlertmanagerReceivers(t *testing.T) { password := os.Getenv("AM_PASSWORD") cfg := AlertmanagerConfig{ + OrgID: 1, URL: amURL, TenantID: tenantID, BasicAuthPassword: password, } - am, err := NewAlertmanager(cfg, 1, nil) + am, err := NewAlertmanager(cfg, nil) require.NoError(t, err) // We should start with the default config.