Change error responses from plain text to JSON (#4845)

* 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 <christian.haudum@gmail.com>

* fixup! Return HTTP errors consistently as JSON response

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

* Return 404 Not Found responses as JSON

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

* fixup! Return 404 Not Found responses as JSON

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

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

docs/sources/upgrading/_index.md

@@ -31,6 +31,24 @@ 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": <http status code>,
+  "message": "<error message>",
+  "status": "error"
+}
+```
+
 ### Promtail
 
 #### `gcplog` labels have changed

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())
 	}
 }

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

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) {

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
 	}
 

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,26 @@ 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 NotFoundHandler(w http.ResponseWriter, r *http.Request) {
+	JSONError(w, 404, "not found")
+}
+
+func JSONError(w http.ResponseWriter, code int, message string, args ...interface{}) {
+	w.Header().Set("Content-Type", "application/json; charset=utf-8")
+	w.WriteHeader(code)
+	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 +57,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 +70,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())
 	}
 }

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)
 		})
 	}
 }