diff --git a/README.md b/README.md index 4538f24..03ad4ca 100644 --- a/README.md +++ b/README.md @@ -228,19 +228,36 @@ not answering. or `unknown` counts as a failure. A body that is not in that shape is judged on its status code alone, so pointing `health_probe_path` at some other endpoint still works. +- **A refused probe is not a sick backend.** Only evidence about the *backend* + takes it out of the pool: a transport failure (connection refused, DNS, TLS, + timeout) or a `5xx` — `503` included, since trapperkeeper answers `503` exactly + when its services are not nominal, so it stays a real failure. A reply that + refuses the *probe request* is evidence about the probe instead: every `4xx` is + the backend answering that our request is the problem (`404`/`410` the path is + not there, `405` it does not take a `GET`, `401`/`403` we are not allowed to + ask, `429` we asked too often), and `501` says it does not implement the + endpoint. A backend answering that way is **left in service** — still queried, + still contributing records — and reported as `probe_unsupported` rather than + `healthy`, so an operator can tell "verified healthy" from "not actually being + checked". A probe path that is wrong for one backend alone can therefore never + strand a working backend. The misconfiguration is logged once per transition, + naming the backend, the probe path and the status. - **Thresholds** — a healthy backend goes down after `health_probe_failures` (default 3) **consecutive** failures; a down one comes back after - `health_probe_successes` (default 2) consecutive successes. One blip cannot - flap a backend out, and one lucky reply cannot flap it back in. A down backend - keeps being probed, so recovery is automatic. + `health_probe_successes` (default 2) consecutive successes, and the same + thresholds gate `probe_unsupported` in and out. One blip cannot flap a backend + out, and one lucky reply cannot flap it back in. A run only counts while the + replies keep their kind, so a stretch of `404`s and a stretch of `503`s never + add up to one threshold. A down backend keeps being probed, so recovery is + automatic. - **Fails open** — if the prober has marked **every** backend down, `pdbmux` queries them all anyway. A wrong `health_probe_path`, a broken prober or a partition that only the prober sees can therefore never black-hole traffic; the worst case is today's behaviour. - **Serves immediately** — the listener never waits for a first probe round, and a backend nobody has probed yet counts as healthy, so a restart drops nothing. -- **Quiet** — only *transitions* (up→down, down→up) are logged, never individual - probes. +- **Quiet** — only *transitions* (up→down, down→up, in and out of + `probe_unsupported`) are logged, never individual probes. - **Partial responses stay partial.** Health state changes which backends are asked, never what a merged answer means: a response built from a subset is still served, as before. Every merged response carries `X-Backends: @@ -248,7 +265,8 @@ not answering. a client can tell a full answer from a partial one. On a cache hit the header describes the stored body, not the current backend count. - **`/healthz`** gives each backend a `state` (`healthy`, `unhealthy`, - `unprobed`, or `unmonitored` when probing is off), `consecutive_failures`, + `probe_unsupported`, `unprobed`, or `unmonitored` when probing is off), + `consecutive_failures`, `consecutive_successes`, `last_probe` and `last_error`, alongside the `reachable` check `/healthz` runs itself — which always asks **every** backend, so a backend queries are skipping is still reported. A `query` object diff --git a/health.go b/health.go index c37c1d0..23c1268 100644 --- a/health.go +++ b/health.go @@ -3,6 +3,7 @@ package main import ( "context" "encoding/json" + "errors" "fmt" "io" "log" @@ -19,36 +20,53 @@ const probeBodyLimit = 1 << 20 const runningState = "running" const ( - stateHealthy = "healthy" - stateUnhealthy = "unhealthy" - stateUnprobed = "unprobed" - stateUnmonitored = "unmonitored" + stateHealthy = "healthy" + stateUnhealthy = "unhealthy" + stateUnprobed = "unprobed" + stateUnmonitored = "unmonitored" + stateProbeUnsupported = "probe_unsupported" +) + +// probeOutcome is what one probe reply said. A rejected probe is deliberately +// neither a success nor a failure: it is evidence about the probe request, not +// about the backend behind it. +type probeOutcome int + +const ( + outcomeOK probeOutcome = iota + outcomeFailure + outcomeRejected ) type probeState struct { - healthy bool - probed bool - failures int - successes int - lastProbe time.Time - lastErr string + healthy bool + unsupported bool + probed bool + outcome probeOutcome + failures int + successes int + lastProbe time.Time + lastErr string } // backendHealth is a copy of one backend's probe state, safe to read outside the // prober's lock. type backendHealth struct { - Healthy bool - Probed bool - Failures int - Successes int - LastProbe time.Time - LastErr string + Healthy bool + Unsupported bool + Probed bool + Failures int + Successes int + LastProbe time.Time + LastErr string } func (h backendHealth) stateName() string { switch { case !h.Probed: return stateUnprobed + case h.Unsupported: + return stateProbeUnsupported case h.Healthy: return stateHealthy default: @@ -56,6 +74,37 @@ func (h backendHealth) stateName() string { } } +// probeRejectedError marks a reply that refused the probe request itself rather +// than reporting the backend unwell. +type probeRejectedError struct { + status int + path string +} + +func (e *probeRejectedError) Error() string { + return fmt.Sprintf("HTTP %d: probe path %q is not usable on this backend", e.status, e.path) +} + +// probeRejects reports whether a status code refuses the probe request rather +// than reporting the service unhealthy. Every 4xx is the backend answering that +// our request is the problem, and 501 says it does not implement the endpoint. +// Other 5xx stay real evidence: trapperkeeper answers 503 exactly when its +// services are not nominal. +func probeRejects(code int) bool { + return (code >= 400 && code < 500) || code == http.StatusNotImplemented +} + +func classifyProbe(err error) probeOutcome { + switch { + case err == nil: + return outcomeOK + case errors.As(err, new(*probeRejectedError)): + return outcomeRejected + default: + return outcomeFailure + } +} + // prober polls each backend's health endpoint on an interval and tracks whether // it is currently answering. A nil *prober means probing is disabled and every // method is a no-op reporting every backend healthy. @@ -163,12 +212,13 @@ func (p *prober) snapshot() map[string]backendHealth { out := make(map[string]backendHealth, len(p.states)) for name, st := range p.states { out[name] = backendHealth{ - Healthy: st.healthy, - Probed: st.probed, - Failures: st.failures, - Successes: st.successes, - LastProbe: st.lastProbe, - LastErr: st.lastErr, + Healthy: st.healthy, + Unsupported: st.unsupported, + Probed: st.probed, + Failures: st.failures, + Successes: st.successes, + LastProbe: st.lastProbe, + LastErr: st.lastErr, } } return out @@ -193,24 +243,49 @@ func (p *prober) record(name string, err error) { p.mu.Unlock() return } + outcome := classifyProbe(err) + // A run only counts while the outcome keeps its kind, so a rejected-probe run + // and a real-failure run never add up to one threshold. + if outcome != st.outcome { + st.failures, st.successes = 0, 0 + } + st.outcome = outcome st.probed = true st.lastProbe = p.now() + var transition string - if err != nil { - st.lastErr = err.Error() - st.successes = 0 - st.failures++ - if st.healthy && st.failures >= p.failures { - st.healthy = false - transition = fmt.Sprintf("warning: backend %q is down after %d consecutive probe failures: %v", name, st.failures, err) - } - } else { + switch outcome { + case outcomeOK: st.lastErr = "" - st.failures = 0 st.successes++ - if !st.healthy && st.successes >= p.successes { - st.healthy = true - transition = fmt.Sprintf("info: backend %q is up after %d consecutive probe successes", name, st.successes) + if st.successes >= p.successes { + switch { + case st.unsupported: + st.unsupported = false + transition = fmt.Sprintf("info: backend %q health probe %q is usable again", name, p.path) + case !st.healthy: + st.healthy = true + transition = fmt.Sprintf("info: backend %q is up after %d consecutive probe successes", name, st.successes) + } + } + case outcomeRejected: + st.lastErr = err.Error() + st.failures++ + if !st.unsupported && st.failures >= p.failures { + // The backend answered and refused the probe request itself, so this says + // nothing about its health: it keeps serving queries, unverified. + st.unsupported, st.healthy = true, true + transition = fmt.Sprintf("warning: backend %q rejected the health probe %d times: %v; leaving it in service but unverified, fix health_probe_path or its authorization", name, st.failures, err) + } + default: + st.lastErr = err.Error() + st.failures++ + if st.failures >= p.failures { + st.unsupported = false + if st.healthy { + st.healthy = false + transition = fmt.Sprintf("warning: backend %q is down after %d consecutive probe failures: %v", name, st.failures, err) + } } } p.mu.Unlock() @@ -236,6 +311,9 @@ func (p *prober) probe(ctx context.Context, b Backend) error { if err != nil { return err } + if probeRejects(resp.StatusCode) { + return &probeRejectedError{status: resp.StatusCode, path: p.path} + } if resp.StatusCode != http.StatusOK { return fmt.Errorf("HTTP %d: %s", resp.StatusCode, strings.TrimSpace(string(body))) } diff --git a/health_test.go b/health_test.go index f1c820e..7b0c18d 100644 --- a/health_test.go +++ b/health_test.go @@ -86,6 +86,19 @@ func markDown(t *testing.T, p *prober, name string) { } } +func markUnsupported(t *testing.T, p *prober, name string) { + t.Helper() + for i := 0; i < p.failures; i++ { + p.record(name, &probeRejectedError{status: http.StatusNotFound, path: p.path}) + } + if !p.healthy(name) { + t.Fatalf("backend %q was excluded by a rejected probe", name) + } + if state := p.snapshot()[name].stateName(); state != stateProbeUnsupported { + t.Fatalf("state = %q, want %q", state, stateProbeUnsupported) + } +} + func TestProber_FailureThresholdDebouncesABlip(t *testing.T) { cfg := healthConfig("http://a.invalid", "http://b.invalid") cfg.HealthProbeFailures = 3 @@ -488,6 +501,215 @@ func TestBackendsHeader_CountsContributors(t *testing.T) { } } +// A reply that refuses the probe request says nothing about the backend, so the +// backend must keep serving queries instead of being stranded out of the pool. +func TestProber_RejectedProbeKeepsBackendInService(t *testing.T) { + codes := []int{ + http.StatusBadRequest, http.StatusUnauthorized, http.StatusForbidden, + http.StatusNotFound, http.StatusMethodNotAllowed, http.StatusGone, + http.StatusTooManyRequests, http.StatusNotImplemented, + } + for _, code := range codes { + sb := newStatusBackend(t) + sb.reply(code, "nope") + p := newTestProber(t, healthConfig(sb.srv.URL, "http://b.invalid")) + + err := p.probe(t.Context(), Backend{Name: "a", URL: sb.srv.URL}) + if classifyProbe(err) != outcomeRejected { + t.Fatalf("HTTP %d classified as %v, want a rejected probe (err %v)", code, classifyProbe(err), err) + } + for i := 0; i < p.failures+3; i++ { + p.record("a", err) + if !p.healthy("a") { + t.Fatalf("HTTP %d excluded backend a after %d probes", code, i+1) + } + } + snap := p.snapshot()["a"] + if got := snap.stateName(); got != stateProbeUnsupported { + t.Errorf("HTTP %d state = %q, want %q", code, got, stateProbeUnsupported) + } + if !strings.Contains(snap.LastErr, defaultHealthProbePath) { + t.Errorf("HTTP %d last_error %q does not name the probe path", code, snap.LastErr) + } + } +} + +// 503 is trapperkeeper's answer when its services are not nominal, so it stays a +// real failure; so does anything that never got an HTTP reply at all. +func TestProber_ServerErrorsAndTransportFailuresStillMarkDown(t *testing.T) { + for _, code := range []int{http.StatusInternalServerError, http.StatusBadGateway, http.StatusServiceUnavailable, http.StatusGatewayTimeout} { + sb := newStatusBackend(t) + sb.reply(code, degradedStatusBody("error")) + p := newTestProber(t, healthConfig(sb.srv.URL, "http://b.invalid")) + + err := p.probe(t.Context(), Backend{Name: "a", URL: sb.srv.URL}) + if classifyProbe(err) != outcomeFailure { + t.Fatalf("HTTP %d classified as %v, want a real failure", code, classifyProbe(err)) + } + for i := 0; i < p.failures; i++ { + p.record("a", err) + } + if p.healthy("a") { + t.Errorf("HTTP %d left backend a in service", code) + } + if got := p.snapshot()["a"].stateName(); got != stateUnhealthy { + t.Errorf("HTTP %d state = %q, want %q", code, got, stateUnhealthy) + } + } + + dead := httptest.NewServer(http.NotFoundHandler()) + deadURL := dead.URL + dead.Close() + p := newTestProber(t, healthConfig(deadURL, "http://b.invalid")) + err := p.probe(t.Context(), Backend{Name: "a", URL: deadURL}) + if classifyProbe(err) != outcomeFailure { + t.Fatalf("a refused connection classified as %v, want a real failure (err %v)", classifyProbe(err), err) + } + for i := 0; i < p.failures; i++ { + p.record("a", err) + } + if p.healthy("a") { + t.Error("a refused connection left the backend in service") + } +} + +func TestProber_RejectedRunAndFailureRunDoNotAddUp(t *testing.T) { + p := newTestProber(t, healthConfig("http://a.invalid", "http://b.invalid")) + markUnsupported(t, p, "a") + for i := 0; i < 20; i++ { + p.record("a", &probeRejectedError{status: http.StatusNotFound, path: p.path}) + } + + // One real failure on top of a long rejected run must not trip the threshold. + p.record("a", errors.New("boom")) + if !p.healthy("a") { + t.Fatal("a single real failure after a rejected run marked the backend down") + } + for i := 1; i < p.failures; i++ { + p.record("a", errors.New("boom")) + } + if p.healthy("a") { + t.Fatal("a full run of real failures should mark the backend down") + } + if got := p.snapshot()["a"].stateName(); got != stateUnhealthy { + t.Errorf("state = %q, want %q", got, stateUnhealthy) + } +} + +func TestProber_RecoversFromProbeUnsupported(t *testing.T) { + p := newTestProber(t, healthConfig("http://a.invalid", "http://b.invalid")) + markUnsupported(t, p, "a") + + for i := 0; i < p.successes; i++ { + p.record("a", nil) + } + if !p.healthy("a") { + t.Fatal("backend left the pool while recovering") + } + if got := p.snapshot()["a"].stateName(); got != stateHealthy { + t.Fatalf("state = %q, want %q", got, stateHealthy) + } +} + +// A rejected probe re-admits a backend the prober had already marked down: the +// evidence that condemned it has been replaced by no evidence at all. +func TestProber_RejectedProbeReadmitsADownBackend(t *testing.T) { + p := newTestProber(t, healthConfig("http://a.invalid", "http://b.invalid")) + markDown(t, p, "a") + markUnsupported(t, p, "a") +} + +func TestProber_LogsProbeRejectionOncePerTransition(t *testing.T) { + var buf strings.Builder + p := newProber(healthConfig("http://a.invalid", "http://b.invalid"), log.New(&buf, "", 0)) + if p == nil { + t.Fatal("newProber returned nil for an enabled config") + } + + rejected := &probeRejectedError{status: http.StatusNotFound, path: p.path} + for i := 0; i < 50; i++ { + p.record("a", rejected) + } + if n := strings.Count(buf.String(), "rejected the health probe"); n != 1 { + t.Fatalf("logged the misconfiguration %d times, want once:\n%s", n, buf.String()) + } + if !strings.Contains(buf.String(), `backend "a"`) || !strings.Contains(buf.String(), defaultHealthProbePath) { + t.Errorf("log line names neither the backend nor the probe path: %s", buf.String()) + } + if !strings.Contains(buf.String(), "404") { + t.Errorf("log line does not name the status: %s", buf.String()) + } + + // Recovery logs its own single line, and a second rejected run logs again. + for i := 0; i < p.successes+5; i++ { + p.record("a", nil) + } + if n := strings.Count(buf.String(), "usable again"); n != 1 { + t.Fatalf("logged recovery %d times, want once:\n%s", n, buf.String()) + } + for i := 0; i < 50; i++ { + p.record("a", rejected) + } + if n := strings.Count(buf.String(), "rejected the health probe"); n != 2 { + t.Fatalf("second rejected run logged %d times in total, want 2:\n%s", n, buf.String()) + } +} + +// The regression this guards: a backend whose probe path is wrong for it alone +// still answers queries perfectly, and must keep contributing records. +func TestFanOut_ProbeUnsupportedBackendStillContributes(t *testing.T) { + a := newFakeBackend(t, `[`+node("h1", "2026-07-20T00:00:00Z")+`]`, `[]`) + b := newFakeBackend(t, `[`+node("h2", "2026-07-20T00:00:00Z")+`]`, `[]`) + srv := newTestServer(healthConfig(a.srv.URL, b.srv.URL)) + markUnsupported(t, srv.health, "b") + + rec := doGet(t, srv.Handler(), nodesPath, "") + if rec.Code != http.StatusOK { + t.Fatalf("status %d: %s", rec.Code, rec.Body.String()) + } + if _, asked := b.params(nodesPath); !asked { + t.Fatal("a backend with an unusable probe path was not queried") + } + body := rec.Body.String() + if !strings.Contains(body, "h1") || !strings.Contains(body, "h2") { + t.Fatalf("both backends should have contributed: %s", body) + } + if got := rec.Header().Get(backendsHeader); got != "2/2" { + t.Errorf("%s = %q, want 2/2", backendsHeader, got) + } +} + +func TestHealthz_ReportsProbeUnsupported(t *testing.T) { + a := newFakeBackend(t, `[]`, `[]`) + b := newFakeBackend(t, `[]`, `[]`) + srv := newTestServer(healthConfig(a.srv.URL, b.srv.URL)) + srv.health.record("a", nil) + markUnsupported(t, srv.health, "b") + + rec := doGet(t, srv.Handler(), "/healthz", "") + if rec.Code != http.StatusOK { + t.Fatalf("status %d: %s", rec.Code, rec.Body.String()) + } + var hr healthReport + if err := json.Unmarshal(rec.Body.Bytes(), &hr); err != nil { + t.Fatal(err) + } + if got := hr.Backends["a"].State; got != stateHealthy { + t.Errorf("a state = %q, want %q", got, stateHealthy) + } + // Distinct from both "verified healthy" and "excluded". + unsupported := hr.Backends["b"] + if unsupported.State != stateProbeUnsupported { + t.Errorf("b state = %q, want %q", unsupported.State, stateProbeUnsupported) + } + if unsupported.Reachable != "ok" { + t.Errorf("b reachable = %q, want ok", unsupported.Reachable) + } + if unsupported.LastError == "" || unsupported.LastProbe == "" { + t.Errorf("b is missing last_error/last_probe: %+v", unsupported) + } +} + func TestStatusBodyHealthy_IgnoresBodiesWithoutStates(t *testing.T) { for _, body := range []string{`{}`, `[]`, `null`, `{"svc":{}}`, `not json`} { if err := statusBodyHealthy([]byte(body)); err != nil { diff --git a/server.go b/server.go index 7b32a2f..c977525 100644 --- a/server.go +++ b/server.go @@ -676,7 +676,7 @@ type healthReport struct { // prober's running state for the same backend. type backendReport struct { Reachable string `json:"reachable"` // "ok" | error text - State string `json:"state"` // healthy | unhealthy | unprobed | unmonitored + State string `json:"state"` // healthy | unhealthy | probe_unsupported | unprobed | unmonitored Failures int `json:"consecutive_failures"` Successes int `json:"consecutive_successes"` LastProbe string `json:"last_probe,omitempty"`