From 6d6d3cfa5597d8dd377e497cdcb7bd638b34cd2a Mon Sep 17 00:00:00 2001 From: Pierre Mdawar Date: Thu, 31 Aug 2023 23:55:20 +0300 Subject: [PATCH] Fix Content-Type header in /healthz when status is not 200 OK (#4437) - 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 --- server/monitor.go | 18 +++++-- server/monitor_test.go | 116 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+), 4 deletions(-) diff --git a/server/monitor.go b/server/monitor.go index 1fddcd84..53c562ac 100644 --- a/server/monitor.go +++ b/server/monitor.go @@ -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) +} + +// 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. diff --git a/server/monitor_test.go b/server/monitor_test.go index 521df043..a8d52e4e 100644 --- a/server/monitor_test.go +++ b/server/monitor_test.go @@ -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) + 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") +}