From 7f2c6f16c88155367053fa2a0d73486bfbf463a7 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 12 Sep 2026 17:19:28 +1000 Subject: [PATCH] accept both factset shapes and skip undecodable facts Real PuppetDB answers /factsets/ with an array; the fallback decoded a bare object, so it only worked against pdbmux's own divergent response. - decode an array of factsets, picking the requested certname - keep accepting a single bare factset object - drop and report a fact record that will not decode instead of failing the whole fetch - fail only when nothing in the response decodes --- internal/pdbmux/pdbmux.go | 70 +++++++++++++++--- internal/pdbmux/pdbmux_test.go | 126 ++++++++++++++++++++++++++++++++- 2 files changed, 184 insertions(+), 12 deletions(-) diff --git a/internal/pdbmux/pdbmux.go b/internal/pdbmux/pdbmux.go index 2952b3a..ef43d6b 100644 --- a/internal/pdbmux/pdbmux.go +++ b/internal/pdbmux/pdbmux.go @@ -4,6 +4,7 @@ package pdbmux import ( + "bytes" "context" "encoding/json" "errors" @@ -22,6 +23,10 @@ var ErrNodeNotFound = errors.New("node not found in pdbmux") 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 func(string) } // New returns a client for baseURL. @@ -38,14 +43,16 @@ type factRecord struct { } type factset struct { - Facts struct { + Certname string `json:"certname"` + Facts struct { Data []factRecord `json:"data"` } `json:"facts"` } // Facts returns the node's facts as the values map a catalog request needs. // It reads /pdb/query/v4/nodes//facts and falls back to -// /pdb/query/v4/factsets/, which returns the same data nested. +// /pdb/query/v4/factsets/, which returns the same data nested in +// either an array of factsets or a single bare factset object. func (c *Client) Facts(ctx context.Context, certname string) (map[string]any, error) { body, status, err := c.get(ctx, "/pdb/query/v4/nodes/"+url.PathEscape(certname)+"/facts") if err != nil { @@ -57,7 +64,7 @@ func (c *Client) Facts(ctx context.Context, certname string) (map[string]any, er return nil, fmt.Errorf("parsing facts for %s: %w", certname, err) } if len(records) > 0 { - return fold(records) + return c.fold(certname, records) } } else if status != http.StatusNotFound { return nil, fmt.Errorf("pdbmux returned %d for %s facts: %s", status, certname, snippet(body)) @@ -74,14 +81,44 @@ func (c *Client) Facts(ctx context.Context, certname string) (map[string]any, er return nil, fmt.Errorf("pdbmux returned %d for %s factset: %s", status, certname, snippet(body)) } - var fs factset - if err := json.Unmarshal(body, &fs); err != nil { + records, err := decodeFactsets(body, certname) + if err != nil { return nil, fmt.Errorf("parsing factset for %s: %w", certname, err) } - if len(fs.Facts.Data) == 0 { + if len(records) == 0 { return nil, fmt.Errorf("%w: %s", ErrNodeNotFound, certname) } - return fold(fs.Facts.Data) + return c.fold(certname, records) +} + +// decodeFactsets reads the two shapes /factsets/ comes back as: real +// PuppetDB returns an array of factsets, pdbmux currently returns a single +// bare object. +func decodeFactsets(body []byte, certname string) ([]factRecord, error) { + if bytes.HasPrefix(bytes.TrimLeft(body, " \t\r\n"), []byte("[")) { + var sets []factset + if err := json.Unmarshal(body, &sets); err != nil { + return nil, err + } + for _, fs := range sets { + if fs.Certname == certname { + return fs.Facts.Data, nil + } + } + if len(sets) == 1 { + return sets[0].Facts.Data, nil + } + if len(sets) == 0 { + return nil, nil + } + return nil, fmt.Errorf("%d factsets returned, none for the requested certname", len(sets)) + } + + var fs factset + if err := json.Unmarshal(body, &fs); err != nil { + return nil, err + } + return fs.Facts.Data, nil } func (c *Client) get(ctx context.Context, path string) ([]byte, int, error) { @@ -105,19 +142,30 @@ func (c *Client) get(ctx context.Context, path string) ([]byte, int, error) { } // fold turns fact records into the flat name -> value map the catalog API -// wants. Structured facts keep their nested shape. -func fold(records []factRecord) (map[string]any, error) { +// wants. Structured facts keep their nested shape. An undecodable record is +// dropped and reported: one bad fact must not cost the whole set. A response +// in which nothing decodes is a failed fetch, not an empty fact set. +func (c *Client) fold(certname string, records []factRecord) (map[string]any, error) { values := make(map[string]any, len(records)) + var skipped []string for _, r := range records { if r.Name == "" { continue } var v any - if err := json.Unmarshal(r.Value, &v); err != nil { - return nil, fmt.Errorf("parsing fact %q: %w", r.Name, err) + if len(r.Value) == 0 || json.Unmarshal(r.Value, &v) != nil { + skipped = append(skipped, r.Name) + continue } values[r.Name] = v } + 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, ", "))) + } return values, nil } diff --git a/internal/pdbmux/pdbmux_test.go b/internal/pdbmux/pdbmux_test.go index adffbce..d8149fe 100644 --- a/internal/pdbmux/pdbmux_test.go +++ b/internal/pdbmux/pdbmux_test.go @@ -5,6 +5,7 @@ import ( "errors" "net/http" "net/http/httptest" + "strings" "testing" "time" ) @@ -15,7 +16,9 @@ const nodeFactsBody = `[ {"certname":"host.example.net","environment":"develop","name":"processorcount","value":4} ]` -const factsetBody = `{ +// The shape real PuppetDB returns: /factsets/ is a query endpoint +// and answers with an array. +const factsetBody = `[{ "certname":"host.example.net", "environment":"develop", "timestamp":"2026-09-12T05:26:11.807Z", @@ -24,6 +27,17 @@ const factsetBody = `{ {"name":"os","value":{"family":"RedHat","release":{"major":"9"}}}, {"name":"processorcount","value":4} ]} +}]` + +// The shape pdbmux itself currently returns for the same route. +const bareFactsetBody = `{ + "certname":"host.example.net", + "environment":"develop", + "facts":{"data":[ + {"name":"id","value":"root"}, + {"name":"os","value":{"family":"RedHat","release":{"major":"9"}}}, + {"name":"processorcount","value":4} + ]} }` func assertFacts(t *testing.T, facts map[string]any) { @@ -157,3 +171,113 @@ func TestFactsContextCancelled(t *testing.T) { t.Fatal("expected a timeout error") } } + +// factsetServer answers the nodes route with a 404 and the factsets route with +// body, which is the path the fallback takes. +func factsetServer(t *testing.T, body string) *httptest.Server { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/pdb/query/v4/factsets/host.example.net" { + _, _ = w.Write([]byte(body)) + return + } + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"error":"No information is known about node host.example.net"}`)) + })) + t.Cleanup(srv.Close) + return srv +} + +func TestFactsFallsBackToBareFactsetObject(t *testing.T) { + srv := factsetServer(t, bareFactsetBody) + + facts, err := New(srv.URL, 5*time.Second).Facts(context.Background(), "host.example.net") + if err != nil { + t.Fatalf("Facts: %v", err) + } + assertFacts(t, facts) +} + +func TestFactsPicksTheRequestedCertnameFromAFactsetArray(t *testing.T) { + srv := factsetServer(t, `[ + {"certname":"other.example.net","facts":{"data":[{"name":"id","value":"nobody"}]}}, + {"certname":"host.example.net","facts":{"data":[ + {"name":"id","value":"root"}, + {"name":"os","value":{"family":"RedHat","release":{"major":"9"}}}, + {"name":"processorcount","value":4} + ]}} + ]`) + + facts, err := New(srv.URL, 5*time.Second).Facts(context.Background(), "host.example.net") + if err != nil { + t.Fatalf("Facts: %v", err) + } + assertFacts(t, facts) +} + +func TestFactsRejectsAFactsetArrayWithNoMatch(t *testing.T) { + srv := factsetServer(t, `[ + {"certname":"a.example.net","facts":{"data":[{"name":"id","value":"a"}]}}, + {"certname":"b.example.net","facts":{"data":[{"name":"id","value":"b"}]}} + ]`) + + _, err := New(srv.URL, 5*time.Second).Facts(context.Background(), "host.example.net") + if err == nil { + t.Fatal("expected an error when no factset matches the certname") + } + if !strings.Contains(err.Error(), "none for the requested certname") { + t.Errorf("unhelpful error: %v", err) + } +} + +func TestFactsEmptyFactsetArrayIsAMissingNode(t *testing.T) { + srv := factsetServer(t, `[]`) + + _, err := New(srv.URL, 5*time.Second).Facts(context.Background(), "host.example.net") + if !errors.Is(err, ErrNodeNotFound) { + t.Fatalf("err = %v, want ErrNodeNotFound", err) + } +} + +func TestFactsSkipsUndecodableRecords(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`[ + {"name":"id","value":"root"}, + {"name":"broken"}, + {"name":"os","value":{"family":"RedHat","release":{"major":"9"}}}, + {"name":"processorcount","value":4} + ]`)) + })) + defer srv.Close() + + var warnings []string + c := New(srv.URL, 5*time.Second) + c.Warn = func(msg string) { warnings = append(warnings, msg) } + + facts, err := c.Facts(context.Background(), "host.example.net") + if err != nil { + t.Fatalf("one bad fact must not fail the fetch: %v", err) + } + assertFacts(t, facts) + if _, ok := facts["broken"]; ok { + t.Error("a valueless fact must be dropped, not stored") + } + if len(warnings) != 1 || !strings.Contains(warnings[0], "broken") { + t.Errorf("the skipped fact must be reported, got %v", warnings) + } +} + +func TestFactsWithNothingDecodableIsAnError(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`[{"name":"a"},{"name":"b"}]`)) + })) + defer srv.Close() + + _, err := New(srv.URL, 5*time.Second).Facts(context.Background(), "host.example.net") + if err == nil { + t.Fatal("a fact set in which nothing decodes must not pass as empty") + } + if !strings.Contains(err.Error(), "no usable facts") { + t.Errorf("unhelpful error: %v", err) + } +}