From 75d4199a750cd7bd7b016bb5554e7260151b31c0 Mon Sep 17 00:00:00 2001 From: Christian Haudum Date: Tue, 30 Nov 2021 10:37:48 +0100 Subject: [PATCH 1/4] Return HTTP errors consistently as JSON response The API returned errors in plain text where the error message was the response body. However, the content type header was set to `application/json`. To make the API responses consistent, errors are now returned as JSON as well. ``` { "message": string, "status": string, "code": int } ``` Signed-off-by: Christian Haudum --- CHANGELOG.md | 1 + pkg/distributor/http.go | 7 ++-- .../frontend/transport/handler.go | 31 +++++--------- .../compactor/deletion/request_handler.go | 40 +++++++++---------- pkg/util/server/error.go | 40 +++++++++++++------ pkg/util/server/error_test.go | 11 +++-- 6 files changed, 67 insertions(+), 63 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c5beb43c83f..3688dd3f8226 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ ## Main +* [4845](https://github.com/grafana/loki/pull/4845) **chaudum** Return error responses consistently as JSON * [4826](https://github.com/grafana/loki/pull/4826) **cyriltovena**: Adds the ability to hedge storage requests. * [4828](https://github.com/grafana/loki/pull/4282) **chaudum**: Set correct `Content-Type` header in query response * [4832](https://github.com/grafana/loki/pull/4832) **taisho6339**: Use http prefix path correctly in Promtail diff --git a/pkg/distributor/http.go b/pkg/distributor/http.go index 4f76edcd1ed7..7d5e18d0f35c 100644 --- a/pkg/distributor/http.go +++ b/pkg/distributor/http.go @@ -10,6 +10,7 @@ import ( "github.com/weaveworks/common/httpgrpc" "github.com/grafana/loki/pkg/loghttp/push" + serverutil "github.com/grafana/loki/pkg/util/server" ) // PushHandler reads a snappy-compressed proto from the HTTP body. @@ -25,7 +26,7 @@ func (d *Distributor) PushHandler(w http.ResponseWriter, r *http.Request) { "err", err, ) } - http.Error(w, err.Error(), http.StatusBadRequest) + serverutil.JSONError(w, http.StatusBadRequest, err.Error()) return } @@ -61,7 +62,7 @@ func (d *Distributor) PushHandler(w http.ResponseWriter, r *http.Request) { "err", body, ) } - http.Error(w, body, int(resp.Code)) + serverutil.JSONError(w, int(resp.Code), body) } else { if d.tenantConfigs.LogPushRequest(userID) { level.Debug(logger).Log( @@ -70,6 +71,6 @@ func (d *Distributor) PushHandler(w http.ResponseWriter, r *http.Request) { "err", err.Error(), ) } - http.Error(w, err.Error(), http.StatusInternalServerError) + serverutil.JSONError(w, http.StatusInternalServerError, err.Error()) } } diff --git a/pkg/lokifrontend/frontend/transport/handler.go b/pkg/lokifrontend/frontend/transport/handler.go index b6689802108b..75db990eaf40 100644 --- a/pkg/lokifrontend/frontend/transport/handler.go +++ b/pkg/lokifrontend/frontend/transport/handler.go @@ -13,17 +13,16 @@ import ( "strings" "time" + querier_stats "github.com/cortexproject/cortex/pkg/querier/stats" + "github.com/cortexproject/cortex/pkg/tenant" + "github.com/cortexproject/cortex/pkg/util" + util_log "github.com/cortexproject/cortex/pkg/util/log" "github.com/go-kit/log" "github.com/go-kit/log/level" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/promauto" - "github.com/weaveworks/common/httpgrpc" - "github.com/weaveworks/common/httpgrpc/server" - querier_stats "github.com/cortexproject/cortex/pkg/querier/stats" - "github.com/cortexproject/cortex/pkg/tenant" - "github.com/cortexproject/cortex/pkg/util" - util_log "github.com/cortexproject/cortex/pkg/util/log" + serverutil "github.com/grafana/loki/pkg/util/server" ) const ( @@ -32,12 +31,6 @@ const ( ServiceTimingHeaderName = "Server-Timing" ) -var ( - errCanceled = httpgrpc.Errorf(StatusClientClosedRequest, context.Canceled.Error()) - errDeadlineExceeded = httpgrpc.Errorf(http.StatusGatewayTimeout, context.DeadlineExceeded.Error()) - errRequestEntityTooLarge = httpgrpc.Errorf(http.StatusRequestEntityTooLarge, "http: request body too large") -) - // Config for a Handler. type HandlerConfig struct { LogQueriesLongerThan time.Duration `yaml:"log_queries_longer_than"` @@ -226,17 +219,11 @@ func formatQueryString(queryString url.Values) (fields []interface{}) { } func writeError(w http.ResponseWriter, err error) { - switch err { - case context.Canceled: - err = errCanceled - case context.DeadlineExceeded: - err = errDeadlineExceeded - default: - if util.IsRequestBodyTooLarge(err) { - err = errRequestEntityTooLarge - } + if util.IsRequestBodyTooLarge(err) { + serverutil.JSONError(w, http.StatusRequestEntityTooLarge, "http: request body too large") + return } - server.WriteError(w, err) + serverutil.WriteError(err, w) } func writeServiceTimingHeader(queryResponseTime time.Duration, headers http.Header, stats *querier_stats.Stats) { diff --git a/pkg/storage/stores/shipper/compactor/deletion/request_handler.go b/pkg/storage/stores/shipper/compactor/deletion/request_handler.go index dfd707fa8f7c..d92f0ed94724 100644 --- a/pkg/storage/stores/shipper/compactor/deletion/request_handler.go +++ b/pkg/storage/stores/shipper/compactor/deletion/request_handler.go @@ -2,18 +2,18 @@ package deletion import ( "encoding/json" - "fmt" "net/http" "time" + "github.com/cortexproject/cortex/pkg/tenant" + "github.com/cortexproject/cortex/pkg/util" + util_log "github.com/cortexproject/cortex/pkg/util/log" "github.com/go-kit/log/level" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/common/model" "github.com/prometheus/prometheus/promql/parser" - "github.com/cortexproject/cortex/pkg/tenant" - "github.com/cortexproject/cortex/pkg/util" - util_log "github.com/cortexproject/cortex/pkg/util/log" + serverutil "github.com/grafana/loki/pkg/util/server" ) // DeleteRequestHandler provides handlers for delete requests @@ -39,21 +39,21 @@ func (dm *DeleteRequestHandler) AddDeleteRequestHandler(w http.ResponseWriter, r ctx := r.Context() userID, err := tenant.TenantID(ctx) if err != nil { - http.Error(w, err.Error(), http.StatusBadRequest) + serverutil.JSONError(w, http.StatusBadRequest, err.Error()) return } params := r.URL.Query() match := params["match[]"] if len(match) == 0 { - http.Error(w, "selectors not set", http.StatusBadRequest) + serverutil.JSONError(w, http.StatusBadRequest, "selectors not set") return } for i := range match { _, err := parser.ParseMetricSelector(match[i]) if err != nil { - http.Error(w, err.Error(), http.StatusBadRequest) + serverutil.JSONError(w, http.StatusBadRequest, err.Error()) return } } @@ -63,7 +63,7 @@ func (dm *DeleteRequestHandler) AddDeleteRequestHandler(w http.ResponseWriter, r if startParam != "" { startTime, err = util.ParseTime(startParam) if err != nil { - http.Error(w, err.Error(), http.StatusBadRequest) + serverutil.JSONError(w, http.StatusBadRequest, err.Error()) return } } @@ -74,12 +74,12 @@ func (dm *DeleteRequestHandler) AddDeleteRequestHandler(w http.ResponseWriter, r if endParam != "" { endTime, err = util.ParseTime(endParam) if err != nil { - http.Error(w, err.Error(), http.StatusBadRequest) + serverutil.JSONError(w, http.StatusBadRequest, err.Error()) return } if endTime > int64(model.Now()) { - http.Error(w, "deletes in future not allowed", http.StatusBadRequest) + serverutil.JSONError(w, http.StatusBadRequest, "deletes in future not allowed") return } } @@ -91,7 +91,7 @@ func (dm *DeleteRequestHandler) AddDeleteRequestHandler(w http.ResponseWriter, r if err := dm.deleteRequestsStore.AddDeleteRequest(ctx, userID, model.Time(startTime), model.Time(endTime), match); err != nil { level.Error(util_log.Logger).Log("msg", "error adding delete request to the store", "err", err) - http.Error(w, err.Error(), http.StatusInternalServerError) + serverutil.JSONError(w, http.StatusInternalServerError, err.Error()) return } @@ -104,20 +104,20 @@ func (dm *DeleteRequestHandler) GetAllDeleteRequestsHandler(w http.ResponseWrite ctx := r.Context() userID, err := tenant.TenantID(ctx) if err != nil { - http.Error(w, err.Error(), http.StatusBadRequest) + serverutil.JSONError(w, http.StatusBadRequest, err.Error()) return } deleteRequests, err := dm.deleteRequestsStore.GetAllDeleteRequestsForUser(ctx, userID) if err != nil { level.Error(util_log.Logger).Log("msg", "error getting delete requests from the store", "err", err) - http.Error(w, err.Error(), http.StatusInternalServerError) + serverutil.JSONError(w, http.StatusInternalServerError, err.Error()) return } if err := json.NewEncoder(w).Encode(deleteRequests); err != nil { level.Error(util_log.Logger).Log("msg", "error marshalling response", "err", err) - http.Error(w, fmt.Sprintf("Error marshalling response: %v", err), http.StatusInternalServerError) + serverutil.JSONError(w, http.StatusInternalServerError, "error marshalling response: %v", err) } } @@ -126,7 +126,7 @@ func (dm *DeleteRequestHandler) CancelDeleteRequestHandler(w http.ResponseWriter ctx := r.Context() userID, err := tenant.TenantID(ctx) if err != nil { - http.Error(w, err.Error(), http.StatusBadRequest) + serverutil.JSONError(w, http.StatusBadRequest, err.Error()) return } @@ -136,28 +136,28 @@ func (dm *DeleteRequestHandler) CancelDeleteRequestHandler(w http.ResponseWriter deleteRequest, err := dm.deleteRequestsStore.GetDeleteRequest(ctx, userID, requestID) if err != nil { level.Error(util_log.Logger).Log("msg", "error getting delete request from the store", "err", err) - http.Error(w, err.Error(), http.StatusInternalServerError) + serverutil.JSONError(w, http.StatusInternalServerError, err.Error()) return } if deleteRequest == nil { - http.Error(w, "could not find delete request with given id", http.StatusBadRequest) + serverutil.JSONError(w, http.StatusBadRequest, "could not find delete request with given id") return } if deleteRequest.Status != StatusReceived { - http.Error(w, "deletion of request which is in process or already processed is not allowed", http.StatusBadRequest) + serverutil.JSONError(w, http.StatusBadRequest, "deletion of request which is in process or already processed is not allowed") return } if deleteRequest.CreatedAt.Add(dm.deleteRequestCancelPeriod).Before(model.Now()) { - http.Error(w, fmt.Sprintf("deletion of request past the deadline of %s since its creation is not allowed", dm.deleteRequestCancelPeriod.String()), http.StatusBadRequest) + serverutil.JSONError(w, http.StatusBadRequest, "deletion of request past the deadline of %s since its creation is not allowed", dm.deleteRequestCancelPeriod.String()) return } if err := dm.deleteRequestsStore.RemoveDeleteRequest(ctx, userID, requestID, deleteRequest.CreatedAt, deleteRequest.StartTime, deleteRequest.EndTime); err != nil { level.Error(util_log.Logger).Log("msg", "error cancelling the delete request", "err", err) - http.Error(w, err.Error(), http.StatusInternalServerError) + serverutil.JSONError(w, http.StatusInternalServerError, err.Error()) return } diff --git a/pkg/util/server/error.go b/pkg/util/server/error.go index 05ee1bc51077..4b55f6d87ede 100644 --- a/pkg/util/server/error.go +++ b/pkg/util/server/error.go @@ -2,7 +2,9 @@ package server import ( "context" + "encoding/json" "errors" + "fmt" "net/http" "google.golang.org/grpc/codes" @@ -26,6 +28,22 @@ const ( ErrDeadlineExceeded = "Request timed out, decrease the duration of the request or add more label matchers (prefer exact match over regex match) to reduce the amount of data processed." ) +type ErrorResponseBody struct { + Code int `json:"code"` + Status string `json:"status"` + Message string `json:"message"` +} + +func JSONError(w http.ResponseWriter, code int, message string, args ...interface{}) { + w.WriteHeader(code) + w.Header().Set("Content-Type", "application/json; charset=utf-8") + json.NewEncoder(w).Encode(ErrorResponseBody{ + Code: code, + Status: "error", + Message: fmt.Sprintf(message, args...), + }) +} + // WriteError write a go error with the correct status code. func WriteError(err error, w http.ResponseWriter) { var ( @@ -35,11 +53,11 @@ func WriteError(err error, w http.ResponseWriter) { me, ok := err.(util.MultiError) if ok && me.IsCancel() { - http.Error(w, ErrClientCanceled, StatusClientClosedRequest) + JSONError(w, StatusClientClosedRequest, ErrClientCanceled) return } if ok && me.IsDeadlineExceeded() { - http.Error(w, ErrDeadlineExceeded, http.StatusGatewayTimeout) + JSONError(w, http.StatusGatewayTimeout, ErrDeadlineExceeded) return } @@ -48,21 +66,19 @@ func WriteError(err error, w http.ResponseWriter) { case errors.Is(err, context.Canceled) || (isRPC && s.Code() == codes.Canceled) || (errors.As(err, &promErr) && errors.Is(promErr.Err, context.Canceled)): - http.Error(w, ErrClientCanceled, StatusClientClosedRequest) + JSONError(w, StatusClientClosedRequest, ErrClientCanceled) case errors.Is(err, context.DeadlineExceeded) || (isRPC && s.Code() == codes.DeadlineExceeded): - http.Error(w, ErrDeadlineExceeded, http.StatusGatewayTimeout) - case errors.As(err, &queryErr): - http.Error(w, err.Error(), http.StatusBadRequest) - case errors.Is(err, logqlmodel.ErrLimit) || errors.Is(err, logqlmodel.ErrParse) || errors.Is(err, logqlmodel.ErrPipeline): - http.Error(w, err.Error(), http.StatusBadRequest) - case errors.Is(err, user.ErrNoOrgID): - http.Error(w, err.Error(), http.StatusBadRequest) + JSONError(w, http.StatusGatewayTimeout, ErrDeadlineExceeded) + case errors.As(err, &queryErr), + errors.Is(err, logqlmodel.ErrLimit) || errors.Is(err, logqlmodel.ErrParse) || errors.Is(err, logqlmodel.ErrPipeline), + errors.Is(err, user.ErrNoOrgID): + JSONError(w, http.StatusBadRequest, err.Error()) default: if grpcErr, ok := httpgrpc.HTTPResponseFromError(err); ok { - http.Error(w, string(grpcErr.Body), int(grpcErr.Code)) + JSONError(w, int(grpcErr.Code), string(grpcErr.Body)) return } - http.Error(w, err.Error(), http.StatusInternalServerError) + JSONError(w, http.StatusInternalServerError, err.Error()) } } diff --git a/pkg/util/server/error_test.go b/pkg/util/server/error_test.go index 07975ab84a97..de90b271bf6f 100644 --- a/pkg/util/server/error_test.go +++ b/pkg/util/server/error_test.go @@ -2,9 +2,9 @@ package server import ( "context" + "encoding/json" "errors" "fmt" - "io/ioutil" "net/http" "net/http/httptest" "testing" @@ -54,12 +54,11 @@ func Test_writeError(t *testing.T) { t.Run(tt.name, func(t *testing.T) { rec := httptest.NewRecorder() WriteError(tt.err, rec) + res := &ErrorResponseBody{} + json.NewDecoder(rec.Result().Body).Decode(res) + require.Equal(t, tt.expectedStatus, res.Code) require.Equal(t, tt.expectedStatus, rec.Result().StatusCode) - b, err := ioutil.ReadAll(rec.Result().Body) - if err != nil { - t.Fatal(err) - } - require.Equal(t, tt.msg, string(b[:len(b)-1])) + require.Equal(t, tt.msg, res.Message) }) } } From f700a241b021fb8e5283abc432d26403e9c7bd6b Mon Sep 17 00:00:00 2001 From: Christian Haudum Date: Wed, 1 Dec 2021 17:00:30 +0100 Subject: [PATCH 2/4] fixup! Return HTTP errors consistently as JSON response Signed-off-by: Christian Haudum --- docs/sources/upgrading/_index.md | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/docs/sources/upgrading/_index.md b/docs/sources/upgrading/_index.md index d9d23d6e3a3a..deb166d57b12 100644 --- a/docs/sources/upgrading/_index.md +++ b/docs/sources/upgrading/_index.md @@ -31,6 +31,26 @@ The output is incredibly verbose as it shows the entire internal config struct u ## Main / Unreleased +### Loki + +#### Error responses from API + +The body of HTTP error responses from API endpoints changed from plain text to +JSON. The `Content-Type` header was previously already set incorrectly to +`application/json`. Therefore returning JSON fixes this incosistency + +The response body has the following schema: + +```json +{ + "code": , + "message": "", + "status": "error" +} +``` + +"404 Not Found" errors still return responses with content type `text/plain`. + ### Promtail #### `gcplog` labels have changed From 48203226c45145d8bbcb2dffbfc4d4d156faacac Mon Sep 17 00:00:00 2001 From: Christian Haudum Date: Thu, 2 Dec 2021 08:29:20 +0100 Subject: [PATCH 3/4] Return 404 Not Found responses as JSON Signed-off-by: Christian Haudum --- docs/sources/upgrading/_index.md | 2 -- pkg/loki/loki.go | 1 + pkg/util/server/error.go | 6 +++++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/docs/sources/upgrading/_index.md b/docs/sources/upgrading/_index.md index deb166d57b12..563e8848aa83 100644 --- a/docs/sources/upgrading/_index.md +++ b/docs/sources/upgrading/_index.md @@ -49,8 +49,6 @@ The response body has the following schema: } ``` -"404 Not Found" errors still return responses with content type `text/plain`. - ### Promtail #### `gcplog` labels have changed diff --git a/pkg/loki/loki.go b/pkg/loki/loki.go index f4f57ea2ae51..2b5f8ceb16aa 100644 --- a/pkg/loki/loki.go +++ b/pkg/loki/loki.go @@ -304,6 +304,7 @@ func (t *Loki) Run(opts RunOpts) error { t.serviceMap = serviceMap t.Server.HTTP.Path("/services").Methods("GET").Handler(http.HandlerFunc(t.servicesHandler)) + t.Server.HTTP.NotFoundHandler = http.HandlerFunc(serverutil.NotFoundHandler) // get all services, create service manager and tell it to start var servs []services.Service diff --git a/pkg/util/server/error.go b/pkg/util/server/error.go index 4b55f6d87ede..6b423d1440a4 100644 --- a/pkg/util/server/error.go +++ b/pkg/util/server/error.go @@ -34,9 +34,13 @@ type ErrorResponseBody struct { Message string `json:"message"` } +func NotFoundHandler(w http.ResponseWriter, r *http.Request) { + JSONError(w, 404, "page not found") +} + func JSONError(w http.ResponseWriter, code int, message string, args ...interface{}) { - w.WriteHeader(code) w.Header().Set("Content-Type", "application/json; charset=utf-8") + w.WriteHeader(code) json.NewEncoder(w).Encode(ErrorResponseBody{ Code: code, Status: "error", From c14902cc64bca306ca5f0e9e210752484309bf08 Mon Sep 17 00:00:00 2001 From: Christian Haudum Date: Thu, 2 Dec 2021 15:31:57 +0100 Subject: [PATCH 4/4] fixup! Return 404 Not Found responses as JSON Signed-off-by: Christian Haudum --- pkg/util/server/error.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/util/server/error.go b/pkg/util/server/error.go index 6b423d1440a4..d8040f5ffdaa 100644 --- a/pkg/util/server/error.go +++ b/pkg/util/server/error.go @@ -35,7 +35,7 @@ type ErrorResponseBody struct { } func NotFoundHandler(w http.ResponseWriter, r *http.Request) { - JSONError(w, 404, "page not found") + JSONError(w, 404, "not found") } func JSONError(w http.ResponseWriter, code int, message string, args ...interface{}) {