feat(api)!: move commit batching onto GitTarget, delete allowedSourceNamespaces, and make sourceNamespace "*" cluster-wide - #330
Merged
Conversation
…ec.commit GitProvider is the connection: a URL, a credential, the branches it will accept. How a folder's writes are batched into commits and how those commits are phrased are properties of the folder, and two GitTargets sharing one GitProvider had no way to disagree about either. GitProvider.spec.push.commitWindow -> GitTarget.spec.commit.window GitProvider.spec.commit.message -> GitTarget.spec.commit.message They are grouped under spec.commit rather than landing as two top-level fields: the move is breaking either way, so the grouping is free in this release and would cost a bump in any later one. commit.committer and commit.signing stay on GitProvider, because both describe the identity that talks to the remote. Both fields are retained in the GitProvider schema and REFUSED rather than deleted. CRD pruning happens on write, so a deleted field would be dropped from a re-applied manifest with no error at all, silently changing a folder's commit cadence. A field-level CEL rule rejects a new apply naming the replacement, and a stored object is refused by the reconciler (Stalled, CommitFieldsRelocated) rather than half-honoured. The commit window is resolved per open window rather than once per worker: a window is bound to exactly one GitTarget by construction, since it finalizes the moment the target changes. Template validation moves with the field, onto the GitTarget's Validated gate, and now covers the window string too. Also pins the apiserver property the whole loud-rejection pattern rests on: a status update onto an object whose STORED spec no longer validates is accepted, so a refused object can still explain itself. Measured on 1.31 with CRDValidationRatcheting explicitly on and off, and on the version this module builds against - the status subresource does not re-validate spec at all, so the pattern does not depend on ratcheting. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…r grants, and make sourceNamespace "*" cluster-wide
Four changes that ship together, because the last one is defined in terms of
the first.
GitTarget.spec.allowedSourceNamespaces removed
ClusterProvider.spec.allowSourceNamespaceOverride -> spec.allowAnySourceNamespace
ClusterProvider.spec.allowedNamespaces -> spec.accessFrom
sourceNamespace: "*" every namespace the GitTarget admits
-> every namespace the credential can read, one cluster-wide watch
allowedSourceNamespaces presented itself as a destination policy and could not
be one: a WatchRule's targetRef is namespace-local, a GitTarget's providerRef is
namespace-local, and spec.path is immutable, so the chain from a Git folder back
to the object that fills it never leaves one namespace. Whoever can create a
WatchRule there could already write into that folder. What it actually bounded
was which source namespaces the folder's own tenant may READ, which for a
credential-scoped provider restates the credential in the one place that cannot
revoke it.
Its selector half was evaluated against Namespace labels in ANOTHER cluster, and
that single choice produced the three-valued verdict, the SourceScopeUnavailable
degradation path, five condition reasons, the establishing/maintaining retention
memory, and the operator's need for source-cluster Namespace get/list/watch. All
of it goes: ~2,900 lines net, and the operator now reads no Namespace objects in
a source cluster at all.
accessFrom STAYS, renamed. It is the one boundary available nowhere else: source
RBAC bounds what a credential may read and cannot express which control-plane
tenant may wield it. Its selector reads control-cluster labels, locally, with no
cross-cluster call, so both halves keep working.
"*" had to be redefined rather than left alone: it was defined in terms of the
deleted field, and RBAC cannot supply the missing definition (it answers "may I
watch X in namespace Y", never "which namespaces may I watch"). It is now one
cluster-wide list and watch, refused outright while allowAnySourceNamespace is
false. The plumbing existed - CellKey documents the empty-namespace case and both
openTargetWatch and openTargetList already branch on it - so this is a deletion
in the compile path plus one router fix, not new machinery. A cluster-wide cell
stays a PEER of a named-namespace cell, never a replacement, because each rule
carries its own operations filter.
Every removed or renamed field is retained in the schema and REFUSED with a
message naming its replacement, verified against the generated CRDs. Pruning
happens on write, so deleting them would drop the value from a re-applied
manifest with no error - and for allowSourceNamespaceOverride: true that would
silently revoke a delegation.
Two breaking semantic changes are stated rather than papered over in
docs/UPGRADING.md: a declared allowedSourceNamespaces could deny a rule's own
namespace, so allowAnySourceNamespace: false is not exactly today's posture; and
"*" widens for anyone who had declared a policy. Source-side label selectors are
LOST with no replacement, and that is the real capability cost.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…es, and scope the wildcard spec Two fixtures still set GitProvider.spec.push.commitWindow, which this release refuses: the playground GitProvider and the two demo-only manifests. The refusal worked as designed and the e2e run caught them, so the values move to GitTarget.spec.commit.window where they now live. The wildcard e2e spec asked for too much. `sourceNamespace: "*"` now mirrors every namespace the credential can read, so watching configmaps AND secrets cluster-wide filed every service-account token in a live k3d cluster into the fixture repository, and the render-fidelity gate had 42 secrets' worth of scopes to settle inside a 90s window. The spec now watches ONE type, and asserts the gate plus the two files that prove the reach - not whole-target Ready, which over a cluster-wide mirror is a throughput property rather than a statement about what "*" means. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important Review skippedToo many files! This PR contains 113 files, which is 13 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (113)
You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
…nswer source-scope-simplification.md and gittarget-api-wave.md were written as proposals and are now descriptions of what shipped, except the additive SelfSubjectAccessReview pass and the trimmed riders. build-order.md's PR 3 row and INDEX.md follow. The wave's step-1 envtest has an answer, and it belongs at the top of the page that asked the question: a status update onto a stored object whose spec no longer validates is ACCEPTED, so the fallback that page describes was not needed. The "*" section stays the definition of record; the reading it called "decided for the wave" is now the shipped one, and the superseded reading is kept because the migration note and several code comments still refer to it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…erage baseline for the deletion The coverage ratchet failed in CI at 76.8% against a 77.4% baseline. The cause is the deletion, not a testing regression, and it is measurable rather than a matter of opinion: the two source-scope files this wave removes outright carried 336/371 statements covered (90.6%) on main, and removing exactly those from main's own coverage profile takes the total from 77.22% to 76.96% by itself. The files that shrank rather than vanished were covered similarly well. Deleting well-covered code lowers a whole-tree average without anything becoming less tested. The rewritten gate is at or near 100%: ResolveWatchRuleSourceScope, decide, overrideDelegated, aggregateSourceScope and summariseAdmitted are all fully covered, as are the new commit-config validation and the per-target commit window. Before lowering anything, the two real gaps this left are closed and one piece of dead surface is removed: - SourceNamespaceDecision.Admitted had no callers once the verdict became two-valued. Deleted rather than left as an untested accessor. - NamespacesFor's bounds check is pinned. It is indexed by an item's position in spec.rules, and the resolved scope and the spec are two objects, so a caller reading them apart must get nil rather than a panic. - The aggregate message's deduplication is pinned, along with the cluster-wide cell being spelled out instead of rendered as the empty string an operator would read as a missing value. The baseline moves to the measured 76.9%. CI's 76.8% sits inside the 0.5% tolerance, and the gate ratchets up again from here on its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…y at admission Review found that the retained-and-refused pattern was only half implemented. Admission rejected each removed or renamed field, which covers the WRITE path and nothing else: an object written by an earlier release keeps its value in etcd and is never re-admitted, so admission alone missed exactly the population that needs telling. Three holes, all confirmed by reading the code rather than assumed: - A stored GitTarget.spec.allowedSourceNamespaces was fully inert. Nothing outside the type declaration read it, so it stayed on the object reading like a bound on which source namespaces reach a folder while enforcing nothing - and the moment its ClusterProvider is migrated, a `sourceNamespace: "*"` rule under it widens from that declared set to every namespace the credential can read. - A stored GitProvider.spec.push / spec.commit.message marked the provider Stalled and stopped nothing. validateProviderAndBranch checks only existence and allowedBranches, and GitProviderReady is published AFTER worker wiring and DeclareForGitTarget, so the target kept writing - at the default cadence and wording, since neither value is read any more. UPGRADING claimed "nothing is silently reinterpreted"; that was false as shipped. - Not in the review, found while fixing it: a stored ClusterProvider. spec.allowedNamespaces leaves accessFrom absent, which is deny-by-default, so every GitTarget through it failed with a message blaming its namespace rather than naming the rename. One shared authz.SupersededFieldRefusal now serves all three kinds under one reason, SupersededFieldStored, wired at the three places that make it a gate rather than a remark: the GitTarget's Validated gate (ahead of provider/branch validation, so it returns before worker wiring and DeclareForGitTarget), both compile paths (which is what closes the bootstrap window - bootstrap seeds the store before the first reconcile on every restart), and GitTargetAdmitted, ahead of the accessFrom policy read. The GitProvider reconciler's private copy of the message is folded into the shared helper; it would have drifted. A DEFAULTED field is refused only at its meaningful value, and e2e is what caught it. allowSourceNamespaceOverride carried +kubebuilder:default=false, so the apiserver wrote it into EVERY stored ClusterProvider - the chart-owned "default" one included, on installs that never used the feature. Refusing every non-nil value refused all of them, and re-applying the clean manifest does not remove it, because a server-defaulted field was never in the user's manifest to remove. Verified against a live cluster, not argued. That is an upgrade nobody could complete, so only `true` is refused, which is also what this repo's own DeclaresNamespacedScope already does with its retained field. Docs, which the review was right to call out separately: docs/spec is declared binding on the code, and two spec pages still described the old behaviour. commit-window-refactor.md named GitProvider.spec.push.commitWindow as the user-facing control and carried a "why it lives there" rationale this release reverses; status-conditions-guide.md documented three condition reasons that no longer exist and a three-valued precedence table. architecture.md still said a wildcard expands to one stream per admitted namespace in the WatchedTypeTable section - the one component the change is about. UPGRADING now states the operational consequence it was missing: an unmigrated object stops writing until it is edited, which is deliberate, because a folder committing on settings nobody chose is worse than one that has visibly stopped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ining and refusing them Product decision: with one known consumer and 17 release-asset downloads across the last twelve releases, ~700 lines of refusal machinery is more surface than the population it protects. The fields go, and docs/UPGRADING.md carries the migration instead. Removed from the schema entirely, rather than retained-and-refused: GitTarget.spec.allowedSourceNamespaces GitProvider.spec.push (and the PushStrategy type) GitProvider.spec.commit.message ClusterProvider.spec.allowedNamespaces ClusterProvider.spec.allowSourceNamespaceOverride With them go authz.SupersededFieldRefusal, the GitTarget refusal helper, the wiring in both compile paths, the GitTarget Validated gate, the GitProvider reconciler check and the ClusterProvider admission check: 1,132 lines deleted against 124 added. ClusterWatchRule.spec.rules[].scope is UNTOUCHED and stays retained-and-refused. It is a narrowed enum rather than a removal, its refusal is three lines, and the envtest pinning the apiserver property it depends on stays with it. What this trades away, stated plainly because the trade is the point: an object still carrying an old spelling is now accepted with the value pruned, and nothing warns. Three of the five prune fail-closed and loud (a missing accessFrom admits nothing; a dropped delegation stalls every cross-namespace rule). One fails OPEN and quiet: dropping allowedSourceNamespaces widens a `sourceNamespace: "*"` rule to every namespace its credential can read. UPGRADING says so in those words. The migration guide therefore leads with a pre-upgrade inventory, and that ordering is measured rather than assumed: an envtest confirmed a value stored under the old schema is invisible IMMEDIATELY once the field leaves the structural schema, not merely after the next write. So the inventory cannot be taken afterwards, and the guide says to recover from Git or an etcd backup if you upgrade first. Both documented commands were run against a live cluster. Also addresses two review points: - The coverage baseline moves to the measured 77.0%, up from 76.9%. Restoring 77.4% as asked would fail CI: when local measured 76.9% CI measured 76.8%, and 77.4% belongs to a tree that still had ~1,400 statements of very-well-covered source-scope code. Reaching it now would mean writing tests for unrelated pre-existing gaps (syncWithRemote, AuthFromSecretData, convergesAs), which is not this PR's business. - The ClusterProvider status/contract mismatch is resolved by the deletion: there is no longer a refusal for its conditions to fail to report. The envtest that pinned the removed refusals is replaced by one that pins the REPLACEMENT spellings round-trip rather than being silently pruned, which is the failure this design can no longer catch any other way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…eld changes CRDs are this product's configuration surface, so the cost of an API change is not the code - it is what happens to objects that already exist, and almost none of that is visible from the Go types. This wave established several of those facts the expensive way; they belong somewhere more durable than a PR thread. The page leads with a decision matrix keyed on the question that actually decides the strategy and is the one most often skipped: when the field is pruned, does the object do MORE or LESS? Less is an outage - loud and recoverable. More is a silent widening. Of the five fields this wave removed, four pruned fail-closed and exactly one failed open, so one row carried nearly all the risk. Measured facts, each naming the test that produced it, and separated from the inferred ones so a reader knows which is which: - A status update does not re-validate spec, on 1.36 and on 1.31 with CRDValidationRatcheting explicitly on AND off. Without this, retain-and-refuse is unusable: you could reject a value and never report the rejection on the object carrying it. - The ratcheting gate cannot be disabled from 1.33; kube-apiserver refuses to start. Test the "gate off" case on <=1.32 or not at all. - A field removed from the structural schema stops being SERVED immediately, not after the next write. This is the opposite of the common assumption and it puts a hard ordering constraint on every migration guide: the inventory must be taken before the new CRDs are applied. - A previously-defaulted field cannot be removed by `kubectl apply`, verified against a live cluster. So refusing every non-nil value of one refuses every object that has ever existed, and the operator cannot fix it. The rule that follows - refuse a defaulted field only at its meaningful value - is what ClusterWatchRuleSpec.DeclaresNamespacedScope was already doing. It also records why a conversion webhook was not the answer here (conversion is per-object, so it cannot move a field to another kind, invent intent for a redefined value, or restore a deleted capability - two of this wave's five changes were mechanically convertible, both trivial renames), and the test for whether a version bump is worth it: read the migration guide you would write either way, and if it does not get shorter, the bump is not paying for anything. The closing section states when the delete-and-document choice expires - a second consumer, an uncontactable one, or a field that fails open on prune - so the reversal is not cargo-culted at a size where it stops being safe. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…are silent Three corrections to the upgrade guide, which is now the only migration guard and therefore has to be right. All three were verified against the code and the commands against a live cluster. Step 4 gave the wrong reason and the wrong order. It said to apply GitProvider first "because a GitTarget reads its commit settings from it" - which is exactly backwards after this wave, since commitWindowFor and the message resolution both read the TARGET now. That was leftover reasoning from the old object model, and following it would leave targets on defaults until their own change landed. There is no ordering requirement at all: nothing here reads its migrated value from another object, so the advice is now one sync, which makes the stall a single reconcile. Step 3 claimed "mirrors do not pause", which was too broad. It is true for a pruned GitTarget or GitProvider field and false for a pruned ClusterProvider policy: an absent accessFrom is deny-by-default, so GitTargetAdmitted denies, Validated goes False and the target stops writing. The two outcomes are opposites and the guide now says so - and says which one to hurry for, because the one that keeps running is the one not telling you anything. Step 5 verified only the migrated window, so a pruned message template could pass unnoticed - a target with no templates commits perfectly happily under the built-in ones. It now reads back the whole spec.commit. Checked on a live cluster: a migrated target renders map[message:map[...] window:30s] and a half-migrated one renders map[window:0s], which is the difference the step tells you to look for. Step 5's framing needed the same correction as step 3 while I was there: Ready does catch a pruned ClusterProvider policy, and does not catch a pruned GitTarget or GitProvider field. Only the second needs the value read back. Co-Authored-By: Claude Opus 5 <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.
PR 3 of the GitTarget API track: the breaking wave, and the only PR in the queue
that costs consumers a coordinated bump. One bump, one release —
0.43.0.Specified by
docs/design/gittarget-api-wave.mdand
docs/design/source-scope-simplification.md;sequenced by
docs/design/build-order.md.The API comes out smaller than it went in:
-829lines of non-test Go,-607of tests and
-72of generated CRD, against+501of documentation.The envtest that gated the planning, and its answer
gittarget-api-wave.md§ "The envtest that has to run before any of this isplanned" asked whether the controller can write a status update onto a stored
ClusterWatchRulewhosespec.rules[].scopeisNamespaced, or whether theapiserver rejects it 422. If a refused object cannot publish status, the object
that most needs to explain itself is the one that cannot.
It can. Measured on Kubernetes 1.36 and on 1.31 with
CRDValidationRatchetingexplicitly on and explicitly off — the statussubresource does not re-validate spec at all, so the property never depended on
ratcheting. A create carrying the same value is still rejected. No enum
widening, no fallback. (Also recorded: from 1.33 the gate cannot be turned off —
kube-apiserver refuses to start on
CRDValidationRatcheting=false.)Pinned as a regression test, because
ClusterWatchRule.spec.rules[].scopestilldepends on it.
What changed
B4 — commit batching and message templates move onto GitTarget
GitProvideris the connection: a URL, a credential, the branches it willaccept. How a folder's writes are batched and phrased describes the folder, and
two
GitTargets sharing oneGitProviderhad no way to disagree about either.Grouped under
spec.commitrather than landing as two top-level fields — themove is breaking either way, so the grouping is free here and would cost a bump
in any later release.
commit.committerandcommit.signingstay onGitProvider; both describe the identity that talks to the remote.The window resolves per open window, not per worker. A window is bound to
exactly one
GitTargetby construction — it finalizes the moment the targetchanges — so a
(provider, branch)worker serving two targets honours twocadences. Template validation moved with the field onto the GitTarget's
Validatedgate and now covers the window string too.spec.commit.messagekeeps its three templates rather than collapsing to thesingle
templatethe design's example YAML shows. That example uses a{{ .Summary }}variable which exists in none of them; the three rendergenuinely different things with different variable sets, so one template could
only have been a fourth thing. Flagged as a judgement call.
The source-scope deletion
Four parts, shipped together because
"*"was defined in terms of the fieldbeing deleted.
allowedSourceNamespacespresented itself as a destination policy and could notbe one:
targetRefis namespace-local,providerRefis namespace-local, andspec.pathis immutable, so the chain from a Git folder back to the object thatfills it never leaves one namespace. Its selector half was evaluated against
Namespacelabels in another cluster, and that single choice produced thethree-valued verdict, the
SourceScopeUnavailabledegradation path, fivecondition reasons, the establishing/maintaining retention memory, and the
operator's need for source-cluster
Namespaceget/list/watch. All of it goes.The operator now reads no
Namespaceobjects in a source cluster at all, andinternal/authz's verdict collapses from three-valued to two.accessFromstays, renamed. It is the one boundary available nowhere else:source RBAC bounds what a credential may read and cannot express which
control-plane tenant may wield it.
"*"is now one cluster-wide list and watch, refused whileallowAnySourceNamespaceis false. The plumbing existed —CellKeydocumentsthe empty-namespace case and both
openTargetWatchandopenTargetListalreadybranch on it — so this is a deletion in the compile path plus one router fix. A
cluster-wide cell stays a peer of a named-namespace cell, never a
replacement, because each rule carries its own
operationsfilter;CellKey'sdoc comment records the bug from the previous attempt.
Superseded fields are deleted outright, not retained and refused
This reversed mid-review, deliberately. The wave was built first with the repo's
retained-and-refused convention — keep the field, reject it, for one release —
and that is now removed in favour of deleting the fields and putting the
migration in a document.
The reason is reach, not taste: one known consumer, and 17 release-asset
downloads across the last twelve releases. At that size the refusal machinery
(457 lines in four files plus wiring at four call sites) is more surface than the
population it protects.
ClusterWatchRule.spec.rules[].scopeis untouched and staysretained-and-refused. It is a narrowed enum rather than a removal, its refusal is
three lines, and it is pre-existing.
What this trades away, since the trade is the point. An object still carrying
an old spelling is accepted with the value pruned, and nothing warns. Four of the
five prune fail-closed and loud — a missing
accessFromadmits nothing, adropped delegation stalls every cross-namespace rule. One fails open and
quiet: dropping
allowedSourceNamespaceswidens a"*"rule to everynamespace its credential can read, with no condition to notice.
docs/UPGRADING.mdsays so in those words, and leads with a pre-upgradeinventory because of it.
Migration
docs/UPGRADING.mdgains a "Safe upgrade order" checklist, and the orderingis measured rather than assumed: an envtest confirmed a value stored under the
old schema is invisible immediately once the field leaves the structural
schema — not after the next write, straight away. So the inventory cannot be
taken afterwards, and the guide says to recover from Git or an etcd backup if you
upgrade first. Every documented command was run against a live cluster.
Two breaking semantic changes are stated rather than papered over:
allowAnySourceNamespace: falseis not exactly today's posture. A declaredallowedSourceNamespaceswas exhaustive with no self-namespace exception, soit could deny a rule's own namespace. The new default matches the no-policy
path, which is what a default install ran.
"*"widens. It keeps its spelling and changes its meaning, so it gets itsown paragraph rather than a shim — there is nothing to rename.
UPGRADING.mdcarries a
jqone-liner to find the affected rules.Source-side label selectors are lost, with no replacement. That is the real
capability cost, accepted rather than overlooked.
New reference: docs/facts/crd-upgrade-strategies.md
The API-server behaviour this wave established is written up as a facts page,
because CRDs are this product's configuration surface and the cost of an API
change is not the code — it is what happens to objects that already exist, almost
none of which is visible from the Go types.
It carries a decision matrix keyed on the question that actually decides the
strategy: when the field is pruned, does the object do more or less? Less
is an outage — loud and recoverable. More is a silent widening. Four measured
facts, each naming its test and separated from the inferred ones, plus why a
conversion webhook could have automated only two of this wave's five changes
(conversion is per-object: it cannot move a field to another kind, invent intent
for a redefined value, or restore a deleted capability).
Not in scope
SelfSubjectAccessReviewpass — out by specification, not by trimming:source-scope-simplification.md§ "The one thing to build" puts it outside thisPR because it is additive and should not widen the release that costs a bump.
#5assertedCommitRequest.spec.author, theCommitRequestlifecycle hole,
meta.LocalObjectReferencefor the six reference shapes, theTooManyStreamscap) — trimmed under the wave document's own rule, sincenothing depends on them and each is under-specified or substantial in its own
right.
TooManyStreamsin particular is explicitly "not planned before the*change lands", and that change is in this PR.defaultClusterProvidermessage (B6) — already shipped. Theexisting
ClusterProviderNotFoundmessage already names both fixes. Confirmed,not assumed.
v1alpha4. Staysv1alpha3. A version bump automates nothing here, andserving two versions would reintroduce the silent prune.
v1alpha4is forsweeping accumulated residue, not for carrying one change.
What PR 2 left that this touches
The one-source-namespace rule needed no rewrite, and each site was read rather
than taken on trust:
internal/git/source_namespaces.go—computes its set from
WatchRuleobjects. Cross-reference to the deleted fieldupdated; the sentence it made stays true.
internal/git/namespace_policy.go—SourceNamespaceWildcardanddeclaredNamespace(). Unchanged.internal/webhook/watchrule_source_namespace_admission.go— no code change needed: it already refuses
"*"on the grounds that it"cannot be shown to be one namespace", which holds under both readings.
internal/manifestanalyzer/source_namespace_fence.go— carries its own copy of the
"*"constant. The comment now records that onlythe spelling is duplicated, and that the meaning has already changed once
with no edit here.
Test coverage
test/e2e/source_namespace_e2e_test.gois rewritten rather than deleted:the placement claim (Git placement follows the mirrored object's own namespace,
never the WatchRule's), the refusal path, and a wildcard spec that asserts the
widening — two namespaces that nothing names both arrive, which is what the
redefinition means and what no API shape shows. The planner and stream-summary
suites under
internal/watch/are likewise kept and adapted.An envtest pins that the replacement spellings round-trip rather than being
silently pruned — a typo in
accessFromorspec.commitis the failure thisdesign can no longer catch any other way.
Validation
task lint,task test(77.0%, ratchet holding) andtask test-e2e(84 passed,0 failed) all pass locally; CI is green on all 16 checks including the six e2e
legs. The API-comment placement check from
AGENTS.mdwas run — regenerated withcontroller-geninto a scratch dir and diffed againstconfig/crd/baseswithevery
descriptionstripped: only descriptions moved, no marker displaced.Two findings worth flagging, because neither was reachable from lint or the unit
suite:
allowSourceNamespaceOverridecarried+kubebuilder:default=false, so theapiserver had written it into every stored
ClusterProvider— including thechart-owned
defaultone, on installs that never used the feature. Verifiedagainst a live cluster that
kubectl applycannot remove a server-defaultedfield. Under the retained-and-refused design that was an unfixable upgrade.
carrying real upgrade residue; CI seeds from current manifests and would not
have reproduced either.