feat(api)!: move commit batching onto GitTarget, delete allowedSourceNamespaces, and make sourceNamespace "*" cluster-wide - #330
Open
sunib wants to merge 5 commits into
Open
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 112 files, which is 12 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 (112)
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>
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.
Specified by
docs/design/gittarget-api-wave.mdand
docs/design/source-scope-simplification.md;sequenced by
docs/design/build-order.md.The envtest that gated the planning, and its answer
gittarget-api-wave.md§ "The envtest that has to run before any of this is planned"asked whether the controller can write a status update onto a stored
ClusterWatchRulewhosespec.rules[].scopeisNamespaced, or whether theapiserver rejects it 422. The whole loud-rejection pattern rests on the answer:
if a refused object cannot publish status, the one object that most needs to
explain itself is the one that cannot.
It can. Measured three ways — Kubernetes 1.36 (what
ENVTEST_K8S_VERSIONderives from
k8s.io/api v0.36.4), and 1.31 withCRDValidationRatchetingexplicitly on and explicitly off. In every case the status update
succeeded, while a create carrying the same value was rejected
Unsupported value: "Namespaced": supported values: "Cluster".The worry was that ratcheting was doing the work and would vanish on an older
cluster. It is not: the status subresource does not re-validate spec even with
the gate off. So "keep the field, refuse the value" is safe here, no enum
widening and no fallback. (Also recorded: on 1.33+ the gate cannot be turned off
at all — kube-apiserver refuses to start on
CRDValidationRatcheting=false.)That probe ships as a regression test rather than being thrown away
(
internal/controller/stored_superseded_value_status_test.go), because fiveretained-and-refused fields now depend on the property.
What is in it
B4 —
commitWindowandcommit.messagemove onto GitTargetGitProvideris 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 theidentity that talks to the remote.
The commit window is resolved per open window rather than once per worker. A
window is bound to exactly one
GitTargetby construction (it finalizes themoment the target changes), so a
(provider, branch)worker serving two targetshonours two cadences. Template validation moves with the field onto the
GitTarget's
Validatedgate, and now covers the window string too.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.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 outright 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, not
new machinery. A cluster-wide cell stays a peer of a named-namespace cell,
never a replacement, because each rule carries its own
operationsfilter;CellKey's doc comment records the bug from the previous attempt.Loud rejections, verified against the generated CRDs
Every removed or renamed field is retained in the schema and refused with a
message naming its replacement — five of them. Pruning happens on write, so
deleting a field drops the value from a re-applied manifest with no error at all;
for
allowSourceNamespaceOverride: truethat would silently revoke adelegation. An envtest applies each one and asserts the rejection names the fix.
What was left out, and why
The riders are trimmed.
gittarget-api-wave.mdsays they can be, sincenothing depends on them, and this PR is already 70+ files across two breaking
commits. Each remaining rider is also under-specified where it stands, or
substantial in its own right:
CommitRequest.spec.author— needs a fail-closed SARindependent of the webhook's
failurePolicy. Real work, not a rider in size.CommitRequestlifecycle hole (F10) —ttlSecondsAfterFinishedor anownerRef, plus the
deleteverb. No design of record beyond the one-line ask.meta.LocalObjectReferencefor the six reference shapes (F12 remainder) —a breaking change across six types with no migration written down.
TooManyStreamscap — the wave document says explicitly it is "notplanned before the
*change lands". That change is in this PR, so its designstarts now rather than shipping with it.
defaultClusterProvidermessage (B6) — already shipped. Theexisting
ClusterProviderNotFoundmessage already names both fixes(
clusterProvider.createDefault, or commit the object). Confirmed, not assumed.The
SelfSubjectAccessReviewpass is out by specification, not by trimming:source-scope-simplification.md§ "The one thing to build" puts it explicitlyoutside this PR because it is additive and should not widen the one release that
costs a bump.
Two breaking semantic changes, said out loud
Both are in
docs/UPGRADING.mdrather than papered over:allowAnySourceNamespace: falseis not exactly today's posture. A declaredallowedSourceNamespaceswas exhaustive once declared, with no self-namespaceexception, so it could deny a rule's own namespace. The new default matches
the no-policy path, which is what a default install ran.
*widens, from "every namespace this GitTarget admits" to "everynamespace this credential can read". For a target that declared no policy it
already resolved to whatever the credential could see; for one that declared
a policy, it is real, and
UPGRADING.mdcarries ajqone-liner to find them.Source-side label selectors are lost, with no replacement. That is the real
capability cost and it is accepted, not overlooked: an N-way restriction costs N
objects wherever it is expressed. It has its own paragraph in the migration note.
sourceNamespace: "*"keeps its spelling and changes its meaning, so it gets itsown
UPGRADING.mdparagraph rather than a shim — there is nothing to rename.What PR 2 left that this touches, confirmed rather than assumed
The one-source-namespace rule needed no rewrite, and each site was read to check
it rather than taken on trust:
internal/git/source_namespaces.go— computes{target's own namespace} ∪ {explicit rules[].sourceNamespace}fromWatchRuleobjects. Its doc comment'scross-reference to the deleted field is updated; the sentence it made stays true.
internal/git/namespace_policy.go—SourceNamespaceWildcardanddeclaredNamespace()refuse to attribute under a wildcard. Unchanged.internal/webhook/watchrule_source_namespace_admission.go— refuses*statically. No code change needed: it already refuses on the grounds that
*"cannot be shown to be one namespace", which holds under both readings.internal/manifestanalyzer/source_namespace_fence.go— carries its own copy ofthe
*constant. The spelling survives; the comment now records that only thespelling is duplicated, and that the meaning has already changed once with no
edit here.
Test coverage
Deleting
test/e2e/source_namespace_e2e_test.gooutright would have removed theonly end-to-end coverage of source scoping, so it is rewritten rather than
dropped: the placement claim (Git placement follows the mirrored object's own
namespace, never the WatchRule's), the refusal path, and a wildcard spec that now
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 keptand adapted rather than deleted with the service they tested; the wildcard cases
now assert one cluster-wide scope and its peer relationship with a named one.
Net: ~2,900 lines deleted, and
internal/authz's three-valued verdictcollapses to a two-valued one — an item is authorized or refused, and a refusal
is terminal, because every input is now a control-plane object the reconcile
already has.
Validation
task lint,task test(unit + coverage ratchet) andtask test-e2eall pass.The API-comment placement check from
AGENTS.mdwas run: regenerated into ascratch dir with
controller-genand diffed againstconfig/crd/baseswith everydescriptionstripped — only descriptions moved, no marker was displaced.