Address reviewer comments.

Will squash after review.

Signed-off-by: Siyuan Zhang <[email protected]>

server/etcdserver/api/etcdhttp/health.go

@@ -98,31 +98,6 @@ func NewHealthHandler(lg *zap.Logger, hfunc func(excludedAlarms StringSet, Seria
 	}
 }
 
-// newHealthHandler generates a http HandlerFunc for a health check function hfunc.
-func newHealthHandler(path string, lg *zap.Logger, hfunc func(*http.Request) (Health, error)) http.HandlerFunc {
-	return func(w http.ResponseWriter, r *http.Request) {
-		if r.Method != http.MethodGet {
-			w.Header().Set("Allow", http.MethodGet)
-			http.Error(w, "Method Not Allowed", http.StatusMethodNotAllowed)
-			lg.Warn("/health error", zap.Int("status-code", http.StatusMethodNotAllowed))
-			return
-		}
-		h, err := hfunc(r)
-		if err != nil {
-			panic(err)
-		}
-		d, _ := json.Marshal(h)
-		if h.Health != "true" {
-			http.Error(w, string(d), http.StatusServiceUnavailable)
-			lg.Warn("Health check error", zap.String("path", path), zap.String("output", string(d)), zap.Int("status-code", http.StatusServiceUnavailable))
-			return
-		}
-		w.WriteHeader(http.StatusOK)
-		w.Write(d)
-		lg.Debug("Health check OK", zap.String("path", path), zap.String("output", string(d)), zap.Int("status-code", http.StatusOK))
-	}
-}
-
 var (
 	healthSuccess = prometheus.NewCounter(prometheus.CounterOpts{
 		Namespace: "etcd",
@@ -261,17 +236,17 @@ func (reg *CheckRegistry) InstallHttpEndpoints(lg *zap.Logger, mux *http.ServeMu
 		subpath := path.Join(reg.path, checkName)
 		check := checkName
 		mux.Handle(subpath, newHealthHandler(subpath, lg,
-			func(r *http.Request) (Health, error) { return reg.runHealthChecks(r.Context(), check) }))
+			func(r *http.Request) Health { return reg.runHealthChecks(r.Context(), check) }))
 	}
 }
 
-func (reg *CheckRegistry) runHealthChecks(ctx context.Context, checkNames ...string) (Health, error) {
+func (reg *CheckRegistry) runHealthChecks(ctx context.Context, checkNames ...string) Health {
 	h := Health{Health: "true"}
 	var individualCheckOutput bytes.Buffer
 	for _, checkName := range checkNames {
 		check, found := reg.checks[checkName]
 		if !found {
-			return Health{Health: "false"}, fmt.Errorf("Health check: %s not registered", checkName)
+			panic(fmt.Errorf("Health check: %s not registered", checkName))
 		}
 		if err := check(ctx); err != nil {
 			fmt.Fprintf(&individualCheckOutput, "[-]%s failed: %v\n", checkName, err)
@@ -281,12 +256,12 @@ func (reg *CheckRegistry) runHealthChecks(ctx context.Context, checkNames ...str
 		}
 	}
 	h.Reason = individualCheckOutput.String()
-	return h, nil
+	return h
 }
 
 // installRootHttpEndpoint installs the http handler for the root path.
 func (reg *CheckRegistry) installRootHttpEndpoint(lg *zap.Logger, mux *http.ServeMux, path string, checks ...string) {
-	hfunc := func(r *http.Request) (Health, error) {
+	hfunc := func(r *http.Request) Health {
 		// extracts the health check names to be excludeList from the query param
 		excluded := getQuerySet(r, "exclude")
 
@@ -296,6 +271,34 @@ func (reg *CheckRegistry) installRootHttpEndpoint(lg *zap.Logger, mux *http.Serv
 	mux.Handle(path, newHealthHandler(path, lg, hfunc))
 }
 
+// newHealthHandler generates a http HandlerFunc for a health check function hfunc.
+func newHealthHandler(path string, lg *zap.Logger, hfunc func(*http.Request) Health) http.HandlerFunc {
+	return func(w http.ResponseWriter, r *http.Request) {
+		if r.Method != http.MethodGet {
+			w.Header().Set("Allow", http.MethodGet)
+			http.Error(w, "Method Not Allowed", http.StatusMethodNotAllowed)
+			lg.Warn("Health request error", zap.String("path", path), zap.Int("status-code", http.StatusMethodNotAllowed))
+			return
+		}
+		h := hfunc(r)
+		// Always returns detailed reason for failed checks.
+		if h.Health != "true" {
+			http.Error(w, h.Reason, http.StatusServiceUnavailable)
+			lg.Error("Health check error", zap.String("path", path), zap.String("reason", h.Reason), zap.Int("status-code", http.StatusServiceUnavailable))
+			return
+		}
+		lg.Debug("Health check OK", zap.String("path", path), zap.String("reason", h.Reason), zap.Int("status-code", http.StatusOK))
+		w.Header().Set("Content-Type", "text/plain; charset=utf-8")
+		w.Header().Set("X-Content-Type-Options", "nosniff")
+		// Skips detailed reason for non-verbose requests.
+		if _, found := r.URL.Query()["verbose"]; !found {
+			fmt.Fprint(w, "ok\n")
+			return
+		}
+		fmt.Fprint(w, h.Reason)
+	}
+}
+
 func filterCheckList(lg *zap.Logger, checks []string, excluded StringSet) []string {
 	filteredList := []string{}
 	for _, chk := range checks {

server/etcdserver/api/etcdhttp/health_test.go

@@ -16,7 +16,6 @@ package etcdhttp
 
 import (
 	"context"
-	"encoding/json"
 	"fmt"
 	"io"
 	"net/http"
@@ -145,14 +144,12 @@ func TestHealthHandler(t *testing.T) {
 			healthCheckURL:   "/health",
 			expectStatusCode: http.StatusServiceUnavailable,
 			missingLeader:    true,
-			notInResult:      []string{"serializable_read"},
 		},
 		{
 			name:             "Healthy if no leader and serializable=true",
 			healthCheckURL:   "/health?serializable=true",
 			expectStatusCode: http.StatusOK,
 			missingLeader:    true,
-			notInResult:      []string{"linearizable_read", "leader"},
 		},
 	}
 
@@ -185,6 +182,7 @@ func TestHttpSubPath(t *testing.T) {
 			apiError:         fmt.Errorf("Unexpected error"),
 			healthCheckURL:   "/readyz/serializable_read",
 			expectStatusCode: http.StatusServiceUnavailable,
+			notInResult:      []string{"data_corruption"},
 		},
 	}
 	for _, tt := range tests {
@@ -266,8 +264,9 @@ func TestSerializableReadCheck(t *testing.T) {
 	tests := []healthTestCase{
 		{
 			name:             "Alive normal",
-			healthCheckURL:   "/livez",
+			healthCheckURL:   "/livez?verbose",
 			expectStatusCode: http.StatusOK,
+			inResult:         []string{"[+]serializable_read ok"},
 		},
 		{
 			name:             "Not alive if range api is not available",
@@ -309,43 +308,22 @@ func checkHttpResponse(t *testing.T, ts *httptest.Server, url string, expectStat
 	if res.StatusCode != expectStatusCode {
 		t.Errorf("want statusCode %d but got %d", expectStatusCode, res.StatusCode)
 	}
-	if expectStatusCode == http.StatusBadRequest {
-		return
-	}
 	defer res.Body.Close()
-	health, err := parseHealthOutput(res.Body)
+	b, err := io.ReadAll(res.Body)
 	if err != nil {
-		t.Fatalf("fail parse health check output %v", err)
-	}
-	expectHealth := "false"
-	if expectStatusCode == http.StatusOK {
-		expectHealth = "true"
-	}
-	if health.Health != expectHealth {
-		t.Errorf("want health %s but got %v", expectHealth, health)
+		t.Fatalf("Failed to read response for %s", url)
 	}
+	result := string(b)
 	for _, substr := range inResult {
-		if !strings.Contains(health.Reason, substr) {
-			t.Errorf("Could not find substring : %s, in response: %s", substr, health.Reason)
+		if !strings.Contains(result, substr) {
+			t.Errorf("Could not find substring : %s, in response: %s", substr, result)
 			return
 		}
 	}
 	for _, substr := range notInResult {
-		if strings.Contains(health.Reason, substr) {
-			t.Errorf("Do not expect substring : %s, in response: %s", substr, health.Reason)
+		if strings.Contains(result, substr) {
+			t.Errorf("Do not expect substring : %s, in response: %s", substr, result)
 			return
 		}
 	}
 }
-
-func parseHealthOutput(body io.Reader) (Health, error) {
-	obj := Health{}
-	d, derr := io.ReadAll(body)
-	if derr != nil {
-		return obj, derr
-	}
-	if err := json.Unmarshal(d, &obj); err != nil {
-		return obj, err
-	}
-	return obj, nil
-}