diff --git a/README.md b/README.md index 9671dd3..07fdba0 100644 --- a/README.md +++ b/README.md @@ -114,22 +114,26 @@ attributes a shared node to the first backend in configured order, while `/nodes` always attributes it to the backend holding the newer `report_timestamp`. Each answer describes the record it is attached to. -While the fact is enabled the configured name is `pdbmux`'s alone. A `/facts` -record of that name coming from a backend is **always dropped**, on every query -shape — including the shapes below, where nothing is injected in its place — so -the fact means exactly one thing and a node never carries two of it. Each request -that drops one logs it once. Only `source_fact_enabled: false` restores upstream +While the fact is enabled, any `/facts` record whose own `name` field equals the +configured name is dropped, on every query shape — including the shapes below, +where nothing is injected in its place. The rule reads the record, not the query, +so a projection that filters on `name` without returning it — say +`["extract",["certname","value"],["=","name","pdbmux_source"]]` — produces rows +that no longer identify themselves, and an upstream value of that name comes +through. Ask for the `name` column and the guarantee holds. Each request that +drops a record logs it once. Only `source_fact_enabled: false` restores upstream records of that name; rename the synthetic fact via `source_fact` if the real one matters more. **Injection is skipped**, and no synthetic record is added, when: -- the query contains an `extract` anywhere outside an `in` subquery — it projects - a column subset, and with a `["function", ...]` column it aggregates. Injecting - there would break the row shape or silently inflate a `count()`, so **aggregate +- the query contains an `extract` outside a subquery — it projects a column + subset, and with a `["function", ...]` column it aggregates. Injecting there + would break the row shape or silently inflate a `count()`, so **aggregate results are never changed**. The whole query is walked, so an `extract` nested - under `and`/`or`/`not`/`from` skips injection too; an `extract` inside an `in` - operand projects the subquery rather than the response, so it does not; + under `and`/`or`/`not`/`from` skips injection too; an `extract` under `in`, + `subquery`, or `select_` projects that subquery rather than the + response, so it does not; - the query is not an AST array — every **PQL-syntax** query (`facts { certname = "web1" }`) lands here. `pdbmux` cannot tell what such a query projects, so it never injects into a PQL response. Use the AST form to get the fact; diff --git a/source.go b/source.go index 2f74449..feec363 100644 --- a/source.go +++ b/source.go @@ -3,6 +3,7 @@ package main import ( "encoding/json" "log" + "strings" ) // sourceInjector owns the configured fact name for one request. A nil @@ -25,9 +26,10 @@ func (s *Server) newSourceInjector(query string, factEntity bool) *sourceInjecto return &sourceInjector{name: s.cfg.SourceFact, inject: injectable(query, factEntity)} } -// claims reports whether an upstream record carries the name pdbmux owns. While -// the feature is enabled the name means one thing on every query shape, so such -// a record is dropped even when the query gate has ruled synthesis out. +// claims reports whether an upstream record carries the name pdbmux owns, and is +// keyed on the record's own name field: a projection that omits the name column +// yields records that cannot be identified, so they pass through. Suppression +// does not depend on the query gate. func (si *sourceInjector) claims(factName string) bool { return si != nil && factName != "" && factName == si.name } @@ -95,7 +97,8 @@ func (si *sourceInjector) stamp(raw json.RawMessage, backend string) json.RawMes // pdbmux cannot tell what it projects, so it changes nothing; // - an `extract` anywhere in the query's own projection scope, which projects a // column subset and, with a `["function", ...]` column, aggregates — injecting -// there would corrupt the row shape or silently inflate a count(); +// there would corrupt the row shape or silently inflate a count(). Operands +// scoped to a subquery are excluded; see hasExtract; // - on the facts entity, any outer constraint on `name`, which selects // specific facts. Subquery operands are not descended into: they choose which // nodes match, not which facts come back. @@ -125,11 +128,18 @@ func injectable(query string, factEntity bool) bool { // projection scope. openvoxdb accepts an extract as an operand of a boolean // operator — engine.clj's user-node->plan-node sends every and/or/not operand // back through itself (src/puppetlabs/puppetdb/query_eng/engine.clj:2697-2733) -// and valid-operator? lists "extract" (:2780-2784) — so the row shape can be +// and valid-operator? lists "extract" (:2779-2784) — so the row shape can be // rewritten below the top level, and the whole tree is walked to fail closed. -// `in` is the one operator not descended into: its operand becomes -// InExpression's :subquery (:2705-2712), projecting the subquery rather than -// the response. +// +// Operators whose operand is scoped to a subquery are not descended into, +// because an extract there projects the subquery rather than the response: +// +// - `in`, whose operand becomes InExpression's :subquery (:2705-2712); +// - `subquery`, which the AST-rewrite stage expands into +// ["in" cols ["extract" cols ["select_" expr]]] (:2111-2123) +// before any plan node is built; +// - `select_`, the explicit subquery form (:1889-1911), reachable +// only under one of the two above in a query openvoxdb accepts. func hasExtract(parts []json.RawMessage) bool { if len(parts) == 0 { return false @@ -138,10 +148,10 @@ func hasExtract(parts []json.RawMessage) bool { if json.Unmarshal(parts[0], &op) != nil { return false } - switch op { - case "extract": + switch { + case op == "extract": return true - case "in": + case op == "in", op == "subquery", strings.HasPrefix(op, "select_"): return false } for _, p := range parts[1:] { diff --git a/source_test.go b/source_test.go index fe63ad5..ae63bf0 100644 --- a/source_test.go +++ b/source_test.go @@ -302,6 +302,32 @@ func TestHandler_SourceDisabledKeepsUpstreamFact(t *testing.T) { } } +// Suppression matches a record's own name field, so rows from a projection that +// filters on name without returning it are not self-identifying and pass +// through. Pinned as a known limit of the guarantee, and documented as one. +func TestHandler_ProjectionWithoutNameColumnCarriesUpstreamValue(t *testing.T) { + a := newFakeBackend(t, `[`+node("h1", "2026-07-20T00:00:00Z")+`]`, `[]`) + b := newFakeBackend(t, `[]`, `[]`) + a.bodies[factsPath] = `[{"certname":"h1","value":"upstream-value"}]` + var buf bytes.Buffer + srv := NewServer(testConfig(a.srv.URL, b.srv.URL, mergeStatic), log.New(&buf, "", 0)) + + rec := doGet(t, srv.Handler(), factsPath, + `["extract",["certname","value"],["=","name","`+defaultSourceFact+`"]]`) + if rec.Code != http.StatusOK { + t.Fatalf("status %d: %s", rec.Code, rec.Body.String()) + } + if got := rec.Body.String(); !strings.Contains(got, "upstream-value") { + t.Errorf("unidentifiable row was dropped: %s", got) + } + if strings.Contains(buf.String(), "dropped") { + t.Errorf("a row with no name field was counted as suppressed: %s", buf.String()) + } + if _, n := sourceValues(t, rec.Body.Bytes(), defaultSourceFact); n != 0 { + t.Errorf("gated projection gained %d synthetic records: %s", n, rec.Body.String()) + } +} + // Covers only that an aggregate /facts response gains no synthetic record and no // rewritten row. It does not cover whether the aggregate rows are correct: // /facts has no parseAggregate branch, so its rows take the certname merge @@ -427,6 +453,11 @@ func TestInjectable(t *testing.T) { {"nested extract on nodes", `["and",["extract",["certname"]]]`, false, false}, // An extract inside an `in` operand projects the subquery, not the response. {"extract under in stays injectable", `["in","certname",["extract",["certname"],["select_facts",["=","name","os"]]]]`, true, true}, + // `subquery` is rewritten to ["in" ... ["extract" ... ["select_x" ...]]] + // before any plan node is built, so its operand is subquery-scoped too. + {"subquery operand stays injectable", `["and",["=","certname","h1"],["subquery","facts",["extract",["certname"],["=","name","os"]]]]`, true, true}, + {"bare subquery stays injectable", `["subquery","facts",["extract",["certname"]]]`, true, true}, + {"extract under select_facts stays injectable", `["and",["select_facts",["extract",["certname"]]]]`, true, true}, {"nodes name filter is not a fact filter", `["=","name","os"]`, false, true}, {"nodes extract", `["extract",["certname"]]`, false, false}, {"unparseable query", `not json`, true, false},