From b820fd6bef3b38a6d8985a9751a6f201d59f152f Mon Sep 17 00:00:00 2001 From: Moustafa Baiou Date: Wed, 29 Jan 2025 16:00:45 -0500 Subject: [PATCH] Alerting: Fix Alertmanager configuration updates (#99610) * Alerting: Fix Alertmanager configuration updates Alertmanager configuration updates would behave inconsistently when performing no-op updates with `mysql` as the store. In particular this bug manifested as a failure to reload the provisioned alertmanager configuration components with no changes to the configuration itself. This would result in a 500 error with mysql store only. The core issue is that we were relying on the number of rows affected by the update query to determine if the configuration was found in the db or not. While this behavior works for certain sql dialects, mysql does not return the number of rows matched by the update query but rather the number of rows actually updated. Also discovered and fixed the mismatched `xorm` tag for the `CreatedAt` field to match the actual column name in the db. References: https://dev.mysql.com/doc/refman/8.4/en/update.html --- pkg/services/ngalert/models/alertmanager.go | 2 +- pkg/services/ngalert/store/alertmanager.go | 17 +++++++++++- .../ngalert/store/alertmanager_test.go | 26 +++++++++++++++++++ .../api/alerting/api_provisioning_test.go | 14 ++++++++++ 4 files changed, 57 insertions(+), 2 deletions(-) diff --git a/pkg/services/ngalert/models/alertmanager.go b/pkg/services/ngalert/models/alertmanager.go index 0b69f682c8a..98e86587625 100644 --- a/pkg/services/ngalert/models/alertmanager.go +++ b/pkg/services/ngalert/models/alertmanager.go @@ -9,7 +9,7 @@ type AlertConfiguration struct { AlertmanagerConfiguration string ConfigurationHash string ConfigurationVersion string - CreatedAt int64 `xorm:"created"` + CreatedAt int64 `xorm:"created_at"` Default bool OrgID int64 `xorm:"org_id"` } diff --git a/pkg/services/ngalert/store/alertmanager.go b/pkg/services/ngalert/store/alertmanager.go index fc07e7a5f35..33e62f46fb9 100644 --- a/pkg/services/ngalert/store/alertmanager.go +++ b/pkg/services/ngalert/store/alertmanager.go @@ -110,9 +110,24 @@ func (st DBstore) SaveAlertmanagerConfigurationWithCallback(ctx context.Context, // UpdateAlertmanagerConfiguration replaces an alertmanager configuration with optimistic locking. It assumes that an existing revision of the configuration exists in the store, and will return an error otherwise. func (st *DBstore) UpdateAlertmanagerConfiguration(ctx context.Context, cmd *models.SaveAlertmanagerConfigurationCmd) error { return st.SQLStore.WithTransactionalDbSession(ctx, func(sess *db.Session) error { + newConfigHash := fmt.Sprintf("%x", md5.Sum([]byte(cmd.AlertmanagerConfiguration))) + // check for no-op update + if newConfigHash == cmd.FetchedConfigurationHash { + // double check that the configuration with this hash is in the db + ok, err := sess.Table("alert_configuration"). + Where("org_id = ? AND configuration_hash = ?", cmd.OrgID, cmd.FetchedConfigurationHash). + Exist() + if err != nil { + return err + } + if !ok { + return ErrVersionLockedObjectNotFound + } + return nil + } config := models.AlertConfiguration{ AlertmanagerConfiguration: cmd.AlertmanagerConfiguration, - ConfigurationHash: fmt.Sprintf("%x", md5.Sum([]byte(cmd.AlertmanagerConfiguration))), + ConfigurationHash: newConfigHash, ConfigurationVersion: cmd.ConfigurationVersion, Default: cmd.Default, OrgID: cmd.OrgID, diff --git a/pkg/services/ngalert/store/alertmanager_test.go b/pkg/services/ngalert/store/alertmanager_test.go index 4433760c8e1..4f27e2b6e83 100644 --- a/pkg/services/ngalert/store/alertmanager_test.go +++ b/pkg/services/ngalert/store/alertmanager_test.go @@ -114,6 +114,32 @@ func TestIntegrationAlertmanagerStore(t *testing.T) { require.ErrorIs(t, err, ErrVersionLockedObjectNotFound) }) + + t.Run("UpdateAlertmanagerConfiguration doesn't update the db if the update is a no-op", func(t *testing.T) { + _, configMD5 := setupConfig(t, "my-config", store) + + originalConfig, err := store.GetLatestAlertmanagerConfiguration(context.Background(), 1) + require.NoError(t, err) + cmd := buildSaveConfigCmd(t, "my-config", 1) + cmd.FetchedConfigurationHash = configMD5 + err = store.UpdateAlertmanagerConfiguration(context.Background(), &cmd) + require.NoError(t, err) + config, err := store.GetLatestAlertmanagerConfiguration(context.Background(), 1) + require.NoError(t, err) + require.Equal(t, "my-config", config.AlertmanagerConfiguration) + require.Equal(t, configMD5, config.ConfigurationHash) + // CreatedAt should not have changed as we didn't touch the config in the DB + require.Equal(t, originalConfig.CreatedAt, config.CreatedAt) + }) + t.Run("UpdateAlertmanagerConfiguration fails if the config doesn't exist and the hashes in the cmd match", func(t *testing.T) { + configRaw := "my-non-existent-config" + configHash := fmt.Sprintf("%x", md5.Sum([]byte(configRaw))) + cmd := buildSaveConfigCmd(t, configRaw, 1) + cmd.FetchedConfigurationHash = configHash + err := store.UpdateAlertmanagerConfiguration(context.Background(), &cmd) + require.Error(t, err) + require.EqualError(t, err, ErrVersionLockedObjectNotFound.Error()) + }) } func TestIntegrationAlertmanagerHash(t *testing.T) { diff --git a/pkg/tests/api/alerting/api_provisioning_test.go b/pkg/tests/api/alerting/api_provisioning_test.go index ec9cda827b8..f4bc4dab890 100644 --- a/pkg/tests/api/alerting/api_provisioning_test.go +++ b/pkg/tests/api/alerting/api_provisioning_test.go @@ -903,6 +903,9 @@ func TestIntegrationExportFileProvision(t *testing.T) { require.Len(t, export.MuteTimings, 1) require.YAMLEq(t, expectedYaml, exportRaw) }) + t.Run("reloading provisioning should not fail", func(t *testing.T) { + apiClient.ReloadAlertingFileProvisioning(t) + }) }) } @@ -1004,6 +1007,17 @@ func TestIntegrationExportFileProvisionContactPoints(t *testing.T) { var export definitions.AlertingFileExport require.NoError(t, yaml.Unmarshal([]byte(exportRaw), &export)) + // verify the file exported matches the file provisioned thing + require.Len(t, export.ContactPoints, 1) + require.YAMLEq(t, string(expectedYaml), exportRaw) + }) + t.Run("reloading provisioning should not change things", func(t *testing.T) { + apiClient.ReloadAlertingFileProvisioning(t) + + exportRaw := apiClient.ExportReceiver(t, "cp_1_$escaped", "yaml", true) + var export definitions.AlertingFileExport + require.NoError(t, yaml.Unmarshal([]byte(exportRaw), &export)) + // verify the file exported matches the file provisioned thing require.Len(t, export.ContactPoints, 1) require.YAMLEq(t, string(expectedYaml), exportRaw)