diff --git a/README.md b/README.md index a1e4f69..f70229d 100644 --- a/README.md +++ b/README.md @@ -242,6 +242,11 @@ A missed host means an unvalidated catalog, so every ambiguity widens: `--explain`, in `unresolved_references` in `--json`, on stderr in plain mode, and set `summary.resolution_incomplete`. +A fact record that arrives with no certname belongs to no host and cannot be +folded into the estate table, so it is counted per fact and reported in +`node_warnings`, in `--explain` and on stderr, and sets +`summary.resolution_incomplete` too. + Paths outside the Puppet modulepath — `.woodpecker/`, `doc/`, `spec/`, `vendor/`, `Gemfile`, `*.md` — cannot reach a catalog and are skipped. diff --git a/cmd/catalog-impact/main.go b/cmd/catalog-impact/main.go index d51cfa2..f9302d7 100644 --- a/cmd/catalog-impact/main.go +++ b/cmd/catalog-impact/main.go @@ -119,12 +119,12 @@ func run(ctx context.Context, out io.Writer, opts *options) error { return fmt.Errorf("indexing %s: %w", opts.repo, err) } - nodes, err := impact.LoadNodes(ctx, pdbmux.New(opts.pdb, opts.timeout)) + nodes, nodeWarnings, err := impact.LoadNodes(ctx, pdbmux.New(opts.pdb, opts.timeout)) if err != nil { return err } - analyzer := &impact.Analyzer{Index: idx, Nodes: nodes} + analyzer := &impact.Analyzer{Index: idx, Nodes: nodes, NodeWarnings: nodeWarnings} result := analyzer.Analyze(opts.base, branch, files, opts.limit) switch { @@ -147,6 +147,9 @@ func warn(r *impact.Result) { for _, line := range impact.UnresolvedLines(r.UnresolvedRefs) { fmt.Fprintf(os.Stderr, "warning: %s did not resolve to a class name; the host list may be incomplete\n", line) } + for _, line := range r.NodeWarnings { + fmt.Fprintf(os.Stderr, "warning: %s; the estate table may be missing hosts\n", line) + } for _, class := range r.OrphanClasses { fmt.Fprintf(os.Stderr, "warning: class %s is declared by nothing and listed in no hiera_include; it reaches no hosts\n", class) } diff --git a/internal/impact/impact.go b/internal/impact/impact.go index 7e243d3..95ed0bb 100644 --- a/internal/impact/impact.go +++ b/internal/impact/impact.go @@ -24,7 +24,8 @@ type Summary struct { Files int `json:"files"` AllHostsTriggered bool `json:"all_hosts_triggered"` // ResolutionIncomplete is set when a class reference could not be resolved - // statically, so the answer is a best effort rather than exact. + // statically, or a fact record could not be attributed to a host, so the + // answer is a best effort rather than exact. ResolutionIncomplete bool `json:"resolution_incomplete"` } @@ -38,7 +39,10 @@ type Result struct { // UnresolvedRefs are class references the parser could not reduce to // names, either in a changed file or on the path from one to a host. UnresolvedRefs []UnresolvedRef `json:"unresolved_references"` - Summary Summary `json:"summary"` + // NodeWarnings are fact records that could not be folded into the estate + // table, so a host may be missing from it entirely. + NodeWarnings []string `json:"node_warnings"` + Summary Summary `json:"summary"` // IgnoredFiles are changed files outside the modulepath — CI, docs, // specs — which cannot reach a catalog. @@ -68,6 +72,8 @@ func (r *Result) Certnames() []string { type Analyzer struct { Index *Index Nodes []Node + // NodeWarnings carries whatever LoadNodes could not fold into Nodes. + NodeWarnings []string } // Analyze maps changed files onto hosts. Files are processed in the order @@ -85,6 +91,7 @@ func (a *Analyzer) Analyze(base, branch string, files []string, limit int) *Resu Base: base, Branch: branch, ChangedFiles: uniqueSorted(files), + NodeWarnings: a.NodeWarnings, TotalHosts: len(a.Nodes), } @@ -154,7 +161,7 @@ func (a *Analyzer) Analyze(base, branch string, files []string, limit int) *Resu Hosts: len(certnames), Files: len(result.ChangedFiles), AllHostsTriggered: len(certnames) > 0 && len(certnames) == len(a.Nodes), - ResolutionIncomplete: len(result.UnresolvedRefs) > 0, + ResolutionIncomplete: len(result.UnresolvedRefs) > 0 || len(result.NodeWarnings) > 0, } if limit > 0 && len(certnames) > limit { diff --git a/internal/impact/impact_test.go b/internal/impact/impact_test.go index ae5bf30..33103b9 100644 --- a/internal/impact/impact_test.go +++ b/internal/impact/impact_test.go @@ -231,10 +231,13 @@ func TestLoadNodesFoldsStructuredOS(t *testing.T) { {Certname: "a.example.net", Name: "enc_role_tier1", Value: "base"}, }} - nodes, err := LoadNodes(context.Background(), src) + nodes, warnings, err := LoadNodes(context.Background(), src) if err != nil { t.Fatalf("LoadNodes: %v", err) } + if len(warnings) != 0 { + t.Errorf("warnings = %v", warnings) + } if !reflect.DeepEqual(src.asked, FactNames) { t.Errorf("asked for %v, want %v", src.asked, FactNames) } @@ -250,11 +253,46 @@ func TestLoadNodesFoldsStructuredOS(t *testing.T) { } func TestLoadNodesRejectsEmptyEstate(t *testing.T) { - if _, err := LoadNodes(context.Background(), &stubFacts{}); err == nil { + if _, _, err := LoadNodes(context.Background(), &stubFacts{}); err == nil { t.Fatal("expected an error for an empty estate") } } +func TestLoadNodesReportsRecordsWithNoCertname(t *testing.T) { + // A record with no certname cannot be attributed to a host. Dropping it in + // silence hides a host, so it has to be reported. + src := &stubFacts{records: []pdbmux.FactRecord{ + {Certname: "a.example.net", Name: "enc_role", Value: "roles::base"}, + {Name: "enc_role", Value: "roles::base"}, + {Name: "enc_role", Value: "roles::web"}, + {Name: "country", Value: "au"}, + }} + + nodes, warnings, err := LoadNodes(context.Background(), src) + if err != nil { + t.Fatalf("LoadNodes: %v", err) + } + if len(nodes) != 1 { + t.Fatalf("nodes = %#v", nodes) + } + want := []string{ + `fact "country": 1 record(s) had no certname and were dropped`, + `fact "enc_role": 2 record(s) had no certname and were dropped`, + } + if !reflect.DeepEqual(warnings, want) { + t.Errorf("warnings = %v, want %v", warnings, want) + } + + r := (&Analyzer{Index: testIndex(t), Nodes: nodes, NodeWarnings: warnings}). + Analyze("base", "branch", []string{"README.md"}, 0) + if !reflect.DeepEqual(r.NodeWarnings, want) { + t.Errorf("Result.NodeWarnings = %v, want %v", r.NodeWarnings, want) + } + if !r.Summary.ResolutionIncomplete { + t.Error("a dropped fact record left the summary looking exact") + } +} + func TestAnalyzeReportsFilesThatReachNoHost(t *testing.T) { // A hieradata file for a role nothing is assigned to maps cleanly and // still selects nobody; so does an orphan class. diff --git a/internal/impact/node.go b/internal/impact/node.go index 03a6399..17922a9 100644 --- a/internal/impact/node.go +++ b/internal/impact/node.go @@ -58,16 +58,19 @@ type FactSource interface { // LoadNodes fetches FactNames for every node and folds them into a sorted // table. Nodes without an enc_role are kept: they still match country, os and -// virtual selectors. -func LoadNodes(ctx context.Context, src FactSource) ([]Node, error) { +// virtual selectors. A record with no certname belongs to no host and cannot +// be kept, so it is returned as a warning rather than dropped in silence. +func LoadNodes(ctx context.Context, src FactSource) ([]Node, []string, error) { records, err := src.FactsByName(ctx, FactNames) if err != nil { - return nil, err + return nil, nil, err } byName := map[string]*Node{} + anonymous := map[string]int{} for _, r := range records { if r.Certname == "" { + anonymous[r.Name]++ continue } n, ok := byName[r.Certname] @@ -84,9 +87,23 @@ func LoadNodes(ctx context.Context, src FactSource) ([]Node, error) { } sort.Slice(nodes, func(i, j int) bool { return nodes[i].Certname < nodes[j].Certname }) if len(nodes) == 0 { - return nil, fmt.Errorf("pdbmux returned no nodes") + return nil, anonymousWarnings(anonymous), fmt.Errorf("pdbmux returned no nodes") } - return nodes, nil + return nodes, anonymousWarnings(anonymous), nil +} + +// anonymousWarnings renders the dropped-record tally one line per fact. +func anonymousWarnings(counts map[string]int) []string { + names := make([]string, 0, len(counts)) + for name := range counts { + names = append(names, name) + } + sort.Strings(names) + out := make([]string, 0, len(names)) + for _, name := range names { + out = append(out, fmt.Sprintf("fact %q: %d record(s) had no certname and were dropped", name, counts[name])) + } + return out } func applyFact(n *Node, name string, value any) { diff --git a/internal/impact/render.go b/internal/impact/render.go index 3bdc7d5..44aed7e 100644 --- a/internal/impact/render.go +++ b/internal/impact/render.go @@ -32,6 +32,9 @@ func RenderJSON(w io.Writer, r *Result) error { if r.UnresolvedRefs == nil { r.UnresolvedRefs = []UnresolvedRef{} } + if r.NodeWarnings == nil { + r.NodeWarnings = []string{} + } return enc.Encode(r) } @@ -66,6 +69,10 @@ func RenderExplain(w io.Writer, r *Result) error { } } + if err := section(w, "fact records dropped (the estate table may be missing hosts)", r.NodeWarnings); err != nil { + return err + } + if err := section(w, "unmapped files (widened to all hosts)", r.UnmappedFiles); err != nil { return err } diff --git a/internal/impact/scan.go b/internal/impact/scan.go index f1076be..b7a763e 100644 --- a/internal/impact/scan.go +++ b/internal/impact/scan.go @@ -82,7 +82,7 @@ func scanReferences(body string) (map[string]refKind, []UnresolvedRef) { s.add(m[1], refOrdering) } for _, m := range resourceRefRe.FindAllStringSubmatch(body, -1) { - s.add(strings.ToLower(m[1]), refOrdering) + s.add(m[1], refOrdering) } return s.refs, dedupeUnresolved(s.unresolved) @@ -94,7 +94,7 @@ func callName(match string) string { } func (s *refScan) add(name string, kind refKind) { - name = strings.TrimPrefix(name, "::") + name = strings.ToLower(strings.TrimPrefix(name, "::")) if name == "" || reserved[name] { return } @@ -162,7 +162,7 @@ func (s *refScan) literal(form string, kind refKind, arg string) { s.add(lit, kind) case anyCaseNameRe.MatchString(lit): // Class[Apache::Ssl] names the class in type form. - s.add(strings.ToLower(lit), kind) + s.add(lit, kind) case quoted: // "profile::role_${role}" is a name only the compiler can finish. s.unresolvedAdd(form, arg, literalPrefix(lit)) @@ -211,13 +211,15 @@ func dedupeUnresolved(in []UnresolvedRef) []UnresolvedRef { } // literalPrefix returns the part of an interpolated name that is fixed text, -// so the report can say what family of classes was missed. +// so the report can say what family of classes was missed. Class names are +// case insensitive and every name the index holds is lowercase, so the prefix +// is lowered too or it matches nothing it is meant to widen to. func literalPrefix(s string) string { i := strings.Index(s, "$") if i <= 0 { return "" } - return s[:i] + return strings.ToLower(s[:i]) } // collapse folds whitespace and clips an expression to a length that reads in @@ -239,50 +241,34 @@ func unquote(s string) (string, bool) { // selectorArms returns the arm text of a ` ? { ... }` selector. func selectorArms(s string) (string, bool) { - depth := 0 - for j := 0; j < len(s); j++ { - switch c := s[j]; c { - case '\'', '"': - j = skipString(s, j) - 1 - case '(', '[', '{': - depth++ - case ')', ']', '}': - depth-- - case '?': - if depth != 0 { - continue - } - k := j + 1 - for k < len(s) && isSpace(s[k]) { - k++ - } - if k < len(s) && s[k] == '{' { - inner, _, ok := balanced(s, k, '{', '}') - return inner, ok - } + var arms string + var ok bool + walkSource(s, 0, anyDelim, func(j int, c byte, depth int) bool { + if c != '?' || depth != 0 { + return false } - } - return "", false + k := j + 1 + for k < len(s) && isSpace(s[k]) { + k++ + } + if k >= len(s) || s[k] != '{' { + return false + } + arms, _, ok = balanced(s, k, '{', '}') + return true + }) + return arms, ok } // cutRocket splits a selector arm on its top-level `=>`. func cutRocket(s string) (string, string, bool) { - depth := 0 - for j := 0; j < len(s); j++ { - switch c := s[j]; c { - case '\'', '"': - j = skipString(s, j) - 1 - case '(', '[', '{': - depth++ - case ')', ']', '}': - depth-- - case '=': - if depth == 0 && j+1 < len(s) && s[j+1] == '>' { - return s[:j], s[j+2:], true - } - } + end := walkSource(s, 0, anyDelim, func(j int, c byte, depth int) bool { + return c == '=' && depth == 0 && j+1 < len(s) && s[j+1] == '>' + }) + if end >= len(s) { + return "", "", false } - return "", "", false + return s[:end], s[end+2:], true } // assignments maps a variable assigned in this file to the expression it was @@ -305,25 +291,19 @@ func statementExpr(s string, i int) string { for i < len(s) && (s[i] == ' ' || s[i] == '\t') { i++ } - depth := 0 - for j := i; j < len(s); j++ { - switch c := s[j]; { - case c == '\'' || c == '"': - j = skipString(s, j) - 1 - case c == '(' || c == '[' || c == '{': - depth++ - case c == ')' || c == ']' || c == '}': - if depth == 0 { - return s[i:j] - } - depth-- - case c == ';' && depth == 0: - return s[i:j] - case c == '\n' && depth == 0 && !continued(s[i:j]): - return s[i:j] + end := walkSource(s, i, anyDelim, func(j int, c byte, depth int) bool { + if depth != 0 { + return false } - } - return s[i:] + switch c { + case ')', ']', '}', ';': + return true + case '\n': + return !continued(s[i:j]) + } + return false + }) + return s[i:end] } // stripSource blanks out # comments and heredoc bodies. Both are text the @@ -452,82 +432,129 @@ func callArguments(s string, i int) (string, bool) { // splitArgs splits an argument list on its top-level commas. func splitArgs(s string) []string { var out []string - depth, start := 0, 0 - for j := 0; j < len(s); j++ { - switch c := s[j]; c { - case '\'', '"': - j = skipString(s, j) - 1 - case '(', '[', '{': - depth++ - case ')', ']', '}': - depth-- - case ',': - if depth == 0 { - out = append(out, s[start:j]) - start = j + 1 - } + start := 0 + walkSource(s, 0, anyDelim, func(j int, c byte, depth int) bool { + if c == ',' && depth == 0 { + out = append(out, s[start:j]) + start = j + 1 } - } + return false + }) return append(out, s[start:]) } // resourceTitles returns the title expression of a resource body whose opening // brace ends at i, i.e. everything up to the colon that closes the title. func resourceTitles(s string, i int) string { - depth := 0 - for j := i; j < len(s); j++ { - switch c := s[j]; c { - case '\'', '"': - j = skipString(s, j) - 1 - case '(', '[', '{': - depth++ + var out string + walkSource(s, i, anyDelim, func(j int, c byte, depth int) bool { + if depth != 0 { + return false + } + switch c { case ')', ']', '}': - if depth == 0 { - return "" - } - depth-- + return true case ':': - if depth == 0 { - return s[i:j] - } + out = s[i:j] + return true } - } - return "" + return false + }) + return out } -// balanced returns the text between the delimiter at i and its match. +// balanced returns the text between the delimiter at i and its match. Only the +// given pair is counted, so an unmatched brace inside a bracket list cannot +// unbalance it. func balanced(s string, i int, opener, closer byte) (string, int, bool) { + end := walkSource(s, i, pairDelim(opener, closer), func(_ int, c byte, depth int) bool { + return c == closer && depth == 1 + }) + if end >= len(s) { + return "", len(s), false + } + return s[i+1 : end], end + 1, true +} + +// delim reports how a byte changes nesting depth: +1 opening, -1 closing. +type delim func(byte) int + +// anyDelim counts all three bracket pairs, which is what an expression scan +// wants: a comma is top level only if no bracket of any kind is open. +func anyDelim(c byte) int { + switch c { + case '(', '[', '{': + return 1 + case ')', ']', '}': + return -1 + } + return 0 +} + +func pairDelim(opener, closer byte) delim { + return func(c byte) int { + switch c { + case opener: + return 1 + case closer: + return -1 + } + return 0 + } +} + +// walkSource visits the bytes of s from i that lie outside string literals, +// passing each one the nesting depth that applies before it. String literals +// are skipped whole by the one shared scanner, so no caller can desync on a +// quote. It returns the index stop accepted, or len(s). +func walkSource(s string, i int, d delim, stop func(j int, c byte, depth int) bool) int { depth := 0 for j := i; j < len(s); j++ { - switch c := s[j]; c { - case '\'', '"': + c := s[j] + if c == '\'' || c == '"' { j = skipString(s, j) - 1 - case opener: - depth++ - case closer: - depth-- - if depth == 0 { - return s[i+1 : j], j + 1, true - } + continue } + if stop(j, c, depth) { + return j + } + depth += d(c) } - return "", len(s), false + return len(s) } -// skipString returns the index just past the quoted string starting at i. +// skipString returns the index just past the quoted string starting at i. The +// opening delimiter is remembered and only the same delimiter closes the +// string, so a `"` inside a single-quoted substring cannot end a double-quoted +// one. Inside a double-quoted string a `${...}` interpolation is code rather +// than text, so it is scanned as such and may hold strings of either quote. func skipString(s string, i int) int { quote := s[i] for j := i + 1; j < len(s); j++ { - switch s[j] { - case '\\': + switch c := s[j]; { + case c == '\\': j++ - case quote: + case c == quote: return j + 1 + case quote == '"' && c == '$' && j+1 < len(s) && s[j+1] == '{': + j = skipInterpolation(s, j+1) - 1 } } return len(s) } +// skipInterpolation returns the index just past the ${...} expression whose +// opening brace is at i. +func skipInterpolation(s string, i int) int { + end := walkSource(s, i, pairDelim('{', '}'), func(_ int, c byte, depth int) bool { + return c == '}' && depth == 1 + }) + if end >= len(s) { + return len(s) + } + return end + 1 +} + // continued reports whether an argument list so far ends on a comma, meaning // the list carries on over the newline. func continued(s string) bool { diff --git a/internal/impact/scan_test.go b/internal/impact/scan_test.go index 5152917..f0944fa 100644 --- a/internal/impact/scan_test.go +++ b/internal/impact/scan_test.go @@ -1,6 +1,7 @@ package impact import ( + "fmt" "reflect" "testing" ) @@ -222,15 +223,119 @@ func TestScanSurvivesMalformedInput(t *testing.T) { func TestStripSourcePreservesLineCount(t *testing.T) { // The statement scan reads newlines, so stripping must not move them. body := "class p {\n $a = @(EOT)\n body\n EOT\n include foo # bar\n}\n" - stripped := stripSource(body) + if err := sameNewlines(body, stripSource(body)); err != nil { + t.Fatal(err) + } +} + +// sameNewlines is the invariant stripSource owes the statement scanner: every +// byte keeps its offset, and a newline stays a newline and nothing else +// becomes one. +func sameNewlines(body, stripped string) error { if len(stripped) != len(body) { - t.Fatalf("stripped length %d, want %d", len(stripped), len(body)) + return fmt.Errorf("stripSource changed length: %d -> %d", len(body), len(stripped)) } for i := range body { - if body[i] == '\n' && stripped[i] != '\n' { - t.Fatalf("newline at %d was stripped", i) + if (body[i] == '\n') != (stripped[i] == '\n') { + return fmt.Errorf("newline at %d moved: %q -> %q", i, body[i], stripped[i]) } } + return nil +} + +func TestScanQuoteStateIsDelimiterAware(t *testing.T) { + tests := []struct { + name string + body string + want []string + unresolved int + }{ + { + // A single-quoted string inside a ${...} interpolation may hold the + // outer delimiter. Toggling on any quote ends the argument list at + // the `)` inside it and loses everything after it. + name: "double quoted delimiter inside an interpolated single quoted string", + body: "class p {\n include(\"${sprintf('a\"a)b\"c', $x)}\", profile::real)\n}\n", + want: []string{"profile::real"}, + unresolved: 1, + }, + { + name: "single quoted string holding a double quote", + body: "class p {\n include('it\"s', profile::real)\n}\n", + want: []string{"profile::real"}, unresolved: 1, + }, + { + name: "double quoted string holding a single quote", + body: "class p {\n include(\"a'b\", profile::real)\n}\n", + want: []string{"profile::real"}, unresolved: 1, + }, + { + name: "interpolation holding a bracket and a quote", + body: "class p {\n notify { \"${facts['a[b']}\": }\n include profile::real\n}\n", + want: []string{"profile::real"}, + }, + { + name: "nested interpolation", + body: "class p {\n $t = \"${a[\"${b['c}']}\"]}\"\n include profile::real\n}\n", + want: []string{"profile::real"}, + }, + { + name: "escaped quote does not end the string", + body: "class p {\n include(\"a\\\")b\", profile::real)\n}\n", + want: []string{"profile::real"}, unresolved: 1, + }, + { + name: "hash inside an interpolated single quoted string is not a comment", + body: "class p {\n $t = \"${a['#']}\"\n include profile::real\n}\n", + want: []string{"profile::real"}, + }, + { + // The comment scan shares the string skipper, so a `#` inside a + // nested double-quoted string must not blank the rest of the line. + name: "hash inside an interpolated double quoted string is not a comment", + body: "class p {\n $t = \"${a[\"#\"]}\" include profile::real\n}\n", + want: []string{"profile::real"}, + }, + { + name: "selector arm quoted with the other delimiter", + body: "class p {\n include $x ? { \"it's\" => 'profile::a', default => 'profile::b' }\n}\n", + want: []string{"profile::a", "profile::b"}, + }, + { + name: "class resource title list with an awkward quote", + body: "class p {\n class { ['profile::a', \"it's\"]: }\n}\n", + want: []string{"profile::a"}, unresolved: 1, + }, + { + name: "heredoc body holding an unbalanced quote", + body: "class p {\n $a = @(EOT)\n it's ) unbalanced \"\n EOT\n include profile::real\n}\n", + want: []string{"profile::real"}, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if err := sameNewlines(tc.body, stripSource(tc.body)); err != nil { + t.Fatal(err) + } + if got := refs(tc.body); !reflect.DeepEqual(got, tc.want) { + t.Errorf("references = %v, want %v", got, tc.want) + } + if got := unresolvedRefs(tc.body); len(got) != tc.unresolved { + t.Errorf("unresolved = %#v, want %d", got, tc.unresolved) + } + }) + } +} + +func TestUnresolvedPrefixIsLowercased(t *testing.T) { + // Class names are case insensitive and the index holds them lowercase, so + // a mixed-case prefix that is not lowered widens to nothing. + body := "class p {\n Class[\"Mysql::${x}\"] -> Service['s']\n}\n" + got := unresolvedRefs(body) + want := UnresolvedRef{Form: "Class[]", Expr: `"Mysql::${x}"`, Prefix: "mysql::"} + if len(got) != 1 || got[0] != want { + t.Errorf("unresolved = %#v, want [%#v]", got, want) + } } func FuzzScanReferences(f *testing.F) { @@ -239,10 +344,11 @@ func FuzzScanReferences(f *testing.F) { f.Add("class p {\n $a = @(EOT)\n include x::y\n EOT\n}\n") f.Add("class p {\n include \"a::${b}\" # '\n}\n") f.Add("))))]]]}}}@(#'\"") + f.Add("class p {\n include(\"${sprintf('a\"a)b\"c', $x)}\", profile::real)\n}\n") f.Fuzz(func(t *testing.T, body string) { stripped := stripSource(body) - if len(stripped) != len(body) { - t.Fatalf("stripSource changed length: %d -> %d", len(body), len(stripped)) + if err := sameNewlines(body, stripped); err != nil { + t.Fatal(err) } scanReferences(stripped) })