diff --git a/pkg/services/ngalert/api/api_alertmanager_test.go b/pkg/services/ngalert/api/api_alertmanager_test.go index abd9a361fd4..9536065db46 100644 --- a/pkg/services/ngalert/api/api_alertmanager_test.go +++ b/pkg/services/ngalert/api/api_alertmanager_test.go @@ -174,7 +174,7 @@ func TestAlertmanagerConfig(t *testing.T) { OrgID: 12, }, } - request := createAmConfigRequest(t) + request := createAmConfigRequest(t, validConfig) response := sut.RoutePostAlertingConfig(&rc, request) @@ -191,7 +191,7 @@ func TestAlertmanagerConfig(t *testing.T) { OrgID: 1, }, } - request := createAmConfigRequest(t) + request := createAmConfigRequest(t, validConfig) response := sut.RoutePostAlertingConfig(&rc, request) @@ -208,13 +208,45 @@ func TestAlertmanagerConfig(t *testing.T) { OrgID: 3, // Org 3 was initialized with broken config. }, } - request := createAmConfigRequest(t) + request := createAmConfigRequest(t, validConfig) response := sut.RoutePostAlertingConfig(&rc, request) require.Equal(t, 202, response.Status()) }) + t.Run("assert config hash doesn't change when sending RouteGetAlertingConfig back to RoutePostAlertingConfig", func(t *testing.T) { + rc := contextmodel.ReqContext{ + Context: &web.Context{ + Req: &http.Request{}, + }, + SignedInUser: &user.SignedInUser{ + OrgID: 1, + }, + } + request := createAmConfigRequest(t, validConfigWithSecureSetting) + + r := sut.RoutePostAlertingConfig(&rc, request) + require.Equal(t, 202, r.Status()) + + am, err := sut.mam.AlertmanagerFor(1) + require.NoError(t, err) + hash := am.Base.ConfigHash() + + getResponse := sut.RouteGetAlertingConfig(&rc) + require.Equal(t, 200, getResponse.Status()) + postable, err := notifier.Load(getResponse.Body()) + require.NoError(t, err) + + r = sut.RoutePostAlertingConfig(&rc, *postable) + require.Equal(t, 202, r.Status()) + + am, err = sut.mam.AlertmanagerFor(1) + require.NoError(t, err) + newHash := am.Base.ConfigHash() + require.Equal(t, hash, newHash) + }) + t.Run("when objects are not provisioned", func(t *testing.T) { t.Run("route from GET config has no provenance", func(t *testing.T) { sut := createSut(t) @@ -259,7 +291,7 @@ func TestAlertmanagerConfig(t *testing.T) { t.Run("contact point from GET config has expected provenance", func(t *testing.T) { sut := createSut(t) rc := createRequestCtxInOrg(1) - request := createAmConfigRequest(t) + request := createAmConfigRequest(t, validConfig) _ = sut.RoutePostAlertingConfig(rc, request) @@ -609,11 +641,11 @@ func createSut(t *testing.T) AlertmanagerSrv { } } -func createAmConfigRequest(t *testing.T) apimodels.PostableUserConfig { +func createAmConfigRequest(t *testing.T, config string) apimodels.PostableUserConfig { t.Helper() request := apimodels.PostableUserConfig{} - err := request.UnmarshalJSON([]byte(validConfig)) + err := request.UnmarshalJSON([]byte(config)) require.NoError(t, err) return request @@ -676,6 +708,41 @@ var validConfig = `{ } ` +var validConfigWithSecureSetting = `{ + "template_files": { + "a": "template" + }, + "alertmanager_config": { + "route": { + "receiver": "grafana-default-email" + }, + "receivers": [{ + "name": "grafana-default-email", + "grafana_managed_receiver_configs": [{ + "uid": "", + "name": "email receiver", + "type": "email", + "isDefault": true, + "settings": { + "addresses": "" + } + }]}, + { + "name": "slack", + "grafana_managed_receiver_configs": [{ + "uid": "", + "name": "slack1", + "type": "slack", + "settings": {"text": "slack text"}, + "secureSettings": { + "url": "secure url" + } + }] + }] + } +} +` + var brokenConfig = ` "alertmanager_config": { "route": { diff --git a/pkg/services/ngalert/api/tooling/definitions/alertmanager.go b/pkg/services/ngalert/api/tooling/definitions/alertmanager.go index 3b4ba3e7782..f8450b4b9fb 100644 --- a/pkg/services/ngalert/api/tooling/definitions/alertmanager.go +++ b/pkg/services/ngalert/api/tooling/definitions/alertmanager.go @@ -277,7 +277,11 @@ type TestReceiversConfigBodyParams struct { } func (c *TestReceiversConfigBodyParams) ProcessConfig(encrypt EncryptFn) error { - return processReceiverConfigs(c.Receivers, encrypt) + err := encryptReceiverConfigs(c.Receivers, encrypt) + if err != nil { + return err + } + return assignReceiverConfigsUIDs(c.Receivers) } type TestReceiversConfigAlertParams struct { @@ -615,9 +619,14 @@ func (c *PostableUserConfig) GetGrafanaReceiverMap() map[string]*PostableGrafana return UIDs } -// ProcessConfig parses grafana receivers, encrypts secrets and assigns UUIDs (if they are missing) -func (c *PostableUserConfig) ProcessConfig(encrypt EncryptFn) error { - return processReceiverConfigs(c.AlertmanagerConfig.Receivers, encrypt) +// EncryptConfig parses grafana receivers and encrypts secrets. +func (c *PostableUserConfig) EncryptConfig(encrypt EncryptFn) error { + return encryptReceiverConfigs(c.AlertmanagerConfig.Receivers, encrypt) +} + +// AssignMissingConfigUIDs assigns missing UUIDs to receiver configs. +func (c *PostableUserConfig) AssignMissingConfigUIDs() error { + return assignReceiverConfigsUIDs(c.AlertmanagerConfig.Receivers) } // MarshalYAML implements yaml.Marshaller. @@ -1285,8 +1294,7 @@ type PostableGrafanaReceivers struct { type EncryptFn func(ctx context.Context, payload []byte) ([]byte, error) -func processReceiverConfigs(c []*PostableApiReceiver, encrypt EncryptFn) error { - seenUIDs := make(map[string]struct{}) +func encryptReceiverConfigs(c []*PostableApiReceiver, encrypt EncryptFn) error { // encrypt secure settings for storing them in DB for _, r := range c { switch r.Type() { @@ -1299,6 +1307,20 @@ func processReceiverConfigs(c []*PostableApiReceiver, encrypt EncryptFn) error { } gr.SecureSettings[k] = base64.StdEncoding.EncodeToString(encryptedData) } + } + default: + } + } + return nil +} + +func assignReceiverConfigsUIDs(c []*PostableApiReceiver) error { + seenUIDs := make(map[string]struct{}) + // encrypt secure settings for storing them in DB + for _, r := range c { + switch r.Type() { + case GrafanaReceiverType: + for _, gr := range r.PostableGrafanaReceivers.GrafanaManagedReceivers { if gr.UID == "" { retries := 5 for i := 0; i < retries; i++ { diff --git a/pkg/services/ngalert/notifier/alertmanager_config.go b/pkg/services/ngalert/notifier/alertmanager_config.go index 2f900d9d7c6..28082f09213 100644 --- a/pkg/services/ngalert/notifier/alertmanager_config.go +++ b/pkg/services/ngalert/notifier/alertmanager_config.go @@ -166,13 +166,21 @@ func (moa *MultiOrgAlertmanager) ApplyAlertmanagerConfiguration(ctx context.Cont } } + // First, we encrypt the new or updated secure settings. Then, we load the existing secure settings from the database + // and add back any that weren't updated. + // We perform these steps in this order to ensure the hash of the secure settings remains stable when no secure + // settings were modified. + if err := config.EncryptConfig(func(ctx context.Context, payload []byte) ([]byte, error) { + return moa.Crypto.Encrypt(ctx, payload, secrets.WithoutScope()) + }); err != nil { + return fmt.Errorf("failed to post process Alertmanager configuration: %w", err) + } + if err := moa.Crypto.LoadSecureSettings(ctx, org, config.AlertmanagerConfig.Receivers); err != nil { return err } - if err := config.ProcessConfig(func(ctx context.Context, payload []byte) ([]byte, error) { - return moa.Crypto.Encrypt(ctx, payload, secrets.WithoutScope()) - }); err != nil { + if err := config.AssignMissingConfigUIDs(); err != nil { return fmt.Errorf("failed to post process Alertmanager configuration: %w", err) } diff --git a/pkg/services/ngalert/notifier/crypto.go b/pkg/services/ngalert/notifier/crypto.go index b0861d205e7..efc84be1574 100644 --- a/pkg/services/ngalert/notifier/crypto.go +++ b/pkg/services/ngalert/notifier/crypto.go @@ -74,19 +74,13 @@ func (c *alertmanagerCrypto) LoadSecureSettings(ctx context.Context, orgId int64 // Frontend sends only the secure settings that have to be updated // Therefore we have to copy from the last configuration only those secure settings not included in the request - for key := range cgmr.SecureSettings { + for key, encryptedValue := range cgmr.SecureSettings { _, ok := gr.SecureSettings[key] if !ok { - decryptedValue, err := c.getDecryptedSecret(cgmr, key) - if err != nil { - return fmt.Errorf("failed to decrypt stored secure setting: %s: %w", key, err) - } - if receivers[i].PostableGrafanaReceivers.GrafanaManagedReceivers[j].SecureSettings == nil { receivers[i].PostableGrafanaReceivers.GrafanaManagedReceivers[j].SecureSettings = make(map[string]string, len(cgmr.SecureSettings)) } - - receivers[i].PostableGrafanaReceivers.GrafanaManagedReceivers[j].SecureSettings[key] = decryptedValue + receivers[i].PostableGrafanaReceivers.GrafanaManagedReceivers[j].SecureSettings[key] = encryptedValue } } } diff --git a/public/app/features/alerting/unified/utils/receiver-form.test.ts b/public/app/features/alerting/unified/utils/receiver-form.test.ts index 49b10b910c9..7f5e460aab8 100644 --- a/public/app/features/alerting/unified/utils/receiver-form.test.ts +++ b/public/app/features/alerting/unified/utils/receiver-form.test.ts @@ -1,4 +1,4 @@ -import { omitEmptyValues } from './receiver-form'; +import { omitEmptyValues, omitEmptyUnlessExisting } from './receiver-form'; describe('Receiver form utils', () => { describe('omitEmptyStringValues', () => { @@ -41,4 +41,26 @@ describe('Receiver form utils', () => { expect(omitEmptyValues(original)).toEqual(expected); }); }); + describe('omitEmptyUnlessExisting', () => { + it('should omit empty strings if no entry in existing', () => { + const existing = { + five_keep: true, + }; + const original = { + one: 'two', + two_remove: '', + three: 0, + four_remove: null, + five_keep: '', + }; + + const expected = { + one: 'two', + three: 0, + five_keep: '', + }; + + expect(omitEmptyUnlessExisting(original, existing)).toEqual(expected); + }); + }); }); diff --git a/public/app/features/alerting/unified/utils/receiver-form.ts b/public/app/features/alerting/unified/utils/receiver-form.ts index 1efc737f8a5..b3de19969fa 100644 --- a/public/app/features/alerting/unified/utils/receiver-form.ts +++ b/public/app/features/alerting/unified/utils/receiver-form.ts @@ -1,4 +1,4 @@ -import { isArray } from 'lodash'; +import { isArray, isNil, omitBy } from 'lodash'; import { AlertManagerCortexConfig, @@ -221,7 +221,7 @@ export function formChannelValuesToGrafanaChannelConfig( ...(existing && existing.type === values.type ? existing.settings ?? {} : {}), ...(values.settings ?? {}), }), - secureSettings: values.secureSettings ?? {}, + secureSettings: omitEmptyUnlessExisting(values.secureSettings, existing?.secureFields), type: values.type, name, disableResolveMessage: @@ -233,6 +233,9 @@ export function formChannelValuesToGrafanaChannelConfig( return channel; } +// null, undefined and '' are deemed unacceptable +const isUnacceptableValue = (value: unknown) => isNil(value) || value === ''; + // will remove properties that have empty ('', null, undefined) object properties. // traverses nested objects and arrays as well. in place, mutates the object. // this is needed because form will submit empty string for not filled in fields, @@ -243,7 +246,7 @@ export function omitEmptyValues(obj: T): T { obj.forEach(omitEmptyValues); } else if (typeof obj === 'object' && obj !== null) { Object.entries(obj).forEach(([key, value]) => { - if (value === '' || value === null || value === undefined) { + if (isUnacceptableValue(value)) { delete (obj as any)[key]; } else { omitEmptyValues(value); @@ -252,3 +255,9 @@ export function omitEmptyValues(obj: T): T { } return obj; } + +// Will remove empty ('', null, undefined) object properties unless they were previously defined. +// existing is a map of property names that were previously defined. +export function omitEmptyUnlessExisting(settings = {}, existing = {}): Record { + return omitBy(settings, (value, key) => isUnacceptableValue(value) && !(key in existing)); +}