From 8093933500e11cee82680c49cb6cf9adf4b2cc0e Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 12 Sep 2026 16:15:00 +1000 Subject: [PATCH 1/8] add the catalog-impact binary Work out which hosts a puppet-prod change can affect, so a catalog test can fan out over exactly those hosts instead of the whole estate. - add cmd/catalog-impact and internal/impact - map hieradata paths onto fact selectors from the hiera hierarchy - trace manifest classes back to role classes through include, contain, class {} and hiera_include, to a fixed point - fetch the estate fact table from pdbmux in one query via FactsByName - widen to all hosts, loudly, for any path that matches no rule - ship the binary and its completions in the RPM --- AGENTS.md | 22 ++ Makefile | 2 +- README.md | 124 +++++++- cmd/catalog-impact/main.go | 152 ++++++++++ cmd/catalog-impact/main_test.go | 89 ++++++ internal/impact/git.go | 53 ++++ internal/impact/git_test.go | 89 ++++++ internal/impact/impact.go | 159 ++++++++++ internal/impact/impact_test.go | 256 ++++++++++++++++ internal/impact/index.go | 279 ++++++++++++++++++ internal/impact/node.go | 132 +++++++++ internal/impact/paths.go | 249 ++++++++++++++++ internal/impact/paths_test.go | 132 +++++++++ internal/impact/render.go | 102 +++++++ internal/impact/resolve.go | 139 +++++++++ internal/impact/resolve_test.go | 125 ++++++++ internal/impact/selector.go | 123 ++++++++ internal/impact/selector_test.go | 88 ++++++ .../testdata/repo/hieradata/common.yaml | 3 + .../hieradata/roles/apps/media/jellyfin.yaml | 3 + .../repo/hieradata/roles/infra/proxmox.yaml | 3 + .../repo/hieradata/virtual/physical.yaml | 3 + .../repo/modules/basemod/manifests/init.pp | 2 + .../repo/modules/jellyfin/manifests/init.pp | 3 + .../modules/jellyfin/manifests/install.pp | 2 + .../repo/modules/orphanmod/manifests/init.pp | 2 + .../modules/orphanmod/spec/orphanmod_spec.rb | 1 + .../repo/site/profiles/files/leaf/y.txt | 1 + .../repo/site/profiles/manifests/base.pp | 8 + .../repo/site/profiles/manifests/contained.pp | 2 + .../repo/site/profiles/manifests/cycle/a.pp | 3 + .../repo/site/profiles/manifests/cycle/b.pp | 3 + .../site/profiles/manifests/defined/thing.pp | 2 + .../repo/site/profiles/manifests/leaf.pp | 6 + .../repo/site/profiles/manifests/lldpd.pp | 2 + .../repo/site/profiles/manifests/mid.pp | 3 + .../repo/site/profiles/manifests/viaclass.pp | 2 + .../repo/site/profiles/templates/leaf/x.erb | 1 + .../roles/manifests/apps/media/jellyfin.pp | 3 + .../site/roles/manifests/infra/k8s/node.pp | 3 + internal/pdbmux/pdbmux.go | 42 +++ internal/pdbmux/pdbmux_test.go | 55 ++++ packaging/nfpm.yaml | 20 ++ scripts/build-rpm.sh | 4 +- 44 files changed, 2492 insertions(+), 5 deletions(-) create mode 100644 cmd/catalog-impact/main.go create mode 100644 cmd/catalog-impact/main_test.go create mode 100644 internal/impact/git.go create mode 100644 internal/impact/git_test.go create mode 100644 internal/impact/impact.go create mode 100644 internal/impact/impact_test.go create mode 100644 internal/impact/index.go create mode 100644 internal/impact/node.go create mode 100644 internal/impact/paths.go create mode 100644 internal/impact/paths_test.go create mode 100644 internal/impact/render.go create mode 100644 internal/impact/resolve.go create mode 100644 internal/impact/resolve_test.go create mode 100644 internal/impact/selector.go create mode 100644 internal/impact/selector_test.go create mode 100644 internal/impact/testdata/repo/hieradata/common.yaml create mode 100644 internal/impact/testdata/repo/hieradata/roles/apps/media/jellyfin.yaml create mode 100644 internal/impact/testdata/repo/hieradata/roles/infra/proxmox.yaml create mode 100644 internal/impact/testdata/repo/hieradata/virtual/physical.yaml create mode 100644 internal/impact/testdata/repo/modules/basemod/manifests/init.pp create mode 100644 internal/impact/testdata/repo/modules/jellyfin/manifests/init.pp create mode 100644 internal/impact/testdata/repo/modules/jellyfin/manifests/install.pp create mode 100644 internal/impact/testdata/repo/modules/orphanmod/manifests/init.pp create mode 100644 internal/impact/testdata/repo/modules/orphanmod/spec/orphanmod_spec.rb create mode 100644 internal/impact/testdata/repo/site/profiles/files/leaf/y.txt create mode 100644 internal/impact/testdata/repo/site/profiles/manifests/base.pp create mode 100644 internal/impact/testdata/repo/site/profiles/manifests/contained.pp create mode 100644 internal/impact/testdata/repo/site/profiles/manifests/cycle/a.pp create mode 100644 internal/impact/testdata/repo/site/profiles/manifests/cycle/b.pp create mode 100644 internal/impact/testdata/repo/site/profiles/manifests/defined/thing.pp create mode 100644 internal/impact/testdata/repo/site/profiles/manifests/leaf.pp create mode 100644 internal/impact/testdata/repo/site/profiles/manifests/lldpd.pp create mode 100644 internal/impact/testdata/repo/site/profiles/manifests/mid.pp create mode 100644 internal/impact/testdata/repo/site/profiles/manifests/viaclass.pp create mode 100644 internal/impact/testdata/repo/site/profiles/templates/leaf/x.erb create mode 100644 internal/impact/testdata/repo/site/roles/manifests/apps/media/jellyfin.pp create mode 100644 internal/impact/testdata/repo/site/roles/manifests/infra/k8s/node.pp diff --git a/AGENTS.md b/AGENTS.md index a5418c1..e2bc53f 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/pdbmux/ # unauthenticated fact client for pdbmux internal/config/ # XDG config file + env var loading packaging/nfpm.yaml # RPM spec (envsubst-templated) @@ -36,6 +42,9 @@ Makefile and to `scripts/build-rpm.sh`, and add its files to certname must be allowed in the puppetserver `auth.conf`. - Facts: `GET /pdb/query/v4/nodes//facts` on pdbmux, falling back to `/pdb/query/v4/factsets/`. No auth, no client cert. +- Estate-wide facts: one AST query against `/pdb/query/v4/facts` + (`pdbmux.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 @@ -52,6 +61,19 @@ 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 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..6a33732 100644 --- a/Makefile +++ b/Makefile @@ -1,6 +1,6 @@ # 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 +BINARIES := catalog-diff catalog-impact DIST := dist VERSION := $(shell git describe --tags --always --dirty 2>/dev/null || echo dev) GOFLAGS := -ldflags="-s -w -X main.version=$(VERSION)" diff --git a/README.md b/README.md index db55727..32ea494 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 `pdbmux`. 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/ ``` +# catalog-diff + ## Usage ```bash @@ -119,6 +123,122 @@ 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 | +| `--pdb` | pdbmux 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` | pdbmux request timeout (default 1m) | + +`--pdb` defaults to the shared configuration described under +[Configuration](#configuration), so `pdb_url` and `CATALOG_DIFF_PDB` apply to +both binaries. No credentials are needed. + +Facts for the whole estate are fetched from pdbmux 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`, + `class { '...': }`, capitalised resource references and defined-type usage. +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. + +Paths outside the Puppet modulepath — `.woodpecker/`, `doc/`, `spec/`, +`vendor/`, `Gemfile`, `*.md` — cannot reach a catalog and are skipped. + +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..0805844 --- /dev/null +++ b/cmd/catalog-impact/main.go @@ -0,0 +1,152 @@ +// 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/pdbmux" +) + +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 +rather than silently returning nothing.` + +type options struct { + repo string + branch string + base string + files []string + pdb 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.pdb, "pdb", cfg.PDBURL, "pdbmux 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, "pdbmux 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, err := impact.LoadNodes(ctx, pdbmux.New(opts.pdb, opts.timeout)) + if err != nil { + return err + } + + analyzer := &impact.Analyzer{Index: idx, Nodes: nodes} + 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 _, 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..29dbcdb --- /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 fakePDB(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"}, + pdb: fakePDB(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"}, + pdb: fakePDB(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..3856e33 --- /dev/null +++ b/internal/impact/impact.go @@ -0,0 +1,159 @@ +package impact + +import "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"` +} + +// 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"` + 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:"-"` + // 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 +} + +// 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{} + result := &Result{ + Base: base, + Branch: branch, + ChangedFiles: uniqueSorted(files), + 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}) + } + } + + for _, ref := range m.classes { + res := resolver.resolve(ref.name) + 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}) + } + } + } + } + + for name := range orphans { + result.OrphanClasses = append(result.OrphanClasses, name) + } + sort.Strings(result.OrphanClasses) + + 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), + } + + 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 +} diff --git a/internal/impact/impact_test.go b/internal/impact/impact_test.go new file mode 100644 index 0000000..03e29ef --- /dev/null +++ b/internal/impact/impact_test.go @@ -0,0 +1,256 @@ +package impact + +import ( + "bytes" + "context" + "encoding/json" + "reflect" + "strings" + "testing" + + "git.unkin.net/unkin/puppet-catalog-tools/internal/pdbmux" +) + +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 pdbmux would. +type stubFacts struct { + records []pdbmux.FactRecord + asked []string +} + +func (s *stubFacts) FactsByName(_ context.Context, names []string) ([]pdbmux.FactRecord, error) { + s.asked = names + return s.records, nil +} + +func TestLoadNodesFoldsStructuredOS(t *testing.T) { + src := &stubFacts{records: []pdbmux.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, err := LoadNodes(context.Background(), src) + if err != nil { + t.Fatalf("LoadNodes: %v", err) + } + 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") + } +} diff --git a/internal/impact/index.go b/internal/impact/index.go new file mode 100644 index 0000000..c60dc75 --- /dev/null +++ b/internal/impact/index.go @@ -0,0 +1,279 @@ +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 ( + commentRe = regexp.MustCompile(`(?m)#.*$`) + includeRe = regexp.MustCompile(`\b(?:include|contain|require)\s*\(?\s*['"]?(?:::)?([a-z][a-z0-9_]*(?:::[a-z0-9_]+)*)['"]?`) + classParamRe = regexp.MustCompile(`\bclass\s*\{\s*['"](?:::)?([a-z][a-z0-9_]*(?:::[a-z0-9_]+)*)['"]`) + classRefRe = regexp.MustCompile(`\bClass\s*\[\s*['"](?:::)?([a-z][a-z0-9_]*(?:::[a-z0-9_]+)*)['"]`) + defineUseRe = regexp.MustCompile(`(?m)^\s*(?:::)?([a-z][a-z0-9_]*(?:::[a-z0-9_]+)+)\s*\{`) + 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 declare it. + includedBy map[string]map[string]bool + // 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 +} + +// 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]bool{}, + moduleClasses: map[string][]string{}, + assetOwners: map[string][]string{}, + hieraInclude: map[string][]string{}, + hieraExclude: map[string][]string{}, + known: map[string]bool{}, + } + + 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 := commentRe.ReplaceAllString(string(data), "") + + self := map[string]bool{} + for _, name := range declared { + self[name] = true + } + for _, ref := range references(body) { + if self[ref] { + continue + } + if i.includedBy[ref] == nil { + i.includedBy[ref] = map[string]bool{} + } + for _, name := range declared { + i.includedBy[ref][name] = true + } + } + + for _, asset := range assetReferences(body) { + i.assetOwners[asset] = append(i.assetOwners[asset], declared...) + } + return nil +} + +// references extracts every class or defined-type name a manifest body names, +// across all the declaration forms this repo uses. +func references(body string) []string { + seen := map[string]bool{} + add := func(name string) { + if name != "" && name != "class" && name != "define" { + seen[name] = true + } + } + for _, re := range []*regexp.Regexp{includeRe, classParamRe, classRefRe, defineUseRe} { + for _, m := range re.FindAllStringSubmatch(body, -1) { + add(m[1]) + } + } + // Resource references (Profiles::Foo::Bar['x']) name a defined type in + // capitalised form. + for _, m := range resourceRefRe.FindAllStringSubmatch(body, -1) { + add(strings.ToLower(m[1])) + } + + out := make([]string, 0, len(seen)) + for name := range seen { + out = append(out, name) + } + sort.Strings(out) + return out +} + +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 declare 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 +} + +// 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/node.go b/internal/impact/node.go new file mode 100644 index 0000000..03a6399 --- /dev/null +++ b/internal/impact/node.go @@ -0,0 +1,132 @@ +// 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/pdbmux" +) + +// 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) ([]pdbmux.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. +func LoadNodes(ctx context.Context, src FactSource) ([]Node, error) { + records, err := src.FactsByName(ctx, FactNames) + if err != nil { + return nil, err + } + + byName := map[string]*Node{} + for _, r := range records { + if r.Certname == "" { + 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, fmt.Errorf("pdbmux returned no nodes") + } + return nodes, nil +} + +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..9558c4a --- /dev/null +++ b/internal/impact/render.go @@ -0,0 +1,102 @@ +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{} + } + 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 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, "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 +} diff --git a/internal/impact/resolve.go b/internal/impact/resolve.go new file mode 100644 index 0000000..b460659 --- /dev/null +++ b/internal/impact/resolve.go @@ -0,0 +1,139 @@ +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 +} + +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 + queue = append(queue, parent) + } + } + + res.orphan = !reached + 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..37f61d5 --- /dev/null +++ b/internal/impact/resolve_test.go @@ -0,0 +1,125 @@ +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) + } +} 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..bbe4146 --- /dev/null +++ b/internal/impact/testdata/repo/site/profiles/manifests/base.pp @@ -0,0 +1,8 @@ +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 + 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/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/leaf.pp b/internal/impact/testdata/repo/site/profiles/manifests/leaf.pp new file mode 100644 index 0000000..16e54dc --- /dev/null +++ b/internal/impact/testdata/repo/site/profiles/manifests/leaf.pp @@ -0,0 +1,6 @@ +class profiles::leaf { + 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/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..5e54151 --- /dev/null +++ b/internal/impact/testdata/repo/site/profiles/manifests/mid.pp @@ -0,0 +1,3 @@ +class profiles::mid { + include profiles::leaf +} 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/pdbmux/pdbmux.go b/internal/pdbmux/pdbmux.go index 2952b3a..b1cf909 100644 --- a/internal/pdbmux/pdbmux.go +++ b/internal/pdbmux/pdbmux.go @@ -128,3 +128,45 @@ func snippet(body []byte) string { } return s } + +// FactRecord is one (certname, fact) pair from an estate-wide fact query. +type FactRecord struct { + Certname string `json:"certname"` + Name string `json:"name"` + Value any `json:"value"` +} + +// FactsByName returns the named facts for every node pdbmux 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} + + encoded, err := json.Marshal(query) + if err != nil { + return nil, fmt.Errorf("encoding fact query: %w", err) + } + path := "/pdb/query/v4/facts?query=" + url.QueryEscape(string(encoded)) + + body, status, err := c.get(ctx, path) + if err != nil { + return nil, err + } + if status != http.StatusOK { + return nil, fmt.Errorf("pdbmux returned %d for fact query: %s", status, snippet(body)) + } + + var records []FactRecord + if err := json.Unmarshal(body, &records); err != nil { + return nil, fmt.Errorf("parsing fact query response: %w", err) + } + return records, nil +} diff --git a/internal/pdbmux/pdbmux_test.go b/internal/pdbmux/pdbmux_test.go index adffbce..15ed23b 100644 --- a/internal/pdbmux/pdbmux_test.go +++ b/internal/pdbmux/pdbmux_test.go @@ -157,3 +157,58 @@ func TestFactsContextCancelled(t *testing.T) { t.Fatal("expected a timeout error") } } + +func TestFactsByName(t *testing.T) { + var gotQuery string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + 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 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") + } +} diff --git a/packaging/nfpm.yaml b/packaging/nfpm.yaml index 2ba43a1..ba195ef 100644 --- a/packaging/nfpm.yaml +++ b/packaging/nfpm.yaml @@ -30,6 +30,13 @@ contents: owner: root group: root + - src: dist/catalog-impact + dst: /usr/bin/catalog-impact + 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 @@ -43,3 +50,16 @@ contents: dst: /usr/share/fish/vendor_completions.d/catalog-diff.fish file_info: mode: 0644 + + - src: dist/completions/catalog-impact.bash + dst: /usr/share/bash-completion/completions/catalog-impact + file_info: + mode: 0644 + - src: dist/completions/_catalog-impact + dst: /usr/share/zsh/site-functions/_catalog-impact + file_info: + mode: 0644 + - src: dist/completions/catalog-impact.fish + dst: /usr/share/fish/vendor_completions.d/catalog-impact.fish + file_info: + mode: 0644 diff --git a/scripts/build-rpm.sh b/scripts/build-rpm.sh index bfcb5b0..382e1cc 100755 --- a/scripts/build-rpm.sh +++ b/scripts/build-rpm.sh @@ -12,7 +12,7 @@ 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) +BINARIES=(catalog-diff catalog-impact) DIST="dist" for b in "${BINARIES[@]}"; do @@ -37,7 +37,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" -- 2.47.3 From 9919676836a4e478195c346fe607eeb7c1ec5a67 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 12 Sep 2026 16:22:27 +1000 Subject: [PATCH 2/8] catch four more class reference forms A params.pp is only ever reached by inherits and $class::var, so changing one resolved to no hosts at all. - index inherits, $class::var reads and create_resources() - match a defined type declared part-way along a line --- README.md | 3 ++- internal/impact/index.go | 17 ++++++++++------ internal/impact/resolve_test.go | 20 +++++++++++++++++++ .../repo/site/profiles/manifests/created.pp | 2 ++ .../repo/site/profiles/manifests/inherited.pp | 2 ++ .../site/profiles/manifests/inline/thing.pp | 2 ++ .../repo/site/profiles/manifests/leaf.pp | 8 ++++++++ .../repo/site/profiles/manifests/mid.pp | 2 +- .../repo/site/profiles/manifests/scopevar.pp | 3 +++ 9 files changed, 51 insertions(+), 8 deletions(-) create mode 100644 internal/impact/testdata/repo/site/profiles/manifests/created.pp create mode 100644 internal/impact/testdata/repo/site/profiles/manifests/inherited.pp create mode 100644 internal/impact/testdata/repo/site/profiles/manifests/inline/thing.pp create mode 100644 internal/impact/testdata/repo/site/profiles/manifests/scopevar.pp diff --git a/README.md b/README.md index 32ea494..bfc9f98 100644 --- a/README.md +++ b/README.md @@ -202,7 +202,8 @@ pluginsynced to every agent and is treated the same way. Finding what applies a class means searching two places, not one: 1. `.pp` files under `site/` and `modules/` for `include`, `contain`, - `class { '...': }`, capitalised resource references and defined-type usage. + `inherits`, `class { '...': }`, capitalised resource references, + `create_resources()`, `$class::var` reads and defined-type usage. 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 diff --git a/internal/impact/index.go b/internal/impact/index.go index c60dc75..bb39396 100644 --- a/internal/impact/index.go +++ b/internal/impact/index.go @@ -17,11 +17,16 @@ import ( var manifestRoots = []string{"site", "modules"} var ( - commentRe = regexp.MustCompile(`(?m)#.*$`) - includeRe = regexp.MustCompile(`\b(?:include|contain|require)\s*\(?\s*['"]?(?:::)?([a-z][a-z0-9_]*(?:::[a-z0-9_]+)*)['"]?`) - classParamRe = regexp.MustCompile(`\bclass\s*\{\s*['"](?:::)?([a-z][a-z0-9_]*(?:::[a-z0-9_]+)*)['"]`) - classRefRe = regexp.MustCompile(`\bClass\s*\[\s*['"](?:::)?([a-z][a-z0-9_]*(?:::[a-z0-9_]+)*)['"]`) - defineUseRe = regexp.MustCompile(`(?m)^\s*(?:::)?([a-z][a-z0-9_]*(?:::[a-z0-9_]+)+)\s*\{`) + commentRe = regexp.MustCompile(`(?m)#.*$`) + includeRe = regexp.MustCompile(`\b(?:include|contain|require|inherits)\s*\(?\s*['"]?(?:::)?([a-z][a-z0-9_]*(?:::[a-z0-9_]+)*)['"]?`) + classParamRe = regexp.MustCompile(`\bclass\s*\{\s*['"](?:::)?([a-z][a-z0-9_]*(?:::[a-z0-9_]+)*)['"]`) + classRefRe = regexp.MustCompile(`\bClass\s*\[\s*['"](?:::)?([a-z][a-z0-9_]*(?:::[a-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+)*)`) @@ -153,7 +158,7 @@ func references(body string) []string { seen[name] = true } } - for _, re := range []*regexp.Regexp{includeRe, classParamRe, classRefRe, defineUseRe} { + for _, re := range []*regexp.Regexp{includeRe, classParamRe, classRefRe, defineUseRe, scopeVarRe, createResRe} { for _, m := range re.FindAllStringSubmatch(body, -1) { add(m[1]) } diff --git a/internal/impact/resolve_test.go b/internal/impact/resolve_test.go index 37f61d5..b800af0 100644 --- a/internal/impact/resolve_test.go +++ b/internal/impact/resolve_test.go @@ -123,3 +123,23 @@ func TestParseHieraListsRegexFallback(t *testing.T) { 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) + } + }) + } +} 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/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 index 16e54dc..56ad986 100644 --- a/internal/impact/testdata/repo/site/profiles/manifests/leaf.pp +++ b/internal/impact/testdata/repo/site/profiles/manifests/leaf.pp @@ -1,4 +1,12 @@ 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/mid.pp b/internal/impact/testdata/repo/site/profiles/manifests/mid.pp index 5e54151..3654ce8 100644 --- a/internal/impact/testdata/repo/site/profiles/manifests/mid.pp +++ b/internal/impact/testdata/repo/site/profiles/manifests/mid.pp @@ -1,3 +1,3 @@ -class profiles::mid { +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' +} -- 2.47.3 From 9ff9edefa88cd535d6946cff44df07a54465e548 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 12 Sep 2026 16:26:29 +1000 Subject: [PATCH 3/8] report changed files that reach no host A hieradata file for a role with no members produces an empty host list, which is indistinguishable from a mapping failure. - list files that mapped cleanly but selected nobody in --explain --- README.md | 3 +++ internal/impact/impact.go | 21 ++++++++++++++++++++- internal/impact/impact_test.go | 31 +++++++++++++++++++++++++++++++ internal/impact/render.go | 3 +++ 4 files changed, 57 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index bfc9f98..8d5b79b 100644 --- a/README.md +++ b/README.md @@ -231,6 +231,9 @@ A missed host means an unvalidated catalog, so every ambiguity widens: 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 diff --git a/internal/impact/impact.go b/internal/impact/impact.go index 3856e33..3f79272 100644 --- a/internal/impact/impact.go +++ b/internal/impact/impact.go @@ -1,6 +1,9 @@ package impact -import "sort" +import ( + "slices" + "sort" +) // Reason explains why one changed file put one host in the result. type Reason struct { @@ -37,6 +40,9 @@ type Result struct { // 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. @@ -66,6 +72,8 @@ func (a *Analyzer) Analyze(base, branch string, files []string, limit int) *Resu reasons := map[string][]Reason{} orphans := map[string]bool{} + // hitFiles records which changed files put at least one host in the result. + hitFiles := map[string]bool{} result := &Result{ Base: base, Branch: branch, @@ -90,6 +98,7 @@ func (a *Analyzer) Analyze(base, branch string, files []string, limit int) *Resu 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 } } @@ -103,11 +112,21 @@ func (a *Analyzer) Analyze(base, branch string, files []string, limit int) *Resu 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) } diff --git a/internal/impact/impact_test.go b/internal/impact/impact_test.go index 03e29ef..ae5bf30 100644 --- a/internal/impact/impact_test.go +++ b/internal/impact/impact_test.go @@ -254,3 +254,34 @@ func TestLoadNodesRejectsEmptyEstate(t *testing.T) { t.Fatal("expected an error for an empty estate") } } + +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/render.go b/internal/impact/render.go index 9558c4a..eac1a12 100644 --- a/internal/impact/render.go +++ b/internal/impact/render.go @@ -63,6 +63,9 @@ func RenderExplain(w io.Writer, r *Result) error { 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, "orphan classes (nothing declares them and no hiera_include lists them)", r.OrphanClasses); err != nil { return err } -- 2.47.3 From 2e2562018f509647d2a37e1995f1126f874e1544 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 12 Sep 2026 16:46:51 +1000 Subject: [PATCH 4/8] index every name in an include list include/contain/require take a list of names; parse the argument list so the second and later names are indexed too. --- internal/impact/index.go | 204 +++++++++++++++++- internal/impact/index_test.go | 131 +++++++++++ internal/impact/resolve_test.go | 11 + .../repo/site/profiles/manifests/base.pp | 1 + .../repo/site/profiles/manifests/listed/a.pp | 2 + .../repo/site/profiles/manifests/listed/b.pp | 2 + 6 files changed, 345 insertions(+), 6 deletions(-) create mode 100644 internal/impact/index_test.go create mode 100644 internal/impact/testdata/repo/site/profiles/manifests/listed/a.pp create mode 100644 internal/impact/testdata/repo/site/profiles/manifests/listed/b.pp diff --git a/internal/impact/index.go b/internal/impact/index.go index bb39396..80ed921 100644 --- a/internal/impact/index.go +++ b/internal/impact/index.go @@ -17,10 +17,16 @@ import ( var manifestRoots = []string{"site", "modules"} var ( - commentRe = regexp.MustCompile(`(?m)#.*$`) - includeRe = regexp.MustCompile(`\b(?:include|contain|require|inherits)\s*\(?\s*['"]?(?:::)?([a-z][a-z0-9_]*(?:::[a-z0-9_]+)*)['"]?`) - classParamRe = regexp.MustCompile(`\bclass\s*\{\s*['"](?:::)?([a-z][a-z0-9_]*(?:::[a-z0-9_]+)*)['"]`) - classRefRe = regexp.MustCompile(`\bClass\s*\[\s*['"](?:::)?([a-z][a-z0-9_]*(?:::[a-z0-9_]+)*)['"]`) + commentRe = regexp.MustCompile(`(?m)#.*$`) + // 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_]+)*$`) + typeNameRe = regexp.MustCompile(`^(?:::)?[A-Z][a-zA-Z0-9_]*(?:::[A-Z][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. @@ -158,9 +164,36 @@ func references(body string) []string { seen[name] = true } } - for _, re := range []*regexp.Regexp{includeRe, classParamRe, classRefRe, defineUseRe, scopeVarRe, createResRe} { + + // 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 + } + for _, name := range classNames(args) { + add(name) + } + } + // class { ['a', 'b']: } declares one resource per title. + for _, loc := range classParamRe.FindAllStringIndex(body, -1) { + for _, name := range classNames(resourceTitles(body, loc[1])) { + add(name) + } + } + // Class['a', 'b'] is an ordering reference to each named class. + for _, loc := range classRefRe.FindAllStringIndex(body, -1) { + inner, _, ok := balanced(body, loc[1]-1, '[', ']') + if !ok { + continue + } + for _, name := range classNames(inner) { + add(name) + } + } + for _, re := range []*regexp.Regexp{inheritsRe, defineUseRe, scopeVarRe, createResRe} { for _, m := range re.FindAllStringSubmatch(body, -1) { - add(m[1]) + add(strings.TrimPrefix(m[1], "::")) } } // Resource references (Profiles::Foo::Bar['x']) name a defined type in @@ -177,6 +210,165 @@ func references(body string) []string { return out } +// 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 + } + depth := 0 + for j := i; j < len(s); j++ { + switch c := s[j]; { + case c == '\'' || c == '"': + j = skipString(s, j) - 1 + case c == '(' || c == '[' || c == '{': + depth++ + case c == ')' || c == ']' || c == '}': + if depth == 0 { + return s[i:j], true + } + depth-- + case c == ';' && depth == 0: + return s[i:j], true + case c == '\n' && depth == 0 && !continued(s[i:j]): + return s[i:j], true + } + } + return s[i:], true +} + +// classNames pulls every class name out of a comma-separated argument list, +// descending into array literals and unwrapping quotes. +func classNames(args string) []string { + var out []string + for _, arg := range splitArgs(args) { + arg = strings.TrimSpace(arg) + if arg == "" { + continue + } + if arg[0] == '[' { + if inner, _, ok := balanced(arg, 0, '[', ']'); ok { + out = append(out, classNames(inner)...) + } + continue + } + if len(arg) > 1 && (arg[0] == '\'' || arg[0] == '"') && arg[len(arg)-1] == arg[0] { + arg = arg[1 : len(arg)-1] + } + switch { + case classNameRe.MatchString(arg): + out = append(out, strings.TrimPrefix(arg, "::")) + case typeNameRe.MatchString(arg): + // Class[Apache::Ssl] names the class in type form. + out = append(out, strings.ToLower(strings.TrimPrefix(arg, "::"))) + } + } + return out +} + +// splitArgs splits an argument list on its top-level commas. +func splitArgs(s string) []string { + var out []string + depth, start := 0, 0 + for j := 0; j < len(s); j++ { + switch c := s[j]; c { + case '\'', '"': + j = skipString(s, j) - 1 + case '(', '[', '{': + depth++ + case ')', ']', '}': + depth-- + case ',': + if depth == 0 { + out = append(out, s[start:j]) + start = j + 1 + } + } + } + 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 { + depth := 0 + for j := i; j < len(s); j++ { + switch c := s[j]; c { + case '\'', '"': + j = skipString(s, j) - 1 + case '(', '[', '{': + depth++ + case ')', ']', '}': + if depth == 0 { + return "" + } + depth-- + case ':': + if depth == 0 { + return s[i:j] + } + } + } + return "" +} + +// balanced returns the text between the delimiter at i and its match. +func balanced(s string, i int, opener, closer byte) (string, int, bool) { + depth := 0 + for j := i; j < len(s); j++ { + switch c := s[j]; c { + case '\'', '"': + j = skipString(s, j) - 1 + case opener: + depth++ + case closer: + depth-- + if depth == 0 { + return s[i+1 : j], j + 1, true + } + } + } + return "", len(s), false +} + +// skipString returns the index just past the quoted string starting at i. +func skipString(s string, i int) int { + quote := s[i] + for j := i + 1; j < len(s); j++ { + switch s[j] { + case '\\': + j++ + case quote: + return j + 1 + } + } + return len(s) +} + +// 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, ",") +} + func assetReferences(body string) []string { var out []string for _, m := range assetRe.FindAllStringSubmatch(body, -1) { diff --git a/internal/impact/index_test.go b/internal/impact/index_test.go new file mode 100644 index 0000000..bf680c6 --- /dev/null +++ b/internal/impact/index_test.go @@ -0,0 +1,131 @@ +package impact + +import ( + "reflect" + "testing" +) + +// refs runs a manifest body through the same comment stripping addManifest +// does, then extracts its references. +func refs(body string) []string { + return references(commentRe.ReplaceAllString(body, "")) +} + +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/resolve_test.go b/internal/impact/resolve_test.go index b800af0..4411692 100644 --- a/internal/impact/resolve_test.go +++ b/internal/impact/resolve_test.go @@ -143,3 +143,14 @@ func TestResolveInheritsScopeVarCreateResourcesAndInlineDefine(t *testing.T) { }) } } + +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/testdata/repo/site/profiles/manifests/base.pp b/internal/impact/testdata/repo/site/profiles/manifests/base.pp index bbe4146..56da530 100644 --- a/internal/impact/testdata/repo/site/profiles/manifests/base.pp +++ b/internal/impact/testdata/repo/site/profiles/manifests/base.pp @@ -4,5 +4,6 @@ class profiles::base { contain profiles::contained class { 'profiles::viaclass': } include profiles::cycle::a + include profiles::listed::a, profiles::listed::b profiles::defined::thing { 'x': } } 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 { +} -- 2.47.3 From 6551ab9f133b3bc19c4f4c887c68ebe14c6f6312 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 12 Sep 2026 16:46:51 +1000 Subject: [PATCH 5/8] ship every cmd binary in the release Take the binary list from cmd/ in the Makefile, the RPM packaging and the release pipeline so a new tool ships without a pipeline edit. --- .gitignore | 6 ++---- .woodpecker/release.yaml | 18 ++++++++++-------- AGENTS.md | 6 +++--- Makefile | 31 +++++++++++++++++++------------ README.md | 2 +- packaging/nfpm.yaml | 40 +++++++++++----------------------------- scripts/build-rpm.sh | 31 ++++++++++++++++++++++--------- 7 files changed, 68 insertions(+), 66 deletions(-) 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 e2bc53f..0fa72fa 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -30,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 diff --git a/Makefile b/Makefile index 6a33732..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 catalog-impact +# 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 8d5b79b..ae02b83 100644 --- a/README.md +++ b/README.md @@ -16,7 +16,7 @@ persisted on the server. ```bash dnf install puppet-catalog-tools # from the artifactapi rpm-internal repo # or -make build # -> dist/ +make build # -> dist/bin/ ``` # catalog-diff diff --git a/packaging/nfpm.yaml b/packaging/nfpm.yaml index ba195ef..e9bab8a 100644 --- a/packaging/nfpm.yaml +++ b/packaging/nfpm.yaml @@ -22,44 +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 - - src: dist/catalog-impact - dst: /usr/bin/catalog-impact - 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 - file_info: - mode: 0644 - - - src: dist/completions/catalog-impact.bash - dst: /usr/share/bash-completion/completions/catalog-impact - file_info: - mode: 0644 - - src: dist/completions/_catalog-impact - dst: /usr/share/zsh/site-functions/_catalog-impact - file_info: - mode: 0644 - - src: dist/completions/catalog-impact.fish - dst: /usr/share/fish/vendor_completions.d/catalog-impact.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 382e1cc..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 catalog-impact) 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}" -- 2.47.3 From 1c1df7f9532bb431ca2fca16d7c8b9c8fd059acf Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 12 Sep 2026 17:18:56 +1000 Subject: [PATCH 6/8] strip source quote-aware and resolve dynamic class references - strip # comments with quote-state tracking so a hash in a string no longer truncates the line - skip heredoc bodies in every @(EOT) form so documentation text is not indexed as a class reference - take every literal name out of a selector, following a bare variable through the assignments in its own file - record a class reference that stays unresolved against its file, with any literal prefix, instead of dropping it - read a class name case insensitively in a title or type reference - tag each edge as a declaration or an ordering reference --- internal/impact/index.go | 293 +++------- internal/impact/index_test.go | 19 +- internal/impact/scan.go | 536 ++++++++++++++++++ internal/impact/scan_test.go | 249 ++++++++ .../repo/site/profiles/manifests/base.pp | 1 + .../repo/site/profiles/manifests/dynamic.pp | 3 + 6 files changed, 873 insertions(+), 228 deletions(-) create mode 100644 internal/impact/scan.go create mode 100644 internal/impact/scan_test.go create mode 100644 internal/impact/testdata/repo/site/profiles/manifests/dynamic.pp diff --git a/internal/impact/index.go b/internal/impact/index.go index 80ed921..a00df35 100644 --- a/internal/impact/index.go +++ b/internal/impact/index.go @@ -17,7 +17,6 @@ import ( var manifestRoots = []string{"site", "modules"} var ( - commentRe = regexp.MustCompile(`(?m)#.*$`) // 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)`) @@ -26,8 +25,11 @@ var ( classParamRe = regexp.MustCompile(`\bclass\s*\{`) classRefRe = regexp.MustCompile(`\bClass\s*\[`) classNameRe = regexp.MustCompile(`^(?:::)?[a-z][a-z0-9_]*(?:::[a-z0-9_]+)*$`) - typeNameRe = regexp.MustCompile(`^(?:::)?[A-Z][a-zA-Z0-9_]*(?:::[A-Z][a-zA-Z0-9_]*)*$`) - defineUseRe = regexp.MustCompile(`(?m)(?:^|[^\w:$.])(?:::)?([a-z][a-z0-9_]*(?:::[a-z0-9_]+)+)\s*\{`) + // 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_]*`) @@ -45,8 +47,8 @@ var ( type Index struct { Root string - // includedBy maps a class name to the classes that declare it. - includedBy map[string]map[string]bool + // 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. @@ -57,18 +59,34 @@ type Index struct { 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]bool{}, + 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 { @@ -131,21 +149,35 @@ func (i *Index) addManifest(rel, full string) error { if err != nil { return fmt.Errorf("reading %s: %w", rel, err) } - body := commentRe.ReplaceAllString(string(data), "") + body := stripSource(string(data)) self := map[string]bool{} for _, name := range declared { self[name] = true } - for _, ref := range references(body) { + refs, unresolved := scanReferences(body) + for ref, kind := range refs { if self[ref] { continue } if i.includedBy[ref] == nil { - i.includedBy[ref] = map[string]bool{} + i.includedBy[ref] = map[string]refKind{} } for _, name := range declared { - i.includedBy[ref][name] = true + 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}) } } @@ -155,220 +187,6 @@ func (i *Index) addManifest(rel, full string) error { return nil } -// references extracts every class or defined-type name a manifest body names, -// across all the declaration forms this repo uses. -func references(body string) []string { - seen := map[string]bool{} - add := func(name string) { - if name != "" && name != "class" && name != "define" { - seen[name] = true - } - } - - // 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 - } - for _, name := range classNames(args) { - add(name) - } - } - // class { ['a', 'b']: } declares one resource per title. - for _, loc := range classParamRe.FindAllStringIndex(body, -1) { - for _, name := range classNames(resourceTitles(body, loc[1])) { - add(name) - } - } - // Class['a', 'b'] is an ordering reference to each named class. - for _, loc := range classRefRe.FindAllStringIndex(body, -1) { - inner, _, ok := balanced(body, loc[1]-1, '[', ']') - if !ok { - continue - } - for _, name := range classNames(inner) { - add(name) - } - } - for _, re := range []*regexp.Regexp{inheritsRe, defineUseRe, scopeVarRe, createResRe} { - for _, m := range re.FindAllStringSubmatch(body, -1) { - add(strings.TrimPrefix(m[1], "::")) - } - } - // Resource references (Profiles::Foo::Bar['x']) name a defined type in - // capitalised form. - for _, m := range resourceRefRe.FindAllStringSubmatch(body, -1) { - add(strings.ToLower(m[1])) - } - - out := make([]string, 0, len(seen)) - for name := range seen { - out = append(out, name) - } - sort.Strings(out) - return out -} - -// 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 - } - depth := 0 - for j := i; j < len(s); j++ { - switch c := s[j]; { - case c == '\'' || c == '"': - j = skipString(s, j) - 1 - case c == '(' || c == '[' || c == '{': - depth++ - case c == ')' || c == ']' || c == '}': - if depth == 0 { - return s[i:j], true - } - depth-- - case c == ';' && depth == 0: - return s[i:j], true - case c == '\n' && depth == 0 && !continued(s[i:j]): - return s[i:j], true - } - } - return s[i:], true -} - -// classNames pulls every class name out of a comma-separated argument list, -// descending into array literals and unwrapping quotes. -func classNames(args string) []string { - var out []string - for _, arg := range splitArgs(args) { - arg = strings.TrimSpace(arg) - if arg == "" { - continue - } - if arg[0] == '[' { - if inner, _, ok := balanced(arg, 0, '[', ']'); ok { - out = append(out, classNames(inner)...) - } - continue - } - if len(arg) > 1 && (arg[0] == '\'' || arg[0] == '"') && arg[len(arg)-1] == arg[0] { - arg = arg[1 : len(arg)-1] - } - switch { - case classNameRe.MatchString(arg): - out = append(out, strings.TrimPrefix(arg, "::")) - case typeNameRe.MatchString(arg): - // Class[Apache::Ssl] names the class in type form. - out = append(out, strings.ToLower(strings.TrimPrefix(arg, "::"))) - } - } - return out -} - -// splitArgs splits an argument list on its top-level commas. -func splitArgs(s string) []string { - var out []string - depth, start := 0, 0 - for j := 0; j < len(s); j++ { - switch c := s[j]; c { - case '\'', '"': - j = skipString(s, j) - 1 - case '(', '[', '{': - depth++ - case ')', ']', '}': - depth-- - case ',': - if depth == 0 { - out = append(out, s[start:j]) - start = j + 1 - } - } - } - 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 { - depth := 0 - for j := i; j < len(s); j++ { - switch c := s[j]; c { - case '\'', '"': - j = skipString(s, j) - 1 - case '(', '[', '{': - depth++ - case ')', ']', '}': - if depth == 0 { - return "" - } - depth-- - case ':': - if depth == 0 { - return s[i:j] - } - } - } - return "" -} - -// balanced returns the text between the delimiter at i and its match. -func balanced(s string, i int, opener, closer byte) (string, int, bool) { - depth := 0 - for j := i; j < len(s); j++ { - switch c := s[j]; c { - case '\'', '"': - j = skipString(s, j) - 1 - case opener: - depth++ - case closer: - depth-- - if depth == 0 { - return s[i+1 : j], j + 1, true - } - } - } - return "", len(s), false -} - -// skipString returns the index just past the quoted string starting at i. -func skipString(s string, i int) int { - quote := s[i] - for j := i + 1; j < len(s); j++ { - switch s[j] { - case '\\': - j++ - case quote: - return j + 1 - } - } - return len(s) -} - -// 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, ",") -} - func assetReferences(body string) []string { var out []string for _, m := range assetRe.FindAllStringSubmatch(body, -1) { @@ -431,7 +249,7 @@ func stringList(v any) []string { return out } -// IncludedBy returns the classes that declare the given class, sorted. +// 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] { @@ -441,6 +259,31 @@ func (i *Index) IncludedBy(class string) []string { 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] } diff --git a/internal/impact/index_test.go b/internal/impact/index_test.go index bf680c6..fb693b6 100644 --- a/internal/impact/index_test.go +++ b/internal/impact/index_test.go @@ -2,13 +2,26 @@ package impact import ( "reflect" + "sort" "testing" ) -// refs runs a manifest body through the same comment stripping addManifest -// does, then extracts its references. +// refs runs a manifest body through the same stripping addManifest does, then +// extracts the class names it resolves. func refs(body string) []string { - return references(commentRe.ReplaceAllString(body, "")) + 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) { diff --git a/internal/impact/scan.go b/internal/impact/scan.go new file mode 100644 index 0000000..f1076be --- /dev/null +++ b/internal/impact/scan.go @@ -0,0 +1,536 @@ +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(strings.ToLower(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.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(strings.ToLower(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. +func literalPrefix(s string) string { + i := strings.Index(s, "$") + if i <= 0 { + return "" + } + return 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) { + depth := 0 + for j := 0; j < len(s); j++ { + switch c := s[j]; c { + case '\'', '"': + j = skipString(s, j) - 1 + case '(', '[', '{': + depth++ + case ')', ']', '}': + depth-- + case '?': + if depth != 0 { + continue + } + k := j + 1 + for k < len(s) && isSpace(s[k]) { + k++ + } + if k < len(s) && s[k] == '{' { + inner, _, ok := balanced(s, k, '{', '}') + return inner, ok + } + } + } + return "", false +} + +// cutRocket splits a selector arm on its top-level `=>`. +func cutRocket(s string) (string, string, bool) { + depth := 0 + for j := 0; j < len(s); j++ { + switch c := s[j]; c { + case '\'', '"': + j = skipString(s, j) - 1 + case '(', '[', '{': + depth++ + case ')', ']', '}': + depth-- + case '=': + if depth == 0 && j+1 < len(s) && s[j+1] == '>' { + return s[:j], s[j+2:], true + } + } + } + return "", "", false +} + +// 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++ + } + depth := 0 + for j := i; j < len(s); j++ { + switch c := s[j]; { + case c == '\'' || c == '"': + j = skipString(s, j) - 1 + case c == '(' || c == '[' || c == '{': + depth++ + case c == ')' || c == ']' || c == '}': + if depth == 0 { + return s[i:j] + } + depth-- + case c == ';' && depth == 0: + return s[i:j] + case c == '\n' && depth == 0 && !continued(s[i:j]): + return s[i:j] + } + } + return s[i:] +} + +// 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 + depth, start := 0, 0 + for j := 0; j < len(s); j++ { + switch c := s[j]; c { + case '\'', '"': + j = skipString(s, j) - 1 + case '(', '[', '{': + depth++ + case ')', ']', '}': + depth-- + case ',': + if depth == 0 { + out = append(out, s[start:j]) + start = j + 1 + } + } + } + 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 { + depth := 0 + for j := i; j < len(s); j++ { + switch c := s[j]; c { + case '\'', '"': + j = skipString(s, j) - 1 + case '(', '[', '{': + depth++ + case ')', ']', '}': + if depth == 0 { + return "" + } + depth-- + case ':': + if depth == 0 { + return s[i:j] + } + } + } + return "" +} + +// balanced returns the text between the delimiter at i and its match. +func balanced(s string, i int, opener, closer byte) (string, int, bool) { + depth := 0 + for j := i; j < len(s); j++ { + switch c := s[j]; c { + case '\'', '"': + j = skipString(s, j) - 1 + case opener: + depth++ + case closer: + depth-- + if depth == 0 { + return s[i+1 : j], j + 1, true + } + } + } + return "", len(s), false +} + +// skipString returns the index just past the quoted string starting at i. +func skipString(s string, i int) int { + quote := s[i] + for j := i + 1; j < len(s); j++ { + switch s[j] { + case '\\': + j++ + case quote: + return j + 1 + } + } + return len(s) +} + +// 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..5152917 --- /dev/null +++ b/internal/impact/scan_test.go @@ -0,0 +1,249 @@ +package impact + +import ( + "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" + stripped := stripSource(body) + if len(stripped) != len(body) { + t.Fatalf("stripped length %d, want %d", len(stripped), len(body)) + } + for i := range body { + if body[i] == '\n' && stripped[i] != '\n' { + t.Fatalf("newline at %d was stripped", i) + } + } +} + +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.Fuzz(func(t *testing.T, body string) { + stripped := stripSource(body) + if len(stripped) != len(body) { + t.Fatalf("stripSource changed length: %d -> %d", len(body), len(stripped)) + } + scanReferences(stripped) + }) +} diff --git a/internal/impact/testdata/repo/site/profiles/manifests/base.pp b/internal/impact/testdata/repo/site/profiles/manifests/base.pp index 56da530..72ba24c 100644 --- a/internal/impact/testdata/repo/site/profiles/manifests/base.pp +++ b/internal/impact/testdata/repo/site/profiles/manifests/base.pp @@ -5,5 +5,6 @@ class profiles::base { 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/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']}" +} -- 2.47.3 From c4d3ed24a92e1b5e93d0b2de8d4978af14f710c5 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 12 Sep 2026 17:19:02 +1000 Subject: [PATCH 7/8] report unresolved class references instead of resolving to silence - walk the classes holding an unresolved reference when tracing any class it could have named, so a dropped edge cannot lose hosts - list unresolved references in --explain, in unresolved_references in --json and on stderr, and flag summary.resolution_incomplete - name an ordering reference as such in the --explain chain --- AGENTS.md | 4 + README.md | 15 ++- cmd/catalog-impact/main.go | 8 +- internal/impact/impact.go | 34 +++++- internal/impact/render.go | 27 +++++ internal/impact/resolve.go | 20 +++- internal/impact/unresolved_test.go | 166 +++++++++++++++++++++++++++++ 7 files changed, 265 insertions(+), 9 deletions(-) create mode 100644 internal/impact/unresolved_test.go diff --git a/AGENTS.md b/AGENTS.md index 0fa72fa..c8f02e7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -71,6 +71,10 @@ Adding a binary: create `cmd//`. The Makefile, `scripts/build-rpm.sh`, 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. diff --git a/README.md b/README.md index ae02b83..2dd05de 100644 --- a/README.md +++ b/README.md @@ -203,7 +203,10 @@ 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. + `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 @@ -226,7 +229,15 @@ 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. +- 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`. Paths outside the Puppet modulepath — `.woodpecker/`, `doc/`, `spec/`, `vendor/`, `Gemfile`, `*.md` — cannot reach a catalog and are skipped. diff --git a/cmd/catalog-impact/main.go b/cmd/catalog-impact/main.go index 0805844..d51cfa2 100644 --- a/cmd/catalog-impact/main.go +++ b/cmd/catalog-impact/main.go @@ -34,8 +34,9 @@ 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 -rather than silently returning nothing.` +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 @@ -143,6 +144,9 @@ 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 _, 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) } diff --git a/internal/impact/impact.go b/internal/impact/impact.go index 3f79272..7e243d3 100644 --- a/internal/impact/impact.go +++ b/internal/impact/impact.go @@ -23,6 +23,9 @@ 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, so the answer is a best effort rather than exact. + ResolutionIncomplete bool `json:"resolution_incomplete"` } // Result is the whole analysis, and the --json document. @@ -32,7 +35,10 @@ type Result struct { ChangedFiles []string `json:"changed_files"` Hosts []Host `json:"hosts"` UnmappedFiles []string `json:"unmapped_files"` - Summary Summary `json:"summary"` + // 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"` + Summary Summary `json:"summary"` // IgnoredFiles are changed files outside the modulepath — CI, docs, // specs — which cannot reach a catalog. @@ -72,6 +78,7 @@ func (a *Analyzer) Analyze(base, branch string, files []string, limit int) *Resu 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{ @@ -102,8 +109,11 @@ func (a *Analyzer) Analyze(base, branch string, files []string, limit int) *Resu } } + 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 @@ -132,6 +142,8 @@ func (a *Analyzer) Analyze(base, branch string, files []string, limit int) *Resu } sort.Strings(result.OrphanClasses) + result.UnresolvedRefs = sortUnresolved(dedupeUnresolved(unresolved)) + certnames := make([]string, 0, len(reasons)) for certname := range reasons { certnames = append(certnames, certname) @@ -139,9 +151,10 @@ func (a *Analyzer) Analyze(base, branch string, files []string, limit int) *Resu sort.Strings(certnames) result.Summary = Summary{ - Hosts: len(certnames), - Files: len(result.ChangedFiles), - AllHostsTriggered: len(certnames) > 0 && len(certnames) == len(a.Nodes), + Hosts: len(certnames), + Files: len(result.ChangedFiles), + AllHostsTriggered: len(certnames) > 0 && len(certnames) == len(a.Nodes), + ResolutionIncomplete: len(result.UnresolvedRefs) > 0, } if limit > 0 && len(certnames) > limit { @@ -176,3 +189,16 @@ func dedupeReasons(in []Reason) []Reason { } 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/render.go b/internal/impact/render.go index eac1a12..3bdc7d5 100644 --- a/internal/impact/render.go +++ b/internal/impact/render.go @@ -29,6 +29,9 @@ func RenderJSON(w io.Writer, r *Result) error { if r.UnmappedFiles == nil { r.UnmappedFiles = []string{} } + if r.UnresolvedRefs == nil { + r.UnresolvedRefs = []UnresolvedRef{} + } return enc.Encode(r) } @@ -56,6 +59,12 @@ func RenderExplain(w io.Writer, r *Result) error { 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, "unmapped files (widened to all hosts)", r.UnmappedFiles); err != nil { return err @@ -66,6 +75,10 @@ func RenderExplain(w io.Writer, r *Result) error { 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 } @@ -103,3 +116,17 @@ func section(w io.Writer, title string, items []string) error { } 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 index b460659..aa433ca 100644 --- a/internal/impact/resolve.go +++ b/internal/impact/resolve.go @@ -22,6 +22,8 @@ type classResolver struct { 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 { @@ -71,12 +73,28 @@ func (r *classResolver) resolve(class string) *resolution { continue } seen[parent] = true - via[parent] = via[current] + " <- " + parent + 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 } 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) + } +} -- 2.47.3 From ae948c7f04990de005909984ab4d97f4bdc54fd3 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 12 Sep 2026 17:54:03 +1000 Subject: [PATCH 8/8] track the opening quote and scan interpolations as code A differently quoted substring inside a ${...} interpolation ended the outer string early and truncated the argument list, dropping every name after it. One shared walker now backs every source scan. Lowercase the recovered prefix of an unresolved reference, and every class name as it is added, so a mixed-case interpolated name still widens. Report fact records with no certname instead of dropping them, and assert stripSource keeps newline positions rather than just length. --- README.md | 5 + cmd/catalog-impact/main.go | 7 +- internal/impact/impact.go | 13 +- internal/impact/impact_test.go | 42 +++++- internal/impact/node.go | 27 +++- internal/impact/render.go | 7 + internal/impact/scan.go | 241 ++++++++++++++++++--------------- internal/impact/scan_test.go | 118 +++++++++++++++- 8 files changed, 335 insertions(+), 125 deletions(-) diff --git a/README.md b/README.md index a1e4f69..f70229d 100644 --- a/README.md +++ b/README.md @@ -242,6 +242,11 @@ A missed host means an unvalidated catalog, so every ambiguity widens: `--explain`, in `unresolved_references` in `--json`, on stderr in plain mode, and set `summary.resolution_incomplete`. +A fact record that arrives with no certname belongs to no host and cannot be +folded into the estate table, so it is counted per fact and reported in +`node_warnings`, in `--explain` and on stderr, and sets +`summary.resolution_incomplete` too. + Paths outside the Puppet modulepath — `.woodpecker/`, `doc/`, `spec/`, `vendor/`, `Gemfile`, `*.md` — cannot reach a catalog and are skipped. diff --git a/cmd/catalog-impact/main.go b/cmd/catalog-impact/main.go index d51cfa2..f9302d7 100644 --- a/cmd/catalog-impact/main.go +++ b/cmd/catalog-impact/main.go @@ -119,12 +119,12 @@ func run(ctx context.Context, out io.Writer, opts *options) error { return fmt.Errorf("indexing %s: %w", opts.repo, err) } - nodes, err := impact.LoadNodes(ctx, pdbmux.New(opts.pdb, opts.timeout)) + nodes, nodeWarnings, err := impact.LoadNodes(ctx, pdbmux.New(opts.pdb, opts.timeout)) if err != nil { return err } - analyzer := &impact.Analyzer{Index: idx, Nodes: nodes} + analyzer := &impact.Analyzer{Index: idx, Nodes: nodes, NodeWarnings: nodeWarnings} result := analyzer.Analyze(opts.base, branch, files, opts.limit) switch { @@ -147,6 +147,9 @@ func warn(r *impact.Result) { 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) } diff --git a/internal/impact/impact.go b/internal/impact/impact.go index 7e243d3..95ed0bb 100644 --- a/internal/impact/impact.go +++ b/internal/impact/impact.go @@ -24,7 +24,8 @@ type Summary struct { Files int `json:"files"` AllHostsTriggered bool `json:"all_hosts_triggered"` // ResolutionIncomplete is set when a class reference could not be resolved - // statically, so the answer is a best effort rather than exact. + // 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"` } @@ -38,7 +39,10 @@ type Result struct { // 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"` - Summary Summary `json:"summary"` + // 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. @@ -68,6 +72,8 @@ func (r *Result) Certnames() []string { 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 @@ -85,6 +91,7 @@ func (a *Analyzer) Analyze(base, branch string, files []string, limit int) *Resu Base: base, Branch: branch, ChangedFiles: uniqueSorted(files), + NodeWarnings: a.NodeWarnings, TotalHosts: len(a.Nodes), } @@ -154,7 +161,7 @@ func (a *Analyzer) Analyze(base, branch string, files []string, limit int) *Resu Hosts: len(certnames), Files: len(result.ChangedFiles), AllHostsTriggered: len(certnames) > 0 && len(certnames) == len(a.Nodes), - ResolutionIncomplete: len(result.UnresolvedRefs) > 0, + ResolutionIncomplete: len(result.UnresolvedRefs) > 0 || len(result.NodeWarnings) > 0, } if limit > 0 && len(certnames) > limit { diff --git a/internal/impact/impact_test.go b/internal/impact/impact_test.go index ae5bf30..33103b9 100644 --- a/internal/impact/impact_test.go +++ b/internal/impact/impact_test.go @@ -231,10 +231,13 @@ func TestLoadNodesFoldsStructuredOS(t *testing.T) { {Certname: "a.example.net", Name: "enc_role_tier1", Value: "base"}, }} - nodes, err := LoadNodes(context.Background(), src) + 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) } @@ -250,11 +253,46 @@ func TestLoadNodesFoldsStructuredOS(t *testing.T) { } func TestLoadNodesRejectsEmptyEstate(t *testing.T) { - if _, err := LoadNodes(context.Background(), &stubFacts{}); err == nil { + 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: []pdbmux.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. diff --git a/internal/impact/node.go b/internal/impact/node.go index 03a6399..17922a9 100644 --- a/internal/impact/node.go +++ b/internal/impact/node.go @@ -58,16 +58,19 @@ type FactSource interface { // 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. -func LoadNodes(ctx context.Context, src FactSource) ([]Node, error) { +// 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, err + 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] @@ -84,9 +87,23 @@ func LoadNodes(ctx context.Context, src FactSource) ([]Node, error) { } sort.Slice(nodes, func(i, j int) bool { return nodes[i].Certname < nodes[j].Certname }) if len(nodes) == 0 { - return nil, fmt.Errorf("pdbmux returned no nodes") + return nil, anonymousWarnings(anonymous), fmt.Errorf("pdbmux returned no nodes") } - return nodes, nil + 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) { diff --git a/internal/impact/render.go b/internal/impact/render.go index 3bdc7d5..44aed7e 100644 --- a/internal/impact/render.go +++ b/internal/impact/render.go @@ -32,6 +32,9 @@ func RenderJSON(w io.Writer, r *Result) error { if r.UnresolvedRefs == nil { r.UnresolvedRefs = []UnresolvedRef{} } + if r.NodeWarnings == nil { + r.NodeWarnings = []string{} + } return enc.Encode(r) } @@ -66,6 +69,10 @@ func RenderExplain(w io.Writer, r *Result) error { } } + 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 } diff --git a/internal/impact/scan.go b/internal/impact/scan.go index f1076be..b7a763e 100644 --- a/internal/impact/scan.go +++ b/internal/impact/scan.go @@ -82,7 +82,7 @@ func scanReferences(body string) (map[string]refKind, []UnresolvedRef) { s.add(m[1], refOrdering) } for _, m := range resourceRefRe.FindAllStringSubmatch(body, -1) { - s.add(strings.ToLower(m[1]), refOrdering) + s.add(m[1], refOrdering) } return s.refs, dedupeUnresolved(s.unresolved) @@ -94,7 +94,7 @@ func callName(match string) string { } func (s *refScan) add(name string, kind refKind) { - name = strings.TrimPrefix(name, "::") + name = strings.ToLower(strings.TrimPrefix(name, "::")) if name == "" || reserved[name] { return } @@ -162,7 +162,7 @@ func (s *refScan) literal(form string, kind refKind, arg string) { s.add(lit, kind) case anyCaseNameRe.MatchString(lit): // Class[Apache::Ssl] names the class in type form. - s.add(strings.ToLower(lit), kind) + s.add(lit, kind) case quoted: // "profile::role_${role}" is a name only the compiler can finish. s.unresolvedAdd(form, arg, literalPrefix(lit)) @@ -211,13 +211,15 @@ func dedupeUnresolved(in []UnresolvedRef) []UnresolvedRef { } // literalPrefix returns the part of an interpolated name that is fixed text, -// so the report can say what family of classes was missed. +// 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 s[:i] + return strings.ToLower(s[:i]) } // collapse folds whitespace and clips an expression to a length that reads in @@ -239,50 +241,34 @@ func unquote(s string) (string, bool) { // selectorArms returns the arm text of a ` ? { ... }` selector. func selectorArms(s string) (string, bool) { - depth := 0 - for j := 0; j < len(s); j++ { - switch c := s[j]; c { - case '\'', '"': - j = skipString(s, j) - 1 - case '(', '[', '{': - depth++ - case ')', ']', '}': - depth-- - case '?': - if depth != 0 { - continue - } - k := j + 1 - for k < len(s) && isSpace(s[k]) { - k++ - } - if k < len(s) && s[k] == '{' { - inner, _, ok := balanced(s, k, '{', '}') - return inner, ok - } + var arms string + var ok bool + walkSource(s, 0, anyDelim, func(j int, c byte, depth int) bool { + if c != '?' || depth != 0 { + return false } - } - 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) { - depth := 0 - for j := 0; j < len(s); j++ { - switch c := s[j]; c { - case '\'', '"': - j = skipString(s, j) - 1 - case '(', '[', '{': - depth++ - case ')', ']', '}': - depth-- - case '=': - if depth == 0 && j+1 < len(s) && s[j+1] == '>' { - return s[:j], s[j+2:], true - } - } + 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 "", "", false + return s[:end], s[end+2:], true } // assignments maps a variable assigned in this file to the expression it was @@ -305,25 +291,19 @@ func statementExpr(s string, i int) string { for i < len(s) && (s[i] == ' ' || s[i] == '\t') { i++ } - depth := 0 - for j := i; j < len(s); j++ { - switch c := s[j]; { - case c == '\'' || c == '"': - j = skipString(s, j) - 1 - case c == '(' || c == '[' || c == '{': - depth++ - case c == ')' || c == ']' || c == '}': - if depth == 0 { - return s[i:j] - } - depth-- - case c == ';' && depth == 0: - return s[i:j] - case c == '\n' && depth == 0 && !continued(s[i:j]): - return s[i:j] + end := walkSource(s, i, anyDelim, func(j int, c byte, depth int) bool { + if depth != 0 { + return false } - } - return s[i:] + 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 @@ -452,82 +432,129 @@ func callArguments(s string, i int) (string, bool) { // splitArgs splits an argument list on its top-level commas. func splitArgs(s string) []string { var out []string - depth, start := 0, 0 - for j := 0; j < len(s); j++ { - switch c := s[j]; c { - case '\'', '"': - j = skipString(s, j) - 1 - case '(', '[', '{': - depth++ - case ')', ']', '}': - depth-- - case ',': - if depth == 0 { - out = append(out, s[start:j]) - start = j + 1 - } + 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 { - depth := 0 - for j := i; j < len(s); j++ { - switch c := s[j]; c { - case '\'', '"': - j = skipString(s, j) - 1 - case '(', '[', '{': - depth++ + var out string + walkSource(s, i, anyDelim, func(j int, c byte, depth int) bool { + if depth != 0 { + return false + } + switch c { case ')', ']', '}': - if depth == 0 { - return "" - } - depth-- + return true case ':': - if depth == 0 { - return s[i:j] - } + out = s[i:j] + return true } - } - return "" + return false + }) + return out } -// balanced returns the text between the delimiter at i and its match. +// 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++ { - switch c := s[j]; c { - case '\'', '"': + c := s[j] + if c == '\'' || c == '"' { j = skipString(s, j) - 1 - case opener: - depth++ - case closer: - depth-- - if depth == 0 { - return s[i+1 : j], j + 1, true - } + continue } + if stop(j, c, depth) { + return j + } + depth += d(c) } - return "", len(s), false + return len(s) } -// skipString returns the index just past the quoted string starting at i. +// 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 s[j] { - case '\\': + switch c := s[j]; { + case c == '\\': j++ - case quote: + 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 { diff --git a/internal/impact/scan_test.go b/internal/impact/scan_test.go index 5152917..f0944fa 100644 --- a/internal/impact/scan_test.go +++ b/internal/impact/scan_test.go @@ -1,6 +1,7 @@ package impact import ( + "fmt" "reflect" "testing" ) @@ -222,15 +223,119 @@ func TestScanSurvivesMalformedInput(t *testing.T) { 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" - stripped := stripSource(body) + 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) { - t.Fatalf("stripped length %d, want %d", 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' { - t.Fatalf("newline at %d was stripped", i) + 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) { @@ -239,10 +344,11 @@ func FuzzScanReferences(f *testing.F) { 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 len(stripped) != len(body) { - t.Fatalf("stripSource changed length: %d -> %d", len(body), len(stripped)) + if err := sameNewlines(body, stripped); err != nil { + t.Fatal(err) } scanReferences(stripped) }) -- 2.47.3