Alerting: Consistently return Prometheus-style responses from rules APIs. (#86600)

* Alerting: Consistently return Prometheus-style responses from rules APIs.

This commit is part refactor and part fix. The /rules API occasionally returns
error responses which are inconsistent with other error responses. This fixes
that, and adds a function to map from Prometheus error type and HTTP code.

* Fix integration tests

* Linter happiness

* Make linter more happy

* Fix up one more place returning non-Prometheus responses

pkg/services/ngalert/api/api_prometheus.go

@@ -95,7 +95,7 @@ func (srv PrometheusSrv) RouteGetAlertStatuses(c *contextmodel.ReqContext) respo
 		})
 	}
 
-	return response.JSON(http.StatusOK, alertResponse)
+	return response.JSON(alertResponse.HTTPStatusCode(), alertResponse)
 }
 
 func formatValues(alertState *state.State) string {
@@ -175,31 +175,6 @@ func (srv PrometheusSrv) RouteGetRuleStatuses(c *contextmodel.ReqContext) respon
 	// As we are using req.Form directly, this triggers a call to ParseForm() if needed.
 	c.Query("")
 
-	dashboardUID := c.Query("dashboard_uid")
-	panelID, err := getPanelIDFromRequest(c.Req)
-	if err != nil {
-		return ErrResp(http.StatusBadRequest, err, "invalid panel_id")
-	}
-	if dashboardUID == "" && panelID != 0 {
-		return ErrResp(http.StatusBadRequest, errors.New("panel_id must be set with dashboard_uid"), "")
-	}
-
-	limitGroups := getInt64WithDefault(c.Req.Form, "limit", -1)
-	limitRulesPerGroup := getInt64WithDefault(c.Req.Form, "limit_rules", -1)
-	limitAlertsPerRule := getInt64WithDefault(c.Req.Form, "limit_alerts", -1)
-	matchers, err := getMatchersFromRequest(c.Req)
-	if err != nil {
-		return ErrResp(http.StatusBadRequest, err, "")
-	}
-	withStates, err := getStatesFromRequest(c.Req)
-	if err != nil {
-		return ErrResp(http.StatusBadRequest, err, "")
-	}
-	withStatesFast := make(map[eval.State]struct{})
-	for _, state := range withStates {
-		withStatesFast[state] = struct{}{}
-	}
-
 	ruleResponse := apimodels.RuleResponse{
 		DiscoveryBase: apimodels.DiscoveryBase{
 			Status: "success",
@@ -209,6 +184,43 @@ func (srv PrometheusSrv) RouteGetRuleStatuses(c *contextmodel.ReqContext) respon
 		},
 	}
 
+	dashboardUID := c.Query("dashboard_uid")
+	panelID, err := getPanelIDFromRequest(c.Req)
+	if err != nil {
+		ruleResponse.DiscoveryBase.Status = "error"
+		ruleResponse.DiscoveryBase.Error = fmt.Sprintf("invalid panel_id: %s", err.Error())
+		ruleResponse.DiscoveryBase.ErrorType = apiv1.ErrBadData
+		return response.JSON(ruleResponse.HTTPStatusCode(), ruleResponse)
+	}
+	if dashboardUID == "" && panelID != 0 {
+		ruleResponse.DiscoveryBase.Status = "error"
+		ruleResponse.DiscoveryBase.Error = "panel_id must be set with dashboard_uid"
+		ruleResponse.DiscoveryBase.ErrorType = apiv1.ErrBadData
+		return response.JSON(ruleResponse.HTTPStatusCode(), ruleResponse)
+	}
+
+	limitGroups := getInt64WithDefault(c.Req.Form, "limit", -1)
+	limitRulesPerGroup := getInt64WithDefault(c.Req.Form, "limit_rules", -1)
+	limitAlertsPerRule := getInt64WithDefault(c.Req.Form, "limit_alerts", -1)
+	matchers, err := getMatchersFromRequest(c.Req)
+	if err != nil {
+		ruleResponse.DiscoveryBase.Status = "error"
+		ruleResponse.DiscoveryBase.Error = err.Error()
+		ruleResponse.DiscoveryBase.ErrorType = apiv1.ErrBadData
+		return response.JSON(ruleResponse.HTTPStatusCode(), ruleResponse)
+	}
+	withStates, err := getStatesFromRequest(c.Req)
+	if err != nil {
+		ruleResponse.DiscoveryBase.Status = "error"
+		ruleResponse.DiscoveryBase.Error = err.Error()
+		ruleResponse.DiscoveryBase.ErrorType = apiv1.ErrBadData
+		return response.JSON(ruleResponse.HTTPStatusCode(), ruleResponse)
+	}
+	withStatesFast := make(map[eval.State]struct{})
+	for _, state := range withStates {
+		withStatesFast[state] = struct{}{}
+	}
+
 	var labelOptions []ngmodels.LabelOption
 	if !getBoolWithDefault(c.Req.Form, queryIncludeInternalLabels, false) {
 		labelOptions = append(labelOptions, ngmodels.WithoutInternalLabels())
@@ -216,12 +228,15 @@ func (srv PrometheusSrv) RouteGetRuleStatuses(c *contextmodel.ReqContext) respon
 
 	namespaceMap, err := srv.store.GetUserVisibleNamespaces(c.Req.Context(), c.SignedInUser.GetOrgID(), c.SignedInUser)
 	if err != nil {
-		return ErrResp(http.StatusInternalServerError, err, "failed to get namespaces visible to the user")
+		ruleResponse.DiscoveryBase.Status = "error"
+		ruleResponse.DiscoveryBase.Error = fmt.Sprintf("failed to get namespaces visible to the user: %s", err.Error())
+		ruleResponse.DiscoveryBase.ErrorType = apiv1.ErrServer
+		return response.JSON(ruleResponse.HTTPStatusCode(), ruleResponse)
 	}
 
 	if len(namespaceMap) == 0 {
 		srv.log.Debug("User does not have access to any namespaces")
-		return response.JSON(http.StatusOK, ruleResponse)
+		return response.JSON(ruleResponse.HTTPStatusCode(), ruleResponse)
 	}
 
 	namespaceUIDs := make([]string, len(namespaceMap))
@@ -240,7 +255,7 @@ func (srv PrometheusSrv) RouteGetRuleStatuses(c *contextmodel.ReqContext) respon
 		ruleResponse.DiscoveryBase.Status = "error"
 		ruleResponse.DiscoveryBase.Error = fmt.Sprintf("failure getting rules: %s", err.Error())
 		ruleResponse.DiscoveryBase.ErrorType = apiv1.ErrServer
-		return response.JSON(http.StatusInternalServerError, ruleResponse)
+		return response.JSON(ruleResponse.HTTPStatusCode(), ruleResponse)
 	}
 
 	// Group rules together by Namespace and Rule Group. Rules are also grouped by Org ID,
@@ -267,7 +282,10 @@ func (srv PrometheusSrv) RouteGetRuleStatuses(c *contextmodel.ReqContext) respon
 		}
 		ok, err := srv.authz.HasAccessToRuleGroup(c.Req.Context(), c.SignedInUser, rules)
 		if err != nil {
-			return response.ErrOrFallback(http.StatusInternalServerError, "cannot authorize access to rule group", err)
+			ruleResponse.DiscoveryBase.Status = "error"
+			ruleResponse.DiscoveryBase.Error = fmt.Sprintf("cannot authorize access to rule group: %s", err.Error())
+			ruleResponse.DiscoveryBase.ErrorType = apiv1.ErrServer
+			return response.JSON(ruleResponse.HTTPStatusCode(), ruleResponse)
 		}
 		if !ok {
 			continue
@@ -317,7 +335,7 @@ func (srv PrometheusSrv) RouteGetRuleStatuses(c *contextmodel.ReqContext) respon
 		ruleResponse.Data.RuleGroups = ruleResponse.Data.RuleGroups[0:limitGroups]
 	}
 
-	return response.JSON(http.StatusOK, ruleResponse)
+	return response.JSON(ruleResponse.HTTPStatusCode(), ruleResponse)
 }
 
 // This is the same as matchers.Matches but avoids the need to create a LabelSet

pkg/services/ngalert/api/tooling/definitions/prom.go

@@ -3,6 +3,7 @@ package definitions
 import (
 	"container/heap"
 	"fmt"
+	"net/http"
 	"sort"
 	"strings"
 	"time"
@@ -98,6 +99,23 @@ type RuleGroup struct {
 	EvaluationTime float64   `json:"evaluationTime"`
 }
 
+// HTTPStatusCode returns the HTTP status code for a given Prometheus style error.
+func (d DiscoveryBase) HTTPStatusCode() int {
+	if d.Status == "success" {
+		return http.StatusOK
+	}
+
+	// Mapping taken from prometheus/web/api/v1/api.go
+	// Note this is not exhaustive as our API does not return
+	// the same spectrum of errors as Prometheus does.
+	switch d.ErrorType {
+	case v1.ErrBadData:
+		return http.StatusBadRequest
+	default:
+		return http.StatusInternalServerError
+	}
+}
+
 // RuleGroupsBy is a function that defines the ordering of Rule Groups.
 type RuleGroupsBy func(a1, a2 *RuleGroup) bool
 

pkg/services/ngalert/api/tooling/definitions/prom_test.go

@@ -1,12 +1,53 @@
 package definitions
 
 import (
+	"net/http"
 	"testing"
 	"time"
 
 	"github.com/stretchr/testify/assert"
 )
 
+func TestDiscoveryBaseHTTPStatusCode(t *testing.T) {
+	tc := []struct {
+		name     string
+		input    DiscoveryBase
+		expected int
+	}{{
+		name: "OK when status is success",
+		input: DiscoveryBase{
+			Status: "success",
+		},
+		expected: http.StatusOK,
+	}, {
+		name: "InternalServerError when status is error but no error type",
+		input: DiscoveryBase{
+			Status: "error",
+		},
+		expected: http.StatusInternalServerError,
+	}, {
+		name: "BadRequest when status is error and type is bad_data",
+		input: DiscoveryBase{
+			Status:    "error",
+			ErrorType: "bad_data",
+		},
+		expected: http.StatusBadRequest,
+	}, {
+		name: "InternalServerError when status is error and type is server_error",
+		input: DiscoveryBase{
+			Status:    "error",
+			ErrorType: "server_error",
+		},
+		expected: http.StatusInternalServerError,
+	}}
+
+	for _, tt := range tc {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.EqualValues(t, tt.expected, tt.input.HTTPStatusCode())
+		})
+	}
+}
+
 func TestSortAlertsByImportance(t *testing.T) {
 	tm1, tm2 := time.Now(), time.Now().Add(time.Second)
 	tc := []struct {
@@ -66,7 +107,6 @@ func TestSortAlertsByImportance(t *testing.T) {
 }
 
 func TestTopKAlertsByImportance(t *testing.T) {
-	//	tm1, tm2 := time.Now(), time.Now().Add(time.Second)
 	tc := []struct {
 		name     string
 		k        int

pkg/tests/api/alerting/api_prometheus_test.go

@@ -625,7 +625,10 @@ func TestIntegrationPrometheusRulesFilterByDashboard(t *testing.T) {
 		require.NoError(t, err)
 		var res map[string]any
 		require.NoError(t, json.Unmarshal(b, &res))
-		require.Equal(t, `invalid panel_id: strconv.ParseInt: parsing "invalid": invalid syntax`, res["message"])
+		// These APIs return Prometheus-like errors.
+		require.Equal(t, "error", res["status"])
+		require.Equal(t, "bad_data", res["errorType"])
+		require.Equal(t, `invalid panel_id: strconv.ParseInt: parsing "invalid": invalid syntax`, res["error"])
 	}
 
 	// Now, let's check a panel_id without dashboard_uid returns a 400 Bad Request response
@@ -643,7 +646,10 @@ func TestIntegrationPrometheusRulesFilterByDashboard(t *testing.T) {
 		require.NoError(t, err)
 		var res map[string]any
 		require.NoError(t, json.Unmarshal(b, &res))
-		require.Equal(t, "panel_id must be set with dashboard_uid", res["message"])
+		// These APIs return Prometheus-like errors.
+		require.Equal(t, "error", res["status"])
+		require.Equal(t, "bad_data", res["errorType"])
+		require.Equal(t, "panel_id must be set with dashboard_uid", res["error"])
 	}
 }