Fix Content-Type header in /healthz when status is not 200 OK #4437
Conversation
wallyqs
changed the title
wallyqs
changed the title
There was a problem hiding this comment.
LGTM, thanks for the contribution. I will make a follow up PR to include this into v2.10 dev branch here: dev...healthz-mon-fixes
| func ResponseHandler(w http.ResponseWriter, r *http.Request, data []byte) { | ||
| handleResponse(http.StatusOK, w, r, data) |
There was a problem hiding this comment.
Note that we currently do not use this handler in most monitoring APIs when serving error responses (one of the reasons why we missed setting header right in /healthz)
Lines 728 to 732 in e5836fc
There was a problem hiding this comment.
BTW shouldn't error responses be also returned as JSON for more consistency? Maybe in the next major version? Because this change might break the current consumers of the API.
There was a problem hiding this comment.
yes this could be done in a different version, maybe in the dev branch for next release cycle
| @@ -2301,19 +2301,26 @@ func (s *Server) HandleAccountStatz(w http.ResponseWriter, r *http.Request) { | |||
| ResponseHandler(w, r, b) | |||
| } | |||
|
|
|||
| // ResponseHandler handles responses for monitoring routes | |||
| // ResponseHandler handles responses for monitoring routes. | |||
There was a problem hiding this comment.
It was not necessary to have this exported but it has been public for quite some time already, introduced in https://github.com/nats-io/nats-server/pull/103/files
|
|
||
| // Another check for JSONP requests. | ||
| jsonpURL := createCallbackURL(t, url) // Adds a callback query param. | ||
| jsonpBody := readBodyEx(t, jsonpURL, statusCode, appJSContent) |
There was a problem hiding this comment.
as you may have found readBodyEx currently uses misses the line of the error which can be fixed by using t.Helper, fixing that in a separate PR when including this change
Follow up from #4437 content-type fix for v2.9.22, some fixes to the response from `/healthz` for dev: - In #[3326](#4097) it was changed to return 500 status when before we used to return 503 so this changes it back. - Also as part of #3326 we started to return `status_code` in the healthz response (e.g `{"status":"ok","status_code":200}`) so this removes it for http responses just relying on the http header.
- Added a new internal function `handleResponse` that accepts the HTTP status code and sets it after setting the headers - Added tests for the `/healthz` endpoint for the `ok`, `error` and `unavailable` statuses - Changed the IETF API health check URL to https://datatracker.ietf.org/doc/html/draft-inadarei-api-health-check Resolves #4436
handleResponsethat accepts the HTTP status code and sets it after setting the headers/healthzendpoint for theok,errorandunavailablestatusesThe function
ResponseHandlerthat handles the monitoring server responses is exported so I had to add another internal function to prevent a breaking change to the API.I don't know if dependent code should use this function, maybe in the next major version this function should be unexported because it's only used for the monitoring endpoints.
Resolves #4436