From a76480d4547bef28bf8e5bd1b6a8e56625e9e6a1 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 12 Sep 2026 16:58:59 +1000 Subject: [PATCH] unwrap the catalog response and classify failures correctly - unwrap the {"catalog": ...} envelope, still accepting a bare catalog - report an environment-less response as a protocol mismatch, not an ENC problem - make Resource.Line a pointer and Resource.Params() map absent onto empty - render a missing manifest location as "(source unknown)" - retry compile failures caused by the compiler's PuppetDB query - stop retrying manifest errors the compiler returns as HTTP 500 - add a trimmed real-server response fixture and tests for each shape --- AGENTS.md | 18 +- README.md | 9 +- cmd/catalog-diff/main.go | 12 +- cmd/catalog-diff/main_test.go | 113 ++++++++ internal/catalog/catalog.go | 23 +- internal/catalog/diff.go | 10 +- internal/catalog/diff_test.go | 86 +++++- internal/catalog/report.go | 12 +- internal/catalog/report_test.go | 34 +++ internal/catalog/testdata/report.txt | 6 +- internal/puppetserver/puppetserver.go | 110 ++++++- internal/puppetserver/puppetserver_test.go | 167 ++++++++++- .../testdata/catalog_response.json | 269 ++++++++++++++++++ 13 files changed, 829 insertions(+), 40 deletions(-) create mode 100644 internal/puppetserver/testdata/catalog_response.json diff --git a/AGENTS.md b/AGENTS.md index a5418c1..8da86bf 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -40,9 +40,20 @@ Makefile and to `scripts/build-rpm.sh`, and add its files to ## Gotchas +- The compiler wraps its response as `{"catalog": {...}}`. `decodeCatalog` + unwraps it and still accepts a bare catalog; decoding straight into + `catalog.Catalog` silently yields an empty one. +- Only `type`, `title`, `tags` and `exported` are present on every resource. + `parameters` is absent on class/stage/anchor resources and `file`/`line` on + resources with no manifest location, so `Resource.Line` is a pointer and + `Resource.Params()` maps absent onto empty. - An ENC-supplied environment outranks the requested one. Every compile checks the returned catalog's `environment` and hard-fails on a mismatch — never - relax that check. + relax that check. A returned environment that is empty is a decode failure, + not a mismatch, and is reported as such. +- The compiler returns HTTP 500 for manifest errors too, so retryability is + decided from the error text: a failed PuppetDB query (`server_urls`, + `/pdb/query/`) is transient, a manifest error never is. - Branch names are converted the way g10k does it (`\W` -> `_`, case preserved). The rule lives in one function, `catalog.BranchToEnvironment`. - Exit codes are a contract: 0 same, 1 compile failed, 2 differences, 3 @@ -62,3 +73,8 @@ Everything is stubbed with `httptest` (a TLS server for puppetserver, a plain one for pdbmux) — no live estate access is required. Renderer output is covered by golden files under `internal/catalog/testdata`; regenerate with `go test ./internal/catalog -update`. + +`internal/puppetserver/testdata/catalog_response.json` is a trimmed capture of +a real OpenVoxServer 8.8 response, kept for its shape: the `catalog` envelope +and resources that omit `parameters`, `file` and `line`. Sensitive values are +replaced. diff --git a/README.md b/README.md index db55727..90d7eb3 100644 --- a/README.md +++ b/README.md @@ -36,7 +36,7 @@ catalog-diff web01.example.net --branch benvin/motd-change --testonly | `--pdb` | pdbmux base URL (default `https://pdbmux.k8s.syd1.au.unkin.net`) | | `--cert` / `--key` / `--cacert` | client mTLS material | | `--timeout` | per-compile timeout (default 10m) | -| `--retry` | retries for transient compile or transport failures (default 2) | +| `--retry` | retries for transient compile or transport failures (default 2; a failed compiler-side PuppetDB query counts, a manifest error does not) | | `--include-classes` | diff `Class` resources (ignored by default; they manage nothing) | | `--ignore` | ignore resources matching `Type[Title]` (glob), repeatable | | `--ignore-attr` | ignore a parameter on every resource, repeatable | @@ -101,8 +101,11 @@ or pass `--environment`. `--environment` bypasses the conversion entirely. Classification runs through an exec ENC (`encapic` -> `encapi`), and an ENC-supplied environment outranks the requested one. Every request sets `options.prefer_requested_environment`, and the returned catalog's own -`environment` is always compared against the one requested. A mismatch is a -hard error (exit 3) rather than a silent diff of an environment against itself. +`environment` is always compared against the one requested. A mismatch between +two named environments is a hard error (exit 3) rather than a silent diff of an +environment against itself. A response that decodes without an environment name +at all is reported as a protocol mismatch instead, because that is a client/ +server problem rather than a classification one. ## What a catalog diff cannot see diff --git a/cmd/catalog-diff/main.go b/cmd/catalog-diff/main.go index c689a8a..65857f1 100644 --- a/cmd/catalog-diff/main.go +++ b/cmd/catalog-diff/main.go @@ -325,16 +325,22 @@ func compile(ctx context.Context, server *puppetserver.Client, hostname, environ return nil, &exitError{code: exitTransport, err: lastErr} } -// transient reports whether a failure is worth retrying: a transport error, or -// a server-side 5xx. A manifest that does not compile is not retried. +// transient reports whether a failure is worth retrying: a transport error, a +// server-side 5xx, or a compile the compiler's own PuppetDB query broke. A +// manifest that does not compile, an environment mismatch and an unusable +// response shape are all settled answers and are not retried. func transient(err error) bool { var mismatch *puppetserver.EnvironmentMismatchError if errors.As(err, &mismatch) { return false } + var respErr *puppetserver.ResponseError + if errors.As(err, &respErr) { + return false + } var compileErr *puppetserver.CompileError if errors.As(err, &compileErr) { - return compileErr.Status >= 500 + return compileErr.Transient() } return true } diff --git a/cmd/catalog-diff/main_test.go b/cmd/catalog-diff/main_test.go index a1b0274..e04148a 100644 --- a/cmd/catalog-diff/main_test.go +++ b/cmd/catalog-diff/main_test.go @@ -345,3 +345,116 @@ func TestRunIgnoreFlags(t *testing.T) { t.Errorf("--ignore not applied:\n%s", out.String()) } } + +// wrappedCatalogFor is the shape OpenVoxServer 8.8 actually returns: the +// catalog under a single "catalog" key. +func wrappedCatalogFor(environment, content string) string { + return `{"catalog":` + catalogFor(environment, content) + `}` +} + +func TestRunAgainstWrappedResponses(t *testing.T) { + opts := testServers(t, func(w http.ResponseWriter, r *http.Request) { + if readEnvironment(t, r) == "develop" { + _, _ = w.Write([]byte(wrappedCatalogFor("develop", "old"))) + return + } + _, _ = w.Write([]byte(wrappedCatalogFor("benvin_test", "new"))) + }) + opts.environment = "benvin_test" + + var out bytes.Buffer + err := run(context.Background(), &out, "host.example.net", opts) + if code := exitCode(t, err); code != exitDifferent { + t.Fatalf("exit code %d, want %d (err %v)", code, exitDifferent, err) + } + if !strings.Contains(out.String(), "File[/etc/motd] content: old -> new") { + t.Errorf("wrapped response not diffed:\n%s", out.String()) + } +} + +func TestRunUnknownResponseShapeDoesNotBlameTheENC(t *testing.T) { + var attempts int + var mu sync.Mutex + opts := testServers(t, func(w http.ResponseWriter, _ *http.Request) { + mu.Lock() + attempts++ + mu.Unlock() + _, _ = w.Write([]byte(`{"unexpected":{"environment":"benvin_test"}}`)) + }) + opts.environment = "benvin_test" + opts.testonly = true + opts.retry = 3 + + err := run(context.Background(), &bytes.Buffer{}, "host.example.net", opts) + if code := exitCode(t, err); code != exitTransport { + t.Fatalf("exit %d, want %d (err %v)", code, exitTransport, err) + } + if strings.Contains(err.Error(), "encapi") || strings.Contains(err.Error(), "environment mismatch") { + t.Errorf("a decode failure must not be reported as an ENC problem: %v", err) + } + mu.Lock() + defer mu.Unlock() + if attempts != 1 { + t.Errorf("an unusable response shape must not be retried, saw %d attempts", attempts) + } +} + +const pdbFailureBody = "Internal Server Error: org.jruby.exceptions.RuntimeError: (PreformattedError) Evaluation Error: " + + "Error while evaluating a Resource Statement, Evaluation Error: Error while evaluating a Function Call, " + + "Failed to execute '/pdb/query/v4' on at least 1 of the following 'server_urls': https://puppetdb.example.net " + + "(file: /etc/puppetlabs/code/environments/develop/site/profiles/manifests/consul/client.pp, line: 31, column: 29) " + + "on node host.example.net" + +func TestRunRetriesPuppetDBQueryFailures(t *testing.T) { + var attempts int + var mu sync.Mutex + opts := testServers(t, func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + attempts++ + first := attempts == 1 + mu.Unlock() + if first { + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(pdbFailureBody)) + return + } + _, _ = w.Write([]byte(wrappedCatalogFor(readEnvironment(t, r), "same"))) + }) + opts.environment = "benvin_test" + opts.testonly = true + opts.retry = 2 + + if err := run(context.Background(), &bytes.Buffer{}, "host.example.net", opts); err != nil { + t.Fatalf("a failed PuppetDB query should have been retried: %v", err) + } + mu.Lock() + defer mu.Unlock() + if attempts != 2 { + t.Errorf("expected one retry, saw %d compile attempts", attempts) + } +} + +func TestRunDoesNotRetryManifestErrorsReturnedAs500(t *testing.T) { + var attempts int + var mu sync.Mutex + opts := testServers(t, func(w http.ResponseWriter, _ *http.Request) { + mu.Lock() + attempts++ + mu.Unlock() + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte("Internal Server Error: org.jruby.exceptions.RuntimeError: (Error) " + + "Could not find class roles::infra::incus::node for host.example.net on node host.example.net")) + }) + opts.environment = "benvin_test" + opts.testonly = true + opts.retry = 3 + + if code := exitCode(t, run(context.Background(), &bytes.Buffer{}, "host.example.net", opts)); code != exitCompile { + t.Fatalf("exit %d, want %d", code, exitCompile) + } + mu.Lock() + defer mu.Unlock() + if attempts != 1 { + t.Errorf("a manifest error must not be retried even at HTTP 500, saw %d attempts", attempts) + } +} diff --git a/internal/catalog/catalog.go b/internal/catalog/catalog.go index f60bfa2..7501d5f 100644 --- a/internal/catalog/catalog.go +++ b/internal/catalog/catalog.go @@ -24,7 +24,10 @@ type Catalog struct { Resources []Resource `json:"resources"` } -// Resource is a single catalog resource. +// Resource is a single catalog resource. Only type, title, tags and exported +// are guaranteed: the compiler omits parameters on class, stage and anchor +// resources, and omits file and line on resources with no manifest location. +// Line is a pointer so an omitted line stays distinguishable from line 0. type Resource struct { Type string `json:"type"` Title string `json:"title"` @@ -32,13 +35,25 @@ type Resource struct { Tags []string `json:"tags"` Exported bool `json:"exported"` File string `json:"file"` - Line int `json:"line"` + Line *int `json:"line"` SensitiveParameters []string `json:"sensitive_parameters"` } // Key is the "Type[Title]" identity used to match resources across catalogs. func (r Resource) Key() string { return r.Type + "[" + r.Title + "]" } +// Params returns the resource's parameters, mapping an omitted "parameters" +// key onto an empty set so absent and empty compare and render identically. +func (r Resource) Params() map[string]any { + if r.Parameters == nil { + return map[string]any{} + } + return r.Parameters +} + +// HasSource reports whether the compiler supplied a manifest location. +func (r Resource) HasSource() bool { return r.File != "" && r.Line != nil } + // Options controls normalisation. The zero value is the default behaviour: // classes ignored, exported resources skipped, ordering-only metaparameters // dropped. @@ -102,7 +117,7 @@ func normalizeTitle(r Resource) string { if r.Type != "File" { return r.Title } - if p, ok := r.Parameters["path"].(string); ok && p != "" { + if p, ok := r.Params()["path"].(string); ok && p != "" { return p } return r.Title @@ -119,7 +134,7 @@ func cleanParameters(r Resource, catalogSensitive, ignoreAttrs []string) map[str } params := make(map[string]any, len(r.Parameters)) - for k, v := range r.Parameters { + for k, v := range r.Params() { if noiseParameters[k] || ignored[k] || strings.HasPrefix(k, "_") { continue } diff --git a/internal/catalog/diff.go b/internal/catalog/diff.go index 197c2ea..92e8cae 100644 --- a/internal/catalog/diff.go +++ b/internal/catalog/diff.go @@ -57,9 +57,9 @@ type Change struct { OldValue any `json:"old_value"` NewValue any `json:"new_value"` OldFile string `json:"old_file,omitempty"` - OldLine int `json:"old_line,omitempty"` + OldLine *int `json:"old_line,omitempty"` NewFile string `json:"new_file,omitempty"` - NewLine int `json:"new_line,omitempty"` + NewLine *int `json:"new_line,omitempty"` Severity string `json:"severity"` // Datatype marks a change where only the datatype differs (28 vs "28"). Datatype bool `json:"datatype,omitempty"` @@ -99,7 +99,7 @@ func Diff(base, test map[string]Resource, opts DiffOptions) []Change { DiffType: DiffRemoved, Type: baseRes.Type, Title: baseRes.Title, - OldValue: baseRes.Parameters, + OldValue: baseRes.Params(), OldFile: baseRes.File, OldLine: baseRes.Line, Severity: Severity(baseRes.Type), @@ -114,7 +114,7 @@ func Diff(base, test map[string]Resource, opts DiffOptions) []Change { DiffType: DiffAdded, Type: testRes.Type, Title: testRes.Title, - NewValue: testRes.Parameters, + NewValue: testRes.Params(), NewFile: testRes.File, NewLine: testRes.Line, Severity: Severity(testRes.Type), @@ -161,7 +161,7 @@ func diffParameters(baseRes, testRes Resource, opts DiffOptions) []Change { Severity: Severity(testRes.Type), } return walk(proto, []string{"parameters"}, - anyMap(baseRes.Parameters), anyMap(testRes.Parameters), opts) + anyMap(baseRes.Params()), anyMap(testRes.Params()), opts) } func anyMap(m map[string]any) any { diff --git a/internal/catalog/diff_test.go b/internal/catalog/diff_test.go index 1f45206..30f6f26 100644 --- a/internal/catalog/diff_test.go +++ b/internal/catalog/diff_test.go @@ -60,6 +60,8 @@ func changeSet(t *testing.T) []Change { ) } +func intPtr(n int) *int { return &n } + func byKey(changes []Change) map[string][]Change { out := map[string][]Change{} for _, c := range changes { @@ -82,7 +84,7 @@ func TestNormalizeDropsNoiseParameters(t *testing.T) { if motd.Parameters["content"] != "new" { t.Errorf("real parameters dropped: %+v", motd.Parameters) } - if motd.File != "site.pp" || motd.Line != 19 { + if motd.File != "site.pp" || motd.Line == nil || *motd.Line != 19 { t.Errorf("file/line context lost: %+v", motd) } if got := motd.Tags; len(got) != 3 || got[0] != "a" || got[2] != "z" { @@ -228,9 +230,9 @@ func TestDiffSuppressesDatatypeChanges(t *testing.T) { } func TestDiffIgnoresFileLineMoves(t *testing.T) { - base := map[string]Resource{"File[/a]": {Type: "File", Title: "/a", File: "a.pp", Line: 1, + base := map[string]Resource{"File[/a]": {Type: "File", Title: "/a", File: "a.pp", Line: intPtr(1), Parameters: map[string]any{"ensure": "file"}}} - test := map[string]Resource{"File[/a]": {Type: "File", Title: "/a", File: "b.pp", Line: 99, + test := map[string]Resource{"File[/a]": {Type: "File", Title: "/a", File: "b.pp", Line: intPtr(99), Parameters: map[string]any{"ensure": "file"}}} if changes := Diff(base, test, DiffOptions{}); len(changes) != 0 { t.Errorf("a resource moving file/line is not a change: %+v", changes) @@ -306,3 +308,81 @@ func TestDiffNoChanges(t *testing.T) { t.Errorf("identical catalogs produced changes: %+v", changes) } } + +// Real resources from a live OpenVoxServer 8.8 catalog. Class, stage and +// anchor resources carry no "parameters" key, and resources with no manifest +// location carry neither "file" nor "line". +const optionalFieldCatalog = `{ + "name":"prodnxsr0009.main.unkin.net","environment":"develop","version":"979c188", + "resources":[ + {"type":"Class","title":"Settings","tags":["class","settings"],"exported":false,"kind":"unknown"}, + {"type":"Anchor","title":"selinux::end","tags":["anchor","selinux::end"],"file":"init.pp","line":111,"exported":false,"kind":"compilable_type"}, + {"type":"File","title":"/opt/puppetlabs/puppet/cache/puppet-selinux/modules/frr_local.loaded","tags":["file"],"file":"module.pp","line":177,"exported":false,"kind":"compilable_type"}, + {"type":"Stage","title":"main","tags":["stage"],"exported":false,"kind":"compilable_type","parameters":{"name":"main"}} + ] +}` + +func TestNormalizeAcceptsMissingParameters(t *testing.T) { + res := normalized(t, optionalFieldCatalog, Options{IncludeClasses: true}) + + for _, key := range []string{"Class[Settings]", "Anchor[selinux::end]"} { + r, ok := res[key] + if !ok { + t.Fatalf("%s dropped: %v", key, res) + } + if r.Parameters == nil { + t.Errorf("%s: absent parameters must normalise to an empty map", key) + } + if len(r.Parameters) != 0 { + t.Errorf("%s: unexpected parameters %v", key, r.Parameters) + } + } +} + +func TestDiffTreatsMissingParametersAsEmpty(t *testing.T) { + absent := normalized(t, `{"resources":[{"type":"Anchor","title":"a","tags":["anchor"],"exported":false}]}`, Options{}) + empty := normalized(t, `{"resources":[{"type":"Anchor","title":"a","tags":["anchor"],"exported":false,"parameters":{}}]}`, Options{}) + + if changes := Diff(absent, empty, DiffOptions{}); len(changes) != 0 { + t.Errorf("absent and empty parameters must compare equal: %+v", changes) + } + if changes := Diff(empty, absent, DiffOptions{}); len(changes) != 0 { + t.Errorf("absent and empty parameters must compare equal in both directions: %+v", changes) + } +} + +func TestDiffRemovedResourceWithoutParametersCarriesEmptyValue(t *testing.T) { + base := normalized(t, optionalFieldCatalog, Options{IncludeClasses: true}) + changes := byKey(Diff(base, map[string]Resource{}, DiffOptions{})) + + removed := changes["Class[Settings]"] + if len(removed) != 1 { + t.Fatalf("expected one removal, got %+v", removed) + } + params, ok := removed[0].OldValue.(map[string]any) + if !ok || len(params) != 0 { + t.Errorf("a resource without parameters must report an empty set, got %#v", removed[0].OldValue) + } +} + +func TestNormalizeKeepsMissingSourceDistinctFromLineZero(t *testing.T) { + res := normalized(t, optionalFieldCatalog, Options{IncludeClasses: true}) + + settings := res["Class[Settings]"] + if settings.HasSource() { + t.Errorf("a resource with no file/line must report no source: %+v", settings) + } + if settings.Line != nil { + t.Errorf("an absent line must stay absent, got %d", *settings.Line) + } + + anchor := res["Anchor[selinux::end]"] + if !anchor.HasSource() || anchor.File != "init.pp" || *anchor.Line != 111 { + t.Errorf("manifest location lost: %+v", anchor) + } + + zero := Resource{Type: "File", Title: "/a", Line: intPtr(0)} + if !zero.HasSource() && zero.File != "" { + t.Error("line 0 with a file is a real location") + } +} diff --git a/internal/catalog/report.go b/internal/catalog/report.go index 0808ff7..12020b3 100644 --- a/internal/catalog/report.go +++ b/internal/catalog/report.go @@ -173,11 +173,17 @@ func structurePath(structure []string) string { return strings.Join(structure, ".") } -func location(file string, line int, opts RenderOptions) string { - if !opts.SourceFileLine || file == "" { +// location renders the manifest source of a change. A resource the compiler +// gave no file or line for is reported as unknown rather than as an empty +// path or line 0, which would read as a real location. +func location(file string, line *int, opts RenderOptions) string { + if !opts.SourceFileLine { return "" } - return fmt.Sprintf(" (%s:%d)", file, line) + if file == "" || line == nil { + return " (source unknown)" + } + return fmt.Sprintf(" (%s:%d)", file, *line) } func parameterLines(indent string, value any) []string { diff --git a/internal/catalog/report_test.go b/internal/catalog/report_test.go index 6a62fe3..df449ca 100644 --- a/internal/catalog/report_test.go +++ b/internal/catalog/report_test.go @@ -138,3 +138,37 @@ func TestRenderTextSourceFileLineIsOptional(t *testing.T) { t.Errorf("--display-source-file-line did not print the source:\n%s", located.String()) } } + +func TestRenderTextMarksMissingSourceUnknown(t *testing.T) { + changes := []Change{ + {DiffType: DiffAdded, Type: "Class", Title: "Settings", Severity: SeverityNormal}, + {DiffType: DiffRemoved, Type: "File", Title: "/etc/motd", OldFile: "site.pp", OldLine: intPtr(9), Severity: SeverityCritical}, + } + var buf bytes.Buffer + if err := RenderText(&buf, NewReport("host.example.net", "develop", "benvin_test", changes), RenderOptions{SourceFileLine: true}); err != nil { + t.Fatal(err) + } + out := buf.String() + if !strings.Contains(out, "Class[Settings] added (source unknown)") { + t.Errorf("a resource with no manifest location must render as unknown:\n%s", out) + } + if strings.Contains(out, ":0)") { + t.Errorf("an absent line must never render as line 0:\n%s", out) + } + if !strings.Contains(out, "(site.pp:9)") { + t.Errorf("a real location must still render:\n%s", out) + } +} + +func TestRenderJSONOmitsAbsentSource(t *testing.T) { + changes := []Change{{DiffType: DiffAdded, Type: "Class", Title: "Settings", Severity: SeverityNormal}} + var buf bytes.Buffer + if err := RenderJSON(&buf, NewReport("host.example.net", "develop", "benvin_test", changes)); err != nil { + t.Fatal(err) + } + for _, key := range []string{"new_file", "new_line", "old_file", "old_line"} { + if strings.Contains(buf.String(), key) { + t.Errorf("%s must be omitted when the compiler gave no location:\n%s", key, buf.String()) + } + } +} diff --git a/internal/catalog/testdata/report.txt b/internal/catalog/testdata/report.txt index a3fb8a4..69a5be7 100644 --- a/internal/catalog/testdata/report.txt +++ b/internal/catalog/testdata/report.txt @@ -8,12 +8,12 @@ Changes : 3 critical, 1 added, 1 removed, 4 changed CRITICAL -------------------------------------------------- - ! Exec[reload] environment.PATH: /bin -> /usr/bin + ! Exec[reload] environment.PATH: /bin -> /usr/bin (source unknown) ~ File[/etc/motd] content: old -> new (site.pp:19) - ~ Package[vim] provider: -> dnf + ~ Package[vim] provider: -> dnf (source unknown) OTHER -------------------------------------------------- - ~ Mysql::Db[app] password: redacted:sha256:f52fbd32b2b3b86f -> redacted:sha256:fb8c2e2b85ca81eb + ~ Mysql::Db[app] password: redacted:sha256:f52fbd32b2b3b86f -> redacted:sha256:fb8c2e2b85ca81eb (source unknown) + Notify[added] added (new.pp:3) - Notify[gone] removed (old.pp:1) diff --git a/internal/puppetserver/puppetserver.go b/internal/puppetserver/puppetserver.go index 825d1f1..7a2720c 100644 --- a/internal/puppetserver/puppetserver.go +++ b/internal/puppetserver/puppetserver.go @@ -89,10 +89,50 @@ func (e *CompileError) Error() string { e.Environment, e.Status, msg) } -// EnvironmentMismatchError means the server compiled a different environment -// from the one asked for. Classification runs through an exec ENC (encapic -> -// encapi) whose environment key outranks the requested one, so a mismatch -// means the diff would have compared an environment against itself. +// puppetDBMarkers appear in a compile failure caused by the compiler's own +// PuppetDB query rather than by the manifest. The estate sees these +// intermittently and the same node compiles on the next attempt. +var puppetDBMarkers = []string{ + "server_urls", + "/pdb/query/", + "/pdb/cmd/", +} + +// manifestMarkers appear in a failure the manifest itself causes. Recompiling +// the same code cannot change the outcome. +var manifestMarkers = []string{ + "evaluation error", + "syntax error", + "could not find class", + "could not find declared class", + "could not find data item", + "parse error", + "compilation-error", +} + +// Transient reports whether the failure is worth retrying. A failed PuppetDB +// query is checked first because the compiler reports it as an evaluation +// error, which otherwise reads as a manifest problem. Anything the manifest +// caused is never retried; every other 5xx is. +func (e *CompileError) Transient() bool { + haystack := strings.ToLower(e.Message + " " + e.Kind) + for _, marker := range puppetDBMarkers { + if strings.Contains(haystack, marker) { + return true + } + } + for _, marker := range manifestMarkers { + if strings.Contains(haystack, marker) { + return false + } + } + return e.Status >= 500 +} + +// EnvironmentMismatchError means the server compiled a different, named +// environment from the one asked for. It is only raised when both names are +// non-empty: an unnamed environment is a decode failure, not an ENC decision, +// and is reported as a ResponseError instead. type EnvironmentMismatchError struct { Certname string Requested string @@ -102,12 +142,36 @@ type EnvironmentMismatchError struct { func (e *EnvironmentMismatchError) Error() string { return fmt.Sprintf( "environment mismatch for %s: requested %q but the server compiled %q. "+ - "The ENC (encapic -> encapi) supplies an authoritative environment for every node "+ - "whose encapi status is not \"testing\", and options.prefer_requested_environment did "+ - "not beat it. Set the node's encapi status to \"testing\" or fix the ENC before diffing", + "options.prefer_requested_environment did not override the environment the node "+ + "resolved to; the usual cause is an ENC-supplied environment (encapic -> encapi) "+ + "for a node whose encapi status is not \"testing\"", e.Certname, e.Requested, e.Returned) } +// ResponseError is a 200 response this client could not turn into a catalog: +// it did not decode, or it decoded without an environment name. Both mean the +// response shape is not the one expected, so retrying cannot help. +type ResponseError struct { + Certname string + Size int + Err error +} + +func (e *ResponseError) Error() string { + if e.Err != nil { + return fmt.Sprintf("catalog response for %s did not decode (%d bytes): %v", + e.Certname, e.Size, e.Err) + } + return fmt.Sprintf( + "catalog response for %s decoded without an environment name (%d bytes): "+ + "the compiler returned a response shape this client does not understand. "+ + "This is a client/server protocol mismatch; nothing about the node's "+ + "classification or environment needs changing", + e.Certname, e.Size) +} + +func (e *ResponseError) Unwrap() error { return e.Err } + // CompileRequest describes one catalog compilation. type CompileRequest struct { Certname string @@ -191,9 +255,12 @@ func (c *Client) Compile(ctx context.Context, req CompileRequest) (*catalog.Cata return nil, compileError(req.Environment, resp.StatusCode, raw) } - var cat catalog.Catalog - if err := json.Unmarshal(raw, &cat); err != nil { - return nil, fmt.Errorf("parsing catalog for %s: %w", req.Certname, err) + cat, err := decodeCatalog(raw) + if err != nil { + return nil, &ResponseError{Certname: req.Certname, Size: len(raw), Err: err} + } + if cat.Environment == "" { + return nil, &ResponseError{Certname: req.Certname, Size: len(raw)} } if cat.Environment != req.Environment { return nil, &EnvironmentMismatchError{ @@ -202,7 +269,28 @@ func (c *Client) Compile(ctx context.Context, req CompileRequest) (*catalog.Cata Returned: cat.Environment, } } - return &cat, nil + return cat, nil +} + +// catalogEnvelope is the wrapper OpenVoxServer / Puppet Server 8 returns from +// POST /puppet/v4/catalog: the catalog itself sits under a single "catalog" +// key. +type catalogEnvelope struct { + Catalog *catalog.Catalog `json:"catalog"` +} + +// decodeCatalog accepts both the wrapped response and a bare catalog, so a +// compiler that does not wrap still works. +func decodeCatalog(raw []byte) (*catalog.Catalog, error) { + var env catalogEnvelope + if err := json.Unmarshal(raw, &env); err == nil && env.Catalog != nil { + return env.Catalog, nil + } + var bare catalog.Catalog + if err := json.Unmarshal(raw, &bare); err != nil { + return nil, err + } + return &bare, nil } // Health checks the unauthenticated /status/v1/simple endpoint. diff --git a/internal/puppetserver/puppetserver_test.go b/internal/puppetserver/puppetserver_test.go index 6e82363..f05a4dd 100644 --- a/internal/puppetserver/puppetserver_test.go +++ b/internal/puppetserver/puppetserver_test.go @@ -8,9 +8,13 @@ import ( "errors" "net/http" "net/http/httptest" + "os" + "path/filepath" "strings" "testing" "time" + + "git.unkin.net/unkin/puppet-catalog-tools/internal/catalog" ) const catalogBody = `{ @@ -64,7 +68,7 @@ func TestCompileSuccess(t *testing.T) { if cat.Environment != "benvin_test" || len(cat.Resources) != 2 { t.Fatalf("unexpected catalog: %+v", cat) } - if cat.Resources[1].File == "" || cat.Resources[1].Line != 12 { + if line := cat.Resources[1].Line; cat.Resources[1].File == "" || line == nil || *line != 12 { t.Errorf("resource context lost: %+v", cat.Resources[1]) } @@ -183,10 +187,12 @@ func TestCompileBadCatalogJSON(t *testing.T) { })) defer srv.Close() - if _, err := tlsClient(t, srv).Compile(context.Background(), CompileRequest{ + _, err := tlsClient(t, srv).Compile(context.Background(), CompileRequest{ Certname: "host.example.net", Environment: "benvin_test", - }); err == nil { - t.Fatal("expected a parse error") + }) + var re *ResponseError + if !errors.As(err, &re) { + t.Fatalf("err = %v (%T), want *ResponseError", err, err) } } @@ -263,3 +269,156 @@ func TestTransactionUUIDUnique(t *testing.T) { t.Errorf("bad uuids %q %q", a, b) } } + +// wrappedResponse is a trimmed capture of a real OpenVoxServer 8.8 response: +// the catalog sits under a single "catalog" key. +func wrappedResponse(t *testing.T) []byte { + t.Helper() + raw, err := os.ReadFile(filepath.Join("testdata", "catalog_response.json")) + if err != nil { + t.Fatal(err) + } + return raw +} + +func compileFixture(t *testing.T, body []byte, environment string) (*catalog.Catalog, error) { + t.Helper() + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write(body) + })) + defer srv.Close() + return tlsClient(t, srv).Compile(context.Background(), CompileRequest{ + Certname: "prodnxsr0009.main.unkin.net", Environment: environment, + }) +} + +func TestCompileUnwrapsServerResponse(t *testing.T) { + cat, err := compileFixture(t, wrappedResponse(t), "develop") + if err != nil { + t.Fatalf("Compile: %v", err) + } + if cat.Environment != "develop" || cat.Name != "prodnxsr0009.main.unkin.net" { + t.Fatalf("envelope not unwrapped: %+v", cat) + } + if len(cat.Resources) != 9 || len(cat.Classes) == 0 || len(cat.Tags) == 0 { + t.Fatalf("catalog body lost: %d resources, %d classes, %d tags", + len(cat.Resources), len(cat.Classes), len(cat.Tags)) + } + if string(cat.Version) != `"979c188c3437099863e0b53c57942ef7ed6523a3"` { + t.Errorf("version = %s, want the string form the server sends", cat.Version) + } +} + +func TestCompileUnwrapsBareCatalog(t *testing.T) { + var wrapped struct { + Catalog json.RawMessage `json:"catalog"` + } + if err := json.Unmarshal(wrappedResponse(t), &wrapped); err != nil { + t.Fatal(err) + } + cat, err := compileFixture(t, wrapped.Catalog, "develop") + if err != nil { + t.Fatalf("an unwrapped catalog must still decode: %v", err) + } + if cat.Environment != "develop" || len(cat.Resources) != 9 { + t.Fatalf("unexpected catalog: %+v", cat) + } +} + +func TestCompileDecodesOptionalResourceFields(t *testing.T) { + cat, err := compileFixture(t, wrappedResponse(t), "develop") + if err != nil { + t.Fatal(err) + } + byKey := map[string]catalog.Resource{} + for _, r := range cat.Resources { + byKey[r.Key()] = r + } + + settings, ok := byKey["Class[Settings]"] + if !ok { + t.Fatalf("fixture lost Class[Settings]: %v", byKey) + } + if settings.Parameters != nil || len(settings.Params()) != 0 { + t.Errorf("a resource without parameters must read as an empty set: %+v", settings) + } + if settings.HasSource() || settings.Line != nil { + t.Errorf("a resource without file/line must report no source: %+v", settings) + } + + anchor := byKey["Anchor[selinux::end]"] + if anchor.Parameters != nil { + t.Errorf("anchor parameters should be absent: %+v", anchor.Parameters) + } + if !anchor.HasSource() || *anchor.Line != 111 { + t.Errorf("anchor manifest location lost: %+v", anchor) + } + + pkg := byKey["Package[openvox-agent]"] + if pkg.Params()["ensure"] != "7.37.2" || !pkg.HasSource() { + t.Errorf("package decoded wrong: %+v", pkg) + } + + key := byKey["File[/etc/dns-updater/key]"] + if len(key.SensitiveParameters) != 1 || key.SensitiveParameters[0] != "content" { + t.Errorf("per-resource sensitive_parameters lost: %+v", key) + } + + member := byKey["Haproxy::Balancermember[prodnxsr0009.main.unkin.net_9443]"] + if !member.Exported { + t.Errorf("exported flag lost: %+v", member) + } +} + +func TestCompileEmptyEnvironmentIsAProtocolError(t *testing.T) { + _, err := compileFixture(t, []byte(`{"unexpected":{"environment":"develop"}}`), "develop") + + var re *ResponseError + if !errors.As(err, &re) { + t.Fatalf("err = %v (%T), want *ResponseError", err, err) + } + var me *EnvironmentMismatchError + if errors.As(err, &me) { + t.Fatal("a response with no environment must not be reported as a mismatch") + } + msg := err.Error() + if !strings.Contains(msg, "protocol mismatch") { + t.Errorf("message must name the real cause: %s", msg) + } + for _, forbidden := range []string{"encapi", "ENC", "testing"} { + if strings.Contains(msg, forbidden) { + t.Errorf("a decode failure must not blame %q: %s", forbidden, msg) + } + } +} + +func TestCompileErrorTransientClassification(t *testing.T) { + const pdbFailure = "Internal Server Error: org.jruby.exceptions.RuntimeError: (PreformattedError) Evaluation Error: " + + "Error while evaluating a Resource Statement, Evaluation Error: Error while evaluating a Function Call, " + + "Failed to execute '/pdb/query/v4' on at least 1 of the following 'server_urls': https://puppetdb.example.net " + + "(file: /etc/puppetlabs/code/environments/develop/site/profiles/manifests/consul/client.pp, line: 31, column: 29) " + + "on node host.example.net" + const missingClass = "Internal Server Error: org.jruby.exceptions.RuntimeError: (Error) " + + "Could not find class roles::infra::incus::node for host.example.net on node host.example.net" + + tests := []struct { + name string + err CompileError + wantRun bool + }{ + {name: "puppetdb query failure", err: CompileError{Status: 500, Message: pdbFailure}, wantRun: true}, + {name: "missing class", err: CompileError{Status: 500, Message: missingClass}}, + {name: "manifest error at 500", err: CompileError{Status: 500, Message: "Evaluation Error: undefined method `escape' for URI:Module"}}, + {name: "manifest error at 400", err: CompileError{Status: 400, Kind: "puppet.server/compilation-error", Message: "Syntax error at '}'"}}, + {name: "bad gateway", err: CompileError{Status: 502, Message: "bad gateway"}, wantRun: true}, + {name: "service unavailable", err: CompileError{Status: 503, Message: ""}, wantRun: true}, + {name: "bad request", err: CompileError{Status: 400, Message: "no message returned"}}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := tc.err.Transient(); got != tc.wantRun { + t.Errorf("Transient() = %v, want %v", got, tc.wantRun) + } + }) + } +} diff --git a/internal/puppetserver/testdata/catalog_response.json b/internal/puppetserver/testdata/catalog_response.json new file mode 100644 index 0000000..3a7d82b --- /dev/null +++ b/internal/puppetserver/testdata/catalog_response.json @@ -0,0 +1,269 @@ +{ + "catalog": { + "tags": [ + "settings", + "roles::infra::incus::node" + ], + "name": "prodnxsr0009.main.unkin.net", + "version": "979c188c3437099863e0b53c57942ef7ed6523a3", + "code_id": null, + "catalog_uuid": "8d7f423e-5bf1-4b64-97b4-4a20c67ec59f", + "catalog_format": 2, + "environment": "develop", + "resources": [ + { + "type": "Stage", + "title": "main", + "tags": [ + "stage" + ], + "exported": false, + "kind": "compilable_type", + "parameters": { + "name": "main" + } + }, + { + "type": "Class", + "title": "Settings", + "tags": [ + "class", + "settings" + ], + "exported": false, + "kind": "unknown" + }, + { + "type": "Class", + "title": "Profiles::Puppet::Agent", + "tags": [ + "class", + "profiles::puppet::agent", + "profiles", + "puppet", + "agent", + "profiles::base", + "base", + "roles::infra::incus::node", + "roles", + "infra", + "incus", + "node" + ], + "exported": false, + "kind": "unknown", + "parameters": { + "version": "7.37.2", + "openvox_enable": true + } + }, + { + "type": "Anchor", + "title": "selinux::end", + "tags": [ + "anchor", + "selinux::end", + "selinux", + "end", + "class", + "selinux::module", + "module", + "frr_local", + "profiles::selinux::frr", + "profiles", + "frr", + "profiles::base", + "base", + "roles::infra::incus::node", + "roles", + "infra", + "incus", + "node" + ], + "file": "/etc/puppetlabs/code/environments/develop/external_modules/selinux/manifests/init.pp", + "line": 111, + "exported": false, + "kind": "compilable_type" + }, + { + "type": "File", + "title": "/opt/puppetlabs/puppet/cache/puppet-selinux/modules/frr_local.loaded", + "tags": [ + "file", + "selinux::module", + "selinux", + "module", + "frr_local", + "class", + "profiles::selinux::frr", + "profiles", + "frr", + "profiles::base", + "base", + "roles::infra::incus::node", + "roles", + "infra", + "incus", + "node" + ], + "file": "/etc/puppetlabs/code/environments/develop/external_modules/selinux/manifests/module.pp", + "line": 177, + "exported": false, + "kind": "compilable_type" + }, + { + "type": "Package", + "title": "openvox-agent", + "tags": [ + "package", + "openvox-agent", + "class", + "profiles::puppet::agent", + "profiles", + "puppet", + "agent", + "profiles::base", + "base", + "roles::infra::incus::node", + "roles", + "infra", + "incus", + "node" + ], + "file": "/etc/puppetlabs/code/environments/develop/site/profiles/manifests/puppet/agent.pp", + "line": 58, + "exported": false, + "kind": "compilable_type", + "parameters": { + "ensure": "7.37.2", + "require": "Yumrepo[openvox]" + } + }, + { + "type": "Service", + "title": "puppet", + "tags": [ + "service", + "puppet", + "class", + "profiles::puppet::agent", + "profiles", + "agent", + "profiles::base", + "base", + "roles::infra::incus::node", + "roles", + "infra", + "incus", + "node" + ], + "file": "/etc/puppetlabs/code/environments/develop/site/profiles/manifests/puppet/agent.pp", + "line": 81, + "exported": false, + "kind": "compilable_type", + "parameters": { + "ensure": "running", + "enable": true, + "hasrestart": true, + "require": "Package[openvox-agent]" + } + }, + { + "type": "File", + "title": "/etc/dns-updater/key", + "tags": [ + "file", + "class", + "profiles::dns::updater", + "profiles", + "dns", + "updater", + "profiles::dns::base", + "base", + "profiles::base", + "roles::infra::incus::node", + "roles", + "infra", + "incus", + "node" + ], + "file": "/etc/puppetlabs/code/environments/develop/site/profiles/manifests/dns/updater.pp", + "line": 97, + "exported": false, + "kind": "compilable_type", + "parameters": { + "ensure": "file", + "owner": "root", + "group": "root", + "mode": "0600", + "show_diff": false, + "content": "REDACTED-IN-FIXTURE" + }, + "sensitive_parameters": [ + "content" + ] + }, + { + "type": "Haproxy::Balancermember", + "title": "prodnxsr0009.main.unkin.net_9443", + "tags": [ + "be_ceph_dashboard_au-syd1-prod", + "haproxy::balancermember", + "haproxy", + "balancermember", + "prodnxsr0009.main.unkin.net_9443", + "profiles::haproxy::balancemember", + "profiles", + "balancemember", + "class", + "profiles::ceph::dashboard", + "ceph", + "dashboard", + "profiles::base", + "base", + "roles::infra::incus::node", + "roles", + "infra", + "incus", + "node" + ], + "file": "/etc/puppetlabs/code/environments/develop/site/profiles/manifests/haproxy/balancemember.pp", + "line": 12, + "exported": true, + "kind": "defined_type", + "parameters": { + "listening_service": "be_ceph_dashboard", + "ports": [ + 9443 + ], + "server_names": "prodnxsr0009", + "ipaddresses": "198.18.23.9", + "options": [ + "cookie prodnxsr0009", + "ssl", + "verify none", + "check", + "inter 2s", + "rise 3", + "fall 2" + ], + "tag": "be_ceph_dashboard_au-syd1-prod" + } + } + ], + "edges": [ + { + "source": "Stage[main]", + "target": "Class[Settings]" + }, + { + "source": "Stage[main]", + "target": "Class[main]" + } + ], + "classes": [ + "settings", + "profiles::puppet::agent" + ] + } +}