feat(api)!: scope by kind — per-item WatchRule source namespaces, cluster-only ClusterWatchRule#259
Merged
Merged
Conversation
… gate Adds `WatchRule.spec.sourceNamespace` so a rule can watch a namespace other than its own on the GitTarget's source cluster — the shared-config-plane multi-tenancy case where a tenant's config namespace and source namespace cannot share a name. PR 4 of docs/design/watchrule-source-namespace/. API (v1alpha3): - `WatchRule.spec.sourceNamespace` (optional; omitted = the rule's own namespace), plus a priority-1 `SourceAuthorized` printer column. - `GitTarget.spec.allowedSourceNamespaces` — a deny-by-default ceiling on which source namespaces may be mirrored into a target, by any rule kind. Declared is exhaustive with no self-namespace exception; empty != omitted. - `ClusterProvider.spec.allowWatchRuleSourceNamespaceOverride` — false-by-default delegation flag; without it a WatchRule may watch only its own namespace. - Generalize `AllowedNamespaces` into a shared `NamespaceMatcher` (JSON field name unchanged); `ClusterProvider.AllowsNamespace` and `GitTarget.AllowsSourceNamespace` become thin wrappers so the control-cluster and source-cluster policies cannot drift. Gate and data plane: - The three-part gate (provider admits the target, provider delegates, target policy admits the namespace) lives in internal/authz with a three-valued result (admitted / denied / cannot-say-yet / permanently-unevaluatable), so a transient source-cluster outage never becomes a terminal Stalled=True. - Route every WatchRule compilation through one gated path (`watch.CompileWatchRule`) used by both the reconciler and the startup bootstrap, so the gate cannot be bypassed on restart. - Source-scope service (manager-owned per-source-cluster Namespace snapshot, refreshed on the existing reconcile cadence) evaluates selector policies and enqueues affected rules on a label change; exact-name policies need no source-cluster access. - Carry the effective source namespace through the compiled rule, the watch selection, the fingerprint, and the stream-readiness lookup, so an override watches — and reports Ready on — the right namespace. - `SourceNamespaceAuthorized` condition (kstatus-compatible; an additional prerequisite of Ready), a ClusterProvider→WatchRules mapper, and a source-cluster Namespace channel. Docs: configuration.md, status-conditions-guide.md, and the design pages. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (32)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (17)
📝 WalkthroughWalkthroughThis change introduces shared namespace-matching policies, per-rule WatchRule source namespaces, ClusterWatchRule cluster-only enforcement, authorization-aware compilation, source-cluster selector resolution, status conditions, watch planning updates, migration documentation, and end-to-end coverage. ChangesSource-namespace API and authorization
Runtime enforcement and watch planning
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant WatchRule
participant WatchRuleReconciler
participant Authz
participant ClusterProvider
participant GitTarget
participant SourceNamespaceScope
participant RuleStore
WatchRule->>WatchRuleReconciler: reconcile source-namespace policy
WatchRuleReconciler->>Authz: resolve per-rule source scope
Authz->>ClusterProvider: check admission and delegation
Authz->>GitTarget: evaluate allowed source namespaces
Authz->>SourceNamespaceScope: resolve selectors and wildcard scope
SourceNamespaceScope-->>Authz: return scope verdict
Authz-->>WatchRuleReconciler: return authorization decision
WatchRuleReconciler->>RuleStore: compile, retain, or remove rule
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)config/crd/bases/configbutler.ai_clusterproviders.yamlTraceback (most recent call last): config/crd/bases/configbutler.ai_clusterwatchrules.yamlTraceback (most recent call last): config/crd/bases/configbutler.ai_gittargets.yamlTraceback (most recent call last):
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/watch/manager.go (1)
307-326: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid blocking reconcile on one source cluster
refreshSourceNamespaceScopeswalks wanted clusters sequentially on the same context, and the namespaceListcall here has no per-cluster timeout. One slow or unreachable cluster can stallReconcileForRuleChangebefore watched-type tables and target watches refresh.internal/watch/manager.go:318,internal/watch/source_namespace_scope.go:265-280🤖 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 `@internal/watch/manager.go` around lines 307 - 326, Update refreshSourceNamespaceScopes and its per-cluster namespace List flow to apply an independent timeout for each source cluster, preventing one slow or unreachable cluster from blocking ReconcileForRuleChange. Ensure the timeout is scoped and canceled per cluster, while allowing the method to finish so refreshWatchedTypeTables and refreshRunningTargetWatches still execute.
🧹 Nitpick comments (3)
test/e2e/source_namespace_e2e_test.go (1)
54-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert on namespace creation errors.
Since the rest of the test relies on these namespaces, it's safer to explicitly assert that they are created successfully. This ensures any setup failure halts the test immediately and provides a clear failure message, matching the error-handling pattern used elsewhere in the suite.
♻️ Proposed refactor
- _, _ = kubectlRun("create", "namespace", configNS) - _, _ = kubectlRun("create", "namespace", sourceNS) + Expect(kubectlRun("create", "namespace", configNS)).Error().NotTo(HaveOccurred()) + Expect(kubectlRun("create", "namespace", sourceNS)).Error().NotTo(HaveOccurred())🤖 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 `@test/e2e/source_namespace_e2e_test.go` around lines 54 - 55, Update the namespace setup in the test to capture and assert the errors returned by both kubectlRun calls for configNS and sourceNS, using the suite’s existing assertion/error-handling pattern so setup failures stop the test with a clear message.internal/authz/source_namespace_test.go (1)
403-417: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the test to match the type being tested.
The method
Terminal()is defined onSourceNamespaceDecision. Consider removing theWatchRuleprefix from the test name to better align with the standard GoTestType_Scenariotesting convention.♻️ Proposed refactor
-func TestWatchRuleSourceNamespaceDecision_TerminalClassification(t *testing.T) { +func TestSourceNamespaceDecision_TerminalClassification(t *testing.T) {🤖 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 `@internal/authz/source_namespace_test.go` around lines 403 - 417, Rename TestWatchRuleSourceNamespaceDecision_TerminalClassification to use the SourceNamespaceDecision type name, such as TestSourceNamespaceDecision_TerminalClassification, while leaving the test logic unchanged.api/v1alpha3/namespace_matcher_test.go (1)
82-126: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider moving type-specific tests to their respective test files.
While the tests themselves are thorough and correct, testing
WatchRule,GitTarget, andClusterProviderbehavior insidenamespace_matcher_test.godeviates slightly from the typical Go convention of grouping tests alongside their target files.Consider moving these functions to
watchrule_types_test.go,gittarget_types_test.go, andclusterprovider_types_test.gorespectively to improve discoverability.🤖 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 `@api/v1alpha3/namespace_matcher_test.go` around lines 82 - 126, Move TestWatchRule_EffectiveSourceNamespace to watchrule_types_test.go, TestGitTarget_SourceNamespacePolicy to gittarget_types_test.go, and TestClusterProvider_DelegationFlagDefaultsClosed to clusterprovider_types_test.go; keep each test’s implementation and assertions unchanged.
🤖 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.
Outside diff comments:
In `@internal/watch/manager.go`:
- Around line 307-326: Update refreshSourceNamespaceScopes and its per-cluster
namespace List flow to apply an independent timeout for each source cluster,
preventing one slow or unreachable cluster from blocking ReconcileForRuleChange.
Ensure the timeout is scoped and canceled per cluster, while allowing the method
to finish so refreshWatchedTypeTables and refreshRunningTargetWatches still
execute.
---
Nitpick comments:
In `@api/v1alpha3/namespace_matcher_test.go`:
- Around line 82-126: Move TestWatchRule_EffectiveSourceNamespace to
watchrule_types_test.go, TestGitTarget_SourceNamespacePolicy to
gittarget_types_test.go, and TestClusterProvider_DelegationFlagDefaultsClosed to
clusterprovider_types_test.go; keep each test’s implementation and assertions
unchanged.
In `@internal/authz/source_namespace_test.go`:
- Around line 403-417: Rename
TestWatchRuleSourceNamespaceDecision_TerminalClassification to use the
SourceNamespaceDecision type name, such as
TestSourceNamespaceDecision_TerminalClassification, while leaving the test logic
unchanged.
In `@test/e2e/source_namespace_e2e_test.go`:
- Around line 54-55: Update the namespace setup in the test to capture and
assert the errors returned by both kubectlRun calls for configNS and sourceNS,
using the suite’s existing assertion/error-handling pattern so setup failures
stop the test with a clear message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ad9cabe2-7f73-4b45-b52a-0322d3e52ed2
📒 Files selected for processing (45)
.coverage-baselineapi/v1alpha3/clusterprovider_types.goapi/v1alpha3/gittarget_types.goapi/v1alpha3/helpers_test.goapi/v1alpha3/namespace_matcher.goapi/v1alpha3/namespace_matcher_test.goapi/v1alpha3/watchrule_types.goapi/v1alpha3/zz_generated.deepcopy.goconfig/crd/bases/configbutler.ai_clusterproviders.yamlconfig/crd/bases/configbutler.ai_gittargets.yamlconfig/crd/bases/configbutler.ai_watchrules.yamldocs/configuration.mddocs/design/watchrule-source-namespace/README.mddocs/design/watchrule-source-namespace/alt-clusterwatchrule-cluster-scope-only.mddocs/design/watchrule-source-namespace/pr4-source-namespace-field.mddocs/spec/status-conditions-guide.mdinternal/authz/clusterprovider_admission_test.gointernal/authz/source_namespace.gointernal/authz/source_namespace_test.gointernal/controller/clusterprovider_controller_test.gointernal/controller/clusterwatchrule_admission_test.gointernal/controller/clusterwatchrule_controller.gointernal/controller/constants.gointernal/controller/gittarget_source_cluster_test.gointernal/controller/stream_status.gointernal/controller/stream_status_test.gointernal/controller/suite_test.gointernal/controller/watchrule_controller.gointernal/controller/watchrule_controller_test.gointernal/controller/watchrule_kstatus_test.gointernal/controller/watchrule_source_namespace.gointernal/controller/watchrule_source_namespace_test.gointernal/rulestore/store.gointernal/watch/bootstrap.gointernal/watch/bootstrap_admission_test.gointernal/watch/manager.gointernal/watch/manager_startup_test.gointernal/watch/source_namespace_planning_test.gointernal/watch/source_namespace_scope.gointernal/watch/source_namespace_stream_summary_test.gointernal/watch/source_namespace_test.gointernal/watch/stream_readiness.gointernal/watch/watched_type_resolver.gointernal/watch/watchrule_compile.gotest/e2e/source_namespace_e2e_test.go
💤 Files with no reviewable changes (2)
- internal/controller/stream_status_test.go
- internal/controller/clusterwatchrule_controller.go
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Replace the rejected alternatives and the superseded PR 6 sketch with a single PR 4 design: ClusterWatchRule becomes cluster-scope-only and WatchRule gains a per-item rules[].sourceNamespace, with the selector-backed "*" wildcard bounded by GitTarget.spec.allowedSourceNamespaces. Rename the ClusterProvider delegation boolean to allowSourceNamespaceOverride. Once ClusterWatchRule has no source-namespace choice, the WatchRule prefix disambiguates nothing, and the field is unreleased so the rename needs no shim. Record the prior art: Flux's AccessFrom ACL independently lands on namespace labels, ORed selectors, deny-when-absent, empty-selector-matches-everything, and a typed AccessDeniedError distinct from "cannot evaluate". It never enumerates a namespace set from a selector, and upstream has been shrinking the field rather than growing it, which is why the wildcard's invalidation path is the half of this design with no precedent to lean on. Keep the shipped-but-unreleased top-level implementation as a baseline record so the reusable authorization, status, and reactivity work is not rebuilt. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/historical-top-level-source-namespace-baseline.md`:
- Around line 281-284: Correct the historical PR 4 note to state that
selector-backed wildcards are supported and resolved using the source-namespace
snapshot for live wildcard sets. Update the affected “Where it applies” text and
any related behavior descriptions, or clearly mark the note as superseded so it
cannot imply this capability is deferred or omitted.
In `@docs/design/watchrule-source-namespace/README.md`:
- Around line 126-128: Clarify the deny-by-default migration guidance in both
docs/design/watchrule-source-namespace/README.md lines 126-128 and
docs/design/watchrule-source-namespace/pr4-cluster-scope-only.md lines 552-554:
qualify the release-note, UPGRADING.md, and migration-checklist warnings so they
apply to converted wildcard or override WatchRule requests, while noting omitted
or own-namespace items retain legacy admission.
🪄 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: 99ddf7c4-869d-4d5c-9e54-29591d4d03c3
📒 Files selected for processing (11)
docs/INDEX.mddocs/TODO.mddocs/design/watchrule-source-namespace/README.mddocs/design/watchrule-source-namespace/historical-top-level-source-namespace-baseline.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-cluster-scope-only.mddocs/design/watchrule-source-namespace/pr5-clusterwatchrule-source-ceiling.mddocs/design/watchrule-source-namespace/pr5-gittarget-deletion-safety.mdtest/e2e/source_namespace_e2e_test.go
💤 Files with no reviewable changes (1)
- docs/design/watchrule-source-namespace/pr5-clusterwatchrule-source-ceiling.md
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/source_namespace_e2e_test.go
…erWatchRule scope Scope is now carried by the rule KIND. `WatchRule` selects namespaced resources and each `spec.rules[]` item names the source namespace it watches; `ClusterWatchRule` selects cluster-scoped resources and has no scope or namespace choice. PR 4 of docs/design/watchrule-source-namespace/. - `ResourceRule.sourceNamespace`: omitted (the rule's own namespace), an exact DNS-1123 name, or `"*"` for every namespace the GitTarget admits. Validated structurally so a malformed namespace is rejected at admission rather than resolving to nothing at compile time. - `EffectiveSourceNamespace()` / `OverridesSourceNamespace()` move down onto the item; `"*"` always counts as an override, so it needs the delegation flag even against a policy listing only the rule's own namespace. - `NamespaceMatcher.SelectorAdmits` exposes the selector half alone, for the enumeration a wildcard needs (a nil selector admits nothing; an empty one admits everything). - `ClusterProvider.spec.allowWatchRuleSourceNamespaceOverride` renamed to `allowSourceNamespaceOverride`. Unreleased, so a plain rename with no shim. Both superseded fields are RETAINED for one release as loud rejections, not deleted. Deleting is the silent option: CRD pruning happens on write, so a re-applied legacy manifest would be accepted with the value dropped and the rule would quietly change what it mirrors. - `WatchRule.spec.sourceNamespace` — rejected by an XValidation rule naming its replacement; `DeclaresRemovedSourceNamespace()` keeps a stored value readable. - `ClusterResourceRule.scope` — optional, defaults to `Cluster`, enum narrowed to `Cluster`; `DeclaresNamespacedScope()` keys the compile-time refusal on the STORED value, never on what the selector happens to resolve. BREAKING CHANGE: `ClusterWatchRule` is cluster-scope-only and `WatchRule.spec.sourceNamespace` moved to `spec.rules[].sourceNamespace`. A namespaced `ClusterWatchRule` becomes a `WatchRule` in the tenant namespace; its namespaced items become `sourceNamespace: "*"` (or explicit names). A target that declares no `allowedSourceNamespaces` admits NOTHING to a `"*"` item, so converting without also declaring that policy narrows production data. See docs/UPGRADING.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`WatchRuleSourceNamespaceAdmitted` decided one namespace for a whole rule. Scope now lives on `spec.rules[]`, so the gate answers the whole rule at once: `ResolveWatchRuleSourceScope` returns a `ResolvedSourceScope` carrying one `SourceNamespaceDecision` per item (its index, what it requested, and the namespace set it resolved to). - The three-part ordering — GitTarget policy, then ClusterProvider delegation, then the rule's own namespace — is unchanged, factored into an `itemGate` that memoises the ClusterProvider verdict. The verdict is identical for every item, so it is fetched once per rule rather than once per item. - A `"*"` item enumerates: the policy's literal `names` are answered locally, the selector half is enumerated against the SOURCE cluster's namespaces, and the two are unioned. A selector that cannot be evaluated makes the whole item Unknown/Unavailable — never a partial set, because a partial set silently narrows what a rule mirrors. - `NoAdmittedSourceNamespaces` is a distinct True reason so a wildcard that resolves to nothing reports that it watches nothing instead of looking healthy. - `SourceNamespaceFieldRemoved` refuses a stored `spec.sourceNamespace` terminally. - `aggregateSourceScope` collapses the items with a stated precedence: denied beats unavailable beats unknown beats allowed. One denied item refuses the whole rule, and the message names which item. `Fingerprint()` renders the resolved sets so callers can key cached work on them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…planning A compiled rule used to hold one source namespace for the whole object. It now holds one resolved namespace SET per item, and every stage from compile to the watched-type table is keyed on that set. - `CompiledRule.SourceNamespace` (string) is replaced by `CompiledResourceRule.SourceNamespaces []string`. `AddOrUpdateWatchRule` takes an index-aligned `sourceNamespaces [][]string` and replaces the whole rule atomically, so a rule is never half-recompiled. `GetWatchRule` is added for callers that need the compiled form back. - `GetMatchingRules` filters the event's namespace per ITEM instead of against the rule object's own namespace, so one rule can legitimately match events from several namespaces. - Wildcard expansion happens at the SELECTION site, not as a read-side filter: `collectWatchRuleSelections` emits one selection per (record x resolved namespace). A read-site filter would have to re-resolve the policy on every event. - `watchRuleFingerprint` hashes each item's RESOLVED namespace set. This is the hazard the design doc calls out: a `"*"` item's inputs are the GitTarget policy and the source cluster's namespaces, neither of which is rule state, so byte-identical rule objects must still re-project the table when the policy moves. Hashing the resolved set is what makes that happen. - Retention is keyed by `SourceScopeSpecHash` (the rule's namespace plus every item's REQUESTED value). An edit or a reorder discards the grant, so a rule cannot inherit a scope it no longer asks for, while an unevaluatable policy keeps the last resolved set instead of sweeping the tree. - `Manager.EnumerateSourceNamespaces` implements the resolver's enumeration half against the source cluster; `unusableSnapshot` is shared by both resolver entry points. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A ClusterWatchRule now selects cluster-scoped types and nothing else. Three enforcement points, deliberately redundant because each one covers a path the others cannot: - Admission — the narrowed `scope` enum rejects `Namespaced` on write. - Compile — `CompileClusterWatchRule` is the single shared path for BOTH the reconciler and bootstrap. It refuses in a fixed order: first the ClusterProvider GitTarget-namespace gate, then `DeclaresNamespacedScope()`. The refusal keys on the STORED value, so a rule admitted by an older CRD is still refused rather than silently re-interpreted. - Resolution — `collectClusterWatchRuleSelections` and `ResolveClusterWatchRuleResources` always match `ResourceScopeCluster`, so a selector like `resources: ["*"]` cannot pull a namespaced type back in. Bootstrap and the reconciler sharing one path is the point: on restart, rules are seeded before any status exists, and a bootstrap that did not refuse would open a namespaced watch the reconciler would only close later. `ClusterWatchRuleReconciler.gateGitTargetAdmission`/`refuseUnauthorizedGitTarget` collapse into `gateClusterWatchRule`/`refuseClusterWatchRule`, and the reconciler no longer calls `AddOrUpdateClusterWatchRule` itself. Test fixtures move to genuinely cluster-scoped types (namespaces, nodes, CRDs, storageclasses) because a ClusterWatchRule now resolves nothing else. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`SourceNamespaceAuthorized` stays one condition per object, now summarising every `spec.rules[]` item. `gateSourceNamespace` publishes the aggregate produced by `ResolveWatchRuleSourceScope`, so the precedence a reader sees is stated once and applied everywhere: a denied item refuses the whole rule and the message names which item lost, an unevaluatable policy reports Unknown rather than quietly watching less, and a wildcard that admits nothing says so. `StreamSummaryForWatchRule` now reads the COMPILED rule instead of the spec. A wildcard's expected stream count is not derivable from the spec — it depends on how many namespaces the item resolved to — so reading the spec left a wildcard rule permanently short of its expected streams and therefore permanently not-Ready. An uncompiled rule correspondingly expects zero streams. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Documents the model and covers the migration end to end in tests. Docs: `configuration.md`, `architecture.md`, `security-model.md` (a new "Which namespaces a rule may read from"), `rbac.md` (a new section on `namespaces` RBAC against a REMOTE source cluster), `status-conditions-guide.md` (the aggregation order and `NoAdmittedSourceNamespaces`), and a new `UPGRADING.md` entry: the two removed capabilities, the conversion, a `jq` one-liner to find affected objects, and the warning that converting without declaring `allowedSourceNamespaces` narrows what a rule mirrors. Tests: `superseded_fields_admission_test.go` asserts against the GENERATED CRDs that `scope: Namespaced` and `spec.sourceNamespace` are rejected, that `rules[].sourceNamespace` including `"*"` is accepted, that a malformed namespace is rejected, and that an omitted `scope` defaults. The e2e source- namespace spec moves to `rules[].sourceNamespace` and gains a wildcard case asserting against real commits that an admitted namespace arrives and an unadmitted one never does. The restart fixture becomes a WatchRule and keeps covering the `apiVersions: ["*"]` startup-snapshot regression. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
sunib
changed the title
The spec passed vacuously in one half and could never pass in the other. - It probed for `<secret>.yaml`, but Secrets are committed SOPS-encrypted as `<name>.sops.yaml` and `findFileByBasename` matches basenames exactly. The positive assertion could therefore never succeed — and the `Consistently` proving an UNADMITTED namespace never arrives was searching for a filename that could not exist either way, so it could never fail. That half is the load-bearing one, which made the vacuous pass the more dangerous defect. - The positive assertion created its object in the namespace the PRECEDING spec's rule already watches by exact name, so it would have passed even if `"*"` expanded to nothing at all. The granted GitTarget now admits a second namespace that no rule item ever names, so only a wildcard can reach it, and the probe objects are ConfigMaps so the assertion does not also ride on the encryption filename convention (the dedicated Secret-encryption spec already covers that). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/rulestore/store.go (1)
675-678: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider deep-copying the other slice fields for full immutability.
While
SourceNamespacesis now explicitly deep-copied to prevent aliasing bugs, the other slice fields inCompiledResourceRule(Operations,APIGroups,APIVersions,Resources) are still shallow-copied via thecopy()function. If consumers were to mutate these slices in the snapshotted rule returned by the store, it would inadvertently modify shared state.Consider deep-copying these slices as well to guarantee full struct immutability, or document that the returned struct's slices must never be mutated.
🤖 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 `@internal/rulestore/store.go` around lines 675 - 678, Update the snapshot-copy logic around CompiledResourceRule and the copy() function to deep-copy Operations, APIGroups, APIVersions, and Resources in addition to SourceNamespaces. Ensure every slice field in the returned snapshotted rule has independent backing storage so consumer mutations cannot affect shared state.
🤖 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.
Nitpick comments:
In `@internal/rulestore/store.go`:
- Around line 675-678: Update the snapshot-copy logic around
CompiledResourceRule and the copy() function to deep-copy Operations, APIGroups,
APIVersions, and Resources in addition to SourceNamespaces. Ensure every slice
field in the returned snapshotted rule has independent backing storage so
consumer mutations cannot affect shared state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8cca7968-55ca-49f5-bfdb-6584d3ea3bfa
📒 Files selected for processing (52)
api/v1alpha3/clusterprovider_types.goapi/v1alpha3/clusterwatchrule_types.goapi/v1alpha3/gittarget_types.goapi/v1alpha3/namespace_matcher.goapi/v1alpha3/namespace_matcher_test.goapi/v1alpha3/watchrule_types.goconfig/crd/bases/configbutler.ai_clusterproviders.yamlconfig/crd/bases/configbutler.ai_clusterwatchrules.yamlconfig/crd/bases/configbutler.ai_gittargets.yamlconfig/crd/bases/configbutler.ai_watchrules.yamlconfig/samples/clusterwatchrule.yamldocs/UPGRADING.mddocs/architecture.mddocs/configuration.mddocs/design/watchrule-source-namespace/pr4-cluster-scope-only.mddocs/rbac.mddocs/security-model.mddocs/spec/status-conditions-guide.mdinternal/authz/source_namespace.gointernal/authz/source_namespace_test.gointernal/controller/clusterwatchrule_admission_test.gointernal/controller/clusterwatchrule_controller.gointernal/controller/superseded_fields_admission_test.gointernal/controller/watchrule_source_namespace.gointernal/controller/watchrule_source_namespace_test.gointernal/rulestore/store.gointernal/rulestore/store_test.gointernal/watch/api_resource_catalog_test.gointernal/watch/bootstrap.gointernal/watch/bootstrap_admission_test.gointernal/watch/config_plane_split_review_fixes_test.gointernal/watch/helpers_test.gointernal/watch/manager_catalog.gointernal/watch/manager_snapshot_test.gointernal/watch/manager_startup_test.gointernal/watch/source_namespace_planning_test.gointernal/watch/source_namespace_scope.gointernal/watch/source_namespace_stream_summary_test.gointernal/watch/source_namespace_test.gointernal/watch/stream_readiness.gointernal/watch/watched_type_helpers_test.gointernal/watch/watched_type_metrics_test.gointernal/watch/watched_type_resolver.gointernal/watch/watched_type_resolver_test.gointernal/watch/watchrule_compile.gotest/e2e/restart_reconcile_e2e_test.gotest/e2e/source_namespace_e2e_test.gotest/e2e/templates/demo/clusterwatchrule-demo.tmpltest/e2e/templates/manager/clusterwatchrule-crd.tmpltest/e2e/templates/restart/clusterwatchrule-wildcard.tmpltest/e2e/templates/restart/watchrule-wildcard.tmpltest/e2e/unsupported_folder_e2e_test.go
💤 Files with no reviewable changes (1)
- test/e2e/templates/restart/clusterwatchrule-wildcard.tmpl
🚧 Files skipped from review as they are similar to previous changes (7)
- config/crd/bases/configbutler.ai_gittargets.yaml
- docs/spec/status-conditions-guide.md
- api/v1alpha3/gittarget_types.go
- internal/controller/watchrule_source_namespace.go
- internal/watch/source_namespace_test.go
- docs/design/watchrule-source-namespace/pr4-cluster-scope-only.md
- internal/controller/clusterwatchrule_admission_test.go
Sibling inference could file one namespace's objects under another namespace's
folder, and then collapse an entire resource type into a single file.
Step 2 of inference ("same type, any namespace") is guarded by the P4 rule: a
cohort may only be reused for a namespace the target has never written if it has
proven itself namespace-agnostic. The bundle branch enforced that properly, via
`spansMultipleNamespaces`. The singleton branch only asked `allSameDir`, which is
"trivially true for zero or one element" — so ONE directory holding ONE
namespace passed the guard, even though it is exactly as consistent with a
per-namespace-segmented layout whose second namespace had simply never been
written. Absence of contrary evidence was being read as proof.
The consequence was worse than one misplaced file, because it cascades. Objects
that exist under the same name in every namespace (kube-root-ca.crt) make the
inferred path collide EXACTLY with the first namespace's file, so the second
namespace's object is appended as another document. That file now genuinely
spans two namespaces, so every later object of the type legitimately prefers the
bundle — and the whole type collapses into one file holding several namespaces'
objects. Observed end to end: four ConfigMaps from two namespaces in a single
file, which was the only ConfigMap file in the tree.
Both branches now require the same positive proof. Declining costs nothing: the
canonical path builds the correct namespace segment directly, which is the
layout the target already uses for the namespace it has written.
This is pre-existing and not introduced by the source-namespace work — the write
path is untouched by it, and placement.go last changed in #239. It is reachable
today wherever one GitTarget receives one type from two namespaces. It matters
now because `sourceNamespace: "*"` reaches that shape from a single rule.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The wildcard spec proved that `"*"` resolves to the admitted set, but every object it created lived in a single namespace, so it could not have caught a placement defect that only appears once a target holds a SECOND one. This adds that case with two ordinary rules and explicit sourceNamespace values — the same shape a wildcard expands to, reached without the wildcard. Keeping it wildcard-free is the point: it separates a defect in wildcard EXPANSION from one in how a target handles more than one source namespace at all, which is what told us the collapse fixed in the previous commit was not caused by this feature. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A GitTarget's allowedSourceNamespaces is a statement about ITS source cluster, but both resolver entry points looked the cluster up through clusterIDForGitTarget, which defaults an undeclared GitTarget to the config plane. That default is right for the read paths it was written for — a status read racing the first Declare — and wrong for authorization: the WatchRule reconciler gates as soon as it has resolved the GitTarget, while Declare is the GitTarget controller's job, and after a restart the two run concurrently. In that window a REMOTE target's selector was evaluated against CONFIG-PLANE Namespace labels, so a namespace could be admitted because a same-named namespace here happened to carry the right label. Key on target.SourceCluster() instead. The two can never disagree: the controller passes exactly that value to DeclareForGitTarget. This also fixes a latent enqueue miss, since enqueueSourceNamespaceChange matches the armed cluster id against the same map — an undeclared target armed "" and so could never match itself. Seven existing tests had to move their snapshot onto the target's own cluster. They passed only because the resolver read the wrong one, which is the clearest evidence the defect was real; the new test uses DIVERGENT labels, because with both clusters labelled alike it would pass against the bug. Also bound the refresh loop that maintains those snapshots. It walked clusters serially, and the Namespace list runs on a config that deliberately carries no rest.Config.Timeout (its watches must stay open) with only its dial bounded, so a cluster that accepts the connection and then hangs blocked ReconcileForRuleChange forever — and with it the watched-type tables and target watches for every tenant. Each cluster now lists under its own deadline, with the same bounded concurrency refreshRemoteCatalogsConcurrently already applies to the catalog for exactly this reason. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The delete path removed the rule from the store but never called ForgetSourceScopeGrant, whose own docstring says it runs "on a REFUSAL or a deletion". Only the refusal half was wired. The grant is what distinguishes a rule MAINTAINING an already-resolved scope from one ESTABLISHING its first, and the two branches are deliberately opposite: maintaining retains the last known-good set and reports Unknown, while establishing must refuse with a terminal, actionable Stalled. A grant left behind by a deleted rule is inherited by the next rule created under that name and spec — a name a different tenant may now own — so an unevaluatable policy reads as maintaining. The rule then sits Unknown and Reconciling indefinitely instead of saying why it will never run. The consequence is narrower than "it keeps running": the maintaining branch compiles nothing, and the deleted rule was already out of the store, so no stream resurrects. The damage is a rule stuck silently in progress. The test recreates a byte-identical rule, which is the case the spec hash cannot catch — only forgetting the grant can — and was verified to fail against the unfixed controller. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…iptions
NamespaceMatcher.names accepted `*`, and expandWildcard copies policy names
verbatim into a rule's resolved scope. The effect is NOT the privilege
escalation it looks like: Kubernetes treats `*` as a LITERAL namespace name — a
list or watch against namespaces/* matches nothing, measured against a live
cluster — and matchesSourceNamespace is exact string equality with no glob. So
a policy of names: ["*"] resolved a `sourceNamespace: "*"` item to a namespace
that cannot exist. The rule reported itself authorized, planned a stream, and
mirrored NOTHING, which is precisely the silent no-op the
NoAdmittedSourceNamespaces reason exists to make loud.
Entries now validate as DNS-1123 labels, which both NamespaceMatcher users
inherit from the shared type, plus a defensive ValidateNames() for policies
already in etcd. The gate treats a failure as Unavailable — a policy that
cannot be evaluated, not a smaller one — so one bad entry condemns the whole
policy rather than silently narrowing to the well-formed remainder. The
"every namespace" declaration is, and remains, `selector: {}`.
BREAKING CHANGE: an allowedSourceNamespaces or allowedNamespaces entry that is
not a valid namespace name is now rejected at admission. `*` was never a
pattern there; it silently matched nothing. Use `selector: {}` to admit every
namespace.
Also trims the CRD descriptions, which had grown to 46 KB of schema text —
allowedSourceNamespaces alone was 47 lines of design rationale in `kubectl
explain`. Descriptions now carry the contract and point at docs/configuration.md
for the full resolution table; rationale moves to comment blocks detached from
the doc comment by a blank line, so it stays in the source and out of the
schema. Total description text drops to 38 KB and nothing of ours exceeds 12
lines. Verified to change descriptions ONLY: placing such a block between a
type's markers and its doc comment silently drops the markers, which cost
ClusterWatchRule its scope=Cluster (the CRD flipped to Namespaced) and both
rule kinds their printer columns before the diff caught it. AGENTS.md records
the convention and that check.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…w-ups Three corrections, all found by reviewing PR 4 against the code: - The migration warning said a target with no declared policy "admits nothing". ReasonLegacySourceNamespace disagrees: a WatchRule whose every item watches its own namespace keeps working with no policy and no delegation flag. Stated unqualified, UPGRADING.md would tell every existing operator their rules break on upgrade, which is false and the more expensive of the two possible errors. The denial applies to converted wildcard and cross-namespace items. - The historical baseline said PR 4 defers selector-backed wildcards. PR 4 ships them and resolves them from the source-namespace snapshot. - A facts doc still said ClusterWatchRule watches namespaced resources. Adds pr4-review-followups.md, the intake for both review passes. Each item was re-verified against the code before being accepted, and one reported blocker did not survive that check — its premise (that Kubernetes reads `*` as an all-namespaces watch) is false, though its recommendation stands for a different reason. The doc records what landed and which test holds it. Also carries architecture.md revisions from a parallel review pass: per-ClusterProvider source contexts, the control-plane/source-cluster split in the flow diagrams, and the distinction between the object mark-and-sweep and the Namespace-label listing that maintains selector authorization. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ecating it The field was carried as a "loud rejection" — a CEL guard, a SourceNamespaceFieldRemoved reason, and a compile-path refusal — on the same reasoning as ClusterResourceRule.scope: deleting a field is the SILENT option, because CRD pruning happens on write, so a re-applied legacy manifest is accepted with the value dropped and the rule quietly changes what it mirrors. That reasoning is about STORED objects, and it does not apply here. spec.sourceNamespace has never existed on main — not in the Go type, not in the generated CRD — so no cluster's etcd can hold one and no manifest in the wild sets one. The only population it protected was this repo's own pre-release fixtures, which move with the change. Retaining a rejection for a field nobody can have set is ceremony, and it leaves a phantom in `kubectl explain` that readers have to be told to ignore. ClusterResourceRule.scope keeps its rejection: that field DID ship, so a stored `Namespaced` value is a real object in a real cluster. Removes the field, the CEL guard, DeclaresRemovedSourceNamespace(), the SourceNamespaceFieldRemoved reason and its refusal path, and the four tests that covered them. UPGRADING.md now describes one retained shim rather than two, with the never-shipped field stated as simply absent. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Scope is now carried by the rule kind.
WatchRuleselects namespaced resources and eachspec.rules[]item names the source namespace it watches;ClusterWatchRuleselects cluster-scopedresources and has no scope or namespace choice at all. This is PR 4 of
docs/design/watchrule-source-namespace/, and it
supersedes the
WatchRule.spec.sourceNamespaceshape this branch started from.Important
Release-gated with PR 5. PR 4 removes the bypass; GitTarget deletion safety lands in PR 5.
They may merge separately but must ship in the same release — see the design README's
release order. Do not cut a release from
mainuntil PR 5 lands.API (v1alpha3)
WatchRule.spec.rules[].sourceNamespace— omitted (the rule's own namespace), an exactDNS-1123 name, or
"*"for every namespace the GitTarget admits. Structurally validated, so amalformed namespace is rejected at admission rather than resolving to nothing at compile time.
ClusterWatchRuleis cluster-scope-only.spec.rules[].scopeis optional, defaults toCluster, and the enum accepts onlyCluster.GitTarget.spec.allowedSourceNamespaces— the deny-by-defaultNamespaceMatcherboundingwhich source namespaces may be mirrored into a target, by any rule kind. A declared policy is
exhaustive (no self-namespace exception); empty
{}is not the same as omitted. It is also what a"*"item enumerates against.ClusterProvider.spec.allowSourceNamespaceOverride(renamed fromallowWatchRuleSourceNamespaceOverride— unreleased, so a plain rename with no shim) — thefalse-by-default delegation flag, required for every cross-namespace request including
"*".Both superseded fields are retained as loud rejections, not deleted
Deleting them is the silent option: CRD pruning happens on write, so a re-applied legacy manifest
would be accepted with the value dropped — no error anywhere — and the rule would quietly change
what it mirrors. So
ClusterWatchRule.spec.rules[].scope: NamespacedandWatchRule.spec.sourceNamespaceare both rejected at apply time, and a stored value (admittedby an older CRD) is refused terminally at compile. They are removed entirely one release from now.
Authorization
ResolveWatchRuleSourceScopeanswers a whole rule at once, returning one decision perspec.rules[]item. The three-part ordering — GitTarget policy, then ClusterProvider delegation, then the rule's
own namespace — is unchanged; the ClusterProvider verdict is identical for every item and so is
memoised per rule.
— so a transient source-cluster outage never becomes a terminal
Stalled=True, and anunevaluatable policy never narrows a running rule to nothing (which would feed a sweep and delete
a tenant's Git content).
"*"item unions the policy's literalnames(answered locally, with no source-cluster access)with the enumerated selector set. A selector that cannot be evaluated makes the whole item
unknown — never a partial set, because a partial set silently narrows what a rule mirrors.
NoAdmittedSourceNamespacesis a distinct reason so a wildcard resolving to nothing reports thatit watches nothing instead of looking healthy.
denied item refuses the whole rule, and the message names which item lost.
Data plane
(
CompiledResourceRule.SourceNamespaces);AddOrUpdateWatchRulereplaces the whole ruleatomically, and matching filters the event namespace per item.
namespace) — rather than as a read-side filter that would re-resolve policy on every event.
watchRuleFingerprinthashes each item's resolved namespace set. A wildcard's inputs are theGitTarget policy and the source cluster's namespaces, neither of which is rule state, so
byte-identical rule objects must still re-project the watched-type table when the policy moves.
value), so an edit or a reorder discards the grant while an unevaluatable policy keeps the last
resolved set instead of sweeping.
the admission enum; a shared
CompileClusterWatchRuleused by both the reconciler and thestartup bootstrap (so a restart cannot open a namespaced watch before status exists); and
resolution that always matches cluster scope, so
resources: ["*"]cannot pull a namespaced typeback in.
SourceNamespaceAuthorizedpublishes the aggregate.StreamSummaryForWatchRulenow reads thecompiled rule, not the spec: a wildcard's expected stream count depends on how many namespaces
it resolved to, so keying on the spec left a wildcard rule permanently not-Ready.
Routing is unchanged
sourceNamespacechanges only which namespace is watched. Git placement always follows eachmirrored object's own namespace, so a rule in
tenant-acmewatchingrepo-configwrites underrepo-config/…, nevertenant-acme/…. The e2e spec asserts exactly this.Migration
docs/UPGRADING.mdcarries the entry: the two capabilities deliberately removed(platform-authored namespaced mirroring from outside the tenant namespace, and rule-author-declared
all-namespace watching — the latter replaced by the destination-declared
allowedSourceNamespaces: {selector: {}}plus"*"), the conversion, ajqone-liner to findaffected objects, and the warning that a target declaring no
allowedSourceNamespacesadmitsnothing to a
"*"item, so converting without also declaring that policy narrows productiondata. There is deliberately no migration tool: the conversion is cross-kind, which a conversion
webhook cannot perform.
Tests
internal/controller/superseded_fields_admission_test.go— envtest against the generated CRDs:scope: Namespacedandspec.sourceNamespacerejected,rules[].sourceNamespace(including"*") accepted, a malformed namespace rejected, an omittedscopedefaulted.TestBootstrap_PreExistingNamespacedClusterRuleIsRefused— the restart window the shared compilepath closes.
TestWatchRuleFingerprint_ChangesWithResolvedSourceScopeandTestWatchedTypeTable_RebuildsWhenOnlyThePolicyChanged— byte-identical rule objects, movingpolicy.
TestCompileWatchRule_RetainsScopeWhenPolicyBecomesUnevaluatable— asserts the retained namespaceset, i.e. the absence of the sweep.
internal/authztruth table extended with every wildcard row, plus per-item mixed, denied-item andempty-wildcard cases at both the authz and reconciler level.
rules[].sourceNamespaceand gains a wildcard caseasserting against real commits that an admitted namespace arrives and an unadmitted one never does.
The restart fixture becomes a
WatchRuleand still covers theapiVersions: ["*"]startup-snapshotregression.
🤖 Generated with Claude Code
Also fixed here: a pre-existing placement collapse
The new two-source-namespace e2e coverage exposed a bug in sibling inference that this feature makes
easy to reach. Step 2 of inference ("same type, any namespace") may only reuse a cohort for an unseen
namespace if that cohort has proven itself namespace-agnostic. The bundle branch enforced that; the
singleton branch only asked "do all the candidate directories agree?", which is trivially true of a
single directory — so one directory holding one namespace passed the guard.
It cascades. Objects that exist under the same name in every namespace (
kube-root-ca.crt) make theinferred path collide exactly with the first namespace's file, so the second namespace's object is
appended as another document; that file then genuinely spans two namespaces, so every later object of
the type legitimately prefers the bundle. Observed end to end: four ConfigMaps from two namespaces in
one file, the only ConfigMap file in the tree.
Not introduced by this branch —
internal/gitandinternal/manifestanalyzerare untouched by itand
placement.golast changed in #239 — butsourceNamespace: "*"reaches the triggering shape froma single rule, so it is fixed here rather than left behind a new one-line trigger.
Summary by CodeRabbit
New Features
SourceAuthorizeddisplay column.Bug Fixes
Documentation