diff --git a/README.md b/README.md index 1b7695b..f2c1d9f 100644 --- a/README.md +++ b/README.md @@ -245,8 +245,10 @@ A missed host means an unvalidated catalog, so every ambiguity widens: A fact record that arrives with no certname belongs to no host, and one whose value does not decode carries nothing to fold; either way it cannot enter the estate table, and a host whose records all fail leaves the table with them. Both -are reported in `node_warnings`, in `--explain` and on stderr, and set -`summary.resolution_incomplete` too. +are reported in `node_warnings`, in `--explain` and on stderr, counted in +`summary.dropped_fact_records`, and set `summary.resolution_incomplete` too. If +the drops empty the table altogether the run fails, and the warnings and the +tally go to stderr with the error rather than dying with it. Paths outside the Puppet modulepath — `.woodpecker/`, `doc/`, `spec/`, `vendor/`, `Gemfile`, `*.md` — cannot reach a catalog and are skipped. diff --git a/cmd/catalog-diff/main.go b/cmd/catalog-diff/main.go index c56f85b..85f48d8 100644 --- a/cmd/catalog-diff/main.go +++ b/cmd/catalog-diff/main.go @@ -254,7 +254,7 @@ func run(ctx context.Context, out io.Writer, hostname string, opts *options) err server := puppetserver.New(opts.server, tlsCfg, opts.timeout) facts := puppetdb.New(opts.puppetdb, opts.timeout) - facts.Warn = func(msg string) { fmt.Fprintln(os.Stderr, "warning:", msg) } + facts.Warn = func(msg string, _ int) { fmt.Fprintln(os.Stderr, "warning:", msg) } healthCtx, cancel := context.WithTimeout(ctx, 30*time.Second) defer cancel() diff --git a/cmd/catalog-impact/main.go b/cmd/catalog-impact/main.go index ffc9a4a..c74263a 100644 --- a/cmd/catalog-impact/main.go +++ b/cmd/catalog-impact/main.go @@ -120,18 +120,21 @@ func run(ctx context.Context, out, errOut io.Writer, opts *options) error { } // A dropped fact record can take a whole host out of the estate table, so - // it rides the same NodeWarnings channel as an unattributable one and - // reaches every output path. + // it rides the same NodeWarnings channel as an unattributable one, through + // every output path and through a failed load. facts := puppetdb.New(opts.puppetdb, opts.timeout) - var dropped []string - facts.Warn = func(msg string) { dropped = append(dropped, msg) } - - nodes, nodeWarnings, err := impact.LoadNodes(ctx, facts) - if err != nil { - return err + var dropped []impact.NodeWarning + facts.Warn = func(msg string, records int) { + dropped = append(dropped, impact.NodeWarning{Message: msg, Records: records}) } - analyzer := &impact.Analyzer{Index: idx, Nodes: nodes, NodeWarnings: append(dropped, nodeWarnings...)} + nodes, nodeWarnings, err := impact.LoadNodes(ctx, facts) + warnings := append(dropped, nodeWarnings...) + if err != nil { + return loadFailed(errOut, warnings, err) + } + + analyzer := &impact.Analyzer{Index: idx, Nodes: nodes, NodeWarnings: warnings} result := analyzer.Analyze(opts.base, branch, files, opts.limit) switch { @@ -154,9 +157,7 @@ func warn(w io.Writer, r *impact.Result) { for _, line := range impact.UnresolvedLines(r.UnresolvedRefs) { _, _ = fmt.Fprintf(w, "warning: %s did not resolve to a class name; the host list may be incomplete\n", line) } - for _, line := range r.NodeWarnings { - _, _ = fmt.Fprintf(w, "warning: %s; the estate table may be missing hosts\n", line) - } + warnNodes(w, r.NodeWarnings) for _, class := range r.OrphanClasses { _, _ = fmt.Fprintf(w, "warning: class %s is declared by nothing and listed in no hiera_include; it reaches no hosts\n", class) } @@ -164,3 +165,20 @@ func warn(w io.Writer, r *impact.Result) { _, _ = fmt.Fprintf(w, "warning: %d hosts affected, output capped at %d by --limit\n", r.Summary.Hosts, len(r.Hosts)) } } + +func warnNodes(w io.Writer, lines []string) { + for _, line := range lines { + _, _ = fmt.Fprintf(w, "warning: %s; the estate table may be missing hosts\n", line) + } +} + +// loadFailed keeps the warnings that explain a failed load from dying with it: +// no report is rendered once the run stops here, so they go to stderr in every +// output mode and the tally rides on the error. +func loadFailed(w io.Writer, warnings []impact.NodeWarning, err error) error { + warnNodes(w, impact.NodeWarningMessages(warnings)) + if n := impact.DroppedRecords(warnings); n > 0 { + return fmt.Errorf("%w (%d fact record(s) dropped)", err, n) + } + return err +} diff --git a/cmd/catalog-impact/main_test.go b/cmd/catalog-impact/main_test.go index d3d398c..3231901 100644 --- a/cmd/catalog-impact/main_test.go +++ b/cmd/catalog-impact/main_test.go @@ -127,14 +127,17 @@ func TestRunReportsDroppedFactRecordsOnStderr(t *testing.T) { if err := run(context.Background(), &out, &errOut, droppedFactOptions(t)); err != nil { t.Fatalf("run: %v", err) } - if strings.Contains(out.String(), "web1.example.net") { + if out.String() != "" { t.Fatalf("web1 should have dropped out of the estate table, output = %q", out.String()) } - for _, want := range []string{"undecodable fact(s)", "the estate table may be missing hosts"} { + for _, want := range []string{"skipped 3 undecodable fact(s)", "the estate table may be missing hosts"} { if !strings.Contains(errOut.String(), want) { t.Errorf("stderr = %q, want it to mention %q", errOut.String(), want) } } + if got := strings.Count(errOut.String(), "\n"); got != 1 { + t.Errorf("stderr has %d warning line(s), want 1: %q", got, errOut.String()) + } } func TestRunReportsDroppedFactRecordsInJSON(t *testing.T) { @@ -149,17 +152,21 @@ func TestRunReportsDroppedFactRecordsInJSON(t *testing.T) { NodeWarnings []string `json:"node_warnings"` Summary struct { ResolutionIncomplete bool `json:"resolution_incomplete"` + DroppedFactRecords int `json:"dropped_fact_records"` } `json:"summary"` } if err := json.Unmarshal(out.Bytes(), &doc); err != nil { t.Fatalf("decoding --json output: %v", err) } - if len(doc.NodeWarnings) == 0 { - t.Error("node_warnings is empty; the dropped records went unreported") + if len(doc.NodeWarnings) != 1 { + t.Errorf("node_warnings = %v, want the one batched warning", doc.NodeWarnings) } if !doc.Summary.ResolutionIncomplete { t.Error("resolution_incomplete is false after records were dropped") } + if doc.Summary.DroppedFactRecords != 3 { + t.Errorf("summary.dropped_fact_records = %d, want 3", doc.Summary.DroppedFactRecords) + } } func TestRunReportsDroppedFactRecordsInExplain(t *testing.T) { @@ -169,9 +176,60 @@ func TestRunReportsDroppedFactRecordsInExplain(t *testing.T) { if err := run(context.Background(), &out, io.Discard, opts); err != nil { t.Fatalf("run: %v", err) } - for _, want := range []string{"fact records dropped", "resolution incomplete", "undecodable fact(s)"} { + // web1 dropped three records in one warning, so the headline must count + // records, not messages. + for _, want := range []string{ + "fact records dropped", + "3 fact record(s) dropped", + "skipped 3 undecodable fact(s)", + } { if !strings.Contains(out.String(), want) { t.Errorf("explain output = %q, want it to mention %q", out.String(), want) } } } + +// allUndecodableEstateBody strips the value from every record of every host, +// so the estate table comes back empty and the load fails. +const allUndecodableEstateBody = `[ + {"certname":"web1.example.net","name":"enc_role"}, + {"certname":"web1.example.net","name":"country"}, + {"certname":"k8s1.example.net","name":"enc_role"}, + {"certname":"k8s1.example.net","name":"country"} +]` + +func TestRunReportsDroppedFactRecordsWhenTheLoadFails(t *testing.T) { + for _, tc := range []struct { + name string + jsonOut bool + explain bool + }{ + {name: "plain"}, + {name: "json", jsonOut: true}, + {name: "explain", explain: true}, + } { + t.Run(tc.name, func(t *testing.T) { + var out, errOut bytes.Buffer + opts := &options{ + repo: "../../internal/impact/testdata/repo", + base: "origin/develop", + files: []string{"modules/jellyfin/manifests/install.pp"}, + puppetdb: fakePuppetDBBody(t, allUndecodableEstateBody), + jsonOut: tc.jsonOut, + explain: tc.explain, + timeout: 5 * time.Second, + } + + err := run(context.Background(), &out, &errOut, opts) + if err == nil { + t.Fatal("an empty estate table must fail the run") + } + if !strings.Contains(err.Error(), "4 fact record(s) dropped") { + t.Errorf("error = %q, want it to account for the 4 dropped records", err) + } + if !strings.Contains(errOut.String(), "skipped 4 undecodable fact(s)") { + t.Errorf("stderr = %q, want the dropped records reported", errOut.String()) + } + }) + } +} diff --git a/internal/impact/impact.go b/internal/impact/impact.go index 95ed0bb..8bc3179 100644 --- a/internal/impact/impact.go +++ b/internal/impact/impact.go @@ -27,6 +27,9 @@ type Summary struct { // 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"` + // DroppedFactRecords counts the fact records that never reached the estate + // table, which is not the number of warnings: one warning can cover many. + DroppedFactRecords int `json:"dropped_fact_records"` } // Result is the whole analysis, and the --json document. @@ -73,7 +76,7 @@ type Analyzer struct { Index *Index Nodes []Node // NodeWarnings carries whatever LoadNodes could not fold into Nodes. - NodeWarnings []string + NodeWarnings []NodeWarning } // Analyze maps changed files onto hosts. Files are processed in the order @@ -91,7 +94,7 @@ func (a *Analyzer) Analyze(base, branch string, files []string, limit int) *Resu Base: base, Branch: branch, ChangedFiles: uniqueSorted(files), - NodeWarnings: a.NodeWarnings, + NodeWarnings: NodeWarningMessages(a.NodeWarnings), TotalHosts: len(a.Nodes), } @@ -162,6 +165,7 @@ func (a *Analyzer) Analyze(base, branch string, files []string, limit int) *Resu Files: len(result.ChangedFiles), AllHostsTriggered: len(certnames) > 0 && len(certnames) == len(a.Nodes), ResolutionIncomplete: len(result.UnresolvedRefs) > 0 || len(result.NodeWarnings) > 0, + DroppedFactRecords: DroppedRecords(a.NodeWarnings), } if limit > 0 && len(certnames) > limit { diff --git a/internal/impact/impact_test.go b/internal/impact/impact_test.go index 8685f4d..cd1ebe5 100644 --- a/internal/impact/impact_test.go +++ b/internal/impact/impact_test.go @@ -275,9 +275,9 @@ func TestLoadNodesReportsRecordsWithNoCertname(t *testing.T) { 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`, + want := []NodeWarning{ + {Message: `fact "country": 1 record(s) had no certname and were dropped`, Records: 1}, + {Message: `fact "enc_role": 2 record(s) had no certname and were dropped`, Records: 2}, } if !reflect.DeepEqual(warnings, want) { t.Errorf("warnings = %v, want %v", warnings, want) @@ -285,12 +285,36 @@ func TestLoadNodesReportsRecordsWithNoCertname(t *testing.T) { 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 !reflect.DeepEqual(r.NodeWarnings, NodeWarningMessages(want)) { + t.Errorf("Result.NodeWarnings = %v, want %v", r.NodeWarnings, NodeWarningMessages(want)) } if !r.Summary.ResolutionIncomplete { t.Error("a dropped fact record left the summary looking exact") } + // Three records in two messages: the summary counts records. + if r.Summary.DroppedFactRecords != 3 { + t.Errorf("Summary.DroppedFactRecords = %d, want 3", r.Summary.DroppedFactRecords) + } +} + +func TestLoadNodesReturnsWarningsWithItsError(t *testing.T) { + // Every record is unattributable, so the table is empty and the load + // fails. The warnings are the only explanation and must survive the error. + src := &stubFacts{records: []puppetdb.FactRecord{ + {Name: "enc_role", Value: "roles::base"}, + {Name: "enc_role", Value: "roles::web"}, + }} + + nodes, warnings, err := LoadNodes(context.Background(), src) + if err == nil { + t.Fatal("expected an error for an estate table with no attributable records") + } + if len(nodes) != 0 { + t.Fatalf("nodes = %#v", nodes) + } + if got := DroppedRecords(warnings); got != 2 { + t.Errorf("warnings account for %d dropped record(s), want 2", got) + } } func TestAnalyzeReportsFilesThatReachNoHost(t *testing.T) { diff --git a/internal/impact/node.go b/internal/impact/node.go index 7d68073..389e289 100644 --- a/internal/impact/node.go +++ b/internal/impact/node.go @@ -56,11 +56,40 @@ type FactSource interface { FactsByName(ctx context.Context, names []string) ([]puppetdb.FactRecord, error) } +// NodeWarning is one report of fact records that never reached the estate +// table, with the number of records it accounts for. One warning can stand for +// many records, so the count travels with the message rather than being +// inferred from it. +type NodeWarning struct { + Message string + Records int +} + +// NodeWarningMessages renders warnings one line each. +func NodeWarningMessages(warnings []NodeWarning) []string { + out := make([]string, 0, len(warnings)) + for _, w := range warnings { + out = append(out, w.Message) + } + return out +} + +// DroppedRecords totals the records the warnings account for. +func DroppedRecords(warnings []NodeWarning) int { + total := 0 + for _, w := range warnings { + total += w.Records + } + return total +} + // 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. 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) { +// Warnings are returned alongside a failure too: they are usually what +// explains it. +func LoadNodes(ctx context.Context, src FactSource) ([]Node, []NodeWarning, error) { records, err := src.FactsByName(ctx, FactNames) if err != nil { return nil, nil, err @@ -93,15 +122,18 @@ func LoadNodes(ctx context.Context, src FactSource) ([]Node, []string, error) { } // anonymousWarnings renders the dropped-record tally one line per fact. -func anonymousWarnings(counts map[string]int) []string { +func anonymousWarnings(counts map[string]int) []NodeWarning { names := make([]string, 0, len(counts)) for name := range counts { names = append(names, name) } sort.Strings(names) - out := make([]string, 0, len(names)) + out := make([]NodeWarning, 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])) + out = append(out, NodeWarning{ + Message: fmt.Sprintf("fact %q: %d record(s) had no certname and were dropped", name, counts[name]), + Records: counts[name], + }) } return out } diff --git a/internal/impact/render.go b/internal/impact/render.go index 73c6e31..332ca83 100644 --- a/internal/impact/render.go +++ b/internal/impact/render.go @@ -64,7 +64,7 @@ func RenderExplain(w io.Writer, r *Result) error { } if r.Summary.ResolutionIncomplete { if err := p("resolution incomplete: %d class reference(s) unresolved, %d fact record(s) dropped", - len(r.UnresolvedRefs), len(r.NodeWarnings)); err != nil { + len(r.UnresolvedRefs), r.Summary.DroppedFactRecords); err != nil { return err } } diff --git a/internal/puppetdb/puppetdb.go b/internal/puppetdb/puppetdb.go index 5c5bfc6..6cae2d5 100644 --- a/internal/puppetdb/puppetdb.go +++ b/internal/puppetdb/puppetdb.go @@ -25,10 +25,13 @@ type Client struct { BaseURL string HTTP *http.Client - // Warn receives a message for every fact record that could not be decoded - // and was therefore dropped. A nil Warn reports to stderr rather than - // discarding: a dropped record must never be silent. - Warn func(string) + // Warn receives one message per batch of dropped fact records, with the + // number of records that batch accounts for — batches differ in size, so + // counting messages is not counting records. A nil Warn reports to stderr + // rather than discarding: a dropped record must never be silent. It is + // called on the goroutine that made the request, so a Client shared across + // goroutines needs a Warn that is safe to call concurrently. + Warn func(msg string, records int) } // New returns a client for baseURL. @@ -115,7 +118,7 @@ func (c *Client) FactsByName(ctx context.Context, names []string) ([]FactRecord, out = append(out, FactRecord{Certname: r.Certname, Name: r.Name, Value: v}) } if len(skipped) > 0 { - c.warn("skipped %d undecodable fact(s) in the estate fact table: %s", + c.warn(len(skipped), "skipped %d undecodable fact(s) in the estate fact table: %s", len(skipped), strings.Join(skipped, ", ")) } return out, nil @@ -247,18 +250,18 @@ func (c *Client) fold(certname string, records []factRecord) (map[string]any, er return nil, fmt.Errorf("no usable facts for %s: all %d records were undecodable", certname, len(records)) } if len(skipped) > 0 { - c.warn("skipped %d undecodable fact(s) for %s: %s", + c.warn(len(skipped), "skipped %d undecodable fact(s) for %s: %s", len(skipped), certname, strings.Join(skipped, ", ")) } return values, nil } -// warn reports a dropped record. An unwired hook falls back to stderr so that +// warn reports records dropped. An unwired hook falls back to stderr so that // forgetting to set one loses visibility, never the record itself. -func (c *Client) warn(format string, args ...any) { +func (c *Client) warn(records int, format string, args ...any) { msg := fmt.Sprintf(format, args...) if c.Warn != nil { - c.Warn(msg) + c.Warn(msg, records) return } fmt.Fprintln(os.Stderr, "warning:", msg) diff --git a/internal/puppetdb/puppetdb_test.go b/internal/puppetdb/puppetdb_test.go index d339fc8..72714ed 100644 --- a/internal/puppetdb/puppetdb_test.go +++ b/internal/puppetdb/puppetdb_test.go @@ -291,8 +291,12 @@ func TestFactsSkipsUndecodableRecords(t *testing.T) { ]`) var warnings []string + var counted int c := New(srv.URL, 5*time.Second) - c.Warn = func(msg string) { warnings = append(warnings, msg) } + c.Warn = func(msg string, records int) { + warnings = append(warnings, msg) + counted += records + } facts, err := c.Facts(context.Background(), "host.example.net") if err != nil { @@ -305,6 +309,9 @@ func TestFactsSkipsUndecodableRecords(t *testing.T) { if len(warnings) != 1 || !strings.Contains(warnings[0], "broken") { t.Errorf("the skipped fact must be reported, got %v", warnings) } + if counted != 1 { + t.Errorf("Warn reported %d dropped record(s), want 1", counted) + } } func TestFactsWithNothingDecodableIsAnError(t *testing.T) { @@ -383,14 +390,20 @@ func TestFactsByNameSkipsUndecodableRecords(t *testing.T) { w.Header().Set("Content-Type", "application/json") _, _ = w.Write([]byte("\ufeff" + `[ {"certname":"a.example.net","name":"country","value":"au"}, - {"certname":"a.example.net","name":"broken"} + {"certname":"a.example.net","name":"broken"}, + {"certname":"b.example.net","name":"broken"}, + {"certname":"c.example.net","name":"broken"} ]`)) })) defer srv.Close() var warnings []string + var counted int c := New(srv.URL, 5*time.Second) - c.Warn = func(msg string) { warnings = append(warnings, msg) } + c.Warn = func(msg string, records int) { + warnings = append(warnings, msg) + counted += records + } records, err := c.FactsByName(context.Background(), []string{"country", "broken"}) if err != nil { @@ -399,9 +412,14 @@ func TestFactsByNameSkipsUndecodableRecords(t *testing.T) { if len(records) != 1 || records[0].Name != "country" { t.Fatalf("records = %#v", records) } + // One query batches its drops into one message, so the count has to come + // from the hook and not from the number of messages. if len(warnings) != 1 || !strings.Contains(warnings[0], "broken") { t.Errorf("the skipped fact must be reported, got %v", warnings) } + if counted != 3 { + t.Errorf("Warn reported %d dropped record(s), want 3", counted) + } } func TestFactsByNameWithoutWarnHookReportsOnStderr(t *testing.T) {