Alerting: Use default identifier for extra configs in Alertmanager API (#107049)

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)
+	identifier := parseConfigIdentifierHeader(c)
-	if err != nil {
-		logger.Error("Failed to parse config identifier header", "error", err, "identifier", identifier)
-		return errorToResponse(err)
-	}
 
 	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)
+	identifier := parseConfigIdentifierHeader(c)
-	if err != nil {
-		logger.Error("failed to parse config identifier header", "err", err)
-		return errorToResponse(err)
-	}
 
 	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)
+	identifier := parseConfigIdentifierHeader(c)
-	if err != nil {
-		logger.Error("Failed to parse config identifier header", "error", err)
-		return errorToResponse(err)
-	}
 
-	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
 }

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.Equal(t, http.StatusAccepted, response.Status())
-		require.Contains(t, string(response.Body()), "identifier cannot be empty")
+		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)
+			identifier := parseConfigIdentifierHeader(rc)
-
+			require.Equal(t, tc.expectedValue, identifier)
-			if tc.expectedError {
-				require.Error(t, err)
-			} else {
-				require.NoError(t, err)
-				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.Equal(t, http.StatusAccepted, response.Status())
-		require.Contains(t, string(response.Body()), "identifier cannot be empty")
+		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.Equal(t, http.StatusAccepted, response.Status())
-		require.Contains(t, string(response.Body()), "identifier cannot be empty")
+		mockAM.AssertExpectations(t)
 	})
 }

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) {
+		t.Run("DELETE without config identifier header should use default identifier", func(t *testing.T) {
-			headers := map[string]string{}
+			createHeaders := map[string]string{
+				"Content-Type":                      "application/yaml",
+				"X-Grafana-Alerting-Merge-Matchers": "environment=test",
+			}
 
-			_, status, _ := apiClient.RawConvertPrometheusDeleteAlertmanagerConfig(t, headers)
+			amConfig := apimodels.AlertmanagerUserConfig{
-			requireStatusCode(t, http.StatusBadRequest, status, "")
+				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, "")
 		})
 	})