-
-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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. | ||||||||||||
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 commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 |
||||||||||||
} | ||||||||||||
|
||||||||||||
// handleResponse handles responses for monitoring routes with a specific HTTP status code. | ||||||||||||
func handleResponse(code int, w http.ResponseWriter, r *http.Request, data []byte) { | ||||||||||||
// Get callback from request | ||||||||||||
callback := r.URL.Query().Get("callback") | ||||||||||||
// If callback is not empty then | ||||||||||||
if callback != "" { | ||||||||||||
// Response for JSONP | ||||||||||||
w.Header().Set("Content-Type", "application/javascript") | ||||||||||||
w.WriteHeader(code) | ||||||||||||
fmt.Fprintf(w, "%s(%s)", callback, data) | ||||||||||||
} else { | ||||||||||||
// Otherwise JSON | ||||||||||||
w.Header().Set("Content-Type", "application/json") | ||||||||||||
w.Header().Set("Access-Control-Allow-Origin", "*") | ||||||||||||
w.WriteHeader(code) | ||||||||||||
w.Write(data) | ||||||||||||
} | ||||||||||||
} | ||||||||||||
|
@@ -3048,7 +3055,7 @@ type HealthStatus struct { | |||||||||||
Error string `json:"error,omitempty"` | ||||||||||||
} | ||||||||||||
|
||||||||||||
// https://tools.ietf.org/id/draft-inadarei-api-health-check-05.html | ||||||||||||
// https://datatracker.ietf.org/doc/html/draft-inadarei-api-health-check | ||||||||||||
func (s *Server) HandleHealthz(w http.ResponseWriter, r *http.Request) { | ||||||||||||
s.mu.Lock() | ||||||||||||
s.httpReqStats[HealthzPath]++ | ||||||||||||
|
@@ -3075,16 +3082,19 @@ func (s *Server) HandleHealthz(w http.ResponseWriter, r *http.Request) { | |||||||||||
JSEnabledOnly: jsEnabledOnly, | ||||||||||||
JSServerOnly: jsServerOnly, | ||||||||||||
}) | ||||||||||||
|
||||||||||||
code := http.StatusOK | ||||||||||||
|
||||||||||||
if hs.Error != _EMPTY_ { | ||||||||||||
s.Warnf("Healthcheck failed: %q", hs.Error) | ||||||||||||
w.WriteHeader(http.StatusServiceUnavailable) | ||||||||||||
code = http.StatusServiceUnavailable | ||||||||||||
} | ||||||||||||
b, err := json.Marshal(hs) | ||||||||||||
if err != nil { | ||||||||||||
s.Errorf("Error marshaling response to /healthz request: %v", err) | ||||||||||||
} | ||||||||||||
|
||||||||||||
ResponseHandler(w, r, b) | ||||||||||||
handleResponse(code, w, r, b) | ||||||||||||
} | ||||||||||||
|
||||||||||||
// Generate health status. | ||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4775,3 +4775,119 @@ func TestMonitorAccountszMappingOrderReporting(t *testing.T) { | |
} | ||
require_True(t, found) | ||
} | ||
|
||
// createCallbackURL adds a callback query parameter for JSONP requests. | ||
func createCallbackURL(t *testing.T, endpoint string) string { | ||
t.Helper() | ||
|
||
u, err := url.Parse(endpoint) | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
|
||
params := u.Query() | ||
params.Set("callback", "callback") | ||
|
||
u.RawQuery = params.Encode() | ||
|
||
return u.String() | ||
} | ||
|
||
// stripCallback removes the JSONP callback function from the response. | ||
// Returns the JSON body without the wrapping callback function. | ||
// If there's no callback function, the data is returned as is. | ||
func stripCallback(data []byte) []byte { | ||
// Cut the JSONP callback function with the opening parentheses. | ||
_, after, found := bytes.Cut(data, []byte("(")) | ||
|
||
if found { | ||
return bytes.TrimSuffix(after, []byte(")")) | ||
} | ||
|
||
return data | ||
} | ||
|
||
// expectHealthStatus makes 1 regular and 1 JSONP request to the URL and checks the | ||
// HTTP status code, Content-Type header and health status string. | ||
func expectHealthStatus(t *testing.T, url string, statusCode int, wantStatus string) { | ||
t.Helper() | ||
|
||
// First check for regular requests. | ||
body := readBodyEx(t, url, statusCode, appJSONContent) | ||
checkHealthStatus(t, body, wantStatus) | ||
|
||
// 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 commentThe reason will be displayed to describe this comment to others. Learn more. as you may have found |
||
checkHealthStatus(t, stripCallback(jsonpBody), wantStatus) | ||
} | ||
|
||
// checkHealthStatus checks the health status from a JSON response. | ||
func checkHealthStatus(t *testing.T, body []byte, wantStatus string) { | ||
t.Helper() | ||
|
||
h := &HealthStatus{} | ||
|
||
if err := json.Unmarshal(body, h); err != nil { | ||
t.Fatalf("error unmarshalling the body: %v", err) | ||
} | ||
|
||
if h.Status != wantStatus { | ||
t.Errorf("want health status %q, got %q", wantStatus, h.Status) | ||
} | ||
} | ||
|
||
// checkHealthzEndpoint makes requests to the /healthz endpoint and checks the health status. | ||
func checkHealthzEndpoint(t *testing.T, address string, statusCode int, wantStatus string) { | ||
t.Helper() | ||
|
||
cases := map[string]string{ | ||
"healthz": fmt.Sprintf("http://%s/healthz", address), | ||
"js-enabled-only": fmt.Sprintf("http://%s/healthz?js-enabled-only=true", address), | ||
"js-server-only": fmt.Sprintf("http://%s/healthz?js-server-only=true", address), | ||
} | ||
|
||
for name, url := range cases { | ||
t.Run(name, func(t *testing.T) { | ||
expectHealthStatus(t, url, statusCode, wantStatus) | ||
}) | ||
} | ||
} | ||
|
||
func TestHealthzStatusOK(t *testing.T) { | ||
s := runMonitorServer() | ||
defer s.Shutdown() | ||
|
||
checkHealthzEndpoint(t, s.MonitorAddr().String(), http.StatusOK, "ok") | ||
} | ||
|
||
func TestHealthzStatusError(t *testing.T) { | ||
s := runMonitorServer() | ||
defer s.Shutdown() | ||
|
||
// Intentionally causing an error in readyForConnections(). | ||
// Note: Private field access, taking advantage of having the tests in the same package. | ||
s.listener = nil | ||
|
||
checkHealthzEndpoint(t, s.MonitorAddr().String(), http.StatusServiceUnavailable, "error") | ||
} | ||
|
||
func TestHealthzStatusUnavailable(t *testing.T) { | ||
opts := DefaultMonitorOptions() | ||
opts.JetStream = true | ||
|
||
s := RunServer(opts) | ||
defer s.Shutdown() | ||
|
||
if !s.JetStreamEnabled() { | ||
t.Fatalf("want JetStream to be enabled first") | ||
} | ||
|
||
err := s.DisableJetStream() | ||
|
||
if err != nil { | ||
t.Fatalf("got an error disabling JetStream: %v", err) | ||
} | ||
|
||
checkHealthzEndpoint(t, s.MonitorAddr().String(), http.StatusServiceUnavailable, "unavailable") | ||
} |
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