From 5cfa934f027416928dab63ed78708161557a8ce8 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Mon, 6 Jul 2026 07:19:53 +0000 Subject: [PATCH 1/2] feat(kustomize): edit images/replicas overrides through to kustomization.yaml A live change produced by a kustomization's images:/replicas: entry is now written back to that entry instead of into the source manifest ("the edit lands where the value lives"): - the analyzer parses images:/replicas: entries and attaches an unambiguous override chain per document, walking true render roots (a referenced base composes with its parent instead of counting as its own root); distinct chains from multiple roots attach nothing and emit an ambiguous-kustomize-overrides diagnostic - a projection inverts the transformer chain per component (image name / tag / digest, replica count), routes each divergence to its supplier (entry or source file), and verifies by re-simulating the render; anything unsafe falls back to today's write-through - manifestedit gains a bounded, comment-preserving kustomization editor that only updates existing fields on existing entries, all-or-nothing - patchExisting runs the projection (covering live events and resync), and /scale field patches route a governed spec.replicas to the entry - live state equal to the overlay's render is now a true no-op: the source file's shadowed tag is no longer overwritten on every event/resync - malformed images:/replicas: values now refuse the folder (they would fail kustomize build; we can no longer vouch for the render) Adds docs/design/gitops-api/ (workstream tracker + F1 design, including the shared-entry divergent-consumers limitation), corpus fixtures pinning the supported/ambiguous boundary, and tests across all three layers. Validation: task lint (0 issues), task test (coverage 73.9% -> 74.4%, baseline raised), task test-e2e (53 passed, 0 failed, 8 skipped). Co-Authored-By: Claude Fable 5 --- .coverage-baseline | 2 +- docs/architecture.md | 11 +- docs/design/gitops-api/README.md | 73 +++ .../f1-images-replicas-edit-through.md | 196 ++++++++ internal/git/inplace_overrides_test.go | 230 +++++++++ internal/git/manifestedit/kustomization.go | 128 +++++ .../git/manifestedit/kustomization_test.go | 140 ++++++ internal/git/manifestedit/split.go | 12 + internal/git/plan_flush.go | 139 +++++- internal/manifestanalyzer/acceptance.go | 3 +- internal/manifestanalyzer/overrides.go | 357 ++++++++++++++ .../manifestanalyzer/overrides_projection.go | 465 ++++++++++++++++++ .../overrides_projection_test.go | 289 +++++++++++ internal/manifestanalyzer/overrides_test.go | 141 ++++++ internal/manifestanalyzer/store.go | 53 +- .../testdata/contextual-namespace/README.md | 4 + .../supported/images-overlay/deployment.yaml | 17 + .../images-overlay/kustomization.yaml | 8 + .../replicas-overlay/base/deployment.yaml | 16 + .../replicas-overlay/base/kustomization.yaml | 5 + .../replicas-overlay/kustomization.yaml | 7 + .../ambiguous-images/kustomization.yaml | 7 + .../ambiguous-images/other/kustomization.yaml | 5 + .../unsupported/ambiguous-images/shared.yaml | 17 + 24 files changed, 2304 insertions(+), 21 deletions(-) create mode 100644 docs/design/gitops-api/README.md create mode 100644 docs/design/gitops-api/f1-images-replicas-edit-through.md create mode 100644 internal/git/inplace_overrides_test.go create mode 100644 internal/git/manifestedit/kustomization.go create mode 100644 internal/git/manifestedit/kustomization_test.go create mode 100644 internal/manifestanalyzer/overrides.go create mode 100644 internal/manifestanalyzer/overrides_projection.go create mode 100644 internal/manifestanalyzer/overrides_projection_test.go create mode 100644 internal/manifestanalyzer/overrides_test.go create mode 100644 internal/manifestanalyzer/testdata/contextual-namespace/supported/images-overlay/deployment.yaml create mode 100644 internal/manifestanalyzer/testdata/contextual-namespace/supported/images-overlay/kustomization.yaml create mode 100644 internal/manifestanalyzer/testdata/contextual-namespace/supported/replicas-overlay/base/deployment.yaml create mode 100644 internal/manifestanalyzer/testdata/contextual-namespace/supported/replicas-overlay/base/kustomization.yaml create mode 100644 internal/manifestanalyzer/testdata/contextual-namespace/supported/replicas-overlay/kustomization.yaml create mode 100644 internal/manifestanalyzer/testdata/contextual-namespace/unsupported/ambiguous-images/kustomization.yaml create mode 100644 internal/manifestanalyzer/testdata/contextual-namespace/unsupported/ambiguous-images/other/kustomization.yaml create mode 100644 internal/manifestanalyzer/testdata/contextual-namespace/unsupported/ambiguous-images/shared.yaml diff --git a/.coverage-baseline b/.coverage-baseline index bf9e5e7a..4955b667 100644 --- a/.coverage-baseline +++ b/.coverage-baseline @@ -1 +1 @@ -73.9 +74.4 diff --git a/docs/architecture.md b/docs/architecture.md index 3ed09f95..4cfd85fa 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -812,7 +812,8 @@ write side is shared with live writes): * before anything is planned, a **structure-only acceptance gate** runs over the scanned subtree; if it finds content the operator cannot safely manage — a kustomization using an unsupported feature (generators / patches / components / helm / replacements / transformers / name(pre|suf)fix / remote - bases), a duplicate manifest identity, an impure managed file, or a standalone non-KRM / invalid YAML — + bases) or malformed `images:`/`replicas:` overrides, a duplicate manifest identity, an impure managed + file, or a standalone non-KRM / invalid YAML — the whole apply is **refused**: nothing is committed, `GitPathAccepted=False`, `Stalled=True`, and `Ready=False` with reason `UnsupportedContent` until a human cleans the path; * desired resources are upserted through the same content derived path as live writes; @@ -901,10 +902,16 @@ hydrates only touched files into buffers for the commit, and flushes only change * **Upserts:** if a managed document for the resource already exists, patch it in place (preserving siblings in a multi document file); if it is sensitive, encrypt the whole document again at its existing path; if no document exists, create a new file at the canonical placement path. +* **Kustomize override edit-through:** a live value produced by a well-formed `images:` or `replicas:` + entry in the document's kustomization chain is written back to that entry (comment-preserving, only + fields the entry already declares); the source manifest keeps its bytes. Anything the inversion cannot + express falls back to the plain in-place patch. See + [gitops-api/f1-images-replicas-edit-through.md](design/gitops-api/f1-images-replicas-edit-through.md). * **Deletes:** use the manifest identity index, so a moved manifest can still be deleted even when it is not at the canonical path. * **Field patches** (currently `/scale` → parent `spec.replicas`) are intentionally narrow: they only - patch an existing parent manifest and never fabricate a parent object from partial subresource data. + patch an existing parent manifest and never fabricate a parent object from partial subresource data; a + `spec.replicas` assignment governed by a `replicas:` override is routed to the entry instead. ### File Placement diff --git a/docs/design/gitops-api/README.md b/docs/design/gitops-api/README.md new file mode 100644 index 00000000..b11b66b4 --- /dev/null +++ b/docs/design/gitops-api/README.md @@ -0,0 +1,73 @@ +# GitOps API: editing existing GitOps folders as a product surface + +> Status: active workstream +> Captured: 2026-07-06 +> Related: +> [../../future/idea-application-editing.md](../../future/idea-application-editing.md), +> [../manifest/contextual-namespace-and-kustomize-folder-editing.md](../manifest/contextual-namespace-and-kustomize-folder-editing.md), +> [../manifest/file-agnostic-placement.md](../manifest/file-agnostic-placement.md), +> [../../finished/current-manifest-support-review.md](../../finished/current-manifest-support-review.md) + +## Goal + +Enable a product layer on top of GitOps Reverser where an end user points the +operator at a Git folder holding **raw YAML or simple Kustomize**, edits those +resources through the Kubernetes API, and the operator writes the changes to a +branch. The product layer then opens the pull request through the Git host's +API. + +The division of responsibility is deliberate and already a recorded decision +([file-agnostic-placement.md](../manifest/file-agnostic-placement.md)): + +- **GitOps Reverser**: watch live state, edit the folder the way a careful + human would (in place, comment-preserving, refusing what it cannot own), + push to a named branch, and expose pollable status (`CommitRequest` + `Pushed=True` + `status.sha` + `status.branch`). +- **Product layer**: session/branch lifecycle policy, PR creation and merge, + branch cleanup, and any UI. + +The operator never gains Git-host knowledge (no GitHub/GitLab API calls). + +## The governing rule + +Every feature in this workstream must keep the repository **round-trippable**: +one source document owns one live object, and every edit the operator writes +must be expressible in both directions (live → Git and Git → live via the +GitOps tool's render). Constructs that are lossy or one-way (arbitrary patches, +generators with hash suffixes, name prefixes, Helm inflation) stay refused — +that refusal is the product's defensible support contract, not a gap. + +## Feature ladder + +Ordered by value-per-risk. Each feature gets its own design doc in this folder +when work starts. + +| # | Feature | Design doc | Status | +|---|---------|-----------|--------| +| F1 | Kustomize `images:` / `replicas:` edit-through — a live change produced by an override entry is written back to that entry, never through into the source manifest | [f1-images-replicas-edit-through.md](f1-images-replicas-edit-through.md) | in progress | +| F2 | Render-root scoping — a GitTarget declares its render root (e.g. `overlays/env1`); base files reached through `../../base` become read-only context, dissolving overlay fan-out ambiguity | — | not designed | +| F3 | Restricted patch authoring — write/update scalar-field strategic-merge patches in an overlay ("live drift lands in the environment's overlay") | — | not designed | +| F4 | New-file placement rules — sibling inference + `spec.newFilePath` template so new resources land in the folder's convention, not the canonical REST path | designed in [version2/gittarget-new-file-placement-rules.md](../manifest/version2/gittarget-new-file-placement-rules.md) | designed, unbuilt | +| F5 | Branch/session ergonomics — base-branch selection, opt-in remote branch cleanup, a GitTarget-level quiescence condition | — | not designed | + +## What already works (baseline, 2026-07-06) + +- Raw-YAML folders (explicit namespaces): in-place, comment-preserving edits; + match-first placement; mark-and-sweep resync. +- Single-context Kustomize folders (`namespace:` + `resources:`/`bases`, local + files and child-directory bases): graph-aware namespace inference; inherited + namespaces are kept out of the file bytes on write. +- Branch writing: `GitTarget.spec.branch` (immutable, glob-authorized via + `GitProvider.spec.allowedBranches`); `CommitRequest` as the per-save control + surface with terminal `Pushed=True` + SHA. +- Refusals: unsupported kustomize features, duplicate identities, impure or + foreign content — refuse-first, never mis-edit. + +## Known boundary (what stays refused) + +Classic base + per-env overlay repositories are refused today on two +independent gates: any overlay feature outside the supported subset +(`patches*`, generators, `components`, `namePrefix`/`nameSuffix`, Helm, remote +bases), and multi-root namespace fan-out over a shared base +(`ambiguous-namespace`). F1 does not change that boundary; F2/F3 are the +features that would move it. diff --git a/docs/design/gitops-api/f1-images-replicas-edit-through.md b/docs/design/gitops-api/f1-images-replicas-edit-through.md new file mode 100644 index 00000000..b81e3879 --- /dev/null +++ b/docs/design/gitops-api/f1-images-replicas-edit-through.md @@ -0,0 +1,196 @@ +# F1: Kustomize `images:` / `replicas:` edit-through + +> Status: implemented (phases A–C landed together with this doc; the ambiguity +> refusal projection into GitTarget status remains future work) +> Captured: 2026-07-06 +> Related: +> [README.md](README.md), +> [../manifest/contextual-namespace-and-kustomize-folder-editing.md](../manifest/contextual-namespace-and-kustomize-folder-editing.md), +> [../../../internal/git/manifestedit/DECISION.md](../../../internal/git/manifestedit/DECISION.md) + +## Problem + +`hasUnsupportedKustomizeFeature` deliberately treats `images:` and `replicas:` +as benign: they do not create resources or change identity, so a kustomization +using them is accepted. But the writer does not *understand* them. With an +overlay like: + +```yaml +# kustomization.yaml +namespace: app +resources: [deployment.yaml] +images: + - name: ghcr.io/example/podinfo + newTag: "6.5.0" +``` + +the live Deployment carries `ghcr.io/example/podinfo:6.5.0` while +`deployment.yaml` says `:6.4.0`. Today `patchExisting` sees that as drift and +writes `6.5.0` into `deployment.yaml`. The render stays correct (the +transformer is idempotent), but: + +- the source file's tag becomes dead text shadowed by the override; +- every deliberate tag bump lands in the wrong place (the source file instead + of the override entry a human would edit); +- the repo silently stops being the file a human authored. + +`replicas:` has the same shape with `spec.replicas`. + +## Decision + +**The edit lands where the value lives.** For every governed component (image +name / tag / digest per container; replica count), determine its current +*supplier*: the last override entry in the document's build chain that sets +that component, or the source document when no entry does. A live change to a +component is applied to its supplier: + +- supplier = override entry → update `newTag` / `newName` / `digest` / + `count` **in the kustomization file**, preserving comments and order; the + source manifest keeps its bytes. +- supplier = source document → the normal in-place patch path (today's + behavior). +- live already equals the forward render → **no change anywhere** (this is + the write-through fix: today the source file is patched). + +### Boundaries (v1) + +1. **Only existing entries are edited.** The operator updates fields that are + already present on an entry that already matches. It never adds or removes + `images:`/`replicas:` entries, never adds missing fields (`newTag` on an + entry that lacks it), and never creates a kustomization file. The human + declares where the knob lives; the operator turns the knob. "Should env + drift become a new override?" is F2/F3 policy, not F1. +2. **Unambiguous context only.** Override routing applies when every supported + render root that reaches the document applies an identical override chain. + Distinct chains from multiple roots emit an `ambiguous-kustomize-overrides` + diagnostic and fall back to today's write-through (no behavior regression; + F2's render-root scoping resolves this case properly). +3. **Acceptance tightens only for garbage.** A kustomization whose `images:` + or `replicas:` value is present but not structurally parseable (not a list + of maps, missing `name`, non-string image fields, non-integer count) is + marked unsupported and refused — such a file fails `kustomize build` + anyway, and we can no longer claim to understand the render. Well-formed + entries keep the folder accepted exactly as today. +4. **Plain documents only.** Sensitive (SOPS) documents keep the + re-encrypt-wholesale path with no override routing. Among field patches + (subresource events), the `/scale` case is routed — a `spec.replicas` + assignment governed by a replicas entry updates the entry, never the file; + other field patches are already bounded and skip routing. +5. **New containers write through.** A container present live but absent from + the source document is written as-is. If its written source form then + matches an override entry, the supplier rule converges the tag into the + entry on the next event/resync — self-healing, no special case. + +### Transformer semantics implemented + +- `images:` entry = `{name, newName?, newTag?, digest?}`. An entry applies to + an image whose *current* name (at that point in the chain) equals `name`. + `newName` replaces the name, `newTag` the tag, `digest` the digest. Entries + apply in listed order within a kustomization; kustomizations apply + innermost-first along the reference path from the file to the render root + (kustomize renders bases before applying a parent's transformers). +- Image fields are any string `image` inside items of a sequence field named + `containers`, `initContainers`, or `ephemeralContainers`, at any depth — + mirroring the builtin transformer's generic traversal. +- `replicas:` entry = `{name, count}`. Applies to `spec.replicas` of + Deployment / ReplicaSet / StatefulSet documents whose `metadata.name` equals + `name` (the builtin transformer's fieldspec set). + +## Mechanics + +The single integration point is `patchExisting` +(`internal/git/plan_flush.go`): both the steady-state event path and the +resync mark-and-sweep upsert funnel through it, so fixing it fixes both. + +### Phase A — model (`internal/manifestanalyzer`) + +- `parseKustomizations` also parses `images` and `replicas` entries; + present-but-unparseable marks the kustomization unsupported (Decision 3). +- The existing render-root graph walk (`assignFromRoot`) additionally records, + per resource file, the ordered **override chain**: the kustomizations along + the reference path (innermost → root) that carry any override entries. +- `DocumentModel` gains `Overrides *KustomizeOverrides` — the collapsed, + unambiguous chain (nil when none or ambiguous), each entry carrying its + source kustomization path so the writer knows which file to edit. +- Conflicting chains from multiple roots emit the + `ambiguous-kustomize-overrides` diagnostic and leave `Overrides` nil. +- Corpus: `testdata/contextual-namespace/supported/images-overlay`, + `supported/replicas-overlay`, `unsupported/ambiguous-images`. + +### Phase B — projection (`internal/manifestanalyzer`) + +A pure function: given the source document's object, the live (projected) +object, and the override chain, return + +- the **desired-for-file** object: governed components rewritten back to the + source values wherever the supplier is an override entry (so the file diff + disappears), everything else untouched; +- the list of **override edits**: `(kustomization path, images|replicas, + entry name, field, new value)` for components whose live value diverges + from the forward render and whose supplier is an entry. + +### Phase C — writer (`internal/git`, `internal/git/manifestedit`) + +- `manifestedit` gains a narrow kustomization editor: set the scalar value of + an existing field on an existing entry (`images[name=X].newTag`, + `replicas[name=Y].count`) via yaml.v3 node editing — comments and order + preserved, single-document files only, refuse-with-diagnostic otherwise. +- `patchExisting` runs the phase-B projection when the document has an + override chain, hands desired-for-file to `Decide`/`Apply` as today, and + applies override edits to the kustomization file's buffer through the same + `fileBuffer` machinery (so `.gittargetignore` shadow checks and the flush + path apply unchanged). + +### Phase D — docs and observability + +- `docs/architecture.md`: the Manifest Aware Writer and acceptance-gate + sections document the supported subset change ("images/replicas overrides + are understood and edited through"). +- Log one line per override edit (kustomization path, entry, field); the + ambiguous case is visible via the diagnostic. A GitTarget status surface + for override routing is deferred. + +## Known limitation: shared entries with divergent live consumers + +One `images:` entry is a **shared knob**: kustomize rewrites every matching +image in the build, so bumping `newTag` moves all consumers together. F1 +mirrors that faithfully for the normal case — when consumers move together +(the entry is bumped Git-side, or a product bumps "the app version" live for +all of them), the routing is stable and idempotent: the first event updates +the entry, later events for sibling consumers see live == render and do +nothing. + +But if consumers **diverge live** — one Deployment is hot-bumped to a new tag +while another keeps the old one — that state is *unrepresentable* in the +source layout: one entry cannot hold two tags. No deterministic writer can +mirror it. F1's behavior is then: the entry follows whichever consumer's +change was processed last, and a full resync can alternate the entry between +the divergent values (each pass "corrects" it toward a different consumer). +The render always matches at least one consumer, never both. + +Remedies: align the consumers live, or give each workload its own image name +(and entry) so each has its own knob. A cross-consumer consistency refusal at +edit time needs the siblings' live state, which the steady-state writer does +not have — batch-wide edit reconciliation is future work, noted for F2. + +## What F1 deliberately does not do + +- No `kustomize build`, no source maps (Option D remains future research). +- No entry creation/deletion, no patch authoring, no `namePrefix`/`nameSuffix` + or generator support — the acceptance boundary for those is unchanged. +- No multi-root overlay support: base + env1/env2/prod fan-out stays refused + (namespace case) or unrouted (no-namespace case) until F2. + +## Test plan + +- Corpus folders pin chain attribution and ambiguity (phase A). +- Table-driven unit tests for forward render + supplier resolution + inversion + (phase B): tag change → entry edit; name change with `newName` → entry edit; + tag change with no `newTag` on the matching entry → file patch; chained + kustomizations; multiple containers; initContainers; replicas on the three + supported kinds and ignored elsewhere. +- Writer tests in the `inplace_edit_test.go` style (phase C): governed tag + change edits kustomization.yaml only; ungoverned change patches the source + file only; mixed change routes both; no-op when live equals render; resync + does not churn a governed folder; comment preservation in the kustomization + file; SOPS documents unaffected. diff --git a/internal/git/inplace_overrides_test.go b/internal/git/inplace_overrides_test.go new file mode 100644 index 00000000..d8a04db5 --- /dev/null +++ b/internal/git/inplace_overrides_test.go @@ -0,0 +1,230 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" + + "github.com/ConfigButler/gitops-reverser/internal/git/manifestedit" + "github.com/ConfigButler/gitops-reverser/internal/types" + "github.com/ConfigButler/gitops-reverser/internal/typeset" +) + +// The F1 edit-through scenarios (docs/design/gitops-api/f1-images-replicas-edit-through.md): +// a live change produced by a kustomization's images:/replicas: entry lands on +// the entry, and the source manifest keeps its bytes. + +const overridesDeploymentYAML = `apiVersion: apps/v1 +kind: Deployment +metadata: + name: web +spec: + selector: + matchLabels: + app: web + template: + metadata: + labels: + app: web + spec: + containers: + - name: podinfo + image: ghcr.io/example/podinfo:6.3.0 # base tag, shadowed by the overlay +` + +const overridesKustomizationYAML = `apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization +namespace: default +resources: + - apps/deployment.yaml +images: + - name: ghcr.io/example/podinfo + newTag: "6.4.0" +replicas: + - name: web + count: 3 +` + +func overridesDeploymentEvent(image string, replicas int64) Event { + return Event{ + Object: &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "apps/v1", + "kind": "Deployment", + "metadata": map[string]interface{}{"name": "web", "namespace": "default"}, + "spec": map[string]interface{}{ + "replicas": replicas, + "selector": map[string]interface{}{"matchLabels": map[string]interface{}{"app": "web"}}, + "template": map[string]interface{}{ + "metadata": map[string]interface{}{"labels": map[string]interface{}{"app": "web"}}, + "spec": map[string]interface{}{ + "containers": []interface{}{ + map[string]interface{}{"name": "podinfo", "image": image}, + }, + }, + }, + }, + }}, + Identifier: types.ResourceIdentifier{ + Group: "apps", Version: "v1", Resource: "deployments", Namespace: "default", Name: "web", + }, + Operation: "UPDATE", + } +} + +func deploymentMapper() typeset.Lookup { + return typeset.NewSnapshotRegistry(typeset.Snapshot{ + Entries: []typeset.Entry{{ + GVK: schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"}, + GVR: schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"}, + Namespaced: true, + Allowed: true, + }}, + }) +} + +// seedOverridesWorktree writes the deployment + kustomization pair and returns +// the two absolute paths (deployment first). +func seedOverridesWorktree(t *testing.T, root string) (string, string) { + t.Helper() + deployPath := filepath.Join(root, "apps", "deployment.yaml") + kustPath := filepath.Join(root, "kustomization.yaml") + require.NoError(t, os.MkdirAll(filepath.Dir(deployPath), 0o750)) + require.NoError(t, os.WriteFile(deployPath, []byte(overridesDeploymentYAML), 0o600)) + require.NoError(t, os.WriteFile(kustPath, []byte(overridesKustomizationYAML), 0o600)) + return deployPath, kustPath +} + +// assertFileBytes pins a file to exact bytes: these scenarios are ABOUT +// formatting preservation, so a semantic (YAML-equal) comparison would hide +// exactly the churn they exist to catch. +func assertFileBytes(t *testing.T, path, want, msg string) { + t.Helper() + got, err := os.ReadFile(path) + require.NoError(t, err) + if string(got) != want { + t.Errorf("%s\n--- want ---\n%s\n--- got ---\n%s", msg, want, got) + } +} + +// A live tag bump governed by the images entry updates kustomization.yaml and +// leaves the source manifest byte-for-byte untouched. +func TestPlanFlush_RoutesImageTagToKustomizationEntry(t *testing.T) { + writer := newContentWriter(types.SensitiveResourcePolicy{}) + worktree := newWorktreeForTest(t) + deployPath, kustPath := seedOverridesWorktree(t, worktree.Filesystem.Root()) + + changed := applyEventsViaPlanFlushWithMapper(t, writer, worktree, deploymentMapper(), + overridesDeploymentEvent("ghcr.io/example/podinfo:6.5.0", 3)) + require.True(t, changed, "the tag bump must land somewhere") + + assertFileBytes(t, deployPath, overridesDeploymentYAML, + "the source manifest must keep its bytes; the entry owns the tag") + + kust, err := os.ReadFile(kustPath) + require.NoError(t, err) + assert.Contains(t, string(kust), `newTag: "6.5.0"`, "the entry absorbs the live tag") + assert.NotContains(t, string(kust), `newTag: "6.4.0"`) +} + +// Live state equal to the overlay's render is a full no-op: the source file's +// "stale" tag is dead text the entry shadows, and must NOT be overwritten +// (the write-through fix F1 exists for). +func TestPlanFlush_LiveMatchingOverlayRenderIsNoOp(t *testing.T) { + writer := newContentWriter(types.SensitiveResourcePolicy{}) + worktree := newWorktreeForTest(t) + deployPath, kustPath := seedOverridesWorktree(t, worktree.Filesystem.Root()) + + changed := applyEventsViaPlanFlushWithMapper(t, writer, worktree, deploymentMapper(), + overridesDeploymentEvent("ghcr.io/example/podinfo:6.4.0", 3)) + assert.False(t, changed, "live == render must write nothing") + + assertFileBytes(t, deployPath, overridesDeploymentYAML, "source manifest must be untouched") + assertFileBytes(t, kustPath, overridesKustomizationYAML, "kustomization must be untouched") +} + +// A live scale governed by the replicas entry updates count and leaves the +// source manifest untouched — including keeping spec.replicas ABSENT, since the +// transformer supplies the field. +func TestPlanFlush_RoutesReplicaCountToKustomizationEntry(t *testing.T) { + writer := newContentWriter(types.SensitiveResourcePolicy{}) + worktree := newWorktreeForTest(t) + deployPath, kustPath := seedOverridesWorktree(t, worktree.Filesystem.Root()) + + changed := applyEventsViaPlanFlushWithMapper(t, writer, worktree, deploymentMapper(), + overridesDeploymentEvent("ghcr.io/example/podinfo:6.4.0", 5)) + require.True(t, changed, "the scale must land somewhere") + + assertFileBytes(t, deployPath, overridesDeploymentYAML, + "spec.replicas must stay out of the source manifest") + + kust, err := os.ReadFile(kustPath) + require.NoError(t, err) + assert.Contains(t, string(kust), "count: 5", "the entry absorbs the live count") + assert.NotContains(t, string(kust), "count: 3") +} + +// A /scale field patch on a governed Deployment routes to the replicas entry; +// spec.replicas never lands in the source manifest. +func TestPlanFlush_RoutesScaleFieldPatchToKustomizationEntry(t *testing.T) { + writer := newContentWriter(types.SensitiveResourcePolicy{}) + worktree := newWorktreeForTest(t) + deployPath, kustPath := seedOverridesWorktree(t, worktree.Filesystem.Root()) + + scale := Event{ + Identifier: types.ResourceIdentifier{ + Group: "apps", Version: "v1", Resource: "deployments", Namespace: "default", Name: "web", + }, + Operation: "UPDATE", + FieldPatch: &FieldPatch{ + Source: "scale", + Assignments: []manifestedit.FieldAssignment{ + {Path: []string{"spec", "replicas"}, Value: int64(7)}, + }, + }, + } + changed := applyEventsViaPlanFlushWithMapper(t, writer, worktree, deploymentMapper(), scale) + require.True(t, changed, "the scale must land in the kustomization") + + assertFileBytes(t, deployPath, overridesDeploymentYAML, + "a governed scale must not write spec.replicas into the source manifest") + + kust, err := os.ReadFile(kustPath) + require.NoError(t, err) + assert.Contains(t, string(kust), "count: 7") +} + +// A change no entry governs keeps today's behavior: it is patched into the +// source manifest and the kustomization stays untouched. +func TestPlanFlush_UngovernedChangeStillPatchesSourceFile(t *testing.T) { + writer := newContentWriter(types.SensitiveResourcePolicy{}) + worktree := newWorktreeForTest(t) + root := worktree.Filesystem.Root() + deployPath, kustPath := seedOverridesWorktree(t, root) + // Repoint the images entry at an image this Deployment does not use. + ungoverned := "apiVersion: kustomize.config.k8s.io/v1beta1\n" + + "kind: Kustomization\n" + + "namespace: default\n" + + "resources:\n - apps/deployment.yaml\n" + + "images:\n - name: someone/else\n newTag: \"1.0.0\"\n" + require.NoError(t, os.WriteFile(kustPath, []byte(ungoverned), 0o600)) + + changed := applyEventsViaPlanFlushWithMapper(t, writer, worktree, deploymentMapper(), + overridesDeploymentEvent("ghcr.io/example/podinfo:6.5.0", 1)) + require.True(t, changed) + + deploy, err := os.ReadFile(deployPath) + require.NoError(t, err) + assert.Contains(t, string(deploy), "ghcr.io/example/podinfo:6.5.0", + "an ungoverned tag change writes through to the source manifest") + assert.Contains(t, string(deploy), "# base tag, shadowed by the overlay", + "the in-place patch keeps hand-authored comments") + + assertFileBytes(t, kustPath, ungoverned, "the kustomization stays untouched") +} diff --git a/internal/git/manifestedit/kustomization.go b/internal/git/manifestedit/kustomization.go new file mode 100644 index 00000000..c84bc8c2 --- /dev/null +++ b/internal/git/manifestedit/kustomization.go @@ -0,0 +1,128 @@ +// SPDX-License-Identifier: Apache-2.0 + +package manifestedit + +import ( + "fmt" + + "gopkg.in/yaml.v3" +) + +// Kustomization override sections the editor accepts. The editor is the +// mechanism half of the F1 images/replicas edit-through +// (docs/design/gitops-api/f1-images-replicas-edit-through.md): it updates the +// scalar value of a field that ALREADY EXISTS on an entry that ALREADY EXISTS, +// and nothing else — it never adds or removes entries, keys, or files. +const ( + KustomizationSectionImages = "images" + KustomizationSectionReplicas = "replicas" +) + +// KustomizationEdit sets one existing scalar field on one existing entry of a +// kustomization.yaml override section. EntryIndex pins the exact entry (two +// entries may share a name and kustomize applies them in order); EntryName is +// re-verified against it so a drifted file is skipped, never mis-edited. +type KustomizationEdit struct { + // Section is KustomizationSectionImages or KustomizationSectionReplicas. + Section string + // EntryIndex is the entry's position within the section sequence. + EntryIndex int + // EntryName is the entry's name: value, verified against EntryIndex. + EntryName string + // Field is the scalar key to update: newName/newTag/digest, or count. + Field string + // Value is the new scalar value; for count it is a decimal integer. + Value string +} + +// PatchKustomization applies the edits to a single-document kustomization file, +// preserving comments, key order, and framing exactly as the manifest patch path +// does. All-or-nothing: any edit that cannot be applied (multi-document file, +// unparseable YAML, missing section/entry/field, a name mismatch at the pinned +// index) skips the whole call and returns the original content with a +// diagnostic — the writer must never guess inside a build directive. +func PatchKustomization(path string, content []byte, edits []KustomizationEdit) (EditResult, []Diagnostic) { + skip := func(format string, args ...interface{}) (EditResult, []Diagnostic) { + return EditResult{Content: content, Mode: EditSkipped}, + []Diagnostic{diag(DiagWarning, Location{Path: path}, format, args...)} + } + + if DocumentCount(content) != 1 { + return skip("kustomization %s is not a single-document file", path) + } + docs := splitDocuments(string(content)) + idx := -1 + for i, d := range docs { + if !isBlankLine(d.body) { + idx = i + break + } + } + if idx < 0 { + return skip("kustomization %s holds no document", path) + } + target := docs[idx].body + + root, empty, err := decodeDoc(target) + if err != nil || empty || root.Kind != yaml.MappingNode { + return skip("kustomization %s is not an editable mapping document", path) + } + for _, e := range edits { + if err := applyKustomizationEdit(root, e); err != nil { + return skip("kustomization %s: %v", path, err) + } + } + + encoded, err := encodeNode(root) + if err != nil { + return skip("kustomization %s: re-encode failed: %v", path, err) + } + body := reskinDocument(target, string(encoded)) + if body == target { + return EditResult{Content: content, Mode: EditNoChange}, nil + } + docs[idx].body = body + return EditResult{Content: []byte(joinDocuments(docs)), Mode: EditPatched}, nil +} + +// applyKustomizationEdit updates one scalar in place, or reports why it cannot. +func applyKustomizationEdit(root *yaml.Node, e KustomizationEdit) error { + section := nodeMapGet(root, e.Section) + if section == nil || section.Kind != yaml.SequenceNode { + return fmt.Errorf("no %s sequence", e.Section) + } + if e.EntryIndex < 0 || e.EntryIndex >= len(section.Content) { + return fmt.Errorf("%s entry %d out of range", e.Section, e.EntryIndex) + } + item := section.Content[e.EntryIndex] + if item.Kind != yaml.MappingNode { + return fmt.Errorf("%s entry %d is not a mapping", e.Section, e.EntryIndex) + } + name := nodeMapGet(item, "name") + if name == nil || name.Value != e.EntryName { + return fmt.Errorf("%s entry %d is not named %q", e.Section, e.EntryIndex, e.EntryName) + } + field := nodeMapGet(item, e.Field) + if field == nil || field.Kind != yaml.ScalarNode { + return fmt.Errorf("%s entry %q has no scalar field %s", e.Section, e.EntryName, e.Field) + } + setOverrideScalar(field, e) + return nil +} + +// setOverrideScalar writes the new value, keeping the value string-typed for the +// image fields (the encoder quotes "1.29"-style values when the tag is !!str) and +// integer-typed for count. An existing quoting style is kept; other styles reset +// to plain so the encoder can choose safely. +func setOverrideScalar(n *yaml.Node, e KustomizationEdit) { + n.Value = e.Value + if e.Field == "count" { + n.Tag = "!!int" + n.Style = 0 + return + } + n.Tag = "!!str" + if n.Style != yaml.SingleQuotedStyle && n.Style != yaml.DoubleQuotedStyle { + n.Style = 0 + } +} diff --git a/internal/git/manifestedit/kustomization_test.go b/internal/git/manifestedit/kustomization_test.go new file mode 100644 index 00000000..ebab8206 --- /dev/null +++ b/internal/git/manifestedit/kustomization_test.go @@ -0,0 +1,140 @@ +// SPDX-License-Identifier: Apache-2.0 + +package manifestedit + +import ( + "strings" + "testing" +) + +const kustomizationFixture = `# pin the app image here +apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization +namespace: app +resources: + - deployment.yaml +images: + - name: ghcr.io/example/podinfo # tracked by the product + newTag: "6.4.0" # deployed version +replicas: + - name: web + count: 3 +` + +func imagesTagEdit(value string) KustomizationEdit { + return KustomizationEdit{ + Section: KustomizationSectionImages, EntryIndex: 0, + EntryName: "ghcr.io/example/podinfo", Field: "newTag", Value: value, + } +} + +func TestPatchKustomization_UpdatesEntryPreservingHandAuthoring(t *testing.T) { + res, diags := PatchKustomization("kustomization.yaml", []byte(kustomizationFixture), + []KustomizationEdit{imagesTagEdit("6.5.0")}) + if res.Mode != EditPatched { + t.Fatalf("Mode = %q, want patched (diags %+v)", res.Mode, diags) + } + got := string(res.Content) + for _, want := range []string{ + "# pin the app image here", + "# tracked by the product", + "# deployed version", + `newTag: "6.4.0"`, + } { + if want == `newTag: "6.4.0"` { + if strings.Contains(got, want) { + t.Errorf("old value must be gone:\n%s", got) + } + continue + } + if !strings.Contains(got, want) { + t.Errorf("hand-authored content %q must survive:\n%s", want, got) + } + } + if !strings.Contains(got, `newTag: "6.5.0"`) { + t.Errorf("new value must keep its quoting style:\n%s", got) + } + // Unrelated sections stay put. + if !strings.Contains(got, "namespace: app") || !strings.Contains(got, "count: 3") { + t.Errorf("unrelated fields must be untouched:\n%s", got) + } +} + +func TestPatchKustomization_QuotesValuesThatWouldChangeType(t *testing.T) { + content := "images:\n- name: app\n newTag: stable\n" + res, diags := PatchKustomization("kustomization.yaml", []byte(content), []KustomizationEdit{{ + Section: KustomizationSectionImages, EntryIndex: 0, + EntryName: "app", Field: "newTag", Value: "1.29", + }}) + if res.Mode != EditPatched { + t.Fatalf("Mode = %q, want patched (diags %+v)", res.Mode, diags) + } + if !strings.Contains(string(res.Content), `newTag: "1.29"`) { + t.Errorf("a float-looking tag must stay a string:\n%s", res.Content) + } +} + +func TestPatchKustomization_UpdatesReplicaCountAsInteger(t *testing.T) { + res, diags := PatchKustomization("kustomization.yaml", []byte(kustomizationFixture), + []KustomizationEdit{{ + Section: KustomizationSectionReplicas, EntryIndex: 0, + EntryName: "web", Field: "count", Value: "5", + }}) + if res.Mode != EditPatched { + t.Fatalf("Mode = %q, want patched (diags %+v)", res.Mode, diags) + } + if !strings.Contains(string(res.Content), "count: 5") { + t.Errorf("count must be a plain integer:\n%s", res.Content) + } +} + +func TestPatchKustomization_SameValueIsNoChange(t *testing.T) { + res, diags := PatchKustomization("kustomization.yaml", []byte(kustomizationFixture), + []KustomizationEdit{imagesTagEdit("6.4.0")}) + if res.Mode != EditNoChange { + t.Fatalf("Mode = %q, want no-change (diags %+v)", res.Mode, diags) + } + if string(res.Content) != kustomizationFixture { + t.Errorf("no-change must return the original bytes") + } +} + +// All-or-nothing: any edit that cannot land skips the whole call, byte-for-byte. +func TestPatchKustomization_RefusalsLeaveContentUntouched(t *testing.T) { + cases := []struct { + name string + content string + edit KustomizationEdit + }{ + {"entry name mismatch at pinned index", kustomizationFixture, KustomizationEdit{ + Section: KustomizationSectionImages, EntryIndex: 0, + EntryName: "someone/else", Field: "newTag", Value: "1", + }}, + {"index out of range", kustomizationFixture, KustomizationEdit{ + Section: KustomizationSectionImages, EntryIndex: 7, + EntryName: "ghcr.io/example/podinfo", Field: "newTag", Value: "1", + }}, + {"field not declared on entry", kustomizationFixture, KustomizationEdit{ + Section: KustomizationSectionImages, EntryIndex: 0, + EntryName: "ghcr.io/example/podinfo", Field: "digest", Value: "sha256:abc", + }}, + {"missing section", "namespace: app\n", imagesTagEdit("1")}, + {"multi-document file", kustomizationFixture + "---\nnamespace: other\n", imagesTagEdit("1")}, + {"unparseable", "images: [::\n", imagesTagEdit("1")}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + res, diags := PatchKustomization("kustomization.yaml", []byte(tc.content), + []KustomizationEdit{tc.edit}) + if res.Mode != EditSkipped { + t.Fatalf("Mode = %q, want skipped", res.Mode) + } + if string(res.Content) != tc.content { + t.Errorf("a refused edit must leave the bytes untouched") + } + if len(diags) == 0 { + t.Errorf("a refusal must carry a diagnostic") + } + }) + } +} diff --git a/internal/git/manifestedit/split.go b/internal/git/manifestedit/split.go index 7901286d..5bc4fde3 100644 --- a/internal/git/manifestedit/split.go +++ b/internal/git/manifestedit/split.go @@ -75,6 +75,18 @@ func DocumentCount(content []byte) int { return n } +// DocumentBody returns the exact bytes of the document at index within content, +// carved by the same byte-faithful splitter every edit path uses. ok is false +// when the index is out of range. Callers use it to parse one document's raw +// object without re-implementing document splitting. +func DocumentBody(content []byte, index int) ([]byte, bool) { + docs := splitDocuments(string(content)) + if index < 0 || index >= len(docs) { + return nil, false + } + return []byte(docs[index].body), true +} + // joinDocuments reassembles documents into file content. func joinDocuments(docs []rawDoc) string { var b strings.Builder diff --git a/internal/git/plan_flush.go b/internal/git/plan_flush.go index 7d986cd2..9bad7280 100644 --- a/internal/git/plan_flush.go +++ b/internal/git/plan_flush.go @@ -6,6 +6,7 @@ import ( "bytes" "context" "fmt" + "math" "os" "path" "path/filepath" @@ -13,6 +14,7 @@ import ( gogit "github.com/go-git/go-git/v5" "sigs.k8s.io/controller-runtime/pkg/log" + sigsyaml "sigs.k8s.io/yaml" "github.com/ConfigButler/gitops-reverser/internal/git/manifestedit" "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" @@ -216,6 +218,14 @@ func (wb *writeBatch) applyFieldPatch(ctx context.Context, event Event) error { return nil } + assignments := event.FieldPatch.Assignments + if dm := wb.store.ByManifestIdentity[id]; dm != nil && dm.Overrides != nil { + assignments = wb.routeGovernedFieldAssignments(ctx, event, dm, assignments) + if len(assignments) == 0 { + return nil + } + } + buf := wb.buffer(filePath) idx, found := currentDocIndex(filePath, buf.current, id) if !found { @@ -224,7 +234,7 @@ func (wb *writeBatch) applyFieldPatch(ctx context.Context, event Event) error { } res, diags := manifestedit.PatchFields( - buf.current, idx, id, event.FieldPatch.Assignments, manifestedit.EditOptions{}, + buf.current, idx, id, assignments, manifestedit.EditOptions{}, ) switch res.Mode { case manifestedit.EditPatched: @@ -242,6 +252,50 @@ func (wb *writeBatch) applyFieldPatch(ctx context.Context, event Event) error { return nil } +// routeGovernedFieldAssignments diverts a spec.replicas assignment whose value a +// replicas override governs to its kustomization entry (the /scale subresource +// case of F1) and returns the assignments the file patch should still apply. An +// ungoverned assignment — any other path, a non-integer value, no matching +// entry — keeps today's bounded file patch. +func (wb *writeBatch) routeGovernedFieldAssignments( + ctx context.Context, + event Event, + dm *manifestanalyzer.DocumentModel, + assignments []manifestedit.FieldAssignment, +) []manifestedit.FieldAssignment { + kept := make([]manifestedit.FieldAssignment, 0, len(assignments)) + for _, a := range assignments { + if len(a.Path) == 2 && a.Path[0] == "spec" && a.Path[1] == "replicas" { + if count, isInt := assignmentInt64(a.Value); isInt { + if edit, governed := manifestanalyzer.ReplicaCountEdit(dm, count); governed { + wb.applyOverrideEdits(ctx, event, []manifestanalyzer.OverrideEdit{edit}) + continue + } + } + } + kept = append(kept, a) + } + return kept +} + +// assignmentInt64 reads a field-assignment value as a whole number (audit JSON +// may deliver it as int64 or float64). +func assignmentInt64(v any) (int64, bool) { + switch n := v.(type) { + case int64: + return n, true + case int: + return int64(n), true + case int32: + return int64(n), true + case float64: + if n == math.Trunc(n) { + return int64(n), true + } + } + return 0, false +} + // resolveFieldPatchTarget locates the parent manifest a field-patch event targets. // The parent is resolved from its objectRef GVR through the same resource-identity // inventory the GVR-only delete uses (PlanDelete), which the live-catalog mapper @@ -265,6 +319,11 @@ func (wb *writeBatch) resolveFieldPatchTarget(event Event) (string, manifestedit // same batch that shifted a multi-document file does not misdirect this edit. A // document the store located but an earlier event already removed is simply absent now, // so there is nothing to patch. +// +// When the document is governed by a kustomize images/replicas override chain, the +// desired projection is first split: values the chain produces are restored to their +// source form (so the file keeps its bytes) and the divergence is routed to the +// override entries instead — see docs/design/gitops-api/f1-images-replicas-edit-through.md. func (wb *writeBatch) patchExisting( ctx context.Context, event Event, @@ -283,16 +342,24 @@ func (wb *writeBatch) patchExisting( desired = desired.DeepCopy() desired.SetNamespace("") } + projected := manifestreport.Project(desired) + var overrideEdits []manifestanalyzer.OverrideEdit + if dm.Overrides != nil { + if gitRaw, parsed := gitDocRawObject(buf.current, idx); parsed { + projected, overrideEdits = manifestanalyzer.SplitDesiredForOverrides(gitRaw, projected, dm.Overrides) + } + } c := manifestedit.Comparison{ Git: gitDoc, - Desired: manifestreport.Project(desired), + Desired: projected, Options: manifestreport.EditOptions(), } res, diags := manifestedit.Apply(c, manifestedit.Decide(c)) + outcome := upsertNoChange switch res.Mode { case manifestedit.EditPatched, manifestedit.EditWholeReplace: buf.current = res.Content - return upsertUpdated, nil + outcome = upsertUpdated case manifestedit.EditNoChange, manifestedit.EditSkipped, manifestedit.EditDeleted: // No-op, an unsafe edit left untouched, or (impossible here) a delete: leave // the bytes as they are. Surface a skip so an operator can see a document Git @@ -301,7 +368,71 @@ func (wb *writeBatch) patchExisting( logManifestDiagnostics(ctx, diags) } } - return upsertNoChange, nil + if wb.applyOverrideEdits(ctx, event, overrideEdits) { + outcome = upsertUpdated + } + return outcome, nil +} + +// applyOverrideEdits folds routed override edits into their kustomization file +// buffers, so they flush (and hit the .gittargetignore shadow precondition) +// exactly like any other planned write. It reports whether any buffer changed. +// A skipped edit (drifted or missing entry) is logged and dropped — the source +// file was deliberately left in its source form, so the next event or resync +// re-decides against the changed kustomization rather than guessing now. +func (wb *writeBatch) applyOverrideEdits( + ctx context.Context, + event Event, + edits []manifestanalyzer.OverrideEdit, +) bool { + if len(edits) == 0 { + return false + } + byPath := map[string][]manifestedit.KustomizationEdit{} + paths := make([]string, 0, len(edits)) + for _, e := range edits { + if _, seen := byPath[e.KustomizationPath]; !seen { + paths = append(paths, e.KustomizationPath) + } + byPath[e.KustomizationPath] = append(byPath[e.KustomizationPath], e.Edit) + } + sort.Strings(paths) + + changed := false + for _, p := range paths { + buf := wb.buffer(p) + if buf.current == nil { + continue // the kustomization vanished within this batch; nothing to edit + } + res, diags := manifestedit.PatchKustomization(p, buf.current, byPath[p]) + switch res.Mode { + case manifestedit.EditPatched: + buf.current = res.Content + changed = true + log.FromContext(ctx).Info("Routed live change to kustomization override", + "kustomization", p, "resource", event.Identifier.String(), "edits", len(byPath[p])) + case manifestedit.EditNoChange: + // Another event in this batch already landed the same value. + case manifestedit.EditSkipped, manifestedit.EditWholeReplace, manifestedit.EditDeleted: + logManifestDiagnostics(ctx, diags) + } + } + return changed +} + +// gitDocRawObject parses one document of a managed file into JSON-typed maps for +// the override projection. parsed is false for an out-of-range index or a body +// that is not a YAML mapping — the projection then simply does not run. +func gitDocRawObject(content []byte, idx int) (map[string]interface{}, bool) { + body, ok := manifestedit.DocumentBody(content, idx) + if !ok { + return nil, false + } + raw := map[string]interface{}{} + if err := sigsyaml.Unmarshal(body, &raw); err != nil { + return nil, false + } + return raw, true } // writeWholeFile renders the event's clean content (sanitized, or SOPS-encrypted for a diff --git a/internal/manifestanalyzer/acceptance.go b/internal/manifestanalyzer/acceptance.go index 71b3f883..5fabfe97 100644 --- a/internal/manifestanalyzer/acceptance.go +++ b/internal/manifestanalyzer/acceptance.go @@ -232,7 +232,8 @@ func unsupportedKustomizeRefusals(store *ManifestStore) []AcceptanceIssue { Path: rd.Location.Path, DocumentIndex: rd.Location.DocumentIndex, Message: "kustomization " + rd.Location.Path + " uses an unsupported feature " + - "(generators/patches/components/helm/replacements/transformers/namePrefix/nameSuffix/remote bases); " + + "(generators/patches/components/helm/replacements/transformers/namePrefix/nameSuffix/remote bases) " + + "or malformed images/replicas overrides; " + "the operator cannot map it back to editable source documents and will not write into this folder", }) } diff --git a/internal/manifestanalyzer/overrides.go b/internal/manifestanalyzer/overrides.go new file mode 100644 index 00000000..be10c7ad --- /dev/null +++ b/internal/manifestanalyzer/overrides.go @@ -0,0 +1,357 @@ +// SPDX-License-Identifier: Apache-2.0 + +package manifestanalyzer + +import ( + "math" + "sort" + "strings" + + "github.com/ConfigButler/gitops-reverser/internal/git/manifestedit" +) + +// This file implements the model half of F1 (kustomize images/replicas +// edit-through): parsing the two supported override transformers out of +// kustomization.yaml and attributing an unambiguous override chain to every +// resource file a render root reaches. The write-side projection consumes the +// attached KustomizeOverrides. See +// docs/design/gitops-api/f1-images-replicas-edit-through.md. + +// reasonAmbiguousOverrides marks a build-time diagnostic for a resource file that +// more than one render root reaches with differing override chains. The store +// attaches no overrides in that case — the writer falls back to plain in-place +// patching (today's write-through) rather than guessing which chain governs. +const reasonAmbiguousOverrides manifestedit.DiagReason = "ambiguous-kustomize-overrides" + +// ImageOverride is one parsed images: entry, carrying the kustomization file it +// came from so the writer knows which file to edit. The Has* booleans record key +// presence: the writer only ever updates a field the entry already declares. +type ImageOverride struct { + // Source is the kustomization file path (slash) that declares the entry. + Source string + // Index is the entry's position within its file's images: sequence, so the + // writer can pin the exact entry even when two entries share a name. + Index int + // Name matches an image whose name equals it at that point in the build chain. + Name string + // NewName / NewTag / Digest replace the matched image's components; each is + // meaningful only when its Has* flag is set. + NewName string + NewTag string + Digest string + // HasNewName / HasNewTag / HasDigest record which keys the entry declares. + HasNewName bool + HasNewTag bool + HasDigest bool +} + +// ReplicaOverride is one parsed replicas: entry, carrying its source +// kustomization file. It applies to spec.replicas of a Deployment, ReplicaSet, or +// StatefulSet whose metadata.name equals Name. +type ReplicaOverride struct { + // Source is the kustomization file path (slash) that declares the entry. + Source string + // Index is the entry's position within its file's replicas: sequence. + Index int + // Name matches the target document's metadata.name. + Name string + // Count is the replica count the entry pins. + Count int64 +} + +// KustomizeOverrides is the flattened, unambiguous override chain governing a +// document: every images:/replicas: entry from the kustomizations along the +// single reference path root→file, in build order (innermost kustomization's +// entries first — kustomize renders bases before applying a parent's +// transformers). Nil on a DocumentModel means no chain, or an ambiguous one. +type KustomizeOverrides struct { + Images []ImageOverride + Replicas []ReplicaOverride +} + +// hasOnlyKeys reports whether the entry map uses only the exact key set +// kustomize's typed entry accepts. An unknown key means we can no longer vouch +// for the render (kustomize itself rejects it), so the kustomization is refused +// as malformed rather than silently misunderstood. +func hasOnlyKeys(m map[string]interface{}, keys ...string) bool { + for k := range m { + known := false + for _, want := range keys { + if k == want { + known = true + break + } + } + if !known { + return false + } + } + return true +} + +// parseImageOverrides parses the images: field of a kustomization. ok is false +// when the field is present but not structurally sound (not a list of maps, a +// missing/empty name, a non-string or empty component, an unknown key) — the +// caller marks the kustomization unsupported, because a folder we cannot parse is +// a folder we cannot claim to understand. +func parseImageOverrides(raw map[string]interface{}, source string) ([]ImageOverride, bool) { + v, present := raw["images"] + if !present || isEmptyValue(v) { + return nil, true + } + list, isList := v.([]interface{}) + if !isList { + return nil, false + } + out := make([]ImageOverride, 0, len(list)) + for i, item := range list { + m, isMap := item.(map[string]interface{}) + if !isMap || !hasOnlyKeys(m, "name", "newName", "newTag", "digest") { + return nil, false + } + entry := ImageOverride{Source: source, Index: i} + var ok bool + if entry.Name, ok = requiredString(m, "name"); !ok { + return nil, false + } + if entry.NewName, entry.HasNewName, ok = optionalString(m, "newName"); !ok { + return nil, false + } + if entry.NewTag, entry.HasNewTag, ok = optionalString(m, "newTag"); !ok { + return nil, false + } + if entry.Digest, entry.HasDigest, ok = optionalString(m, "digest"); !ok { + return nil, false + } + out = append(out, entry) + } + return out, true +} + +// parseReplicaOverrides parses the replicas: field of a kustomization. ok is +// false when the field is present but malformed (see parseImageOverrides). +func parseReplicaOverrides(raw map[string]interface{}, source string) ([]ReplicaOverride, bool) { + v, present := raw["replicas"] + if !present || isEmptyValue(v) { + return nil, true + } + list, isList := v.([]interface{}) + if !isList { + return nil, false + } + out := make([]ReplicaOverride, 0, len(list)) + for i, item := range list { + m, isMap := item.(map[string]interface{}) + if !isMap || !hasOnlyKeys(m, "name", "count") { + return nil, false + } + entry := ReplicaOverride{Source: source, Index: i} + var ok bool + if entry.Name, ok = requiredString(m, "name"); !ok { + return nil, false + } + if entry.Count, ok = integerField(m, "count"); !ok || entry.Count < 0 { + return nil, false + } + out = append(out, entry) + } + return out, true +} + +func requiredString(m map[string]interface{}, key string) (string, bool) { + s, isStr := m[key].(string) + if !isStr || strings.TrimSpace(s) == "" { + return "", false + } + return s, true +} + +// optionalString reads an optional string key, returning (value, present, ok): +// present reports whether the key exists, ok is false for a non-string or blank +// value (declared-but-empty is a broken transform, not an unset one). +func optionalString(m map[string]interface{}, key string) (string, bool, bool) { + v, exists := m[key] + if !exists { + return "", false, true + } + s, isStr := v.(string) + if !isStr || strings.TrimSpace(s) == "" { + return "", false, false + } + return s, true, true +} + +// integerField reads a whole-number field. sigs.k8s.io/yaml decodes YAML numbers +// as float64 (via JSON), so an integral float is accepted; anything else is not. +func integerField(m map[string]interface{}, key string) (int64, bool) { + switch n := m[key].(type) { + case float64: + if n != math.Trunc(n) { + return 0, false + } + return int64(n), true + case int: + return int64(n), true + case int64: + return n, true + default: + return 0, false + } +} + +// overrideAssignment collects, per resource file, the distinct override chains +// the render roots reach it with. Exactly one distinct chain attaches its +// flattened overrides; more than one (with any overrides at stake) is the +// ambiguous case the store refuses to route. +type overrideAssignment struct { + chainKeys map[string]struct{} + overrides *KustomizeOverrides + anyOverrides bool +} + +func (a *overrideAssignment) ambiguous() bool { + return a != nil && len(a.chainKeys) > 1 && a.anyOverrides +} + +// kustomizeOverrideAssignments walks every render root — a kustomization no other +// kustomization references, i.e. the directory a human would `kustomize build` — +// and records, per resource file, the kustomization chain along the reference +// path. Unlike the namespace walk (which treats every namespace-bearing +// kustomization as a root and refuses parent/child conflicts), a referenced base +// is not a root here: its transformers compose with its parent's, innermost +// first, exactly as kustomize applies them. +func kustomizeOverrideAssignments( + kusts map[string]*kustomizationDoc, + resourceFiles map[string]struct{}, +) map[string]*overrideAssignment { + out := map[string]*overrideAssignment{} + for _, rootDir := range renderRoots(kusts) { + root := kusts[rootDir] + if root == nil || root.unsupported { + continue + } + visited := map[string]struct{}{} + var stack []*kustomizationDoc + var walk func(dir string, cur *kustomizationDoc) + walk = func(dir string, cur *kustomizationDoc) { + if cur == nil || cur.unsupported { + return + } + if _, seen := visited[dir]; seen { + return + } + visited[dir] = struct{}{} + stack = append(stack, cur) + for _, entry := range cur.resources { + target := cleanJoin(dir, entry) + switch { + case target == "": + // empty, or escapes the scanned root: contributes no chain. + case mapHasKey(resourceFiles, target): + recordOverrideChain(out, target, stack) + default: + walk(target, kusts[target]) + } + } + stack = stack[:len(stack)-1] + } + walk(rootDir, root) + } + return out +} + +// renderRoots returns the kustomization directories no other kustomization in +// the subtree references — the directories a build would be invoked on — in +// sorted order for deterministic walks. +func renderRoots(kusts map[string]*kustomizationDoc) []string { + referenced := map[string]struct{}{} + for dir, k := range kusts { + for _, entry := range k.resources { + target := cleanJoin(dir, entry) + if target == "" { + continue + } + if _, ok := kusts[target]; ok { + referenced[target] = struct{}{} + } + } + } + roots := make([]string, 0, len(kusts)) + for dir := range kusts { + if _, ok := referenced[dir]; !ok { + roots = append(roots, dir) + } + } + sort.Strings(roots) + return roots +} + +// recordOverrideChain records one root→file chain. The walk descends root-first, +// so the stack is outermost-first; build order (innermost kustomization's +// transformers first) is its reverse. +func recordOverrideChain(out map[string]*overrideAssignment, file string, stack []*kustomizationDoc) { + chain := make([]*kustomizationDoc, len(stack)) + for i, k := range stack { + chain[len(stack)-1-i] = k + } + keys := make([]string, len(chain)) + for i, k := range chain { + keys[i] = k.path + } + key := strings.Join(keys, "\x00") + + a := out[file] + if a == nil { + a = &overrideAssignment{chainKeys: map[string]struct{}{}} + out[file] = a + } + flat := flattenOverrideChain(chain) + if _, seen := a.chainKeys[key]; !seen { + a.chainKeys[key] = struct{}{} + if a.overrides == nil { + a.overrides = flat + } + } + if flat != nil { + a.anyOverrides = true + } +} + +// flattenOverrideChain concatenates the chain's entries in build order. Nil when +// no kustomization in the chain declares any override. +func flattenOverrideChain(chain []*kustomizationDoc) *KustomizeOverrides { + var ov KustomizeOverrides + for _, k := range chain { + ov.Images = append(ov.Images, k.images...) + ov.Replicas = append(ov.Replicas, k.replicas...) + } + if len(ov.Images) == 0 && len(ov.Replicas) == 0 { + return nil + } + return &ov +} + +// resolveOverrides returns the overrides to attach to a document in the given +// file, plus an ambiguity diagnostic when distinct chains with overrides at +// stake reach it. Attribution is purely structural (no API source needed), so it +// also works in structure-only analysis. +func resolveOverrides( + loc manifestedit.Location, + assignments map[string]*overrideAssignment, +) (*KustomizeOverrides, *manifestedit.Diagnostic) { + a := assignments[filepathToSlash(loc.Path)] + if a == nil { + return nil, nil + } + if a.ambiguous() { + return nil, &manifestedit.Diagnostic{ + Level: manifestedit.DiagWarning, + Reason: reasonAmbiguousOverrides, + Message: "multiple render roots reach this file with different images/replicas override chains; " + + "refusing to route edits through any of them", + Path: loc.Path, + DocumentIndex: loc.DocumentIndex, + } + } + return a.overrides, nil +} diff --git a/internal/manifestanalyzer/overrides_projection.go b/internal/manifestanalyzer/overrides_projection.go new file mode 100644 index 00000000..e22c4f1b --- /dev/null +++ b/internal/manifestanalyzer/overrides_projection.go @@ -0,0 +1,465 @@ +// SPDX-License-Identifier: Apache-2.0 + +package manifestanalyzer + +import ( + "fmt" + "sort" + "strconv" + "strings" + + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + + "github.com/ConfigButler/gitops-reverser/internal/git/manifestedit" +) + +// This file is the projection half of F1 (see overrides.go for the model half): +// given the source document as Git holds it, the live desired object, and the +// governing override chain, it splits the live state into what the SOURCE FILE +// should hold and what the KUSTOMIZATION ENTRIES should hold — "the edit lands +// where the value lives". See +// docs/design/gitops-api/f1-images-replicas-edit-through.md. + +// OverrideEdit routes one live-value change to a field of an existing +// kustomization override entry. +type OverrideEdit struct { + // KustomizationPath is the kustomization file (slash, relative to the + // GitTarget subtree) declaring the entry. + KustomizationPath string + // Edit is the bounded scalar update the manifestedit editor applies. + Edit manifestedit.KustomizationEdit +} + +// SplitDesiredForOverrides maps the live desired object back through the +// override chain. It returns the object the source document should be compared +// against (a copy of desired with override-produced values restored to their +// source form) plus the entry edits for values whose supplier is an override +// entry. Anything it cannot route safely — a component removal an entry +// supplies, conflicting values for one entry field, or a simulated render that +// would not reproduce live — falls back to the unmodified live value +// (today's write-through), never to a guess. +// +// gitRaw is the source document parsed as JSON-typed maps (sigs.k8s.io/yaml); +// desired is the sanitized projection the writer would otherwise compare. The +// returned object is always a copy; desired is never mutated. +func SplitDesiredForOverrides( + gitRaw map[string]interface{}, + desired *unstructured.Unstructured, + ov *KustomizeOverrides, +) (*unstructured.Unstructured, []OverrideEdit) { + if ov == nil || desired == nil || gitRaw == nil { + return desired, nil + } + out := desired.DeepCopy() + edits := projectImages(gitRaw, out, ov.Images) + edits = append(edits, projectReplicas(gitRaw, out, ov.Replicas)...) + return out, edits +} + +// imageRef is an image reference split into its three overridable components. +type imageRef struct { + name string + tag string + digest string +} + +// parseImageRef splits name[:tag][@digest]. A colon inside the registry host +// (e.g. localhost:5000/app) is not a tag separator. +func parseImageRef(s string) imageRef { + ref := imageRef{} + rest := s + if i := strings.Index(rest, "@"); i >= 0 { + ref.digest = rest[i+1:] + rest = rest[:i] + } + if i := strings.LastIndex(rest, ":"); i > strings.LastIndex(rest, "/") { + ref.tag = rest[i+1:] + rest = rest[:i] + } + ref.name = rest + return ref +} + +func (r imageRef) String() string { + out := r.name + if r.tag != "" { + out += ":" + r.tag + } + if r.digest != "" { + out += "@" + r.digest + } + return out +} + +// imageSuppliers records which override entry last supplied each component of a +// rendered image; nil means the source file supplies it. +type imageSuppliers struct { + name *ImageOverride + tag *ImageOverride + digest *ImageOverride +} + +// renderImage runs the override chain over a source image, kustomize-style: +// each entry whose name matches the image's CURRENT name rewrites the +// components it declares, in chain order. +func renderImage(src imageRef, entries []ImageOverride) (imageRef, imageSuppliers) { + cur := src + var sup imageSuppliers + for i := range entries { + e := &entries[i] + if e.Name != cur.name { + continue + } + if e.HasNewName { + cur.name = e.NewName + sup.name = e + } + if e.HasNewTag { + cur.tag = e.NewTag + sup.tag = e + } + if e.HasDigest { + cur.digest = e.Digest + sup.digest = e + } + } + return cur, sup +} + +// containerSlot is one container-shaped list item holding an image, addressed +// by its list path plus container name so the live and Git objects align. +type containerSlot struct { + key string + item map[string]interface{} + image string +} + +func isContainerListKey(k string) bool { + switch k { + case "containers", "initContainers", "ephemeralContainers": + return true + default: + return false + } +} + +// collectContainerSlots walks the object for container lists at any depth, +// mirroring the builtin image transformer's generic traversal. Slots are sorted +// by key for deterministic edit output. +func collectContainerSlots(obj map[string]interface{}) []containerSlot { + var out []containerSlot + var walk func(prefix string, v interface{}) + walk = func(prefix string, v interface{}) { + switch t := v.(type) { + case map[string]interface{}: + for k, val := range t { + p := prefix + "/" + k + if isContainerListKey(k) { + if list, ok := val.([]interface{}); ok { + out = append(out, containerSlotsOf(p, list)...) + continue + } + } + walk(p, val) + } + case []interface{}: + for i, item := range t { + walk(fmt.Sprintf("%s/%d", prefix, i), item) + } + } + } + walk("", obj) + sort.Slice(out, func(i, j int) bool { return out[i].key < out[j].key }) + return out +} + +func containerSlotsOf(listPath string, list []interface{}) []containerSlot { + var out []containerSlot + for _, item := range list { + m, isMap := item.(map[string]interface{}) + if !isMap { + continue + } + name, _ := m["name"].(string) + image, hasImage := m["image"].(string) + if name == "" || !hasImage { + continue + } + out = append(out, containerSlot{key: listPath + "\x00" + name, item: m, image: image}) + } + return out +} + +// slotPlan is the per-container outcome of the inversion: the image the source +// file should hold, the live image it must render to, and the entry edits that +// make the render true. +type slotPlan struct { + slot containerSlot + fileImage string + live imageRef + edits []OverrideEdit +} + +// projectImages inverts the image chain for every container the live object and +// the Git document share. It mutates out's container images to their +// source-file form and returns the entry edits — or routes nothing (leaving the +// live values in out, today's write-through) when the inversion is unsafe: +// conflicting edits to one entry field, a removal an entry supplies, or a +// simulation that fails to reproduce live. +func projectImages( + gitRaw map[string]interface{}, + out *unstructured.Unstructured, + entries []ImageOverride, +) []OverrideEdit { + if len(entries) == 0 { + return nil + } + gitImages := map[string]string{} + for _, s := range collectContainerSlots(gitRaw) { + gitImages[s.key] = s.image + } + + var plans []slotPlan + for _, slot := range collectContainerSlots(out.Object) { + src, exists := gitImages[slot.key] + if !exists { + continue // a new container writes through; the supplier rule converges it later + } + plan, routable := invertImage(slot, src, entries) + if !routable { + return nil // one unroutable container abandons routing for the object + } + plans = append(plans, plan) + } + edits, ok := collectConsistentEdits(plans) + if !ok || !simulateImageRender(plans, entries, edits) { + return nil + } + for _, p := range plans { + p.slot.item["image"] = p.fileImage + } + return edits +} + +// invertImage computes one container's source-file image and entry edits. +// routable is false when a live change cannot be expressed on the existing +// entries (a component removal whose supplier is an entry). +func invertImage(slot containerSlot, src string, entries []ImageOverride) (slotPlan, bool) { + srcRef := parseImageRef(src) + rendered, sup := renderImage(srcRef, entries) + live := parseImageRef(slot.image) + plan := slotPlan{slot: slot, live: live} + if rendered == live { + plan.fileImage = src + return plan, true + } + + newSrc := srcRef + route := func(entry *ImageOverride, field, value string) { + plan.edits = append(plan.edits, OverrideEdit{ + KustomizationPath: entry.Source, + Edit: manifestedit.KustomizationEdit{ + Section: manifestedit.KustomizationSectionImages, + EntryIndex: entry.Index, + EntryName: entry.Name, + Field: field, + Value: value, + }, + }) + } + if live.name != rendered.name { + if sup.name != nil { + route(sup.name, "newName", live.name) + } else { + newSrc.name = live.name + } + } + if live.tag != rendered.tag { + switch { + case sup.tag == nil: + newSrc.tag = live.tag + case live.tag == "": + return plan, false // tag removal cannot be expressed on an entry + default: + route(sup.tag, "newTag", live.tag) + } + } + if live.digest != rendered.digest { + switch { + case sup.digest == nil: + newSrc.digest = live.digest + case live.digest == "": + return plan, false // digest removal cannot be expressed on an entry + default: + route(sup.digest, "digest", live.digest) + } + } + plan.fileImage = newSrc.String() + return plan, true +} + +// collectConsistentEdits dedupes the per-container edits, refusing when two +// containers demand different values for the same entry field. Output order is +// deterministic (path, section, index, field). +func collectConsistentEdits(plans []slotPlan) ([]OverrideEdit, bool) { + type key struct { + path string + section string + index int + field string + } + byKey := map[key]OverrideEdit{} + for _, p := range plans { + for _, e := range p.edits { + k := key{e.KustomizationPath, e.Edit.Section, e.Edit.EntryIndex, e.Edit.Field} + if prev, seen := byKey[k]; seen && prev.Edit.Value != e.Edit.Value { + return nil, false + } + byKey[k] = e + } + } + out := make([]OverrideEdit, 0, len(byKey)) + for _, e := range byKey { + out = append(out, e) + } + sort.Slice(out, func(i, j int) bool { + a, b := out[i], out[j] + if a.KustomizationPath != b.KustomizationPath { + return a.KustomizationPath < b.KustomizationPath + } + if a.Edit.Section != b.Edit.Section { + return a.Edit.Section < b.Edit.Section + } + if a.Edit.EntryIndex != b.Edit.EntryIndex { + return a.Edit.EntryIndex < b.Edit.EntryIndex + } + return a.Edit.Field < b.Edit.Field + }) + return out, true +} + +// simulateImageRender verifies the whole inversion: with the edits applied to a +// copy of the entries, every planned source image must render exactly to its +// live image. Chained entries can interact (a newName edit can change which +// later entry matches), so this closes the loop the per-component logic cannot. +func simulateImageRender(plans []slotPlan, entries []ImageOverride, edits []OverrideEdit) bool { + sim := make([]ImageOverride, len(entries)) + copy(sim, entries) + for _, oe := range edits { + for i := range sim { + if sim[i].Source != oe.KustomizationPath || sim[i].Index != oe.Edit.EntryIndex { + continue + } + switch oe.Edit.Field { + case "newName": + sim[i].NewName = oe.Edit.Value + case "newTag": + sim[i].NewTag = oe.Edit.Value + case "digest": + sim[i].Digest = oe.Edit.Value + } + } + } + for _, p := range plans { + rendered, _ := renderImage(parseImageRef(p.fileImage), sim) + if rendered != p.live { + return false + } + } + return true +} + +// replicaKinds are the kinds the builtin replica transformer touches. +func isReplicaKind(kind string) bool { + switch kind { + case "Deployment", "ReplicaSet", "StatefulSet": + return true + default: + return false + } +} + +// projectReplicas inverts the replicas transformer: when an entry pins this +// document's replica count, the source form of spec.replicas is restored on out +// (including its absence — the transformer creates the field) and a count edit +// is emitted only when live diverges from the pinned count. +func projectReplicas( + gitRaw map[string]interface{}, + out *unstructured.Unstructured, + entries []ReplicaOverride, +) []OverrideEdit { + if len(entries) == 0 || !isReplicaKind(out.GetKind()) { + return nil + } + sup := replicaSupplier(entries, out.GetName()) + if sup == nil { + return nil + } + liveCount, liveHas, err := unstructured.NestedInt64(out.Object, "spec", "replicas") + if err != nil || !liveHas { + return nil + } + + restoreSourceReplicas(gitRaw, out) + if liveCount == sup.Count { + return nil + } + return []OverrideEdit{{ + KustomizationPath: sup.Source, + Edit: manifestedit.KustomizationEdit{ + Section: manifestedit.KustomizationSectionReplicas, + EntryIndex: sup.Index, + EntryName: sup.Name, + Field: "count", + Value: strconv.FormatInt(liveCount, 10), + }, + }} +} + +// replicaSupplier is the last entry in the chain matching the document's name — +// the one whose count the render ends up with. +func replicaSupplier(entries []ReplicaOverride, name string) *ReplicaOverride { + var sup *ReplicaOverride + for i := range entries { + if entries[i].Name == name { + sup = &entries[i] + } + } + return sup +} + +// ReplicaCountEdit returns the entry edit that absorbs a live replica count for +// the document, when its override chain governs spec.replicas. The writer's +// field-patch path (the /scale subresource) uses it to route a scale to the +// kustomization entry instead of writing the count into the source manifest. +func ReplicaCountEdit(dm *DocumentModel, count int64) (OverrideEdit, bool) { + if dm == nil || dm.Overrides == nil || !isReplicaKind(dm.ManifestIdentity.Kind) { + return OverrideEdit{}, false + } + sup := replicaSupplier(dm.Overrides.Replicas, dm.ManifestIdentity.Name) + if sup == nil { + return OverrideEdit{}, false + } + return OverrideEdit{ + KustomizationPath: sup.Source, + Edit: manifestedit.KustomizationEdit{ + Section: manifestedit.KustomizationSectionReplicas, + EntryIndex: sup.Index, + EntryName: sup.Name, + Field: "count", + Value: strconv.FormatInt(count, 10), + }, + }, true +} + +// restoreSourceReplicas puts spec.replicas back to the source document's form on +// the desired copy: the source's own value when it has one, absent when the +// source omits it (the transformer supplies the field either way). +func restoreSourceReplicas(gitRaw map[string]interface{}, out *unstructured.Unstructured) { + spec, _ := gitRaw["spec"].(map[string]interface{}) + if src, ok := integerField(spec, "replicas"); ok { + _ = unstructured.SetNestedField(out.Object, src, "spec", "replicas") + return + } + unstructured.RemoveNestedField(out.Object, "spec", "replicas") +} diff --git a/internal/manifestanalyzer/overrides_projection_test.go b/internal/manifestanalyzer/overrides_projection_test.go new file mode 100644 index 00000000..e1812a5b --- /dev/null +++ b/internal/manifestanalyzer/overrides_projection_test.go @@ -0,0 +1,289 @@ +// SPDX-License-Identifier: Apache-2.0 + +package manifestanalyzer + +import ( + "testing" + + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + + "github.com/ConfigButler/gitops-reverser/internal/git/manifestedit" +) + +func deploymentObj(image string, replicas *int64) map[string]interface{} { + spec := map[string]interface{}{ + "template": map[string]interface{}{ + "spec": map[string]interface{}{ + "containers": []interface{}{ + map[string]interface{}{"name": "app", "image": image}, + }, + }, + }, + } + if replicas != nil { + spec["replicas"] = *replicas + } + return map[string]interface{}{ + "apiVersion": "apps/v1", + "kind": "Deployment", + "metadata": map[string]interface{}{"name": "web"}, + "spec": spec, + } +} + +func desiredOf(obj map[string]interface{}) *unstructured.Unstructured { + u := &unstructured.Unstructured{Object: obj} + // Live objects carry int64 replicas; normalize the fixture. + if v, ok, _ := unstructured.NestedFieldNoCopy(obj, "spec", "replicas"); ok { + if n, isInt := v.(int); isInt { + _ = unstructured.SetNestedField(obj, int64(n), "spec", "replicas") + } + } + return u +} + +func desiredImage(t *testing.T, u *unstructured.Unstructured) string { + t.Helper() + slots := collectContainerSlots(u.Object) + if len(slots) != 1 { + t.Fatalf("want exactly one container slot, got %d", len(slots)) + } + return slots[0].image +} + +func imgEntry(name string, set map[string]string) ImageOverride { + e := ImageOverride{Source: "kustomization.yaml", Name: name} + if v, ok := set["newName"]; ok { + e.NewName, e.HasNewName = v, true + } + if v, ok := set["newTag"]; ok { + e.NewTag, e.HasNewTag = v, true + } + if v, ok := set["digest"]; ok { + e.Digest, e.HasDigest = v, true + } + return e +} + +// TestSplitDesired_TagRoutedToEntry pins the core F1 behavior: a live tag change +// produced by a newTag entry lands on the entry and the file keeps its bytes. +func TestSplitDesired_TagRoutedToEntry(t *testing.T) { + git := deploymentObj("ghcr.io/example/app:1.0.0", nil) + desired := desiredOf(deploymentObj("ghcr.io/example/app:2.0.0", nil)) + ov := &KustomizeOverrides{Images: []ImageOverride{ + imgEntry("ghcr.io/example/app", map[string]string{"newTag": "1.5.0"}), + }} + + out, edits := SplitDesiredForOverrides(git, desired, ov) + if got := desiredImage(t, out); got != "ghcr.io/example/app:1.0.0" { + t.Errorf("file image = %q, want the source form restored", got) + } + if len(edits) != 1 { + t.Fatalf("want one entry edit, got %+v", edits) + } + e := edits[0] + if e.KustomizationPath != "kustomization.yaml" || + e.Edit != (manifestedit.KustomizationEdit{ + Section: manifestedit.KustomizationSectionImages, EntryIndex: 0, + EntryName: "ghcr.io/example/app", Field: "newTag", Value: "2.0.0", + }) { + t.Errorf("unexpected edit %+v", e) + } +} + +// TestSplitDesired_LiveMatchesRenderIsNoOp pins the write-through fix: live equal +// to the rendered value must restore the source form and route nothing, so the +// source file's "stale" tag is never overwritten. +func TestSplitDesired_LiveMatchesRenderIsNoOp(t *testing.T) { + git := deploymentObj("ghcr.io/example/app:1.0.0", nil) + desired := desiredOf(deploymentObj("ghcr.io/example/app:1.5.0", nil)) + ov := &KustomizeOverrides{Images: []ImageOverride{ + imgEntry("ghcr.io/example/app", map[string]string{"newTag": "1.5.0"}), + }} + + out, edits := SplitDesiredForOverrides(git, desired, ov) + if got := desiredImage(t, out); got != "ghcr.io/example/app:1.0.0" { + t.Errorf("file image = %q, want source form", got) + } + if len(edits) != 0 { + t.Errorf("want no edits, got %+v", edits) + } +} + +// TestSplitDesired_UngovernedComponentPatchesFile: the matching entry declares +// only newName, so a tag change is file-supplied and flows into the source image +// while the name stays in its source form. +func TestSplitDesired_UngovernedComponentPatchesFile(t *testing.T) { + git := deploymentObj("old/app:1.0.0", nil) + desired := desiredOf(deploymentObj("new/app:2.0.0", nil)) + ov := &KustomizeOverrides{Images: []ImageOverride{ + imgEntry("old/app", map[string]string{"newName": "new/app"}), + }} + + out, edits := SplitDesiredForOverrides(git, desired, ov) + if got := desiredImage(t, out); got != "old/app:2.0.0" { + t.Errorf("file image = %q, want source name with the live tag", got) + } + if len(edits) != 0 { + t.Errorf("tag is file-supplied here; want no edits, got %+v", edits) + } +} + +// TestSplitDesired_NameChangeRoutedToNewName: a live name change whose supplier +// is a newName entry updates the entry. +func TestSplitDesired_NameChangeRoutedToNewName(t *testing.T) { + git := deploymentObj("old/app:1.0.0", nil) + desired := desiredOf(deploymentObj("mirror/app:1.0.0", nil)) + ov := &KustomizeOverrides{Images: []ImageOverride{ + imgEntry("old/app", map[string]string{"newName": "new/app"}), + }} + + out, edits := SplitDesiredForOverrides(git, desired, ov) + if got := desiredImage(t, out); got != "old/app:1.0.0" { + t.Errorf("file image = %q, want untouched source", got) + } + if len(edits) != 1 || edits[0].Edit.Field != "newName" || edits[0].Edit.Value != "mirror/app" { + t.Fatalf("want one newName edit to mirror/app, got %+v", edits) + } +} + +// TestSplitDesired_RemovalUnroutable: live drops the digest an entry supplies; +// nothing can express that on the entry, so the whole object writes through. +func TestSplitDesired_RemovalUnroutable(t *testing.T) { + git := deploymentObj("app:1.0.0", nil) + desired := desiredOf(deploymentObj("app:1.0.0", nil)) + ov := &KustomizeOverrides{Images: []ImageOverride{ + imgEntry("app", map[string]string{"digest": "sha256:abc"}), + }} + + out, edits := SplitDesiredForOverrides(git, desired, ov) + if got := desiredImage(t, out); got != "app:1.0.0" { + t.Errorf("file image = %q, want the live value written through", got) + } + if len(edits) != 0 { + t.Errorf("want no edits on write-through, got %+v", edits) + } +} + +// TestSplitDesired_ConflictingContainersAbandonRouting: two containers governed +// by one entry cannot pin two different tags on it. +func TestSplitDesired_ConflictingContainersAbandonRouting(t *testing.T) { + containers := func(tagA, tagB string) map[string]interface{} { + return map[string]interface{}{ + "apiVersion": "apps/v1", + "kind": "Deployment", + "metadata": map[string]interface{}{"name": "web"}, + "spec": map[string]interface{}{ + "template": map[string]interface{}{ + "spec": map[string]interface{}{ + "containers": []interface{}{ + map[string]interface{}{"name": "a", "image": "app:" + tagA}, + map[string]interface{}{"name": "b", "image": "app:" + tagB}, + }, + }, + }, + }, + } + } + git := containers("1.0.0", "1.0.0") + desired := desiredOf(containers("2.0.0", "3.0.0")) + ov := &KustomizeOverrides{Images: []ImageOverride{ + imgEntry("app", map[string]string{"newTag": "1.5.0"}), + }} + + out, edits := SplitDesiredForOverrides(git, desired, ov) + if len(edits) != 0 { + t.Fatalf("conflicting demands must abandon routing, got %+v", edits) + } + slots := collectContainerSlots(out.Object) + if slots[0].image != "app:2.0.0" || slots[1].image != "app:3.0.0" { + t.Errorf("write-through must keep the live values, got %q / %q", slots[0].image, slots[1].image) + } +} + +// TestSplitDesired_ChainedEntriesCompose: a base renames the image, the parent +// pins the tag of the renamed image; a live tag change lands on the parent entry. +func TestSplitDesired_ChainedEntriesCompose(t *testing.T) { + git := deploymentObj("app:1.0.0", nil) + desired := desiredOf(deploymentObj("mirror/app:9.9.9", nil)) + base := imgEntry("app", map[string]string{"newName": "mirror/app"}) + base.Source = "base/kustomization.yaml" + parent := imgEntry("mirror/app", map[string]string{"newTag": "2.0.0"}) + ov := &KustomizeOverrides{Images: []ImageOverride{base, parent}} + + out, edits := SplitDesiredForOverrides(git, desired, ov) + if got := desiredImage(t, out); got != "app:1.0.0" { + t.Errorf("file image = %q, want untouched source", got) + } + if len(edits) != 1 || edits[0].KustomizationPath != "kustomization.yaml" || + edits[0].Edit.Field != "newTag" || edits[0].Edit.Value != "9.9.9" { + t.Fatalf("want one newTag edit on the parent entry, got %+v", edits) + } +} + +// TestSplitDesired_ReplicasRoutedToEntry: a pinned count absorbs the live scale; +// the file's replicas field is restored to its source form (absent here). +func TestSplitDesired_ReplicasRoutedToEntry(t *testing.T) { + five := int64(5) + git := deploymentObj("app:1.0.0", nil) // source has no spec.replicas + desired := desiredOf(deploymentObj("app:1.0.0", &five)) + ov := &KustomizeOverrides{Replicas: []ReplicaOverride{ + {Source: "kustomization.yaml", Index: 0, Name: "web", Count: 3}, + }} + + out, edits := SplitDesiredForOverrides(git, desired, ov) + if _, has, _ := unstructured.NestedInt64(out.Object, "spec", "replicas"); has { + t.Errorf("source has no replicas field, so the desired-for-file must not either") + } + if len(edits) != 1 || + edits[0].Edit != (manifestedit.KustomizationEdit{ + Section: manifestedit.KustomizationSectionReplicas, EntryIndex: 0, + EntryName: "web", Field: "count", Value: "5", + }) { + t.Fatalf("want one count edit to 5, got %+v", edits) + } +} + +// TestSplitDesired_ReplicasMatchRestoresSource: live equals the pinned count; the +// source's own (stale) value is restored and nothing is routed. +func TestSplitDesired_ReplicasMatchRestoresSource(t *testing.T) { + one, three := int64(1), int64(3) + git := deploymentObj("app:1.0.0", &one) + desired := desiredOf(deploymentObj("app:1.0.0", &three)) + ov := &KustomizeOverrides{Replicas: []ReplicaOverride{ + {Source: "kustomization.yaml", Index: 0, Name: "web", Count: 3}, + }} + + out, edits := SplitDesiredForOverrides(git, desired, ov) + if got, _, _ := unstructured.NestedInt64(out.Object, "spec", "replicas"); got != 1 { + t.Errorf("desired-for-file replicas = %d, want the source's 1", got) + } + if len(edits) != 0 { + t.Errorf("want no edits, got %+v", edits) + } +} + +// TestSplitDesired_ReplicasIgnoresOtherKinds: the replica transformer only +// touches Deployment/ReplicaSet/StatefulSet. +func TestSplitDesired_ReplicasIgnoresOtherKinds(t *testing.T) { + git := map[string]interface{}{ + "apiVersion": "v1", "kind": "ConfigMap", + "metadata": map[string]interface{}{"name": "web"}, + } + desired := desiredOf(map[string]interface{}{ + "apiVersion": "v1", "kind": "ConfigMap", + "metadata": map[string]interface{}{"name": "web"}, + "spec": map[string]interface{}{"replicas": int64(5)}, + }) + ov := &KustomizeOverrides{Replicas: []ReplicaOverride{ + {Source: "kustomization.yaml", Index: 0, Name: "web", Count: 3}, + }} + + out, edits := SplitDesiredForOverrides(git, desired, ov) + if got, _, _ := unstructured.NestedInt64(out.Object, "spec", "replicas"); got != 5 { + t.Errorf("non-workload kinds are untouched, got replicas %d", got) + } + if len(edits) != 0 { + t.Errorf("want no edits, got %+v", edits) + } +} diff --git a/internal/manifestanalyzer/overrides_test.go b/internal/manifestanalyzer/overrides_test.go new file mode 100644 index 00000000..29f244f1 --- /dev/null +++ b/internal/manifestanalyzer/overrides_test.go @@ -0,0 +1,141 @@ +// SPDX-License-Identifier: Apache-2.0 + +package manifestanalyzer + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/ConfigButler/gitops-reverser/internal/git/manifestedit" + "github.com/ConfigButler/gitops-reverser/internal/typeset" +) + +// The corpus tests pin override-chain attribution on the example folders: a +// single-root overlay attaches its entries, a nested base composes +// innermost-first, and two roots with differing chains attach nothing and emit +// the ambiguity diagnostic. See +// docs/design/gitops-api/f1-images-replicas-edit-through.md. + +func corpusDeployment(t *testing.T, store *ManifestStore, ns string) *DocumentModel { + t.Helper() + dm := store.ByManifestIdentity[manifestedit.Identity{ + APIVersion: "apps/v1", Kind: "Deployment", Namespace: ns, Name: "web", + }] + if dm == nil { + t.Fatalf("Deployment web should be indexed under namespace %q", ns) + } + return dm +} + +func TestKustomizeOverridesCorpus_ImagesOverlay(t *testing.T) { + store := corpusStore(t, "supported/images-overlay") + dm := corpusDeployment(t, store, "app") + if dm.Overrides == nil || len(dm.Overrides.Images) != 1 || len(dm.Overrides.Replicas) != 0 { + t.Fatalf("want exactly one image override, got %+v", dm.Overrides) + } + img := dm.Overrides.Images[0] + if img.Source != "kustomization.yaml" || img.Name != "ghcr.io/example/podinfo" { + t.Errorf("entry source/name = %q/%q, want kustomization.yaml/ghcr.io/example/podinfo", + img.Source, img.Name) + } + if !img.HasNewTag || img.NewTag != "6.5.0" || img.HasNewName || img.HasDigest { + t.Errorf("entry should declare only newTag 6.5.0, got %+v", img) + } + if hasOverrideAmbiguityDiag(store) { + t.Errorf("single-root overlay must not be ambiguous") + } +} + +func TestKustomizeOverridesCorpus_ReplicasOverlayChain(t *testing.T) { + store := corpusStore(t, "supported/replicas-overlay") + dm := corpusDeployment(t, store, "app") + if dm.Overrides == nil || len(dm.Overrides.Images) != 1 || len(dm.Overrides.Replicas) != 1 { + t.Fatalf("want one image + one replica override from the chain, got %+v", dm.Overrides) + } + // Build order: the base's own entries come before the referencing root's. + img := dm.Overrides.Images[0] + if img.Source != "base/kustomization.yaml" { + t.Errorf("innermost (base) images entry should come first, got source %q", img.Source) + } + if !img.HasNewName || img.NewName != "ghcr.io/example/podinfo-mirror" { + t.Errorf("base entry should declare newName, got %+v", img) + } + rep := dm.Overrides.Replicas[0] + if rep.Source != "kustomization.yaml" || rep.Name != "web" || rep.Count != 3 { + t.Errorf("replica entry = %+v, want web:3 from kustomization.yaml", rep) + } +} + +func TestKustomizeOverridesCorpus_AmbiguousImages(t *testing.T) { + store := corpusStore(t, "unsupported/ambiguous-images") + dm := corpusDeployment(t, store, "app") + if dm.Overrides != nil { + t.Errorf("distinct chains from two roots must attach no overrides, got %+v", dm.Overrides) + } + if !hasOverrideAmbiguityDiag(store) { + t.Errorf("want an %s diagnostic", reasonAmbiguousOverrides) + } +} + +// TestKustomizeOverridesNestedBaseIsNotARoot pins the render-root rule: a base +// referenced by another kustomization is not walked as its own root, so the +// nested layout yields ONE chain (base+parent composed), not two conflicting ones. +func TestKustomizeOverridesNestedBaseIsNotARoot(t *testing.T) { + store := corpusStore(t, "supported/replicas-overlay") + for _, d := range store.Diagnostics { + if d.Reason == reasonAmbiguousOverrides { + t.Fatalf("nested base must not create an ambiguous chain: %s", d.Message) + } + } +} + +// TestKustomizationOverrideParsing pins the malformed-overrides boundary: a +// kustomization whose images:/replicas: cannot be parsed is unsupported (it +// would fail kustomize build, and we can no longer vouch for the render), while +// well-formed entries keep the folder supported. +func TestKustomizationOverrideParsing(t *testing.T) { + cases := []struct { + name string + content string + unsupported bool + }{ + {"well-formed images", "images:\n - name: a/b\n newTag: \"1.2\"\n", false}, + {"well-formed replicas", "replicas:\n - name: web\n count: 3\n", false}, + {"empty images list", "images: []\n", false}, + {"images not a list", "images: oops\n", true}, + {"images entry not a map", "images:\n - just-a-string\n", true}, + {"images entry missing name", "images:\n - newTag: \"1.2\"\n", true}, + {"images unknown key", "images:\n - name: a/b\n newtag: \"1.2\"\n", true}, + {"non-string newTag", "images:\n - name: a/b\n newTag: 1.29\n", true}, + {"blank newName", "images:\n - name: a/b\n newName: \"\"\n", true}, + {"replicas count string", "replicas:\n - name: web\n count: \"3\"\n", true}, + {"replicas count fractional", "replicas:\n - name: web\n count: 2.5\n", true}, + {"replicas count negative", "replicas:\n - name: web\n count: -1\n", true}, + {"replicas unknown key", "replicas:\n - name: web\n count: 3\n kind: Deployment\n", true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := kustomizationUsesUnsupportedFeature([]byte(tc.content)); got != tc.unsupported { + t.Errorf("kustomizationUsesUnsupportedFeature = %v, want %v", got, tc.unsupported) + } + }) + } +} + +func corpusStore(t *testing.T, dir string) *ManifestStore { + t.Helper() + mapper := typeset.NewSnapshotRegistry(sampleClusterSnapshot()) + fsys := os.DirFS(filepath.Join("testdata", "contextual-namespace", dir)) + return BuildStore(context.Background(), fsys, mapper) +} + +func hasOverrideAmbiguityDiag(store *ManifestStore) bool { + for _, d := range store.Diagnostics { + if d.Reason == reasonAmbiguousOverrides { + return true + } + } + return false +} diff --git a/internal/manifestanalyzer/store.go b/internal/manifestanalyzer/store.go index 77e02ce1..4638e93f 100644 --- a/internal/manifestanalyzer/store.go +++ b/internal/manifestanalyzer/store.go @@ -161,6 +161,15 @@ type DocumentModel struct { // See NamespaceSourceKind. NamespaceSource NamespaceSource + // Overrides is the unambiguous kustomize images/replicas override chain + // governing this document's file, in build order — nil when no supported + // render root supplies one, or when distinct roots disagree (the + // ambiguous-kustomize-overrides diagnostic). The writer routes a live change + // produced by one of these entries back to the entry instead of writing it + // through into the source document. See + // docs/design/gitops-api/f1-images-replicas-edit-through.md. + Overrides *KustomizeOverrides + // ResourceIdentity is the API-side identity (GVR + namespace + name). It is set // only when the injected GVK->GVR mapper resolves the document's GVK to a single // served, allowed resource; structure-only analysis (and any unresolved lookup) @@ -344,7 +353,10 @@ func buildStore( } yamlFiles := scan.YAMLFiles inv, indexDiags := manifestedit.IndexFiles(yamlFiles) - nsAssignments := kustomizeNamespaceAssignments(yamlFiles) + kusts := parseKustomizations(yamlFiles) + resourceFiles := resourceFilePaths(yamlFiles) + nsAssignments := kustomizeNamespaceAssignments(kusts, resourceFiles) + ovAssignments := kustomizeOverrideAssignments(kusts, resourceFiles) store := &ManifestStore{ FilesByPath: map[string]*FileModel{}, @@ -375,7 +387,7 @@ func buildStore( }) continue } - store.materialize(ctx, r, lookup, nsAssignments) + store.materialize(ctx, r, lookup, nsAssignments, ovAssignments) } // Record every allowlisted file with no named record as a whole-file retention, @@ -444,12 +456,17 @@ func (s *ManifestStore) materialize( r manifestedit.DocumentRecord, lookup typeset.Lookup, nsAssignments map[string]namespaceAssignment, + ovAssignments map[string]*overrideAssignment, ) { gvk := gvkOf(r.Identity) identity, nsSource, diag := resolveNamespaceContext(ctx, r.Identity, gvk, lookup, r.Location, nsAssignments) if diag != nil { s.Diagnostics = append(s.Diagnostics, *diag) } + overrides, ovDiag := resolveOverrides(r.Location, ovAssignments) + if ovDiag != nil { + s.Diagnostics = append(s.Diagnostics, *ovDiag) + } fm := s.FilesByPath[r.Location.Path] if fm == nil { @@ -459,6 +476,7 @@ func (s *ManifestStore) materialize( dm := &DocumentModel{ ManifestIdentity: identity, NamespaceSource: nsSource, + Overrides: overrides, Editable: r.Editable && !r.Encrypted, Cause: causeFor(r), } @@ -579,10 +597,12 @@ type namespaceAssignment struct { // a namespace source). See the "Kustomize subset proposal" in // docs/design/manifest/contextual-namespace-and-kustomize-folder-editing.md. type kustomizationDoc struct { - path string // kustomization file path (slash) - namespace string // the namespace: transformer value - resources []string // resources + bases entries, raw and relative to the file's dir - unsupported bool // uses generators/patches/components/remote bases/name(pre|suf)fix/... + path string // kustomization file path (slash) + namespace string // the namespace: transformer value + resources []string // resources + bases entries, raw and relative to the file's dir + images []ImageOverride // parsed images: entries, in listed order + replicas []ReplicaOverride // parsed replicas: entries, in listed order + unsupported bool // uses generators/patches/components/remote bases/name(pre|suf)fix/... } // kustomizeNamespaceAssignments walks each supported kustomization as a render root and @@ -591,10 +611,10 @@ type kustomizationDoc struct { // overrides a child's namespace) accumulates both, which resolveNamespaceContext then // refuses as ambiguous. Following the graph — not the nearest kustomization on disk — // is the safety property the design doc requires. -func kustomizeNamespaceAssignments(files []manifestedit.FileContent) map[string]namespaceAssignment { - kusts := parseKustomizations(files) - resourceFiles := resourceFilePaths(files) - +func kustomizeNamespaceAssignments( + kusts map[string]*kustomizationDoc, + resourceFiles map[string]struct{}, +) map[string]namespaceAssignment { // nsByFile[file][namespace] = kustomization path that first assigned it. nsByFile := map[string]map[string]string{} for dir, root := range kusts { @@ -704,7 +724,11 @@ func parseKustomizations(files []manifestedit.FileContent) map[string]*kustomiza } doc.namespace = strings.TrimSpace(stringField(raw, "namespace")) doc.resources = append(stringList(raw, "resources"), stringList(raw, "bases")...) - doc.unsupported = hasUnsupportedKustomizeFeature(raw) || hasRemoteResource(doc.resources) + images, imagesOK := parseImageOverrides(raw, doc.path) + replicas, replicasOK := parseReplicaOverrides(raw, doc.path) + doc.images, doc.replicas = images, replicas + doc.unsupported = hasUnsupportedKustomizeFeature(raw) || hasRemoteResource(doc.resources) || + !imagesOK || !replicasOK out[slashDir(f.Path)] = doc } return out @@ -721,14 +745,17 @@ func kustomizationUsesUnsupportedFeature(content []byte) bool { return true } resources := append(stringList(raw, "resources"), stringList(raw, "bases")...) - return hasUnsupportedKustomizeFeature(raw) || hasRemoteResource(resources) + _, imagesOK := parseImageOverrides(raw, "") + _, replicasOK := parseReplicaOverrides(raw, "") + return hasUnsupportedKustomizeFeature(raw) || hasRemoteResource(resources) || !imagesOK || !replicasOK } // hasUnsupportedKustomizeFeature reports whether a kustomization uses a field that // creates resources or mutates 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; benign transformers -// (labels, annotations, images) do not. +// (labels, annotations) do not. images/replicas are parsed separately (overrides.go) +// and disqualify only when malformed. func hasUnsupportedKustomizeFeature(raw map[string]interface{}) bool { unsupported := []string{ "generators", "configMapGenerator", "secretGenerator", diff --git a/internal/manifestanalyzer/testdata/contextual-namespace/README.md b/internal/manifestanalyzer/testdata/contextual-namespace/README.md index 49eeaefe..85eef272 100644 --- a/internal/manifestanalyzer/testdata/contextual-namespace/README.md +++ b/internal/manifestanalyzer/testdata/contextual-namespace/README.md @@ -12,6 +12,10 @@ asserts the outcome. None`); the ambiguous case also emits an `ambiguous-namespace` diagnostic. These are the inputs the pending `RepositoryValid` refusal will fail the GitTarget on. +The `images-overlay`, `replicas-overlay`, and `ambiguous-images` folders pin the +F1 override-chain attribution the same way (`overrides_test.go`); see +`docs/design/gitops-api/f1-images-replicas-edit-through.md`. + See `docs/design/manifest/contextual-namespace-and-kustomize-folder-editing.md` (the "Supported and unsupported example folders" matrix). Add a new folder here whenever a new "can we support X?" question comes up. diff --git a/internal/manifestanalyzer/testdata/contextual-namespace/supported/images-overlay/deployment.yaml b/internal/manifestanalyzer/testdata/contextual-namespace/supported/images-overlay/deployment.yaml new file mode 100644 index 00000000..16814cd7 --- /dev/null +++ b/internal/manifestanalyzer/testdata/contextual-namespace/supported/images-overlay/deployment.yaml @@ -0,0 +1,17 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: web +spec: + replicas: 1 + selector: + matchLabels: + app: web + template: + metadata: + labels: + app: web + spec: + containers: + - name: podinfo + image: ghcr.io/example/podinfo:6.4.0 diff --git a/internal/manifestanalyzer/testdata/contextual-namespace/supported/images-overlay/kustomization.yaml b/internal/manifestanalyzer/testdata/contextual-namespace/supported/images-overlay/kustomization.yaml new file mode 100644 index 00000000..3f5166ff --- /dev/null +++ b/internal/manifestanalyzer/testdata/contextual-namespace/supported/images-overlay/kustomization.yaml @@ -0,0 +1,8 @@ +# Single-root overlay pinning an image tag: the entry, not the source file, +# owns the deployed tag. +namespace: app +resources: + - deployment.yaml +images: + - name: ghcr.io/example/podinfo + newTag: "6.5.0" diff --git a/internal/manifestanalyzer/testdata/contextual-namespace/supported/replicas-overlay/base/deployment.yaml b/internal/manifestanalyzer/testdata/contextual-namespace/supported/replicas-overlay/base/deployment.yaml new file mode 100644 index 00000000..cd4ca8db --- /dev/null +++ b/internal/manifestanalyzer/testdata/contextual-namespace/supported/replicas-overlay/base/deployment.yaml @@ -0,0 +1,16 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: web +spec: + selector: + matchLabels: + app: web + template: + metadata: + labels: + app: web + spec: + containers: + - name: podinfo + image: ghcr.io/example/podinfo:6.4.0 diff --git a/internal/manifestanalyzer/testdata/contextual-namespace/supported/replicas-overlay/base/kustomization.yaml b/internal/manifestanalyzer/testdata/contextual-namespace/supported/replicas-overlay/base/kustomization.yaml new file mode 100644 index 00000000..441c0783 --- /dev/null +++ b/internal/manifestanalyzer/testdata/contextual-namespace/supported/replicas-overlay/base/kustomization.yaml @@ -0,0 +1,5 @@ +resources: + - deployment.yaml +images: + - name: ghcr.io/example/podinfo + newName: ghcr.io/example/podinfo-mirror diff --git a/internal/manifestanalyzer/testdata/contextual-namespace/supported/replicas-overlay/kustomization.yaml b/internal/manifestanalyzer/testdata/contextual-namespace/supported/replicas-overlay/kustomization.yaml new file mode 100644 index 00000000..7bc1cc29 --- /dev/null +++ b/internal/manifestanalyzer/testdata/contextual-namespace/supported/replicas-overlay/kustomization.yaml @@ -0,0 +1,7 @@ +# Single-root overlay pinning a replica count via the replicas transformer. +namespace: app +resources: + - base +replicas: + - name: web + count: 3 diff --git a/internal/manifestanalyzer/testdata/contextual-namespace/unsupported/ambiguous-images/kustomization.yaml b/internal/manifestanalyzer/testdata/contextual-namespace/unsupported/ambiguous-images/kustomization.yaml new file mode 100644 index 00000000..d8a145be --- /dev/null +++ b/internal/manifestanalyzer/testdata/contextual-namespace/unsupported/ambiguous-images/kustomization.yaml @@ -0,0 +1,7 @@ +# Two render roots reach shared.yaml with different override chains: no chain +# can be trusted, so no override routing applies (ambiguous-kustomize-overrides). +resources: + - shared.yaml +images: + - name: ghcr.io/example/podinfo + newTag: "1.0.0" diff --git a/internal/manifestanalyzer/testdata/contextual-namespace/unsupported/ambiguous-images/other/kustomization.yaml b/internal/manifestanalyzer/testdata/contextual-namespace/unsupported/ambiguous-images/other/kustomization.yaml new file mode 100644 index 00000000..a6946ffd --- /dev/null +++ b/internal/manifestanalyzer/testdata/contextual-namespace/unsupported/ambiguous-images/other/kustomization.yaml @@ -0,0 +1,5 @@ +resources: + - ../shared.yaml +images: + - name: ghcr.io/example/podinfo + newTag: "2.0.0" diff --git a/internal/manifestanalyzer/testdata/contextual-namespace/unsupported/ambiguous-images/shared.yaml b/internal/manifestanalyzer/testdata/contextual-namespace/unsupported/ambiguous-images/shared.yaml new file mode 100644 index 00000000..4c8e0c3d --- /dev/null +++ b/internal/manifestanalyzer/testdata/contextual-namespace/unsupported/ambiguous-images/shared.yaml @@ -0,0 +1,17 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: web + namespace: app +spec: + selector: + matchLabels: + app: web + template: + metadata: + labels: + app: web + spec: + containers: + - name: podinfo + image: ghcr.io/example/podinfo:6.4.0 From d81a07342cc82cbcfbee7e195848f697e37f3ccd Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Mon, 6 Jul 2026 07:51:52 +0000 Subject: [PATCH 2/2] fix(kustomize): record every override chain path; surface silent fallbacks Review follow-ups for #198: - override-chain cycle protection moves from a per-walk visited set to on-path tracking, so a diamond (one render root reaching a shared base through two overlays) records both chains and trips the ambiguous-kustomize-overrides refusal instead of silently attributing the first path (CodeRabbit major); adds the diamond-images corpus fixture - the write batch logs store-build diagnostics (warning+) once per batch as "manifest store diagnostic", so ambiguity/scope fallbacks the writer takes silently leave a trace in the live path, not just the analyzer CLI - targeted tests for behavior that was only implicitly covered: DocumentBody, digest routing, applyOverrideEdits refusal paths, and two resync integration tests (governed folder in sync -> zero churn; governed drift -> routed to the entry) - docs: gitops-api README marks F1 implemented (CodeRabbit minor), the F1 doc describes the diamond-safe walk and corrects the diagnostic-visibility phrasing, and configuration.md gains a user-facing "Kustomize support in the target path" section (supported subset / loud refusals / silent fallbacks and where to see them) Validation: task lint (0 issues), task test (coverage 74.4% -> 74.5%, baseline raised), task test-e2e (53 passed, 0 failed, 8 skipped). Co-Authored-By: Claude Fable 5 --- .coverage-baseline | 2 +- docs/configuration.md | 32 +++++++++ docs/design/gitops-api/README.md | 2 +- .../f1-images-replicas-edit-through.md | 14 ++-- internal/git/inplace_overrides_test.go | 72 +++++++++++++++++++ .../git/manifestedit/kustomization_test.go | 14 ++++ internal/git/plan_flush.go | 21 ++++++ internal/manifestanalyzer/overrides.go | 14 +++- .../overrides_projection_test.go | 22 ++++++ internal/manifestanalyzer/overrides_test.go | 26 +++++-- .../diamond-images/a/kustomization.yaml | 5 ++ .../diamond-images/b/kustomization.yaml | 5 ++ .../diamond-images/base/deployment.yaml | 17 +++++ .../diamond-images/base/kustomization.yaml | 2 + .../diamond-images/kustomization.yaml | 7 ++ 15 files changed, 240 insertions(+), 15 deletions(-) create mode 100644 internal/manifestanalyzer/testdata/contextual-namespace/unsupported/diamond-images/a/kustomization.yaml create mode 100644 internal/manifestanalyzer/testdata/contextual-namespace/unsupported/diamond-images/b/kustomization.yaml create mode 100644 internal/manifestanalyzer/testdata/contextual-namespace/unsupported/diamond-images/base/deployment.yaml create mode 100644 internal/manifestanalyzer/testdata/contextual-namespace/unsupported/diamond-images/base/kustomization.yaml create mode 100644 internal/manifestanalyzer/testdata/contextual-namespace/unsupported/diamond-images/kustomization.yaml diff --git a/.coverage-baseline b/.coverage-baseline index 4955b667..1927d47b 100644 --- a/.coverage-baseline +++ b/.coverage-baseline @@ -1 +1 @@ -74.4 +74.5 diff --git a/docs/configuration.md b/docs/configuration.md index c7c330ce..81d706bc 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -382,6 +382,38 @@ Entries are `resource` for the core API group or `group/resource` for grouped AP API version, so a served CRD version change does not change the sensitive classification. The custom resource still needs a `GitTarget` with `spec.encryption` configured before Git writes can succeed. +### Kustomize support in the target path + +A target path may contain `kustomization.yaml` files. The operator retains them as build directives +(it never sweeps them) and understands a deliberately small, round-trippable subset: + +- **`namespace:` + `resources:`/`bases`** (local files and directory bases): a namespace-less + resource file inherits its namespace from the kustomization that references it, and + `metadata.namespace` is kept out of the file on write. +- **`images:` and `replicas:` overrides**: a live change *produced by* an override entry — an image + tag, name, or digest pinned by `images:`, or a replica count pinned by `replicas:` (including + `kubectl scale`) — is written back **to that entry**, preserving comments, and the source manifest + keeps its bytes. Only fields the entry already declares are updated; the operator never adds or + removes entries. Note that one entry is a shared knob, exactly as in kustomize itself: updating it + affects every resource in the build whose image matches. + +Anything outside that subset **refuses the whole target path before anything is written**: +`patches`/`patchesStrategicMerge`/`patchesJson6902`, generators, `components`, Helm fields, +`replacements`, `transformers`, `namePrefix`/`nameSuffix`, remote bases, and `images:`/`replicas:` +values that do not parse (those would fail `kustomize build` too). A refusal is loud: the target +reports `GitPathAccepted=False`, `Stalled=True`, and `Ready=False` with reason `UnsupportedContent` +until the path is cleaned up. + +Two situations fall back to plain in-place editing of the source manifest instead of refusing: +a resource file reachable from more than one render root with differing override chains +(ambiguous — the operator will not guess which chain governs), and a live change an entry cannot +express (for example a removed digest, or two containers demanding different values from one +entry). These fallbacks are recorded as store diagnostics — visible in the analyzer CLI and, for +the running operator, in the logs at debug verbosity (`manifest store diagnostic`). + +For design details and the exact boundary, see +[design/gitops-api/f1-images-replicas-edit-through.md](design/gitops-api/f1-images-replicas-edit-through.md). + ## `WatchRule` `WatchRule` is the normal namespaced watcher. It only watches resources in its own namespace and diff --git a/docs/design/gitops-api/README.md b/docs/design/gitops-api/README.md index b11b66b4..a089e195 100644 --- a/docs/design/gitops-api/README.md +++ b/docs/design/gitops-api/README.md @@ -44,7 +44,7 @@ when work starts. | # | Feature | Design doc | Status | |---|---------|-----------|--------| -| F1 | Kustomize `images:` / `replicas:` edit-through — a live change produced by an override entry is written back to that entry, never through into the source manifest | [f1-images-replicas-edit-through.md](f1-images-replicas-edit-through.md) | in progress | +| F1 | Kustomize `images:` / `replicas:` edit-through — a live change produced by an override entry is written back to that entry, never through into the source manifest | [f1-images-replicas-edit-through.md](f1-images-replicas-edit-through.md) | implemented ([#198](https://github.com/ConfigButler/gitops-reverser/pull/198)) | | F2 | Render-root scoping — a GitTarget declares its render root (e.g. `overlays/env1`); base files reached through `../../base` become read-only context, dissolving overlay fan-out ambiguity | — | not designed | | F3 | Restricted patch authoring — write/update scalar-field strategic-merge patches in an overlay ("live drift lands in the environment's overlay") | — | not designed | | F4 | New-file placement rules — sibling inference + `spec.newFilePath` template so new resources land in the folder's convention, not the canonical REST path | designed in [version2/gittarget-new-file-placement-rules.md](../manifest/version2/gittarget-new-file-placement-rules.md) | designed, unbuilt | diff --git a/docs/design/gitops-api/f1-images-replicas-edit-through.md b/docs/design/gitops-api/f1-images-replicas-edit-through.md index b81e3879..0c2f88a5 100644 --- a/docs/design/gitops-api/f1-images-replicas-edit-through.md +++ b/docs/design/gitops-api/f1-images-replicas-edit-through.md @@ -108,7 +108,11 @@ resync mark-and-sweep upsert funnel through it, so fixing it fixes both. present-but-unparseable marks the kustomization unsupported (Decision 3). - The existing render-root graph walk (`assignFromRoot`) additionally records, per resource file, the ordered **override chain**: the kustomizations along - the reference path (innermost → root) that carry any override entries. + the reference path (innermost → root) that carry any override entries. Every + distinct path within a root is recorded (cycle protection is on the current + path, not per walk), so a diamond — one root reaching a shared base through + two overlays — trips the ambiguity refusal instead of silently attributing + the first path. - `DocumentModel` gains `Overrides *KustomizeOverrides` — the collapsed, unambiguous chain (nil when none or ambiguous), each entry carrying its source kustomization path so the writer knows which file to edit. @@ -146,9 +150,11 @@ object, and the override chain, return - `docs/architecture.md`: the Manifest Aware Writer and acceptance-gate sections document the supported subset change ("images/replicas overrides are understood and edited through"). -- Log one line per override edit (kustomization path, entry, field); the - ambiguous case is visible via the diagnostic. A GitTarget status surface - for override routing is deferred. +- Log one line per override edit (kustomization path, entry, field). The + ambiguity fallback is recorded as a store diagnostic, surfaced by the + analyzer CLI / scan report and logged by the live writer once per write + batch at debug verbosity (`manifest store diagnostic`). A GitTarget status + surface for override routing is deferred. ## Known limitation: shared entries with divergent live consumers diff --git a/internal/git/inplace_overrides_test.go b/internal/git/inplace_overrides_test.go index d8a04db5..94936055 100644 --- a/internal/git/inplace_overrides_test.go +++ b/internal/git/inplace_overrides_test.go @@ -3,6 +3,7 @@ package git import ( + "context" "os" "path/filepath" "testing" @@ -13,6 +14,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "github.com/ConfigButler/gitops-reverser/internal/git/manifestedit" + "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" "github.com/ConfigButler/gitops-reverser/internal/types" "github.com/ConfigButler/gitops-reverser/internal/typeset" ) @@ -200,6 +202,76 @@ func TestPlanFlush_RoutesScaleFieldPatchToKustomizationEntry(t *testing.T) { assert.Contains(t, string(kust), "count: 7") } +// An override edit that cannot land (the entry lacks the field, or the +// kustomization is not in the subtree) is dropped without touching any buffer — +// the source file was already left in source form, so the next event re-decides. +func TestApplyOverrideEdits_SkipLeavesBuffersUntouched(t *testing.T) { + kust := "images:\n - name: app\n newName: mirror/app\n" + scan := manifestanalyzer.FolderScan{YAMLFiles: []manifestedit.FileContent{ + {Path: "kustomization.yaml", Content: []byte(kust)}, + }} + wb := newWriteBatch(context.Background(), newContentWriter(types.SensitiveResourcePolicy{}), nil, scan) + + fieldMissing := manifestanalyzer.OverrideEdit{ + KustomizationPath: "kustomization.yaml", + Edit: manifestedit.KustomizationEdit{ + Section: manifestedit.KustomizationSectionImages, EntryIndex: 0, + EntryName: "app", Field: "newTag", Value: "2.0.0", + }, + } + fileMissing := manifestanalyzer.OverrideEdit{ + KustomizationPath: "gone/kustomization.yaml", + Edit: fieldMissing.Edit, + } + changed := wb.applyOverrideEdits(context.Background(), Event{}, []manifestanalyzer.OverrideEdit{ + fieldMissing, fileMissing, + }) + assert.False(t, changed, "no edit can land, so no buffer may change") + assert.Equal(t, kust, string(wb.buffer("kustomization.yaml").current), + "a refused edit must leave the kustomization bytes untouched") +} + +// A resync over a governed folder whose live state equals the overlay render is +// a complete no-op: no churn on the source manifest OR the kustomization. This +// pins the mark-and-sweep path (M8) explicitly, not just via shared code. +func TestResync_GovernedFolderInSyncIsNoOp(t *testing.T) { + writer := newContentWriter(types.SensitiveResourcePolicy{}) + worktree := newWorktreeForTest(t) + deployPath, kustPath := seedOverridesWorktree(t, worktree.Filesystem.Root()) + + stats, changed := applyResyncViaWorktree(t, writer, deploymentMapper(), worktree, + desiredOverridesDeployment("ghcr.io/example/podinfo:6.4.0", 3)) + assert.False(t, changed, "live == render must resync to zero changes") + assert.Zero(t, stats.Created+stats.Updated+stats.Deleted) + assertFileBytes(t, deployPath, overridesDeploymentYAML, "resync must not churn the source manifest") + assertFileBytes(t, kustPath, overridesKustomizationYAML, "resync must not churn the kustomization") +} + +// A resync that finds governed drift routes it to the kustomization entry, just +// like the steady-state path. +func TestResync_GovernedDriftRoutesToKustomizationEntry(t *testing.T) { + writer := newContentWriter(types.SensitiveResourcePolicy{}) + worktree := newWorktreeForTest(t) + deployPath, kustPath := seedOverridesWorktree(t, worktree.Filesystem.Root()) + + stats, changed := applyResyncViaWorktree(t, writer, deploymentMapper(), worktree, + desiredOverridesDeployment("ghcr.io/example/podinfo:6.5.0", 3)) + require.True(t, changed) + assert.Equal(t, 1, stats.Updated, "the routed edit counts as one update") + assertFileBytes(t, deployPath, overridesDeploymentYAML, "the source manifest keeps its bytes") + + kust, err := os.ReadFile(kustPath) + require.NoError(t, err) + assert.Contains(t, string(kust), `newTag: "6.5.0"`) +} + +// desiredOverridesDeployment adapts the live Deployment fixture into a resync +// snapshot entry. +func desiredOverridesDeployment(image string, replicas int64) manifestanalyzer.DesiredResource { + event := overridesDeploymentEvent(image, replicas) + return manifestanalyzer.DesiredResource{Resource: event.Identifier, Object: event.Object} +} + // A change no entry governs keeps today's behavior: it is patched into the // source manifest and the kustomization stays untouched. func TestPlanFlush_UngovernedChangeStillPatchesSourceFile(t *testing.T) { diff --git a/internal/git/manifestedit/kustomization_test.go b/internal/git/manifestedit/kustomization_test.go index ebab8206..b976c383 100644 --- a/internal/git/manifestedit/kustomization_test.go +++ b/internal/git/manifestedit/kustomization_test.go @@ -99,6 +99,20 @@ func TestPatchKustomization_SameValueIsNoChange(t *testing.T) { } } +func TestDocumentBody(t *testing.T) { + content := []byte("a: 1\n---\n# second\nb: 2\n") + body, ok := DocumentBody(content, 1) + if !ok || string(body) != "# second\nb: 2\n" { + t.Errorf("DocumentBody(1) = %q, %v; want the exact second-document bytes", body, ok) + } + if _, ok := DocumentBody(content, 2); ok { + t.Errorf("an out-of-range index must report ok=false") + } + if _, ok := DocumentBody(content, -1); ok { + t.Errorf("a negative index must report ok=false") + } +} + // All-or-nothing: any edit that cannot land skips the whole call, byte-for-byte. func TestPatchKustomization_RefusalsLeaveContentUntouched(t *testing.T) { cases := []struct { diff --git a/internal/git/plan_flush.go b/internal/git/plan_flush.go index 9bad7280..f38c9b96 100644 --- a/internal/git/plan_flush.go +++ b/internal/git/plan_flush.go @@ -87,6 +87,12 @@ func newWriteBatch( // .gittargetignore, so the structure-only acceptance gate (run by writeBatch.refusal) and // the write-plan precondition (run by writeBatch.flush) read both from the store. store := manifestanalyzer.BuildStoreFromScan(ctx, scan, mapper, manifestanalyzer.WriterAllowlist()) + // Surface the store's build-time warnings (ambiguous namespace or override + // context, scope mismatches) once per batch: these drive silent fallbacks — + // e.g. an ambiguous override chain falls back to write-through — and without + // this line the live path would leave no trace of why. The analyzer CLI and + // scan mode show the same diagnostics offline. + logStoreDiagnostics(ctx, store.Diagnostics) contentByPath := make(map[string][]byte, len(scan.YAMLFiles)) for _, f := range scan.YAMLFiles { contentByPath[f.Path] = f.Content @@ -756,6 +762,21 @@ func sortedBaseKeys(byBase map[string][]Event) []string { return keys } +// logStoreDiagnostics surfaces store build-time diagnostics of warning level or +// above at low verbosity — the trace for decisions the writer makes silently +// (ambiguity fallbacks, scope mismatches). Info-level index chatter is dropped. +func logStoreDiagnostics(ctx context.Context, diags []manifestedit.Diagnostic) { + logger := log.FromContext(ctx) + for _, d := range diags { + if d.Level == manifestedit.DiagInfo { + continue + } + logger.V(1).Info("manifest store diagnostic", + "level", d.Level, "reason", d.Reason, "file", d.Path, + "documentIndex", d.DocumentIndex, "message", d.Message) + } +} + // logManifestDiagnostics surfaces manifestedit diagnostics at low verbosity so a // skipped edit is observable without noise on the happy path. func logManifestDiagnostics(ctx context.Context, diags []manifestedit.Diagnostic) { diff --git a/internal/manifestanalyzer/overrides.go b/internal/manifestanalyzer/overrides.go index be10c7ad..f7acfd02 100644 --- a/internal/manifestanalyzer/overrides.go +++ b/internal/manifestanalyzer/overrides.go @@ -220,6 +220,13 @@ func (a *overrideAssignment) ambiguous() bool { // kustomization as a root and refuses parent/child conflicts), a referenced base // is not a root here: its transformers compose with its parent's, innermost // first, exactly as kustomize applies them. +// +// Cycle protection is on the CURRENT PATH, not per walk: a diamond (one root +// reaching a shared base through two overlays) must record both paths so their +// differing chains trip the ambiguity refusal, while a true reference cycle +// still terminates. Real kustomize rejects the diamond outright (duplicate +// resources), so ambiguity — never silent first-path attribution — is the +// honest outcome. func kustomizeOverrideAssignments( kusts map[string]*kustomizationDoc, resourceFiles map[string]struct{}, @@ -230,17 +237,17 @@ func kustomizeOverrideAssignments( if root == nil || root.unsupported { continue } - visited := map[string]struct{}{} + onPath := map[string]struct{}{} var stack []*kustomizationDoc var walk func(dir string, cur *kustomizationDoc) walk = func(dir string, cur *kustomizationDoc) { if cur == nil || cur.unsupported { return } - if _, seen := visited[dir]; seen { + if _, cycling := onPath[dir]; cycling { return } - visited[dir] = struct{}{} + onPath[dir] = struct{}{} stack = append(stack, cur) for _, entry := range cur.resources { target := cleanJoin(dir, entry) @@ -254,6 +261,7 @@ func kustomizeOverrideAssignments( } } stack = stack[:len(stack)-1] + delete(onPath, dir) } walk(rootDir, root) } diff --git a/internal/manifestanalyzer/overrides_projection_test.go b/internal/manifestanalyzer/overrides_projection_test.go index e1812a5b..095df743 100644 --- a/internal/manifestanalyzer/overrides_projection_test.go +++ b/internal/manifestanalyzer/overrides_projection_test.go @@ -147,6 +147,28 @@ func TestSplitDesired_NameChangeRoutedToNewName(t *testing.T) { } } +// TestSplitDesired_DigestRoutedToEntry: a live digest change whose supplier is a +// digest entry updates the entry; tag and name keep their source form. +func TestSplitDesired_DigestRoutedToEntry(t *testing.T) { + git := deploymentObj("app:1.0.0", nil) + desired := desiredOf(deploymentObj("app:1.0.0@sha256:ccc", nil)) + ov := &KustomizeOverrides{Images: []ImageOverride{ + imgEntry("app", map[string]string{"digest": "sha256:bbb"}), + }} + + out, edits := SplitDesiredForOverrides(git, desired, ov) + if got := desiredImage(t, out); got != "app:1.0.0" { + t.Errorf("file image = %q, want untouched source (no digest)", got) + } + if len(edits) != 1 || + edits[0].Edit != (manifestedit.KustomizationEdit{ + Section: manifestedit.KustomizationSectionImages, EntryIndex: 0, + EntryName: "app", Field: "digest", Value: "sha256:ccc", + }) { + t.Fatalf("want one digest edit to sha256:ccc, got %+v", edits) + } +} + // TestSplitDesired_RemovalUnroutable: live drops the digest an entry supplies; // nothing can express that on the entry, so the whole object writes through. func TestSplitDesired_RemovalUnroutable(t *testing.T) { diff --git a/internal/manifestanalyzer/overrides_test.go b/internal/manifestanalyzer/overrides_test.go index 29f244f1..e06d93be 100644 --- a/internal/manifestanalyzer/overrides_test.go +++ b/internal/manifestanalyzer/overrides_test.go @@ -18,20 +18,20 @@ import ( // the ambiguity diagnostic. See // docs/design/gitops-api/f1-images-replicas-edit-through.md. -func corpusDeployment(t *testing.T, store *ManifestStore, ns string) *DocumentModel { +func corpusDeployment(t *testing.T, store *ManifestStore) *DocumentModel { t.Helper() dm := store.ByManifestIdentity[manifestedit.Identity{ - APIVersion: "apps/v1", Kind: "Deployment", Namespace: ns, Name: "web", + APIVersion: "apps/v1", Kind: "Deployment", Namespace: "app", Name: "web", }] if dm == nil { - t.Fatalf("Deployment web should be indexed under namespace %q", ns) + t.Fatalf("Deployment web should be indexed under namespace app") } return dm } func TestKustomizeOverridesCorpus_ImagesOverlay(t *testing.T) { store := corpusStore(t, "supported/images-overlay") - dm := corpusDeployment(t, store, "app") + dm := corpusDeployment(t, store) if dm.Overrides == nil || len(dm.Overrides.Images) != 1 || len(dm.Overrides.Replicas) != 0 { t.Fatalf("want exactly one image override, got %+v", dm.Overrides) } @@ -50,7 +50,7 @@ func TestKustomizeOverridesCorpus_ImagesOverlay(t *testing.T) { func TestKustomizeOverridesCorpus_ReplicasOverlayChain(t *testing.T) { store := corpusStore(t, "supported/replicas-overlay") - dm := corpusDeployment(t, store, "app") + dm := corpusDeployment(t, store) if dm.Overrides == nil || len(dm.Overrides.Images) != 1 || len(dm.Overrides.Replicas) != 1 { t.Fatalf("want one image + one replica override from the chain, got %+v", dm.Overrides) } @@ -68,9 +68,23 @@ func TestKustomizeOverridesCorpus_ReplicasOverlayChain(t *testing.T) { } } +// A diamond under ONE render root (root → a → base, root → b → base) must +// record both paths: their chains differ, so no overrides attach and the +// ambiguity diagnostic fires. Pins the on-path (not per-walk) cycle protection. +func TestKustomizeOverridesCorpus_DiamondUnderOneRoot(t *testing.T) { + store := corpusStore(t, "unsupported/diamond-images") + dm := corpusDeployment(t, store) + if dm.Overrides != nil { + t.Errorf("a diamond's conflicting chains must attach no overrides, got %+v", dm.Overrides) + } + if !hasOverrideAmbiguityDiag(store) { + t.Errorf("want an %s diagnostic for the diamond", reasonAmbiguousOverrides) + } +} + func TestKustomizeOverridesCorpus_AmbiguousImages(t *testing.T) { store := corpusStore(t, "unsupported/ambiguous-images") - dm := corpusDeployment(t, store, "app") + dm := corpusDeployment(t, store) if dm.Overrides != nil { t.Errorf("distinct chains from two roots must attach no overrides, got %+v", dm.Overrides) } diff --git a/internal/manifestanalyzer/testdata/contextual-namespace/unsupported/diamond-images/a/kustomization.yaml b/internal/manifestanalyzer/testdata/contextual-namespace/unsupported/diamond-images/a/kustomization.yaml new file mode 100644 index 00000000..1a299320 --- /dev/null +++ b/internal/manifestanalyzer/testdata/contextual-namespace/unsupported/diamond-images/a/kustomization.yaml @@ -0,0 +1,5 @@ +resources: + - ../base +images: + - name: ghcr.io/example/podinfo + newTag: "1.0.0" diff --git a/internal/manifestanalyzer/testdata/contextual-namespace/unsupported/diamond-images/b/kustomization.yaml b/internal/manifestanalyzer/testdata/contextual-namespace/unsupported/diamond-images/b/kustomization.yaml new file mode 100644 index 00000000..4cd9270d --- /dev/null +++ b/internal/manifestanalyzer/testdata/contextual-namespace/unsupported/diamond-images/b/kustomization.yaml @@ -0,0 +1,5 @@ +resources: + - ../base +images: + - name: ghcr.io/example/podinfo + newTag: "2.0.0" diff --git a/internal/manifestanalyzer/testdata/contextual-namespace/unsupported/diamond-images/base/deployment.yaml b/internal/manifestanalyzer/testdata/contextual-namespace/unsupported/diamond-images/base/deployment.yaml new file mode 100644 index 00000000..4c8e0c3d --- /dev/null +++ b/internal/manifestanalyzer/testdata/contextual-namespace/unsupported/diamond-images/base/deployment.yaml @@ -0,0 +1,17 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: web + namespace: app +spec: + selector: + matchLabels: + app: web + template: + metadata: + labels: + app: web + spec: + containers: + - name: podinfo + image: ghcr.io/example/podinfo:6.4.0 diff --git a/internal/manifestanalyzer/testdata/contextual-namespace/unsupported/diamond-images/base/kustomization.yaml b/internal/manifestanalyzer/testdata/contextual-namespace/unsupported/diamond-images/base/kustomization.yaml new file mode 100644 index 00000000..9519a26d --- /dev/null +++ b/internal/manifestanalyzer/testdata/contextual-namespace/unsupported/diamond-images/base/kustomization.yaml @@ -0,0 +1,2 @@ +resources: + - deployment.yaml diff --git a/internal/manifestanalyzer/testdata/contextual-namespace/unsupported/diamond-images/kustomization.yaml b/internal/manifestanalyzer/testdata/contextual-namespace/unsupported/diamond-images/kustomization.yaml new file mode 100644 index 00000000..ee655d15 --- /dev/null +++ b/internal/manifestanalyzer/testdata/contextual-namespace/unsupported/diamond-images/kustomization.yaml @@ -0,0 +1,7 @@ +# One render root reaches the shared base through two overlays whose override +# chains differ (a diamond). Real kustomize rejects this build (duplicate +# resources); the analyzer must record BOTH paths and refuse to route +# (ambiguous-kustomize-overrides), never silently attribute the first path. +resources: + - a + - b