From b93cde0adb3eb1a1cac26f4e88b661e602da8865 Mon Sep 17 00:00:00 2001 From: Alexander Akhmetov Date: Thu, 26 Jun 2025 12:44:49 +0200 Subject: [PATCH] Alerting: Use default identifier for extra configs in Alertmanager API (#107049) --- .../ngalert/api/api_convert_prometheus.go | 29 +++---- .../api/api_convert_prometheus_test.go | 78 +++++++++++++------ ...pi_convert_prometheus_alertmanager_test.go | 37 +++++++-- 3 files changed, 93 insertions(+), 51 deletions(-) diff --git a/pkg/services/ngalert/api/api_convert_prometheus.go b/pkg/services/ngalert/api/api_convert_prometheus.go index abe75af0573..345569be3c8 100644 --- a/pkg/services/ngalert/api/api_convert_prometheus.go +++ b/pkg/services/ngalert/api/api_convert_prometheus.go @@ -56,7 +56,8 @@ const ( mergeMatchersHeader = "X-Grafana-Alerting-Merge-Matchers" // configIdentifierHeader is the header that specifies the identifier for imported Alertmanager config. - configIdentifierHeader = "X-Grafana-Alerting-Config-Identifier" + configIdentifierHeader = "X-Grafana-Alerting-Config-Identifier" + defaultConfigIdentifier = "default" ) var ( @@ -539,11 +540,7 @@ func (srv *ConvertPrometheusSrv) RouteConvertPrometheusPostAlertmanagerConfig(c logger := srv.logger.FromContext(c.Req.Context()) - identifier, err := parseConfigIdentifierHeader(c) - if err != nil { - logger.Error("Failed to parse config identifier header", "error", err, "identifier", identifier) - return errorToResponse(err) - } + identifier := parseConfigIdentifierHeader(c) mergeMatchers, err := parseMergeMatchersHeader(c) if err != nil { @@ -581,11 +578,7 @@ func (srv *ConvertPrometheusSrv) RouteConvertPrometheusGetAlertmanagerConfig(c * logger := srv.logger.FromContext(c.Req.Context()) ctx := c.Req.Context() - identifier, err := parseConfigIdentifierHeader(c) - if err != nil { - logger.Error("failed to parse config identifier header", "err", err) - return errorToResponse(err) - } + identifier := parseConfigIdentifierHeader(c) cfg, err := srv.am.GetAlertmanagerConfiguration(ctx, c.GetOrgID(), false) if err != nil { @@ -629,13 +622,9 @@ func (srv *ConvertPrometheusSrv) RouteConvertPrometheusDeleteAlertmanagerConfig( logger := srv.logger.FromContext(c.Req.Context()) - identifier, err := parseConfigIdentifierHeader(c) - if err != nil { - logger.Error("Failed to parse config identifier header", "error", err) - return errorToResponse(err) - } + identifier := parseConfigIdentifierHeader(c) - err = srv.am.DeleteExtraConfiguration(c.Req.Context(), c.GetOrgID(), identifier) + err := srv.am.DeleteExtraConfiguration(c.Req.Context(), c.GetOrgID(), identifier) if err != nil { logger.Error("Failed to delete alertmanager configuration", "error", err, "identifier", identifier) return errorToResponse(fmt.Errorf("failed to delete alertmanager configuration: %w", err)) @@ -812,10 +801,10 @@ func formatMergeMatchers(matchers amconfig.Matchers) string { return strings.Join(pairs, ",") } -func parseConfigIdentifierHeader(c *contextmodel.ReqContext) (string, error) { +func parseConfigIdentifierHeader(c *contextmodel.ReqContext) string { identifier := strings.TrimSpace(c.Req.Header.Get(configIdentifierHeader)) if identifier == "" { - return "", errInvalidHeaderValue(configIdentifierHeader, errors.New("identifier cannot be empty")) + return defaultConfigIdentifier } - return identifier, nil + return identifier } diff --git a/pkg/services/ngalert/api/api_convert_prometheus_test.go b/pkg/services/ngalert/api/api_convert_prometheus_test.go index 4495ac6019e..7478783da62 100644 --- a/pkg/services/ngalert/api/api_convert_prometheus_test.go +++ b/pkg/services/ngalert/api/api_convert_prometheus_test.go @@ -1556,14 +1556,22 @@ func TestRouteConvertPrometheusPostAlertmanagerConfig(t *testing.T) { mockAM.AssertExpectations(t) }) - t.Run("should return error when identifier header is missing", func(t *testing.T) { + t.Run("should use default identifier when header is missing", func(t *testing.T) { rc := createRequestCtx() + rc.Req.Header.Set(mergeMatchersHeader, "test=value") + mockAM := &mockAlertmanager{} + mockAM.On("SaveAndApplyExtraConfiguration", mock.Anything, int64(1), mock.MatchedBy(func(extraConfig apimodels.ExtraConfiguration) bool { + return extraConfig.Identifier == defaultConfigIdentifier + })).Return(nil) + + ft := featuremgmt.WithFeatures(featuremgmt.FlagAlertingImportAlertmanagerAPI) + srv, _, _ := createConvertPrometheusSrv(t, withAlertmanager(mockAM), withFeatureToggles(ft)) amCfg := apimodels.AlertmanagerUserConfig{} response := srv.RouteConvertPrometheusPostAlertmanagerConfig(rc, amCfg) - require.Equal(t, http.StatusBadRequest, response.Status()) - require.Contains(t, string(response.Body()), "identifier cannot be empty") + require.Equal(t, http.StatusAccepted, response.Status()) + mockAM.AssertExpectations(t) }) t.Run("should return error when merge matchers header has invalid format", func(t *testing.T) { @@ -1594,19 +1602,42 @@ func TestRouteConvertPrometheusGetAlertmanagerConfig(t *testing.T) { require.Equal(t, http.StatusNotImplemented, response.Status()) }) - t.Run("without config identifier header should return 400", func(t *testing.T) { + t.Run("without config identifier header should use default identifier", func(t *testing.T) { mockAM := &mockAlertmanager{} + mockAM.On("GetAlertmanagerConfiguration", mock.Anything, orgID, false).Return(apimodels.GettableUserConfig{ + ExtraConfigs: []apimodels.ExtraConfiguration{ + { + Identifier: defaultConfigIdentifier, + AlertmanagerConfig: `route: + receiver: default +receivers: + - name: default`, + }, + }, + }, nil) ft := featuremgmt.WithFeatures(featuremgmt.FlagAlertingImportAlertmanagerAPI) srv, _, _ := createConvertPrometheusSrv(t, withAlertmanager(mockAM), withFeatureToggles(ft)) rc := createRequestCtx() response := srv.RouteConvertPrometheusGetAlertmanagerConfig(rc) - require.Equal(t, http.StatusBadRequest, response.Status()) + require.Equal(t, http.StatusOK, response.Status()) + mockAM.AssertExpectations(t) }) - t.Run("with empty config identifier header should return 400", func(t *testing.T) { + t.Run("with empty config identifier header should use default identifier", func(t *testing.T) { mockAM := &mockAlertmanager{} + mockAM.On("GetAlertmanagerConfiguration", mock.Anything, orgID, false).Return(apimodels.GettableUserConfig{ + ExtraConfigs: []apimodels.ExtraConfiguration{ + { + Identifier: defaultConfigIdentifier, + AlertmanagerConfig: `route: + receiver: default +receivers: + - name: default`, + }, + }, + }, nil) ft := featuremgmt.WithFeatures(featuremgmt.FlagAlertingImportAlertmanagerAPI) srv, _, _ := createConvertPrometheusSrv(t, withAlertmanager(mockAM), withFeatureToggles(ft)) @@ -1614,7 +1645,8 @@ func TestRouteConvertPrometheusGetAlertmanagerConfig(t *testing.T) { rc.Req.Header.Set(configIdentifierHeader, "") response := srv.RouteConvertPrometheusGetAlertmanagerConfig(rc) - require.Equal(t, http.StatusBadRequest, response.Status()) + require.Equal(t, http.StatusOK, response.Status()) + mockAM.AssertExpectations(t) }) t.Run("should return config when it is found", func(t *testing.T) { @@ -1820,14 +1852,14 @@ func TestParseConfigIdentifierHeader(t *testing.T) { expectedError: false, }, { - name: "empty identifier should return error", + name: "empty identifier should return the default value", headerValue: "", - expectedError: true, + expectedValue: defaultConfigIdentifier, }, { - name: "whitespace only identifier should return error", + name: "whitespace only identifier should return the default value", headerValue: " ", - expectedError: true, + expectedValue: defaultConfigIdentifier, }, } @@ -1836,14 +1868,8 @@ func TestParseConfigIdentifierHeader(t *testing.T) { rc := createRequestCtx() rc.Req.Header.Set(configIdentifierHeader, tc.headerValue) - identifier, err := parseConfigIdentifierHeader(rc) - - if tc.expectedError { - require.Error(t, err) - } else { - require.NoError(t, err) - require.Equal(t, tc.expectedValue, identifier) - } + identifier := parseConfigIdentifierHeader(rc) + require.Equal(t, tc.expectedValue, identifier) }) } } @@ -1904,13 +1930,14 @@ func TestRouteConvertPrometheusDeleteAlertmanagerConfig(t *testing.T) { mockAM.AssertExpectations(t) }) - t.Run("should return error when identifier header is missing", func(t *testing.T) { + t.Run("should use default identifier when header is missing", func(t *testing.T) { + mockAM.On("DeleteExtraConfiguration", mock.Anything, orgID, defaultConfigIdentifier).Return(nil).Once() rc := createRequestCtx() response := srv.RouteConvertPrometheusDeleteAlertmanagerConfig(rc) - require.Equal(t, http.StatusBadRequest, response.Status()) - require.Contains(t, string(response.Body()), "identifier cannot be empty") + require.Equal(t, http.StatusAccepted, response.Status()) + mockAM.AssertExpectations(t) }) t.Run("should return error when DeleteExtraConfiguration fails", func(t *testing.T) { @@ -1937,13 +1964,14 @@ func TestRouteConvertPrometheusDeleteAlertmanagerConfig(t *testing.T) { require.Equal(t, http.StatusNotImplemented, response.Status()) }) - t.Run("should return error for empty identifier header", func(t *testing.T) { + t.Run("should use default identifier for empty identifier header", func(t *testing.T) { + mockAM.On("DeleteExtraConfiguration", mock.Anything, orgID, defaultConfigIdentifier).Return(nil).Once() rc := createRequestCtx() rc.Req.Header.Set(configIdentifierHeader, "") response := srv.RouteConvertPrometheusDeleteAlertmanagerConfig(rc) - require.Equal(t, http.StatusBadRequest, response.Status()) - require.Contains(t, string(response.Body()), "identifier cannot be empty") + require.Equal(t, http.StatusAccepted, response.Status()) + mockAM.AssertExpectations(t) }) } diff --git a/pkg/tests/api/alerting/api_convert_prometheus_alertmanager_test.go b/pkg/tests/api/alerting/api_convert_prometheus_alertmanager_test.go index 231cd17e8cb..2c1369ae6a0 100644 --- a/pkg/tests/api/alerting/api_convert_prometheus_alertmanager_test.go +++ b/pkg/tests/api/alerting/api_convert_prometheus_alertmanager_test.go @@ -126,7 +126,9 @@ func TestIntegrationConvertPrometheusAlertmanagerEndpoints(t *testing.T) { }) t.Run("error cases", func(t *testing.T) { - t.Run("POST without config identifier header should fail", func(t *testing.T) { + t.Run("POST without config identifier header should use default identifier", func(t *testing.T) { + defer cleanup("") + headers := map[string]string{ "Content-Type": "application/yaml", "X-Grafana-Alerting-Merge-Matchers": "environment=test", @@ -137,7 +139,14 @@ func TestIntegrationConvertPrometheusAlertmanagerEndpoints(t *testing.T) { } _, status, _ := apiClient.RawConvertPrometheusPostAlertmanagerConfig(t, amConfig, headers) - requireStatusCode(t, http.StatusBadRequest, status, "") + requireStatusCode(t, http.StatusAccepted, status, "") + + getHeaders := map[string]string{ + "X-Grafana-Alerting-Config-Identifier": "default", + } + responseConfig, status, _ := apiClient.RawConvertPrometheusGetAlertmanagerConfig(t, getHeaders) + requireStatusCode(t, http.StatusOK, status, "") + require.NotEmpty(t, responseConfig.AlertmanagerConfig) }) t.Run("POST without merge matchers header should fail", func(t *testing.T) { @@ -184,11 +193,27 @@ func TestIntegrationConvertPrometheusAlertmanagerEndpoints(t *testing.T) { requireStatusCode(t, http.StatusBadRequest, status, "") }) - t.Run("DELETE without config identifier header should fail", func(t *testing.T) { - headers := map[string]string{} + t.Run("DELETE without config identifier header should use default identifier", func(t *testing.T) { + createHeaders := map[string]string{ + "Content-Type": "application/yaml", + "X-Grafana-Alerting-Merge-Matchers": "environment=test", + } - _, status, _ := apiClient.RawConvertPrometheusDeleteAlertmanagerConfig(t, headers) - requireStatusCode(t, http.StatusBadRequest, status, "") + amConfig := apimodels.AlertmanagerUserConfig{ + AlertmanagerConfig: testAlertmanagerConfigYAML, + } + + _, status, _ := apiClient.RawConvertPrometheusPostAlertmanagerConfig(t, amConfig, createHeaders) + requireStatusCode(t, http.StatusAccepted, status, "") + + _, status, _ = apiClient.RawConvertPrometheusGetAlertmanagerConfig(t, nil) + requireStatusCode(t, http.StatusOK, status, "") + + _, status, _ = apiClient.RawConvertPrometheusDeleteAlertmanagerConfig(t, nil) + requireStatusCode(t, http.StatusAccepted, status, "") + + _, status, _ = apiClient.RawConvertPrometheusGetAlertmanagerConfig(t, nil) + requireStatusCode(t, http.StatusNotFound, status, "") }) })