Change error responses from plain text to JSON #4845

chaudum · 2021-11-30T09:39:12Z

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

What this PR does / why we need it:

Set correct content-type header when returning error responses with plain text body.

Which issue(s) this PR fixes:
Fixes #4844

Checklist

Documentation added
Tests updated
Add an entry in the CHANGELOG.md about the changes.

dannykopping · 2021-11-30T11:50:47Z

pkg/lokifrontend/frontend/transport/handler.go

@@ -236,6 +236,7 @@ func writeError(w http.ResponseWriter, err error) {
 			err = errRequestEntityTooLarge
 		}
 	}
+	w.Header().Set("Content-Type", "text/plain; charset=UTF-8")


Hhmm, I wonder if it's better for us to instead format the error as JSON so that at least we have some consistency.
What made you take this approach vs that one?

Yeah, good point, I've also considered that, and thinking about it again, it's clearly the more consistent approach. I guess, people do not really depend on the response body being kept the same.
Also, I think there are still other occurrences where the response body is plain text. Will update the PR.

Wouldn't that break the API? I'm not sure how strict we are there.

A few thoughts:

the API is inconsistent

if we're changing anything it's technically breaking the API

I'd rather break it in the direction of consistency 😝

Agree with consistency even by breaking the API response. But should definitely add it in our docs/sources/upgrading/_index.md. (/cc @chaudum )

DylanGuedes · 2021-11-30T18:13:16Z

pkg/util/server/error.go

+
+func JSONError(w http.ResponseWriter, code int, message string, args ...interface{}) {
+	w.WriteHeader(code)
+	w.Header().Set("Content-Type", "application/json; charset=UTF8")


nit

Suggested change

w.Header().Set("Content-Type", "application/json; charset=UTF8")

w.Header().Set("Content-Type", "application/json; charset=UTF-8")

pkg/util/server/error_test.go

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>

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

KMiller-Grafana

Docs portion of the PR looks good to me.

dannykopping

Partial review; have a couple questions

dannykopping · 2021-12-02T06:41:12Z

docs/sources/upgrading/_index.md

+}
+```
+
+"404 Not Found" errors still return responses with content type `text/plain`.


Why do we special-case 404s? This is going to make writing Loki clients complex.

404 errors are handled differently than errors that occur in our handlers. However, it should be possible to override the NotFoundHandler of the router.

Can you look into that please? We should really try have consistency across the board if possible.

It was actually pretty easy to achieve: 4820322

dannykopping · 2021-12-02T06:44:35Z

pkg/storage/stores/shipper/compactor/deletion/request_handler.go

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


I'm not too familiar with how HTTP errors are handled, but are they not all handled pkg/util/server/error.go -> WriteError? If we use http.Error here does it kill the request and write this error directly?

WriteError handles generic errors as Internal Server Error. Therefore we bypass WriteError and call JSONError directly, so we can specify a status code and an error message.

http.Error sets the status code, content type header (text/plain) and writes the response body. Usually you want to return thereafter.

OK thanks for that. I was looking for a way that we could somehow add middleware to catch these http.Error calls, but they write directly to the http.ResponseWriter :(

I would've loved to have stuck with idiomatic gorilla/mux & net/http, but it seems we don't have a good layer of indirection here.

What you've got here looks like a good solution 👍

An option to catch and resolve errors in a middleware would be, to write an occurring error to the request context and check for it in the middleware handler. I don't think that is very idiomatic, though. Writing the error to the response writer seems to be the common way.

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

dannykopping

LGTM 👍 great work @chaudum
One tiny nit then I'll merge

dannykopping · 2021-12-02T14:17:45Z

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


<nit>

Suggested change

JSONError(w, 404, "page not found")

JSONError(w, 404, "not found")

</nit>

We don't really work with "pages" - let's not confuse the operator

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

chaudum · 2021-12-03T06:38:11Z

@darrenjaneczek Could this potentially affect the Loki datasource plugin / GEL plugin?

…nt type from plain text to JSON is a breaking change of the API and we should wait to do this at a major release. Signed-off-by: Ed Welch <edward.welch@grafana.com>

…5772) * This reverts the changes from #4845, unfortunately changing the content type from plain text to JSON is a breaking change of the API and we should wait to do this at a major release. Signed-off-by: Ed Welch <edward.welch@grafana.com> * fix tests which were updated after initial change. Signed-off-by: Ed Welch <edward.welch@grafana.com>

…5772) * This reverts the changes from #4845, unfortunately changing the content type from plain text to JSON is a breaking change of the API and we should wait to do this at a major release. Signed-off-by: Ed Welch <edward.welch@grafana.com> * fix tests which were updated after initial change. Signed-off-by: Ed Welch <edward.welch@grafana.com> (cherry picked from commit c158b2c)

…5772) (#5774) * This reverts the changes from #4845, unfortunately changing the content type from plain text to JSON is a breaking change of the API and we should wait to do this at a major release. Signed-off-by: Ed Welch <edward.welch@grafana.com> * fix tests which were updated after initial change. Signed-off-by: Ed Welch <edward.welch@grafana.com> (cherry picked from commit c158b2c) Co-authored-by: Ed Welch <edward.welch@grafana.com>

… API (grafana#5772) (grafana#5774) * This reverts the changes from grafana#4845, unfortunately changing the content type from plain text to JSON is a breaking change of the API and we should wait to do this at a major release. Signed-off-by: Ed Welch <edward.welch@grafana.com> * fix tests which were updated after initial change. Signed-off-by: Ed Welch <edward.welch@grafana.com> (cherry picked from commit c158b2c) Co-authored-by: Ed Welch <edward.welch@grafana.com>

pull-request-size bot added the size/XS label Nov 30, 2021

chaudum force-pushed the chaudum/error-content-type branch 2 times, most recently from 6e99e83 to da5fd0d Compare November 30, 2021 10:09

chaudum changed the title ~~Set correct content type header when returning 4xx errors~~ Set correct content type header when returning plain text error response Nov 30, 2021

chaudum marked this pull request as ready for review November 30, 2021 10:50

chaudum requested a review from a team as a code owner November 30, 2021 10:50

dannykopping reviewed Nov 30, 2021

View reviewed changes

chaudum marked this pull request as draft November 30, 2021 16:33

pull-request-size bot added size/M and removed size/XS labels Nov 30, 2021

DylanGuedes reviewed Nov 30, 2021

View reviewed changes

pull-request-size bot added size/L and removed size/M labels Dec 1, 2021

chaudum changed the title ~~Set correct content type header when returning plain text error response~~ Change error responses from plain text to JSON Dec 1, 2021

chaudum force-pushed the chaudum/error-content-type branch from 22f0a78 to 693fda0 Compare December 1, 2021 08:19

chaudum force-pushed the chaudum/error-content-type branch from d23c854 to 75d4199 Compare December 1, 2021 09:41

chaudum marked this pull request as ready for review December 1, 2021 15:47

fixup! Return HTTP errors consistently as JSON response

f700a24

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

chaudum requested a review from KMiller-Grafana as a code owner December 1, 2021 16:00

KMiller-Grafana approved these changes Dec 1, 2021

View reviewed changes

dannykopping reviewed Dec 2, 2021

View reviewed changes

Return 404 Not Found responses as JSON

4820322

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

dannykopping approved these changes Dec 2, 2021

View reviewed changes

fixup! Return 404 Not Found responses as JSON

c14902c

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

dannykopping merged commit e573a4d into main Dec 2, 2021

dannykopping deleted the chaudum/error-content-type branch December 2, 2021 14:52

slim-bean mentioned this pull request Apr 5, 2022

Loki: Revert #4845 which changed the format of errors from the API #5772

Merged

slim-bean mentioned this pull request Apr 5, 2022

Wrong content type header for Bad Request response #4844

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change error responses from plain text to JSON #4845

Change error responses from plain text to JSON #4845

chaudum commented Nov 30, 2021 •

edited

dannykopping Nov 30, 2021

chaudum Nov 30, 2021

jeschkies Nov 30, 2021

dannykopping Nov 30, 2021

kavirajk Dec 1, 2021

DylanGuedes Nov 30, 2021

KMiller-Grafana left a comment

dannykopping left a comment

dannykopping Dec 2, 2021

chaudum Dec 2, 2021

dannykopping Dec 2, 2021

chaudum Dec 2, 2021

dannykopping Dec 2, 2021

chaudum Dec 2, 2021

dannykopping Dec 2, 2021

chaudum Dec 2, 2021

dannykopping left a comment

dannykopping Dec 2, 2021

chaudum commented Dec 3, 2021

	w.Header().Set("Content-Type", "application/json; charset=UTF8")
	w.Header().Set("Content-Type", "application/json; charset=UTF-8")

	JSONError(w, 404, "page not found")
	JSONError(w, 404, "not found")

Change error responses from plain text to JSON #4845

Change error responses from plain text to JSON #4845

Conversation

chaudum commented Nov 30, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KMiller-Grafana left a comment

Choose a reason for hiding this comment

dannykopping left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannykopping left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaudum commented Dec 3, 2021

chaudum commented Nov 30, 2021 •

edited