feat: run gitops-reverser outside a single cluster (7 boundary features)#220
Closed
sunib wants to merge 15 commits into
Closed
feat: run gitops-reverser outside a single cluster (7 boundary features)#220sunib wants to merge 15 commits into
sunib wants to merge 15 commits into
Conversation
…enant asks Umbrella doc plus per-feature designs for the changes that let gitops-reverser run outside the single-cluster shape it was built for: 1. config-plane / watched-cluster split (GitTarget.spec.sourceCluster) 2. identity-based write exclusion (excludeUsers / excludeFieldManagers) 3. --redis-key-prefix 4. public pkg/manifestanalyzer 5. CommitRequest spec.author with an assert-author RBAC guard 6. movable GitTarget destination (retarget) 7. graceful degradation without apiregistration.k8s.io Design only; implementation follows in this branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds multi-cluster GitTarget mirroring, mutable destination retargeting, rule-level write exclusions, asserted commit authors, configurable Redis key prefixes, and public manifest-analyzer JSON APIs. It updates Kubernetes schemas, controllers, watch routing, admission authorization, tests, Helm wiring, and documentation. ChangesAPI and CRD contracts
Multi-cluster watching and retargeting
Write exclusions and asserted authors
Redis and manifest-analyzer interfaces
End-to-end validation and documentation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CommitRequest
participant AdmissionWebhook
participant SubjectAccessReview
participant CommitRequestController
participant GitWorker
CommitRequest->>AdmissionWebhook: Submit spec.author
AdmissionWebhook->>SubjectAccessReview: Check assert-author permission
SubjectAccessReview-->>AdmissionWebhook: Return authorization verdict
AdmissionWebhook-->>CommitRequest: Allow or deny admission
CommitRequestController->>CommitRequestController: Verify recorded verdict
CommitRequestController->>GitWorker: Attach asserted author
GitWorker->>GitWorker: Create commit with asserted author and operator committer
sequenceDiagram
participant GitTargetController
participant WatchManager
participant SourceClusterResolver
participant RemoteCluster
GitTargetController->>SourceClusterResolver: Resolve kubeconfig Secret
SourceClusterResolver->>RemoteCluster: Discover source-cluster API
RemoteCluster-->>WatchManager: Return catalog and watchable resources
WatchManager->>RemoteCluster: Open list/watch streams
RemoteCluster-->>WatchManager: Stream source-cluster events
WatchManager-->>GitTargetController: Report stream readiness
sequenceDiagram
participant GitTargetController
participant WatchManager
participant RedisStore
participant GitMaterialization
GitTargetController->>WatchManager: Begin retarget
WatchManager->>RedisStore: Forget target watch cursors
GitTargetController->>GitMaterialization: Build fresh snapshot at new destination
GitMaterialization-->>GitTargetController: Snapshot accepted
GitTargetController->>GitTargetController: Set observedDestination and clear Retargeting
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! |
…urces The APIService trigger informer was created unconditionally. On an API server without an aggregation layer (kcp, and other non-aggregating control planes) apiregistration.k8s.io is not served at all, so client-go's reflector retried and logged forever — benign, endlessly repeated noise, which is exactly how a real error gets missed. Both trigger informers (CustomResourceDefinition, APIService) are now gated on discovery reporting the resource served with list+watch, and re-evaluated after every successful catalog refresh rather than once at startup. An aggregation layer installed later is picked up without a restart; an absent one is logged once, not once per refresh. The decision is extracted as the pure selectAPISurfaceTriggers so the no-aggregation case is testable without an API server. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Redis offers 16 logical databases, and --redis-db was the only separator this operator provided. An install that runs one reverser per tenant, or per branch environment, hits that ceiling long before it hits any real Redis limit. Every key (watch resume cursors, attribution facts, command author records) was rooted at the compile-time const "gitops-reverser". That root is now a RedisStoreConfig field threaded into RedisStore, AttributionIndex and CommandAuthorStore, and surfaced as --redis-key-prefix / queue.redis.keyPrefix. It defaults to the historic value, so an upgrade that sets nothing keeps reading the keys the previous release wrote. The prefix is validated rather than escaped: the attribution fact-index gauge SCANs "<prefix>:author:v1:audit:*", so a glob metacharacter in the prefix would silently count the wrong keyspace. '%' is rejected for the same reason against escapeKeyField. ':' is allowed (a prefix like "cell-a:tenant-7" nests naturally); a trailing ':' is normalized away. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The acceptance gate that decides "may this folder become a GitTarget?" is the same one the writer enforces before it commits a byte — which makes it the authoritative answer for an onboarding wizard. But Go forbids importing another module's internal/ tree, so a downstream tool could only exec the binary and parse a CLI output that carried no stability promise, or reimplement the rules and guarantee drift from the writer. pkg/manifestanalyzer publishes two entry points — ScanFolder (may this folder be adopted, and if not, why) and ScanRepo (which folders in this repository could be) — over its own JSON-tagged types carrying a SchemaVersion. It delegates every decision to internal/manifestanalyzer, so it cannot disagree with the operator; the public shape is a copy rather than a type alias, so the engine stays free to move. The CLI's --format json for both modes now goes through this package, which is what keeps the two contracts identical. That drops `plan` from scan-mode JSON (structurally always empty there — no cluster state, no desired resources) and adds schemaVersion; the internal RenderScanJSON/RenderRepoJSON are retired. Documented in docs/UPGRADING.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A reverser paired with a GitOps forward leg (Flux, Argo CD) on the same branch
commits its own forward leg's applies. The tool stamps its labels and
managedFields onto the object it applies; that is a live UPDATE; the reverser
mirrors it; the new commit re-triggers the tool. It terminates only because
content eventually stops changing — a convergence property standing in for a
missing invariant. Nothing on GitTargetSpec, WatchRuleSpec or ResourceRule could
express "this write is not interesting": the only filters were operations and
the type matchers.
ResourceRule and ClusterResourceRule gain two optional fields:
excludeFieldManagers — reads metadata.managedFields off the live object, so it
needs no audit fact, cannot race the attribution grace window, and works in
configured-author mode. The "last writer" is the manager of the newest
managedFields entry; a timestamp tie excludes only when every tied manager is
listed. NOT evaluated for DELETE, because managedFields names who last wrote
an object, not who deleted it — otherwise a human deleting a Flux-managed
resource would be silently ignored, the exact failure a label selector has.
excludeUsers — matches the attributed identity, so it needs --author-attribution.
An unresolved author fails open: losing a human's edit because we could not
identify its author is worse than mirroring one machine write. Logged once per
rule when attribution is off, since the clause can then never match.
Rules stay a logical OR, so an exclusion is a veto WITHIN its rule: an event is
routed when at least one rule both selects it and does not exclude its writer.
The watched-type table therefore keeps per-rule clauses instead of a merged
operation set, folded by exclusion fingerprint, and the watch spec covers the
exclusions so a rule edit restarts the affected watch.
Enforcement sits in routeLiveTargetWatchEvent, before the content dedup — an
excluded write must not seed the dedup cache with content that never reached Git
— and attribution is resolved early only when some clause declares excludeUsers,
then reused rather than looked up twice.
An exclusion suppresses a WRITE, never the state a mark-and-sweep reconciles
against: a replay/resync still lists the object, or the sweep would delete its
file from Git.
New metric: gitopsreverser_watch_events_excluded_total{gittarget_namespace,
gittarget_name, group, resource, reason=field_manager|user}.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Commit attribution is derived from an apiserver audit fact. That is the right
default — it names the actor who really caused a change and cannot be forged by
anyone who can create a CommitRequest. But it means attribution is only available
where an audit webhook can be configured, which excludes every hosted control
plane whose apiserver flags the operator does not own. An authenticated control
plane in front of the API already knows who the human is; making it re-derive
that identity through the audit stream is a long way around.
CommitRequestSpec gains spec.author {name, email}. Asserting an author is a
privilege, authorized by an RBAC verb on the target in the style of bind,
escalate and impersonate:
apiGroups: ["configbutler.ai"], resources: ["gittargets"],
resourceNames: ["tenants"], verbs: ["assert-author"]
The /validate-operator-types webhook issues a SubjectAccessReview for the
requester and denies an unauthorized create — the legible error. But that webhook
is failurePolicy: Ignore by design, so a guard living only there would be
bypassable by taking it down. The controller is the real gate: it honors
spec.author only when an admission record exists for the object's UID AND that
record carries the authorized verdict. No record — webhook off, bypassed, or no
Redis — means the assertion is ignored, the commit is authored by the configured
committer, and the request reports AuthorAttributed=False with reason
AuthorAssertionUnverified. Fail-closed, independent of failurePolicy.
An asserted author differs from the existing window-matching Author in both
directions: it binds to ANY open window for the GitTarget (the assertion is a
statement about the commit being made, not a claim to be whichever actor the
audit stream recorded), and it becomes the commit's author signature rather than
merely selecting a window. The committer stays the operator's configured
identity, so a reader can always tell the reverser committed on someone's behalf.
Adds the first authorizationv1 client in the repo, and the RBAC to create
SubjectAccessReviews. Without it every assertion is denied: an unverifiable
privilege is not a granted one.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Both were immutable, and the reasoning was sound: a GitTarget materializes at exactly one (provider, branch, folder), and letting that move would orphan the old materialization and silently invalidate the initial-snapshot gate. The cost fell on anyone who ever repoints a target — an operator that stamps a GitTarget with a default branch or folder before the user has chosen one has frozen the object against that default, and its only recovery is to catch the admission rejection and delete-and-recreate. The invariant is preserved by making the destination a thing status OBSERVES rather than a thing the spec cannot change: a successful snapshot is valid for the destination recorded in status.observedDestination. When spec and observedDestination disagree, the snapshot is by definition stale, and the GitTarget says so (Retargeting=True). On a destination change the controller tears the old materialization down BEFORE anything else runs. That ordering is the whole safety property: the writer reads spec.path fresh per write while the branch worker is bound to the branch its event stream was registered against, so a live event arriving mid-move would otherwise land at the new path on the OLD branch. Teardown cancels the watches, unregisters the stream, and — the load-bearing part — drops the durable resume cursors, which are keyed by GitTarget UID and would otherwise make the new folder receive only the changes that happen after the move. The rebuild then runs the ordinary Validated gate (allowedBranches, path overlap) against the new destination and drives a full replay into it. Teardown is generation-scoped, so a second destination change arriving mid-move tears down again rather than quietly continuing to build the first one. The old folder is never deleted. Deleting from Git is the one irreversible thing this operator can do, and a destination change is the moment an operator is least sure of what they meant; the path being left may also already have a new owner. The Retargeting=False message names the abandoned folder so it can be removed by hand. spec.providerRef stays immutable: a different repository is a different object, with nothing to migrate and nothing to observe. Its CEL message now points at the supported move rather than just refusing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The operator built exactly one client from ctrl.GetConfigOrDie(), and that single
rest.Config served two jobs that are conceptually unrelated: reading its own CRs
and Git credentials (the config plane), and watching the resources it mirrors
(the watched cluster). Because GitProvider.spec.secretRef is a local reference and
WatchRule.spec.targetRef must name a same-namespace GitTarget, all four objects had
to sit in one namespace ON THE CLUSTER BEING WATCHED. Nothing chose this; it fell
out of having one kubeconfig. For anyone mirroring a cluster they also hand to
someone else, a Git write credential — usually scoped far wider than the one repo a
GitTarget names — then lives one RBAC rule away from that cluster's users.
GitTarget gains spec.sourceCluster.kubeConfigSecretRef, shaped like Flux's
Kustomization.spec.kubeConfig. It sits on GitTarget rather than WatchRule because a
GitTarget already owns exactly one materialization; adding the source makes it one
(cluster, provider, branch, folder). On WatchRule, two rules could name different
clusters for one folder and the mark-and-sweep would alternately delete each
cluster's objects.
The watch manager grew a clusterContext: the API catalog, type registry, clients,
and API-surface trigger informers that were Manager-wide singletons are in fact
properties of ONE cluster. A zero-value Manager creates exactly one, keyed
LocalClusterID, and every single-cluster install behaves as before.
- Watched-type tables resolve a GitTarget's rules against ITS cluster's registry.
A type served only locally never resolves for a remote target: mirroring the
wrong cluster into a folder is worse than mirroring none.
- RefreshAPIResourceCatalog refreshes every active cluster but returns the local
cluster's error only, so an unreachable remote fails its own GitTargets rather
than everyone's.
- The writer's GVK lookup is a union over live registries (local first). Branch
workers are keyed by (provider, branch) and shared across GitTargets that may
mirror different clusters, so it cannot be per-target.
- The kubeconfig Secret is read on demand, parsed, and dropped; only its
resourceVersion is kept, so a rotation rebuilds the clients exactly once. No
Secret informer, per docs/future/secret-value-retention-plan.md.
- sourceCluster is part of the destination identity, so changing it retargets.
Rotating the Secret's contents does not.
The rule store's six-positional-string AddOrUpdate signature — where a transposed
pair would have silently mirrored the wrong folder — becomes a TargetBinding built
by one shared constructor.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…thor write_exclusion_e2e_test.go stands in for a GitOps forward leg with `kubectl apply --server-side --field-manager=kustomize-controller` plus the labels such a tool stamps — which is exactly what one does, and the field manager is the whole signal excludeFieldManagers reads. Two GitTargets watch the same ConfigMaps into sibling folders, one declining the forward leg's writes and one not. The control target is what makes the assertion sharp: once the forward leg's labels appear in the mirrored folder the event has demonstrably been processed, so their absence in the excluded folder is the exclusion working rather than a race we did not wait long enough for. A second spec pins that a DELETE of an object the excluded manager last wrote is still mirrored. gittarget_retarget_e2e_test.go pins the three things only an e2e can show together: the new folder receives the resources that ALREADY existed (which is what dropping the resume cursors buys), the old folder is left in place, and status.observedDestination follows the content rather than the spec. asserted_author_e2e_test.go grants assert-author to one user and not another, scoped by resourceNames to a single GitTarget, and pins that the denial names the RBAC rule that would grant it, that an authorized assertion becomes the commit's author signature, and that the committer stays the operator. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
configuration.md gains the four user-facing sections: mirroring a remote cluster (spec.sourceCluster), moving a GitTarget (retarget), ignoring a GitOps forward leg's own writes (excludeFieldManagers / excludeUsers), asserting a commit author, and sharing one Redis with --redis-key-prefix. security-model.md gains the two new trust boundaries — the source-cluster kubeconfig Secret, and the assert-author verb, which is to be treated exactly like impersonate — plus why keeping Git credentials off the watched cluster was the point of the config-plane split. architecture.md's mental model names the clusterContext, and the package map picks up pkg/manifestanalyzer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…sage The e2e caught it: settleDestination named the folder a retarget abandoned, and then the next periodic reconcile overwrote that message with the steady-state "materialized at <dest>". The abandoned folder is the one thing a retarget leaves behind and nothing deletes, and that message was the only place in status where an operator learned which folder it was. settleDestination now returns early once the condition is already settled at the current destination, so the message survives every later reconcile, and it emits a Retargeted Kubernetes event carrying the same fact for anyone watching the object rather than reading its status. A second retarget re-settles and names the freshly abandoned folder. Also: the asserted-author e2e gives its CommitRequest a closeDelaySeconds collect window. Without one, a request created immediately after an edit resolves NoWindowInGrace — the edit's watch event has not reached the branch worker yet, so there is no open window to attach to. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The first e2e run found a real gap in my own premise: internal/sanitize already strips kustomize.toolkit.fluxcd.io/* labels and annotations (along with kro.run/, applyset.kubernetes.io/, managedFields, and the server-generated metadata) from Git content. So an apply that only stamps a Flux label carries no Git-writable change at all, and the content dedup drops it as a no-op long before any exclusion is consulted — which is why the control target never showed the label either. The forward leg now applies a different `data` value, which is what actually happens whenever Git and the cluster disagree about a managed field. That is the case the exclusion decides, and the control target proves the event was processed. Pinned as a unit test (OperationalLabelsAreNotGitContent) and written into the design doc and configuration.md, because it changes what an operator should read into a zero rate on gitopsreverser_watch_events_excluded_total: a zero rate does not by itself mean the exclusion is misconfigured. The dedup-cache test also now uses distinct content, so "an excluded write must not seed the dedup cache" is a real assertion rather than one that would pass either way. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The design doc claimed a retarget onto a conflicting path leaves the target "continuing to serve the old destination". It does not, and cannot: the teardown happens before the Validated gate runs, precisely so a live event can never be written to the new path on the old branch. A refused retarget therefore mirrors nothing until a free destination is chosen. That is the price of the ordering, and it is the right price — but the doc should say so rather than promise a fallback the code does not implement. Also documents the generation-scoped teardown, the resume-cursor drop, and the Retargeted event; and fixes the observedDestination.sourceCluster example (it is <namespace>/<name>/<key>) and the quoted CEL message to match the real one. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two bugs this branch introduced, one it exposed, and two hot-path costs. 1. gitTargetLosesConflict: once spec.path became mutable, a GitTarget's creation timestamp stopped being a faithful "who claimed this folder first". An OLDER target retargeting ONTO a YOUNGER incumbent's folder won the overlap conflict and evicted a bystander that never changed. Claim STRENGTH now beats age: a target is established when status.observedDestination agrees with its spec, and pending when it is asking to move. Established beats pending; creation time only breaks ties between equal claims, exactly as before. 2. The source cluster reaches the data plane through the compiled rules, and a rule recompiles on its GitTarget's generation bump — a race the GitTarget's own reconcile can win. Two consequences, both fixed: the watch-spec fingerprint now covers the cluster (the watch key is identical across clusters, so repointing a GitTarget reused the running watch, still bound to the old cluster), and the controller waits for every rule to agree with spec.sourceCluster before declaring. A table whose rules disagree resolves to no types at all: mirroring the wrong cluster into a folder is worse than mirroring none. 3. A destination change arriving before a previous one settled orphaned the folder that move had begun building — observedDestination still names the original, so nothing ever named the intermediate. status.retargetingTo tracks the in-flight destination, and a RetargetSuperseded event names any folder abandoned that way, including on a mid-move revert. 4. refreshRunningTargetWatches could resurrect a GitTarget's watch set that was torn down (deleted or retargeted) between snapshotting the running keys and re-taking the lock, leaking goroutines that write stream state after teardown. The refresh path now refuses to CREATE a set. 5. Two hot paths: unionLookup.ByGVK took clustersMu once per Git document the writer scans (now an atomic snapshot published when the cluster set changes), and every watch reconnect re-read the remote cluster's kubeconfig Secret under a global mutex (now a per-cluster mutex, with rotation detected on the catalog-refresh cadence instead). Also corrects two API doc comments that promised behavior the code does not have — a refused retarget does NOT keep serving its old destination, because the teardown precedes validation by design — and documents that excludeFieldManagers means "this manager's writes never cause a commit", not "this manager's changes never reach Git": the next non-excluded write to the object commits its whole current state. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…each An AuthorAssertionUnverified request degrades to the pre-existing committer-fallback path: it carries no author, so it finalizes whichever unattributed open window exists for its GitTarget. It cannot reach a window the audit stream attributed to a named actor, and the commit is never signed with the rejected identity. Same reach a CommitRequest with no spec.author from an unattributed submitter has always had; the guard denies the identity claim, not the ability to save. Worth saying out loud in the threat model. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
sunib
marked this pull request as ready for review
July 9, 2026 23:12
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/controller/gittarget_controller.go (1)
321-328: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the validation success message to include source cluster.
The
Validated=Truemessage says "Provider, branch, and placement policy validation passed" butevaluateValidatedGatenow also validates the source cluster kubeconfig (lines 307–319). An operator seeing this message might not realize the source cluster was checked. The failure path correctly namesSourceClusterUnreachable, but the success path should also reflect all validations that passed.✏️ Proposed fix
- "Provider, branch, and placement policy validation passed", + "Provider, branch, placement policy, and source cluster validation passed",🤖 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 321 - 328, Update the success message in evaluateValidatedGate to mention source cluster validation alongside provider, branch, and placement policy validation, while preserving the existing Validated=True condition and reason.
🧹 Nitpick comments (1)
internal/git/commit_request_attach_loop.go (1)
83-99: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: extract the nil-check-and-extract-username pattern into a helper.
The nil guard before accessing
.Usernameis duplicated inhandleAttachCommitRequest(lines 92–95) andattachToOpenWindow(lines 146–149). A small helper would keep the two call sites in sync if theAssertedAuthortype changes.The static analysis hint about log injection (CWE-117) is a false positive: values are passed as separate structured key-value pairs to
logr.Logger.Info, not concatenated into the message string, and the logged values are Kubernetes-validated identifiers.♻️ Optional helper extraction
+// assertedAuthorUsername returns the username from a possibly-nil AssertedAuthor. +func assertedAuthorUsername(a *AssertedAuthor) string { + if a == nil { + return "" + } + return a.Username +} + func (l *branchWorkerEventLoop) handleAttachCommitRequest(req *AttachCommitRequest) { // ... - assertedAuthor := "" - if req.AssertedAuthor != nil { - assertedAuthor = req.AssertedAuthor.Username - } + assertedAuthor := assertedAuthorUsername(req.AssertedAuthor) l.w.Log.Info("CommitRequest registered with worker", "request", id.Namespace+"/"+id.Name, "author", req.Author, "assertedAuthor", assertedAuthor, "target", req.GitTargetNamespace+"/"+req.GitTargetName, "closeDelaySeconds", req.CloseDelaySeconds) } func (l *branchWorkerEventLoop) attachToOpenWindow(pcr *pendingCommitRequest) { l.openWindow.pendingMessage = pcr.message l.openWindow.pendingAuthor = pcr.assertedAuthor id := pcr.id l.openWindow.pendingCR = &id pcr.attached = true - assertedAuthor := "" - if pcr.assertedAuthor != nil { - assertedAuthor = pcr.assertedAuthor.Username - } + assertedAuthor := assertedAuthorUsername(pcr.assertedAuthor) l.w.Log.Info("CommitRequest attached to open window", "request", id.Namespace+"/"+id.Name, "author", pcr.author, "assertedAuthor", assertedAuthor, "windowAuthor", l.openWindow.Author, "target", pcr.gitTargetNamespace+"/"+pcr.gitTargetName) }Also applies to: 138-154
🤖 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/commit_request_attach_loop.go` around lines 83 - 99, Optionally extract the duplicated nil-check and username extraction into a small helper, then use it in both handleAttachCommitRequest and attachToOpenWindow to keep AssertedAuthor handling consistent.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 `@internal/watch/manager_catalog.go`:
- Around line 390-396: Update refreshClusterCredentials to also set
cc.triggerFactory to nil when clearing the cached dynamic and discovery clients,
ensuring trigger informers are recreated with the rotated credentials.
---
Outside diff comments:
In `@internal/controller/gittarget_controller.go`:
- Around line 321-328: Update the success message in evaluateValidatedGate to
mention source cluster validation alongside provider, branch, and placement
policy validation, while preserving the existing Validated=True condition and
reason.
---
Nitpick comments:
In `@internal/git/commit_request_attach_loop.go`:
- Around line 83-99: Optionally extract the duplicated nil-check and username
extraction into a small helper, then use it in both handleAttachCommitRequest
and attachToOpenWindow to keep AssertedAuthor handling consistent.
🪄 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: 1cd09017-31c6-4565-988d-373c98261bcc
📒 Files selected for processing (103)
.coverage-baselineapi/v1alpha3/clusterwatchrule_types.goapi/v1alpha3/commitrequest_types.goapi/v1alpha3/gittarget_types.goapi/v1alpha3/watchrule_types.goapi/v1alpha3/zz_generated.deepcopy.gocharts/gitops-reverser/templates/deployment.yamlcharts/gitops-reverser/templates/validate-operator-types-webhook.yamlcharts/gitops-reverser/values.yamlcmd/main.gocmd/main_audit_server_test.gocmd/manifest-analyzer/main.goconfig/crd/bases/configbutler.ai_clusterwatchrules.yamlconfig/crd/bases/configbutler.ai_commitrequests.yamlconfig/crd/bases/configbutler.ai_gittargets.yamlconfig/crd/bases/configbutler.ai_watchrules.yamlconfig/rbac/role.yamlconfig/webhook/validating-webhook.yamldocs/README.mddocs/UPGRADING.mddocs/architecture.mddocs/configuration.mddocs/design/gitops-api/f8-repo-discovery-and-onboarding-scan.mddocs/design/multi-tenant/README.mddocs/design/multi-tenant/asserted-commit-author.mddocs/design/multi-tenant/config-plane-split.mddocs/design/multi-tenant/gittarget-retarget.mddocs/design/multi-tenant/identity-write-exclusion.mddocs/security-model.mdinternal/controller/clusterwatchrule_controller.gointernal/controller/commitrequest_asserted_author_test.gointernal/controller/commitrequest_controller.gointernal/controller/commitrequest_finalize.gointernal/controller/gittarget_controller.gointernal/controller/gittarget_immutability_test.gointernal/controller/gittarget_path_overlap.gointernal/controller/gittarget_path_overlap_test.gointernal/controller/gittarget_retarget.gointernal/controller/gittarget_retarget_test.gointernal/controller/gittarget_source_cluster.gointernal/controller/watchrule_controller.gointernal/git/asserted_author_test.gointernal/git/branch_worker.gointernal/git/commit_request_attach.gointernal/git/commit_request_attach_loop.gointernal/git/open_window.gointernal/git/pending_writes.gointernal/git/types.gointernal/manifestanalyzer/render.gointernal/manifestanalyzer/render_test.gointernal/manifestanalyzer/repowalk.gointernal/manifestanalyzer/repowalk_test.gointernal/queue/attribution_index.gointernal/queue/command_author_store.gointernal/queue/key_prefix.gointernal/queue/key_prefix_test.gointernal/queue/redis_store.gointernal/queue/watch_cursor_forget_test.gointernal/rulestore/store.gointernal/rulestore/store_test.gointernal/telemetry/exporter.gointernal/watch/api_resource_catalog.gointernal/watch/api_resource_catalog_test.gointernal/watch/api_surface_triggers_test.gointernal/watch/author_resolver.gointernal/watch/bootstrap.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/retarget_test.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_helpers_test.gointernal/watch/watched_type_metrics_test.gointernal/watch/watched_type_resolver.gointernal/watch/watched_type_resolver_test.gointernal/watch/watched_type_table.gointernal/watch/watched_type_table_test.gointernal/watch/write_exclusion.gointernal/watch/write_exclusion_routing_test.gointernal/watch/write_exclusion_test.gointernal/watch/write_exclusion_test_helpers_test.gointernal/webhook/author_assertion.gointernal/webhook/author_assertion_test.gointernal/webhook/validate_operator_types_handler.gopkg/manifestanalyzer/doc.gopkg/manifestanalyzer/folder.gopkg/manifestanalyzer/folder_test.gopkg/manifestanalyzer/repo.gopkg/manifestanalyzer/repo_test.gotest/e2e/asserted_author_e2e_test.gotest/e2e/gittarget_retarget_e2e_test.gotest/e2e/write_exclusion_e2e_test.gotest/mutationlab/e2e/configmap_scenarios_test.gotest/mutationlab/e2e/crd_conversion_test.gotest/mutationlab/e2e/workload_scenarios_test.go
💤 Files with no reviewable changes (3)
- internal/manifestanalyzer/repowalk.go
- internal/manifestanalyzer/render_test.go
- internal/manifestanalyzer/render.go
Comment on lines
+390
to
+396
| if cc.triggerFactory == nil { | ||
| dynamicClient, err := m.clusterDynamicClient(ctx, cc.id) | ||
| if err != nil { | ||
| log.V(1).Info("skipping API surface trigger informers - no client available", "err", err.Error()) | ||
| return | ||
| } | ||
| cc.triggerFactory = dynamicinformer.NewDynamicSharedInformerFactory(dynamicClient, 0) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether refreshClusterCredentials clears triggerFactory or
# otherwise ensures the factory's dynamic client stays valid after credential rotation.
# First, find the definition of refreshClusterCredentials
rg -n --type=go 'func \(m \*Manager\) refreshClusterCredentials' internal/watch/
# Then check if triggerFactory is ever reset to nil outside of ensureAPISurfaceTriggerInformers
rg -n --type=go -C3 'triggerFactory\s*=\s*nil' internal/watch/
# Also check the clusterContext struct definition for triggerFactory
rg -n --type=go -C2 'triggerFactory' internal/watch/cluster_context.goRepository: ConfigButler/gitops-reverser
Length of output: 602
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the credential refresh path and any triggerFactory lifecycle handling.
sed -n '220,340p' internal/watch/cluster_context.go
printf '\n---\n'
sed -n '340,460p' internal/watch/manager_catalog.go
printf '\n---\n'
# Find every place triggerFactory is assigned or cleared.
rg -n --type=go -C 3 'triggerFactory\s*=' internal/watch/Repository: ConfigButler/gitops-reverser
Length of output: 9987
Reset cc.triggerFactory on credential rotation. refreshClusterCredentials clears the cached dynamic client and discovery client, but the informer factory stays bound to the old client. Clear it too so CRD/APIService trigger informers rebuild with the rotated credentials.
🤖 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 390 - 396, Update
refreshClusterCredentials to also set cc.triggerFactory to nil when clearing the
cached dynamic and discovery clients, ensuring trigger informers are recreated
with the rotated credentials.
This was referenced Jul 10, 2026
feat(rbac)!: least-privilege RBAC, a strict chart values schema, and informers that stop on 403
#223
Merged
sunib
added a commit
that referenced
this pull request
Jul 17, 2026
…ote-cluster mirroring (#249) * docs(config-plane-split): land design doc, e2e scaffold, and index updates 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> * feat(gittarget): add immutable spec.kubeConfig (Flux meta.KubeConfigReference) 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> * feat(watch): per-source-cluster data plane for GitTarget.spec.kubeConfig 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> * feat(gittarget): status conditions for spec.kubeConfig (Validated, SourceClusterReachable, 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> * feat(webhook): fail-closed admission SAR on GitTarget.spec.kubeConfig.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> * docs(config-plane-split): printer columns, minimal remote-read ClusterRole, 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> * test(e2e): fix source-cluster scaffold key indentation (8->6 spaces) 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> * fix(config-plane-split): address PR review — drop admission webhook, 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> * test(e2e): source-cluster corner on kcp workspaces (3-workspace distinctness) 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> * docs(config-plane-split): e2e test plan now describes the kcp-workspace harness Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(e2e): pin kcp root shard + front-proxy to a single replica 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> * test(e2e): isolate source-cluster mirror specs, assert on outcome not 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> * fix(watch): declare refreshes only its own cluster; refresh loop cleans 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> * test(e2e): give the manager 300s to recover after the webhook-TLS node 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> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GitOps Reverser was designed for one operator, in one cluster, watching that cluster, writing to Git. Every assumption that shape allows was taken. This PR moves seven of those edges.
They are independent features; each is useful on its own. One commit each, plus design docs, user docs, and e2e coverage for the three with user-visible behavior.
GitTarget.spec.sourceCluster.kubeConfigSecretRefexcludeFieldManagers,excludeUsers--redis-key-prefixpkg/manifestanalyzerCommitRequest.spec.authorwith anassert-authorRBAC guardGitTargetdestination (retarget)apiregistration.k8s.io#2 is the one to read first
It is not a niche problem. A reverser paired with a GitOps forward leg on the same branch commits its own forward leg's applies: the tool stamps its labels and
managedFieldsonto the object it applies, that is a live UPDATE, the reverser mirrors it, which re-triggers the tool. It terminates only because content eventually stops changing — a convergence property standing in for a missing invariant. Before this PR there was no identity-based filter anywhere onGitTargetSpec,WatchRuleSpecorResourceRule.A label selector cannot fix it: a GitOps tool's labels persist on the object, so a selector would also ignore a human's later edit of a tool-managed resource.
excludeFieldManagersreads the last writer offmanagedFields, which is a property of the write rather than of the object.Design decisions worth reviewing:
DELETEis never excluded by field manager.managedFieldsnames who last wrote an object, not who deleted it — otherwise a human deleting a Flux-managed resource is silently ignored, the exact failure a label selector has.excludeUsershandles deletes.Notable design calls in the rest
#1 puts
sourceClusteronGitTarget, notWatchRule, because aGitTargetalready owns exactly one materialization. OnWatchRule, two rules could name different clusters for one folder and the mark-and-sweep would alternately delete each cluster's objects. The watch manager grew aclusterContext— the API catalog, type registry, clients, and trigger informers that were Manager-wide singletons are properties of one cluster. A single-cluster install creates exactly one and behaves as before. A type served only locally never resolves for a remote target: mirroring the wrong cluster into a folder is worse than mirroring none.#5 — the
/validate-operator-typeswebhook isfailurePolicy: Ignoreby design, so a guard living only there would be bypassable by taking it down. The webhook issues theSubjectAccessReviewand denies (the legible error), but the controller is the real gate: it honorsspec.authoronly against an admission record carrying the authorized verdict. No record — webhook off, bypassed, or no Redis — means the assertion is ignored, not honored. Fail-closed, independent offailurePolicy.#6 — the destination becomes a thing status observes (
status.observedDestination) rather than a thing the spec cannot change, which preserves the initial-snapshot invariant. Teardown happens before validation: the writer readsspec.pathfresh per write while the branch worker is bound to the branch its stream was registered against, so a live event arriving mid-move would otherwise land at the new path on the old branch. It drops the durable resume cursors, without which the new folder would only ever receive changes that happen after the move. The old folder is never deleted — deleting from Git is the one irreversible thing this operator does.#4 — the CLI's
--format jsonnow goes throughpkg/manifestanalyzer, which is what keeps the published contract and the printed one identical. That dropsplanfrom scan-mode JSON (structurally always empty there) and addsschemaVersion. Seedocs/UPGRADING.md.Breaking changes
Both documented in
docs/UPGRADING.md:manifest-analyzer --mode scan --format jsonno longer carriesplan, and gainsschemaVersion.GitTarget.spec.branch/.spec.pathare now mutable. An apply that used to be rejected now retargets.What the tests caught
The e2e specs and a multi-angle self-review found five things worth calling out, all fixed in this branch:
excludeFieldManagersis narrower than it looks.internal/sanitizealready stripskustomize.toolkit.fluxcd.io/*labels and annotations from Git content, so an apply that only stamps them carries no change to mirror and is dropped as a no-op before any exclusion is consulted. The exclusion decides the case where the forward leg's apply changes real content. My first e2e assumed otherwise and passed for the wrong reason. Now pinned as a unit test and documented — including that a zero rate on the excluded-events metric does not by itself mean the exclusion is misconfigured.A mover could evict an incumbent. Once
spec.pathis mutable, creation time stops being a faithful "who claimed this folder first": an older GitTarget retargeting onto a younger incumbent's folder won the overlap conflict. Claim strength now beats age — established (spec agrees withobservedDestination) beats pending (asking to move) — and creation time only breaks ties between equal claims.A
sourceClusterchange would not restart the watches. The watch key(GVR, namespace)is identical across clusters, so the spec fingerprint compared equal and the running watch stayed bound to the old cluster. The fingerprint now covers the cluster, the controller waits for every rule to agree withspec.sourceClusterbefore declaring, and a table whose rules disagree resolves to no types at all.The abandoned-folder name was transient. The
Retargeting=Falsemessage naming the folder a move left behind was overwritten by the next periodic reconcile — the only place in status an operator learns which folder is now unmanaged Git content. It now survives, plus aRetargetedevent. Andstatus.retargetingTo+ aRetargetSupersededevent close the case where a second destination change orphans the folder the first move had begun building.Two hot paths.
unionLookup.ByGVKtook a mutex once per Git document the writer scans; every watch reconnect re-read the remote kubeconfig Secret under a global lock. Both fixed.Validation
task lintcleantask testclean (coverage 75.7%, baseline raised)task test-e2e— 61 passed, 0 failed, including three new spec files (six specs):write_exclusion_e2e_test.go,gittarget_retarget_e2e_test.go,asserted_author_e2e_test.go🤖 Generated with Claude Code
Summary by CodeRabbit