From 148be4fe0f208dbc8b20c53fa74ac5cb29aa3c70 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sun, 6 Sep 2026 16:09:55 +1000 Subject: [PATCH] Synthesise the /facts/ drilldown /fact-names advertises the fact, so its drilldown must not be a dead link. - Serve the source fact's own path from the /facts merge that produces the records, so certname set, owner and environment match /facts. - Filter /facts// by the owning backend. - Keep the aggregate, query-gate and disabled paths answering as before. --- README.md | 33 ++++-- e2e_puppetboard_test.go | 55 +++++++--- e2e_query_test.go | 77 +++++++++++++ factroutes_test.go | 238 ++++++++++++++++++++++++++++++++-------- merge.go | 22 ++++ server.go | 73 ++++++++---- 6 files changed, 399 insertions(+), 99 deletions(-) diff --git a/README.md b/README.md index 1b2626e..bc2eb3c 100644 --- a/README.md +++ b/README.md @@ -26,7 +26,7 @@ not PQL) is forwarded verbatim. |---|---| | `GET /pdb/query/v4/nodes` | Fan out to all backends, dedupe by `certname`, keep the record with the newer `report_timestamp`, stamped with the winning backend's name (see provenance). An `extract`/`count` query is **summed** instead. | | `GET /pdb/query/v4/facts` | Fan out to all, and per `certname` keep **all** facts from the backend that owns that node (see merge semantics), plus a synthetic `pdbmux_source` fact naming it. | -| `GET /pdb/query/v4/facts/[/]` | Same fan-out and merge as `/facts`, and an `extract`/`count` query is **summed** the same way. The path segment is a `name` constraint, so no synthetic `pdbmux_source` record is added unless the path names it. | +| `GET /pdb/query/v4/facts/[/]` | Same fan-out and merge as `/facts`, and an `extract`/`count` query is **summed** the same way. The path segment is a `name` constraint, so no synthetic `pdbmux_source` record is added — except on the fact's own path, which is **synthesised** from the `/facts` merge (see provenance). | | `GET /pdb/query/v4/fact-names` | Fan out to all and serve the **union** of the flat name arrays, deduped and re-sorted, re-paged across backends, plus the `pdbmux_source` name while injection is on. `order_by` is only valid on `name`. | | `GET /pdb/query/v4/resources` | An `extract`/`count` query is fanned out and **summed**; any other query is an unmerged pass-through. | | `GET /pdb/query/v4/reports` | Fan out to all and serve the **union**, deduped by report `hash`, re-ordered and re-paged across backends. | @@ -71,7 +71,9 @@ paths add two more headers `pdbmux` sets itself, `X-Cache` and `Age` — see backend actually returned facts for it). - `/facts/` and `/facts//` are the same records with one more constraint applied upstream, so they take the same rule — and the same - aggregate branch, since a count row has no `certname` there either. + aggregate branch, since a count row has no `certname` there either. The + `pdbmux_source` path is the exception: no backend holds that name, so it is + synthesised from the `/facts` merge (see provenance). - **`/fact-names`** — a flat array of strings, not records: **union**, deduped by the name and re-sorted, ascending unless `order_by` says otherwise. `name` is the only column the entity projects, so an `order_by` on any other field is @@ -162,24 +164,31 @@ matters more. subquery narrows which *nodes* match, not which facts come back, so injection still happens; - the path is `/facts/` for any other fact. The path segment is the same - outer `name` constraint, so only `/facts/pdbmux_source` may carry the record; - `/facts//` never does, since the pinned value need not equal the - backend name the record holds; + outer `name` constraint, so only the fact's own path carries the record; - injection is turned off (see `source_fact_enabled`). +**The fact's own path is synthesised.** `/pdb/query/v4/fact-names` lists the name +while injection is on, so a client that discovers names there can click through +to it, and `/pdb/query/v4/facts/pdbmux_source` has to answer. No backend holds a +record of that name, so the route does not serve its own fan-out: it takes the +records from the `/facts` merge that produces them, which makes the `certname` +set, the owner and the `environment` identical to the ones an unfiltered `/facts` +response reports, and lets the request's own `query` narrow the result upstream. +That costs one `/facts` fan-out per cache miss — the widest fan-out `pdbmux` +makes — on a rare, user-initiated path. `/facts/pdbmux_source/` pins the +backend name, so it answers with the nodes that backend owns, and with `[]` for a +value naming no backend. An `extract`/`count` query still takes the summing +branch, and the gated query shapes above still answer `[]`, as does every form +while injection is off — with the name kept out of `/fact-names`, since nothing +then produces it. + **Not supported in v1: server-side filtering on the fact.** A query that selects it — `["=","name","pdbmux_source"]`, or an `extract` naming it — is forwarded to the backends like any other, and they return nothing, because the fact does not exist upstream. `pdbmux` does not evaluate the AST itself, so it cannot answer such a query correctly for every operator (`not`, `or`, subqueries) and does not pretend to for some. Read the fact from an unfiltered (or `certname`-filtered) -`/facts` response and filter client-side. `/pdb/query/v4/facts/pdbmux_source` is -the same story: the route is merged and injection is allowed there, but the -backends hold no record to attach it to, so it answers empty. -`/pdb/query/v4/fact-names` still lists the name while injection is on, because -every merged `/facts` response does carry records of it — leaving it out hides a -fact every node has from any client that discovers names there. Turning injection -off removes it, since nothing then produces it. +`/facts` response, from the path route above, and filter client-side. **Not covered:** `/factsets` and `/inventory`. Both carry facts, but `pdbmux` does not merge either today — they take the unmerged pass-through path, where diff --git a/e2e_puppetboard_test.go b/e2e_puppetboard_test.go index d168732..68b483f 100644 --- a/e2e_puppetboard_test.go +++ b/e2e_puppetboard_test.go @@ -153,6 +153,42 @@ func TestPuppetboardFactsOverview(t *testing.T) { t.Errorf("Puppetboard facts overview does not list %s", name) } } + + // A name the overview lists is a link a user can click, so the one pdbmux + // owns has to lead to a page with every node on it rather than an empty one. + t.Run("the owned fact is listed and its link resolves", func(t *testing.T) { + if !strings.Contains(body, defaultSourceFact) { + t.Fatalf("Puppetboard facts overview does not list %s", defaultSourceFact) + } + listed := pbFactRows(t, defaultSourceFact) + for _, cn := range allNodes { + if !listed[cn] { + t.Errorf("the %s drilldown omits %s, so the overview links to a dead page", defaultSourceFact, cn) + } + } + }) +} + +// pbFactRows reads the JSON table a Puppetboard fact page renders from, and +// returns the certnames it lists. +func pbFactRows(t *testing.T, name string) map[string]bool { + t.Helper() + var payload struct { + Data [][]string `json:"data"` + } + body := pbPage(t, pbAllEnvs+"/fact/"+name+"/json") + if err := json.Unmarshal([]byte(body), &payload); err != nil { + t.Fatalf("decoding the %s drilldown table: %v: %s", name, err, body) + } + listed := map[string]bool{} + for _, row := range payload.Data { + for _, cn := range allNodes { + if len(row) > 0 && strings.Contains(row[0], cn) { + listed[cn] = true + } + } + } + return listed } // The single-fact drilldown is the page that exercises the merged /facts/ @@ -162,26 +198,9 @@ func TestPuppetboardFactDrilldown(t *testing.T) { t.Errorf("Puppetboard fact page for osfamily does not name it") } - // The page's table is filled from this endpoint, so it is what a user sees. - var payload struct { - Data [][]string `json:"data"` - } - body := pbPage(t, pbAllEnvs+"/fact/osfamily/json") - if err := json.Unmarshal([]byte(body), &payload); err != nil { - t.Fatalf("decoding the fact drilldown table: %v: %s", err, body) - } - - listed := map[string]bool{} - for _, row := range payload.Data { - for _, cn := range allNodes { - if len(row) > 0 && strings.Contains(row[0], cn) { - listed[cn] = true - } - } - } - // Puppetboard fetches a single fact through GET /pdb/query/v4/facts/, // so the page is only whole if that path route merges every backend. + listed := pbFactRows(t, "osfamily") missing := []string{} for _, cn := range allNodes { if !listed[cn] { diff --git a/e2e_query_test.go b/e2e_query_test.go index 4f7bb4e..17a6861 100644 --- a/e2e_query_test.go +++ b/e2e_query_test.go @@ -456,6 +456,83 @@ func TestSourceFactInjectionAndGating(t *testing.T) { }) } +// /fact-names advertises the fact, so its drilldown has to answer with the same +// records /facts carries rather than the empty set the backends hold. +func TestSourceFactDrilldown(t *testing.T) { + want := map[string]string{ + nodeAlpha: backendAName, + nodeBeta: backendBName, + nodeGamma: backendBName, + nodeShared: backendBName, // won on freshness, not on configured order + } + path := factsPath + "/" + defaultSourceFact + + t.Run("one record per node, attributed as /facts attributes it", func(t *testing.T) { + resp := get(t, path, nil) + rows := resp.rows(t) + if got := e2eCertnames(rows); !equalStrings(got, allNodes) { + t.Fatalf("%s certnames = %v, want %v", path, got, allNodes) + } + if len(rows) != len(allNodes) { + t.Fatalf("%s returned %d records for %d nodes", path, len(rows), len(allNodes)) + } + unfiltered := get(t, factsPath, nil).rows(t) + for _, row := range rows { + cn, _ := row["certname"].(string) + if row["name"] != defaultSourceFact { + t.Errorf("%s returned a record named %v", path, row["name"]) + } + if row["value"] != want[cn] { + t.Errorf("%s for %s = %v, want %q", path, cn, row["value"], want[cn]) + } + // The drilldown must not disagree with the response it stands for. + if got, ok := factValue(unfiltered, cn, defaultSourceFact); !ok || got != row["value"] { + t.Errorf("%s for %s = %v, want the %s value %v", path, cn, row["value"], factsPath, got) + } + if _, ok := row["environment"]; !ok { + t.Errorf("%s record for %s has no environment key", path, cn) + } + } + if got := resp.header.Get(backendsHeader); got != "2/2" { + t.Errorf("%s = %q, want %q", backendsHeader, got, "2/2") + } + }) + + t.Run("the value sub-route filters by owning backend", func(t *testing.T) { + for _, tc := range []struct { + value string + want []string + }{ + {backendAName, []string{nodeAlpha}}, + {backendBName, []string{nodeBeta, nodeGamma, nodeShared}}, + {"nosuchbackend", nil}, + } { + rows := get(t, path+"/"+tc.value, nil).rows(t) + if got := e2eCertnames(rows); !equalStrings(got, tc.want) { + t.Errorf("%s/%s certnames = %v, want %v", path, tc.value, got, tc.want) + } + } + }) + + t.Run("a certname query narrows the drilldown", func(t *testing.T) { + rows := get(t, path, query(`["=","certname","`+nodeAlpha+`"]`)).rows(t) + if got := e2eCertnames(rows); !equalStrings(got, []string{nodeAlpha}) { + t.Errorf("certname-filtered %s = %v, want only %s", path, got, nodeAlpha) + } + }) + + // An aggregate carries no certname to attribute, so it stays summed. + t.Run("an aggregate is still summed", func(t *testing.T) { + ctx := context.Background() + const q = `["extract",[["function","count"]]]` + wantA := backendCount(ctx, t, h.a, path, q) + wantB := backendCount(ctx, t, h.b, path, q) + if got := countOf(t, get(t, path, query(q)).rows(t)); got != wantA+wantB { + t.Errorf("%s count = %d, want %d (%s=%d + %s=%d)", path, got, wantA+wantB, h.a.name, wantA, h.b.name, wantB) + } + }) +} + // X-Backends has to report what the response was actually built from, not what // is configured. func TestBackendsHeaderReportsContributors(t *testing.T) { diff --git a/factroutes_test.go b/factroutes_test.go index ced7655..722dd21 100644 --- a/factroutes_test.go +++ b/factroutes_test.go @@ -34,22 +34,24 @@ func factNamesBody(names ...string) string { func TestFactsSubPath(t *testing.T) { for _, tc := range []struct { - path string - wantName string - wantValue bool + path string + wantName string + wantValue string + wantValued bool }{ - {factsPath, "", false}, - {factsPath + "/", "", false}, - {roleFactPath, "role", false}, - {roleFactPath + "/web", "role", true}, - {roleFactPath + "/", "", false}, - {roleFactPath + "/web/extra", "", false}, - {nodesPath + "/h1/facts/role", "", false}, - {factNamesPath, "", false}, + {factsPath, "", "", false}, + {factsPath + "/", "", "", false}, + {roleFactPath, "role", "", false}, + {roleFactPath + "/web", "role", "web", true}, + {roleFactPath + "/", "", "", false}, + {roleFactPath + "/web/extra", "", "", false}, + {nodesPath + "/h1/facts/role", "", "", false}, + {factNamesPath, "", "", false}, } { - name, valued := factsSubPath(tc.path) - if name != tc.wantName || valued != tc.wantValue { - t.Errorf("factsSubPath(%q) = (%q, %v), want (%q, %v)", tc.path, name, valued, tc.wantName, tc.wantValue) + name, value, valued := factsSubPath(tc.path) + if name != tc.wantName || value != tc.wantValue || valued != tc.wantValued { + t.Errorf("factsSubPath(%q) = (%q, %q, %v), want (%q, %q, %v)", + tc.path, name, value, valued, tc.wantName, tc.wantValue, tc.wantValued) } } } @@ -187,7 +189,7 @@ func TestHandler_FactsByNameNonAggregateStillMergedByCertname(t *testing.T) { // A summed row is not the merged record set the cache stores, so it stays live. func TestHandler_FactsByNameAggregateNotCached(t *testing.T) { - for _, path := range []string{roleFactPath, roleFactPath + "/web"} { + for _, path := range []string{roleFactPath, roleFactPath + "/web", sourceFactURL} { t.Run(path, func(t *testing.T) { a := newCountingBackend(t, map[string]string{path: `[{"count":7}]`}) b := newCountingBackend(t, map[string]string{path: `[{"count":3}]`}) @@ -236,48 +238,172 @@ func TestHandler_FactsByNameNotInjected(t *testing.T) { } } -// Only the source fact's own path may carry it; the upstream record of that name -// is still dropped, as it is on every other query shape. -func TestHandler_FactsBySourceNamePathInjectsAndSuppresses(t *testing.T) { - a := newFakeBackend(t, `[]`, `[]`) - a.bodies[sourceFactURL] = `[` + fact("h1", defaultSourceFact, "stale", "") + `]` - b := newFakeBackend(t, `[]`, `[]`) - b.bodies[sourceFactURL] = `[]` - srv := newTestServer(testConfig(a.srv.URL, b.srv.URL, mergeStatic)) +// sourceFactBackends is a pair of fake backends whose /facts bodies give h1 to a +// and h2/h3 to b, the shape every drilldown assertion below rests on. +func sourceFactBackends(t *testing.T) (*fakeBackend, *fakeBackend) { + t.Helper() + a := newFakeBackend(t, + `[`+node("h1", "2026-01-01T00:00:00Z")+`,`+node("h3", "2026-01-01T00:00:00Z")+`]`, + `[`+factEnv("h1", "osfamily", "RedHat", "prod")+`,`+factEnv("h3", "osfamily", "RedHat", "prod")+`]`) + b := newFakeBackend(t, + `[`+node("h2", "2026-01-01T00:00:00Z")+`,`+node("h3", "2026-06-01T00:00:00Z")+`]`, + `[`+factEnv("h2", "osfamily", "Debian", "dev")+`,`+factEnv("h3", "osfamily", "Debian", "dev")+`]`) + return a, b +} - rec := doGet(t, srv.Handler(), sourceFactURL, "") - got, n := sourceValues(t, rec.Body.Bytes(), defaultSourceFact) - if n != 1 { - t.Fatalf("%s carried %d %s records, want exactly the synthetic one: %s", sourceFactURL, n, defaultSourceFact, rec.Body.String()) - } - if got["h1"] != "a" { - t.Errorf("%s for h1 = %q, want the owning backend %q", defaultSourceFact, got["h1"], "a") +// The drilldown has to answer with the records /facts carries, which no backend +// holds: the certname set and the per-node owner come from the same merge. +func TestHandler_FactsBySourceNamePathSynthesisesOneRecordPerNode(t *testing.T) { + for _, merge := range []string{mergeStatic, mergeFreshness} { + t.Run(merge, func(t *testing.T) { + a, b := sourceFactBackends(t) + srv := newTestServer(testConfig(a.srv.URL, b.srv.URL, merge)) + + // h3 lives in both: static keeps the first configured backend, freshness + // the one holding its newer report. + wantH3 := "a" + if merge == mergeFreshness { + wantH3 = "b" + } + want := map[string]string{"h1": "a", "h2": "b", "h3": wantH3} + + rec := doGet(t, srv.Handler(), sourceFactURL, "") + if rec.Code != http.StatusOK { + t.Fatalf("status %d: %s", rec.Code, rec.Body.String()) + } + got, n := sourceValues(t, rec.Body.Bytes(), defaultSourceFact) + if n != len(want) || !reflect.DeepEqual(got, want) { + t.Fatalf("%s = %v (%d records), want %v", sourceFactURL, got, n, want) + } + + // The drilldown must agree with the records the unfiltered route reports. + unfiltered, _ := sourceValues(t, doGet(t, srv.Handler(), factsPath, "").Body.Bytes(), defaultSourceFact) + if !reflect.DeepEqual(got, unfiltered) { + t.Errorf("%s = %v, want the same attribution %s reports: %v", sourceFactURL, got, factsPath, unfiltered) + } + if h := rec.Header().Get(backendsHeader); h != "2/2" { + t.Errorf("%s = %q, want 2/2", backendsHeader, h) + } + }) } } -// The // form also pins a value the synthetic record's own value -// need not equal, so it never carries one. -func TestHandler_FactsBySourceNameAndValueNotInjected(t *testing.T) { - path := sourceFactURL + "/a" - a := newFakeBackend(t, `[]`, `[]`) - a.bodies[path] = `[` + fact("h1", defaultSourceFact, "a", "") + `]` - b := newFakeBackend(t, `[]`, `[]`) - b.bodies[path] = `[]` - srv := newTestServer(testConfig(a.srv.URL, b.srv.URL, mergeStatic)) +// A fact record's four keys are always present, and environment is the owning +// backend's, since that is the record the merge attributed the node to. +func TestHandler_FactsBySourceNamePathCarriesTheOwnersEnvironment(t *testing.T) { + a, b := sourceFactBackends(t) + srv := newTestServer(testConfig(a.srv.URL, b.srv.URL, mergeFreshness)) - rec := doGet(t, srv.Handler(), path, "") - if _, n := sourceValues(t, rec.Body.Bytes(), defaultSourceFact); n != 0 { - t.Errorf("%s appeared on %s: %s", defaultSourceFact, path, rec.Body.String()) + rec := doGet(t, srv.Handler(), sourceFactURL, "") + var rows []map[string]any + if err := json.Unmarshal(rec.Body.Bytes(), &rows); err != nil { + t.Fatalf("unmarshal %s: %v", rec.Body.String(), err) + } + want := map[string]string{"h1": "prod", "h2": "dev", "h3": "dev"} + for _, row := range rows { + for _, key := range []string{"certname", "name", "value", "environment"} { + if _, ok := row[key]; !ok { + t.Fatalf("synthetic record %v is missing %s", row, key) + } + } + cn, _ := row["certname"].(string) + if got := row["environment"]; got != want[cn] { + t.Errorf("environment of %s = %v, want the owner's %q", cn, got, want[cn]) + } + } +} + +// The pinned value names a backend, so the sub-route is the drilldown filtered +// by owner; a value naming no backend matches nothing. +func TestHandler_FactsBySourceNameAndValueFiltersByOwner(t *testing.T) { + for _, tc := range []struct { + value string + want map[string]string + }{ + {"a", map[string]string{"h1": "a"}}, + {"b", map[string]string{"h2": "b", "h3": "b"}}, + {"nosuchbackend", map[string]string{}}, + } { + t.Run(tc.value, func(t *testing.T) { + a, b := sourceFactBackends(t) + srv := newTestServer(testConfig(a.srv.URL, b.srv.URL, mergeFreshness)) + + rec := doGet(t, srv.Handler(), sourceFactURL+"/"+tc.value, "") + got, n := sourceValues(t, rec.Body.Bytes(), defaultSourceFact) + if n != len(tc.want) || !reflect.DeepEqual(got, tc.want) { + t.Errorf("%s/%s = %v (%d records), want %v", sourceFactURL, tc.value, got, n, tc.want) + } + }) + } +} + +// Nothing produces the fact while injection is off, so the drilldown is empty +// and the name is not advertised for a client to click through to. +func TestHandler_FactsBySourceNamePathEmptyWhenDisabled(t *testing.T) { + a, b := sourceFactBackends(t) + a.bodies[factNamesPath] = factNamesBody("osfamily") + b.bodies[factNamesPath] = `[]` + cfg := testConfig(a.srv.URL, b.srv.URL, mergeFreshness) + cfg.SourceFactEnabled = false + srv := newTestServer(cfg) + + // No backend holds the fact, so the route falls through to the plain merge. + for _, fb := range []*fakeBackend{a, b} { + fb.bodies[sourceFactURL] = `[]` + fb.bodies[sourceFactURL+"/a"] = `[]` + } + for _, path := range []string{sourceFactURL, sourceFactURL + "/a"} { + if got := doGet(t, srv.Handler(), path, "").Body.String(); got != "[]\n" { + t.Errorf("disabled %s = %q, want %q", path, got, "[]\n") + } + } + if got := names(t, doGet(t, srv.Handler(), factNamesPath, "").Body.Bytes()); !reflect.DeepEqual(got, []string{"osfamily"}) { + t.Errorf("disabled %s = %v, want no %s entry", factNamesPath, got, defaultSourceFact) + } +} + +// The query narrows the certnames upstream, so the drilldown only covers what it +// selects. +func TestHandler_FactsBySourceNamePathRespectsQuery(t *testing.T) { + a, b := sourceFactBackends(t) + a.bodies[factsPath] = `[` + factEnv("h1", "osfamily", "RedHat", "prod") + `]` + b.bodies[factsPath] = `[]` + srv := newTestServer(testConfig(a.srv.URL, b.srv.URL, mergeFreshness)) + + const q = `["=","certname","h1"]` + rec := doGet(t, srv.Handler(), sourceFactURL, q) + got, n := sourceValues(t, rec.Body.Bytes(), defaultSourceFact) + if n != 1 || got["h1"] != "a" { + t.Fatalf("%s?query=%s = %v (%d records), want only h1: %s", sourceFactURL, q, got, n, rec.Body.String()) + } + // The query has to reach the backends for them to narrow anything. + for _, fb := range []*fakeBackend{a, b} { + if seen := fb.gotQuery(factsPath); seen != q { + t.Errorf("backend got query %q on %s, want %q", seen, factsPath, q) + } + } +} + +// pdbmux owns the name, so an upstream record of it is dropped rather than +// listed beside the synthetic one. +func TestHandler_FactsBySourceNamePathSuppressesUpstream(t *testing.T) { + a, b := sourceFactBackends(t) + a.bodies[factsPath] = `[` + factEnv("h1", "osfamily", "RedHat", "prod") + `,` + + fact("h1", defaultSourceFact, "stale", "") + `]` + b.bodies[factsPath] = `[]` + srv := newTestServer(testConfig(a.srv.URL, b.srv.URL, mergeFreshness)) + + rec := doGet(t, srv.Handler(), sourceFactURL, "") + got, n := sourceValues(t, rec.Body.Bytes(), defaultSourceFact) + if n != 1 || got["h1"] != "a" { + t.Fatalf("%s = %v (%d records), want exactly the synthetic one: %s", sourceFactURL, got, n, rec.Body.String()) } } // A name-constrained query on the source fact's own path is still gated by the // query, not just the path. func TestHandler_FactsBySourceNamePathRespectsQueryGate(t *testing.T) { - a := newFakeBackend(t, `[]`, `[]`) - a.bodies[sourceFactURL] = `[` + fact("h1", defaultSourceFact, "stale", "") + `]` - b := newFakeBackend(t, `[]`, `[]`) - b.bodies[sourceFactURL] = `[]` + a, b := sourceFactBackends(t) srv := newTestServer(testConfig(a.srv.URL, b.srv.URL, mergeStatic)) rec := doGet(t, srv.Handler(), sourceFactURL, `["extract",["certname","value"]]`) @@ -286,6 +412,24 @@ func TestHandler_FactsBySourceNamePathRespectsQueryGate(t *testing.T) { } } +// An aggregate carries no certname to attribute, so the route stays on the +// summing path rather than synthesising records. +func TestHandler_FactsBySourceNameAggregateStillSummed(t *testing.T) { + for _, path := range []string{sourceFactURL, sourceFactURL + "/a"} { + t.Run(path, func(t *testing.T) { + a, b := sourceFactBackends(t) + a.bodies[path] = `[{"count":7}]` + b.bodies[path] = `[{"count":3}]` + srv := newTestServer(testConfig(a.srv.URL, b.srv.URL, mergeFreshness)) + + rec := doGet(t, srv.Handler(), path, `["extract",[["function","count"]]]`) + if got := counts(t, rec.Body.Bytes(), "count"); !slices.Equal(got, []float64{10}) { + t.Errorf("count = %v, want [10]", got) + } + }) + } +} + func TestHandler_FactNamesUnionDedupedAndSorted(t *testing.T) { a := newFakeBackend(t, `[]`, `[]`) a.bodies[factNamesPath] = factNamesBody("kernel", "only_a", "osfamily") @@ -472,7 +616,7 @@ func TestHandler_FactNamesSurvivesOneBackend(t *testing.T) { // Both routes are merged record sets, so they use the same cache /facts does. func TestHandler_FactRoutesAreCached(t *testing.T) { - for _, path := range []string{roleFactPath, roleFactPath + "/web", factNamesPath} { + for _, path := range []string{roleFactPath, roleFactPath + "/web", sourceFactURL, factNamesPath} { t.Run(path, func(t *testing.T) { a := newFakeBackend(t, `[]`, `[]`) a.bodies[path] = `[]` diff --git a/merge.go b/merge.go index ece3b96..672b1f9 100644 --- a/merge.go +++ b/merge.go @@ -171,6 +171,28 @@ func mergeFacts(results []backendResult, owner func(certname string) string, inj return out } +// sourceFactRecords keeps the merged /facts records naming the fact pdbmux +// owns, and, when the path pins a value, only those naming that backend. The +// merge drops every upstream record of that name, so what survives is exactly +// the synthetic set — one record per certname the merge attributed. +func sourceFactRecords(merged []json.RawMessage, name, value string, valued bool) []json.RawMessage { + out := []json.RawMessage{} + for _, raw := range merged { + var m struct { + Name string `json:"name"` + Value string `json:"value"` + } + if json.Unmarshal(raw, &m) != nil || m.Name != name { + continue + } + if valued && m.Value != value { + continue + } + out = append(out, raw) + } + return out +} + // mergeFactNames unions the backends' /fact-names arrays, dedupes by the name // itself and re-sorts, since each backend only ordered its own slice. owned is // the fact name pdbmux injects, or "" while injection is off: it is listed diff --git a/server.go b/server.go index 4f23639..689ed38 100644 --- a/server.go +++ b/server.go @@ -145,8 +145,8 @@ func (s *Server) handleQuery(w http.ResponseWriter, r *http.Request) { case eventCountsPath, aggregateEventCountsPath: s.serveSummed(w, r, r.URL.Path, inferredColumns) default: - if name, valued := factsSubPath(r.URL.Path); name != "" { - s.serveFactsByName(w, r, name, valued) + if name, value, valued := factsSubPath(r.URL.Path); name != "" { + s.serveFactsByName(w, r, name, value, valued) return } if isReportSubResource(r.URL.Path) { @@ -158,28 +158,32 @@ func (s *Server) handleQuery(w http.ResponseWriter, r *http.Request) { } // factsSubPath matches /pdb/query/v4/facts/ and /pdb/query/v4/facts//, -// returning the fact name the path constrains and whether it also pins a value. -// openvoxdb serves both from the facts entity, ANDing ["=","name",] (and -// ["=","value",]) onto the request's own query, so the record shape is -// exactly /facts' — src/puppetlabs/puppetdb/http/handlers.clj:283-302 and +// returning the fact name the path constrains, the value it pins and whether it +// pins one at all. openvoxdb serves both from the facts entity, ANDing +// ["=","name",] (and ["=","value",]) onto the request's own query, +// so the record shape is exactly /facts' — +// src/puppetlabs/puppetdb/http/handlers.clj:283-302 and // src/puppetlabs/puppetdb/http/query.clj:136-143,193-209. -func factsSubPath(path string) (name string, valued bool) { +func factsSubPath(path string) (name, value string, valued bool) { rest, ok := strings.CutPrefix(path, factsPath+"/") if !ok { - return "", false + return "", "", false } name, value, hasValue := strings.Cut(rest, "/") if name == "" { - return "", false + return "", "", false } if hasValue && (value == "" || strings.Contains(value, "/")) { - return "", false + return "", "", false } - return name, hasValue + if !hasValue { + value = "" + } + return name, value, hasValue } func isFactsSubPath(path string) bool { - name, _ := factsSubPath(path) + name, _, _ := factsSubPath(path) return name != "" } @@ -302,13 +306,40 @@ func (s *Server) serveUnion(w http.ResponseWriter, r *http.Request, path string, // Both path forms are the facts entity with a name (and value) constraint ANDed // on, so an aggregate over them carries no certname for the per-certname merge -// to key on, and is summed instead — the same split /nodes makes. -func (s *Server) serveFactsByName(w http.ResponseWriter, r *http.Request, name string, valued bool) { +// to key on, and is summed instead — the same split /nodes makes. A plain query +// on the source fact's own path is the one name no backend can answer for, so it +// is synthesised instead of fanned out as-is. +func (s *Server) serveFactsByName(w http.ResponseWriter, r *http.Request, name, value string, valued bool) { if spec := parseAggregate(r.URL.Query().Get("query")); spec != nil { s.serveSummed(w, r, r.URL.Path, spec.columns) return } - s.serveMerged(w, r, r.URL.Path, s.mergeFactsByNameResponse(r, name, valued)) + if inject := s.newSourceInjector(r.URL.Query().Get("query"), true); inject.claims(name) && inject.injects() { + s.serveSourceFact(w, r, inject, value, valued) + return + } + s.serveMerged(w, r, r.URL.Path, s.mergeFactsByNameResponse(r)) +} + +// serveSourceFact answers the drilldown on pdbmux's own fact. No backend holds a +// record of that name, so the route's own fan-out would return nothing; the +// records are taken from the /facts merge that produces them instead, which +// makes the certname set, the owner and the environment identical to the ones an +// unfiltered /facts response reports, and lets the request's query narrow the +// result upstream. That costs one extra fan-out on a rare, user-initiated path. +func (s *Server) serveSourceFact(w http.ResponseWriter, r *http.Request, inject *sourceInjector, value string, valued bool) { + params := queryParams(r.URL.Query().Get("query")) + merge := s.mergeFactsWith(inject) + s.serveCached(w, r, r.URL.Path, params, func(ctx context.Context) (cachedResponse, error) { + alive, err := s.aliveResults(ctx, factsPath, params) + if err != nil { + return cachedResponse{}, err + } + recs := sourceFactRecords(merge(alive), inject.name, value, valued) + resp := cachedResponse{Body: encodeRecords(recs), Records: -1} + s.countBackends(&resp, alive) + return resp, nil + }) } // A count row carries no certname, so the certname-keyed merge would collapse every backend's count into one backend's; aggregates take the summing path instead. @@ -592,14 +623,12 @@ func (s *Server) mergeFactsResponse(r *http.Request) func([]backendResult) []jso } // The path segment of /facts/ is the same outer `name` constraint the -// query gate already rules injection out on, so the synthetic record survives -// only on the source fact's own path — and not on the // form, -// whose pinned value the record's own value need not equal. -func (s *Server) mergeFactsByNameResponse(r *http.Request, name string, valued bool) func([]backendResult) []json.RawMessage { +// query gate already rules injection out on, so nothing is synthesised here: +// the source fact's own path is diverted to serveSourceFact before this. +// Suppression of an upstream record of the owned name stays on. +func (s *Server) mergeFactsByNameResponse(r *http.Request) func([]backendResult) []json.RawMessage { inject := s.newSourceInjector(r.URL.Query().Get("query"), true) - if valued || !inject.claims(name) { - inject.disableInject() - } + inject.disableInject() return s.mergeFactsWith(inject) }