From 86e4958bcb40b177e6e22d4295db7ee3860780f8 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 12 Sep 2026 20:09:52 +1000 Subject: [PATCH] report fact records dropped from the estate table catalog-impact built its PuppetDB client without a Warn hook, so a host whose fact records all failed to decode vanished from the estate table with no warning on any output path. - wire catalog-impact's Warn hook into NodeWarnings - default an unset Warn to stderr so a dropped record is never silent - thread stderr through run() instead of writing os.Stderr directly - count dropped fact records in the explain resolution-incomplete line --- README.md | 7 ++- cmd/catalog-impact/main.go | 29 +++++---- cmd/catalog-impact/main_test.go | 96 ++++++++++++++++++++++++++++-- internal/impact/render.go | 4 +- internal/puppetdb/puppetdb.go | 29 ++++++--- internal/puppetdb/puppetdb_test.go | 46 ++++++++++++++ 6 files changed, 183 insertions(+), 28 deletions(-) diff --git a/README.md b/README.md index b7beb59..1b7695b 100644 --- a/README.md +++ b/README.md @@ -242,9 +242,10 @@ 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 +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. Paths outside the Puppet modulepath — `.woodpecker/`, `doc/`, `spec/`, diff --git a/cmd/catalog-impact/main.go b/cmd/catalog-impact/main.go index bdf4adc..ca43706 100644 --- a/cmd/catalog-impact/main.go +++ b/cmd/catalog-impact/main.go @@ -67,7 +67,7 @@ func main() { SilenceUsage: true, SilenceErrors: true, RunE: func(cmd *cobra.Command, args []string) error { - return run(cmd.Context(), cmd.OutOrStdout(), opts) + return run(cmd.Context(), cmd.OutOrStdout(), cmd.ErrOrStderr(), opts) }, } @@ -89,7 +89,7 @@ func main() { os.Exit(exitOK) } -func run(ctx context.Context, out io.Writer, opts *options) error { +func run(ctx context.Context, out, errOut io.Writer, opts *options) error { if opts.jsonOut && opts.explain { return fmt.Errorf("--json and --explain are mutually exclusive") } @@ -119,12 +119,19 @@ func run(ctx context.Context, out io.Writer, opts *options) error { return fmt.Errorf("indexing %s: %w", opts.repo, err) } - nodes, nodeWarnings, err := impact.LoadNodes(ctx, puppetdb.New(opts.puppetdb, opts.timeout)) + // 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. + 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 } - analyzer := &impact.Analyzer{Index: idx, Nodes: nodes, NodeWarnings: nodeWarnings} + analyzer := &impact.Analyzer{Index: idx, Nodes: nodes, NodeWarnings: append(dropped, nodeWarnings...)} result := analyzer.Analyze(opts.base, branch, files, opts.limit) switch { @@ -134,26 +141,26 @@ func run(ctx context.Context, out io.Writer, opts *options) error { return impact.RenderExplain(out, result) } - warn(result) + warn(errOut, result) return impact.RenderPlain(out, result) } // warn surfaces the widenings and dead ends on stderr so a piped host list // stays clean. -func warn(r *impact.Result) { +func warn(w io.Writer, r *impact.Result) { for _, file := range r.UnmappedFiles { - fmt.Fprintf(os.Stderr, "warning: %s matches no mapping rule; every host is included\n", file) + fmt.Fprintf(w, "warning: %s matches no mapping rule; every host is included\n", file) } 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) + 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(os.Stderr, "warning: %s; the estate table may be missing hosts\n", line) + fmt.Fprintf(w, "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) + fmt.Fprintf(w, "warning: class %s is declared by nothing and listed in no hiera_include; it reaches no hosts\n", class) } if r.Truncated { - fmt.Fprintf(os.Stderr, "warning: %d hosts affected, output capped at %d by --limit\n", r.Summary.Hosts, len(r.Hosts)) + fmt.Fprintf(w, "warning: %d hosts affected, output capped at %d by --limit\n", r.Summary.Hosts, len(r.Hosts)) } } diff --git a/cmd/catalog-impact/main_test.go b/cmd/catalog-impact/main_test.go index 3723b19..d3d398c 100644 --- a/cmd/catalog-impact/main_test.go +++ b/cmd/catalog-impact/main_test.go @@ -3,6 +3,8 @@ package main import ( "bytes" "context" + "encoding/json" + "io" "net/http" "net/http/httptest" "strings" @@ -48,7 +50,7 @@ func TestRunWithExplicitFileList(t *testing.T) { puppetdb: fakePuppetDB(t), timeout: 5 * time.Second, } - if err := run(context.Background(), &out, opts); err != nil { + if err := run(context.Background(), &out, io.Discard, opts); err != nil { t.Fatalf("run: %v", err) } if got, want := out.String(), "web1.example.net\n"; got != want { @@ -66,7 +68,7 @@ func TestRunExplain(t *testing.T) { explain: true, timeout: 5 * time.Second, } - if err := run(context.Background(), &out, opts); err != nil { + if err := run(context.Background(), &out, io.Discard, opts); err != nil { t.Fatalf("run: %v", err) } if !strings.Contains(out.String(), "the whole estate is affected") { @@ -76,14 +78,100 @@ func TestRunExplain(t *testing.T) { func TestRunRejectsConflictingOutputFlags(t *testing.T) { opts := &options{jsonOut: true, explain: true} - if err := run(context.Background(), &bytes.Buffer{}, opts); err == nil { + if err := run(context.Background(), &bytes.Buffer{}, io.Discard, opts); err == nil { t.Fatal("expected an error for --json with --explain") } } func TestRunRejectsNegativeLimit(t *testing.T) { opts := &options{limit: -1} - if err := run(context.Background(), &bytes.Buffer{}, opts); err == nil { + if err := run(context.Background(), &bytes.Buffer{}, io.Discard, opts); err == nil { t.Fatal("expected an error for a negative --limit") } } + +// undecodableEstateBody strips the value from every one of web1's records, so +// each decodes to nothing and the host leaves the estate table with it. +const undecodableEstateBody = `[ + {"certname":"web1.example.net","name":"enc_role"}, + {"certname":"web1.example.net","name":"country"}, + {"certname":"web1.example.net","name":"os"}, + {"certname":"k8s1.example.net","name":"enc_role","value":"roles::infra::k8s::node"}, + {"certname":"k8s1.example.net","name":"country","value":"au"}, + {"certname":"k8s1.example.net","name":"os","value":{"name":"AlmaLinux","release":{"major":"9","minor":"8"}}} +]` + +func fakePuppetDBBody(t *testing.T, body string) string { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(body)) + })) + t.Cleanup(srv.Close) + return srv.URL +} + +func droppedFactOptions(t *testing.T) *options { + t.Helper() + return &options{ + repo: "../../internal/impact/testdata/repo", + base: "origin/develop", + files: []string{"modules/jellyfin/manifests/install.pp"}, + puppetdb: fakePuppetDBBody(t, undecodableEstateBody), + timeout: 5 * time.Second, + } +} + +func TestRunReportsDroppedFactRecordsOnStderr(t *testing.T) { + var out, errOut bytes.Buffer + if err := run(context.Background(), &out, &errOut, droppedFactOptions(t)); err != nil { + t.Fatalf("run: %v", err) + } + if strings.Contains(out.String(), "web1.example.net") { + 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"} { + if !strings.Contains(errOut.String(), want) { + t.Errorf("stderr = %q, want it to mention %q", errOut.String(), want) + } + } +} + +func TestRunReportsDroppedFactRecordsInJSON(t *testing.T) { + var out bytes.Buffer + opts := droppedFactOptions(t) + opts.jsonOut = true + if err := run(context.Background(), &out, io.Discard, opts); err != nil { + t.Fatalf("run: %v", err) + } + + var doc struct { + NodeWarnings []string `json:"node_warnings"` + Summary struct { + ResolutionIncomplete bool `json:"resolution_incomplete"` + } `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 !doc.Summary.ResolutionIncomplete { + t.Error("resolution_incomplete is false after records were dropped") + } +} + +func TestRunReportsDroppedFactRecordsInExplain(t *testing.T) { + var out bytes.Buffer + opts := droppedFactOptions(t) + opts.explain = true + 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)"} { + if !strings.Contains(out.String(), want) { + t.Errorf("explain output = %q, want it to mention %q", out.String(), want) + } + } +} diff --git a/internal/impact/render.go b/internal/impact/render.go index 44aed7e..73c6e31 100644 --- a/internal/impact/render.go +++ b/internal/impact/render.go @@ -63,8 +63,8 @@ func RenderExplain(w io.Writer, r *Result) error { } } if r.Summary.ResolutionIncomplete { - if err := p("resolution incomplete: %d class reference(s) could not be resolved statically", - len(r.UnresolvedRefs)); err != nil { + if err := p("resolution incomplete: %d class reference(s) unresolved, %d fact record(s) dropped", + len(r.UnresolvedRefs), len(r.NodeWarnings)); err != nil { return err } } diff --git a/internal/puppetdb/puppetdb.go b/internal/puppetdb/puppetdb.go index 284fa0d..5c5bfc6 100644 --- a/internal/puppetdb/puppetdb.go +++ b/internal/puppetdb/puppetdb.go @@ -12,6 +12,7 @@ import ( "io" "net/http" "net/url" + "os" "strings" "time" ) @@ -24,8 +25,9 @@ type Client struct { BaseURL string HTTP *http.Client - // Warn, when set, receives a message for every fact record that could not - // be decoded and was therefore dropped. + // 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) } @@ -112,9 +114,9 @@ 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 != nil { - c.Warn(fmt.Sprintf("skipped %d undecodable fact(s) in the estate fact table: %s", - len(skipped), strings.Join(skipped, ", "))) + if len(skipped) > 0 { + c.warn("skipped %d undecodable fact(s) in the estate fact table: %s", + len(skipped), strings.Join(skipped, ", ")) } return out, nil } @@ -244,13 +246,24 @@ func (c *Client) fold(certname string, records []factRecord) (map[string]any, er if len(values) == 0 { return nil, fmt.Errorf("no usable facts for %s: all %d records were undecodable", certname, len(records)) } - if len(skipped) > 0 && c.Warn != nil { - c.Warn(fmt.Sprintf("skipped %d undecodable fact(s) for %s: %s", - len(skipped), certname, strings.Join(skipped, ", "))) + if len(skipped) > 0 { + c.warn("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 +// forgetting to set one loses visibility, never the record itself. +func (c *Client) warn(format string, args ...any) { + msg := fmt.Sprintf(format, args...) + if c.Warn != nil { + c.Warn(msg) + return + } + fmt.Fprintln(os.Stderr, "warning:", msg) +} + func snippet(body []byte) string { s := strings.TrimSpace(string(body)) if len(s) > 200 { diff --git a/internal/puppetdb/puppetdb_test.go b/internal/puppetdb/puppetdb_test.go index 37224c4..d339fc8 100644 --- a/internal/puppetdb/puppetdb_test.go +++ b/internal/puppetdb/puppetdb_test.go @@ -4,9 +4,12 @@ import ( "context" "encoding/json" "errors" + "io" "net/http" "net/http/httptest" + "os" "strings" + "sync" "testing" "time" ) @@ -400,3 +403,46 @@ func TestFactsByNameSkipsUndecodableRecords(t *testing.T) { t.Errorf("the skipped fact must be reported, got %v", warnings) } } + +func TestFactsByNameWithoutWarnHookReportsOnStderr(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`[{"certname":"a.example.net","name":"broken"}]`)) + })) + defer srv.Close() + + stderr := captureStderr(t) + if _, err := New(srv.URL, 5*time.Second).FactsByName(context.Background(), []string{"broken"}); err != nil { + t.Fatalf("FactsByName: %v", err) + } + if got := stderr(); !strings.Contains(got, "broken") { + t.Errorf("an unwired Warn must still report the drop, stderr = %q", got) + } +} + +// captureStderr redirects os.Stderr for the duration of the test and returns a +// func that restores it and yields what was written. +func captureStderr(t *testing.T) func() string { + t.Helper() + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("pipe: %v", err) + } + saved := os.Stderr + os.Stderr = w + + var once sync.Once + var out string + read := func() string { + once.Do(func() { + os.Stderr = saved + _ = w.Close() + b, _ := io.ReadAll(r) + _ = r.Close() + out = string(b) + }) + return out + } + t.Cleanup(func() { read() }) + return read +}