feat: render-root scoping + tolerate patches and patched overlays reading ../../base - #239
Merged
Conversation
Render-root scoping (docs/design/support-boundary/render-root-scoping.md §4): a GitTarget whose spec.path is a kustomize overlay may now read a base OUTSIDE its subtree via ../../base, while writes stay inside spec.path. The writer re-roots its scan at renderBase — the lowest common ancestor of spec.path and every base it reaches — so the store, attribution, and the render oracle run in one coordinate system with no ..-escaping paths. The write jail (writeSubdir) keeps a planned write inside spec.path. When a subtree reads no out-of-scope base, renderBase == spec.path and writeSubdir == "", so every existing layout is byte-identical to before. Effects: - an overlay pointed at ../../base now renders and is accepted (was refused for a kustomize build failure); - a base-derived object materialises, so an images:/replicas: bump routes to the overlay's own entry and the read-only base keeps its bytes; - a base-owned field edit an overlay cannot express is refused and reported, never silently written through; - fanInPrecondition is generalised to any file reachable from more than one render root (ReachedByMultipleRenderRoots), not only the override-ambiguous case. Adds the minimal-overlay corpus fixture (2-rendered/kustomize-overlay-minimal), the first to surface the kustomize-overlay / overlay-fan-out-unsupported verdict; support-today.md regenerated (existing rows unchanged). Unit tests exercise the write path with real kustomize builds. Deferred to a follow-up: the discovery-side flip (scan_repo still reports the overlay unsupported) and a dedicated cluster e2e. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (33)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
A `patches:` block used to refuse the whole GitTarget. Not the edit — the TARGET.
A folder whose patch pins a replica count also lost images:/replicas: edit-through,
which the patch has nothing to do with. It is the single biggest refusal cause in
the layout corpus.
A strategic-merge patch named by `path:` is now READ-ONLY BUILD CONTEXT:
- the folder is accepted, and what it renders is mirrored;
- the patch FILE is retained, never managed. This is the part nothing else in the
store would have got right: a sparse patch IS a KRM document, so materialised it
would be indexed as a manifest, matched to a live object, mirrored over with a
whole Deployment, or swept as an orphan nothing in the cluster answers to. That
a patch is not a resource is not our claim — it is the RENDER's: a patch file
never appears as a rendered object's origin;
- images:/replicas: edit-through works in a patched folder, exactly as anywhere;
- an edit to a field the PATCH owns is refused per OBJECT, not per folder.
Tolerating a patch is not authoring one. Nothing is ever written into a patch file.
Exactly one shape is tolerated; the rest refuse BY NAME, because "unsupported" is not
something a user can act on and "your patch is inline, and we can only read one from a
file" is: patches-inline (which is also where an inline JSON6902 op list arrives, since
it decodes into the same field), patches-json6902 (a path to an op list — a YAML
sequence, not a sparse KRM document), patches-outside-tree.
MEASURED, NOT ASSUMED: FixKustomization folds `bases` into resources and `imageTags`
into images, but it does NOT fold `patchesStrategicMerge` or `patchesJson6902` into
`Patches`. They keep refusing under their own names, and a test pins that so a kustomize
bump cannot silently widen what we accept.
THE CORPUS SAYS SOMETHING THE PLAN DID NOT EXPECT, and it is the deliverable here:
tolerating patches accepts ZERO new candidates. Every patched overlay in the corpus also
reads a base from OUTSIDE its own folder, so `patches` was masking the real refusal.
flux-monorepo/apps/{staging,production} now report overlay-fan-out-unsupported — the
verdict render-root-scoping.md §5 records as never having been observed, because
refused-structural always fired first and hid it. Render-root scoping, not patches, is
the single blocker on the corpus's most tractable layout.
The mechanism itself is proven by fixtures at both levels: a self-contained patched root
is accepted, its image bump routes to the entry, its in-sync state is a no-op, and its
patch-owned scale is refused.
The prompt's stage 1 shipped, and three of its premises did not survive contact with the measurement: the gate was the projection rather than the deny-list, FixKustomization does not fold the deprecated spellings, and tolerating patches accepted zero new corpus candidates because every patched overlay in the corpus is really blocked on render-root scoping.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
The tolerate-patches work changed parseKustomization's signature (adding the file tree it resolves patch paths against). Adapt render-scope's KustomizationResourceEntries caller — it reads only the resources graph, so a nil tree is correct. Regenerate the layout baseline over both changes: with patches tolerated, flux-monorepo and kustomize-overlays drop `patches` from their refusals and now report the render-root-scoping barrier (overlay-fan-out-unsupported) that patches was masking. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
sunib
changed the title
Codecov flagged the render-scope error/edge paths as the PR's coverage gap. Add tests for the branches the happy-path suite missed: a transitively-read base (overlay -> ../../base -> ../shared) with a .yml base kustomization, foreign content in the overlay re-keyed to render coordinates, a base's non-manifest file skipped, a remote base skipped rather than scanned, and the common-ancestor / relative-under / minimal-dirs helpers at their boundaries. Raises render_scope.go coverage (rekeyScan 71%->93%; several helpers to 100%); the residual is OS-error paths that need fault injection. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t, dual-role patches Five review findings on render-root scoping + tolerate-patches: - F1/F3: resolve the EXACT reachable file set of the resources/patches graph instead of walking whole base directories. An external resource FILE (resources: [../../shared/x.yaml]) is now pulled into the render scope (was refused); unrelated content a base does not reference is no longer imported (it could wrongly refuse a buildable folder); and an ancestor base no longer re-scans overlay-local files into a duplicate refusal. Adds KustomizationBuildRefs (resources + patch path files). - F2: placement now rebases a declared/canonical path under the write jail (WriteScope) instead of resolving against renderBase and being skipped, so an overlay GitTarget's new objects land inside spec.path as documented. - F4: a file listed as both a resource and a patch is materialised and mirrored, not silently retained as a patch-only build input. - F5: configuration.md / architecture.md / support-contract.md updated — path-based patches are tolerated and overlays reading ../../base are shipped; the scan-repo discovery flip is called out as the remaining follow-up. Tests: external resource file, transitive base, unrelated-content exclusion, write-jail placement (canonical + declared), dual-role path mirrored. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ase kustomizations - P1 (blocker): a new object in an overlay could be committed to a file no kustomization includes — never rendered, and the oracle skipped (silent divergence), introduced when F2 replaced the earlier safe skip with a rebased write. Placement now (a) restricts sibling inference to WRITABLE siblings so it never infers beside a read-only base doc, (b) restricts the single-kustomization- root fallback to WRITABLE kustomizations so an overlay resolves to its own root, and (c) governs a new overlay object by the write scope's own render root when its subdirectory has none. The object lands beside the overlay kustomization, gains a resources: entry, renders, and is oracle-verified. All gated on WriteScope != "", so self-contained placement is unchanged. - P2 (dual kustomization files): a base holding both kustomization.yaml and .yml is now imported whole, so the render reaches kustomize's own "multiple kustomization files" refusal instead of masking it by reading only the first. - P2 (symlink): readKustomization goes through the guarded reader (Lstat + regular-file), so external-base discovery never follows a symlinked kustomization outside the worktree. Tests: new object in an overlay renders (end-to-end), dual-file base imported both, symlinked base kustomization not followed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
sunib
changed the title
This was referenced Jul 15, 2026
sunib
added a commit
that referenced
this pull request
Jul 15, 2026
…rrides and $patch:delete (#244) * test(overlay): prove new-object placement into an external-base overlay; fix stale docs render-root-scoping.md §4 labelled "new object -> overlay-local file plus resources: entry" as Planned, but #239's render-root scoping already wired it: WriteScope threads the overlay's write jail into placement, the base kustomization is skipped as out-of-jail (writableCohort / resolveKustomizeRoot), appendKustomizationResource adds the resources: entry in the same commit, and the render oracle verifies the result. It simply had no test proving it and the docs still read "Planned". Add TestPlacement_ExternalBaseOverlay_NewObject: a GitTarget rooted at an overlay that reads ../../base gains a new ConfigMap. It asserts the file lands inside the overlay, the overlay's OWN kustomization gains the resources: entry (never the base's), the read-only base is untouched byte-for-byte, and the render oracle accepts the flush. Correct the stale docs (render-root-scoping §1/§4/§5/top, README, support-contract, kustomize-support-boundary) to mark new-object creation shipped. What remains for an overlay: adding a MISSING images:/replicas: declaration (no source value to attribute yet) and base-owned-field patch authoring. No production code changed — this proves and documents existing behaviour. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(overlay): author a missing images:/replicas: entry when a base value is changed per-environment An external-base overlay could edit an EXISTING images:/replicas: entry, but when the base itself supplied an image or replica count, changing it in one environment tried to write the read-only base and was refused ("planned write path base/deployment.yaml escapes the GitTarget write scope"). So "edit the image in dev" did not work unless the overlay already declared an entry. Author the entry instead. When the projection finds a diverging image component or replica count whose supplier is the SOURCE document and that source is out of the write jail (a base an overlay reads), it emits a Create OverrideEdit into the overlay's own kustomization rather than writing the base. The writer applies it with a new manifestedit.AppendKustomizationOverride primitive (creating the images:/replicas: section if the overlay has none), and the base source form stays unchanged so nothing is written to the base. Every authored entry is put to the existing re-render oracle before it can commit, so a proposal that over-reaches (an images: name shared by another object) is refused there, not written. Idempotent on resync: once the entry exists the store sees it and the change routes to it, no duplicate. Tests: manifestedit primitive unit tests; overrides projection call-site updates; and writer integration TestOverlayAuthors_{ImageEntry,ReplicaEntry,Idempotent}. Docs updated (render-root-scoping, support-contract, README, kustomize-support-boundary). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(overlay): author $patch: delete for an object inherited from a base Deleting an object an external-base overlay inherits from its base tried to delete the base document (out of the write jail, shared by other environments) and was refused. Author a `$patch: delete` in the overlay instead: when the delete target is out of the write jail and the overlay has a supported render root, applyDelete writes a small `$patch: delete` document inside spec.path, names it in the overlay's own patches: (new manifestedit.AppendKustomizationPatch primitive, creating the patches: section if absent), and keeps the existing Removed intent so the re-render oracle proves the object leaves the render. The base is never touched. The patch pins the object by apiVersion/kind/namespace/name; a patch that does not match makes kustomize fail to build and the oracle refuses the flush (proven: the namespace-less first cut was correctly refused, the namespaced one commits). It needs no field attribution, so it does not wait on strategic-merge field-patch authoring — which stays refused. This is a deliberate, narrow widening of the patch-authoring boundary for the delete slice only; noted in patch-authoring.md and the render-root-scoping/support-contract records. Tests: AppendKustomizationPatch unit tests; integration TestOverlayAuthors_DeletePatch_ForInheritedObject. Coverage baseline bumped 76.4% -> 76.5%. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
sunib
added a commit
that referenced
this pull request
Jul 21, 2026
Sibling inference could file one namespace's objects under another namespace's
folder, and then collapse an entire resource type into a single file.
Step 2 of inference ("same type, any namespace") is guarded by the P4 rule: a
cohort may only be reused for a namespace the target has never written if it has
proven itself namespace-agnostic. The bundle branch enforced that properly, via
`spansMultipleNamespaces`. The singleton branch only asked `allSameDir`, which is
"trivially true for zero or one element" — so ONE directory holding ONE
namespace passed the guard, even though it is exactly as consistent with a
per-namespace-segmented layout whose second namespace had simply never been
written. Absence of contrary evidence was being read as proof.
The consequence was worse than one misplaced file, because it cascades. Objects
that exist under the same name in every namespace (kube-root-ca.crt) make the
inferred path collide EXACTLY with the first namespace's file, so the second
namespace's object is appended as another document. That file now genuinely
spans two namespaces, so every later object of the type legitimately prefers the
bundle — and the whole type collapses into one file holding several namespaces'
objects. Observed end to end: four ConfigMaps from two namespaces in a single
file, which was the only ConfigMap file in the tree.
Both branches now require the same positive proof. Declining costs nothing: the
canonical path builds the correct namespace segment directly, which is the
layout the target already uses for the namespace it has written.
This is pre-existing and not introduced by the source-namespace work — the write
path is untouched by it, and placement.go last changed in #239. It is reachable
today wherever one GitTarget receives one type from two namespaces. It matters
now because `sourceNamespace: "*"` reaches that shape from a single rule.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
sunib
added a commit
that referenced
this pull request
Jul 21, 2026
…ster-only ClusterWatchRule (#259) * feat(watch): add WatchRule.spec.sourceNamespace and its authorization gate Adds `WatchRule.spec.sourceNamespace` so a rule can watch a namespace other than its own on the GitTarget's source cluster — the shared-config-plane multi-tenancy case where a tenant's config namespace and source namespace cannot share a name. PR 4 of docs/design/watchrule-source-namespace/. API (v1alpha3): - `WatchRule.spec.sourceNamespace` (optional; omitted = the rule's own namespace), plus a priority-1 `SourceAuthorized` printer column. - `GitTarget.spec.allowedSourceNamespaces` — a deny-by-default ceiling on which source namespaces may be mirrored into a target, by any rule kind. Declared is exhaustive with no self-namespace exception; empty != omitted. - `ClusterProvider.spec.allowWatchRuleSourceNamespaceOverride` — false-by-default delegation flag; without it a WatchRule may watch only its own namespace. - Generalize `AllowedNamespaces` into a shared `NamespaceMatcher` (JSON field name unchanged); `ClusterProvider.AllowsNamespace` and `GitTarget.AllowsSourceNamespace` become thin wrappers so the control-cluster and source-cluster policies cannot drift. Gate and data plane: - The three-part gate (provider admits the target, provider delegates, target policy admits the namespace) lives in internal/authz with a three-valued result (admitted / denied / cannot-say-yet / permanently-unevaluatable), so a transient source-cluster outage never becomes a terminal Stalled=True. - Route every WatchRule compilation through one gated path (`watch.CompileWatchRule`) used by both the reconciler and the startup bootstrap, so the gate cannot be bypassed on restart. - Source-scope service (manager-owned per-source-cluster Namespace snapshot, refreshed on the existing reconcile cadence) evaluates selector policies and enqueues affected rules on a label change; exact-name policies need no source-cluster access. - Carry the effective source namespace through the compiled rule, the watch selection, the fingerprint, and the stream-readiness lookup, so an override watches — and reports Ready on — the right namespace. - `SourceNamespaceAuthorized` condition (kstatus-compatible; an additional prerequisite of Ready), a ClusterProvider→WatchRules mapper, and a source-cluster Namespace channel. Docs: configuration.md, status-conditions-guide.md, and the design pages. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs: starting to consider alternative approach * docs: include the deletion modes * docs: redirect PR 4 to scope-by-kind and align field names Replace the rejected alternatives and the superseded PR 6 sketch with a single PR 4 design: ClusterWatchRule becomes cluster-scope-only and WatchRule gains a per-item rules[].sourceNamespace, with the selector-backed "*" wildcard bounded by GitTarget.spec.allowedSourceNamespaces. Rename the ClusterProvider delegation boolean to allowSourceNamespaceOverride. Once ClusterWatchRule has no source-namespace choice, the WatchRule prefix disambiguates nothing, and the field is unreleased so the rename needs no shim. Record the prior art: Flux's AccessFrom ACL independently lands on namespace labels, ORed selectors, deny-when-absent, empty-selector-matches-everything, and a typed AccessDeniedError distinct from "cannot evaluate". It never enumerates a namespace set from a selector, and upstream has been shrinking the field rather than growing it, which is why the wildcard's invalidation path is the half of this design with no precedent to lean on. Keep the shipped-but-unreleased top-level implementation as a baseline record so the reusable authorization, status, and reactivity work is not rebuilt. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(api)!: move sourceNamespace onto WatchRule rules[], narrow ClusterWatchRule scope Scope is now carried by the rule KIND. `WatchRule` selects namespaced resources and each `spec.rules[]` item names the source namespace it watches; `ClusterWatchRule` selects cluster-scoped resources and has no scope or namespace choice. PR 4 of docs/design/watchrule-source-namespace/. - `ResourceRule.sourceNamespace`: omitted (the rule's own namespace), an exact DNS-1123 name, or `"*"` for every namespace the GitTarget admits. Validated structurally so a malformed namespace is rejected at admission rather than resolving to nothing at compile time. - `EffectiveSourceNamespace()` / `OverridesSourceNamespace()` move down onto the item; `"*"` always counts as an override, so it needs the delegation flag even against a policy listing only the rule's own namespace. - `NamespaceMatcher.SelectorAdmits` exposes the selector half alone, for the enumeration a wildcard needs (a nil selector admits nothing; an empty one admits everything). - `ClusterProvider.spec.allowWatchRuleSourceNamespaceOverride` renamed to `allowSourceNamespaceOverride`. Unreleased, so a plain rename with no shim. Both superseded fields are RETAINED for one release as loud rejections, not deleted. Deleting is the silent option: CRD pruning happens on write, so a re-applied legacy manifest would be accepted with the value dropped and the rule would quietly change what it mirrors. - `WatchRule.spec.sourceNamespace` — rejected by an XValidation rule naming its replacement; `DeclaresRemovedSourceNamespace()` keeps a stored value readable. - `ClusterResourceRule.scope` — optional, defaults to `Cluster`, enum narrowed to `Cluster`; `DeclaresNamespacedScope()` keys the compile-time refusal on the STORED value, never on what the selector happens to resolve. BREAKING CHANGE: `ClusterWatchRule` is cluster-scope-only and `WatchRule.spec.sourceNamespace` moved to `spec.rules[].sourceNamespace`. A namespaced `ClusterWatchRule` becomes a `WatchRule` in the tenant namespace; its namespaced items become `sourceNamespace: "*"` (or explicit names). A target that declares no `allowedSourceNamespaces` admits NOTHING to a `"*"` item, so converting without also declaring that policy narrows production data. See docs/UPGRADING.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(authz): resolve a WatchRule's whole per-item source scope `WatchRuleSourceNamespaceAdmitted` decided one namespace for a whole rule. Scope now lives on `spec.rules[]`, so the gate answers the whole rule at once: `ResolveWatchRuleSourceScope` returns a `ResolvedSourceScope` carrying one `SourceNamespaceDecision` per item (its index, what it requested, and the namespace set it resolved to). - The three-part ordering — GitTarget policy, then ClusterProvider delegation, then the rule's own namespace — is unchanged, factored into an `itemGate` that memoises the ClusterProvider verdict. The verdict is identical for every item, so it is fetched once per rule rather than once per item. - A `"*"` item enumerates: the policy's literal `names` are answered locally, the selector half is enumerated against the SOURCE cluster's namespaces, and the two are unioned. A selector that cannot be evaluated makes the whole item Unknown/Unavailable — never a partial set, because a partial set silently narrows what a rule mirrors. - `NoAdmittedSourceNamespaces` is a distinct True reason so a wildcard that resolves to nothing reports that it watches nothing instead of looking healthy. - `SourceNamespaceFieldRemoved` refuses a stored `spec.sourceNamespace` terminally. - `aggregateSourceScope` collapses the items with a stated precedence: denied beats unavailable beats unknown beats allowed. One denied item refuses the whole rule, and the message names which item. `Fingerprint()` renders the resolved sets so callers can key cached work on them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(watch): carry the resolved source scope through compile and planning A compiled rule used to hold one source namespace for the whole object. It now holds one resolved namespace SET per item, and every stage from compile to the watched-type table is keyed on that set. - `CompiledRule.SourceNamespace` (string) is replaced by `CompiledResourceRule.SourceNamespaces []string`. `AddOrUpdateWatchRule` takes an index-aligned `sourceNamespaces [][]string` and replaces the whole rule atomically, so a rule is never half-recompiled. `GetWatchRule` is added for callers that need the compiled form back. - `GetMatchingRules` filters the event's namespace per ITEM instead of against the rule object's own namespace, so one rule can legitimately match events from several namespaces. - Wildcard expansion happens at the SELECTION site, not as a read-side filter: `collectWatchRuleSelections` emits one selection per (record x resolved namespace). A read-site filter would have to re-resolve the policy on every event. - `watchRuleFingerprint` hashes each item's RESOLVED namespace set. This is the hazard the design doc calls out: a `"*"` item's inputs are the GitTarget policy and the source cluster's namespaces, neither of which is rule state, so byte-identical rule objects must still re-project the table when the policy moves. Hashing the resolved set is what makes that happen. - Retention is keyed by `SourceScopeSpecHash` (the rule's namespace plus every item's REQUESTED value). An edit or a reorder discards the grant, so a rule cannot inherit a scope it no longer asks for, while an unevaluatable policy keeps the last resolved set instead of sweeping the tree. - `Manager.EnumerateSourceNamespaces` implements the resolver's enumeration half against the source cluster; `unusableSnapshot` is shared by both resolver entry points. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(watch)!: ClusterWatchRule becomes cluster-scope-only A ClusterWatchRule now selects cluster-scoped types and nothing else. Three enforcement points, deliberately redundant because each one covers a path the others cannot: - Admission — the narrowed `scope` enum rejects `Namespaced` on write. - Compile — `CompileClusterWatchRule` is the single shared path for BOTH the reconciler and bootstrap. It refuses in a fixed order: first the ClusterProvider GitTarget-namespace gate, then `DeclaresNamespacedScope()`. The refusal keys on the STORED value, so a rule admitted by an older CRD is still refused rather than silently re-interpreted. - Resolution — `collectClusterWatchRuleSelections` and `ResolveClusterWatchRuleResources` always match `ResourceScopeCluster`, so a selector like `resources: ["*"]` cannot pull a namespaced type back in. Bootstrap and the reconciler sharing one path is the point: on restart, rules are seeded before any status exists, and a bootstrap that did not refuse would open a namespaced watch the reconciler would only close later. `ClusterWatchRuleReconciler.gateGitTargetAdmission`/`refuseUnauthorizedGitTarget` collapse into `gateClusterWatchRule`/`refuseClusterWatchRule`, and the reconciler no longer calls `AddOrUpdateClusterWatchRule` itself. Test fixtures move to genuinely cluster-scoped types (namespaces, nodes, CRDs, storageclasses) because a ClusterWatchRule now resolves nothing else. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(status): aggregate source-namespace authorization per rule item `SourceNamespaceAuthorized` stays one condition per object, now summarising every `spec.rules[]` item. `gateSourceNamespace` publishes the aggregate produced by `ResolveWatchRuleSourceScope`, so the precedence a reader sees is stated once and applied everywhere: a denied item refuses the whole rule and the message names which item lost, an unevaluatable policy reports Unknown rather than quietly watching less, and a wildcard that admits nothing says so. `StreamSummaryForWatchRule` now reads the COMPILED rule instead of the spec. A wildcard's expected stream count is not derivable from the spec — it depends on how many namespaces the item resolved to — so reading the spec left a wildcard rule permanently short of its expected streams and therefore permanently not-Ready. An uncompiled rule correspondingly expects zero streams. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs: scope by kind, plus the PR 4 migration guide Documents the model and covers the migration end to end in tests. Docs: `configuration.md`, `architecture.md`, `security-model.md` (a new "Which namespaces a rule may read from"), `rbac.md` (a new section on `namespaces` RBAC against a REMOTE source cluster), `status-conditions-guide.md` (the aggregation order and `NoAdmittedSourceNamespaces`), and a new `UPGRADING.md` entry: the two removed capabilities, the conversion, a `jq` one-liner to find affected objects, and the warning that converting without declaring `allowedSourceNamespaces` narrows what a rule mirrors. Tests: `superseded_fields_admission_test.go` asserts against the GENERATED CRDs that `scope: Namespaced` and `spec.sourceNamespace` are rejected, that `rules[].sourceNamespace` including `"*"` is accepted, that a malformed namespace is rejected, and that an omitted `scope` defaults. The e2e source- namespace spec moves to `rules[].sourceNamespace` and gains a wildcard case asserting against real commits that an admitted namespace arrives and an unadmitted one never does. The restart fixture becomes a WatchRule and keeps covering the `apiVersions: ["*"]` startup-snapshot regression. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(e2e): make the wildcard spec prove the wildcard The spec passed vacuously in one half and could never pass in the other. - It probed for `<secret>.yaml`, but Secrets are committed SOPS-encrypted as `<name>.sops.yaml` and `findFileByBasename` matches basenames exactly. The positive assertion could therefore never succeed — and the `Consistently` proving an UNADMITTED namespace never arrives was searching for a filename that could not exist either way, so it could never fail. That half is the load-bearing one, which made the vacuous pass the more dangerous defect. - The positive assertion created its object in the namespace the PRECEDING spec's rule already watches by exact name, so it would have passed even if `"*"` expanded to nothing at all. The granted GitTarget now admits a second namespace that no rule item ever names, so only a wildcard can reach it, and the probe objects are ConfigMaps so the assertion does not also ride on the encryption filename convention (the dedicated Secret-encryption spec already covers that). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(placement): prove namespace-agnosticism before reusing a cohort Sibling inference could file one namespace's objects under another namespace's folder, and then collapse an entire resource type into a single file. Step 2 of inference ("same type, any namespace") is guarded by the P4 rule: a cohort may only be reused for a namespace the target has never written if it has proven itself namespace-agnostic. The bundle branch enforced that properly, via `spansMultipleNamespaces`. The singleton branch only asked `allSameDir`, which is "trivially true for zero or one element" — so ONE directory holding ONE namespace passed the guard, even though it is exactly as consistent with a per-namespace-segmented layout whose second namespace had simply never been written. Absence of contrary evidence was being read as proof. The consequence was worse than one misplaced file, because it cascades. Objects that exist under the same name in every namespace (kube-root-ca.crt) make the inferred path collide EXACTLY with the first namespace's file, so the second namespace's object is appended as another document. That file now genuinely spans two namespaces, so every later object of the type legitimately prefers the bundle — and the whole type collapses into one file holding several namespaces' objects. Observed end to end: four ConfigMaps from two namespaces in a single file, which was the only ConfigMap file in the tree. Both branches now require the same positive proof. Declining costs nothing: the canonical path builds the correct namespace segment directly, which is the layout the target already uses for the namespace it has written. This is pre-existing and not introduced by the source-namespace work — the write path is untouched by it, and placement.go last changed in #239. It is reachable today wherever one GitTarget receives one type from two namespaces. It matters now because `sourceNamespace: "*"` reaches that shape from a single rule. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(e2e): one GitTarget with two source namespaces keeps them apart The wildcard spec proved that `"*"` resolves to the admitted set, but every object it created lived in a single namespace, so it could not have caught a placement defect that only appears once a target holds a SECOND one. This adds that case with two ordinary rules and explicit sourceNamespace values — the same shape a wildcard expands to, reached without the wildcard. Keeping it wildcard-free is the point: it separates a defect in wildcard EXPANSION from one in how a target handles more than one source namespace at all, which is what told us the collapse fixed in the previous commit was not caused by this feature. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(watch): key a source-namespace policy on the target's own cluster A GitTarget's allowedSourceNamespaces is a statement about ITS source cluster, but both resolver entry points looked the cluster up through clusterIDForGitTarget, which defaults an undeclared GitTarget to the config plane. That default is right for the read paths it was written for — a status read racing the first Declare — and wrong for authorization: the WatchRule reconciler gates as soon as it has resolved the GitTarget, while Declare is the GitTarget controller's job, and after a restart the two run concurrently. In that window a REMOTE target's selector was evaluated against CONFIG-PLANE Namespace labels, so a namespace could be admitted because a same-named namespace here happened to carry the right label. Key on target.SourceCluster() instead. The two can never disagree: the controller passes exactly that value to DeclareForGitTarget. This also fixes a latent enqueue miss, since enqueueSourceNamespaceChange matches the armed cluster id against the same map — an undeclared target armed "" and so could never match itself. Seven existing tests had to move their snapshot onto the target's own cluster. They passed only because the resolver read the wrong one, which is the clearest evidence the defect was real; the new test uses DIVERGENT labels, because with both clusters labelled alike it would pass against the bug. Also bound the refresh loop that maintains those snapshots. It walked clusters serially, and the Namespace list runs on a config that deliberately carries no rest.Config.Timeout (its watches must stay open) with only its dial bounded, so a cluster that accepts the connection and then hangs blocked ReconcileForRuleChange forever — and with it the watched-type tables and target watches for every tenant. Each cluster now lists under its own deadline, with the same bounded concurrency refreshRemoteCatalogsConcurrently already applies to the catalog for exactly this reason. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(controller): forget a deleted WatchRule's retained source scope The delete path removed the rule from the store but never called ForgetSourceScopeGrant, whose own docstring says it runs "on a REFUSAL or a deletion". Only the refusal half was wired. The grant is what distinguishes a rule MAINTAINING an already-resolved scope from one ESTABLISHING its first, and the two branches are deliberately opposite: maintaining retains the last known-good set and reports Unknown, while establishing must refuse with a terminal, actionable Stalled. A grant left behind by a deleted rule is inherited by the next rule created under that name and spec — a name a different tenant may now own — so an unevaluatable policy reads as maintaining. The rule then sits Unknown and Reconciling indefinitely instead of saying why it will never run. The consequence is narrower than "it keeps running": the maintaining branch compiles nothing, and the deleted rule was already out of the store, so no stream resurrects. The damage is a rule stuck silently in progress. The test recreates a byte-identical rule, which is the case the spec hash cannot catch — only forgetting the grant can — and was verified to fail against the unfixed controller. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(api)!: reject a namespace pattern in a policy, and trim CRD descriptions NamespaceMatcher.names accepted `*`, and expandWildcard copies policy names verbatim into a rule's resolved scope. The effect is NOT the privilege escalation it looks like: Kubernetes treats `*` as a LITERAL namespace name — a list or watch against namespaces/* matches nothing, measured against a live cluster — and matchesSourceNamespace is exact string equality with no glob. So a policy of names: ["*"] resolved a `sourceNamespace: "*"` item to a namespace that cannot exist. The rule reported itself authorized, planned a stream, and mirrored NOTHING, which is precisely the silent no-op the NoAdmittedSourceNamespaces reason exists to make loud. Entries now validate as DNS-1123 labels, which both NamespaceMatcher users inherit from the shared type, plus a defensive ValidateNames() for policies already in etcd. The gate treats a failure as Unavailable — a policy that cannot be evaluated, not a smaller one — so one bad entry condemns the whole policy rather than silently narrowing to the well-formed remainder. The "every namespace" declaration is, and remains, `selector: {}`. BREAKING CHANGE: an allowedSourceNamespaces or allowedNamespaces entry that is not a valid namespace name is now rejected at admission. `*` was never a pattern there; it silently matched nothing. Use `selector: {}` to admit every namespace. Also trims the CRD descriptions, which had grown to 46 KB of schema text — allowedSourceNamespaces alone was 47 lines of design rationale in `kubectl explain`. Descriptions now carry the contract and point at docs/configuration.md for the full resolution table; rationale moves to comment blocks detached from the doc comment by a blank line, so it stays in the source and out of the schema. Total description text drops to 38 KB and nothing of ours exceeds 12 lines. Verified to change descriptions ONLY: placing such a block between a type's markers and its doc comment silently drops the markers, which cost ClusterWatchRule its scope=Cluster (the CRD flipped to Namespaced) and both rule kinds their printer columns before the diff caught it. AGENTS.md records the convention and that check. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs: correct the PR 4 migration guidance and record the review follow-ups Three corrections, all found by reviewing PR 4 against the code: - The migration warning said a target with no declared policy "admits nothing". ReasonLegacySourceNamespace disagrees: a WatchRule whose every item watches its own namespace keeps working with no policy and no delegation flag. Stated unqualified, UPGRADING.md would tell every existing operator their rules break on upgrade, which is false and the more expensive of the two possible errors. The denial applies to converted wildcard and cross-namespace items. - The historical baseline said PR 4 defers selector-backed wildcards. PR 4 ships them and resolves them from the source-namespace snapshot. - A facts doc still said ClusterWatchRule watches namespaced resources. Adds pr4-review-followups.md, the intake for both review passes. Each item was re-verified against the code before being accepted, and one reported blocker did not survive that check — its premise (that Kubernetes reads `*` as an all-namespaces watch) is false, though its recommendation stands for a different reason. The doc records what landed and which test holds it. Also carries architecture.md revisions from a parallel review pass: per-ClusterProvider source contexts, the control-plane/source-cluster split in the flow diagrams, and the distinction between the object mark-and-sweep and the Namespace-label listing that maintains selector authorization. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(api)!: delete WatchRule.spec.sourceNamespace instead of deprecating it The field was carried as a "loud rejection" — a CEL guard, a SourceNamespaceFieldRemoved reason, and a compile-path refusal — on the same reasoning as ClusterResourceRule.scope: deleting a field is the SILENT option, because CRD pruning happens on write, so a re-applied legacy manifest is accepted with the value dropped and the rule quietly changes what it mirrors. That reasoning is about STORED objects, and it does not apply here. spec.sourceNamespace has never existed on main — not in the Go type, not in the generated CRD — so no cluster's etcd can hold one and no manifest in the wild sets one. The only population it protected was this repo's own pre-release fixtures, which move with the change. Retaining a rejection for a field nobody can have set is ceremony, and it leaves a phantom in `kubectl explain` that readers have to be told to ignore. ClusterResourceRule.scope keeps its rejection: that field DID ship, so a stored `Namespaced` value is a real object in a real cluster. Removes the field, the CEL guard, DeclaresRemovedSourceNamespace(), the SourceNamespaceFieldRemoved reason and its refusal path, and the four tests that covered them. UPGRADING.md now describes one retained shim rather than two, with the never-shipped field stated as simply absent. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Delivers the two connected halves that make a patched kustomize overlay reading
../../basework end-to-end — folding in the tolerate-patches work from #235, which is incomplete on its own (see below). Implements render-root-scoping.md §7 steps 1, 4, and 6 (step 3, the oracle, already shipped).Two capabilities, deliberately combined because neither is useful for a real overlay without the other:
1. Read
../../baseas render context (render-root scoping)A
GitTargetwhosespec.pathis an overlay may now read a base outside its subtree via../../base, while writes stay insidespec.path. The writer re-roots its scan atrenderBase— the lowest common ancestor ofspec.pathand every base it reaches (internal/git/render_scope.go) — so the store, attribution, and the render oracle run in one coordinate system with no..-escaping paths. The write jail (writeSubdir) keeps a planned write insidespec.path. When a subtree reads no out-of-scope base, this is the identity — every existing layout is byte-identical to before.2. Tolerate a patch, without authoring one (folded from #235)
A path-based strategic-merge
patches:is now read-only build context: the folder is accepted, the patch file is retained (never managed/swept),images:/replicas:edit-through works in a patched folder, and an edit to a field the patch owns is refused per object. Inline / JSON6902 / out-of-tree patches refuse by name (patches-inline,patches-json6902,patches-outside-tree).Why combine them
#235's own finding: "Tolerating patches accepts zero new candidates — every patched overlay in the corpus also reads a base from outside its own folder, so
patcheswas masking the real refusal. Render-root scoping, not patches, is the single blocker on the corpus's most tractable layout." Conversely, render-root scoping alone leaves a patched overlay refused on thepatches:key. Together,flux-monorepo/apps/{production,staging}(patch +../../base) is handled by the writer: rendered, accepted, image/replica edit-through, patch-owned edits refused per-object.Effects on the corpus baseline
flux-monorepo/apps/{staging,production}refused-structural: patcheskustomize-overlay/overlay-fan-out-unsupportedkustomize-overlays/apps/frontend/overlays/*…, patches, …patchesdrops out (still refused for generators + name(pre/suf)fix)2-rendered/kustomize-overlay-minimal(new)kustomize-overlay/overlay-fan-out-unsupportedAlso
fanInPreconditiongeneralised to any file reachable from more than one render root (ReachedByMultipleRenderRoots), not only the override-ambiguous case.kustomize-overlayverdict (every other overlay trippedrefused-structuralfirst).Tests
Writer + analyzer unit tests exercise the write path against real kustomize (krusty) builds: overlay-base resolution, image edit-through to the overlay entry with the base untouched, in-sync no-op, base-field refusal, shared-base fan-in refusal, base-escaping-repo-root refusal; plus the patches tests (tolerated
path:merge, refused inline/json6902/out-of-tree, "patch file is never a render origin", deprecated spellings not folded).Validation
task fmt / generate / manifests / vet / lint✓ ·task test✓ (unit coverage → 76.2%, baseline bumped) ·task test-e2erunning locally + on CI.Deferred to a follow-up
scan_repostill classifies these overlays asoverlay-fan-out-unsupported; this PR changes the live writer, not the discovery/onboarding report.Folds in and supersedes #235.
🤖 Generated with Claude Code