feat: config-plane split, immutable GitTarget.spec.kubeConfig for remote-cluster mirroring#249
Merged
Conversation
…dates The RED-FIRST scaffold (test/e2e/source_cluster_e2e_test.go) is dormant behind E2E_ENABLE_SOURCE_CLUSTER; the design doc (docs/design/config-plane-split.md) is the spec for GitTarget.spec.kubeConfig. Supersede §5 of the multi-cluster audit doc and add the INDEX entry. .fossa.yml excludes external-sources/ from scans. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…eference) Step 1 of the config-plane split (docs/design/config-plane-split.md): a GitTarget may name the source cluster it mirrors FROM via an inline, optional, immutable spec.kubeConfig — Flux's meta.KubeConfigReference verbatim, the same field Kustomization.spec.kubeConfig uses. Omitted means "the cluster I run in". - go.mod: import ONLY github.com/fluxcd/pkg/apis/meta@v1.31.0 (no pkg/runtime, no pkg/auth); its embedded secretRef/configMapRef CEL travels into our CRD. - Two spec-level CEL rules: immutability (like providerRef/branch/path), and a configMapRef-reject guard so v1alpha3 is "secretRef only" (configMapRef = provider/workload-identity auth is deferred). - GitTarget.SourceClusterID() renders the data-plane cluster id "<namespace>/<name>/<key>" (empty key is its own identity); "" = local cluster. - Regenerated deepcopy + CRD; synced chart CRDs (helm-sync). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Steps 2-4 of the config-plane split (docs/design/config-plane-split.md): the watch
manager grows a per-cluster context so a GitTarget can mirror a remote cluster
reached by its spec.kubeConfig, resolved against that cluster's own type registry.
- clusterContext (cluster_context.go): the catalog/registry/clients that used to be
Manager-wide are now per cluster, keyed LocalClusterID (""). A zero-value Manager
still creates exactly one local context, so single-cluster behavior is byte-for-byte
unchanged. Contexts are refcounted: the last GitTarget leaving a remote tears its
context down; the local one is never torn down. Trigger informers stay local-only
(a "refresh sooner than the 30s tick" latency optimization); remote catalog freshness
rides the periodic refresh — a deliberate cut from the reference's per-cluster triggers.
- SourceClusterResolver (source_cluster_resolver.go) + internal/kubeconfig: parse the
cluster id -> read the Secret from the config plane -> value/value.yaml key fallback
(no schema default) -> REJECT unsafe kubeconfigs (exec / insecure-skip-tls-verify),
diverging from Flux's silent strip -> build rest.Config, drop the bytes, keep the
Secret resourceVersion as the rotation token. Rotation is picked up on the refresh
cadence, not the hot watch path.
- Capture-on-Declare (NOT per-rule): DeclareForGitTarget records the GitTarget's source
cluster the same way it records the UID. Because spec.kubeConfig is immutable there is
no rules-disagree window, so the reference's CompiledSourceClusters race apparatus is
gone entirely.
- Per-cluster resolution: RefreshAPIResourceCatalog refreshes every active cluster
(returning only the local error); watched-type tables, scope resolution, stream
readiness, and watch/list opens all resolve against the GitTarget's OWN cluster.
- Target-scoped GVK->GVR (NOT a union): the git writer resolves each folder's documents
against its GitTarget's source-cluster registry, threaded via Event/ResolvedTargetMetadata
SourceClusterID. A union is a correctness bug (two clusters can serve one GVK under
different GVRs/scopes); replaced with SetClusterMapper + ClusterTypeLookup.
- cmd/main.go: wire the resolver + cluster mapper; add --source-cluster-qps/-burst and
--insecure-kubeconfig-exec/-tls flags.
Unit tests: kubeconfig key fallback + reject-unsafe; resolver id parse/fallback/reject/
version; clusterContext refcount teardown, capture, per-cluster registry, reachability
classification. task test + lint green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…urceClusterReachable, GitProviderReady)
Step 6 of the config-plane split. Makes the source-cluster split legible in one
`kubectl get gittarget`, splitting inputs from runtime and source from destination.
- Validated extended (controller, no dial): a bad spec.kubeConfig now fails Validated
with the exact input reason — KubeConfigSecretNotFound / KubeConfigKeyNotFound /
KubeConfigInvalid / KubeConfigExecNotAllowed / KubeConfigInsecureTLSNotAllowed — via
internal/kubeconfig, the same contract the resolver enforces. Reachability is
deliberately NOT checked here.
- SourceClusterReachable (new, runtime): projected from the data plane's discovery
attempt. True/LocalCluster when kubeConfig is omitted, Unknown before first discovery,
False (SourceClusterUnreachable / AuthenticationFailed / AccessDenied) after a real
failed attempt. A bounded 15s dial timeout on the source client keeps an unreachable
remote from hanging the refresh loop.
- GitProviderReady (new, projection): mirrors the referenced GitProvider's Ready, folded
into the GitTarget's Ready via the existing Watches(&GitProvider{}) trigger. Absent /
unobserved provider readiness is Unknown and does NOT downgrade Ready — only an explicit
Ready=False does — so a not-yet-reconciled provider never blocks its target.
- Ready folding only ever DOWNGRADES: a source/provider problem holds the target below
Ready, but a healthy pair never overrides a still-replaying stream.
Unit tests for validateKubeConfig (all five reasons + value.yaml fallback + opt-in) and
gitProviderReadiness (ready/not-ready/absent). task test + lint green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
….secretRef Step 5 of the config-plane split — the credential-reference authorization boundary (docs/design/config-plane-split.md "Security"). Closes the confused-deputy escalation the split opens: a tenant granted create on GitTarget but NOT Secret-read could name a privileged kubeconfig Secret they cannot read, and the operator (which can) would mirror that remote cluster's state into a repo the tenant controls. - ValidateGitTargetKubeConfigHandler: on GitTarget CREATE/UPDATE, when spec.kubeConfig.secretRef is set, issues a SubjectAccessReview for the requesting user's `get` on the named Secret and DENIES if they lack it. FAIL-CLOSED: any authorizer error, or a nil authorizer, denies — a missing verdict never silently admits the escalation. A GitTarget without kubeConfig.secretRef is admitted with no check. - Built fresh (the design's claim that SAR machinery already exists on main was inaccurate — the asserted-author SAR lives only on the closed PR #220); the SAR-building pattern is mined from that branch's author_assertion.go. - Registered at /validate-gittarget-kubeconfig with a real SubjectAccessReview client; config/webhook + chart template use failurePolicy: Fail, scoped ONLY to configbutler.ai/gittargets so a webhook-down state can never deadlock bootstrap. - RBAC: create on authorization.k8s.io/subjectaccessreviews (regenerated role.yaml). Unit tests (fake authorizer): allow-when-authorized, deny-when-not, fail-closed on error, fail-closed without an authorizer, allow-when-no-secretRef. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rRole, index Step 7 polish for the config-plane split: - GitTarget printer columns (priority=1) for SourceClusterReachable (shows the reason — LocalCluster / SourceClusterUnreachable / …) and GitProviderReady. - The design doc's Security section now carries the concrete minimal ClusterRole the kubeconfig identity is bound to on the SOURCE cluster (discovery + read-only on the mirrored types; never any write verb). - INDEX + design-doc header reflect spec.kubeConfig (not the old spec.sourceCluster wrapper) and that the feature is now built. The e2e scaffold's scenarios 5 (rotation), 6 (admission SAR) and 7 (GitProviderReady) remain design-documented follow-ups per the scaffold's own note; the feature code they exercise is implemented and unit-tested in this PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
The RED-FIRST scaffold's applyGitTargetWithKubeConfig indented the optional `key:` line 8 spaces (nested under `name:`) instead of 6 (a sibling under `secretRef:`), producing invalid YAML — "mapping values are not allowed here" — so `kubectl apply` failed and the GitTarget was never created. Only the "a missing key" case passes a non-empty key, so it was the one spec the bug hid behind (and, being in an Ordered container, it blocked the rest). With the fix the gated source-cluster suite is green: E2E_ENABLE_SOURCE_CLUSTER=true E2E_LABEL_FILTER=source-cluster task test-e2e -> 8 Passed | 0 Failed (scenarios 1 x5 reasons, 2, 3, 4; 8 self-skips w/o a 2nd cluster). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
Validation complete ✅All AGENTS.md gates pass:
The gated run exercises the scaffold's single-cluster scenarios end-to-end against the deployed operator:
One RED-FIRST scaffold bug fixed along the way (2c7f039): the optional |
📝 WalkthroughWalkthroughGitTargets can reference immutable Secret-backed kubeconfigs for remote source clusters. The controller validates kubeconfig safety, the watch manager maintains per-cluster state and scoped discovery, readiness exposes source/provider conditions, and Git writes preserve source-cluster-aware resource mapping. ChangesSource-cluster mirroring
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant GitTarget
participant GitTargetReconciler
participant watch.Manager
participant SourceClusterResolver
participant SourceClusterAPI
participant GitWorker
GitTarget->>GitTargetReconciler: submit kubeConfig reference
GitTargetReconciler->>SourceClusterResolver: validate and resolve Secret kubeconfig
SourceClusterResolver-->>GitTargetReconciler: REST config and version
GitTargetReconciler->>watch.Manager: declare source-cluster watches
watch.Manager->>SourceClusterAPI: discover and watch target resources
SourceClusterAPI-->>watch.Manager: event with SourceClusterID
watch.Manager->>GitWorker: route source-aware event
GitWorker-->>GitTarget: write using cluster-specific GVK-to-GVR mapping
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/git/plan_flush.go (1)
43-83: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd non-empty source-cluster tests for the mapper wiring.
All changed tests still pass
"", so a regression that ignoresclusterMapperwould pass.
internal/git/plan_flush.go#L43-L83: test that live events with a remote ID use the remote lookup.internal/git/worker_manager.go#L100-L107: verifySetClusterMapperis propagated to newly created workers.internal/git/worker_manager.go#L178-L182: assert the injected worker field is used after startup.internal/git/resync_flush.go#L292-L341: test both refusal scanning and resync application with a remote lookup.internal/git/resync_flush_test.go#L68-L68: extend the helper or add a dedicated test accepting a non-empty cluster ID.As per coding guidelines, new Go code must be covered with tests and total coverage must 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 `@internal/git/plan_flush.go` around lines 43 - 83, Add non-empty remote-cluster coverage for mapper wiring: in internal/git/plan_flush.go lines 43-83, verify live events with a remote ID use the remote lookup; in internal/git/worker_manager.go lines 100-107, verify SetClusterMapper reaches newly created workers; in internal/git/worker_manager.go lines 178-182, verify the injected worker field is used after startup; in internal/git/resync_flush.go lines 292-341, cover refusal scanning and resync application with a remote lookup; and in internal/git/resync_flush_test.go line 68, extend the helper or add a test accepting a non-empty cluster ID. Ensure the added Go behavior is covered without reducing total coverage.Source: Coding guidelines
🧹 Nitpick comments (2)
internal/controller/gittarget_source_cluster_test.go (1)
76-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename tests to follow the required naming convention. As per coding guidelines, tests must use the
TestFunctionName_Scenario(t *testing.T)naming convention. Both of these table-driven tests omit the_Scenariosuffix.
internal/controller/gittarget_source_cluster_test.go#L76-L76: Rename toTestValidateKubeConfig_AllScenariosor similar.internal/controller/gittarget_source_cluster_test.go#L144-L144: Rename toTestGitProviderReadiness_AllScenariosor similar.🤖 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` at line 76, Rename the table-driven test ValidateKubeConfig in internal/controller/gittarget_source_cluster_test.go:76-76 to follow the TestFunctionName_Scenario convention, such as TestValidateKubeConfig_AllScenarios. Also rename GitProviderReadiness in internal/controller/gittarget_source_cluster_test.go:144-144 to a matching suffixed form, such as TestGitProviderReadiness_AllScenarios.Source: Coding guidelines
internal/watch/source_cluster_resolver_test.go (1)
53-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winApply the required Go test naming convention.
internal/watch/source_cluster_resolver_test.go#L53-L67: rename to a scenario-qualified name such asTestParseSourceClusterID_ValidAndMalformedIDs.internal/watch/cluster_context_test.go#L51-L75: rename toTestForgetGitTargetCluster_ReferenceCountedTeardown.internal/watch/cluster_context_test.go#L87-L132: add scenario suffixes toTestClusterTypeLookup,TestClassifySourceClusterReachFailure, andTestRecordClusterReachability.As per coding guidelines, tests must use
TestFunctionName_Scenario(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/watch/source_cluster_resolver_test.go` around lines 53 - 67, Apply scenario-qualified Go test names: rename TestParseSourceClusterID to TestParseSourceClusterID_ValidAndMalformedIDs in internal/watch/source_cluster_resolver_test.go:53-67; rename TestForgetGitTargetCluster to TestForgetGitTargetCluster_ReferenceCountedTeardown in internal/watch/cluster_context_test.go:51-75; and add descriptive scenario suffixes to TestClusterTypeLookup, TestClassifySourceClusterReachFailure, and TestRecordClusterReachability in internal/watch/cluster_context_test.go:87-132, preserving each test’s behavior.Source: Coding guidelines
🤖 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 `@api/v1alpha3/gittarget_types.go`:
- Around line 49-57: Add a CEL validation rule alongside the existing kubeConfig
validations to reject an empty spec.kubeConfig.secretRef.name while allowing
kubeConfig or secretRef to be absent. Preserve the current immutability and
configMapRef restrictions, and target the kubeConfig validation annotations
rather than webhook logic.
In `@internal/kubeconfig/kubeconfig.go`:
- Around line 87-113: Extend checkParsed to reject file-backed credential and CA
fields in each kubeconfig AuthInfo and Cluster when the corresponding embedded
data is absent; require tokenFile, client-certificate, client-key, and
certificate-authority to be replaced by token, client-certificate-data,
client-key-data, and certificate-authority-data. Return distinct RejectionError
results using the existing policy-check pattern and preserve the current exec
and insecure-TLS checks.
In `@internal/watch/cluster_context.go`:
- Around line 420-427: The error path in Manager.refreshClusterCredentials must
fail closed instead of retaining cached clients when resolveRemoteConfig fails.
Invalidate the cluster’s cached discovery and dynamic clients, surface the
resolver error through the cluster’s existing status/error mechanism, and stop
or restart the active watches for that cluster so revoked or invalid credentials
take effect.
In `@internal/watch/manager_catalog.go`:
- Around line 63-74: Update Manager.RefreshAPIResourceCatalog so remote clusters
refresh independently or through bounded concurrency instead of serially
blocking the caller’s reconcile path. Preserve the local cluster refresh and
localErr return behavior, while ensuring remote timeouts cannot accumulate
before the method returns.
In `@internal/watch/source_cluster_resolver.go`:
- Around line 18-23: Remove the shared sourceClusterDialTimeout usage from the
source-cluster rest.Config, especially in the resolver logic around the config
construction at sourceClusterResolver. Configure only transport-level connection
establishment timeouts, leaving dynamic watch clients unbounded, and apply
scoped deadlines exclusively to finite discovery and list operations.
In `@internal/watch/watched_type_resolver.go`:
- Around line 184-193: The combinedRegistryRevision fingerprint must include
cluster identity, not just summed revisions. Update
Manager.combinedRegistryRevision to sort active cluster IDs deterministically
and derive a stable fingerprint from each clusterID paired with its registry
Revision, preserving change detection when clusters are replaced with different
identities.
In `@test/e2e/source_cluster_e2e_test.go`:
- Around line 333-353: Update the GVK-to-GVR scenario in the `It("resolves
GVK->GVR against the source cluster, not a union", ...)` test so it does not
unconditionally call `Fail` when `E2E_SOURCE_CLUSTER_KUBECONFIG` is configured.
Either implement the documented remote-cluster provisioning, assertions, and
cleanup using the existing E2E helpers, or retain the scenario as skipped until
that harness is available; remove the unconditional failure path.
- Around line 252-259: In the “fails Validated (no dial)” test loop, stop
discarding the result from applyGitTargetWithKubeConfig and assert that the
GitTarget is successfully admitted before calling verifyResourceCondition.
Preserve the existing validation-status assertion so the test reaches
controller-owned rejection cases rather than masking webhook or RBAC failures as
timeouts.
---
Outside diff comments:
In `@internal/git/plan_flush.go`:
- Around line 43-83: Add non-empty remote-cluster coverage for mapper wiring: in
internal/git/plan_flush.go lines 43-83, verify live events with a remote ID use
the remote lookup; in internal/git/worker_manager.go lines 100-107, verify
SetClusterMapper reaches newly created workers; in
internal/git/worker_manager.go lines 178-182, verify the injected worker field
is used after startup; in internal/git/resync_flush.go lines 292-341, cover
refusal scanning and resync application with a remote lookup; and in
internal/git/resync_flush_test.go line 68, extend the helper or add a test
accepting a non-empty cluster ID. Ensure the added Go behavior is covered
without reducing total coverage.
---
Nitpick comments:
In `@internal/controller/gittarget_source_cluster_test.go`:
- Line 76: Rename the table-driven test ValidateKubeConfig in
internal/controller/gittarget_source_cluster_test.go:76-76 to follow the
TestFunctionName_Scenario convention, such as
TestValidateKubeConfig_AllScenarios. Also rename GitProviderReadiness in
internal/controller/gittarget_source_cluster_test.go:144-144 to a matching
suffixed form, such as TestGitProviderReadiness_AllScenarios.
In `@internal/watch/source_cluster_resolver_test.go`:
- Around line 53-67: Apply scenario-qualified Go test names: rename
TestParseSourceClusterID to TestParseSourceClusterID_ValidAndMalformedIDs in
internal/watch/source_cluster_resolver_test.go:53-67; rename
TestForgetGitTargetCluster to
TestForgetGitTargetCluster_ReferenceCountedTeardown in
internal/watch/cluster_context_test.go:51-75; and add descriptive scenario
suffixes to TestClusterTypeLookup, TestClassifySourceClusterReachFailure, and
TestRecordClusterReachability in internal/watch/cluster_context_test.go:87-132,
preserving each test’s behavior.
🪄 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: 0c2f39f5-afe1-4507-b967-1ef8cf3b0d73
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (43)
.fossa.ymlapi/v1alpha3/gittarget_types.goapi/v1alpha3/zz_generated.deepcopy.gocharts/gitops-reverser/templates/validate-gittarget-kubeconfig-webhook.yamlcmd/main.goconfig/crd/bases/configbutler.ai_gittargets.yamlconfig/rbac/role.yamlconfig/webhook/validating-webhook.yamldocs/INDEX.mddocs/design/config-plane-split.mddocs/design/multi-cluster-audit-ingestion-implications.mdgo.modinternal/controller/gittarget_controller.gointernal/controller/gittarget_source_cluster.gointernal/controller/gittarget_source_cluster_test.gointernal/git/branch_worker.gointernal/git/pending_writes.gointernal/git/placement_test.gointernal/git/plan_flush.gointernal/git/render_fidelity_test.gointernal/git/resync_flush.gointernal/git/resync_flush_test.gointernal/git/types.gointernal/git/worker_manager.gointernal/kubeconfig/kubeconfig.gointernal/kubeconfig/kubeconfig_test.gointernal/watch/cluster_context.gointernal/watch/cluster_context_test.gointernal/watch/manager.gointernal/watch/manager_catalog.gointernal/watch/manager_snapshot_test.gointernal/watch/materialization.gointernal/watch/scope_resolve.gointernal/watch/source_cluster_resolver.gointernal/watch/source_cluster_resolver_test.gointernal/watch/stream_readiness.gointernal/watch/target_watch.gointernal/watch/target_watch_test.gointernal/watch/watched_type_resolver.gointernal/watch/watched_type_resolver_test.gointernal/webhook/gittarget_kubeconfig_handler.gointernal/webhook/gittarget_kubeconfig_handler_test.gotest/e2e/source_cluster_e2e_test.go
…harden data plane Product decision: all configuration is picked up on the reconcile loop, like every other resource — no validating webhook. Removes the fail-closed validate-gittarget-kubeconfig admission webhook (handler, chart template, SUT overlay, SAR RBAC); kubeconfig content validation already lives in the reconcile loop. The confused-deputy boundary is now namespace RBAC (secretRef resolves only from the GitTarget's own namespace, same as GitProvider.spec.secretRef). This also fixes the E2E (quickstart-install) failure: the fail-closed webhook rejected the README quickstart's local GitTarget while the operator pod was mid-rollout. Review findings: - P1(a): a deleted/invalid kubeconfig Secret now STOPS the mirror. The controller forgets a remote GitTarget's watch declaration on Validated=False, and the credential refresh drops cached clients fail-closed on a definitive credential failure (Secret gone / unparseable / unsafe), while staying tolerant of transient resolve errors. - P1(b): the watched-type refresh gate is now identity-aware. The summed registry revision is replaced by a fingerprint of each active cluster's (id, revision), plus a GitTarget->cluster mapping fingerprint, so a delete/recreate retarget can never reuse the previous cluster's watched-type table. - P2: WatchRule/ClusterWatchRule ResourcesResolved status resolves against the GitTarget's source-cluster registry, not the local one. CodeRabbit: - Critical: reject file-backed kubeconfig fields (tokenFile, client-certificate, client-key, certificate-authority) — client-go would read them from the operator Pod's filesystem; require the embedded *-data forms (unconditional). - Major: source-cluster REST config uses a dialer timeout, not rest.Config.Timeout, so remote watches are not cut off every interval; finite discovery gets a request timeout on a config copy. - Major: remote catalog refreshes run with bounded concurrency instead of serially, so one unreachable remote cannot delay all reconciliation. - Reject empty spec.kubeConfig.secretRef.name via CEL; rename controller tests to the _Scenario convention. 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 `@internal/controller/gittarget_controller.go`:
- Around line 411-412: Update the gitDest construction in the teardown flow
before ForgetGitTargetDeclaration to include target.UID via the same WithUID
pattern used when declaring the target in Reconcile, ensuring both resource
references match exactly.
In `@internal/watch/manager_catalog.go`:
- Around line 409-410: Protect the refreshClusterTypeRegistry call and its
update to cc.typeRefusalsLogged with resourceCatalogMu, ensuring status
resolution and catalog refresh cannot access the registry concurrently. Apply
the synchronization in the surrounding catalog refresh path while preserving the
existing cluster lookup and registry-refresh behavior.
🪄 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: 74d87e4f-0b13-413b-9a9f-8b2ee5a36ec1
📒 Files selected for processing (14)
api/v1alpha3/gittarget_types.gocmd/main.goconfig/crd/bases/configbutler.ai_gittargets.yamldocs/design/config-plane-split.mdinternal/controller/gittarget_controller.gointernal/controller/gittarget_source_cluster_test.gointernal/kubeconfig/kubeconfig.gointernal/kubeconfig/kubeconfig_test.gointernal/watch/cluster_context.gointernal/watch/config_plane_split_review_fixes_test.gointernal/watch/manager_catalog.gointernal/watch/source_cluster_resolver.gointernal/watch/watched_type_resolver.gointernal/watch/watched_type_resolver_test.go
💤 Files with no reviewable changes (1)
- cmd/main.go
🚧 Files skipped from review as they are similar to previous changes (9)
- internal/controller/gittarget_source_cluster_test.go
- config/crd/bases/configbutler.ai_gittargets.yaml
- internal/watch/source_cluster_resolver.go
- api/v1alpha3/gittarget_types.go
- internal/watch/watched_type_resolver_test.go
- internal/watch/watched_type_resolver.go
- internal/kubeconfig/kubeconfig_test.go
- docs/design/config-plane-split.md
- internal/watch/cluster_context.go
Comment on lines
+411
to
+412
| gitDest := types.NewResourceReference(target.Name, target.Namespace) | ||
| r.EventRouter.WatchManager.ForgetGitTargetDeclaration(gitDest) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Include the UID in the gitDest resource reference.
The gitDest constructed here is missing the .WithUID(string(target.UID)) call, which is used when declaring the target in Reconcile (around line 221).
If the watch manager relies on the UID for identification, state tracking, or map keys (e.g., gitDest.String()), omitting it during teardown will fail to match the active declaration. This could result in a resource leak where the remote mirror continues running on a dead or unsafe credential despite the target transitioning to a Validated=False state.
🐛 Proposed fix
- gitDest := types.NewResourceReference(target.Name, target.Namespace)
+ gitDest := types.NewResourceReference(target.Name, target.Namespace).WithUID(string(target.UID))
r.EventRouter.WatchManager.ForgetGitTargetDeclaration(gitDest)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| gitDest := types.NewResourceReference(target.Name, target.Namespace) | |
| r.EventRouter.WatchManager.ForgetGitTargetDeclaration(gitDest) | |
| gitDest := types.NewResourceReference(target.Name, target.Namespace).WithUID(string(target.UID)) | |
| r.EventRouter.WatchManager.ForgetGitTargetDeclaration(gitDest) |
🤖 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_controller.go` around lines 411 - 412, Update
the gitDest construction in the teardown flow before ForgetGitTargetDeclaration
to include target.UID via the same WithUID pattern used when declaring the
target in Reconcile, ensuring both resource references match exactly.
Comment on lines
+409
to
+410
| cc := m.cluster(m.clusterIDForGitTarget(gitDest)) | ||
| m.refreshClusterTypeRegistry(cc) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Serialize status-path registry publication.
refreshClusterTypeRegistry updates cc.typeRefusalsLogged, but this call occurs outside resourceCatalogMu. Concurrent status resolution and catalog refresh can race on that map, potentially causing a concurrent-map panic.
Proposed fix
cc := m.cluster(m.clusterIDForGitTarget(gitDest))
+m.resourceCatalogMu.Lock()
m.refreshClusterTypeRegistry(cc)
+m.resourceCatalogMu.Unlock()
reg := cc.registry📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| cc := m.cluster(m.clusterIDForGitTarget(gitDest)) | |
| m.refreshClusterTypeRegistry(cc) | |
| cc := m.cluster(m.clusterIDForGitTarget(gitDest)) | |
| m.resourceCatalogMu.Lock() | |
| m.refreshClusterTypeRegistry(cc) | |
| m.resourceCatalogMu.Unlock() |
🤖 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_catalog.go` around lines 409 - 410, Protect the
refreshClusterTypeRegistry call and its update to cc.typeRefusalsLogged with
resourceCatalogMu, ensuring status resolution and catalog refresh cannot access
the registry concurrently. Apply the synchronization in the surrounding catalog
refresh path while preserving the existing cluster lookup and registry-refresh
behavior.
…nctness) Replaces the dormant source-cluster scaffold with a real remote-mirror corner that proves source-cluster identity is load-bearing — no second k3d cluster needed. kcp (kcp-dev) is installed into the e2e cluster BY Flux, like every other dependency: a kcp-operator HelmRelease (dependsOn cert-manager) plus RootShard/FrontProxy/admin Kubeconfig CRs (test/e2e/setup/kcp, applied by hack/e2e/setup-kcp.sh). kcp gives cheap LOGICAL clusters — workspaces — each a real Kubernetes API a GitTarget mirrors via spec.kubeConfig, reached in-cluster at frontproxy-front-proxy.kcp.svc.cluster.local over verifiable TLS (the admin kubeconfig is fully embedded, so it passes the operator's kubeconfig safety checks unchanged). Specs (test/e2e/source_cluster_e2e_test.go + kcp_workspace_test.go): - Scenario 1 input validation now asserts the apply SUCCEEDS (CodeRabbit) and adds a file-path-credential case for the new KubeConfigFileReferenceNotAllowed rejection. - Scenario 4 mirrors a ConfigMap out of a real kcp workspace (was a self-referencing in-cluster fake that could not tell a local watch from a remote one). - Scenario 8 (centerpiece): three workspaces each hold the SAME namespace + resource name with different content; three GitTargets mirror them into three folders. Three distinct files with three distinct values prove state is keyed by SOURCE CLUSTER, not by (namespace, GVR) — the union bug the scaffold's Fail() placeholder described. Wiring: `task test-e2e-source-cluster` + a dedicated CI leg install kcp and run the `source-cluster` label (SOURCE_CLUSTER_GINKGO_PROCS=1). The corner is excluded from the default filter and full-core (like bi-directional), so the other legs never install kcp; the specs Skip when kcp is absent, so a default `task test-e2e` never turns them red. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
Review response — reconcile-loop model, data-plane hardening, real kcp e2eTwo commits: Product change: no validating webhook for kubeConfigRemoved the P1/P2 review findings
CodeRabbitAll 8 addressed (replies inline): the Critical file-path-credential exfiltration (reject e2e — the source-cluster corner runs on kcpInstead of a second k3d cluster, kcp (installed by Flux, like every other dep) gives cheap logical clusters — workspaces. The centerpiece spec brings up three workspaces holding the same namespace + resource name with different content; three GitTargets mirror them into three folders, and three distinct files prove state is keyed by source cluster, not Validation of the local gates (fmt/generate/manifests/vet/lint 0-issues, unit tests + coverage ratchet) is green; the kcp corner is being validated against a real cluster now. |
…ce harness Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The operator defaults both to 2; a single-node e2e runner shares the cluster with the whole gitops-reverser stack (gitea, valkey, prometheus, flux, cert-manager), so halve the kcp footprint. HA is not what the source-cluster specs test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… StreamsRunning The distinctness spec flaked: the mirror spec (Scenario 4) deleted its workspace but left its GitTarget pointing at the now-deleted workspace, and that dangling target churned the operator (failing discovery every reconcile) enough to starve the next spec's initial resync, so its writes never opened a commit window. - cleanupWorkspaceTarget tears down each spec's WatchRule + GitTarget + Secret + workspace in dependency order (target first, so the operator forgets the source cluster before its workspace disappears), between specs — not only at AfterAll. - The distinctness spec stands up all three mirrors, then asserts on the OUTCOME (three distinct files in Git), instead of gating each on a StreamsRunning condition that can lag its running stream. The 420s ceiling covers a periodic resync fallback. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ns up on credential change Two P1s from review, both aligned with the reconcile model (periodic re-check, no Secret watch): P1-a (starved status / flaky remote-mirror e2e): EnsureGitTargetWatches refreshed EVERY active cluster's catalog on the declare path, which runs on the single GitTarget controller worker. An unreachable source cluster's full dial timeout (15s) therefore blocked a HEALTHY target's own reconcile, so its StreamsRunning lagged past the e2e assertion. Now the declare path refreshes only the GitTarget's OWN cluster (refreshClusterForDeclare); the background RefreshAPIResourceCatalog loop still keeps every other cluster fresh and updates SourceClusterReachable. P1-b (revoked/rotated credential kept mirroring): dropClusterClients cleared the cache but never cancelled the already-running watch, and a still-valid rotation/repoint was not caught at all. The 30s catalog-refresh loop — which already re-reads the Secret — now invalidates the cluster's watches on any value CHANGE or definitive LOSS (invalidateClusterWatches: cancel each GitTarget's watch + enqueue it). The enqueued reconcile re-declares the target on the freshly rebuilt client (rotation) or holds it Validated=False (revocation). The GitTarget->cluster mapping is kept so re-declare targets the same cluster. dropClusterClients now reports whether it dropped, so invalidation fires once on the transition, not every 30s. No Secret watch — this rides the existing refresh cadence. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
808-824: 📐 Maintainability & Code Quality | 🔵 TrivialRun
task lint-actions. This catches actionlint issues in.github/workflows/ci.yml.🤖 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 @.github/workflows/ci.yml around lines 808 - 824, Run task lint-actions and resolve any actionlint findings in the source-cluster workflow entry, preserving its existing task command, coverage behavior, and k3d configuration.Source: Coding guidelines
🤖 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 `@test/e2e/Taskfile.yml`:
- Around line 1015-1019: Update the status task’s kcp readiness validation to
check that the etcd, root-kcp, and front-proxy workloads are available and
ready, rather than only confirming the persistent kcp-admin-kubeconfig Secret
exists. Keep the existing kcp.ready and modification-time checks, and make the
workload checks use the configured CTX and kcp namespace.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 808-824: Run task lint-actions and resolve any actionlint findings
in the source-cluster workflow entry, preserving its existing task command,
coverage behavior, and k3d configuration.
🪄 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: 8fac0f6c-fa16-4219-833c-1b9cf9e376d7
📒 Files selected for processing (16)
.github/workflows/ci.ymldocs/design/config-plane-split.mdhack/e2e/setup-kcp.shinternal/watch/cluster_context.gointernal/watch/config_plane_split_review_fixes_test.gointernal/watch/manager_catalog.gointernal/watch/target_watch.gotest/e2e/Taskfile.ymltest/e2e/kcp_workspace_test.gotest/e2e/setup/kcp/base/etcd.yamltest/e2e/setup/kcp/base/issuer.yamltest/e2e/setup/kcp/base/kcp-operator.yamltest/e2e/setup/kcp/base/kustomization.yamltest/e2e/setup/kcp/base/namespace.yamltest/e2e/setup/kcp/instance.yamltest/e2e/source_cluster_e2e_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/watch/cluster_context.go
- internal/watch/target_watch.go
- internal/watch/manager_catalog.go
Comment on lines
+1015
to
+1019
| status: | ||
| - test -f "{{.CS}}/kcp.ready" | ||
| - test -z "$(find "{{.KCP_DIR}}" hack/e2e/setup-kcp.sh -type f -newer "{{.CS}}/kcp.ready" -print -quit)" | ||
| - | | ||
| kubectl --context "{{.CTX}}" -n kcp get secret kcp-admin-kubeconfig >/dev/null 2>&1 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Verify the kcp workloads, not only the persistent Secret.
The Secret survives pod failures, so this status check can skip _kcp-ready while etcd, root-kcp, or the front-proxy is unavailable, producing a later opaque E2E failure.
Proposed fix
status:
- test -f "{{.CS}}/kcp.ready"
- test -z "$(find "{{.KCP_DIR}}" hack/e2e/setup-kcp.sh -type f -newer "{{.CS}}/kcp.ready" -print -quit)"
- |
kubectl --context "{{.CTX}}" -n kcp get secret kcp-admin-kubeconfig >/dev/null 2>&1
+ - kubectl --context "{{.CTX}}" -n kcp wait --for=condition=Ready pod -l app=etcd --timeout=1s
+ - kubectl --context "{{.CTX}}" -n kcp wait --for=condition=Available deployment/root-kcp --timeout=1s
+ - kubectl --context "{{.CTX}}" -n kcp wait --for=condition=Available deployment/frontproxy-front-proxy --timeout=1s📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| status: | |
| - test -f "{{.CS}}/kcp.ready" | |
| - test -z "$(find "{{.KCP_DIR}}" hack/e2e/setup-kcp.sh -type f -newer "{{.CS}}/kcp.ready" -print -quit)" | |
| - | | |
| kubectl --context "{{.CTX}}" -n kcp get secret kcp-admin-kubeconfig >/dev/null 2>&1 | |
| status: | |
| - test -f "{{.CS}}/kcp.ready" | |
| - test -z "$(find "{{.KCP_DIR}}" hack/e2e/setup-kcp.sh -type f -newer "{{.CS}}/kcp.ready" -print -quit)" | |
| - | | |
| kubectl --context "{{.CTX}}" -n kcp get secret kcp-admin-kubeconfig >/dev/null 2>&1 | |
| - kubectl --context "{{.CTX}}" -n kcp wait --for=condition=Ready pod -l app=etcd --timeout=1s | |
| - kubectl --context "{{.CTX}}" -n kcp wait --for=condition=Available deployment/root-kcp --timeout=1s | |
| - kubectl --context "{{.CTX}}" -n kcp wait --for=condition=Available deployment/frontproxy-front-proxy --timeout=1s |
🤖 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 1015 - 1019, Update the status task’s kcp
readiness validation to check that the etcd, root-kcp, and front-proxy workloads
are available and ready, rather than only confirming the persistent
kcp-admin-kubeconfig Secret exists. Keep the existing kcp.ready and
modification-time checks, and make the workload checks use the configured CTX
and kcp namespace.
…e restart The _webhook-tls-ready step restarts the k3d SERVER node; on a single-node CI runner that reschedules the whole control plane at once, heavier than the initial deploy (which already gets 300s). 180s was a slow-runner outlier for the new source-cluster leg (every other leg passed it on the same run). Bump to 300s and dump pod/deploy/events diagnostics on timeout so a genuine hang is debuggable. Headroom only affects a slow recovery; a healthy manager is back in under a minute. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
sunib
changed the title
This was referenced Jul 18, 2026
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.
Config-plane split —
GitTarget.spec.kubeConfigImplements feature #1 of the multi-tenant workstream, standalone: a
GitTargetmay name thecluster it mirrors from via an inline, optional, immutable
spec.kubeConfig— Flux'smeta.KubeConfigReferenceverbatim (the same fieldKustomization.spec.kubeConfiguses) — sothe operator reads its own config and Git credentials from the cluster it runs in while watching
resources on another, and one operator can mirror many clusters. Omitted means "the cluster I run
in": today's single-cluster behavior, byte-for-byte.
Spec:
docs/design/config-plane-split.md.Locked decisions honored
spec.kubeConfig= Fluxmeta.KubeConfigReference(nosourceClusterwrapper). Import only
github.com/fluxcd/pkg/apis/meta@v1.31.0— nopkg/runtime, nopkg/auth.secretRefonly;configMapRef(workload identity) is CEL-rejected as reserved.Validated=Falsewith a legiblereason), not Flux's silent strip. Opt-in via
--insecure-kubeconfig-exec/--insecure-kubeconfig-tls.Validatedextended (inputs, no dial) withKubeConfig*reasons; newSourceClusterReachable(runtime, data-plane);GitProviderReadyprojection folded intoReady.SubjectAccessReviewon the referenced Secret.Deliberate deltas from the closed PR #220 (per the design)
GitTargetproperty captured onDeclareForGitTarget(thegitTargetUIDspattern). BecausekubeConfigis immutable there is no"rules caught up?" window — feat: run gitops-reverser outside a single cluster (7 boundary features) #220's
CompiledSourceClusters/sourceClusterRulesCaughtUpraceapparatus is gone entirely.
unionLookup. Two clusters can serve one GVK under differentGVRs/scopes; a first-wins union mis-files/deletes manifests. Replaced with a cluster-scoped
writer lookup threaded via
Event/ResolvedTargetMetadata.SourceClusterID.value→value.yamlkey fallback (no schema default).Validated, reachability onSourceClusterReachable.Commits (mapped to the design's Build order)
feat(gittarget)— API + CRD: inline immutablespec.kubeConfig, immutability +configMapRef-reject CEL,SourceClusterID().feat(watch)—clusterContext(per-cluster catalog/registry/clients, refcounted teardown),SourceClusterResolver+internal/kubeconfig, capture-on-Declare, per-cluster resolution, target-scoped GVK→GVR, cmd/main wiring + flags.feat(gittarget)— status:ValidatedKubeConfig*reasons,SourceClusterReachable,GitProviderReadyprojection.feat(webhook)— fail-closed admission SAR onspec.kubeConfig.secretRef.docs— printer columns, minimal remote-read ClusterRole, INDEX.Scoping cut (documented)
Per-cluster trigger informers are kept local-only — a "refresh sooner than the 30s tick"
latency optimization. Remote catalog freshness rides the periodic refresh; per-cluster trigger
informers are a follow-up. Everything else (catalog, registry, clients) is fully per-cluster.
Testing
task fmt / generate / manifests / vet / lint / testall green; coverage within ratchet tolerance.internal/kubeconfig(key fallback + reject-unsafe), resolver (id parse/fallback/reject/version token),clusterContext(refcount teardown, per-cluster registry, reachability classification), controller (validateKubeConfigall five reasons,gitProviderReadiness), webhook (allow/deny/fail-closed SAR).task test-e2e(default suite, single-cluster integrity): running — will update.test/e2e/source_cluster_e2e_test.go, gatedE2E_ENABLE_SOURCE_CLUSTER) scenarios 1–4 exercise this feature; scenarios 5/6/7 remain design-documented follow-ups (their feature code is implemented + unit-tested).🤖 Generated with Claude Code
Summary by CodeRabbit
SourceReachableandProviderReady.