Synthesise the /facts/<source-fact> drilldown
ci/woodpecker/pr/build Pipeline was successful
ci/woodpecker/pr/test Pipeline was successful
ci/woodpecker/pr/pre-commit Pipeline was successful

/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/<source-fact>/<value> by the owning backend.
- Keep the aggregate, query-gate and disabled paths answering as before.
This commit is contained in:
2026-09-06 16:09:55 +10:00
parent cc71902a0d
commit 148be4fe0f
6 changed files with 399 additions and 99 deletions
+21 -12
View File
@@ -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/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` | 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/<name>[/<value>]` | 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/<name>[/<value>]` | 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/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/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. | | `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). backend actually returned facts for it).
- `/facts/<name>` and `/facts/<name>/<value>` are the same records with one - `/facts/<name>` and `/facts/<name>/<value>` are the same records with one
more constraint applied upstream, so they take the same rule — and the same 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 - **`/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 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 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 subquery narrows which *nodes* match, not which facts come back, so injection
still happens; still happens;
- the path is `/facts/<name>` for any other fact. The path segment is the same - the path is `/facts/<name>` for any other fact. The path segment is the same
outer `name` constraint, so only `/facts/pdbmux_source` may carry the record; outer `name` constraint, so only the fact's own path carries the record;
`/facts/<name>/<value>` never does, since the pinned value need not equal the
backend name the record holds;
- injection is turned off (see `source_fact_enabled`). - 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/<value>` 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 **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 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 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 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 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) 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 `/facts` response, from the path route above, and filter client-side.
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.
**Not covered:** `/factsets` and `/inventory`. Both carry facts, but `pdbmux` **Not covered:** `/factsets` and `/inventory`. Both carry facts, but `pdbmux`
does not merge either today — they take the unmerged pass-through path, where does not merge either today — they take the unmerged pass-through path, where
+37 -18
View File
@@ -153,6 +153,42 @@ func TestPuppetboardFactsOverview(t *testing.T) {
t.Errorf("Puppetboard facts overview does not list %s", name) 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/<name> // The single-fact drilldown is the page that exercises the merged /facts/<name>
@@ -162,26 +198,9 @@ func TestPuppetboardFactDrilldown(t *testing.T) {
t.Errorf("Puppetboard fact page for osfamily does not name it") 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/<name>, // Puppetboard fetches a single fact through GET /pdb/query/v4/facts/<name>,
// so the page is only whole if that path route merges every backend. // so the page is only whole if that path route merges every backend.
listed := pbFactRows(t, "osfamily")
missing := []string{} missing := []string{}
for _, cn := range allNodes { for _, cn := range allNodes {
if !listed[cn] { if !listed[cn] {
+77
View File
@@ -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 // X-Backends has to report what the response was actually built from, not what
// is configured. // is configured.
func TestBackendsHeaderReportsContributors(t *testing.T) { func TestBackendsHeaderReportsContributors(t *testing.T) {
+191 -47
View File
@@ -34,22 +34,24 @@ func factNamesBody(names ...string) string {
func TestFactsSubPath(t *testing.T) { func TestFactsSubPath(t *testing.T) {
for _, tc := range []struct { for _, tc := range []struct {
path string path string
wantName string wantName string
wantValue bool wantValue string
wantValued bool
}{ }{
{factsPath, "", false}, {factsPath, "", "", false},
{factsPath + "/", "", false}, {factsPath + "/", "", "", false},
{roleFactPath, "role", false}, {roleFactPath, "role", "", false},
{roleFactPath + "/web", "role", true}, {roleFactPath + "/web", "role", "web", true},
{roleFactPath + "/", "", false}, {roleFactPath + "/", "", "", false},
{roleFactPath + "/web/extra", "", false}, {roleFactPath + "/web/extra", "", "", false},
{nodesPath + "/h1/facts/role", "", false}, {nodesPath + "/h1/facts/role", "", "", false},
{factNamesPath, "", false}, {factNamesPath, "", "", false},
} { } {
name, valued := factsSubPath(tc.path) name, value, valued := factsSubPath(tc.path)
if name != tc.wantName || valued != tc.wantValue { if name != tc.wantName || value != tc.wantValue || valued != tc.wantValued {
t.Errorf("factsSubPath(%q) = (%q, %v), want (%q, %v)", tc.path, name, valued, tc.wantName, tc.wantValue) 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. // A summed row is not the merged record set the cache stores, so it stays live.
func TestHandler_FactsByNameAggregateNotCached(t *testing.T) { 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) { t.Run(path, func(t *testing.T) {
a := newCountingBackend(t, map[string]string{path: `[{"count":7}]`}) a := newCountingBackend(t, map[string]string{path: `[{"count":7}]`})
b := newCountingBackend(t, map[string]string{path: `[{"count":3}]`}) 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 // sourceFactBackends is a pair of fake backends whose /facts bodies give h1 to a
// is still dropped, as it is on every other query shape. // and h2/h3 to b, the shape every drilldown assertion below rests on.
func TestHandler_FactsBySourceNamePathInjectsAndSuppresses(t *testing.T) { func sourceFactBackends(t *testing.T) (*fakeBackend, *fakeBackend) {
a := newFakeBackend(t, `[]`, `[]`) t.Helper()
a.bodies[sourceFactURL] = `[` + fact("h1", defaultSourceFact, "stale", "") + `]` a := newFakeBackend(t,
b := newFakeBackend(t, `[]`, `[]`) `[`+node("h1", "2026-01-01T00:00:00Z")+`,`+node("h3", "2026-01-01T00:00:00Z")+`]`,
b.bodies[sourceFactURL] = `[]` `[`+factEnv("h1", "osfamily", "RedHat", "prod")+`,`+factEnv("h3", "osfamily", "RedHat", "prod")+`]`)
srv := newTestServer(testConfig(a.srv.URL, b.srv.URL, mergeStatic)) 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, "") // The drilldown has to answer with the records /facts carries, which no backend
got, n := sourceValues(t, rec.Body.Bytes(), defaultSourceFact) // holds: the certname set and the per-node owner come from the same merge.
if n != 1 { func TestHandler_FactsBySourceNamePathSynthesisesOneRecordPerNode(t *testing.T) {
t.Fatalf("%s carried %d %s records, want exactly the synthetic one: %s", sourceFactURL, n, defaultSourceFact, rec.Body.String()) for _, merge := range []string{mergeStatic, mergeFreshness} {
} t.Run(merge, func(t *testing.T) {
if got["h1"] != "a" { a, b := sourceFactBackends(t)
t.Errorf("%s for h1 = %q, want the owning backend %q", defaultSourceFact, got["h1"], "a") 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 /<name>/<value> form also pins a value the synthetic record's own value // A fact record's four keys are always present, and environment is the owning
// need not equal, so it never carries one. // backend's, since that is the record the merge attributed the node to.
func TestHandler_FactsBySourceNameAndValueNotInjected(t *testing.T) { func TestHandler_FactsBySourceNamePathCarriesTheOwnersEnvironment(t *testing.T) {
path := sourceFactURL + "/a" a, b := sourceFactBackends(t)
a := newFakeBackend(t, `[]`, `[]`) srv := newTestServer(testConfig(a.srv.URL, b.srv.URL, mergeFreshness))
a.bodies[path] = `[` + fact("h1", defaultSourceFact, "a", "") + `]`
b := newFakeBackend(t, `[]`, `[]`)
b.bodies[path] = `[]`
srv := newTestServer(testConfig(a.srv.URL, b.srv.URL, mergeStatic))
rec := doGet(t, srv.Handler(), path, "") rec := doGet(t, srv.Handler(), sourceFactURL, "")
if _, n := sourceValues(t, rec.Body.Bytes(), defaultSourceFact); n != 0 { var rows []map[string]any
t.Errorf("%s appeared on %s: %s", defaultSourceFact, path, rec.Body.String()) 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 // A name-constrained query on the source fact's own path is still gated by the
// query, not just the path. // query, not just the path.
func TestHandler_FactsBySourceNamePathRespectsQueryGate(t *testing.T) { func TestHandler_FactsBySourceNamePathRespectsQueryGate(t *testing.T) {
a := newFakeBackend(t, `[]`, `[]`) a, b := sourceFactBackends(t)
a.bodies[sourceFactURL] = `[` + fact("h1", defaultSourceFact, "stale", "") + `]`
b := newFakeBackend(t, `[]`, `[]`)
b.bodies[sourceFactURL] = `[]`
srv := newTestServer(testConfig(a.srv.URL, b.srv.URL, mergeStatic)) srv := newTestServer(testConfig(a.srv.URL, b.srv.URL, mergeStatic))
rec := doGet(t, srv.Handler(), sourceFactURL, `["extract",["certname","value"]]`) 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) { func TestHandler_FactNamesUnionDedupedAndSorted(t *testing.T) {
a := newFakeBackend(t, `[]`, `[]`) a := newFakeBackend(t, `[]`, `[]`)
a.bodies[factNamesPath] = factNamesBody("kernel", "only_a", "osfamily") 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. // Both routes are merged record sets, so they use the same cache /facts does.
func TestHandler_FactRoutesAreCached(t *testing.T) { 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) { t.Run(path, func(t *testing.T) {
a := newFakeBackend(t, `[]`, `[]`) a := newFakeBackend(t, `[]`, `[]`)
a.bodies[path] = `[]` a.bodies[path] = `[]`
+22
View File
@@ -171,6 +171,28 @@ func mergeFacts(results []backendResult, owner func(certname string) string, inj
return out 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 // 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 // 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 // the fact name pdbmux injects, or "" while injection is off: it is listed
+51 -22
View File
@@ -145,8 +145,8 @@ func (s *Server) handleQuery(w http.ResponseWriter, r *http.Request) {
case eventCountsPath, aggregateEventCountsPath: case eventCountsPath, aggregateEventCountsPath:
s.serveSummed(w, r, r.URL.Path, inferredColumns) s.serveSummed(w, r, r.URL.Path, inferredColumns)
default: default:
if name, valued := factsSubPath(r.URL.Path); name != "" { if name, value, valued := factsSubPath(r.URL.Path); name != "" {
s.serveFactsByName(w, r, name, valued) s.serveFactsByName(w, r, name, value, valued)
return return
} }
if isReportSubResource(r.URL.Path) { 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/<name> and /pdb/query/v4/facts/<name>/<value>, // factsSubPath matches /pdb/query/v4/facts/<name> and /pdb/query/v4/facts/<name>/<value>,
// returning the fact name the path constrains and whether it also pins a value. // returning the fact name the path constrains, the value it pins and whether it
// openvoxdb serves both from the facts entity, ANDing ["=","name",<name>] (and // pins one at all. openvoxdb serves both from the facts entity, ANDing
// ["=","value",<value>]) onto the request's own query, so the record shape is // ["=","name",<name>] (and ["=","value",<value>]) onto the request's own query,
// exactly /facts' — src/puppetlabs/puppetdb/http/handlers.clj:283-302 and // 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. // 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+"/") rest, ok := strings.CutPrefix(path, factsPath+"/")
if !ok { if !ok {
return "", false return "", "", false
} }
name, value, hasValue := strings.Cut(rest, "/") name, value, hasValue := strings.Cut(rest, "/")
if name == "" { if name == "" {
return "", false return "", "", false
} }
if hasValue && (value == "" || strings.Contains(value, "/")) { 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 { func isFactsSubPath(path string) bool {
name, _ := factsSubPath(path) name, _, _ := factsSubPath(path)
return name != "" 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 // 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 // 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. // to key on, and is summed instead — the same split /nodes makes. A plain query
func (s *Server) serveFactsByName(w http.ResponseWriter, r *http.Request, name string, valued bool) { // 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 { if spec := parseAggregate(r.URL.Query().Get("query")); spec != nil {
s.serveSummed(w, r, r.URL.Path, spec.columns) s.serveSummed(w, r, r.URL.Path, spec.columns)
return 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. // 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/<name> is the same outer `name` constraint the // The path segment of /facts/<name> is the same outer `name` constraint the
// query gate already rules injection out on, so the synthetic record survives // query gate already rules injection out on, so nothing is synthesised here:
// only on the source fact's own path — and not on the /<name>/<value> form, // the source fact's own path is diverted to serveSourceFact before this.
// whose pinned value the record's own value need not equal. // Suppression of an upstream record of the owned name stays on.
func (s *Server) mergeFactsByNameResponse(r *http.Request, name string, valued bool) func([]backendResult) []json.RawMessage { func (s *Server) mergeFactsByNameResponse(r *http.Request) func([]backendResult) []json.RawMessage {
inject := s.newSourceInjector(r.URL.Query().Get("query"), true) inject := s.newSourceInjector(r.URL.Query().Get("query"), true)
if valued || !inject.claims(name) { inject.disableInject()
inject.disableInject()
}
return s.mergeFactsWith(inject) return s.mergeFactsWith(inject)
} }