add the catalog-impact binary #2
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: unkin/puppet-catalog-tools#2
Reference in New Issue
Block a user
Delete Branch "benvin/catalog-impact"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Catalog testing has no way to know which hosts a puppet-prod change reaches, so a branch is compiled against the whole estate or against a guess.
Requires #1.
Reviewed the diff against
benvin/scaffold-catalog-diff, verified the mapping rules against the realpuppet-prodcheckout, and ranmake test/make lint(both pass: race-enabled tests green, 0 lint issues).Blocking
Reference regexes silently drop comma-separated names.
internal/impact/index.go'sincludeRe/classRefRematch one identifier per keyword occurrence, not a list.include foo::bar, baz::qux(valid Puppet, and legal forcontain/Class['a','b']too) indexes onlyfoo::bar—baz::quxis never added toincludedBy, so a change to whatever declaresbaz::quxreports zero hosts instead of whatever hostsbaz::quxreaches. Not currently present inpuppet-prod(checked), but it's a direct violation of the stated over-include bias and will silently under-report the day someone writesinclude a, b. Needs the regexes (or a post-match split) to walk the whole comma list..woodpecker/release.yamlwas not updated:for name in catalog-diff; do(line 30) andASSETS="catalog-diff-linux-amd64 ..."(line 130) still only build/attachcatalog-diff's cross-platform binaries to the Gitea release.packaging/nfpm.yamlandscripts/build-rpm.shwere correctly updated forcatalog-impact, so the RPM ships it, but the standalone release binaries won't. Given the PR's own framing ("add the catalog-impact binary" as a full peer of catalog-diff), this looks like a missed spot rather than intentional.Not blocking (would have done differently)
roles/<t1..3>,country/.../region/...,os/<name>/<name><major>.<minor>, etc.) is hand-encoded inpaths.go/hieradataSelectorrather than derived fromhiera.yamlitself. It matches the real file today, but nothing ties the two together — a hierarchy edit in puppet-prod won't be caught by anything short of this tool quietly mis-mapping paths.inherits,require,Class[...]ordering refs,create_resources(), scope-variable reads and bare defined-type declarations. All in service of the one stated goal, so not scope creep, just worth a mention next time.Verified correct (spot-checked against
puppet-prod, not just the fixtures): thehiera_include-only trap (jellyfinmodule, applied solely viahieradata/roles/apps/media/jellyfin.yaml, zeroincludeinsite/) is resolved through the listing file's hierarchy position, not through class reverse-lookup — matches real repo structure exactly. Tier-prefix selection, cycle termination (BFS with aseenset, not recursive — no stack-overflow risk),hiera_excludesubtraction, and os name/major/minor matching all check out. Output ordering is deterministic despite Go map iteration because every map-derived list is sorted before being written out.Review at head
6551ab9(base benvin/scaffold-catalog-diff). Verified against a real puppet-prod checkout, ran the parser adversarially, and exercised the full build/test/rpm chain.Blocking — under-include (parser can silently miss a real reference, no fallback signal):
include $facts['role'] ? { 'web' => 'profile::web', default => 'profile::base' }→references()returns neither name. Same for the indirected form$profile = $x ? {...}; include $profile.commentRe = (?m)#.*$doesn't track quote state. A literal#inside a string (hex color, MOTD text, ticket ref, URL fragment) truncates the rest of that physical line — including any include/contain/require that follows on the same line. Repro:$x = "port #1" ; include profile::afterhash→ afterhash never appears. Same via a selector holding a color value:$x = $y ? { 'a' => '#ff0000', default => '#00ff00' } ; include profile::afterselect→ dropped.include "profile::role_${role}"→ nothing captured.None of these are live in puppet-prod today (no
include $varin the repo; the one#-in-string line has nothing after it on the same line), but there's no equivalent of the path-level "matches no rule, widened to the whole estate" loud fallback for a class the parser can't statically resolve — it just disappears, which is the one failure mode this tool is designed never to have.Not blocking (over-inclusion / noise, consistent with the stated safe-direction bias):
content => @(EOT)\n include profile::secret_admin\n EOTproduces a bogus reference from doc prose. 10 files in puppet-prod use heredocs; none trigger a live false positive today, butsite/profiles/manifests/selinux/frr.pp's SELinux-policy heredoc (a literalrequire { ... }CIL block,;-terminated lines) walkscallArguments's depth-tracker straight past the heredoc's own closing brace and only stops at a bare;deep inside the policy body — it happens to yield nothing only because the resulting blob fails the strict identifier regex. Works by luck of content, not by heredoc awareness.require => Class['foo']is indexed as a real dependency via the standaloneClass[...]regex, even though Puppet's ordering metaparameters (require/before/notify/subscribe) never themselves cause inclusion. This looks deliberate (there's a test asserting it), and it's safe-direction, but--explainwill describe it as "why" the class is pulled in when it's really just an ordering constraint.Verified correct:
jellyfin(module class with zeroincludeanywhere in site/, applied only viahiera_include: - jellyfinin hieradata/roles/apps/media/jellyfin.yaml) resolves correctly to just the apps::media::jellyfin hosts.make build,make completions,make rpmall succeed at this head; the RPM ships both binaries plus bash/zsh/fish completions at correct paths/modes, no hardcoded binary name left anywhere.go test -race ./...all green,golangci-lint run ./...0 issues.Not flagging atomicity: the Makefile/nfpm/release.yaml rework is coupled to this PR only because catalog-impact is the second binary and the old hardcoded-single-name scheme can't express that.
Targeted review of the latest work only:
internal/impact/scan.go, the unresolved-reference widening it feeds, and merge commitf34dacc. Not re-litigating hieradata path mapping or packaging.Ran: parser over the full real
puppet-prodcheckout,go test -race ./...,make lint,make pre-commit, 25s/~800k execs ofFuzzScanReferences— all clean, no crashes. Verdict: not mergeable as-is — one confirmed silent-loss bug and one confirmed dead-safety-net bug, both in the exact mechanism this PR exists to guarantee. Both are small, targeted fixes.Blocking
1.
stripSource/scan.go's quote handling can silently drop a class reference outright — not resolved, not reported unresolved.Root cause: every string-aware helper (
skipString,balanced,splitArgs,statementExpr,selectorArms,cutRocket,resourceTitles) tracks "am I inside a string" by toggling on any unescaped quote character equal to whatever opened the current would-be string — it has no concept of quote-type nesting. That's fine for the very common case of a single-quoted hash key inside a double-quoted interpolation ("${facts['role']}"works correctly, verified). It breaks when a nested, differently-quoted substring itself contains a literal, unescaped instance of the outer delimiter followed by a structural character (),],},,) before the true closing quote:scanReferences(stripSource(body))producesrefs = map[]andunresolved = [{Form: "include", Expr:"${sprintf('a"a}].profile::realappears nowhere — not resolved, not reported unresolved. The literal)inside the single-quoted string desyncs the toggle and is read as the real closing paren ofinclude(...), truncating the argument list beforeprofile::real. A related, non-losing but noisy variant (an embedded comma instead of a paren) just garbles one unresolved entry into two nonsensical fragments, polluting--explainoutput — same root cause.This directly contradicts the PR's stated invariant ("anything that still won't reduce to literal names becomes an
UnresolvedRef... which both reports loudly and widens") — here nothing is even recognized as unresolved, because the tokenizer itself desyncs before the scanner ever sees there was an argument to fail on. Confirmed via isolated unit test, reproducible, not a fluke of my harness (checkedstripSourcedidn't touch the body — it doesn't need to, no comments/heredocs involved).Not found live in the real
puppet-prodcheckout (see below — 0 unresolved refs today), and the fuzzer's 25s run (~800k execs) didn't hit it either since the seed corpus has no multi-quote-type nesting. But it's a real, constructible input, and the whole point of this tool is that such inputs must never silently disappear.2.
UnresolvedRef.Prefixis never lowercased, so the widening safety net silently stops working for exactly the mixed-case idiom this repo already uses.literalPrefix()(scan.go) takes the raw-case text before the first$in an interpolated string literal and stores it verbatim asUnresolvedRef.Prefix. Every resolved class name flowing through the index (IncludedBy,known, the resolver's BFS queue) is normalized to lowercase viaanyCaseNameRe— but this one code path is the sole exception.Index.UnresolvedIncludersthen does a case-sensitivestrings.HasPrefix(class, u.Ref.Prefix).Concretely:
include "APP::${team}_role"yieldsUnresolvedRef{Prefix: "APP::"}(mixed case, untouched). Since everyclassstring passed intoUnresolvedIncludersis guaranteed lowercase,strings.HasPrefix("app::web_role", "APP::")isfalse— the widening that's supposed to catch "this reference might have namedapp::web_role" never fires. That's a silent regression in the exact fallback mechanism whose entire job is to never drop coverage.This isn't hypothetical style: the repo already contains this exact case-insensitive idiom today, just not interpolated —
site/profiles/manifests/sql/galera_member.pp:76,187,195has bothClass['Mysql::Server']andClass['Mysql::server'], and the PR correctly folds both tomysql::server(verified). An interpolated version of the same idiom,Class["Mysql::${x}"], would silently lose its widening safety net. One-line fix: lowercase the prefix inliteralPrefix(or at theunresolvedAddcall site) to match how every other class-name comparison in this file is normalized.Neither of the above shows up on the real
puppet-prodcheckout:BuildIndex(puppet-prod)— 349 known classes indexed, 0UnresolvedRefs produced anywhere in the repo today. So both bugs are latent against the current corpus, confirmed only via targeted unit tests against the parser's own logic — not evidence of a live incident, but real gaps against a stated hard invariant.Non-blocking
3. The fuzzer doesn't assert the property it's there for.
FuzzScanReferencesonly checkslen(stripped) == len(body). The comment onstripSource(and the separate, non-fuzzedTestStripSourcePreservesLineCount) says what actually matters is that newline positions survive, since "the statement scanner reads newlines." By inspection the current implementation can't violate that (blank()explicitly skips\n, and the buffer is never resized), so nothing is hidden today — but the fuzz property is strictly weaker than the invariant it's meant to guard, so a future edit that broke newline placement while somehow preserving length wouldn't be caught by 800k fuzz execs. Cheap fix: fold the same newline-position check fromTestStripSourcePreservesLineCountinto the fuzz function.4. Widening via an empty
Prefix(item 2 in the brief) — confirmed as designed, not a bug, but a sharp edge worth flagging.strings.HasPrefix(class, "")is always true, so anUnresolvedRefwith no recoverable prefix matches every class queried in everyresolve()walk — intentional given "unknown could be anything." Confirmed the BFS still terminates safely (bounded by theseenmap — no loop, no blowup) and theviachain text stays coherent (reads as "X might apply Y via this unresolved reference in Z", not garbage). Not exercised at all today (0 unresolved refs inpuppet-prod), but the mechanism means: the day any commonly-included file (e.g.profiles::base) picks up even one bare-variableinclude $x-style reference, most future changes will resolve to "whole estate affected" via that one file, silently regardless of what actually changed. Not asking for a redesign — the over-include bias is correct — just flagging it as worth watching for in review of future PRs that touch widely-included profiles.5.
Client.Warngap (item 5 in the brief) — assessed: non-blocking, and narrower than framed. Confirmedcmd/catalog-impact/main.gobuilds apdbmux.Clientwithout setting.Warn, unlikecatalog-diff/main.go:257. Butcatalog-impact's only fact-fetch path (impact.LoadNodes→Client.FactsByName) never callsfold()and never callsWarnat all —FactsByNamedecodes straight into[]FactRecord{Value any}, which for a well-formed JSON body essentially never fails to decode. So wiring.Warntoday would be a no-op. The actual (much narrower) silent-drop on this path is ininternal/impact/node.go:LoadNodesdrops any record with an emptyCertnamewith zero logging (~line 70), andapplyFact'sos-fact handling silently no-ops on a shape mismatch. Neither is part of this PR's new work, and neither has an existing hook to report through — worth a follow-up ticket to giveLoadNodesits own loud-drop path, but not a reason to block this PR.Merge commit
f34dacc— verified cleanDiffed both parent tips against the merge for the two files it touched:
internal/pdbmux/pdbmux_test.go: all ofscaffold-catalog-diff's new fold/decodeFactsetstests (bare-object fallback, array-with-match, array-no-match, empty array, skip-undecodable, nothing-decodable) are present, followed by all ofcatalog-impact'sFactsByNametests (query shape, empty names, server error) — no duplicates, nothing dropped from either side.AGENTS.md: both sides' additions (pdbmux factset-shape note, catalog gotchas, newcatalog-impact gotchassection) are present together; nothing reverted.FactsByName(pdbmux.go) is a self-contained raw AST-query path independent offold/decodeFactsets, so it doesn't regress or depend on the parent's factset rework — verified by reading the implementation, not just by "it compiles."go test -race ./...passes.Also checked, no issues found
warn()→ stderr,--jsonviaunresolved_references+summary.resolution_incomplete,--explainvia both a summary line and a full section.RenderJSONnil-guardsHosts/ChangedFiles/UnmappedFiles/UnresolvedRefsto[], so--jsonstays well-formed (nonull) with zero unresolved references.Result.UnresolvedRefs/hosts/reasons are all explicitly sorted before rendering; map iteration order (scan.go'srefs, index.go'sincludedBy) never leaks into output ordering.literal()lowercases both theclassNameReandanyCaseNameRebranches beforeadd(), and theresourceRefRematch is lowercased at its call site. The one gap is finding #2 above.Happy to point at exact line numbers for the fixes if useful — both are small (a lowercase call, and either a real interpolation-aware tokenizer or at minimum quote-type-aware nesting in the shared string-skipping helpers).
Reviewed commits
ae948c7(parser rewrite) anda5f2734(merge ofbenvin/scaffold-catalog-diff).Blocking
cmd/catalog-impact/main.gobuilds its client withpuppetdb.New(opts.puppetdb, opts.timeout)and never sets.Warn(contrastcmd/catalog-diff/main.go, which does:facts.Warn = func(msg string) {...}). The merge's consolidatedFactsByNamenow drops-and-warns undecodable fact records through that samec.Warncallback — previouslyFactsByNamedecoded straight toanyand could only hard-fail the whole estate fetch. SinceWarnis nil here,if len(skipped) > 0 && c.Warn != nil { c.Warn(...) }never fires: the warning string is built and thrown away.Concrete repro: if every requested fact for one host (enc_role/country/os/virtual/...) fails to decode — e.g. a record with no
valuekey at all,len(r.Value)==0— that host is excluded fromoutinsideFactsByName, never reachesLoadNodes'sbyNamemap, and disappears from the entire estate table with zero trace: no stderr line, nonode_warningsentry, no--explainsection,summary.resolution_incompletestaysfalse. That's a silent under-include — the one thing the design bias doc says must never happen, and the exact failure mode this stack was supposed to make visible (seeTestLoadNodesReportsRecordsWithNoCertname, which does correctly surface the no-certname case end to end).Fix: wire
Warnincmd/catalog-impact/main.gothe same waycatalog-diffdoes, and ideally route the message intoResult.NodeWarnings/node_warningstoo (the channelanonymousWarningsalready built for exactly this class of problem) rather than stderr-only, so it shows up in--json/--explainas well as plain mode.TestFactsByNameSkipsUndecodableRecordssetsc.Warnmanually and passes — that masked the fact that the real CLI never wires it up; worth a CLI-level (orAnalyzer-level) test that fails without the fix.Non-blocking / clean
walkSource/balanced/skipString/skipInterpolationunification against all six original scanners (splitArgs,statementExpr,resourceTitles,selectorArms,cutRocket,balanced): the depth semantics line up exactly (pre-decrement check on close,pairDelimscoped tobalanced's own bracket pair so an unmatched brace elsewhere still can't unbalance it,anyDelimfor the rest matching the original's generic three-bracket counting).${, an interpolation containing heredoc-marker-looking text, and interpolation nested inside a heredoc body all resolve safely (tounresolved, never a wrong or silently-dropped answer, never a hang/panic) — verified with a 20s/~172k-exec fuzz run (no new crashers) plus targeted manual cases against the built package.literalPrefixlowering is a genuine correctness fix, not just refactor churn:index.go's widening check (strings.HasPrefix(class, u.Ref.Prefix)) compares against a lowercase index, so an unlowered prefix from a mixed-case interpolated name (e.g.Class["Mysql::${x}"]) would previously never widen-match — a silent under-include. Covered byTestUnresolvedPrefixIsLowercased.queryFacts/decodeRecords/decodeValuefor bothFactsandFactsByName. The--pdb→--puppetdbrename is fully propagated (grepped forpdb_url/PDBURL/opts.pdb/CATALOG_DIFF_PDB— no leftovers), andFactsByNamepicks up BOM-tolerance as a side effect it didn't have pre-merge.pdbmux.*unkin.netoccurrences remain (default URL + README mirror).node_warningsfollows the same nil→[]guard as the other--jsonfields (RenderJSON), so--jsonstays well-formed in the new empty/warning cases.go test -race ./...,make lint,make pre-commitall green on this branch.Work out which hosts a puppet-prod change can affect, so a catalog test can fan out over exactly those hosts instead of the whole estate. - add cmd/catalog-impact and internal/impact - map hieradata paths onto fact selectors from the hiera hierarchy - trace manifest classes back to role classes through include, contain, class {} and hiera_include, to a fixed point - fetch the estate fact table from pdbmux in one query via FactsByName - widen to all hosts, loudly, for any path that matches no rule - ship the binary and its completions in the RPMA differently quoted substring inside a ${...} interpolation ended the outer string early and truncated the argument list, dropping every name after it. One shared walker now backs every source scan. Lowercase the recovered prefix of an unresolved reference, and every class name as it is added, so a mixed-case interpolated name still widens. Report fact records with no certname instead of dropping them, and assert stripSource keeps newline positions rather than just length.