From c82f7f6de4bbb5a3066618c17345057040f15a0c Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Mon, 7 Sep 2026 11:31:44 +0000 Subject: [PATCH 1/6] fix(watchrule): re-enqueue a rule when the GitTarget readiness it mirrors moves WatchRule and ClusterWatchRule mirror their GitTarget's readiness into their own GitTargetReady condition, but watched the GitTarget through GenerationChangedPredicate, which drops status-only updates. A GitTarget publishes a heal as exactly such an update, so the mirrored condition stayed frozen on the failing snapshot until the rule's own periodic requeue. That left the object contradicting itself for up to a full requeue interval: StreamsRunning=True with 2/2 streams (the stream-state channel tells the rule directly) beside GitTargetReady=False reporting 1/2 and one blocked. It fails safe, but an alert on rule readiness would fire for minutes on every watch reconnect, so the field could not be used for monitoring. Replace that predicate on the GitTarget edge with gitTargetReadyProjectionChanged, which fires on a spec change or on a move in the mirrored verdict. It compares Status and Reason only: Message is free-form and moves with transient stream detail, so including it would re-enqueue every rule of a target on churn that changes no verdict, and a message-only drift self-corrects on the next Status or Reason move. The two comments claiming the GitTarget edge cannot carry a ClusterProvider change are updated: it still cannot be relied on, but the reason is now that only a readiness move re-enqueues, not that every status update is dropped. Co-Authored-By: Claude Opus 5 --- .../controller/clusterwatchrule_controller.go | 18 +++-- .../controller/gittarget_dependency_status.go | 38 ++++++++++ .../gittarget_dependency_status_test.go | 76 +++++++++++++++++++ internal/controller/watchrule_controller.go | 23 +++--- 4 files changed, 139 insertions(+), 16 deletions(-) create mode 100644 internal/controller/gittarget_dependency_status_test.go diff --git a/internal/controller/clusterwatchrule_controller.go b/internal/controller/clusterwatchrule_controller.go index c4d5293c..c0925dd0 100644 --- a/internal/controller/clusterwatchrule_controller.go +++ b/internal/controller/clusterwatchrule_controller.go @@ -389,16 +389,19 @@ func (r *ClusterWatchRuleReconciler) SetupWithManager(mgr ctrl.Manager) error { // already suppresses no-op writes, so the loop has no fuel; this makes it structural, and // matches what GitProvider and ClusterProvider already do. For(&configbutleraiv1alpha3.ClusterWatchRule{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})). - // GenerationChangedPredicate keeps these watches reacting to a freshly - // applied or spec-changed dependency while ignoring the status-only - // updates the controllers write themselves — without it every GitTarget - // or GitProvider heartbeat would re-list and re-enqueue all - // ClusterWatchRules. + // This rule MIRRORS the GitTarget's readiness into its own GitTargetReady, so the watch has + // to see the status-only update that carries a heal — GenerationChangedPredicate would drop + // exactly that. gitTargetReadyProjectionChanged fires on a spec change or on a move in the + // mirrored verdict, and on nothing else. Watches( &configbutleraiv1alpha3.GitTarget{}, handler.EnqueueRequestsFromMapFunc(r.gitTargetToClusterWatchRules), - builder.WithPredicates(predicate.GenerationChangedPredicate{}), + builder.WithPredicates(gitTargetReadyProjectionChanged()), ). + // Nothing of the GitProvider's STATUS is mirrored here, so GenerationChangedPredicate is + // right on this edge: it reacts to a freshly applied or spec-changed provider while ignoring + // the status-only updates its controller writes itself — without it every GitProvider + // heartbeat would re-list and re-enqueue all ClusterWatchRules. Watches( &configbutleraiv1alpha3.GitProvider{}, handler.EnqueueRequestsFromMapFunc(r.gitProviderToClusterWatchRules), @@ -408,7 +411,8 @@ func (r *ClusterWatchRuleReconciler) SetupWithManager(mgr ctrl.Manager) error { // namespace stops the GitTarget (which does watch ClusterProvider) but leaves this rule's // compiled entry resident until the next periodic reconcile, so the admission gate would // converge on a ~10m delay instead of on the event. The GitTarget's own status flip cannot - // carry it: that is a status-only update, which GenerationChangedPredicate above drops. + // be relied on to carry it: the edge above re-enqueues only on a move in the MIRRORED + // readiness, and a revocation need not move it. Watches( &configbutleraiv1alpha3.ClusterProvider{}, handler.EnqueueRequestsFromMapFunc(r.clusterProviderToClusterWatchRules), diff --git a/internal/controller/gittarget_dependency_status.go b/internal/controller/gittarget_dependency_status.go index f257cc53..3922f7f0 100644 --- a/internal/controller/gittarget_dependency_status.go +++ b/internal/controller/gittarget_dependency_status.go @@ -6,6 +6,8 @@ import ( "fmt" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/predicate" configbutleraiv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" ) @@ -38,3 +40,39 @@ func gitTargetReadyCondition(target configbutleraiv1alpha3.GitTarget) conditionV Message: fmt.Sprintf("Waiting for GitTarget %s/%s to publish Ready", target.Namespace, target.Name), } } + +// gitTargetReadyProjectionChanged is the GitTarget watch predicate for the WatchRule and +// ClusterWatchRule controllers: fire on create/delete, and on an update when the SPEC changed +// (generation) OR the readiness those rules MIRROR changed. +// +// GenerationChangedPredicate alone is wrong on this edge, for the same reason it cannot carry a +// ClusterProvider change to a GitTarget: a GitTarget that heals publishes its recovery as a +// status-only update, which that predicate deliberately drops. The rule's mirrored GitTargetReady +// then stayed frozen on the failing snapshot until the rule's own periodic requeue — leaving an +// object that contradicts itself for minutes, with StreamsRunning already back to True because the +// stream-state channel told it so directly. That made the mirrored condition unusable for alerting. +// +// Only Status and Reason are compared. Both are low-cardinality and are what a printer column or an +// alert keys on, whereas the mirrored Message is free-form and moves with transient stream detail; +// including it would re-enqueue every rule of a target on churn that changes no verdict. A +// message-only drift self-corrects on the next Status or Reason move. +func gitTargetReadyProjectionChanged() predicate.Predicate { + return predicate.Funcs{ + CreateFunc: func(event.CreateEvent) bool { return true }, + DeleteFunc: func(event.DeleteEvent) bool { return true }, + GenericFunc: func(event.GenericEvent) bool { return false }, + UpdateFunc: func(e event.UpdateEvent) bool { + oldTarget, ok1 := e.ObjectOld.(*configbutleraiv1alpha3.GitTarget) + newTarget, ok2 := e.ObjectNew.(*configbutleraiv1alpha3.GitTarget) + if !ok1 || !ok2 { + return true + } + if oldTarget.Generation != newTarget.Generation { + return true + } + before := gitTargetReadyCondition(*oldTarget) + after := gitTargetReadyCondition(*newTarget) + return before.Status != after.Status || before.Reason != after.Reason + }, + } +} diff --git a/internal/controller/gittarget_dependency_status_test.go b/internal/controller/gittarget_dependency_status_test.go new file mode 100644 index 00000000..5eba34b8 --- /dev/null +++ b/internal/controller/gittarget_dependency_status_test.go @@ -0,0 +1,76 @@ +// SPDX-License-Identifier: Apache-2.0 + +package controller + +import ( + "testing" + + "github.com/stretchr/testify/assert" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/event" + + configbutleraiv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" +) + +// gitTargetWithReady builds a GitTarget carrying one Ready condition, the input gitTargetReadyCondition +// projects into the readiness a WatchRule mirrors. +func gitTargetWithReady( + gen int64, + status metav1.ConditionStatus, + reason, message string, +) *configbutleraiv1alpha3.GitTarget { + t := &configbutleraiv1alpha3.GitTarget{ + ObjectMeta: metav1.ObjectMeta{Name: "repo", Namespace: "team-a", Generation: gen}, + } + if status != "" { + t.Status.Conditions = []metav1.Condition{{ + Type: GitTargetConditionReady, Status: status, Reason: reason, Message: message, + }} + } + return t +} + +func TestGitTargetReadyProjectionChanged(t *testing.T) { + p := gitTargetReadyProjectionChanged() + + // The reported bug: a GitTarget heals and publishes it as a status-only update. Dropping this + // left the rule's mirrored GitTargetReady frozen on the failing snapshot for a full requeue, + // contradicting the rule's own stream count. + assert.True(t, p.Update(event.UpdateEvent{ + ObjectOld: gitTargetWithReady(1, metav1.ConditionFalse, "WatchError", "1/2 streams running"), + ObjectNew: gitTargetWithReady(1, metav1.ConditionTrue, GitTargetReasonReady, "all streams running"), + }), "a heal (status-only, same generation) must fire") + + assert.True(t, p.Update(event.UpdateEvent{ + ObjectOld: gitTargetWithReady(1, metav1.ConditionFalse, "WatchError", "blocked"), + ObjectNew: gitTargetWithReady(1, metav1.ConditionFalse, ReasonProgressing, "blocked"), + }), "a reason move at the same status must fire") + + assert.False(t, p.Update(event.UpdateEvent{ + ObjectOld: gitTargetWithReady(1, metav1.ConditionTrue, GitTargetReasonReady, "all streams running"), + ObjectNew: gitTargetWithReady(1, metav1.ConditionTrue, GitTargetReasonReady, "all streams running"), + }), "an unchanged projection must not fire") + + // Message is deliberately excluded: it moves with transient stream detail, and firing on it + // would re-enqueue every rule of a target on churn that changes no verdict. + assert.False(t, p.Update(event.UpdateEvent{ + ObjectOld: gitTargetWithReady(1, metav1.ConditionFalse, "WatchError", "1 blocked (a)"), + ObjectNew: gitTargetWithReady(1, metav1.ConditionFalse, "WatchError", "1 blocked (b)"), + }), "a message-only change must not fire") + + assert.True(t, p.Update(event.UpdateEvent{ + ObjectOld: gitTargetWithReady(1, metav1.ConditionTrue, GitTargetReasonReady, "ready"), + ObjectNew: gitTargetWithReady(2, metav1.ConditionTrue, GitTargetReasonReady, "ready"), + }), "a spec (generation) change must fire") + + // A GitTarget that has not published Ready yet projects as Unknown, so the first publish is a + // move the rule has to see. + assert.True(t, p.Update(event.UpdateEvent{ + ObjectOld: gitTargetWithReady(1, "", "", ""), + ObjectNew: gitTargetWithReady(1, metav1.ConditionTrue, GitTargetReasonReady, "ready"), + }), "the first Ready publish must fire") + + assert.True(t, p.Create(event.CreateEvent{})) + assert.True(t, p.Delete(event.DeleteEvent{})) + assert.False(t, p.Generic(event.GenericEvent{})) +} diff --git a/internal/controller/watchrule_controller.go b/internal/controller/watchrule_controller.go index ca6027e2..1bf2c219 100644 --- a/internal/controller/watchrule_controller.go +++ b/internal/controller/watchrule_controller.go @@ -282,25 +282,30 @@ func (r *WatchRuleReconciler) SetupWithManager(mgr ctrl.Manager) error { // already suppresses no-op writes, so the loop has no fuel; this makes it structural, and // matches what GitProvider and ClusterProvider already do. For(&configbutleraiv1alpha3.WatchRule{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})). - // GenerationChangedPredicate keeps these watches reacting to a freshly - // applied or spec-changed dependency while ignoring the status-only - // updates the controllers write themselves — without it every GitTarget - // or GitProvider heartbeat would re-list and re-enqueue all WatchRules. + // This rule MIRRORS the GitTarget's readiness into its own GitTargetReady, so the watch has + // to see the status-only update that carries a heal — GenerationChangedPredicate would drop + // exactly that. gitTargetReadyProjectionChanged fires on a spec change or on a move in the + // mirrored verdict, and on nothing else. Watches( &configbutleraiv1alpha3.GitTarget{}, handler.EnqueueRequestsFromMapFunc(r.gitTargetToWatchRules), - builder.WithPredicates(predicate.GenerationChangedPredicate{}), + builder.WithPredicates(gitTargetReadyProjectionChanged()), ). + // Nothing of the GitProvider's STATUS is mirrored here, so GenerationChangedPredicate is + // right on this edge: it reacts to a freshly applied or spec-changed provider while ignoring + // the status-only updates its controller writes itself — without it every GitProvider + // heartbeat would re-list and re-enqueue all WatchRules. Watches( &configbutleraiv1alpha3.GitProvider{}, handler.EnqueueRequestsFromMapFunc(r.gitProviderToWatchRules), builder.WithPredicates(predicate.GenerationChangedPredicate{}), ). // React to a ClusterProvider's allowAnySourceNamespace (or accessFrom) - // changing. The GitTarget->WatchRules edge above CANNOT carry this: a ClusterProvider change - // reaches the GitTarget as a STATUS update, which GenerationChangedPredicate deliberately - // drops. Without this mapper, flipping the delegation flag would leave every affected - // WatchRule un-reconciled until its periodic requeue — so a REVOCATION would take minutes. + // changing. The GitTarget->WatchRules edge above still cannot be relied on to carry this: a + // ClusterProvider change reaches the GitTarget as a status update that need not move the + // GitTarget's readiness at all, and only a readiness move re-enqueues the rules. Without + // this mapper, flipping the delegation flag would leave every affected WatchRule + // un-reconciled until its periodic requeue — so a REVOCATION would take minutes. Watches( &configbutleraiv1alpha3.ClusterProvider{}, handler.EnqueueRequestsFromMapFunc(r.clusterProviderToWatchRules), From 2c8eee091e12eb870fbfb8aa52720bbd061aadc0 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Mon, 7 Sep 2026 11:31:55 +0000 Subject: [PATCH 2/6] fix(flags): name the resume-cursor cost in the transport flag's own help --author-attribution-transport=memory ends with "--redis-addr may be empty", which reads as an invitation to drop Redis entirely. The consequence of doing so was documented only on --redis-addr: that address also holds each GitTarget's watch resume cursors, independently of attribution, so an empty one makes every watch cold-replay on restart instead of resuming. An adopter evaluating the flag reads the flag, so say it there too. Co-Authored-By: Claude Opus 5 --- cmd/main.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cmd/main.go b/cmd/main.go index d7d76856..84e7a846 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -559,7 +559,10 @@ func parseFlagsWithArgs(fs *flag.FlagSet, args []string) (appConfig, error) { "appends to Redis streams, \"memory\" to an in-process ring. Redis is the production choice and "+ "the only one that survives a restart or reaches a second replica; memory is for the single-pod "+ "install where running a Valkey StatefulSet to name commit authors is out of proportion, and it "+ - "is refused with more than one replica. With \"memory\", --redis-addr may be empty.") + "is refused with more than one replica. With \"memory\", --redis-addr may be empty -- but note "+ + "that this does not by itself let you drop Redis: --redis-addr also holds each GitTarget's watch "+ + "resume cursors, independently of attribution, so an empty address additionally means every "+ + "watch cold-replays on restart instead of resuming.") fs.DurationVar(&cfg.attributionFactTTL, "author-attribution-ttl", queue.DefaultAttributionFactTTL, "How long an attribution fact is retained waiting for the matching watch event to join it. It "+ "bounds stream retention and the in-memory index together, and doubles as the replay horizon a "+ From f3526c160d71c1d55e3d09a82bbc7586fc3dc51f Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Mon, 7 Sep 2026 11:32:11 +0000 Subject: [PATCH 3/6] docs: close the gaps a 0.40.1 to 0.43.0 adoption reported Six corrections, from a consumer's field report on upgrading across three releases in one step. Which commit-message template renders a commit follows the commit's SHAPE, not the window setting: pending_writes.go picks eventTemplate whenever a write carries exactly one event. Both the field doc and configuration.md claimed eventTemplate was "only used when spec.commit.window is 0s", which is wrong for nearly every commit a human makes by hand, and left multi-event commits falling through to the built-in wording for anyone who set only one template. Say that with a non-zero window you want both. The Go floor for module consumers is now stated in the README and has an UPGRADING entry. It moved from 1.26.5 to 1.27.0 in v0.41.1, a patch release whose changelog reads "let's release some security updates", and it stops the build rather than the migration: the golang images set GOTOOLCHAIN=local, so it is a hard stop, and it cascades into golangci-lint refusing a newer target as a config-load error. Going forward a directive move is treated as consumer-breaking and gets an entry. The memory transport frees ATTRIBUTION from Redis, not the install. Three places advertised it without the resume-cursor caveat. UPGRADING gains a section for CRs written by a controller rather than by hand: the inventory is a code search, the apply happens on the controller's schedule, and the verification step is a test asserting the old field is ABSENT, because a controller applying with fieldValidation: Ignore is the quiet client and the object stays Ready=True either way. Deleting allowedSourceNamespaces relocated the tenancy boundary onto RBAC. Say so to operators who granted broad RBAC because the destination policy was doing the fencing: their ClusterRole is now the fence. Two behaviours that surprised an adopter are documented rather than changed. Inside --author-attribution-ttl a fact can name a later write by a different identity on the same name, because the join's floor is a last-writer-wins (namespace, name) tier. And placement never relocates a document it did not create, so a folder keeps its layout for as long as its documents live; re-place by deleting and recreating the object. Co-Authored-By: Claude Opus 5 --- README.md | 4 ++ api/v1alpha3/gitprovider_types.go | 13 ++-- .../crd/bases/configbutler.ai_gittargets.yaml | 13 ++-- docs/INDEX.md | 3 +- docs/UPGRADING.md | 68 +++++++++++++++++++ docs/architecture.md | 5 ++ docs/components.md | 2 +- docs/configuration.md | 39 +++++++++-- 8 files changed, 129 insertions(+), 18 deletions(-) diff --git a/README.md b/README.md index 6c95ff61..b9d22a08 100644 --- a/README.md +++ b/README.md @@ -132,6 +132,10 @@ Early-stage software; CRDs and behavior may still change. - Tested against Kubernetes `1.37` at the API level (envtest) and `1.36` end-to-end (k3s, which has no stable `1.37` release yet); other versions may work but are not in the matrix. - Runtime behavior is deterministic: no AI or heuristic mutation at runtime. +- **Minimum Go for module consumers: `1.27.1`.** Running the image needs no Go at all, but anything + importing this repo as a module (`pkg/manifestanalyzer`, for example) is bound by the `go` + directive in [`go.mod`](go.mod), which is the source of truth. That floor can move in any release, + including a patch release; when it does, the release notes say so. Good fit for pilots, lab clusters, brownfield discovery, and design partners who can tolerate change. Production use should follow an environment-specific review. diff --git a/api/v1alpha3/gitprovider_types.go b/api/v1alpha3/gitprovider_types.go index f4eb2a4d..01a53d4a 100644 --- a/api/v1alpha3/gitprovider_types.go +++ b/api/v1alpha3/gitprovider_types.go @@ -174,8 +174,11 @@ type CommitterSpec struct { // GitTarget.spec.commit.message; the identically-shaped GitProvider.spec.commit.message is // retained only to reject a manifest that still sets it there. type CommitMessageSpec struct { - // EventTemplate is a Go text/template string for per-event commit messages - // (used when spec.commit.window is "0s"; one event per commit). + // EventTemplate is a Go text/template string for the commit message of a commit carrying + // exactly ONE event. Which template renders a commit follows the commit's shape, not the + // window setting: a "0s" window makes every commit single-event, but a commit window that + // closes around a single change renders through this template too. With a non-zero window, + // set GroupTemplate as well — both shapes occur. // Available variables: Operation, Group, Version, Resource, Namespace, Name, // APIVersion, Username, GitTarget. // +optional @@ -191,9 +194,9 @@ type CommitMessageSpec struct { // +optional ReconcileTemplate string `json:"reconcileTemplate,omitempty"` - // GroupTemplate is a Go text/template string for grouped commit messages - // (the commit-window path; one commit per (author, gitTarget) group - // produced by the batching pipeline). + // GroupTemplate is a Go text/template string for the commit message of a commit carrying TWO + // OR MORE events (one commit per (author, gitTarget) group produced by the batching + // pipeline). A group that closed around a single event renders through EventTemplate instead. // Available variables: Author, GitTarget, Count, Operations (map of // CREATE/UPDATE/DELETE counts), Resources (slice of {Group, Version, // Resource, Namespace, Name}). diff --git a/config/crd/bases/configbutler.ai_gittargets.yaml b/config/crd/bases/configbutler.ai_gittargets.yaml index a149066b..05cb0322 100644 --- a/config/crd/bases/configbutler.ai_gittargets.yaml +++ b/config/crd/bases/configbutler.ai_gittargets.yaml @@ -151,16 +151,19 @@ spec: properties: eventTemplate: description: |- - EventTemplate is a Go text/template string for per-event commit messages - (used when spec.commit.window is "0s"; one event per commit). + EventTemplate is a Go text/template string for the commit message of a commit carrying + exactly ONE event. Which template renders a commit follows the commit's shape, not the + window setting: a "0s" window makes every commit single-event, but a commit window that + closes around a single change renders through this template too. With a non-zero window, + set GroupTemplate as well — both shapes occur. Available variables: Operation, Group, Version, Resource, Namespace, Name, APIVersion, Username, GitTarget. type: string groupTemplate: description: |- - GroupTemplate is a Go text/template string for grouped commit messages - (the commit-window path; one commit per (author, gitTarget) group - produced by the batching pipeline). + GroupTemplate is a Go text/template string for the commit message of a commit carrying TWO + OR MORE events (one commit per (author, gitTarget) group produced by the batching + pipeline). A group that closed around a single event renders through EventTemplate instead. Available variables: Author, GitTarget, Count, Operations (map of CREATE/UPDATE/DELETE counts), Resources (slice of {Group, Version, Resource, Namespace, Name}). diff --git a/docs/INDEX.md b/docs/INDEX.md index 361e55b5..c66d3851 100644 --- a/docs/INDEX.md +++ b/docs/INDEX.md @@ -199,7 +199,8 @@ expander, replacing the expander with one collection fact that every removal in uid membership or by scope. That last part is a capability gain rather than a like-for-like swap: a collection delete the API server sent no response body for used to lose its author entirely. `exact_deletecollection_item` is replaced by `deletecollection_body_uid` and `deletecollection_scope`, and -`--author-attribution-transport=memory` runs attribution with no Redis on a single replica. +`--author-attribution-transport=memory` runs attribution with no Redis on a single replica (the +watch resume cursors still live in Redis, so this frees attribution from it rather than the install). Shipped as #283, #284, #286 and #287. Before it, diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index c52de042..6453285b 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -146,6 +146,25 @@ Either way the inventory is step 1 rather than a footnote, because a pruned valu from the API. The two removal strategies are priced in [`facts/crd-upgrade-strategies.md`](facts/crd-upgrade-strategies.md). +### If a controller writes your CRs + +Every step above assumes you apply manifests by hand. If your `GitProvider`/`GitTarget`/`WatchRule` +objects are generated by a controller or an installer of your own, three of them change shape: + +- **The inventory is a code search, not a cluster query.** `kubectl ... | jq` tells you what is + deployed today, which you already know: whatever your generator emits. Grep the generator for the + old spellings instead, and treat its templates as the artifact to migrate. +- **The apply happens on the controller's schedule, not yours.** There is no single-sync moment you + control, so the fix is a code change plus a redeploy, and the objects converge whenever your + reconciler next writes them. Plan for a window where old and new spellings are both in flight. +- **The verification step is a test, not a `kubectl get`.** A controller that applies with + `fieldValidation: Ignore` is the quiet client described above: it is accepted and pruned, and the + object stays `Ready=True`. Assert the **absence** of the old spelling in your own test suite, + because nothing in the object's status distinguishes migrated from silently defaulted. + +That last point is the one worth spending a test on. It is the difference between finding out at +upgrade time and finding out when a value you thought you had set turns out never to have applied. + ## Every reference names the kind it points at Four fields are renamed. This is the same change as the one below, seen from the other side: once a @@ -510,6 +529,15 @@ kubectl get watchrules -A -o json | For each one, either name the namespaces explicitly in `rules[].sourceNamespace`, or keep `"*"` and make the credential's RBAC the fence you meant the policy to be. +**If you granted the reverser broad RBAC *because* the destination policy was doing the fencing, +re-read your ClusterRole: it is now the fence.** The tenancy boundary did not disappear in this +release, it relocated. It used to be declared by the destination, visible on the `GitTarget`, and +reviewable in a pull request alongside the rule it constrained. It is now a property of the +credential, expressed in RBAC and reviewed wherever you review RBAC. A narrow, hand-written +ClusterRole that was defence-in-depth before this release is load-bearing after it, and the +packaged `*/*` role fences nothing at all. This is worth writing down next to the role itself, so +that nobody widens it later on the assumption that something downstream is still checking. + Two things get better. A `"*"` rule over a type in a hundred-namespace cluster was a hundred watch connections and a hundred list calls at warm-up, each with its own cursor and its own share of the apiserver watch cache; it is one of each now, and the saving grows with the cluster. And its failure @@ -639,6 +667,46 @@ different one the namespace is now written explicitly — omitting it would have kustomize and rendered a different object than the one being mirrored. This also now applies to a path you declared, which previously wrote a `namespace:` line the rest of that folder omits. +## 0.41.1 — the Go floor moved (patch release; breaking for module consumers) + +The `go` directive in `go.mod` moved from `1.26.5` to `1.27.0` in `v0.41.1`, a release whose +changelog entry reads only *"let's release some security updates"*. It has moved again since. The +directive in [`go.mod`](../go.mod) is always the source of truth; the README carries the current +floor. + +**If you run the image, this does not affect you.** The toolchain is baked into the build. + +**If you import this repo as a Go module** — `pkg/manifestanalyzer` is the usual reason — it affects +you before any other step on this page, because it stops the build rather than the migration: + +```console +$ go get github.com/ConfigButler/gitops-reverser@v0.43.0 +go: github.com/ConfigButler/gitops-reverser@v0.43.0 requires go >= 1.27.0 + (running go 1.26.5; GOTOOLCHAIN=local) +``` + +The official `golang` images set `GOTOOLCHAIN=local`, so the container's Go is the only Go there is +and this is a hard stop, not a download prompt. It also cascades into linting: `golangci-lint` +refuses a target newer than the Go it was built with, and reports it as a config-load error rather +than as a finding, which reads like a broken config file rather than a version floor: + +```console +Error: can't load config: the Go language version (go1.26) used to +build golangci-lint is lower than the targeted Go version (1.27.0) +``` + +### Migration + +Move the pinned Go toolchain **and** the pinned `golangci-lint` in your build image together, before +starting any other migration on this page. Upgrading one without the other trades the first error +for the second. + +### Going forward + +A `go` directive move is treated as consumer-breaking for module users. It gets an entry here and a +line in the release notes even when it ships in a patch release, so it is never again something you +discover from a failing build. + ## 0.41.0 — attribution facts travel on a selectable transport, and Redis is no longer implied Attribution stopped meaning Redis. The audit receiver appends its facts to a per-type **stream**, diff --git a/docs/architecture.md b/docs/architecture.md index f17c59e5..082ea70d 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -656,6 +656,11 @@ travel: | `redis` (default) | Redis Streams, per type, with a retention window a restarting process replays | any install; **required** for more than one replica | | `memory` | an in-process ring | single replica, no Redis; every fact is lost on restart, by design | +`memory` removes *attribution's* need for Redis. It does not on its own remove the install's: +`--redis-addr` also holds each `GitTarget`'s watch resume cursors, independently of attribution. So +dropping the Valkey StatefulSet as well means every watch cold-replays on restart instead of +resuming, across every target. + | Endpoint | Role | |---|---| | `/audit-webhook/` | One audit route's stream; some ClusterProvider must carry that route | diff --git a/docs/components.md b/docs/components.md index 0969dbb3..58d249c1 100644 --- a/docs/components.md +++ b/docs/components.md @@ -132,7 +132,7 @@ Optional, and deliberately unable to affect *what* is written. It only names the | Component | Path | Role | |---|---|---| | Audit ingress | [`internal/webhook/audit_handler.go`](../internal/webhook/audit_handler.go) | receives kube-apiserver audit events on `/audit-webhook/` and publishes a minimal fact | -| Fact transport | [`internal/queue/fact_stream.go`](../internal/queue/fact_stream.go) | Redis Streams by default, an in-process ring with `--author-attribution-transport=memory` | +| Fact transport | [`internal/queue/fact_stream.go`](../internal/queue/fact_stream.go) | Redis Streams by default, an in-process ring with `--author-attribution-transport=memory` (which frees attribution from Redis, but not the watch resume cursors) | | Fact index and follower | [`internal/queue/fact_index.go`](../internal/queue/fact_index.go) | a bounded, TTL'd in-process index, followed per type while at least one watch needs it | | Author resolver | [`internal/watch/author_resolver.go`](../internal/watch/author_resolver.go) | joins a watch event to a fact within a bounded grace window | | Command author store | [`internal/queue/command_author_store.go`](../internal/queue/command_author_store.go) | the `CommitRequest` submitter captured at admission | diff --git a/docs/configuration.md b/docs/configuration.md index b4a02aad..ee3592ea 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -521,15 +521,21 @@ write rather than stopping the mirror. #### Commit message templates -There are three templates, one per commit shape: - -- `spec.commit.message.eventTemplate`: per-event commits (only used when `spec.commit.window` is - `0s`). -- `spec.commit.message.groupTemplate`: grouped commits produced by the commit window (the - common case). +There are three templates, one per commit shape. **Which one renders a commit is decided by the +shape of that commit, not by the window setting:** + +- `spec.commit.message.eventTemplate`: a commit carrying exactly one event. A `0s` window makes + every commit this shape, but it is not the only way to get one. A commit window that closes around + a single change renders through this template too, which is most edits a human makes by hand. +- `spec.commit.message.groupTemplate`: a commit carrying two or more events, which is what a burst + inside the commit window produces. - `spec.commit.message.reconcileTemplate`: reconcile commits (the mark-and-sweep reconcile path; one commit per synced type). +So with a non-zero window you probably want **both** `eventTemplate` and `groupTemplate` set. +Setting only one leaves the other shape rendering through the built-in wording, and both shapes +occur in normal operation. + ```yaml spec: commit: @@ -811,6 +817,18 @@ exists, updates and deletes always edit it in place at its current location (fou not path), so changing placement never moves an existing file; it only affects resources created after the change. +**This is the permanent contract, not a gap waiting to be closed.** The operator never relocates a +document it did not create, because moving a file is indistinguishable in Git from deleting one and +writing another, and doing that on your behalf would rewrite history you may have built tooling or +review habits around. The practical consequence is worth stating plainly: a folder laid out under an +older policy keeps that layout for as long as its documents live, however you change `spec.placement` +afterwards. There is no re-layout operation; treat the layout a folder already has as given. + +To re-place a document deliberately, remove it and let the mirror write it again: delete the object +in the cluster (the mirror removes the document), then recreate it, and the new write goes through +the current placement policy. Deleting the file in Git alone is not enough, since the next reconcile +restores it at the path the object's document identity already resolves to. + #### How a path is chosen (the resolution ladder) For each new resource the operator walks this order and stops at the first that produces a path: @@ -1451,6 +1469,15 @@ When attribution is enabled, these flags tune the join: - `--author-attribution-ttl` (default `10m`): how long an attribution fact is retained waiting for the matching watch event to join it. + + **Inside that window a fact can name a later write by a different identity, on purpose.** The join + tries the strongest evidence first, and its floor is a `(namespace, name)` tier that is + last-writer-wins and bounded only by this TTL. So an out-of-band write, such as a hand-run + `kubectl` command against a mirrored object, leaves a fact that a subsequent write to the same + name can match while the TTL still holds it, and that later commit is attributed to whoever made + the out-of-band change. It clears itself once the fact expires. A shorter TTL narrows the window + at the cost of losing actors whose audit delivery is slow. The full ranking and the reasoning + behind the floor are in [`spec/attribution.md`](spec/attribution.md). - `--author-attribution-grace` (default `3s`): bounded per-event wait for a matching audit fact before a watch event ships authored by the `attribution-unresolved` sentinel. Note the delivery floor: the apiserver's own `--audit-webhook-batch-max-wait` delays every fact by up to that much, so a grace at or From ca1d4c05c225cc81c4247c954d23d28dc899ad50 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Mon, 7 Sep 2026 12:28:29 +0000 Subject: [PATCH 4/6] fix(watchrule): prove the readiness wiring, and correct three claims review caught Review of #343 found the predicate unit test could not notice either controller losing the predicate, and three documentation claims that were wrong or overstated. A wiring test now drives a status-only GitTarget recovery through the real SetupWithManager wiring. It lives in its own envtest package, and the isolation is load-bearing rather than tidiness: in internal/controller's suite the GitTarget controller is live and rewrites the status the test writes by hand on a 10s cadence, and a second manager racing the suite's on rule status produced 100ms retry requeues that masked a removed predicate entirely. Verified by mutation: reverting either controller to GenerationChangedPredicate fails both tests, and the propagation budget (4s) sits below both requeue cadences that could otherwise rescue them. The template documentation was still misleading. The window coalesces by destination path before anything renders, so "one event" is not "one action": ten edits to one ConfigMap inside a window are ONE changed document and render through eventTemplate. Say documents rather than events, and say the coalescing. The layout re-place procedure was incomplete to the point of being wrong. Delete and recreate inside one window, same author, coalesce by path and cancel out: the deletion never reaches Git and the recreated object is updated in place at the old location, which looks exactly like placement being ignored. The procedure now says to wait for the deletion to be committed and pushed. Two overstatements are withdrawn. "Permanent contract" was an unnecessary product commitment, and the justification given for it was wrong: moving a document in a new commit does not rewrite history. And the cross-identity attribution window is a known limitation of the weakest join tier, not something done on purpose; the tier is kept because an aggregated-API write carries no uid or resourceVersion, so dropping it would leave that class unattributed. Finally, extending retain-and-reject to value-carrying deletions is now recorded in UPGRADING as deferred rather than left looking settled. Co-Authored-By: Claude Opus 5 --- .coverage-baseline | 2 +- api/v1alpha3/gitprovider_types.go | 18 +- .../crd/bases/configbutler.ai_gittargets.yaml | 18 +- docs/UPGRADING.md | 9 + docs/configuration.md | 77 +++-- .../wiring/readiness_wiring_test.go | 290 ++++++++++++++++++ 6 files changed, 368 insertions(+), 46 deletions(-) create mode 100644 internal/controller/wiring/readiness_wiring_test.go diff --git a/.coverage-baseline b/.coverage-baseline index cd27a8f4..9affa4ba 100644 --- a/.coverage-baseline +++ b/.coverage-baseline @@ -1 +1 @@ -77.2 +77.3 diff --git a/api/v1alpha3/gitprovider_types.go b/api/v1alpha3/gitprovider_types.go index 01a53d4a..5c849bd5 100644 --- a/api/v1alpha3/gitprovider_types.go +++ b/api/v1alpha3/gitprovider_types.go @@ -174,11 +174,13 @@ type CommitterSpec struct { // GitTarget.spec.commit.message; the identically-shaped GitProvider.spec.commit.message is // retained only to reject a manifest that still sets it there. type CommitMessageSpec struct { - // EventTemplate is a Go text/template string for the commit message of a commit carrying - // exactly ONE event. Which template renders a commit follows the commit's shape, not the - // window setting: a "0s" window makes every commit single-event, but a commit window that - // closes around a single change renders through this template too. With a non-zero window, - // set GroupTemplate as well — both shapes occur. + // EventTemplate is a Go text/template string for the commit message of a commit that changes + // exactly ONE document. Which template renders a commit follows how many documents it changes, + // not the window setting: a "0s" window makes every commit single-document, but a window that + // closes around a single change renders through this template too. Note that repeated edits to + // one resource inside a window coalesce by destination path, so ten updates to one ConfigMap + // are ONE changed document and render here, not through GroupTemplate. With a non-zero window, + // set GroupTemplate as well: both shapes occur. // Available variables: Operation, Group, Version, Resource, Namespace, Name, // APIVersion, Username, GitTarget. // +optional @@ -194,9 +196,9 @@ type CommitMessageSpec struct { // +optional ReconcileTemplate string `json:"reconcileTemplate,omitempty"` - // GroupTemplate is a Go text/template string for the commit message of a commit carrying TWO - // OR MORE events (one commit per (author, gitTarget) group produced by the batching - // pipeline). A group that closed around a single event renders through EventTemplate instead. + // GroupTemplate is a Go text/template string for the commit message of a commit that changes TWO + // OR MORE documents (one commit per (author, gitTarget) group produced by the batching + // pipeline). A group that changed only one document renders through EventTemplate instead. // Available variables: Author, GitTarget, Count, Operations (map of // CREATE/UPDATE/DELETE counts), Resources (slice of {Group, Version, // Resource, Namespace, Name}). diff --git a/config/crd/bases/configbutler.ai_gittargets.yaml b/config/crd/bases/configbutler.ai_gittargets.yaml index 05cb0322..616000d3 100644 --- a/config/crd/bases/configbutler.ai_gittargets.yaml +++ b/config/crd/bases/configbutler.ai_gittargets.yaml @@ -151,19 +151,21 @@ spec: properties: eventTemplate: description: |- - EventTemplate is a Go text/template string for the commit message of a commit carrying - exactly ONE event. Which template renders a commit follows the commit's shape, not the - window setting: a "0s" window makes every commit single-event, but a commit window that - closes around a single change renders through this template too. With a non-zero window, - set GroupTemplate as well — both shapes occur. + EventTemplate is a Go text/template string for the commit message of a commit that changes + exactly ONE document. Which template renders a commit follows how many documents it changes, + not the window setting: a "0s" window makes every commit single-document, but a window that + closes around a single change renders through this template too. Note that repeated edits to + one resource inside a window coalesce by destination path, so ten updates to one ConfigMap + are ONE changed document and render here, not through GroupTemplate. With a non-zero window, + set GroupTemplate as well: both shapes occur. Available variables: Operation, Group, Version, Resource, Namespace, Name, APIVersion, Username, GitTarget. type: string groupTemplate: description: |- - GroupTemplate is a Go text/template string for the commit message of a commit carrying TWO - OR MORE events (one commit per (author, gitTarget) group produced by the batching - pipeline). A group that closed around a single event renders through EventTemplate instead. + GroupTemplate is a Go text/template string for the commit message of a commit that changes TWO + OR MORE documents (one commit per (author, gitTarget) group produced by the batching + pipeline). A group that changed only one document renders through EventTemplate instead. Available variables: Author, GitTarget, Count, Operations (map of CREATE/UPDATE/DELETE counts), Resources (slice of {Group, Version, Resource, Namespace, Name}). diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index 6453285b..993a1db1 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -146,6 +146,15 @@ Either way the inventory is step 1 rather than a footnote, because a pruned valu from the API. The two removal strategies are priced in [`facts/crd-upgrade-strategies.md`](facts/crd-upgrade-strategies.md). +**A pruned value still has no in-band signal, and that is an open gap rather than a settled +position.** `ClusterWatchRule.spec.rules[].scope` was retained for a release as a loud rejection +before being deleted, precisely because deleting a field outright is the silent option. That +treatment was not extended to the value-carrying deletions in this release +(`allowedSourceNamespaces`, `push.commitWindow`, `commit.message`), so for those the inventory and +your own tests are the whole safety net. Extending retain-and-reject to value-carrying deletions, or +publishing a condition or event on an object applied with a field the CRD no longer serves, is +deferred, not declined. + ### If a controller writes your CRs Every step above assumes you apply manifests by hand. If your `GitProvider`/`GitTarget`/`WatchRule` diff --git a/docs/configuration.md b/docs/configuration.md index ee3592ea..b34e56f4 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -521,20 +521,26 @@ write rather than stopping the mirror. #### Commit message templates -There are three templates, one per commit shape. **Which one renders a commit is decided by the -shape of that commit, not by the window setting:** - -- `spec.commit.message.eventTemplate`: a commit carrying exactly one event. A `0s` window makes - every commit this shape, but it is not the only way to get one. A commit window that closes around - a single change renders through this template too, which is most edits a human makes by hand. -- `spec.commit.message.groupTemplate`: a commit carrying two or more events, which is what a burst - inside the commit window produces. +There are three templates. **Which one renders a commit is decided by how many documents that commit +changes, not by the window setting:** + +- `spec.commit.message.eventTemplate`: a commit that changes exactly **one** document. +- `spec.commit.message.groupTemplate`: a commit that changes **two or more**. - `spec.commit.message.reconcileTemplate`: reconcile commits (the mark-and-sweep reconcile path; one commit per synced type). -So with a non-zero window you probably want **both** `eventTemplate` and `groupTemplate` set. -Setting only one leaves the other shape rendering through the built-in wording, and both shapes -occur in normal operation. +Two consequences are easy to get wrong, and both follow from the window coalescing by destination +path before anything is rendered: + +- **A `0s` window is not the only way to reach `eventTemplate`.** A window that closes around a + single change renders through it too, which is most edits a human makes by hand. +- **"One document" is not "one action".** Repeated edits to the same resource inside one window + collapse to a single entry, last write wins. Ten updates to one ConfigMap are one changed + document, so they render through `eventTemplate`, and `groupTemplate` never sees them. + +So with a non-zero window you want **both** `eventTemplate` and `groupTemplate` set. Setting only +one leaves the other shape rendering through the built-in wording, and both shapes occur in normal +operation. ```yaml spec: @@ -817,17 +823,25 @@ exists, updates and deletes always edit it in place at its current location (fou not path), so changing placement never moves an existing file; it only affects resources created after the change. -**This is the permanent contract, not a gap waiting to be closed.** The operator never relocates a -document it did not create, because moving a file is indistinguishable in Git from deleting one and -writing another, and doing that on your behalf would rewrite history you may have built tooling or -review habits around. The practical consequence is worth stating plainly: a folder laid out under an -older policy keeps that layout for as long as its documents live, however you change `spec.placement` -afterwards. There is no re-layout operation; treat the layout a folder already has as given. +**There is no re-layout operation today.** The practical consequence is worth stating plainly: a +folder laid out under an older policy keeps that layout for as long as its documents live, however +you change `spec.placement` afterwards. Placement governs the first write and nothing else. This is +a description of current behavior rather than a promise: a deliberate migration that moves documents +in a new commit is a coherent feature and is not ruled out. + +To re-place a document by hand today, remove it and let the mirror write it again: + +1. Delete the object in the cluster. +2. **Wait until the deletion is committed and pushed** and the document is gone from the branch. +3. Recreate the object. The new write has no document to update, so it goes through the current + placement policy. -To re-place a document deliberately, remove it and let the mirror write it again: delete the object -in the cluster (the mirror removes the document), then recreate it, and the new write goes through -the current placement policy. Deleting the file in Git alone is not enough, since the next reconcile -restores it at the path the object's document identity already resolves to. +Step 2 is not a formality. Delete and recreate inside one commit window, by the same author, +coalesce by destination path into a single entry, so the pair cancels out: the deletion never +reaches Git, the document is still there, and the recreated object is updated in place at the old +location. From the outside this looks exactly like placement being ignored. Deleting the file in +Git alone does not work either, since the next reconcile restores it at the path the object's +document identity already resolves to. #### How a path is chosen (the resolution ladder) @@ -1470,14 +1484,19 @@ When attribution is enabled, these flags tune the join: - `--author-attribution-ttl` (default `10m`): how long an attribution fact is retained waiting for the matching watch event to join it. - **Inside that window a fact can name a later write by a different identity, on purpose.** The join - tries the strongest evidence first, and its floor is a `(namespace, name)` tier that is - last-writer-wins and bounded only by this TTL. So an out-of-band write, such as a hand-run - `kubectl` command against a mirrored object, leaves a fact that a subsequent write to the same - name can match while the TTL still holds it, and that later commit is attributed to whoever made - the out-of-band change. It clears itself once the fact expires. A shorter TTL narrows the window - at the cost of losing actors whose audit delivery is slow. The full ranking and the reasoning - behind the floor are in [`spec/attribution.md`](spec/attribution.md). + **Inside that window a fact can name a later write by a different identity. This is a known + limitation of the weakest join tier, not a desired property.** The join tries the strongest + evidence first, and its floor is a `(namespace, name)` tier that is last-writer-wins and bounded + only by this TTL. A write that misses the exact tier can settle for that floor. So an out-of-band + change, such as a hand-run `kubectl` command against a mirrored object, leaves a fact that a + later write to the same name can match while the TTL still holds it, and that commit is then + attributed to whoever made the out-of-band change. It clears itself once the fact expires. + + The floor exists because some writes carry nothing stronger: an aggregated-API write's audit + record holds a name and neither a uid nor a resourceVersion, and removing the tier would leave + that whole class unattributed. Narrowing the TTL narrows the exposure, at the cost of losing + actors whose audit delivery is slow. The full ranking is in + [`spec/attribution.md`](spec/attribution.md). - `--author-attribution-grace` (default `3s`): bounded per-event wait for a matching audit fact before a watch event ships authored by the `attribution-unresolved` sentinel. Note the delivery floor: the apiserver's own `--audit-webhook-batch-max-wait` delays every fact by up to that much, so a grace at or diff --git a/internal/controller/wiring/readiness_wiring_test.go b/internal/controller/wiring/readiness_wiring_test.go new file mode 100644 index 00000000..6e1da319 --- /dev/null +++ b/internal/controller/wiring/readiness_wiring_test.go @@ -0,0 +1,290 @@ +// SPDX-License-Identifier: Apache-2.0 + +// Package wiring holds integration tests for controller WATCH WIRING: what a controller is woken +// up by, rather than what one reconcile computes. +// +// It runs its own envtest with only the controllers under test registered. That isolation is the +// point rather than tidiness. In internal/controller's suite the GitTarget controller is live and +// owns the GitTarget status these tests write by hand, rewriting it on a 10s cadence, and a second +// manager racing the suite's on rule status produced 100ms retry requeues that masked a removed +// predicate entirely. Both made a wiring assertion here unreliable in opposite directions. +package wiring + +import ( + "context" + "os" + "path/filepath" + "testing" + "time" + + meta "github.com/fluxcd/pkg/apis/meta" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + apimeta "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/rest" + "k8s.io/client-go/util/retry" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/envtest" + "sigs.k8s.io/controller-runtime/pkg/manager" + metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" + + configbutleraiv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" + "github.com/ConfigButler/gitops-reverser/internal/controller" + "github.com/ConfigButler/gitops-reverser/internal/rulestore" +) + +// propagationBudget is shorter than BOTH cadences a rule could otherwise be rescued by: +// RequeueStreamSettleInterval (10s) while converging and RequeueSteadyInterval (5m) once settled. +// Only an event-driven enqueue meets it, which is what makes these tests fail — rather than merely +// run slower — if the GitTarget watch goes back to GenerationChangedPredicate. +const propagationBudget = 4 * time.Second + +// settleBudget is the generous wait for a first reconcile, which races manager start and cache sync +// and has no bearing on what is being proven. +const settleBudget = 30 * time.Second + +var ( + testEnv *envtest.Environment + k8sClient client.Client + restCfg *rest.Config +) + +func TestMain(m *testing.M) { + os.Exit(run(m)) +} + +// run bootstraps envtest, runs the tests, and always tears the environment down. +func run(m *testing.M) int { + testEnv = &envtest.Environment{ + CRDDirectoryPaths: []string{filepath.Join("..", "..", "..", "config", "crd", "bases")}, + ErrorIfCRDPathMissing: true, + } + + var err error + restCfg, err = testEnv.Start() + if err != nil { + panic("start envtest: " + err.Error()) + } + defer func() { _ = testEnv.Stop() }() + + if err := configbutleraiv1alpha3.AddToScheme(scheme.Scheme); err != nil { + panic("add scheme: " + err.Error()) + } + k8sClient, err = client.New(restCfg, client.Options{Scheme: scheme.Scheme}) + if err != nil { + panic("build client: " + err.Error()) + } + + return m.Run() +} + +// startManager runs a manager carrying only what register installs, and stops it when the test ends. +func startManager(t *testing.T, register func(manager.Manager) error) { + t.Helper() + + mgr, err := manager.New(restCfg, manager.Options{ + Scheme: scheme.Scheme, + Metrics: metricsserver.Options{BindAddress: "0"}, + LeaderElection: false, + }) + require.NoError(t, err) + require.NoError(t, register(mgr)) + + mgrCtx, cancel := context.WithCancel(context.Background()) + done := make(chan struct{}) + go func() { + defer close(done) + _ = mgr.Start(mgrCtx) + }() + t.Cleanup(func() { + cancel() + <-done + }) + + require.True(t, mgr.GetCache().WaitForCacheSync(mgrCtx)) +} + +// seedTarget creates a namespace, a GitProvider and a GitTarget, and returns the namespace. When +// clusterProvider is non-empty it also creates that ClusterProvider admitting every namespace and +// points the GitTarget at it: a ClusterWatchRule is refused (and its GitTargetReady overwritten with +// the refusal) unless the source cluster admits the target's namespace. +func seedTarget(ctx context.Context, t *testing.T, name, clusterProvider string) string { + t.Helper() + + namespace := name + require.NoError(t, k8sClient.Create(ctx, &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: namespace}, + })) + require.NoError(t, k8sClient.Create(ctx, &configbutleraiv1alpha3.GitProvider{ + ObjectMeta: metav1.ObjectMeta{Name: "provider", Namespace: namespace}, + Spec: configbutleraiv1alpha3.GitProviderSpec{ + URL: "https://github.com/test/repo.git", + AllowedBranches: []string{"*"}, + SecretRef: &meta.LocalObjectReference{Name: "git-credentials"}, + }, + })) + target := &configbutleraiv1alpha3.GitTarget{ + ObjectMeta: metav1.ObjectMeta{Name: "target", Namespace: namespace}, + Spec: configbutleraiv1alpha3.GitTargetSpec{ + GitProviderRef: meta.LocalObjectReference{Name: "provider"}, + Branch: "main", + Path: "mirror", + }, + } + if clusterProvider != "" { + // selector: {} is the "every namespace" form; names: ["*"] would be a literal name. + require.NoError(t, k8sClient.Create(ctx, &configbutleraiv1alpha3.ClusterProvider{ + ObjectMeta: metav1.ObjectMeta{Name: clusterProvider}, + Spec: configbutleraiv1alpha3.ClusterProviderSpec{ + AccessFrom: &configbutleraiv1alpha3.NamespaceMatcher{ + Selector: &metav1.LabelSelector{}, + }, + }, + })) + target.Spec.ClusterProviderRef = &meta.LocalObjectReference{Name: clusterProvider} + } + require.NoError(t, k8sClient.Create(ctx, target)) + return namespace +} + +// setTargetReady writes ONLY the GitTarget's status. The spec, and therefore the generation, is +// untouched: this is exactly the shape of update GenerationChangedPredicate drops. +func setTargetReady( + ctx context.Context, + t *testing.T, + namespace string, + status metav1.ConditionStatus, + reason string, +) { + t.Helper() + + key := types.NamespacedName{Name: "target", Namespace: namespace} + target := &configbutleraiv1alpha3.GitTarget{} + require.NoError(t, k8sClient.Get(ctx, key, target)) + generationBefore := target.Generation + + require.NoError(t, retry.RetryOnConflict(retry.DefaultRetry, func() error { + if err := k8sClient.Get(ctx, key, target); err != nil { + return err + } + apimeta.SetStatusCondition(&target.Status.Conditions, metav1.Condition{ + Type: controller.GitTargetConditionReady, + Status: status, + Reason: reason, + Message: "set by the wiring test", + }) + return k8sClient.Status().Update(ctx, target) + })) + + require.NoError(t, k8sClient.Get(ctx, key, target)) + require.Equal(t, generationBefore, target.Generation, "the recovery must be a status-only update") +} + +// awaitMirroredReady polls the rule's mirrored GitTargetReady until it reads want, or fails. +func awaitMirroredReady( + ctx context.Context, + t *testing.T, + key types.NamespacedName, + fresh func() client.Object, + want metav1.ConditionStatus, + budget time.Duration, +) { + t.Helper() + + deadline := time.Now().Add(budget) + last := "absent" + for time.Now().Before(deadline) { + obj := fresh() + if err := k8sClient.Get(ctx, key, obj); err == nil { + var conditions []metav1.Condition + switch typed := obj.(type) { + case *configbutleraiv1alpha3.WatchRule: + conditions = typed.Status.Conditions + case *configbutleraiv1alpha3.ClusterWatchRule: + conditions = typed.Status.Conditions + } + if c := apimeta.FindStatusCondition(conditions, controller.ConditionTypeGitTargetReady); c != nil { + if c.Status == want { + return + } + last = string(c.Status) + } + } + time.Sleep(50 * time.Millisecond) + } + t.Fatalf("GitTargetReady never reached %q within %s (last seen %q)", want, budget, last) +} + +// TestWatchRuleFollowsGitTargetStatusOnlyRecovery is the regression test for a rule whose mirrored +// GitTargetReady lagged its target by a full requeue. It fails if the GitTarget watch stops +// reacting to a status-only readiness move. +func TestWatchRuleFollowsGitTargetStatusOnlyRecovery(t *testing.T) { + ctx := context.Background() + startManager(t, func(m manager.Manager) error { + return (&controller.WatchRuleReconciler{ + Client: m.GetClient(), + Scheme: m.GetScheme(), + RuleStore: rulestore.NewStore(), + }).SetupWithManager(m) + }) + + namespace := seedTarget(ctx, t, "wiring-watchrule", "") + rule := &configbutleraiv1alpha3.WatchRule{ + ObjectMeta: metav1.ObjectMeta{Name: "rule", Namespace: namespace}, + Spec: configbutleraiv1alpha3.WatchRuleSpec{ + GitTargetRef: meta.LocalObjectReference{Name: "target"}, + Rules: []configbutleraiv1alpha3.ResourceRule{{Resources: []string{"configmaps"}}}, + }, + } + require.NoError(t, k8sClient.Create(ctx, rule)) + key := client.ObjectKeyFromObject(rule) + + // Drive the target to a known FAILING readiness and let the rule settle on it. + setTargetReady(ctx, t, namespace, metav1.ConditionFalse, "WatchError") + awaitMirroredReady(ctx, t, key, func() client.Object { + return &configbutleraiv1alpha3.WatchRule{} + }, metav1.ConditionFalse, settleBudget) + + // Heal it, status-only, and require the rule to follow inside the budget. + setTargetReady(ctx, t, namespace, metav1.ConditionTrue, "Succeeded") + awaitMirroredReady(ctx, t, key, func() client.Object { + return &configbutleraiv1alpha3.WatchRule{} + }, metav1.ConditionTrue, propagationBudget) +} + +// TestClusterWatchRuleFollowsGitTargetStatusOnlyRecovery is the same regression for the +// cluster-scoped rule, which carried the identical wiring. +func TestClusterWatchRuleFollowsGitTargetStatusOnlyRecovery(t *testing.T) { + ctx := context.Background() + startManager(t, func(m manager.Manager) error { + return (&controller.ClusterWatchRuleReconciler{ + Client: m.GetClient(), + Scheme: m.GetScheme(), + RuleStore: rulestore.NewStore(), + }).SetupWithManager(m) + }) + + namespace := seedTarget(ctx, t, "wiring-clusterwatchrule", "wiring-source") + rule := &configbutleraiv1alpha3.ClusterWatchRule{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster-rule"}, + Spec: configbutleraiv1alpha3.ClusterWatchRuleSpec{ + GitTargetRef: meta.NamespacedObjectReference{Name: "target", Namespace: namespace}, + Rules: []configbutleraiv1alpha3.ClusterResourceRule{{Resources: []string{"namespaces"}}}, + }, + } + require.NoError(t, k8sClient.Create(ctx, rule)) + key := client.ObjectKeyFromObject(rule) + + setTargetReady(ctx, t, namespace, metav1.ConditionFalse, "WatchError") + awaitMirroredReady(ctx, t, key, func() client.Object { + return &configbutleraiv1alpha3.ClusterWatchRule{} + }, metav1.ConditionFalse, settleBudget) + + setTargetReady(ctx, t, namespace, metav1.ConditionTrue, "Succeeded") + awaitMirroredReady(ctx, t, key, func() client.Object { + return &configbutleraiv1alpha3.ClusterWatchRule{} + }, metav1.ConditionTrue, propagationBudget) +} From ddcdb55cb83adaa221dd1a5d81cccc446da2fbf0 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Mon, 7 Sep 2026 12:50:44 +0000 Subject: [PATCH 5/6] docs: name the real template-selection unit, and gate the re-place route on prune Two corrections to text this branch introduced. Template selection counts the entries the commit window RETAINED, one per distinct destination path. It does not count documents changed, which is what the previous wording claimed: commitMetadata renders from PendingWrite.Events before the write decides what differs, so two retained entries of which only one differs still render through groupTemplate even though the commit changes one file. Say "retained resource entries" and regenerate the CRD descriptions. The re-place route needed a prune prerequisite. It deletes the cluster object and waits for the mirror to remove the document, which spec.prune.mode: Never never does, so the wait could not complete and the procedure stranded anyone who had set it. The claim that deleting a file in Git restores its former path is removed: it was wrong. The writer resolves an existing document through ManifestStore's identity index, which is rebuilt from the current folder scan, so a resource whose document is no longer there is placed as new under the current policy. That is now stated as the reason the route works at all. Co-Authored-By: Claude Opus 5 --- api/v1alpha3/gitprovider_types.go | 22 +++++----- .../crd/bases/configbutler.ai_gittargets.yaml | 22 +++++----- docs/configuration.md | 42 +++++++++++-------- 3 files changed, 49 insertions(+), 37 deletions(-) diff --git a/api/v1alpha3/gitprovider_types.go b/api/v1alpha3/gitprovider_types.go index 5c849bd5..fdca41c1 100644 --- a/api/v1alpha3/gitprovider_types.go +++ b/api/v1alpha3/gitprovider_types.go @@ -174,13 +174,13 @@ type CommitterSpec struct { // GitTarget.spec.commit.message; the identically-shaped GitProvider.spec.commit.message is // retained only to reject a manifest that still sets it there. type CommitMessageSpec struct { - // EventTemplate is a Go text/template string for the commit message of a commit that changes - // exactly ONE document. Which template renders a commit follows how many documents it changes, - // not the window setting: a "0s" window makes every commit single-document, but a window that - // closes around a single change renders through this template too. Note that repeated edits to - // one resource inside a window coalesce by destination path, so ten updates to one ConfigMap - // are ONE changed document and render here, not through GroupTemplate. With a non-zero window, - // set GroupTemplate as well: both shapes occur. + // EventTemplate is a Go text/template string for the commit message of a commit whose window + // retained exactly ONE resource entry. Selection counts retained entries, one per distinct + // destination path, not the window setting and not how many documents actually change: a "0s" + // window makes every commit single-entry, but a window that closes around a single change + // renders through this template too. Repeated edits to one resource coalesce, so ten updates to + // one ConfigMap are ONE entry and render here. With a non-zero window, set GroupTemplate as + // well: both shapes occur. // Available variables: Operation, Group, Version, Resource, Namespace, Name, // APIVersion, Username, GitTarget. // +optional @@ -196,9 +196,11 @@ type CommitMessageSpec struct { // +optional ReconcileTemplate string `json:"reconcileTemplate,omitempty"` - // GroupTemplate is a Go text/template string for the commit message of a commit that changes TWO - // OR MORE documents (one commit per (author, gitTarget) group produced by the batching - // pipeline). A group that changed only one document renders through EventTemplate instead. + // GroupTemplate is a Go text/template string for the commit message of a commit whose window + // retained TWO OR MORE resource entries (one commit per (author, gitTarget) group produced by + // the batching pipeline). A window that retained one entry renders through EventTemplate + // instead. Entries are counted before the write decides what differs, so an entry already + // matching Git still counts toward this template being chosen. // Available variables: Author, GitTarget, Count, Operations (map of // CREATE/UPDATE/DELETE counts), Resources (slice of {Group, Version, // Resource, Namespace, Name}). diff --git a/config/crd/bases/configbutler.ai_gittargets.yaml b/config/crd/bases/configbutler.ai_gittargets.yaml index 616000d3..08a26851 100644 --- a/config/crd/bases/configbutler.ai_gittargets.yaml +++ b/config/crd/bases/configbutler.ai_gittargets.yaml @@ -151,21 +151,23 @@ spec: properties: eventTemplate: description: |- - EventTemplate is a Go text/template string for the commit message of a commit that changes - exactly ONE document. Which template renders a commit follows how many documents it changes, - not the window setting: a "0s" window makes every commit single-document, but a window that - closes around a single change renders through this template too. Note that repeated edits to - one resource inside a window coalesce by destination path, so ten updates to one ConfigMap - are ONE changed document and render here, not through GroupTemplate. With a non-zero window, - set GroupTemplate as well: both shapes occur. + EventTemplate is a Go text/template string for the commit message of a commit whose window + retained exactly ONE resource entry. Selection counts retained entries, one per distinct + destination path, not the window setting and not how many documents actually change: a "0s" + window makes every commit single-entry, but a window that closes around a single change + renders through this template too. Repeated edits to one resource coalesce, so ten updates to + one ConfigMap are ONE entry and render here. With a non-zero window, set GroupTemplate as + well: both shapes occur. Available variables: Operation, Group, Version, Resource, Namespace, Name, APIVersion, Username, GitTarget. type: string groupTemplate: description: |- - GroupTemplate is a Go text/template string for the commit message of a commit that changes TWO - OR MORE documents (one commit per (author, gitTarget) group produced by the batching - pipeline). A group that changed only one document renders through EventTemplate instead. + GroupTemplate is a Go text/template string for the commit message of a commit whose window + retained TWO OR MORE resource entries (one commit per (author, gitTarget) group produced by + the batching pipeline). A window that retained one entry renders through EventTemplate + instead. Entries are counted before the write decides what differs, so an entry already + matching Git still counts toward this template being chosen. Available variables: Author, GitTarget, Count, Operations (map of CREATE/UPDATE/DELETE counts), Resources (slice of {Group, Version, Resource, Namespace, Name}). diff --git a/docs/configuration.md b/docs/configuration.md index b34e56f4..c5ffacdc 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -521,22 +521,25 @@ write rather than stopping the mirror. #### Commit message templates -There are three templates. **Which one renders a commit is decided by how many documents that commit -changes, not by the window setting:** +There are three templates. **Which one renders a commit is decided by how many resource entries the +commit window retained** (one per distinct destination path), not by the window setting: -- `spec.commit.message.eventTemplate`: a commit that changes exactly **one** document. -- `spec.commit.message.groupTemplate`: a commit that changes **two or more**. +- `spec.commit.message.eventTemplate`: a commit whose window retained exactly **one** entry. +- `spec.commit.message.groupTemplate`: a commit whose window retained **two or more**. - `spec.commit.message.reconcileTemplate`: reconcile commits (the mark-and-sweep reconcile path; one commit per synced type). -Two consequences are easy to get wrong, and both follow from the window coalescing by destination -path before anything is rendered: +"Retained entries" is the precise unit, and it is neither of the two things it is easily read as: - **A `0s` window is not the only way to reach `eventTemplate`.** A window that closes around a single change renders through it too, which is most edits a human makes by hand. -- **"One document" is not "one action".** Repeated edits to the same resource inside one window - collapse to a single entry, last write wins. Ten updates to one ConfigMap are one changed - document, so they render through `eventTemplate`, and `groupTemplate` never sees them. +- **An entry is not an action.** Repeated edits to one resource inside a window collapse to a single + entry, last write wins. Ten updates to one ConfigMap are one retained entry, so they render + through `eventTemplate` and `groupTemplate` never sees them. +- **An entry is not a changed document either.** Selection counts entries before the write decides + what actually differs, so an entry whose content already matches Git still counts. Two retained + entries of which only one differs render through `groupTemplate`, even though the commit changes + one file. So with a non-zero window you want **both** `eventTemplate` and `groupTemplate` set. Setting only one leaves the other shape rendering through the built-in wording, and both shapes occur in normal @@ -829,19 +832,24 @@ you change `spec.placement` afterwards. Placement governs the first write and no a description of current behavior rather than a promise: a deliberate migration that moves documents in a new commit is a coherent feature and is not ruled out. -To re-place a document by hand today, remove it and let the mirror write it again: +To re-place a document by hand today, get it out of the folder and let the mirror write it again. +The writer rebuilds its identity index from the current scan of the folder, so a resource with no +document there is placed as new, under the current policy. + +**Check `spec.prune.mode` first.** The route below deletes the cluster object and relies on the +mirror removing its document, which needs `OnEvent` (the default) or `Always`. Under `Never` the +mirror removes nothing, so step 2 never completes and you would be left waiting on a deletion that +is not coming. 1. Delete the object in the cluster. 2. **Wait until the deletion is committed and pushed** and the document is gone from the branch. -3. Recreate the object. The new write has no document to update, so it goes through the current +3. Recreate the object. It now has no document to update, so the write goes through the current placement policy. -Step 2 is not a formality. Delete and recreate inside one commit window, by the same author, -coalesce by destination path into a single entry, so the pair cancels out: the deletion never -reaches Git, the document is still there, and the recreated object is updated in place at the old -location. From the outside this looks exactly like placement being ignored. Deleting the file in -Git alone does not work either, since the next reconcile restores it at the path the object's -document identity already resolves to. +Step 2 is not a formality. A delete and a recreate inside one commit window, by the same author, +coalesce by destination path into a single retained entry, so the pair cancels out: the deletion +never reaches Git, the document is still there, and the recreated object is updated in place at its +old location. From the outside that looks exactly like placement being ignored. #### How a path is chosen (the resolution ladder) From 334ea0d4cce9009fed9d3acbe6b89fd4195f36b1 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Mon, 7 Sep 2026 13:46:17 +0000 Subject: [PATCH 6/6] chore: apply three review conventions to the new entry and wiring tests All three hold against the repo's own rules, so all three are applied. The 0.41.1 UPGRADING heading was the only shipped-release heading on the page in past tense; every neighbour reads "attribution facts travel", "the attribution metrics are relabelled", "scope is removed". The entry now leads with the release as the actor, which keeps the historical fact while matching the page. TestMain had no godoc, and AGENTS.md requires one on every export. The two wiring tests are renamed to TestWatchRule_FollowsGitTargetStatusOnlyRecovery and TestClusterWatchRule_FollowsGitTargetStatusOnlyRecovery. AGENTS.md states the TestFunctionName_Scenario convention and 1580 of 1800 test functions in the tree follow it, so this is the house style rather than a preference. Co-Authored-By: Claude Opus 5 --- docs/UPGRADING.md | 8 ++++---- internal/controller/wiring/readiness_wiring_test.go | 9 +++++---- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index 993a1db1..4ce667f4 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -676,11 +676,11 @@ different one the namespace is now written explicitly — omitting it would have kustomize and rendered a different object than the one being mirrored. This also now applies to a path you declared, which previously wrote a `namespace:` line the rest of that folder omits. -## 0.41.1 — the Go floor moved (patch release; breaking for module consumers) +## 0.41.1 — the Go floor rises for module consumers (patch release; breaking for module consumers) -The `go` directive in `go.mod` moved from `1.26.5` to `1.27.0` in `v0.41.1`, a release whose -changelog entry reads only *"let's release some security updates"*. It has moved again since. The -directive in [`go.mod`](../go.mod) is always the source of truth; the README carries the current +`v0.41.1` raises the `go` directive in `go.mod` from `1.26.5` to `1.27.0`, in a release whose +changelog entry reads only *"let's release some security updates"*. The floor rises again after it. +The directive in [`go.mod`](../go.mod) is always the source of truth; the README carries the current floor. **If you run the image, this does not affect you.** The toolchain is baked into the build. diff --git a/internal/controller/wiring/readiness_wiring_test.go b/internal/controller/wiring/readiness_wiring_test.go index 6e1da319..41e7d1fd 100644 --- a/internal/controller/wiring/readiness_wiring_test.go +++ b/internal/controller/wiring/readiness_wiring_test.go @@ -52,6 +52,7 @@ var ( restCfg *rest.Config ) +// TestMain bootstraps the shared envtest environment the wiring tests run against. func TestMain(m *testing.M) { os.Exit(run(m)) } @@ -218,10 +219,10 @@ func awaitMirroredReady( t.Fatalf("GitTargetReady never reached %q within %s (last seen %q)", want, budget, last) } -// TestWatchRuleFollowsGitTargetStatusOnlyRecovery is the regression test for a rule whose mirrored +// TestWatchRule_FollowsGitTargetStatusOnlyRecovery is the regression test for a rule whose mirrored // GitTargetReady lagged its target by a full requeue. It fails if the GitTarget watch stops // reacting to a status-only readiness move. -func TestWatchRuleFollowsGitTargetStatusOnlyRecovery(t *testing.T) { +func TestWatchRule_FollowsGitTargetStatusOnlyRecovery(t *testing.T) { ctx := context.Background() startManager(t, func(m manager.Manager) error { return (&controller.WatchRuleReconciler{ @@ -255,9 +256,9 @@ func TestWatchRuleFollowsGitTargetStatusOnlyRecovery(t *testing.T) { }, metav1.ConditionTrue, propagationBudget) } -// TestClusterWatchRuleFollowsGitTargetStatusOnlyRecovery is the same regression for the +// TestClusterWatchRule_FollowsGitTargetStatusOnlyRecovery is the same regression for the // cluster-scoped rule, which carried the identical wiring. -func TestClusterWatchRuleFollowsGitTargetStatusOnlyRecovery(t *testing.T) { +func TestClusterWatchRule_FollowsGitTargetStatusOnlyRecovery(t *testing.T) { ctx := context.Background() startManager(t, func(m manager.Manager) error { return (&controller.ClusterWatchRuleReconciler{