From 72380f27e6d91b5439a83dab3549edf14ed29fe8 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sun, 13 Sep 2026 14:11:44 +1000 Subject: [PATCH] Serve the owner's own answer on the per-certname routes Unanimity is the right rule for a fan-out of peers, but the per-certname routes are not one: a backend that does not hold the certname answers 404 to say so, not to disagree, so requiring it to agree turned the owner's real 500 into a 502 that described neither backend. - Add askOrder, which says whether a set of backends was asked as peers or owner-first, and resolve each round's replies under its own rule. - Serve the first backend that answered on owner-routed paths, so an unreachable owner still falls back rather than collapsing to 502. - Keep unanimity for the merged, meta, metrics and pass-through routes. - Cover the owner routes: owner errors against a non-owner 404, both erroring differently, an unreachable owner, and a non-owner error behind the owner's 200. - Record what clientRefusal's 4xx exemption assumes about client certs. --- README.md | 10 +++++- certname.go | 6 +++- certname_test.go | 91 ++++++++++++++++++++++++++++++++++++++++++++++++ meta.go | 2 +- server.go | 20 ++++++----- upstream.go | 74 ++++++++++++++++++++++++++++++++++----- 6 files changed, 182 insertions(+), 21 deletions(-) diff --git a/README.md b/README.md index afb5017..daca7db 100644 --- a/README.md +++ b/README.md @@ -34,7 +34,7 @@ not PQL) is forwarded verbatim. | `GET /pdb/query/v4/event-counts` | Fan out to all and **sum** each subject's counts into one row per subject. | | `GET /pdb/query/v4/aggregate-event-counts` | Fan out to all and **sum** the summary object's counts. | | `GET /pdb/query/v4/reports//{events,logs,metrics}` | Ask every backend; serve the answer from whichever backend actually holds that report. `404` when none does. | -| `GET /pdb/query/v4/{nodes,factsets,catalogs}/[/...]` | Ask the backend that owns that `certname` — the same owner the `/facts` merge attributes records to — and serve its reply verbatim. The remaining backends are tried after it, so a node only one backend holds is still served, and openvoxdb's own `404` body is replayed when none holds it. | +| `GET /pdb/query/v4/{nodes,factsets,catalogs}/[/...]` | Ask the backend that owns that `certname` — the same owner the `/facts` merge attributes records to — and serve its reply verbatim, success or error. The remaining backends are tried after it, so a node only one backend holds is still served, and openvoxdb's own `404` body is replayed when none holds it. | | `GET /pdb/query/v4/*` (any other) | No merge rule, so backends are tried in configured order and the first success is streamed back verbatim; if all reject it, the first upstream error response is replayed. | | `GET /pdb/meta/v1/version` | Fan out to all and report the **lowest** version any backend runs. | | `GET /pdb/meta/v1/server-time` | Fan out to all and serve the first reachable backend's clock. | @@ -61,6 +61,14 @@ describes: backends **disagreeing** on the status, or a backend that answered nothing at all. The rule is the same on `/pdb/query`, `/pdb/meta` and `/metrics`, so no route answers a failure differently from any other. +Unanimity is asked for only where the backends are peers answering the same +question. The per-certname routes are not: one backend **owns** the node and the +others are fallbacks, so the owner's reply is the answer whatever they said — a +backend that does not hold the certname answers `404` to say the node is not its +own, not to disagree about the request. When the owner answers nothing at all the +first fallback that did answer stands in, so an unreachable owner does not take +the node with it, and `502` is left for a request no backend answered. + A unanimous `4xx` blames the request, so it is not counted as a partial round on `/healthz` and nothing about it is cached. A unanimous `5xx` is the backends reporting their own fault, so it is replayed just as faithfully but still counts diff --git a/certname.go b/certname.go index e369511..31ec2ac 100644 --- a/certname.go +++ b/certname.go @@ -50,8 +50,12 @@ func isCertnameRoute(path string) bool { return certnameFor(path) != "" } // which is the record array the merges are built on. The remaining backends are // tried after it, so a node only one backend holds is still served and a // certname no backend holds still answers with openvoxdb's own 404 body. +// +// askOwnerFirst is what makes the owner's reply the answer even when it is an +// error. A backend that does not hold the certname 404s to say so, so it has no +// opinion to weigh against the owner's. func (s *Server) serveByOwner(w http.ResponseWriter, r *http.Request) { - s.proxyOrdered(w, r, s.ownerFirst(r.Context(), certnameFor(r.URL.Path))) + s.proxyOrdered(w, r, s.ownerFirst(r.Context(), certnameFor(r.URL.Path)), askOwnerFirst) } // ownerFirst puts the backend holding a certname's newest report ahead of the diff --git a/certname_test.go b/certname_test.go index 576b213..a4249fc 100644 --- a/certname_test.go +++ b/certname_test.go @@ -55,6 +55,13 @@ type voxBackend struct { // nodes maps a certname it holds to that node's report_timestamp. nodes map[string]string fail bool + // perNodeStatus and perNodeBody answer the per-certname routes with a fixed + // reply, leaving /nodes alone so the freshness map still resolves an owner. + perNodeStatus int + perNodeBody string + // perNodeDead aborts the per-certname routes without a status, standing in + // for a backend that is unreachable only for that request. + perNodeDead bool mu sync.Mutex gotParams map[string]url.Values @@ -73,6 +80,17 @@ func newVoxBackend(t *testing.T, name string, nodes map[string]string) *voxBacke } w.Header().Set("Content-Type", "application/json;charset=utf-8") + if isCertnameRoute(r.URL.Path) { + if vb.perNodeDead { + panic(http.ErrAbortHandler) + } + if vb.perNodeStatus != 0 { + w.WriteHeader(vb.perNodeStatus) + _, _ = io.WriteString(w, vb.perNodeBody) + return + } + } + if certname, ok := strings.CutPrefix(r.URL.Path, factsetsPath+"/"); ok { ts, held := vb.nodes[certname] if !held { @@ -342,3 +360,76 @@ func TestCertnameRoutes_ClaimedByTheRouteTable(t *testing.T) { } } } + +// The owner holds the node, so its answer is the answer: a non-owner's 404 only +// says the node is not its own, and demanding it agree turns the owner's real +// error into a 502 that describes neither backend. +func TestCertnameRoutes_OwnerAnswerWins(t *testing.T) { + const certname = "h1.example.net" + for _, tc := range []struct { + name string + // setUp configures the owner (a, which reported later) and the backend + // that does not hold the node (b). + setUp func(a, b *voxBackend) + wantCode int + wantBody string + }{ + { + name: "owner errors while the non-owner 404s", + setUp: func(a, b *voxBackend) { a.perNodeStatus, a.perNodeBody = http.StatusInternalServerError, "boom" }, + wantCode: http.StatusInternalServerError, + wantBody: "boom", + }, + { + name: "owner and non-owner error differently", + setUp: func(a, b *voxBackend) { + a.perNodeStatus, a.perNodeBody = http.StatusInternalServerError, "boom" + b.perNodeStatus, b.perNodeBody = http.StatusServiceUnavailable, "busy" + }, + wantCode: http.StatusInternalServerError, + wantBody: "boom", + }, + { + // The fallback chain is there so an unreachable owner does not take the + // node with it; a backend that did answer explains more than a 502. + name: "owner unreachable, fallback answers", + setUp: func(a, b *voxBackend) { + a.perNodeDead = true + b.perNodeStatus, b.perNodeBody = http.StatusServiceUnavailable, "busy" + }, + wantCode: http.StatusServiceUnavailable, + wantBody: "busy", + }, + { + name: "owner succeeds while the non-owner errors", + setUp: func(a, b *voxBackend) { + b.perNodeStatus, b.perNodeBody = http.StatusInternalServerError, "boom" + }, + wantCode: http.StatusOK, + wantBody: `"a"`, + }, + { + // Nothing answered at all, so there is no upstream reply to replay. + name: "every backend unreachable", + setUp: func(a, b *voxBackend) { a.perNodeDead, b.perNodeDead = true, true }, + wantCode: http.StatusBadGateway, + }, + } { + t.Run(tc.name, func(t *testing.T) { + a := newVoxBackend(t, "a", map[string]string{certname: "2026-07-20T00:00:00Z"}) + b := newVoxBackend(t, "b", map[string]string{}) + tc.setUp(a, b) + srv := newTestServer(testConfig(a.srv.URL, b.srv.URL, mergeFreshness)) + + for _, rt := range perNodePaths { + rec := doGet(t, srv.Handler(), rt.path(certname), "") + if rec.Code != tc.wantCode { + t.Fatalf("%s: status = %d, want %d: %s", rt.name, rec.Code, tc.wantCode, rec.Body.String()) + } + if tc.wantBody != "" && !strings.Contains(rec.Body.String(), tc.wantBody) { + t.Errorf("%s: body = %q, want it to contain %q", rt.name, rec.Body.String(), tc.wantBody) + } + } + }) + } +} diff --git a/meta.go b/meta.go index 5621d12..b44d04e 100644 --- a/meta.go +++ b/meta.go @@ -86,7 +86,7 @@ func (s *Server) aliveRaw(w http.ResponseWriter, results []rawResult, path strin alive = append(alive, res) } if len(alive) == 0 { - s.writeUpstreamError(w, upstreamOutcome(rawUpstreamErrors(results))) + s.writeUpstreamError(w, peerOutcome(rawUpstreamErrors(results))) return nil, false } return alive, true diff --git a/server.go b/server.go index 60008dd..68b4e47 100644 --- a/server.go +++ b/server.go @@ -535,7 +535,7 @@ func (s *Server) serveFirstHolder(w http.ResponseWriter, r *http.Request) { if len(alive) == 0 { // Unanimity is the whole answer here too: every backend saying 404 means // nobody holds the report, while one silent backend leaves that unknown. - s.writeUpstreamError(w, upstreamOutcome(backendUpstreamErrors(results))) + s.writeUpstreamError(w, peerOutcome(backendUpstreamErrors(results))) return } for _, res := range alive { @@ -922,19 +922,21 @@ func (s *Server) queryBackend(ctx context.Context, b Backend, path string, param return recs, total, err } -// The record shape is unknown, so a union would be guesswork: the first 2xx wins and the first error response is replayed when none succeeds. +// The record shape is unknown, so a union would be guesswork: the first 2xx wins, and every backend was asked the same question, so only a reply they all gave is replayed. func (s *Server) proxyUnmerged(w http.ResponseWriter, r *http.Request) { - s.proxyOrdered(w, r, s.cfg.Backends) + s.proxyOrdered(w, r, s.cfg.Backends, askPeers) } -// proxyOrdered asks backends in the given order, which is what decides the answer when more than one of them holds the path. -func (s *Server) proxyOrdered(w http.ResponseWriter, r *http.Request, backends []Backend) { - refusals := make([]*upstreamError, 0, len(backends)) +// proxyOrdered asks backends in the given order, which is what decides the +// answer when more than one of them holds the path. order says why they are in +// that order, which is what decides whose reply is served when none answers 2xx. +func (s *Server) proxyOrdered(w http.ResponseWriter, r *http.Request, backends []Backend, order askOrder) { + replies := make([]*upstreamError, 0, len(backends)) for _, b := range backends { resp, err := s.passThrough(r, b) if err != nil { s.log.Printf("warning: backend %q pass-through failed for %s: %v", b.Name, r.URL.Path, err) - refusals = append(refusals, nil) + replies = append(replies, nil) continue } if resp.StatusCode >= 200 && resp.StatusCode < 300 { @@ -946,9 +948,9 @@ func (s *Server) proxyOrdered(w http.ResponseWriter, r *http.Request, backends [ } body, _ := io.ReadAll(resp.Body) _ = resp.Body.Close() - refusals = append(refusals, newUpstreamError(resp.StatusCode, resp.Header.Get("Content-Type"), body)) + replies = append(replies, newUpstreamError(resp.StatusCode, resp.Header.Get("Content-Type"), body)) } - s.writeUpstreamError(w, upstreamOutcome(refusals)) + s.writeUpstreamError(w, order.outcome(replies)) } func (s *Server) passThrough(r *http.Request, b Backend) (*http.Response, error) { diff --git a/upstream.go b/upstream.go index b8b36a2..16a4afe 100644 --- a/upstream.go +++ b/upstream.go @@ -82,17 +82,66 @@ func unanimousUpstreamError(errs []*upstreamError) *upstreamError { return first } -// upstreamOutcome is the error to answer a fan-out with when it produced -// nothing: the backends' own unanimous reply where there is one, and pdbmux's -// gateway error otherwise. Every handler ends here, so no route answers a -// failure differently from any other. -func upstreamOutcome(errs []*upstreamError) error { +// askOrder says why a set of backends was asked, which is what decides whose +// reply becomes the client's answer when none of them answered 2xx. The two +// cases are not settings on one rule, they are different questions, and reading +// a reply under the wrong one is how a real answer turns into a 502. +type askOrder int + +const ( + // askPeers: every backend was asked the same question and any of them could + // have answered it, so each reply is an opinion about that question. Only a + // reply they all gave is the estate's answer; anything else leaves a backend + // whose reply is evidence about the backend, which is what 502 reports. + askPeers askOrder = iota + // askOwnerFirst: the path names one entity, the backend holding it is asked + // first and the rest only as fallbacks in case it does not answer. A backend + // that does not hold the entity replies 404 to say exactly that, which is not + // a dissent from the holder's reply, so requiring the two to agree asks a + // question nobody was posed. The first backend that answered is the one that + // knows. + askOwnerFirst +) + +// outcome turns a round's replies — one entry per backend in the order they were +// asked, nil where a backend produced no HTTP status at all — into the error to +// answer with. +func (o askOrder) outcome(replies []*upstreamError) error { + if o == askOwnerFirst { + return ownerOutcome(replies) + } + return peerOutcome(replies) +} + +// peerOutcome is the error to answer a fan-out of peers with when it produced +// nothing: their unanimous reply where there is one, and pdbmux's gateway error +// otherwise. +func peerOutcome(errs []*upstreamError) error { if ue := unanimousUpstreamError(errs); ue != nil { return ue } return errAllBackendsFailed } +// ownerOutcome is the error to answer an owner-routed path with: the reply of +// the first backend that answered. The owner is asked first, so that is the +// owner's own reply — it holds the entity, so its 500 is the truth about this +// request whatever a backend that does not hold the entity said. +// +// An owner that produced no status at all leaves a nil entry and the next +// backend's reply stands instead. The fallback chain exists so a node whose +// owner is unreachable is still served, and a backend that did answer explains +// more than a 502 that describes neither. Only a round where nothing answered is +// pdbmux's own gateway error. +func ownerOutcome(replies []*upstreamError) error { + for _, ue := range replies { + if ue != nil && replayableStatus(ue.status) { + return ue + } + } + return errAllBackendsFailed +} + // backendUpstreamErrors reduces a merged fan-out's results to one entry per // backend, nil where the backend answered or failed without a status. func backendUpstreamErrors(results []backendResult) []*upstreamError { @@ -111,14 +160,21 @@ func backendUpstreamErrors(results []backendResult) []*upstreamError { // which is why they are neither counted as degraded service nor answered from // the cache; a unanimous 5xx is replayed just the same but is the backends // reporting their own fault. +// +// Every 4xx openvoxdb answers a fan-out with is about the request, including the +// 403 on /metrics/v2/list, which is that backend's own policy rather than an +// authentication failure. That holds only while pdbmux presents no client +// certificate: authenticate to backends and a rejected certificate becomes a +// 403 on every route at once, which this would read as a healthy estate refusing +// a bad query and hide a total outage behind a replayed 403. func clientRefusal(err error) bool { var ue *upstreamError return errors.As(err, &ue) && ue != nil && ue.status < 500 } -// writeUpstreamError answers a fan-out that produced no records. A unanimous -// upstream error is replayed with the backend's own status, content type and -// explanation; anything else is reported as a gateway failure. +// writeUpstreamError answers a round that produced no records. An upstream +// error the askOrder resolved to is replayed with the backend's own status, +// content type and explanation; anything else is reported as a gateway failure. func (s *Server) writeUpstreamError(w http.ResponseWriter, err error) { var ue *upstreamError if !errors.As(err, &ue) { @@ -129,7 +185,7 @@ func (s *Server) writeUpstreamError(w http.ResponseWriter, err error) { if len(strings.TrimSpace(string(body))) == 0 { setContentType(w, "text/plain; charset=utf-8") w.WriteHeader(ue.status) - _, _ = fmt.Fprintf(w, "every backend answered %d %s\n", ue.status, http.StatusText(ue.status)) + _, _ = fmt.Fprintf(w, "upstream answered %d %s\n", ue.status, http.StatusText(ue.status)) return } setContentType(w, ue.contentType)