diff --git a/api/handler/triggers.go b/api/handler/triggers.go index 090ed6607..8ee2787a0 100644 --- a/api/handler/triggers.go +++ b/api/handler/triggers.go @@ -9,6 +9,8 @@ import ( "strings" "time" + prometheus "github.com/prometheus/client_golang/api/prometheus/v1" + "github.com/go-chi/chi" "github.com/go-chi/render" "github.com/moira-alert/moira" @@ -150,10 +152,40 @@ func createTrigger(writer http.ResponseWriter, request *http.Request) { } } +func is4xxCode(statusCode int64) bool { + return statusCode >= 400 && statusCode < 500 +} + +func errorResponseOnPrometheusError(promErr *prometheus.Error) *api.ErrorResponse { + // In github.com/prometheus/client_golang/api/prometheus/v1 Error has field `Type` + // which can be used to understand "the reason" of error. There are some constants in the lib. + if promErr.Type == prometheus.ErrBadData { + return api.ErrorInvalidRequest(fmt.Errorf("invalid prometheus targets: %w", promErr)) + } + + // VictoriaMetrics also supports prometheus api, BUT puts status code into Error.Type. + // So we can't just use constants from prometheus api client lib. + statusCode, err := strconv.ParseInt(string(promErr.Type), 10, 64) + if err != nil { + return api.ErrorInternalServer(promErr) + } + + codes4xxLeadTo500 := map[int64]struct{}{ + http.StatusUnauthorized: {}, + http.StatusForbidden: {}, + } + + if _, leadTo500 := codes4xxLeadTo500[statusCode]; is4xxCode(statusCode) && !leadTo500 { + return api.ErrorInvalidRequest(promErr) + } + + return api.ErrorInternalServer(promErr) +} + func getTriggerFromRequest(request *http.Request) (*dto.Trigger, *api.ErrorResponse) { trigger := &dto.Trigger{} if err := render.Bind(request, trigger); err != nil { - switch err.(type) { // nolint:errorlint + switch typedErr := err.(type) { // nolint:errorlint case local.ErrParseExpr, local.ErrEvalExpr, local.ErrUnknownFunction: return nil, api.ErrorInvalidRequest(fmt.Errorf("invalid graphite targets: %s", err.Error())) case expression.ErrInvalidExpression: @@ -169,6 +201,8 @@ func getTriggerFromRequest(request *http.Request) (*dto.Trigger, *api.ErrorRespo return nil, response case *json.UnmarshalTypeError: return nil, api.ErrorInvalidRequest(fmt.Errorf("invalid payload: %s", err.Error())) + case *prometheus.Error: + return nil, errorResponseOnPrometheusError(typedErr) default: return nil, api.ErrorInternalServer(err) } @@ -208,10 +242,14 @@ func triggerCheck(writer http.ResponseWriter, request *http.Request) { response := dto.TriggerCheckResponse{} if err := render.Bind(request, trigger); err != nil { - switch err.(type) { // nolint:errorlint + switch typedErr := err.(type) { // nolint:errorlint case expression.ErrInvalidExpression, local.ErrParseExpr, local.ErrEvalExpr, local.ErrUnknownFunction: - // TODO write comment, why errors are ignored, it is not obvious. - // In getTriggerFromRequest these types of errors lead to 400. + // TODO: move ErrInvalidExpression to separate case + + // These errors are skipped because if there are error from local source then it will be caught in + // dto.TargetVerification and will be explained in detail. + case *prometheus.Error: + render.Render(writer, request, errorResponseOnPrometheusError(typedErr)) //nolint default: render.Render(writer, request, api.ErrorInvalidRequest(err)) //nolint return diff --git a/api/handler/triggers_test.go b/api/handler/triggers_test.go index 83d0a1bea..f2624d6ef 100644 --- a/api/handler/triggers_test.go +++ b/api/handler/triggers_test.go @@ -12,6 +12,11 @@ import ( "testing" "time" + logging "github.com/moira-alert/moira/logging/zerolog_adapter" + "github.com/moira-alert/moira/metric_source/remote" + + prometheus "github.com/prometheus/client_golang/api/prometheus/v1" + "github.com/moira-alert/moira" "github.com/moira-alert/moira/api" dataBase "github.com/moira-alert/moira/database" @@ -139,6 +144,116 @@ func TestGetTriggerFromRequest(t *testing.T) { So(err, ShouldHaveSameTypeAs, api.ErrorInvalidRequest(fmt.Errorf(""))) }) }) + + Convey("With incorrect targets errors", t, func() { + graphiteLocalSrc := mock_metric_source.NewMockMetricSource(mockCtrl) + graphiteRemoteSrc := mock_metric_source.NewMockMetricSource(mockCtrl) + prometheusSrc := mock_metric_source.NewMockMetricSource(mockCtrl) + allSourceProvider := metricSource.CreateTestMetricSourceProvider(graphiteLocalSrc, graphiteRemoteSrc, prometheusSrc) + + graphiteLocalSrc.EXPECT().GetMetricsTTLSeconds().Return(int64(3600)).AnyTimes() + graphiteRemoteSrc.EXPECT().GetMetricsTTLSeconds().Return(int64(3600)).AnyTimes() + prometheusSrc.EXPECT().GetMetricsTTLSeconds().Return(int64(3600)).AnyTimes() + + triggerWarnValue := 0.0 + triggerErrorValue := 1.0 + ttlState := moira.TTLState("NODATA") + triggerDTO := dto.Trigger{ + TriggerModel: dto.TriggerModel{ + ID: "test_id", + Name: "Test trigger", + Desc: new(string), + Targets: []string{"foo.bar"}, + WarnValue: &triggerWarnValue, + ErrorValue: &triggerErrorValue, + TriggerType: "rising", + Tags: []string{"Normal", "DevOps", "DevOpsGraphite-duty"}, + TTLState: &ttlState, + TTL: moira.DefaultTTL, + Schedule: &moira.ScheduleData{}, + Expression: "", + Patterns: []string{}, + ClusterId: moira.DefaultCluster, + MuteNewMetrics: false, + AloneMetrics: map[string]bool{}, + CreatedAt: &time.Time{}, + UpdatedAt: &time.Time{}, + CreatedBy: "", + UpdatedBy: "anonymous", + }, + } + + Convey("for graphite remote", func() { + triggerDTO.TriggerSource = moira.GraphiteRemote + body, _ := json.Marshal(triggerDTO) + + request := httptest.NewRequest(http.MethodPut, "/trigger", bytes.NewReader(body)) + request.Header.Add("content-type", "application/json") + request = request.WithContext(middleware.SetContextValueForTest(request.Context(), "metricSourceProvider", allSourceProvider)) + + testLogger, _ := logging.GetLogger("Test") + + request = middleware.WithLogEntry(request, middleware.NewLogEntry(testLogger, request)) + + var returnedErr error = remote.ErrRemoteTriggerResponse{ + InternalError: fmt.Errorf(""), + } + + graphiteRemoteSrc.EXPECT().Fetch(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(nil, returnedErr) + + _, errRsp := getTriggerFromRequest(request) + So(errRsp, ShouldResemble, api.ErrorRemoteServerUnavailable(returnedErr)) + }) + + Convey("for prometheus remote", func() { + triggerDTO.TriggerSource = moira.PrometheusRemote + body, _ := json.Marshal(triggerDTO) + + Convey("with error type = bad_data got bad request", func() { + request := httptest.NewRequest(http.MethodPut, "/trigger", bytes.NewReader(body)) + request.Header.Add("content-type", "application/json") + request = request.WithContext(middleware.SetContextValueForTest(request.Context(), "metricSourceProvider", allSourceProvider)) + + var returnedErr error = &prometheus.Error{ + Type: prometheus.ErrBadData, + } + + prometheusSrc.EXPECT().Fetch(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(nil, returnedErr) + + _, errRsp := getTriggerFromRequest(request) + So(errRsp, ShouldResemble, api.ErrorInvalidRequest(fmt.Errorf("invalid prometheus targets: %w", returnedErr))) + }) + + Convey("with other types internal server error is returned", func() { + otherTypes := []prometheus.ErrorType{ + prometheus.ErrBadResponse, + prometheus.ErrCanceled, + prometheus.ErrClient, + prometheus.ErrExec, + prometheus.ErrTimeout, + prometheus.ErrServer, + } + + for _, errType := range otherTypes { + request := httptest.NewRequest(http.MethodPut, "/trigger", bytes.NewReader(body)) + request.Header.Add("content-type", "application/json") + request = request.WithContext(middleware.SetContextValueForTest(request.Context(), "metricSourceProvider", allSourceProvider)) + + var returnedErr error = &prometheus.Error{ + Type: errType, + } + + prometheusSrc.EXPECT().Fetch(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(nil, returnedErr) + + _, errRsp := getTriggerFromRequest(request) + So(errRsp, ShouldResemble, api.ErrorInternalServer(returnedErr)) + } + }) + }) + }) } func TestGetMetricTTLByTrigger(t *testing.T) {