feat: ClusterProvider — source clusters by name, reconcile-time namespace authorization#251
Merged
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (84)
📝 WalkthroughWalkthroughThis PR introduces a cluster-scoped ChangesClusterProvider API, CRDs, and Installation
Estimated code review effort: 4 (Complex) | ~75 minutes Provider Validation and Source-Client Resolution
Provider-Aware Audit Routing and Attribution Storage
Provider Identity Through Watches and Git Writes
Runtime Wiring, Chart Configuration, and End-to-End Validation
Design, Security, and Supporting Documentation
Sequence Diagram(s)sequenceDiagram
participant GitTarget
participant ClusterProvider
participant SourceClusterResolver
participant AuditHandler
participant AttributionIndex
GitTarget->>ClusterProvider: reference via spec.clusterProviderRef
ClusterProvider->>SourceClusterResolver: validate/resolve kubeconfig
SourceClusterResolver-->>GitTarget: rest.Config for watch
AuditHandler->>ClusterProvider: check provider exists (named route)
AuditHandler->>AttributionIndex: record fact under providerName
GitTarget->>AttributionIndex: lookup author scoped by providerName
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
sunib
marked this pull request as ready for review
July 18, 2026 18:33
…pace authorization
Implements docs/design/multi-cluster-author-attribution.md: a cluster-scoped
ClusterProvider (the read-side peer of GitProvider) that a GitTarget references by
NAME, making that name the source cluster's identity everywhere — the attribution
fact-index key and the /audit-webhook/<name> ingress route. SourceClusterID is
retired, and GitTarget.spec.kubeConfig is replaced by a reference to a provider.
A written assessment of what shipped versus what was designed, including the gaps,
lives in docs/design/multi-cluster-author-attribution-evaluation.md.
What ships:
* ClusterProvider CRD + reconciler — cluster-scoped; kubeConfig optional (omit =
in-cluster) and immutable; CEL (name=="default")==!has(kubeConfig);
allowedNamespaces; kubeconfig Validated gate. Ready == Validated.
* Retired SourceClusterID — the engine is keyed by provider name ("default", no ""
sentinel); the resolver looks up the ClusterProvider and reads its Secret from the
operator namespace.
* Namespace authorization, deny-by-default — a GitTarget may reference a
ClusterProvider only from a namespace its spec.allowedNamespaces admits.
* Name-keyed facts — a cluster:<name> keyspace dimension, which also fixes the
rv-only-hatch correctness bug; providerName threaded through recorder and
resolver, with cross-cluster isolation tests.
* No finalizer, no fact purge — provider facts expire on their own short TTL and a
recreated source has fresh object UIDs, so there is nothing a purge would protect.
* ClusterProviderReady projected onto GitTarget; the ClusterProvider watch fires on
a Ready-condition change rather than generation-only, and a Namespace label-change
watch re-enqueues that namespace's GitTargets so selector-based revocation
converges promptly.
* Remote ingest — /audit-webhook/<name> routing, gated on the ClusterProvider
existing. Auth remains the shared CA-verified client cert; per-provider cert
binding is future work, called out as Gap 1 in the evaluation doc.
* Install ordering — CRDs are applied before the resources that instantiate them,
and the shipped installer is two signed files: dist/crds.yaml, then
dist/install.yaml.
Namespace authorization is enforced ONCE, at reconcile:
An earlier revision of this work also enforced it at admission, via a
validate-gittarget-namespace validating webhook with failurePolicy: Fail. That
webhook is gone.
It was never the boundary. A webhook fires only at admission, so tightening
allowedNamespaces after a GitTarget exists is invisible to it. checkSourceAuthorization
covers exactly that and is strictly stronger: it runs on every reconcile, inside the
Validated gate, which returns BEFORE DeclareForGitTarget. An unauthorized GitTarget
therefore starts no watch and writes nothing to Git — it sits Validated=False /
NamespaceNotAuthorized.
It also cost a hard dependency. It was the only failurePolicy: Fail webhook in the
chart, which made the apiserver depend on the manager Pod to admit a GitTarget at
all — and that broke the documented quickstart, whose whole audience is first-time
users on a fresh cluster:
Error: INSTALLATION FAILED: ... Kind=GitTarget: Internal error occurred:
failed calling webhook "validate-gittarget-namespace.configbutler.ai":
no endpoints available for service "gitops-reverser"
Removing it restores README step 4 to a single `helm install ... --set
quickstart.enabled=true` and the quickstart e2e leg to a single helm pass. Both were
proven by running them from a clean cluster, not assumed:
* one-command install on a genuinely fresh slate — cluster cleaned, release
uninstalled, all six CRDs deleted, local image: exit 0, and the starter
GitProvider/GitTarget/WatchRule are all admitted.
* INSTALL_MODE=helm task test-e2e-quickstart-helm: 1 Passed, 0 Failed.
* the chart now renders exactly one ValidatingWebhookConfiguration
(validate-operator-types, failurePolicy: Ignore).
The surviving gate is pinned by TestReconcile_UnauthorizedNamespaceStartsNoWatch,
which drives the real Reconcile and asserts both halves: the condition, and that the
watch manager never received a declaration. That needed one exported accessor,
watch.Manager.DeclaredSourceCluster, because the capture-on-Declare map had no
reader and clusterIDForGitTarget deliberately hides the not-yet-declared case behind
the local-cluster default, so it cannot tell "refused" from "declared local". The
test carries a positive control through the real DeclareForGitTarget so the negative
assertion cannot pass vacuously. Exercised live as well: tightening the default
provider's allowedNamespaces after the GitTarget existed flipped it to
NamespaceNotAuthorized in ~5s, with StreamsRunning, GitPathAccepted and Ready all
downgraded — the case admission could not have caught.
Not a breaking change: none of this has shipped in a release, so there is no upgrade
path to preserve. Flagged for release notes rather than treated as a break.
Validation: task fmt -> generate -> manifests -> vet -> lint -> test -> test-e2e all
green; unit coverage holds at the 76.1 baseline; e2e 58 Passed / 0 Failed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
sunib
force-pushed
the
feat/cluster-provider-author-attribution
branch
from
July 18, 2026 18:38
a3d7b59 to
a9793fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/watch/cluster_context.go (1)
516-520: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winContinue refreshing in-cluster source providers.
Once a source resolves in-cluster,
cc.isLocal()permanently skips provider resolution. If that immutable provider is deleted and recreated as remote, the manager can retain its old in-cluster credentials and mirror the wrong cluster. Only the config-plane context should bypass refresh.Proposed fix
func (m *Manager) refreshClusterCredentials(ctx context.Context, cc *clusterContext) { - if cc.isLocal() { + if cc.configPlane { return } cfg, version, inCluster, err := m.resolveSourceConfig(ctx, cc)🤖 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/cluster_context.go` around lines 516 - 520, Update Manager.refreshClusterCredentials so the early return bypasses refresh only for the config-plane context, rather than using cc.isLocal(). Allow in-cluster source providers to continue through resolveSourceConfig, so recreated remote providers receive refreshed credentials and cluster selection.
🟡 Minor comments (12)
docs/design/clusterprovider-fact-purge.md-78-82 (1)
78-82: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the documented default TTL.
DefaultAttributionFactTTLis10 * time.Minute, not 15 minutes. Update this note or centralize the value reference so the flag help and documentation cannot drift.🤖 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/design/clusterprovider-fact-purge.md` around lines 78 - 82, Correct the TTL discrepancy described in the note: verify the actual DefaultAttributionFactTTL value and align the flag help and configuration documentation with that 10-minute default, or centralize the shared value reference so they cannot diverge. Update the surrounding note to remove the incorrect 15-minute claim while preserving the chart-specific behavior statement.cmd/main.go-619-625 (1)
619-625: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNormalize
clusterAnnotationKeybefore validation and use.A whitespace-only value passes this check when attribution is enabled, but remains non-empty in
NewAuditHandler, unintentionally enabling a shared route that cannot match real annotations.+ cfg.clusterAnnotationKey = strings.TrimSpace(cfg.clusterAnnotationKey) - if strings.TrimSpace(cfg.clusterAnnotationKey) != "" && !cfg.authorAttribution { + if cfg.clusterAnnotationKey != "" && !cfg.authorAttribution {🤖 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 `@cmd/main.go` around lines 619 - 625, Normalize cfg.clusterAnnotationKey by trimming whitespace before the validation shown and before it is passed to NewAuditHandler, ensuring whitespace-only values become empty and cannot enable annotation routing. Reuse the normalized value consistently in the author-attribution validation and handler configuration.charts/gitops-reverser/templates/clusterprovider-default.yaml-20-22 (1)
20-22: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winQuote the rendered Secret reference strings.
Valid Secret names or keys resembling YAML booleans/numbers can otherwise render with the wrong type.
Proposed fix
- name: {{ .secretRef.name }} + name: {{ .secretRef.name | quote }} {{- with .secretRef.key }} - key: {{ . }} + key: {{ . | quote }}🤖 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 `@charts/gitops-reverser/templates/clusterprovider-default.yaml` around lines 20 - 22, Quote the rendered Secret reference values in the template fields under the Secret reference block: update both .secretRef.name and the conditional .secretRef.key output to render as YAML strings, preserving the existing with behavior and values.internal/watch/materialization.go-12-13 (1)
12-13: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDo not describe
defaultas necessarily identifying the operator’s cluster.
SourceCluster()returns the provider name; a provider nameddefaultmay point to either the local or a remote cluster.Proposed fix
-// referenced ClusterProvider's name, "default" for the cluster the operator runs in. It is +// referenced ClusterProvider's name, defaulting to "default" when the reference is omitted. It is🤖 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/materialization.go` around lines 12 - 13, Update the comment describing clusterID and SourceCluster() so it states only that "default" is a provider name, without implying it necessarily identifies the operator’s local cluster; preserve the explanation that SourceCluster() returns the referenced ClusterProvider’s name.api/v1alpha3/clusterprovider_types.go-144-147 (1)
144-147: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winUpdate the authorization documentation after removing the validating webhook.
The API source and generated CRD still claim namespace authorization occurs at admission, while this PR intentionally moved enforcement to reconciliation.
api/v1alpha3/clusterprovider_types.go#L144-L147: describe reconciliation-time enforcement before watches and Git writes.api/v1alpha3/clusterprovider_types.go#L181-L186: remove the reference to an admission webhook sharing this predicate.config/crd/bases/configbutler.ai_clusterproviders.yaml#L48-L51: regenerate the CRD after correcting the source comments.🤖 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/clusterprovider_types.go` around lines 144 - 147, The authorization documentation still describes admission-time enforcement and a validating webhook. Update api/v1alpha3/clusterprovider_types.go lines 144-147 to document reconciliation-time enforcement before watches and Git writes, and remove the admission-webhook reference at lines 181-186. Regenerate config/crd/bases/configbutler.ai_clusterproviders.yaml lines 48-51 from the corrected API comments.api/v1alpha3/clusterprovider_types.go-68-68 (1)
68-68: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAddress the over-120-character kubebuilder markers.
These three marker lines exceed the repository limit and may fail the configured Go lint gate. Shorten them or apply the repository’s narrow exception pattern without changing marker semantics.
As per coding guidelines, “Keep Go code within the 120-character line limit enforced by
.golangci.yml.”Also applies to: 72-72, 76-76
🤖 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/clusterprovider_types.go` at line 68, Shorten the kubebuilder validation markers near the kubeConfig fields in the ClusterProvider type so each line stays within 120 characters while preserving the existing rule and message semantics. Apply the repository’s established narrow lint exception pattern only if shortening is not feasible, covering all three markers.Source: Coding guidelines
internal/watch/source_cluster_resolver.go-32-41 (1)
32-41: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign the resolver documentation with its actual contract.
Lines 39-41 say in-cluster providers never reach this resolver, but Lines 91-95 deliberately resolve them and the interface identifies this resolver as the authority for that decision. Remove the remote-only claim.
🤖 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/source_cluster_resolver.go` around lines 32 - 41, Update the documentation for secretSourceClusterResolver to remove the claim that only remote providers reach it or that the in-cluster default provider is excluded. Keep the explanation of resolving ClusterProvider names through the operator namespace and preserve the resolver’s role as the authority for both in-cluster and remote provider resolution.internal/queue/attribution_index.go-472-503 (1)
472-503: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the retired finalizer contract.
The PR removes ClusterProvider purge finalizers and relies on fact TTL, but this godoc says the method is invoked by that finalizer. Update it to identify the actual caller, or remove the dormant purge API so deletion guarantees are not misrepresented.
🤖 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/queue/attribution_index.go` around lines 472 - 503, Update the godoc for AttributionIndex.PurgeClusterFacts to remove the obsolete ClusterProvider delete-finalizer claim and accurately describe its current caller, or remove PurgeClusterFacts entirely if no production code invokes it. Ensure the documentation does not promise deletion guarantees that the retired finalizer no longer provides.charts/gitops-reverser/values.schema.json-320-324 (1)
320-324: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate the
allowedNamespacespolicy shape.
additionalProperties: trueaccepts typos such asselectorsorname, bypassing Helm validation and potentially leaving the provider with an unintended authorization policy. Define the supportednamesandselectorproperties and reject unknown fields.🤖 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 `@charts/gitops-reverser/values.schema.json` around lines 320 - 324, Update the allowedNamespaces schema definition to explicitly declare the supported names and selector properties, including their appropriate types, and set additionalProperties to false. Preserve the existing policy object while ensuring unknown fields such as selectors or name are rejected during Helm validation.internal/webhook/audit_handler_test.go-344-358 (1)
344-358: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the required scenario suffix to this test name.
Rename it to something such as
TestProviderRouteForPath_RouteMapping.As per coding guidelines, “Use the naming convention
TestFunctionName_Scenario(t *testing.T)for tests.”🤖 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/webhook/audit_handler_test.go` around lines 344 - 358, Rename the test function TestProviderRouteForPath to include its scenario suffix, such as TestProviderRouteForPath_RouteMapping, while leaving the test assertions and coverage unchanged.Source: Coding guidelines
test/e2e/source_cluster_e2e_test.go-43-45 (1)
43-45: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winResolve the operator namespace at runtime.
Pinning these Secrets to
defaultE2ENamespacebreaks source-cluster tests when the controller is installed under a configured non-default namespace. UseresolveE2ENamespace()when rendering and applying the Secret.Proposed fix
- sourceClusterOperatorNS = defaultE2ENamespace+ operatorNS := resolveE2ENamespace() - manifest, err := kubectlRunInNamespace(sourceClusterOperatorNS, "create", "secret", "generic", name, + manifest, err := kubectlRunInNamespace(operatorNS, "create", "secret", "generic", name, ... - _, err = kubectlRunWithStdin(sourceClusterOperatorNS, manifest, "apply", "-f", "-") + _, err = kubectlRunWithStdin(operatorNS, manifest, "apply", "-f", "-")Also applies to: 160-164
🤖 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_cluster_e2e_test.go` around lines 43 - 45, Update source-cluster Secret rendering and application to resolve the operator namespace at runtime via resolveE2ENamespace() instead of the fixed sourceClusterOperatorNS/defaultE2ENamespace value, including the corresponding logic around the additionally affected block. Preserve the existing namespace behavior for cluster-scoped providers.test/e2e/Taskfile.yml-1245-1246 (1)
1245-1246: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTrack every input consumed by the CRD build.
A change to
config/crd/kustomization.yamlwill not invalidatecrds.installed, allowing tests to reuse stale CRDs.Proposed fix
sources: - - config/crd/bases/*.yaml + - config/crd/**/*🤖 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/Taskfile.yml` around lines 1245 - 1246, Update the CRD build task’s sources list to include config/crd/kustomization.yaml alongside the existing CRD base manifests, ensuring changes to either input invalidate crds.installed and rebuild the CRDs.
🧹 Nitpick comments (4)
internal/controller/gittarget_source_cluster_test.go (1)
27-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the required scenario suffix for these test names.
internal/controller/gittarget_source_cluster_test.go#L27-L45: rename to a form such asTestNamespaceToGitTargets_MatchingNamespace.internal/controller/gittarget_source_cluster_test.go#L47-L70: rename toTestClusterProviderReadyOrSpecChanged_EventFiltering.internal/controller/gittarget_source_cluster_test.go#L115-L174: rename toTestCheckSourceAuthorization_AllScenarios.As per coding guidelines, “Use the naming convention
TestFunctionName_Scenario(t *testing.T)for tests.” <coding_guidelines>🤖 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/controller/gittarget_source_cluster_test.go` around lines 27 - 45, Rename the tests to follow the TestFunctionName_Scenario convention: update internal/controller/gittarget_source_cluster_test.go lines 27-45 to TestNamespaceToGitTargets_MatchingNamespace, lines 47-70 to TestClusterProviderReadyOrSpecChanged_EventFiltering, and lines 115-174 to TestCheckSourceAuthorization_AllScenarios.Source: Coding guidelines
internal/watch/source_cluster_resolver_test.go (2)
91-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the version-token contents instead of checking only non-empty.
With the current fixtures,
"0/"passes this assertion without proving either generation or Secret resourceVersion is included. Assign explicit values and assert the exact token.Proposed test adjustment
+ provider := clusterProvider("value") + provider.Generation = 3 + secret := kubeconfigSecret("value", resolverKubeConfig) + secret.ResourceVersion = "7" r := newResolver(t, kubeconfig.SafetyPolicy{}, - clusterProvider("value"), kubeconfigSecret("value", resolverKubeConfig)) + provider, secret) ... - assert.NotEmpty(t, version, "the provider generation + Secret resourceVersion form the version token") + assert.Equal(t, "3/7", version)🤖 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/source_cluster_resolver_test.go` around lines 91 - 102, The test TestResolveSourceCluster_ValidAppliesThrottleAndVersion should assign explicit provider generation and Secret resourceVersion values in its fixtures, then assert the exact expected version token returned by ResolveSourceCluster instead of only checking that version is non-empty.
128-147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse table-driven subtests for these resolver scenarios.
The sequential
requirecalls abort the test and hide later cases after the first failure. Model the missing-resource and safety-policy cases as named table entries.As per coding guidelines, “Write table-driven tests where appropriate.”
Also applies to: 149-162
🤖 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/source_cluster_resolver_test.go` around lines 128 - 147, Refactor TestResolveSourceCluster_MissingProviderSecretAndKey into named table-driven subtests, with separate entries for the absent ClusterProvider, absent kubeconfig Secret, and missing resolved key scenarios. Each subtest should construct its resolver, invoke ResolveSourceCluster, and assert its expected error or kubeconfig.ReasonKeyNotFound result independently so one failure does not prevent the remaining cases from running.Source: Coding guidelines
docs/finished/config-plane-split.md (1)
3-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix markdown blockquote formatting.
Add a
>on the empty line to prevent breaking the blockquote block.♻️ Proposed fix
> **finished** — shipped or closed. Kept for context only; **nothing here binds**. For current behaviour see [`../spec/`](../spec/). Index: [`../INDEX.md`](../INDEX.md) - +> > Shipped 2026-07-17 as [`#249`](https://github.com/ConfigButler/gitops-reverser/pull/249). Supersedes the `SourceCluster` CRD proposal in > [`multi-cluster-audit-ingestion-implications.md`](../design/multi-cluster-audit-ingestion-implications.md) §5.🤖 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/finished/config-plane-split.md` around lines 3 - 6, Add the missing blockquote marker to the empty line between the introductory note and the “Shipped” line in the document, keeping both lines within the same Markdown blockquote.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 @.coverage-baseline:
- Line 1: Restore the .coverage-baseline value to 76.5 and add tests covering
the new provider paths so overall coverage meets or exceeds that baseline.
Ensure the coverage ratchet is not lowered.
In `@config/rbac/role.yaml`:
- Line 42: Add the missing update permission for the clusterproviders/finalizers
subresource in the RBAC rules, alongside the existing clusterproviders resource
entries. Apply the same permission to the corresponding rule identified by the
second occurrence, preserving the existing main-resource and status permissions.
In `@docs/design/multi-cluster-author-attribution-evaluation.md`:
- Around line 184-187: Update the shipping-flow diagram’s APISERVER audit POST
endpoint, including the corresponding flow at the referenced duplicate section,
to use the named route `/audit-webhook/<provider>` instead of bare
`/audit-webhook`; alternatively show explicit configuration of
`--author-attribution-cluster-annotation-key` before using the bare route.
In `@docs/design/multi-cluster-author-attribution.md`:
- Around line 55-60: Update the affected v3 documentation sections to remove or
clearly label them as historical, ensuring they no longer prescribe a reserved
in-cluster default, map the bare /audit-webhook route to it, or describe
superseded purge-finalizer behavior. Keep the v4/shipped model authoritative:
default is an ordinary provider name and routing uses the provider name.
- Around line 128-130: Update the namespace-authorization documentation to
consistently describe reconciliation-only enforcement: in
docs/design/multi-cluster-author-attribution.md lines 128-130, 242-245, 408-410,
451-453, and 478-480, remove current admission-denial requirements, retain the
reconcile-time Validated=False/NamespaceNotAuthorized, no-watch/no-write, and
revocation behaviors, and mark admission as deferred where relevant; update
docs/design/multi-cluster-author-attribution-evaluation.md lines 401-404 to test
reconcile denial and policy-revocation behavior instead of admission denial.
In `@internal/controller/clusterprovider_controller.go`:
- Around line 92-93: Update shedLegacyFinalizer to return the underlying
provider Update error instead of converting failures to false, and have the
reconciliation path propagate that error when invoking it. Also propagate errors
returned by updateStatusAndRequeue rather than discarding them, preserving
successful finalizer handling while allowing transient API failures to trigger
reconciliation retry.
- Line 71: Update the RBAC marker for clusterproviders to remove the create and
delete verbs, retaining only the read, update, patch, and status-writing
permissions required by the reconciler.
In `@internal/controller/gittarget_source_cluster.go`:
- Around line 127-140: Update the readiness switch in the ClusterProvider
condition evaluation to handle metav1.ConditionFalse separately, preserving the
existing not-ready message and False result only for explicit False. Return
metav1.ConditionUnknown with the not-ready reason and appropriate message for
explicit Unknown, and add a test covering the Unknown status.
In `@internal/webhook/audit_handler.go`:
- Around line 173-180: Update the audit authorization flow around
ProviderResolver.ProviderExists and the corresponding shared-stream path to
verify the authenticated client identity in r.TLS.PeerCertificates matches the
selected provider or its permitted provider set before recording facts. Reject
unauthorized provider associations, while preserving the existing
unknown-provider handling and allowing valid shared-stream identities.
---
Outside diff comments:
In `@internal/watch/cluster_context.go`:
- Around line 516-520: Update Manager.refreshClusterCredentials so the early
return bypasses refresh only for the config-plane context, rather than using
cc.isLocal(). Allow in-cluster source providers to continue through
resolveSourceConfig, so recreated remote providers receive refreshed credentials
and cluster selection.
---
Minor comments:
In `@api/v1alpha3/clusterprovider_types.go`:
- Around line 144-147: The authorization documentation still describes
admission-time enforcement and a validating webhook. Update
api/v1alpha3/clusterprovider_types.go lines 144-147 to document
reconciliation-time enforcement before watches and Git writes, and remove the
admission-webhook reference at lines 181-186. Regenerate
config/crd/bases/configbutler.ai_clusterproviders.yaml lines 48-51 from the
corrected API comments.
- Line 68: Shorten the kubebuilder validation markers near the kubeConfig fields
in the ClusterProvider type so each line stays within 120 characters while
preserving the existing rule and message semantics. Apply the repository’s
established narrow lint exception pattern only if shortening is not feasible,
covering all three markers.
In `@charts/gitops-reverser/templates/clusterprovider-default.yaml`:
- Around line 20-22: Quote the rendered Secret reference values in the template
fields under the Secret reference block: update both .secretRef.name and the
conditional .secretRef.key output to render as YAML strings, preserving the
existing with behavior and values.
In `@charts/gitops-reverser/values.schema.json`:
- Around line 320-324: Update the allowedNamespaces schema definition to
explicitly declare the supported names and selector properties, including their
appropriate types, and set additionalProperties to false. Preserve the existing
policy object while ensuring unknown fields such as selectors or name are
rejected during Helm validation.
In `@cmd/main.go`:
- Around line 619-625: Normalize cfg.clusterAnnotationKey by trimming whitespace
before the validation shown and before it is passed to NewAuditHandler, ensuring
whitespace-only values become empty and cannot enable annotation routing. Reuse
the normalized value consistently in the author-attribution validation and
handler configuration.
In `@docs/design/clusterprovider-fact-purge.md`:
- Around line 78-82: Correct the TTL discrepancy described in the note: verify
the actual DefaultAttributionFactTTL value and align the flag help and
configuration documentation with that 10-minute default, or centralize the
shared value reference so they cannot diverge. Update the surrounding note to
remove the incorrect 15-minute claim while preserving the chart-specific
behavior statement.
In `@internal/queue/attribution_index.go`:
- Around line 472-503: Update the godoc for AttributionIndex.PurgeClusterFacts
to remove the obsolete ClusterProvider delete-finalizer claim and accurately
describe its current caller, or remove PurgeClusterFacts entirely if no
production code invokes it. Ensure the documentation does not promise deletion
guarantees that the retired finalizer no longer provides.
In `@internal/watch/materialization.go`:
- Around line 12-13: Update the comment describing clusterID and SourceCluster()
so it states only that "default" is a provider name, without implying it
necessarily identifies the operator’s local cluster; preserve the explanation
that SourceCluster() returns the referenced ClusterProvider’s name.
In `@internal/watch/source_cluster_resolver.go`:
- Around line 32-41: Update the documentation for secretSourceClusterResolver to
remove the claim that only remote providers reach it or that the in-cluster
default provider is excluded. Keep the explanation of resolving ClusterProvider
names through the operator namespace and preserve the resolver’s role as the
authority for both in-cluster and remote provider resolution.
In `@internal/webhook/audit_handler_test.go`:
- Around line 344-358: Rename the test function TestProviderRouteForPath to
include its scenario suffix, such as TestProviderRouteForPath_RouteMapping,
while leaving the test assertions and coverage unchanged.
In `@test/e2e/source_cluster_e2e_test.go`:
- Around line 43-45: Update source-cluster Secret rendering and application to
resolve the operator namespace at runtime via resolveE2ENamespace() instead of
the fixed sourceClusterOperatorNS/defaultE2ENamespace value, including the
corresponding logic around the additionally affected block. Preserve the
existing namespace behavior for cluster-scoped providers.
In `@test/e2e/Taskfile.yml`:
- Around line 1245-1246: Update the CRD build task’s sources list to include
config/crd/kustomization.yaml alongside the existing CRD base manifests,
ensuring changes to either input invalidate crds.installed and rebuild the CRDs.
---
Nitpick comments:
In `@docs/finished/config-plane-split.md`:
- Around line 3-6: Add the missing blockquote marker to the empty line between
the introductory note and the “Shipped” line in the document, keeping both lines
within the same Markdown blockquote.
In `@internal/controller/gittarget_source_cluster_test.go`:
- Around line 27-45: Rename the tests to follow the TestFunctionName_Scenario
convention: update internal/controller/gittarget_source_cluster_test.go lines
27-45 to TestNamespaceToGitTargets_MatchingNamespace, lines 47-70 to
TestClusterProviderReadyOrSpecChanged_EventFiltering, and lines 115-174 to
TestCheckSourceAuthorization_AllScenarios.
In `@internal/watch/source_cluster_resolver_test.go`:
- Around line 91-102: The test
TestResolveSourceCluster_ValidAppliesThrottleAndVersion should assign explicit
provider generation and Secret resourceVersion values in its fixtures, then
assert the exact expected version token returned by ResolveSourceCluster instead
of only checking that version is non-empty.
- Around line 128-147: Refactor
TestResolveSourceCluster_MissingProviderSecretAndKey into named table-driven
subtests, with separate entries for the absent ClusterProvider, absent
kubeconfig Secret, and missing resolved key scenarios. Each subtest should
construct its resolver, invoke ResolveSourceCluster, and assert its expected
error or kubeconfig.ReasonKeyNotFound result independently so one failure does
not prevent the remaining cases from running.
🪄 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: 87387027-a31a-45e8-959b-bbc7b981e8bc
⛔ Files ignored due to path filters (2)
docs/images/config-basics.excalidraw.svgis excluded by!**/*.svgdocs/images/config-cluster.excalidraw.svgis excluded by!**/*.svg
📒 Files selected for processing (82)
.coverage-baseline.github/RELEASES.md.github/workflows/ci.yml.github/workflows/release.yml.gitignorePROJECTREADME.mdSECURITY.mdTaskfile-build.ymlapi/v1alpha3/clusterprovider_types.goapi/v1alpha3/gittarget_types.goapi/v1alpha3/zz_generated.deepcopy.gocharts/gitops-reverser/README.mdcharts/gitops-reverser/templates/clusterprovider-default.yamlcharts/gitops-reverser/templates/deployment.yamlcharts/gitops-reverser/values.schema.jsoncharts/gitops-reverser/values.yamlcmd/main.gocmd/main_audit_server_test.goconfig/clusterprovider-default.yamlconfig/crd/bases/configbutler.ai_clusterproviders.yamlconfig/crd/bases/configbutler.ai_gittargets.yamlconfig/crd/kustomization.yamlconfig/kustomization.yamlconfig/rbac/role.yamlconfig/samples/clusterprovider.yamldocs/INDEX.mddocs/architecture.mddocs/attribution-setup-guide.mddocs/ci-overview.mddocs/configuration.mddocs/design/clusterprovider-fact-purge.mddocs/design/multi-cluster-audit-ingestion-implications.mddocs/design/multi-cluster-author-attribution-evaluation.mddocs/design/multi-cluster-author-attribution.mddocs/facts/audit-webhook-api-server-connectivity.mddocs/finished/config-plane-split.mddocs/finished/documentation-triage.mdhack/e2e/install-plain-manifests.shhack/generate-audit-webhook-kubeconfig.shinternal/audit/outcome/outcome.gointernal/audit/outcome/outcome_test.gointernal/controller/clusterprovider_controller.gointernal/controller/clusterprovider_controller_test.gointernal/controller/clusterprovider_controller_unit_test.gointernal/controller/clusterprovider_predicate_test.gointernal/controller/condition_wait_test.gointernal/controller/constants.gointernal/controller/gittarget_controller.gointernal/controller/gittarget_source_cluster.gointernal/controller/gittarget_source_cluster_test.gointernal/controller/suite_test.gointernal/controller/watchrule_controller_test.gointernal/git/pending_writes.gointernal/git/plan_flush.gointernal/git/resync_flush.gointernal/git/types.gointernal/queue/attribution_index.gointernal/queue/attribution_index_deletecollection_test.gointernal/queue/attribution_index_test.gointernal/queue/key_prefix_test.gointernal/watch/author_resolver.gointernal/watch/author_resolver_test.gointernal/watch/cluster_context.gointernal/watch/cluster_context_test.gointernal/watch/cluster_model_test.gointernal/watch/manager.gointernal/watch/manager_catalog.gointernal/watch/materialization.gointernal/watch/source_cluster_resolver.gointernal/watch/source_cluster_resolver_test.gointernal/watch/target_watch.gointernal/watch/target_watch_test.gointernal/webhook/audit_handler.gointernal/webhook/audit_handler_test.gointernal/webhook/audit_identity_test.gointernal/webhook/audit_metrics_test.gointernal/webhook/fuzz_test.gotest/e2e/Taskfile.ymltest/e2e/cluster/audit/webhook-config.yamltest/e2e/quickstart_framework_e2e_test.gotest/e2e/source_cluster_e2e_test.go
| @@ -1 +1 @@ | |||
| 76.5 | |||
| 76.1 | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Restore the coverage ratchet instead of lowering it.
Add coverage for the new provider paths and retain the previous 76.5 baseline.
As per coding guidelines, “Cover new Go code with tests and ensure total coverage does not regress.”
🤖 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 @.coverage-baseline at line 1, Restore the .coverage-baseline value to 76.5
and add tests covering the new provider paths so overall coverage meets or
exceeds that baseline. Ensure the coverage ratchet is not lowered.
Source: Coding guidelines
| - apiGroups: | ||
| - configbutler.ai | ||
| resources: | ||
| - clusterproviders |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Grant permission to remove legacy ClusterProvider finalizers.
The controller’s legacy-finalizer cleanup requires update on clusterproviders/finalizers; the new main-resource and status rules do not grant that subresource permission.
Proposed fix
- apiGroups:
- configbutler.ai
resources:
+ - clusterproviders/finalizers
- gitproviders/finalizers
verbs:
- updateAlso applies to: 58-58
🤖 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 `@config/rbac/role.yaml` at line 42, Add the missing update permission for the
clusterproviders/finalizers subresource in the RBAC rules, alongside the
existing clusterproviders resource entries. Apply the same permission to the
corresponding rule identified by the second occurrence, preserving the existing
main-resource and status permissions.
Comment on lines
+184
to
+187
| NOTES -. "kubectl get secret → audit-webhook.kubeconfig" .-> Admin | ||
| Admin["Admin (hand-carry / GitOps)"] -- "copy cert + CA to node, restart apiserver once" --> APISERVER | ||
| APISERVER -- "POST /audit-webhook (mTLS, batch)" --> H | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the named audit route in the shipping flow.
The current handler rejects bare /audit-webhook unless annotation routing is configured. This flow should post to /audit-webhook/<provider>, or explicitly configure --author-attribution-cluster-annotation-key.
Also applies to: 215-220
🤖 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/design/multi-cluster-author-attribution-evaluation.md` around lines 184
- 187, Update the shipping-flow diagram’s APISERVER audit POST endpoint,
including the corresponding flow at the referenced duplicate section, to use the
named route `/audit-webhook/<provider>` instead of bare `/audit-webhook`;
alternatively show explicit configuration of
`--author-attribution-cluster-annotation-key` before using the bare route.
Comment on lines
+173
to
+180
| // The connection is already CA-authenticated (the audit server requires a client cert), so gate | ||
| // on the named ClusterProvider existing rather than accepting facts for an unknown cluster. | ||
| // Every name is gated, "default" included — it is an ordinary provider. | ||
| if h.config.ProviderResolver == nil { | ||
| http.NotFound(w, r) | ||
| return auditRoute{}, false | ||
| } | ||
| exists, err := h.config.ProviderResolver.ProviderExists(ctx, providerName) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Authorize the authenticated client for the selected provider.
ProviderExists only proves the name exists. Neither route compares it with r.TLS.PeerCertificates, so any client trusted by the common mTLS CA can submit facts under another provider and cause cross-cluster author misattribution. Bind peer identity to the permitted provider—or provider set for shared streams—before recording facts.
Also applies to: 379-385
🤖 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/webhook/audit_handler.go` around lines 173 - 180, Update the audit
authorization flow around ProviderResolver.ProviderExists and the corresponding
shared-stream path to verify the authenticated client identity in
r.TLS.PeerCertificates matches the selected provider or its permitted provider
set before recording facts. Reject unauthorized provider associations, while
preserving the existing unknown-provider handling and allowing valid
shared-stream identities.
… coverage ratchet Addresses the CodeRabbit review on #251. - shedLegacyFinalizer now returns its Update error instead of swallowing it. A stranded object gets no further events of its own, so a dropped conflict left it in Terminating until an operator restart; the error is what requeues it. - reconcileClusterProvider propagates the status-write error on the invalid path rather than discarding it, so a failed write retries instead of leaving a stale status for a whole steady interval. - clusterProviderReadiness distinguishes an explicit Ready=False from Ready=Unknown. The default branch collapsed both to False, contradicting the documented "only an explicit False downgrades" contract and holding a GitTarget down on a provider that was merely mid-reconcile. - Narrow the clusterproviders RBAC marker to get;list;watch;update;patch. The reconciler never creates or deletes one. Coverage: restore and advance the ratchet (76.1 -> 76.6, above main's 76.5) by covering the paths this PR added — the api/v1alpha3 helpers (AllowsNamespace's deny-by-default and fail-closed contracts, SourceCluster, IsInCluster, IsLocalSource) which had no package tests at all, the audit outcome label helpers, the attribution-index resourceVersion precedence, the new error paths above, and checkSourceAuthorization's invalid-selector and read-error branches. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jul 20, 2026
feat(api)!: scope by kind — per-item WatchRule source namespaces, cluster-only ClusterWatchRule
#259
Merged
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.
Adds a cluster-scoped
ClusterProvider— the read-side peer ofGitProvider. It names a cluster the operator mirrors from, and that name becomes the cluster's identity everywhere: the attribution fact-index key and the/audit-webhook/<name>ingress route.Implements
docs/design/multi-cluster-author-attribution.md.multi-cluster-author-attribution-evaluation.mdassesses what shipped against that design, including the gaps.The model
A
GitTargetnames its source cluster by reference, defaulting to a provider calleddefault:kubeConfigis optional and immutable; CEL enforces(name == "default") == !has(kubeConfig), sodefaultis the local cluster and any other name is remote.GitTargetmirrors only through a provider that exists —defaultincluded — and the operator never creates one. A missing provider is a hardNotReadygate, so a target can never fall back to an implicit in-cluster identity.ClusterProviderReadyis projected onto theGitTarget, so onekubectl get gittargetseparates a source-side problem from a destination-side one.Replaces
SourceClusterID: the engine is keyed by provider name throughout (no""sentinel), and inlineGitTarget.spec.kubeConfigis gone in favour of the reference.Who may use a source cluster
spec.allowedNamespaces(names and/or selector, deny-by-default) decides which control-cluster namespaces may reference a provider. It exists to close a confused-deputy: a provider holds a credential that can read much of a remote cluster, and anyGitTargetreferencing it makes the operator export that cluster's state into the target's repo.It is enforced at reconcile, on every reconcile — inside the
Validatedgate, which returns beforeDeclareForGitTarget. An unauthorizedGitTargetstarts no watch and writes nothing to Git; it sitsValidated=False/NamespaceNotAuthorizedwithStreamsRunning,GitPathAcceptedandReadyall downgraded.Reconcile rather than admission is deliberate, and is the stronger of the two: admission sees a
GitTargetonly as it is written, so tightening a policy would not affect targets that already exist. Re-checking every reconcile means revocation actually stops an active mirror. Propagation is prompt, not just eventual — the provider watch fires on aReadychange, and a Namespace label-change watch re-enqueues that namespace's targets so selector-based revocation converges without waiting for the periodic resync.A consequence worth stating plainly: the chart installs no
failurePolicy: Failwebhook, so the API server never depends on the operator Pod to admit aGitTarget, and installing the operator and its resources in one pass works.Attribution across clusters
Facts are keyed by a
cluster:<name>dimension, which also fixes a latent correctness bug in the resourceVersion-only hatch (facts from different clusters could previously collide on an rv match).providerNameis threaded through the recorder and resolver, with cross-cluster isolation tests.Remote clusters reach the operator at
/audit-webhook/<name>, gated on the provider existing. Facts expire on their own short TTL and a recreated source has fresh object UIDs, so there is no purge step and no finalizer.Install
CRDs are applied before the resources that instantiate them. The shipped installer is two independently signed and attested files, applied in order:
dist/crds.yaml, thendist/install.yaml. Helm is unaffected — it has its own CRD phase.Known limits
/audit-webhook/<name>is gated on the name existing, behind one shared CA-verified client cert. Anyone holding that cert can post to any provider's route and forge author facts for it. The design's per-provider cert binding (cert == path) is not built — see Gap 1 in the evaluation doc. This is an accepted privileged-control-plane assumption, documented inSECURITY.md.source-clusterleg, so none of it gates a merge.attribution.modeenum, noingressLimits, and runtime-reachability status fields are deferred.Ready==Validated.Compatibility
Removes
GitTarget.spec.kubeConfigand defaults the replacement ref todefault. Nothing here has shipped in a release, so there is no upgrade path to preserve — flagged for release notes rather than treated as a break.Validation:
task fmt→generate→manifests→vet→lint→test→test-e2eall green; unit coverage holds at the 76.1 baseline; e2e 58 passed / 0 failed.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation