diff --git a/docs/INDEX.md b/docs/INDEX.md index c93225f2..f44afc54 100644 --- a/docs/INDEX.md +++ b/docs/INDEX.md @@ -144,7 +144,7 @@ so you never have to read a proposal to find out it already shipped. | [`gittarget-api-wave.md`](design/gittarget-api-wave.md) | **partly built**, filed as [#294](https://github.com/ConfigButler/gitops-reverser/issues/294). One breaking wave on `GitTarget`, of which steps 2 to 7 have **shipped**: `spec.suspend`, `status.placement` and the reconcile-request annotation in [#326](https://github.com/ConfigButler/gitops-reverser/pull/326), and B4's `commitWindow`/`commit.message` move plus the source-scope deletion in [#330](https://github.com/ConfigButler/gitops-reverser/pull/330). Organizing principle: **the folder is described on the GitTarget, the connection describes only the connection** — and this is where that became a struct boundary rather than a sentence, since grouping a field is free only in a release that is already breaking. `spec.mode` and `GitTarget.spec.interval` are both **dropped**, with re-open triggers, and `status.lastHandledReconcileAt` was dropped in the build for a stated reason. Step 8, the riders, is closed: the `CommitRequest` lifecycle hole shipped in [#400](https://github.com/ConfigButler/gitops-reverser/pull/400), the missing source-cluster provider message in [#258](https://github.com/ConfigButler/gitops-reverser/pull/258), and an asserted `CommitRequest.spec.author` is **declined** because authorship is established, never asserted ([#339](https://github.com/ConfigButler/gitops-reverser/issues/339)); #294 is closed. The reference collapse and the `scope` removal shipped in [#333](https://github.com/ConfigButler/gitops-reverser/pull/333); the `TooManyStreams` cap and the Event on a changed resolution were dropped. Also records that staying `v1alpha3` on loud rejections is a **one-consumer countdown** | | [`target-watch-plan.md`](design/target-watch-plan.md) | **partly built.** The diff is built and applied; removal semantics are not. The companion to [`watch-manager-ownership.md`](design/watch-manager-ownership.md): the ownership page says WHO applies a plan, this one says WHAT a plan is and what changing it may touch. A resource collection — group, resource, namespace, deliberately no served version — is the one identity the watch stream, the render-fidelity scope and the mark-and-sweep boundary all agree on, because a key that does not round-trip to the scope it sweeps under is the class of error that deletes user data. The plan is diffed into `keep`/`start`/`restart`/`stop` and applied per collection, so adding one WatchRule stops re-initializing every unrelated collection into a queue shared with other tenants; a `restart` is a served-version change, which is why the version is spec DATA rather than identity. Readiness and the fidelity revision are per scope, so a KEPT collection holds the result its own initialization produced rather than being asked to prove itself again over an unrelated edit. `stop` never touches files — removal is a Git-side sweep under the target's existing `spec.prune.mode`, not a watch-layer delete. "Cut at the producer" is the accepted consequence: nothing fences the queue, so a deselected collection may leave a short tail of writes, bounded by the queue and converged afterwards. Still open: the `stop` classification wants a settled `TypeRemoved` from `typeset` (see TODO), and removal on INTENT is undecided. | | [`collection-terminology-rename.md`](design/collection-terminology-rename.md) | **step 1, shipped.** Replaced cells with resource collections and removed per-rule `operations` from both rule kinds. Rules observe every object event; GitTarget prune mode controls removal. Records the rename census (760 Go identifiers, 265 documentation occurrences), the operation-filter inventory, API/Helm migration, compatibility boundaries, and test cases. | -| [`watches-labels-simplification.md`](design/watches-labels-simplification.md) | **step 2, implemented.** After the collection rename and operation-filter removal, native per-rule `objectSelector` (#146). Kubernetes is authoritative: `Always` compares selected identities with all managed Git identities in the type/namespace scope, without comparing Git labels. Full support includes supported Kustomize layouts from the first release, plus filtered-removal attribution and ordered producer replacement. | +| [`watches-labels-simplification.md`](design/watches-labels-simplification.md) | **step 2, implemented.** After the collection rename and operation-filter removal, native per-rule `objectSelector` (#146). Kubernetes is authoritative: `Always` compares selected identities with all managed Git identities in the type/namespace scope, without comparing Git labels. Full support includes supported Kustomize layouts from the first release, plus label-exit attribution (now selector-agnostic, see the attribution spec) and ordered producer replacement. | | [`label-selection-follow-ups.md`](design/label-selection-follow-ups.md) | **design, open.** What the label-selection review rounds left open, with the evidence for each: real deletions in a selected collection wait the full 3s attribution grace (the watch cannot tell them from a label exit, measured), the resync fence that stops overlapping collections from coalescing (kept on purpose), same-second overlap ties, and one store rebuild per object coming back. Also lists what was fixed on the branch | | [`attribution-removal-wait-options.md`](design/attribution-removal-wait-options.md) | a removal now waits for evidence about the DELETION rather than accepting the object's last write, which stopped it naming whoever last edited the object as the author of a deletion they did not perform. Enumerates the eight situations a resolution can be in and shows the cost is concentrated in exactly one: a removal for which no delete fact will ever arrive (a graceful pod delete, a status-only removal, a type the audit policy skips) spends the whole grace to return the answer it had at t=0, measured at ~3.1s against ~70ms when evidence is present. Prices five options against that, and recommends a per-route watermark — stop waiting once the fact stream has demonstrably moved past this event — over a second timeout flag whose right value lives in the API server's config rather than ours. Open: the decision, and how common the case is outside the e2e suite | | [`commitrequest-save-wait-options.md`](design/commitrequest-save-wait-options.md) | **design, decided** — a five-phase plan, reported from a live cluster: a `CommitRequest` can expire before the write it exists to publish arrives, because the close delay defaulted to `0` while the write is still held head-of-line waiting for an audit fact, so the save resolves `Ready=True`/`NoWindowInGrace` and the edit commits seconds later under the target's `liveTemplate`. The decision is to repair the default (`spec.closeDelay`, now a duration string defaulting to `"2s"`, **shipped as phase 0**) and then let a request **name the write it is waiting for**, reusing the `(uid, resourceVersion)` identity the attribution join already keys on, including its settled `ExactCapable` rule. Records why the fact index cannot BE the wait (it sits upstream of the watch event, so a fact-keyed wait fires before the window opens) and what to take from it anyway — the register-then-check waiter discipline, and a best-effort retrospective oracle bounded by a TUNABLE TTL. Twelve situations, and the page is deliberate about the two it cannot fix cheaply: row 11 (a saturated queue is rejected in the WATCH path and never reaches the worker, so no worker-side wait can diagnose it) is an accepted, named gap rather than a promise, and row 10 is fixed only by a hook in the fidelity-drop branch that marks a wait terminally UNSATISFIABLE — satisfying before that gate would arm a finalize for a write that never commits. Carries a **preconditions chapter** for saves on today's build (audit policy first, then delay sizing, fidelity warm-up, save-button debounce, and assert on the commit rather than on `Ready`). `CommitterActivity` is deferred, not recommended: by its own arrival-not-existence rule it cannot satisfy the already-arrived case. Open: situation 12 (one actor, two saves, one window — blocks phase 2), whether the fact-index lookup earns its coupling, and how an unsatisfied wait is graded | diff --git a/docs/architecture.md b/docs/architecture.md index 3334878f..5a98a531 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -1018,25 +1018,25 @@ same `user`/`serviceaccount`/`none` vocabulary `commits_total{author_kind}` uses | `deletecollection_body_uid` | uid in a collection fact's set | the API server said this request deleted this object | | `latest` | uid (the object's own delete fact) | who removed it | | `name` | namespace + name | the same, for a fact with no uid (an aggregated API's usual shape) | -| `latest` | uid (a write fact) | who last *wrote* it; a fallback for a removal | +| `latest` | uid (a delete fact) | the object's own delete, when the sticky slot was evicted | | `deletecollection_scope` | namespace + selector + window | a collection request covering it was made | | `resource_version` | rv (the escape hatch for a fact with no uid) | a fact for this exact version, unidentified | | `absent` | none | nothing usable arrived in time | Three rules carry most of the behavior. **A fact about a deletion may not be replaced by a fact about a write:** a removal fact takes a sticky, uid-keyed slot that the ordinary last-writer-wins -structures cannot reach, and a removal asks that slot first. That slot is the one structure the fact -TTL does not bound: a uid is unique across space and time, so the statement cannot be superseded, -and its horizon is the index's caps instead. It is still in-memory: a restart re-warms the index -from one TTL of stream retention like everything else. Without the slot, a finalizer patch's fact -overwrites the deleter's, because both carry the resourceVersion the *deletion* stamped -([spec/attribution.md](spec/attribution.md)). **A removal never returns on a write fact -without looking further:** the per-object tiers are last-writer-wins, -so for a removal they hold whoever last *edited* the object, which is not who deleted it; such a -match is held as a fallback while the wait continues for evidence about the deletion itself. And -**an exact-capable event may not fall through to the removal tiers:** a create or update presents -the resourceVersion its own write produced, so if the exact tier misses, the uid pointer may name an -older, different author. +structures cannot reach, and a removal of a terminating object asks that slot first. That slot is +the one structure the fact TTL does not bound: a uid is unique across space and time, so the +statement cannot be superseded, and its horizon is the index's caps instead. It is still in-memory: +a restart re-warms the index from one TTL of stream retention like everything else. Without the +slot, a finalizer patch's fact overwrites the deleter's, because both carry the resourceVersion the +*deletion* stamped ([spec/attribution.md](spec/attribution.md)). **A removal is credited only to +the request that caused it:** a write at its own resourceVersion (an object relabeled out of a +rule's `objectSelector`) or a fact about the deletion. The object's last *edit* says nothing about +who removed it, so it never answers, and a removal without such a fact is unresolved. None of this +reads the watch's label selector. And **an exact-capable event may not fall through to the removal +tiers:** a create or update presents the resourceVersion its own write produced, so if the exact +tier misses, the uid pointer may name an older, different author. No branch on this path depends on the type. Every decision is made on the verb, on whether the event is a removal, and on which fields the event happens to carry. That is why an aggregated API needed diff --git a/docs/configuration.md b/docs/configuration.md index c27da7cb..ce115020 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -2535,6 +2535,12 @@ the reference configuration of `1s`). The default of `"2s"` clears that; a loade may want `"4s"` to `"5s"`, and a save created before its writes needs the time to its first write on top. +The floor assumes the stream is not busy. A watch stream attributes its events in order, so a save's +edit can also queue behind an earlier event of the same type that is still waiting for its fact, for +up to `--author-attribution-grace` each. That is rare when the audit policy covers the type, and the +[suggested alert](interpreting-metrics.md#suggested-alerts) on `tier="absent"` says when it is not. +Widening `attachTimeout` hides that delay rather than removing it. + `maxDuration`, and `idleTimeout` when set, cover the span of the save's own writes after the attach. The worst case from registration to the finalize is `attachTimeout` plus `maxDuration`; the push follows, with its cooldown and any retries. @@ -2761,18 +2767,19 @@ When attribution is enabled, these flags tune the join: below it will lose actors systematically. The wait ends the moment the fact arrives, so a generous grace costs latency only when a fact never comes. - **A removal is the exception, and it is worth budgeting for.** It waits for evidence about the - DELETION rather than settling for the object's last write, so an object edited by one person and - deleted by another is credited to the deleter rather than to the editor. Sometimes no delete fact - ever arrives, most often because the cluster's **audit policy excludes the type** (which is true of - every type in the recommended policy's runtime-noise list). In that case the - removal spends the whole grace before naming the last writer, which is the same answer it would - have given immediately. Measured across one e2e run, where the grace is 10s, the cost lands on - exactly that case and nowhere else: a removal that finds its delete evidence resolves in about - 70ms, while one that never does averages about 3.1s before falling back. Creates and updates do - not consult the deletion tiers at all. The watch shard is single-threaded, so the wait also delays - the events queued behind it on the same `(GitTarget, type, scope)`. Lowering this flag bounds both - directly. + **A removal is the exception, and it is worth budgeting for.** It is credited only to the request + that caused it: its delete, or, when the object left a rule's `objectSelector`, the write that + relabeled it. The object's last write never answers it, so an object edited by one person and + deleted by another is credited to the deleter, never to the editor. Sometimes no such fact ever + arrives, most often because the cluster's **audit policy excludes the type** (which is true of + every type in the recommended policy's runtime-noise list). In that case the removal spends the + whole grace and then ships unresolved. Measured across one e2e run, where the grace is 10s, the + cost lands on exactly that case and nowhere else: a removal that finds its evidence resolves in + about 70ms, while one that never does waits the grace. Creates and updates do not consult the + deletion tiers at all. The watch shard is single-threaded, so the wait also delays the events + queued behind it on the same `(GitTarget, type, scope)`. Lowering this flag bounds both directly. + [What attribution gives up, on purpose](spec/attribution.md#what-attribution-gives-up-on-purpose) + lists every trade of this kind, including the one wrong author it accepts. **Watching a type your audit policy excludes costs more than attribution.** Every removal on such a type spends the full grace before shipping unresolved, because the fact it is waiting for was diff --git a/docs/design/attribution-removal-wait-options.md b/docs/design/attribution-removal-wait-options.md index 87b063ea..855d4e1b 100644 --- a/docs/design/attribution-removal-wait-options.md +++ b/docs/design/attribution-removal-wait-options.md @@ -8,6 +8,12 @@ > object's last write, which fixed a real mis-attribution. The cost is concentrated in one case — > a removal for which no delete fact will ever arrive — and there it spends the whole grace window > to return the answer it would have returned immediately. +> +> **Update 2026-10-22.** A removal no longer falls back to the object's last write at all: with no +> evidence about what caused it, it is unresolved. Situations 4 and 5 below therefore end in the +> deleter or in `absent`, never in the last writer. The cost this page is about is unchanged: +> situation 5 still spends the whole grace. See +> [what attribution gives up, on purpose](../spec/attribution.md#what-attribution-gives-up-on-purpose). ## The question diff --git a/docs/design/label-selection-follow-ups.md b/docs/design/label-selection-follow-ups.md index 4f9f573b..f32e0c98 100644 --- a/docs/design/label-selection-follow-ups.md +++ b/docs/design/label-selection-follow-ups.md @@ -1,6 +1,6 @@ # Label selection: follow-ups -> **design**: open. Nothing here blocks label selection from shipping. +> **design**: open. Nothing here blocks label selection from shipping. Item 1 is resolved. > Index: [`../INDEX.md`](../INDEX.md). Parent: [`watches-labels-simplification.md`](watches-labels-simplification.md). > Checked: 2026-09-29, against `feat/watch-object-selector` (PR #399). @@ -29,37 +29,21 @@ reaches it today. ### 1. Real deletions in a selected collection wait the full attribution grace -**Today.** A `DELETED` frame from a collection with a selector is a *filtered removal*. It -resolves only on the fact at the object's exact `(uid, resourceVersion)`. See -[attribution.md, "A removal from a label-selected collection"](../spec/attribution.md). A label -exit always produces that fact. A real deletion never does, because a delete response carries no -resourceVersion that matches the frame's. So every real deletion from a selected collection waits -out `DefaultAttributionGraceWindow` (3s) and then commits unresolved. - -**Cost.** A stream is single-threaded, and attribution runs inline on it. Deleting a namespace -with N selected objects therefore stalls that stream for about N × 3s. With 200 objects that is -about 10 minutes behind, which risks a 410 and a relist. Collections without a selector are not -affected. `gitopsreverser_watch_event_handling_seconds` shows the stall. - -**Why it was not fixed.** The obvious fix does not work. It would tell a deletion from a label -exit by checking whether the frame's labels still match the selector. They do in both cases: the -mutationlab `selector-membership` scenario measured that a label exit's `DELETED` carries the -object as it was *before* the relabeling write. Accepting a uid-only deletion fact is also wrong, -for the reason attribution.md gives: a relabel-out followed by someone else's delete produces -exactly that fact. - -**Options.** - -- End the wait early once a uid-only deletion fact has arrived *and* the exact slot is still - empty after a short sub-grace. This is still a guess about ordering, and - `TestFactIndex_FilteredRemovalWaitsForADelayedExitFactPastAUIDOnlyDeletion` pins the case it - would get wrong. -- Take attribution off the stream goroutine. Buffer each event with its pending lookup, and - release events in order as their lookups settle. The per-event latency stays the same, but a - burst costs one grace window instead of N. This is the real fix, and it changes the ordering - spec in [`watch-event-ordering-and-attribution-grace.md`](../spec/watch-event-ordering-and-attribution-grace.md). -- Accept the limitation and document it in the configuration reference, next to - `objectSelector`. +**Resolved 2026-10-22.** A `DELETED` frame from a selected collection used to resolve only on the +fact at the object's exact `(uid, resourceVersion)`, which a real deletion never fills. Every such +deletion waited out the grace and committed unresolved, and a prune of N objects stalled its stream +for N x grace (it made the Flux E12 save spec fail `NoWindow`). + +Attribution no longer reads the selector. A removal of an object that was not terminating asks for +a write at its exact version first (the exit), then for deletion evidence, so a real deletion +resolves on its delete fact in about one audit batch. The one case this names wrongly, a +relabel-out followed by someone else's delete whose fact is in the index first, is accepted and +listed in +[what attribution gives up, on purpose](../spec/attribution.md#what-attribution-gives-up-on-purpose). + +Head-of-line blocking itself is still open: an event that legitimately waits out the grace still +delays every later event on its stream. That is ranked in +[`open-asks-priority.md`](open-asks-priority.md). ### 2. The resync fence stops overlapping collections from coalescing diff --git a/docs/design/watches-labels-simplification.md b/docs/design/watches-labels-simplification.md index 52f080e9..0d313263 100644 --- a/docs/design/watches-labels-simplification.md +++ b/docs/design/watches-labels-simplification.md @@ -223,8 +223,15 @@ There is also an avoidable delay. When an exact PATCH/UPDATE wins lookup, `await still treats it as a write fallback and waits the remaining grace for delete evidence (default `3s`). This is not true of every exit: eligible delete evidence can finish the wait earlier. -> **As built:** exactly this policy, and the mutationlab `selector-membership` capture explains its -> cost: a deletion never carries the `DELETED` frame's resourceVersion (an immediate delete answers +> **Superseded (2026-10-22).** The selector-aware policy below was replaced by one that never reads +> the selector: a removal of an object that was not terminating is credited first to a write at its +> exact UID/RV (the exit), then to deletion evidence, as any removal is. Real deletions on a selected +> stream now resolve on their delete fact instead of waiting out the grace and shipping unresolved, +> which stalled the stream for N x grace on a prune. The cost is one accepted wrong author, listed in +> [what attribution gives up, on purpose](../spec/attribution.md#what-attribution-gives-up-on-purpose). +> +> **As built until then:** exactly this policy, and the mutationlab `selector-membership` capture +> explains its cost: a deletion never carries the `DELETED` frame's resourceVersion (an immediate delete answers > with a `Status` holding only the uid; a finalizer-held one is stamped one step earlier), so a real > deletion from a selected collection is unresolved. A uid-keyed deletion fact is not accepted > instead: an object relabeled out and then deleted by someone else produces one that would name diff --git a/docs/interpreting-metrics.md b/docs/interpreting-metrics.md index d8ed5c5b..5513ec47 100644 --- a/docs/interpreting-metrics.md +++ b/docs/interpreting-metrics.md @@ -1035,10 +1035,10 @@ equally hold a write, and a value that could mean either must not claim a verb. | `tier` | Meaning | | --- | --- | -| `delete_sticky` | The sticky removal pointer: a fact whose own verb is a delete, filed by UID into a slot no later *write* fact may overwrite. Only a removal consults it, and it is asked before `exact`, because a removal's resourceVersion is the one the deletion stamped — the version a finalizer patch's own fact carries too. It is the only tier the fact TTL does not bound: a UID is unique across space and time, so the statement can never be superseded. | -| `exact` | Exact UID+resourceVersion match: this actor produced this exact version. | +| `delete_sticky` | The sticky removal pointer: a fact whose own verb is a delete, filed by UID into a slot no later *write* fact may overwrite. Only a removal consults it. For a terminating object it is asked before `exact`, because the removal's resourceVersion is the one the deletion stamped, which a finalizer patch's own fact carries too. It is the only tier the fact TTL does not bound: a UID is unique across space and time, so the statement can never be superseded. | +| `exact` | Exact UID+resourceVersion match: this actor produced this exact version. For a removal of an object that was not terminating, it is the write that took the object out of view: a label exit. | | `deletecollection_body_uid` | A removal whose UID was in the set the API server said a `deletecollection` deleted. No over-attribution risk: either the object was in that set or it was not. It outranks `latest`, because `latest` names whoever last *wrote* an object while a removal asks who *deleted* it. | -| `latest` | The UID-latest tier: the object's own last fact, keyed by UID alone. A removal consults it, and a match here describing a *write* is held as a fallback while the wait continues for evidence about the deletion. | +| `latest` | The UID-latest tier: the object's own last fact, keyed by UID alone. A removal accepts it only when that fact is a delete, which `delete_sticky` normally answers first, so it appears only when the sticky pointer was evicted under the index caps. A *write* here never answers a removal. | | `name` | A match on `(namespace, name)` for a fact carrying neither a UID nor a resourceVersion — the usual shape of an aggregated API's audit event, and of a delete the API server answered with a `Status`. | | `deletecollection_scope` | A removal matched to a `deletecollection` by scope alone — same type and namespace, the request's selector accepting the object's labels, within the collection window. The weakest evidence the join has, and the only one that can name the wrong actor, which is why it is reached only when every more specific tier missed. | | `resource_version` | The RV-only escape hatch: a fact that carried a resourceVersion and no UID, matched on that version alone. | @@ -1064,9 +1064,10 @@ sum by (tier) (rate(gitopsreverser_attribution_resolutions_total[5m])) ``` **Is the grace window paying for itself?** `event_kind` is `write` or `removal`, and the split is -the point: a removal holds a fallback and keeps waiting for evidence about the deletion, a write -does not. The removal wait is the number `--author-attribution-grace` is tuned from, and a p95 -approaching the configured grace means removals are sitting out the whole window: +the point: a removal waits for evidence about what caused it, which may never come (a type the +audit policy drops), while a write waits only for the fact at its own version. The removal wait is +the number `--author-attribution-grace` is tuned from, and a p95 approaching the configured grace +means removals are sitting out the whole window: ```promql histogram_quantile(0.95, @@ -1355,6 +1356,7 @@ sum by (outcome) (rate(gitopsreverser_secret_encryptions_total[5m])) | `gitopsreverser_resource_condition{type="Ready", status="False"} == 1`, `for: 15m` | A declared object has not been accepted. The `== 1` is not optional: each condition publishes one series per status, so the selector alone matches the two zeroes too. | | `gitopsreverser_resource_condition{type="Stalled", status="True"} == 1` | Permanently wedged: `Stalled` is the kstatus "nothing will retry" signal, so this needs a human and will not clear on its own. `reason` names the gate. | | any `gitopsreverser_*` series with `otel_metric_overflow="true"` | A metric family exceeded its per-instrument cardinality cap, so the labels identifying those series are gone and every query over that family is under-reporting. Raise the cap or cut a label. | +| `sum by (group, resource) (rate(gitopsreverser_attribution_resolutions_total{tier="absent"}[10m])) > 0`, `for: 15m` | Events of this type keep waiting out the whole `--author-attribution-grace` and shipping `unknown (attribution unresolved)`. Each one held its watch stream for the full grace, so every later event of the type, including a save's edit, reached its branch worker that much later. Check the audit policy for the type and the `KubeProvider`'s audit route before anything else. An occasional absence is ordinary; a sustained rate is a configuration problem or a fact that can never arrive. | | `sum by (gittarget_namespace, gittarget_name) (gitopsreverser_watch_types) == 0`, `for: 10m` | A GitTarget's rules resolve to no watchable type — a typo in `resources:`, or a type the default watch policy excludes. It mirrors nothing while looking healthy everywhere else. The 10 minutes let a freshly declared target settle. | --- diff --git a/docs/spec/attribution.md b/docs/spec/attribution.md index 08268c59..f3c910b8 100644 --- a/docs/spec/attribution.md +++ b/docs/spec/attribution.md @@ -14,9 +14,8 @@ > What is still **open** is not here. It is ranked in > [`../design/open-asks-priority.md`](../design/open-asks-priority.md). Removal-wait alternatives are in > [`../design/attribution-removal-wait-options.md`](../design/attribution-removal-wait-options.md). -> The [filtered-removal policy](../design/watches-labels-simplification.md#attribution-for-filtered-removals) -> for label-filtered `DELETED` events is built, and is the last of the -> [four rules that are easy to miss](#four-rules-that-are-easy-to-miss) below. +> What attribution deliberately gives up for speed, and the one wrong author it accepts, is listed in +> [what attribution gives up, on purpose](#what-attribution-gives-up-on-purpose). > > Related: [`watch-event-ordering-and-attribution-grace.md`](watch-event-ordering-and-attribution-grace.md), > [`commitrequest-admission-authorship.md`](commitrequest-admission-authorship.md), @@ -334,59 +333,63 @@ grace for one to arrive. flowchart TD A[Watch event] --> B[scope = audit route + group/resource] B --> C{scope known?} - C -->|no| Z[absent: committer-authored] - C -->|yes| AA{a removal, and a sticky
removal pointer for its uid?} - AA -->|yes| AB[delete_sticky] - AA -->|no| D{a fact under
this uid and rv?} + C -->|no| Z[absent: unresolved] + C -->|yes| F{is this a removal?} + F -->|no: a write| D{a fact under
this uid and rv?} D -->|match| E[exact] - D -->|no match| F{is this a removal?} - - F -->|yes| G{uid in a collection's
uid set?} - G -->|yes| H[deletecollection_body_uid] - G -->|no| I{latest uid
is a DELETE fact?} - I -->|yes| J[latest: the object's own delete] - I -->|no, it is a write| K[hold it as a fallback] - K --> L{name tier holds
a DELETE fact?} - L -->|yes| M[name] - L -->|no| N{fallback held?} - N -->|yes| O[latest: last writer] - N -->|no| P{collection covers
this scope + selector?} - P -->|yes| Q[deletecollection_scope] - P -->|no| R - - F -->|no| R{rv-only hatch?} + D -->|no match| R{rv-only hatch?} R -->|match| S[resource_version] R -->|no match| T{name tier?} T -->|match| U[name] T -->|no match| Z + F -->|yes| V{object terminating?} + V -->|no| W{a patch, update or delete
at this uid and rv?} + W -->|yes| E2[exact: what removed it] + W -->|no| AA + V -->|yes| AA{sticky removal pointer
for its uid?} + AA -->|yes| AB[delete_sticky] + AA -->|no| X{terminating, and a delete
at this uid and rv?} + X -->|yes| E2 + X -->|no| G{uid in a collection's
uid set?} + G -->|yes| H[deletecollection_body_uid] + G -->|no| L{a DELETE fact under
its uid or name?} + L -->|yes| M[latest / name] + L -->|no| P{collection covers
this scope + selector?} + P -->|yes| Q[deletecollection_scope] + P -->|no| Z + style Z fill:#7f1d1d,color:#fff style AB fill:#14532d,color:#fff style E fill:#14532d,color:#fff + style E2 fill:#14532d,color:#fff style H fill:#14532d,color:#fff - style J fill:#14532d,color:#fff style M fill:#14532d,color:#fff - style O fill:#166534,color:#fff style Q fill:#166534,color:#fff style S fill:#166534,color:#fff style U fill:#14532d,color:#fff ``` +A removal's ladder is shown for an object that was not terminating; for a terminating one, the +sticky pointer is asked first and only a delete at the exact version counts. A write fact anywhere +else (the object's last edit under its uid, its name, or another version) never answers a removal. + ### The tiers, strongest first | Tier | Key | `tier` label | What it asserts | |---|---|---|---| +| exact | uid + rv | `exact` | this actor produced this exact version; for a removal, the write that took the object out of view | | delete, sticky | uid (a delete fact, sticky) | `delete_sticky` | who asked for this object's deletion | -| exact | uid + rv | `exact` | this actor produced this exact version | | collection uid | uid in a collection's set | `deletecollection_body_uid` | the API server said this request deleted this object | -| latest, delete | uid | `latest` | this object's own delete fact | +| latest, delete | uid | `latest` | this object's own delete fact, if the sticky pointer was evicted | | name, delete | namespace + name | `name` | this object's own delete fact, when it has no uid | -| latest, write | uid | `latest` | who last wrote it; a fallback for a removal | | collection scope | namespace + selector + window | `deletecollection_scope` | a collection request covering it was made | | rv-only | rv | `resource_version` | a fact with an rv but no uid | | name | namespace + name | `name` | the only key an aggregated write has | -| absent | none | `absent` | committer-authored | +| absent | none | `absent` | the explicit unresolved author | + +For a terminating object the sticky row ranks above the exact row; see the rules below. Who the evidence named is the separate `actor_kind` label (`user` / `serviceaccount` / `none`), so every row above can be asked about either kind of actor. `tier` names the evidence and `actor_kind` @@ -400,33 +403,28 @@ here is last-writer-wins, and a finalizer patch's fact carries the resourceVersi stamped, so it lands under the deleter's exact key and the deleter's uid key alike. That used to attribute a finalized deletion to the cleanup controller rather than to the human who asked for it. A removal fact therefore also takes a **sticky removal pointer**, keyed by uid, that only another -removal fact may fill, and a removal consults that pointer before the exact tier. It is the one -structure the TTL does not bound: a uid is unique across space and time, so the statement can never -be superseded, and its horizon is the index's caps instead. It is strictly uid-keyed, because the -same stickiness on the name tier would be a defect: a name is reused after a delete and recreate. - -**A removal never returns on a write fact without looking further.** The per-object tiers are -last-writer-wins, so for a removal they hold whoever last **edited** the object, which is not who -deleted it. Such a match is held as a fallback while the search continues, and the caller keeps -waiting for delete evidence until the grace expires. A fact about the deletion, filed under any key, -ends the wait immediately. Waiting never costs an attribution: the worst case returns exactly what -returning early would have returned, one grace later. +removal fact may fill, and a removal of a terminating object consults that pointer before the exact +tier. It is the one structure the TTL does not bound: a uid is unique across space and time, so the +statement can never be superseded, and its horizon is the index's caps instead. It is strictly +uid-keyed, because the same stickiness on the name tier would be a defect: a name is reused after a +delete and recreate. + +**A removal is credited only to the request that caused it.** That is a fact at the removal's own +resourceVersion by a verb that produced it, or a fact about a deletion, filed under any key. The +object's last WRITE is not either: it says who edited the object, not who removed it, and a removal +with nothing better is unresolved when the grace expires. Every match ends the wait at once. **An exact-capable event may not fall through to the removal tiers.** A create or update presents the resourceVersion its own write produced. If the exact tier misses, the `latest` pointer may name an -older, different author, so the lookup skips straight to the rv hatch and the name tier. The gate is -one-directional, and only one of the two tiers is gated: the exact tier is tried for *any* query -carrying a uid and a resourceVersion, a removal included. What an exact-capable event may not do is -the reverse, reaching the tiers below. +older, different author, so the lookup skips straight to the rv hatch and the name tier. -**A removal from a label-selected collection resolves on the exact write alone.** When a rule +**A removal is told apart from a label exit by the facts, never by the selector.** When a rule carries `objectSelector`, a `DELETED` frame can mean the object stopped matching the selector rather -than that it was deleted, and the watch cannot tell the two apart. Such a removal reads only the fact -at the object's exact `(uid, resourceVersion)`: a `patch`, `update`, or `delete` there names the -actor, and ends the wait the moment it arrives. For a terminating object only a deletion there -counts, since that slot can hold the finalizer patch, which names whoever cleared a finalizer rather -than who asked for the deletion. Nothing else is consulted, and without that fact the removal is -unresolved when the grace expires. +than that it was deleted, and the watch cannot tell the two apart. Attribution does not try to either, +and it never asks whether the stream was selected. For an object that was **not** terminating, a +`patch`, `update` or `delete` at the frame's exact `(uid, resourceVersion)` names the actor and is +asked first; then deletion evidence, as for any removal. For a terminating object only a deletion at +that version counts, since that slot can hold the finalizer patch. The shapes behind this were measured against a real API server in the mutationlab `selector-membership` scenario @@ -438,15 +436,15 @@ replays them in every delivery order: relabeling write produced, and that write's audit response carries the same one. The exit is always attributable when its fact arrives. - A deletion never carries the `DELETED` frame's resourceVersion. An immediate delete is answered - with a `Status` naming only the uid, and a finalizer-held one is stamped one step before its - final `DELETED`. So a real deletion from a selected collection is unresolved here. A - finalizer-held one was already attributed when its `deletionTimestamp` arrived (§1). -- A deletion fact keyed by the uid alone is not evidence, however close it looks. An object - relabeled out and then deleted by someone else leaves exactly such a fact, and it can arrive - before the exit's own fact, or instead of it. It identifies the object, not the removal. + with a `Status` naming only the uid, so on a stream with no selector the exact slot stays empty + for a removal and the rule above never fires there. A real deletion from a selected collection + resolves on its delete fact, exactly as it would on an unselected one. +- An object relabeled out and then deleted by someone else leaves both a write at the exit's version + and a uid-only delete. When both are in the index, the exit's write is asked first and wins. When + only the delete is, the deleter is named: the one wrong author this design accepts, below. -An unselected removal, and a `MODIFIED` frame carrying a `deletionTimestamp` (deletion as intent, -§1), keep the rules above. Uncertain authorship costs the commit its author, never the removal. +The index files a whole delivered batch before it wakes any waiting removal, so a relabel and a +delete that arrive together are always read together. ### The wait @@ -507,31 +505,54 @@ Four details do real work: The registry exposes a `len()` purely so a test can assert a resolver left nothing behind. The loop around the `select` matters too, because a wake-up is only a hint. The resolver re-runs the -whole lookup, and if what arrived was not good enough (a write fact when it needs delete evidence) it -keeps waiting rather than treating the signal as a result. +whole lookup, and if what arrived does not answer this query (a write fact when a removal needs what +caused it) it keeps waiting rather than treating the signal as a result. **The wait blocks its shard.** `attachAuthor` runs on the watch shard's own goroutine and a shard -processes its events serially, so a removal that waits out its grace is head-of-line blocking for +processes its events serially, so an event that waits out its grace is head-of-line blocking for every later event of that type: the commit window for a subsequent write cannot open until its event is processed. This is a known, measured limitation rather than a subtlety: three removals in one e2e run spent 20.18s between them, and a Deployment create queued behind them opened its window about -ten seconds late. The lookup-ordering half of that is fixed (it is what the sticky pointer and the -tier ordering did); the structural half is open, and both it and the question of when a removal -should stop waiting are ranked in -[`../design/open-asks-priority.md`](../design/open-asks-priority.md). +ten seconds late. Later, a Flux prune on a label-selected stream did the same to a save's edit until +removals stopped asking about the selector. The lookup half of that is fixed; the structural half is +open, and both it and the question of when a removal should stop waiting are ranked in +[`../design/open-asks-priority.md`](../design/open-asks-priority.md). An event that waits out the grace +always resolves `absent`, so the +[suggested alert](../interpreting-metrics.md#suggested-alerts) on that tier is the alert on this. + +## What attribution gives up, on purpose + +Attribution does not wait for every author it could eventually find. Each trade below is a choice, +not an accident; anything that changes one changes this list. + +| Trade | What is lost | What is bought | +|---|---|---| +| Every event waits at most `--author-attribution-grace` (3s) for its fact | A fact that arrives later: the event ships unresolved | Bounded commit latency | +| A fact that arrives after its commit does not rewrite it | That commit's author | Immutable history | +| Initial snapshots, resyncs, and the relist after an expired watch cursor ship as the configured author | The author of anything changed during a watch gap that ends in a relist | No grace wait over every object of a type | +| A removal is credited only to the request that caused it | A removal with no delete fact is unresolved rather than credited to the object's last editor | No wrong author on a deletion | +| A removal waits for evidence it may never get: a type the audit policy drops, an aggregated create | The full grace on the shard, then unresolved | The right deleter whenever the fact is merely late. Ranked open in [removal-wait options](../design/attribution-removal-wait-options.md) | +| Attribution runs inline on the shard | Every later event on the shard waits behind the slowest lookup | Delivery order within a watch, with no buffer | +| **Accepted wrong author.** An object relabeled out of a selection and deleted by someone else within about one audit batch, where the delete's fact is in the index first (two API servers batching independently) or the relabel's fact is lost | The deleter is named for the exit. She did delete the object, but the relabel is what took it out of view | Every real deletion on a selected stream resolves on its delete fact in about one batch, instead of waiting out the grace and shipping unresolved | + +The last row is the one exception to "a wrong author is worse than no author", and it is narrow on +purpose: it needs a relabel and a delete of one object by two actors within about a second, and then +either an HA control plane reordering their audit, or a lost fact. Refusing it would have made every +deletion in a label-selected collection pay the full grace, and stall its stream for that long. ## No branch anywhere depends on the type Verified across the whole path: `internal/queue`, `internal/auditutil`, and the resolver contain no comparison against a group, a resource, a kind or an API version. The only `Resource ==` in the path -is `Resource == ""`, a presence check. +is `Resource == ""`, a presence check. Nor does anything read the watch's label selector: what the +API server selected is its business, and attribution only combines the facts that come in. Everything dispatches on one of three things: - **the verb**: `deletecollection` publishes a collection fact, and `delete` plus `deletecollection` are what `isRemovalVerb` recognizes as evidence about a deletion; -- **the operation kind**: `ExactCapable` is false for a removal, which is what unlocks the weaker - tiers; +- **the operation kind**: `ExactCapable` is false for a removal, which is what unlocks the removal + tiers, and `Terminating` (the object's `deletionTimestamp`) orders them; - **which fields are present**: uid, resourceVersion, name, request body, response body. **This is a requirement, not an accident**, and the corpus proves it. Two ConfigMap deletes, same diff --git a/docs/spec/watch-event-ordering-and-attribution-grace.md b/docs/spec/watch-event-ordering-and-attribution-grace.md index dfc630bd..93d2eedf 100644 --- a/docs/spec/watch-event-ordering-and-attribution-grace.md +++ b/docs/spec/watch-event-ordering-and-attribution-grace.md @@ -73,14 +73,12 @@ With attribution enabled, an unresolved event uses the explicit `unknown (attribution unresolved)` author. With attribution disabled, the configured committer also authors the change. A fact arriving after a commit is written does not rewrite it. -The index determines what counts as a resolution. For a removal query, even an exact UID/RV -PATCH match is held while [`FactIndex.Await`](../../internal/queue/fact_index.go) waits for deletion -evidence, up to the grace deadline. `ExactCapable=false` does not disable the exact lookup. -When the exact fact is missing, an earlier writer's fallback can be returned at expiry. A removal -from a label-selected collection is the exception: under the -[filtered-removal attribution policy](../design/watches-labels-simplification.md#attribution-for-filtered-removals) -it reads only the fact at the object's exact UID/RV, returns as soon as that fact arrives, and is -unresolved without it, so no earlier writer can author a label exit. Both paths keep the same +The index determines what counts as a resolution. A removal resolves only on a fact about the +request that caused it: the write at its own UID/RV (a label exit), or deletion evidence. Either ends +the wait in [`FactIndex.Await`](../../internal/queue/fact_index.go) at once; an earlier writer is +never returned, and without such a fact the removal is unresolved at the grace deadline. The policy +is the same whether or not the watch carries a label selector +([attribution contract](attribution.md#four-rules-that-are-easy-to-miss)), and it keeps the same inline ordering. ### Preserve ordered release if attribution becomes concurrent diff --git a/internal/queue/author_fact.go b/internal/queue/author_fact.go index 811fead2..2af1e233 100644 --- a/internal/queue/author_fact.go +++ b/internal/queue/author_fact.go @@ -71,16 +71,19 @@ const ( // AttributionDeleteSticky is the sticky removal pointer: a delete-verb fact filed by uid into a // slot no later WRITE may overwrite. The stickiness is the whole reason it can answer at all — // every other structure would have been overwritten by the finalizer patch following the - // delete. Consulted before the exact tier, and only by a removal. + // delete. Only a removal consults it: before the exact tier for a terminating object, after it + // for one that was not (see FactIndex.lookupRemoval). // // The only tier the TTL does not bound: a uid is unique across space and time, so the statement // can never be superseded. See docs/spec/attribution.md. AttributionDeleteSticky AttributionResult = "delete_sticky" // AttributionExact is an exact UID+resourceVersion match: this actor produced this exact version. + // For a removal it is the write that took the object out of view, such as a label exit. AttributionExact AttributionResult = "exact" - // AttributionLatest is the uid-latest tier — the object's own last write or its own delete fact, - // keyed by uid alone. It is the tier the removal path turns on: a match here that describes a - // WRITE is held as a fallback while the wait continues for evidence about the deletion. + // AttributionLatest is the uid-latest tier, the object's own last fact keyed by uid alone. A + // removal accepts it only when that fact is a delete, which the sticky pointer normally answers + // first, so it is reported only when the pointer was evicted. A write here is never an answer: + // it says who edited the object, not who removed it. AttributionLatest AttributionResult = "latest" // AttributionResourceVersion is the rv-only escape hatch: a fact that carried a resourceVersion // and no uid, matched on that version alone. It and AttributionLatest were one value ("weak") diff --git a/internal/queue/fact_index.go b/internal/queue/fact_index.go index b6c308b9..72f27264 100644 --- a/internal/queue/fact_index.go +++ b/internal/queue/fact_index.go @@ -90,17 +90,12 @@ type FactQuery struct { // resourceVersion. The watch side always knows it; only the audit side can be missing it. Name string // ExactCapable is true for ADDED and MODIFIED, whose resourceVersion is the one the write - // produced. A removal's is not, so it consults the weaker tiers the exact-capable events skip. + // produced. A removal's is not, so it consults the removal tiers the exact-capable events skip. ExactCapable bool - // FilteredRemoval marks a DELETED frame from a label-selected collection. The object may have - // left the selection rather than been deleted, and every weaker tier, a uid-only deletion - // included, can name someone who did not cause THIS removal. It resolves only on the write at - // the exact (uid, resourceVersion) (see lookupFilteredRemoval), returns as soon as that arrives, - // and is otherwise unresolved. ExactCapable is ignored for it. - FilteredRemoval bool - // Terminating reports that the removed object carried a deletionTimestamp. For a filtered - // removal it narrows the eligible evidence to a deletion: a finalizer PATCH at the final - // resourceVersion says who cleared a finalizer, not who asked for the deletion. + // Terminating reports that the removed object carried a deletionTimestamp. It decides what a + // fact at the removal's own resourceVersion means: for an object that was not terminating it is + // the write that took the object out of view (a label exit), and for a terminating one it can be + // the finalizer PATCH, which says who cleared a finalizer, not who asked for the deletion. Terminating bool } @@ -184,12 +179,14 @@ func NewFactIndex(cfg FactIndexConfig) *FactIndex { // whole retention window on start, so stamping those with the read time would give each a second // full TTL and let a restart resurrect facts the horizon had retired. Reading the append time off // the entry's position is what makes SweepInterval bound memory rather than correctness. +// +// The whole entry is filed before anyone is woken, so a resolver never reads half a batch. One audit +// batch can carry both the write that took an object out of a selection and a later delete of it, +// and a removal woken between the two would credit the delete. func (i *FactIndex) Apply(ctx context.Context, entry FactEntry) { scope := factScope{route: entry.Key.AuditRoute, groupResource: entry.Key.groupResource()} at := entryAppendTime(entry.ID, time.Now()) - for _, fact := range entry.Facts { - i.waiters.wake(i.store(ctx, scope, fact, at)) - } + i.waiters.wake(i.store(ctx, scope, entry.Facts, at)) } // entryAppendTime reads the append time out of a transport position. Stream IDs are millisecond @@ -219,25 +216,21 @@ func entryAppendTime(id string, now time.Time) time.Time { // The order of the first two statements is the design: the waiter is registered BEFORE the index // is read, so a fact applied in the gap signals a waiter that is already listening. // -// A match does not always end the wait. For a REMOVAL the strongest early fact is often the -// object's last WRITE, which says who edited it and nothing about who deleted it. Returning on -// that answered "who deleted this" with "who last edited it" whenever someone else touched the -// object first. Such a match is held as a FALLBACK and returned only if the grace expires, so -// waiting never loses attribution: the worst case is what returning early would have given. +// Every match ends the wait. A removal used to hold the object's last WRITE as a fallback and return +// it when the grace expired, which answered "who deleted this" with "who last edited it". Lookup now +// returns only a fact about the request that caused the removal (see lookupRemoval), so there is +// nothing to hold: a removal with no such fact is unresolved. func (i *FactIndex) Await(ctx context.Context, query FactQuery, grace time.Duration) AuthorResolution { waiter := i.waiters.register(query.waiterKeys()) defer i.waiters.unregister(waiter) - fallback := AuthorResolution{Result: AttributionAbsent} if resolution := i.Lookup(query); resolution.Result != AttributionAbsent { - if !query.awaitsBetterEvidence(resolution) { - recordFactEvent(ctx, factOpMatched) - return resolution - } - fallback = resolution + recordFactEvent(ctx, factOpMatched) + return resolution } + absent := AuthorResolution{Result: AttributionAbsent} if grace <= 0 { - return i.settle(ctx, fallback) + return absent } timer := time.NewTimer(grace) @@ -245,79 +238,35 @@ func (i *FactIndex) Await(ctx context.Context, query FactQuery, grace time.Durat for { select { case <-ctx.Done(): - return i.settle(ctx, fallback) + return absent case <-timer.C: - return i.settle(ctx, fallback) + return absent case <-waiter.ch: - resolution := i.Lookup(query) - if resolution.Result == AttributionAbsent { - continue - } - if !query.awaitsBetterEvidence(resolution) { + if resolution := i.Lookup(query); resolution.Result != AttributionAbsent { recordFactEvent(ctx, factOpMatched) return resolution } - fallback = resolution } } } -// settle returns the fallback a removal was holding, counting it as the match it is. Waiting is -// never allowed to turn an attribution into an absence: a removal whose only evidence is the last -// write still names that writer once nothing better has arrived. -func (i *FactIndex) settle(ctx context.Context, fallback AuthorResolution) AuthorResolution { - if fallback.Result != AttributionAbsent { - recordFactEvent(ctx, factOpMatched) - } - return fallback -} - -// awaitsBetterEvidence reports whether a match should be held as a fallback rather than returned. -// -// True for exactly one shape: a REMOVAL matched to a fact that is not about a removal. The sticky -// pointer holds only removal facts, so a match there ends the wait at once. The per-object tiers -// are last-writer-wins, so for a collection member they hold whoever edited it last. Both -// collection tiers, and a per-object fact whose own verb is a delete, end the wait. -func (q FactQuery) awaitsBetterEvidence(resolution AuthorResolution) bool { - if q.ExactCapable || q.FilteredRemoval || resolution.Result == AttributionAbsent { - return false - } - switch resolution.Result { - case AttributionDeleteSticky, AttributionDeleteCollectionBodyUID, AttributionDeleteCollectionScope: - return false - case AttributionExact, AttributionLatest, AttributionResourceVersion, - AttributionName, AttributionAbsent: - } - return !isRemovalVerb(resolution.Fact.Verb) -} - // isRemovalVerb reports whether a fact describes a deletion rather than a write. func isRemovalVerb(verb string) bool { return strings.EqualFold(verb, "delete") || strings.EqualFold(verb, deleteCollectionVerb) } -// Lookup reads the index once, trying the tiers strongest-first: +// Lookup reads the index once. A write (an ADDED or MODIFIED event) tries, strongest first: +// +// 1. the exact (uid, rv) fact; +// 2. the rv-only escape hatch; +// 3. the (namespace, name) floor. // -// 1. the sticky removal pointer for that uid, for a removal only; -// 2. the exact (uid, rv) fact, the only exact-capable join; -// 3. a collection fact whose uid set contains this object; -// 4. the last-writer-wins fact for that uid, for a removal whose rv never matches; -// 5. a collection fact whose scope, selector, and window cover it; -// 6. the rv-only escape hatch; -// 7. the (namespace, name) floor. +// A removal reads its own ladder (lookupRemoval), which returns only a fact about the request that +// caused the removal. // // The name tier is last because a name is reused after a delete and recreate, so it can name the // author of a previous object that held it, where a uid cannot. Nothing carrying a uid or rv // reaches it, so ranking it last costs the stronger tiers nothing. -// -// The two collection tiers sit on OPPOSITE sides of the latest tier on purpose. Uid membership -// outranks it because the two answer different questions: the latest tier says who last WROTE an -// object, a removal asks who DELETED it, and a collection delete files one fact about the -// collection, leaving the uid's latest entry holding whoever wrote the object last. Uid membership -// is the API server stating THIS request deleted THIS object, so nothing weaker may answer first. -// -// Scope matching stays below because it can name the wrong human: an unrelated delete by another -// actor in the same window is claimed by its own fact at tier 3 and never reaches tier 4. func (i *FactIndex) Lookup(query FactQuery) AuthorResolution { now := time.Now() cutoff := now.Add(-i.ttl) @@ -328,36 +277,14 @@ func (i *FactIndex) Lookup(query FactQuery) AuthorResolution { if !ok { return AuthorResolution{Result: AttributionAbsent} } - if query.FilteredRemoval { - return lookupFilteredRemoval(facts, query, cutoff) - } - return i.lookupTiers(facts, query, now, cutoff) -} - -// lookupTiers is Lookup's ladder for every query but a filtered removal. The caller holds i.mu. -func (i *FactIndex) lookupTiers(facts *scopeFacts, query FactQuery, now, cutoff time.Time) AuthorResolution { - // The removal pointer is consulted BEFORE the exact tier, and only for an event that is not - // exact-capable. That ordering is half of the fix and useless without the other half: the slot is - // what preserves the deleter's fact, and being asked first is what makes it reachable. A removal's - // own resourceVersion is the one the DELETION stamped, so the exact tier can hold a fact about the - // finalizer patch that carries the very same version — a stronger-looking answer to a question it - // is not about. An exact-capable event never reaches here, so a create or update still resolves at - // the exact tier exactly as before. if !query.ExactCapable { - if resolution := stickyRemoval(facts, query); resolution.Result != AttributionAbsent { - return resolution - } + return i.lookupRemoval(facts, query, now, cutoff) } if query.UID != "" && query.ResourceVersion != "" { if fact, found := facts.lookupExact(query.UID, query.ResourceVersion, cutoff); found { return AuthorResolution{Fact: fact, Result: AttributionExact} } } - if !query.ExactCapable { - if resolution := i.lookupRemoval(facts, query, now, cutoff); resolution.Result != AttributionAbsent { - return resolution - } - } if query.ResourceVersion != "" { if fact, found := facts.lookupRV(query.ResourceVersion, cutoff); found { return AuthorResolution{Fact: fact, Result: AttributionResourceVersion} @@ -371,107 +298,113 @@ func (i *FactIndex) lookupTiers(facts *scopeFacts, query FactQuery, now, cutoff return AuthorResolution{Result: AttributionAbsent} } -// lookupFilteredRemoval is the whole lookup for a filtered removal: the write at the removed -// object's exact (uid, resourceVersion), and nothing else. See FactQuery.FilteredRemoval. +// lookupRemoval answers "which request caused this removal" and nothing weaker. The caller holds +// i.mu. Two kinds of fact can answer it: // -// The shapes were measured against a real API server (test/mutationlab/e2e/selector_membership_test.go). -// A label exit's DELETED carries the resourceVersion the relabeling PATCH or UPDATE produced, and -// that write's audit response carries the same one, so the exit's actor is exactly findable. +// - A fact AT the removal's own resourceVersion, by a verb that produced that version. When the +// object was not terminating, a write there is what took the object out of view: a label exit, +// whose DELETED carries the resourceVersion the relabeling PATCH or UPDATE produced, which that +// write's audit response carries too (measured in test/mutationlab/e2e/selector_membership_test.go). +// A real deletion never fills that slot (an immediate delete is answered with a Status naming the +// uid alone), so on a stream with no selector it is simply empty. That is why nothing here asks +// whether the stream was label-selected. +// - A fact about a DELETION: the sticky removal pointer, a collection that named the uid, the +// object's own delete by uid or name, or a collection whose scope covers it. // -// A deletion fact keyed by the uid alone is NOT evidence here, however tempting. It identifies the -// object, not the removal: an object relabeled out and then deleted outright by someone else has a -// uid-only deletion fact (an immediate delete is answered with a Status naming the uid and no -// resourceVersion) that has nothing to do with the exit, and it can arrive before the exit's own -// fact or instead of it. A deletion never carries the DELETED frame's resourceVersion, so a real -// deletion from a selected collection is unresolved here; a finalizer-held one was already -// attributed when its deletionTimestamp arrived (deletion as intent). Uncertain authorship costs -// the commit its author, never the removal. -func lookupFilteredRemoval(facts *scopeFacts, query FactQuery, cutoff time.Time) AuthorResolution { - if query.UID == "" || query.ResourceVersion == "" { - return AuthorResolution{Result: AttributionAbsent} - } - if fact, found := facts.lookupExact(query.UID, query.ResourceVersion, cutoff); found && - filteredRemovalEvidence(fact.Verb, query.Terminating) { - return AuthorResolution{Fact: fact, Result: AttributionExact} - } - return AuthorResolution{Result: AttributionAbsent} -} - -// filteredRemovalEvidence reports whether a fact at the removed object's exact version may name -// the actor of a filtered removal. -func filteredRemovalEvidence(verb string, terminating bool) bool { - if isRemovalVerb(verb) { - return true - } - return !terminating && (strings.EqualFold(verb, "patch") || strings.EqualFold(verb, "update")) -} - -// stickyRemoval reads the head of the removal ladder: the uid-keyed slot holding the fact that this -// object was DELETED, which no later fact about a write may overwrite. It is the one tier consulted -// before the exact tier, and only for an event that is not exact-capable. -func stickyRemoval(facts *scopeFacts, query FactQuery) AuthorResolution { - if query.UID == "" { - return AuthorResolution{Result: AttributionAbsent} - } - if fact, found := facts.lookupRemovalPointer(query.UID); found { - return AuthorResolution{Fact: fact, Result: AttributionDeleteSticky} - } - return AuthorResolution{Result: AttributionAbsent} -} - -// lookupRemoval reads the tiers only a removal may consult, in the order they rank: the uid set of -// a collection that named this object, the object's own delete fact (by uid, then by name), its -// last-writer fact, and finally a collection whose scope covers it. See Lookup for why uid -// membership and scope matching sit on opposite sides of the latest tier. +// The order between the two depends only on the object. For one that was not terminating, the fact +// at its version comes first: when an object is relabeled out and then deleted by someone else, the +// relabel is the removal a selected stream saw. For a terminating object the sticky pointer comes +// first, because a finalizer PATCH can carry the version the final DELETED shows, and only a delete +// at that version counts. +// +// A write fact anywhere else is not evidence. The object's last writer says who edited it, not who +// removed it, and a removal with nothing better is unresolved: a wrong author is worse than none. // -// The ordering rule inside it is the one the whole removal path turns on: a fact about the DELETION -// outranks a fact about a write, whichever key each happens to be filed under. A write fact answers -// "who last edited this", which is not the question a removal asks. +// The one case this ladder can get wrong is accepted and documented (docs/spec/attribution.md): an object +// relabeled out of a selection and deleted by someone else within about one audit batch, where the +// delete's fact is in the index before the relabel's (two API servers batching independently) or the +// relabel's fact is lost. The deleter is named; she did delete the object. func (i *FactIndex) lookupRemoval( facts *scopeFacts, query FactQuery, now, cutoff time.Time, ) AuthorResolution { - var writeFallback AuthorResolution - haveWriteFallback := false + atVersion := lookupAtVersion(facts, query, cutoff) + if !query.Terminating && atVersion.Result != AttributionAbsent { + return atVersion + } + if resolution := stickyRemoval(facts, query); resolution.Result != AttributionAbsent { + return resolution + } + if atVersion.Result != AttributionAbsent { + return atVersion + } if query.UID != "" { if fact, found := facts.matchCollectionUID(query, cutoff); found { return AuthorResolution{Fact: fact, Result: AttributionDeleteCollectionBodyUID} } - if fact, found := facts.lookupLatest(query.UID, cutoff); found { - resolution := AuthorResolution{Fact: fact, Result: AttributionLatest} - // The object's own delete fact: the strongest thing a removal can hope for below uid - // membership, so it ends the search here. - if isRemovalVerb(fact.Verb) { - return resolution - } - // A WRITE fact says who last edited the object, not who deleted it. Hold it and keep - // looking for evidence about the deletion rather than answering with it. - writeFallback, haveWriteFallback = resolution, true + if fact, found := facts.lookupLatest(query.UID, cutoff); found && isRemovalVerb(fact.Verb) { + return AuthorResolution{Fact: fact, Result: AttributionLatest} } } // The object's own delete fact keyed by NAME, the only key it has when the audit event names no // uid: an aggregated API's delete, whose body the API server never decodes. A delete answered // with a Status on an ordinary type names the uid in its details, so that fact reaches the uid // tiers above (auditutil.IdentityFromAuditEvent). - // - // It must be reachable HERE, above the write fallback, or not at all for a removal: returning - // the uid tier's write fact ends the lookup, and the caller then waits out the whole grace for - // evidence sitting in this tier. That wait blocks the watch shard's serial goroutine. if query.Name != "" { if fact, found := facts.lookupName(query.Namespace, query.Name, cutoff); found && isRemovalVerb(fact.Verb) { return AuthorResolution{Fact: fact, Result: AttributionName} } } - if haveWriteFallback { - return writeFallback - } + // Scope matching is last because it can name the wrong human: an unrelated delete by another + // actor in the same window is claimed by its own fact above and never reaches here. if fact, found := facts.matchCollectionScope(query, now, cutoff, i.collectionWindow); found { return AuthorResolution{Fact: fact, Result: AttributionDeleteCollectionScope} } return AuthorResolution{Result: AttributionAbsent} } +// lookupAtVersion reads the fact filed at the removal's own resourceVersion, by uid and rv or, for a +// fact that carries no uid, by rv alone, and keeps it only when its verb can have caused the removal. +func lookupAtVersion(facts *scopeFacts, query FactQuery, cutoff time.Time) AuthorResolution { + if query.ResourceVersion == "" { + return AuthorResolution{Result: AttributionAbsent} + } + if query.UID != "" { + if fact, found := facts.lookupExact(query.UID, query.ResourceVersion, cutoff); found && + causedRemovalAtVersion(fact.Verb, query.Terminating) { + return AuthorResolution{Fact: fact, Result: AttributionExact} + } + } + if fact, found := facts.lookupRV(query.ResourceVersion, cutoff); found && + causedRemovalAtVersion(fact.Verb, query.Terminating) { + return AuthorResolution{Fact: fact, Result: AttributionResourceVersion} + } + return AuthorResolution{Result: AttributionAbsent} +} + +// causedRemovalAtVersion reports whether a fact at the removal's own resourceVersion names the +// request that caused it. A delete always does. A PATCH or UPDATE does only for an object that was +// not terminating: for a terminating one it is the finalizer being cleared. +func causedRemovalAtVersion(verb string, terminating bool) bool { + if isRemovalVerb(verb) { + return true + } + return !terminating && (strings.EqualFold(verb, "patch") || strings.EqualFold(verb, "update")) +} + +// stickyRemoval reads the uid-keyed slot holding the fact that this object was DELETED, which no +// later fact about a write may overwrite. +func stickyRemoval(facts *scopeFacts, query FactQuery) AuthorResolution { + if query.UID == "" { + return AuthorResolution{Result: AttributionAbsent} + } + if fact, found := facts.lookupRemovalPointer(query.UID); found { + return AuthorResolution{Fact: fact, Result: AttributionDeleteSticky} + } + return AuthorResolution{Result: AttributionAbsent} +} + // Run follows the subscription set until the context ends, applying what it reads and reporting // what it lost. It returns only when the context ends: a transport failure is retried, because a // follower that gave up would leave attribution silently dead for the life of the process. @@ -543,18 +476,21 @@ func (i *FactIndex) Len() int { return i.total } -// store files one fact under every structure it can serve and returns the waiter keys it filled. -// The policy mirrors the tiers Lookup reads: a collection fact is about a collection and lands only -// there, a uid-bearing fact takes the exact and latest tiers, and the rv-only hatch exists for the -// fact that has an rv and no uid — a uid-bearing fact's rv-only entry would be dead, since the watch -// side always carries a uid. -func (i *FactIndex) store(ctx context.Context, scope factScope, fact AuthorFact, now time.Time) []factWaiterKey { +// store files one entry's facts under every structure each can serve and returns the waiter keys +// they filled. The policy mirrors the tiers Lookup reads: a collection fact is about a collection and +// lands only there, a uid-bearing fact takes the exact and latest tiers, and the rv-only hatch exists +// for the fact that has an rv and no uid. A uid-bearing fact's rv-only entry would be dead, since the +// watch side always carries a uid. +func (i *FactIndex) store(ctx context.Context, scope factScope, batch []AuthorFact, now time.Time) []factWaiterKey { i.mu.Lock() defer i.mu.Unlock() facts := i.scopeFor(scope) before := facts.count - keys := i.file(facts, scope, fact, now) + var keys []factWaiterKey + for _, fact := range batch { + keys = append(keys, i.file(facts, scope, fact, now)...) + } i.total += facts.count - before i.enforceCaps(ctx, scope, facts) return keys @@ -702,13 +638,6 @@ func (q FactQuery) scope() factScope { func (q FactQuery) waiterKeys() []factWaiterKey { scope := q.scope() var keys []factWaiterKey - if q.FilteredRemoval { - // Only the exact slot can answer. - if q.UID == "" || q.ResourceVersion == "" { - return nil - } - return []factWaiterKey{{scope: scope, kind: factKindExact, value: exactWaiterValue(q.UID, q.ResourceVersion)}} - } if q.UID != "" && q.ResourceVersion != "" { keys = append(keys, factWaiterKey{scope: scope, kind: factKindExact, value: exactWaiterValue(q.UID, q.ResourceVersion)}) diff --git a/internal/queue/fact_index_corpus_test.go b/internal/queue/fact_index_corpus_test.go index 6bb2d5b0..de86f745 100644 --- a/internal/queue/fact_index_corpus_test.go +++ b/internal/queue/fact_index_corpus_test.go @@ -95,7 +95,8 @@ func TestFactIndex_FinalizerRemovalNamesTheDeleter(t *testing.T) { // The watch event the deletion-as-intent rule renders as a removal: the object as it looked when // deletionTimestamp appeared, so its resourceVersion is the colliding one. ExactCapable is false - // because the operator maps it to a DELETE (operationForLiveTargetWatchEvent). + // because the operator maps it to a DELETE (operationForLiveTargetWatchEvent), and Terminating is + // true because the object carries the deletionTimestamp that made it one. removal := FactQuery{ AuditRoute: "prod-eu-1", GroupResource: schema.GroupResource{Resource: "configmaps"}, @@ -104,6 +105,7 @@ func TestFactIndex_FinalizerRemovalNamesTheDeleter(t *testing.T) { Namespace: deleteFact.Namespace, Name: deleteFact.Name, ExactCapable: false, + Terminating: true, } t.Run("the deleter is named while their fact is the only one filed", func(t *testing.T) { diff --git a/internal/queue/fact_index_filtered_removal_test.go b/internal/queue/fact_index_filtered_removal_test.go deleted file mode 100644 index 5b8e4965..00000000 --- a/internal/queue/fact_index_filtered_removal_test.go +++ /dev/null @@ -1,154 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 - -package queue - -import ( - "testing" - "time" - - "github.com/stretchr/testify/require" -) - -// A filtered removal is a DELETED frame from a label-selected collection: the object may have left -// the selection rather than been deleted. These tests pin its policy: exact evidence resolves at -// once, and every weaker tier a physical removal would fall back to is refused. - -func filteredRemovalQuery(route string, terminating bool) FactQuery { - query := objectQuery(route, factIndexTestUID, "11", false) - query.Name = "web" - query.Labels = map[string]string{"team": "a"} - query.FilteredRemoval = true - query.Terminating = terminating - return query -} - -func verbFact(author, rv, verb string) AuthorFact { - fact := objectFact(author, rv) - fact.Verb = verb - fact.Name = "web" - return fact -} - -// weakerEvidence is every fallback a physical removal could resolve through, none of which may -// name the actor of a label exit: Alice's last write, her deletion of an earlier version (sticky), -// her deletecollection covering the object, an rv-only fact at the removal's version, and a -// name-only delete. -func weakerEvidence(rv string) []AuthorFact { - return []AuthorFact{ - objectFact("alice", "10"), - verbFact("alice", "9", "delete"), - aliceCollectionFact("", factIndexTestUID), - {Namespace: "team-a", ResourceVersion: rv, Author: "alice", Verb: "patch"}, - {Namespace: "team-a", Name: "web", Author: "alice", Verb: "delete"}, - } -} - -func TestFactIndex_FilteredRemovalResolvesTheExactLabelWriteImmediately(t *testing.T) { - for _, verb := range []string{"patch", "update"} { - t.Run(verb, func(t *testing.T) { - route := "filtered-" + verb - h := newFactIndexHarness(t, FactIndexConfig{}) - evidence := append(weakerEvidence("11"), verbFact("bob", "11", verb)) - h.publish(factIndexTestStream(route), evidence...) - h.waitForFacts(len(evidence)) - - started := time.Now() - resolution := h.resolve(filteredRemovalQuery(route, false)) - require.Equal(t, AttributionExact, resolution.Result) - require.Equal(t, "bob", resolution.Fact.Author) - require.Less(t, time.Since(started), time.Second, "exact evidence must not wait out the grace") - }) - } -} - -func TestFactIndex_FilteredRemovalResolvesALateLabelWriteOnArrival(t *testing.T) { - const route = "filtered-late" - h := newFactIndexHarness(t, FactIndexConfig{}) - evidence := weakerEvidence("11") - h.publish(factIndexTestStream(route), evidence...) - h.waitForFacts(len(evidence)) - - done := make(chan AuthorResolution, 1) - started := time.Now() - go func() { - done <- h.index.Await(t.Context(), filteredRemovalQuery(route, false), factIndexTestGrace) - }() - time.Sleep(50 * time.Millisecond) - h.publish(factIndexTestStream(route), verbFact("bob", "11", "patch")) - - resolution := <-done - require.Equal(t, "bob", resolution.Fact.Author) - require.Less(t, time.Since(started), factIndexTestGrace/2, "a late arrival ends the wait when it lands") -} - -func TestFactIndex_FilteredRemovalRefusesEveryWeakerTier(t *testing.T) { - const route = "filtered-weak" - h := newFactIndexHarness(t, FactIndexConfig{}) - evidence := weakerEvidence("11") - h.publish(factIndexTestStream(route), evidence...) - h.waitForFacts(len(evidence)) - - h.absent(filteredRemovalQuery(route, false)) - - // The same evidence still attributes an ordinary removal, so the refusal is the policy's. - unfiltered := filteredRemovalQuery(route, false) - unfiltered.FilteredRemoval = false - require.NotEqual(t, AttributionAbsent, h.resolve(unfiltered).Result) -} - -func TestFactIndex_FilteredRemovalOfATerminatingObjectNeedsTheDeletion(t *testing.T) { - const route = "filtered-terminating" - h := newFactIndexHarness(t, FactIndexConfig{}) - h.publish(factIndexTestStream(route), verbFact("finalizer-controller", "11", "patch")) - h.waitForFacts(1) - - h.absent(filteredRemovalQuery(route, true)) - - h.publish(factIndexTestStream(route), verbFact("carol", "11", "delete")) - resolution := h.resolve(filteredRemovalQuery(route, true)) - require.Equal(t, "carol", resolution.Fact.Author, "exact deletion evidence names the initiator") -} - -func TestFactIndex_FilteredRemovalAcceptsAnExactDeletion(t *testing.T) { - const route = "filtered-deleted" - h := newFactIndexHarness(t, FactIndexConfig{}) - h.publish(factIndexTestStream(route), verbFact("dave", "11", "delete")) - - resolution := h.resolve(filteredRemovalQuery(route, false)) - require.Equal(t, AttributionExact, resolution.Result) - require.Equal(t, "dave", resolution.Fact.Author) -} - -// Bob relabels the object out at rv 11 and Carol then deletes it; her delete is answered with a -// Status, so her fact carries the uid and no resourceVersion. It reaches the index first, and Bob's -// exact fact is delayed, not lost: the exit is still Bob's. -func TestFactIndex_FilteredRemovalWaitsForADelayedExitFactPastAUIDOnlyDeletion(t *testing.T) { - const route = "filtered-delayed-exit" - h := newFactIndexHarness(t, FactIndexConfig{}) - h.publish(factIndexTestStream(route), verbFact("carol", "", "delete")) - h.waitForFacts(1) - done := make(chan AuthorResolution, 1) - go func() { - done <- h.index.Await(t.Context(), filteredRemovalQuery(route, false), factIndexTestGrace) - }() - time.Sleep(50 * time.Millisecond) - h.publish(factIndexTestStream(route), verbFact("bob", "11", "patch")) - - resolution := <-done - require.Equal(t, AttributionExact, resolution.Result) - require.Equal(t, "bob", resolution.Fact.Author) -} - -// Without the exit's own fact, a uid-only deletion by someone else leaves the removal unresolved. -func TestFactIndex_FilteredRemovalRefusesAUIDOnlyDeletion(t *testing.T) { - for _, terminating := range []bool{false, true} { - route := "filtered-uid-only-delete" - if terminating { - route += "-terminating" - } - h := newFactIndexHarness(t, FactIndexConfig{}) - h.publish(factIndexTestStream(route), verbFact("carol", "", "delete"), verbFact("dave", "10", "delete")) - h.waitForFacts(2) - h.absent(filteredRemovalQuery(route, terminating)) - } -} diff --git a/internal/queue/fact_index_removal_cause_test.go b/internal/queue/fact_index_removal_cause_test.go new file mode 100644 index 00000000..fa3e163a --- /dev/null +++ b/internal/queue/fact_index_removal_cause_test.go @@ -0,0 +1,174 @@ +// SPDX-License-Identifier: Apache-2.0 + +package queue + +import ( + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +// A removal is attributed to the request that caused it and to nothing weaker: a fact at the +// removal's own resourceVersion (a label exit's write), or a fact about a deletion. The policy is +// decided by the shape of the facts and the object, never by whether the stream carried a label +// selector. These tests pin it, including the one case it is known to get wrong. + +func removalQuery(route string, terminating bool) FactQuery { + query := objectQuery(route, factIndexTestUID, "11", false) + query.Name = "web" + query.Labels = map[string]string{"team": "a"} + query.Terminating = terminating + return query +} + +func verbFact(author, rv, verb string) AuthorFact { + fact := objectFact(author, rv) + fact.Verb = verb + fact.Name = "web" + return fact +} + +// deletionEvidence is every fact about a deletion a removal can resolve through, all by Alice: her +// deletion (sticky), her deletecollection naming the object, and a name-only delete. +func deletionEvidence() []AuthorFact { + return []AuthorFact{ + verbFact("alice", "9", "delete"), + aliceCollectionFact("", factIndexTestUID), + {Namespace: "team-a", Name: "web", Author: "alice", Verb: "delete"}, + } +} + +// A write at the removal's own version is what took the object out of view, and it outranks any +// deletion evidence: the selected stream saw the exit, not the delete. +func TestFactIndex_RemovalNamesTheWriteAtItsOwnVersionFirst(t *testing.T) { + for _, verb := range []string{"patch", "update"} { + t.Run(verb, func(t *testing.T) { + route := "removal-exit-" + verb + h := newFactIndexHarness(t, FactIndexConfig{}) + evidence := append(deletionEvidence(), verbFact("bob", "11", verb)) + h.publish(factIndexTestStream(route), evidence...) + h.waitForFacts(len(evidence)) + + started := time.Now() + resolution := h.resolve(removalQuery(route, false)) + require.Equal(t, AttributionExact, resolution.Result) + require.Equal(t, "bob", resolution.Fact.Author) + require.Less(t, time.Since(started), time.Second, "the exit's write must not wait out the grace") + }) + } +} + +func TestFactIndex_RemovalResolvesALateExitWriteOnArrival(t *testing.T) { + const route = "removal-exit-late" + h := newFactIndexHarness(t, FactIndexConfig{}) + done := make(chan AuthorResolution, 1) + started := time.Now() + go func() { + done <- h.index.Await(t.Context(), removalQuery(route, false), factIndexTestGrace) + }() + time.Sleep(50 * time.Millisecond) + h.publish(factIndexTestStream(route), verbFact("bob", "11", "patch")) + + resolution := <-done + require.Equal(t, "bob", resolution.Fact.Author) + require.Less(t, time.Since(started), factIndexTestGrace/2, "a late arrival ends the wait when it lands") +} + +// A real deletion, from any stream, resolves on the delete's own fact as soon as it lands. It used to +// wait out the whole grace on a label-selected stream and then ship unresolved. +func TestFactIndex_RemovalResolvesAUIDOnlyDeletionAtOnce(t *testing.T) { + for _, terminating := range []bool{false, true} { + route := "removal-uid-only-delete" + if terminating { + route += "-terminating" + } + h := newFactIndexHarness(t, FactIndexConfig{}) + h.publish(factIndexTestStream(route), verbFact("carol", "", "delete")) + h.waitForFacts(1) + + started := time.Now() + resolution := h.resolve(removalQuery(route, terminating)) + require.Equal(t, AttributionDeleteSticky, resolution.Result) + require.Equal(t, "carol", resolution.Fact.Author) + require.Less(t, time.Since(started), time.Second) + } +} + +// The object's last writer is not evidence about its removal. A removal with nothing but writes at +// other versions is unresolved, however long it waits: a wrong author is worse than none. +func TestFactIndex_RemovalDoesNotNameTheLastWriter(t *testing.T) { + const route = "removal-last-writer" + h := newFactIndexHarness(t, FactIndexConfig{}) + writes := []AuthorFact{ + objectFact("alice", "10"), + {Namespace: "team-a", ResourceVersion: "10", Author: "alice", Verb: "patch"}, + {Namespace: "team-a", Name: "web", Author: "alice", Verb: "update"}, + } + h.publish(factIndexTestStream(route), writes...) + h.waitForFacts(len(writes)) + + h.absent(removalQuery(route, false)) + h.absent(removalQuery(route, true)) +} + +// A fact that carries no uid, filed by resourceVersion alone, names the exit the same way. +func TestFactIndex_RemovalNamesAnRVOnlyWriteAtItsOwnVersion(t *testing.T) { + const route = "removal-rv-only-exit" + h := newFactIndexHarness(t, FactIndexConfig{}) + h.publish(factIndexTestStream(route), + AuthorFact{Namespace: "team-a", ResourceVersion: "11", Author: "bob", Verb: "patch"}) + + resolution := h.resolve(removalQuery(route, false)) + require.Equal(t, AttributionResourceVersion, resolution.Result) + require.Equal(t, "bob", resolution.Fact.Author) +} + +// For a terminating object the fact at its version can be the finalizer PATCH, which says who cleared +// a finalizer rather than who asked for the deletion. Only a deletion counts. +func TestFactIndex_RemovalOfATerminatingObjectNeedsTheDeletion(t *testing.T) { + const route = "removal-terminating" + h := newFactIndexHarness(t, FactIndexConfig{}) + h.publish(factIndexTestStream(route), verbFact("finalizer-controller", "11", "patch")) + h.waitForFacts(1) + + h.absent(removalQuery(route, true)) + + h.publish(factIndexTestStream(route), verbFact("carol", "11", "delete")) + resolution := h.resolve(removalQuery(route, true)) + require.Equal(t, "carol", resolution.Fact.Author, "the deletion names the initiator") +} + +// Bob relabels the object out and Carol deletes it, and both facts arrive in ONE audit batch, Carol's +// first. The index files the whole batch before it wakes anyone, so a removal already waiting reads +// both and names the exit. +func TestFactIndex_RemovalReadsAWholeBatchBeforeWaking(t *testing.T) { + const route = "removal-one-batch" + h := newFactIndexHarness(t, FactIndexConfig{}) + done := make(chan AuthorResolution, 1) + go func() { + done <- h.index.Await(t.Context(), removalQuery(route, false), factIndexTestGrace) + }() + time.Sleep(50 * time.Millisecond) + h.publish(factIndexTestStream(route), verbFact("carol", "", "delete"), verbFact("bob", "11", "patch")) + + resolution := <-done + require.Equal(t, AttributionExact, resolution.Result) + require.Equal(t, "bob", resolution.Fact.Author) +} + +// ACCEPTED RISK, documented in docs/spec/attribution.md. Bob relabels the object out and Carol then +// deletes it, and Carol's fact reaches the index in an earlier batch than Bob's: two API servers +// batching independently, or Bob's fact lost. The removal ends on Carol's delete and names her. She +// did delete the object, but Bob's write is what took it out of the selection. Waiting for Bob instead +// would make every real deletion on a selected stream wait out the grace and ship unresolved. +func TestFactIndex_RemovalNamesTheDeleterWhenTheExitFactArrivesLater(t *testing.T) { + const route = "removal-exit-after-delete" + h := newFactIndexHarness(t, FactIndexConfig{}) + h.publish(factIndexTestStream(route), verbFact("carol", "", "delete")) + h.waitForFacts(1) + + resolution := h.resolve(removalQuery(route, false)) + require.Equal(t, AttributionDeleteSticky, resolution.Result) + require.Equal(t, "carol", resolution.Fact.Author) +} diff --git a/internal/queue/fact_index_selector_corpus_test.go b/internal/queue/fact_index_selector_corpus_test.go index a89a6f91..de3dbce7 100644 --- a/internal/queue/fact_index_selector_corpus_test.go +++ b/internal/queue/fact_index_selector_corpus_test.go @@ -14,9 +14,10 @@ import ( "sigs.k8s.io/yaml" ) -// These drive the filtered-removal policy with the shapes the selector-membership scenario captured -// from a real API server (test/mutationlab/e2e/selector_membership_test.go, row 19), delivered in -// the orders an audit stream can produce. See FactQuery.FilteredRemoval. +// These drive the removal ladder with the shapes the selector-membership scenario captured from a +// real API server (test/mutationlab/e2e/selector_membership_test.go, row 19), delivered in the orders +// an audit stream can produce. The query carries no selector: the shapes alone decide. See +// FactIndex.lookupRemoval. const corpusSelectorMembershipDir = "../../test/mutationlab/corpus/configmap/selector-membership" @@ -85,7 +86,6 @@ func corpusRemovalQuery(route string, removal selectedRemoval) FactQuery { UID: removal.uid, ResourceVersion: removal.resourceVersion, Namespace: "", - FilteredRemoval: true, Terminating: removal.terminating, } } @@ -95,10 +95,10 @@ func configMapFactStream(route string) FactStreamKey { } // Bob relabels cm-exit-deleted out of the selection; Carol then deletes the same uid. Carol's delete -// is a Status naming that uid with no resourceVersion. It identifies the object and says nothing -// about which removal it caused, so it may never name the author of Bob's exit: not when Bob's fact -// is merely late, and not when it never arrives. -func TestFilteredRemoval_CapturedExitThenDeletionNamesOnlyTheExit(t *testing.T) { +// is a Status naming that uid with no resourceVersion, and Bob's write sits at the exit's exact +// (uid, resourceVersion). When both are in hand, the exit is Bob's. When Carol's is in hand first, +// the removal names her: the accepted risk documented in docs/spec/attribution.md. +func TestRemoval_CapturedExitThenDeletion(t *testing.T) { exits := corpusSelectedRemovals(t, "cm-exit-deleted") require.Len(t, exits, 1, "the selected stream holds one removal: the exit") exit := exits[0] @@ -112,39 +112,31 @@ func TestFilteredRemoval_CapturedExitThenDeletionNamesOnlyTheExit(t *testing.T) require.Equal(t, exit.uid, bob.UID) require.Equal(t, exit.resourceVersion, bob.ResourceVersion) - t.Run("the exit's fact arrives after the deletion's, within the grace", func(t *testing.T) { - const route = "corpus-exit-delayed" - h := newFactIndexHarness(t, FactIndexConfig{}) - h.publish(configMapFactStream(route), carol) - h.waitForFacts(1) - done := make(chan AuthorResolution, 1) - go func() { - done <- h.index.Await(t.Context(), corpusRemovalQuery(route, exit), factIndexTestGrace) - }() - time.Sleep(50 * time.Millisecond) - h.publish(configMapFactStream(route), bob) - - resolution := <-done - require.Equal(t, AttributionExact, resolution.Result) - require.Equal(t, "bob@example.com", resolution.Fact.Author, "the later deleter must not take the exit") - }) t.Run("both arrive in one batch, the deletion first", func(t *testing.T) { const route = "corpus-exit-batched" h := newFactIndexHarness(t, FactIndexConfig{}) h.publish(configMapFactStream(route), carol, bob) require.Equal(t, "bob@example.com", h.resolve(corpusRemovalQuery(route, exit)).Fact.Author) }) - t.Run("the exit's fact never arrives", func(t *testing.T) { + t.Run("both in hand before the removal is attributed", func(t *testing.T) { + const route = "corpus-exit-both" + h := newFactIndexHarness(t, FactIndexConfig{}) + h.publish(configMapFactStream(route), carol) + h.publish(configMapFactStream(route), bob) + h.waitForFacts(2) + require.Equal(t, "bob@example.com", h.resolve(corpusRemovalQuery(route, exit)).Fact.Author) + }) + t.Run("accepted risk: only the deletion's fact is in hand", func(t *testing.T) { const route = "corpus-exit-missing" h := newFactIndexHarness(t, FactIndexConfig{}) h.publish(configMapFactStream(route), carol) h.waitForFacts(1) - h.absent(corpusRemovalQuery(route, exit)) + require.Equal(t, "carol@example.com", h.resolve(corpusRemovalQuery(route, exit)).Fact.Author) }) } // Both of cm-member's exits, a PUT and a PATCH, are named from the captured writes. -func TestFilteredRemoval_CapturedExitsNameTheRelabelingWrite(t *testing.T) { +func TestRemoval_CapturedExitsNameTheRelabelingWrite(t *testing.T) { exits := corpusSelectedRemovals(t, "cm-member") require.Len(t, exits, 2) const route = "corpus-member-exits" @@ -163,19 +155,20 @@ func TestFilteredRemoval_CapturedExitsNameTheRelabelingWrite(t *testing.T) { // A real deletion from a selected collection never carries its DELETED's resourceVersion: an // immediate delete answers with a Status, and a finalizer-held one is stamped a step earlier. Both -// are unresolved here, by design; the finalizer-held one was already attributed when its -// deletionTimestamp arrived. -func TestFilteredRemoval_CapturedDeletionsAreUnresolved(t *testing.T) { +// resolve on the deletion's own fact, naming who asked for it; the finalizer controller's PATCH is +// not evidence. +func TestRemoval_CapturedDeletionsNameTheDeleter(t *testing.T) { for _, tc := range []struct { name string facts []AuthorFact + want string }{ - {"cm-gone", []AuthorFact{corpusAuditFact(t, "cm-gone", "delete", "bob@example.com")}}, + {"cm-gone", []AuthorFact{corpusAuditFact(t, "cm-gone", "delete", "bob@example.com")}, "bob@example.com"}, {"cm-held", []AuthorFact{ corpusAuditFact(t, "cm-held", "delete", "alice@example.com"), corpusAuditFact(t, "cm-held", "patch", "system:serviceaccount:lab-actors:membership-finalizer-controller"), - }}, + }, "alice@example.com"}, } { t.Run(tc.name, func(t *testing.T) { removals := corpusSelectedRemovals(t, tc.name) @@ -184,7 +177,7 @@ func TestFilteredRemoval_CapturedDeletionsAreUnresolved(t *testing.T) { h := newFactIndexHarness(t, FactIndexConfig{}) h.publish(configMapFactStream(route), tc.facts...) h.waitForFacts(len(tc.facts)) - h.absent(corpusRemovalQuery(route, removals[0])) + require.Equal(t, tc.want, h.resolve(corpusRemovalQuery(route, removals[0])).Fact.Author) }) } } diff --git a/internal/queue/fact_index_test.go b/internal/queue/fact_index_test.go index b79cf5d6..e1c5f6ba 100644 --- a/internal/queue/fact_index_test.go +++ b/internal/queue/fact_index_test.go @@ -152,10 +152,9 @@ func TestFactIndex_JoinPolicyDependsOnTheEventKind(t *testing.T) { // pointer may name a different, older author than the create or update this event represents. harness.absent(objectQuery("prod-eu-1", "uid-1", "999", true)) - // A removal's rv never matches the write's, so it is the event kind that consults latest. - removal := harness.resolve(objectQuery("prod-eu-1", "uid-1", "999", false)) - require.Equal(t, AttributionLatest, removal.Result) - require.Equal(t, "alice", removal.Fact.Author) + // A removal's rv never matches the write's, and the object's last write says who edited it, not + // who removed it: a removal with only that is unresolved. + harness.absent(objectQuery("prod-eu-1", "uid-1", "999", false)) } func TestFactIndex_RouteIsolatesOtherwiseIdenticalFacts(t *testing.T) { @@ -189,8 +188,13 @@ func TestFactIndex_LatestTierIsLastWriterWins(t *testing.T) { harness.publish(key, objectFact("alice", "101"), objectFact("bob", "102")) harness.waitForFacts(3) - removal := harness.resolve(objectQuery("prod-eu-1", "uid-1", "999", false)) - require.Equal(t, "bob", removal.Fact.Author, "the latest tier must name the last fact applied") + harness.index.mu.Lock() + latest, found := harness.index.scopes[factScope{ + route: "prod-eu-1", groupResource: groupResourceKey("apps", "deployments"), + }].lookupLatest(factIndexTestUID, time.Now().Add(-time.Hour)) + harness.index.mu.Unlock() + require.True(t, found) + require.Equal(t, "bob", latest.Author, "the latest tier must name the last fact applied") // The exact tier is immutable per (uid, rv), so the earlier write is still exactly joinable. require.Equal(t, "alice", harness.resolve(objectQuery("prod-eu-1", "uid-1", "101", true)).Fact.Author) @@ -462,11 +466,13 @@ func TestFactIndex_CollectionScopeMatchStopsAtTheWindow(t *testing.T) { func TestFactIndex_CollectionScopeIsWeakerThanAnObjectsOwnFact(t *testing.T) { harness := newFactIndexHarness(t, FactIndexConfig{}) key := factIndexTestStream("prod-eu-1") - harness.publish(key, aliceCollectionFact(""), objectFact("bob", "101")) - harness.waitForFacts(3) + bobDeletes := objectFact("bob", "101") + bobDeletes.Verb = "delete" + harness.publish(key, aliceCollectionFact(""), bobDeletes) + harness.waitForFacts(4) resolution := harness.resolve(objectQuery("prod-eu-1", "uid-1", "999", false)) - require.Equal(t, AttributionLatest, resolution.Result) + require.Equal(t, AttributionDeleteSticky, resolution.Result) require.Equal(t, "bob", resolution.Fact.Author) } @@ -569,9 +575,8 @@ func TestFactIndex_NameTierRanksBelowEveryStrongerTier(t *testing.T) { require.Equal(t, AttributionExact, exact.Result) require.Equal(t, "uid-tier", exact.Fact.Author) - // And the latest tier, which a removal consults, also outranks it. - removal := harness.resolve(namedQuery(factIndexTestUID, "999", "fl-1", false)) - require.Equal(t, "uid-tier", removal.Fact.Author) + // Neither write is evidence about a removal, whichever key it is filed under. + harness.absent(namedQuery(factIndexTestUID, "999", "fl-1", false)) } func TestFactIndex_NameTierResolvesAnAggregatedRemovalToItsDeleter(t *testing.T) { @@ -605,18 +610,17 @@ func TestFactIndex_ARemovalReachesItsDeleteFactKeyedByName(t *testing.T) { require.Equal(t, "the-deleter", removal.Fact.Author) } -func TestFactIndex_ANameKeyedWriteDoesNotOutrankTheObjectsOwnUIDFact(t *testing.T) { +func TestFactIndex_ARemovalTakesNoWriteFactUnderAnyKey(t *testing.T) { harness := newFactIndexHarness(t, FactIndexConfig{}) - // Only a fact about the DELETION may jump ahead of the uid tier. A name-keyed WRITE is not that, - // and must not displace the object's own uid-keyed evidence. + // Only a fact about the DELETION, or the write at the removal's own version, names a removal. A + // write under the uid or the name is neither. harness.publish(factIndexTestStream("prod-eu-1"), objectFact("uid-tier", "101"), aggregatedFact("name-tier-write", "cm-parent", "update"), ) harness.waitForFacts(3) - removal := harness.resolve(namedQuery(factIndexTestUID, "999", "cm-parent", false)) - require.Equal(t, "uid-tier", removal.Fact.Author) + harness.absent(namedQuery(factIndexTestUID, "999", "cm-parent", false)) } func TestFactIndex_ARemovalStillPrefersItsOwnUIDKeyedDeleteFact(t *testing.T) { @@ -829,10 +833,10 @@ func TestFactIndex_ARemovalWaitsForEvidenceAboutTheDeletion(t *testing.T) { "a removal must wait for evidence about the deletion, not settle for the last edit") } -// Waiting must never cost an attribution. When nothing better arrives, the write fact that was held -// back is returned exactly as it would have been returned immediately — the only difference is the -// wait, and a removal that spends its grace is the case the grace window is for. -func TestFactIndex_ARemovalStillNamesTheLastWriterWhenNothingBetterArrives(t *testing.T) { +// The object's last writer is not who removed it. A removal whose only fact is a write waits out its +// grace for evidence about the deletion and, when none arrives, is unresolved: a wrong author is +// worse than none. +func TestFactIndex_ARemovalWithOnlyAWriteFactIsUnresolved(t *testing.T) { harness := newFactIndexHarness(t, FactIndexConfig{}) harness.publish(factIndexTestStream("prod-eu-1"), objectFact("bob", "101")) harness.waitForFacts(2) @@ -841,10 +845,9 @@ func TestFactIndex_ARemovalStillNamesTheLastWriterWhenNothingBetterArrives(t *te resolution := harness.index.Await(t.Context(), removalFactQuery(factIndexTestUID), 150*time.Millisecond) - require.Equal(t, AttributionLatest, resolution.Result) - require.Equal(t, "bob", resolution.Fact.Author, "waiting must not turn a match into an absence") + require.Equal(t, AttributionAbsent, resolution.Result) require.GreaterOrEqual(t, time.Since(start), 150*time.Millisecond, - "the fallback is only taken once the grace has actually elapsed") + "it still waits the grace, since a delete fact may be on its way") } // An object's OWN delete fact is the strongest evidence a removal can have, so it ends the wait @@ -930,8 +933,7 @@ func TestFactIndex_ARecreatedNameDoesNotInheritThePreviousDeleter(t *testing.T) resolution := harness.index.Lookup(removal) require.NotEqual(t, "alice", resolution.Fact.Author, "the previous object's deleter must not be inherited through a reused name") - require.Equal(t, "bob", resolution.Fact.Author) - require.Equal(t, AttributionLatest, resolution.Result, "which leaves the ordinary fallback") + require.Equal(t, AttributionAbsent, resolution.Result, "and bob's write is not evidence of the removal") // The first object's pointer is untouched by any of that: it is a statement about a uid. previous := harness.index.Lookup(removalFactQuery("uid-old")) @@ -951,21 +953,19 @@ func removalFactQuery(uid string) FactQuery { } } -// TestFactIndex_LatestAndResourceVersionAreDistinctTiers pins the split the metric surface turns -// on. Both used to report "weak", and they are different evidence: the uid tier is the OBJECT's own -// last write, while the rv-only hatch is a fact that carried a resourceVersion and no uid at all. -// The removal path turns on the first specifically, so a reader that cannot tell them apart cannot -// tell a held fallback from an unidentified match. -func TestFactIndex_LatestAndResourceVersionAreDistinctTiers(t *testing.T) { +// TestFactIndex_ExactAndResourceVersionAreDistinctTiers pins the split the metric surface turns on: +// a removal's own version can be matched by a fact that carries the object's uid, or by one that +// carried a resourceVersion and no uid at all. They are different evidence, so they report apart. +func TestFactIndex_ExactAndResourceVersionAreDistinctTiers(t *testing.T) { harness := newFactIndexHarness(t, FactIndexConfig{}) key := factIndexTestStream("prod-eu-1") rvOnly := AuthorFact{Namespace: "team-a", ResourceVersion: "202", Author: "rv-actor", Verb: "update"} harness.publish(key, objectFact("alice", "101"), rvOnly) harness.waitForFacts(3) - // A removal joins the object's own uid: the latest tier. - removal := harness.resolve(objectQuery("prod-eu-1", factIndexTestUID, "999", false)) - require.Equal(t, AttributionLatest, removal.Result) + // A removal at the version alice's write produced joins her uid-bearing fact: the exact tier. + removal := harness.resolve(objectQuery("prod-eu-1", factIndexTestUID, "101", false)) + require.Equal(t, AttributionExact, removal.Result) require.Equal(t, "alice", removal.Fact.Author) // A fact with no uid is reachable only through the resourceVersion it carried. diff --git a/internal/watch/author_resolver.go b/internal/watch/author_resolver.go index 5de0b59a..08897447 100644 --- a/internal/watch/author_resolver.go +++ b/internal/watch/author_resolver.go @@ -65,10 +65,9 @@ type AuthorQuery struct { // ExactCapable is true for ADDED and MODIFIED, whose resourceVersion is the one the write // produced. A removal's is not, so it consults the weaker tiers the exact-capable events skip. ExactCapable bool - // FilteredRemoval and Terminating select the exact-evidence policy for a DELETED frame from a - // selected collection; see queue.FactQuery.FilteredRemoval. - FilteredRemoval bool - Terminating bool + // Terminating reports that a removed object carried a deletionTimestamp; see + // queue.FactQuery.Terminating. + Terminating bool } // factQuery renders the query the way the index is keyed. @@ -82,7 +81,6 @@ func (q AuthorQuery) factQuery() queue.FactQuery { Labels: q.Labels, Name: q.Name, ExactCapable: q.ExactCapable, - FilteredRemoval: q.FilteredRemoval, Terminating: q.Terminating, } } @@ -203,7 +201,7 @@ func (r *attributionResolver) userInfoForResolution( // attributionEventKindWrite and attributionEventKindRemoval are the bounded event kinds on the wait // histogram. They come from ExactCapable, which is the same split the wait design turns on: a -// removal holds a fallback and keeps waiting for evidence about the deletion, a write does not. An +// removal waits for evidence about what caused it, a write for the fact at its own version. An // absent write and an absent removal used to be one series, and the removal wait is the number // anyone tuning --author-attribution-grace actually needs. const ( diff --git a/internal/watch/filtered_removal_test.go b/internal/watch/removal_query_test.go similarity index 59% rename from internal/watch/filtered_removal_test.go rename to internal/watch/removal_query_test.go index ab4b37ef..36059f53 100644 --- a/internal/watch/filtered_removal_test.go +++ b/internal/watch/removal_query_test.go @@ -18,29 +18,25 @@ import ( "github.com/ConfigButler/gitops-reverser/internal/types" ) -// Only a DELETED frame from a selected collection can be a label exit, so only it takes the -// exact-evidence policy. An unselected removal and a MODIFIED carrying a deletionTimestamp keep the -// existing deletion attribution. -func TestRouteLiveTargetWatchEvent_OnlySelectedDeletionsUseTheFilteredRemovalPolicy(t *testing.T) { +// The author query for a removal is the same whether or not its stream carries a label selector. +// A DELETED frame from a selected stream may be a label exit, and the index tells that apart from a +// deletion by the shape of the facts alone. Only the object decides Terminating. +func TestRouteLiveTargetWatchEvent_RemovalQueryIgnoresTheSelector(t *testing.T) { terminating := configMapObject("12") terminating.SetDeletionTimestamp(&metav1.Time{Time: time.Unix(0, 0).UTC()}) cases := []struct { name string - selector string event watch.Event - wantFiltered bool wantTerminating bool }{ - {"selected DELETED", teamA, watch.Event{Type: watch.Deleted, Object: configMapObject("12")}, true, false}, - {"selected DELETED of a terminating object", teamA, - watch.Event{Type: watch.Deleted, Object: terminating}, true, true}, - {"unselected DELETED", "", watch.Event{Type: watch.Deleted, Object: configMapObject("12")}, false, false}, - {"selected MODIFIED with deletionTimestamp", teamA, - watch.Event{Type: watch.Modified, Object: terminating}, false, true}, + {"DELETED", watch.Event{Type: watch.Deleted, Object: configMapObject("12")}, false}, + {"DELETED of a terminating object", watch.Event{Type: watch.Deleted, Object: terminating}, true}, + {"MODIFIED with deletionTimestamp", watch.Event{Type: watch.Modified, Object: terminating}, true}, } for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { + var queries []queue.FactQuery + for _, selector := range []string{teamA, ""} { gitDest := types.NewResourceReference("target", "default") enqueuer := &recordingEnqueuer{} stream := reconcile.NewGitTargetEventStream(gitDest.Name, gitDest.Namespace, enqueuer, logr.Discard()) @@ -54,12 +50,14 @@ func TestRouteLiveTargetWatchEvent_OnlySelectedDeletionsUseTheFilteredRemovalPol } _, err := manager.routeLiveTargetWatchEvent(context.Background(), logr.Discard(), gitDest, - testStream(targetWatchKey{GVR: configmapsGVR, Namespace: "apps", LabelSelector: tc.selector}), + testStream(targetWatchKey{GVR: configmapsGVR, Namespace: "apps", LabelSelector: selector}), tc.event) - require.NoError(t, err) - assert.Equal(t, tc.wantFiltered, lookup.lastQuery.FilteredRemoval) - assert.Equal(t, tc.wantTerminating, lookup.lastQuery.Terminating) - }) + require.NoError(t, err, tc.name) + assert.False(t, lookup.lastQuery.ExactCapable, tc.name) + assert.Equal(t, tc.wantTerminating, lookup.lastQuery.Terminating, tc.name) + queries = append(queries, lookup.lastQuery) + } + assert.Equal(t, queries[0], queries[1], "%s: a selector must not change the query", tc.name) } } diff --git a/internal/watch/target_watch.go b/internal/watch/target_watch.go index 859fcfca..5795ede6 100644 --- a/internal/watch/target_watch.go +++ b/internal/watch/target_watch.go @@ -1206,9 +1206,7 @@ func (m *Manager) routeLiveTargetWatchEvent( "resource", event.Identifier.String()) return rv, nil } - // A DELETED frame on a selected collection may be the object leaving the selection rather - // than being deleted, so it is attributed under the filtered-removal policy. - m.attachAuthor(ctx, &event, stream.key.GVR, u, ev.Type == watch.Deleted && stream.key.LabelSelector != "") + m.attachAuthor(ctx, &event, stream.key.GVR, u) // The gate re-checks after attribution, which waits on the audit grace: a stream retired or // cancelled while an event was waiting for its author must not enqueue on the way out. var routeErr error @@ -1258,15 +1256,14 @@ func (m *Manager) routeLiveTargetWatchEvent( // the strongest available key. Configured-author mode (nil resolver) leaves UserInfo // zero, so the writer authors that commit as the configured committer. // -// filteredRemoval marks a DELETED frame from a selected collection. It may be the object leaving -// the selection, which no deletion fallback can attribute, so it resolves on exact evidence only -// (see queue.FactQuery.FilteredRemoval). +// Nothing here asks whether the stream carries a label selector. A DELETED frame from a selected +// collection may be the object leaving the selection, and the index tells that apart from a +// deletion by the shape of the facts alone (see queue.FactIndex.lookupRemoval). func (m *Manager) attachAuthor( ctx context.Context, event *git.Event, gvr schema.GroupVersionResource, u *unstructured.Unstructured, - filteredRemoval bool, ) { // A nil resolver is configured-author mode: nothing is attempted, and the event's zero // Attribution is already AttributionNotAttempted — the constant is the empty string so that @@ -1302,7 +1299,6 @@ func (m *Manager) attachAuthor( Labels: u.GetLabels(), Name: u.GetName(), ExactCapable: exactCapable, - FilteredRemoval: filteredRemoval, Terminating: u.GetDeletionTimestamp() != nil, }) // Stamp the outcome even when no actor was named: an unresolved attribution is a fact the diff --git a/test/e2e/commit_request_e2e_test.go b/test/e2e/commit_request_e2e_test.go index 934397f7..34b34a2e 100644 --- a/test/e2e/commit_request_e2e_test.go +++ b/test/e2e/commit_request_e2e_test.go @@ -493,11 +493,18 @@ spec: } // applyCommitRequestWithGenerateName creates a CommitRequest using -// metadata.generateName and returns the server-allocated name. It sets a generous attachTimeout -// because the spec creates the Deployment and this CommitRequest back-to-back: authorship is -// settled synchronously at admission (no controller-side wait), so a zero wait would race the -// Deployment's watch event and could resolve NoWindow before the window opens. Waiting to attach -// is the documented mechanism for a CommitRequest issued concurrently with its work (UC2). +// metadata.generateName and returns the server-allocated name. The spec creates its work and this +// CommitRequest back-to-back: authorship is settled synchronously at admission (no controller-side +// wait), so a zero wait would race the work's watch event and could resolve NoWindow before the +// window opens. Waiting to attach is the documented mechanism for a CommitRequest issued +// concurrently with its work (UC2). +// +// The wait is deliberately tight: it covers audit delivery (--audit-webhook-batch-max-wait=1s) and +// the join, within the 4s to 5s the configuration reference recommends for a loaded cluster, and +// stays well under --author-attribution-grace (10s here, config/deployment.yaml). A watch stream +// attributes its events in order, so a work event held behind another event that waits out the whole +// grace fails the spec with NoWindow instead of passing slowly. Do not widen it to make such a +// failure go away: that wait is the bug. func applyCommitRequestWithGenerateName(namespace, prefix, gitTargetName, message string) string { GinkgoHelper() manifest := fmt.Sprintf(`apiVersion: configbutler.ai/v1alpha3 @@ -510,8 +517,8 @@ spec: name: %s message: %q window: - attachTimeout: "8s" - maxDuration: "8s" + attachTimeout: "4s" + maxDuration: "4s" `, prefix, namespace, gitTargetName, message) out, err := kubectlRunWithStdin(namespace, manifest, "create", "-f", "-", "-o", "jsonpath={.metadata.name}")