diff --git a/.coverage-baseline b/.coverage-baseline index 3c2d8494..add29667 100644 --- a/.coverage-baseline +++ b/.coverage-baseline @@ -1 +1 @@ -75.3 +75.4 diff --git a/Taskfile-build.yml b/Taskfile-build.yml index 9e777210..8c45281f 100644 --- a/Taskfile-build.yml +++ b/Taskfile-build.yml @@ -132,6 +132,10 @@ tasks: - fmt - vet - setup-envtest + # cmd's chart-args test runs `helm template`, which evaluates every template — + # including rbac.yaml, which reads the gitignored chart/config/role.yaml this + # syncs. Without it a fresh checkout fails the unit suite. + - helm-sync cmds: - | export KUBEBUILDER_ASSETS="$({{.ENVTEST}} use {{.ENVTEST_K8S_VERSION}} --bin-dir "$(pwd)/bin" -p path)" diff --git a/charts/gitops-reverser/templates/deployment.yaml b/charts/gitops-reverser/templates/deployment.yaml index b54acffc..66a9a755 100644 --- a/charts/gitops-reverser/templates/deployment.yaml +++ b/charts/gitops-reverser/templates/deployment.yaml @@ -68,9 +68,12 @@ spec: {{- end }} - --redis-addr={{ .Values.queue.redis.addr }} {{- with .Values.queue.redis.auth.username }} - - --redis-username={{ . | quote }} + - {{ printf "--redis-username=%s" . | quote }} {{- end }} - --redis-db={{ .Values.queue.redis.db }} + {{- with .Values.queue.redis.keyPrefix }} + - {{ printf "--redis-key-prefix=%s" . | quote }} + {{- end }} {{- if not .Values.queue.redis.tls.enabled }} - --redis-insecure {{- end }} diff --git a/charts/gitops-reverser/values.yaml b/charts/gitops-reverser/values.yaml index 7fecb653..36be158c 100644 --- a/charts/gitops-reverser/values.yaml +++ b/charts/gitops-reverser/values.yaml @@ -184,6 +184,12 @@ queue: # Optional Redis username for ACL-based auth (Redis 6+). Not sensitive; stays as a plain value. username: "" db: 0 + # Root namespace for every key this release writes (cursors, attribution facts, command author + # records). Give each reverser its own prefix to share one Redis/Valkey between more than the 16 + # logical databases `db` can separate. A Valkey ACL can enforce it (~:*) rather than trust + # it. Changing it orphans the keys written under the previous prefix: cursors then cold-replay + # once, which is safe. Allowed characters: [A-Za-z0-9], '-', '_', '.' and ':'. + keyPrefix: "gitops-reverser" tls: enabled: false diff --git a/cmd/main.go b/cmd/main.go index 1d6c01e4..6a8e4511 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -194,9 +194,12 @@ func main() { AuthValue: cfg.redisPassword, DB: cfg.redisDB, TLSEnabled: !cfg.redisInsecure, + KeyPrefix: cfg.redisKeyPrefix, }) fatalIfErr(err, "unable to build Redis cursor store") watchMgr.WatchCursorStore = redisStore + setupLog.Info("Redis keyspace", "addr", cfg.redisAddr, "db", cfg.redisDB, + "keyPrefix", redisStore.KeyPrefix()) redisGate = newRedisReadinessGate(redisStore) fatalIfErr(mgr.Add(redisGate), "unable to add redis readiness gate") @@ -346,6 +349,7 @@ type appConfig struct { redisUsername string redisPassword string redisDB int + redisKeyPrefix string redisInsecure bool authorAttribution bool attributionFactTTL time.Duration @@ -433,6 +437,12 @@ func parseFlagsWithArgs(fs *flag.FlagSet, args []string) (appConfig, error) { "Redis password. Prefer setting via REDIS_PASSWORD env var from a Secret.", ) fs.IntVar(&cfg.redisDB, "redis-db", 0, "Redis database index (default 0).") + fs.StringVar(&cfg.redisKeyPrefix, "redis-key-prefix", queue.DefaultKeyPrefix, + "Root namespace for every key this operator writes to Redis/Valkey (cursors, attribution "+ + "facts, command author records). Give each reverser its own prefix to share one "+ + "Redis/Valkey between more than the 16 logical databases --redis-db can separate. "+ + "Allowed characters are [A-Za-z0-9], '-', '_', '.' and ':'; a Valkey ACL can enforce "+ + "the prefix (~:*) rather than trust it.") fs.BoolVar(&cfg.redisInsecure, "redis-insecure", false, "Connect to Redis over plain TCP instead of TLS (default false; TLS). Redis carries each "+ "GitTarget's watch cursors and, when attribution is on, the audit facts — prefer TLS. Set "+ @@ -476,6 +486,13 @@ func parseFlagsWithArgs(fs *flag.FlagSet, args []string) (appConfig, error) { return appConfig{}, err } cfg.redisAddr = strings.TrimSpace(cfg.redisAddr) + // Validated even when redis-addr is empty: a typo'd prefix is a configuration error + // whether or not this run happens to open the connection it names. + normalizedKeyPrefix, err := queue.ValidateKeyPrefix(cfg.redisKeyPrefix) + if err != nil { + return appConfig{}, err + } + cfg.redisKeyPrefix = normalizedKeyPrefix if err := validateAuditConfig(cfg); err != nil { return appConfig{}, err } diff --git a/cmd/main_audit_server_test.go b/cmd/main_audit_server_test.go index c6e48019..b05c6922 100644 --- a/cmd/main_audit_server_test.go +++ b/cmd/main_audit_server_test.go @@ -21,6 +21,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/ConfigButler/gitops-reverser/internal/queue" ) func TestParseFlagsWithArgs_Defaults(t *testing.T) { @@ -97,6 +99,7 @@ func TestParseFlagsWithArgs_CustomAuditValues(t *testing.T) { "--redis-username=user", "--redis-password=pass", "--redis-db=2", + "--redis-key-prefix=cell-a:tenant-7:", "--redis-insecure", "--author-attribution-ttl=20m", "--author-attribution-grace=750ms", @@ -119,11 +122,37 @@ func TestParseFlagsWithArgs_CustomAuditValues(t *testing.T) { assert.Equal(t, "user", cfg.redisUsername) assert.Equal(t, "pass", cfg.redisPassword) assert.Equal(t, 2, cfg.redisDB) + // The trailing colon is normalized away, so "tenant-7:" and "tenant-7" name one keyspace. + assert.Equal(t, "cell-a:tenant-7", cfg.redisKeyPrefix) assert.True(t, cfg.redisInsecure) assert.Equal(t, 20*time.Minute, cfg.attributionFactTTL) assert.Equal(t, 750*time.Millisecond, cfg.attributionGrace) } +func TestParseFlagsWithArgs_RedisKeyPrefixDefault(t *testing.T) { + fs := flag.NewFlagSet("test-redis-key-prefix-default", flag.ContinueOnError) + cfg, err := parseFlagsWithArgs(fs, []string{"--author-attribution=false"}) + require.NoError(t, err) + // An upgrade that sets nothing must keep writing the keys the previous release wrote. + assert.Equal(t, queue.DefaultKeyPrefix, cfg.redisKeyPrefix) +} + +func TestParseFlagsWithArgs_RedisKeyPrefixRejectsGlob(t *testing.T) { + fs := flag.NewFlagSet("test-redis-key-prefix-glob", flag.ContinueOnError) + // A glob metacharacter would corrupt the attribution fact-index SCAN pattern. + _, err := parseFlagsWithArgs(fs, []string{"--redis-key-prefix=tenant-*"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "redis-key-prefix") +} + +func TestParseFlagsWithArgs_RedisKeyPrefixRejectsEmpty(t *testing.T) { + fs := flag.NewFlagSet("test-redis-key-prefix-empty", flag.ContinueOnError) + // Rejected even without Redis: an empty prefix silently un-namespaces the keyspace. + _, err := parseFlagsWithArgs(fs, []string{"--author-attribution=false", "--redis-addr=", "--redis-key-prefix="}) + require.Error(t, err) + assert.Contains(t, err.Error(), "non-empty") +} + func TestParseFlagsWithArgs_RedisAddrRequiredWhenAttributionEnabled(t *testing.T) { fs := flag.NewFlagSet("test-redis-required", flag.ContinueOnError) // Default --author-attribution=true, so an empty redis-addr must be rejected. diff --git a/cmd/main_chart_args_test.go b/cmd/main_chart_args_test.go new file mode 100644 index 00000000..a4fc8867 --- /dev/null +++ b/cmd/main_chart_args_test.go @@ -0,0 +1,108 @@ +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "flag" + "os" + "os/exec" + "testing" + + "github.com/stretchr/testify/require" + appsv1 "k8s.io/api/apps/v1" + "sigs.k8s.io/yaml" +) + +// chartInputs are read by the test process itself so `go test` records them as cache +// inputs. helm reads the chart in a subprocess, which the test log cannot see, so without +// these reads a chart-only edit would replay a cached PASS. +var chartInputs = []string{ + "../charts/gitops-reverser/values.yaml", + "../charts/gitops-reverser/templates/deployment.yaml", +} + +// renderManagerArgs renders the chart's Deployment and returns the manager container's +// argv, exactly as the kubelet would hand it to the binary. +func renderManagerArgs(t *testing.T, setValues ...string) []string { + t.Helper() + + for _, path := range chartInputs { + _, err := os.ReadFile(path) + require.NoError(t, err) + } + + args := []string{ + "template", "gitops-reverser", "../charts/gitops-reverser", + "--show-only", "templates/deployment.yaml", + } + for _, sv := range setValues { + args = append(args, "--set", sv) + } + out, err := exec.Command("helm", args...).CombinedOutput() + require.NoErrorf(t, err, "helm template failed: %s", out) + + var deploy appsv1.Deployment + require.NoError(t, yaml.Unmarshal(out, &deploy)) + + for _, c := range deploy.Spec.Template.Spec.Containers { + if c.Name == "manager" { + return c.Args + } + } + t.Fatalf("no manager container in rendered Deployment:\n%s", out) + return nil +} + +// The chart writes the manager's argv and the binary parses it; nothing else enforces that +// the two agree. A value interpolated as a bare `{{ . | quote }}` inside a YAML plain scalar +// keeps its double quotes as literal characters, so the flag arrives with them attached and +// a validating flag rejects it — the manager then CrashLoopBackOffs on a chart that renders, +// lints and packages cleanly. Parse what the chart actually emits. +func TestChartRendersArgsTheBinaryAccepts(t *testing.T) { + tests := map[string]struct { + setValues []string + wantKeyPrefix string + wantRedisAddr string + wantRedisDBSet int + wantUsername string + }{ + "chart defaults": { + wantKeyPrefix: "gitops-reverser", + }, + "nested tenant prefix": { + setValues: []string{"queue.redis.keyPrefix=cell-a:tenant-7"}, + wantKeyPrefix: "cell-a:tenant-7", + }, + "redis enabled with auth": { + setValues: []string{ + "queue.redis.addr=redis.example.com:6379", + "queue.redis.auth.username=reverser", + "queue.redis.db=3", + "queue.redis.keyPrefix=tenant-7", + }, + wantKeyPrefix: "tenant-7", + wantRedisAddr: "redis.example.com:6379", + wantRedisDBSet: 3, + wantUsername: "reverser", + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + argv := renderManagerArgs(t, tc.setValues...) + + cfg, err := parseFlagsWithArgs(flag.NewFlagSet("manager", flag.ContinueOnError), argv) + require.NoError(t, err, "the binary must accept the argv the chart renders") + + require.Equal(t, tc.wantKeyPrefix, cfg.redisKeyPrefix) + if tc.wantRedisAddr != "" { + require.Equal(t, tc.wantRedisAddr, cfg.redisAddr) + require.Equal(t, tc.wantRedisDBSet, cfg.redisDB) + } + if tc.wantUsername != "" { + // A quoted-value render would reach Redis as `"reverser"` and fail ACL auth. + require.Equal(t, tc.wantUsername, cfg.redisUsername) + } + }) + } +} diff --git a/cmd/manifest-analyzer/main.go b/cmd/manifest-analyzer/main.go index 71d64399..c44c9754 100644 --- a/cmd/manifest-analyzer/main.go +++ b/cmd/manifest-analyzer/main.go @@ -9,27 +9,29 @@ // Usage: // // manifest-analyzer [flags] -// manifest-analyzer --mode repo-walker [flags] +// manifest-analyzer --mode scan-repo [flags] // manifest-analyzer --mode discovery [flags] // -// --mode analyze|scan|repo-walker|discovery what to produce (default analyze) -// analyze: the structural report (files, GVK inventory) -// scan: the adoption dry-run (acceptance + plan), the -// shared scan-mode pipeline with no flush -// repo-walker: the F8 whole-repo onboarding scan — walk -// every folder, classify candidate GitTargets. -// Report-only: --policy is not applied (the -// repo-level refuse gate is deferred) -// discovery: raw Kubernetes API discovery dump +// --mode analyze|scan-folder|scan-repo|discovery what to produce (default analyze) +// analyze: the structural report (files, GVK inventory) +// scan-folder: may THIS folder become a GitTarget? The +// adoption dry-run (acceptance + plan), the +// shared scan pipeline with no flush +// scan-repo: which folders under this repo root could +// become GitTargets? Classifies every +// candidate. Report-only: --policy is not +// applied (the repo-level refuse gate is +// deferred) +// discovery: raw Kubernetes API discovery dump // --format text|json output format (default text) // --policy report|refuse // report: always exit 0 (analysis only) // refuse: exit 1 when the folder would be refused -// (analyze: any acceptance issue; scan: not accepted) +// (analyze: any acceptance issue; scan-folder: not accepted) // // The tool is structure-only and needs no cluster: it reports duplicate // identities, KRM vs. non-KRM classification, multi-document files, and the -// inventory of every GVK found. Scan mode additionally applies the non-API KRM +// inventory of every GVK found. scan-folder additionally applies the non-API KRM // allowlist (kustomization.yaml is retained, not flagged), runs the full adoption // acceptance gate, and renders the plan — which is empty here because no cluster // state is available to compare against. @@ -49,6 +51,7 @@ import ( "k8s.io/client-go/tools/clientcmd" "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" + publicanalyzer "github.com/ConfigButler/gitops-reverser/pkg/manifestanalyzer" ) // Process exit codes. @@ -87,9 +90,9 @@ func runWithDiscoveryClientFactory( ) int { fs := flag.NewFlagSet("manifest-analyzer", flag.ContinueOnError) fs.SetOutput(stderr) - mode := fs.String("mode", "analyze", "what to produce: analyze|scan|repo-walker|discovery") + mode := fs.String("mode", "analyze", "what to produce: analyze|scan-folder|scan-repo|discovery") format := fs.String("format", "text", "output format: text|json") - policy := fs.String("policy", "report", "adoption policy: report|refuse (repo-walker is report-only)") + policy := fs.String("policy", "report", "adoption policy: report|refuse (scan-repo is report-only)") kubeconfig := fs.String( "kubeconfig", "", @@ -98,7 +101,7 @@ func runWithDiscoveryClientFactory( contextName := fs.String("context", "", "kubeconfig context for --mode discovery") fs.Usage = func() { fmt.Fprintln(stderr, "usage: manifest-analyzer [flags] ") - fmt.Fprintln(stderr, " manifest-analyzer --mode repo-walker [flags] ") + fmt.Fprintln(stderr, " manifest-analyzer --mode scan-repo [flags] ") fmt.Fprintln(stderr, " manifest-analyzer --mode discovery [flags]") fs.PrintDefaults() } @@ -129,8 +132,8 @@ func runWithDiscoveryClientFactory( // to stderr. Splitting it out of run keeps the top-level dispatch simple. func validChoices(mode, format, policy string, stderr io.Writer) bool { switch { - case mode != "analyze" && mode != "scan" && mode != "repo-walker" && mode != "discovery": - fmt.Fprintf(stderr, "error: unknown mode %q (want analyze|scan|repo-walker|discovery)\n", mode) + case mode != "analyze" && mode != "scan-folder" && mode != "scan-repo" && mode != "discovery": + fmt.Fprintf(stderr, "error: unknown mode %q (want analyze|scan-folder|scan-repo|discovery)\n", mode) case format != "text" && format != "json": fmt.Fprintf(stderr, "error: unknown format %q (want text|json)\n", format) case policy != "report" && policy != "refuse": @@ -144,10 +147,10 @@ func validChoices(mode, format, policy string, stderr io.Writer) bool { // runDirMode dispatches the directory-argument modes (everything but discovery). func runDirMode(mode, dir, format, policy string, stdout, stderr io.Writer) int { switch mode { - case "scan": - return runScan(dir, format, policy, stdout, stderr) - case "repo-walker": - return runRepoWalk(dir, format, stdout, stderr) + case "scan-folder": + return runScanFolder(dir, format, policy, stdout, stderr) + case "scan-repo": + return runScanRepo(dir, format, stdout, stderr) default: return runAnalyze(dir, format, policy, stdout, stderr) } @@ -233,11 +236,28 @@ func failedGroupVersions(failed map[schema.GroupVersion]error) map[string]string return out } -// runScan runs the adoption dry-run (the shared scan-mode pipeline) and applies the +// runScanFolder runs the adoption dry-run (the shared scan pipeline) and applies the // refuse policy over the acceptance decision. It is structure-only: no cluster // state, so the plan is empty, but the acceptance gate is the full one — it applies // the non-API KRM allowlist and the impure-managed-file / mixed-file refusals. -func runScan(dir, format, policy string, stdout, stderr io.Writer) int { +// +// --format json goes through pkg/manifestanalyzer, so the CLI's machine-readable output +// and the published Go contract are the same document and cannot drift. Text output stays +// on the internal renderer, which can show the plan the public report deliberately omits. +func runScanFolder(dir, format, policy string, stdout, stderr io.Writer) int { + if format == "json" { + report, err := publicanalyzer.ScanFolder(context.Background(), dir) + if err != nil { + fmt.Fprintf(stderr, "error: %v\n", err) + return exitUsage + } + if err := report.WriteJSON(stdout); err != nil { + fmt.Fprintf(stderr, "error: %v\n", err) + return exitUsage + } + return scanExitCode(policy, report.Accepted) + } + scanPolicy := manifestanalyzer.ScanPolicy{ Acceptance: manifestanalyzer.AcceptancePolicy{Allowlist: manifestanalyzer.DefaultAllowlist()}, } @@ -246,41 +266,41 @@ func runScan(dir, format, policy string, stdout, stderr io.Writer) int { fmt.Fprintf(stderr, "error: %v\n", err) return exitUsage } + manifestanalyzer.RenderScanText(stdout, result) + return scanExitCode(policy, result.Acceptance.Accepted) +} - if format == "json" { - if err := manifestanalyzer.RenderScanJSON(stdout, result); err != nil { - fmt.Fprintf(stderr, "error: %v\n", err) - return exitUsage - } - } else { - manifestanalyzer.RenderScanText(stdout, result) - } - - if policy == "refuse" && !result.Acceptance.Accepted { +func scanExitCode(policy string, accepted bool) int { + if policy == "refuse" && !accepted { return exitRefused } return exitOK } -// runRepoWalk runs the F8 whole-repo onboarding scan: walk every folder, enumerate +// runScanRepo runs the F8 whole-repo onboarding scan: walk every folder, enumerate // candidate GitTarget subtrees, classify each one's layout and acceptance, and emit the // report. It is read-only and needs no cluster. Exit codes stay simple for this cut // (exitOK, or exitUsage on an I/O error); the repo-level --policy refuse gate is // deferred per the design doc. -func runRepoWalk(root, format string, stdout, stderr io.Writer) int { - rep, err := manifestanalyzer.WalkRepo(context.Background(), root) - if err != nil { - fmt.Fprintf(stderr, "error: %v\n", err) - return exitUsage - } - +func runScanRepo(root, format string, stdout, stderr io.Writer) int { if format == "json" { - if err := manifestanalyzer.RenderRepoJSON(stdout, rep); err != nil { + report, err := publicanalyzer.ScanRepo(context.Background(), root) + if err != nil { fmt.Fprintf(stderr, "error: %v\n", err) return exitUsage } - } else { - manifestanalyzer.RenderRepoText(stdout, rep) + if err := report.WriteJSON(stdout); err != nil { + fmt.Fprintf(stderr, "error: %v\n", err) + return exitUsage + } + return exitOK + } + + rep, err := manifestanalyzer.ScanRepo(context.Background(), root) + if err != nil { + fmt.Fprintf(stderr, "error: %v\n", err) + return exitUsage } + manifestanalyzer.RenderRepoText(stdout, rep) return exitOK } diff --git a/cmd/manifest-analyzer/main_test.go b/cmd/manifest-analyzer/main_test.go index c1a5d308..868b939c 100644 --- a/cmd/manifest-analyzer/main_test.go +++ b/cmd/manifest-analyzer/main_test.go @@ -99,7 +99,7 @@ func TestRun_ScanTextRefuses(t *testing.T) { // fixtureDir contains a non-KRM values.yaml, so scan mode refuses under --policy // refuse and prints the acceptance verdict and the (empty, structure-only) plan. var out, errBuf bytes.Buffer - if code := run([]string{"--mode", "scan", "--policy", "refuse", fixtureDir(t)}, &out, &errBuf); code != 1 { + if code := run([]string{"--mode", "scan-folder", "--policy", "refuse", fixtureDir(t)}, &out, &errBuf); code != 1 { t.Fatalf("scan refuse with issues: exit = %d, want 1 (stderr=%s)", code, errBuf.String()) } for _, want := range []string{"Acceptance: REFUSED", "Plan: no changes"} { @@ -114,7 +114,7 @@ func TestRun_ScanCleanPasses(t *testing.T) { write(t, clean, "deploy.yaml", deployYAML) var out, errBuf bytes.Buffer - if code := run([]string{"--mode", "scan", "--policy", "refuse", clean}, &out, &errBuf); code != 0 { + if code := run([]string{"--mode", "scan-folder", "--policy", "refuse", clean}, &out, &errBuf); code != 0 { t.Fatalf("scan refuse on clean tree: exit = %d, want 0 (stderr=%s)", code, errBuf.String()) } if !strings.Contains(out.String(), "Acceptance: accepted") { @@ -124,7 +124,7 @@ func TestRun_ScanCleanPasses(t *testing.T) { func TestRun_ScanJSON(t *testing.T) { var out, errBuf bytes.Buffer - code := run([]string{"--mode", "scan", "--format", "json", fixtureDir(t)}, &out, &errBuf) + code := run([]string{"--mode", "scan-folder", "--format", "json", fixtureDir(t)}, &out, &errBuf) if code != 0 { t.Fatalf("exit = %d, stderr=%s", code, errBuf.String()) } @@ -137,10 +137,10 @@ func TestRun_ScanJSON(t *testing.T) { } } -// repoWalkFixture builds a tiny two-folder repo: a plain KRM app folder and a kustomize -// overlay reaching an out-of-subtree base, so repo-walker reports both an accepted plain +// scanRepoFixture builds a tiny two-folder repo: a plain KRM app folder and a kustomize +// overlay reaching an out-of-subtree base, so scan-repo reports both an accepted plain // candidate and a refused kustomize-overlay one. -func repoWalkFixture(t *testing.T) string { +func scanRepoFixture(t *testing.T) string { t.Helper() root := t.TempDir() write(t, mkdir(t, root, "plain"), "deploy.yaml", deployYAML) @@ -162,21 +162,21 @@ func mkdir(t *testing.T, dir, sub string) string { return full } -func TestRun_RepoWalkText(t *testing.T) { +func TestRun_ScanRepoText(t *testing.T) { var out, errBuf bytes.Buffer - if code := run([]string{"--mode", "repo-walker", repoWalkFixture(t)}, &out, &errBuf); code != 0 { + if code := run([]string{"--mode", "scan-repo", scanRepoFixture(t)}, &out, &errBuf); code != 0 { t.Fatalf("exit = %d, want 0 (stderr=%s)", code, errBuf.String()) } for _, want := range []string{"candidates:", "kustomize-overlay", "overlay-fan-out-needs-f2"} { if !strings.Contains(out.String(), want) { - t.Errorf("repo-walker text missing %q:\n%s", want, out.String()) + t.Errorf("scan-repo text missing %q:\n%s", want, out.String()) } } } -func TestRun_RepoWalkJSON(t *testing.T) { +func TestRun_ScanRepoJSON(t *testing.T) { var out, errBuf bytes.Buffer - args := []string{"--mode", "repo-walker", "--format", "json", repoWalkFixture(t)} + args := []string{"--mode", "scan-repo", "--format", "json", scanRepoFixture(t)} if code := run(args, &out, &errBuf); code != 0 { t.Fatalf("exit = %d, want 0 (stderr=%s)", code, errBuf.String()) } @@ -188,13 +188,13 @@ func TestRun_RepoWalkJSON(t *testing.T) { Summary map[string]any `json:"summary"` } if err := json.Unmarshal(out.Bytes(), &parsed); err != nil { - t.Fatalf("repo-walker JSON invalid: %v\n%s", err, out.String()) + t.Fatalf("scan-repo JSON invalid: %v\n%s", err, out.String()) } if len(parsed.Candidates) != 2 { t.Fatalf("want 2 candidates, got %d: %s", len(parsed.Candidates), out.String()) } if parsed.Summary == nil { - t.Errorf("repo-walker JSON missing summary: %s", out.String()) + t.Errorf("scan-repo JSON missing summary: %s", out.String()) } } @@ -274,9 +274,13 @@ func TestRun_Errors(t *testing.T) { {"bad policy", []string{"--policy", "delete", "x"}, 2}, {"discovery rejects dir", []string{"--mode", "discovery", "x"}, 2}, {"missing dir", []string{filepath.Join("definitely", "missing", "dir")}, 2}, - {"scan missing dir", []string{"--mode", "scan", filepath.Join("definitely", "missing")}, 2}, - {"repo-walker missing dir", []string{"--mode", "repo-walker", filepath.Join("definitely", "missing")}, 2}, - {"repo-walker no args", []string{"--mode", "repo-walker"}, 2}, + {"scan-folder missing dir", []string{"--mode", "scan-folder", filepath.Join("definitely", "missing")}, 2}, + {"scan-repo missing dir", []string{"--mode", "scan-repo", filepath.Join("definitely", "missing")}, 2}, + {"scan-repo no args", []string{"--mode", "scan-repo"}, 2}, + // The pre-freeze names carry no back-compat alias: they are usage errors, not + // silent fallbacks onto the default analyze mode. + {"retired scan mode", []string{"--mode", "scan", "x"}, 2}, + {"retired repo-walker mode", []string{"--mode", "repo-walker", "x"}, 2}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index 781af79a..69a97244 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -7,6 +7,49 @@ guidance that the changelog's breaking-change entries link to. We are pre-1.0, so breaking changes bump the **minor** version (release-please is configured with `bump-minor-pre-major`) rather than the major. Read the relevant entry before upgrading across it. +## Unreleased — `manifest-analyzer` scan modes renamed, and `--format json` now emits a versioned contract (next minor; breaking) + +The analyzer's machine-readable output moved to the new public +[`pkg/manifestanalyzer`](../pkg/manifestanalyzer), which is a supported Go API and the single +definition of the JSON documents the CLI prints. Freezing that contract is also the moment to name +the CLI modes after the question each answers, so the CLI, the Go API and the docs use one pair of +nouns — **folder** and **repo**. + +**Modes renamed. There are no back-compat aliases: the old names now exit 2 (usage error).** + +| Before | After | Answers | +|---|---|---| +| `--mode scan` | `--mode scan-folder` | May **this folder** become a `GitTarget`? (`ScanFolder`) | +| `--mode repo-walker` | `--mode scan-repo` | Which folders under **this repo root** could? (`ScanRepo`) | + +`repo-walker` named an internal traversal phase rather than a contract, and a bare `scan` was +asymmetric once a repo-level scan existed. `--mode analyze` and `--mode discovery` are unchanged. + +The JSON documents also gained a `schemaVersion` field, and one field was dropped: + +- `--mode scan-folder --format json` no longer carries `plan`. In folder-scan mode the analyzer has + no cluster state and no desired resources, so `plan` was structurally always + `{"counts":{},"actions":null}` — it never carried information. The meaningful fields (`accepted`, + `issues`, `retained`) are unchanged, and `issues` now marshals as `[]` rather than `null` when + there are none. +- `--mode scan-repo --format json` is otherwise unchanged. +- Retained entries now omit `identity` for a whole-file retention (an ordinary + `kustomization.yaml`, which names no resource) instead of emitting a zero-valued object. It is + still present for the refused mixed-file case, where a named resource hides inside a build + directive. +- `--mode analyze` and every `--format text` output are unchanged. + +**Migration** + +- Replace `--mode scan` with `--mode scan-folder`, and `--mode repo-walker` with `--mode scan-repo`. + A stale invocation fails loudly rather than falling back to the default `analyze` mode. +- Read `schemaVersion` and ignore fields you do not know; new fields are added within a schema major. +- If you exec'd the binary only to reach the acceptance verdict, prefer importing + `pkg/manifestanalyzer` and calling `ScanFolder` / `ScanRepo`. They run the same acceptance gate the + operator's writer enforces, so a tool built on them cannot drift from the operator that will later + adopt (or refuse) the folder. +- If you parsed `plan` from folder-scan mode, you were reading an empty object; drop the field. + ## Unreleased — chart defaults now run Redis-free (next minor; behavior change) The Helm chart now defaults to the simple, Redis-free `configured-author` path, so a bare diff --git a/docs/design/gitops-api/f8-repo-discovery-and-onboarding-scan.md b/docs/design/gitops-api/f8-repo-discovery-and-onboarding-scan.md index 1019934e..0bd46d55 100644 --- a/docs/design/gitops-api/f8-repo-discovery-and-onboarding-scan.md +++ b/docs/design/gitops-api/f8-repo-discovery-and-onboarding-scan.md @@ -62,7 +62,7 @@ layer — exactly the division already recorded in Scanning every folder sounds like it re-opens every kustomize question. It does not, because **discovery is broad and read-only while the write contract stays narrow and -principled** — they are deliberately different jobs. repo-walker classifies what a folder +principled** — they are deliberately different jobs. scan-repo classifies what a folder *is*; it never widens what the operator will *write*. The write boundary is not a hand-maintained feature allowlist but a **consequence of one rule**: every edit must have a single writable Git destination that round-trips @@ -84,7 +84,7 @@ share one engine (`Scan` is documented as "the one planner shared by the manifest-analyzer CLI and the controller's scan path", `internal/manifestanalyzer/scan.go`), and `manifest-analyzer` already scans a **single** folder read-only with no cluster: -`ScanDir(ctx, dir, nil, nil, policy)` (`--mode scan`). +`ScanDir(ctx, dir, nil, nil, policy)` (`--mode scan-folder`). The building blocks a repo-wide pass needs all exist: @@ -212,7 +212,7 @@ why rendered and editable diverge for overlays). Plus a repo-level summary: `exitUsage=2`). The eventual design: `--policy report` always exits 0 (pure report); `--policy refuse` exits 1 when **zero** candidates are acceptable — i.e. "is this repo onboardable at all?", the useful CI/onboarding gate. **The first cut does not yet apply -`--policy` in repo-walker mode** — it always exits 0 (or 2 on an I/O error); the refuse +`--policy` in scan-repo mode** — it always exits 0 (or 2 on an I/O error); the refuse gate is deferred. ## CLI surface @@ -220,15 +220,23 @@ gate is deferred. A new mode on `manifest-analyzer` (shipped form): ``` -manifest-analyzer --mode repo-walker --format json +manifest-analyzer --mode scan-repo --format json ``` -Naming note: the existing `--mode discovery` is **Kubernetes API discovery** (a -served-GVR dump against a kubeconfig), unrelated to repo discovery. The first cut -named the new mode `repo-walker` (not `repo`) to stay clear of it; renaming the -existing mode to `--mode api-discovery` so "discovery" is not overloaded remains a -possible follow-up. `--policy` is accepted for symmetry but the repo-level refuse gate -is deferred (see [First cut](#first-cut-shipped-2026-07-09)). +Naming note: the modes are named after the product question they answer, matching the +public [`pkg/manifestanalyzer`](../../../pkg/manifestanalyzer) entry points — +`scan-folder` asks "may **this folder** become a GitTarget?" (`ScanFolder`) and +`scan-repo` asks "which folders under **this repo root** could?" (`ScanRepo`). The first +cut shipped these as `scan` and `repo-walker`; both were renamed when the package became +a supported contract, with no back-compat aliases. `repo-walker` named an internal +traversal phase rather than a contract, and a bare `scan` was asymmetric once a +repo-level scan existed. + +The existing `--mode discovery` is **Kubernetes API discovery** (a served-GVR dump +against a kubeconfig), unrelated to repo discovery; renaming it to `--mode api-discovery` +so "discovery" is not overloaded remains a possible follow-up. `--policy` is accepted for +symmetry but the repo-level refuse gate is deferred (see +[First cut](#first-cut-shipped-2026-07-09)). The report should also be exposed as a **library function** (the product is likely part of the same Go binary family), with the CLI mode as the thin wrapper @@ -239,13 +247,18 @@ likely part of the same Go binary family), with the CLI mode as the thin wrapper A lean first slice landed on branch `feat/gitops-api-f8`. It **reports**; it does not yet **propose**. Library-first, as recommended above. -**Surface.** `WalkRepo(ctx, root) (RepoReport, error)` in -`internal/manifestanalyzer/repowalk.go`, with `--mode repo-walker ` +**Surface.** `ScanRepo(ctx, root) (RepoReport, error)` in +`internal/manifestanalyzer/scan_repo.go`, with `--mode scan-repo ` (`--format text|json`) as a thin CLI wrapper. Read-only, no cluster, symlinks never followed — the same posture as `ScanDir`, over the whole tree. It reuses `collectFiles`, `parseKustomizations`/`renderRoots`, the `Scan`/acceptance gate, and mirrors `gittarget_path_overlap` for nesting. +A product layer consuming this from Go imports [`pkg/manifestanalyzer`](../../../pkg/manifestanalyzer) +— `ScanRepo` and `ScanFolder` — rather than exec'ing the binary. That package is the +supported, versioned projection of the reports below; the engine above stays internal and +free to move. See [../multi-tenant/README.md](../multi-tenant/README.md) item 4. + **Report shape as shipped**, refining the sketch above. Per candidate: `path`, `layout`, `acceptedByOperator`, `refusalReasons[]` (`{code, detail}`), `renderRoot`, `readScope[]`, `inferredNamespace`, `resources`, `overlapsWith[]`. @@ -292,7 +305,7 @@ generation, the `--mode discovery` rename, and the repo-level `--policy refuse` (exit codes stay simple — `exitOK`, or `exitUsage` on an I/O error). **Corpus.** Golden reports under -`internal/manifestanalyzer/testdata/repo-walker/{supported,unsupported}//` with a +`internal/manifestanalyzer/testdata/scan-repo/{supported,unsupported}//` with a sibling `.golden.json` each (regenerate with `UPDATE_GOLDEN=1`), covering plain-per-env, kustomize-single, base+overlays, HelmRelease document vs. helm inflation, unsupported kustomize (incl. `openapi`/`crds`), fleet-root, overlapping, no-krm, and @@ -349,9 +362,10 @@ nested base (deduped `rendered`), an overlay base holding parked YAML (excluded ## Open questions -1. **Mode naming / the `discovery` collision** — _partly settled:_ the new mode shipped - as `repo-walker` (no collision). Whether to also rename the K8s-API mode to - `api-discovery` is still open. +1. **Mode naming / the `discovery` collision** — _partly settled:_ the repo mode is + `scan-repo` and the folder mode is `scan-folder`, mirroring `ScanRepo` / `ScanFolder` + (no collision). Whether to also rename the K8s-API mode to `api-discovery` is still + open. 2. **Stop at `GitTarget`, or also propose `WatchRule`s?** — moot for now: the first cut proposes neither (reports only). 3. **Read-scope depth for overlays pre-F2** — how far up the tree to follow @@ -361,7 +375,7 @@ nested base (deduped `rendered`), an overlay base holding parked YAML (excluded double-counts a shared nested base). How many levels the product should *display* remains a presentation choice. 4. **Library vs. CLI as the product's integration point** — _settled:_ library-first - (`WalkRepo`), CLI (`--mode repo-walker`) as the thin wrapper + CI gate. + (`ScanRepo`), CLI (`--mode scan-repo`) as the thin wrapper + CI gate. 5. **`overlay-fan-out-needs-f2` naming** — the reason code reads as if *fan-out* (a base shared by several overlays) is the trigger, but the real trigger is a base **escaping the render root's own subtree**, true even at fan-out = 1. The first cut carries the @@ -370,5 +384,5 @@ nested base (deduped `rendered`), an overlay base holding parked YAML (excluded and rely on the detail. _Not settled._ 6. **Repo-level `--policy refuse` gate** — the eventual "zero acceptable candidates → exit 1" onboarding gate (see [Exit codes](#the-report-contract)) is **deferred**: the first - cut always exits 0 (or 2 on an I/O error) and `--policy` is not applied in repo-walker + cut always exits 0 (or 2 on an I/O error) and `--policy` is not applied in scan-repo mode. Open: implement it as the CI/onboarding gate when the product needs it. diff --git a/docs/design/gitops-api/gittarget-granularity-and-cross-environment-edits.md b/docs/design/gitops-api/gittarget-granularity-and-cross-environment-edits.md index 84e6e8b2..001540a8 100644 --- a/docs/design/gitops-api/gittarget-granularity-and-cross-environment-edits.md +++ b/docs/design/gitops-api/gittarget-granularity-and-cross-environment-edits.md @@ -314,10 +314,10 @@ later — not a reason to make the operator's write unit span environments. A is also where the code and docs already point: the product model fixes environment = GitTarget = watch scope = write scope with shared read-only bases ([kustomize-support-boundary-and-product-model.md §5](kustomize-support-boundary-and-product-model.md#L208)), -and the repo-walker already classifies an out-of-subtree base as a +and the repo scan already classifies an out-of-subtree base as a forward-looking F2 gap (`overlay-fan-out-needs-f2`), not permanent unsupported structure -([repowalk.go](../../../internal/manifestanalyzer/repowalk.go#L37)). A is the +([scan_repo.go](../../../internal/manifestanalyzer/scan_repo.go#L37)). A is the direction of travel; this decision commits to it. **Onboarding UX is not a reason to pick B.** A produces N targets per app, but @@ -469,7 +469,7 @@ precondition and is refused outright (§1). - **F2 scope gains one concrete capability under A/C:** follow `../../base` for *reading* (today dropped), while the write jail stays at `spec.path`. Much of the read-scope / render-root / out-of-subtree-base logic already exists - read-only in the F8 repo-walker and can be promoted into the live analyzer. + read-only in the F8 repo scan and can be promoted into the live analyzer. - **Cross-environment editing is decided:** product promotion at launch; Option C (base-as-variant) later and only for editing *shared defaults*, not as the "every environment" answer. Neither reopens the fan-in invariant. diff --git a/docs/design/gitops-api/kustomize-support-boundary-and-product-model.md b/docs/design/gitops-api/kustomize-support-boundary-and-product-model.md index 4233a9c0..7f5f7736 100644 --- a/docs/design/gitops-api/kustomize-support-boundary-and-product-model.md +++ b/docs/design/gitops-api/kustomize-support-boundary-and-product-model.md @@ -121,7 +121,7 @@ Explicitly **out of scope**, and worth saying in user docs: much-later feature, not part of this boundary.) This allowlist is a **write** contract, not a discovery one. The read-only -onboarding scanner ([F8 repo-walker](f8-repo-discovery-and-onboarding-scan.md)) +onboarding scanner ([F8 repo scan](f8-repo-discovery-and-onboarding-scan.md)) walks a whole repo and classifies each folder against *exactly* this boundary — `plain` / `kustomize-single` (accepted), `kustomize-overlay` (forward-looking, needs F2), `refused-structural` (the permanent wall above) — without widening diff --git a/internal/manifestanalyzer/render.go b/internal/manifestanalyzer/render.go index 913cca89..81853bd9 100644 --- a/internal/manifestanalyzer/render.go +++ b/internal/manifestanalyzer/render.go @@ -10,7 +10,6 @@ import ( "strings" "github.com/ConfigButler/gitops-reverser/internal/git/manifestedit" - "github.com/ConfigButler/gitops-reverser/internal/types" ) // RenderJSON writes the report as indented JSON. It is the machine-readable form @@ -197,102 +196,3 @@ func planActionTarget(a PlanAction) string { } return fmt.Sprintf("%s#%d", a.Ref.FilePath, a.Ref.DocumentIndex) } - -// RenderScanJSON writes the scan as indented JSON: the machine-readable form shared -// by the CLI and the GitTarget status path. It omits the live desired objects, which -// are unbounded; it carries only the decided plan. -func RenderScanJSON(w io.Writer, result ScanResult) error { - enc := json.NewEncoder(w) - enc.SetIndent("", " ") - return enc.Encode(scanToJSON(result)) -} - -// scanJSON is the compact, bounded JSON projection of a ScanResult. -type scanJSON struct { - Accepted bool `json:"accepted"` - Issues []AcceptanceIssue `json:"issues"` - Retained []retainedJSON `json:"retained,omitempty"` - Plan planJSON `json:"plan"` -} - -type retainedJSON struct { - Path string `json:"path"` - DocumentIndex int `json:"documentIndex"` - Identity manifestedit.Identity `json:"identity"` -} - -type planJSON struct { - Counts map[string]int `json:"counts"` - Actions []planActionJSON `json:"actions"` - Diagnostics []manifestedit.Diagnostic `json:"diagnostics,omitempty"` -} - -type planActionJSON struct { - Kind string `json:"kind"` - Path string `json:"path,omitempty"` - // DocumentIndex is a pointer so that index 0 — a real, common target for a patch - // or drop on a file's first document — is preserved, while a create (which has no - // existing document location) omits it. A plain int with omitempty would drop the - // meaningful 0 and weaken the machine-readable contract. - DocumentIndex *int `json:"documentIndex,omitempty"` - Identity manifestedit.Identity `json:"identity"` - Resource string `json:"resource,omitempty"` - Reason string `json:"reason,omitempty"` -} - -// scanToJSON builds the compact JSON projection. -func scanToJSON(result ScanResult) scanJSON { - out := scanJSON{ - Accepted: result.Acceptance.Accepted, - Issues: result.Acceptance.Issues, - Plan: planJSON{ - Counts: planCounts(result.Plan), - Diagnostics: result.Plan.Diagnostics, - }, - } - for _, rd := range result.Acceptance.Retained { - out.Retained = append(out.Retained, retainedJSON{ - Path: rd.Location.Path, DocumentIndex: rd.Location.DocumentIndex, Identity: rd.Identity, - }) - } - for _, a := range result.Plan.Actions { - out.Plan.Actions = append(out.Plan.Actions, planActionJSON{ - Kind: string(a.Kind), - Path: planActionTarget(a), - DocumentIndex: documentIndexJSON(a), - Identity: a.Identity, - Resource: resourceString(a.Resource), - Reason: a.Reason, - }) - } - return out -} - -// documentIndexJSON returns the action's document index as a pointer: nil for a -// create (no existing document location, so the field is omitted), otherwise the -// real index — including a meaningful 0. -func documentIndexJSON(a PlanAction) *int { - if a.Kind == PlanCreate { - return nil - } - i := a.Ref.DocumentIndex - return &i -} - -// planCounts converts the plan's per-kind counts to string keys for JSON. -func planCounts(plan Plan) map[string]int { - out := map[string]int{} - for kind, n := range plan.Counts() { - out[string(kind)] = n - } - return out -} - -// resourceString renders a resolved resource identity, or "" when it is the zero -// value (a document a structure-only store never resolved). -func resourceString(r types.ResourceIdentifier) string { - if r == (types.ResourceIdentifier{}) { - return "" - } - return r.Key() -} diff --git a/internal/manifestanalyzer/render_test.go b/internal/manifestanalyzer/render_test.go index 7b8f1ce0..ce691830 100644 --- a/internal/manifestanalyzer/render_test.go +++ b/internal/manifestanalyzer/render_test.go @@ -146,86 +146,3 @@ func TestRenderScanText_Accepted(t *testing.T) { t.Errorf("expected no-changes plan line: %s", out) } } - -func TestRenderScanJSON(t *testing.T) { - var buf bytes.Buffer - if err := RenderScanJSON(&buf, scanWithRefusalAndPlan(t)); err != nil { - t.Fatalf("RenderScanJSON: %v", err) - } - - var parsed struct { - Accepted bool `json:"accepted"` - Issues []struct { - Kind string `json:"kind"` - } `json:"issues"` - Retained []struct { - Path string `json:"path"` - } `json:"retained"` - Plan struct { - Counts map[string]int `json:"counts"` - Actions []struct { - Kind string `json:"kind"` - Path string `json:"path"` - Resource string `json:"resource"` - } `json:"actions"` - } `json:"plan"` - } - if err := json.Unmarshal(buf.Bytes(), &parsed); err != nil { - t.Fatalf("scan JSON is invalid: %v\n%s", err, buf.String()) - } - if parsed.Accepted { - t.Errorf("scan JSON should report refusal") - } - if len(parsed.Issues) == 0 || len(parsed.Retained) != 1 { - t.Errorf("scan JSON issues=%d retained=%d", len(parsed.Issues), len(parsed.Retained)) - } - if parsed.Plan.Counts["create"] != 1 || parsed.Plan.Counts["patch"] != 1 { - t.Errorf("scan JSON plan counts = %+v", parsed.Plan.Counts) - } - if len(parsed.Plan.Actions) != 2 { - t.Errorf("scan JSON should carry both actions, got %+v", parsed.Plan.Actions) - } -} - -// TestRenderScanJSON_PreservesDocumentIndexZero guards the machine-readable contract: -// a patch on a file's first document must serialize documentIndex 0 (not omit it), -// while a create — which has no existing document location — omits it. -func TestRenderScanJSON_PreservesDocumentIndexZero(t *testing.T) { - fsys := fstest.MapFS{"deploy.yaml": {Data: []byte(deployYAML)}} - desired := []DesiredResource{desiredDeployWeb(3), desiredConfigMap("new")} // patch deploy#0 + create - result := Scan(context.Background(), fsys, snapMapper(), desired, ScanPolicy{}) - - var buf bytes.Buffer - if err := RenderScanJSON(&buf, result); err != nil { - t.Fatalf("RenderScanJSON: %v", err) - } - if !strings.Contains(buf.String(), `"documentIndex": 0`) { - t.Errorf("a patch on the first document must keep documentIndex 0:\n%s", buf.String()) - } - - var parsed struct { - Plan struct { - Actions []struct { - Kind string `json:"kind"` - DocumentIndex *int `json:"documentIndex"` - } `json:"actions"` - } `json:"plan"` - } - if err := json.Unmarshal(buf.Bytes(), &parsed); err != nil { - t.Fatalf("invalid JSON: %v", err) - } - for _, a := range parsed.Plan.Actions { - switch a.Kind { - case "patch": - if a.DocumentIndex == nil || *a.DocumentIndex != 0 { - t.Errorf("patch action should carry documentIndex 0, got %+v", a.DocumentIndex) - } - case "create": - if a.DocumentIndex != nil { - t.Errorf("create action should omit documentIndex, got %v", *a.DocumentIndex) - } - default: - t.Errorf("unexpected action kind %q", a.Kind) - } - } -} diff --git a/internal/manifestanalyzer/repowalk.go b/internal/manifestanalyzer/scan_repo.go similarity index 97% rename from internal/manifestanalyzer/repowalk.go rename to internal/manifestanalyzer/scan_repo.go index 99cb3a55..07eed438 100644 --- a/internal/manifestanalyzer/repowalk.go +++ b/internal/manifestanalyzer/scan_repo.go @@ -4,7 +4,6 @@ package manifestanalyzer import ( "context" - "encoding/json" "fmt" "io" "io/fs" @@ -153,7 +152,7 @@ type RepoSummary struct { // RepoReport is the whole-repo discovery report: the machine-readable contract the // product layer consumes. type RepoReport struct { - // Root is the scanned repository root as passed to WalkRepo. It is informational. + // Root is the scanned repository root as passed to ScanRepo. It is informational. Root string `json:"root,omitempty"` // Candidates are the enumerated subtrees, sorted by path. Candidates []RepoCandidate `json:"candidates"` @@ -161,12 +160,12 @@ type RepoReport struct { Summary RepoSummary `json:"summary"` } -// WalkRepo is the F8 whole-repo discovery pass (the library entry point; the CLI -// --mode repo-walker is a thin wrapper). It is read-only, writes nothing, needs no +// ScanRepo is the F8 whole-repo discovery pass (the library entry point; the CLI +// --mode scan-repo is a thin wrapper). It is read-only, writes nothing, needs no // cluster, and never follows symlinks — the same posture as ScanDir, just over the // whole tree rather than one subtree. It verifies root is a directory, then walks // os.DirFS(root). -func WalkRepo(ctx context.Context, root string) (RepoReport, error) { +func ScanRepo(ctx context.Context, root string) (RepoReport, error) { info, err := os.Stat(root) if err != nil { return RepoReport{}, err @@ -174,13 +173,13 @@ func WalkRepo(ctx context.Context, root string) (RepoReport, error) { if !info.IsDir() { return RepoReport{}, fmt.Errorf("not a directory: %s", root) } - rep := walkRepoFS(ctx, os.DirFS(root)) + rep := scanRepoFS(ctx, os.DirFS(root)) rep.Root = root return rep, nil } -// walkRepoFS is WalkRepo over an fs.FS, so it is testable against an in-memory tree. -func walkRepoFS(ctx context.Context, fsys fs.FS) RepoReport { +// scanRepoFS is ScanRepo over an fs.FS, so it is testable against an in-memory tree. +func scanRepoFS(ctx context.Context, fsys fs.FS) RepoReport { scan := collectFiles(fsys) kusts := parseKustomizations(scan.YAMLFiles) // Structure-only whole-repo store built with the live writer's allowlist (WriterAllowlist: @@ -711,14 +710,6 @@ func sortedKeysOf(m map[string]struct{}) []string { return out } -// RenderRepoJSON writes the repo report as indented JSON — the product's interface, -// matching the existing --format json convention. -func RenderRepoJSON(w io.Writer, rep RepoReport) error { - enc := json.NewEncoder(w) - enc.SetIndent("", " ") - return enc.Encode(rep) -} - // RenderRepoText writes a compact human summary of the repo report: one line per // candidate, then the roll-up. It is a convenience view; JSON is the contract. func RenderRepoText(w io.Writer, rep RepoReport) { diff --git a/internal/manifestanalyzer/repowalk_test.go b/internal/manifestanalyzer/scan_repo_test.go similarity index 75% rename from internal/manifestanalyzer/repowalk_test.go rename to internal/manifestanalyzer/scan_repo_test.go index 3ee87410..dd4544ba 100644 --- a/internal/manifestanalyzer/repowalk_test.go +++ b/internal/manifestanalyzer/scan_repo_test.go @@ -5,6 +5,7 @@ package manifestanalyzer import ( "bytes" "context" + "encoding/json" "os" "path/filepath" "testing" @@ -23,16 +24,16 @@ metadata: namespace: demo ` -// TestWalkRepo_Golden drives the whole discovery corpus under testdata/repo-walker. +// TestScanRepo_Golden drives the whole discovery corpus under testdata/scan-repo. // Each fixture is a self-contained repo with a sibling .golden.json pinning its // report, so layout classification, refusal codes, overlap detection, namespace // inference, and the rendered/editable split are all fixed by real layouts rather than // prose. The corpus is split supported/ vs unsupported/ mirroring the // contextual-namespace corpus. See // docs/design/gitops-api/f8-repo-discovery-and-onboarding-scan.md. -func TestWalkRepo_Golden(t *testing.T) { +func TestScanRepo_Golden(t *testing.T) { for _, group := range []string{"supported", "unsupported"} { - base := filepath.Join("testdata", "repo-walker", group) + base := filepath.Join("testdata", "scan-repo", group) entries, err := os.ReadDir(base) if err != nil { t.Fatalf("read corpus %s: %v", base, err) @@ -52,17 +53,22 @@ func TestWalkRepo_Golden(t *testing.T) { // golden is path-independent. func checkGoldenFixture(t *testing.T, fixture string) { t.Helper() - rep, err := WalkRepo(context.Background(), fixture) + rep, err := ScanRepo(context.Background(), fixture) if err != nil { - t.Fatalf("WalkRepo(%s): %v", fixture, err) + t.Fatalf("ScanRepo(%s): %v", fixture, err) } if rep.Root != fixture { t.Errorf("Root = %q, want %q", rep.Root, fixture) } rep.Root = "" + // The engine's own report shape, not the published one. pkg/manifestanalyzer owns the + // machine-readable contract and pins it separately; this corpus pins the classification + // facts (layout, refusal codes, overlaps, namespace inference, rendered/editable split). var buf bytes.Buffer - if err := RenderRepoJSON(&buf, rep); err != nil { + enc := json.NewEncoder(&buf) + enc.SetIndent("", " ") + if err := enc.Encode(rep); err != nil { t.Fatalf("render json: %v", err) } @@ -82,11 +88,11 @@ func checkGoldenFixture(t *testing.T, fixture string) { } } -// TestWalkRepo_RefusalCodesStayDistinct pins the load-bearing distinction the design +// TestScanRepo_RefusalCodesStayDistinct pins the load-bearing distinction the design // calls out: a forward-looking overlay-fan-out-needs-f2 must never collapse into the // permanent refused-structural boundary. base-overlays is the former; helm-inflation and // unsupported-kustomize are the latter. -func TestWalkRepo_RefusalCodesStayDistinct(t *testing.T) { +func TestScanRepo_RefusalCodesStayDistinct(t *testing.T) { cases := []struct { fixture string wantCode string @@ -99,9 +105,9 @@ func TestWalkRepo_RefusalCodesStayDistinct(t *testing.T) { } for _, tc := range cases { t.Run(tc.fixture, func(t *testing.T) { - rep, err := WalkRepo(context.Background(), filepath.Join("testdata", "repo-walker", tc.fixture)) + rep, err := ScanRepo(context.Background(), filepath.Join("testdata", "scan-repo", tc.fixture)) if err != nil { - t.Fatalf("WalkRepo: %v", err) + t.Fatalf("ScanRepo: %v", err) } if len(rep.Candidates) == 0 { t.Fatalf("expected at least one candidate") @@ -127,15 +133,15 @@ func assertRefused(t *testing.T, fixture string, c RepoCandidate, layout Layout, } } -// TestWalkRepo_RenderedCountDedupesNestedBase guards the double-count: an overlay whose +// TestScanRepo_RenderedCountDedupesNestedBase guards the double-count: an overlay whose // out-of-subtree base pulls in a nested base must count each rendered document exactly // once (rendered=2 here: base/deployment + base/common/configmap), and readScope must be // the minimal [base] rather than [base, base/common]. A regression would report 3. -func TestWalkRepo_RenderedCountDedupesNestedBase(t *testing.T) { - fixture := filepath.Join("testdata", "repo-walker", "unsupported", "overlay-nested-base") - rep, err := WalkRepo(context.Background(), fixture) +func TestScanRepo_RenderedCountDedupesNestedBase(t *testing.T) { + fixture := filepath.Join("testdata", "scan-repo", "unsupported", "overlay-nested-base") + rep, err := ScanRepo(context.Background(), fixture) if err != nil { - t.Fatalf("WalkRepo: %v", err) + t.Fatalf("ScanRepo: %v", err) } if len(rep.Candidates) != 1 { t.Fatalf("want 1 candidate, got %d: %+v", len(rep.Candidates), rep.Candidates) @@ -149,14 +155,14 @@ func TestWalkRepo_RenderedCountDedupesNestedBase(t *testing.T) { } } -// TestWalkRepo_RenderedExcludesParkedYAML guards that rendered counts only what the +// TestScanRepo_RenderedExcludesParkedYAML guards that rendered counts only what the // kustomization graph reaches: a base holding a parked.yaml its kustomization does not // list must not inflate rendered. overlay-parked-base renders 1 (base/deployment), not 2. -func TestWalkRepo_RenderedExcludesParkedYAML(t *testing.T) { - fixture := filepath.Join("testdata", "repo-walker", "unsupported", "overlay-parked-base") - rep, err := WalkRepo(context.Background(), fixture) +func TestScanRepo_RenderedExcludesParkedYAML(t *testing.T) { + fixture := filepath.Join("testdata", "scan-repo", "unsupported", "overlay-parked-base") + rep, err := ScanRepo(context.Background(), fixture) if err != nil { - t.Fatalf("WalkRepo: %v", err) + t.Fatalf("ScanRepo: %v", err) } if len(rep.Candidates) != 1 { t.Fatalf("want 1 candidate, got %d: %+v", len(rep.Candidates), rep.Candidates) @@ -166,14 +172,14 @@ func TestWalkRepo_RenderedExcludesParkedYAML(t *testing.T) { } } -// TestWalkRepo_RefusedPlainSurfacesGateReasons guards that a plain candidate the +// TestScanRepo_RefusedPlainSurfacesGateReasons guards that a plain candidate the // acceptance gate refuses reports WHY (the gate issues) rather than a bare // acceptedByOperator: false. plain-nonkrm holds a non-KRM values.yaml. -func TestWalkRepo_RefusedPlainSurfacesGateReasons(t *testing.T) { - fixture := filepath.Join("testdata", "repo-walker", "unsupported", "plain-nonkrm") - rep, err := WalkRepo(context.Background(), fixture) +func TestScanRepo_RefusedPlainSurfacesGateReasons(t *testing.T) { + fixture := filepath.Join("testdata", "scan-repo", "unsupported", "plain-nonkrm") + rep, err := ScanRepo(context.Background(), fixture) if err != nil { - t.Fatalf("WalkRepo: %v", err) + t.Fatalf("ScanRepo: %v", err) } if len(rep.Candidates) != 1 { t.Fatalf("want 1 candidate, got %d", len(rep.Candidates)) @@ -190,17 +196,17 @@ func TestWalkRepo_RefusedPlainSurfacesGateReasons(t *testing.T) { } } -// TestWalkRepo_SopsBootstrapAcceptedLikeWriter guards that repo-walker's acceptance +// TestScanRepo_SopsBootstrapAcceptedLikeWriter guards that scan-repo's acceptance // matches the live writer's allowlist: a folder holding the operator's .sops.yaml // bootstrap config is accepted (the writer retains it), not refused as non-KRM, and the // .sops.yaml is not counted as non-KRM noise. -func TestWalkRepo_SopsBootstrapAcceptedLikeWriter(t *testing.T) { +func TestScanRepo_SopsBootstrapAcceptedLikeWriter(t *testing.T) { fsys := fstest.MapFS{ "app/deployment.yaml": {Data: []byte(deployYAMLNS)}, "app/.sops.yaml": {Data: []byte( "creation_rules:\n - path_regex: .*\n age: age1exampleexampleexampleexampleexampleexampleexample\n")}, } - rep := walkRepoFS(context.Background(), fsys) + rep := scanRepoFS(context.Background(), fsys) if len(rep.Candidates) != 1 { t.Fatalf("want 1 candidate, got %d: %+v", len(rep.Candidates), rep.Candidates) } @@ -214,10 +220,10 @@ func TestWalkRepo_SopsBootstrapAcceptedLikeWriter(t *testing.T) { } } -// TestWalkRepo_NotADirectory covers the usage error path: a missing or non-directory +// TestScanRepo_NotADirectory covers the usage error path: a missing or non-directory // root is an error, not an empty report. -func TestWalkRepo_NotADirectory(t *testing.T) { - if _, err := WalkRepo(context.Background(), filepath.Join("testdata", "does-not-exist")); err == nil { +func TestScanRepo_NotADirectory(t *testing.T) { + if _, err := ScanRepo(context.Background(), filepath.Join("testdata", "does-not-exist")); err == nil { t.Fatal("expected an error for a missing root") } } diff --git a/internal/manifestanalyzer/store.go b/internal/manifestanalyzer/store.go index 845b0a61..1e4286bc 100644 --- a/internal/manifestanalyzer/store.go +++ b/internal/manifestanalyzer/store.go @@ -806,7 +806,7 @@ func kustomizationUsesUnsupportedFeature(content []byte) bool { // or mutate resource identity (name/namespace) in ways the contextual-namespace writer // cannot map back to an editable source document. Their presence disqualifies a // kustomization as a namespace source. It is the single source of truth for both the -// boolean gate (hasUnsupportedKustomizeFeature) and the repo-walker's per-feature +// boolean gate (hasUnsupportedKustomizeFeature) and the repo scan's per-feature // refusal detail (unsupportedKustomizeFeatures). It returns a fresh slice on every call, // so no shared state can be mutated. func unsupportedKustomizeFeatureKeys() []string { diff --git a/internal/manifestanalyzer/testdata/repo-walker/supported/fleet-root.golden.json b/internal/manifestanalyzer/testdata/scan-repo/supported/fleet-root.golden.json similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/supported/fleet-root.golden.json rename to internal/manifestanalyzer/testdata/scan-repo/supported/fleet-root.golden.json diff --git a/internal/manifestanalyzer/testdata/repo-walker/supported/fleet-root/apps/podinfo/deployment.yaml b/internal/manifestanalyzer/testdata/scan-repo/supported/fleet-root/apps/podinfo/deployment.yaml similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/supported/fleet-root/apps/podinfo/deployment.yaml rename to internal/manifestanalyzer/testdata/scan-repo/supported/fleet-root/apps/podinfo/deployment.yaml diff --git a/internal/manifestanalyzer/testdata/repo-walker/supported/fleet-root/clusters/prod/flux-system.yaml b/internal/manifestanalyzer/testdata/scan-repo/supported/fleet-root/clusters/prod/flux-system.yaml similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/supported/fleet-root/clusters/prod/flux-system.yaml rename to internal/manifestanalyzer/testdata/scan-repo/supported/fleet-root/clusters/prod/flux-system.yaml diff --git a/internal/manifestanalyzer/testdata/repo-walker/supported/fleet-root/infra/ingress-nginx/deployment.yaml b/internal/manifestanalyzer/testdata/scan-repo/supported/fleet-root/infra/ingress-nginx/deployment.yaml similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/supported/fleet-root/infra/ingress-nginx/deployment.yaml rename to internal/manifestanalyzer/testdata/scan-repo/supported/fleet-root/infra/ingress-nginx/deployment.yaml diff --git a/internal/manifestanalyzer/testdata/repo-walker/supported/helm-release-document.golden.json b/internal/manifestanalyzer/testdata/scan-repo/supported/helm-release-document.golden.json similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/supported/helm-release-document.golden.json rename to internal/manifestanalyzer/testdata/scan-repo/supported/helm-release-document.golden.json diff --git a/internal/manifestanalyzer/testdata/repo-walker/supported/helm-release-document/app/helmrelease.yaml b/internal/manifestanalyzer/testdata/scan-repo/supported/helm-release-document/app/helmrelease.yaml similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/supported/helm-release-document/app/helmrelease.yaml rename to internal/manifestanalyzer/testdata/scan-repo/supported/helm-release-document/app/helmrelease.yaml diff --git a/internal/manifestanalyzer/testdata/repo-walker/supported/kustomize-single.golden.json b/internal/manifestanalyzer/testdata/scan-repo/supported/kustomize-single.golden.json similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/supported/kustomize-single.golden.json rename to internal/manifestanalyzer/testdata/scan-repo/supported/kustomize-single.golden.json diff --git a/internal/manifestanalyzer/testdata/repo-walker/supported/kustomize-single/app/deployment.yaml b/internal/manifestanalyzer/testdata/scan-repo/supported/kustomize-single/app/deployment.yaml similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/supported/kustomize-single/app/deployment.yaml rename to internal/manifestanalyzer/testdata/scan-repo/supported/kustomize-single/app/deployment.yaml diff --git a/internal/manifestanalyzer/testdata/repo-walker/supported/kustomize-single/app/kustomization.yaml b/internal/manifestanalyzer/testdata/scan-repo/supported/kustomize-single/app/kustomization.yaml similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/supported/kustomize-single/app/kustomization.yaml rename to internal/manifestanalyzer/testdata/scan-repo/supported/kustomize-single/app/kustomization.yaml diff --git a/internal/manifestanalyzer/testdata/repo-walker/supported/kustomize-single/app/service.yaml b/internal/manifestanalyzer/testdata/scan-repo/supported/kustomize-single/app/service.yaml similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/supported/kustomize-single/app/service.yaml rename to internal/manifestanalyzer/testdata/scan-repo/supported/kustomize-single/app/service.yaml diff --git a/internal/manifestanalyzer/testdata/repo-walker/supported/no-krm.golden.json b/internal/manifestanalyzer/testdata/scan-repo/supported/no-krm.golden.json similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/supported/no-krm.golden.json rename to internal/manifestanalyzer/testdata/scan-repo/supported/no-krm.golden.json diff --git a/internal/manifestanalyzer/testdata/repo-walker/supported/no-krm/docs/README.md b/internal/manifestanalyzer/testdata/scan-repo/supported/no-krm/docs/README.md similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/supported/no-krm/docs/README.md rename to internal/manifestanalyzer/testdata/scan-repo/supported/no-krm/docs/README.md diff --git a/internal/manifestanalyzer/testdata/repo-walker/supported/no-krm/docs/notes.txt b/internal/manifestanalyzer/testdata/scan-repo/supported/no-krm/docs/notes.txt similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/supported/no-krm/docs/notes.txt rename to internal/manifestanalyzer/testdata/scan-repo/supported/no-krm/docs/notes.txt diff --git a/internal/manifestanalyzer/testdata/repo-walker/supported/overlapping.golden.json b/internal/manifestanalyzer/testdata/scan-repo/supported/overlapping.golden.json similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/supported/overlapping.golden.json rename to internal/manifestanalyzer/testdata/scan-repo/supported/overlapping.golden.json diff --git a/internal/manifestanalyzer/testdata/repo-walker/supported/overlapping/apps/web/deployment.yaml b/internal/manifestanalyzer/testdata/scan-repo/supported/overlapping/apps/web/deployment.yaml similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/supported/overlapping/apps/web/deployment.yaml rename to internal/manifestanalyzer/testdata/scan-repo/supported/overlapping/apps/web/deployment.yaml diff --git a/internal/manifestanalyzer/testdata/repo-walker/supported/overlapping/apps/web/extra/configmap.yaml b/internal/manifestanalyzer/testdata/scan-repo/supported/overlapping/apps/web/extra/configmap.yaml similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/supported/overlapping/apps/web/extra/configmap.yaml rename to internal/manifestanalyzer/testdata/scan-repo/supported/overlapping/apps/web/extra/configmap.yaml diff --git a/internal/manifestanalyzer/testdata/repo-walker/supported/plain-per-env.golden.json b/internal/manifestanalyzer/testdata/scan-repo/supported/plain-per-env.golden.json similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/supported/plain-per-env.golden.json rename to internal/manifestanalyzer/testdata/scan-repo/supported/plain-per-env.golden.json diff --git a/internal/manifestanalyzer/testdata/repo-walker/supported/plain-per-env/apps/app/prod/deployment.yaml b/internal/manifestanalyzer/testdata/scan-repo/supported/plain-per-env/apps/app/prod/deployment.yaml similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/supported/plain-per-env/apps/app/prod/deployment.yaml rename to internal/manifestanalyzer/testdata/scan-repo/supported/plain-per-env/apps/app/prod/deployment.yaml diff --git a/internal/manifestanalyzer/testdata/repo-walker/supported/plain-per-env/apps/app/prod/service.yaml b/internal/manifestanalyzer/testdata/scan-repo/supported/plain-per-env/apps/app/prod/service.yaml similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/supported/plain-per-env/apps/app/prod/service.yaml rename to internal/manifestanalyzer/testdata/scan-repo/supported/plain-per-env/apps/app/prod/service.yaml diff --git a/internal/manifestanalyzer/testdata/repo-walker/supported/plain-per-env/apps/app/test/deployment.yaml b/internal/manifestanalyzer/testdata/scan-repo/supported/plain-per-env/apps/app/test/deployment.yaml similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/supported/plain-per-env/apps/app/test/deployment.yaml rename to internal/manifestanalyzer/testdata/scan-repo/supported/plain-per-env/apps/app/test/deployment.yaml diff --git a/internal/manifestanalyzer/testdata/repo-walker/supported/plain-per-env/apps/app/test/service.yaml b/internal/manifestanalyzer/testdata/scan-repo/supported/plain-per-env/apps/app/test/service.yaml similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/supported/plain-per-env/apps/app/test/service.yaml rename to internal/manifestanalyzer/testdata/scan-repo/supported/plain-per-env/apps/app/test/service.yaml diff --git a/internal/manifestanalyzer/testdata/repo-walker/unsupported/base-overlays.golden.json b/internal/manifestanalyzer/testdata/scan-repo/unsupported/base-overlays.golden.json similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/unsupported/base-overlays.golden.json rename to internal/manifestanalyzer/testdata/scan-repo/unsupported/base-overlays.golden.json diff --git a/internal/manifestanalyzer/testdata/repo-walker/unsupported/base-overlays/base/deployment.yaml b/internal/manifestanalyzer/testdata/scan-repo/unsupported/base-overlays/base/deployment.yaml similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/unsupported/base-overlays/base/deployment.yaml rename to internal/manifestanalyzer/testdata/scan-repo/unsupported/base-overlays/base/deployment.yaml diff --git a/internal/manifestanalyzer/testdata/repo-walker/unsupported/base-overlays/base/kustomization.yaml b/internal/manifestanalyzer/testdata/scan-repo/unsupported/base-overlays/base/kustomization.yaml similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/unsupported/base-overlays/base/kustomization.yaml rename to internal/manifestanalyzer/testdata/scan-repo/unsupported/base-overlays/base/kustomization.yaml diff --git a/internal/manifestanalyzer/testdata/repo-walker/unsupported/base-overlays/base/service.yaml b/internal/manifestanalyzer/testdata/scan-repo/unsupported/base-overlays/base/service.yaml similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/unsupported/base-overlays/base/service.yaml rename to internal/manifestanalyzer/testdata/scan-repo/unsupported/base-overlays/base/service.yaml diff --git a/internal/manifestanalyzer/testdata/repo-walker/unsupported/base-overlays/overlays/acc/kustomization.yaml b/internal/manifestanalyzer/testdata/scan-repo/unsupported/base-overlays/overlays/acc/kustomization.yaml similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/unsupported/base-overlays/overlays/acc/kustomization.yaml rename to internal/manifestanalyzer/testdata/scan-repo/unsupported/base-overlays/overlays/acc/kustomization.yaml diff --git a/internal/manifestanalyzer/testdata/repo-walker/unsupported/base-overlays/overlays/prod/kustomization.yaml b/internal/manifestanalyzer/testdata/scan-repo/unsupported/base-overlays/overlays/prod/kustomization.yaml similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/unsupported/base-overlays/overlays/prod/kustomization.yaml rename to internal/manifestanalyzer/testdata/scan-repo/unsupported/base-overlays/overlays/prod/kustomization.yaml diff --git a/internal/manifestanalyzer/testdata/repo-walker/unsupported/base-overlays/overlays/test/kustomization.yaml b/internal/manifestanalyzer/testdata/scan-repo/unsupported/base-overlays/overlays/test/kustomization.yaml similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/unsupported/base-overlays/overlays/test/kustomization.yaml rename to internal/manifestanalyzer/testdata/scan-repo/unsupported/base-overlays/overlays/test/kustomization.yaml diff --git a/internal/manifestanalyzer/testdata/repo-walker/unsupported/helm-inflation.golden.json b/internal/manifestanalyzer/testdata/scan-repo/unsupported/helm-inflation.golden.json similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/unsupported/helm-inflation.golden.json rename to internal/manifestanalyzer/testdata/scan-repo/unsupported/helm-inflation.golden.json diff --git a/internal/manifestanalyzer/testdata/repo-walker/unsupported/helm-inflation/app/kustomization.yaml b/internal/manifestanalyzer/testdata/scan-repo/unsupported/helm-inflation/app/kustomization.yaml similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/unsupported/helm-inflation/app/kustomization.yaml rename to internal/manifestanalyzer/testdata/scan-repo/unsupported/helm-inflation/app/kustomization.yaml diff --git a/internal/manifestanalyzer/testdata/repo-walker/unsupported/overlay-nested-base.golden.json b/internal/manifestanalyzer/testdata/scan-repo/unsupported/overlay-nested-base.golden.json similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/unsupported/overlay-nested-base.golden.json rename to internal/manifestanalyzer/testdata/scan-repo/unsupported/overlay-nested-base.golden.json diff --git a/internal/manifestanalyzer/testdata/repo-walker/unsupported/overlay-nested-base/base/common/configmap.yaml b/internal/manifestanalyzer/testdata/scan-repo/unsupported/overlay-nested-base/base/common/configmap.yaml similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/unsupported/overlay-nested-base/base/common/configmap.yaml rename to internal/manifestanalyzer/testdata/scan-repo/unsupported/overlay-nested-base/base/common/configmap.yaml diff --git a/internal/manifestanalyzer/testdata/repo-walker/unsupported/overlay-nested-base/base/common/kustomization.yaml b/internal/manifestanalyzer/testdata/scan-repo/unsupported/overlay-nested-base/base/common/kustomization.yaml similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/unsupported/overlay-nested-base/base/common/kustomization.yaml rename to internal/manifestanalyzer/testdata/scan-repo/unsupported/overlay-nested-base/base/common/kustomization.yaml diff --git a/internal/manifestanalyzer/testdata/repo-walker/unsupported/overlay-nested-base/base/deployment.yaml b/internal/manifestanalyzer/testdata/scan-repo/unsupported/overlay-nested-base/base/deployment.yaml similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/unsupported/overlay-nested-base/base/deployment.yaml rename to internal/manifestanalyzer/testdata/scan-repo/unsupported/overlay-nested-base/base/deployment.yaml diff --git a/internal/manifestanalyzer/testdata/repo-walker/unsupported/overlay-nested-base/base/kustomization.yaml b/internal/manifestanalyzer/testdata/scan-repo/unsupported/overlay-nested-base/base/kustomization.yaml similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/unsupported/overlay-nested-base/base/kustomization.yaml rename to internal/manifestanalyzer/testdata/scan-repo/unsupported/overlay-nested-base/base/kustomization.yaml diff --git a/internal/manifestanalyzer/testdata/repo-walker/unsupported/overlay-nested-base/overlays/test/kustomization.yaml b/internal/manifestanalyzer/testdata/scan-repo/unsupported/overlay-nested-base/overlays/test/kustomization.yaml similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/unsupported/overlay-nested-base/overlays/test/kustomization.yaml rename to internal/manifestanalyzer/testdata/scan-repo/unsupported/overlay-nested-base/overlays/test/kustomization.yaml diff --git a/internal/manifestanalyzer/testdata/repo-walker/unsupported/overlay-parked-base.golden.json b/internal/manifestanalyzer/testdata/scan-repo/unsupported/overlay-parked-base.golden.json similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/unsupported/overlay-parked-base.golden.json rename to internal/manifestanalyzer/testdata/scan-repo/unsupported/overlay-parked-base.golden.json diff --git a/internal/manifestanalyzer/testdata/repo-walker/unsupported/overlay-parked-base/base/deployment.yaml b/internal/manifestanalyzer/testdata/scan-repo/unsupported/overlay-parked-base/base/deployment.yaml similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/unsupported/overlay-parked-base/base/deployment.yaml rename to internal/manifestanalyzer/testdata/scan-repo/unsupported/overlay-parked-base/base/deployment.yaml diff --git a/internal/manifestanalyzer/testdata/repo-walker/unsupported/overlay-parked-base/base/kustomization.yaml b/internal/manifestanalyzer/testdata/scan-repo/unsupported/overlay-parked-base/base/kustomization.yaml similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/unsupported/overlay-parked-base/base/kustomization.yaml rename to internal/manifestanalyzer/testdata/scan-repo/unsupported/overlay-parked-base/base/kustomization.yaml diff --git a/internal/manifestanalyzer/testdata/repo-walker/unsupported/overlay-parked-base/base/parked.yaml b/internal/manifestanalyzer/testdata/scan-repo/unsupported/overlay-parked-base/base/parked.yaml similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/unsupported/overlay-parked-base/base/parked.yaml rename to internal/manifestanalyzer/testdata/scan-repo/unsupported/overlay-parked-base/base/parked.yaml diff --git a/internal/manifestanalyzer/testdata/repo-walker/unsupported/overlay-parked-base/overlays/test/kustomization.yaml b/internal/manifestanalyzer/testdata/scan-repo/unsupported/overlay-parked-base/overlays/test/kustomization.yaml similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/unsupported/overlay-parked-base/overlays/test/kustomization.yaml rename to internal/manifestanalyzer/testdata/scan-repo/unsupported/overlay-parked-base/overlays/test/kustomization.yaml diff --git a/internal/manifestanalyzer/testdata/repo-walker/unsupported/plain-nonkrm.golden.json b/internal/manifestanalyzer/testdata/scan-repo/unsupported/plain-nonkrm.golden.json similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/unsupported/plain-nonkrm.golden.json rename to internal/manifestanalyzer/testdata/scan-repo/unsupported/plain-nonkrm.golden.json diff --git a/internal/manifestanalyzer/testdata/repo-walker/unsupported/plain-nonkrm/app/deployment.yaml b/internal/manifestanalyzer/testdata/scan-repo/unsupported/plain-nonkrm/app/deployment.yaml similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/unsupported/plain-nonkrm/app/deployment.yaml rename to internal/manifestanalyzer/testdata/scan-repo/unsupported/plain-nonkrm/app/deployment.yaml diff --git a/internal/manifestanalyzer/testdata/repo-walker/unsupported/plain-nonkrm/app/values.yaml b/internal/manifestanalyzer/testdata/scan-repo/unsupported/plain-nonkrm/app/values.yaml similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/unsupported/plain-nonkrm/app/values.yaml rename to internal/manifestanalyzer/testdata/scan-repo/unsupported/plain-nonkrm/app/values.yaml diff --git a/internal/manifestanalyzer/testdata/repo-walker/unsupported/unsupported-kustomize.golden.json b/internal/manifestanalyzer/testdata/scan-repo/unsupported/unsupported-kustomize.golden.json similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/unsupported/unsupported-kustomize.golden.json rename to internal/manifestanalyzer/testdata/scan-repo/unsupported/unsupported-kustomize.golden.json diff --git a/internal/manifestanalyzer/testdata/repo-walker/unsupported/unsupported-kustomize/app/deployment.yaml b/internal/manifestanalyzer/testdata/scan-repo/unsupported/unsupported-kustomize/app/deployment.yaml similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/unsupported/unsupported-kustomize/app/deployment.yaml rename to internal/manifestanalyzer/testdata/scan-repo/unsupported/unsupported-kustomize/app/deployment.yaml diff --git a/internal/manifestanalyzer/testdata/repo-walker/unsupported/unsupported-kustomize/app/kustomization.yaml b/internal/manifestanalyzer/testdata/scan-repo/unsupported/unsupported-kustomize/app/kustomization.yaml similarity index 100% rename from internal/manifestanalyzer/testdata/repo-walker/unsupported/unsupported-kustomize/app/kustomization.yaml rename to internal/manifestanalyzer/testdata/scan-repo/unsupported/unsupported-kustomize/app/kustomization.yaml diff --git a/internal/queue/attribution_index.go b/internal/queue/attribution_index.go index 187421d6..9f9063a9 100644 --- a/internal/queue/attribution_index.go +++ b/internal/queue/attribution_index.go @@ -28,8 +28,10 @@ import ( // from one that never arrived. Configurable via --author-attribution-ttl. const DefaultAttributionFactTTL = 15 * time.Minute -// keyPrefix is the fixed root namespace for every Redis key (cursors and facts alike). -const keyPrefix = "gitops-reverser" +// DefaultKeyPrefix is the root namespace every Redis key (cursors, facts, and command +// author records alike) carries when --redis-key-prefix is not set. It is also the value +// every release before the flag existed used, so the default is a no-op upgrade. +const DefaultKeyPrefix = "gitops-reverser" const ( // attributionKeySuffix namespaces audit-sourced resource author facts under the @@ -105,6 +107,10 @@ type AuthorResolution struct { type AttributionIndex struct { client *redis.Client factTTL time.Duration + // keyPrefix is the root namespace this index writes and reads under, shared with the + // RedisStore that built it. Empty only in tests constructed by hand; every key builder + // resolves it so an empty value still lands on DefaultKeyPrefix. + keyPrefix string } // RecordFact stores the attribution fact for one accepted, mutating audit event. A @@ -406,7 +412,7 @@ func attributionResultForFact(fact AuthorFact, weak bool) AttributionResult { // factKeyBase is the per-type prefix shared by every fact key, e.g. // "gitops-reverser:author:v1:audit:apps/deployments". func (a *AttributionIndex) factKeyBase(gr string) string { - return keyPrefix + attributionKeySuffix + gr + return resolveKeyPrefix(a.keyPrefix) + attributionKeySuffix + gr } // factKeyExact is the immutable per-write fact key, e.g. @@ -447,7 +453,7 @@ func (a *AttributionIndex) recordFactIndexSize(ctx context.Context) { } var cursor uint64 var count int64 - pattern := keyPrefix + attributionKeySuffix + "*" + pattern := resolveKeyPrefix(a.keyPrefix) + attributionKeySuffix + "*" for { keys, next, err := a.client.Scan(ctx, cursor, pattern, attributionFactScanBatchSize).Result() if err != nil { diff --git a/internal/queue/command_author_store.go b/internal/queue/command_author_store.go index 747f2374..ad012b48 100644 --- a/internal/queue/command_author_store.go +++ b/internal/queue/command_author_store.go @@ -42,6 +42,8 @@ type CommandAuthor struct { type CommandAuthorStore struct { client *redis.Client ttl time.Duration + // keyPrefix is the root namespace shared with the RedisStore that built this store. + keyPrefix string } // RecordCommandAuthor is the admission-side write: capture the authenticated submitter @@ -78,5 +80,5 @@ func (s *CommandAuthorStore) LookupCommandAuthor( // key identifies the command by UID alone — globally unique, like the watch cursor key, // so namespace/name/kind would be redundant (kept out for a tight key). func (s *CommandAuthorStore) key(uid types.UID) string { - return keyPrefix + commandAuthorKeySuffix + escapeKeyField(string(uid)) + return resolveKeyPrefix(s.keyPrefix) + commandAuthorKeySuffix + escapeKeyField(string(uid)) } diff --git a/internal/queue/key_prefix.go b/internal/queue/key_prefix.go new file mode 100644 index 00000000..d2e5bd11 --- /dev/null +++ b/internal/queue/key_prefix.go @@ -0,0 +1,66 @@ +// SPDX-License-Identifier: Apache-2.0 + +package queue + +import ( + "fmt" + "strings" +) + +// maxKeyPrefixLength bounds the root namespace. Redis keys are capped at 512MB, so this +// is not a protocol limit — it is a legibility limit, and a guard against a prefix that +// dwarfs the key it namespaces. +const maxKeyPrefixLength = 128 + +// ValidateKeyPrefix checks a --redis-key-prefix value and returns its normalized form. +// +// Two independent constraints shape the allowed character set: +// +// - The attribution telemetry gauge SCANs ":author:v1:audit:*". Redis glob +// metacharacters (*, ?, [, ], \) in the prefix would silently make that pattern match +// the wrong keyspace, so they are rejected rather than escaped — a prefix is an +// operator-chosen identifier, not user data. +// - Key fields (uid, resourceVersion, namespace) are ':'-delimited and %-escaped by +// escapeKeyField. '%' in the prefix would make an escaped key ambiguous with an +// unescaped one, so it is rejected too. +// +// ':' is allowed, because a prefix like "cell-a:tenant-7" is a natural nesting and every +// suffix constant already begins with ':'. A trailing ':' is normalized away so +// "tenant-7:" and "tenant-7" name the same keyspace rather than two that differ by an +// empty segment. +// +// An empty prefix is rejected: an unprefixed keyspace collides with Redis's own key +// namespace conventions and, more importantly, silently un-namespaces an install that +// meant to set the flag and passed the empty string. Use DefaultKeyPrefix to opt out. +func ValidateKeyPrefix(prefix string) (string, error) { + normalized := strings.TrimRight(strings.TrimSpace(prefix), ":") + if normalized == "" { + return "", fmt.Errorf("redis-key-prefix must be a non-empty identifier (default %q)", DefaultKeyPrefix) + } + if len(normalized) > maxKeyPrefixLength { + return "", fmt.Errorf("redis-key-prefix must be at most %d characters, got %d", + maxKeyPrefixLength, len(normalized)) + } + for _, r := range normalized { + switch { + case r >= 'a' && r <= 'z', r >= 'A' && r <= 'Z', r >= '0' && r <= '9': + case r == '-', r == '_', r == '.', r == ':': + default: + return "", fmt.Errorf( + "redis-key-prefix %q contains %q; allowed characters are [A-Za-z0-9], '-', '_', '.' and ':'", + prefix, string(r)) + } + } + return normalized, nil +} + +// resolveKeyPrefix normalizes a prefix for internal use, falling back to the default when +// empty. Construction paths that did not go through ValidateKeyPrefix (tests, and the +// zero-value RedisStoreConfig) land here rather than building keys with an empty root. +func resolveKeyPrefix(prefix string) string { + normalized := strings.TrimRight(strings.TrimSpace(prefix), ":") + if normalized == "" { + return DefaultKeyPrefix + } + return normalized +} diff --git a/internal/queue/key_prefix_test.go b/internal/queue/key_prefix_test.go new file mode 100644 index 00000000..44d1ae2f --- /dev/null +++ b/internal/queue/key_prefix_test.go @@ -0,0 +1,150 @@ +// SPDX-License-Identifier: Apache-2.0 + +package queue + +import ( + "context" + "strings" + "testing" + + "github.com/alicebob/miniredis/v2" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/runtime/schema" +) + +func TestValidateKeyPrefix(t *testing.T) { + t.Parallel() + + tests := map[string]struct { + in string + want string + wantErr string + }{ + "default": {in: DefaultKeyPrefix, want: "gitops-reverser"}, + "nested with colon": {in: "cell-a:tenant-7", want: "cell-a:tenant-7"}, + "trailing colon dropped": {in: "tenant-7:", want: "tenant-7"}, + "surrounding space": {in: " tenant-7 ", want: "tenant-7"}, + "dots and underscores": {in: "rev_1.example.com", want: "rev_1.example.com"}, + + "empty": {in: "", wantErr: "non-empty"}, + "only colons": {in: ":::", wantErr: "non-empty"}, + "glob star": {in: "tenant-*", wantErr: `contains "*"`}, + "glob question": {in: "tenant-?", wantErr: `contains "?"`}, + "glob bracket": {in: "tenant-[a]", wantErr: `contains "["`}, + "escape char": {in: `tenant\a`, wantErr: `contains "\\"`}, + "percent": {in: "tenant%3A", wantErr: `contains "%"`}, + "slash": {in: "tenant/a", wantErr: `contains "/"`}, + "space inside": {in: "tenant a", wantErr: `contains " "`}, + "too long": {in: strings.Repeat("a", maxKeyPrefixLength+1), wantErr: "at most 128 characters"}, + "exactly max long": { + in: strings.Repeat("a", maxKeyPrefixLength), + want: strings.Repeat("a", maxKeyPrefixLength), + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + got, err := ValidateKeyPrefix(tc.in) + if tc.wantErr != "" { + require.Error(t, err) + require.Contains(t, err.Error(), tc.wantErr) + return + } + require.NoError(t, err) + require.Equal(t, tc.want, got) + }) + } +} + +func TestResolveKeyPrefix_FallsBackToDefault(t *testing.T) { + t.Parallel() + // A hand-built store (tests, zero-value config) must never write to an unprefixed + // keyspace — that would be indistinguishable from another tool's keys. + require.Equal(t, DefaultKeyPrefix, resolveKeyPrefix("")) + require.Equal(t, DefaultKeyPrefix, resolveKeyPrefix(" ")) + require.Equal(t, "tenant-7", resolveKeyPrefix("tenant-7:")) +} + +// newPrefixedRedisStore builds a store on a fresh miniredis under an explicit prefix. +func newPrefixedRedisStore(t *testing.T, prefix string) *RedisStore { + t.Helper() + mr := miniredis.RunT(t) + store, err := NewRedisStore(RedisStoreConfig{Addr: mr.Addr(), KeyPrefix: prefix}) + require.NoError(t, err) + return store +} + +func TestRedisStore_KeyPrefixReachesEveryKeyFamily(t *testing.T) { + t.Parallel() + + store := newPrefixedRedisStore(t, "cell-a:tenant-7") + require.Equal(t, "cell-a:tenant-7", store.KeyPrefix()) + + gvr := schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"} + require.Equal(t, + "cell-a:tenant-7:watch:v1:target:gtuid-3:apps/deployments:namespace:team-a:last-rv", + store.watchCursorKey("gtuid-3", gvr, "team-a")) + + idx := store.AttributionIndex(0) + require.Equal(t, "cell-a:tenant-7:author:v1:audit:apps/deployments:object:uid-1:101", + idx.factKeyExact("apps/deployments", "uid-1", "101")) + require.Equal(t, "cell-a:tenant-7:author:v1:audit:apps/deployments:object:uid-1:last", + idx.factKeyLast("apps/deployments", "uid-1")) + require.Equal(t, "cell-a:tenant-7:author:v1:audit:apps/deployments:rv:101", + idx.factKeyRV("apps/deployments", "101")) + + require.Equal(t, "cell-a:tenant-7:author:v1:command:cr-uid", store.CommandAuthorStore().key("cr-uid")) +} + +func TestRedisStore_DefaultPrefixIsUnchanged(t *testing.T) { + t.Parallel() + + // The flag defaults to DefaultKeyPrefix, so an upgrade must not orphan the keys a + // previous release wrote. + store := newPrefixedRedisStore(t, DefaultKeyPrefix) + require.Equal(t, "gitops-reverser:watch:v1:target:gtuid-3:configmaps:cluster:last-rv", + store.watchCursorKey("gtuid-3", coreConfigmapsGVR(), "")) + require.Equal(t, "gitops-reverser:author:v1:command:cr-uid", store.CommandAuthorStore().key("cr-uid")) +} + +// Two reversers sharing one Redis/Valkey and one logical database must not read each +// other's cursors. Sixteen databases is the only separator Redis offers; a prefix is not +// bounded that way. +func TestRedisStore_DistinctPrefixesIsolateCursors(t *testing.T) { + t.Parallel() + + mr := miniredis.RunT(t) + ctx := context.Background() + gvr := coreConfigmapsGVR() + + tenantA, err := NewRedisStore(RedisStoreConfig{Addr: mr.Addr(), KeyPrefix: "tenant-a"}) + require.NoError(t, err) + tenantB, err := NewRedisStore(RedisStoreConfig{Addr: mr.Addr(), KeyPrefix: "tenant-b"}) + require.NoError(t, err) + + require.NoError(t, tenantA.RecordWatchCursor(ctx, "same-uid", gvr, "", "111")) + + rv, ok := tenantA.LookupWatchCursor(ctx, "same-uid", gvr, "") + require.True(t, ok) + require.Equal(t, "111", rv) + + _, ok = tenantB.LookupWatchCursor(ctx, "same-uid", gvr, "") + require.False(t, ok, "tenant-b must not see tenant-a's cursor on the same UID and database") + + require.NoError(t, tenantB.RecordWatchCursor(ctx, "same-uid", gvr, "", "222")) + rv, ok = tenantA.LookupWatchCursor(ctx, "same-uid", gvr, "") + require.True(t, ok) + require.Equal(t, "111", rv, "tenant-b's write must not clobber tenant-a's cursor") +} + +// The attribution telemetry gauge SCANs ":author:v1:audit:*". A prefix that +// contained a glob metacharacter would make it count the wrong keyspace; validation +// rejects those, so the pattern is always a literal prefix plus one trailing star. +func TestAttributionIndex_ScanPatternIsPrefixed(t *testing.T) { + t.Parallel() + + store := newPrefixedRedisStore(t, "tenant-a") + idx := store.AttributionIndex(0) + require.Equal(t, "tenant-a:author:v1:audit:", idx.factKeyBase("")) +} diff --git a/internal/queue/redis_store.go b/internal/queue/redis_store.go index 81e4b264..cd369ff7 100644 --- a/internal/queue/redis_store.go +++ b/internal/queue/redis_store.go @@ -34,6 +34,11 @@ type RedisStoreConfig struct { AuthValue string DB int TLSEnabled bool + // KeyPrefix is the root namespace for every key this connection's stores own. Empty + // means DefaultKeyPrefix. Several reversers can then share one Redis/Valkey without + // sharing a logical database — Redis offers only 16 of those, and --redis-db was the + // only separator this operator provided. + KeyPrefix string } // RedisStore is the required Redis/Valkey-backed store. It owns the connection and @@ -42,7 +47,8 @@ type RedisStoreConfig struct { // and knows nothing about attribution: author attribution is an optional layer built // on the same connection via AttributionIndex. type RedisStore struct { - client *redis.Client + client *redis.Client + keyPrefix string } // NewRedisStore opens the Redis/Valkey connection that backs the resume cursors. @@ -61,7 +67,13 @@ func NewRedisStore(cfg RedisStoreConfig) (*RedisStore, error) { options.TLSConfig = &tls.Config{MinVersion: tls.VersionTLS12} } - return &RedisStore{client: redis.NewClient(options)}, nil + return &RedisStore{client: redis.NewClient(options), keyPrefix: resolveKeyPrefix(cfg.KeyPrefix)}, nil +} + +// KeyPrefix returns the root namespace every key this store's family owns is written +// under. Reported at startup so an operator can confirm which keyspace a pod claims. +func (s *RedisStore) KeyPrefix() string { + return s.keyPrefix } // AttributionIndex builds the optional author-attribution fact index on this store's @@ -71,7 +83,7 @@ func (s *RedisStore) AttributionIndex(factTTL time.Duration) *AttributionIndex { if factTTL <= 0 { factTTL = DefaultAttributionFactTTL } - return &AttributionIndex{client: s.client, factTTL: factTTL} + return &AttributionIndex{client: s.client, factTTL: factTTL, keyPrefix: s.keyPrefix} } // CommandAuthorStore builds the command-authorship store on this connection. Wire it @@ -79,7 +91,7 @@ func (s *RedisStore) AttributionIndex(factTTL time.Duration) *AttributionIndex { // record lives in the same top-level author domain as audit facts but in the separate // command subfamily, with its own fixed cleanup TTL. func (s *RedisStore) CommandAuthorStore() *CommandAuthorStore { - return &CommandAuthorStore{client: s.client, ttl: commandAuthorRecordTTL} + return &CommandAuthorStore{client: s.client, ttl: commandAuthorRecordTTL, keyPrefix: s.keyPrefix} } // Ping checks liveness of the underlying Redis/Valkey connection. The readiness gate @@ -138,6 +150,6 @@ func (s *RedisStore) watchCursorKey( if namespace != "" { scope = "namespace:" + escapeKeyField(namespace) } - return keyPrefix + watchCursorKeySuffix + "target:" + escapeKeyField(gitTargetUID) + ":" + + return s.keyPrefix + watchCursorKeySuffix + "target:" + escapeKeyField(gitTargetUID) + ":" + groupResourceKey(gvr.Group, gvr.Resource) + ":" + scope + ":last-rv" } diff --git a/internal/watch/api_resource_catalog.go b/internal/watch/api_resource_catalog.go index 1b100061..12c76deb 100644 --- a/internal/watch/api_resource_catalog.go +++ b/internal/watch/api_resource_catalog.go @@ -101,6 +101,44 @@ func (c *APIResourceCatalog) Scan(sensitive types.SensitiveResourcePolicy) (type return scan, true } +// ServesWatchable reports whether the latest scan saw this exact group/version/resource +// served with both list and watch verbs. It is the precondition for opening an informer +// on a resource the operator does not require: an API server that does not aggregate does +// not serve apiregistration.k8s.io at all, and a blind informer on it retries forever. +// It deliberately ignores the allow/deny watch policy, which governs what the operator +// mirrors, not what it may inspect. Reports false before the first trusted scan. +func (c *APIResourceCatalog) ServesWatchable(gvr schema.GroupVersionResource) bool { + c.mu.RLock() + defer c.mu.RUnlock() + if !c.ready { + return false + } + for _, entry := range c.lastScan.Entries { + if entry.GVR != gvr { + continue + } + return hasVerbs(entry.Verbs, "list", "watch") + } + return false +} + +// hasVerbs reports whether every wanted verb is present in the sorted verb list. +func hasVerbs(verbs []string, wanted ...string) bool { + for _, want := range wanted { + found := false + for _, verb := range verbs { + if verb == want { + found = true + break + } + } + if !found { + return false + } + } + return true +} + // DegradedGroupVersions returns the group/versions the latest scan reported as // failed, sorted for stable logging. func (c *APIResourceCatalog) DegradedGroupVersions() []schema.GroupVersion { diff --git a/internal/watch/api_surface_triggers_test.go b/internal/watch/api_surface_triggers_test.go new file mode 100644 index 00000000..319806bc --- /dev/null +++ b/internal/watch/api_surface_triggers_test.go @@ -0,0 +1,161 @@ +// SPDX-License-Identifier: Apache-2.0 + +package watch + +import ( + "testing" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" +) + +// aggregatingDiscovery is the common test surface plus a served apiregistration.k8s.io/v1, +// i.e. an ordinary kube-apiserver rather than a control plane without an aggregation layer. +func aggregatingDiscovery() staticCatalogDiscovery { + disco := newCommonTestDiscovery() + disco.groups = append(disco.groups, testAPIGroup("apiregistration.k8s.io", "v1")) + disco.resources = append(disco.resources, &metav1.APIResourceList{ + GroupVersion: "apiregistration.k8s.io/v1", + APIResources: []metav1.APIResource{ + {Name: "apiservices", Kind: "APIService", Verbs: metav1.Verbs{"get", "list", "watch"}}, + }, + }) + return disco +} + +func TestAPIResourceCatalog_ServesWatchable(t *testing.T) { + t.Parallel() + + tests := map[string]struct { + discovery staticCatalogDiscovery + gvr schema.GroupVersionResource + want bool + }{ + "crd served on a stock apiserver": { + discovery: newCommonTestDiscovery(), + gvr: crdTriggerGVR(), + want: true, + }, + "apiservices absent without an aggregation layer": { + discovery: newCommonTestDiscovery(), + gvr: apiServiceTriggerGVR(), + want: false, + }, + "apiservices served when the group is aggregated": { + discovery: aggregatingDiscovery(), + gvr: apiServiceTriggerGVR(), + want: true, + }, + "served but not watchable": { + discovery: staticCatalogDiscovery{ + groups: []*metav1.APIGroup{testAPIGroup("apiregistration.k8s.io", "v1")}, + resources: []*metav1.APIResourceList{{ + GroupVersion: "apiregistration.k8s.io/v1", + APIResources: []metav1.APIResource{ + {Name: "apiservices", Kind: "APIService", Verbs: metav1.Verbs{"get", "list"}}, + }, + }}, + }, + gvr: apiServiceTriggerGVR(), + want: false, + }, + "unknown resource": { + discovery: newCommonTestDiscovery(), + gvr: schema.GroupVersionResource{Group: "nope.example.com", Version: "v1", Resource: "widgets"}, + want: false, + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + catalog := NewAPIResourceCatalog() + _, err := catalog.Refresh(tc.discovery) + require.NoError(t, err) + require.Equal(t, tc.want, catalog.ServesWatchable(tc.gvr)) + }) + } +} + +func TestAPIResourceCatalog_ServesWatchable_BeforeFirstScan(t *testing.T) { + t.Parallel() + // A catalog that has never accepted a trusted scan must not claim anything is + // watchable, or the manager would open an informer on an API surface it has not seen. + require.False(t, NewAPIResourceCatalog().ServesWatchable(crdTriggerGVR())) +} + +func TestSelectAPISurfaceTriggers_SkipsUnservedAPIServices(t *testing.T) { + t.Parallel() + + catalog := newCommonTestCatalog(t) + start, unserved := selectAPISurfaceTriggers(catalog, map[schema.GroupVersionResource]struct{}{}) + + require.Equal(t, []schema.GroupVersionResource{crdTriggerGVR()}, start, + "only the served CRD trigger should start") + require.Equal(t, []schema.GroupVersionResource{apiServiceTriggerGVR()}, unserved, + "apiservices must be reported unserved, not started blindly") +} + +func TestSelectAPISurfaceTriggers_StartsBothWhenAggregationIsServed(t *testing.T) { + t.Parallel() + + catalog := NewAPIResourceCatalog() + _, err := catalog.Refresh(aggregatingDiscovery()) + require.NoError(t, err) + + start, unserved := selectAPISurfaceTriggers(catalog, map[schema.GroupVersionResource]struct{}{}) + + require.ElementsMatch(t, apiSurfaceTriggerGVRs(), start) + require.Empty(t, unserved) +} + +func TestSelectAPISurfaceTriggers_NeverRestartsARunningInformer(t *testing.T) { + t.Parallel() + + catalog := NewAPIResourceCatalog() + _, err := catalog.Refresh(aggregatingDiscovery()) + require.NoError(t, err) + + started := map[schema.GroupVersionResource]struct{}{crdTriggerGVR(): {}} + start, unserved := selectAPISurfaceTriggers(catalog, started) + + require.Equal(t, []schema.GroupVersionResource{apiServiceTriggerGVR()}, start) + require.Empty(t, unserved) +} + +// An aggregation layer installed after startup must be picked up on the next catalog +// refresh, without restarting the operator. +func TestSelectAPISurfaceTriggers_PicksUpAggregationInstalledLater(t *testing.T) { + t.Parallel() + + catalog := newCommonTestCatalog(t) + started := map[schema.GroupVersionResource]struct{}{} + + start, unserved := selectAPISurfaceTriggers(catalog, started) + require.Equal(t, []schema.GroupVersionResource{crdTriggerGVR()}, start) + require.Equal(t, []schema.GroupVersionResource{apiServiceTriggerGVR()}, unserved) + for _, gvr := range start { + started[gvr] = struct{}{} + } + + _, err := catalog.Refresh(aggregatingDiscovery()) + require.NoError(t, err) + + start, unserved = selectAPISurfaceTriggers(catalog, started) + require.Equal(t, []schema.GroupVersionResource{apiServiceTriggerGVR()}, start) + require.Empty(t, unserved) +} + +func TestEnsureAPISurfaceTriggerInformers_NoOpBeforeStart(t *testing.T) { + t.Parallel() + + // triggerCtx is nil until Start runs. Calling in through a controller-driven catalog + // refresh before then must not create a factory bound to a dead context. + m := &Manager{Log: logr.Discard()} + m.ensureAPISurfaceTriggerInformers(m.Log) + + require.Nil(t, m.triggerFactory) + require.Empty(t, m.triggersStarted) +} diff --git a/internal/watch/manager.go b/internal/watch/manager.go index 352d4fd0..101d9bad 100644 --- a/internal/watch/manager.go +++ b/internal/watch/manager.go @@ -19,6 +19,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/watch" "k8s.io/client-go/dynamic" + "k8s.io/client-go/dynamic/dynamicinformer" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/event" @@ -96,6 +97,20 @@ type Manager struct { discoveryClient func() (apiResourceDiscovery, error) // catalogRefreshCh coalesces API-surface trigger watch events into manager reconciliation. catalogRefreshCh chan struct{} + // triggersMu guards the API-surface trigger informer set below. The informers are + // (re-)evaluated after every catalog refresh — which controllers drive, not just the + // manager's own loop — so this is not a startup-only structure. + triggersMu sync.Mutex + // triggerCtx is the manager's lifetime context, the stop channel every trigger informer + // is started with. Set once by Start; nil before then, which defers informer creation. + triggerCtx context.Context + // triggerFactory is the shared dynamic informer factory backing the trigger informers. + triggerFactory dynamicinformer.DynamicSharedInformerFactory + // triggersStarted is the set of trigger resources whose informer is already running. + triggersStarted map[schema.GroupVersionResource]struct{} + // triggersSkipLogged records which unserved trigger resources have already been logged, + // so a permanently absent aggregation layer produces one line, not one per refresh. + triggersSkipLogged map[schema.GroupVersionResource]struct{} // catalogReadyOnce guards the one-time "catalog ready" log line, matching the // firstMessage/firstGroupReady sync.Once pattern used by the audit consumer. catalogReadyOnce sync.Once @@ -180,6 +195,10 @@ func (m *Manager) Start(ctx context.Context) error { defer log.Info("watch ingestion manager stopping") m.initializeManagerState() + // Arm the trigger informers before the first refresh: each successful catalog refresh + // re-evaluates which triggers discovery actually serves, and starts the ones that + // became available. They are stopped by this context, never by a reconcile's. + m.setTriggerContext(ctx) if err := m.bootstrapRuleStore(ctx, log.WithName("bootstrap")); err != nil { log.Error(err, "RuleStore bootstrap failed, continuing with current in-memory state") @@ -189,7 +208,6 @@ func (m *Manager) Start(ctx context.Context) error { if err := m.ReconcileForRuleChange(ctx); err != nil { log.Error(err, "Initial reconciliation failed, will retry periodically") } - m.startAPISurfaceTriggerInformers(ctx, log.WithName("catalog-triggers")) // Periodic reconciliation for CRD detection and missed changes periodicTicker := time.NewTicker(periodicReconcileInterval) diff --git a/internal/watch/manager_catalog.go b/internal/watch/manager_catalog.go index b4a4323d..8b5cc2f7 100644 --- a/internal/watch/manager_catalog.go +++ b/internal/watch/manager_catalog.go @@ -72,6 +72,10 @@ func (m *Manager) RefreshAPIResourceCatalog(ctx context.Context) error { stats := catalog.Stats() recordCatalogStats(ctx, stats) m.logCatalogTransitions(catalog, stats) + // The fresh scan is the only source of truth for which trigger resources this API + // server actually serves, so trigger informers are (re-)armed here rather than once + // at startup. An aggregation layer installed later is picked up on its refresh. + m.ensureAPISurfaceTriggerInformers(m.Log.WithName("catalog-triggers")) } return refreshErr } @@ -360,37 +364,119 @@ func (m *Manager) signalCatalogRefresh() { } } -func (m *Manager) startAPISurfaceTriggerInformers(ctx context.Context, log logr.Logger) { - cfg := m.restConfig() - if cfg == nil { - log.V(1).Info("skipping API surface trigger informers - no REST config available") +// ensureAPISurfaceTriggerInformers starts the CRD and APIService trigger informers, but +// only for the resources discovery reports as served with list+watch. Neither is universal: +// an API server without an aggregation layer serves no apiregistration.k8s.io, and a blind +// informer on it makes client-go's reflector retry and log forever — benign, endlessly +// repeated noise that is exactly how a real error gets missed. +// +// It is idempotent and re-evaluated after every successful catalog refresh, so an +// aggregation layer (or the apiextensions group) installed later is picked up without a +// restart. Informers already started are never restarted, and a skip is logged once per +// resource, not once per refresh. +func (m *Manager) ensureAPISurfaceTriggerInformers(log logr.Logger) { + m.triggersMu.Lock() + defer m.triggersMu.Unlock() + + ctx := m.triggerCtx + if ctx == nil { + // Start has not run yet; the first refresh happens inside it and re-enters here. return } - dynamicClient, err := dynamic.NewForConfig(cfg) - if err != nil { - log.Error(err, "failed to create API surface trigger client") + catalog := m.apiResourceCatalog() + if !catalog.Ready() { + log.V(1).Info("deferring API surface trigger informers - catalog not ready") return } + if m.triggerFactory == nil { + cfg := m.restConfig() + if cfg == nil { + log.V(1).Info("skipping API surface trigger informers - no REST config available") + return + } + dynamicClient, err := dynamic.NewForConfig(cfg) + if err != nil { + log.Error(err, "failed to create API surface trigger client") + return + } + m.triggerFactory = dynamicinformer.NewDynamicSharedInformerFactory(dynamicClient, 0) + m.triggersStarted = map[schema.GroupVersionResource]struct{}{} + m.triggersSkipLogged = map[schema.GroupVersionResource]struct{}{} + } - factory := dynamicinformer.NewDynamicSharedInformerFactory(dynamicClient, 0) handler := cache.ResourceEventHandlerFuncs{ AddFunc: func(any) { m.signalCatalogRefresh() }, UpdateFunc: func(any, any) { m.signalCatalogRefresh() }, DeleteFunc: func(any) { m.signalCatalogRefresh() }, } - informers := []cache.SharedIndexInformer{ - factory.ForResource(crdTriggerGVR()).Informer(), - factory.ForResource(apiServiceTriggerGVR()).Informer(), + + start, unserved := selectAPISurfaceTriggers(catalog, m.triggersStarted) + for _, gvr := range unserved { + if _, logged := m.triggersSkipLogged[gvr]; logged { + continue + } + m.triggersSkipLogged[gvr] = struct{}{} + log.Info("API surface trigger not served by this API server; "+ + "the catalog refreshes on its periodic tick instead", "gvr", gvr.String()) } - for _, informer := range informers { + + var fresh []cache.SharedIndexInformer + for _, gvr := range start { + informer := m.triggerFactory.ForResource(gvr).Informer() if _, addErr := informer.AddEventHandler(handler); addErr != nil { - log.Error(addErr, "failed to add API surface trigger handler") - return + log.Error(addErr, "failed to add API surface trigger handler", "gvr", gvr.String()) + continue + } + m.triggersStarted[gvr] = struct{}{} + delete(m.triggersSkipLogged, gvr) + fresh = append(fresh, informer) + log.Info("API surface trigger informer started", "gvr", gvr.String()) + } + if len(fresh) == 0 { + return + } + + // Start is idempotent per informer: it launches only the ones not yet running. + m.triggerFactory.Start(ctx.Done()) + go waitForAPISurfaceTriggerSync(ctx, log, fresh) +} + +// apiSurfaceTriggerGVRs are the resources whose changes mean the API surface moved: a CRD +// (custom types appear/disappear) and an APIService (an aggregated group appears/goes +// unhealthy). Neither is guaranteed to be served. +func apiSurfaceTriggerGVRs() []schema.GroupVersionResource { + return []schema.GroupVersionResource{crdTriggerGVR(), apiServiceTriggerGVR()} +} + +// selectAPISurfaceTriggers splits the trigger resources not yet running into the ones +// discovery says are watchable now (start) and the ones it does not serve (unserved). It +// is the whole decision behind ensureAPISurfaceTriggerInformers, kept pure so the +// "no aggregation layer" case is testable without an API server. +func selectAPISurfaceTriggers( + catalog *APIResourceCatalog, + started map[schema.GroupVersionResource]struct{}, +) ([]schema.GroupVersionResource, []schema.GroupVersionResource) { + var start, unserved []schema.GroupVersionResource + for _, gvr := range apiSurfaceTriggerGVRs() { + if _, running := started[gvr]; running { + continue } + if catalog.ServesWatchable(gvr) { + start = append(start, gvr) + continue + } + unserved = append(unserved, gvr) } + return start, unserved +} - factory.Start(ctx.Done()) - go waitForAPISurfaceTriggerSync(ctx, log, informers) +// setTriggerContext records the manager's lifetime context, the stop channel every +// trigger informer is started with. Informers must outlive the reconcile call that +// discovers their resource became available, so they can never use its context. +func (m *Manager) setTriggerContext(ctx context.Context) { + m.triggersMu.Lock() + defer m.triggersMu.Unlock() + m.triggerCtx = ctx } func waitForAPISurfaceTriggerSync(ctx context.Context, log logr.Logger, informers []cache.SharedIndexInformer) { diff --git a/pkg/manifestanalyzer/doc.go b/pkg/manifestanalyzer/doc.go new file mode 100644 index 00000000..9190acad --- /dev/null +++ b/pkg/manifestanalyzer/doc.go @@ -0,0 +1,34 @@ +// SPDX-License-Identifier: Apache-2.0 + +// Package manifestanalyzer is the public, stable answer to two questions a tool built +// around GitOps Reverser needs to ask about a Git repository, without a cluster and +// without writing anything: +// +// ScanFolder — may this folder become a GitTarget, and if not, why? +// ScanRepo — which folders in this repository could, and what shape is each one? +// +// The decisions come from the same acceptance gate the operator's writer enforces before +// it commits a byte, so a tool built on this package cannot drift from the operator that +// will later refuse the folder. Nothing here re-implements a rule. +// +// # Stability +// +// This package is the supported surface. The types below carry JSON tags and a +// [SchemaVersion]; fields are added, never repurposed or removed within a schema major. +// Everything under internal/ is not covered by that promise and is not importable from +// another module. +// +// The command-line equivalents are `manifest-analyzer --mode scan-folder --format json` and +// `--mode scan-repo --format json`, which emit exactly the documents [FolderReport] +// and [RepoReport] marshal to. Exec the binary if Go is not your language; import this +// package if it is. +// +// # What it does not do +// +// Neither entry point resolves types against a live cluster, so neither reports whether a +// document's kind is actually served, nor produces a write plan. Both are structure-only: +// they read bytes, never follow symlinks, and never write. The operator applies the same +// gate plus the cluster-aware checks when a GitTarget adopts the folder — a folder this +// package accepts can still be refused for a reason only a cluster can see (an unresolved +// kind, an out-of-scope resource). +package manifestanalyzer diff --git a/pkg/manifestanalyzer/folder.go b/pkg/manifestanalyzer/folder.go new file mode 100644 index 00000000..56a9a1d7 --- /dev/null +++ b/pkg/manifestanalyzer/folder.go @@ -0,0 +1,188 @@ +// SPDX-License-Identifier: Apache-2.0 + +package manifestanalyzer + +import ( + "context" + "encoding/json" + "io" + "io/fs" + + internalanalyzer "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" +) + +// SchemaVersion identifies the JSON contract [FolderReport] and [RepoReport] marshal to. +// It bumps only on a breaking change — a field removed, renamed, or given a new meaning. +// Adding a field is not breaking, so consumers must ignore fields they do not know. +const SchemaVersion = "v1" + +// IssueKind classifies why a folder was not accepted. The values are the operator's own +// refusal codes, stable across releases. +type IssueKind string + +const ( + // IssueDuplicate marks a document that duplicates an earlier manifest identity. + IssueDuplicate IssueKind = "duplicate-identity" + // IssueNonKRM marks YAML that does not parse as a Kubernetes manifest. + IssueNonKRM IssueKind = "non-krm-yaml" + // IssueInvalidYAML marks a document that does not parse as YAML. + IssueInvalidYAML IssueKind = "invalid-yaml" + // IssueImpureManagedFile marks a file holding managed resources that also holds a + // non-managed document. A managed file may contain only valid KRM documents. + IssueImpureManagedFile IssueKind = "impure-managed-file" + // IssueMixedFile marks a managed file that also holds an allowlisted non-API KRM + // document (a kustomization). Allowlisted KRM must be retained in its own file. + IssueMixedFile IssueKind = "mixed-managed-allowlisted" + // IssueUnresolvedKRM marks recognized KRM that cannot be tied to a single served, + // followable resource. Only a cluster-aware scan reports this. + IssueUnresolvedKRM IssueKind = "unresolved-krm" + // IssueOutOfScope marks a watched kind whose resource falls outside the GitTarget's + // scope (right kind, wrong namespace). Only a cluster-aware scan reports this. + IssueOutOfScope IssueKind = "out-of-scope" + // IssueUnsupportedKustomize marks a kustomization.yaml using a feature the writer + // cannot map back to editable source documents (generators, patches, components, + // Helm inflation, replacements, transformers, name prefixes, remote bases). + IssueUnsupportedKustomize IssueKind = "unsupported-kustomize" + // IssueForeignFile marks a non-YAML regular file the operator cannot manage. + IssueForeignFile IssueKind = "foreign-file" + // IssueForeignSymlink marks a symlink, which a writer could follow out of the subtree. + IssueForeignSymlink IssueKind = "foreign-symlink" + // IssueForeignSubmodule marks a nested Git submodule. + IssueForeignSubmodule IssueKind = "foreign-submodule" + // IssueIgnoreShadowsManaged marks a .gittargetignore pattern that matches a path the + // operator writes, which would blind it to its own file. + IssueIgnoreShadowsManaged IssueKind = "ignore-shadows-managed" + // IssueWriteEscapesScope marks a planned write that would leave the GitTarget's path. + IssueWriteEscapesScope IssueKind = "write-escapes-scope" + // IssueWriteFanIn marks an in-place edit of a source file that more than one kustomize + // render root reaches. + IssueWriteFanIn IssueKind = "write-fan-in" +) + +// Issue is one reason a folder is not accepted, or one fact a stricter policy may treat +// as blocking. +type Issue struct { + Kind IssueKind `json:"kind"` + // Path is the offending file, slash-separated and relative to the scan root. Empty + // when the issue is about the folder as a whole. + Path string `json:"path"` + // DocumentIndex is the zero-based index of the offending document within Path. + DocumentIndex int `json:"documentIndex"` + // Message is a human-readable explanation. It is not a stable string. + Message string `json:"message"` +} + +// Identity names one Kubernetes document as it appears in the file. +type Identity struct { + APIVersion string `json:"apiVersion"` + Kind string `json:"kind"` + Namespace string `json:"namespace"` + Name string `json:"name"` +} + +// RetainedDocument is content the operator keeps but never writes: build directives such +// as a kustomization.yaml. They are read as context, never edited as resources. +type RetainedDocument struct { + Path string `json:"path"` + DocumentIndex int `json:"documentIndex"` + // Identity is set only when a named Kubernetes resource is hiding inside an + // allowlisted build-directive file — the mixed-file case, which is refused. It is nil + // for the ordinary whole-file retention of a kustomization.yaml, which names no + // resource. + Identity *Identity `json:"identity,omitempty"` + // Unsupported reports that this retained file uses a construct the writer cannot map + // back to editable source. It is the cause of an IssueUnsupportedKustomize. + Unsupported bool `json:"unsupported,omitempty"` +} + +// FolderReport answers "may this folder become a GitTarget?". +type FolderReport struct { + SchemaVersion string `json:"schemaVersion"` + // Root is the scanned folder as passed to ScanFolder. Informational; empty for + // ScanFolderFS. + Root string `json:"root,omitempty"` + // Accepted is the gate decision. When false, Issues says why. + Accepted bool `json:"accepted"` + // Issues is empty when Accepted, and never nil in the marshaled JSON. + Issues []Issue `json:"issues"` + // Retained lists the build directives read as context. + Retained []RetainedDocument `json:"retained,omitempty"` +} + +// ScanFolder runs the adoption gate over a folder on disk. It is read-only, needs no +// cluster, and never follows symlinks. +// +// The returned error covers only I/O: a folder that cannot be adopted is a successful +// scan with Accepted=false, not an error. +func ScanFolder(ctx context.Context, root string) (FolderReport, error) { + result, err := internalanalyzer.ScanDir(ctx, root, nil, nil, folderScanPolicy()) + if err != nil { + return FolderReport{}, err + } + report := folderReportFrom(result.Acceptance) + report.Root = root + return report, nil +} + +// ScanFolderFS is ScanFolder over an arbitrary [io/fs.FS] — an in-memory tree, a tarball, +// or a Git tree exposed as a filesystem. It cannot fail: an unreadable file becomes an +// issue, not an error. +func ScanFolderFS(ctx context.Context, fsys fs.FS) FolderReport { + return folderReportFrom(internalanalyzer.Scan(ctx, fsys, nil, nil, folderScanPolicy()).Acceptance) +} + +// WriteJSON writes the report as indented JSON — byte-for-byte what +// `manifest-analyzer --mode scan-folder --format json` prints. +func (r FolderReport) WriteJSON(w io.Writer) error { + if r.Issues == nil { + r.Issues = []Issue{} + } + enc := json.NewEncoder(w) + enc.SetIndent("", " ") + return enc.Encode(r) +} + +// folderScanPolicy is the structure-only adoption gate: the default allowlist, no +// followability lookup, and no desired state. It is the same policy the CLI's scan-folder mode +// applies, kept in one place so the two can never disagree. +func folderScanPolicy() internalanalyzer.ScanPolicy { + return internalanalyzer.ScanPolicy{ + Acceptance: internalanalyzer.AcceptancePolicy{Allowlist: internalanalyzer.DefaultAllowlist()}, + } +} + +// folderReportFrom projects the internal acceptance decision onto the public contract. +// It is deliberately a copy rather than a type alias: the public shape must be free to +// stay still while the internal one moves. +func folderReportFrom(acc internalanalyzer.Acceptance) FolderReport { + report := FolderReport{ + SchemaVersion: SchemaVersion, + Accepted: acc.Accepted, + Issues: make([]Issue, 0, len(acc.Issues)), + } + for _, issue := range acc.Issues { + report.Issues = append(report.Issues, Issue{ + Kind: IssueKind(issue.Kind), + Path: issue.Path, + DocumentIndex: issue.DocumentIndex, + Message: issue.Message, + }) + } + for _, rd := range acc.Retained { + doc := RetainedDocument{ + Path: rd.Location.Path, + DocumentIndex: rd.Location.DocumentIndex, + Unsupported: rd.Unsupported, + } + if rd.Identity.Kind != "" { + doc.Identity = &Identity{ + APIVersion: rd.Identity.APIVersion, + Kind: rd.Identity.Kind, + Namespace: rd.Identity.Namespace, + Name: rd.Identity.Name, + } + } + report.Retained = append(report.Retained, doc) + } + return report +} diff --git a/pkg/manifestanalyzer/folder_test.go b/pkg/manifestanalyzer/folder_test.go new file mode 100644 index 00000000..fb343095 --- /dev/null +++ b/pkg/manifestanalyzer/folder_test.go @@ -0,0 +1,176 @@ +// SPDX-License-Identifier: Apache-2.0 + +package manifestanalyzer_test + +import ( + "bytes" + "context" + "encoding/json" + "os" + "path/filepath" + "testing" + "testing/fstest" + + "github.com/stretchr/testify/require" + + "github.com/ConfigButler/gitops-reverser/pkg/manifestanalyzer" +) + +const configMapYAML = `apiVersion: v1 +kind: ConfigMap +metadata: + name: settings + namespace: demo +data: + key: value +` + +const kustomizationHelmYAML = `apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization +helmCharts: + - name: podinfo + repo: https://stefanprodan.github.io/podinfo +` + +func TestScanFolder_AcceptsPlainKRM(t *testing.T) { + t.Parallel() + + root := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(root, "cm.yaml"), []byte(configMapYAML), 0o600)) + + report, err := manifestanalyzer.ScanFolder(context.Background(), root) + require.NoError(t, err) + + require.True(t, report.Accepted) + require.Empty(t, report.Issues) + require.Equal(t, manifestanalyzer.SchemaVersion, report.SchemaVersion) + require.Equal(t, root, report.Root) +} + +// A folder the operator would refuse must come back as a successful scan with +// Accepted=false. Refusal is a verdict, never an error. +func TestScanFolder_RefusalIsNotAnError(t *testing.T) { + t.Parallel() + + root := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(root, "cm.yaml"), []byte(configMapYAML), 0o600)) + require.NoError(t, os.WriteFile(filepath.Join(root, "kustomization.yaml"), []byte(kustomizationHelmYAML), 0o600)) + + report, err := manifestanalyzer.ScanFolder(context.Background(), root) + require.NoError(t, err) + + require.False(t, report.Accepted) + require.NotEmpty(t, report.Issues) + kinds := make([]manifestanalyzer.IssueKind, 0, len(report.Issues)) + for _, issue := range report.Issues { + kinds = append(kinds, issue.Kind) + } + require.Contains(t, kinds, manifestanalyzer.IssueUnsupportedKustomize, + "Helm inflation is the permanent support boundary and must be named as such") +} + +func TestScanFolder_MissingDirIsAnError(t *testing.T) { + t.Parallel() + _, err := manifestanalyzer.ScanFolder(context.Background(), filepath.Join(t.TempDir(), "absent")) + require.Error(t, err) +} + +func TestScanFolderFS_ReportsDuplicateIdentity(t *testing.T) { + t.Parallel() + + fsys := fstest.MapFS{ + "a.yaml": {Data: []byte(configMapYAML)}, + "b.yaml": {Data: []byte(configMapYAML)}, + } + report := manifestanalyzer.ScanFolderFS(context.Background(), fsys) + + require.False(t, report.Accepted) + require.Equal(t, manifestanalyzer.SchemaVersion, report.SchemaVersion) + require.Empty(t, report.Root, "an fs.FS has no path to report") + + var found bool + for _, issue := range report.Issues { + if issue.Kind == manifestanalyzer.IssueDuplicate { + found = true + require.NotEmpty(t, issue.Path) + require.NotEmpty(t, issue.Message) + } + } + require.True(t, found, "two documents with the same identity must be refused as duplicates") +} + +// The retained set names the build directives the operator reads but never writes. A +// consumer relies on it to explain "we understand this kustomization; we will not edit it". +func TestScanFolder_ReportsRetainedKustomization(t *testing.T) { + t.Parallel() + + root := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(root, "cm.yaml"), []byte(configMapYAML), 0o600)) + require.NoError(t, os.WriteFile(filepath.Join(root, "kustomization.yaml"), + []byte("apiVersion: kustomize.config.k8s.io/v1beta1\nkind: Kustomization\nresources:\n - cm.yaml\n"), 0o600)) + + report, err := manifestanalyzer.ScanFolder(context.Background(), root) + require.NoError(t, err) + require.True(t, report.Accepted) + require.Len(t, report.Retained, 1) + require.Equal(t, "kustomization.yaml", report.Retained[0].Path) + require.False(t, report.Retained[0].Unsupported) + require.Nil(t, report.Retained[0].Identity, + "a whole-file retention names no resource; only the refused mixed-file case does") +} + +// A kustomization whose feature set the writer cannot map back to source is retained and +// flagged, so a consumer can point at the exact file that made the folder unadoptable. +func TestScanFolder_FlagsUnsupportedRetainedKustomization(t *testing.T) { + t.Parallel() + + root := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(root, "kustomization.yaml"), []byte(kustomizationHelmYAML), 0o600)) + + report, err := manifestanalyzer.ScanFolder(context.Background(), root) + require.NoError(t, err) + require.False(t, report.Accepted) + require.Len(t, report.Retained, 1) + require.True(t, report.Retained[0].Unsupported) +} + +// The JSON document is the published contract: schemaVersion is always present, and +// issues is an empty array rather than null so a consumer can iterate it unconditionally. +func TestFolderReport_WriteJSON_Contract(t *testing.T) { + t.Parallel() + + root := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(root, "cm.yaml"), []byte(configMapYAML), 0o600)) + report, err := manifestanalyzer.ScanFolder(context.Background(), root) + require.NoError(t, err) + + var buf bytes.Buffer + require.NoError(t, report.WriteJSON(&buf)) + + require.Contains(t, buf.String(), `"issues": []`, "issues must marshal as [] and never null") + + var raw map[string]any + require.NoError(t, json.Unmarshal(buf.Bytes(), &raw)) + require.Equal(t, "v1", raw["schemaVersion"]) + require.Equal(t, true, raw["accepted"]) + require.NotContains(t, raw, "retained", "an empty retained set is omitted") +} + +// The public report must survive a marshal/unmarshal round trip unchanged, or a consumer +// storing it and reading it back would see a different verdict than the one produced. +func TestFolderReport_JSONRoundTrip(t *testing.T) { + t.Parallel() + + root := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(root, "cm.yaml"), []byte(configMapYAML), 0o600)) + require.NoError(t, os.WriteFile(filepath.Join(root, "kustomization.yaml"), []byte(kustomizationHelmYAML), 0o600)) + report, err := manifestanalyzer.ScanFolder(context.Background(), root) + require.NoError(t, err) + + var buf bytes.Buffer + require.NoError(t, report.WriteJSON(&buf)) + + var decoded manifestanalyzer.FolderReport + require.NoError(t, json.Unmarshal(buf.Bytes(), &decoded)) + require.Equal(t, report, decoded) +} diff --git a/pkg/manifestanalyzer/repo.go b/pkg/manifestanalyzer/repo.go new file mode 100644 index 00000000..37de0de8 --- /dev/null +++ b/pkg/manifestanalyzer/repo.go @@ -0,0 +1,179 @@ +// SPDX-License-Identifier: Apache-2.0 + +package manifestanalyzer + +import ( + "context" + "encoding/json" + "io" + + internalanalyzer "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" +) + +// Layout is the structural shape of a candidate folder. +type Layout string + +const ( + // LayoutPlain is a directory of raw KRM documents with explicit namespaces and no + // kustomization. Accepted. + LayoutPlain Layout = "plain" + // LayoutKustomizeSingle is a self-contained render root: one kustomization whose + // resources graph stays within its own subtree. Accepted. + LayoutKustomizeSingle Layout = "kustomize-single" + // LayoutKustomizeOverlay is a render root reaching a base outside its own subtree + // (the classic base/ + overlays/{env} shape). Refused today, with a forward-looking + // reason: it becomes accepted when render-root scoping ships. + LayoutKustomizeOverlay Layout = "kustomize-overlay" + // LayoutRefusedStructural is a render root whose kustomization uses a construct the + // writer cannot map back to editable source. This is the permanent support boundary, + // never a "not yet". + LayoutRefusedStructural Layout = "refused-structural" +) + +// Refusal reason codes. The distinction is load-bearing and a consumer must not collapse +// it: one is a "not yet", the other is permanent. +const ( + // ReasonOverlayFanOutNeedsF2 is the forward-looking refusal that flips to accepted + // when render-root scoping ships. + ReasonOverlayFanOutNeedsF2 = "overlay-fan-out-needs-f2" + // ReasonRefusedStructural is the permanent support boundary. + ReasonRefusedStructural = "refused-structural" +) + +// RefusalReason is one machine-readable reason a candidate is not accepted. +type RefusalReason struct { + Code string `json:"code"` + // Detail is human-readable and not a stable string. + Detail string `json:"detail"` +} + +// ResourceCounts splits the KRM a candidate covers into what it renders versus what it +// could actually edit. The two are equal for a plain or self-contained candidate and +// diverge for an overlay, which renders documents it cannot own. +type ResourceCounts struct { + Rendered int `json:"rendered"` + Editable int `json:"editable"` + NonKRM int `json:"nonKrm"` +} + +// Candidate is one folder that could become a GitTarget. +type Candidate struct { + // Path is slash-separated and relative to the repository root. + Path string `json:"path"` + Layout Layout `json:"layout"` + // AcceptedByOperator reports whether the operator would adopt this folder today. + AcceptedByOperator bool `json:"acceptedByOperator"` + RefusalReasons []RefusalReason `json:"refusalReasons,omitempty"` + // RenderRoot reports whether the candidate is a kustomize render root. + RenderRoot bool `json:"renderRoot"` + // ReadScope lists base directories outside this candidate's subtree that its + // kustomization reads. Empty for plain and self-contained candidates. + ReadScope []string `json:"readScope,omitempty"` + // InferredNamespace is the namespace the candidate resolves to, when unambiguous. + InferredNamespace string `json:"inferredNamespace,omitempty"` + Resources ResourceCounts `json:"resources"` + // OverlapsWith lists candidate paths this one nests with. Two overlapping candidates + // can never both become GitTargets — a folder has exactly one owner. + OverlapsWith []string `json:"overlapsWith,omitempty"` +} + +// OverlapConflict records that Ancestor strictly contains Descendant. +type OverlapConflict struct { + Ancestor string `json:"ancestor"` + Descendant string `json:"descendant"` +} + +// RepoSummary is the repository-level roll-up. +type RepoSummary struct { + CandidatesByLayout map[Layout]int `json:"candidatesByLayout"` + Accepted int `json:"accepted"` + Refused int `json:"refused"` + OverlapConflicts []OverlapConflict `json:"overlapConflicts,omitempty"` + // FleetRoot reports that the repository root is a cluster/fleet root. A GitTarget + // points at an app subtree, never at such a root. + FleetRoot bool `json:"fleetRoot,omitempty"` + // UnsupportedConstructs is the sorted, de-duplicated set of unsupported kustomize + // features seen across refused candidates, so a tool can say "this repository uses + // Helm inflation, which the operator does not manage". + UnsupportedConstructs []string `json:"unsupportedConstructs,omitempty"` +} + +// RepoReport answers "which folders in this repository could become GitTargets?". +type RepoReport struct { + SchemaVersion string `json:"schemaVersion"` + // Root is the scanned repository root as passed to ScanRepo. Informational. + Root string `json:"root,omitempty"` + Candidates []Candidate `json:"candidates"` + Summary RepoSummary `json:"summary"` +} + +// ScanRepo walks a whole repository and enumerates the folders that could become +// GitTargets, classifying each one's layout and reporting why a folder is refused. It is +// read-only, needs no cluster, and never follows symlinks. +func ScanRepo(ctx context.Context, root string) (RepoReport, error) { + rep, err := internalanalyzer.ScanRepo(ctx, root) + if err != nil { + return RepoReport{}, err + } + return repoReportFrom(rep), nil +} + +// WriteJSON writes the report as indented JSON — byte-for-byte what +// `manifest-analyzer --mode scan-repo --format json` prints. +func (r RepoReport) WriteJSON(w io.Writer) error { + if r.Candidates == nil { + r.Candidates = []Candidate{} + } + enc := json.NewEncoder(w) + enc.SetIndent("", " ") + return enc.Encode(r) +} + +// repoReportFrom projects the internal discovery report onto the public contract. +func repoReportFrom(rep internalanalyzer.RepoReport) RepoReport { + out := RepoReport{ + SchemaVersion: SchemaVersion, + Root: rep.Root, + Candidates: make([]Candidate, 0, len(rep.Candidates)), + Summary: RepoSummary{ + CandidatesByLayout: make(map[Layout]int, len(rep.Summary.CandidatesByLayout)), + Accepted: rep.Summary.Accepted, + Refused: rep.Summary.Refused, + FleetRoot: rep.Summary.FleetRoot, + UnsupportedConstructs: rep.Summary.UnsupportedConstructs, + }, + } + for layout, n := range rep.Summary.CandidatesByLayout { + out.Summary.CandidatesByLayout[Layout(layout)] = n + } + for _, conflict := range rep.Summary.OverlapConflicts { + out.Summary.OverlapConflicts = append(out.Summary.OverlapConflicts, OverlapConflict{ + Ancestor: conflict.Ancestor, Descendant: conflict.Descendant, + }) + } + for _, cand := range rep.Candidates { + out.Candidates = append(out.Candidates, candidateFrom(cand)) + } + return out +} + +func candidateFrom(cand internalanalyzer.RepoCandidate) Candidate { + out := Candidate{ + Path: cand.Path, + Layout: Layout(cand.Layout), + AcceptedByOperator: cand.AcceptedByOperator, + RenderRoot: cand.RenderRoot, + ReadScope: cand.ReadScope, + InferredNamespace: cand.InferredNamespace, + Resources: ResourceCounts{ + Rendered: cand.Resources.Rendered, + Editable: cand.Resources.Editable, + NonKRM: cand.Resources.NonKRM, + }, + OverlapsWith: cand.OverlapsWith, + } + for _, reason := range cand.RefusalReasons { + out.RefusalReasons = append(out.RefusalReasons, RefusalReason{Code: reason.Code, Detail: reason.Detail}) + } + return out +} diff --git a/pkg/manifestanalyzer/repo_test.go b/pkg/manifestanalyzer/repo_test.go new file mode 100644 index 00000000..7550eb96 --- /dev/null +++ b/pkg/manifestanalyzer/repo_test.go @@ -0,0 +1,168 @@ +// SPDX-License-Identifier: Apache-2.0 + +package manifestanalyzer_test + +import ( + "bytes" + "context" + "encoding/json" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/ConfigButler/gitops-reverser/pkg/manifestanalyzer" +) + +// corpusRoot is the discovery corpus the engine's own golden tests drive. The public +// package reads the same fixtures rather than growing a second copy: the point of this +// package is that it cannot disagree with the engine, and a duplicated corpus is the +// first way that guarantee rots. Tests run with the package directory as cwd. +const corpusRoot = "../../internal/manifestanalyzer/testdata/scan-repo" + +func fixture(t *testing.T, group, name string) string { + t.Helper() + path := filepath.Join(corpusRoot, group, name) + info, err := os.Stat(path) + require.NoError(t, err, "discovery corpus fixture %s must exist", path) + require.True(t, info.IsDir()) + return path +} + +func TestScanRepo_PlainPerEnvironmentFoldersAreCandidates(t *testing.T) { + t.Parallel() + + report, err := manifestanalyzer.ScanRepo(context.Background(), fixture(t, "supported", "plain-per-env")) + require.NoError(t, err) + + require.Equal(t, manifestanalyzer.SchemaVersion, report.SchemaVersion) + require.NotEmpty(t, report.Candidates) + for _, cand := range report.Candidates { + require.Equal(t, manifestanalyzer.LayoutPlain, cand.Layout) + require.True(t, cand.AcceptedByOperator, "plain KRM folders are the launch layout") + require.Empty(t, cand.RefusalReasons) + require.False(t, cand.RenderRoot) + } + require.Equal(t, len(report.Candidates), report.Summary.Accepted) + require.Zero(t, report.Summary.Refused) +} + +// The two refusal codes mean different things and a consumer must be able to tell them +// apart: one is "not yet, when render-root scoping ships", the other is the permanent +// support boundary. Collapsing them would mislabel an onboardable repository as hopeless. +func TestScanRepo_RefusalCodesStayDistinct(t *testing.T) { + t.Parallel() + + tests := map[string]struct { + group, name string + wantCode string + wantLayout manifestanalyzer.Layout + }{ + "overlay fan-out is a not-yet": { + group: "unsupported", name: "base-overlays", + wantCode: manifestanalyzer.ReasonOverlayFanOutNeedsF2, + wantLayout: manifestanalyzer.LayoutKustomizeOverlay, + }, + "helm inflation is permanent": { + group: "unsupported", name: "helm-inflation", + wantCode: manifestanalyzer.ReasonRefusedStructural, + wantLayout: manifestanalyzer.LayoutRefusedStructural, + }, + "unsupported kustomize is permanent": { + group: "unsupported", name: "unsupported-kustomize", + wantCode: manifestanalyzer.ReasonRefusedStructural, + wantLayout: manifestanalyzer.LayoutRefusedStructural, + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + report, err := manifestanalyzer.ScanRepo(context.Background(), fixture(t, tc.group, tc.name)) + require.NoError(t, err) + + var seen bool + for _, cand := range report.Candidates { + if cand.Layout != tc.wantLayout { + continue + } + seen = true + require.False(t, cand.AcceptedByOperator) + codes := make([]string, 0, len(cand.RefusalReasons)) + for _, reason := range cand.RefusalReasons { + codes = append(codes, reason.Code) + } + require.Contains(t, codes, tc.wantCode) + } + require.True(t, seen, "expected at least one %s candidate", tc.wantLayout) + }) + } +} + +func TestScanRepo_ReportsOverlapConflicts(t *testing.T) { + t.Parallel() + + report, err := manifestanalyzer.ScanRepo(context.Background(), fixture(t, "supported", "overlapping")) + require.NoError(t, err) + + require.NotEmpty(t, report.Summary.OverlapConflicts, + "two nested candidates can never both own a folder; the conflict must surface") + for _, conflict := range report.Summary.OverlapConflicts { + require.NotEmpty(t, conflict.Ancestor) + require.NotEmpty(t, conflict.Descendant) + require.NotEqual(t, conflict.Ancestor, conflict.Descendant) + } +} + +func TestScanRepo_FleetRootIsNeverACandidate(t *testing.T) { + t.Parallel() + + root := fixture(t, "supported", "fleet-root") + report, err := manifestanalyzer.ScanRepo(context.Background(), root) + require.NoError(t, err) + + require.True(t, report.Summary.FleetRoot) + for _, cand := range report.Candidates { + require.NotEqual(t, ".", cand.Path, "a GitTarget points at an app subtree, never the fleet root") + } +} + +func TestScanRepo_MissingRootIsAnError(t *testing.T) { + t.Parallel() + _, err := manifestanalyzer.ScanRepo(context.Background(), filepath.Join(t.TempDir(), "absent")) + require.Error(t, err) +} + +func TestRepoReport_WriteJSON_Contract(t *testing.T) { + t.Parallel() + + report, err := manifestanalyzer.ScanRepo(context.Background(), fixture(t, "supported", "kustomize-single")) + require.NoError(t, err) + + var buf bytes.Buffer + require.NoError(t, report.WriteJSON(&buf)) + + var raw map[string]any + require.NoError(t, json.Unmarshal(buf.Bytes(), &raw)) + require.Equal(t, "v1", raw["schemaVersion"]) + require.Contains(t, raw, "candidates") + require.Contains(t, raw, "summary") + + var decoded manifestanalyzer.RepoReport + require.NoError(t, json.Unmarshal(buf.Bytes(), &decoded)) + require.Equal(t, report, decoded) +} + +// An empty repository still marshals candidates as an array, so a consumer can iterate it +// without a nil check. +func TestRepoReport_WriteJSON_EmptyCandidatesIsArray(t *testing.T) { + t.Parallel() + + report, err := manifestanalyzer.ScanRepo(context.Background(), t.TempDir()) + require.NoError(t, err) + + var buf bytes.Buffer + require.NoError(t, report.WriteJSON(&buf)) + require.Contains(t, buf.String(), `"candidates": []`) +}