docs: five-PR plan for WatchRule source namespaces, plus PR 1 — namespace-scoped resync#254
Merged
Merged
Conversation
…ad to 1.0 The quick start pinned cert-manager v1.20.3, a version that appeared nowhere else in the repo and was never exercised. It also omitted the rollout wait (the controller cannot start until cert-manager issues the admission certificate) and told users to check `Ready` on the GitTarget, which is held at Unknown until first source discovery — the e2e asserts `Validated` for exactly this reason. Also: - Document the Kustomize support that already ships. The write path runs kustomize itself as a pre-commit oracle; the README previously claimed it "cannot infer the original structure of arbitrary templates or overlays", which the code contradicts. - Stop over-hedging delivery. While the watch is connected every change is seen and committed; collapsing to current state only happens across a gap. - Reframe HA from a standalone caveat into a road-to-1.0 list alongside the configuration surface (six CRDs still v1alpha3, no conversion path) and docs. - Move the attribution mode tables into the attribution setup guide, which already opened with the same concept. - Warn that `helm uninstall` removes the cluster-scoped default ClusterProvider, and that the default RBAC grants cluster-wide Secret reads. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…vate Dependabot covers gomod, github-actions, and Dockerfile FROM lines, and it is healthy — every direct Go dependency is current. The pins it cannot see had drifted, including a cert-manager version in the README that existed nowhere else in the repo. Cluster charts (validated by the e2e suite): - cert-manager 1.19.4 -> 1.21.0. Its three breaking changes (tokenrequest RBAC, cert-manager-edit ACME permissions, metrics Helm values) do not apply: we set no cert-manager values beyond crds.enabled, use a self-signed Issuer, and have no serviceAccountRef. - valkey 0.9.3 -> 0.10.0, kcp-operator 0.7.7 -> 0.8.3. - gitea 12.5.1 -> 12.5.3, deliberately NOT the latest 12.7.0. Gitea 1.26 removed the _csrf form input and cookie from /user/login, which the giteaclient WebSession scrapes to verify SSH signing keys, so 12.6.0+ fails the signing e2e. 12.5.3 carries the same Gitea 1.25.5, so this is a chart-only patch. Devcontainer toolchain (needs an image rebuild to take effect): flux2 2.9.2, flux-operator 0.55.0, helm 4.2.3, task 3.52.0, cosign 3.1.2. cosign is pinned twice and both places move together. Renovate is scoped to only what Dependabot cannot see, so the two never open competing PRs: Flux HelmRelease charts, the devcontainer ENV pins, the cert-manager URL in the README, and the cosign-installer input. It holds gitea below 12.6.0 until the web session helper is reworked. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Records the investigation behind unpinning Gitea rather than migrating to Forgejo, read from the upstream sources in external-sources/. The premise for migrating was that we were stuck on chart 12.5.x because Gitea 1.26 removed the `_csrf` form input from /user/login, which our WebSession scrapes. That premise does not survive contact with the source: Gitea and Forgejo made the *same* change, both replacing form-token CSRF with Go's stdlib http.CrossOriginProtection, which admits any request carrying neither Sec-Fetch-Site nor Origin. The fix is a deletion, and it works on either server — so migration and unpinning are not the same piece of work. That leaves a cost comparison Gitea wins: it keeps the literal "gitea" signing namespace (Forgejo switched to the ROOT_URL hostname, the one hazardous change), keeps the session cookie name, keeps an HTTP chart repo, and has the better trusted-key lever — TRUSTED_SSH_KEYS reports the commit's own committer email where Forgejo's instance key reports a fixed configured address, which is what our signing assertion checks. Also records why no Go SDK is adopted on either server: there is no official Forgejo SDK, no SDK can cover the web-UI verification flow, the Gitea SDK's version gating is meaningless against Forgejo, and it would add five dependencies to test-only code that is currently stdlib-only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Gitea 1.26 replaced form-token CSRF with Go's stdlib
http.CrossOriginProtection (routers/web/web.go). That check returns nil when a
request carries neither Sec-Fetch-Site nor Origin — every non-browser Go client —
and POST /user/login is exempt besides, being SignOutRequired. So the token our
WebSession scraped is not merely absent, it is unnecessary.
Delete fetchCSRF, extractCSRFToken, csrfInputRE and both form.Set("_csrf", ...)
calls, and move the chart from 12.5.3 (Gitea 1.25.5) to 12.7.0 (Gitea 1.27.0) —
the version whose signing spec failed earlier today. HTML-scraping a hidden input
was the most brittle part of the client, so this is a net reduction in fragility
as well as ~45 fewer lines.
The Renovate guard holding gitea below 12.6.0 goes with it; it existed only to
describe this problem.
Validated on Gitea 1.27.0: task lint, task test (coverage 76.6%, baseline
unchanged), and task test-e2e — 58 passed, 0 failed, including the signing specs
that drive the web session.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Validation climbs a ladder: OpenAPI schema, then CEL XValidation, then the reconciler for anything cross-object. A validating admission webhook is not a rung — it is correct only when the information exists solely at admission time and nowhere on the persisted object. Writing it down once so it stops being re-argued per feature. The reasoning was already settled in practice by the config-plane split, which enforces ClusterProvider.spec.allowedNamespaces on every reconcile rather than at admission: reconcile-time re-evaluates continuously (so it catches a policy tightened after the object was created), the data-plane ordering is what provides the security property, and admission buys only feedback latency in exchange for cert wiring in the chart. Also records why /validate-operator-types is not a counterexample: it captures submitter identity and always allows, so it is not a validation gate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…sions The record had grown to 474 lines carrying a build order, a test plan, an API dump and open questions that are all either shipped or now owned elsewhere. Cut to the decisions and the reversals, which is the part a finished doc is actually for. Corrects one claim that read as current but never shipped: an earlier draft reserved the name "default" for the in-cluster cluster via CEL. What shipped derives locality from spec.kubeConfig alone, for every name — a provider named "default" may carry a kubeConfig and mirror a remote cluster. Records why the reservation was rejected (it pins a name to a physical cluster, the same silent-retarget hazard the immutability rules exist to prevent). Also drops the "enforced in two places" admission-webhook wording for allowedNamespaces, which shipped as a single reconcile-time gate, and points at the new spec/where-validation-lives.md for the general rule. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Revises the WatchRule source-namespace design. The previous revision gated the new field on kubeConfig presence (remote-only, in-cluster forbidden), which is wrong twice over: it derives an addressing capability from a connectivity setting, so setting a kubeconfig to reach a cluster silently grants namespace addressing; and it welds the in-cluster case shut forever, conflating "unsafe by default" with "impossible". Replaced by an explicit deny-by-default allow-list on the platform-owned object, ClusterProvider.spec.allowedSourceNamespaces, mirroring the existing allowedNamespaces exactly. One mechanism instead of two: an empty allow-list already is a gate, so no separate boolean is needed, and the only capability a boolean uniquely adds — "any namespace" — is the one the doc argues must not ship upstream. Names: sourceNamespace on WatchRule (the longer sourceClusterNamespace was justified only by the remote-only rule, which is gone), and the allowed* prefix retained on the provider because the allow-list IS the gate and the name must foreclose a fail-open reading. Records allowedGitTargetNamespaces + allowedSourceNamespaces as the symmetric pair to adopt at the next API version rather than breaking v1alpha3. Keeps the two code findings that make the upstream ask small: the Git path already uses the source object's namespace, so the write side needs no change, and ClusterWatchRule already watches the source cluster across all namespaces but carries two pre-existing defects. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 19 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 (6)
📝 WalkthroughWalkthroughThe PR introduces namespace-aware resync scopes across watch and Git pipelines, updates commit template context, adds resync regression tests, documents validation and source-namespace authorization designs, revises a multi-cluster attribution record, and changes the e2e KCP operator chart pin. ChangesNamespace-scoped resync
Validation and source-namespace documentation
Sequence Diagram(s)sequenceDiagram
participant WatchKey
participant EventRouter
participant GitResync
participant Worktree
WatchKey->>EventRouter: derive GVR and namespace scope
EventRouter->>GitResync: enqueue ResyncRequest with ResyncScope
GitResync->>Worktree: build scoped resync plan
Worktree-->>GitResync: apply namespace-aware sweep
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
The comment claimed AllowsNamespace was "the single authorization predicate shared by the admission webhook and the reconcile-time refusal, so the two can never diverge". There is no such webhook — the proposal was dropped, and checkSourceAuthorization in gittarget_source_cluster.go is the only non-test caller. Replaces it with what actually holds: enforcement is reconcile-time and nowhere else, it returns before DeclareForGitTarget, and that is deliberate rather than incidental because continuous re-evaluation also covers a policy tightened after the GitTarget was created — something admission could not see. Comment-only: no generated CRD drift (it documents a method, not a field), and `task lint` passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…ment-and-source-namespace
The dep refresh read v0.8.3 (the appVersion kcp-operator 0.7.7 ships) as the chart version. No such chart exists — kcp-dev/helm-charts publishes nothing above 0.7.x — so the HelmRelease never left SourceNotReady and the e2e source-cluster leg died in setup with "no 'kcp-operator' chart with version matching '0.8.3' found". The earlier fix on this branch was invisible to the merge: it restored the merge-base value, so the three-way merge saw a change only on main's side and took 0.8.3. Merging main in first makes this pin an explicit change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…R pages The single 783-line design had grown past the point where it could be picked up and implemented. Split it into a README that carries the decision and the sequencing, plus one page per shippable PR: - PR 1: collapse the ClusterWatchRule stream scope - PR 2: admit ClusterWatchRule targets - PR 3: the sourceNamespace field and its GitTarget allow-list gate - PR 4: the ClusterProvider source ceiling that binds ClusterWatchRule too PR 4 is not optional cleanup: a multi-tenant deployment runs a ClusterWatchRule per tenant from day one to capture CRDs, so an allow-list that binds only WatchRule would leave the wider path unguarded. Repoint the INDEX entry and the multi-cluster-attribution cross-reference at the new directory. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A per-namespace replay produced a `desired` set covering one namespace, but the resulting mark-and-sweep was scoped by (group, resource) only. Every managed document of that type in every other namespace was therefore absent from `desired` — and a sweep deletes what is absent. The scope was present at both ends and discarded in the middle: `targetWatchSpecs` already built per-namespace watch keys, `enqueueReplayResync` passed only `key.GVR` onward, and `resyncPlan`'s predicate matched group and resource while `types.ResourceIdentifier` carried the namespace all along. Replace `ScopeGVR *schema.GroupVersionResource` with `Scope *ResyncScope` (GVR + Namespace) across ResyncRequest and PendingWrite, so the two halves of a scope travel together and a call site cannot pass one without the other. An empty Namespace keeps today's all-namespaces meaning, which is what a ClusterWatchRule's cluster-wide stream gathers. `resyncHealKey` now separates namespaces too, so a parked heal for one namespace no longer replaces another's. The defect is latent today: a GitTarget can only watch one namespace per GVR, because WatchRule.targetRef is namespace-local and a ClusterWatchRule's "" key collapses the type to all-namespaces. It goes live with the first change that fans a GitTarget out across namespaces — which is every remaining PR in docs/design/watchrule-source-namespace/. Hence this lands first. Verified: reverting the namespace half of ResyncScope.Matches fails TestResync_NamespaceScopedSweepLeavesSiblingNamespacesAlone and the sibling-namespace row of TestResyncScope_MatchesRespectsTypeAndNamespace. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
docs/INDEX.md (1)
83-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImprove sentence flow.
Consider replacing "also why" with "and why" for better readability, as suggested by static analysis.
📝 Proposed fix
-| [`e2e-git-server-choice.md`](design/e2e-git-server-choice.md) | stay on Gitea or move to Forgejo — the `_csrf` pin is fixable in place on both, so the migration is now a preference call, not a fix; also why we adopt no SDK either way | +| [`e2e-git-server-choice.md`](design/e2e-git-server-choice.md) | stay on Gitea or move to Forgejo — the `_csrf` pin is fixable in place on both, so the migration is now a preference call, not a fix; and why we adopt no SDK either way | | [`watchrule-source-namespace/`](design/watchrule-source-namespace/README.md) | letting a WatchRule address a differently-named namespace on its source cluster — a deny-by-default `allowedSourceNamespaces` on the **GitTarget** (so scope is per-tenant, not a provider-wide union), unlocked by a false-by-default delegation flag on the ClusterProvider. Split into five implementable PRs: three prerequisite scope fixes (the namespace-blind resync sweep that would delete other namespaces' manifests, the cluster-wide/named stream collapse, and ClusterWatchRule's unchecked GitTarget attachment), the field and its gate, and the ceiling that makes the allow-list bind ClusterWatchRule too — required because a multi-tenant deployment runs a ClusterWatchRule per tenant from day one to capture CRDs |🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/INDEX.md` around lines 83 - 84, Update the e2e-git-server-choice.md entry in the docs index to replace “also why” with “and why,” preserving the rest of the description unchanged.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/watchrule-source-namespace/pr1-namespace-scoped-resync.md`:
- Around line 15-42: Update the design document’s defect description to reflect
that event_router.go now carries git.ResyncScope and target_watch.go passes
resyncScopeForWatchKey(key), preserving key.Namespace. Remove or rewrite the
stale “PR1 is not started” release-gate narrative, and clearly label the
described behavior as historical or identify only the remaining
namespace-scoping gap.
In `@docs/design/watchrule-source-namespace/pr2-stream-scope-collapse.md`:
- Around line 31-33: Clarify the empty-namespace claim for namespaced selections
only: in docs/design/watchrule-source-namespace/pr2-stream-scope-collapse.md
lines 31-33, state that a declared ceiling removes the "" selection for
Namespaced streams, while
docs/design/watchrule-source-namespace/pr5-clusterwatchrule-source-ceiling.md
lines 62-66 must preserve the cluster-scoped exception and lines 90-106 must
describe the resulting expansion behavior accordingly.
In `@docs/design/watchrule-source-namespace/pr4-source-namespace-field.md`:
- Around line 239-246: Define one consistent contract for unknown
selector-policy handling across the PR4 and PR5 design sections: distinguish
terminal failures from retryable or unresolved failures, specify the resulting
status, RuleStore retention, stream behavior, and resync semantics, and align
the tests accordingly. In
docs/design/watchrule-source-namespace/pr4-source-namespace-field.md lines
239-246, document the agreed terminal-versus-unknown behavior; in lines 286-290,
align compiled-rule removal with that contract; and in
docs/design/watchrule-source-namespace/pr5-clusterwatchrule-source-ceiling.md
lines 147-159, align retained scope and status semantics.
In `@docs/design/watchrule-source-namespace/README.md`:
- Around line 202-210: Correct the phase references in the compatibility table
in docs/design/watchrule-source-namespace/README.md: assign source fields,
delegation, and SourceNamespaceAuthorized to PR4, ClusterWatchRule target
admission to PR3, and the ClusterWatchRule ceiling to PR5. In
docs/design/watchrule-source-namespace/pr5-clusterwatchrule-source-ceiling.md,
attribute introduction of the source Namespace informer to PR4 and its extension
to PR5.
- Around line 53-65: Resolve the contradictory self-namespace contract by
choosing one behavior and applying it consistently: in
docs/design/watchrule-source-namespace/README.md lines 53-65, retain or revise
the no-self-exception rule; in
docs/design/watchrule-source-namespace/pr4-source-namespace-field.md lines
144-150, align the authorization gate and denial message; and in
docs/design/watchrule-source-namespace/pr5-clusterwatchrule-source-ceiling.md
lines 37-52, align the invariant and compatibility table. Update related tests
to enforce the selected contract, including whether adding a policy may deny an
existing WatchRule.
In `@docs/finished/multi-cluster-author-attribution.md`:
- Line 37: Update the fenced ASCII relationship diagram in
multi-cluster-author-attribution.md to specify a language identifier, preferably
text, on the opening fence so it satisfies markdownlint MD040.
---
Nitpick comments:
In `@docs/INDEX.md`:
- Around line 83-84: Update the e2e-git-server-choice.md entry in the docs index
to replace “also why” with “and why,” preserving the rest of the description
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3e211184-f8ed-4cc8-9bef-0ee094f21411
📒 Files selected for processing (25)
api/v1alpha3/clusterprovider_types.godocs/INDEX.mddocs/design/watchrule-source-namespace/README.mddocs/design/watchrule-source-namespace/pr1-namespace-scoped-resync.mddocs/design/watchrule-source-namespace/pr2-stream-scope-collapse.mddocs/design/watchrule-source-namespace/pr3-clusterwatchrule-target-admission.mddocs/design/watchrule-source-namespace/pr4-source-namespace-field.mddocs/design/watchrule-source-namespace/pr5-clusterwatchrule-source-ceiling.mddocs/finished/multi-cluster-author-attribution.mddocs/spec/README.mddocs/spec/where-validation-lives.mdinternal/git/commit.gointernal/git/commit_request_attach_test.gointernal/git/commit_test.gointernal/git/resync_flush.gointernal/git/resync_flush_test.gointernal/git/resync_heal_test.gointernal/git/resync_push_test.gointernal/git/resync_scope_test.gointernal/git/types.gointernal/watch/event_router.gointernal/watch/event_router_test.gointernal/watch/resync_scope_test.gointernal/watch/target_watch.gotest/e2e/setup/kcp/base/kcp-operator.yaml
Concurrent design-doc pass over the WatchRule source-namespace workstream, committed so it is not carried alongside the PR 2 code change: - README: PR 1 marked landed, PR 2 marked landed, and the compatibility table's PR-number column corrected (it was off by one against the phase split). - PR 1 page: rewritten past-tense as the record of what shipped, with a "what the tree looks like now" section naming ResyncScope, resyncScopeForWatchKey, and the scope.Matches predicate. - PR 2 page: status block describing the landed shape (WatchScopes, one stream per scope at both read sites), and a sharper PR 5 interaction — the ceiling drops the "" key only for namespaced selections, so cluster-scoped rules still emit it. - PR 4 / PR 5 pages: consistency with the above. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ce pages Follow-up to 9dc9df8, covering the remaining PR #254 review findings. - PR 5: drop the stale "own namespace, plus what the policy admits" row. The overview and PR 4 both settled on no implicit self-namespace exception; PR 5 was the only page still carrying the superseded carve-out, which made the authorization boundary read two ways. - PR 5: attribute the source Namespace informer and the SourceNamespaceAuthorized condition to PR 4, not PR 3. PR 3 is the ClusterWatchRule target-admission fix and introduces neither. - PR 4 / PR 5: settle the unevaluatable-policy contract that the two pages stated in opposite terms. They are one contract with two instances, split on whether a scope is being *established* or *maintained* — failing closed while establishing means never starting a stream, while failing closed on a maintained scope means narrowing to nothing, and a narrowed set is the input to a sweep. Hence a permanent Forbidden is Stalled in the first case and a retained Unknown in the second. Recorded as a table in PR 4, referenced from PR 5, and pinned by a test pair so an implementation cannot satisfy one page while contradicting the other. - README: header said PR 1 landed while the phase table said 1 and 2. - multi-cluster-author-attribution: language on the ASCII fence (MD040). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
Review response — all six findings triaged, five acceptedThanks — the cross-file consistency checks caught three real contradictions that a single-page read would not have. Changes are in 9dc9df8 and 4f37759, docs-only. Accepted1. PR 1 page described the defect in the present tense (major). Correct and my mistake: PR 1's code is in this PR, so One item is deliberately carried forward rather than ticked: retention-on-revocation is documented but untested, because nothing in this PR can revoke a namespace — no code path removes one from a watch set yet. It is recorded on the page as owned by PR 5's revocation tests rather than silently dropped. 2. The 3. The self-namespace contract was stated two ways (major). Confirmed. The overview and PR 4 both carry the settled no-self-exception rule; PR 5's invariant and compatibility table still carried the superseded "own namespace, plus what the policy admits". PR 5 was the stale page — it now matches, with a pointer to the overview section that argues the decision so the carve-out does not creep back. 4. Phase references were off (minor). The whole PR column of the compatibility table was still on the old four-page numbering. Corrected: source fields/delegation/denial → PR 4; stream-scope fix → PR 2; target admission → PR 3; ceiling → PR 5; condition and printer column → 4 and 5, since PR 5 adds them to ClusterWatchRule. Also fixed three PR-3 attributions inside PR 5 — the source Namespace informer and 5. MD040 on the ASCII diagram. Fenced as Accepted as a defect, but resolved differently than suggestedUnknown selector-policy handling (major). The two pages do contradict each other and that needed fixing — but not by picking one behavior, because the two situations are genuinely different and collapsing them would introduce a bug. The suggestion was to define one terminal-vs-unknown contract and align both pages to it. The problem is that "fail closed" means opposite operational things in the two places:
So the fix is one contract split on the right axis: establishing versus maintaining a scope, not retryable versus terminal. There is now a table in PR 4 giving the condition, reason, To keep the halves from drifting apart again, the contract is pinned by a test pair rather than a single test: A ClusterWatchRule that has never resolved its ceiling falls in the establishing column, which is the case the original PR 5 text did not distinguish. Declined
Pre-merge checksTwo warnings, both acknowledged and neither actioned:
Note for anyone reading the diff: PRs 1 and 2 are now marked landed in the phase table. PR 2's code is on a separate branch and not part of this PR — the doc pages were updated in the same pass to avoid leaving the folder describing a state that is about to be stale. |
sunib
added a commit
that referenced
this pull request
Jul 20, 2026
…e streams (#255) * docs(readme): correct quick start, document Kustomize support, add road to 1.0 The quick start pinned cert-manager v1.20.3, a version that appeared nowhere else in the repo and was never exercised. It also omitted the rollout wait (the controller cannot start until cert-manager issues the admission certificate) and told users to check `Ready` on the GitTarget, which is held at Unknown until first source discovery — the e2e asserts `Validated` for exactly this reason. Also: - Document the Kustomize support that already ships. The write path runs kustomize itself as a pre-commit oracle; the README previously claimed it "cannot infer the original structure of arbitrary templates or overlays", which the code contradicts. - Stop over-hedging delivery. While the watch is connected every change is seen and committed; collapsing to current state only happens across a gap. - Reframe HA from a standalone caveat into a road-to-1.0 list alongside the configuration surface (six CRDs still v1alpha3, no conversion path) and docs. - Move the attribution mode tables into the attribution setup guide, which already opened with the same concept. - Warn that `helm uninstall` removes the cluster-scoped default ClusterProvider, and that the default RBAC grants cluster-wide Secret reads. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(deps): refresh the version pins Dependabot cannot see, add Renovate Dependabot covers gomod, github-actions, and Dockerfile FROM lines, and it is healthy — every direct Go dependency is current. The pins it cannot see had drifted, including a cert-manager version in the README that existed nowhere else in the repo. Cluster charts (validated by the e2e suite): - cert-manager 1.19.4 -> 1.21.0. Its three breaking changes (tokenrequest RBAC, cert-manager-edit ACME permissions, metrics Helm values) do not apply: we set no cert-manager values beyond crds.enabled, use a self-signed Issuer, and have no serviceAccountRef. - valkey 0.9.3 -> 0.10.0, kcp-operator 0.7.7 -> 0.8.3. - gitea 12.5.1 -> 12.5.3, deliberately NOT the latest 12.7.0. Gitea 1.26 removed the _csrf form input and cookie from /user/login, which the giteaclient WebSession scrapes to verify SSH signing keys, so 12.6.0+ fails the signing e2e. 12.5.3 carries the same Gitea 1.25.5, so this is a chart-only patch. Devcontainer toolchain (needs an image rebuild to take effect): flux2 2.9.2, flux-operator 0.55.0, helm 4.2.3, task 3.52.0, cosign 3.1.2. cosign is pinned twice and both places move together. Renovate is scoped to only what Dependabot cannot see, so the two never open competing PRs: Flux HelmRelease charts, the devcontainer ENV pins, the cert-manager URL in the README, and the cosign-installer input. It holds gitea below 12.6.0 until the web session helper is reworked. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(design): decide the e2e Git server — stay on Gitea, adopt no SDK Records the investigation behind unpinning Gitea rather than migrating to Forgejo, read from the upstream sources in external-sources/. The premise for migrating was that we were stuck on chart 12.5.x because Gitea 1.26 removed the `_csrf` form input from /user/login, which our WebSession scrapes. That premise does not survive contact with the source: Gitea and Forgejo made the *same* change, both replacing form-token CSRF with Go's stdlib http.CrossOriginProtection, which admits any request carrying neither Sec-Fetch-Site nor Origin. The fix is a deletion, and it works on either server — so migration and unpinning are not the same piece of work. That leaves a cost comparison Gitea wins: it keeps the literal "gitea" signing namespace (Forgejo switched to the ROOT_URL hostname, the one hazardous change), keeps the session cookie name, keeps an HTTP chart repo, and has the better trusted-key lever — TRUSTED_SSH_KEYS reports the commit's own committer email where Forgejo's instance key reports a fixed configured address, which is what our signing assertion checks. Also records why no Go SDK is adopted on either server: there is no official Forgejo SDK, no SDK can cover the web-UI verification flow, the Gitea SDK's version gating is meaningless against Forgejo, and it would add five dependencies to test-only code that is currently stdlib-only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(e2e): unpin Gitea to 12.7.0 by deleting the CSRF scraper Gitea 1.26 replaced form-token CSRF with Go's stdlib http.CrossOriginProtection (routers/web/web.go). That check returns nil when a request carries neither Sec-Fetch-Site nor Origin — every non-browser Go client — and POST /user/login is exempt besides, being SignOutRequired. So the token our WebSession scraped is not merely absent, it is unnecessary. Delete fetchCSRF, extractCSRFToken, csrfInputRE and both form.Set("_csrf", ...) calls, and move the chart from 12.5.3 (Gitea 1.25.5) to 12.7.0 (Gitea 1.27.0) — the version whose signing spec failed earlier today. HTML-scraping a hidden input was the most brittle part of the client, so this is a net reduction in fragility as well as ~45 fewer lines. The Renovate guard holding gitea below 12.6.0 goes with it; it existed only to describe this problem. Validated on Gitea 1.27.0: task lint, task test (coverage 76.6%, baseline unchanged), and task test-e2e — 58 passed, 0 failed, including the signing specs that drive the web session. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(spec): record where validation lives, repo-wide Validation climbs a ladder: OpenAPI schema, then CEL XValidation, then the reconciler for anything cross-object. A validating admission webhook is not a rung — it is correct only when the information exists solely at admission time and nowhere on the persisted object. Writing it down once so it stops being re-argued per feature. The reasoning was already settled in practice by the config-plane split, which enforces ClusterProvider.spec.allowedNamespaces on every reconcile rather than at admission: reconcile-time re-evaluates continuously (so it catches a policy tightened after the object was created), the data-plane ordering is what provides the security property, and admission buys only feedback latency in exchange for cert wiring in the chart. Also records why /validate-operator-types is not a counterexample: it captures submitter identity and always allows, so it is not a validation gate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(finished): trim the multi-cluster attribution record to its decisions The record had grown to 474 lines carrying a build order, a test plan, an API dump and open questions that are all either shipped or now owned elsewhere. Cut to the decisions and the reversals, which is the part a finished doc is actually for. Corrects one claim that read as current but never shipped: an earlier draft reserved the name "default" for the in-cluster cluster via CEL. What shipped derives locality from spec.kubeConfig alone, for every name — a provider named "default" may carry a kubeConfig and mirror a remote cluster. Records why the reservation was rejected (it pins a name to a physical cluster, the same silent-retarget hazard the immutability rules exist to prevent). Also drops the "enforced in two places" admission-webhook wording for allowedNamespaces, which shipped as a single reconcile-time gate, and points at the new spec/where-validation-lives.md for the general rule. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(design): authorize WatchRule source namespaces on the provider Revises the WatchRule source-namespace design. The previous revision gated the new field on kubeConfig presence (remote-only, in-cluster forbidden), which is wrong twice over: it derives an addressing capability from a connectivity setting, so setting a kubeconfig to reach a cluster silently grants namespace addressing; and it welds the in-cluster case shut forever, conflating "unsafe by default" with "impossible". Replaced by an explicit deny-by-default allow-list on the platform-owned object, ClusterProvider.spec.allowedSourceNamespaces, mirroring the existing allowedNamespaces exactly. One mechanism instead of two: an empty allow-list already is a gate, so no separate boolean is needed, and the only capability a boolean uniquely adds — "any namespace" — is the one the doc argues must not ship upstream. Names: sourceNamespace on WatchRule (the longer sourceClusterNamespace was justified only by the remote-only rule, which is gone), and the allowed* prefix retained on the provider because the allow-list IS the gate and the name must foreclose a fail-open reading. Records allowedGitTargetNamespaces + allowedSourceNamespaces as the symmetric pair to adopt at the next API version rather than breaking v1alpha3. Keeps the two code findings that make the upstream ask small: the Git path already uses the source object's namespace, so the write side needs no change, and ClusterWatchRule already watches the source cluster across all namespaces but carries two pre-existing defects. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(api): correct the AllowsNamespace enforcement comment The comment claimed AllowsNamespace was "the single authorization predicate shared by the admission webhook and the reconcile-time refusal, so the two can never diverge". There is no such webhook — the proposal was dropped, and checkSourceAuthorization in gittarget_source_cluster.go is the only non-test caller. Replaces it with what actually holds: enforcement is reconcile-time and nowhere else, it returns before DeclareForGitTarget, and that is deliberate rather than incidental because continuous re-evaluation also covers a policy tightened after the GitTarget was created — something admission could not see. Comment-only: no generated CRD drift (it documents a method, not a field), and `task lint` passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs: getting one step further and fixing kcp version mistake * docs: current stand * fix(e2e): pin kcp-operator to chart 0.7.7, not appVersion 0.8.3 The dep refresh read v0.8.3 (the appVersion kcp-operator 0.7.7 ships) as the chart version. No such chart exists — kcp-dev/helm-charts publishes nothing above 0.7.x — so the HelmRelease never left SourceNotReady and the e2e source-cluster leg died in setup with "no 'kcp-operator' chart with version matching '0.8.3' found". The earlier fix on this branch was invisible to the merge: it restored the merge-base value, so the three-way merge saw a change only on main's side and took 0.8.3. Merging main in first makes this pin an explicit change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(design): split the WatchRule source-namespace design into four PR pages The single 783-line design had grown past the point where it could be picked up and implemented. Split it into a README that carries the decision and the sequencing, plus one page per shippable PR: - PR 1: collapse the ClusterWatchRule stream scope - PR 2: admit ClusterWatchRule targets - PR 3: the sourceNamespace field and its GitTarget allow-list gate - PR 4: the ClusterProvider source ceiling that binds ClusterWatchRule too PR 4 is not optional cleanup: a multi-tenant deployment runs a ClusterWatchRule per tenant from day one to capture CRDs, so an allow-list that binds only WatchRule would leave the wider path unguarded. Repoint the INDEX entry and the multi-cluster-attribution cross-reference at the new directory. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs: Folding in more findings * fix(resync): scope the mark-and-sweep by namespace, not only by type A per-namespace replay produced a `desired` set covering one namespace, but the resulting mark-and-sweep was scoped by (group, resource) only. Every managed document of that type in every other namespace was therefore absent from `desired` — and a sweep deletes what is absent. The scope was present at both ends and discarded in the middle: `targetWatchSpecs` already built per-namespace watch keys, `enqueueReplayResync` passed only `key.GVR` onward, and `resyncPlan`'s predicate matched group and resource while `types.ResourceIdentifier` carried the namespace all along. Replace `ScopeGVR *schema.GroupVersionResource` with `Scope *ResyncScope` (GVR + Namespace) across ResyncRequest and PendingWrite, so the two halves of a scope travel together and a call site cannot pass one without the other. An empty Namespace keeps today's all-namespaces meaning, which is what a ClusterWatchRule's cluster-wide stream gathers. `resyncHealKey` now separates namespaces too, so a parked heal for one namespace no longer replaces another's. The defect is latent today: a GitTarget can only watch one namespace per GVR, because WatchRule.targetRef is namespace-local and a ClusterWatchRule's "" key collapses the type to all-namespaces. It goes live with the first change that fans a GitTarget out across namespaces — which is every remaining PR in docs/design/watchrule-source-namespace/. Hence this lands first. Verified: reverting the namespace half of ResyncScope.Matches fails TestResync_NamespaceScopedSweepLeavesSiblingNamespacesAlone and the sibling-namespace row of TestResyncScope_MatchesRespectsTypeAndNamespace. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(design): refresh the source-namespace workstream pages Concurrent design-doc pass over the WatchRule source-namespace workstream, committed so it is not carried alongside the PR 2 code change: - README: PR 1 marked landed, PR 2 marked landed, and the compatibility table's PR-number column corrected (it was off by one against the phase split). - PR 1 page: rewritten past-tense as the record of what shipped, with a "what the tree looks like now" section naming ResyncScope, resyncScopeForWatchKey, and the scope.Matches predicate. - PR 2 page: status block describing the landed shape (WatchScopes, one stream per scope at both read sites), and a sharper PR 5 interaction — the ceiling drops the "" key only for namespaced selections, so cluster-scoped rules still emit it. - PR 4 / PR 5 pages: consistency with the above. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(design): resolve the review contradictions in the source-namespace pages Follow-up to 9dc9df8, covering the remaining PR #254 review findings. - PR 5: drop the stale "own namespace, plus what the policy admits" row. The overview and PR 4 both settled on no implicit self-namespace exception; PR 5 was the only page still carrying the superseded carve-out, which made the authorization boundary read two ways. - PR 5: attribute the source Namespace informer and the SourceNamespaceAuthorized condition to PR 4, not PR 3. PR 3 is the ClusterWatchRule target-admission fix and introduces neither. - PR 4 / PR 5: settle the unevaluatable-policy contract that the two pages stated in opposite terms. They are one contract with two instances, split on whether a scope is being *established* or *maintained* — failing closed while establishing means never starting a stream, while failing closed on a maintained scope means narrowing to nothing, and a narrowed set is the input to a sweep. Hence a permanent Forbidden is Stalled in the first case and a retained Unknown in the second. Recorded as a table in PR 4, referenced from PR 5, and pinned by a test pair so an implementation cannot satisfy one page while contradicting the other. - README: header said PR 1 landed while the phase table said 1 and 2. - multi-cluster-author-attribution: language on the ASCII fence (MD040). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(watch): a cluster-wide selection must not collapse named-namespace streams `SnapshotNamespaces()` returned nil whenever a WatchedType carried the "" namespace key, and nil meant all-namespaces at every read site. A WatchRule scoped to one namespace and a ClusterWatchRule scoped cluster-wide, on the same GVR and the same GitTarget, therefore folded into a single all-namespaces stream: the named scope survived only in the plan hash, and `targetWatchSpecs` substituted `NamespaceOps[""]` for its operation set, so a CREATE-only named rule co-resident with an UPDATE-only cluster-wide rule lost its filter too. Under PR 4 that is a gate bypass, not just over-watching. A WatchRule authorized only for `repo-config` would receive events from every namespace the credential can read, with its allowedSourceNamespaces check having passed before the data plane widened it. Replace `SnapshotNamespaces` with `WatchedType.WatchScopes`, which returns every scope including the cluster-wide "" rather than collapsing to it, and project one stream per scope at BOTH read sites — a fix in one path only would leave the plan hash and the running streams describing different mirrors: - `targetWatchSpecs` keys a watch per scope, each carrying NamespaceOps[ns], so the per-namespace operation sets survive. - `snapshotGVRsFromTable` and `resolveSnapshotGVRForType` both project through one shared `snapshotGVRScopes` helper, so the whole-target and per-type paths cannot drift apart. `snapshotGVR.namespaces []string` became a single `namespace string`, matching targetWatchKey and matching how a dynamic client List actually takes a namespace ("" = all). Distinct streams rather than subtracting the named scope from the cluster-wide one: "all namespaces except team-a" is not expressible as a watch, and the subtraction would need a sweep scope shaped as a complement, which git.ResyncScope cannot represent. Each stream's replay now pairs a desired set with a sweep scoped identically — which is exactly why this had to land after PR 1. A cluster-scoped GVR is unaffected: collectWatchRuleSelections hardcodes ResourceScopeNamespaced, so only a namespaced record can ever acquire a named namespace key, and a cluster-scoped type still yields exactly one "" scope. TestBuildWatchedTypeTable_ClusterWideOverridesNamedNamespaces asserted the collapse as intended ("matching the historic gvrSnapshotEntry collapse") and is deleted, not skipped. Verified: restoring the collapse fails all three replacements — the table-level scope test, the targetWatchSpecs stream/op-set test, and the snapshotGVRsFromTable test — each on its own assertion. 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.
Settles where validation lives, reworks the WatchRule source-namespace design into an implementable five-PR plan, and lands the first of those PRs.
Docs
docs/design/watchrule-source-namespace/— the former single design page, restructured into an overview plus one page per expected PR, each carrying its own verified-in-code evidence, implementation steps, test plan, and done-when.Two decisions changed during the rework:
GitTarget.allowedSourceNamespacesis a property of the destination, not of the requesting rule. When declared it is exhaustive for every rule writing to that target, ClusterWatchRule included — otherwise the allow-list would be enforced only on the rule kind that cannot bypass it. This matters from day one: a ClusterWatchRule is the only way to select cluster-scoped types, so a multi-tenant deployment runs one per tenant immediately.The overview is explicit about what the ceiling cannot do: cluster-scoped objects have no namespace, so a tenant selecting CRDs receives every CRD the credential can read.
Code — PR 1 of 5: namespace-scoped resync
A per-namespace replay produced a
desiredset covering one namespace, but the resulting mark-and-sweep was scoped by(group, resource)only. Every managed document of that type in every other namespace was therefore absent fromdesired, and a sweep deletes what is absent.The scope was present at both ends and discarded in the middle:
targetWatchSpecsalready built per-namespace watch keys,enqueueReplayResyncpassed onlykey.GVRonward, andresyncPlan's predicate matched group and resource whiletypes.ResourceIdentifiercarried the namespace all along.ScopeGVR *schema.GroupVersionResource→Scope *ResyncScope(GVR + Namespace) acrossResyncRequestandPendingWrite, so both halves of a scope travel together.Namespacekeeps today's all-namespaces meaning — what a ClusterWatchRule's cluster-wide stream gathers.resyncHealKeyseparates namespaces, so a parked heal for one no longer replaces another's.resyncScopeForWatchKeyis the single watch-key → scope conversion, so the namespace a stream gathered and the namespace its sweep may touch cannot drift apart.The defect is latent today — a GitTarget can only watch one namespace per GVR, because
WatchRule.targetRefis namespace-local and a ClusterWatchRule's""key collapses the type to all-namespaces. It goes live with the first change that fans a GitTarget out across namespaces, which is every remaining PR in the folder. Hence it lands first and blocks the rest.Verified by reverting the namespace half of
ResyncScope.Matches:TestResync_NamespaceScopedSweepLeavesSiblingNamespacesAloneand the sibling-namespace row ofTestResyncScope_MatchesRespectsTypeAndNamespaceboth fail without the fix.Remaining PRs
sourceNamespacefield and gate · 5. the ClusterWatchRule ceilingPRs 4 and 5 must ship together.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation