diff --git a/.gitignore b/.gitignore index eabcfb3..3e85f49 100644 --- a/.gitignore +++ b/.gitignore @@ -1,5 +1,3 @@ -# built binaries (repo root only — not the cmd/ source dirs) -/catalog-diff -# cross-compiled release artifacts (e.g. catalog-diff-linux-amd64) -/catalog-diff-* +# build output: binaries, completions, cross-compiled release artifacts and +# packages all land under dist/ dist/ diff --git a/.woodpecker/release.yaml b/.woodpecker/release.yaml index bc9e653..edcb424 100644 --- a/.woodpecker/release.yaml +++ b/.woodpecker/release.yaml @@ -17,8 +17,8 @@ steps: memory: 2Gi cpu: 2 - # Build every binary into dist/ (consumed by the RPM step) plus the - # cross-platform binaries attached to the Gitea release. + # Build every binary into dist/bin (consumed by the RPM step) plus the + # cross-platform binaries attached to the Gitea release, in dist/release. - name: build image: git.unkin.net/unkin/almalinux9-gobuilder:20260606 commands: @@ -27,12 +27,13 @@ steps: # 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 + mkdir -p dist/release + for name in $$(make print-binaries); 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}" + -o "dist/release/$${name}-$${os}-$${arch}" "./cmd/$${name}" done done depends_on: [test] @@ -126,11 +127,12 @@ steps: 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}" + # Everything to attach lives in dist/release, so the asset list is the + # directory rather than a hand-kept list of binary names. 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 \ + if [ -n "$$RPM" ]; then cp "$$RPM" dist/release/; fi + (cd dist/release && sha256sum * >../sha256sums.txt && mv ../sha256sums.txt .) + tea releases assets create "${CI_COMMIT_TAG}" dist/release/* \ --login gitea --repo "${CI_REPO}" depends_on: [upload-rpm] backend_options: diff --git a/AGENTS.md b/AGENTS.md index caa3e4a..2107411 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -9,14 +9,20 @@ 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. +- **`catalog-impact`** — maps a control repo change onto the hosts it can + affect, by reading hieradata paths as hierarchy positions and tracing + manifest classes backwards to role classes. ## Structure ``` cmd/catalog-diff/ # the CLI (flags, exit codes, orchestration) +cmd/catalog-impact/ # the CLI (flags, output modes) internal/catalog/ # catalog types, normalisation, diff engine, renderers, # branch.go (BranchToEnvironment) internal/puppetserver/ # mTLS client for POST /puppet/v4/catalog + /status/v1/simple +internal/impact/ # path -> selector rules, repo index, reverse lookup, + # estate fact table, renderers internal/puppetdb/ # unauthenticated fact client for PuppetDB internal/config/ # XDG config file + env var loading packaging/nfpm.yaml # RPM spec (envsubst-templated) @@ -24,9 +30,9 @@ 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`. +Adding a binary: create `cmd//`. The Makefile, `scripts/build-rpm.sh`, +`packaging/nfpm.yaml` and the release pipeline all take the shipped list from +`cmd/`, so nothing else needs editing. ## Transport @@ -38,6 +44,10 @@ Makefile and to `scripts/build-rpm.sh`, and add its files to `["=","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. +- Estate-wide facts: the same endpoint and record shape, with an `extract` + query selecting the wanted fact names (`puppetdb.Client.FactsByName`). The + estate is ~150 nodes, so `catalog-impact` pulls the whole table once and + filters in Go rather than querying per host. - The two services have different auth models and stay separate clients. ## Gotchas @@ -71,6 +81,23 @@ Makefile and to `scripts/build-rpm.sh`, and add its files to File titles keyed by `path`, datatype-only changes suppressed). Removing one floods the output with noise. +## catalog-impact gotchas + +- The reverse lookup must search hieradata as well as manifests. `profiles::base` + declares `($hiera_include - $hiera_exclude).include`, and some modules — the + `jellyfin` one, for instance — are applied *only* by a `hiera_include` list in + a role yaml. A manifest-only search reports them as orphans. +- `hieradata/roles//.yaml` is a **prefix** match on the role tiers, not + an exact one. `site/roles/manifests/.../.pp` is exact. +- The bias is to over-include: an unmapped path widens to the whole estate and + says so on stderr. Never turn a widening into a silent empty result. +- The same applies in the parser. A class reference it cannot reduce to literal + names becomes an `UnresolvedRef` against its file, and every class that + reference could have named is treated as reaching the classes the file + declares. Never drop a reference the parser failed on. +- The repo index comes from the `--repo` checkout, not from `--branch`. CI runs + on the branch checkout, so the two agree there. + ## Testing ```bash diff --git a/Makefile b/Makefile index 12715cd..a510f08 100644 --- a/Makefile +++ b/Makefile @@ -1,25 +1,31 @@ -# 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 +# All shipped binaries, discovered from cmd/. Every tool is a separate main +# package there, so adding one needs no edit here. +BINARIES := $(notdir $(wildcard cmd/*)) DIST := dist +BINDIR := $(DIST)/bin +COMPDIR := $(DIST)/completions 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 +.PHONY: all build test lint fmt pre-commit clean install completions print-binaries rpm rpm-package patch minor major _tag all: build -# Build every binary into dist/ so the nfpm packaging step +# Build every binary into dist/bin 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; \ + CGO_ENABLED=0 GOOS=$(OS) GOARCH=$(ARCH) go build $(GOFLAGS) -o $(BINDIR)/$$b ./cmd/$$b || exit 1; \ done +# The binary list, for CI steps that cross-compile the same set. +print-binaries: + @echo $(BINARIES) + test: go test -v -race ./... @@ -33,18 +39,19 @@ pre-commit: pre-commit run --all-files clean: - rm -rf $(DIST) $(BINARIES) + rm -rf $(DIST) install: go install $(GOFLAGS) ./... -# Generate bash/zsh/fish completions for every binary into dist/completions. +# Generate bash/zsh/fish completions for every binary into dist/completions, +# named and laid out exactly as they install (the RPM globs these directories). completions: build - @mkdir -p $(DIST)/completions + @mkdir -p $(COMPDIR)/bash $(COMPDIR)/zsh $(COMPDIR)/fish @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; \ + $(BINDIR)/$$b completion bash > $(COMPDIR)/bash/$$b; \ + $(BINDIR)/$$b completion zsh > $(COMPDIR)/zsh/_$$b; \ + $(BINDIR)/$$b completion fish > $(COMPDIR)/fish/$$b.fish; \ done # Build the binaries then package them (with completions) into an RPM via nfpm. diff --git a/README.md b/README.md index d39bd41..b7beb59 100644 --- a/README.md +++ b/README.md @@ -1,9 +1,11 @@ # puppet-catalog-tools -CLI tooling for the Puppet estate. Currently one binary: +CLI tooling for the Puppet estate: - **`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. +- **`catalog-impact`** — works out which hosts a control repo change can + affect, and prints their hostnames. Catalogs are compiled over the puppetserver API (`POST /puppet/v4/catalog`) with facts read from PuppetDB. Nothing is compiled locally and nothing is @@ -14,9 +16,11 @@ persisted on the server. ```bash dnf install puppet-catalog-tools # from the artifactapi rpm-internal repo # or -make build # -> dist/catalog-diff +make build # -> dist/bin/ ``` +# catalog-diff + ## Usage ```bash @@ -122,6 +126,142 @@ 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. +# catalog-impact + +Given a change to the `puppet-prod` control repo, `catalog-impact` works out +which files it touches and which hosts those files affect, then prints the +affected hostnames — one per line, sorted and deduplicated, ready to pipe into +a fan-out that compiles each host's catalog. + +```bash +catalog-impact -r ~/src/puppet-prod # current branch vs origin/develop +catalog-impact -r ~/src/puppet-prod -b benvin/motd --explain +catalog-impact --files hieradata/common.yaml --json +git diff --name-only origin/develop... | paste -sd, | xargs catalog-impact --files +``` + +| Flag | Description | +|---|---| +| `-r, --repo` | path to a puppet-prod checkout (default `.`) | +| `-b, --branch` | branch under test (default: the repo's current branch) | +| `--base` | base ref to diff against (default `origin/develop`) | +| `--files` | explicit changed-file list, bypassing git entirely | +| `--puppetdb` | PuppetDB base URL (default `https://pdbmux.k8s.syd1.au.unkin.net`) | +| `--json` | structured output with per-host reasons | +| `--explain` | human-readable: for each host, which changed files hit it and why | +| `--limit` | cap the number of hosts emitted (0 = no cap) | +| `--timeout` | PuppetDB request timeout (default 1m) | + +`--puppetdb` defaults to the shared configuration described under +[Configuration](#configuration), so `puppetdb_url` and `CATALOG_DIFF_PUPPETDB` +apply to both binaries. No credentials are needed. + +Facts for the whole estate are fetched from PuppetDB in a single query and every +selector is evaluated in memory, so the cost is one HTTP request regardless of +how many files changed. + +## How a changed file becomes a host list + +Classification runs through an exec ENC and `manifests/site.pp` is empty, so a +node's `enc_role` fact is its classification. Everything below keys off that +and the facts `hiera.yaml` interpolates. + +**Hieradata paths become fact selectors**, mirroring the hierarchy: + +| Path | Selects | +|---|---| +| `hieradata/nodes/.yaml` | that node | +| `hieradata/roles/[/[/]]` | role-tier **prefix** — `roles/infra/puppet.yaml` hits every `roles::infra::puppet::*` host | +| `hieradata/country//region//[/...]` | country and region and role-tier prefix | +| `hieradata/country//roles/[/...]` | country and role-tier prefix | +| `hieradata/country//region/` | country and region | +| `hieradata/country/` | country | +| `hieradata/virtual/.yaml` | `virtual` == v | +| `hieradata/os//[.].yaml` | `os.name` and `os.release.major` (and minor when present) | +| `hieradata/os//all_releases.yaml` | every host with that `os.name` | +| `hieradata/common.{yaml,eyaml}` | every host | + +**Repo-global files select every host**: `hiera.yaml`, `environment.conf`, +`manifests/site.pp`, `Puppetfile`. The Puppetfile pins external modules whose +code is not in this repo, so there is nothing to trace — mapping it to the +whole estate is the deliberate conservative choice. `lib/` under any module is +pluginsynced to every agent and is treated the same way. + +**Code paths become classes**, then the classes are traced backwards: + +- `site/roles/manifests///.pp` is class + `roles::::::`, which maps to hosts by exact `enc_role`. A role + class is a leaf — nothing includes it, the ENC assigns it. +- `site/profiles/manifests/**.pp` and `modules//manifests/**.pp` become + their autoloaded class names (`init.pp` counts as both the namespace and + `::init`) and need a reverse lookup. +- `modules//{templates,files}/**` and `site/profiles/{templates,files}/**` + are attributed to the classes whose manifests reference them through + `template()`, `epp()` or `puppet:///modules/...`. An unreferenced asset falls + back to every class its module defines. + +## The reverse lookup + +Finding what applies a class means searching two places, not one: + +1. `.pp` files under `site/` and `modules/` for `include`, `contain`, + `inherits`, `class { '...': }`, capitalised resource references, + `create_resources()`, `$class::var` reads and defined-type usage. Comments + are stripped with quote tracking and heredoc bodies are skipped, so a `#` in + a string never truncates a line and documentation in a heredoc never becomes + an edge. +2. `hieradata/**/*.{yaml,eyaml}` for the class as a list item under + `hiera_include:`. `profiles::base` declares + `($hiera_include - $hiera_exclude).include` with a `unique` merge across the + whole hierarchy, and it is included by essentially every role. + +The second is not an edge case. `hieradata/roles/apps/media/jellyfin.yaml` is +the only thing that applies the `jellyfin` module — there is no `include +jellyfin` anywhere — so a manifest-only search reports it as orphaned and finds +no hosts. A `hiera_include` hit is resolved through the hieradata path rules +above: the position of the listing file in the hierarchy decides which hosts +get the class. `hiera_exclude` entries are subtracted the same way. + +The walk runs to a fixed point, because profiles include profiles several +layers deep — `profiles::base` alone pulls in around twenty-five. Cycles are +visited once. + +## Bias and limits + +A missed host means an unvalidated catalog, so every ambiguity widens: + +- a path matching no rule is reported on stderr and mapped to every host; +- an asset no manifest references widens to its whole module; +- an unrecognised hieradata path in a `hiera_include` hit widens to every host; +- a class reference that is not a literal name is reported as unresolved and + widened. A selector yields every literal name in its arms, and a bare + variable is followed through the assignments in its own file. What is left — + `include "profile::role_${role}"`, `include lookup('x')` — is recorded + against its file, and every class the reference could have named is treated + as reaching the classes that file declares. The references appear in + `--explain`, in `unresolved_references` in `--json`, on stderr in plain mode, + and set `summary.resolution_incomplete`. + +A fact record that arrives with no certname belongs to no host and cannot be +folded into the estate table, so it is counted per fact and reported in +`node_warnings`, in `--explain` and on stderr, and sets +`summary.resolution_incomplete` too. + +Paths outside the Puppet modulepath — `.woodpecker/`, `doc/`, `spec/`, +`vendor/`, `Gemfile`, `*.md` — cannot reach a catalog and are skipped. + +`--explain` also lists changed files that mapped cleanly but reached no host, +which normally means the role they belong to has no members. + +Two things the tool cannot see: + +- A class nothing declares and no `hiera_include` lists reaches no hosts. That + is reported as an orphan on stderr rather than silently dropped. +- The index is built from the checkout at `--repo`, not from `--branch`. In CI + the checkout is the branch under test; pointing `--branch` at a different ref + than the working tree means classes the branch adds look orphaned. + + ## Development ```bash diff --git a/cmd/catalog-impact/main.go b/cmd/catalog-impact/main.go new file mode 100644 index 0000000..bdf4adc --- /dev/null +++ b/cmd/catalog-impact/main.go @@ -0,0 +1,159 @@ +// Command catalog-impact maps a change in the Puppet control repo onto the +// hosts whose catalogs it can alter. +package main + +import ( + "context" + "fmt" + "io" + "os" + "time" + + "github.com/spf13/cobra" + + "git.unkin.net/unkin/puppet-catalog-tools/internal/config" + "git.unkin.net/unkin/puppet-catalog-tools/internal/impact" + "git.unkin.net/unkin/puppet-catalog-tools/internal/puppetdb" +) + +var version = "dev" + +// Exit codes, also documented in --help and the README. +const ( + exitOK = 0 // analysis completed + exitUsage = 1 // transport, git or usage error +) + +const longHelp = `Work out which hosts a change to the Puppet control repo can affect, and print +their hostnames one per line. + +Changed files are mapped onto hosts two ways. A hieradata path is read as its +position in the hiera hierarchy and turned into a fact selector. A manifest, +template or file is turned into the classes it defines, and those classes are +traced backwards — through include, contain and class {} in manifests, and +through hiera_include in hieradata — until only role classes and hierarchy +positions remain. + +The bias is to over-include: an unrecognised path widens to the whole estate, +and a class reference that is not a literal name — a selector, an interpolated +name, a variable — is reported rather than silently dropped.` + +type options struct { + repo string + branch string + base string + files []string + puppetdb string + jsonOut bool + explain bool + limit int + timeout time.Duration +} + +func main() { + cfg, err := config.Load() + if err != nil { + fmt.Fprintln(os.Stderr, "config error:", err) + os.Exit(exitUsage) + } + + opts := &options{} + root := &cobra.Command{ + Use: "catalog-impact", + Short: "List the hosts a Puppet control repo change affects", + Long: longHelp, + Args: cobra.NoArgs, + Version: version, + SilenceUsage: true, + SilenceErrors: true, + RunE: func(cmd *cobra.Command, args []string) error { + return run(cmd.Context(), cmd.OutOrStdout(), opts) + }, + } + + f := root.Flags() + f.StringVarP(&opts.repo, "repo", "r", ".", "path to a puppet-prod checkout") + f.StringVarP(&opts.branch, "branch", "b", "", "branch under test (default: the repo's current branch)") + f.StringVar(&opts.base, "base", "origin/develop", "base ref to diff against") + f.StringSliceVar(&opts.files, "files", nil, "explicit changed-file list, bypassing git") + f.StringVar(&opts.puppetdb, "puppetdb", cfg.PuppetDBURL, "PuppetDB base URL") + f.BoolVar(&opts.jsonOut, "json", false, "emit structured output with per-host reasons") + f.BoolVar(&opts.explain, "explain", false, "human-readable: for each host, which changed files hit it and why") + f.IntVar(&opts.limit, "limit", 0, "cap the number of hosts emitted (0 = no cap)") + f.DurationVar(&opts.timeout, "timeout", time.Minute, "PuppetDB request timeout") + + if err := root.Execute(); err != nil { + fmt.Fprintln(os.Stderr, "error:", err) + os.Exit(exitUsage) + } + os.Exit(exitOK) +} + +func run(ctx context.Context, out io.Writer, opts *options) error { + if opts.jsonOut && opts.explain { + return fmt.Errorf("--json and --explain are mutually exclusive") + } + if opts.limit < 0 { + return fmt.Errorf("--limit must not be negative") + } + + branch := opts.branch + files := opts.files + if len(files) == 0 { + var err error + if branch == "" { + if branch, err = impact.CurrentBranch(opts.repo); err != nil { + return err + } + } + if files, err = impact.ChangedFiles(opts.repo, opts.base, branch); err != nil { + return err + } + } + if branch == "" { + branch = "(explicit file list)" + } + + idx, err := impact.BuildIndex(opts.repo) + if err != nil { + return fmt.Errorf("indexing %s: %w", opts.repo, err) + } + + nodes, nodeWarnings, err := impact.LoadNodes(ctx, puppetdb.New(opts.puppetdb, opts.timeout)) + if err != nil { + return err + } + + analyzer := &impact.Analyzer{Index: idx, Nodes: nodes, NodeWarnings: nodeWarnings} + result := analyzer.Analyze(opts.base, branch, files, opts.limit) + + switch { + case opts.jsonOut: + return impact.RenderJSON(out, result) + case opts.explain: + return impact.RenderExplain(out, result) + } + + warn(result) + return impact.RenderPlain(out, result) +} + +// warn surfaces the widenings and dead ends on stderr so a piped host list +// stays clean. +func warn(r *impact.Result) { + for _, file := range r.UnmappedFiles { + fmt.Fprintf(os.Stderr, "warning: %s matches no mapping rule; every host is included\n", file) + } + for _, line := range impact.UnresolvedLines(r.UnresolvedRefs) { + fmt.Fprintf(os.Stderr, "warning: %s did not resolve to a class name; the host list may be incomplete\n", line) + } + for _, line := range r.NodeWarnings { + fmt.Fprintf(os.Stderr, "warning: %s; the estate table may be missing hosts\n", line) + } + for _, class := range r.OrphanClasses { + fmt.Fprintf(os.Stderr, "warning: class %s is declared by nothing and listed in no hiera_include; it reaches no hosts\n", class) + } + if r.Truncated { + fmt.Fprintf(os.Stderr, "warning: %d hosts affected, output capped at %d by --limit\n", r.Summary.Hosts, len(r.Hosts)) + } +} diff --git a/cmd/catalog-impact/main_test.go b/cmd/catalog-impact/main_test.go new file mode 100644 index 0000000..3723b19 --- /dev/null +++ b/cmd/catalog-impact/main_test.go @@ -0,0 +1,89 @@ +package main + +import ( + "bytes" + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" +) + +const estateBody = `[ + {"certname":"web1.example.net","name":"enc_role","value":"roles::apps::media::jellyfin"}, + {"certname":"web1.example.net","name":"enc_role_tier1","value":"apps"}, + {"certname":"web1.example.net","name":"enc_role_tier2","value":"media"}, + {"certname":"web1.example.net","name":"enc_role_tier3","value":"jellyfin"}, + {"certname":"web1.example.net","name":"country","value":"au"}, + {"certname":"web1.example.net","name":"region","value":"syd1"}, + {"certname":"web1.example.net","name":"virtual","value":"lxc"}, + {"certname":"web1.example.net","name":"os","value":{"name":"AlmaLinux","release":{"major":"9","minor":"7"}}}, + {"certname":"k8s1.example.net","name":"enc_role","value":"roles::infra::k8s::node"}, + {"certname":"k8s1.example.net","name":"enc_role_tier1","value":"infra"}, + {"certname":"k8s1.example.net","name":"enc_role_tier2","value":"k8s"}, + {"certname":"k8s1.example.net","name":"enc_role_tier3","value":"node"}, + {"certname":"k8s1.example.net","name":"country","value":"au"}, + {"certname":"k8s1.example.net","name":"region","value":"syd1"}, + {"certname":"k8s1.example.net","name":"virtual","value":"physical"}, + {"certname":"k8s1.example.net","name":"os","value":{"name":"AlmaLinux","release":{"major":"9","minor":"8"}}} +]` + +func fakePuppetDB(t *testing.T) string { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(estateBody)) + })) + t.Cleanup(srv.Close) + return srv.URL +} + +func TestRunWithExplicitFileList(t *testing.T) { + var out bytes.Buffer + opts := &options{ + repo: "../../internal/impact/testdata/repo", + base: "origin/develop", + files: []string{"modules/jellyfin/manifests/install.pp"}, + puppetdb: fakePuppetDB(t), + timeout: 5 * time.Second, + } + if err := run(context.Background(), &out, opts); err != nil { + t.Fatalf("run: %v", err) + } + if got, want := out.String(), "web1.example.net\n"; got != want { + t.Errorf("output = %q, want %q", got, want) + } +} + +func TestRunExplain(t *testing.T) { + var out bytes.Buffer + opts := &options{ + repo: "../../internal/impact/testdata/repo", + base: "origin/develop", + files: []string{"hieradata/common.yaml"}, + puppetdb: fakePuppetDB(t), + explain: true, + timeout: 5 * time.Second, + } + if err := run(context.Background(), &out, opts); err != nil { + t.Fatalf("run: %v", err) + } + if !strings.Contains(out.String(), "the whole estate is affected") { + t.Errorf("explain output = %q", out.String()) + } +} + +func TestRunRejectsConflictingOutputFlags(t *testing.T) { + opts := &options{jsonOut: true, explain: true} + if err := run(context.Background(), &bytes.Buffer{}, opts); err == nil { + t.Fatal("expected an error for --json with --explain") + } +} + +func TestRunRejectsNegativeLimit(t *testing.T) { + opts := &options{limit: -1} + if err := run(context.Background(), &bytes.Buffer{}, opts); err == nil { + t.Fatal("expected an error for a negative --limit") + } +} diff --git a/internal/impact/git.go b/internal/impact/git.go new file mode 100644 index 0000000..8e6f2a8 --- /dev/null +++ b/internal/impact/git.go @@ -0,0 +1,53 @@ +package impact + +import ( + "bytes" + "fmt" + "os/exec" + "strings" +) + +// CurrentBranch returns the branch checked out in repo. +func CurrentBranch(repo string) (string, error) { + out, err := git(repo, "rev-parse", "--abbrev-ref", "HEAD") + if err != nil { + return "", err + } + branch := strings.TrimSpace(out) + if branch == "" || branch == "HEAD" { + return "", fmt.Errorf("%s is not on a branch; pass --branch", repo) + } + return branch, nil +} + +// ChangedFiles lists the paths that differ between base and branch, using the +// three-dot form so only the branch's own commits count. Renames contribute +// both the old and the new path, because a moved manifest changes the classes +// at both ends. +func ChangedFiles(repo, base, branch string) ([]string, error) { + out, err := git(repo, "diff", "--name-status", "--find-renames", base+"..."+branch) + if err != nil { + return nil, err + } + + var files []string + for _, line := range strings.Split(out, "\n") { + fields := strings.Split(strings.TrimSpace(line), "\t") + if len(fields) < 2 || fields[0] == "" { + continue + } + files = append(files, fields[1:]...) + } + return uniqueSorted(files), nil +} + +func git(repo string, args ...string) (string, error) { + cmd := exec.Command("git", append([]string{"-C", repo}, args...)...) + var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + if err := cmd.Run(); err != nil { + return "", fmt.Errorf("git %s: %w: %s", strings.Join(args, " "), err, strings.TrimSpace(stderr.String())) + } + return stdout.String(), nil +} diff --git a/internal/impact/git_test.go b/internal/impact/git_test.go new file mode 100644 index 0000000..597a1f9 --- /dev/null +++ b/internal/impact/git_test.go @@ -0,0 +1,89 @@ +package impact + +import ( + "os" + "os/exec" + "path/filepath" + "reflect" + "testing" +) + +func gitRepo(t *testing.T) string { + t.Helper() + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not available") + } + dir := t.TempDir() + run := func(args ...string) { + t.Helper() + cmd := exec.Command("git", append([]string{"-C", dir}, args...)...) + cmd.Env = append(os.Environ(), "GIT_CONFIG_GLOBAL=/dev/null", "GIT_CONFIG_SYSTEM=/dev/null") + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v: %s", args, err, out) + } + } + write := func(name, body string) { + t.Helper() + p := filepath.Join(dir, name) + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(p, []byte(body), 0o600); err != nil { + t.Fatal(err) + } + } + + run("init", "--initial-branch=develop") + run("config", "user.email", "test@example.net") + run("config", "user.name", "test") + write("keep.pp", "keep\n") + write("gone.pp", "gone\n") + write("old/name.pp", "some reasonably long body so rename detection fires\n") + run("add", ".") + run("commit", "-m", "base") + + run("checkout", "-b", "feature") + write("added.pp", "added\n") + write("keep.pp", "changed\n") + if err := os.Remove(filepath.Join(dir, "gone.pp")); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Join(dir, "new"), 0o755); err != nil { + t.Fatal(err) + } + run("mv", "old/name.pp", "new/name.pp") + run("add", "-A") + run("commit", "-m", "feature") + run("checkout", "develop") + return dir +} + +func TestChangedFilesCoversAddDeleteAndRename(t *testing.T) { + dir := gitRepo(t) + got, err := ChangedFiles(dir, "develop", "feature") + if err != nil { + t.Fatalf("ChangedFiles: %v", err) + } + want := []string{"added.pp", "gone.pp", "keep.pp", "new/name.pp", "old/name.pp"} + if !reflect.DeepEqual(got, want) { + t.Errorf("ChangedFiles = %v, want %v", got, want) + } +} + +func TestCurrentBranch(t *testing.T) { + dir := gitRepo(t) + got, err := CurrentBranch(dir) + if err != nil { + t.Fatalf("CurrentBranch: %v", err) + } + if got != "develop" { + t.Errorf("CurrentBranch = %q, want develop", got) + } +} + +func TestChangedFilesReportsGitErrors(t *testing.T) { + dir := gitRepo(t) + if _, err := ChangedFiles(dir, "develop", "no-such-branch"); err == nil { + t.Fatal("expected an error for a missing ref") + } +} diff --git a/internal/impact/impact.go b/internal/impact/impact.go new file mode 100644 index 0000000..95ed0bb --- /dev/null +++ b/internal/impact/impact.go @@ -0,0 +1,211 @@ +package impact + +import ( + "slices" + "sort" +) + +// Reason explains why one changed file put one host in the result. +type Reason struct { + File string `json:"file"` + Rule string `json:"rule"` + Detail string `json:"detail"` +} + +// Host is an affected node and every reason it is affected. +type Host struct { + Certname string `json:"certname"` + Reasons []Reason `json:"reasons"` +} + +// Summary is the headline count. +type Summary struct { + Hosts int `json:"hosts"` + Files int `json:"files"` + AllHostsTriggered bool `json:"all_hosts_triggered"` + // ResolutionIncomplete is set when a class reference could not be resolved + // statically, or a fact record could not be attributed to a host, so the + // answer is a best effort rather than exact. + ResolutionIncomplete bool `json:"resolution_incomplete"` +} + +// Result is the whole analysis, and the --json document. +type Result struct { + Base string `json:"base"` + Branch string `json:"branch"` + ChangedFiles []string `json:"changed_files"` + Hosts []Host `json:"hosts"` + UnmappedFiles []string `json:"unmapped_files"` + // UnresolvedRefs are class references the parser could not reduce to + // names, either in a changed file or on the path from one to a host. + UnresolvedRefs []UnresolvedRef `json:"unresolved_references"` + // NodeWarnings are fact records that could not be folded into the estate + // table, so a host may be missing from it entirely. + NodeWarnings []string `json:"node_warnings"` + Summary Summary `json:"summary"` + + // IgnoredFiles are changed files outside the modulepath — CI, docs, + // specs — which cannot reach a catalog. + IgnoredFiles []string `json:"-"` + // OrphanClasses are classes a changed file defines that nothing declares + // and no hieradata file pulls in. + OrphanClasses []string `json:"-"` + // EmptyFiles are changed files that mapped cleanly but reached no host, + // which usually means the role they belong to has no members. + EmptyFiles []string `json:"-"` + // Truncated is set when --limit cut the host list. + Truncated bool `json:"-"` + // TotalHosts is the estate size the selectors were evaluated against. + TotalHosts int `json:"-"` +} + +// Certnames returns the affected hostnames, sorted. +func (r *Result) Certnames() []string { + out := make([]string, 0, len(r.Hosts)) + for _, h := range r.Hosts { + out = append(out, h.Certname) + } + return out +} + +// Analyzer evaluates changed files against one estate fact table. +type Analyzer struct { + Index *Index + Nodes []Node + // NodeWarnings carries whatever LoadNodes could not fold into Nodes. + NodeWarnings []string +} + +// Analyze maps changed files onto hosts. Files are processed in the order +// given; everything in the result is sorted, so output does not move between +// runs. +func (a *Analyzer) Analyze(base, branch string, files []string, limit int) *Result { + resolver := newClassResolver(a.Index, a.Nodes) + + reasons := map[string][]Reason{} + orphans := map[string]bool{} + var unresolved []UnresolvedRef + // hitFiles records which changed files put at least one host in the result. + hitFiles := map[string]bool{} + result := &Result{ + Base: base, + Branch: branch, + ChangedFiles: uniqueSorted(files), + NodeWarnings: a.NodeWarnings, + TotalHosts: len(a.Nodes), + } + + for _, file := range result.ChangedFiles { + m := classifyPath(file, a.Index) + + switch { + case m.ignored: + result.IgnoredFiles = append(result.IgnoredFiles, file) + continue + case m.unknown: + result.UnmappedFiles = append(result.UnmappedFiles, file) + m.targets = []target{allTarget(RuleUnmapped, + fmtDetail("%s matches no mapping rule; widened to the whole estate", file))} + } + + for _, t := range m.targets { + for _, certname := range t.sel.Select(a.Nodes) { + reasons[certname] = append(reasons[certname], + Reason{File: file, Rule: t.rule, Detail: t.detail}) + hitFiles[file] = true + } + } + + unresolved = append(unresolved, a.Index.UnresolvedIn(file)...) + + for _, ref := range m.classes { + res := resolver.resolve(ref.name) + unresolved = append(unresolved, res.unresolved...) + if res.orphan { + orphans[ref.name] = true + continue + } + for certname, hits := range res.hosts { + for _, h := range hits { + reasons[certname] = append(reasons[certname], + Reason{File: file, Rule: h.rule, Detail: ref.why + "; " + h.detail}) + hitFiles[file] = true + } + } + } + } + + // A file that mapped cleanly but reached nobody usually belongs to a role + // with no members. That is a real answer, not a failure, but it is worth + // showing rather than leaving as silence. + for _, file := range result.ChangedFiles { + if !hitFiles[file] && !slices.Contains(result.IgnoredFiles, file) { + result.EmptyFiles = append(result.EmptyFiles, file) + } + } + + for name := range orphans { + result.OrphanClasses = append(result.OrphanClasses, name) + } + sort.Strings(result.OrphanClasses) + + result.UnresolvedRefs = sortUnresolved(dedupeUnresolved(unresolved)) + + certnames := make([]string, 0, len(reasons)) + for certname := range reasons { + certnames = append(certnames, certname) + } + sort.Strings(certnames) + + result.Summary = Summary{ + Hosts: len(certnames), + Files: len(result.ChangedFiles), + AllHostsTriggered: len(certnames) > 0 && len(certnames) == len(a.Nodes), + ResolutionIncomplete: len(result.UnresolvedRefs) > 0 || len(result.NodeWarnings) > 0, + } + + if limit > 0 && len(certnames) > limit { + certnames = certnames[:limit] + result.Truncated = true + } + + for _, certname := range certnames { + result.Hosts = append(result.Hosts, Host{Certname: certname, Reasons: dedupeReasons(reasons[certname])}) + } + return result +} + +func dedupeReasons(in []Reason) []Reason { + sort.Slice(in, func(i, j int) bool { + if in[i].File != in[j].File { + return in[i].File < in[j].File + } + if in[i].Rule != in[j].Rule { + return in[i].Rule < in[j].Rule + } + return in[i].Detail < in[j].Detail + }) + out := in[:0] + var prev Reason + for idx, r := range in { + if idx > 0 && r == prev { + continue + } + prev = r + out = append(out, r) + } + return out +} + +func sortUnresolved(in []UnresolvedRef) []UnresolvedRef { + sort.Slice(in, func(i, j int) bool { + if in[i].File != in[j].File { + return in[i].File < in[j].File + } + if in[i].Form != in[j].Form { + return in[i].Form < in[j].Form + } + return in[i].Expr < in[j].Expr + }) + return in +} diff --git a/internal/impact/impact_test.go b/internal/impact/impact_test.go new file mode 100644 index 0000000..8685f4d --- /dev/null +++ b/internal/impact/impact_test.go @@ -0,0 +1,325 @@ +package impact + +import ( + "bytes" + "context" + "encoding/json" + "reflect" + "strings" + "testing" + + "git.unkin.net/unkin/puppet-catalog-tools/internal/puppetdb" +) + +func testAnalyzer(t *testing.T) *Analyzer { + t.Helper() + return &Analyzer{Index: testIndex(t), Nodes: fixtureNodes()} +} + +func analyze(t *testing.T, files ...string) *Result { + t.Helper() + return testAnalyzer(t).Analyze("origin/develop", "benvin/test", files, 0) +} + +func TestAnalyzeCommonAndHieraTriggerEveryHost(t *testing.T) { + all := []string{"bare1", "k8s1", "k8s2", "prox1", "web1"} + for _, file := range []string{"hieradata/common.yaml", "hiera.yaml", "environment.conf", "Puppetfile", "manifests/site.pp"} { + r := analyze(t, file) + if got := r.Certnames(); !reflect.DeepEqual(got, all) { + t.Errorf("%s affected %v, want every host", file, got) + } + if !r.Summary.AllHostsTriggered { + t.Errorf("%s did not set all_hosts_triggered", file) + } + } +} + +func TestAnalyzeHieradataRolePrefix(t *testing.T) { + r := analyze(t, "hieradata/roles/infra.yaml") + if got, want := r.Certnames(), []string{"k8s1", "k8s2", "prox1"}; !reflect.DeepEqual(got, want) { + t.Errorf("hosts = %v, want %v", got, want) + } + if r.Summary.AllHostsTriggered { + t.Error("a role-prefix change must not report the whole estate") + } +} + +func TestAnalyzeRoleManifestIsExact(t *testing.T) { + r := analyze(t, "site/roles/manifests/infra/k8s/node.pp") + if got, want := r.Certnames(), []string{"k8s1", "k8s2"}; !reflect.DeepEqual(got, want) { + t.Errorf("hosts = %v, want %v", got, want) + } + if r.Hosts[0].Reasons[0].Rule != RuleRoleClass { + t.Errorf("rule = %q, want %q", r.Hosts[0].Reasons[0].Rule, RuleRoleClass) + } +} + +func TestAnalyzeTemplateAttribution(t *testing.T) { + r := analyze(t, "site/profiles/templates/leaf/x.erb") + if got, want := r.Certnames(), []string{"k8s1", "k8s2", "web1"}; !reflect.DeepEqual(got, want) { + t.Errorf("hosts = %v, want %v", got, want) + } +} + +func TestAnalyzeUnattributedModuleAssetWidensToTheModule(t *testing.T) { + // Nothing references this template, so it falls back to every class the + // module defines — here, the hiera_include-only jellyfin module. + r := analyze(t, "modules/jellyfin/templates/unreferenced.erb") + if got, want := r.Certnames(), []string{"web1"}; !reflect.DeepEqual(got, want) { + t.Errorf("hosts = %v, want %v", got, want) + } +} + +func TestAnalyzeUnmappedFileWidensToEveryHost(t *testing.T) { + r := analyze(t, "totally/unknown/thing.txt") + if got, want := r.UnmappedFiles, []string{"totally/unknown/thing.txt"}; !reflect.DeepEqual(got, want) { + t.Errorf("unmapped = %v, want %v", got, want) + } + if len(r.Hosts) != len(fixtureNodes()) { + t.Errorf("unmapped file affected %d hosts, want all %d", len(r.Hosts), len(fixtureNodes())) + } + if r.Hosts[0].Reasons[0].Rule != RuleUnmapped { + t.Errorf("rule = %q", r.Hosts[0].Reasons[0].Rule) + } +} + +func TestAnalyzeIgnoresNonPuppetPaths(t *testing.T) { + r := analyze(t, ".woodpecker/test.yaml", "modules/orphanmod/spec/orphanmod_spec.rb", "README.md") + if len(r.Hosts) != 0 { + t.Errorf("CI and spec changes affected %v", r.Certnames()) + } + if len(r.IgnoredFiles) != 3 { + t.Errorf("ignored = %v", r.IgnoredFiles) + } +} + +func TestAnalyzeReportsOrphanClasses(t *testing.T) { + r := analyze(t, "modules/orphanmod/manifests/init.pp") + if got, want := r.OrphanClasses, []string{"orphanmod", "orphanmod::init"}; !reflect.DeepEqual(got, want) { + t.Errorf("orphans = %v, want %v", got, want) + } + if len(r.Hosts) != 0 { + t.Errorf("orphan class reached %v", r.Certnames()) + } +} + +func TestAnalyzeLimit(t *testing.T) { + r := testAnalyzer(t).Analyze("origin/develop", "benvin/test", []string{"hieradata/common.yaml"}, 2) + if got, want := r.Certnames(), []string{"bare1", "k8s1"}; !reflect.DeepEqual(got, want) { + t.Errorf("hosts = %v, want %v", got, want) + } + if !r.Truncated { + t.Error("Truncated not set") + } + if r.Summary.Hosts != len(fixtureNodes()) { + t.Errorf("summary.hosts = %d, want the untruncated count", r.Summary.Hosts) + } +} + +func TestAnalyzeIsDeterministic(t *testing.T) { + files := []string{ + "hieradata/roles/apps/media/jellyfin.yaml", + "site/profiles/manifests/leaf.pp", + "modules/jellyfin/manifests/install.pp", + "hieradata/virtual/physical.yaml", + "totally/unknown/thing.txt", + } + var first string + for i := range 20 { + var buf bytes.Buffer + if err := RenderJSON(&buf, analyze(t, files...)); err != nil { + t.Fatalf("RenderJSON: %v", err) + } + if i == 0 { + first = buf.String() + continue + } + if buf.String() != first { + t.Fatal("output changed between runs") + } + } +} + +func TestRenderJSONShape(t *testing.T) { + var buf bytes.Buffer + if err := RenderJSON(&buf, analyze(t, "hieradata/roles/apps/media/jellyfin.yaml")); err != nil { + t.Fatalf("RenderJSON: %v", err) + } + var doc struct { + Base string `json:"base"` + Branch string `json:"branch"` + ChangedFiles []string `json:"changed_files"` + UnmappedFiles []string `json:"unmapped_files"` + Hosts []struct { + Certname string `json:"certname"` + Reasons []struct { + File string `json:"file"` + Rule string `json:"rule"` + Detail string `json:"detail"` + } `json:"reasons"` + } `json:"hosts"` + Summary struct { + Hosts int `json:"hosts"` + Files int `json:"files"` + AllHostsTriggered bool `json:"all_hosts_triggered"` + } `json:"summary"` + } + if err := json.Unmarshal(buf.Bytes(), &doc); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if doc.Base != "origin/develop" || doc.Branch != "benvin/test" { + t.Errorf("base/branch = %q/%q", doc.Base, doc.Branch) + } + if len(doc.Hosts) != 1 || doc.Hosts[0].Certname != "web1" { + t.Fatalf("hosts = %#v", doc.Hosts) + } + if len(doc.Hosts[0].Reasons) != 1 || doc.Hosts[0].Reasons[0].Rule != RuleRole { + t.Errorf("reasons = %#v", doc.Hosts[0].Reasons) + } + if doc.Summary.Files != 1 || doc.Summary.Hosts != 1 || doc.Summary.AllHostsTriggered { + t.Errorf("summary = %#v", doc.Summary) + } +} + +func TestRenderPlainIsOneHostPerLine(t *testing.T) { + var buf bytes.Buffer + if err := RenderPlain(&buf, analyze(t, "hieradata/roles/infra.yaml")); err != nil { + t.Fatalf("RenderPlain: %v", err) + } + if got, want := buf.String(), "k8s1\nk8s2\nprox1\n"; got != want { + t.Errorf("output = %q, want %q", got, want) + } +} + +func TestRenderExplainNamesFileRuleAndHost(t *testing.T) { + var buf bytes.Buffer + if err := RenderExplain(&buf, analyze(t, "modules/jellyfin/manifests/install.pp", "bogus/path")); err != nil { + t.Fatalf("RenderExplain: %v", err) + } + out := buf.String() + for _, want := range []string{"web1", RuleHieraInclude, "hieradata/roles/apps/media/jellyfin.yaml", "bogus/path", "unmapped files"} { + if !strings.Contains(out, want) { + t.Errorf("explain output missing %q:\n%s", want, out) + } + } +} + +// stubFacts serves a canned fact table the way PuppetDB would. +type stubFacts struct { + records []puppetdb.FactRecord + asked []string +} + +func (s *stubFacts) FactsByName(_ context.Context, names []string) ([]puppetdb.FactRecord, error) { + s.asked = names + return s.records, nil +} + +func TestLoadNodesFoldsStructuredOS(t *testing.T) { + src := &stubFacts{records: []puppetdb.FactRecord{ + {Certname: "b.example.net", Name: "enc_role", Value: "roles::infra::k8s::node"}, + {Certname: "b.example.net", Name: "enc_role_tier1", Value: "infra"}, + {Certname: "b.example.net", Name: "enc_role_tier2", Value: "k8s"}, + {Certname: "b.example.net", Name: "enc_role_tier3", Value: "node"}, + {Certname: "b.example.net", Name: "country", Value: "au"}, + {Certname: "b.example.net", Name: "region", Value: "syd1"}, + {Certname: "b.example.net", Name: "virtual", Value: "physical"}, + {Certname: "b.example.net", Name: "os", Value: map[string]any{ + "name": "AlmaLinux", "release": map[string]any{"major": "9", "minor": "7"}, + }}, + {Certname: "a.example.net", Name: "enc_role", Value: "roles::base"}, + {Certname: "a.example.net", Name: "enc_role_tier1", Value: "base"}, + }} + + nodes, warnings, err := LoadNodes(context.Background(), src) + if err != nil { + t.Fatalf("LoadNodes: %v", err) + } + if len(warnings) != 0 { + t.Errorf("warnings = %v", warnings) + } + if !reflect.DeepEqual(src.asked, FactNames) { + t.Errorf("asked for %v, want %v", src.asked, FactNames) + } + if len(nodes) != 2 || nodes[0].Certname != "a.example.net" { + t.Fatalf("nodes = %#v", nodes) + } + want := Node{Certname: "b.example.net", Role: "roles::infra::k8s::node", + Tier1: "infra", Tier2: "k8s", Tier3: "node", Country: "au", Region: "syd1", + Virtual: "physical", OSName: "AlmaLinux", OSMajor: "9", OSMinor: "7"} + if nodes[1] != want { + t.Errorf("node = %#v, want %#v", nodes[1], want) + } +} + +func TestLoadNodesRejectsEmptyEstate(t *testing.T) { + if _, _, err := LoadNodes(context.Background(), &stubFacts{}); err == nil { + t.Fatal("expected an error for an empty estate") + } +} + +func TestLoadNodesReportsRecordsWithNoCertname(t *testing.T) { + // A record with no certname cannot be attributed to a host. Dropping it in + // silence hides a host, so it has to be reported. + src := &stubFacts{records: []puppetdb.FactRecord{ + {Certname: "a.example.net", Name: "enc_role", Value: "roles::base"}, + {Name: "enc_role", Value: "roles::base"}, + {Name: "enc_role", Value: "roles::web"}, + {Name: "country", Value: "au"}, + }} + + nodes, warnings, err := LoadNodes(context.Background(), src) + if err != nil { + t.Fatalf("LoadNodes: %v", err) + } + if len(nodes) != 1 { + t.Fatalf("nodes = %#v", nodes) + } + want := []string{ + `fact "country": 1 record(s) had no certname and were dropped`, + `fact "enc_role": 2 record(s) had no certname and were dropped`, + } + if !reflect.DeepEqual(warnings, want) { + t.Errorf("warnings = %v, want %v", warnings, want) + } + + r := (&Analyzer{Index: testIndex(t), Nodes: nodes, NodeWarnings: warnings}). + Analyze("base", "branch", []string{"README.md"}, 0) + if !reflect.DeepEqual(r.NodeWarnings, want) { + t.Errorf("Result.NodeWarnings = %v, want %v", r.NodeWarnings, want) + } + if !r.Summary.ResolutionIncomplete { + t.Error("a dropped fact record left the summary looking exact") + } +} + +func TestAnalyzeReportsFilesThatReachNoHost(t *testing.T) { + // A hieradata file for a role nothing is assigned to maps cleanly and + // still selects nobody; so does an orphan class. + r := analyze(t, "hieradata/roles/infra/nosuch.yaml", "modules/orphanmod/manifests/init.pp", ".woodpecker/test.yaml") + want := []string{"hieradata/roles/infra/nosuch.yaml", "modules/orphanmod/manifests/init.pp"} + if !reflect.DeepEqual(r.EmptyFiles, want) { + t.Errorf("EmptyFiles = %v, want %v", r.EmptyFiles, want) + } + if len(r.Hosts) != 0 { + t.Errorf("hosts = %v", r.Certnames()) + } +} + +func TestAnalyzeDoesNotCallAHittingFileEmpty(t *testing.T) { + r := analyze(t, "hieradata/roles/apps/media/jellyfin.yaml", "hieradata/roles/infra/nosuch.yaml") + if got, want := r.EmptyFiles, []string{"hieradata/roles/infra/nosuch.yaml"}; !reflect.DeepEqual(got, want) { + t.Errorf("EmptyFiles = %v, want %v", got, want) + } +} + +func TestRenderExplainListsFilesThatReachNoHost(t *testing.T) { + var buf bytes.Buffer + if err := RenderExplain(&buf, analyze(t, "hieradata/roles/infra/nosuch.yaml")); err != nil { + t.Fatalf("RenderExplain: %v", err) + } + if !strings.Contains(buf.String(), "reach no host") || + !strings.Contains(buf.String(), "hieradata/roles/infra/nosuch.yaml") { + t.Errorf("explain output = %q", buf.String()) + } +} diff --git a/internal/impact/index.go b/internal/impact/index.go new file mode 100644 index 0000000..a00df35 --- /dev/null +++ b/internal/impact/index.go @@ -0,0 +1,319 @@ +package impact + +import ( + "fmt" + "io/fs" + "os" + "path/filepath" + "regexp" + "sort" + "strings" + + "gopkg.in/yaml.v3" +) + +// manifestRoots are the modulepath entries that live in this repo. +// external_modules/ is Puppetfile-managed and not checked in. +var manifestRoots = []string{"site", "modules"} + +var ( + // include/contain/require are functions taking a list of names; the + // argument list is parsed rather than matched so every name is seen. + declFuncRe = regexp.MustCompile(`(?:^|[^\w:$.])(?:include|contain|require)`) + // inherits takes exactly one name, so a list parse does not apply. + inheritsRe = regexp.MustCompile(`\binherits\s+(?:::)?([a-z][a-z0-9_]*(?:::[a-z0-9_]+)*)`) + classParamRe = regexp.MustCompile(`\bclass\s*\{`) + classRefRe = regexp.MustCompile(`\bClass\s*\[`) + classNameRe = regexp.MustCompile(`^(?:::)?[a-z][a-z0-9_]*(?:::[a-z0-9_]+)*$`) + // a class name is case insensitive in a type reference or a resource + // title, so Class['Mysql::server'] and include Mysql::Server both name + // mysql::server. + anyCaseNameRe = regexp.MustCompile(`^(?:::)?[A-Za-z][A-Za-z0-9_]*(?:::[A-Za-z0-9_]+)*$`) + defineUseRe = regexp.MustCompile(`(?m)(?:^|[^\w:$.])(?:::)?([a-z][a-z0-9_]*(?:::[a-z0-9_]+)+)\s*\{`) + // $mod::params::thing reads a class variable, which is a dependency on the + // class that declares it. + scopeVarRe = regexp.MustCompile(`\$(?:::)?([a-z][a-z0-9_]*(?:::[a-z0-9_]+)+)::[a-z_][a-z0-9_]*`) + // create_resources('mod::type', ...) names a defined type in a string. + createResRe = regexp.MustCompile(`create_resources\s*\(\s*['"](?:::)?([a-z][a-z0-9_]*(?:::[a-z0-9_]+)*)['"]`) + resourceRefRe = regexp.MustCompile(`\b([A-Z][a-zA-Z0-9_]*(?:::[A-Z][a-zA-Z0-9_]*)+)\s*\[`) + assetRe = regexp.MustCompile(`(?:template|epp|file)\s*\(\s*['"]([a-z0-9_]+/[^'"]+)['"]|puppet:///modules/([a-z0-9_]+/[^'"\s]+)`) + hieraBlockRe = regexp.MustCompile(`(?m)^(hiera_include|hiera_exclude):\s*$((?:\n\s+-\s*\S+)*)`) + hieraItemRe = regexp.MustCompile(`(?m)^\s+-\s*['"]?([^'"\s]+)['"]?\s*$`) +) + +// Index is a static view of the control repo: which file defines which class, +// which classes reference which, which hieradata files pull classes in through +// hiera_include, and which manifest owns which template or file. +type Index struct { + Root string + + // includedBy maps a class name to the classes that reference it, and how. + includedBy map[string]map[string]refKind + // moduleClasses maps a module name to the classes it defines. + moduleClasses map[string][]string + // assetOwners maps "/" to the classes referencing it. + assetOwners map[string][]string + // hieraInclude and hieraExclude map a class name to the hieradata files + // listing it. + hieraInclude map[string][]string + hieraExclude map[string][]string + // known holds every class this repo defines. + known map[string]bool + // unresolved maps a manifest file to the class references in it that no + // static resolution could name. + unresolved map[string][]UnresolvedRef + // unresolvedIncluders records, for each unresolved reference, the classes + // whose manifest holds it, so a class the reference might have named still + // reaches them. + unresolvedIncluders []UnresolvedIncluder +} + +// UnresolvedIncluder is an unresolved class reference together with the classes +// declared by the manifest holding it. Any class the reference could have named +// is treated as reaching those classes, because a dropped edge loses hosts. +type UnresolvedIncluder struct { + Ref UnresolvedRef + Classes []string +} + +// BuildIndex walks the control repo at root and indexes it. +func BuildIndex(root string) (*Index, error) { + idx := &Index{ + Root: root, + includedBy: map[string]map[string]refKind{}, + moduleClasses: map[string][]string{}, + assetOwners: map[string][]string{}, + hieraInclude: map[string][]string{}, + hieraExclude: map[string][]string{}, + known: map[string]bool{}, + unresolved: map[string][]UnresolvedRef{}, + } + + for _, base := range manifestRoots { + dir := filepath.Join(root, base) + if _, err := os.Stat(dir); err != nil { + continue + } + if err := filepath.WalkDir(dir, func(p string, d fs.DirEntry, err error) error { + if err != nil || d.IsDir() || !strings.HasSuffix(p, ".pp") { + return err + } + rel, relErr := filepath.Rel(root, p) + if relErr != nil { + return relErr + } + return idx.addManifest(filepath.ToSlash(rel), p) + }); err != nil { + return nil, err + } + } + + hieraDir := filepath.Join(root, "hieradata") + if _, err := os.Stat(hieraDir); err == nil { + if err := filepath.WalkDir(hieraDir, func(p string, d fs.DirEntry, err error) error { + if err != nil || d.IsDir() { + return err + } + if ext := filepath.Ext(p); ext != ".yaml" && ext != ".eyaml" { + return nil + } + rel, relErr := filepath.Rel(root, p) + if relErr != nil { + return relErr + } + return idx.addHieradata(filepath.ToSlash(rel), p) + }); err != nil { + return nil, err + } + } + + for module := range idx.moduleClasses { + sort.Strings(idx.moduleClasses[module]) + } + return idx, nil +} + +func (i *Index) addManifest(rel, full string) error { + parts := strings.Split(rel, "/") + if len(parts) < 4 || parts[2] != "manifests" { + return nil + } + module := parts[1] + declared := ManifestClasses(module, parts[3:]) + for _, name := range declared { + i.known[name] = true + i.moduleClasses[module] = append(i.moduleClasses[module], name) + } + + data, err := os.ReadFile(full) //nolint:gosec // paths come from walking the given repo + if err != nil { + return fmt.Errorf("reading %s: %w", rel, err) + } + body := stripSource(string(data)) + + self := map[string]bool{} + for _, name := range declared { + self[name] = true + } + refs, unresolved := scanReferences(body) + for ref, kind := range refs { + if self[ref] { + continue + } + if i.includedBy[ref] == nil { + i.includedBy[ref] = map[string]refKind{} + } + for _, name := range declared { + if old, ok := i.includedBy[ref][name]; !ok || kind > old { + i.includedBy[ref][name] = kind + } + } + } + + for idx := range unresolved { + unresolved[idx].File = rel + } + if len(unresolved) > 0 && len(declared) > 0 { + i.unresolved[rel] = unresolved + for _, ref := range unresolved { + i.unresolvedIncluders = append(i.unresolvedIncluders, + UnresolvedIncluder{Ref: ref, Classes: declared}) + } + } + + for _, asset := range assetReferences(body) { + i.assetOwners[asset] = append(i.assetOwners[asset], declared...) + } + return nil +} + +func assetReferences(body string) []string { + var out []string + for _, m := range assetRe.FindAllStringSubmatch(body, -1) { + for _, g := range m[1:] { + if g != "" { + out = append(out, g) + } + } + } + return out +} + +func (i *Index) addHieradata(rel, full string) error { + data, err := os.ReadFile(full) //nolint:gosec // paths come from walking the given repo + if err != nil { + return fmt.Errorf("reading %s: %w", rel, err) + } + include, exclude := parseHieraLists(data) + for _, name := range include { + i.hieraInclude[name] = append(i.hieraInclude[name], rel) + } + for _, name := range exclude { + i.hieraExclude[name] = append(i.hieraExclude[name], rel) + } + return nil +} + +// parseHieraLists reads hiera_include and hiera_exclude. eyaml files can carry +// block scalars a strict parser chokes on, so a regex scan backs the YAML up. +func parseHieraLists(data []byte) (include, exclude []string) { + var doc map[string]any + if err := yaml.Unmarshal(data, &doc); err == nil && doc != nil { + return stringList(doc["hiera_include"]), stringList(doc["hiera_exclude"]) + } + for _, m := range hieraBlockRe.FindAllStringSubmatch(string(data), -1) { + var items []string + for _, item := range hieraItemRe.FindAllStringSubmatch(m[2], -1) { + items = append(items, item[1]) + } + if m[1] == "hiera_include" { + include = append(include, items...) + } else { + exclude = append(exclude, items...) + } + } + return include, exclude +} + +func stringList(v any) []string { + items, ok := v.([]any) + if !ok { + return nil + } + var out []string + for _, item := range items { + if s, ok := item.(string); ok && s != "" { + out = append(out, s) + } + } + return out +} + +// IncludedBy returns the classes that reference the given class, sorted. +func (i *Index) IncludedBy(class string) []string { + names := make([]string, 0, len(i.includedBy[class])) + for name := range i.includedBy[class] { + names = append(names, name) + } + sort.Strings(names) + return names +} + +// EdgeNote describes an edge that is not a declaration, so an explanation can +// say why the class is reached. It is empty for a plain declaration. +func (i *Index) EdgeNote(class, parent string) string { + if i.includedBy[class][parent] == refDeclare { + return "" + } + return " (ordering or data reference, not a declaration)" +} + +// UnresolvedIncluders returns the unresolved references that could have named +// the given class, matched on whatever literal prefix was recoverable. +func (i *Index) UnresolvedIncluders(class string) []UnresolvedIncluder { + var out []UnresolvedIncluder + for _, u := range i.unresolvedIncluders { + if strings.HasPrefix(class, u.Ref.Prefix) { + out = append(out, u) + } + } + return out +} + +// UnresolvedIn returns the class references in a manifest file that no static +// resolution could name. +func (i *Index) UnresolvedIn(file string) []UnresolvedRef { return i.unresolved[file] } + +// ModuleClasses returns every class the module defines, sorted. +func (i *Index) ModuleClasses(module string) []string { return i.moduleClasses[module] } + +// AssetOwners returns the classes referencing a "/" template or +// file, sorted and deduplicated. +func (i *Index) AssetOwners(asset string) []string { return uniqueSorted(i.assetOwners[asset]) } + +// HieraInclude returns the hieradata files listing the class under +// hiera_include, sorted. +func (i *Index) HieraInclude(class string) []string { return uniqueSorted(i.hieraInclude[class]) } + +// HieraExclude returns the hieradata files listing the class under +// hiera_exclude, sorted. +func (i *Index) HieraExclude(class string) []string { return uniqueSorted(i.hieraExclude[class]) } + +// Known reports whether the repo defines the class. +func (i *Index) Known(class string) bool { return i.known[class] } + +func uniqueSorted(in []string) []string { + if len(in) == 0 { + return nil + } + seen := map[string]bool{} + out := make([]string, 0, len(in)) + for _, s := range in { + if !seen[s] { + seen[s] = true + out = append(out, s) + } + } + sort.Strings(out) + return out +} diff --git a/internal/impact/index_test.go b/internal/impact/index_test.go new file mode 100644 index 0000000..fb693b6 --- /dev/null +++ b/internal/impact/index_test.go @@ -0,0 +1,144 @@ +package impact + +import ( + "reflect" + "sort" + "testing" +) + +// refs runs a manifest body through the same stripping addManifest does, then +// extracts the class names it resolves. +func refs(body string) []string { + names, _ := scanReferences(stripSource(body)) + out := make([]string, 0, len(names)) + for name := range names { + out = append(out, name) + } + sort.Strings(out) + return out +} + +// unresolvedRefs returns the references the scan could not reduce to names. +func unresolvedRefs(body string) []UnresolvedRef { + _, out := scanReferences(stripSource(body)) + return out +} + +func TestReferencesReadsWholeArgumentList(t *testing.T) { + tests := []struct { + name string + body string + want []string + }{ + { + name: "bare list", + body: "class p {\n include foo, bar, baz\n}\n", + want: []string{"bar", "baz", "foo"}, + }, + { + name: "top scope names", + body: "class p {\n include ::foo::bar, ::baz\n}\n", + want: []string{"baz", "foo::bar"}, + }, + { + name: "require function", + body: "class p {\n require foo, bar\n}\n", + want: []string{"bar", "foo"}, + }, + { + name: "contain", + body: "class p {\n contain foo, bar\n}\n", + want: []string{"bar", "foo"}, + }, + { + name: "parenthesised", + body: "class p {\n include(foo, bar)\n}\n", + want: []string{"bar", "foo"}, + }, + { + name: "quoted names", + body: "class p {\n include 'foo::bar', \"baz::qux\"\n}\n", + want: []string{"baz::qux", "foo::bar"}, + }, + { + name: "multi line with trailing comma", + body: "class p {\n include(\n foo::a,\n foo::b,\n )\n}\n", + want: []string{"foo::a", "foo::b"}, + }, + { + name: "multi line statement form", + body: "class p {\n include foo::a,\n foo::b\n}\n", + want: []string{"foo::a", "foo::b"}, + }, + { + name: "array argument", + body: "class p {\n include [foo, bar]\n}\n", + want: []string{"bar", "foo"}, + }, + { + name: "class resource title list", + body: "class p {\n class { ['foo', 'bar']: }\n}\n", + want: []string{"bar", "foo"}, + }, + { + name: "class reference title list", + body: "class p {\n Class['foo'] -> Class['bar', 'baz']\n}\n", + want: []string{"bar", "baz", "foo"}, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := refs(tc.body); !reflect.DeepEqual(got, tc.want) { + t.Errorf("references = %v, want %v", got, tc.want) + } + }) + } +} + +func TestReferencesStopsAtStatementEnd(t *testing.T) { + tests := []struct { + name string + body string + want []string + }{ + { + name: "next statement is not swallowed", + body: "class p {\n include foo\n notify { 'x': }\n include bar\n}\n", + want: []string{"bar", "foo"}, + }, + { + name: "trailing comment is not a name", + body: "class p {\n include foo, bar # baz, qux\n}\n", + want: []string{"bar", "foo"}, + }, + { + name: "block close ends the list", + body: "class p {\n if $x { include foo }\n include bar\n}\n", + want: []string{"bar", "foo"}, + }, + { + name: "require attribute is not a call", + body: "class p {\n service { 'x':\n require => Class['foo'],\n }\n include bar\n}\n", + want: []string{"bar", "foo"}, + }, + { + name: "semicolon ends the list", + body: "class p {\n include foo; include bar\n}\n", + want: []string{"bar", "foo"}, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := refs(tc.body); !reflect.DeepEqual(got, tc.want) { + t.Errorf("references = %v, want %v", got, tc.want) + } + }) + } +} + +func TestReferencesInheritsTakesOneName(t *testing.T) { + // inherits is not a function and never takes a list. + if got, want := refs("class p inherits foo::bar {\n}\n"), []string{"foo::bar"}; !reflect.DeepEqual(got, want) { + t.Errorf("references = %v, want %v", got, want) + } +} diff --git a/internal/impact/node.go b/internal/impact/node.go new file mode 100644 index 0000000..7d68073 --- /dev/null +++ b/internal/impact/node.go @@ -0,0 +1,149 @@ +// Package impact maps a change in the Puppet control repo onto the hosts whose +// catalogs that change can alter. +package impact + +import ( + "context" + "fmt" + "sort" + "strings" + + "git.unkin.net/unkin/puppet-catalog-tools/internal/puppetdb" +) + +// FactNames are the facts every selector is evaluated against. They are +// fetched for the whole estate in one query. +var FactNames = []string{ + "enc_role", + "enc_role_tier1", + "enc_role_tier2", + "enc_role_tier3", + "country", + "region", + "virtual", + "os", +} + +// Node is the slice of a node's facts the hiera hierarchy keys off. +type Node struct { + Certname string + Role string + Tier1 string + Tier2 string + Tier3 string + Country string + Region string + Virtual string + OSName string + OSMajor string + OSMinor string +} + +// Tiers returns the role tiers as a slice, truncated at the first empty one. +func (n Node) Tiers() []string { + tiers := make([]string, 0, 3) + for _, t := range []string{n.Tier1, n.Tier2, n.Tier3} { + if t == "" { + break + } + tiers = append(tiers, t) + } + return tiers +} + +// FactSource supplies the estate-wide fact table. +type FactSource interface { + FactsByName(ctx context.Context, names []string) ([]puppetdb.FactRecord, error) +} + +// LoadNodes fetches FactNames for every node and folds them into a sorted +// table. Nodes without an enc_role are kept: they still match country, os and +// virtual selectors. A record with no certname belongs to no host and cannot +// be kept, so it is returned as a warning rather than dropped in silence. +func LoadNodes(ctx context.Context, src FactSource) ([]Node, []string, error) { + records, err := src.FactsByName(ctx, FactNames) + if err != nil { + return nil, nil, err + } + + byName := map[string]*Node{} + anonymous := map[string]int{} + for _, r := range records { + if r.Certname == "" { + anonymous[r.Name]++ + continue + } + n, ok := byName[r.Certname] + if !ok { + n = &Node{Certname: r.Certname} + byName[r.Certname] = n + } + applyFact(n, r.Name, r.Value) + } + + nodes := make([]Node, 0, len(byName)) + for _, n := range byName { + nodes = append(nodes, *n) + } + sort.Slice(nodes, func(i, j int) bool { return nodes[i].Certname < nodes[j].Certname }) + if len(nodes) == 0 { + return nil, anonymousWarnings(anonymous), fmt.Errorf("puppetdb returned no nodes") + } + return nodes, anonymousWarnings(anonymous), nil +} + +// anonymousWarnings renders the dropped-record tally one line per fact. +func anonymousWarnings(counts map[string]int) []string { + names := make([]string, 0, len(counts)) + for name := range counts { + names = append(names, name) + } + sort.Strings(names) + out := make([]string, 0, len(names)) + for _, name := range names { + out = append(out, fmt.Sprintf("fact %q: %d record(s) had no certname and were dropped", name, counts[name])) + } + return out +} + +func applyFact(n *Node, name string, value any) { + switch name { + case "enc_role": + n.Role = str(value) + case "enc_role_tier1": + n.Tier1 = str(value) + case "enc_role_tier2": + n.Tier2 = str(value) + case "enc_role_tier3": + n.Tier3 = str(value) + case "country": + n.Country = str(value) + case "region": + n.Region = str(value) + case "virtual": + n.Virtual = str(value) + case "os": + os, ok := value.(map[string]any) + if !ok { + return + } + n.OSName = str(os["name"]) + release, ok := os["release"].(map[string]any) + if !ok { + return + } + n.OSMajor = str(release["major"]) + n.OSMinor = str(release["minor"]) + } +} + +func str(v any) string { + switch t := v.(type) { + case string: + return t + case nil: + return "" + default: + return strings.TrimSpace(fmt.Sprint(t)) + } +} diff --git a/internal/impact/paths.go b/internal/impact/paths.go new file mode 100644 index 0000000..b109745 --- /dev/null +++ b/internal/impact/paths.go @@ -0,0 +1,249 @@ +package impact + +import ( + "path" + "strings" +) + +// Rules, as they appear in --json output and --explain. +const ( + RuleCommon = "hieradata/common" + RuleNode = "hieradata/node" + RuleRole = "hieradata/role" + RuleCountry = "hieradata/country" + RuleCountryRegion = "hieradata/country-region" + RuleCountryRegRole = "hieradata/country-region-role" + RuleCountryRole = "hieradata/country-role" + RuleVirtual = "hieradata/virtual" + RuleOS = "hieradata/os" + RuleRepoGlobal = "repo-global" + RulePluginsync = "pluginsync" + RuleRoleClass = "role-class" + RuleHieraInclude = "hiera_include" + RuleUnmapped = "unmapped" + RuleClassReverse = "class-reverse-lookup" +) + +// repoGlobalFiles change classification or module resolution for the whole +// estate. Puppetfile pins external modules whose code is not in this repo, so +// it is deliberately mapped to every host rather than guessed at. +var repoGlobalFiles = map[string]bool{ + "hiera.yaml": true, + "environment.conf": true, + "Puppetfile": true, + "Puppetfile.lock": true, +} + +// ignoredPrefixes hold paths outside the Puppet modulepath: CI, docs, specs +// and the bundler vendor tree. Nothing under them can reach a catalog. +var ignoredPrefixes = []string{ + ".woodpecker/", ".github/", ".claude/", "ci/", "doc/", "docs/", "spec/", "vendor/", "examples/", +} + +var ignoredFiles = map[string]bool{ + "Gemfile": true, "Gemfile.lock": true, "Rakefile": true, + ".rubocop.yml": true, ".reek.yml": true, ".pre-commit-config.yaml": true, + ".gitignore": true, ".fixtures.yml": true, ".ruby-version": true, + "catalog_diff.py": true, "deploy.py": true, +} + +// moduleSubdirIgnored are module subtrees that are not consulted during a +// catalog compile. +var moduleSubdirIgnored = map[string]bool{"spec": true, "examples": true, "doc": true, "docs": true} + +// classRef is a class a changed file defines, awaiting reverse lookup. +type classRef struct { + name string + why string +} + +// mapping is what a single changed path resolves to before hosts are selected. +type mapping struct { + targets []target + classes []classRef + ignored bool + unknown bool +} + +// classifyPath maps one repo-relative path onto selectors or classes. idx may +// be nil, in which case asset files fall back to their whole module. +func classifyPath(p string, idx *Index) mapping { + p = path.Clean(strings.TrimPrefix(p, "./")) + + if repoGlobalFiles[p] { + return mapping{targets: []target{allTarget(RuleRepoGlobal, + fmtDetail("%s is repo-global; every catalog reads it", p))}} + } + if strings.HasPrefix(p, "manifests/") { + return mapping{targets: []target{allTarget(RuleRepoGlobal, + "the environment manifest applies to every node")}} + } + if ignoredFiles[p] || strings.HasSuffix(p, ".md") { + return mapping{ignored: true} + } + for _, prefix := range ignoredPrefixes { + if strings.HasPrefix(p, prefix) { + return mapping{ignored: true} + } + } + + if rest, ok := strings.CutPrefix(p, "hieradata/"); ok { + sel, rule, ok := hieradataSelector(rest) + if !ok { + return mapping{unknown: true} + } + return mapping{targets: []target{{sel: sel, rule: rule, + detail: fmtDetail("hieradata file selects %s", sel)}}} + } + + if strings.HasPrefix(p, "site/") || strings.HasPrefix(p, "modules/") { + return classifyModulePath(p, idx) + } + + return mapping{unknown: true} +} + +// classifyModulePath handles everything under the modulepath roots: site/ (the +// roles and profiles modules) and modules/ (in-repo modules). +func classifyModulePath(p string, idx *Index) mapping { + parts := strings.Split(p, "/") + if len(parts) < 3 { + return mapping{unknown: true} + } + module, subdir := parts[1], parts[2] + rest := parts[3:] + + if moduleSubdirIgnored[subdir] { + return mapping{ignored: true} + } + + if subdir == "manifests" && len(rest) > 0 && strings.HasSuffix(rest[len(rest)-1], ".pp") { + var classes []classRef + for _, name := range ManifestClasses(module, rest) { + classes = append(classes, classRef{name: name, why: fmtDetail("%s defines class %s", p, name)}) + } + return mapping{classes: classes} + } + + // lib/ and facts.d/ are pluginsynced to every agent, so a custom fact, + // function or type there can move any catalog. + if subdir == "lib" || subdir == "facts.d" { + return mapping{targets: []target{allTarget(RulePluginsync, + fmtDetail("%s is pluginsynced to every agent", p))}} + } + + if subdir == "templates" || subdir == "files" { + asset := module + "/" + strings.Join(rest, "/") + if idx != nil { + if owners := idx.AssetOwners(asset); len(owners) > 0 { + var classes []classRef + for _, name := range owners { + classes = append(classes, classRef{name: name, + why: fmtDetail("%s is referenced by class %s", p, name)}) + } + return mapping{classes: classes} + } + } + return moduleWide(p, module, idx, "no manifest references this asset") + } + + return moduleWide(p, module, idx, fmtDetail("%s/ is module-wide", subdir)) +} + +// moduleWide attributes a file to every class its module defines. It is the +// deliberate over-include fallback for module data that cannot be traced to a +// single class. +func moduleWide(p, module string, idx *Index, why string) mapping { + if idx == nil { + return mapping{unknown: true} + } + names := idx.ModuleClasses(module) + if len(names) == 0 { + return mapping{unknown: true} + } + var classes []classRef + for _, name := range names { + classes = append(classes, classRef{name: name, + why: fmtDetail("%s: %s, attributed to class %s", p, why, name)}) + } + return mapping{classes: classes} +} + +// ManifestClasses returns the class names a manifest file autoloads as. An +// init.pp is reachable both as the namespace itself and as ::init, +// and this repo uses both spellings. +func ManifestClasses(module string, rest []string) []string { + segs := append([]string{module}, rest...) + segs[len(segs)-1] = strings.TrimSuffix(segs[len(segs)-1], ".pp") + + full := strings.Join(segs, "::") + if segs[len(segs)-1] != "init" { + return []string{full} + } + short := strings.Join(segs[:len(segs)-1], "::") + return []string{short, full} +} + +// hieradataSelector maps a path under hieradata/ onto the facts the hiera +// hierarchy interpolates into it. +func hieradataSelector(rel string) (Selector, string, bool) { + ext := path.Ext(rel) + if ext != ".yaml" && ext != ".eyaml" { + return Selector{}, "", false + } + parts := strings.Split(strings.TrimSuffix(rel, ext), "/") + + switch { + case len(parts) == 1 && parts[0] == "common": + return Selector{All: true}, RuleCommon, true + + case parts[0] == "nodes" && len(parts) == 2: + return Selector{Certname: parts[1]}, RuleNode, true + + case parts[0] == "virtual" && len(parts) == 2: + return Selector{Virtual: parts[1]}, RuleVirtual, true + + case parts[0] == "os" && len(parts) == 3: + return osSelector(parts[1], parts[2]), RuleOS, true + + case parts[0] == "roles" && len(parts) >= 2 && len(parts) <= 4: + return Selector{RoleTiers: parts[1:]}, RuleRole, true + + case parts[0] == "country" && len(parts) == 2: + return Selector{Country: parts[1]}, RuleCountry, true + + case parts[0] == "country" && len(parts) == 4 && parts[2] == "region": + return Selector{Country: parts[1], Region: parts[3]}, RuleCountryRegion, true + + case parts[0] == "country" && len(parts) >= 5 && len(parts) <= 7 && parts[2] == "region": + return Selector{Country: parts[1], Region: parts[3], RoleTiers: parts[4:]}, RuleCountryRegRole, true + + case parts[0] == "country" && len(parts) >= 4 && len(parts) <= 6 && parts[2] == "roles": + return Selector{Country: parts[1], RoleTiers: parts[3:]}, RuleCountryRole, true + } + + return Selector{}, "", false +} + +// osSelector reads os//[.].yaml and its +// all_releases.yaml sibling. +func osSelector(osName, file string) Selector { + sel := Selector{OSName: osName} + if file == "all_releases" { + return sel + } + version := file + if len(file) >= len(osName) && strings.EqualFold(file[:len(osName)], osName) { + version = file[len(osName):] + } + version = strings.TrimLeft(version, "-_") + if version == "" { + return sel + } + major, minor, hasMinor := strings.Cut(version, ".") + sel.OSMajor = major + if hasMinor { + sel.OSMinor = minor + } + return sel +} diff --git a/internal/impact/paths_test.go b/internal/impact/paths_test.go new file mode 100644 index 0000000..254a8d6 --- /dev/null +++ b/internal/impact/paths_test.go @@ -0,0 +1,132 @@ +package impact + +import ( + "reflect" + "testing" +) + +func TestHieradataSelector(t *testing.T) { + tests := []struct { + name string + path string + rule string + want Selector + }{ + {"common yaml", "common.yaml", RuleCommon, Selector{All: true}}, + {"common eyaml", "common.eyaml", RuleCommon, Selector{All: true}}, + {"node", "nodes/web01.example.net.yaml", RuleNode, Selector{Certname: "web01.example.net"}}, + {"virtual", "virtual/physical.yaml", RuleVirtual, Selector{Virtual: "physical"}}, + + {"role tier1", "roles/infra.yaml", RuleRole, Selector{RoleTiers: []string{"infra"}}}, + {"role tier2", "roles/infra/puppet.yaml", RuleRole, Selector{RoleTiers: []string{"infra", "puppet"}}}, + {"role tier3", "roles/infra/puppet/master.yaml", RuleRole, + Selector{RoleTiers: []string{"infra", "puppet", "master"}}}, + {"role eyaml", "roles/apps/media.eyaml", RuleRole, Selector{RoleTiers: []string{"apps", "media"}}}, + + {"country", "country/au.yaml", RuleCountry, Selector{Country: "au"}}, + {"country eyaml", "country/au.eyaml", RuleCountry, Selector{Country: "au"}}, + {"country region", "country/au/region/syd1.yaml", RuleCountryRegion, + Selector{Country: "au", Region: "syd1"}}, + {"country region tier1", "country/au/region/syd1/infra.yaml", RuleCountryRegRole, + Selector{Country: "au", Region: "syd1", RoleTiers: []string{"infra"}}}, + {"country region tier3", "country/au/region/syd1/infra/puppet/master.eyaml", RuleCountryRegRole, + Selector{Country: "au", Region: "syd1", RoleTiers: []string{"infra", "puppet", "master"}}}, + {"country roles tier1", "country/au/roles/infra.yaml", RuleCountryRole, + Selector{Country: "au", RoleTiers: []string{"infra"}}}, + {"country roles tier3", "country/au/roles/infra/puppet/master.yaml", RuleCountryRole, + Selector{Country: "au", RoleTiers: []string{"infra", "puppet", "master"}}}, + + {"os all releases", "os/AlmaLinux/all_releases.yaml", RuleOS, Selector{OSName: "AlmaLinux"}}, + {"os major", "os/AlmaLinux/AlmaLinux9.yaml", RuleOS, Selector{OSName: "AlmaLinux", OSMajor: "9"}}, + {"os major minor", "os/AlmaLinux/AlmaLinux9.7.yaml", RuleOS, + Selector{OSName: "AlmaLinux", OSMajor: "9", OSMinor: "7"}}, + {"os debian major", "os/Debian/Debian11.yaml", RuleOS, Selector{OSName: "Debian", OSMajor: "11"}}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got, rule, ok := hieradataSelector(tc.path) + if !ok { + t.Fatalf("hieradataSelector(%q) not recognised", tc.path) + } + if rule != tc.rule { + t.Errorf("rule = %q, want %q", rule, tc.rule) + } + if !reflect.DeepEqual(got, tc.want) { + t.Errorf("selector = %#v, want %#v", got, tc.want) + } + }) + } +} + +func TestHieradataSelectorRejects(t *testing.T) { + for _, p := range []string{ + "README.md", + "roles/a/b/c/d.yaml", + "country/au/region/syd1/a/b/c/d.yaml", + "os/AlmaLinux/AlmaLinux9.json", + "whatever/thing.yaml", + } { + if _, _, ok := hieradataSelector(p); ok { + t.Errorf("hieradataSelector(%q) unexpectedly recognised", p) + } + } +} + +func TestManifestClasses(t *testing.T) { + tests := []struct { + module string + rest []string + want []string + }{ + {"profiles", []string{"base.pp"}, []string{"profiles::base"}}, + {"profiles", []string{"base", "motd.pp"}, []string{"profiles::base::motd"}}, + {"roles", []string{"infra", "k8s", "node.pp"}, []string{"roles::infra::k8s::node"}}, + {"jellyfin", []string{"init.pp"}, []string{"jellyfin", "jellyfin::init"}}, + {"profiles", []string{"cloudinit", "init.pp"}, []string{"profiles::cloudinit", "profiles::cloudinit::init"}}, + } + for _, tc := range tests { + if got := ManifestClasses(tc.module, tc.rest); !reflect.DeepEqual(got, tc.want) { + t.Errorf("ManifestClasses(%q, %v) = %v, want %v", tc.module, tc.rest, got, tc.want) + } + } +} + +func TestClassifyPathRepoGlobal(t *testing.T) { + for _, p := range []string{"hiera.yaml", "environment.conf", "Puppetfile", "manifests/site.pp"} { + m := classifyPath(p, nil) + if len(m.targets) != 1 || !m.targets[0].sel.All { + t.Fatalf("classifyPath(%q) = %#v, want an all-hosts target", p, m) + } + if m.targets[0].rule != RuleRepoGlobal { + t.Errorf("classifyPath(%q) rule = %q", p, m.targets[0].rule) + } + } +} + +func TestClassifyPathIgnoredAndUnknown(t *testing.T) { + for _, p := range []string{".woodpecker/test.yaml", "Gemfile", "doc/notes.md", "vendor/bundle/x.rb", "README.md"} { + if m := classifyPath(p, nil); !m.ignored { + t.Errorf("classifyPath(%q) should be ignored, got %#v", p, m) + } + } + for _, p := range []string{"random/thing.txt", "hieradata/nope/deep/deeper/x.yaml"} { + if m := classifyPath(p, nil); !m.unknown { + t.Errorf("classifyPath(%q) should be unknown, got %#v", p, m) + } + } +} + +func TestClassifyPathPluginsync(t *testing.T) { + m := classifyPath("modules/libs/lib/facter/netbox.rb", nil) + if len(m.targets) != 1 || !m.targets[0].sel.All || m.targets[0].rule != RulePluginsync { + t.Fatalf("pluginsync path = %#v, want an all-hosts pluginsync target", m) + } +} + +func TestClassifyPathManifest(t *testing.T) { + m := classifyPath("site/profiles/manifests/base/motd.pp", nil) + if len(m.classes) != 1 || m.classes[0].name != "profiles::base::motd" { + t.Fatalf("classes = %#v", m.classes) + } +} diff --git a/internal/impact/render.go b/internal/impact/render.go new file mode 100644 index 0000000..44aed7e --- /dev/null +++ b/internal/impact/render.go @@ -0,0 +1,139 @@ +package impact + +import ( + "encoding/json" + "fmt" + "io" +) + +// RenderPlain writes one hostname per line, ready to pipe. +func RenderPlain(w io.Writer, r *Result) error { + for _, h := range r.Hosts { + if _, err := fmt.Fprintln(w, h.Certname); err != nil { + return err + } + } + return nil +} + +// RenderJSON writes the structured report. +func RenderJSON(w io.Writer, r *Result) error { + enc := json.NewEncoder(w) + enc.SetIndent("", " ") + if r.Hosts == nil { + r.Hosts = []Host{} + } + if r.ChangedFiles == nil { + r.ChangedFiles = []string{} + } + if r.UnmappedFiles == nil { + r.UnmappedFiles = []string{} + } + if r.UnresolvedRefs == nil { + r.UnresolvedRefs = []UnresolvedRef{} + } + if r.NodeWarnings == nil { + r.NodeWarnings = []string{} + } + return enc.Encode(r) +} + +// RenderExplain writes the human-readable report: what each file mapped to, +// and for each host which changed files reach it. +func RenderExplain(w io.Writer, r *Result) error { + p := func(format string, args ...any) error { + _, err := fmt.Fprintf(w, format+"\n", args...) + return err + } + + if err := p("%s...%s", r.Base, r.Branch); err != nil { + return err + } + if err := p("%d changed file(s), %d of %d host(s) affected", r.Summary.Files, r.Summary.Hosts, r.TotalHosts); err != nil { + return err + } + if r.Summary.AllHostsTriggered { + if err := p("the whole estate is affected"); err != nil { + return err + } + } + if r.Truncated { + if err := p("host list truncated by --limit"); err != nil { + return err + } + } + if r.Summary.ResolutionIncomplete { + if err := p("resolution incomplete: %d class reference(s) could not be resolved statically", + len(r.UnresolvedRefs)); err != nil { + return err + } + } + + if err := section(w, "fact records dropped (the estate table may be missing hosts)", r.NodeWarnings); err != nil { + return err + } + + if err := section(w, "unmapped files (widened to all hosts)", r.UnmappedFiles); err != nil { + return err + } + if err := section(w, "ignored files (outside the Puppet modulepath)", r.IgnoredFiles); err != nil { + return err + } + if err := section(w, "changed files that reach no host (the role they belong to has no members)", r.EmptyFiles); err != nil { + return err + } + if err := section(w, "unresolved class references (the host list may be incomplete)", + UnresolvedLines(r.UnresolvedRefs)); err != nil { + return err + } + if err := section(w, "orphan classes (nothing declares them and no hiera_include lists them)", r.OrphanClasses); err != nil { + return err + } + + if len(r.Hosts) == 0 { + return p("\nno hosts affected") + } + if err := p("\nhosts:"); err != nil { + return err + } + for _, h := range r.Hosts { + if err := p(" %s", h.Certname); err != nil { + return err + } + for _, reason := range h.Reasons { + if err := p(" %s [%s] %s", reason.File, reason.Rule, reason.Detail); err != nil { + return err + } + } + } + return nil +} + +func section(w io.Writer, title string, items []string) error { + if len(items) == 0 { + return nil + } + if _, err := fmt.Fprintf(w, "\n%s:\n", title); err != nil { + return err + } + for _, item := range items { + if _, err := fmt.Fprintf(w, " %s\n", item); err != nil { + return err + } + } + return nil +} + +// UnresolvedLines renders unresolved references one per line for the report +// and for the stderr warnings. +func UnresolvedLines(refs []UnresolvedRef) []string { + out := make([]string, 0, len(refs)) + for _, u := range refs { + line := fmt.Sprintf("%s: %s %s", u.File, u.Form, u.Expr) + if u.Prefix != "" { + line += fmt.Sprintf(" (names a class under %s)", u.Prefix) + } + out = append(out, line) + } + return out +} diff --git a/internal/impact/resolve.go b/internal/impact/resolve.go new file mode 100644 index 0000000..aa433ca --- /dev/null +++ b/internal/impact/resolve.go @@ -0,0 +1,157 @@ +package impact + +import ( + "sort" + "strings" +) + +// hit is one reason a host is affected, before it is attached to a file. +type hit struct { + rule string + detail string +} + +// classResolver walks the include graph backwards from a changed class to the +// hosts that end up with it. +type classResolver struct { + idx *Index + nodes []Node + cache map[string]*resolution +} + +type resolution struct { + hosts map[string][]hit + orphan bool + // unresolved are the references whose incompleteness widened this walk. + unresolved []UnresolvedRef +} + +func newClassResolver(idx *Index, nodes []Node) *classResolver { + return &classResolver{idx: idx, nodes: nodes, cache: map[string]*resolution{}} +} + +// resolve returns the hosts a class reaches. The walk runs to a fixed point +// over both routes a class can be applied by — a manifest declaring it, and a +// hieradata file listing it under hiera_include — and stops at role classes, +// which the ENC assigns directly. +func (r *classResolver) resolve(class string) *resolution { + if cached, ok := r.cache[class]; ok { + return cached + } + res := &resolution{hosts: map[string][]hit{}} + r.cache[class] = res + + // via records how each class was reached, so --explain can print the chain. + via := map[string]string{class: class} + seen := map[string]bool{class: true} + queue := []string{class} + reached := false + + for len(queue) > 0 { + current := queue[0] + queue = queue[1:] + + if strings.HasPrefix(current, "roles::") { + sel := Selector{RoleExact: current} + detail := fmtDetail("role %s carries %s (%s)", current, class, via[current]) + for _, certname := range sel.Select(r.nodes) { + res.hosts[certname] = append(res.hosts[certname], hit{rule: RuleRoleClass, detail: detail}) + } + reached = true + } + + if hosts, detail, ok := r.viaHiera(current); ok { + reached = true + for _, certname := range hosts { + res.hosts[certname] = append(res.hosts[certname], + hit{rule: RuleHieraInclude, detail: detail + " (" + via[current] + ")"}) + } + } + + for _, parent := range r.idx.IncludedBy(current) { + if seen[parent] { + continue + } + seen[parent] = true + via[parent] = via[current] + " <- " + parent + r.idx.EdgeNote(current, parent) + queue = append(queue, parent) + } + + // A reference the parser could not name might have been this class, so + // the classes holding it are walked too rather than dropped. + for _, u := range r.idx.UnresolvedIncluders(current) { + for _, parent := range u.Classes { + if seen[parent] { + continue + } + seen[parent] = true + via[parent] = via[current] + " <- " + parent + + fmtDetail(" (unresolved %s %s in %s)", u.Ref.Form, u.Ref.Expr, u.Ref.File) + queue = append(queue, parent) + } + res.unresolved = append(res.unresolved, u.Ref) + } + } + + res.orphan = !reached + res.unresolved = dedupeUnresolved(res.unresolved) + return res +} + +// viaHiera applies a class that hieradata pulls in with hiera_include. The +// hierarchy position of the listing file decides which hosts get it, and +// hiera_exclude entries are subtracted because profiles::base declares +// ($hiera_include - $hiera_exclude). +func (r *classResolver) viaHiera(class string) ([]string, string, bool) { + includes := r.idx.HieraInclude(class) + if len(includes) == 0 { + return nil, "", false + } + + included := map[string]bool{} + var sources []string + for _, file := range includes { + sel := hieraFileSelector(file) + for _, certname := range sel.Select(r.nodes) { + included[certname] = true + } + sources = append(sources, fmtDetail("%s [%s]", file, sel)) + } + + excludes := r.idx.HieraExclude(class) + var excluded []string + for _, file := range excludes { + sel := hieraFileSelector(file) + for _, certname := range sel.Select(r.nodes) { + if included[certname] { + delete(included, certname) + excluded = append(excluded, certname) + } + } + } + + hosts := make([]string, 0, len(included)) + for certname := range included { + hosts = append(hosts, certname) + } + sort.Strings(hosts) + + detail := fmtDetail("hiera_include %s in %s", class, strings.Join(sources, ", ")) + if len(excluded) > 0 { + detail += fmtDetail("; hiera_exclude in %s drops %d host(s)", + strings.Join(excludes, ", "), len(uniqueSorted(excluded))) + } + return hosts, detail, true +} + +// hieraFileSelector maps a hieradata file back onto its hierarchy position. An +// unrecognised path widens to the whole estate rather than silently selecting +// nothing. +func hieraFileSelector(file string) Selector { + rel := strings.TrimPrefix(file, "hieradata/") + sel, _, ok := hieradataSelector(rel) + if !ok { + return Selector{All: true} + } + return sel +} diff --git a/internal/impact/resolve_test.go b/internal/impact/resolve_test.go new file mode 100644 index 0000000..4411692 --- /dev/null +++ b/internal/impact/resolve_test.go @@ -0,0 +1,156 @@ +package impact + +import ( + "reflect" + "testing" +) + +func testIndex(t *testing.T) *Index { + t.Helper() + idx, err := BuildIndex("testdata/repo") + if err != nil { + t.Fatalf("BuildIndex: %v", err) + } + return idx +} + +func resolvedHosts(t *testing.T, class string) []string { + t.Helper() + res := newClassResolver(testIndex(t), fixtureNodes()).resolve(class) + hosts := make([]string, 0, len(res.hosts)) + for certname := range res.hosts { + hosts = append(hosts, certname) + } + return uniqueSorted(hosts) +} + +func TestResolveThroughIncludeChain(t *testing.T) { + // profiles::leaf <- profiles::mid <- profiles::base <- two role classes. + if got, want := resolvedHosts(t, "profiles::leaf"), []string{"k8s1", "k8s2", "web1"}; !reflect.DeepEqual(got, want) { + t.Errorf("profiles::leaf = %v, want %v", got, want) + } +} + +func TestResolveContainAndClassResource(t *testing.T) { + for _, class := range []string{"profiles::contained", "profiles::viaclass"} { + if got, want := resolvedHosts(t, class), []string{"k8s1", "k8s2", "web1"}; !reflect.DeepEqual(got, want) { + t.Errorf("%s = %v, want %v", class, got, want) + } + } +} + +func TestResolveDefinedTypeUsage(t *testing.T) { + if got, want := resolvedHosts(t, "profiles::defined::thing"), []string{"k8s1", "k8s2", "web1"}; !reflect.DeepEqual(got, want) { + t.Errorf("profiles::defined::thing = %v, want %v", got, want) + } +} + +func TestResolveTerminatesOnCycle(t *testing.T) { + // profiles::cycle::a and ::b include each other; ::a is pulled in by + // profiles::base, so the walk must reach the roles and stop. + if got, want := resolvedHosts(t, "profiles::cycle::b"), []string{"k8s1", "k8s2", "web1"}; !reflect.DeepEqual(got, want) { + t.Errorf("profiles::cycle::b = %v, want %v", got, want) + } +} + +func TestResolveViaHieraIncludeOnly(t *testing.T) { + // The jellyfin module is applied by hiera_include in a role yaml and by + // nothing in any manifest — a .pp-only search would call it orphaned. + if got, want := resolvedHosts(t, "jellyfin"), []string{"web1"}; !reflect.DeepEqual(got, want) { + t.Errorf("jellyfin = %v, want %v", got, want) + } + // One hop further in: jellyfin::install is only reachable through jellyfin. + if got, want := resolvedHosts(t, "jellyfin::install"), []string{"web1"}; !reflect.DeepEqual(got, want) { + t.Errorf("jellyfin::install = %v, want %v", got, want) + } +} + +func TestResolveHieraIncludeFromVirtualHierarchy(t *testing.T) { + if got, want := resolvedHosts(t, "profiles::lldpd"), []string{"k8s1", "k8s2", "prox1"}; !reflect.DeepEqual(got, want) { + t.Errorf("profiles::lldpd = %v, want %v", got, want) + } +} + +func TestResolveHonoursHieraExclude(t *testing.T) { + // common.yaml pulls basemod in for everyone; roles/infra/proxmox.yaml + // excludes it again, so the proxmox node drops out. + if got, want := resolvedHosts(t, "basemod"), []string{"bare1", "k8s1", "k8s2", "web1"}; !reflect.DeepEqual(got, want) { + t.Errorf("basemod = %v, want %v", got, want) + } +} + +func TestResolveOrphanClass(t *testing.T) { + res := newClassResolver(testIndex(t), fixtureNodes()).resolve("orphanmod") + if !res.orphan { + t.Errorf("orphanmod should be orphaned, hosts = %v", res.hosts) + } + if len(res.hosts) != 0 { + t.Errorf("orphanmod reached %v", res.hosts) + } +} + +func TestIndexIgnoresCommentedReferences(t *testing.T) { + if got := testIndex(t).IncludedBy("profiles::commented"); len(got) != 0 { + t.Errorf("a commented-out class name was indexed as a reference: %v", got) + } +} + +func TestIndexAssetOwners(t *testing.T) { + idx := testIndex(t) + for _, asset := range []string{"profiles/leaf/x.erb", "profiles/leaf/y.txt"} { + if got, want := idx.AssetOwners(asset), []string{"profiles::leaf"}; !reflect.DeepEqual(got, want) { + t.Errorf("AssetOwners(%q) = %v, want %v", asset, got, want) + } + } +} + +func TestIndexModuleClasses(t *testing.T) { + got := testIndex(t).ModuleClasses("jellyfin") + want := []string{"jellyfin", "jellyfin::init", "jellyfin::install"} + if !reflect.DeepEqual(got, want) { + t.Errorf("ModuleClasses(jellyfin) = %v, want %v", got, want) + } +} + +func TestParseHieraListsRegexFallback(t *testing.T) { + // eyaml payloads a strict parser rejects still have to yield their lists. + broken := []byte("---\nhiera_include:\n - alpha\n - beta\nhiera_exclude:\n - gamma\nbad: [unclosed\n") + include, exclude := parseHieraLists(broken) + if !reflect.DeepEqual(include, []string{"alpha", "beta"}) { + t.Errorf("include = %v", include) + } + if !reflect.DeepEqual(exclude, []string{"gamma"}) { + t.Errorf("exclude = %v", exclude) + } +} + +func TestResolveInheritsScopeVarCreateResourcesAndInlineDefine(t *testing.T) { + // Four reference forms a plain include grep misses: class inheritance, + // a $class::var read, create_resources() naming a type in a string, and a + // defined type declared part-way along a line. + tests := map[string]string{ + "inherits": "profiles::inherited", + "scope variable": "profiles::scopevar", + "create_resources": "profiles::created", + "inline define": "profiles::inline::thing", + } + want := []string{"k8s1", "k8s2", "web1"} + for name, class := range tests { + t.Run(name, func(t *testing.T) { + if got := resolvedHosts(t, class); !reflect.DeepEqual(got, want) { + t.Errorf("%s = %v, want %v", class, got, want) + } + }) + } +} + +func TestResolveEveryNameInAnIncludeList(t *testing.T) { + // profiles::base pulls both classes in with one comma-separated include; + // the second name must not be dropped. + want := []string{"k8s1", "k8s2", "web1"} + for _, class := range []string{"profiles::listed::a", "profiles::listed::b"} { + if got := resolvedHosts(t, class); !reflect.DeepEqual(got, want) { + t.Errorf("%s = %v, want %v", class, got, want) + } + } +} diff --git a/internal/impact/scan.go b/internal/impact/scan.go new file mode 100644 index 0000000..b7a763e --- /dev/null +++ b/internal/impact/scan.go @@ -0,0 +1,563 @@ +package impact + +import ( + "regexp" + "strings" +) + +// refKind separates a construct that applies a class from one that only names +// it for ordering or to read a value out of it. +type refKind int + +const ( + refOrdering refKind = iota + refDeclare +) + +// UnresolvedRef is a class reference the parser saw but could not reduce to +// literal names. Dropping one silently loses hosts, so it is recorded against +// its file and reported instead. +type UnresolvedRef struct { + File string `json:"file"` + Form string `json:"form"` + Expr string `json:"expr"` + Prefix string `json:"prefix,omitempty"` +} + +// reserved are bare words an argument list or selector arm can hold that are +// values rather than class names. +var reserved = map[string]bool{ + "undef": true, "true": true, "false": true, "default": true, + "class": true, "define": true, +} + +// maxVarDepth caps how far a bare variable is followed through same-file +// assignments before the reference is called unresolved. +const maxVarDepth = 4 + +// assignRe finds `$name = ` while skipping `==`, `=~` and `=>`. +var assignRe = regexp.MustCompile(`(?:^|[^\w$.])\$([a-z_][a-zA-Z0-9_]*)\s*=[^=~>]`) + +type refScan struct { + refs map[string]refKind + unresolved []UnresolvedRef + vars map[string]string + depth int +} + +// scanReferences extracts every class or defined-type name a manifest body +// names, plus every class reference it could not resolve to literal names. +func scanReferences(body string) (map[string]refKind, []UnresolvedRef) { + s := &refScan{refs: map[string]refKind{}, vars: assignments(body)} + + // include/contain/require: every name in the argument list. + for _, loc := range declFuncRe.FindAllStringIndex(body, -1) { + args, ok := callArguments(body, loc[1]) + if !ok { + continue + } + s.args(callName(body[loc[0]:loc[1]]), refDeclare, args) + } + // class { ['a', 'b']: } declares one resource per title. + for _, loc := range classParamRe.FindAllStringIndex(body, -1) { + s.args("class {}", refDeclare, resourceTitles(body, loc[1])) + } + // Class['a', 'b'] names classes that are declared elsewhere; it orders + // against them rather than applying them. + for _, loc := range classRefRe.FindAllStringIndex(body, -1) { + inner, _, ok := balanced(body, loc[1]-1, '[', ']') + if !ok { + continue + } + s.args("Class[]", refOrdering, inner) + } + for _, re := range []*regexp.Regexp{inheritsRe, defineUseRe, createResRe} { + for _, m := range re.FindAllStringSubmatch(body, -1) { + s.add(m[1], refDeclare) + } + } + // A $mod::params::thing read and a Mod::Type['x'] reference both depend on + // another class without declaring it. + for _, m := range scopeVarRe.FindAllStringSubmatch(body, -1) { + s.add(m[1], refOrdering) + } + for _, m := range resourceRefRe.FindAllStringSubmatch(body, -1) { + s.add(m[1], refOrdering) + } + + return s.refs, dedupeUnresolved(s.unresolved) +} + +// callName trims the separator declFuncRe matched ahead of the function name. +func callName(match string) string { + return strings.TrimLeftFunc(match, func(r rune) bool { return r < 'a' || r > 'z' }) +} + +func (s *refScan) add(name string, kind refKind) { + name = strings.ToLower(strings.TrimPrefix(name, "::")) + if name == "" || reserved[name] { + return + } + if old, ok := s.refs[name]; !ok || kind > old { + s.refs[name] = kind + } +} + +func (s *refScan) args(form string, kind refKind, args string) { + for _, arg := range splitArgs(args) { + s.arg(form, kind, arg) + } +} + +// arg reduces one argument expression to class names, descending into arrays, +// parentheses and selector arms. Anything it cannot reduce becomes an +// unresolved reference rather than nothing. +func (s *refScan) arg(form string, kind refKind, arg string) { + arg = strings.TrimSpace(arg) + if arg == "" { + return + } + switch arg[0] { + case '[': + s.nested(form, kind, arg, '[', ']') + case '(': + s.nested(form, kind, arg, '(', ')') + default: + if arms, ok := selectorArms(arg); ok { + s.selector(form, kind, arg, arms) + return + } + s.literal(form, kind, arg) + } +} + +func (s *refScan) nested(form string, kind refKind, arg string, opener, closer byte) { + inner, _, ok := balanced(arg, 0, opener, closer) + if !ok { + s.unresolvedAdd(form, arg, "") + return + } + s.args(form, kind, inner) +} + +// selector takes every literal name out of every arm, which is strictly better +// than widening: a selector over role names lists all the classes it can pick. +func (s *refScan) selector(form string, kind refKind, arg, arms string) { + for _, arm := range splitArgs(arms) { + _, value, ok := cutRocket(arm) + if !ok { + if strings.TrimSpace(arm) != "" { + s.unresolvedAdd(form, arg, "") + } + continue + } + s.arg(form, kind, value) + } +} + +func (s *refScan) literal(form string, kind refKind, arg string) { + lit, quoted := unquote(arg) + switch { + case classNameRe.MatchString(lit): + s.add(lit, kind) + case anyCaseNameRe.MatchString(lit): + // Class[Apache::Ssl] names the class in type form. + s.add(lit, kind) + case quoted: + // "profile::role_${role}" is a name only the compiler can finish. + s.unresolvedAdd(form, arg, literalPrefix(lit)) + case strings.HasPrefix(arg, "$"): + s.variable(form, kind, arg) + default: + s.unresolvedAdd(form, arg, "") + } +} + +// variable follows a bare $name through the assignments in the same file, and +// gives up loudly when that leads nowhere. +func (s *refScan) variable(form string, kind refKind, arg string) { + expr, ok := s.vars[strings.TrimPrefix(strings.TrimPrefix(arg, "$"), "::")] + if !ok || s.depth >= maxVarDepth { + s.unresolvedAdd(form, arg, "") + return + } + before := len(s.refs) + len(s.unresolved) + s.depth++ + s.arg(form, kind, expr) + s.depth-- + if len(s.refs)+len(s.unresolved) == before { + s.unresolvedAdd(form, arg, "") + } +} + +func (s *refScan) unresolvedAdd(form, expr, prefix string) { + s.unresolved = append(s.unresolved, UnresolvedRef{Form: form, Expr: collapse(expr), Prefix: prefix}) +} + +func dedupeUnresolved(in []UnresolvedRef) []UnresolvedRef { + if len(in) == 0 { + return nil + } + seen := map[UnresolvedRef]bool{} + out := in[:0] + for _, u := range in { + if seen[u] { + continue + } + seen[u] = true + out = append(out, u) + } + return out +} + +// literalPrefix returns the part of an interpolated name that is fixed text, +// so the report can say what family of classes was missed. Class names are +// case insensitive and every name the index holds is lowercase, so the prefix +// is lowered too or it matches nothing it is meant to widen to. +func literalPrefix(s string) string { + i := strings.Index(s, "$") + if i <= 0 { + return "" + } + return strings.ToLower(s[:i]) +} + +// collapse folds whitespace and clips an expression to a length that reads in +// a report. +func collapse(s string) string { + out := strings.Join(strings.Fields(s), " ") + if len(out) > 80 { + out = out[:80] + "..." + } + return out +} + +func unquote(s string) (string, bool) { + if len(s) > 1 && (s[0] == '\'' || s[0] == '"') && s[len(s)-1] == s[0] { + return s[1 : len(s)-1], true + } + return s, false +} + +// selectorArms returns the arm text of a ` ? { ... }` selector. +func selectorArms(s string) (string, bool) { + var arms string + var ok bool + walkSource(s, 0, anyDelim, func(j int, c byte, depth int) bool { + if c != '?' || depth != 0 { + return false + } + k := j + 1 + for k < len(s) && isSpace(s[k]) { + k++ + } + if k >= len(s) || s[k] != '{' { + return false + } + arms, _, ok = balanced(s, k, '{', '}') + return true + }) + return arms, ok +} + +// cutRocket splits a selector arm on its top-level `=>`. +func cutRocket(s string) (string, string, bool) { + end := walkSource(s, 0, anyDelim, func(j int, c byte, depth int) bool { + return c == '=' && depth == 0 && j+1 < len(s) && s[j+1] == '>' + }) + if end >= len(s) { + return "", "", false + } + return s[:end], s[end+2:], true +} + +// assignments maps a variable assigned in this file to the expression it was +// assigned, so `$p = $x ? {...}; include $p` still yields names. +func assignments(body string) map[string]string { + out := map[string]string{} + for _, loc := range assignRe.FindAllStringSubmatchIndex(body, -1) { + name := body[loc[2]:loc[3]] + if _, seen := out[name]; seen { + continue + } + out[name] = statementExpr(body, loc[1]-1) + } + return out +} + +// statementExpr reads one expression starting at i, ending at the top-level +// semicolon, newline or closing delimiter that terminates the statement. +func statementExpr(s string, i int) string { + for i < len(s) && (s[i] == ' ' || s[i] == '\t') { + i++ + } + end := walkSource(s, i, anyDelim, func(j int, c byte, depth int) bool { + if depth != 0 { + return false + } + switch c { + case ')', ']', '}', ';': + return true + case '\n': + return !continued(s[i:j]) + } + return false + }) + return s[i:end] +} + +// stripSource blanks out # comments and heredoc bodies. Both are text the +// class scan must not read: a `#` inside a string is not a comment, and an +// `include` in heredoc documentation is not an edge. Blanking rather than +// deleting keeps the line structure the statement scan relies on. +func stripSource(src string) string { + out := []byte(src) + blank := func(from, to int) { + for k := from; k < to && k < len(out); k++ { + if out[k] != '\n' { + out[k] = ' ' + } + } + } + + var pending []string + for i := 0; i < len(src); { + switch c := src[i]; { + case c == '\'' || c == '"': + i = skipString(src, i) + case c == '#': + j := i + for j < len(src) && src[j] != '\n' { + j++ + } + blank(i, j) + i = j + case c == '@' && i+1 < len(src) && src[i+1] == '(': + tag, next, ok := heredocTag(src, i+1) + if !ok { + i++ + continue + } + pending = append(pending, tag) + i = next + case c == '\n': + i++ + for _, tag := range pending { + i = blankHeredoc(src, blank, i, tag) + } + pending = pending[:0] + default: + i++ + } + } + return string(out) +} + +// heredocTag reads the tag out of @(EOT), @("EOT"), @(EOT/L) and @(EOT:json/nt). +func heredocTag(s string, i int) (string, int, bool) { + j := i + 1 + for j < len(s) && s[j] != ')' && s[j] != '\n' { + j++ + } + if j >= len(s) || s[j] != ')' { + return "", i + 1, false + } + spec := s[i+1 : j] + if k := strings.IndexAny(spec, ":/"); k >= 0 { + spec = spec[:k] + } + spec = strings.Trim(strings.TrimSpace(spec), `"'`) + if spec == "" { + return "", j + 1, false + } + return spec, j + 1, true +} + +// blankHeredoc blanks the lines from i up to and including the line that ends +// the heredoc, and returns the index just past it. +func blankHeredoc(s string, blank func(from, to int), i int, tag string) int { + for i < len(s) { + end := strings.IndexByte(s[i:], '\n') + if end < 0 { + blank(i, len(s)) + return len(s) + } + line := s[i : i+end] + blank(i, i+end) + i += end + 1 + if isHeredocEnd(line, tag) { + return i + } + } + return i +} + +// isHeredocEnd matches the terminator line, with its optional `|` indentation +// marker and `-` trailing-newline marker. +func isHeredocEnd(line, tag string) bool { + t := strings.TrimSpace(line) + t = strings.TrimSpace(strings.TrimPrefix(t, "|")) + t = strings.TrimSpace(strings.TrimPrefix(t, "-")) + return t == tag +} + +func isSpace(c byte) bool { return c == ' ' || c == '\t' || c == '\n' || c == '\r' } + +// callArguments returns the argument text of the function call whose name ends +// at i, in either the parenthesised or the bare statement form. It reports +// false when the keyword is not a call at all, such as a require => attribute. +func callArguments(s string, i int) (string, bool) { + if i >= len(s) { + return "", false + } + if s[i] != '(' && s[i] != ' ' && s[i] != '\t' { + return "", false + } + for i < len(s) && (s[i] == ' ' || s[i] == '\t') { + i++ + } + if i >= len(s) { + return "", false + } + if s[i] == '(' { + inner, _, ok := balanced(s, i, '(', ')') + return inner, ok + } + if s[i] == '=' || s[i] == ',' || s[i] == ';' || s[i] == '\n' { + return "", false + } + return statementExpr(s, i), true +} + +// splitArgs splits an argument list on its top-level commas. +func splitArgs(s string) []string { + var out []string + start := 0 + walkSource(s, 0, anyDelim, func(j int, c byte, depth int) bool { + if c == ',' && depth == 0 { + out = append(out, s[start:j]) + start = j + 1 + } + return false + }) + return append(out, s[start:]) +} + +// resourceTitles returns the title expression of a resource body whose opening +// brace ends at i, i.e. everything up to the colon that closes the title. +func resourceTitles(s string, i int) string { + var out string + walkSource(s, i, anyDelim, func(j int, c byte, depth int) bool { + if depth != 0 { + return false + } + switch c { + case ')', ']', '}': + return true + case ':': + out = s[i:j] + return true + } + return false + }) + return out +} + +// balanced returns the text between the delimiter at i and its match. Only the +// given pair is counted, so an unmatched brace inside a bracket list cannot +// unbalance it. +func balanced(s string, i int, opener, closer byte) (string, int, bool) { + end := walkSource(s, i, pairDelim(opener, closer), func(_ int, c byte, depth int) bool { + return c == closer && depth == 1 + }) + if end >= len(s) { + return "", len(s), false + } + return s[i+1 : end], end + 1, true +} + +// delim reports how a byte changes nesting depth: +1 opening, -1 closing. +type delim func(byte) int + +// anyDelim counts all three bracket pairs, which is what an expression scan +// wants: a comma is top level only if no bracket of any kind is open. +func anyDelim(c byte) int { + switch c { + case '(', '[', '{': + return 1 + case ')', ']', '}': + return -1 + } + return 0 +} + +func pairDelim(opener, closer byte) delim { + return func(c byte) int { + switch c { + case opener: + return 1 + case closer: + return -1 + } + return 0 + } +} + +// walkSource visits the bytes of s from i that lie outside string literals, +// passing each one the nesting depth that applies before it. String literals +// are skipped whole by the one shared scanner, so no caller can desync on a +// quote. It returns the index stop accepted, or len(s). +func walkSource(s string, i int, d delim, stop func(j int, c byte, depth int) bool) int { + depth := 0 + for j := i; j < len(s); j++ { + c := s[j] + if c == '\'' || c == '"' { + j = skipString(s, j) - 1 + continue + } + if stop(j, c, depth) { + return j + } + depth += d(c) + } + return len(s) +} + +// skipString returns the index just past the quoted string starting at i. The +// opening delimiter is remembered and only the same delimiter closes the +// string, so a `"` inside a single-quoted substring cannot end a double-quoted +// one. Inside a double-quoted string a `${...}` interpolation is code rather +// than text, so it is scanned as such and may hold strings of either quote. +func skipString(s string, i int) int { + quote := s[i] + for j := i + 1; j < len(s); j++ { + switch c := s[j]; { + case c == '\\': + j++ + case c == quote: + return j + 1 + case quote == '"' && c == '$' && j+1 < len(s) && s[j+1] == '{': + j = skipInterpolation(s, j+1) - 1 + } + } + return len(s) +} + +// skipInterpolation returns the index just past the ${...} expression whose +// opening brace is at i. +func skipInterpolation(s string, i int) int { + end := walkSource(s, i, pairDelim('{', '}'), func(_ int, c byte, depth int) bool { + return c == '}' && depth == 1 + }) + if end >= len(s) { + return len(s) + } + return end + 1 +} + +// continued reports whether an argument list so far ends on a comma, meaning +// the list carries on over the newline. +func continued(s string) bool { + s = strings.TrimRight(s, " \t\r") + return strings.HasSuffix(s, ",") +} diff --git a/internal/impact/scan_test.go b/internal/impact/scan_test.go new file mode 100644 index 0000000..f0944fa --- /dev/null +++ b/internal/impact/scan_test.go @@ -0,0 +1,355 @@ +package impact + +import ( + "fmt" + "reflect" + "testing" +) + +func TestScanSelectorArgumentKeepsEveryBranch(t *testing.T) { + tests := []struct { + name string + body string + want []string + }{ + { + name: "selector as the argument", + body: "class p {\n include $facts['role'] ? { 'web' => 'profile::web', default => 'profile::base' }\n}\n", + want: []string{"profile::base", "profile::web"}, + }, + { + name: "selector reached through a variable", + body: "class p {\n $profile = $facts['role'] ? { 'web' => 'profile::web', default => 'profile::base' }\n" + + " include $profile\n}\n", + want: []string{"profile::base", "profile::web"}, + }, + { + name: "selector inside a parenthesised call", + body: "class p {\n include(($x ? { default => 'profile::sel' }))\n}\n", + want: []string{"profile::sel"}, + }, + { + name: "selector arm holding an array", + body: "class p {\n include $x ? { 'a' => ['profile::a', 'profile::b'], default => 'profile::c' }\n}\n", + want: []string{"profile::a", "profile::b", "profile::c"}, + }, + { + name: "undef arm contributes nothing", + body: "class p {\n include $x ? { 'a' => 'profile::a', default => undef }\n}\n", + want: []string{"profile::a"}, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := refs(tc.body); !reflect.DeepEqual(got, tc.want) { + t.Errorf("references = %v, want %v", got, tc.want) + } + if got := unresolvedRefs(tc.body); len(got) != 0 { + t.Errorf("a fully resolved selector reported %v", got) + } + }) + } +} + +func TestScanHashInsideAStringIsNotAComment(t *testing.T) { + tests := []struct { + name string + body string + want []string + }{ + { + name: "double quoted", + body: "class p {\n $x = \"port #1\" ; include profile::afterhash\n}\n", + want: []string{"profile::afterhash"}, + }, + { + name: "single quoted", + body: "class p {\n $x = 'colour #fff' ; include profile::aftercolour\n}\n", + want: []string{"profile::aftercolour"}, + }, + { + name: "url fragment", + body: "class p {\n $u = 'https://h/p#frag'\n include profile::afterurl\n}\n", + want: []string{"profile::afterurl"}, + }, + { + name: "multi line string", + body: "class p {\n $s = \"#!/bin/sh\nexit 0\"\n include profile::afterscript\n}\n", + want: []string{"profile::afterscript"}, + }, + { + name: "a real comment is still stripped", + body: "class p {\n include profile::live # include profile::dead\n}\n", + want: []string{"profile::live"}, + }, + { + name: "a hash in a comment after a string", + body: "class p {\n $x = 'a' # ticket #42 include profile::dead\n include profile::live\n}\n", + want: []string{"profile::live"}, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := refs(tc.body); !reflect.DeepEqual(got, tc.want) { + t.Errorf("references = %v, want %v", got, tc.want) + } + }) + } +} + +func TestScanSkipsHeredocBodies(t *testing.T) { + body := "class p {\n" + + " $a = @(EOT)\n include profile::plain\n | EOT\n" + + " $b = @(\"EOT\")\n include profile::quoted\n |-EOT\n" + + " $c = @(EOT/L)\n include profile::escapes\n EOT\n" + + " $d = @(EOT:json/nt)\n include profile::syntax\n - EOT\n" + + " include profile::real\n}\n" + if got, want := refs(body), []string{"profile::real"}; !reflect.DeepEqual(got, want) { + t.Errorf("references = %v, want %v", got, want) + } + if got := unresolvedRefs(body); len(got) != 0 { + t.Errorf("heredoc scan reported %v", got) + } +} + +func TestScanHeredocBodyDoesNotSwallowFollowingCode(t *testing.T) { + // An unbalanced brace and quote in the body must not leak into the scan. + body := "class p {\n $a = @(EOF)\n { \"unclosed ' ]]] \n EOF\n include profile::after\n}\n" + if got, want := refs(body), []string{"profile::after"}; !reflect.DeepEqual(got, want) { + t.Errorf("references = %v, want %v", got, want) + } +} + +func TestScanReportsInterpolatedClassName(t *testing.T) { + body := "class p {\n include \"profile::role_${role}\"\n}\n" + if got := refs(body); len(got) != 0 { + t.Errorf("an interpolated name yielded %v, want nothing resolved", got) + } + got := unresolvedRefs(body) + want := []UnresolvedRef{{Form: "include", Expr: `"profile::role_${role}"`, Prefix: "profile::role_"}} + if !reflect.DeepEqual(got, want) { + t.Errorf("unresolved = %#v, want %#v", got, want) + } +} + +func TestScanReportsUnresolvableArguments(t *testing.T) { + tests := []struct { + name string + body string + want UnresolvedRef + }{ + { + name: "bare variable", + body: "class p {\n include $unknown\n}\n", + want: UnresolvedRef{Form: "include", Expr: "$unknown"}, + }, + { + name: "variable assigned from a function", + body: "class p {\n $c = lookup('profile')\n include $c\n}\n", + want: UnresolvedRef{Form: "include", Expr: "lookup('profile')"}, + }, + { + name: "function call argument", + body: "class p {\n contain lookup('profile')\n}\n", + want: UnresolvedRef{Form: "contain", Expr: "lookup('profile')"}, + }, + { + name: "class resource with a computed title", + body: "class p {\n class { $title: }\n}\n", + want: UnresolvedRef{Form: "class {}", Expr: "$title"}, + }, + { + name: "interpolated class reference", + body: "class p {\n Class[\"profile::${x}\"] -> Service['s']\n}\n", + want: UnresolvedRef{Form: "Class[]", Expr: `"profile::${x}"`, Prefix: "profile::"}, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := unresolvedRefs(tc.body) + if len(got) != 1 || got[0] != tc.want { + t.Errorf("unresolved = %#v, want [%#v]", got, tc.want) + } + }) + } +} + +func TestScanCaseInsensitiveClassTitle(t *testing.T) { + // Class names are case insensitive in a resource title, so a mixed-case + // one must resolve rather than widen. + body := "class p {\n service { 's': require => Class['Mysql::server'] }\n}\n" + if got, want := refs(body), []string{"mysql::server"}; !reflect.DeepEqual(got, want) { + t.Errorf("references = %v, want %v", got, want) + } + if got := unresolvedRefs(body); len(got) != 0 { + t.Errorf("unresolved = %v", got) + } +} + +func TestScanSurvivesMalformedInput(t *testing.T) { + bodies := []string{ + "", + "class p {", + "class p {\n include [foo, bar\n", + "class p {\n include 'foo\n", + "class p {\n include \"foo::${\n", + "class p {\n include (\n", + "class p {\n Class[\n", + "class p {\n class { : }\n}\n", + "class p {\n include $x ? {\n", + "class p {\n include $x ? { 'a' => }\n}\n", + "class p {\n $a = @(EOT)\n no terminator here\n", + "@(", + "@()\n", + "#", + "$", + "))))]]]}}}", + "include " + "a::b, " + "include include include", + "class p {\n include [[a::b, [c::d, e::f]], g::h]\n}\n", + } + for _, body := range bodies { + names, unresolved := scanReferences(stripSource(body)) + _, _ = names, unresolved + } + + // The nested-array case must still resolve every name. + got := refs("class p {\n include [[a::b, [c::d, e::f]], g::h]\n}\n") + want := []string{"a::b", "c::d", "e::f", "g::h"} + if !reflect.DeepEqual(got, want) { + t.Errorf("nested arrays = %v, want %v", got, want) + } +} + +func TestStripSourcePreservesLineCount(t *testing.T) { + // The statement scan reads newlines, so stripping must not move them. + body := "class p {\n $a = @(EOT)\n body\n EOT\n include foo # bar\n}\n" + if err := sameNewlines(body, stripSource(body)); err != nil { + t.Fatal(err) + } +} + +// sameNewlines is the invariant stripSource owes the statement scanner: every +// byte keeps its offset, and a newline stays a newline and nothing else +// becomes one. +func sameNewlines(body, stripped string) error { + if len(stripped) != len(body) { + return fmt.Errorf("stripSource changed length: %d -> %d", len(body), len(stripped)) + } + for i := range body { + if (body[i] == '\n') != (stripped[i] == '\n') { + return fmt.Errorf("newline at %d moved: %q -> %q", i, body[i], stripped[i]) + } + } + return nil +} + +func TestScanQuoteStateIsDelimiterAware(t *testing.T) { + tests := []struct { + name string + body string + want []string + unresolved int + }{ + { + // A single-quoted string inside a ${...} interpolation may hold the + // outer delimiter. Toggling on any quote ends the argument list at + // the `)` inside it and loses everything after it. + name: "double quoted delimiter inside an interpolated single quoted string", + body: "class p {\n include(\"${sprintf('a\"a)b\"c', $x)}\", profile::real)\n}\n", + want: []string{"profile::real"}, + unresolved: 1, + }, + { + name: "single quoted string holding a double quote", + body: "class p {\n include('it\"s', profile::real)\n}\n", + want: []string{"profile::real"}, unresolved: 1, + }, + { + name: "double quoted string holding a single quote", + body: "class p {\n include(\"a'b\", profile::real)\n}\n", + want: []string{"profile::real"}, unresolved: 1, + }, + { + name: "interpolation holding a bracket and a quote", + body: "class p {\n notify { \"${facts['a[b']}\": }\n include profile::real\n}\n", + want: []string{"profile::real"}, + }, + { + name: "nested interpolation", + body: "class p {\n $t = \"${a[\"${b['c}']}\"]}\"\n include profile::real\n}\n", + want: []string{"profile::real"}, + }, + { + name: "escaped quote does not end the string", + body: "class p {\n include(\"a\\\")b\", profile::real)\n}\n", + want: []string{"profile::real"}, unresolved: 1, + }, + { + name: "hash inside an interpolated single quoted string is not a comment", + body: "class p {\n $t = \"${a['#']}\"\n include profile::real\n}\n", + want: []string{"profile::real"}, + }, + { + // The comment scan shares the string skipper, so a `#` inside a + // nested double-quoted string must not blank the rest of the line. + name: "hash inside an interpolated double quoted string is not a comment", + body: "class p {\n $t = \"${a[\"#\"]}\" include profile::real\n}\n", + want: []string{"profile::real"}, + }, + { + name: "selector arm quoted with the other delimiter", + body: "class p {\n include $x ? { \"it's\" => 'profile::a', default => 'profile::b' }\n}\n", + want: []string{"profile::a", "profile::b"}, + }, + { + name: "class resource title list with an awkward quote", + body: "class p {\n class { ['profile::a', \"it's\"]: }\n}\n", + want: []string{"profile::a"}, unresolved: 1, + }, + { + name: "heredoc body holding an unbalanced quote", + body: "class p {\n $a = @(EOT)\n it's ) unbalanced \"\n EOT\n include profile::real\n}\n", + want: []string{"profile::real"}, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if err := sameNewlines(tc.body, stripSource(tc.body)); err != nil { + t.Fatal(err) + } + if got := refs(tc.body); !reflect.DeepEqual(got, tc.want) { + t.Errorf("references = %v, want %v", got, tc.want) + } + if got := unresolvedRefs(tc.body); len(got) != tc.unresolved { + t.Errorf("unresolved = %#v, want %d", got, tc.unresolved) + } + }) + } +} + +func TestUnresolvedPrefixIsLowercased(t *testing.T) { + // Class names are case insensitive and the index holds them lowercase, so + // a mixed-case prefix that is not lowered widens to nothing. + body := "class p {\n Class[\"Mysql::${x}\"] -> Service['s']\n}\n" + got := unresolvedRefs(body) + want := UnresolvedRef{Form: "Class[]", Expr: `"Mysql::${x}"`, Prefix: "mysql::"} + if len(got) != 1 || got[0] != want { + t.Errorf("unresolved = %#v, want [%#v]", got, want) + } +} + +func FuzzScanReferences(f *testing.F) { + f.Add("class p {\n include foo::bar\n}\n") + f.Add("class p {\n include $x ? { 'a' => 'b::c', default => undef }\n}\n") + f.Add("class p {\n $a = @(EOT)\n include x::y\n EOT\n}\n") + f.Add("class p {\n include \"a::${b}\" # '\n}\n") + f.Add("))))]]]}}}@(#'\"") + f.Add("class p {\n include(\"${sprintf('a\"a)b\"c', $x)}\", profile::real)\n}\n") + f.Fuzz(func(t *testing.T, body string) { + stripped := stripSource(body) + if err := sameNewlines(body, stripped); err != nil { + t.Fatal(err) + } + scanReferences(stripped) + }) +} diff --git a/internal/impact/selector.go b/internal/impact/selector.go new file mode 100644 index 0000000..fa2c842 --- /dev/null +++ b/internal/impact/selector.go @@ -0,0 +1,123 @@ +package impact + +import ( + "fmt" + "strings" +) + +// Selector is a conjunction of fact constraints. Every field left at its zero +// value is unconstrained, so an empty Selector with All unset matches nothing +// and is treated as a programming error by the callers that build them. +type Selector struct { + // All matches the whole estate (hieradata/common.yaml, repo-global files). + All bool + // Certname pins one host (hieradata/nodes/.yaml). + Certname string + // RoleTiers is a prefix match on enc_role_tier1..3, so a two-tier + // hieradata file covers every role nested under it. + RoleTiers []string + // RoleExact matches enc_role verbatim, which is how a role class maps to + // hosts. + RoleExact string + Country string + Region string + Virtual string + OSName string + OSMajor string + OSMinor string +} + +// Match reports whether the node satisfies every constraint set on s. +func (s Selector) Match(n Node) bool { + if s.All { + return true + } + if s.Certname != "" && s.Certname != n.Certname { + return false + } + if s.RoleExact != "" && s.RoleExact != n.Role { + return false + } + if s.Country != "" && s.Country != n.Country { + return false + } + if s.Region != "" && s.Region != n.Region { + return false + } + if s.Virtual != "" && s.Virtual != n.Virtual { + return false + } + if s.OSName != "" && !strings.EqualFold(s.OSName, n.OSName) { + return false + } + if s.OSMajor != "" && s.OSMajor != n.OSMajor { + return false + } + if s.OSMinor != "" && s.OSMinor != n.OSMinor { + return false + } + if len(s.RoleTiers) > 0 { + tiers := n.Tiers() + if len(tiers) < len(s.RoleTiers) { + return false + } + for i, want := range s.RoleTiers { + if tiers[i] != want { + return false + } + } + } + return true +} + +// Select returns the certnames of every matching node, sorted. +func (s Selector) Select(nodes []Node) []string { + var out []string + for _, n := range nodes { + if s.Match(n) { + out = append(out, n.Certname) + } + } + return out +} + +// String renders the selector the way --explain prints it. +func (s Selector) String() string { + if s.All { + return "all hosts" + } + var parts []string + add := func(k, v string) { + if v != "" { + parts = append(parts, k+"="+v) + } + } + add("certname", s.Certname) + add("enc_role", s.RoleExact) + if len(s.RoleTiers) > 0 { + parts = append(parts, "role-prefix="+strings.Join(s.RoleTiers, "::")) + } + add("country", s.Country) + add("region", s.Region) + add("virtual", s.Virtual) + add("os.name", s.OSName) + add("os.release.major", s.OSMajor) + add("os.release.minor", s.OSMinor) + if len(parts) == 0 { + return "nothing" + } + return strings.Join(parts, " ") +} + +// target is one resolved "this file reaches these hosts, for this reason". +type target struct { + sel Selector + rule string + detail string +} + +func allTarget(rule, detail string) target { + return target{sel: Selector{All: true}, rule: rule, detail: detail} +} + +func fmtDetail(format string, args ...any) string { return fmt.Sprintf(format, args...) } diff --git a/internal/impact/selector_test.go b/internal/impact/selector_test.go new file mode 100644 index 0000000..24088c8 --- /dev/null +++ b/internal/impact/selector_test.go @@ -0,0 +1,88 @@ +package impact + +import ( + "reflect" + "testing" +) + +// fixtureNodes mirrors the shape of the real estate: one country, two regions, +// physical and virtual hosts, and a mix of OS releases. +func fixtureNodes() []Node { + return []Node{ + {Certname: "web1", Role: "roles::apps::media::jellyfin", + Tier1: "apps", Tier2: "media", Tier3: "jellyfin", + Country: "au", Region: "syd1", Virtual: "lxc", + OSName: "AlmaLinux", OSMajor: "9", OSMinor: "7"}, + {Certname: "k8s1", Role: "roles::infra::k8s::node", + Tier1: "infra", Tier2: "k8s", Tier3: "node", + Country: "au", Region: "syd1", Virtual: "physical", + OSName: "AlmaLinux", OSMajor: "9", OSMinor: "8"}, + {Certname: "k8s2", Role: "roles::infra::k8s::node", + Tier1: "infra", Tier2: "k8s", Tier3: "node", + Country: "au", Region: "drw1", Virtual: "physical", + OSName: "Debian", OSMajor: "12", OSMinor: "1"}, + {Certname: "prox1", Role: "roles::infra::proxmox::node", + Tier1: "infra", Tier2: "proxmox", Tier3: "node", + Country: "nz", Region: "akl1", Virtual: "physical", + OSName: "AlmaLinux", OSMajor: "8", OSMinor: "10"}, + {Certname: "bare1", Role: "roles::base", Tier1: "base", + Country: "au", Region: "syd1", Virtual: "lxc", + OSName: "AlmaLinux", OSMajor: "9", OSMinor: "7"}, + } +} + +func TestSelectorSelect(t *testing.T) { + nodes := fixtureNodes() + tests := []struct { + name string + sel Selector + want []string + }{ + {"all", Selector{All: true}, []string{"web1", "k8s1", "k8s2", "prox1", "bare1"}}, + {"certname", Selector{Certname: "k8s2"}, []string{"k8s2"}}, + {"role exact", Selector{RoleExact: "roles::infra::k8s::node"}, []string{"k8s1", "k8s2"}}, + {"tier1 prefix", Selector{RoleTiers: []string{"infra"}}, []string{"k8s1", "k8s2", "prox1"}}, + {"tier2 prefix", Selector{RoleTiers: []string{"infra", "k8s"}}, []string{"k8s1", "k8s2"}}, + {"tier3 exact", Selector{RoleTiers: []string{"apps", "media", "jellyfin"}}, []string{"web1"}}, + {"tier1 only node", Selector{RoleTiers: []string{"base"}}, []string{"bare1"}}, + {"tier prefix longer than node", Selector{RoleTiers: []string{"base", "x"}}, nil}, + {"country", Selector{Country: "au"}, []string{"web1", "k8s1", "k8s2", "bare1"}}, + {"country region", Selector{Country: "au", Region: "syd1"}, []string{"web1", "k8s1", "bare1"}}, + {"country region role", Selector{Country: "au", Region: "syd1", RoleTiers: []string{"infra"}}, + []string{"k8s1"}}, + {"country role", Selector{Country: "au", RoleTiers: []string{"infra", "k8s"}}, []string{"k8s1", "k8s2"}}, + {"virtual", Selector{Virtual: "physical"}, []string{"k8s1", "k8s2", "prox1"}}, + {"os name", Selector{OSName: "AlmaLinux"}, []string{"web1", "k8s1", "prox1", "bare1"}}, + {"os major", Selector{OSName: "AlmaLinux", OSMajor: "9"}, []string{"web1", "k8s1", "bare1"}}, + {"os major minor", Selector{OSName: "AlmaLinux", OSMajor: "9", OSMinor: "7"}, + []string{"web1", "bare1"}}, + {"os name case insensitive", Selector{OSName: "almalinux", OSMajor: "8"}, []string{"prox1"}}, + {"os other name", Selector{OSName: "Debian", OSMajor: "12"}, []string{"k8s2"}}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := tc.sel.Select(nodes); !reflect.DeepEqual(got, tc.want) { + t.Errorf("Select() = %v, want %v", got, tc.want) + } + }) + } +} + +func TestSelectorString(t *testing.T) { + if got := (Selector{All: true}).String(); got != "all hosts" { + t.Errorf("all selector = %q", got) + } + got := Selector{Country: "au", Region: "syd1", RoleTiers: []string{"infra", "puppet"}}.String() + want := "role-prefix=infra::puppet country=au region=syd1" + if got != want { + t.Errorf("String() = %q, want %q", got, want) + } +} + +func TestNodeTiersTruncatesAtGap(t *testing.T) { + n := Node{Tier1: "base"} + if got := n.Tiers(); !reflect.DeepEqual(got, []string{"base"}) { + t.Errorf("Tiers() = %v", got) + } +} diff --git a/internal/impact/testdata/repo/hieradata/common.yaml b/internal/impact/testdata/repo/hieradata/common.yaml new file mode 100644 index 0000000..7526ef0 --- /dev/null +++ b/internal/impact/testdata/repo/hieradata/common.yaml @@ -0,0 +1,3 @@ +--- +hiera_include: + - basemod diff --git a/internal/impact/testdata/repo/hieradata/roles/apps/media/jellyfin.yaml b/internal/impact/testdata/repo/hieradata/roles/apps/media/jellyfin.yaml new file mode 100644 index 0000000..fcd65d3 --- /dev/null +++ b/internal/impact/testdata/repo/hieradata/roles/apps/media/jellyfin.yaml @@ -0,0 +1,3 @@ +--- +hiera_include: + - jellyfin diff --git a/internal/impact/testdata/repo/hieradata/roles/infra/proxmox.yaml b/internal/impact/testdata/repo/hieradata/roles/infra/proxmox.yaml new file mode 100644 index 0000000..f24947f --- /dev/null +++ b/internal/impact/testdata/repo/hieradata/roles/infra/proxmox.yaml @@ -0,0 +1,3 @@ +--- +hiera_exclude: + - basemod diff --git a/internal/impact/testdata/repo/hieradata/virtual/physical.yaml b/internal/impact/testdata/repo/hieradata/virtual/physical.yaml new file mode 100644 index 0000000..eb6f755 --- /dev/null +++ b/internal/impact/testdata/repo/hieradata/virtual/physical.yaml @@ -0,0 +1,3 @@ +--- +hiera_include: + - profiles::lldpd diff --git a/internal/impact/testdata/repo/modules/basemod/manifests/init.pp b/internal/impact/testdata/repo/modules/basemod/manifests/init.pp new file mode 100644 index 0000000..e70aa74 --- /dev/null +++ b/internal/impact/testdata/repo/modules/basemod/manifests/init.pp @@ -0,0 +1,2 @@ +class basemod { +} diff --git a/internal/impact/testdata/repo/modules/jellyfin/manifests/init.pp b/internal/impact/testdata/repo/modules/jellyfin/manifests/init.pp new file mode 100644 index 0000000..9ebc2a3 --- /dev/null +++ b/internal/impact/testdata/repo/modules/jellyfin/manifests/init.pp @@ -0,0 +1,3 @@ +class jellyfin { + include jellyfin::install +} diff --git a/internal/impact/testdata/repo/modules/jellyfin/manifests/install.pp b/internal/impact/testdata/repo/modules/jellyfin/manifests/install.pp new file mode 100644 index 0000000..7c7a471 --- /dev/null +++ b/internal/impact/testdata/repo/modules/jellyfin/manifests/install.pp @@ -0,0 +1,2 @@ +class jellyfin::install { +} diff --git a/internal/impact/testdata/repo/modules/orphanmod/manifests/init.pp b/internal/impact/testdata/repo/modules/orphanmod/manifests/init.pp new file mode 100644 index 0000000..2749f3c --- /dev/null +++ b/internal/impact/testdata/repo/modules/orphanmod/manifests/init.pp @@ -0,0 +1,2 @@ +class orphanmod { +} diff --git a/internal/impact/testdata/repo/modules/orphanmod/spec/orphanmod_spec.rb b/internal/impact/testdata/repo/modules/orphanmod/spec/orphanmod_spec.rb new file mode 100644 index 0000000..f0c08e7 --- /dev/null +++ b/internal/impact/testdata/repo/modules/orphanmod/spec/orphanmod_spec.rb @@ -0,0 +1 @@ +not a manifest diff --git a/internal/impact/testdata/repo/site/profiles/files/leaf/y.txt b/internal/impact/testdata/repo/site/profiles/files/leaf/y.txt new file mode 100644 index 0000000..7bacbfd --- /dev/null +++ b/internal/impact/testdata/repo/site/profiles/files/leaf/y.txt @@ -0,0 +1 @@ +leaf file diff --git a/internal/impact/testdata/repo/site/profiles/manifests/base.pp b/internal/impact/testdata/repo/site/profiles/manifests/base.pp new file mode 100644 index 0000000..72ba24c --- /dev/null +++ b/internal/impact/testdata/repo/site/profiles/manifests/base.pp @@ -0,0 +1,10 @@ +class profiles::base { + # a comment naming profiles::commented must not count as a reference + include profiles::mid + contain profiles::contained + class { 'profiles::viaclass': } + include profiles::cycle::a + include profiles::listed::a, profiles::listed::b + include profiles::dynamic + profiles::defined::thing { 'x': } +} diff --git a/internal/impact/testdata/repo/site/profiles/manifests/contained.pp b/internal/impact/testdata/repo/site/profiles/manifests/contained.pp new file mode 100644 index 0000000..85d6fa7 --- /dev/null +++ b/internal/impact/testdata/repo/site/profiles/manifests/contained.pp @@ -0,0 +1,2 @@ +class profiles::contained { +} diff --git a/internal/impact/testdata/repo/site/profiles/manifests/created.pp b/internal/impact/testdata/repo/site/profiles/manifests/created.pp new file mode 100644 index 0000000..c89c36f --- /dev/null +++ b/internal/impact/testdata/repo/site/profiles/manifests/created.pp @@ -0,0 +1,2 @@ +define profiles::created () { +} diff --git a/internal/impact/testdata/repo/site/profiles/manifests/cycle/a.pp b/internal/impact/testdata/repo/site/profiles/manifests/cycle/a.pp new file mode 100644 index 0000000..3b6c8d3 --- /dev/null +++ b/internal/impact/testdata/repo/site/profiles/manifests/cycle/a.pp @@ -0,0 +1,3 @@ +class profiles::cycle::a { + include profiles::cycle::b +} diff --git a/internal/impact/testdata/repo/site/profiles/manifests/cycle/b.pp b/internal/impact/testdata/repo/site/profiles/manifests/cycle/b.pp new file mode 100644 index 0000000..5906429 --- /dev/null +++ b/internal/impact/testdata/repo/site/profiles/manifests/cycle/b.pp @@ -0,0 +1,3 @@ +class profiles::cycle::b { + include profiles::cycle::a +} diff --git a/internal/impact/testdata/repo/site/profiles/manifests/defined/thing.pp b/internal/impact/testdata/repo/site/profiles/manifests/defined/thing.pp new file mode 100644 index 0000000..a7ed94c --- /dev/null +++ b/internal/impact/testdata/repo/site/profiles/manifests/defined/thing.pp @@ -0,0 +1,2 @@ +define profiles::defined::thing () { +} diff --git a/internal/impact/testdata/repo/site/profiles/manifests/dynamic.pp b/internal/impact/testdata/repo/site/profiles/manifests/dynamic.pp new file mode 100644 index 0000000..5822a39 --- /dev/null +++ b/internal/impact/testdata/repo/site/profiles/manifests/dynamic.pp @@ -0,0 +1,3 @@ +class profiles::dynamic { + include "profiles::dyn_${facts['role']}" +} diff --git a/internal/impact/testdata/repo/site/profiles/manifests/inherited.pp b/internal/impact/testdata/repo/site/profiles/manifests/inherited.pp new file mode 100644 index 0000000..e9f62d3 --- /dev/null +++ b/internal/impact/testdata/repo/site/profiles/manifests/inherited.pp @@ -0,0 +1,2 @@ +class profiles::inherited { +} diff --git a/internal/impact/testdata/repo/site/profiles/manifests/inline/thing.pp b/internal/impact/testdata/repo/site/profiles/manifests/inline/thing.pp new file mode 100644 index 0000000..01b88f5 --- /dev/null +++ b/internal/impact/testdata/repo/site/profiles/manifests/inline/thing.pp @@ -0,0 +1,2 @@ +define profiles::inline::thing () { +} diff --git a/internal/impact/testdata/repo/site/profiles/manifests/leaf.pp b/internal/impact/testdata/repo/site/profiles/manifests/leaf.pp new file mode 100644 index 0000000..56ad986 --- /dev/null +++ b/internal/impact/testdata/repo/site/profiles/manifests/leaf.pp @@ -0,0 +1,14 @@ +class profiles::leaf { + $copied = $profiles::scopevar::thing + create_resources('profiles::created', {}) + + case $copied { + 'x': { profiles::inline::thing { 'mid-line': } } + default: {} + } + + file { '/tmp/leaf': + content => template('profiles/leaf/x.erb'), + source => 'puppet:///modules/profiles/leaf/y.txt', + } +} diff --git a/internal/impact/testdata/repo/site/profiles/manifests/listed/a.pp b/internal/impact/testdata/repo/site/profiles/manifests/listed/a.pp new file mode 100644 index 0000000..9472e7f --- /dev/null +++ b/internal/impact/testdata/repo/site/profiles/manifests/listed/a.pp @@ -0,0 +1,2 @@ +class profiles::listed::a { +} diff --git a/internal/impact/testdata/repo/site/profiles/manifests/listed/b.pp b/internal/impact/testdata/repo/site/profiles/manifests/listed/b.pp new file mode 100644 index 0000000..6d70de6 --- /dev/null +++ b/internal/impact/testdata/repo/site/profiles/manifests/listed/b.pp @@ -0,0 +1,2 @@ +class profiles::listed::b { +} diff --git a/internal/impact/testdata/repo/site/profiles/manifests/lldpd.pp b/internal/impact/testdata/repo/site/profiles/manifests/lldpd.pp new file mode 100644 index 0000000..225698b --- /dev/null +++ b/internal/impact/testdata/repo/site/profiles/manifests/lldpd.pp @@ -0,0 +1,2 @@ +class profiles::lldpd { +} diff --git a/internal/impact/testdata/repo/site/profiles/manifests/mid.pp b/internal/impact/testdata/repo/site/profiles/manifests/mid.pp new file mode 100644 index 0000000..3654ce8 --- /dev/null +++ b/internal/impact/testdata/repo/site/profiles/manifests/mid.pp @@ -0,0 +1,3 @@ +class profiles::mid inherits profiles::inherited { + include profiles::leaf +} diff --git a/internal/impact/testdata/repo/site/profiles/manifests/scopevar.pp b/internal/impact/testdata/repo/site/profiles/manifests/scopevar.pp new file mode 100644 index 0000000..2d68546 --- /dev/null +++ b/internal/impact/testdata/repo/site/profiles/manifests/scopevar.pp @@ -0,0 +1,3 @@ +class profiles::scopevar { + $thing = 'x' +} diff --git a/internal/impact/testdata/repo/site/profiles/manifests/viaclass.pp b/internal/impact/testdata/repo/site/profiles/manifests/viaclass.pp new file mode 100644 index 0000000..b9e9ec7 --- /dev/null +++ b/internal/impact/testdata/repo/site/profiles/manifests/viaclass.pp @@ -0,0 +1,2 @@ +class profiles::viaclass { +} diff --git a/internal/impact/testdata/repo/site/profiles/templates/leaf/x.erb b/internal/impact/testdata/repo/site/profiles/templates/leaf/x.erb new file mode 100644 index 0000000..1fd5618 --- /dev/null +++ b/internal/impact/testdata/repo/site/profiles/templates/leaf/x.erb @@ -0,0 +1 @@ +leaf template diff --git a/internal/impact/testdata/repo/site/roles/manifests/apps/media/jellyfin.pp b/internal/impact/testdata/repo/site/roles/manifests/apps/media/jellyfin.pp new file mode 100644 index 0000000..7d065c5 --- /dev/null +++ b/internal/impact/testdata/repo/site/roles/manifests/apps/media/jellyfin.pp @@ -0,0 +1,3 @@ +class roles::apps::media::jellyfin { + include profiles::base +} diff --git a/internal/impact/testdata/repo/site/roles/manifests/infra/k8s/node.pp b/internal/impact/testdata/repo/site/roles/manifests/infra/k8s/node.pp new file mode 100644 index 0000000..189712b --- /dev/null +++ b/internal/impact/testdata/repo/site/roles/manifests/infra/k8s/node.pp @@ -0,0 +1,3 @@ +class roles::infra::k8s::node { + include profiles::base +} diff --git a/internal/impact/unresolved_test.go b/internal/impact/unresolved_test.go new file mode 100644 index 0000000..d2d898e --- /dev/null +++ b/internal/impact/unresolved_test.go @@ -0,0 +1,166 @@ +package impact + +import ( + "bytes" + "encoding/json" + "os" + "path/filepath" + "reflect" + "strings" + "testing" +) + +// tempRepo writes a throwaway control repo from a path -> content map. +func tempRepo(t *testing.T, files map[string]string) string { + t.Helper() + root := t.TempDir() + for rel, content := range files { + full := filepath.Join(root, filepath.FromSlash(rel)) + if err := os.MkdirAll(filepath.Dir(full), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(full, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + } + return root +} + +func buildIndex(t *testing.T, files map[string]string) *Index { + t.Helper() + idx, err := BuildIndex(tempRepo(t, files)) + if err != nil { + t.Fatalf("BuildIndex: %v", err) + } + return idx +} + +func TestIndexRecordsUnresolvedReferenceAgainstItsFile(t *testing.T) { + idx := testIndex(t) + got := idx.UnresolvedIn("site/profiles/manifests/dynamic.pp") + want := []UnresolvedRef{{ + File: "site/profiles/manifests/dynamic.pp", + Form: "include", + Expr: `"profiles::dyn_${facts['role']}"`, + Prefix: "profiles::dyn_", + }} + if !reflect.DeepEqual(got, want) { + t.Errorf("UnresolvedIn = %#v, want %#v", got, want) + } +} + +func TestResolveWidensThroughAnUnresolvedPrefix(t *testing.T) { + // profiles::dynamic includes "profiles::dyn_${...}". Any class under that + // prefix must reach whatever profiles::dynamic reaches. + if got, want := resolvedHosts(t, "profiles::dyn_web"), []string{"k8s1", "k8s2", "web1"}; !reflect.DeepEqual(got, want) { + t.Errorf("profiles::dyn_web = %v, want %v", got, want) + } + // A class outside the prefix is not dragged in by it. + res := newClassResolver(testIndex(t), fixtureNodes()).resolve("profiles::unrelated_thing") + if !res.orphan { + t.Errorf("an unrelated class was widened to %v", res.hosts) + } +} + +func TestResolveWidensThroughAnUnresolvedVariable(t *testing.T) { + // With no literal prefix to match on, every class is a candidate, so the + // holding class's hosts are included rather than dropped. + idx := buildIndex(t, map[string]string{ + "site/roles/manifests/base.pp": "class roles::base {\n include profiles::base\n}\n", + "site/profiles/manifests/base.pp": "class profiles::base {\n include profiles::wild\n}\n", + "site/profiles/manifests/wild.pp": "class profiles::wild {\n $c = lookup('profile')\n include $c\n}\n", + "site/profiles/manifests/standalone.pp": "class profiles::standalone {\n}\n", + }) + res := newClassResolver(idx, fixtureNodes()).resolve("profiles::standalone") + if got, want := uniqueSorted(hostNames(res)), []string{"bare1"}; !reflect.DeepEqual(got, want) { + t.Errorf("hosts = %v, want %v", got, want) + } + if len(res.unresolved) != 1 || res.unresolved[0].File != "site/profiles/manifests/wild.pp" { + t.Errorf("resolution did not report the unresolved reference: %#v", res.unresolved) + } +} + +func hostNames(res *resolution) []string { + out := make([]string, 0, len(res.hosts)) + for certname := range res.hosts { + out = append(out, certname) + } + return out +} + +func TestAnalyzeReportsUnresolvedReferencesInAChangedFile(t *testing.T) { + r := analyze(t, "site/profiles/manifests/dynamic.pp") + if !r.Summary.ResolutionIncomplete { + t.Error("summary did not flag the incomplete resolution") + } + if len(r.UnresolvedRefs) != 1 || r.UnresolvedRefs[0].Prefix != "profiles::dyn_" { + t.Fatalf("UnresolvedRefs = %#v", r.UnresolvedRefs) + } + + var explain bytes.Buffer + if err := RenderExplain(&explain, r); err != nil { + t.Fatalf("RenderExplain: %v", err) + } + for _, want := range []string{"resolution incomplete", "unresolved class references", + "site/profiles/manifests/dynamic.pp", "profiles::dyn_"} { + if !strings.Contains(explain.String(), want) { + t.Errorf("explain output missing %q:\n%s", want, explain.String()) + } + } + + var out bytes.Buffer + if err := RenderJSON(&out, r); err != nil { + t.Fatalf("RenderJSON: %v", err) + } + var doc struct { + UnresolvedRefs []UnresolvedRef `json:"unresolved_references"` + Summary struct { + ResolutionIncomplete bool `json:"resolution_incomplete"` + } `json:"summary"` + } + if err := json.Unmarshal(out.Bytes(), &doc); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if !doc.Summary.ResolutionIncomplete || len(doc.UnresolvedRefs) != 1 { + t.Errorf("json = %s", out.String()) + } +} + +func TestAnalyzeReportsNoUnresolvedReferencesForACleanFile(t *testing.T) { + r := analyze(t, "site/profiles/manifests/leaf.pp") + if r.Summary.ResolutionIncomplete || len(r.UnresolvedRefs) != 0 { + t.Errorf("clean file reported %#v", r.UnresolvedRefs) + } +} + +func TestExplainCallsAnOrderingReferenceWhatItIs(t *testing.T) { + idx := buildIndex(t, map[string]string{ + "site/roles/manifests/base.pp": "class roles::base {\n include profiles::base\n}\n", + "site/profiles/manifests/base.pp": "class profiles::base {\n service { 's': require => Class['profiles::ordered'] }\n}\n", + "site/profiles/manifests/ordered.pp": "class profiles::ordered {\n}\n", + }) + res := newClassResolver(idx, fixtureNodes()).resolve("profiles::ordered") + hits := res.hosts["bare1"] + if len(hits) == 0 { + t.Fatalf("profiles::ordered reached no host: %#v", res.hosts) + } + if !strings.Contains(hits[0].detail, "not a declaration") { + t.Errorf("detail = %q, want it to mark the ordering reference", hits[0].detail) + } +} + +func TestExplainLeavesADeclarationUnannotated(t *testing.T) { + idx := buildIndex(t, map[string]string{ + "site/roles/manifests/base.pp": "class roles::base {\n include profiles::base\n}\n", + "site/profiles/manifests/base.pp": "class profiles::base {\n include profiles::ordered\n}\n", + "site/profiles/manifests/ordered.pp": "class profiles::ordered {\n}\n", + }) + res := newClassResolver(idx, fixtureNodes()).resolve("profiles::ordered") + hits := res.hosts["bare1"] + if len(hits) == 0 { + t.Fatalf("profiles::ordered reached no host: %#v", res.hosts) + } + if strings.Contains(hits[0].detail, "not a declaration") { + t.Errorf("detail = %q, an include is a declaration", hits[0].detail) + } +} diff --git a/internal/puppetdb/puppetdb.go b/internal/puppetdb/puppetdb.go index b3812a1..284fa0d 100644 --- a/internal/puppetdb/puppetdb.go +++ b/internal/puppetdb/puppetdb.go @@ -37,22 +37,24 @@ func New(baseURL string, timeout time.Duration) *Client { } } +// factRecord is one fact as the facts endpoint returns it. Value stays raw so +// that a single undecodable fact can be dropped without costing the rest. 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) - } +// FactRecord is one (certname, fact) pair with its value decoded. +type FactRecord struct { + Certname string + Name string + Value any +} - body, status, err := c.get(ctx, "/pdb/query/v4/facts", string(query)) +// Facts returns the node's facts as the values map a catalog request needs. +func (c *Client) Facts(ctx context.Context, certname string) (map[string]any, error) { + body, status, err := c.queryFacts(ctx, []string{"=", "certname", certname}, certname) if err != nil { return nil, err } @@ -73,16 +75,92 @@ func (c *Client) Facts(ctx context.Context, certname string) (map[string]any, er 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) { +// FactsByName returns the named facts for every node PuppetDB knows about, in +// a single query. Tools that evaluate selectors over the whole estate want the +// table once rather than a request per host. +func (c *Client) FactsByName(ctx context.Context, names []string) ([]FactRecord, error) { + if len(names) == 0 { + return nil, nil + } + clauses := make([]any, 0, len(names)+1) + clauses = append(clauses, "or") + for _, n := range names { + clauses = append(clauses, []any{"=", "name", n}) + } + query := []any{"extract", []string{"certname", "name", "value"}, clauses} + + body, status, err := c.queryFacts(ctx, query, "the estate") + if err != nil { + return nil, err + } + if status != http.StatusOK { + return nil, fmt.Errorf("puppetdb returned %d for fact query: %s", status, snippet(body)) + } + + records, err := decodeRecords(body, "the estate") + if err != nil { + return nil, err + } + + out := make([]FactRecord, 0, len(records)) + var skipped []string + for _, r := range records { + v, ok := decodeValue(r) + if !ok { + skipped = append(skipped, r.Name) + continue + } + out = append(out, FactRecord{Certname: r.Certname, Name: r.Name, Value: v}) + } + if len(skipped) > 0 && c.Warn != nil { + c.Warn(fmt.Sprintf("skipped %d undecodable fact(s) in the estate fact table: %s", + len(skipped), strings.Join(skipped, ", "))) + } + return out, nil +} + +// queryFacts runs an AST query against the facts endpoint. subject names what +// was asked for so a failure says so. +func (c *Client) queryFacts(ctx context.Context, query any, subject string) ([]byte, int, error) { + encoded, err := json.Marshal(query) + if err != nil { + return nil, 0, fmt.Errorf("building facts query for %s: %w", subject, err) + } + return c.get(ctx, "/pdb/query/v4/facts", string(encoded)) +} + +// decodeRecords reads the flat array of fact records the facts endpoint +// answers with. +func decodeRecords(body []byte, subject 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) + return nil, fmt.Errorf("parsing facts for %s: %w", subject, err) + } + return records, nil +} + +// decodeValue reports a record's value and whether it decoded at all. +func decodeValue(r factRecord) (any, bool) { + if len(r.Value) == 0 { + return nil, false + } + var v any + if json.Unmarshal(r.Value, &v) != nil { + return nil, false + } + return v, true +} + +// decodeFacts reads the fact records a single-node 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) { + records, err := decodeRecords(body, certname) + if err != nil { + return nil, err } kept := make([]factRecord, 0, len(records)) @@ -156,8 +234,8 @@ func (c *Client) fold(certname string, records []factRecord) (map[string]any, er if r.Name == "" { continue } - var v any - if len(r.Value) == 0 || json.Unmarshal(r.Value, &v) != nil { + v, ok := decodeValue(r) + if !ok { skipped = append(skipped, r.Name) continue } diff --git a/internal/puppetdb/puppetdb_test.go b/internal/puppetdb/puppetdb_test.go index 7d9b450..37224c4 100644 --- a/internal/puppetdb/puppetdb_test.go +++ b/internal/puppetdb/puppetdb_test.go @@ -315,3 +315,88 @@ func TestFactsWithNothingDecodableIsAnError(t *testing.T) { t.Errorf("unhelpful error: %v", err) } } + +func TestFactsByName(t *testing.T) { + var gotQuery, gotPath string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotPath = r.URL.Path + gotQuery = r.URL.Query().Get("query") + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`[ + {"certname":"a.example.net","name":"country","value":"au"}, + {"certname":"a.example.net","name":"os","value":{"name":"AlmaLinux","release":{"major":"9","minor":"7"}}} + ]`)) + })) + defer srv.Close() + + records, err := New(srv.URL, 5*time.Second).FactsByName(context.Background(), []string{"country", "os"}) + if err != nil { + t.Fatalf("FactsByName: %v", err) + } + if gotPath != "/pdb/query/v4/facts" { + t.Errorf("path = %s", gotPath) + } + if gotQuery != `["extract",["certname","name","value"],["or",["=","name","country"],["=","name","os"]]]` { + t.Errorf("query = %s", gotQuery) + } + if len(records) != 2 { + t.Fatalf("records = %#v", records) + } + if records[0].Certname != "a.example.net" || records[0].Value != "au" { + t.Errorf("first record = %#v", records[0]) + } + os, ok := records[1].Value.(map[string]any) + if !ok || os["name"] != "AlmaLinux" { + t.Errorf("structured os fact lost: %#v", records[1].Value) + } +} + +func TestFactsByNameEmptyNames(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { + t.Error("no request should be made for an empty fact list") + })) + defer srv.Close() + + records, err := New(srv.URL, 5*time.Second).FactsByName(context.Background(), nil) + if err != nil || records != nil { + t.Errorf("FactsByName(nil) = %v, %v", records, err) + } +} + +func TestFactsByNameServerError(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() + + if _, err := New(srv.URL, 5*time.Second).FactsByName(context.Background(), []string{"country"}); err == nil { + t.Fatal("expected an error for a 500") + } +} + +func TestFactsByNameSkipsUndecodableRecords(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte("\ufeff" + `[ + {"certname":"a.example.net","name":"country","value":"au"}, + {"certname":"a.example.net","name":"broken"} + ]`)) + })) + defer srv.Close() + + var warnings []string + c := New(srv.URL, 5*time.Second) + c.Warn = func(msg string) { warnings = append(warnings, msg) } + + records, err := c.FactsByName(context.Background(), []string{"country", "broken"}) + if err != nil { + t.Fatalf("FactsByName: %v", err) + } + if len(records) != 1 || records[0].Name != "country" { + t.Fatalf("records = %#v", records) + } + if len(warnings) != 1 || !strings.Contains(warnings[0], "broken") { + t.Errorf("the skipped fact must be reported, got %v", warnings) + } +} diff --git a/packaging/nfpm.yaml b/packaging/nfpm.yaml index 2ba43a1..e9bab8a 100644 --- a/packaging/nfpm.yaml +++ b/packaging/nfpm.yaml @@ -22,24 +22,26 @@ replaces: provides: - puppet-catalog-tools +# Globs, so a new tool under cmd/ ships without editing this file. contents: - - src: dist/catalog-diff - dst: /usr/bin/catalog-diff + - src: dist/bin/* + dst: /usr/bin 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 + # Shell completions (generated by scripts/build-rpm.sh before packaging, + # already named as they install). + - src: dist/completions/bash/* + dst: /usr/share/bash-completion/completions file_info: mode: 0644 - - src: dist/completions/_catalog-diff - dst: /usr/share/zsh/site-functions/_catalog-diff + - src: dist/completions/zsh/* + dst: /usr/share/zsh/site-functions file_info: mode: 0644 - - src: dist/completions/catalog-diff.fish - dst: /usr/share/fish/vendor_completions.d/catalog-diff.fish + - src: dist/completions/fish/* + dst: /usr/share/fish/vendor_completions.d file_info: mode: 0644 diff --git a/scripts/build-rpm.sh b/scripts/build-rpm.sh index bfcb5b0..b88b361 100755 --- a/scripts/build-rpm.sh +++ b/scripts/build-rpm.sh @@ -12,24 +12,37 @@ 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" +BIN_DIR="${DIST}/bin" +COMP_DIR="${DIST}/completions" + +# Every tool is a main package under cmd/, so the shipped list is the contents +# of cmd/ — the same list the Makefile builds. +BINARIES=() +for dir in cmd/*/; do + BINARIES+=("$(basename "${dir}")") +done +if [ ${#BINARIES[@]} -eq 0 ]; then + echo "ERROR: no binaries found under cmd/" >&2 + exit 1 +fi for b in "${BINARIES[@]}"; do - if [ ! -f "${DIST}/${b}" ]; then - echo "ERROR: ${DIST}/${b} not found; run 'make build' first" >&2 + if [ ! -f "${BIN_DIR}/${b}" ]; then + echo "ERROR: ${BIN_DIR}/${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}" +# match the shipped flags/subcommands. They are written under the name and +# directory they install as, because nfpm globs these directories. +rm -rf "${COMP_DIR}" +mkdir -p "${COMP_DIR}/bash" "${COMP_DIR}/zsh" "${COMP_DIR}/fish" 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" + "./${BIN_DIR}/${b}" completion bash >"${COMP_DIR}/bash/${b}" + "./${BIN_DIR}/${b}" completion zsh >"${COMP_DIR}/zsh/_${b}" + "./${BIN_DIR}/${b}" completion fish >"${COMP_DIR}/fish/${b}.fish" done export PACKAGE_NAME="${PACKAGE}" @@ -37,7 +50,7 @@ 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_DESCRIPTION="Puppet catalog tooling: catalog-diff compiles a host's catalog in two environments and diffs them; catalog-impact lists the hosts a control repo change affects" export PACKAGE_MAINTAINER="Ben Vincent " export PACKAGE_HOMEPAGE="https://git.unkin.net/unkin/puppet-catalog-tools" export PACKAGE_LICENSE="MIT"