From 53007a9257915c9f499636878345e70cfc5578ce Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 12 Sep 2026 15:54:25 +1000 Subject: [PATCH 1/9] add repo scaffold and the catalog-diff CLI Compile a host's catalog in a base and a test environment over the puppetserver API, then diff the two. - add catalog-diff CLI with cobra flags, config precedence and exit codes - add puppetserver mTLS client for POST /puppet/v4/catalog - add pdbmux fact client - add catalog normalisation, diff engine and text/JSON renderers - add Makefile, nfpm RPM packaging and woodpecker pipelines --- .gitignore | 5 + .pre-commit-config.yaml | 32 ++ .woodpecker/build.yaml | 18 + .woodpecker/pre-commit.yaml | 18 + .woodpecker/release.yaml | 145 ++++++++ .woodpecker/test.yaml | 33 ++ AGENTS.md | 64 ++++ Makefile | 78 +++++ README.md | 128 ++++++- cmd/catalog-diff/main.go | 366 +++++++++++++++++++++ cmd/catalog-diff/main_test.go | 347 +++++++++++++++++++ go.mod | 13 + go.sum | 13 + internal/catalog/branch.go | 23 ++ internal/catalog/branch_test.go | 48 +++ internal/catalog/catalog.go | 228 +++++++++++++ internal/catalog/diff.go | 273 +++++++++++++++ internal/catalog/diff_test.go | 308 +++++++++++++++++ internal/catalog/report.go | 214 ++++++++++++ internal/catalog/report_test.go | 140 ++++++++ internal/catalog/testdata/report.json | 96 ++++++ internal/catalog/testdata/report.txt | 19 ++ internal/config/config.go | 112 +++++++ internal/config/config_test.go | 91 +++++ internal/pdbmux/pdbmux.go | 130 ++++++++ internal/pdbmux/pdbmux_test.go | 159 +++++++++ internal/puppetserver/puppetserver.go | 279 ++++++++++++++++ internal/puppetserver/puppetserver_test.go | 265 +++++++++++++++ packaging/nfpm.yaml | 45 +++ scripts/build-rpm.sh | 49 +++ 30 files changed, 3738 insertions(+), 1 deletion(-) create mode 100644 .gitignore create mode 100644 .pre-commit-config.yaml create mode 100644 .woodpecker/build.yaml create mode 100644 .woodpecker/pre-commit.yaml create mode 100644 .woodpecker/release.yaml create mode 100644 .woodpecker/test.yaml create mode 100644 AGENTS.md create mode 100644 Makefile create mode 100644 cmd/catalog-diff/main.go create mode 100644 cmd/catalog-diff/main_test.go create mode 100644 go.mod create mode 100644 go.sum create mode 100644 internal/catalog/branch.go create mode 100644 internal/catalog/branch_test.go create mode 100644 internal/catalog/catalog.go create mode 100644 internal/catalog/diff.go create mode 100644 internal/catalog/diff_test.go create mode 100644 internal/catalog/report.go create mode 100644 internal/catalog/report_test.go create mode 100644 internal/catalog/testdata/report.json create mode 100644 internal/catalog/testdata/report.txt create mode 100644 internal/config/config.go create mode 100644 internal/config/config_test.go create mode 100644 internal/pdbmux/pdbmux.go create mode 100644 internal/pdbmux/pdbmux_test.go create mode 100644 internal/puppetserver/puppetserver.go create mode 100644 internal/puppetserver/puppetserver_test.go create mode 100644 packaging/nfpm.yaml create mode 100755 scripts/build-rpm.sh diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..eabcfb3 --- /dev/null +++ b/.gitignore @@ -0,0 +1,5 @@ +# built binaries (repo root only — not the cmd/ source dirs) +/catalog-diff +# cross-compiled release artifacts (e.g. catalog-diff-linux-amd64) +/catalog-diff-* +dist/ diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml new file mode 100644 index 0000000..0dee7b7 --- /dev/null +++ b/.pre-commit-config.yaml @@ -0,0 +1,32 @@ +repos: + - repo: https://github.com/pre-commit/pre-commit-hooks + rev: v4.6.0 + hooks: + - id: trailing-whitespace + - id: end-of-file-fixer + - id: check-yaml + - id: check-merge-conflict + - id: mixed-line-ending + args: [--fix=lf] + + - repo: https://github.com/dnephin/pre-commit-golang + rev: v0.5.1 + hooks: + - id: go-fmt + + # dnephin's go-vet/go-unit-tests hooks resolve packages from a root Go file, + # which this module does not have (every binary lives under cmd/). + - repo: local + hooks: + - id: go-vet + name: go vet + entry: go vet ./... + language: system + types: [go] + pass_filenames: false + - id: go-unit-tests + name: go test + entry: go test ./... + language: system + types: [go] + pass_filenames: false diff --git a/.woodpecker/build.yaml b/.woodpecker/build.yaml new file mode 100644 index 0000000..74123b5 --- /dev/null +++ b/.woodpecker/build.yaml @@ -0,0 +1,18 @@ +when: + - event: pull_request + +steps: + - name: build + image: golang:1.25 + commands: + - make build + backend_options: + kubernetes: + serviceAccountName: default + resources: + requests: + memory: 512Mi + cpu: 1 + limits: + memory: 2Gi + cpu: 2 diff --git a/.woodpecker/pre-commit.yaml b/.woodpecker/pre-commit.yaml new file mode 100644 index 0000000..d57b508 --- /dev/null +++ b/.woodpecker/pre-commit.yaml @@ -0,0 +1,18 @@ +when: + - event: pull_request + +steps: + - name: pre-commit + image: git.unkin.net/unkin/almalinux9-gobuilder:20260606 + commands: + - uvx pre-commit run --all-files + backend_options: + kubernetes: + serviceAccountName: default + resources: + requests: + memory: 512Mi + cpu: 1 + limits: + memory: 2Gi + cpu: 2 diff --git a/.woodpecker/release.yaml b/.woodpecker/release.yaml new file mode 100644 index 0000000..bc9e653 --- /dev/null +++ b/.woodpecker/release.yaml @@ -0,0 +1,145 @@ +when: + - event: tag + +steps: + - name: test + image: golang:1.25 + commands: + - go test -race ./... + backend_options: + kubernetes: + serviceAccountName: default + resources: + requests: + memory: 512Mi + cpu: 1 + limits: + memory: 2Gi + cpu: 2 + + # Build every binary into dist/ (consumed by the RPM step) plus the + # cross-platform binaries attached to the Gitea release. + - name: build + image: git.unkin.net/unkin/almalinux9-gobuilder:20260606 + commands: + - make build VERSION=${CI_COMMIT_TAG} + # Shell variables/expansions are escaped as $$ so Woodpecker leaves them + # for the shell instead of substituting them (as pipeline vars) at parse + # time. ${CI_COMMIT_TAG} is a real Woodpecker var and stays single-$. + - | + for name in catalog-diff; do + for osarch in linux/amd64 linux/arm64 darwin/amd64 darwin/arm64; do + os="$${osarch%/*}"; arch="$${osarch#*/}" + GOOS="$$os" GOARCH="$$arch" \ + go build -ldflags="-s -w -X main.version=${CI_COMMIT_TAG}" \ + -o "$${name}-$${os}-$${arch}" "./cmd/$${name}" + done + done + depends_on: [test] + backend_options: + kubernetes: + serviceAccountName: default + resources: + requests: + memory: 512Mi + cpu: 1 + limits: + memory: 2Gi + cpu: 2 + + # Package the built binaries + generated shell completions into an RPM. + - name: package + image: git.unkin.net/unkin/almalinux9-rpmbuilder:latest + commands: + - ./scripts/build-rpm.sh ${CI_COMMIT_TAG} + depends_on: [build] + backend_options: + kubernetes: + serviceAccountName: default + resources: + requests: + memory: 512Mi + cpu: 1 + limits: + memory: 2Gi + cpu: 2 + + # Publish the RPM to the artifactapi local rpm repo (a real yum repo; + # repodata regenerates automatically). + - name: upload-rpm + image: git.unkin.net/unkin/almalinux9-base:20260606 + commands: + - | + HOST="https://artifactapi.k8s.syd1.au.unkin.net" + REPO="rpm-internal" + for rpm in dist/*.rpm; do + FILE=$$(basename "$$rpm") + # artifactapi has no HEAD route (returns 405); probe with GET against + # the served path (RPMs are stored under Packages/) to avoid re-upload. + code=$$(curl -s -o /dev/null -w '%{http_code}' "$$HOST/api/v2/remotes/$$REPO/files/Packages/$$FILE" || true) + if [ "$$code" = "200" ]; then + echo "$$FILE already exists in $$REPO (HTTP $$code); skipping upload" + continue + fi + echo "Uploading $$FILE to $$REPO (existence probe returned $$code)" + curl -f -X PUT \ + "$$HOST/api/v2/remotes/$$REPO/files/$$FILE" \ + -H "Content-Type: application/x-rpm" \ + --data-binary @"$$rpm" + done + depends_on: [package] + backend_options: + kubernetes: + serviceAccountName: default + resources: + requests: + memory: 128Mi + cpu: 100m + limits: + memory: 512Mi + cpu: 500m + + # Cut a Gitea release with the cross-platform binaries attached. + - name: release + image: git.unkin.net/unkin/almalinux9-base:20260606 + environment: + RELEASER_TOKEN: + from_secret: RELEASER_TOKEN + commands: + - | + curl --output /usr/local/bin/tea https://artifactapi.k8s.syd1.au.unkin.net/api/v1/remote/gitea-dl/tea/0.12.0/tea-0.12.0-linux-amd64 && chmod +x /usr/local/bin/tea + tea logins add --name gitea --url https://git.unkin.net --token "$${RELEASER_TOKEN}" --no-version-check + # Several tags can point at the same commit, so skip tags on the current + # commit and pick the newest semver tag that is a real ancestor. + CUR_SHA=$$(git rev-list -n1 "${CI_COMMIT_TAG}") + PREV_TAG="" + for t in $$(git tag --sort=-v:refname); do + [ "$$t" = "${CI_COMMIT_TAG}" ] && continue + [ "$$(git rev-list -n1 "$$t")" = "$$CUR_SHA" ] && continue + if git merge-base --is-ancestor "$$t" "${CI_COMMIT_TAG}" 2>/dev/null; then + PREV_TAG="$$t"; break + fi + done + if [ -n "$$PREV_TAG" ]; then + NOTES=$$(git log "$${PREV_TAG}..${CI_COMMIT_TAG}" --pretty=format:"- %s") + else + NOTES=$$(git log --pretty=format:"- %s") + fi + tea releases create --tag "${CI_COMMIT_TAG}" --title "${CI_COMMIT_TAG}" --note "$${NOTES}" --login gitea --repo "${CI_REPO}" + RPM=$$(ls dist/*.rpm 2>/dev/null | head -1) + ASSETS="catalog-diff-linux-amd64 catalog-diff-linux-arm64 catalog-diff-darwin-amd64 catalog-diff-darwin-arm64" + [ -n "$$RPM" ] && ASSETS="$$ASSETS $$RPM" + sha256sum $$ASSETS > sha256sums.txt + tea releases assets create "${CI_COMMIT_TAG}" $$ASSETS sha256sums.txt \ + --login gitea --repo "${CI_REPO}" + depends_on: [upload-rpm] + backend_options: + kubernetes: + serviceAccountName: default + resources: + requests: + memory: 128Mi + cpu: 100m + limits: + memory: 512Mi + cpu: 500m diff --git a/.woodpecker/test.yaml b/.woodpecker/test.yaml new file mode 100644 index 0000000..5e179a7 --- /dev/null +++ b/.woodpecker/test.yaml @@ -0,0 +1,33 @@ +when: + - event: pull_request + +steps: + - name: lint + image: golangci/golangci-lint:latest + commands: + - golangci-lint run ./... + backend_options: + kubernetes: + serviceAccountName: default + resources: + requests: + memory: 512Mi + cpu: 1 + limits: + memory: 2Gi + cpu: 2 + + - name: test + image: golang:1.25 + commands: + - go test -v -race ./... + backend_options: + kubernetes: + serviceAccountName: default + resources: + requests: + memory: 512Mi + cpu: 1 + limits: + memory: 2Gi + cpu: 2 diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000..a5418c1 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,64 @@ +# AGENTS.md + +## Project Overview + +Go CLI tooling for the Puppet estate, shipped as one RPM +(`puppet-catalog-tools`). Every tool is a separate `main` package under `cmd/`; +shared code lives in `internal/`. + +- **`catalog-diff`** — compiles a host's catalog twice (base environment and + the environment built from the branch under test) over the puppetserver API + and diffs the result. + +## Structure + +``` +cmd/catalog-diff/ # the CLI (flags, exit codes, orchestration) +internal/catalog/ # catalog types, normalisation, diff engine, renderers, + # branch.go (BranchToEnvironment) +internal/puppetserver/ # mTLS client for POST /puppet/v4/catalog + /status/v1/simple +internal/pdbmux/ # unauthenticated fact client for pdbmux +internal/config/ # XDG config file + env var loading +packaging/nfpm.yaml # RPM spec (envsubst-templated) +scripts/build-rpm.sh # completions + nfpm packaging +.woodpecker/ # CI: build, test, pre-commit (PR) + release (tag) +``` + +Adding a binary: create `cmd//`, add the name to `BINARIES` in the +Makefile and to `scripts/build-rpm.sh`, and add its files to +`packaging/nfpm.yaml`. + +## Transport + +- Catalogs: `POST /puppet/v4/catalog` with `persistence` off, + `options.prefer_requested_environment: true`, facts and synthesised trusted + facts in the body. Auth is client mTLS against the estate Puppet CA; the + certname must be allowed in the puppetserver `auth.conf`. +- Facts: `GET /pdb/query/v4/nodes//facts` on pdbmux, falling back to + `/pdb/query/v4/factsets/`. No auth, no client cert. +- The two services have different auth models and stay separate clients. + +## Gotchas + +- 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. +- 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 + transport/auth/mismatch/usage. `--testonly` never returns 2. +- Normalisation carries a pile of false-positive fixes (classes and exported + resources skipped, `before`/`require` dropped, `notify`/`subscribe` sorted, + File titles keyed by `path`, datatype-only changes suppressed). Removing one + floods the output with noise. + +## Testing + +```bash +make test # go test -v -race ./... +``` + +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`. diff --git a/Makefile b/Makefile new file mode 100644 index 0000000..12715cd --- /dev/null +++ b/Makefile @@ -0,0 +1,78 @@ +# All shipped binaries. Every tool is a separate main package under cmd/, so +# adding one is a matter of adding its name here. +BINARIES := catalog-diff +DIST := dist +VERSION := $(shell git describe --tags --always --dirty 2>/dev/null || echo dev) +GOFLAGS := -ldflags="-s -w -X main.version=$(VERSION)" +OS ?= $(shell go env GOOS) +ARCH ?= $(shell go env GOARCH) + +.PHONY: all build test lint fmt pre-commit clean install completions rpm rpm-package patch minor major _tag + +all: build + +# Build every binary into dist/ so the nfpm packaging step +# (scripts/build-rpm.sh) can find them. Each main package needs its own -o, so +# they are built individually rather than with a single ./... invocation. +build: + @for b in $(BINARIES); do \ + echo "building $$b"; \ + CGO_ENABLED=0 GOOS=$(OS) GOARCH=$(ARCH) go build $(GOFLAGS) -o $(DIST)/$$b ./cmd/$$b || exit 1; \ + done + +test: + go test -v -race ./... + +lint: + golangci-lint run ./... + +fmt: + gofmt -w . + +pre-commit: + pre-commit run --all-files + +clean: + rm -rf $(DIST) $(BINARIES) + +install: + go install $(GOFLAGS) ./... + +# Generate bash/zsh/fish completions for every binary into dist/completions. +completions: build + @mkdir -p $(DIST)/completions + @for b in $(BINARIES); do \ + $(DIST)/$$b completion bash > $(DIST)/completions/$$b.bash; \ + $(DIST)/$$b completion zsh > $(DIST)/completions/_$$b; \ + $(DIST)/$$b completion fish > $(DIST)/completions/$$b.fish; \ + done + +# Build the binaries then package them (with completions) into an RPM via nfpm. +rpm: build rpm-package + +# Package already-built binaries into an RPM (used by CI after the build step). +rpm-package: + ./scripts/build-rpm.sh $(VERSION) + +# Bump helpers — read the latest semver tag and create the next one. +# If no tag exists yet, start from v0.0.0. +_LATEST := $(shell git tag --sort=-v:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+$$' | head -1) +_BASE := $(if $(_LATEST),$(_LATEST),v0.0.0) +_MAJ := $(shell echo $(_BASE) | sed 's/^v//' | cut -d. -f1) +_MIN := $(shell echo $(_BASE) | sed 's/^v//' | cut -d. -f2) +_PAT := $(shell echo $(_BASE) | sed 's/^v//' | cut -d. -f3) + +patch: + @NEW=v$(_MAJ).$(_MIN).$(shell expr $(_PAT) + 1); \ + git tag $$NEW && echo "Tagged $$NEW" && $(MAKE) _tag TAG=$$NEW + +minor: + @NEW=v$(_MAJ).$(shell expr $(_MIN) + 1).0; \ + git tag $$NEW && echo "Tagged $$NEW" && $(MAKE) _tag TAG=$$NEW + +major: + @NEW=v$(shell expr $(_MAJ) + 1).0.0; \ + git tag $$NEW && echo "Tagged $$NEW" && $(MAKE) _tag TAG=$$NEW + +_tag: + git push origin $(TAG) diff --git a/README.md b/README.md index 1781657..db55727 100644 --- a/README.md +++ b/README.md @@ -1,3 +1,129 @@ # puppet-catalog-tools -Go CLIs to diff Puppet catalogs between branches and map puppet-prod changes to affected hosts \ No newline at end of file +CLI tooling for the Puppet estate. Currently one binary: + +- **`catalog-diff`** — compiles a host's catalog in a base environment and in + the environment built from the branch under test, then diffs the two. + +Catalogs are compiled over the puppetserver API (`POST /puppet/v4/catalog`) +with facts read from `pdbmux`. Nothing is compiled locally and nothing is +persisted on the server. + +## Install + +```bash +dnf install puppet-catalog-tools # from the artifactapi rpm-internal repo +# or +make build # -> dist/catalog-diff +``` + +## Usage + +```bash +catalog-diff web01.example.net --branch benvin/motd-change +catalog-diff web01.example.net --environment benvin_motd_change --json +catalog-diff web01.example.net --branch benvin/motd-change --testonly +``` + +| Flag | Description | +|---|---| +| `-b, --branch` | branch under test (required unless `--environment` is given) | +| `--environment` | explicit environment name, bypasses branch name conversion | +| `--base` | base environment to diff against (default `develop`) | +| `--testonly` | compile the test environment only; result via exit code | +| `--json` | machine-readable output | +| `--server` | puppetserver base URL (default `https://puppet.k8s.syd1.au.unkin.net:8140`) | +| `--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) | +| `--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 | +| `--display-datatype-changes` | report changes where only the datatype differs (`28` vs `"28"`) | +| `--display-detail-add` | expand the parameters of added resources | +| `--display-source-file-line` | print the manifest file and line each change came from | + +Both compilations run concurrently, so a diff costs roughly one compile. + +### Exit codes + +| Code | Meaning | +|---|---| +| 0 | catalogs compiled, no differences | +| 1 | catalog compilation failed in one or both environments | +| 2 | catalogs compiled, differences present | +| 3 | transport, auth, environment-mismatch or usage error | + +`--testonly` prints nothing on success and returns 0, 1 or 3 — never 2. + +## Configuration + +Precedence (lowest to highest): defaults < config file < env vars < flags. + +Config file: `$XDG_CONFIG_HOME/puppet-catalog-tools/config.yaml` +(`~/.config/puppet-catalog-tools/config.yaml`). Write a default with +`catalog-diff config init`; print the active values with `catalog-diff config show`. + +```yaml +server_url: https://puppet.k8s.syd1.au.unkin.net:8140 +pdb_url: https://pdbmux.k8s.syd1.au.unkin.net +cert: /etc/puppetlabs/puppet/ssl/certs/catalog-diff.pem +key: /etc/puppetlabs/puppet/ssl/private_keys/catalog-diff.pem +cacert: /etc/puppetlabs/puppet/ssl/certs/ca.pem +base_environment: develop +``` + +| Env var | Config key | +|---|---| +| `CATALOG_DIFF_SERVER` | `server_url` | +| `CATALOG_DIFF_PDB` | `pdb_url` | +| `CATALOG_DIFF_CERT` | `cert` | +| `CATALOG_DIFF_KEY` | `key` | +| `CATALOG_DIFF_CACERT` | `cacert` | +| `CATALOG_DIFF_BASE_ENVIRONMENT` | `base_environment` | + +`catalog-diff` needs a client certificate whose certname is allowed to +`POST /puppet/v4/catalog` in the puppetserver `auth.conf`. pdbmux needs no +credentials. + +## Branch to environment name + +Puppet environment names must match `\A[a-z0-9_]+\Z`, so a branch name is +converted the way g10k deploys it: every character outside `[0-9A-Za-z_]` +becomes `_` (`benvin/foo-bar` -> `benvin_foo_bar`). Case is preserved, because +g10k preserves it — a branch with capitals therefore deploys to a directory +Puppet rejects, and `catalog-diff` fails fast telling you to rename the branch +or pass `--environment`. `--environment` bypasses the conversion entirely. + +## Environment override + +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. + +## What a catalog diff cannot see + +- Facts come from PuppetDB's last report, not a live agent run, so changes to + custom facts are not tested. +- Provider changes and order-of-operations problems (dependency cycles) do not + appear in a catalog. +- Catalogs are fetched over the API rather than compiled from a checkout, so a + change to a static file's content — without its `source` path changing — + produces no readable text diff. + +Puppet catalogs can contain unencrypted secrets. Parameters the catalog marks +as sensitive are replaced with a digest so a change is still visible without +the plaintext reaching stdout or a CI log; anything Puppet does not mark stays +in the clear. + +## Development + +```bash +make build test lint fmt pre-commit +make rpm # build + package into dist/*.rpm via nfpm +``` + +Golden-file tests: `go test ./internal/catalog -update` rewrites them. diff --git a/cmd/catalog-diff/main.go b/cmd/catalog-diff/main.go new file mode 100644 index 0000000..ddcf9f9 --- /dev/null +++ b/cmd/catalog-diff/main.go @@ -0,0 +1,366 @@ +// Command catalog-diff compiles a host's Puppet catalog in two environments +// and reports what changes between them. +package main + +import ( + "context" + "errors" + "fmt" + "io" + "math/rand" + "os" + "sync" + "time" + + "github.com/spf13/cobra" + + "git.unkin.net/unkin/puppet-catalog-tools/internal/catalog" + "git.unkin.net/unkin/puppet-catalog-tools/internal/config" + "git.unkin.net/unkin/puppet-catalog-tools/internal/pdbmux" + "git.unkin.net/unkin/puppet-catalog-tools/internal/puppetserver" +) + +var version = "dev" + +// Exit codes, also documented in --help and the README. +const ( + exitSame = 0 // compiled, no differences + exitCompile = 1 // catalog compilation failed + exitDifferent = 2 // compiled, differences present + exitTransport = 3 // transport, auth, environment-mismatch or usage error +) + +const longHelp = `Compile a host's Puppet catalog in a base environment and in the environment +built from the branch under test, then diff the two. + +Branch names are converted to environment names the way g10k deploys them: +every character outside [0-9A-Za-z_] becomes "_". Use --environment to bypass +that conversion. + +Exit codes: + 0 catalogs compiled, no differences + 1 catalog compilation failed in one or both environments + 2 catalogs compiled, differences present + 3 transport, auth, environment-mismatch or usage error + +--testonly compiles the test environment only and returns 0, 1 or 3 (never 2).` + +type options struct { + branch string + environment string + base string + testonly bool + jsonOut bool + server string + pdb string + cert string + key string + cacert string + timeout time.Duration + retry int + includeClasses bool + ignoreResources []string + ignoreAttrs []string + datatypeChanges bool + detailAdd bool + sourceFileLine bool +} + +// exitError carries the exit code a failure should produce. +type exitError struct { + code int + err error +} + +func (e *exitError) Error() string { return e.err.Error() } +func (e *exitError) Unwrap() error { return e.err } + +func fail(code int, format string, args ...any) *exitError { + return &exitError{code: code, err: fmt.Errorf(format, args...)} +} + +func main() { + cfg, err := config.Load() + if err != nil { + fmt.Fprintln(os.Stderr, "config error:", err) + os.Exit(exitTransport) + } + + opts := &options{} + root := &cobra.Command{ + Use: "catalog-diff ", + Short: "Diff a host's Puppet catalog between two environments", + Long: longHelp, + Args: cobra.ExactArgs(1), + Version: version, + SilenceUsage: true, + SilenceErrors: true, + RunE: func(cmd *cobra.Command, args []string) error { + return run(cmd.Context(), cmd.OutOrStdout(), args[0], opts) + }, + } + + f := root.Flags() + f.StringVarP(&opts.branch, "branch", "b", "", "branch under test (required unless --environment is given)") + f.StringVar(&opts.environment, "environment", "", "explicit puppet environment name, bypasses branch name conversion") + f.StringVar(&opts.base, "base", cfg.BaseEnvironment, "base environment to diff against") + f.BoolVar(&opts.testonly, "testonly", false, "compile the test environment only; no output, result via exit code") + f.BoolVar(&opts.jsonOut, "json", false, "machine-readable output") + f.StringVar(&opts.server, "server", cfg.ServerURL, "puppetserver base URL") + f.StringVar(&opts.pdb, "pdb", cfg.PDBURL, "pdbmux base URL") + f.StringVar(&opts.cert, "cert", cfg.Cert, "client certificate PEM (CATALOG_DIFF_CERT)") + f.StringVar(&opts.key, "key", cfg.Key, "client private key PEM (CATALOG_DIFF_KEY)") + f.StringVar(&opts.cacert, "cacert", cfg.CACert, "CA bundle PEM used to verify puppetserver (CATALOG_DIFF_CACERT)") + f.DurationVar(&opts.timeout, "timeout", 10*time.Minute, "per-compile timeout") + f.IntVar(&opts.retry, "retry", 2, "retries for transient compile or transport failures") + f.BoolVar(&opts.includeClasses, "include-classes", false, "diff Class resources, which manage nothing themselves") + f.StringArrayVar(&opts.ignoreResources, "ignore", nil, "ignore resources matching Type[Title] (glob), repeatable") + f.StringArrayVar(&opts.ignoreAttrs, "ignore-attr", nil, "ignore a parameter on every resource, repeatable") + f.BoolVar(&opts.datatypeChanges, "display-datatype-changes", false, "report changes where only the datatype differs (28 vs \"28\")") + f.BoolVar(&opts.detailAdd, "display-detail-add", false, "expand the parameters of added resources") + f.BoolVar(&opts.sourceFileLine, "display-source-file-line", false, "print the manifest file and line each change came from") + + root.AddCommand(configCmd()) + + if err := root.Execute(); err != nil { + fmt.Fprintln(os.Stderr, "error:", err) + var ee *exitError + if errors.As(err, &ee) { + os.Exit(ee.code) + } + os.Exit(exitTransport) + } +} + +func configCmd() *cobra.Command { + cmd := &cobra.Command{Use: "config", Short: "Manage configuration"} + cmd.AddCommand(&cobra.Command{ + Use: "init", + Short: "Write a default config file to " + config.Path(), + RunE: func(_ *cobra.Command, _ []string) error { + path, err := config.WriteDefault() + if err != nil { + return err + } + fmt.Println("Config written to", path) + return nil + }, + }) + cmd.AddCommand(&cobra.Command{ + Use: "show", + Short: "Print the active configuration", + RunE: func(_ *cobra.Command, _ []string) error { + cfg, err := config.Load() + if err != nil { + return err + } + fmt.Printf("config file : %s\n", config.Path()) + fmt.Printf("server_url : %s\n", cfg.ServerURL) + fmt.Printf("pdb_url : %s\n", cfg.PDBURL) + fmt.Printf("cert : %s\n", cfg.Cert) + fmt.Printf("key : %s\n", cfg.Key) + fmt.Printf("cacert : %s\n", cfg.CACert) + fmt.Printf("base_environment : %s\n", cfg.BaseEnvironment) + return nil + }, + }) + return cmd +} + +// resolveEnvironment turns the flags into the environment to test. +func resolveEnvironment(opts *options) (string, error) { + if opts.environment != "" && opts.branch != "" { + return "", fail(exitTransport, "--branch and --environment are mutually exclusive") + } + if opts.environment != "" { + if !catalog.ValidEnvironmentName(opts.environment) { + return "", fail(exitTransport, "%q is not a valid puppet environment name (must match [a-z0-9_]+)", opts.environment) + } + return opts.environment, nil + } + if opts.branch == "" { + return "", fail(exitTransport, "one of --branch or --environment is required") + } + env := catalog.BranchToEnvironment(opts.branch) + if !catalog.ValidEnvironmentName(env) { + return "", fail(exitTransport, + "branch %q deploys as environment %q, which puppet rejects (environment names must match [a-z0-9_]+). "+ + "Rename the branch or pass --environment", opts.branch, env) + } + return env, nil +} + +func run(ctx context.Context, out io.Writer, hostname string, opts *options) error { + if ctx == nil { + ctx = context.Background() + } + + testEnv, err := resolveEnvironment(opts) + if err != nil { + return err + } + if !catalog.ValidEnvironmentName(opts.base) { + return fail(exitTransport, "%q is not a valid base environment name (must match [a-z0-9_]+)", opts.base) + } + if testEnv == opts.base && !opts.testonly { + return fail(exitTransport, "test and base environments are both %q; there is nothing to diff", testEnv) + } + + tlsCfg, err := puppetserver.TLSConfig(opts.cert, opts.key, opts.cacert) + if err != nil { + return &exitError{code: exitTransport, err: err} + } + + server := puppetserver.New(opts.server, tlsCfg, opts.timeout) + facts := pdbmux.New(opts.pdb, opts.timeout) + + healthCtx, cancel := context.WithTimeout(ctx, 30*time.Second) + defer cancel() + if err := server.Health(healthCtx); err != nil { + return &exitError{code: exitTransport, err: err} + } + + factValues, err := facts.Facts(ctx, hostname) + if err != nil { + return &exitError{code: exitTransport, err: err} + } + + if opts.testonly { + _, err := compile(ctx, server, hostname, testEnv, factValues, opts) + return err + } + + // Both compilations are independent, so run them together: one host costs + // one compile's wall clock rather than two. + var ( + wg sync.WaitGroup + testCat, baseCat *catalog.Catalog + testErr, baseErr error + ) + wg.Add(2) + go func() { + defer wg.Done() + testCat, testErr = compile(ctx, server, hostname, testEnv, factValues, opts) + }() + go func() { + defer wg.Done() + baseCat, baseErr = compile(ctx, server, hostname, opts.base, factValues, opts) + }() + wg.Wait() + + // Report the test environment's failure first: it is the one under review. + if testErr != nil { + return testErr + } + if baseErr != nil { + return baseErr + } + + normOpts := catalog.Options{ + IncludeClasses: opts.includeClasses, + IgnoreResources: opts.ignoreResources, + IgnoreAttrs: opts.ignoreAttrs, + } + changes := catalog.Diff( + catalog.Normalize(baseCat, normOpts), + catalog.Normalize(testCat, normOpts), + catalog.DiffOptions{DisplayDatatypeChanges: opts.datatypeChanges}, + ) + report := catalog.NewReport(hostname, opts.base, testEnv, changes) + + if opts.jsonOut { + err = catalog.RenderJSON(out, report) + } else { + err = catalog.RenderText(out, report, catalog.RenderOptions{ + Colour: isTerminal(os.Stdout), + DetailAdd: opts.detailAdd, + SourceFileLine: opts.sourceFileLine, + }) + } + if err != nil { + return &exitError{code: exitTransport, err: err} + } + if len(changes) > 0 { + return &exitError{code: exitDifferent, err: errSilent} + } + return nil +} + +// errSilent marks an exit code that carries no message of its own: the diff +// itself has already been printed. +var errSilent = errors.New("differences present") + +// compile runs one compilation, retrying transient failures, and maps the +// final failure onto an exit code. +func compile(ctx context.Context, server *puppetserver.Client, hostname, environment string, facts map[string]any, opts *options) (*catalog.Catalog, error) { + var lastErr error + for attempt := 0; attempt <= opts.retry; attempt++ { + if attempt > 0 { + if err := sleepCtx(ctx, backoff(attempt)); err != nil { + break + } + } + cat, err := server.Compile(ctx, puppetserver.CompileRequest{ + Certname: hostname, + Environment: environment, + Facts: facts, + TrustedFacts: puppetserver.TrustedFacts(hostname), + }) + if err == nil { + return cat, nil + } + lastErr = err + if !transient(err) { + break + } + } + + var compileErr *puppetserver.CompileError + if errors.As(lastErr, &compileErr) { + return nil, &exitError{code: exitCompile, err: lastErr} + } + 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. +func transient(err error) bool { + var mismatch *puppetserver.EnvironmentMismatchError + if errors.As(err, &mismatch) { + return false + } + var compileErr *puppetserver.CompileError + if errors.As(err, &compileErr) { + return compileErr.Status >= 500 + } + return true +} + +// backoff grows exponentially from 2s and adds up to 50% jitter, so parallel +// compiles do not retry in lockstep against an autoscaling compiler pool. +func backoff(attempt int) time.Duration { + base := 2 * time.Second << (attempt - 1) + if base > 30*time.Second { + base = 30 * time.Second + } + return base + time.Duration(rand.Int63n(int64(base/2))) +} + +func sleepCtx(ctx context.Context, d time.Duration) error { + timer := time.NewTimer(d) + defer timer.Stop() + select { + case <-ctx.Done(): + return ctx.Err() + case <-timer.C: + return nil + } +} + +func isTerminal(f *os.File) bool { + info, err := f.Stat() + if err != nil { + return false + } + return info.Mode()&os.ModeCharDevice != 0 +} diff --git a/cmd/catalog-diff/main_test.go b/cmd/catalog-diff/main_test.go new file mode 100644 index 0000000..a1b0274 --- /dev/null +++ b/cmd/catalog-diff/main_test.go @@ -0,0 +1,347 @@ +package main + +import ( + "bytes" + "context" + "encoding/json" + "encoding/pem" + "errors" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "sync" + "testing" + "time" +) + +const factsBody = `[ + {"certname":"host.example.net","environment":"develop","name":"os","value":{"family":"RedHat"}} +]` + +func catalogFor(environment, content string) string { + return `{"name":"host.example.net","environment":"` + environment + `","version":1,"classes":["settings"], + "resources":[ + {"type":"File","title":"/etc/motd","file":"site.pp","line":9,"parameters":{"ensure":"file","content":"` + content + `"}} + ]}` +} + +// testServers stands up a fake puppetserver (TLS) and pdbmux (plain), and +// returns options wired to them with the server's own cert as the CA bundle. +func testServers(t *testing.T, compile http.HandlerFunc) *options { + t.Helper() + + puppet := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/status/v1/simple" { + _, _ = w.Write([]byte("running")) + return + } + compile(w, r) + })) + t.Cleanup(puppet.Close) + + pdb := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(factsBody)) + })) + t.Cleanup(pdb.Close) + + caPath := filepath.Join(t.TempDir(), "ca.pem") + block := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: puppet.Certificate().Raw}) + if err := os.WriteFile(caPath, block, 0o600); err != nil { + t.Fatal(err) + } + + return &options{ + base: "develop", + server: puppet.URL, + pdb: pdb.URL, + cacert: caPath, + timeout: 10 * time.Second, + } +} + +func exitCode(t *testing.T, err error) int { + t.Helper() + if err == nil { + return exitSame + } + var ee *exitError + if !errors.As(err, &ee) { + t.Fatalf("error carries no exit code: %v", err) + } + return ee.code +} + +func TestResolveEnvironment(t *testing.T) { + tests := []struct { + name string + opts options + want string + wantCode int + }{ + {name: "branch is sanitised", opts: options{branch: "benvin/foo-bar"}, want: "benvin_foo_bar"}, + {name: "explicit environment wins", opts: options{environment: "develop"}, want: "develop"}, + {name: "neither given", opts: options{}, wantCode: exitTransport}, + {name: "both given", opts: options{branch: "a", environment: "b"}, wantCode: exitTransport}, + {name: "branch with capitals is rejected", opts: options{branch: "benvin/PUP-1"}, wantCode: exitTransport}, + {name: "invalid explicit environment", opts: options{environment: "Not-Valid"}, wantCode: exitTransport}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got, err := resolveEnvironment(&tc.opts) + if tc.wantCode != 0 { + if code := exitCode(t, err); code != tc.wantCode { + t.Fatalf("exit code %d, want %d (err %v)", code, tc.wantCode, err) + } + return + } + if err != nil { + t.Fatalf("resolveEnvironment: %v", err) + } + if got != tc.want { + t.Errorf("= %q, want %q", got, tc.want) + } + }) + } +} + +func TestResolveEnvironmentCapitalsMessage(t *testing.T) { + _, err := resolveEnvironment(&options{branch: "benvin/PUP-1"}) + if err == nil { + t.Fatal("expected an error") + } + msg := err.Error() + for _, want := range []string{"benvin/PUP-1", "benvin_PUP_1", "--environment"} { + if !strings.Contains(msg, want) { + t.Errorf("message missing %q: %s", want, msg) + } + } +} + +func TestRunDiff(t *testing.T) { + opts := testServers(t, func(w http.ResponseWriter, r *http.Request) { + body := readEnvironment(t, r) + if body == "develop" { + _, _ = w.Write([]byte(catalogFor("develop", "old"))) + return + } + _, _ = w.Write([]byte(catalogFor("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("diff not rendered:\n%s", out.String()) + } +} + +func TestRunJSON(t *testing.T) { + opts := testServers(t, func(w http.ResponseWriter, r *http.Request) { + if readEnvironment(t, r) == "develop" { + _, _ = w.Write([]byte(catalogFor("develop", "old"))) + return + } + _, _ = w.Write([]byte(catalogFor("benvin_test", "new"))) + }) + opts.environment = "benvin_test" + opts.jsonOut = true + + 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(), `"base_environment": "develop"`) { + t.Errorf("unexpected json:\n%s", out.String()) + } +} + +func TestRunTestonlyIsSilent(t *testing.T) { + var compiled []string + opts := testServers(t, func(w http.ResponseWriter, r *http.Request) { + env := readEnvironment(t, r) + compiled = append(compiled, env) + _, _ = w.Write([]byte(catalogFor(env, "new"))) + }) + opts.environment = "benvin_test" + opts.testonly = true + + var out bytes.Buffer + if err := run(context.Background(), &out, "host.example.net", opts); err != nil { + t.Fatalf("run: %v", err) + } + if out.Len() != 0 { + t.Errorf("--testonly printed output: %q", out.String()) + } + if len(compiled) != 1 || compiled[0] != "benvin_test" { + t.Errorf("--testonly must compile the test environment only, compiled %v", compiled) + } +} + +func TestRunCompilationFailureExit1(t *testing.T) { + opts := testServers(t, func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(`{"kind":"puppet.server/compilation-error","message":"Server Error: Syntax error at '}'","issue_kind":"PARSE_ERROR"}`)) + }) + opts.environment = "benvin_test" + + err := run(context.Background(), &bytes.Buffer{}, "host.example.net", opts) + if code := exitCode(t, err); code != exitCompile { + t.Fatalf("exit code %d, want %d (err %v)", code, exitCompile, err) + } +} + +func TestRunEnvironmentMismatchExit3(t *testing.T) { + opts := testServers(t, func(w http.ResponseWriter, _ *http.Request) { + // The ENC overrides the requested environment. + _, _ = w.Write([]byte(catalogFor("develop", "new"))) + }) + opts.environment = "benvin_test" + + err := run(context.Background(), &bytes.Buffer{}, "host.example.net", opts) + if code := exitCode(t, err); code != exitTransport { + t.Fatalf("exit code %d, want %d (err %v)", code, exitTransport, err) + } + if !strings.Contains(err.Error(), "environment mismatch") { + t.Errorf("unhelpful mismatch error: %v", err) + } +} + +func TestRunUsageErrors(t *testing.T) { + opts := testServers(t, func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(catalogFor("develop", "old"))) + }) + + if code := exitCode(t, run(context.Background(), &bytes.Buffer{}, "host.example.net", opts)); code != exitTransport { + t.Errorf("missing --branch/--environment: exit %d, want %d", code, exitTransport) + } + + same := *opts + same.environment = "develop" + if code := exitCode(t, run(context.Background(), &bytes.Buffer{}, "host.example.net", &same)); code != exitTransport { + t.Errorf("base == test: exit %d, want %d", code, exitTransport) + } + + badCA := *opts + badCA.environment = "benvin_test" + badCA.cacert = "/nonexistent/ca.pem" + if code := exitCode(t, run(context.Background(), &bytes.Buffer{}, "host.example.net", &badCA)); code != exitTransport { + t.Errorf("unreadable CA bundle: exit %d, want %d", code, exitTransport) + } +} + +func TestRunUnreachableServerExit3(t *testing.T) { + opts := testServers(t, func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(catalogFor("benvin_test", "new"))) + }) + opts.environment = "benvin_test" + opts.server = "https://127.0.0.1:1" + + if code := exitCode(t, run(context.Background(), &bytes.Buffer{}, "host.example.net", opts)); code != exitTransport { + t.Errorf("exit %d, want %d", code, exitTransport) + } +} + +// readEnvironment returns the environment a compile request asked for. +func readEnvironment(t *testing.T, r *http.Request) string { + t.Helper() + var body struct { + Environment string `json:"environment"` + } + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + t.Fatalf("decoding compile request: %v", err) + } + return body.Environment +} + +func TestRunIdenticalCatalogsExit0(t *testing.T) { + opts := testServers(t, func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(catalogFor(readEnvironment(t, r), "same"))) + }) + opts.environment = "benvin_test" + + var out bytes.Buffer + if err := run(context.Background(), &out, "host.example.net", opts); err != nil { + t.Fatalf("identical catalogs must exit 0: %v", err) + } + if !strings.Contains(out.String(), "No differences between environments.") { + t.Errorf("unexpected output:\n%s", out.String()) + } +} + +func TestRunRetriesTransientFailures(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.StatusBadGateway) + _, _ = w.Write([]byte("bad gateway")) + return + } + _, _ = w.Write([]byte(catalogFor(readEnvironment(t, r), "same"))) + }) + opts.environment = "benvin_test" + opts.retry = 2 + + if err := run(context.Background(), &bytes.Buffer{}, "host.example.net", opts); err != nil { + t.Fatalf("a 502 should have been retried: %v", err) + } + mu.Lock() + defer mu.Unlock() + if attempts < 3 { + t.Errorf("expected a retry, saw %d compile attempts", attempts) + } +} + +func TestRunDoesNotRetryCompilationErrors(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.StatusBadRequest) + _, _ = w.Write([]byte(`{"kind":"puppet.server/compilation-error","message":"Syntax error"}`)) + }) + opts.environment = "benvin_test" + opts.retry = 3 + opts.testonly = true + + 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, saw %d attempts", attempts) + } +} + +func TestRunIgnoreFlags(t *testing.T) { + opts := testServers(t, func(w http.ResponseWriter, r *http.Request) { + if readEnvironment(t, r) == "develop" { + _, _ = w.Write([]byte(catalogFor("develop", "old"))) + return + } + _, _ = w.Write([]byte(catalogFor("benvin_test", "new"))) + }) + opts.environment = "benvin_test" + opts.ignoreResources = []string{"File[/etc/*]"} + + var out bytes.Buffer + if err := run(context.Background(), &out, "host.example.net", opts); err != nil { + t.Fatalf("ignored resource should leave no differences: %v", err) + } + if !strings.Contains(out.String(), "No differences") { + t.Errorf("--ignore not applied:\n%s", out.String()) + } +} diff --git a/go.mod b/go.mod new file mode 100644 index 0000000..da738ff --- /dev/null +++ b/go.mod @@ -0,0 +1,13 @@ +module git.unkin.net/unkin/puppet-catalog-tools + +go 1.25.0 + +require ( + github.com/spf13/cobra v1.10.2 + gopkg.in/yaml.v3 v3.0.1 +) + +require ( + github.com/inconshreveable/mousetrap v1.1.0 // indirect + github.com/spf13/pflag v1.0.9 // indirect +) diff --git a/go.sum b/go.sum new file mode 100644 index 0000000..47edb24 --- /dev/null +++ b/go.sum @@ -0,0 +1,13 @@ +github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g= +github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2s0bqwp9tc8= +github.com/inconshreveable/mousetrap v1.1.0/go.mod h1:vpF70FUmC8bwa3OWnCshd2FqLfsEA9PFc4w1p2J65bw= +github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM= +github.com/spf13/cobra v1.10.2 h1:DMTTonx5m65Ic0GOoRY2c16WCbHxOOw6xxezuLaBpcU= +github.com/spf13/cobra v1.10.2/go.mod h1:7C1pvHqHw5A4vrJfjNwvOdzYu0Gml16OCs2GRiTUUS4= +github.com/spf13/pflag v1.0.9 h1:9exaQaMOCwffKiiiYk6/BndUBv+iRViNW+4lEMi0PvY= +github.com/spf13/pflag v1.0.9/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg= +go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= +gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= +gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/internal/catalog/branch.go b/internal/catalog/branch.go new file mode 100644 index 0000000..b820dba --- /dev/null +++ b/internal/catalog/branch.go @@ -0,0 +1,23 @@ +package catalog + +import "regexp" + +// reInvalidCharacters mirrors g10k's branch sanitisation (\W -> "_"). +var reInvalidCharacters = regexp.MustCompile(`\W`) + +// reValidEnvironment is Puppet's own environment name rule. +var reValidEnvironment = regexp.MustCompile(`^[a-z0-9_]+$`) + +// BranchToEnvironment converts a git branch name into the environment +// directory name g10k deploys it as: every character outside [0-9A-Za-z_] +// becomes "_". Case is preserved, because g10k preserves it. +func BranchToEnvironment(branch string) string { + return reInvalidCharacters.ReplaceAllString(branch, "_") +} + +// ValidEnvironmentName reports whether name is a legal Puppet environment +// name. g10k can produce names Puppet rejects (it keeps uppercase), so +// callers must check the result of BranchToEnvironment before using it. +func ValidEnvironmentName(name string) bool { + return reValidEnvironment.MatchString(name) +} diff --git a/internal/catalog/branch_test.go b/internal/catalog/branch_test.go new file mode 100644 index 0000000..9880e22 --- /dev/null +++ b/internal/catalog/branch_test.go @@ -0,0 +1,48 @@ +package catalog + +import "testing" + +func TestBranchToEnvironment(t *testing.T) { + tests := []struct { + branch string + want string + }{ + {"develop", "develop"}, + {"benvin/foo-bar", "benvin_foo_bar"}, + {"benvin/scaffold-catalog-diff", "benvin_scaffold_catalog_diff"}, + {"feature/PUP-1234", "feature_PUP_1234"}, + {"release/1.2.3", "release_1_2_3"}, + {"already_ok_123", "already_ok_123"}, + {"dots.and spaces", "dots_and_spaces"}, + {"trailing/", "trailing_"}, + {"", ""}, + {"UPPER", "UPPER"}, + {"héllo", "h_llo"}, + } + for _, tc := range tests { + if got := BranchToEnvironment(tc.branch); got != tc.want { + t.Errorf("BranchToEnvironment(%q) = %q, want %q", tc.branch, got, tc.want) + } + } +} + +func TestValidEnvironmentName(t *testing.T) { + tests := []struct { + name string + want bool + }{ + {"develop", true}, + {"benvin_foo_bar", true}, + {"env123", true}, + {"feature_PUP_1234", false}, + {"UPPER", false}, + {"has-dash", false}, + {"has.dot", false}, + {"", false}, + } + for _, tc := range tests { + if got := ValidEnvironmentName(tc.name); got != tc.want { + t.Errorf("ValidEnvironmentName(%q) = %v, want %v", tc.name, got, tc.want) + } + } +} diff --git a/internal/catalog/catalog.go b/internal/catalog/catalog.go new file mode 100644 index 0000000..f60bfa2 --- /dev/null +++ b/internal/catalog/catalog.go @@ -0,0 +1,228 @@ +package catalog + +import ( + "crypto/sha256" + "encoding/hex" + "encoding/json" + "fmt" + "path" + "sort" + "strconv" + "strings" +) + +// Catalog is the subset of a Puppet v4 catalog this tool reads. +type Catalog struct { + Name string `json:"name"` + Version json.RawMessage `json:"version"` + Environment string `json:"environment"` + CodeID string `json:"code_id"` + CatalogUUID string `json:"catalog_uuid"` + Tags []string `json:"tags"` + Classes []string `json:"classes"` + SensitiveParameters []string `json:"sensitive_parameters"` + Resources []Resource `json:"resources"` +} + +// Resource is a single catalog resource. +type Resource struct { + Type string `json:"type"` + Title string `json:"title"` + Parameters map[string]any `json:"parameters"` + Tags []string `json:"tags"` + Exported bool `json:"exported"` + File string `json:"file"` + 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 + "]" } + +// Options controls normalisation. The zero value is the default behaviour: +// classes ignored, exported resources skipped, ordering-only metaparameters +// dropped. +type Options struct { + // IncludeClasses keeps Class resources, which manage nothing themselves. + IncludeClasses bool + // IgnoreResources holds "Type[Title]" patterns (glob) to drop entirely. + IgnoreResources []string + // IgnoreAttrs holds parameter names to drop from every resource. + IgnoreAttrs []string +} + +// noiseParameters never carry change signal. before/require are ordering only +// and their targets are rarely visible; notify/subscribe are kept because they +// cause real work, but their arrays are sorted before comparison. +var noiseParameters = map[string]bool{ + "loglevel": true, + "stage": true, + "before": true, + "require": true, +} + +// relationshipParameters are the kept metaparameters whose value Puppet writes +// inconsistently as either a scalar or an array. +var relationshipParameters = map[string]bool{ + "notify": true, + "subscribe": true, +} + +// Normalize indexes a catalog by resource key, applying every normalisation +// that stops known false positives: classes and exported resources dropped, +// noise parameters stripped, File titles keyed by path, relationship +// metaparameters sorted, sensitive values redacted. +func Normalize(c *Catalog, opts Options) map[string]Resource { + out := make(map[string]Resource, len(c.Resources)) + for _, r := range c.Resources { + if r.Exported { + continue + } + if !opts.IncludeClasses && r.Type == "Class" { + continue + } + + title := normalizeTitle(r) + r.Parameters = cleanParameters(r, c.SensitiveParameters, opts.IgnoreAttrs) + r.Title = title + r.Tags = sortedTags(r.Tags) + + if matchesAny(r.Key(), opts.IgnoreResources) { + continue + } + out[r.Key()] = r + } + return out +} + +// normalizeTitle keys a File by its path parameter, so file{'my-name': +// path=>'/etc/foo'} and file{'/etc/foo':} compare equal. cleanParameters has +// already removed the path parameter. +func normalizeTitle(r Resource) string { + if r.Type != "File" { + return r.Title + } + if p, ok := r.Parameters["path"].(string); ok && p != "" { + return p + } + return r.Title +} + +func cleanParameters(r Resource, catalogSensitive, ignoreAttrs []string) map[string]any { + sensitive := map[string]bool{} + for _, name := range append(append([]string(nil), catalogSensitive...), r.SensitiveParameters...) { + sensitive[name] = true + } + ignored := map[string]bool{} + for _, name := range ignoreAttrs { + ignored[name] = true + } + + params := make(map[string]any, len(r.Parameters)) + for k, v := range r.Parameters { + if noiseParameters[k] || ignored[k] || strings.HasPrefix(k, "_") { + continue + } + if r.Type == "File" && k == "path" { + continue + } + if sensitive[k] { + params[k] = redact(v) + continue + } + if relationshipParameters[k] { + params[k] = sortedRelationship(v) + continue + } + params[k] = v + } + return params +} + +// sortedRelationship turns a scalar into a one-element list and sorts the +// result, so notify => "Service[x]" and notify => ["Service[x]"] compare equal +// and list order never registers as a change. +func sortedRelationship(v any) any { + items, ok := v.([]any) + if !ok { + items = []any{v} + } + sorted := append([]any(nil), items...) + sort.SliceStable(sorted, func(i, j int) bool { + return stringRepr(sorted[i]) < stringRepr(sorted[j]) + }) + return sorted +} + +// redact replaces a sensitive value with a digest, so a change is still +// visible but the plaintext never reaches stdout or a CI log. +func redact(v any) string { + sum := sha256.Sum256([]byte(stringRepr(v))) + return "redacted:sha256:" + hex.EncodeToString(sum[:])[:16] +} + +func sortedTags(tags []string) []string { + if len(tags) == 0 { + return nil + } + out := append([]string(nil), tags...) + sort.Strings(out) + return out +} + +// matchesAny reports whether a "Type[Title]" key matches one of the patterns. +// A pattern may glob in either half; a pattern without brackets matches the +// type and any title. +func matchesAny(key string, patterns []string) bool { + for _, p := range patterns { + if matchesPattern(key, p) { + return true + } + } + return false +} + +func matchesPattern(key, pattern string) bool { + keyType, keyTitle := splitKey(key) + patType, patTitle := splitKey(pattern) + if patTitle == "" && !strings.Contains(pattern, "[") { + patTitle = "*" + } + typeOK, err := path.Match(patType, keyType) + if err != nil || !typeOK { + return false + } + titleOK, err := path.Match(patTitle, keyTitle) + return err == nil && titleOK +} + +func splitKey(key string) (string, string) { + open := strings.Index(key, "[") + if open < 0 || !strings.HasSuffix(key, "]") { + return key, "" + } + return key[:open], key[open+1 : len(key)-1] +} + +// stringRepr renders a value the way the datatype-insensitive comparison sees +// it: nil and "" are the same string, 28 and "28" are the same string. +func stringRepr(v any) string { + switch t := v.(type) { + case nil: + return "" + case string: + return t + case bool: + return fmt.Sprintf("%t", t) + case float64: + return strconv.FormatFloat(t, 'f', -1, 64) + case int: + return strconv.Itoa(t) + default: + b, err := json.Marshal(v) + if err != nil { + return fmt.Sprintf("%v", v) + } + return string(b) + } +} diff --git a/internal/catalog/diff.go b/internal/catalog/diff.go new file mode 100644 index 0000000..197c2ea --- /dev/null +++ b/internal/catalog/diff.go @@ -0,0 +1,273 @@ +package catalog + +import ( + "encoding/json" + "sort" + "strconv" +) + +// Diff types, mirroring octocatalog-diff's taxonomy. +const ( + DiffAdded = "+" // whole resource added + DiffRemoved = "-" // whole resource removed + DiffChanged = "~" // attribute changed + DiffElemented = "!" // element changed inside a nested array or hash +) + +// Severities. +const ( + SeverityCritical = "critical" + SeverityNormal = "normal" +) + +// criticalTypes are the resource types whose changes can break a host. +var criticalTypes = map[string]bool{ + "Service": true, + "Package": true, + "File": true, + "Exec": true, + "Mount": true, + "User": true, + "Group": true, + "Cron": true, + "Systemd::Unit_file": true, + "Systemd::Service": true, + "Docker::Image": true, + "Docker::Container": true, +} + +// IsCritical reports whether a resource type is in the critical set. +func IsCritical(resourceType string) bool { return criticalTypes[resourceType] } + +// Severity buckets a resource type. +func Severity(resourceType string) string { + if IsCritical(resourceType) { + return SeverityCritical + } + return SeverityNormal +} + +// Change is one difference between the base and test catalogs. Structure is +// the path to the changed value, e.g. ["parameters","device"]. +type Change struct { + DiffType string `json:"diff_type"` + Type string `json:"type"` + Title string `json:"title"` + Structure []string `json:"structure"` + OldValue any `json:"old_value"` + NewValue any `json:"new_value"` + OldFile string `json:"old_file,omitempty"` + OldLine int `json:"old_line,omitempty"` + NewFile string `json:"new_file,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"` +} + +// Key is the "Type[Title]" identity of the resource the change belongs to. +func (c Change) Key() string { return c.Type + "[" + c.Title + "]" } + +// Summary counts a change set. +type Summary struct { + Added int `json:"added"` + Removed int `json:"removed"` + Changed int `json:"changed"` + Critical int `json:"critical"` +} + +// DiffOptions controls what the comparison reports. +type DiffOptions struct { + // DisplayDatatypeChanges keeps changes where only the datatype differs. + DisplayDatatypeChanges bool +} + +// Diff compares two normalised catalogs. Whole-resource additions and removals +// are determined from the key sets first, so a new resource is never confused +// with a new element inside an existing one. file and line are reported for +// context but never drive equality, so a resource moving line is not a change. +// The result is sorted, so the same inputs always render identically. +func Diff(base, test map[string]Resource, opts DiffOptions) []Change { + var changes []Change + + for key, baseRes := range base { + if _, ok := test[key]; ok { + continue + } + + changes = append(changes, Change{ + DiffType: DiffRemoved, + Type: baseRes.Type, + Title: baseRes.Title, + OldValue: baseRes.Parameters, + OldFile: baseRes.File, + OldLine: baseRes.Line, + Severity: Severity(baseRes.Type), + }) + } + + for key, testRes := range test { + if _, ok := base[key]; ok { + continue + } + changes = append(changes, Change{ + DiffType: DiffAdded, + Type: testRes.Type, + Title: testRes.Title, + NewValue: testRes.Parameters, + NewFile: testRes.File, + NewLine: testRes.Line, + Severity: Severity(testRes.Type), + }) + } + + for key, baseRes := range base { + testRes, ok := test[key] + if !ok { + continue + } + changes = append(changes, diffParameters(baseRes, testRes, opts)...) + } + + sort.SliceStable(changes, func(i, j int) bool { + a, b := changes[i], changes[j] + if a.Key() != b.Key() { + return a.Key() < b.Key() + } + if a.DiffType != b.DiffType { + return a.DiffType < b.DiffType + } + return structureKey(a.Structure) < structureKey(b.Structure) + }) + return changes +} + +func structureKey(s []string) string { + out := "" + for _, part := range s { + out += "\x00" + part + } + return out +} + +func diffParameters(baseRes, testRes Resource, opts DiffOptions) []Change { + proto := Change{ + Type: testRes.Type, + Title: testRes.Title, + OldFile: baseRes.File, + OldLine: baseRes.Line, + NewFile: testRes.File, + NewLine: testRes.Line, + Severity: Severity(testRes.Type), + } + return walk(proto, []string{"parameters"}, + anyMap(baseRes.Parameters), anyMap(testRes.Parameters), opts) +} + +func anyMap(m map[string]any) any { + out := make(map[string]any, len(m)) + for k, v := range m { + out[k] = v + } + return out +} + +// walk compares two values at a path, recursing into matching containers so a +// change deep inside a hash or array is reported as an element change ("!") +// against the smallest structure that actually differs. +func walk(proto Change, structure []string, oldVal, newVal any, opts DiffOptions) []Change { + if deepEqual(oldVal, newVal) { + return nil + } + + oldMap, oldIsMap := oldVal.(map[string]any) + newMap, newIsMap := newVal.(map[string]any) + if oldIsMap && newIsMap { + var changes []Change + for _, k := range unionKeys(oldMap, newMap) { + changes = append(changes, walk(proto, child(structure, k), oldMap[k], newMap[k], opts)...) + } + return changes + } + + oldList, oldIsList := oldVal.([]any) + newList, newIsList := newVal.([]any) + if oldIsList && newIsList && len(oldList) == len(newList) { + var changes []Change + for i := range oldList { + changes = append(changes, walk(proto, child(structure, strconv.Itoa(i)), oldList[i], newList[i], opts)...) + } + return changes + } + + datatypeOnly := stringRepr(oldVal) == stringRepr(newVal) + if datatypeOnly && !opts.DisplayDatatypeChanges { + return nil + } + + change := proto + change.Structure = append([]string(nil), structure...) + change.OldValue = oldVal + change.NewValue = newVal + change.Datatype = datatypeOnly + // A difference directly on a parameter is an attribute change; anything + // deeper is an element change inside that attribute. + if len(structure) <= 2 { + change.DiffType = DiffChanged + } else { + change.DiffType = DiffElemented + } + return []Change{change} +} + +func unionKeys(a, b map[string]any) []string { + seen := make(map[string]bool, len(a)+len(b)) + keys := make([]string, 0, len(a)+len(b)) + for k := range a { + seen[k] = true + keys = append(keys, k) + } + for k := range b { + if !seen[k] { + keys = append(keys, k) + } + } + sort.Strings(keys) + return keys +} + +// child extends a structure path without aliasing the parent's backing array. +func child(structure []string, part string) []string { + out := make([]string, 0, len(structure)+1) + return append(append(out, structure...), part) +} + +// deepEqual compares two decoded JSON values structurally. Encoding both sides +// sorts map keys, so map ordering never registers as a change. +func deepEqual(a, b any) bool { + ab, errA := json.Marshal(a) + bb, errB := json.Marshal(b) + if errA != nil || errB != nil { + return false + } + return string(ab) == string(bb) +} + +// Summarise counts a change set. +func Summarise(changes []Change) Summary { + var s Summary + for _, c := range changes { + switch c.DiffType { + case DiffAdded: + s.Added++ + case DiffRemoved: + s.Removed++ + default: + s.Changed++ + } + if c.Severity == SeverityCritical { + s.Critical++ + } + } + return s +} diff --git a/internal/catalog/diff_test.go b/internal/catalog/diff_test.go new file mode 100644 index 0000000..1f45206 --- /dev/null +++ b/internal/catalog/diff_test.go @@ -0,0 +1,308 @@ +package catalog + +import ( + "encoding/json" + "testing" +) + +func mustCatalog(t *testing.T, body string) *Catalog { + t.Helper() + var c Catalog + if err := json.Unmarshal([]byte(body), &c); err != nil { + t.Fatalf("parsing catalog fixture: %v", err) + } + return &c +} + +const baseCatalog = `{ + "name":"host.example.net","environment":"develop","version":1, + "classes":["settings","motd"], + "resources":[ + {"type":"Stage","title":"main","parameters":{"name":"main"}}, + {"type":"Class","title":"Motd","parameters":{"content":"old"}}, + {"type":"File","title":"/etc/motd","file":"site.pp","line":10,"tags":["file","z","a"],"parameters":{"ensure":"file","content":"old","loglevel":"notice","_meta":"x","require":"Package[vim]","notify":["Service[sshd]","Service[chronyd]"]}}, + {"type":"Service","title":"sshd","file":"ssh.pp","line":4,"tags":["service"],"parameters":{"ensure":"running","enable":true,"port":28}}, + {"type":"Notify","title":"gone","file":"old.pp","line":1,"parameters":{"message":"bye"}}, + {"type":"Package","title":"vim","parameters":{"ensure":"installed"}}, + {"type":"Exec","title":"reload","parameters":{"command":"/bin/true","environment":{"HOME":"/root","PATH":"/bin"}}}, + {"type":"Nagios_service","title":"exported","exported":true,"parameters":{"check":"x"}}, + {"type":"Mysql::Db","title":"app","sensitive_parameters":["password"],"parameters":{"user":"app","password":"hunter2"}} + ] +}` + +const testCatalog = `{ + "name":"host.example.net","environment":"benvin_test","version":2, + "classes":["settings","motd"], + "resources":[ + {"type":"Stage","title":"main","parameters":{"name":"main"}}, + {"type":"Class","title":"Motd","parameters":{"content":"new"}}, + {"type":"File","title":"motd-managed","file":"site.pp","line":19,"tags":["a","file","z"],"parameters":{"path":"/etc/motd","ensure":"file","content":"new","loglevel":"debug","stage":"main","_meta":"y","require":"Package[vim2]","notify":["Service[chronyd]","Service[sshd]"]}}, + {"type":"Service","title":"sshd","file":"ssh.pp","line":4,"tags":["service"],"parameters":{"ensure":"running","enable":true,"port":"28"}}, + {"type":"Notify","title":"added","file":"new.pp","line":3,"parameters":{"message":"hi"}}, + {"type":"Package","title":"vim","parameters":{"ensure":"installed","provider":"dnf"}}, + {"type":"Exec","title":"reload","parameters":{"command":"/bin/true","environment":{"HOME":"/root","PATH":"/usr/bin"}}}, + {"type":"Nagios_service","title":"exported","exported":true,"parameters":{"check":"y"}}, + {"type":"Mysql::Db","title":"app","sensitive_parameters":["password"],"parameters":{"user":"app","password":"hunter3"}} + ] +}` + +func normalized(t *testing.T, body string, opts Options) map[string]Resource { + t.Helper() + return Normalize(mustCatalog(t, body), opts) +} + +func changeSet(t *testing.T) []Change { + t.Helper() + return Diff( + normalized(t, baseCatalog, Options{}), + normalized(t, testCatalog, Options{}), + DiffOptions{}, + ) +} + +func byKey(changes []Change) map[string][]Change { + out := map[string][]Change{} + for _, c := range changes { + out[c.Key()] = append(out[c.Key()], c) + } + return out +} + +func TestNormalizeDropsNoiseParameters(t *testing.T) { + res := normalized(t, testCatalog, Options{}) + motd, ok := res["File[/etc/motd]"] + if !ok { + t.Fatalf("File not re-keyed by its path parameter: %v", res) + } + for _, drop := range []string{"loglevel", "stage", "_meta", "before", "require", "path"} { + if _, present := motd.Parameters[drop]; present { + t.Errorf("parameter %q should have been dropped", drop) + } + } + if motd.Parameters["content"] != "new" { + t.Errorf("real parameters dropped: %+v", motd.Parameters) + } + if motd.File != "site.pp" || motd.Line != 19 { + t.Errorf("file/line context lost: %+v", motd) + } + if got := motd.Tags; len(got) != 3 || got[0] != "a" || got[2] != "z" { + t.Errorf("tags not sorted: %v", got) + } +} + +func TestNormalizeSkipsClassesAndExported(t *testing.T) { + res := normalized(t, testCatalog, Options{}) + if _, ok := res["Class[Motd]"]; ok { + t.Error("Class resources must be ignored by default") + } + if _, ok := res["Nagios_service[exported]"]; ok { + t.Error("exported resources must be skipped") + } + + withClasses := normalized(t, testCatalog, Options{IncludeClasses: true}) + if _, ok := withClasses["Class[Motd]"]; !ok { + t.Error("--include-classes must keep Class resources") + } +} + +func TestNormalizeRedactsSensitiveParameters(t *testing.T) { + base := normalized(t, baseCatalog, Options{})["Mysql::Db[app]"] + test := normalized(t, testCatalog, Options{})["Mysql::Db[app]"] + + for _, r := range []Resource{base, test} { + value, _ := r.Parameters["password"].(string) + if value == "hunter2" || value == "hunter3" { + t.Fatalf("sensitive value leaked: %q", value) + } + if len(value) < len("redacted:sha256:") || value[:16] != "redacted:sha256:" { + t.Fatalf("unexpected redaction %q", value) + } + } + if base.Parameters["password"] == test.Parameters["password"] { + t.Error("a changed secret must still show as a change") + } +} + +func TestNormalizeIgnoreFlags(t *testing.T) { + res := normalized(t, testCatalog, Options{ + IgnoreResources: []string{"Notify[*]", "Package[vim]"}, + IgnoreAttrs: []string{"ensure"}, + }) + if _, ok := res["Notify[added]"]; ok { + t.Error("--ignore glob did not drop Notify[added]") + } + if _, ok := res["Package[vim]"]; ok { + t.Error("--ignore did not drop Package[vim]") + } + if _, ok := res["File[/etc/motd]"].Parameters["ensure"]; ok { + t.Error("--ignore-attr did not drop the ensure parameter") + } +} + +func TestNormalizeRelationshipParameters(t *testing.T) { + scalar := Normalize(&Catalog{Resources: []Resource{ + {Type: "Service", Title: "a", Parameters: map[string]any{"notify": "Service[b]"}}, + }}, Options{}) + list := Normalize(&Catalog{Resources: []Resource{ + {Type: "Service", Title: "a", Parameters: map[string]any{"notify": []any{"Service[b]"}}}, + }}, Options{}) + + if changes := Diff(scalar, list, DiffOptions{}); len(changes) != 0 { + t.Errorf("scalar and single-element array must compare equal: %+v", changes) + } + + unsorted := Normalize(&Catalog{Resources: []Resource{ + {Type: "Service", Title: "a", Parameters: map[string]any{"subscribe": []any{"File[y]", "File[x]"}}}, + }}, Options{}) + sorted := Normalize(&Catalog{Resources: []Resource{ + {Type: "Service", Title: "a", Parameters: map[string]any{"subscribe": []any{"File[x]", "File[y]"}}}, + }}, Options{}) + if changes := Diff(unsorted, sorted, DiffOptions{}); len(changes) != 0 { + t.Errorf("relationship array order must not matter: %+v", changes) + } +} + +func TestDiffResourceAddedRemoved(t *testing.T) { + changes := byKey(changeSet(t)) + + added := changes["Notify[added]"] + if len(added) != 1 || added[0].DiffType != DiffAdded { + t.Fatalf("added resource not detected: %+v", added) + } + if added[0].NewValue == nil || added[0].OldValue != nil { + t.Errorf("added resource must carry only new_value: %+v", added[0]) + } + + removed := changes["Notify[gone]"] + if len(removed) != 1 || removed[0].DiffType != DiffRemoved { + t.Fatalf("removed resource not detected: %+v", removed) + } + if removed[0].OldValue == nil || removed[0].NewValue != nil { + t.Errorf("removed resource must carry only old_value: %+v", removed[0]) + } +} + +func TestDiffAttributeAndElementChanges(t *testing.T) { + changes := byKey(changeSet(t)) + + motd := changes["File[/etc/motd]"] + if len(motd) != 1 { + t.Fatalf("expected one change on the motd file, got %+v", motd) + } + if motd[0].DiffType != DiffChanged { + t.Errorf("attribute change must be %q: %+v", DiffChanged, motd[0]) + } + if got := motd[0].Structure; len(got) != 2 || got[0] != "parameters" || got[1] != "content" { + t.Errorf("unexpected structure %v", got) + } + if motd[0].OldValue != "old" || motd[0].NewValue != "new" { + t.Errorf("unexpected values: %+v", motd[0]) + } + + exec := changes["Exec[reload]"] + if len(exec) != 1 || exec[0].DiffType != DiffElemented { + t.Fatalf("nested change must be %q: %+v", DiffElemented, exec) + } + if got := exec[0].Structure; len(got) != 3 || got[2] != "PATH" { + t.Errorf("nested structure not reported: %v", got) + } + if exec[0].OldValue != "/bin" || exec[0].NewValue != "/usr/bin" { + t.Errorf("nested values wrong: %+v", exec[0]) + } +} + +func TestDiffSuppressesDatatypeChanges(t *testing.T) { + if changes := byKey(changeSet(t))["Service[sshd]"]; len(changes) != 0 { + t.Errorf("28 vs \"28\" must be suppressed by default: %+v", changes) + } + + shown := Diff( + normalized(t, baseCatalog, Options{}), + normalized(t, testCatalog, Options{}), + DiffOptions{DisplayDatatypeChanges: true}, + ) + sshd := byKey(shown)["Service[sshd]"] + if len(sshd) != 1 || !sshd[0].Datatype { + t.Fatalf("--display-datatype-changes must surface the change: %+v", sshd) + } +} + +func TestDiffIgnoresFileLineMoves(t *testing.T) { + base := map[string]Resource{"File[/a]": {Type: "File", Title: "/a", File: "a.pp", Line: 1, + Parameters: map[string]any{"ensure": "file"}}} + test := map[string]Resource{"File[/a]": {Type: "File", Title: "/a", File: "b.pp", Line: 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) + } +} + +func TestDiffSeverity(t *testing.T) { + for _, c := range changeSet(t) { + want := SeverityNormal + if IsCritical(c.Type) { + want = SeverityCritical + } + if c.Severity != want { + t.Errorf("%s severity = %q, want %q", c.Key(), c.Severity, want) + } + } + if !IsCritical("Systemd::Unit_file") || !IsCritical("Docker::Container") { + t.Error("critical type list incomplete") + } + if IsCritical("Notify") { + t.Error("Notify must not be critical") + } +} + +func TestDiffDeterministicOrdering(t *testing.T) { + first := changeSet(t) + for i := 0; i < 20; i++ { + next := changeSet(t) + if len(first) != len(next) { + t.Fatalf("change count varies: %d vs %d", len(first), len(next)) + } + for j := range first { + if first[j].Key() != next[j].Key() || structureKey(first[j].Structure) != structureKey(next[j].Structure) { + t.Fatalf("ordering is not stable at %d: %+v vs %+v", j, first[j], next[j]) + } + } + } + for i := 1; i < len(first); i++ { + if first[i-1].Key() > first[i].Key() { + t.Fatalf("changes not sorted by resource: %q before %q", first[i-1].Key(), first[i].Key()) + } + } +} + +func TestDiffIgnoresMapOrdering(t *testing.T) { + base := map[string]Resource{"File[/a]": {Type: "File", Title: "/a", Parameters: map[string]any{ + "opts": map[string]any{"b": 1, "a": 2}, + }}} + test := map[string]Resource{"File[/a]": {Type: "File", Title: "/a", Parameters: map[string]any{ + "opts": map[string]any{"a": 2, "b": 1}, + }}} + if changes := Diff(base, test, DiffOptions{}); len(changes) != 0 { + t.Errorf("map key ordering reported as a change: %+v", changes) + } +} + +func TestSummarise(t *testing.T) { + s := Summarise(changeSet(t)) + if s.Added != 1 || s.Removed != 1 { + t.Errorf("unexpected summary: %+v", s) + } + if s.Changed != 4 { + t.Errorf("changed = %d, want 4 (motd content, vim provider, exec PATH, db password)", s.Changed) + } + if s.Critical != 3 { + t.Errorf("critical = %d, want 3 (File, Package, Exec)", s.Critical) + } +} + +func TestDiffNoChanges(t *testing.T) { + norm := normalized(t, baseCatalog, Options{}) + if changes := Diff(norm, norm, DiffOptions{}); len(changes) != 0 { + t.Errorf("identical catalogs produced changes: %+v", changes) + } +} diff --git a/internal/catalog/report.go b/internal/catalog/report.go new file mode 100644 index 0000000..0808ff7 --- /dev/null +++ b/internal/catalog/report.go @@ -0,0 +1,214 @@ +package catalog + +import ( + "encoding/json" + "fmt" + "io" + "sort" + "strings" +) + +// Report is the full result of a catalog-diff run and the shape of --json. +type Report struct { + Hostname string `json:"hostname"` + BaseEnvironment string `json:"base_environment"` + TestEnvironment string `json:"test_environment"` + Compiled map[string]bool `json:"compiled"` + Changes []Change `json:"changes"` + Summary Summary `json:"summary"` +} + +// NewReport builds a report from a change set. +func NewReport(hostname, baseEnv, testEnv string, changes []Change) Report { + if changes == nil { + changes = []Change{} + } + return Report{ + Hostname: hostname, + BaseEnvironment: baseEnv, + TestEnvironment: testEnv, + Compiled: map[string]bool{baseEnv: true, testEnv: true}, + Changes: changes, + Summary: Summarise(changes), + } +} + +// RenderJSON writes the report as indented JSON. +func RenderJSON(w io.Writer, r Report) error { + enc := json.NewEncoder(w) + enc.SetIndent("", " ") + return enc.Encode(r) +} + +// RenderOptions controls the text renderer. +type RenderOptions struct { + // Colour emits ANSI escapes; callers set it only for a TTY. + Colour bool + // DetailAdd expands the parameters of added resources. + DetailAdd bool + // SourceFileLine appends the originating manifest file and line. + SourceFileLine bool +} + +const ( + ansiReset = "\033[0m" + ansiBold = "\033[1m" + ansiRed = "\033[31m" +) + +// RenderText writes the human/agent readable report. Changes are grouped +// critical first, then ordered by resource key, so CI logs diff cleanly +// between runs. +func RenderText(w io.Writer, r Report, opts RenderOptions) error { + var b strings.Builder + + bold := func(s string) string { + if !opts.Colour { + return s + } + return ansiBold + s + ansiReset + } + red := func(s string) string { + if !opts.Colour { + return s + } + return ansiRed + s + ansiReset + } + + b.WriteString(bold("Puppet Catalog Diff") + "\n") + b.WriteString(strings.Repeat("=", 50) + "\n") + fmt.Fprintf(&b, "Host : %s\n", r.Hostname) + fmt.Fprintf(&b, "Base environment : %s\n", r.BaseEnvironment) + fmt.Fprintf(&b, "Test environment : %s\n", r.TestEnvironment) + fmt.Fprintf(&b, "Compiled : %s\n", compiledLine(r.Compiled)) + fmt.Fprintf(&b, "Changes : %d critical, %d added, %d removed, %d changed\n\n", + r.Summary.Critical, r.Summary.Added, r.Summary.Removed, r.Summary.Changed) + + if len(r.Changes) == 0 { + b.WriteString("No differences between environments.\n") + _, err := io.WriteString(w, b.String()) + return err + } + + var critical, other []Change + for _, c := range r.Changes { + if c.Severity == SeverityCritical { + critical = append(critical, c) + } else { + other = append(other, c) + } + } + + writeGroup := func(title string, changes []Change) { + if len(changes) == 0 { + return + } + b.WriteString(title + "\n") + b.WriteString(strings.Repeat("-", 50) + "\n") + for _, c := range changes { + for _, line := range describe(c, opts) { + b.WriteString(" " + line + "\n") + } + } + b.WriteString("\n") + } + + writeGroup(red(bold("CRITICAL")), critical) + writeGroup(bold("OTHER"), other) + + _, err := io.WriteString(w, strings.TrimRight(b.String(), "\n")+"\n") + return err +} + +func compiledLine(compiled map[string]bool) string { + envs := make([]string, 0, len(compiled)) + for env := range compiled { + envs = append(envs, env) + } + sort.Strings(envs) + parts := make([]string, 0, len(envs)) + for _, env := range envs { + status := "failed" + if compiled[env] { + status = "ok" + } + parts = append(parts, env+"="+status) + } + return strings.Join(parts, " ") +} + +// describe renders one change as one or more lines. Added resources collapse +// to a single line unless DetailAdd is set; changed resources always expand. +func describe(c Change, opts RenderOptions) []string { + switch c.DiffType { + case DiffAdded: + lines := []string{fmt.Sprintf("+ %s added%s", c.Key(), location(c.NewFile, c.NewLine, opts))} + if opts.DetailAdd { + lines = append(lines, parameterLines(" ", c.NewValue)...) + } + return lines + case DiffRemoved: + return []string{fmt.Sprintf("- %s removed%s", c.Key(), location(c.OldFile, c.OldLine, opts))} + default: + note := "" + if c.Datatype { + note = " (datatype only)" + } + return []string{fmt.Sprintf("%s %s %s: %s -> %s%s%s", + c.DiffType, c.Key(), structurePath(c.Structure), + renderValue(c.OldValue), renderValue(c.NewValue), note, + location(c.NewFile, c.NewLine, opts))} + } +} + +// structurePath drops the leading "parameters" element, which every attribute +// change shares, and joins the rest. +func structurePath(structure []string) string { + if len(structure) > 0 && structure[0] == "parameters" { + structure = structure[1:] + } + if len(structure) == 0 { + return "(resource)" + } + return strings.Join(structure, ".") +} + +func location(file string, line int, opts RenderOptions) string { + if !opts.SourceFileLine || file == "" { + return "" + } + return fmt.Sprintf(" (%s:%d)", file, line) +} + +func parameterLines(indent string, value any) []string { + params, ok := value.(map[string]any) + if !ok { + return nil + } + names := make([]string, 0, len(params)) + for name := range params { + names = append(names, name) + } + sort.Strings(names) + lines := make([]string, 0, len(names)) + for _, name := range names { + lines = append(lines, fmt.Sprintf("%s%s: %s", indent, name, renderValue(params[name]))) + } + return lines +} + +// renderValue prints a value compactly: strings bare, everything else as +// compact JSON, and an absent value as "". +func renderValue(v any) string { + if v == nil { + return "" + } + if s, ok := v.(string); ok { + return s + } + b, err := json.Marshal(v) + if err != nil { + return fmt.Sprintf("%v", v) + } + return string(b) +} diff --git a/internal/catalog/report_test.go b/internal/catalog/report_test.go new file mode 100644 index 0000000..6a62fe3 --- /dev/null +++ b/internal/catalog/report_test.go @@ -0,0 +1,140 @@ +package catalog + +import ( + "bytes" + "flag" + "os" + "path/filepath" + "strings" + "testing" +) + +var update = flag.Bool("update", false, "rewrite the golden files") + +func golden(t *testing.T, name string, got []byte) { + t.Helper() + path := filepath.Join("testdata", name) + if *update { + if err := os.WriteFile(path, got, 0o644); err != nil { + t.Fatalf("writing golden %s: %v", path, err) + } + return + } + want, err := os.ReadFile(path) + if err != nil { + t.Fatalf("reading golden %s: %v (run go test -update)", path, err) + } + if !bytes.Equal(want, got) { + t.Errorf("output does not match %s\n--- want ---\n%s\n--- got ---\n%s", path, want, got) + } +} + +func testReport(t *testing.T) Report { + t.Helper() + return NewReport("host.example.net", "develop", "benvin_test", changeSet(t)) +} + +func TestRenderTextGolden(t *testing.T) { + var buf bytes.Buffer + if err := RenderText(&buf, testReport(t), RenderOptions{SourceFileLine: true}); err != nil { + t.Fatalf("RenderText: %v", err) + } + golden(t, "report.txt", buf.Bytes()) +} + +func TestRenderJSONGolden(t *testing.T) { + var buf bytes.Buffer + if err := RenderJSON(&buf, testReport(t)); err != nil { + t.Fatalf("RenderJSON: %v", err) + } + golden(t, "report.json", buf.Bytes()) +} + +func TestRenderTextNoChanges(t *testing.T) { + var buf bytes.Buffer + if err := RenderText(&buf, NewReport("host.example.net", "develop", "benvin_test", nil), RenderOptions{}); err != nil { + t.Fatalf("RenderText: %v", err) + } + if !strings.Contains(buf.String(), "No differences between environments.") { + t.Errorf("unexpected empty-diff output:\n%s", buf.String()) + } +} + +func TestRenderTextColourOnlyWhenAsked(t *testing.T) { + var plain, coloured bytes.Buffer + report := testReport(t) + if err := RenderText(&plain, report, RenderOptions{}); err != nil { + t.Fatal(err) + } + if err := RenderText(&coloured, report, RenderOptions{Colour: true}); err != nil { + t.Fatal(err) + } + if strings.Contains(plain.String(), "\033[") { + t.Error("plain output contains ANSI escapes") + } + if !strings.Contains(coloured.String(), "\033[") { + t.Error("coloured output contains no ANSI escapes") + } +} + +func TestRenderTextGroupsCriticalFirst(t *testing.T) { + var buf bytes.Buffer + if err := RenderText(&buf, testReport(t), RenderOptions{SourceFileLine: true}); err != nil { + t.Fatal(err) + } + out := buf.String() + crit, other := strings.Index(out, "CRITICAL"), strings.Index(out, "OTHER") + if crit < 0 || other < 0 || crit > other { + t.Errorf("critical changes not listed first:\n%s", out) + } +} + +func TestRenderJSONEmptyChangesIsArray(t *testing.T) { + var buf bytes.Buffer + if err := RenderJSON(&buf, NewReport("host.example.net", "develop", "benvin_test", nil)); err != nil { + t.Fatal(err) + } + if !strings.Contains(buf.String(), `"changes": []`) { + t.Errorf("empty change list must render as [], got:\n%s", buf.String()) + } +} + +func TestRenderTextCollapsesAddedResources(t *testing.T) { + report := testReport(t) + + var collapsed bytes.Buffer + if err := RenderText(&collapsed, report, RenderOptions{}); err != nil { + t.Fatal(err) + } + if strings.Contains(collapsed.String(), "message: hi") { + t.Errorf("added resource expanded without --display-detail-add:\n%s", collapsed.String()) + } + + var detailed bytes.Buffer + if err := RenderText(&detailed, report, RenderOptions{DetailAdd: true}); err != nil { + t.Fatal(err) + } + if !strings.Contains(detailed.String(), "message: hi") { + t.Errorf("--display-detail-add did not expand the added resource:\n%s", detailed.String()) + } +} + +func TestRenderTextSourceFileLineIsOptional(t *testing.T) { + report := testReport(t) + + var plain bytes.Buffer + if err := RenderText(&plain, report, RenderOptions{}); err != nil { + t.Fatal(err) + } + if strings.Contains(plain.String(), "site.pp") { + t.Errorf("file:line printed without --display-source-file-line:\n%s", plain.String()) + } + + var located bytes.Buffer + if err := RenderText(&located, report, RenderOptions{SourceFileLine: true}); err != nil { + t.Fatal(err) + } + if !strings.Contains(located.String(), "site.pp:19") { + t.Errorf("--display-source-file-line did not print the source:\n%s", located.String()) + } +} diff --git a/internal/catalog/testdata/report.json b/internal/catalog/testdata/report.json new file mode 100644 index 0000000..551c7d5 --- /dev/null +++ b/internal/catalog/testdata/report.json @@ -0,0 +1,96 @@ +{ + "hostname": "host.example.net", + "base_environment": "develop", + "test_environment": "benvin_test", + "compiled": { + "benvin_test": true, + "develop": true + }, + "changes": [ + { + "diff_type": "!", + "type": "Exec", + "title": "reload", + "structure": [ + "parameters", + "environment", + "PATH" + ], + "old_value": "/bin", + "new_value": "/usr/bin", + "severity": "critical" + }, + { + "diff_type": "~", + "type": "File", + "title": "/etc/motd", + "structure": [ + "parameters", + "content" + ], + "old_value": "old", + "new_value": "new", + "old_file": "site.pp", + "old_line": 10, + "new_file": "site.pp", + "new_line": 19, + "severity": "critical" + }, + { + "diff_type": "~", + "type": "Mysql::Db", + "title": "app", + "structure": [ + "parameters", + "password" + ], + "old_value": "redacted:sha256:f52fbd32b2b3b86f", + "new_value": "redacted:sha256:fb8c2e2b85ca81eb", + "severity": "normal" + }, + { + "diff_type": "+", + "type": "Notify", + "title": "added", + "structure": null, + "old_value": null, + "new_value": { + "message": "hi" + }, + "new_file": "new.pp", + "new_line": 3, + "severity": "normal" + }, + { + "diff_type": "-", + "type": "Notify", + "title": "gone", + "structure": null, + "old_value": { + "message": "bye" + }, + "new_value": null, + "old_file": "old.pp", + "old_line": 1, + "severity": "normal" + }, + { + "diff_type": "~", + "type": "Package", + "title": "vim", + "structure": [ + "parameters", + "provider" + ], + "old_value": null, + "new_value": "dnf", + "severity": "critical" + } + ], + "summary": { + "added": 1, + "removed": 1, + "changed": 4, + "critical": 3 + } +} diff --git a/internal/catalog/testdata/report.txt b/internal/catalog/testdata/report.txt new file mode 100644 index 0000000..a3fb8a4 --- /dev/null +++ b/internal/catalog/testdata/report.txt @@ -0,0 +1,19 @@ +Puppet Catalog Diff +================================================== +Host : host.example.net +Base environment : develop +Test environment : benvin_test +Compiled : benvin_test=ok develop=ok +Changes : 3 critical, 1 added, 1 removed, 4 changed + +CRITICAL +-------------------------------------------------- + ! Exec[reload] environment.PATH: /bin -> /usr/bin + ~ File[/etc/motd] content: old -> new (site.pp:19) + ~ Package[vim] provider: -> dnf + +OTHER +-------------------------------------------------- + ~ Mysql::Db[app] password: redacted:sha256:f52fbd32b2b3b86f -> redacted:sha256:fb8c2e2b85ca81eb + + Notify[added] added (new.pp:3) + - Notify[gone] removed (old.pp:1) diff --git a/internal/config/config.go b/internal/config/config.go new file mode 100644 index 0000000..586bb36 --- /dev/null +++ b/internal/config/config.go @@ -0,0 +1,112 @@ +// Package config loads the shared configuration for the puppet-catalog-tools +// binaries. Precedence (lowest to highest): defaults < config file < env vars +// < CLI flags (applied by the caller). +package config + +import ( + "fmt" + "os" + "path/filepath" + + "gopkg.in/yaml.v3" +) + +const ( + appName = "puppet-catalog-tools" + configFileName = "config.yaml" + + // DefaultServerURL is the k8s OpenVoxServer compiler pool. The legacy VM + // masters behind puppet.query.consul remain reachable via --server. + DefaultServerURL = "https://puppet.k8s.syd1.au.unkin.net:8140" + // DefaultPDBURL is pdbmux, the PuppetDB-compatible merging proxy. + DefaultPDBURL = "https://pdbmux.k8s.syd1.au.unkin.net" + // DefaultBaseEnvironment is the environment branches are diffed against. + DefaultBaseEnvironment = "develop" +) + +// Config holds every configurable value. Fields map 1:1 to config file keys. +type Config struct { + ServerURL string `yaml:"server_url"` + PDBURL string `yaml:"pdb_url"` + Cert string `yaml:"cert"` + Key string `yaml:"key"` + CACert string `yaml:"cacert"` + BaseEnvironment string `yaml:"base_environment"` +} + +// Default returns the built-in configuration. +func Default() Config { + return Config{ + ServerURL: DefaultServerURL, + PDBURL: DefaultPDBURL, + BaseEnvironment: DefaultBaseEnvironment, + } +} + +// Dir returns the XDG config directory for these tools. +func Dir() string { + base := os.Getenv("XDG_CONFIG_HOME") + if base == "" { + home, _ := os.UserHomeDir() + base = filepath.Join(home, ".config") + } + return filepath.Join(base, appName) +} + +// Path returns the full path to the config file. +func Path() string { return filepath.Join(Dir(), configFileName) } + +// Load reads the config file (if present), then applies env var overrides. +func Load() (Config, error) { + cfg := Default() + + path := Path() + data, err := os.ReadFile(path) + if err != nil && !os.IsNotExist(err) { + return cfg, fmt.Errorf("reading config %s: %w", path, err) + } + if err == nil { + if err := yaml.Unmarshal(data, &cfg); err != nil { + return cfg, fmt.Errorf("parsing config %s: %w", path, err) + } + } + + env := map[string]*string{ + "CATALOG_DIFF_SERVER": &cfg.ServerURL, + "CATALOG_DIFF_PDB": &cfg.PDBURL, + "CATALOG_DIFF_CERT": &cfg.Cert, + "CATALOG_DIFF_KEY": &cfg.Key, + "CATALOG_DIFF_CACERT": &cfg.CACert, + "CATALOG_DIFF_BASE_ENVIRONMENT": &cfg.BaseEnvironment, + } + for name, field := range env { + if v := os.Getenv(name); v != "" { + *field = v + } + } + + return cfg, nil +} + +// WriteDefault creates the config directory and writes a default config file. +// It refuses to overwrite an existing file. +func WriteDefault() (string, error) { + if err := os.MkdirAll(Dir(), 0o755); err != nil { + return "", fmt.Errorf("creating config dir: %w", err) + } + path := Path() + if _, err := os.Stat(path); err == nil { + return "", fmt.Errorf("config already exists at %s", path) + } + data, err := yaml.Marshal(Default()) + if err != nil { + return "", err + } + header := "# puppet-catalog-tools configuration\n" + + "# Overridable with CATALOG_DIFF_SERVER, CATALOG_DIFF_PDB, CATALOG_DIFF_CERT,\n" + + "# CATALOG_DIFF_KEY, CATALOG_DIFF_CACERT, CATALOG_DIFF_BASE_ENVIRONMENT.\n\n" + if err := os.WriteFile(path, append([]byte(header), data...), 0o644); err != nil { + return "", fmt.Errorf("writing config: %w", err) + } + return path, nil +} diff --git a/internal/config/config_test.go b/internal/config/config_test.go new file mode 100644 index 0000000..dae3ad0 --- /dev/null +++ b/internal/config/config_test.go @@ -0,0 +1,91 @@ +package config + +import ( + "os" + "path/filepath" + "testing" +) + +func TestLoadDefaults(t *testing.T) { + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + + cfg, err := Load() + if err != nil { + t.Fatalf("Load: %v", err) + } + if cfg.ServerURL != DefaultServerURL || cfg.PDBURL != DefaultPDBURL { + t.Errorf("unexpected defaults: %+v", cfg) + } + if cfg.BaseEnvironment != DefaultBaseEnvironment { + t.Errorf("base environment = %q", cfg.BaseEnvironment) + } +} + +func TestLoadPrecedence(t *testing.T) { + dir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", dir) + + if err := os.MkdirAll(filepath.Join(dir, appName), 0o755); err != nil { + t.Fatal(err) + } + body := "server_url: https://file.example:8140\npdb_url: https://file.example\ncert: /file/cert.pem\n" + if err := os.WriteFile(Path(), []byte(body), 0o644); err != nil { + t.Fatal(err) + } + + cfg, err := Load() + if err != nil { + t.Fatalf("Load: %v", err) + } + if cfg.ServerURL != "https://file.example:8140" { + t.Errorf("config file ignored: %q", cfg.ServerURL) + } + if cfg.Cert != "/file/cert.pem" { + t.Errorf("cert = %q", cfg.Cert) + } + + t.Setenv("CATALOG_DIFF_SERVER", "https://env.example:8140") + t.Setenv("CATALOG_DIFF_CERT", "/env/cert.pem") + cfg, err = Load() + if err != nil { + t.Fatalf("Load: %v", err) + } + if cfg.ServerURL != "https://env.example:8140" { + t.Errorf("env did not override file: %q", cfg.ServerURL) + } + if cfg.Cert != "/env/cert.pem" { + t.Errorf("cert = %q", cfg.Cert) + } + if cfg.PDBURL != "https://file.example" { + t.Errorf("unset env clobbered file value: %q", cfg.PDBURL) + } +} + +func TestLoadBadYAML(t *testing.T) { + dir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", dir) + if err := os.MkdirAll(filepath.Join(dir, appName), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(Path(), []byte("server_url: [unclosed\n"), 0o644); err != nil { + t.Fatal(err) + } + if _, err := Load(); err == nil { + t.Fatal("expected a parse error") + } +} + +func TestWriteDefault(t *testing.T) { + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + + path, err := WriteDefault() + if err != nil { + t.Fatalf("WriteDefault: %v", err) + } + if _, err := os.Stat(path); err != nil { + t.Fatalf("config not written: %v", err) + } + if _, err := WriteDefault(); err == nil { + t.Fatal("expected refusal to overwrite an existing config") + } +} diff --git a/internal/pdbmux/pdbmux.go b/internal/pdbmux/pdbmux.go new file mode 100644 index 0000000..2952b3a --- /dev/null +++ b/internal/pdbmux/pdbmux.go @@ -0,0 +1,130 @@ +// Package pdbmux reads node facts from pdbmux, the estate's PuppetDB +// compatible merging proxy. It is plain HTTPS, GET-only and unauthenticated — +// deliberately a separate client from the mTLS puppetserver one. +package pdbmux + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "net/http" + "net/url" + "strings" + "time" +) + +// ErrNodeNotFound is returned when pdbmux knows nothing about a certname. +var ErrNodeNotFound = errors.New("node not found in pdbmux") + +// Client queries pdbmux. +type Client struct { + BaseURL string + HTTP *http.Client +} + +// New returns a client for baseURL. +func New(baseURL string, timeout time.Duration) *Client { + return &Client{ + BaseURL: strings.TrimRight(baseURL, "/"), + HTTP: &http.Client{Timeout: timeout}, + } +} + +type factRecord struct { + Name string `json:"name"` + Value json.RawMessage `json:"value"` +} + +type factset struct { + 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. +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 { + return nil, err + } + if status == http.StatusOK { + var records []factRecord + if err := json.Unmarshal(body, &records); err != nil { + return nil, fmt.Errorf("parsing facts for %s: %w", certname, err) + } + if len(records) > 0 { + return fold(records) + } + } else if status != http.StatusNotFound { + return nil, fmt.Errorf("pdbmux returned %d for %s facts: %s", status, certname, snippet(body)) + } + + body, status, err = c.get(ctx, "/pdb/query/v4/factsets/"+url.PathEscape(certname)) + if err != nil { + return nil, err + } + if status == http.StatusNotFound { + return nil, fmt.Errorf("%w: %s", ErrNodeNotFound, certname) + } + if status != http.StatusOK { + 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 { + return nil, fmt.Errorf("parsing factset for %s: %w", certname, err) + } + if len(fs.Facts.Data) == 0 { + return nil, fmt.Errorf("%w: %s", ErrNodeNotFound, certname) + } + return fold(fs.Facts.Data) +} + +func (c *Client) get(ctx context.Context, path string) ([]byte, int, error) { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, c.BaseURL+path, nil) + if err != nil { + return nil, 0, err + } + req.Header.Set("Accept", "application/json") + + resp, err := c.HTTP.Do(req) + if err != nil { + return nil, 0, fmt.Errorf("pdbmux request failed: %w", err) + } + defer func() { _ = resp.Body.Close() }() + + body, err := io.ReadAll(resp.Body) + if err != nil { + return nil, resp.StatusCode, fmt.Errorf("reading pdbmux response: %w", err) + } + return body, resp.StatusCode, nil +} + +// 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) { + values := make(map[string]any, len(records)) + 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) + } + values[r.Name] = v + } + return values, nil +} + +func snippet(body []byte) string { + s := strings.TrimSpace(string(body)) + if len(s) > 200 { + return s[:200] + "..." + } + return s +} diff --git a/internal/pdbmux/pdbmux_test.go b/internal/pdbmux/pdbmux_test.go new file mode 100644 index 0000000..adffbce --- /dev/null +++ b/internal/pdbmux/pdbmux_test.go @@ -0,0 +1,159 @@ +package pdbmux + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "testing" + "time" +) + +const nodeFactsBody = `[ + {"certname":"host.example.net","environment":"develop","name":"id","value":"root"}, + {"certname":"host.example.net","environment":"develop","name":"os","value":{"family":"RedHat","release":{"major":"9"}}}, + {"certname":"host.example.net","environment":"develop","name":"processorcount","value":4} +]` + +const factsetBody = `{ + "certname":"host.example.net", + "environment":"develop", + "timestamp":"2026-09-12T05:26:11.807Z", + "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) { + t.Helper() + if facts["id"] != "root" { + t.Errorf("id = %v", facts["id"]) + } + if facts["processorcount"] != float64(4) { + t.Errorf("processorcount = %#v", facts["processorcount"]) + } + os, ok := facts["os"].(map[string]any) + if !ok { + t.Fatalf("os fact not structured: %#v", facts["os"]) + } + if os["family"] != "RedHat" { + t.Errorf("os.family = %v", os["family"]) + } + release, ok := os["release"].(map[string]any) + if !ok || release["major"] != "9" { + t.Errorf("nested os.release lost: %#v", os["release"]) + } +} + +func TestFactsFromNodesEndpoint(t *testing.T) { + var paths []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + paths = append(paths, r.URL.Path) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(nodeFactsBody)) + })) + defer srv.Close() + + 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) + if len(paths) != 1 || paths[0] != "/pdb/query/v4/nodes/host.example.net/facts" { + t.Errorf("unexpected requests: %v", paths) + } +} + +func TestFactsFallsBackToFactsets(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/pdb/query/v4/nodes/host.example.net/facts": + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"error":"No information is known about node host.example.net"}`)) + case "/pdb/query/v4/factsets/host.example.net": + _, _ = w.Write([]byte(factsetBody)) + default: + t.Errorf("unexpected path %s", r.URL.Path) + } + })) + defer srv.Close() + + 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 TestFactsEmptyArrayFallsBack(t *testing.T) { + 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(factsetBody)) + return + } + _, _ = w.Write([]byte(`[]`)) + })) + defer srv.Close() + + 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 TestFactsMissingNode(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"error":"No information is known about node nope.example.net"}`)) + })) + defer srv.Close() + + _, err := New(srv.URL, 5*time.Second).Facts(context.Background(), "nope.example.net") + if !errors.Is(err, ErrNodeNotFound) { + t.Fatalf("err = %v, want ErrNodeNotFound", err) + } +} + +func TestFactsServerError(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`boom`)) + })) + defer srv.Close() + + _, err := New(srv.URL, 5*time.Second).Facts(context.Background(), "host.example.net") + if err == nil { + t.Fatal("expected an error on HTTP 500") + } + if errors.Is(err, ErrNodeNotFound) { + t.Fatalf("500 misreported as a missing node: %v", err) + } +} + +func TestFactsBadJSON(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`not json`)) + })) + defer srv.Close() + + if _, err := New(srv.URL, 5*time.Second).Facts(context.Background(), "host.example.net"); err == nil { + t.Fatal("expected a parse error") + } +} + +func TestFactsContextCancelled(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + time.Sleep(200 * time.Millisecond) + _, _ = w.Write([]byte(nodeFactsBody)) + })) + defer srv.Close() + + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Millisecond) + defer cancel() + if _, err := New(srv.URL, 5*time.Second).Facts(ctx, "host.example.net"); err == nil { + t.Fatal("expected a timeout error") + } +} diff --git a/internal/puppetserver/puppetserver.go b/internal/puppetserver/puppetserver.go new file mode 100644 index 0000000..825d1f1 --- /dev/null +++ b/internal/puppetserver/puppetserver.go @@ -0,0 +1,279 @@ +// Package puppetserver compiles catalogs against an OpenVoxServer/Puppet +// Server 8 compiler over client mTLS. It is a separate client from pdbmux: +// different endpoint, different auth model. +package puppetserver + +import ( + "bytes" + "context" + "crypto/rand" + "crypto/tls" + "crypto/x509" + "encoding/json" + "errors" + "fmt" + "io" + "net/http" + "os" + "strings" + "time" + + "git.unkin.net/unkin/puppet-catalog-tools/internal/catalog" +) + +// Client talks to a puppetserver compiler. +type Client struct { + BaseURL string + HTTP *http.Client +} + +// New returns a client for baseURL using tlsCfg for transport and client auth. +func New(baseURL string, tlsCfg *tls.Config, timeout time.Duration) *Client { + return &Client{ + BaseURL: strings.TrimRight(baseURL, "/"), + HTTP: &http.Client{ + Timeout: timeout, + Transport: &http.Transport{TLSClientConfig: tlsCfg}, + }, + } +} + +// TLSConfig builds a client mTLS config from PEM files. certFile/keyFile are +// the client certificate issued by the estate's Puppet CA; caFile is the CA +// bundle used to verify the server. +func TLSConfig(certFile, keyFile, caFile string) (*tls.Config, error) { + cfg := &tls.Config{MinVersion: tls.VersionTLS12} + + if (certFile == "") != (keyFile == "") { + return nil, errors.New("--cert and --key must be given together") + } + if certFile != "" { + pair, err := tls.LoadX509KeyPair(certFile, keyFile) + if err != nil { + return nil, fmt.Errorf("loading client certificate: %w", err) + } + cfg.Certificates = []tls.Certificate{pair} + } + if caFile != "" { + pem, err := os.ReadFile(caFile) + if err != nil { + return nil, fmt.Errorf("reading CA bundle: %w", err) + } + pool := x509.NewCertPool() + if !pool.AppendCertsFromPEM(pem) { + return nil, fmt.Errorf("no certificates found in %s", caFile) + } + cfg.RootCAs = pool + } + return cfg, nil +} + +// CompileError is a catalog that the server refused to compile. +type CompileError struct { + Environment string + Status int + Kind string + Message string +} + +func (e *CompileError) Error() string { + msg := e.Message + if msg == "" { + msg = "no message returned" + } + if e.Kind != "" { + return fmt.Sprintf("catalog compilation failed in environment %q (HTTP %d, %s): %s", + e.Environment, e.Status, e.Kind, msg) + } + return fmt.Sprintf("catalog compilation failed in environment %q (HTTP %d): %s", + 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. +type EnvironmentMismatchError struct { + Certname string + Requested string + Returned string +} + +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", + e.Certname, e.Requested, e.Returned) +} + +// CompileRequest describes one catalog compilation. +type CompileRequest struct { + Certname string + Environment string + Facts map[string]any + TrustedFacts map[string]any +} + +type catalogRequest struct { + Certname string `json:"certname"` + Environment string `json:"environment"` + Persistence persistence `json:"persistence"` + Facts factValues `json:"facts"` + TrustedFacts factValues `json:"trusted_facts"` + TransactionUUID string `json:"transaction_uuid"` + Options map[string]any `json:"options"` +} + +type persistence struct { + Facts bool `json:"facts"` + Catalog bool `json:"catalog"` +} + +type factValues struct { + Values map[string]any `json:"values"` +} + +// Compile compiles one catalog. It always verifies that the catalog the server +// returned is for the environment that was requested. +func (c *Client) Compile(ctx context.Context, req CompileRequest) (*catalog.Catalog, error) { + facts := req.Facts + if facts == nil { + facts = map[string]any{} + } + trusted := req.TrustedFacts + if trusted == nil { + trusted = TrustedFacts(req.Certname) + } + + uuid, err := transactionUUID() + if err != nil { + return nil, err + } + + body, err := json.Marshal(catalogRequest{ + Certname: req.Certname, + Environment: req.Environment, + Persistence: persistence{Facts: false, Catalog: false}, + Facts: factValues{Values: facts}, + TrustedFacts: factValues{Values: trusted}, + TransactionUUID: uuid, + Options: map[string]any{ + "prefer_requested_environment": true, + "capture_logs": false, + "log_level": "info", + }, + }) + if err != nil { + return nil, fmt.Errorf("encoding catalog request: %w", err) + } + + httpReq, err := http.NewRequestWithContext(ctx, http.MethodPost, c.BaseURL+"/puppet/v4/catalog", bytes.NewReader(body)) + if err != nil { + return nil, err + } + httpReq.Header.Set("Content-Type", "application/json") + httpReq.Header.Set("Accept", "application/json") + + resp, err := c.HTTP.Do(httpReq) + if err != nil { + return nil, fmt.Errorf("catalog request to %s failed: %w", c.BaseURL, err) + } + defer func() { _ = resp.Body.Close() }() + + raw, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("reading catalog response: %w", err) + } + + if resp.StatusCode != http.StatusOK { + 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) + } + if cat.Environment != req.Environment { + return nil, &EnvironmentMismatchError{ + Certname: req.Certname, + Requested: req.Environment, + Returned: cat.Environment, + } + } + return &cat, nil +} + +// Health checks the unauthenticated /status/v1/simple endpoint. +func (c *Client) Health(ctx context.Context) error { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, c.BaseURL+"/status/v1/simple", nil) + if err != nil { + return err + } + resp, err := c.HTTP.Do(req) + if err != nil { + return fmt.Errorf("puppetserver %s unreachable: %w", c.BaseURL, err) + } + defer func() { _ = resp.Body.Close() }() + + body, err := io.ReadAll(resp.Body) + if err != nil { + return fmt.Errorf("reading puppetserver status: %w", err) + } + status := strings.TrimSpace(string(body)) + if resp.StatusCode != http.StatusOK || status != "running" { + return fmt.Errorf("puppetserver %s is not running (HTTP %d: %s)", c.BaseURL, resp.StatusCode, status) + } + return nil +} + +// compileError decodes a Puppet error body. Puppet Server reports the failure +// kind as "kind" or "issue_kind" depending on the error path. +func compileError(environment string, status int, raw []byte) error { + var body struct { + Kind string `json:"kind"` + IssueKind string `json:"issue_kind"` + Message string `json:"message"` + } + e := &CompileError{Environment: environment, Status: status} + if err := json.Unmarshal(raw, &body); err != nil { + e.Message = strings.TrimSpace(string(raw)) + } else { + e.Kind = body.Kind + if e.Kind == "" { + e.Kind = body.IssueKind + } + e.Message = body.Message + if e.Message == "" { + e.Message = strings.TrimSpace(string(raw)) + } + } + if len(e.Message) > 4000 { + e.Message = e.Message[:4000] + "..." + } + return e +} + +// TrustedFacts synthesises the trusted facts hash for a certname, matching the +// shape Puppet builds from a client certificate. +func TrustedFacts(certname string) map[string]any { + hostname, domain, _ := strings.Cut(certname, ".") + return map[string]any{ + "authenticated": "remote", + "certname": certname, + "extensions": map[string]any{}, + "hostname": hostname, + "domain": domain, + } +} + +func transactionUUID() (string, error) { + var b [16]byte + if _, err := rand.Read(b[:]); err != nil { + return "", fmt.Errorf("generating transaction uuid: %w", err) + } + b[6] = (b[6] & 0x0f) | 0x40 + b[8] = (b[8] & 0x3f) | 0x80 + return fmt.Sprintf("%x-%x-%x-%x-%x", b[0:4], b[4:6], b[6:8], b[8:10], b[10:16]), nil +} diff --git a/internal/puppetserver/puppetserver_test.go b/internal/puppetserver/puppetserver_test.go new file mode 100644 index 0000000..6e82363 --- /dev/null +++ b/internal/puppetserver/puppetserver_test.go @@ -0,0 +1,265 @@ +package puppetserver + +import ( + "context" + "crypto/tls" + "crypto/x509" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" +) + +const catalogBody = `{ + "tags":["settings"], + "name":"host.example.net", + "version":1757649971, + "code_id":null, + "catalog_uuid":"6e4f9c7a-1111-4a2b-9c3d-000000000001", + "catalog_format":1, + "environment":"benvin_test", + "resources":[ + {"type":"Stage","title":"main","tags":["stage"],"exported":false,"parameters":{"name":"main"}}, + {"type":"File","title":"/etc/motd","tags":["file"],"exported":false,"file":"/etc/puppetlabs/code/environments/benvin_test/manifests/site.pp","line":12,"parameters":{"ensure":"file","content":"hello"}} + ], + "edges":[{"source":"Stage[main]","target":"File[/etc/motd]"}], + "classes":["settings"] +}` + +// puppetErrorBody is the shape Puppet Server returns for a manifest that does +// not compile. +const puppetErrorBody = `{"kind":"puppet.server/compilation-error","message":"Server Error: Evaluation Error: Error while evaluating a Function Call, Could not find data item foo in any Hiera data file (file: /etc/puppetlabs/code/environments/benvin_test/manifests/site.pp, line: 12, column: 5) on node host.example.net","issue_kind":"RUNTIME_ERROR"}` + +func tlsClient(t *testing.T, srv *httptest.Server) *Client { + t.Helper() + pool := x509.NewCertPool() + pool.AddCert(srv.Certificate()) + return New(srv.URL, &tls.Config{RootCAs: pool, MinVersion: tls.VersionTLS12}, 5*time.Second) +} + +func TestCompileSuccess(t *testing.T) { + var got catalogRequest + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost || r.URL.Path != "/puppet/v4/catalog" { + t.Errorf("unexpected request %s %s", r.Method, r.URL.Path) + } + if err := json.NewDecoder(r.Body).Decode(&got); err != nil { + t.Errorf("decoding request: %v", err) + } + _, _ = w.Write([]byte(catalogBody)) + })) + defer srv.Close() + + cat, err := tlsClient(t, srv).Compile(context.Background(), CompileRequest{ + Certname: "host.example.net", + Environment: "benvin_test", + Facts: map[string]any{"os": map[string]any{"family": "RedHat"}}, + }) + if err != nil { + t.Fatalf("Compile: %v", err) + } + 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 { + t.Errorf("resource context lost: %+v", cat.Resources[1]) + } + + if got.Certname != "host.example.net" || got.Environment != "benvin_test" { + t.Errorf("request certname/environment wrong: %+v", got) + } + if got.Persistence.Facts || got.Persistence.Catalog { + t.Errorf("persistence must be off: %+v", got.Persistence) + } + if got.Options["prefer_requested_environment"] != true { + t.Errorf("prefer_requested_environment not set: %+v", got.Options) + } + if got.TransactionUUID == "" { + t.Error("no transaction_uuid sent") + } + if got.Facts.Values["os"] == nil { + t.Errorf("facts not sent: %+v", got.Facts) + } + if got.TrustedFacts.Values["certname"] != "host.example.net" { + t.Errorf("trusted facts not synthesised: %+v", got.TrustedFacts) + } +} + +func TestCompileError(t *testing.T) { + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(puppetErrorBody)) + })) + defer srv.Close() + + _, err := tlsClient(t, srv).Compile(context.Background(), CompileRequest{ + Certname: "host.example.net", + Environment: "benvin_test", + }) + var ce *CompileError + if !errors.As(err, &ce) { + t.Fatalf("err = %v (%T), want *CompileError", err, err) + } + if ce.Status != http.StatusBadRequest || ce.Kind != "puppet.server/compilation-error" { + t.Errorf("unexpected compile error: %+v", ce) + } + if ce.Environment != "benvin_test" { + t.Errorf("environment not recorded: %q", ce.Environment) + } + if !strings.Contains(ce.Error(), "Could not find data item foo") { + t.Errorf("puppet message lost: %s", ce.Error()) + } +} + +func TestCompileServerError(t *testing.T) { + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte("Internal Server Error")) + })) + defer srv.Close() + + _, err := tlsClient(t, srv).Compile(context.Background(), CompileRequest{ + Certname: "host.example.net", Environment: "benvin_test", + }) + var ce *CompileError + if !errors.As(err, &ce) || ce.Status != http.StatusInternalServerError { + t.Fatalf("err = %v, want a 500 CompileError", err) + } +} + +func TestCompileEnvironmentMismatch(t *testing.T) { + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(catalogBody)) + })) + defer srv.Close() + + _, err := tlsClient(t, srv).Compile(context.Background(), CompileRequest{ + Certname: "host.example.net", + Environment: "benvin_other", + }) + var me *EnvironmentMismatchError + if !errors.As(err, &me) { + t.Fatalf("err = %v (%T), want *EnvironmentMismatchError", err, err) + } + if me.Requested != "benvin_other" || me.Returned != "benvin_test" { + t.Errorf("mismatch detail wrong: %+v", me) + } + msg := me.Error() + if !strings.Contains(msg, "benvin_other") || !strings.Contains(msg, "benvin_test") || !strings.Contains(msg, "encapi") { + t.Errorf("mismatch message must name both environments and the ENC: %s", msg) + } +} + +func TestCompileTimeout(t *testing.T) { + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + time.Sleep(500 * time.Millisecond) + _, _ = w.Write([]byte(catalogBody)) + })) + defer srv.Close() + + pool := x509.NewCertPool() + pool.AddCert(srv.Certificate()) + c := New(srv.URL, &tls.Config{RootCAs: pool, MinVersion: tls.VersionTLS12}, 30*time.Millisecond) + + _, err := c.Compile(context.Background(), CompileRequest{ + Certname: "host.example.net", Environment: "benvin_test", + }) + if err == nil { + t.Fatal("expected a timeout error") + } + var ce *CompileError + if errors.As(err, &ce) { + t.Fatalf("timeout misreported as a compile error: %v", err) + } +} + +func TestCompileBadCatalogJSON(t *testing.T) { + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("not json")) + })) + defer srv.Close() + + if _, err := tlsClient(t, srv).Compile(context.Background(), CompileRequest{ + Certname: "host.example.net", Environment: "benvin_test", + }); err == nil { + t.Fatal("expected a parse error") + } +} + +func TestHealth(t *testing.T) { + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/status/v1/simple" { + t.Errorf("unexpected path %s", r.URL.Path) + } + _, _ = w.Write([]byte("running")) + })) + defer srv.Close() + + if err := tlsClient(t, srv).Health(context.Background()); err != nil { + t.Fatalf("Health: %v", err) + } +} + +func TestHealthUnhealthy(t *testing.T) { + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusServiceUnavailable) + _, _ = w.Write([]byte("starting")) + })) + defer srv.Close() + + if err := tlsClient(t, srv).Health(context.Background()); err == nil { + t.Fatal("expected an error for a non-running server") + } +} + +func TestTrustedFacts(t *testing.T) { + tf := TrustedFacts("host.example.net") + if tf["certname"] != "host.example.net" || tf["hostname"] != "host" || tf["domain"] != "example.net" { + t.Errorf("unexpected trusted facts: %+v", tf) + } + if tf["authenticated"] != "remote" { + t.Errorf("authenticated = %v", tf["authenticated"]) + } + if _, ok := tf["extensions"].(map[string]any); !ok { + t.Errorf("extensions must be a hash: %#v", tf["extensions"]) + } + + short := TrustedFacts("standalone") + if short["hostname"] != "standalone" || short["domain"] != "" { + t.Errorf("unqualified certname handled wrong: %+v", short) + } +} + +func TestTLSConfig(t *testing.T) { + if _, err := TLSConfig("cert.pem", "", ""); err == nil { + t.Fatal("expected an error when --key is missing") + } + if _, err := TLSConfig("/nonexistent/cert.pem", "/nonexistent/key.pem", ""); err == nil { + t.Fatal("expected an error for unreadable client certificate") + } + cfg, err := TLSConfig("", "", "") + if err != nil { + t.Fatalf("TLSConfig: %v", err) + } + if len(cfg.Certificates) != 0 || cfg.RootCAs != nil { + t.Errorf("empty config should carry no material: %+v", cfg) + } +} + +func TestTransactionUUIDUnique(t *testing.T) { + a, err := transactionUUID() + if err != nil { + t.Fatal(err) + } + b, err := transactionUUID() + if err != nil { + t.Fatal(err) + } + if a == b || len(a) != 36 { + t.Errorf("bad uuids %q %q", a, b) + } +} diff --git a/packaging/nfpm.yaml b/packaging/nfpm.yaml new file mode 100644 index 0000000..2ba43a1 --- /dev/null +++ b/packaging/nfpm.yaml @@ -0,0 +1,45 @@ +--- +# nfpm config for the puppet-catalog-tools RPM. +# Rendered through envsubst (see scripts/build-rpm.sh) then fed to `nfpm pkg`. + +name: ${PACKAGE_NAME} +version: ${PACKAGE_VERSION} +release: ${PACKAGE_RELEASE} +arch: ${PACKAGE_ARCH} +platform: ${PACKAGE_PLATFORM} +section: default +priority: extra +description: "${PACKAGE_DESCRIPTION}" + +maintainer: ${PACKAGE_MAINTAINER} +homepage: ${PACKAGE_HOMEPAGE} +license: ${PACKAGE_LICENSE} + +disable_globbing: false + +replaces: + - puppet-catalog-tools +provides: + - puppet-catalog-tools + +contents: + - src: dist/catalog-diff + dst: /usr/bin/catalog-diff + file_info: + mode: 0755 + owner: root + group: root + + # Shell completions (generated by scripts/build-rpm.sh before packaging). + - src: dist/completions/catalog-diff.bash + dst: /usr/share/bash-completion/completions/catalog-diff + file_info: + mode: 0644 + - src: dist/completions/_catalog-diff + dst: /usr/share/zsh/site-functions/_catalog-diff + file_info: + mode: 0644 + - src: dist/completions/catalog-diff.fish + dst: /usr/share/fish/vendor_completions.d/catalog-diff.fish + file_info: + mode: 0644 diff --git a/scripts/build-rpm.sh b/scripts/build-rpm.sh new file mode 100755 index 0000000..bfcb5b0 --- /dev/null +++ b/scripts/build-rpm.sh @@ -0,0 +1,49 @@ +#!/usr/bin/env bash +# +# Package the (already built) puppet-catalog-tools binaries into an RPM with +# nfpm, bundling generated bash/zsh/fish shell completions. +# Usage: scripts/build-rpm.sh [version] (version defaults to $CI_COMMIT_TAG) +# +set -euo pipefail + +ROOT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +cd "${ROOT_DIR}" + +VERSION="${1:-${CI_COMMIT_TAG:-0.0.0-dev}}" +VERSION="${VERSION#v}" # strip a leading v +PACKAGE="puppet-catalog-tools" +BINARIES=(catalog-diff) +DIST="dist" + +for b in "${BINARIES[@]}"; do + if [ ! -f "${DIST}/${b}" ]; then + echo "ERROR: ${DIST}/${b} not found; run 'make build' first" >&2 + exit 1 + fi +done + +# Generate shell completions from the freshly built binaries so they always +# match the shipped flags/subcommands. +COMP_DIR="${DIST}/completions" +mkdir -p "${COMP_DIR}" +for b in "${BINARIES[@]}"; do + "./${DIST}/${b}" completion bash >"${COMP_DIR}/${b}.bash" + "./${DIST}/${b}" completion zsh >"${COMP_DIR}/_${b}" + "./${DIST}/${b}" completion fish >"${COMP_DIR}/${b}.fish" +done + +export PACKAGE_NAME="${PACKAGE}" +export PACKAGE_VERSION="${VERSION}" +export PACKAGE_RELEASE="1" +export PACKAGE_ARCH="amd64" +export PACKAGE_PLATFORM="linux" +export PACKAGE_DESCRIPTION="Puppet catalog tooling: catalog-diff compiles a host's catalog in two environments and diffs them" +export PACKAGE_MAINTAINER="Ben Vincent " +export PACKAGE_HOMEPAGE="https://git.unkin.net/unkin/puppet-catalog-tools" +export PACKAGE_LICENSE="MIT" + +envsubst "${DIST}/nfpm.yaml" +nfpm pkg --config "${DIST}/nfpm.yaml" --target "${DIST}" --packager rpm + +echo "Built:" +ls -1 "${DIST}"/*.rpm From dcbd810c346a2c2c098d0eff21d22a6acf3415a4 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 12 Sep 2026 15:55:34 +1000 Subject: [PATCH 2/9] keep stderr quiet when the diff exits with differences --- cmd/catalog-diff/main.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cmd/catalog-diff/main.go b/cmd/catalog-diff/main.go index ddcf9f9..c689a8a 100644 --- a/cmd/catalog-diff/main.go +++ b/cmd/catalog-diff/main.go @@ -123,7 +123,10 @@ func main() { root.AddCommand(configCmd()) if err := root.Execute(); err != nil { - fmt.Fprintln(os.Stderr, "error:", err) + // errSilent only carries an exit code; the diff is already printed. + if !errors.Is(err, errSilent) { + fmt.Fprintln(os.Stderr, "error:", err) + } var ee *exitError if errors.As(err, &ee) { os.Exit(ee.code) From a76480d4547bef28bf8e5bd1b6a8e56625e9e6a1 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 12 Sep 2026 16:58:59 +1000 Subject: [PATCH 3/9] 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" + ] + } +} From 1aea8e8dbf03126290f6289989163f22a5b0e13f Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 12 Sep 2026 17:00:41 +1000 Subject: [PATCH 4/9] assert line 0 with a file counts as a real source location --- internal/catalog/diff_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/catalog/diff_test.go b/internal/catalog/diff_test.go index 30f6f26..e2b0c0e 100644 --- a/internal/catalog/diff_test.go +++ b/internal/catalog/diff_test.go @@ -381,8 +381,8 @@ func TestNormalizeKeepsMissingSourceDistinctFromLineZero(t *testing.T) { t.Errorf("manifest location lost: %+v", anchor) } - zero := Resource{Type: "File", Title: "/a", Line: intPtr(0)} - if !zero.HasSource() && zero.File != "" { + zero := Resource{Type: "File", Title: "/a", File: "site.pp", Line: intPtr(0)} + if !zero.HasSource() { t.Error("line 0 with a file is a real location") } } From 6e4ee5420318206c389de8a33a9e45c37e841021 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 12 Sep 2026 17:19:28 +1000 Subject: [PATCH 5/9] validate flag ranges and make exitError printable when empty A negative --retry skipped the compile loop, leaving a nil error inside exitError that panicked when printed. - reject a negative or oversized --retry and a non-positive --timeout as usage errors (exit 3) - print an exit code when exitError carries no inner error - cap the backoff shift so a large --retry cannot overflow - cancel the sibling compile on the first failure, and do not report that cancellation as the cause - report a context cancellation during the retry backoff instead of the stale compile error --- cmd/catalog-diff/main.go | 106 ++++++++++++++++++++++++---- cmd/catalog-diff/main_test.go | 129 +++++++++++++++++++++++++++++++++- 2 files changed, 218 insertions(+), 17 deletions(-) diff --git a/cmd/catalog-diff/main.go b/cmd/catalog-diff/main.go index 65857f1..11c1c66 100644 --- a/cmd/catalog-diff/main.go +++ b/cmd/catalog-diff/main.go @@ -30,6 +30,10 @@ const ( exitTransport = 3 // transport, auth, environment-mismatch or usage error ) +// maxRetry bounds --retry: past this the backoff is already pinned at its +// ceiling and the run outlives any review it could inform. +const maxRetry = 20 + const longHelp = `Compile a host's Puppet catalog in a base environment and in the environment built from the branch under test, then diff the two. @@ -72,8 +76,22 @@ type exitError struct { err error } -func (e *exitError) Error() string { return e.err.Error() } -func (e *exitError) Unwrap() error { return e.err } +func (e *exitError) Error() string { + if e == nil { + return "" + } + if e.err == nil { + return fmt.Sprintf("failed with exit code %d", e.code) + } + return e.err.Error() +} + +func (e *exitError) Unwrap() error { + if e == nil { + return nil + } + return e.err +} func fail(code int, format string, args ...any) *exitError { return &exitError{code: code, err: fmt.Errorf(format, args...)} @@ -170,6 +188,22 @@ func configCmd() *cobra.Command { return cmd } +// validate rejects flag values the rest of the run cannot act on. A negative +// retry count would skip the compile loop entirely and a non-positive timeout +// cancels every request before it is sent, so both are usage errors. +func validate(opts *options) error { + if opts.retry < 0 { + return fail(exitTransport, "--retry must be zero or more, got %d", opts.retry) + } + if opts.retry > maxRetry { + return fail(exitTransport, "--retry must be at most %d, got %d", maxRetry, opts.retry) + } + if opts.timeout <= 0 { + return fail(exitTransport, "--timeout must be positive, got %s", opts.timeout) + } + return nil +} + // resolveEnvironment turns the flags into the environment to test. func resolveEnvironment(opts *options) (string, error) { if opts.environment != "" && opts.branch != "" { @@ -198,6 +232,10 @@ func run(ctx context.Context, out io.Writer, hostname string, opts *options) err ctx = context.Background() } + if err := validate(opts); err != nil { + return err + } + testEnv, err := resolveEnvironment(opts) if err != nil { return err @@ -216,6 +254,7 @@ func run(ctx context.Context, out io.Writer, hostname string, opts *options) err server := puppetserver.New(opts.server, tlsCfg, opts.timeout) facts := pdbmux.New(opts.pdb, opts.timeout) + facts.Warn = func(msg string) { fmt.Fprintln(os.Stderr, "warning:", msg) } healthCtx, cancel := context.WithTimeout(ctx, 30*time.Second) defer cancel() @@ -234,7 +273,11 @@ func run(ctx context.Context, out io.Writer, hostname string, opts *options) err } // Both compilations are independent, so run them together: one host costs - // one compile's wall clock rather than two. + // one compile's wall clock rather than two. The first failure cancels its + // sibling — a quick manifest error should not wait out a full compile. + compileCtx, abandon := context.WithCancel(ctx) + defer abandon() + var ( wg sync.WaitGroup testCat, baseCat *catalog.Catalog @@ -243,19 +286,31 @@ func run(ctx context.Context, out io.Writer, hostname string, opts *options) err wg.Add(2) go func() { defer wg.Done() - testCat, testErr = compile(ctx, server, hostname, testEnv, factValues, opts) + testCat, testErr = compile(compileCtx, server, hostname, testEnv, factValues, opts) + if testErr != nil { + abandon() + } }() go func() { defer wg.Done() - baseCat, baseErr = compile(ctx, server, hostname, opts.base, factValues, opts) + baseCat, baseErr = compile(compileCtx, server, hostname, opts.base, factValues, opts) + if baseErr != nil { + abandon() + } }() wg.Wait() - // Report the test environment's failure first: it is the one under review. - if testErr != nil { + // Report the test environment's failure first: it is the one under review, + // unless it only stopped because the base compile failed first. + switch { + case testErr != nil && baseErr != nil: + if abandoned(ctx, testErr) { + return baseErr + } return testErr - } - if baseErr != nil { + case testErr != nil: + return testErr + case baseErr != nil: return baseErr } @@ -293,14 +348,25 @@ func run(ctx context.Context, out io.Writer, hostname string, opts *options) err // itself has already been printed. var errSilent = errors.New("differences present") +// abandoned reports whether err is only the fallout of cancelling a compile +// because its sibling failed first, rather than a failure in its own right. +func abandoned(ctx context.Context, err error) bool { + return ctx.Err() == nil && errors.Is(err, context.Canceled) +} + // compile runs one compilation, retrying transient failures, and maps the // final failure onto an exit code. func compile(ctx context.Context, server *puppetserver.Client, hostname, environment string, facts map[string]any, opts *options) (*catalog.Catalog, error) { var lastErr error for attempt := 0; attempt <= opts.retry; attempt++ { if attempt > 0 { + // The cancellation, not the error that earned the retry, is why + // this compile stopped. if err := sleepCtx(ctx, backoff(attempt)); err != nil { - break + if lastErr != nil { + err = fmt.Errorf("%w while retrying after: %v", err, lastErr) + } + return nil, &exitError{code: exitTransport, err: err} } } cat, err := server.Compile(ctx, puppetserver.CompileRequest{ @@ -318,6 +384,9 @@ func compile(ctx context.Context, server *puppetserver.Client, hostname, environ } } + if lastErr == nil { + return nil, &exitError{code: exitTransport, err: fmt.Errorf("no compile was attempted for %s in %s", hostname, environment)} + } var compileErr *puppetserver.CompileError if errors.As(lastErr, &compileErr) { return nil, &exitError{code: exitCompile, err: lastErr} @@ -346,13 +415,20 @@ func transient(err error) bool { } // backoff grows exponentially from 2s and adds up to 50% jitter, so parallel -// compiles do not retry in lockstep against an autoscaling compiler pool. +// compiles do not retry in lockstep against an autoscaling compiler pool. The +// shift is capped: an unbounded one overflows into a negative delay. func backoff(attempt int) time.Duration { - base := 2 * time.Second << (attempt - 1) - if base > 30*time.Second { - base = 30 * time.Second + const ( + start = 2 * time.Second + limit = 30 * time.Second + ) + d := limit + if attempt >= 1 && attempt < 32 { + if shifted := start << (attempt - 1); shifted > 0 && shifted < limit { + d = shifted + } } - return base + time.Duration(rand.Int63n(int64(base/2))) + return d + time.Duration(rand.Int63n(int64(d/2))) } func sleepCtx(ctx context.Context, d time.Duration) error { diff --git a/cmd/catalog-diff/main_test.go b/cmd/catalog-diff/main_test.go index e04148a..748fbcd 100644 --- a/cmd/catalog-diff/main_test.go +++ b/cmd/catalog-diff/main_test.go @@ -402,7 +402,7 @@ func TestRunUnknownResponseShapeDoesNotBlameTheENC(t *testing.T) { 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) " + + "(file: /etc/puppetlabs/code/environments/develop/site/profiles/manifests/app/client.pp, line: 31, column: 29) " + "on node host.example.net" func TestRunRetriesPuppetDBQueryFailures(t *testing.T) { @@ -443,7 +443,7 @@ func TestRunDoesNotRetryManifestErrorsReturnedAs500(t *testing.T) { 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")) + "Could not find class roles::app::demo::node for host.example.net on node host.example.net")) }) opts.environment = "benvin_test" opts.testonly = true @@ -458,3 +458,128 @@ func TestRunDoesNotRetryManifestErrorsReturnedAs500(t *testing.T) { t.Errorf("a manifest error must not be retried even at HTTP 500, saw %d attempts", attempts) } } + +func TestValidateRejectsOutOfRangeFlags(t *testing.T) { + tests := []struct { + name string + opts options + want string + }{ + {name: "negative retry", opts: options{retry: -1, timeout: time.Minute}, want: "--retry"}, + {name: "retry above the cap", opts: options{retry: maxRetry + 1, timeout: time.Minute}, want: "--retry"}, + {name: "zero timeout", opts: options{}, want: "--timeout"}, + {name: "negative timeout", opts: options{timeout: -time.Second}, want: "--timeout"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := validate(&tc.opts) + if code := exitCode(t, err); code != exitTransport { + t.Fatalf("exit %d, want %d", code, exitTransport) + } + if !strings.Contains(err.Error(), tc.want) { + t.Errorf("message must name %s: %v", tc.want, err) + } + }) + } +} + +func TestValidateAcceptsBoundaryValues(t *testing.T) { + for _, opts := range []options{ + {retry: 0, timeout: time.Nanosecond}, + {retry: maxRetry, timeout: time.Hour}, + } { + if err := validate(&opts); err != nil { + t.Errorf("validate(%+v) = %v, want nil", opts, err) + } + } +} + +func TestRunRejectsANegativeRetryWithoutPanicking(t *testing.T) { + opts := testServers(t, func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(catalogFor(readEnvironment(t, r), "same"))) + }) + opts.environment = "benvin_test" + opts.retry = -1 + + 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) + } + // main prints this before exiting; a nil inner error used to panic here. + if msg := err.Error(); !strings.Contains(msg, "--retry") { + t.Errorf("unhelpful usage error: %s", msg) + } +} + +func TestExitErrorWithNoInnerErrorIsPrintable(t *testing.T) { + e := &exitError{code: exitTransport} + if msg := e.Error(); !strings.Contains(msg, "3") { + t.Errorf("Error() = %q, want the exit code", msg) + } + if e.Unwrap() != nil { + t.Error("Unwrap of an empty exitError must be nil") + } + var nilErr *exitError + if msg := nilErr.Error(); msg == "" { + t.Error("a nil *exitError must still print something") + } +} + +func TestBackoffStaysWithinBounds(t *testing.T) { + for _, attempt := range []int{0, 1, 2, 5, 31, 32, 64, 1 << 30} { + d := backoff(attempt) + if d <= 0 || d > 45*time.Second { + t.Errorf("backoff(%d) = %s, out of range", attempt, d) + } + } +} + +func TestRunCancelsTheSiblingCompileOnFirstFailure(t *testing.T) { + const slow = 4 * time.Second + opts := testServers(t, func(w http.ResponseWriter, r *http.Request) { + if readEnvironment(t, r) == "develop" { + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(`{"kind":"puppet.server/compilation-error","message":"Syntax error"}`)) + return + } + select { + case <-r.Context().Done(): + case <-time.After(slow): + _, _ = w.Write([]byte(catalogFor("benvin_test", "new"))) + } + }) + opts.environment = "benvin_test" + + start := time.Now() + err := run(context.Background(), &bytes.Buffer{}, "host.example.net", opts) + elapsed := time.Since(start) + + // The base compile failed for real; the cancelled sibling is not the cause. + if code := exitCode(t, err); code != exitCompile { + t.Fatalf("exit %d, want %d (err %v)", code, exitCompile, err) + } + if elapsed > slow/2 { + t.Errorf("the sibling compile was not cancelled: run took %s", elapsed) + } +} + +func TestRunReportsCancellationDuringRetryBackoff(t *testing.T) { + opts := testServers(t, func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusBadGateway) + _, _ = w.Write([]byte("bad gateway")) + }) + opts.environment = "benvin_test" + opts.testonly = true + opts.retry = 5 + + ctx, cancel := context.WithTimeout(context.Background(), 500*time.Millisecond) + defer cancel() + + err := run(ctx, &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 !errors.Is(err, context.DeadlineExceeded) { + t.Errorf("the cancellation must be the reported cause, got %v", err) + } +} From 7f2c6f16c88155367053fa2a0d73486bfbf463a7 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 12 Sep 2026 17:19:28 +1000 Subject: [PATCH 6/9] 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) + } +} From 496f258cb013be18843e282b62440e7211ae1af3 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 12 Sep 2026 17:19:28 +1000 Subject: [PATCH 7/9] make the catalog fixture synthetic The capture carried a production certname, real role and class names and internal paths into a public repo. - replace hostnames, class names and paths with fake equivalents - keep the shape under test: catalog envelope, string version, null code_id, resources with and without parameters and file/line, the exported and sensitive_parameters cases - document the range of --retry and --timeout --- AGENTS.md | 19 ++- README.md | 4 +- internal/catalog/diff_test.go | 16 +- internal/puppetserver/puppetserver_test.go | 24 +-- .../testdata/catalog_response.json | 147 +++++++++--------- 5 files changed, 106 insertions(+), 104 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 8da86bf..3783807 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -35,7 +35,9 @@ Makefile and to `scripts/build-rpm.sh`, and add its files to facts in the body. Auth is client mTLS against the estate Puppet CA; the certname must be allowed in the puppetserver `auth.conf`. - Facts: `GET /pdb/query/v4/nodes//facts` on pdbmux, falling back to - `/pdb/query/v4/factsets/`. No auth, no client cert. + `/pdb/query/v4/factsets/`. No auth, no client cert. Real PuppetDB + answers the factsets route with an *array*; pdbmux currently answers with a + bare object. The fallback accepts both — do not narrow it to one. - The two services have different auth models and stay separate clients. ## Gotchas @@ -58,6 +60,12 @@ Makefile and to `scripts/build-rpm.sh`, and add its files to preserved). The rule lives in one function, `catalog.BranchToEnvironment`. - Exit codes are a contract: 0 same, 1 compile failed, 2 differences, 3 transport/auth/mismatch/usage. `--testonly` never returns 2. +- Numeric flags are validated up front (`validate`). A negative `--retry` used + to skip the compile loop and leave a nil error inside `exitError`; that type + now prints safely whatever it holds. +- The two compiles share a cancellable context: the first failure abandons its + sibling. An error that is only that cancellation is not the cause and is not + what gets reported. - Normalisation carries a pile of false-positive fixes (classes and exported resources skipped, `before`/`require` dropped, `notify`/`subscribe` sorted, File titles keyed by `path`, datatype-only changes suppressed). Removing one @@ -74,7 +82,8 @@ 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. +`internal/puppetserver/testdata/catalog_response.json` is a synthetic +OpenVoxServer 8.8 response, kept for its shape: the `catalog` envelope, a +string `version`, a null `code_id`, and resources that omit `parameters`, +`file` and `line`, plus the exported and `sensitive_parameters` cases. This +repo is public — hostnames, class names and paths in fixtures stay fake. diff --git a/README.md b/README.md index 90d7eb3..d67ec4e 100644 --- a/README.md +++ b/README.md @@ -35,8 +35,8 @@ catalog-diff web01.example.net --branch benvin/motd-change --testonly | `--server` | puppetserver base URL (default `https://puppet.k8s.syd1.au.unkin.net:8140`) | | `--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; a failed compiler-side PuppetDB query counts, a manifest error does not) | +| `--timeout` | per-compile timeout (default 10m; must be positive) | +| `--retry` | retries for transient compile or transport failures (default 2, range 0-20; 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 | diff --git a/internal/catalog/diff_test.go b/internal/catalog/diff_test.go index e2b0c0e..3f4fd1c 100644 --- a/internal/catalog/diff_test.go +++ b/internal/catalog/diff_test.go @@ -309,15 +309,15 @@ func TestDiffNoChanges(t *testing.T) { } } -// 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". +// Resources shaped like an 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", + "name":"web01.example.test","environment":"develop","version":"0123abc", "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":"Anchor","title":"examplemod::end","tags":["anchor","examplemod::end"],"file":"init.pp","line":111,"exported":false,"kind":"compilable_type"}, + {"type":"File","title":"/opt/puppetlabs/puppet/cache/examplemod/modules/example_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"}} ] }` @@ -325,7 +325,7 @@ const optionalFieldCatalog = `{ func TestNormalizeAcceptsMissingParameters(t *testing.T) { res := normalized(t, optionalFieldCatalog, Options{IncludeClasses: true}) - for _, key := range []string{"Class[Settings]", "Anchor[selinux::end]"} { + for _, key := range []string{"Class[Settings]", "Anchor[examplemod::end]"} { r, ok := res[key] if !ok { t.Fatalf("%s dropped: %v", key, res) @@ -376,7 +376,7 @@ func TestNormalizeKeepsMissingSourceDistinctFromLineZero(t *testing.T) { t.Errorf("an absent line must stay absent, got %d", *settings.Line) } - anchor := res["Anchor[selinux::end]"] + anchor := res["Anchor[examplemod::end]"] if !anchor.HasSource() || anchor.File != "init.pp" || *anchor.Line != 111 { t.Errorf("manifest location lost: %+v", anchor) } diff --git a/internal/puppetserver/puppetserver_test.go b/internal/puppetserver/puppetserver_test.go index f05a4dd..b108373 100644 --- a/internal/puppetserver/puppetserver_test.go +++ b/internal/puppetserver/puppetserver_test.go @@ -270,8 +270,8 @@ func TestTransactionUUIDUnique(t *testing.T) { } } -// wrappedResponse is a trimmed capture of a real OpenVoxServer 8.8 response: -// the catalog sits under a single "catalog" key. +// wrappedResponse is a synthetic OpenVoxServer 8.8 response shaped like the +// real thing: 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")) @@ -288,7 +288,7 @@ func compileFixture(t *testing.T, body []byte, environment string) (*catalog.Cat })) defer srv.Close() return tlsClient(t, srv).Compile(context.Background(), CompileRequest{ - Certname: "prodnxsr0009.main.unkin.net", Environment: environment, + Certname: "web01.example.test", Environment: environment, }) } @@ -297,14 +297,14 @@ func TestCompileUnwrapsServerResponse(t *testing.T) { if err != nil { t.Fatalf("Compile: %v", err) } - if cat.Environment != "develop" || cat.Name != "prodnxsr0009.main.unkin.net" { + if cat.Environment != "develop" || cat.Name != "web01.example.test" { 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"` { + if string(cat.Version) != `"0123456789abcdef0123456789abcdef01234567"` { t.Errorf("version = %s, want the string form the server sends", cat.Version) } } @@ -346,7 +346,7 @@ func TestCompileDecodesOptionalResourceFields(t *testing.T) { t.Errorf("a resource without file/line must report no source: %+v", settings) } - anchor := byKey["Anchor[selinux::end]"] + anchor := byKey["Anchor[examplemod::end]"] if anchor.Parameters != nil { t.Errorf("anchor parameters should be absent: %+v", anchor.Parameters) } @@ -354,17 +354,17 @@ func TestCompileDecodesOptionalResourceFields(t *testing.T) { t.Errorf("anchor manifest location lost: %+v", anchor) } - pkg := byKey["Package[openvox-agent]"] - if pkg.Params()["ensure"] != "7.37.2" || !pkg.HasSource() { + pkg := byKey["Package[example-agent]"] + if pkg.Params()["ensure"] != "1.2.3" || !pkg.HasSource() { t.Errorf("package decoded wrong: %+v", pkg) } - key := byKey["File[/etc/dns-updater/key]"] + key := byKey["File[/etc/example/secret.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]"] + member := byKey["Haproxy::Balancermember[web01.example.test_9443]"] if !member.Exported { t.Errorf("exported flag lost: %+v", member) } @@ -396,10 +396,10 @@ 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) " + + "(file: /etc/puppetlabs/code/environments/develop/site/profiles/manifests/app/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" + "Could not find class roles::app::demo::node for host.example.net on node host.example.net" tests := []struct { name string diff --git a/internal/puppetserver/testdata/catalog_response.json b/internal/puppetserver/testdata/catalog_response.json index 3a7d82b..827d488 100644 --- a/internal/puppetserver/testdata/catalog_response.json +++ b/internal/puppetserver/testdata/catalog_response.json @@ -2,12 +2,12 @@ "catalog": { "tags": [ "settings", - "roles::infra::incus::node" + "roles::app::demo::node" ], - "name": "prodnxsr0009.main.unkin.net", - "version": "979c188c3437099863e0b53c57942ef7ed6523a3", + "name": "web01.example.test", + "version": "0123456789abcdef0123456789abcdef01234567", "code_id": null, - "catalog_uuid": "8d7f423e-5bf1-4b64-97b4-4a20c67ec59f", + "catalog_uuid": "00000000-0000-4000-8000-0000000000aa", "catalog_format": 2, "environment": "develop", "resources": [ @@ -35,129 +35,124 @@ }, { "type": "Class", - "title": "Profiles::Puppet::Agent", + "title": "Profiles::App::Agent", "tags": [ "class", - "profiles::puppet::agent", + "profiles::app::agent", "profiles", - "puppet", + "app", "agent", "profiles::base", "base", - "roles::infra::incus::node", + "roles::app::demo::node", "roles", - "infra", - "incus", + "demo", "node" ], "exported": false, "kind": "unknown", "parameters": { - "version": "7.37.2", - "openvox_enable": true + "version": "1.2.3", + "manage_service": true } }, { "type": "Anchor", - "title": "selinux::end", + "title": "examplemod::end", "tags": [ "anchor", - "selinux::end", - "selinux", + "examplemod::end", + "examplemod", "end", "class", - "selinux::module", + "examplemod::module", "module", - "frr_local", - "profiles::selinux::frr", + "example_local", + "profiles::app::module", "profiles", - "frr", + "app", "profiles::base", "base", - "roles::infra::incus::node", + "roles::app::demo::node", "roles", - "infra", - "incus", + "demo", "node" ], - "file": "/etc/puppetlabs/code/environments/develop/external_modules/selinux/manifests/init.pp", + "file": "/etc/puppetlabs/code/environments/develop/external_modules/examplemod/manifests/init.pp", "line": 111, "exported": false, "kind": "compilable_type" }, { "type": "File", - "title": "/opt/puppetlabs/puppet/cache/puppet-selinux/modules/frr_local.loaded", + "title": "/opt/puppetlabs/puppet/cache/examplemod/modules/example_local.loaded", "tags": [ "file", - "selinux::module", - "selinux", + "examplemod::module", + "examplemod", "module", - "frr_local", + "example_local", "class", - "profiles::selinux::frr", + "profiles::app::module", "profiles", - "frr", + "app", "profiles::base", "base", - "roles::infra::incus::node", + "roles::app::demo::node", "roles", - "infra", - "incus", + "demo", "node" ], - "file": "/etc/puppetlabs/code/environments/develop/external_modules/selinux/manifests/module.pp", + "file": "/etc/puppetlabs/code/environments/develop/external_modules/examplemod/manifests/module.pp", "line": 177, "exported": false, "kind": "compilable_type" }, { "type": "Package", - "title": "openvox-agent", + "title": "example-agent", "tags": [ "package", - "openvox-agent", + "example-agent", "class", - "profiles::puppet::agent", + "profiles::app::agent", "profiles", - "puppet", + "app", "agent", "profiles::base", "base", - "roles::infra::incus::node", + "roles::app::demo::node", "roles", - "infra", - "incus", + "demo", "node" ], - "file": "/etc/puppetlabs/code/environments/develop/site/profiles/manifests/puppet/agent.pp", + "file": "/etc/puppetlabs/code/environments/develop/site/profiles/manifests/app/agent.pp", "line": 58, "exported": false, "kind": "compilable_type", "parameters": { - "ensure": "7.37.2", - "require": "Yumrepo[openvox]" + "ensure": "1.2.3", + "require": "Yumrepo[example]" } }, { "type": "Service", - "title": "puppet", + "title": "example-agent", "tags": [ "service", - "puppet", + "example-agent", "class", - "profiles::puppet::agent", + "profiles::app::agent", "profiles", "agent", "profiles::base", "base", - "roles::infra::incus::node", + "roles::app::demo::node", "roles", - "infra", - "incus", + "demo", "node" ], - "file": "/etc/puppetlabs/code/environments/develop/site/profiles/manifests/puppet/agent.pp", + "file": "/etc/puppetlabs/code/environments/develop/site/profiles/manifests/app/agent.pp", "line": 81, "exported": false, "kind": "compilable_type", @@ -165,29 +160,28 @@ "ensure": "running", "enable": true, "hasrestart": true, - "require": "Package[openvox-agent]" + "require": "Package[example-agent]" } }, { "type": "File", - "title": "/etc/dns-updater/key", + "title": "/etc/example/secret.key", "tags": [ "file", "class", - "profiles::dns::updater", + "profiles::app::secret", "profiles", - "dns", - "updater", - "profiles::dns::base", + "app", + "secret", + "profiles::app::base", "base", "profiles::base", - "roles::infra::incus::node", + "roles::app::demo::node", "roles", - "infra", - "incus", + "demo", "node" ], - "file": "/etc/puppetlabs/code/environments/develop/site/profiles/manifests/dns/updater.pp", + "file": "/etc/puppetlabs/code/environments/develop/site/profiles/manifests/app/secret.pp", "line": 97, "exported": false, "kind": "compilable_type", @@ -205,41 +199,40 @@ }, { "type": "Haproxy::Balancermember", - "title": "prodnxsr0009.main.unkin.net_9443", + "title": "web01.example.test_9443", "tags": [ - "be_ceph_dashboard_au-syd1-prod", + "be_example_dashboard_test", "haproxy::balancermember", "haproxy", "balancermember", - "prodnxsr0009.main.unkin.net_9443", - "profiles::haproxy::balancemember", + "web01.example.test_9443", + "profiles::app::balancermember", "profiles", - "balancemember", + "balancermember", "class", - "profiles::ceph::dashboard", - "ceph", + "profiles::app::dashboard", + "app", "dashboard", "profiles::base", "base", - "roles::infra::incus::node", + "roles::app::demo::node", "roles", - "infra", - "incus", + "demo", "node" ], - "file": "/etc/puppetlabs/code/environments/develop/site/profiles/manifests/haproxy/balancemember.pp", + "file": "/etc/puppetlabs/code/environments/develop/site/profiles/manifests/app/balancermember.pp", "line": 12, "exported": true, "kind": "defined_type", "parameters": { - "listening_service": "be_ceph_dashboard", + "listening_service": "be_example_dashboard", "ports": [ 9443 ], - "server_names": "prodnxsr0009", - "ipaddresses": "198.18.23.9", + "server_names": "web01", + "ipaddresses": "192.0.2.9", "options": [ - "cookie prodnxsr0009", + "cookie web01", "ssl", "verify none", "check", @@ -247,7 +240,7 @@ "rise 3", "fall 2" ], - "tag": "be_ceph_dashboard_au-syd1-prod" + "tag": "be_example_dashboard_test" } } ], @@ -263,7 +256,7 @@ ], "classes": [ "settings", - "profiles::puppet::agent" + "profiles::app::agent" ] } } From 5cbe4c87136701ab643658044a13a8cdde9580cd Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 12 Sep 2026 17:35:09 +1000 Subject: [PATCH 8/9] stop serving another node's factset Also strip a leading BOM and stop reporting a certname mismatch as a parse error. --- internal/pdbmux/pdbmux.go | 52 ++++++++++++----- internal/pdbmux/pdbmux_test.go | 103 ++++++++++++++++++++++++++++++++- 2 files changed, 140 insertions(+), 15 deletions(-) diff --git a/internal/pdbmux/pdbmux.go b/internal/pdbmux/pdbmux.go index ef43d6b..c99b042 100644 --- a/internal/pdbmux/pdbmux.go +++ b/internal/pdbmux/pdbmux.go @@ -83,7 +83,7 @@ func (c *Client) Facts(ctx context.Context, certname string) (map[string]any, er records, err := decodeFactsets(body, certname) if err != nil { - return nil, fmt.Errorf("parsing factset for %s: %w", certname, err) + return nil, err } if len(records) == 0 { return nil, fmt.Errorf("%w: %s", ErrNodeNotFound, certname) @@ -93,34 +93,60 @@ func (c *Client) Facts(ctx context.Context, certname string) (map[string]any, er // decodeFactsets reads the two shapes /factsets/ comes back as: real // PuppetDB returns an array of factsets, pdbmux currently returns a single -// bare object. +// bare object. A factset belonging to another node is never substituted for +// the one that was asked for. func decodeFactsets(body []byte, certname string) ([]factRecord, error) { + body = bytes.TrimPrefix(body, []byte("\xef\xbb\xbf")) + 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 + return nil, fmt.Errorf("parsing factset for %s: %w", certname, err) } if len(sets) == 0 { return nil, nil } - return nil, fmt.Errorf("%d factsets returned, none for the requested certname", len(sets)) + for _, fs := range sets { + if sameCertname(fs.Certname, certname) { + return fs.Facts.Data, nil + } + } + if len(sets) == 1 && sets[0].Certname == "" { + return sets[0].Facts.Data, nil + } + return nil, fmt.Errorf("no factset for %s: pdbmux returned %s", + certname, strings.Join(certnames(sets), ", ")) } var fs factset if err := json.Unmarshal(body, &fs); err != nil { - return nil, err + return nil, fmt.Errorf("parsing factset for %s: %w", certname, err) + } + if fs.Certname != "" && !sameCertname(fs.Certname, certname) { + return nil, fmt.Errorf("no factset for %s: pdbmux returned %s", certname, fs.Certname) } return fs.Facts.Data, nil } +// sameCertname compares certnames case-insensitively: they are DNS names, and +// PuppetDB matches the per-node route exactly, so folding can only absorb a +// backend that normalised case, never mask a different node. +func sameCertname(got, want string) bool { + return got != "" && strings.EqualFold(got, want) +} + +func certnames(sets []factset) []string { + names := make([]string, 0, len(sets)) + for _, fs := range sets { + if fs.Certname == "" { + names = append(names, "(unnamed)") + continue + } + names = append(names, fs.Certname) + } + return names +} + func (c *Client) get(ctx context.Context, path string) ([]byte, int, error) { req, err := http.NewRequestWithContext(ctx, http.MethodGet, c.BaseURL+path, nil) if err != nil { diff --git a/internal/pdbmux/pdbmux_test.go b/internal/pdbmux/pdbmux_test.go index d8149fe..888315a 100644 --- a/internal/pdbmux/pdbmux_test.go +++ b/internal/pdbmux/pdbmux_test.go @@ -225,8 +225,107 @@ func TestFactsRejectsAFactsetArrayWithNoMatch(t *testing.T) { 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) + assertNamesBoth(t, err, "host.example.net", "a.example.net", "b.example.net") +} + +// assertNamesBoth checks the mismatch error names the node asked for and the +// ones that came back, and that it is not dressed up as a parse failure. +func assertNamesBoth(t *testing.T, err error, want string, got ...string) { + t.Helper() + if !strings.Contains(err.Error(), want) { + t.Errorf("error does not name the requested certname %s: %v", want, err) + } + for _, g := range got { + if !strings.Contains(err.Error(), g) { + t.Errorf("error does not name the returned certname %s: %v", g, err) + } + } + if strings.Contains(err.Error(), "parsing factset") { + t.Errorf("a certname mismatch is not a parse failure: %v", err) + } +} + +func TestFactsRejectsASoleFactsetForAnotherNode(t *testing.T) { + srv := factsetServer(t, `[{ + "certname":"totally-different-host.example.net", + "facts":{"data":[ + {"name":"id","value":"nobody"}, + {"name":"os","value":{"family":"Debian"}} + ]} + }]`) + + facts, err := New(srv.URL, 5*time.Second).Facts(context.Background(), "host.example.net") + if err == nil { + t.Fatalf("another node's facts were served as ours: %#v", facts) + } + assertNamesBoth(t, err, "host.example.net", "totally-different-host.example.net") +} + +func TestFactsRejectsABareFactsetForAnotherNode(t *testing.T) { + srv := factsetServer(t, `{ + "certname":"totally-different-host.example.net", + "facts":{"data":[{"name":"id","value":"nobody"}]} + }`) + + facts, err := New(srv.URL, 5*time.Second).Facts(context.Background(), "host.example.net") + if err == nil { + t.Fatalf("another node's facts were served as ours: %#v", facts) + } + assertNamesBoth(t, err, "host.example.net", "totally-different-host.example.net") +} + +// The per-node route already names the node, so a factset that asserts no +// certname of its own cannot be contradicting it. +func TestFactsAcceptsAFactsetWithNoCertname(t *testing.T) { + body := `{"facts":{"data":[ + {"name":"id","value":"root"}, + {"name":"os","value":{"family":"RedHat","release":{"major":"9"}}}, + {"name":"processorcount","value":4} + ]}}` + + for name, resp := range map[string]string{"object": body, "array": "[" + body + "]"} { + t.Run(name, func(t *testing.T) { + srv := factsetServer(t, resp) + 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 TestFactsMatchesCertnameCaseInsensitively(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.HasPrefix(r.URL.Path, "/pdb/query/v4/factsets/") { + _, _ = w.Write([]byte(factsetBody)) + return + } + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"error":"unknown node"}`)) + })) + defer srv.Close() + + 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 TestFactsToleratesALeadingBOM(t *testing.T) { + for name, resp := range map[string]string{ + "array": "\ufeff" + factsetBody, + "object": "\ufeff" + bareFactsetBody, + } { + t.Run(name, func(t *testing.T) { + srv := factsetServer(t, resp) + 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) + }) } } From 3f14554faed70d10359bb5f5f9f38c9a3c44d667 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 12 Sep 2026 17:49:57 +1000 Subject: [PATCH 9/9] read facts from PuppetDB's query API - rename internal/pdbmux to internal/puppetdb - fetch facts with one GET /pdb/query/v4/facts AST query per node - drop the per-node fallback route and its two response shapes - rename --pdb to --puppetdb, CATALOG_DIFF_PDB to CATALOG_DIFF_PUPPETDB and pdb_url to puppetdb_url - describe the fact source generically in the README and AGENTS.md --- AGENTS.md | 12 +- README.md | 12 +- cmd/catalog-diff/main.go | 10 +- cmd/catalog-diff/main_test.go | 12 +- internal/config/config.go | 15 +- internal/config/config_test.go | 8 +- internal/pdbmux/pdbmux.go | 204 ----------- internal/puppetdb/puppetdb.go | 182 ++++++++++ .../puppetdb_test.go} | 339 +++++++----------- internal/puppetserver/puppetserver.go | 2 +- 10 files changed, 355 insertions(+), 441 deletions(-) delete mode 100644 internal/pdbmux/pdbmux.go create mode 100644 internal/puppetdb/puppetdb.go rename internal/{pdbmux/pdbmux_test.go => puppetdb/puppetdb_test.go} (53%) diff --git a/AGENTS.md b/AGENTS.md index 3783807..caa3e4a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -17,7 +17,7 @@ cmd/catalog-diff/ # the CLI (flags, exit codes, orchestration) internal/catalog/ # catalog types, normalisation, diff engine, renderers, # branch.go (BranchToEnvironment) internal/puppetserver/ # mTLS client for POST /puppet/v4/catalog + /status/v1/simple -internal/pdbmux/ # unauthenticated fact client for pdbmux +internal/puppetdb/ # unauthenticated fact client for PuppetDB internal/config/ # XDG config file + env var loading packaging/nfpm.yaml # RPM spec (envsubst-templated) scripts/build-rpm.sh # completions + nfpm packaging @@ -34,10 +34,10 @@ Makefile and to `scripts/build-rpm.sh`, and add its files to `options.prefer_requested_environment: true`, facts and synthesised trusted facts in the body. Auth is client mTLS against the estate Puppet CA; the certname must be allowed in the puppetserver `auth.conf`. -- Facts: `GET /pdb/query/v4/nodes//facts` on pdbmux, falling back to - `/pdb/query/v4/factsets/`. No auth, no client cert. Real PuppetDB - answers the factsets route with an *array*; pdbmux currently answers with a - bare object. The fallback accepts both — do not narrow it to one. +- Facts: `GET /pdb/query/v4/facts` with the AST query + `["=","certname",""]`. No auth, no client cert. The response is a + flat array of `{certname, environment, name, value}` records; a record for + another node is never folded into the result. - The two services have different auth models and stay separate clients. ## Gotchas @@ -78,7 +78,7 @@ make test # go test -v -race ./... ``` Everything is stubbed with `httptest` (a TLS server for puppetserver, a plain -one for pdbmux) — no live estate access is required. Renderer output is +one for PuppetDB) — no live estate access is required. Renderer output is covered by golden files under `internal/catalog/testdata`; regenerate with `go test ./internal/catalog -update`. diff --git a/README.md b/README.md index d67ec4e..d39bd41 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ CLI tooling for the Puppet estate. Currently one binary: the environment built from the branch under test, then diffs the two. Catalogs are compiled over the puppetserver API (`POST /puppet/v4/catalog`) -with facts read from `pdbmux`. Nothing is compiled locally and nothing is +with facts read from PuppetDB. Nothing is compiled locally and nothing is persisted on the server. ## Install @@ -33,7 +33,7 @@ catalog-diff web01.example.net --branch benvin/motd-change --testonly | `--testonly` | compile the test environment only; result via exit code | | `--json` | machine-readable output | | `--server` | puppetserver base URL (default `https://puppet.k8s.syd1.au.unkin.net:8140`) | -| `--pdb` | pdbmux base URL (default `https://pdbmux.k8s.syd1.au.unkin.net`) | +| `--puppetdb` | PuppetDB base URL (default `https://pdbmux.k8s.syd1.au.unkin.net`) | | `--cert` / `--key` / `--cacert` | client mTLS material | | `--timeout` | per-compile timeout (default 10m; must be positive) | | `--retry` | retries for transient compile or transport failures (default 2, range 0-20; a failed compiler-side PuppetDB query counts, a manifest error does not) | @@ -67,7 +67,7 @@ Config file: `$XDG_CONFIG_HOME/puppet-catalog-tools/config.yaml` ```yaml server_url: https://puppet.k8s.syd1.au.unkin.net:8140 -pdb_url: https://pdbmux.k8s.syd1.au.unkin.net +puppetdb_url: https://pdbmux.k8s.syd1.au.unkin.net cert: /etc/puppetlabs/puppet/ssl/certs/catalog-diff.pem key: /etc/puppetlabs/puppet/ssl/private_keys/catalog-diff.pem cacert: /etc/puppetlabs/puppet/ssl/certs/ca.pem @@ -77,15 +77,15 @@ base_environment: develop | Env var | Config key | |---|---| | `CATALOG_DIFF_SERVER` | `server_url` | -| `CATALOG_DIFF_PDB` | `pdb_url` | +| `CATALOG_DIFF_PUPPETDB` | `puppetdb_url` | | `CATALOG_DIFF_CERT` | `cert` | | `CATALOG_DIFF_KEY` | `key` | | `CATALOG_DIFF_CACERT` | `cacert` | | `CATALOG_DIFF_BASE_ENVIRONMENT` | `base_environment` | `catalog-diff` needs a client certificate whose certname is allowed to -`POST /puppet/v4/catalog` in the puppetserver `auth.conf`. pdbmux needs no -credentials. +`POST /puppet/v4/catalog` in the puppetserver `auth.conf`. PuppetDB is queried +without credentials. ## Branch to environment name diff --git a/cmd/catalog-diff/main.go b/cmd/catalog-diff/main.go index 11c1c66..c56f85b 100644 --- a/cmd/catalog-diff/main.go +++ b/cmd/catalog-diff/main.go @@ -16,7 +16,7 @@ import ( "git.unkin.net/unkin/puppet-catalog-tools/internal/catalog" "git.unkin.net/unkin/puppet-catalog-tools/internal/config" - "git.unkin.net/unkin/puppet-catalog-tools/internal/pdbmux" + "git.unkin.net/unkin/puppet-catalog-tools/internal/puppetdb" "git.unkin.net/unkin/puppet-catalog-tools/internal/puppetserver" ) @@ -56,7 +56,7 @@ type options struct { testonly bool jsonOut bool server string - pdb string + puppetdb string cert string key string cacert string @@ -125,7 +125,7 @@ func main() { f.BoolVar(&opts.testonly, "testonly", false, "compile the test environment only; no output, result via exit code") f.BoolVar(&opts.jsonOut, "json", false, "machine-readable output") f.StringVar(&opts.server, "server", cfg.ServerURL, "puppetserver base URL") - f.StringVar(&opts.pdb, "pdb", cfg.PDBURL, "pdbmux base URL") + f.StringVar(&opts.puppetdb, "puppetdb", cfg.PuppetDBURL, "PuppetDB base URL") f.StringVar(&opts.cert, "cert", cfg.Cert, "client certificate PEM (CATALOG_DIFF_CERT)") f.StringVar(&opts.key, "key", cfg.Key, "client private key PEM (CATALOG_DIFF_KEY)") f.StringVar(&opts.cacert, "cacert", cfg.CACert, "CA bundle PEM used to verify puppetserver (CATALOG_DIFF_CACERT)") @@ -177,7 +177,7 @@ func configCmd() *cobra.Command { } fmt.Printf("config file : %s\n", config.Path()) fmt.Printf("server_url : %s\n", cfg.ServerURL) - fmt.Printf("pdb_url : %s\n", cfg.PDBURL) + fmt.Printf("puppetdb_url : %s\n", cfg.PuppetDBURL) fmt.Printf("cert : %s\n", cfg.Cert) fmt.Printf("key : %s\n", cfg.Key) fmt.Printf("cacert : %s\n", cfg.CACert) @@ -253,7 +253,7 @@ func run(ctx context.Context, out io.Writer, hostname string, opts *options) err } server := puppetserver.New(opts.server, tlsCfg, opts.timeout) - facts := pdbmux.New(opts.pdb, opts.timeout) + facts := puppetdb.New(opts.puppetdb, opts.timeout) facts.Warn = func(msg string) { fmt.Fprintln(os.Stderr, "warning:", msg) } healthCtx, cancel := context.WithTimeout(ctx, 30*time.Second) diff --git a/cmd/catalog-diff/main_test.go b/cmd/catalog-diff/main_test.go index 748fbcd..76bd51a 100644 --- a/cmd/catalog-diff/main_test.go +++ b/cmd/catalog-diff/main_test.go @@ -27,7 +27,7 @@ func catalogFor(environment, content string) string { ]}` } -// testServers stands up a fake puppetserver (TLS) and pdbmux (plain), and +// testServers stands up a fake puppetserver (TLS) and PuppetDB (plain), and // returns options wired to them with the server's own cert as the CA bundle. func testServers(t *testing.T, compile http.HandlerFunc) *options { t.Helper() @@ -53,11 +53,11 @@ func testServers(t *testing.T, compile http.HandlerFunc) *options { } return &options{ - base: "develop", - server: puppet.URL, - pdb: pdb.URL, - cacert: caPath, - timeout: 10 * time.Second, + base: "develop", + server: puppet.URL, + puppetdb: pdb.URL, + cacert: caPath, + timeout: 10 * time.Second, } } diff --git a/internal/config/config.go b/internal/config/config.go index 586bb36..05e93c6 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -18,8 +18,8 @@ const ( // DefaultServerURL is the k8s OpenVoxServer compiler pool. The legacy VM // masters behind puppet.query.consul remain reachable via --server. DefaultServerURL = "https://puppet.k8s.syd1.au.unkin.net:8140" - // DefaultPDBURL is pdbmux, the PuppetDB-compatible merging proxy. - DefaultPDBURL = "https://pdbmux.k8s.syd1.au.unkin.net" + // DefaultPuppetDBURL is the PuppetDB facts are read from. + DefaultPuppetDBURL = "https://pdbmux.k8s.syd1.au.unkin.net" // DefaultBaseEnvironment is the environment branches are diffed against. DefaultBaseEnvironment = "develop" ) @@ -27,7 +27,7 @@ const ( // Config holds every configurable value. Fields map 1:1 to config file keys. type Config struct { ServerURL string `yaml:"server_url"` - PDBURL string `yaml:"pdb_url"` + PuppetDBURL string `yaml:"puppetdb_url"` Cert string `yaml:"cert"` Key string `yaml:"key"` CACert string `yaml:"cacert"` @@ -38,7 +38,7 @@ type Config struct { func Default() Config { return Config{ ServerURL: DefaultServerURL, - PDBURL: DefaultPDBURL, + PuppetDBURL: DefaultPuppetDBURL, BaseEnvironment: DefaultBaseEnvironment, } } @@ -73,7 +73,7 @@ func Load() (Config, error) { env := map[string]*string{ "CATALOG_DIFF_SERVER": &cfg.ServerURL, - "CATALOG_DIFF_PDB": &cfg.PDBURL, + "CATALOG_DIFF_PUPPETDB": &cfg.PuppetDBURL, "CATALOG_DIFF_CERT": &cfg.Cert, "CATALOG_DIFF_KEY": &cfg.Key, "CATALOG_DIFF_CACERT": &cfg.CACert, @@ -103,8 +103,9 @@ func WriteDefault() (string, error) { return "", err } header := "# puppet-catalog-tools configuration\n" + - "# Overridable with CATALOG_DIFF_SERVER, CATALOG_DIFF_PDB, CATALOG_DIFF_CERT,\n" + - "# CATALOG_DIFF_KEY, CATALOG_DIFF_CACERT, CATALOG_DIFF_BASE_ENVIRONMENT.\n\n" + "# Overridable with CATALOG_DIFF_SERVER, CATALOG_DIFF_PUPPETDB,\n" + + "# CATALOG_DIFF_CERT, CATALOG_DIFF_KEY, CATALOG_DIFF_CACERT,\n" + + "# CATALOG_DIFF_BASE_ENVIRONMENT.\n\n" if err := os.WriteFile(path, append([]byte(header), data...), 0o644); err != nil { return "", fmt.Errorf("writing config: %w", err) } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index dae3ad0..9f1ea22 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -13,7 +13,7 @@ func TestLoadDefaults(t *testing.T) { if err != nil { t.Fatalf("Load: %v", err) } - if cfg.ServerURL != DefaultServerURL || cfg.PDBURL != DefaultPDBURL { + if cfg.ServerURL != DefaultServerURL || cfg.PuppetDBURL != DefaultPuppetDBURL { t.Errorf("unexpected defaults: %+v", cfg) } if cfg.BaseEnvironment != DefaultBaseEnvironment { @@ -28,7 +28,7 @@ func TestLoadPrecedence(t *testing.T) { if err := os.MkdirAll(filepath.Join(dir, appName), 0o755); err != nil { t.Fatal(err) } - body := "server_url: https://file.example:8140\npdb_url: https://file.example\ncert: /file/cert.pem\n" + body := "server_url: https://file.example:8140\npuppetdb_url: https://file.example\ncert: /file/cert.pem\n" if err := os.WriteFile(Path(), []byte(body), 0o644); err != nil { t.Fatal(err) } @@ -56,8 +56,8 @@ func TestLoadPrecedence(t *testing.T) { if cfg.Cert != "/env/cert.pem" { t.Errorf("cert = %q", cfg.Cert) } - if cfg.PDBURL != "https://file.example" { - t.Errorf("unset env clobbered file value: %q", cfg.PDBURL) + if cfg.PuppetDBURL != "https://file.example" { + t.Errorf("unset env clobbered file value: %q", cfg.PuppetDBURL) } } diff --git a/internal/pdbmux/pdbmux.go b/internal/pdbmux/pdbmux.go deleted file mode 100644 index c99b042..0000000 --- a/internal/pdbmux/pdbmux.go +++ /dev/null @@ -1,204 +0,0 @@ -// Package pdbmux reads node facts from pdbmux, the estate's PuppetDB -// compatible merging proxy. It is plain HTTPS, GET-only and unauthenticated — -// deliberately a separate client from the mTLS puppetserver one. -package pdbmux - -import ( - "bytes" - "context" - "encoding/json" - "errors" - "fmt" - "io" - "net/http" - "net/url" - "strings" - "time" -) - -// ErrNodeNotFound is returned when pdbmux knows nothing about a certname. -var ErrNodeNotFound = errors.New("node not found in pdbmux") - -// Client queries 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. -func New(baseURL string, timeout time.Duration) *Client { - return &Client{ - BaseURL: strings.TrimRight(baseURL, "/"), - HTTP: &http.Client{Timeout: timeout}, - } -} - -type factRecord struct { - Name string `json:"name"` - Value json.RawMessage `json:"value"` -} - -type factset 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 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 { - return nil, err - } - if status == http.StatusOK { - var records []factRecord - if err := json.Unmarshal(body, &records); err != nil { - return nil, fmt.Errorf("parsing facts for %s: %w", certname, err) - } - if len(records) > 0 { - 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)) - } - - body, status, err = c.get(ctx, "/pdb/query/v4/factsets/"+url.PathEscape(certname)) - if err != nil { - return nil, err - } - if status == http.StatusNotFound { - return nil, fmt.Errorf("%w: %s", ErrNodeNotFound, certname) - } - if status != http.StatusOK { - return nil, fmt.Errorf("pdbmux returned %d for %s factset: %s", status, certname, snippet(body)) - } - - records, err := decodeFactsets(body, certname) - if err != nil { - return nil, err - } - if len(records) == 0 { - return nil, fmt.Errorf("%w: %s", ErrNodeNotFound, certname) - } - 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. A factset belonging to another node is never substituted for -// the one that was asked for. -func decodeFactsets(body []byte, certname string) ([]factRecord, error) { - body = bytes.TrimPrefix(body, []byte("\xef\xbb\xbf")) - - if bytes.HasPrefix(bytes.TrimLeft(body, " \t\r\n"), []byte("[")) { - var sets []factset - if err := json.Unmarshal(body, &sets); err != nil { - return nil, fmt.Errorf("parsing factset for %s: %w", certname, err) - } - if len(sets) == 0 { - return nil, nil - } - for _, fs := range sets { - if sameCertname(fs.Certname, certname) { - return fs.Facts.Data, nil - } - } - if len(sets) == 1 && sets[0].Certname == "" { - return sets[0].Facts.Data, nil - } - return nil, fmt.Errorf("no factset for %s: pdbmux returned %s", - certname, strings.Join(certnames(sets), ", ")) - } - - var fs factset - if err := json.Unmarshal(body, &fs); err != nil { - return nil, fmt.Errorf("parsing factset for %s: %w", certname, err) - } - if fs.Certname != "" && !sameCertname(fs.Certname, certname) { - return nil, fmt.Errorf("no factset for %s: pdbmux returned %s", certname, fs.Certname) - } - return fs.Facts.Data, nil -} - -// sameCertname compares certnames case-insensitively: they are DNS names, and -// PuppetDB matches the per-node route exactly, so folding can only absorb a -// backend that normalised case, never mask a different node. -func sameCertname(got, want string) bool { - return got != "" && strings.EqualFold(got, want) -} - -func certnames(sets []factset) []string { - names := make([]string, 0, len(sets)) - for _, fs := range sets { - if fs.Certname == "" { - names = append(names, "(unnamed)") - continue - } - names = append(names, fs.Certname) - } - return names -} - -func (c *Client) get(ctx context.Context, path string) ([]byte, int, error) { - req, err := http.NewRequestWithContext(ctx, http.MethodGet, c.BaseURL+path, nil) - if err != nil { - return nil, 0, err - } - req.Header.Set("Accept", "application/json") - - resp, err := c.HTTP.Do(req) - if err != nil { - return nil, 0, fmt.Errorf("pdbmux request failed: %w", err) - } - defer func() { _ = resp.Body.Close() }() - - body, err := io.ReadAll(resp.Body) - if err != nil { - return nil, resp.StatusCode, fmt.Errorf("reading pdbmux response: %w", err) - } - return body, resp.StatusCode, nil -} - -// fold turns fact records into the flat name -> value map the catalog API -// 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 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 -} - -func snippet(body []byte) string { - s := strings.TrimSpace(string(body)) - if len(s) > 200 { - return s[:200] + "..." - } - return s -} diff --git a/internal/puppetdb/puppetdb.go b/internal/puppetdb/puppetdb.go new file mode 100644 index 0000000..b3812a1 --- /dev/null +++ b/internal/puppetdb/puppetdb.go @@ -0,0 +1,182 @@ +// Package puppetdb reads node facts from PuppetDB's query API. It is GET-only +// and unauthenticated — deliberately a separate client from the mTLS +// puppetserver one. +package puppetdb + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "io" + "net/http" + "net/url" + "strings" + "time" +) + +// ErrNodeNotFound is returned when PuppetDB knows no facts for a certname. +var ErrNodeNotFound = errors.New("node not found in PuppetDB") + +// Client queries PuppetDB. +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. +func New(baseURL string, timeout time.Duration) *Client { + return &Client{ + BaseURL: strings.TrimRight(baseURL, "/"), + HTTP: &http.Client{Timeout: timeout}, + } +} + +type factRecord struct { + Certname string `json:"certname"` + Name string `json:"name"` + Value json.RawMessage `json:"value"` +} + +// Facts returns the node's facts as the values map a catalog request needs. +// It reads /pdb/query/v4/facts with an AST query selecting the certname, which +// answers with a flat array of fact records. +func (c *Client) Facts(ctx context.Context, certname string) (map[string]any, error) { + query, err := json.Marshal([]string{"=", "certname", certname}) + if err != nil { + return nil, fmt.Errorf("building facts query for %s: %w", certname, err) + } + + body, status, err := c.get(ctx, "/pdb/query/v4/facts", string(query)) + if err != nil { + return nil, err + } + if status == http.StatusNotFound { + return nil, fmt.Errorf("%w: %s", ErrNodeNotFound, certname) + } + if status != http.StatusOK { + return nil, fmt.Errorf("puppetdb returned %d for %s facts: %s", status, certname, snippet(body)) + } + + records, err := decodeFacts(body, certname) + if err != nil { + return nil, err + } + if len(records) == 0 { + return nil, fmt.Errorf("%w: %s", ErrNodeNotFound, certname) + } + return c.fold(certname, records) +} + +// decodeFacts reads the fact records the query returns. A record belonging to +// another node is never substituted for the one that was asked for. A record +// that carries no certname of its own cannot be contradicting the query that +// named one, so it is kept. +func decodeFacts(body []byte, certname string) ([]factRecord, error) { + body = bytes.TrimPrefix(body, []byte("\xef\xbb\xbf")) + + var records []factRecord + if err := json.Unmarshal(body, &records); err != nil { + return nil, fmt.Errorf("parsing facts for %s: %w", certname, err) + } + + kept := make([]factRecord, 0, len(records)) + for _, r := range records { + if r.Certname == "" || sameCertname(r.Certname, certname) { + kept = append(kept, r) + } + } + if len(kept) == 0 && len(records) > 0 { + return nil, fmt.Errorf("no facts for %s: puppetdb returned %s", + certname, strings.Join(certnames(records), ", ")) + } + return kept, nil +} + +// sameCertname compares certnames case-insensitively: they are DNS names, so +// folding can only absorb a certname that was normalised for case, never mask +// a different node. +func sameCertname(got, want string) bool { + return got != "" && strings.EqualFold(got, want) +} + +// certnames lists the distinct nodes a response spoke for, in the order they +// appeared, so a mismatch error names what actually came back. +func certnames(records []factRecord) []string { + seen := make(map[string]bool, len(records)) + names := make([]string, 0, len(records)) + for _, r := range records { + name := r.Certname + if name == "" { + name = "(unnamed)" + } + if seen[name] { + continue + } + seen[name] = true + names = append(names, name) + } + return names +} + +func (c *Client) get(ctx context.Context, path, query string) ([]byte, int, error) { + target := c.BaseURL + path + "?" + url.Values{"query": {query}}.Encode() + req, err := http.NewRequestWithContext(ctx, http.MethodGet, target, nil) + if err != nil { + return nil, 0, err + } + req.Header.Set("Accept", "application/json") + + resp, err := c.HTTP.Do(req) + if err != nil { + return nil, 0, fmt.Errorf("puppetdb request failed: %w", err) + } + defer func() { _ = resp.Body.Close() }() + + body, err := io.ReadAll(resp.Body) + if err != nil { + return nil, resp.StatusCode, fmt.Errorf("reading puppetdb response: %w", err) + } + return body, resp.StatusCode, nil +} + +// fold turns fact records into the flat name -> value map the catalog API +// 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 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 +} + +func snippet(body []byte) string { + s := strings.TrimSpace(string(body)) + if len(s) > 200 { + return s[:200] + "..." + } + return s +} diff --git a/internal/pdbmux/pdbmux_test.go b/internal/puppetdb/puppetdb_test.go similarity index 53% rename from internal/pdbmux/pdbmux_test.go rename to internal/puppetdb/puppetdb_test.go index 888315a..7d9b450 100644 --- a/internal/pdbmux/pdbmux_test.go +++ b/internal/puppetdb/puppetdb_test.go @@ -1,7 +1,8 @@ -package pdbmux +package puppetdb import ( "context" + "encoding/json" "errors" "net/http" "net/http/httptest" @@ -10,36 +11,14 @@ import ( "time" ) -const nodeFactsBody = `[ +// The shape PuppetDB answers /pdb/query/v4/facts with: a flat array of fact +// records, each naming the node it belongs to. +const factsBody = `[ {"certname":"host.example.net","environment":"develop","name":"id","value":"root"}, {"certname":"host.example.net","environment":"develop","name":"os","value":{"family":"RedHat","release":{"major":"9"}}}, {"certname":"host.example.net","environment":"develop","name":"processorcount","value":4} ]` -// 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", - "facts":{"data":[ - {"name":"id","value":"root"}, - {"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) { t.Helper() if facts["id"] != "root" { @@ -61,61 +40,98 @@ func assertFacts(t *testing.T, facts map[string]any) { } } -func TestFactsFromNodesEndpoint(t *testing.T) { - var paths []string +// factsServer answers the query route with body and fails the test on any +// other path. +func factsServer(t *testing.T, body string) *httptest.Server { + t.Helper() srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - paths = append(paths, r.URL.Path) - w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(nodeFactsBody)) - })) - defer srv.Close() - - 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) - if len(paths) != 1 || paths[0] != "/pdb/query/v4/nodes/host.example.net/facts" { - t.Errorf("unexpected requests: %v", paths) - } -} - -func TestFactsFallsBackToFactsets(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - switch r.URL.Path { - case "/pdb/query/v4/nodes/host.example.net/facts": - w.WriteHeader(http.StatusNotFound) - _, _ = w.Write([]byte(`{"error":"No information is known about node host.example.net"}`)) - case "/pdb/query/v4/factsets/host.example.net": - _, _ = w.Write([]byte(factsetBody)) - default: + if r.URL.Path != "/pdb/query/v4/facts" { t.Errorf("unexpected path %s", r.URL.Path) - } - })) - defer srv.Close() - - 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 TestFactsEmptyArrayFallsBack(t *testing.T) { - 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(factsetBody)) + w.WriteHeader(http.StatusNotFound) return } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(body)) + })) + t.Cleanup(srv.Close) + return srv +} + +func TestFactsQueriesByCertname(t *testing.T) { + var paths, queries []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + paths = append(paths, r.URL.Path) + queries = append(queries, r.URL.Query().Get("query")) + _, _ = w.Write([]byte(factsBody)) + })) + defer srv.Close() + + 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) + if len(paths) != 1 || paths[0] != "/pdb/query/v4/facts" { + t.Errorf("unexpected requests: %v", paths) + } + if len(queries) != 1 || queries[0] != `["=","certname","host.example.net"]` { + t.Errorf("unexpected query: %v", queries) + } +} + +// A certname with characters the AST would otherwise have to escape must +// survive both JSON encoding and query-string encoding intact. +func TestFactsEscapesTheCertnameInTheQuery(t *testing.T) { + var got string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + got = r.URL.Query().Get("query") _, _ = w.Write([]byte(`[]`)) })) defer srv.Close() + const certname = `we"ird &host` + _, _ = New(srv.URL, 5*time.Second).Facts(context.Background(), certname) + + var ast []string + if err := json.Unmarshal([]byte(got), &ast); err != nil { + t.Fatalf("query %q is not valid JSON: %v", got, err) + } + if len(ast) != 3 || ast[0] != "=" || ast[1] != "certname" || ast[2] != certname { + t.Errorf("query = %q, decoded %#v", got, ast) + } +} + +// The response shape as PuppetDB really returns it, structured facts and all. +func TestFactsDecodesARealResponse(t *testing.T) { + srv := factsServer(t, `[ + {"certname":"host.example.net","environment":"develop","name":"id","value":"root"}, + {"certname":"host.example.net","environment":"develop","name":"processorcount","value":4}, + {"certname":"host.example.net","environment":"develop","name":"os","value":{"name":"AlmaLinux","distro":{"id":"AlmaLinux","release":{"full":"9.7","major":"9","minor":"7"}},"family":"RedHat","release":{"full":"9.7","major":"9","minor":"7"},"selinux":{"enabled":false},"architecture":"x86_64"}} + ]`) + 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) + os := facts["os"].(map[string]any) + if os["architecture"] != "x86_64" { + t.Errorf("os.architecture = %v", os["architecture"]) + } + selinux, ok := os["selinux"].(map[string]any) + if !ok || selinux["enabled"] != false { + t.Errorf("nested os.selinux lost: %#v", os["selinux"]) + } +} + +// An unknown node is not an HTTP error: the query simply matches nothing. +func TestFactsEmptyResultIsAMissingNode(t *testing.T) { + srv := factsServer(t, `[]`) + + _, err := New(srv.URL, 5*time.Second).Facts(context.Background(), "nope.example.net") + if !errors.Is(err, ErrNodeNotFound) { + t.Fatalf("err = %v, want ErrNodeNotFound", err) + } } func TestFactsMissingNode(t *testing.T) { @@ -148,10 +164,7 @@ func TestFactsServerError(t *testing.T) { } func TestFactsBadJSON(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - _, _ = w.Write([]byte(`not json`)) - })) - defer srv.Close() + srv := factsServer(t, `not json`) if _, err := New(srv.URL, 5*time.Second).Facts(context.Background(), "host.example.net"); err == nil { t.Fatal("expected a parse error") @@ -161,7 +174,7 @@ func TestFactsBadJSON(t *testing.T) { func TestFactsContextCancelled(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { time.Sleep(200 * time.Millisecond) - _, _ = w.Write([]byte(nodeFactsBody)) + _, _ = w.Write([]byte(factsBody)) })) defer srv.Close() @@ -172,62 +185,6 @@ func TestFactsContextCancelled(t *testing.T) { } } -// 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") - } - assertNamesBoth(t, err, "host.example.net", "a.example.net", "b.example.net") -} - // assertNamesBoth checks the mismatch error names the node asked for and the // ones that came back, and that it is not dressed up as a parse failure. func assertNamesBoth(t *testing.T, err error, want string, got ...string) { @@ -240,19 +197,16 @@ func assertNamesBoth(t *testing.T, err error, want string, got ...string) { t.Errorf("error does not name the returned certname %s: %v", g, err) } } - if strings.Contains(err.Error(), "parsing factset") { + if strings.Contains(err.Error(), "parsing facts") { t.Errorf("a certname mismatch is not a parse failure: %v", err) } } -func TestFactsRejectsASoleFactsetForAnotherNode(t *testing.T) { - srv := factsetServer(t, `[{ - "certname":"totally-different-host.example.net", - "facts":{"data":[ - {"name":"id","value":"nobody"}, - {"name":"os","value":{"family":"Debian"}} - ]} - }]`) +func TestFactsRejectsRecordsForAnotherNode(t *testing.T) { + srv := factsServer(t, `[ + {"certname":"totally-different-host.example.net","name":"id","value":"nobody"}, + {"certname":"totally-different-host.example.net","name":"os","value":{"family":"Debian"}} + ]`) facts, err := New(srv.URL, 5*time.Second).Facts(context.Background(), "host.example.net") if err == nil { @@ -261,50 +215,52 @@ func TestFactsRejectsASoleFactsetForAnotherNode(t *testing.T) { assertNamesBoth(t, err, "host.example.net", "totally-different-host.example.net") } -func TestFactsRejectsABareFactsetForAnotherNode(t *testing.T) { - srv := factsetServer(t, `{ - "certname":"totally-different-host.example.net", - "facts":{"data":[{"name":"id","value":"nobody"}]} - }`) +func TestFactsRejectsAMixedResponseNamingEveryNode(t *testing.T) { + srv := factsServer(t, `[ + {"certname":"a.example.net","name":"id","value":"a"}, + {"certname":"b.example.net","name":"id","value":"b"} + ]`) - facts, err := New(srv.URL, 5*time.Second).Facts(context.Background(), "host.example.net") + _, err := New(srv.URL, 5*time.Second).Facts(context.Background(), "host.example.net") if err == nil { - t.Fatalf("another node's facts were served as ours: %#v", facts) + t.Fatal("expected an error when no record matches the certname") } - assertNamesBoth(t, err, "host.example.net", "totally-different-host.example.net") + assertNamesBoth(t, err, "host.example.net", "a.example.net", "b.example.net") } -// The per-node route already names the node, so a factset that asserts no -// certname of its own cannot be contradicting it. -func TestFactsAcceptsAFactsetWithNoCertname(t *testing.T) { - body := `{"facts":{"data":[ +func TestFactsDropsAForeignRecordFromAnOtherwiseMatchingResponse(t *testing.T) { + srv := factsServer(t, `[ + {"certname":"host.example.net","name":"id","value":"root"}, + {"certname":"other.example.net","name":"id","value":"nobody"}, + {"certname":"host.example.net","name":"os","value":{"family":"RedHat","release":{"major":"9"}}}, + {"certname":"host.example.net","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) +} + +// The query already names the node, so a record that asserts no certname of +// its own cannot be contradicting it. +func TestFactsAcceptsRecordsWithNoCertname(t *testing.T) { + srv := factsServer(t, `[ {"name":"id","value":"root"}, {"name":"os","value":{"family":"RedHat","release":{"major":"9"}}}, {"name":"processorcount","value":4} - ]}}` + ]`) - for name, resp := range map[string]string{"object": body, "array": "[" + body + "]"} { - t.Run(name, func(t *testing.T) { - srv := factsetServer(t, resp) - 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) - }) + 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 TestFactsMatchesCertnameCaseInsensitively(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if strings.HasPrefix(r.URL.Path, "/pdb/query/v4/factsets/") { - _, _ = w.Write([]byte(factsetBody)) - return - } - w.WriteHeader(http.StatusNotFound) - _, _ = w.Write([]byte(`{"error":"unknown node"}`)) - })) - defer srv.Close() + srv := factsServer(t, factsBody) facts, err := New(srv.URL, 5*time.Second).Facts(context.Background(), "HOST.example.net") if err != nil { @@ -314,40 +270,22 @@ func TestFactsMatchesCertnameCaseInsensitively(t *testing.T) { } func TestFactsToleratesALeadingBOM(t *testing.T) { - for name, resp := range map[string]string{ - "array": "\ufeff" + factsetBody, - "object": "\ufeff" + bareFactsetBody, - } { - t.Run(name, func(t *testing.T) { - srv := factsetServer(t, resp) - 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) - }) - } -} + srv := factsServer(t, "\ufeff"+factsBody) -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) + 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 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() + srv := factsServer(t, `[ + {"certname":"host.example.net","name":"id","value":"root"}, + {"certname":"host.example.net","name":"broken"}, + {"certname":"host.example.net","name":"os","value":{"family":"RedHat","release":{"major":"9"}}}, + {"certname":"host.example.net","name":"processorcount","value":4} + ]`) var warnings []string c := New(srv.URL, 5*time.Second) @@ -367,10 +305,7 @@ func TestFactsSkipsUndecodableRecords(t *testing.T) { } 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() + srv := factsServer(t, `[{"name":"a"},{"name":"b"}]`) _, err := New(srv.URL, 5*time.Second).Facts(context.Background(), "host.example.net") if err == nil { diff --git a/internal/puppetserver/puppetserver.go b/internal/puppetserver/puppetserver.go index 7a2720c..f8f47e0 100644 --- a/internal/puppetserver/puppetserver.go +++ b/internal/puppetserver/puppetserver.go @@ -1,5 +1,5 @@ // Package puppetserver compiles catalogs against an OpenVoxServer/Puppet -// Server 8 compiler over client mTLS. It is a separate client from pdbmux: +// Server 8 compiler over client mTLS. It is a separate client from puppetdb: // different endpoint, different auth model. package puppetserver