diff --git a/api/v1alpha3/commitrequest_schema_test.go b/api/v1alpha3/commitrequest_schema_test.go index f6e92dd1..2f8ac120 100644 --- a/api/v1alpha3/commitrequest_schema_test.go +++ b/api/v1alpha3/commitrequest_schema_test.go @@ -19,7 +19,7 @@ import ( // TestCommitRequestSchemaRevision: raise CommitRequestSchemaRevision, and the revision line, and add // the new hash here. var commitRequestSchemaHashes = map[int]string{ - 1: "ea99cba6116e0f562c0be931c13d6580eabdc2e7ba32d486d20a0a5029163e99", + 1: "395df39c86bc4c0350eb2c476dd0ae02db2a7513f03962103416123b0cbb5476", } // commitRequestRevisionLine is the last line of the kind's description. diff --git a/api/v1alpha3/commitrequest_types.go b/api/v1alpha3/commitrequest_types.go index 4bfc6b3c..147ef5c7 100644 --- a/api/v1alpha3/commitrequest_types.go +++ b/api/v1alpha3/commitrequest_types.go @@ -228,6 +228,9 @@ type CommitRequestStatus struct { // holds the saved objects. // - False, reason CommitDropped: the branch no longer holds the commit; // its history was rewritten after the push. Only this is False. + // - Unknown, reason TimedOut: the apply did not settle within 10 minutes + // of Ready, so the request stopped following it. The commit stays + // pushed; the message says where the apply last stood. // - Unknown, reason Lost: GitOps Reverser restarted before it knew. // +optional // +listType=map diff --git a/charts/gitops-reverser/README.md b/charts/gitops-reverser/README.md index 24da08ae..47c3d314 100644 --- a/charts/gitops-reverser/README.md +++ b/charts/gitops-reverser/README.md @@ -345,6 +345,11 @@ curl http://localhost:8080/metrics ## Upgrading +Before upgrading, read the [upgrade guide](../../docs/UPGRADING.md) entries for every release you +cross, and follow their steps in order. Upgrading from 0.54.0 starts with +[draining the `CommitRequest`s](../../docs/UPGRADING.md#upgrading-from-0540-drain-the-commitrequests-first). +Then apply the new release's `crds.yaml` ([CRD lifecycle](#crd-lifecycle)), and upgrade. + ### Standard Upgrade ```bash diff --git a/config/crd/bases/configbutler.ai_commitrequests.yaml b/config/crd/bases/configbutler.ai_commitrequests.yaml index 64e4b8eb..ce638139 100644 --- a/config/crd/bases/configbutler.ai_commitrequests.yaml +++ b/config/crd/bases/configbutler.ai_commitrequests.yaml @@ -289,6 +289,9 @@ spec: holds the saved objects. - False, reason CommitDropped: the branch no longer holds the commit; its history was rewritten after the push. Only this is False. + - Unknown, reason TimedOut: the apply did not settle within 10 minutes + of Ready, so the request stopped following it. The commit stays + pushed; the message says where the apply last stood. - Unknown, reason Lost: GitOps Reverser restarted before it knew. items: description: Condition contains details for one aspect of the current diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index 6c1ae59d..e482c909 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -7,6 +7,53 @@ guidance that the changelog's breaking-change entries link to. We are pre-1.0, so breaking changes bump the **minor** version (release-please is configured with `bump-minor-pre-major`) rather than the major. Read the relevant entry before upgrading across it. +## Upgrading from 0.54.0: drain the `CommitRequest`s first + +Existing providers, branches, targets and claims keep running across this upgrade; none of the +0.53.0 recreation [below](#upgrading-from-0530-recreate-the-connection-objects-in-one-pass) applies. +What needs ordering is the `CommitRequest`s that are in progress: 0.54.0 wrote progress conditions +this release does not take over, so a request still open when it starts can stay open for good. + +1. Stop creating `CommitRequest`s: pause the automation and people that create them. +2. Wait for every request to end. This lists the ones that have not, and should come back empty + once each has passed its `spec.window` bounds: + + ```bash + kubectl get commitrequests -A -o json | jq -r '.items[] + | select([.status.conditions[]? | select((.type == "Ready" or .type == "Stalled") + and .status == "True")] | length == 0) + | "\(.metadata.namespace)/\(.metadata.name)"' + ``` + +3. Adjust the audit policy and values, as the [entry below](#the-admission-webhook-is-gone-a-commitrequests-author-comes-from-audit) + describes: `commitrequests` `create` and `patch` at `RequestResponse` level, and no + `servers.admission` or `--admission-webhook*` settings. +4. Apply the release's `crds.yaml`, then upgrade, as the + [chart's upgrade steps](../charts/gitops-reverser/README.md#upgrading) describe. +5. Remove the admission webhook's leftovers. + - **Helm** deletes the `ValidatingWebhookConfiguration` and the `Certificate` it rendered, but + not the Secret cert-manager wrote for that certificate. Delete it: + + ```bash + kubectl -n gitops-reverser delete secret gitops-reverser-admission-server-cert --ignore-not-found + ``` + + - **`install.yaml`** is applied with `kubectl apply`, which removes nothing. Delete the three + resources yourself: + + ```bash + kubectl delete validatingwebhookconfiguration gitops-reverser-validate-operator-types --ignore-not-found + kubectl -n gitops-reverser delete certificate gitops-reverser-admission-server-cert --ignore-not-found + kubectl -n gitops-reverser delete secret gitops-reverser-admission-server-cert --ignore-not-found + ``` + + The names are those of a release named `gitops-reverser` in namespace `gitops-reverser`; use + yours if they differ. +6. Run the command from step 2 again. A request it still lists was open across the upgrade and may + never end; one made on the cluster GitOps Reverser runs in, for a `GitTarget` that mirrors another + cluster, certainly will not. Delete each, and create it again: on the cluster the `GitTarget` + mirrors, for such a `GitTarget`. Then resume creating requests. + ## The admission webhook is gone; a `CommitRequest`'s author comes from audit GitOps Reverser installs no admission webhook. The chart no longer renders the @@ -22,8 +69,10 @@ those that remain expire on their own. certificate with `servers.metrics.tls.enabled`. The default install needs no cert-manager. - **A request's author.** A `CommitRequest`'s author comes from its own audit event, as a write's does, and is reported in `status.author`. The `AuthorAttributed` condition is gone. Keep `create` - of `commitrequests` in your audit policy; the write catch-all of the - [tuned policy](../test/e2e/cluster/audit/policy.yaml) covers it. A request no audit event names, on + and `patch` of `commitrequests` in your audit policy at `RequestResponse` level: the author is + matched by the uid and resourceVersion in the response, which `Metadata` level leaves out, and + server-side apply creates with a `patch`. The write catch-all of the + [tuned policy](../test/e2e/cluster/audit/policy.yaml) covers both. A request no audit event names, on a cluster that sends audit events, ends `Ready=True` with reason `AuthorUnresolved` and saves nothing. With attribution off, a request names nobody, as it did with the webhook off. - **Status while it runs.** A `CommitRequest` in progress is `Ready=Unknown`, reason `Saving` until the diff --git a/docs/architecture.md b/docs/architecture.md index 4081a098..438712e1 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -172,8 +172,9 @@ the Flux `Kustomization` applier and the Argo CD `Application` applier, its auto **Redis/Valkey is optional but advised.** The default configured-author mode runs without it: a plain `helm install` comes up healthy and watches initialize from current state on startup. When an endpoint -is configured, Redis stores cursors for reconnecting running watches and the coordination records used by -CommitRequest author capture and HA. Attribution does not require it on its own: its facts travel on a +is configured, Redis stores cursors for reconnecting running watches. A CommitRequest's author comes from +the audit fact of the write that created it, like any write's, and needs no Redis record. +Attribution does not require it on its own: its facts travel on a selectable transport, Redis Streams by default and an in-process ring with `--author-attribution-transport=memory`, which is refused with more than one replica. HA will require Redis as the shared store across replicas. diff --git a/docs/attribution-setup-guide.md b/docs/attribution-setup-guide.md index 62341e03..0a11b425 100644 --- a/docs/attribution-setup-guide.md +++ b/docs/attribution-setup-guide.md @@ -120,10 +120,12 @@ other subresources. Keep manual scale writes if you want their authors recorded. The example includes Secret writes at `RequestResponse` level. To keep Secret bodies out of the webhook, exclude core `secrets` too. Those writes then have no audit evidence for attribution. -Keep `create` of `commitrequests` in the `configbutler.ai` group covered, at `Metadata` level at -least: a `CommitRequest`'s author comes from its own audit event, as a write's does. Without it a -request names nobody, and cannot save the window of an author the audit names. The write catch-all -covers it. +Keep `create` and `patch` of `commitrequests` in the `configbutler.ai` group covered at +`RequestResponse` level. A `CommitRequest`'s author comes from the audit event that created it, +matched by the uid and resourceVersion in the event's `responseObject`. `Metadata` level leaves +the response out, so the request cannot be matched. `patch` is there because server-side apply +creates an object with a `patch`. Without the creating event, a request names nobody and cannot +save the window of an author the audit names. The write catch-all covers both verbs. The audit policy controls evidence collection; `WatchRule` and `ClusterWatchRule` control what gets written to Git. diff --git a/docs/configuration.md b/docs/configuration.md index 2749da5b..c310b351 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -3059,9 +3059,12 @@ separates work the worker already collected from work that reaches it afterward. write was made after the request: a write still held for its audit fact arrives later and lands in the next window. -Competing requests from one author are served first come, first served. A window carries at most -one request; the next waits for the next window. A re-sent request is recognized by its identity -and restarts nothing: its deadlines and, for `Next`, its one close are fixed at first registration. +Competing requests from one author are not queued. A window carries one message, so while an +earlier request of the same author on the same `GitTarget` waits for a window or collects one, a +later request is refused at once with [`SaveInProgress`](#its-status). A `Next` request is not +refused by one that is collecting: it closes that window under the earlier request's message, then +waits for its own. A re-sent request is recognized by its identity and restarts nothing: its +deadlines and, for `Next`, its one close are fixed at first registration. #### Sizing the request window @@ -3255,6 +3258,25 @@ request produced a pushed commit: A request is served again after GitOps Reverser restarts: the path that owns it finds its own claim, and resumes with the author it recorded. +### How long a request lives + +A `CommitRequest` is a single save, not a standing order: every stage it passes through ends on its +own, and the status says which limit ended it. To save again, create a new request. + +| Stage | Ends after | What the request then says | +| --- | --- | --- | +| Picked up, author named, waiting for a branch worker | `attachTimeout` + `maxDuration` + 2 minutes, counted from creation | `Ready=False`, `Stalled=True`, reason `FinalizeFailed`: the `GitTarget` did not start a branch worker, or the request was not acted on, within the safety window | +| Waiting for a window (`WaitingForWindow`) | `attachTimeout` | `Ready=True`, reason `NoWindow` (see [When there is nothing to commit](#when-there-is-nothing-to-commit)) | +| Collecting its window (`CollectingWindow`) | `maxDuration`, at most `5m` | the window closes and is committed | +| Waiting for the push (`WaitingForPush`) | no limit of its own | stays `Ready=Unknown`; `Reconciling` says why the push waits | +| Waiting for its apply, with `catchUp: AfterPush` | 10 minutes from `Ready` | `Applied=Unknown`, reason `TimedOut` (see [Applying a save at once](#applying-a-save-at-once)) | +| Finished | 48 hours, by default | deleted (see [Finished requests are deleted after 48 hours](#finished-requests-are-deleted-after-48-hours)) | + +The push is the one wait without a limit, deliberately. Once the window is committed, the commit is +going to the remote: failing the request while the worker still retries, every 10 seconds doubling +to 5 minutes, would report a save as failed that then lands. A worker that stops before the push +fails the request instead. A request that waits there needs the remote fixed, not a new request. + ### Applying a save at once A save is in Git once it is pushed, and the cluster already runs it, so by default nothing applies @@ -3272,10 +3294,13 @@ already at the tip needs none. The request reports the outcome in its `Applied` | `True` | `Applied` | the applier applied a revision that holds the commit; `status.appliedRevision` names it, as the applier reported it | | `True` | `NothingToApply`, `NoApplierClaim`, `ApplierGone` | no apply was needed: the request pushed no commit, or the `GitTarget` names no applier claim, or stopped naming the one asked. The cluster already holds what was saved | | `False` | `CommitDropped` | the branch no longer holds the commit: its history was rewritten after the push. The only `False` | +| `Unknown` | `TimedOut` | the apply did not settle within 10 minutes of `Ready`, so the request stopped following it. The commit stays pushed, and the applier may still apply it; the message says where the apply last stood, and the applier claim's status reports later applies | | `Unknown` | `Lost` | GitOps Reverser restarted before it learnt how the apply went | -Wait for `Applied=True` for "saved and applied"; `Ready=True` stays "saved". A finished request is -deleted only after its `Applied` condition is settled. +Wait for `Applied=True` for "saved and applied"; `Ready=True` stays "saved". A request follows its +apply for 10 minutes at most: an applier that cannot catch up does not keep it open, and a request +that ends `TimedOut` needs the applier looked at, not a new request. A finished request is deleted +only after its `Applied` condition is settled. ### Finished requests are deleted after 48 hours diff --git a/docs/design/commit-timing-surface.md b/docs/design/commit-timing-surface.md index a76ff757..d9ee7cca 100644 --- a/docs/design/commit-timing-surface.md +++ b/docs/design/commit-timing-surface.md @@ -166,8 +166,10 @@ Two rules keep the edges honest: never closes a second window. - **`Next` needs time to wait.** CEL refuses `attach: Next` with `attachTimeout: 0s`: the save would close the author's window and give up in the same instant, collecting nothing. -- **Competing saves are served first come, first served,** in the order the worker first registered - them. A window carries at most one save; the next waits for the next window. +- **Competing saves are not queued.** A window carries one message, so a later save of the same + author on the same `GitTarget` is refused with `SaveInProgress` while the earlier one waits or + collects. A `Next` save closes a collecting window under the earlier message, then waits for its + own. What `Next` guarantees is narrower than its name suggests. It separates work the worker already collected from work that reaches the worker afterward. It cannot prove a write was made after the diff --git a/docs/spec/commitrequest-design.md b/docs/spec/commitrequest-design.md index f3d89339..50d16c1e 100644 --- a/docs/spec/commitrequest-design.md +++ b/docs/spec/commitrequest-design.md @@ -102,9 +102,9 @@ A window is only ever opened by a write; a request attaches to one and never ope registration, the worker sets the attach deadline to registration plus `window.attachTimeout`, and repeated reconciles are idempotent: they restart nothing and, for `attach: Next`, close nothing again. With `CurrentOrNext` the request attaches to the author's window already open at registration, before -any deadline is consulted, so `attachTimeout: 0s` can attach at all. Otherwise it waits, and waiting -requests are served first come, first served. A request whose deadline has passed resolves and never -takes a later window; when its deadline and a matching write are ready on the same wake, expiry wins. +any deadline is consulted, so `attachTimeout: 0s` can attach at all. Otherwise it waits. A request +whose deadline has passed resolves and never takes a later window; when its deadline and a matching +write are ready on the same wake, expiry wins. On attach, the request's `idleTimeout` and `maxDuration` replace the window's timers: `maxDuration` runs from the attach, and so does the first idle interval. A waiting request gets a new window before @@ -129,7 +129,9 @@ its first edits before they reach Git. A Git host's push notice leaves an attach Each request attaches to at most one window and cannot rename a finalized commit, including one waiting for push. A bundle provides no ordering guarantee. The wait does not reserve a transaction. Competing requests are -served in registration order, first come, first served. +not queued: a later request of the same author on the same `GitTarget` is refused with +`SaveInProgress` while the earlier one waits or collects, except that a `Next` request closes a +collecting window under the earlier message and then waits for its own. `spec.message` is literal, including template-like text and surrounding spaces. It is never parsed as a template, so a request author cannot execute one. Omission uses diff --git a/internal/controller/commitrequest_apply.go b/internal/controller/commitrequest_apply.go index 20dfd60e..6c919749 100644 --- a/internal/controller/commitrequest_apply.go +++ b/internal/controller/commitrequest_apply.go @@ -21,12 +21,16 @@ import ( // save's alone and never changes here. // crReasonApplyLost is the Applied reason for an apply the worker knows nothing of: GitOps Reverser -// restarted before it learnt how it went, or the record aged out. +// restarted before it learnt how it went. const crReasonApplyLost = "Lost" +// crReasonApplyTimedOut is the Applied reason for an apply that did not settle within +// git.SaveApplyTimeout of Ready: the request stops following it. +const crReasonApplyTimedOut = "TimedOut" + // applyLostAfter is how long, from Ready, a request may find its worker knowing nothing of its apply -// before Applied says it is lost. The worker records the apply as the request resolves, so only a -// restart, or a record past its TTL, leaves it unknown. +// before Applied says it is lost. The worker records the apply as the request resolves, and keeps +// it past git.SaveApplyTimeout, so only a restart leaves it unknown. const applyLostAfter = time.Minute // followsApply reports whether a finished request still reports its apply: it asked for one, its @@ -36,10 +40,12 @@ func followsApply(cr *configbutleraiv1alpha3.CommitRequest) bool { conditionIsTrue(cr.Status.Conditions, ConditionTypeReady) && !applyReported(cr) } -// applyReported reports whether Applied is settled: True, False, or Unknown because it was lost. +// applyReported reports whether Applied is settled: True, False, or Unknown because it was lost or +// timed out. func applyReported(cr *configbutleraiv1alpha3.CommitRequest) bool { c := apimeta.FindStatusCondition(cr.Status.Conditions, ConditionTypeApplied) - return c != nil && (c.Status != metav1.ConditionUnknown || c.Reason == crReasonApplyLost) + return c != nil && (c.Status != metav1.ConditionUnknown || + c.Reason == crReasonApplyLost || c.Reason == crReasonApplyTimedOut) } // followApply reads where the request's apply stands, writes it to Applied, and comes back until it @@ -48,7 +54,7 @@ func applyReported(cr *configbutleraiv1alpha3.CommitRequest) bool { // // A request that pushed no commit is settled here, without asking its worker: one that ended before // it reached a worker, such as AuthorUnresolved, is one its worker never hears of, and would -// otherwise be reported lost. +// otherwise be reported lost. One not settled git.SaveApplyTimeout after Ready times out. func (c *commitRequestCore) followApply( ctx context.Context, p commitRequestPath, cr *configbutleraiv1alpha3.CommitRequest, ) (time.Duration, error) { @@ -66,8 +72,11 @@ func (c *commitRequestCore) followApply( "name", client.ObjectKeyFromObject(cr), "err", err.Error()) return commitRequestPollInterval, nil } - if !known { + switch { + case !known: apply = unknownApply(cr) + case !apply.Done && readyFor(cr) >= git.SaveApplyTimeout: + apply = timedOutApply(cr, apply) } } before := cr.Status.DeepCopy() @@ -97,12 +106,29 @@ func nothingToApply() git.SaveApply { // unknownApply is the apply of a request its worker knows nothing of: still being taken on, shortly // after Ready, and lost after applyLostAfter. func unknownApply(cr *configbutleraiv1alpha3.CommitRequest) git.SaveApply { - if ready := apimeta.FindStatusCondition(cr.Status.Conditions, ConditionTypeReady); ready != nil && - time.Since(ready.LastTransitionTime.Time) < applyLostAfter { + if readyFor(cr) < applyLostAfter { return git.SaveApply{Reason: git.SaveApplyApplying, Message: "waiting for the branch worker to take the apply"} } return git.SaveApply{Reason: crReasonApplyLost, - Message: "GitOps Reverser does not know how the apply went: it restarted before it learnt, or kept no record"} + Message: "GitOps Reverser does not know how the apply went: it restarted before it learnt"} +} + +// timedOutApply is the apply of a request whose applier did not apply its commit within +// git.SaveApplyTimeout of Ready, last standing at apply. +func timedOutApply(cr *configbutleraiv1alpha3.CommitRequest, apply git.SaveApply) git.SaveApply { + return git.SaveApply{Reason: crReasonApplyTimedOut, + Message: "the applier did not apply commit " + cr.Status.Commit + " within " + git.SaveApplyTimeout.String() + + " of the push, so this request stopped following it; the commit stays pushed, and the GitTarget's " + + "applier claim reports its applies. Last seen: " + apply.Message} +} + +// readyFor is how long the request has been Ready, zero while it is not. +func readyFor(cr *configbutleraiv1alpha3.CommitRequest) time.Duration { + ready := apimeta.FindStatusCondition(cr.Status.Conditions, ConditionTypeReady) + if ready == nil || ready.Status != metav1.ConditionTrue { + return 0 + } + return time.Since(ready.LastTransitionTime.Time) } // setAppliedCondition writes apply to Applied and status.appliedRevision. diff --git a/internal/controller/commitrequest_apply_test.go b/internal/controller/commitrequest_apply_test.go index 933f5b4c..658fe3ae 100644 --- a/internal/controller/commitrequest_apply_test.go +++ b/internal/controller/commitrequest_apply_test.go @@ -119,6 +119,31 @@ func TestCommitRequestReconcile_AnApplyNobodyKnowsIsLost(t *testing.T) { assert.False(t, followsApply(&got), "lost is settled") } +// TestCommitRequestReconcile_AnApplyThatDoesNotSettleTimesOut: an apply still running when +// git.SaveApplyTimeout has passed since Ready stops being followed, and Applied says why. +func TestCommitRequestReconcile_AnApplyThatDoesNotSettleTimesOut(t *testing.T) { + c := newCommitRequestClient(t, nil, catchingUp(newCommitRequest("save-1"))) + f := committedFinalizer() + f.apply, f.applyKnown = git.SaveApply{Reason: git.SaveApplyRetrying, Message: "the apply failed"}, true + r := &CommitRequestReconciler{Client: c, APIReader: c, Finalizer: f} + reconcileCommitRequest(t, r, "save-1") + + reconcileCommitRequest(t, r, "save-1") + got := fetchCommitRequest(t, c, "save-1") + requireCondition(t, got, ConditionTypeApplied, metav1.ConditionUnknown, git.SaveApplyRetrying) + + ready := apimeta.FindStatusCondition(got.Status.Conditions, ConditionTypeReady) + ready.LastTransitionTime = metav1.NewTime(time.Now().Add(-git.SaveApplyTimeout)) + require.NoError(t, c.Status().Update(context.Background(), &got)) + reconcileCommitRequest(t, r, "save-1") + got = fetchCommitRequest(t, c, "save-1") + requireCondition(t, got, ConditionTypeApplied, metav1.ConditionUnknown, crReasonApplyTimedOut) + applied := apimeta.FindStatusCondition(got.Status.Conditions, ConditionTypeApplied) + assert.Contains(t, applied.Message, "within 10m0s") + assert.Contains(t, applied.Message, "the apply failed", "it says where the apply last stood") + assert.False(t, followsApply(&got), "a timeout is settled") +} + // TestCommitRequestReconcile_NoCatchUpReportsNoApply: the default asks no apply, and has no Applied. func TestCommitRequestReconcile_NoCatchUpReportsNoApply(t *testing.T) { c := newCommitRequestClient(t, nil, newCommitRequest("save-1")) diff --git a/internal/controller/commitrequest_server.go b/internal/controller/commitrequest_server.go index dbc0ba8b..e01cdfde 100644 --- a/internal/controller/commitrequest_server.go +++ b/internal/controller/commitrequest_server.go @@ -108,7 +108,9 @@ type CommitRequestServer struct { watchers map[k8stypes.NamespacedName]*commitRequestWatcher reports map[k8stypes.NamespacedName]CommitRequestsReport events chan event.GenericEvent - streams commitRequestStreams + + streamsOnce sync.Once + streams *commitRequestStreams } // commitRequestWatcher is the watch of one KubeProvider's CommitRequests, in the namespaces it lends. @@ -586,11 +588,9 @@ func (s *CommitRequestServer) consider( } func (s *CommitRequestServer) core() *commitRequestCore { - s.mu.Lock() - s.streams.set = s.FactStreams - s.mu.Unlock() + s.streamsOnce.Do(func() { s.streams = &commitRequestStreams{set: s.FactStreams} }) return &commitRequestCore{ - reader: s.Client, finalizer: s.Finalizer, authors: s.Authors, streams: &s.streams, ttl: s.TTL, + reader: s.Client, finalizer: s.Finalizer, authors: s.Authors, streams: s.streams, ttl: s.TTL, } } diff --git a/internal/controller/commitrequest_server_test.go b/internal/controller/commitrequest_server_test.go index 756f02e7..67215634 100644 --- a/internal/controller/commitrequest_server_test.go +++ b/internal/controller/commitrequest_server_test.go @@ -30,6 +30,7 @@ import ( configv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" "github.com/ConfigButler/gitops-reverser/internal/git" + "github.com/ConfigButler/gitops-reverser/internal/queue" ) // fakeDiscovery serves resources, and fails with err when it is set. @@ -398,3 +399,18 @@ func (f *fixedCommitRequestsReporter) CommitRequestsReport(types.NamespacedName) } func (f *fixedCommitRequestsReporter) CommitRequestsEvents() <-chan event.GenericEvent { return nil } + +// Requests are served concurrently, and each step reads the server's fact streams: they are set +// once, never rewritten under a lock the readers do not take. Run with -race. +func TestCommitRequestServer_StepsShareTheFactStreams(t *testing.T) { + s := &CommitRequestServer{FactStreams: queue.NewFactStreamSet()} + var wg sync.WaitGroup + for i := range 8 { + wg.Go(func() { + s.core().streams.ensure([]string{"route-a", "route-b"}[i%2]) + }) + } + wg.Wait() + first := s.core().streams + assert.Same(t, first, s.core().streams, "every step shares one set of streams") +} diff --git a/internal/git/save_apply.go b/internal/git/save_apply.go index e9149957..97434096 100644 --- a/internal/git/save_apply.go +++ b/internal/git/save_apply.go @@ -23,6 +23,10 @@ import ( // carries them again. A save that pushed no commit, whose GitTarget names no applier, or whose applier // stops being named is settled as applied: no apply is needed, and the cluster, where the saved objects // came from, already holds them. Only a save whose commit the branch lost is settled as not applied. +// +// A save's apply is followed for SaveApplyTimeout at most: the controller then stops following it +// and says so in Applied. The worker keeps a record that is not settled at least that long, so it +// never ages out while the controller still reads it. // Why a save's apply was settled, or why it still waits: the Applied condition's reasons. const ( @@ -35,6 +39,11 @@ const ( SaveApplyCommitDropped = "CommitDropped" ) +// SaveApplyTimeout is how long, from Ready, the controller follows the apply a CommitRequest asked +// for. A save is not kept waiting on an applier that does not catch up: past it, Applied settles as +// timed out, though the commit stays pushed and the applier may still apply it. +const SaveApplyTimeout = 10 * time.Minute + // SaveApply is how the apply a CommitRequest asked for stands. type SaveApply struct { // Done is whether it is settled: it changes no more. @@ -50,7 +59,9 @@ type SaveApply struct { } type saveApplyEntry struct { - apply SaveApply + apply SaveApply + // askedAt is when the save asked for its apply, and updatedAt when apply was last recorded. + askedAt time.Time updatedAt time.Time } @@ -62,7 +73,12 @@ func (w *BranchWorker) recordSaveApply(id commitRequestID, apply SaveApply) { if w.crApplies == nil { w.crApplies = map[commitRequestID]saveApplyEntry{} } - w.crApplies[id] = saveApplyEntry{apply: apply, updatedAt: time.Now()} + now := time.Now() + askedAt := now + if entry, ok := w.crApplies[id]; ok { + askedAt = entry.askedAt + } + w.crApplies[id] = saveApplyEntry{apply: apply, askedAt: askedAt, updatedAt: now} } // LookupSaveApply reports where the apply a CommitRequest asked for stands, and false while the @@ -74,10 +90,17 @@ func (w *BranchWorker) LookupSaveApply(namespace, name, uid string) (SaveApply, return entry.apply, ok } -// collectSaveApplies drops the entries older than the outcome TTL. The caller holds crOutcomesMu. +// collectSaveApplies drops a settled entry once it is older than the outcome TTL. One not settled is +// kept the outcome TTL past SaveApplyTimeout, counted from when it was asked: the controller reads +// Ready within the outcome TTL of that, and stops following at SaveApplyTimeout from Ready. The +// caller holds crOutcomesMu. func (w *BranchWorker) collectSaveApplies(now time.Time) { for id, entry := range w.crApplies { - if now.Sub(entry.updatedAt) > commitRequestOutcomeTTL { + expired := now.Sub(entry.updatedAt) > commitRequestOutcomeTTL + if !entry.apply.Done { + expired = now.Sub(entry.askedAt) > commitRequestOutcomeTTL+SaveApplyTimeout + } + if expired { delete(w.crApplies, id) } } diff --git a/internal/git/save_apply_test.go b/internal/git/save_apply_test.go new file mode 100644 index 00000000..4521cf32 --- /dev/null +++ b/internal/git/save_apply_test.go @@ -0,0 +1,48 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" +) + +// ageSaveApply moves a save's apply record back by by, as if it was asked and last recorded then. +func ageSaveApply(w *BranchWorker, id commitRequestID, by time.Duration) { + w.crOutcomesMu.Lock() + defer w.crOutcomesMu.Unlock() + entry := w.crApplies[id] + entry.askedAt = entry.askedAt.Add(-by) + entry.updatedAt = entry.updatedAt.Add(-by) + w.crApplies[id] = entry +} + +// An apply that runs longer than the outcome TTL keeps its record while the controller still +// follows it, so another save resolving never turns it into a lost one; it is collected only past +// SaveApplyTimeout. A settled record is collected after the outcome TTL. +func TestSaveApply_AnUnsettledRecordOutlivesTheFollowDeadline(t *testing.T) { + w := &BranchWorker{} + running := commitRequestID{Namespace: "default", Name: "running", UID: "u1"} + settled := commitRequestID{Namespace: "default", Name: "settled", UID: "u2"} + w.recordSaveApply(running, SaveApply{Reason: SaveApplyApplying}) + w.recordSaveApply(settled, SaveApply{Done: true, Applied: true, Reason: SaveApplyApplied}) + collect := func() { + w.recordCommitRequestOutcome(commitRequestID{Namespace: "default", Name: "other"}, FinalizeResult{}) + } + + ageSaveApply(w, running, commitRequestOutcomeTTL+time.Minute) + ageSaveApply(w, settled, 2*commitRequestOutcomeTTL) + collect() + _, kept := w.LookupSaveApply(running.Namespace, running.Name, running.UID) + assert.True(t, kept, "an unsettled apply is kept past the outcome TTL") + _, kept = w.LookupSaveApply(settled.Namespace, settled.Name, settled.UID) + assert.False(t, kept, "a settled apply is collected after the outcome TTL") + + w.recordSaveApply(running, SaveApply{Reason: SaveApplyRetrying}) // a retry keeps when it was asked + ageSaveApply(w, running, SaveApplyTimeout) + collect() + _, kept = w.LookupSaveApply(running.Namespace, running.Name, running.UID) + assert.False(t, kept, "past the follow deadline nothing reads it any more") +} diff --git a/internal/mutationlab/normalize/normalize.go b/internal/mutationlab/normalize/normalize.go index f44e7adc..ea1cd4d8 100644 --- a/internal/mutationlab/normalize/normalize.go +++ b/internal/mutationlab/normalize/normalize.go @@ -56,6 +56,14 @@ func expiredMessageRVs(message string) []string { // stable without losing the ordering that matters. const tsPlaceholder = "" +// listRVPlaceholder replaces a list's own metadata.resourceVersion, which stays out of the +// relational space. It is the revision the API server read the list at, not one any object +// carries: on k3s v1.37.1 a deletecollection's list returned the last deleted object's +// resourceVersion in some runs and a newer one in others, as an unrelated write landed in between. +// Numbered with the rest, it shifted every later placeholder by one, so the corpus alternated +// between two shapes on one cluster. +const listRVPlaceholder = "" + // isTimestampKey reports whether a JSON key's string value is an RFC3339 // timestamp the normalizer collapses to tsPlaceholder. "time" covers // managedFields[].time; lastTransitionTime/lastUpdateTime cover status.conditions[] @@ -188,9 +196,14 @@ func (c *collector) walk(v any) { c.collectGenerateNameSuffix(t) // Walk keys in sorted order so first-appearance indexing is deterministic // regardless of Go's randomized map iteration. + list := isList(t) for _, k := range sortedKeys(t) { - c.collectScalar(k, t[k]) - c.walk(t[k]) + v := t[k] + if list && k == "metadata" { + v = withoutKey(v, "resourceVersion") + } + c.collectScalar(k, v) + c.walk(v) } case []any: for _, e := range t { @@ -398,6 +411,11 @@ func (idx *indices) transform(v any) any { // association key (k:{"ip":"10.42.3.14"}) does not churn the corpus. out[idx.rewriteKey(k)] = idx.transformScalar(k, val, t) } + if meta, ok := out["metadata"].(map[string]any); ok && isList(t) { + if _, has := meta["resourceVersion"]; has { + meta["resourceVersion"] = listRVPlaceholder + } + } return out case []any: out := make([]any, len(t)) @@ -586,6 +604,28 @@ func (idx *indices) rewriteGeneratedName(v any, parent map[string]any) (string, return "", false } +// isList reports whether m is a list object: items beside its own metadata. +func isList(m map[string]any) bool { + _, items := m["items"].([]any) + _, meta := m["metadata"].(map[string]any) + return items && meta +} + +// withoutKey returns v with key left out when v is a map, and v unchanged otherwise. +func withoutKey(v any, key string) any { + m, ok := v.(map[string]any) + if !ok { + return v + } + out := make(map[string]any, len(m)) + for k, val := range m { + if k != key { + out[k] = val + } + } + return out +} + func placeholder(prefix string, n int) string { return fmt.Sprintf("<%s-%d>", prefix, n) } func lookup(m map[string]string, s string) any { diff --git a/internal/mutationlab/normalize/normalize_test.go b/internal/mutationlab/normalize/normalize_test.go index 797f3aab..366e75fc 100644 --- a/internal/mutationlab/normalize/normalize_test.go +++ b/internal/mutationlab/normalize/normalize_test.go @@ -309,6 +309,29 @@ func TestNormalize_DeletecollectionFanoutKeepsDistinctIdentities(t *testing.T) { } } +func TestNormalize_ListResourceVersionStaysOutOfTheRelationalSpace(t *testing.T) { + // Row 9 on k3s v1.37.1: the deletecollection list's resourceVersion is the last item's in some + // runs and a newer one in others. Both must normalize alike, and the watch DELETED placeholders + // after it must not shift. + audit := func(listRV string) string { + return `{"responseObject":{"kind":"ConfigMapList","items":[` + + `{"metadata":{"uid":"ua","resourceVersion":"10"}},{"metadata":{"uid":"ub","resourceVersion":"11"}}],` + + `"metadata":{"resourceVersion":"` + listRV + `"}}}` + } + deleted := `{"type":"DELETED","object":{"metadata":{"uid":"ua","resourceVersion":"13"}}}` + want := []string{ + `{"responseObject":{"items":[{"metadata":{"resourceVersion":"","uid":""}},` + + `{"metadata":{"resourceVersion":"","uid":""}}],"kind":"ConfigMapList",` + + `"metadata":{"resourceVersion":""}}}`, + `{"object":{"metadata":{"resourceVersion":"","uid":""}},"type":"DELETED"}`, + } + for _, listRV := range []string{"11", "12"} { + if got := normJSON(t, audit(listRV), deleted); !reflect.DeepEqual(got, want) { + t.Errorf("list rv %s:\n got %v\nwant %v", listRV, got, want) + } + } +} + func TestNormalize_FinalizerTerminalDeleteSameUIDHigherRV(t *testing.T) { // Row 8: the terminal DELETED keeps the same but a higher , proving // it is the same object at a later resourceVersion. diff --git a/internal/queue/fact_index.go b/internal/queue/fact_index.go index 5a86fbd3..4920e7ef 100644 --- a/internal/queue/fact_index.go +++ b/internal/queue/fact_index.go @@ -536,6 +536,12 @@ func (i *FactIndex) file(facts *scopeFacts, scope factScope, fact AuthorFact, no // tier too, and a removal query already waits on that key. facts.putRemoval(fact.UID, &indexedFact{fact: fact, at: now, seq: i.nextSeq()}) } + if fact.Created { + // The creating fact also takes its own slot, which a later write may not overwrite: a + // no-op patch shares the creation's (uid, rv) and would replace it in the exact tier. + // Like the removal slot it needs no waiter key, since the latest tier's key covers it. + facts.putCreation(fact.UID, &indexedFact{fact: fact, at: now, seq: i.nextSeq()}) + } return append(keys, factWaiterKey{scope: scope, kind: factKindLatest, value: fact.UID}) case fact.ResourceVersion != "": facts.putRV(fact.ResourceVersion, &indexedFact{fact: fact, at: now, seq: i.nextSeq()}) diff --git a/internal/queue/fact_index_store.go b/internal/queue/fact_index_store.go index bd83e0d3..649cd929 100644 --- a/internal/queue/fact_index_store.go +++ b/internal/queue/fact_index_store.go @@ -8,7 +8,7 @@ import ( "k8s.io/apimachinery/pkg/labels" ) -// factKind names one of the six match structures a fact can land in. The set is closed: a fact +// factKind names one of the seven match structures a fact can land in. The set is closed: a fact // that fits none of them is not stored, because a fact nothing can ever join is only memory. type factKind uint8 @@ -20,6 +20,9 @@ const ( // factKindRemoval is (uid), the sticky removal pointer: the fact that this object was DELETED and // by whom. It is the one structure a later fact may not overwrite — see putRemoval. factKindRemoval + // factKindCreation is (uid), the fact that CREATED this object. Only a creating fact is filed + // there, so a later write cannot overwrite it — see putCreation. + factKindCreation // factKindRV is (rv), the escape hatch for a fact with an rv but no uid. factKindRV // factKindName is (namespace, name), the floor for a fact with neither a uid nor an rv. Only an @@ -148,7 +151,10 @@ type scopeFacts struct { latest map[string]*indexedFact // removals is the sticky removal pointer, keyed by uid: "this object was deleted, and this actor // asked for it". It is deliberately NOT swept by the TTL — see putRemoval and sweep. - removals map[string]*indexedFact + removals map[string]*indexedFact + // creations is the creating fact, keyed by uid. Unlike removals it is swept by the TTL like the + // ordinary tiers: the one lookup that reads it waits only a grace period for its fact. + creations map[string]*indexedFact rvOnly map[string]*indexedFact byName map[nameFactKey]*indexedFact collections []*indexedCollection @@ -160,11 +166,12 @@ type scopeFacts struct { func newScopeFacts() *scopeFacts { return &scopeFacts{ - exact: map[exactFactKey]*indexedFact{}, - latest: map[string]*indexedFact{}, - removals: map[string]*indexedFact{}, - rvOnly: map[string]*indexedFact{}, - byName: map[nameFactKey]*indexedFact{}, + exact: map[exactFactKey]*indexedFact{}, + latest: map[string]*indexedFact{}, + removals: map[string]*indexedFact{}, + creations: map[string]*indexedFact{}, + rvOnly: map[string]*indexedFact{}, + byName: map[nameFactKey]*indexedFact{}, } } @@ -197,6 +204,17 @@ func (s *scopeFacts) putRemoval(uid string, entry *indexedFact) { putKeyed(s, s.removals, uid, entry, factRef{kind: factKindRemoval, uid: uid}) } +// putCreation stores the fact that created an object. Only a fact marked Created is filed here, so +// a later write cannot reach the slot. +// +// The exact tier cannot serve this, and that was measured: a no-op patch (a reapply that changes +// nothing) returns the object at the resourceVersion it already had, so its fact is filed under the +// creating fact's own (uid, rv) and replaces it. A CommitRequest re-applied before its author was +// resolved then lost its creator. +func (s *scopeFacts) putCreation(uid string, entry *indexedFact) { + putKeyed(s, s.creations, uid, entry, factRef{kind: factKindCreation, uid: uid}) +} + // putRV stores the rv-only escape hatch. func (s *scopeFacts) putRV(rv string, entry *indexedFact) { putKeyed(s, s.rvOnly, rv, entry, factRef{kind: factKindRV, rv: rv}) @@ -241,20 +259,9 @@ func (s *scopeFacts) lookupLatest(uid string, cutoff time.Time) (AuthorFact, boo return liveFact(s.latest[uid], cutoff) } -// lookupCreation reads the fact that created the object with uid, from the exact tier, where every -// fact carrying a uid and a resourceVersion is filed under its own version, so no later write -// replaces it. It walks the tier, which costs one pass over a type's facts: it serves only the -// CommitRequest author lookup, made once per request. +// lookupCreation reads the fact that created the object with uid. func (s *scopeFacts) lookupCreation(uid string, cutoff time.Time) (AuthorFact, bool) { - if uid == "" { - return AuthorFact{}, false - } - for key, entry := range s.exact { - if key.uid == uid && entry.fact.Created { - return liveFact(entry, cutoff) - } - } - return AuthorFact{}, false + return liveFact(s.creations[uid], cutoff) } // lookupRemovalPointer reads the sticky removal pointer. It takes NO cutoff, and it is the only lookup here @@ -341,6 +348,12 @@ func (s *scopeFacts) sweep(cutoff time.Time) int { removed++ } } + for key, entry := range s.creations { + if entry.at.Before(cutoff) { + delete(s.creations, key) + removed++ + } + } for key, entry := range s.rvOnly { if entry.at.Before(cutoff) { delete(s.rvOnly, key) @@ -404,6 +417,8 @@ func (s *scopeFacts) remove(ref factRef) bool { return removeKeyed(s.latest, ref.uid, ref.seq) case factKindRemoval: return removeKeyed(s.removals, ref.uid, ref.seq) + case factKindCreation: + return removeKeyed(s.creations, ref.uid, ref.seq) case factKindRV: return removeKeyed(s.rvOnly, ref.rv, ref.seq) case factKindName: @@ -480,6 +495,8 @@ func (s *scopeFacts) live(ref factRef) bool { return liveKeyed(s.latest, ref.uid, ref.seq) case factKindRemoval: return liveKeyed(s.removals, ref.uid, ref.seq) + case factKindCreation: + return liveKeyed(s.creations, ref.uid, ref.seq) case factKindRV: return liveKeyed(s.rvOnly, ref.rv, ref.seq) case factKindName: diff --git a/internal/queue/fact_index_test.go b/internal/queue/fact_index_test.go index 754b0f8b..096ed823 100644 --- a/internal/queue/fact_index_test.go +++ b/internal/queue/fact_index_test.go @@ -165,7 +165,7 @@ func TestFactIndex_CreationQueryReadsOnlyTheCreatingFact(t *testing.T) { created := objectFact("alice", "101") created.Verb, created.Created = "create", true harness.publish(factIndexTestStream("prod-eu-1"), created, objectFact("label-controller", "102")) - harness.waitForFacts(3) + harness.waitForFacts(4) creation := FactQuery{AuditRoute: "prod-eu-1", GroupResource: deploymentsGroupResource(), UID: factIndexTestUID, Namespace: "team-a", ExactCapable: true, Creation: true} @@ -183,6 +183,25 @@ func TestFactIndex_CreationQueryReadsOnlyTheCreatingFact(t *testing.T) { harness.absent(creation) } +// TestFactIndex_CreationSurvivesANoOpWriteAtItsVersion: a no-op patch returns the object at the +// resourceVersion the create stamped, so its fact shares the creating fact's (uid, rv). It must not +// replace the creator: a CommitRequest re-applied before its author was resolved lost its creator +// that way and settled AuthorUnresolved. +func TestFactIndex_CreationSurvivesANoOpWriteAtItsVersion(t *testing.T) { + harness := newFactIndexHarness(t, FactIndexConfig{}) + created := objectFact("alice", "101") + created.Verb, created.Created = "create", true + noOp := objectFact("ci-bot", "101") + noOp.Verb = "patch" + harness.publish(factIndexTestStream("prod-eu-1"), created, noOp) + harness.waitForFacts(3) + + resolution := harness.resolve(FactQuery{AuditRoute: "prod-eu-1", GroupResource: deploymentsGroupResource(), + UID: factIndexTestUID, Namespace: "team-a", ExactCapable: true, Creation: true}) + require.Equal(t, AttributionExact, resolution.Result) + require.Equal(t, "alice", resolution.Fact.Author, "the creator, not the no-op writer") +} + // TestFactIndex_CreationQueryWaitsForTheCreatingFact: a creation query parked before the fact arrives // is woken by it. func TestFactIndex_CreationQueryWaitsForTheCreatingFact(t *testing.T) { diff --git a/internal/queue/key_prefix.go b/internal/queue/key_prefix.go index 4be44954..26d8ed7d 100644 --- a/internal/queue/key_prefix.go +++ b/internal/queue/key_prefix.go @@ -12,8 +12,8 @@ import ( // dwarfs the key it namespaces. const maxKeyPrefixLength = 128 -// DefaultKeyPrefix is the root namespace every Redis key (cursors, fact streams, and command -// author records alike) carries when --redis-key-prefix is not set. It is also the value every +// DefaultKeyPrefix is the root namespace every Redis key (cursors and fact streams alike) carries +// when --redis-key-prefix is not set. It is also the value every // release before the flag existed used, so the default is a no-op upgrade. const DefaultKeyPrefix = "gitops-reverser" diff --git a/test/e2e/cluster/audit/policy.yaml b/test/e2e/cluster/audit/policy.yaml index 2aa32aca..c538b1eb 100644 --- a/test/e2e/cluster/audit/policy.yaml +++ b/test/e2e/cluster/audit/policy.yaml @@ -52,8 +52,8 @@ rules: - delete - deletecollection # No resources listed = Matches everything else (including CRDs). - # This also captures `commitrequests.configbutler.ai` create events, which - # name a CommitRequest's author, as a write's event names its author. If this - # catch-all is ever narrowed, keep at least Metadata-level capture for - # `commitrequests` create events — the author is joined by uid and - # resourceVersion, which Metadata carries. + # This also captures the `commitrequests.configbutler.ai` event that created + # a CommitRequest, which names its author. If this catch-all is ever narrowed, + # keep RequestResponse capture of `commitrequests` create AND patch (server-side + # apply creates with a patch): the author is joined by the uid and + # resourceVersion in responseObject, which Metadata level leaves out. diff --git a/test/mutationlab/corpus/CLUSTER.md b/test/mutationlab/corpus/CLUSTER.md index 66a26d27..19453d39 100644 --- a/test/mutationlab/corpus/CLUSTER.md +++ b/test/mutationlab/corpus/CLUSTER.md @@ -32,9 +32,13 @@ records it for the same request. Every other scenario reproduced unchanged. Moving from `v1.36.4+k3s1` to `v1.37.1+k3s1` (2026-10-27, the e2e cluster's version) changed three shapes, each reproduced by a second capture: -- `configmap/deletecollection/`: the `deletecollection` response's list - resourceVersion is now the last deleted object's own resourceVersion, where - 1.36 returned a newer one, so the watch DELETED placeholders shift by one; +- `configmap/deletecollection/`: the watch DELETED placeholders shifted by + one. The `deletecollection` response's list resourceVersion is the revision + the API server read the list at, which on 1.37.1 is the last deleted object's + resourceVersion in some runs and a newer one in others (two of four captures + each), where 1.36 returned a newer one in every run. The normalizer now writes + a list's own resourceVersion as ``, outside the numbered `` + placeholders, so both outcomes give the same corpus; - `pod/graceful-delete/`: the deleted pod's `PodReadyToStartContainers` condition now carries `reason: PodSandboxNotReady` and a message; - `configmap/owner-ref-cascade/`: the garbage collector's `userAgent` names the diff --git a/test/mutationlab/corpus/configmap/deletecollection/audit.deletecollection.yaml b/test/mutationlab/corpus/configmap/deletecollection/audit.deletecollection.yaml index 64c07d38..ba700a41 100644 --- a/test/mutationlab/corpus/configmap/deletecollection/audit.deletecollection.yaml +++ b/test/mutationlab/corpus/configmap/deletecollection/audit.deletecollection.yaml @@ -48,7 +48,7 @@ responseObject: uid: kind: ConfigMapList metadata: - resourceVersion: + resourceVersion: responseStatus: code: 200 metadata: {}