-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Content-Type header in /healthz when status is not 200 OK #4437
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the contribution!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
c, err := s.Connz(connzOpts) | |
if err != nil { | |
w.WriteHeader(http.StatusBadRequest) | |
w.Write([]byte(err.Error())) | |
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
handleResponse
that accepts the HTTP status code and sets it after setting the headers/healthz
endpoint for theok
,error
andunavailable
statusesThe function
ResponseHandler
that 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