From b605340668438340a1eb34a89b872a08e2e08a0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jean-Philippe=20Qu=C3=A9m=C3=A9ner?= Date: Thu, 16 Dec 2021 13:33:10 +0100 Subject: [PATCH] Alerting: log errors happening in the API on server side (#43192) * Alerting: log errors happening in the API on server side * adapt tests to reflect changed payload --- pkg/services/ngalert/api/util.go | 2 +- .../alerting/api_admin_configuration_test.go | 2 +- .../api_alertmanager_configuration_test.go | 4 +- .../api/alerting/api_alertmanager_test.go | 38 +++++++++---------- pkg/tests/api/alerting/api_prometheus_test.go | 6 +-- pkg/tests/api/alerting/api_ruler_test.go | 6 +-- 6 files changed, 29 insertions(+), 29 deletions(-) diff --git a/pkg/services/ngalert/api/util.go b/pkg/services/ngalert/api/util.go index 1c435295472..6eab8554d53 100644 --- a/pkg/services/ngalert/api/util.go +++ b/pkg/services/ngalert/api/util.go @@ -258,7 +258,7 @@ func ErrResp(status int, err error, msg string, args ...interface{}) *response.N if msg != "" { err = errors.WithMessagef(err, msg, args...) } - return response.Error(status, err.Error(), nil) + return response.Error(status, "API error", err) } // accessForbiddenResp creates a response of forbidden access. diff --git a/pkg/tests/api/alerting/api_admin_configuration_test.go b/pkg/tests/api/alerting/api_admin_configuration_test.go index 3ced6ca3aca..d6d5d1e3e00 100644 --- a/pkg/tests/api/alerting/api_admin_configuration_test.go +++ b/pkg/tests/api/alerting/api_admin_configuration_test.go @@ -64,7 +64,7 @@ func TestAdminConfiguration_SendingToExternalAlertmanagers(t *testing.T) { resp := getRequest(t, alertsURL, http.StatusNotFound) // nolint b, err := ioutil.ReadAll(resp.Body) require.NoError(t, err) - require.JSONEq(t, string(b), "{\"message\": \"no admin configuration available\"}") + require.JSONEq(t, string(b), "{\"message\": \"API error\",\"error\": \"no admin configuration available\"}") } // Now, lets re-set external Alertmanagers for main organisation. diff --git a/pkg/tests/api/alerting/api_alertmanager_configuration_test.go b/pkg/tests/api/alerting/api_alertmanager_configuration_test.go index bdfab20116f..96628bc1bae 100644 --- a/pkg/tests/api/alerting/api_alertmanager_configuration_test.go +++ b/pkg/tests/api/alerting/api_alertmanager_configuration_test.go @@ -88,7 +88,7 @@ func TestAlertmanagerConfigurationIsTransactional(t *testing.T) { } ` resp := postRequest(t, alertConfigURL, payload, http.StatusBadRequest) // nolint - require.JSONEq(t, `{"message":"failed to save and apply Alertmanager configuration: failed to build integration map: the receiver is invalid: failed to validate receiver \"slack.receiver\" of type \"slack\": token must be specified when using the Slack chat API"}`, getBody(t, resp.Body)) + require.JSONEq(t, `{"message": "API error","error":"failed to save and apply Alertmanager configuration: failed to build integration map: the receiver is invalid: failed to validate receiver \"slack.receiver\" of type \"slack\": token must be specified when using the Slack chat API"}`, getBody(t, resp.Body)) resp = getRequest(t, alertConfigURL, http.StatusOK) // nolint require.JSONEq(t, defaultAlertmanagerConfigJSON, getBody(t, resp.Body)) @@ -208,7 +208,7 @@ func TestAlertmanagerConfigurationPersistSecrets(t *testing.T) { ` resp := postRequest(t, alertConfigURL, payload, http.StatusBadRequest) // nolint - require.JSONEq(t, `{"message": "unknown receiver: invalid"}`, getBody(t, resp.Body)) + require.JSONEq(t, `{"message": "API error","error": "unknown receiver: invalid"}`, getBody(t, resp.Body)) } // The secure settings must be present diff --git a/pkg/tests/api/alerting/api_alertmanager_test.go b/pkg/tests/api/alerting/api_alertmanager_test.go index 4629af0c466..da8b2f1a3ca 100644 --- a/pkg/tests/api/alerting/api_alertmanager_test.go +++ b/pkg/tests/api/alerting/api_alertmanager_test.go @@ -96,7 +96,7 @@ func TestAMConfigAccess(t *testing.T) { desc: "viewer request should fail", url: "http://viewer:viewer@%s/api/alertmanager/grafana/config/api/v1/alerts", expStatus: http.StatusForbidden, - expBody: `{"message": "permission denied"}`, + expBody: `{"message": "API error","error": "permission denied"}`, }, { desc: "editor request should succeed", @@ -166,7 +166,7 @@ func TestAMConfigAccess(t *testing.T) { desc: "viewer request should fail", url: "http://viewer:viewer@%s/api/alertmanager/grafana/config/api/v1/alerts", expStatus: http.StatusForbidden, - expBody: `{"message": "permission denied"}`, + expBody: `{"message": "API error","error": "permission denied"}`, }, { desc: "editor request should succeed", @@ -229,7 +229,7 @@ func TestAMConfigAccess(t *testing.T) { desc: "viewer request should fail", url: "http://viewer:viewer@%s/api/alertmanager/grafana/api/v2/silences", expStatus: http.StatusForbidden, - expBody: `{"message": "permission denied"}`, + expBody: `{"message": "API error","error": "permission denied"}`, }, { desc: "editor request should succeed", @@ -335,7 +335,7 @@ func TestAMConfigAccess(t *testing.T) { desc: "viewer request should fail", url: "http://viewer:viewer@%s/api/alertmanager/grafana/api/v2/silence/%s", expStatus: http.StatusForbidden, - expBody: `{"message": "permission denied"}`, + expBody: `{"message": "API error","error": "permission denied"}`, }, { desc: "editor request should succeed", @@ -603,7 +603,7 @@ func TestRulerAccess(t *testing.T) { desc: "viewer request should fail", url: "http://viewer:viewer@%s/api/ruler/grafana/api/v1/rules/default", expStatus: http.StatusForbidden, - expectedResponse: `{"message":"user does not have permissions to edit the namespace: user does not have permissions to edit the namespace"}`, + expectedResponse: `{"message": "API error","error":"user does not have permissions to edit the namespace: user does not have permissions to edit the namespace"}`, }, { desc: "editor request should succeed", @@ -890,7 +890,7 @@ func TestAlertRuleCRUD(t *testing.T) { Data: []ngmodels.AlertQuery{}, }, }, - expectedResponse: `{"message":"failed to update rule group: invalid alert rule: no queries or expressions are found"}`, + expectedResponse: `{"message": "API error","error":"failed to update rule group: invalid alert rule: no queries or expressions are found"}`, }, { desc: "alert rule with empty title", @@ -920,7 +920,7 @@ func TestAlertRuleCRUD(t *testing.T) { }, }, }, - expectedResponse: `{"message":"failed to update rule group: invalid alert rule: title is empty"}`, + expectedResponse: `{"message": "API error","error":"failed to update rule group: invalid alert rule: title is empty"}`, }, { desc: "alert rule with too long name", @@ -950,7 +950,7 @@ func TestAlertRuleCRUD(t *testing.T) { }, }, }, - expectedResponse: `{"message":"failed to update rule group: invalid alert rule: name length should not be greater than 190"}`, + expectedResponse: `{"message": "API error","error":"failed to update rule group: invalid alert rule: name length should not be greater than 190"}`, }, { desc: "alert rule with too long rulegroup", @@ -980,7 +980,7 @@ func TestAlertRuleCRUD(t *testing.T) { }, }, }, - expectedResponse: `{"message":"failed to update rule group: invalid alert rule: rule group name length should not be greater than 190"}`, + expectedResponse: `{"message": "API error","error":"failed to update rule group: invalid alert rule: rule group name length should not be greater than 190"}`, }, { desc: "alert rule with invalid interval", @@ -1011,7 +1011,7 @@ func TestAlertRuleCRUD(t *testing.T) { }, }, }, - expectedResponse: `{"message":"failed to update rule group: invalid alert rule: interval (1s) should be non-zero and divided exactly by scheduler interval: 10s"}`, + expectedResponse: `{"message": "API error","error":"failed to update rule group: invalid alert rule: interval (1s) should be non-zero and divided exactly by scheduler interval: 10s"}`, }, { desc: "alert rule with unknown datasource", @@ -1041,7 +1041,7 @@ func TestAlertRuleCRUD(t *testing.T) { }, }, }, - expectedResponse: `{"message":"failed to validate alert rule \"AlwaysFiring\": invalid query A: data source not found: unknown"}`, + expectedResponse: `{"message": "API error","error":"failed to validate alert rule \"AlwaysFiring\": invalid query A: data source not found: unknown"}`, }, { desc: "alert rule with invalid condition", @@ -1071,7 +1071,7 @@ func TestAlertRuleCRUD(t *testing.T) { }, }, }, - expectedResponse: `{"message":"failed to validate alert rule \"AlwaysFiring\": condition B not found in any query or expression: it should be one of: [A]"}`, + expectedResponse: `{"message": "API error","error":"failed to validate alert rule \"AlwaysFiring\": condition B not found in any query or expression: it should be one of: [A]"}`, }, } @@ -1361,7 +1361,7 @@ func TestAlertRuleCRUD(t *testing.T) { require.NoError(t, err) assert.Equal(t, http.StatusNotFound, resp.StatusCode) - require.JSONEq(t, `{"message":"failed to update rule group: failed to get alert rule unknown: could not find alert rule"}`, string(b)) + require.JSONEq(t, `{"message": "API error","error":"failed to update rule group: failed to get alert rule unknown: could not find alert rule"}`, string(b)) // let's make sure that rule definitions are not affected by the failed POST request. u = fmt.Sprintf("http://grafana:password@%s/api/ruler/grafana/api/v1/rules/default", grafanaListedAddr) @@ -1480,7 +1480,7 @@ func TestAlertRuleCRUD(t *testing.T) { require.NoError(t, err) assert.Equal(t, http.StatusBadRequest, resp.StatusCode) - require.JSONEq(t, fmt.Sprintf(`{"message":"failed to validate alert rule \"AlwaysAlerting\": conflicting UID \"%s\" found"}`, ruleUID), string(b)) + require.JSONEq(t, fmt.Sprintf(`{"message": "API error","error":"failed to validate alert rule \"AlwaysAlerting\": conflicting UID \"%s\" found"}`, ruleUID), string(b)) // let's make sure that rule definitions are not affected by the failed POST request. u = fmt.Sprintf("http://grafana:password@%s/api/ruler/grafana/api/v1/rules/default", grafanaListedAddr) @@ -1885,7 +1885,7 @@ func TestAlertRuleCRUD(t *testing.T) { require.NoError(t, err) require.Equal(t, http.StatusNotFound, resp.StatusCode) - require.JSONEq(t, `{"message":"failed to delete rule group: rule group not found under this namespace"}`, string(b)) + require.JSONEq(t, `{"message": "API error","error":"failed to delete rule group: rule group not found under this namespace"}`, string(b)) }) t.Run("succeed if the rule group name does exist", func(t *testing.T) { @@ -2093,7 +2093,7 @@ func TestQuota(t *testing.T) { b, err := ioutil.ReadAll(resp.Body) require.NoError(t, err) assert.Equal(t, http.StatusForbidden, resp.StatusCode) - require.JSONEq(t, `{"message":"quota reached"}`, string(b)) + require.JSONEq(t, `{"message": "API error","error":"quota reached"}`, string(b)) }) t.Run("when quota limit exceed updating existing rule should succeed", func(t *testing.T) { @@ -2388,7 +2388,7 @@ func TestEval(t *testing.T) { } `, expectedStatusCode: http.StatusBadRequest, - expectedResponse: `{"message":"invalid condition: condition B not found in any query or expression: it should be one of: [A]"}`, + expectedResponse: `{"message": "API error","error":"invalid condition: condition B not found in any query or expression: it should be one of: [A]"}`, }, { desc: "unknown query datasource", @@ -2413,7 +2413,7 @@ func TestEval(t *testing.T) { } `, expectedStatusCode: http.StatusBadRequest, - expectedResponse: `{"message":"invalid condition: invalid query A: data source not found: unknown"}`, + expectedResponse: `{"message": "API error","error":"invalid condition: invalid query A: data source not found: unknown"}`, }, } @@ -2569,7 +2569,7 @@ func TestEval(t *testing.T) { } `, expectedStatusCode: http.StatusBadRequest, - expectedResponse: `{"message":"invalid queries or expressions: invalid query A: data source not found: unknown"}`, + expectedResponse: `{"message": "API error","error":"invalid queries or expressions: invalid query A: data source not found: unknown"}`, }, } diff --git a/pkg/tests/api/alerting/api_prometheus_test.go b/pkg/tests/api/alerting/api_prometheus_test.go index bcf6ae5d926..5633e74a8df 100644 --- a/pkg/tests/api/alerting/api_prometheus_test.go +++ b/pkg/tests/api/alerting/api_prometheus_test.go @@ -201,7 +201,7 @@ func TestPrometheusRules(t *testing.T) { require.NoError(t, err) assert.Equal(t, 400, resp.StatusCode) - require.JSONEq(t, `{"message":"failed to update rule group: invalid alert rule: cannot have Panel ID without a Dashboard UID"}`, string(b)) + require.JSONEq(t, `{"message": "API error","error":"failed to update rule group: invalid alert rule: cannot have Panel ID without a Dashboard UID"}`, string(b)) } // Now, let's see how this looks like. @@ -593,7 +593,7 @@ func TestPrometheusRulesFilterByDashboard(t *testing.T) { require.Equal(t, http.StatusBadRequest, resp.StatusCode) b, err := ioutil.ReadAll(resp.Body) require.NoError(t, err) - require.JSONEq(t, `{"message":"invalid panel_id: strconv.ParseInt: parsing \"invalid\": invalid syntax"}`, string(b)) + require.JSONEq(t, `{"message": "API error","error":"invalid panel_id: strconv.ParseInt: parsing \"invalid\": invalid syntax"}`, string(b)) } // Now, let's check a panel_id without dashboard_uid returns a 400 Bad Request response @@ -609,7 +609,7 @@ func TestPrometheusRulesFilterByDashboard(t *testing.T) { require.Equal(t, http.StatusBadRequest, resp.StatusCode) b, err := ioutil.ReadAll(resp.Body) require.NoError(t, err) - require.JSONEq(t, `{"message":"panel_id must be set with dashboard_uid"}`, string(b)) + require.JSONEq(t, `{"message": "API error","error":"panel_id must be set with dashboard_uid"}`, string(b)) } } diff --git a/pkg/tests/api/alerting/api_ruler_test.go b/pkg/tests/api/alerting/api_ruler_test.go index 3ab3ceb4bb7..ac924499cf6 100644 --- a/pkg/tests/api/alerting/api_ruler_test.go +++ b/pkg/tests/api/alerting/api_ruler_test.go @@ -421,7 +421,7 @@ func TestAlertRuleConflictingTitle(t *testing.T) { require.NoError(t, err) assert.Equal(t, http.StatusInternalServerError, resp.StatusCode) - require.JSONEq(t, `{"message":"failed to update rule group: a conflicting alert rule is found: rule title under the same organisation and folder should be unique"}`, string(b)) + require.JSONEq(t, `{"message": "API error","error":"failed to update rule group: a conflicting alert rule is found: rule title under the same organisation and folder should be unique"}`, string(b)) }) t.Run("trying to create alert with same title under another folder should succeed", func(t *testing.T) { @@ -772,7 +772,7 @@ func TestRulerRulesFilterByDashboard(t *testing.T) { require.Equal(t, http.StatusBadRequest, resp.StatusCode) b, err := ioutil.ReadAll(resp.Body) require.NoError(t, err) - require.JSONEq(t, `{"message":"invalid panel_id: strconv.ParseInt: parsing \"invalid\": invalid syntax"}`, string(b)) + require.JSONEq(t, `{"message": "API error","error":"invalid panel_id: strconv.ParseInt: parsing \"invalid\": invalid syntax"}`, string(b)) } // Now, let's check a panel_id without dashboard_uid returns a 400 Bad Request response @@ -788,6 +788,6 @@ func TestRulerRulesFilterByDashboard(t *testing.T) { require.Equal(t, http.StatusBadRequest, resp.StatusCode) b, err := ioutil.ReadAll(resp.Body) require.NoError(t, err) - require.JSONEq(t, `{"message":"panel_id must be set with dashboard_uid"}`, string(b)) + require.JSONEq(t, `{"message": "API error","error":"panel_id must be set with dashboard_uid"}`, string(b)) } }