From 6ea97e41fba7d8cee7cc2d084a5764351a87c374 Mon Sep 17 00:00:00 2001 From: Steve Simpson Date: Fri, 19 Apr 2024 21:03:20 +0200 Subject: [PATCH] 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 | 80 ++++++++++++------- .../ngalert/api/tooling/definitions/prom.go | 18 +++++ .../api/tooling/definitions/prom_test.go | 42 +++++++++- pkg/tests/api/alerting/api_prometheus_test.go | 10 ++- 4 files changed, 116 insertions(+), 34 deletions(-) diff --git a/pkg/services/ngalert/api/api_prometheus.go b/pkg/services/ngalert/api/api_prometheus.go index a3161e9eb50a..dae93551bea7 100644 --- a/pkg/services/ngalert/api/api_prometheus.go +++ b/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 diff --git a/pkg/services/ngalert/api/tooling/definitions/prom.go b/pkg/services/ngalert/api/tooling/definitions/prom.go index c97d94d770d5..bdbe93c9c960 100644 --- a/pkg/services/ngalert/api/tooling/definitions/prom.go +++ b/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 diff --git a/pkg/services/ngalert/api/tooling/definitions/prom_test.go b/pkg/services/ngalert/api/tooling/definitions/prom_test.go index 5546cd374f15..2c1e60c76a25 100644 --- a/pkg/services/ngalert/api/tooling/definitions/prom_test.go +++ b/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 diff --git a/pkg/tests/api/alerting/api_prometheus_test.go b/pkg/tests/api/alerting/api_prometheus_test.go index da3b58a8aa3f..6b0dae19c3cf 100644 --- a/pkg/tests/api/alerting/api_prometheus_test.go +++ b/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"]) } }