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.
2026-09-13 14:11:44 +10:00
parent 121bfacc2f
commit 72380f27e6
6 changed files with 182 additions and 21 deletions
@@ -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/<hash>/{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}/<certname>[/...]` | 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}/<certname>[/...]` | 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
@@ -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
@@ -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)
+				}
+			}
+		})
+	}
+}
@@ -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
@@ -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) {
@@ -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)