diff --git a/pkg/services/ngalert/api/api_alertmanager_guards.go b/pkg/services/ngalert/api/api_alertmanager_guards.go index 1c130e0fac6..be03baab717 100644 --- a/pkg/services/ngalert/api/api_alertmanager_guards.go +++ b/pkg/services/ngalert/api/api_alertmanager_guards.go @@ -38,17 +38,20 @@ func (srv AlertmanagerSrv) k8sApiServiceGuard(currentConfig apimodels.GettableUs // - Since the UIDs stored in the database for the purposes of per-receiver RBAC are generated based on the receiver // name, we would need to ensure continuity of permissions when a receiver is renamed. This would, preferably, // require detecting renames and updating the permissions UID in the database. - // - It would need to determine newly created and deleted receivers so it can populate per-receiver access control defaults. + // - Editors don't have permission to manage all receiver permissions by default, only admin users do. This means that + // certain combined operations (e.g. swapping the name of receivers) would require careful handling to ensure + // that the correct permissions are maintained, and considering receivers don't have a unique identifier outside + // their name, this is non-trivial. // Neither of these are insurmountable, but considering this endpoint will be removed once FlagAlertingApiServer // becomes GA, the complexity may not be worthwhile. To that end, for now we reject any request that attempts to - // modify receivers. + // update receivers, while allowing operations that add or remove receivers. delta, err := calculateReceiversDelta(currentConfig.AlertmanagerConfig.Receivers, newConfig.AlertmanagerConfig.Receivers) if err != nil { return err } - if !delta.IsEmpty() { - return fmt.Errorf("cannot modify receivers using this API while per-receiver RBAC is enabled; either disable the `alertingApiServer` feature flag or use an API that supports per-receiver RBAC (e.g. provisioning or receivers API)") + if len(delta.Updated) > 0 { + return fmt.Errorf("cannot update receivers using this API while per-receiver RBAC is enabled; either disable the `alertingApiServer` feature flag or use an API that supports per-receiver RBAC (e.g. provisioning or receivers API)") } return nil } diff --git a/pkg/tests/apis/alerting/notifications/receivers/receiver_test.go b/pkg/tests/apis/alerting/notifications/receivers/receiver_test.go index 8965613379f..f17d6bb0bc2 100644 --- a/pkg/tests/apis/alerting/notifications/receivers/receiver_test.go +++ b/pkg/tests/apis/alerting/notifications/receivers/receiver_test.go @@ -14,6 +14,7 @@ import ( "testing" "github.com/grafana/alerting/notify" + "github.com/prometheus/alertmanager/config" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/api/errors" @@ -1127,14 +1128,65 @@ func TestIntegrationRejectConfigApiReceiverModification(t *testing.T) { cliCfg := helper.Org1.Admin.NewRestConfig() legacyCli := alerting.NewAlertingLegacyAPIClient(helper.GetEnv().Server.HTTPServer.Listener.Addr().String(), cliCfg.Username, cliCfg.Password) - // This config has new and modified receivers. + adminK8sClient, err := versioned.NewForConfig(cliCfg) + require.NoError(t, err) + adminClient := adminK8sClient.NotificationsV0alpha1().Receivers("default") + alertmanagerRaw, err := testData.ReadFile(path.Join("test-data", "notification-settings.json")) require.NoError(t, err) var amConfig definitions.PostableUserConfig require.NoError(t, json.Unmarshal(alertmanagerRaw, &amConfig)) + persistInitialConfig(t, amConfig, adminClient, legacyCli) + + // We modify the receiver, this should cause the POST to be rejected. + userDefinedReceiver := amConfig.AlertmanagerConfig.Receivers[slices.IndexFunc(amConfig.AlertmanagerConfig.Receivers, func(receiver *definitions.PostableApiReceiver) bool { + return receiver.Receiver.Name == "user-defined" + })] + userDefinedReceiver.GrafanaManagedReceivers[0].DisableResolveMessage = !userDefinedReceiver.GrafanaManagedReceivers[0].DisableResolveMessage success, err := legacyCli.PostConfiguration(t, amConfig) - require.Falsef(t, success, "Expected receiver modification to be rejected, but got %s", err) + require.Falsef(t, success, "Expected receiver modification to be rejected, but got %t", success) + require.ErrorContainsf(t, err, "alertingApiServer", "Expected error message to contain 'alertingApiServer', but got %s", err) + + // Revert the change. + userDefinedReceiver.GrafanaManagedReceivers[0].DisableResolveMessage = !userDefinedReceiver.GrafanaManagedReceivers[0].DisableResolveMessage + + // We add a receiver, this should be accepted. + amConfig.AlertmanagerConfig.Receivers = append(amConfig.AlertmanagerConfig.Receivers, &definitions.PostableApiReceiver{ + Receiver: config.Receiver{ + Name: "new receiver", + }, + PostableGrafanaReceivers: definitions.PostableGrafanaReceivers{ + GrafanaManagedReceivers: []*definitions.PostableGrafanaReceiver{ + { + Name: "new receiver", + Type: "email", + Settings: []byte(`{"addresses": ""}`), + }, + }, + }, + }) + success, err = legacyCli.PostConfiguration(t, amConfig) + require.Truef(t, success, "Expected receiver modification to be accepted, but got %s", err) + require.NoError(t, err) + + // Sanity check. + gettable, status, body := legacyCli.GetAlertmanagerConfigWithStatus(t) + require.Equalf(t, http.StatusOK, status, body) + require.Lenf(t, gettable.AlertmanagerConfig.Receivers, 3, "Expected 3 receivers, got %d", len(gettable.AlertmanagerConfig.Receivers)) + + // We remove the receiver, this should be accepted. + amConfig.AlertmanagerConfig.Receivers = slices.DeleteFunc(amConfig.AlertmanagerConfig.Receivers, func(receiver *definitions.PostableApiReceiver) bool { + return receiver.GrafanaManagedReceivers[0].Name == "new receiver" + }) + success, err = legacyCli.PostConfiguration(t, amConfig) + require.Truef(t, success, "Expected receiver modification to be accepted, but got %s", err) + require.NoError(t, err) + + // Sanity check. + gettable, status, body = legacyCli.GetAlertmanagerConfigWithStatus(t) + require.Equalf(t, http.StatusOK, status, body) + require.Lenf(t, gettable.AlertmanagerConfig.Receivers, 2, "Expected 2 receivers, got %d", len(gettable.AlertmanagerConfig.Receivers)) } func TestIntegrationReferentialIntegrity(t *testing.T) {