Skip to content

fix(timeline): scope event-pipeline drop metrics to the active cluster - #1301

Open
hisco wants to merge 3 commits into
mainfrom
radar-drop-provenance
Open

fix(timeline): scope event-pipeline drop metrics to the active cluster#1301
hisco wants to merge 3 commits into
mainfrom
radar-drop-provenance

Conversation

@hisco

@hisco hisco commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Problem

Event-pipeline drop records (RecentDrops) name individual resources (kind / namespace / name). They were process-global, carried no cluster provenance, and were not reset on a kubeconfig context switch. After switching clusters, the drop surfaces — /api/debug/events, /api/debug/events/diagnose (drop_history + its recommendations), and /api/diagnostics — exposed a previously-connected cluster's resource names to whoever can read the same kind on the new cluster.

Found by cross-review of the timeline per-kind RBAC work; this is its dedicated fix.

Fix

  • Stamp provenance at wiring time. DropRecord gains a ClusterContext, captured from the informer's recordClusterContext (the same wiring-time capture the timeline events use), passed through all RecordDrop call sites. Informer shutdown on a switch is asynchronous (up to ~5s, then abandoned), so a straggler callback can record a drop after the switch — stamping the wiring-time context keeps it attributed to the cluster it came from instead of being read live (which would mis-attribute it to the new cluster).
  • Filter on read. timeline.DropsForCluster(drops, active) keeps only the active cluster's drops, applied in handleDebugEvents and /api/diagnostics. GetDiagnosis scopes both its DropHistory and the recommendations derived from it.
  • Reset the un-stamped counters. ResetMetricsForContextSwitch clears the per-cluster pipeline counters on switch (they carry no resource identity, so they only need a reset, not stamping). Process uptime is preserved — it measures the pipeline's lifetime, not a per-cluster session.

Stamp+filter (not reset alone) is what closes the late-callback race — reset-at-switch-time can't stop a straggler from repopulating RecentDrops afterward, but a straggler stamped with the old cluster is filtered out on read.

Tests

  • Unit (internal/timeline/metrics_test.go): reset clears drops+counters while preserving uptime; a straggler drop stamped with the old cluster is filtered out of the active-cluster view.
  • e2e: two kind clusters — generate noisy-secret drops on cluster A, POST /api/contexts/<B>, assert A's resource names are gone from RecentDrops on B. Passes.
  • go build, go vet, go test ./... green.

Scope

Deliberately limited to drop-metrics cross-cluster provenance. GetDiagnosis's timeline-store query (its TimelineEvents) cluster-scoping is a separate change and is not touched here.

https://claude.ai/code/session_01XNhMe6Zdqwj5EoBazNDnmW


Note

Medium Risk
Touches diagnostic/debug surfaces and context-switch lifecycle; mitigates information disclosure rather than changing core auth, with documented best-effort behavior on unstamped counters after async informer drain.

Overview
Fixes a cross-cluster leak where global RecentDrops (kind/namespace/name) could show a previous kubeconfig context’s resources on debug/diagnostics APIs after a switch.

DropRecord is stamped with ClusterContext at informer wiring time (same as timeline events), and all RecordDrop call sites pass that context so late straggler callbacks stay attributed to the old cluster.

On read, DropsForCluster limits drops to ActiveClusterContext() in /api/debug/events and diagnostics; GetDiagnosis filters drop history (and thus recommendations) by active cluster. On switch, timeline reset now also runs ResetMetricsForContextSwitch to clear drops and per-cluster counters while keeping process uptime.

Unit tests cover reset behavior and filtering stale stamped drops after a simulated straggler.

Reviewed by Cursor Bugbot for commit 670a750. Bugbot is set up for automated code reviews on this repo. Configure here.

@nadaverell

Copy link
Copy Markdown
Contributor

Adversarial review (self + codex cross-model) of the full branch delta. Green light — contingent on landing together with #1300, after it.

The contingency

Standalone, this PR does not close the diagnose-endpoint leak: GetDiagnosis receives clusterContext but deliberately does not scope its timeline-store query (the PR description calls this out as separate work). Codex independently confirmed the consequence: the SQLite store persists across context switches, so /api/debug/events/diagnose can still return a previously-connected cluster's event rows (names, diffs, owners) for a matching kind/ns/name. That fix lives in #1300 (store.Query(..., ClusterContext: ...)). Merge #1300 first; this PR then completes the drop-metrics half.

Rebase note (after #1300 lands): handleDiagnostics and handleDebugEvents conflict on the RecentDrops line — the resolution must compose both filters, cluster provenance and per-user RBAC: s.filterDropsByRBAC(r, timeline.DropsForCluster(...)). Picking either one silently drops the other protection. Same for GetDiagnosis: keep #1300's store-query scoping + allow callback and this PR's stamped drop.ClusterContext filter (which is the more precise mechanism for DropHistory).

Vetted green (checked, no action needed)

  • Call-site coverage is compiler-enforced: RecordDrop's signature change means a missed stamp site fails the build — verified all sites in internal/k8s/cache.go, dynamic_cache.go, and manager.go (broadcastEvent uses event.ClusterContext).
  • Stamp-at-wiring is real: recordClusterContext is captured at informer wiring time in both caches, not read live — so a straggler callback firing after the switch stays attributed to its origin cluster.
  • Stamp+filter closes the race reset alone can't: TestDropsForCluster_FiltersStaleClusterDrops pins the straggler-after-reset case; strict equality also covers in-cluster (both "").
  • Uptime preservation: ResetMetricsForContextSwitch keeps startTime; tested.
  • Read-path safety: GetSnapshot deep-copies, so filtering never mutates shared state.

Triaged, not blocking

  • Straggler informers can contaminate the reset counters (codex, medium): true — a draining informer can IncrementReceived/IncrementRecorded after the reset. Exposure is kind names only (no resource identity), a ~5s shutdown window, on debug surfaces; the PR explicitly scopes counters to reset-only for this reason. The generation-token fix codex proposes is disproportionate to the exposure. Fine as a documented residue.

Drop records (RecentDrops) name individual resources (kind/namespace/name) and
were process-global with no cluster provenance and no reset on a kubeconfig
context switch. After switching clusters, the debug/diagnostics drop surfaces
(/api/debug/events, /api/debug/events/diagnose, /api/diagnostics) exposed a
previously-connected cluster's resource names to whoever can read the same kind
on the new cluster.

Stamp each DropRecord with the cluster context captured at informer wiring time
(matching how timeline events are attributed), so a straggler callback firing
during the asynchronous informer-shutdown window after a switch stays truthfully
attributed to the cluster it came from rather than being read live. Read paths
filter drops to the active cluster via timeline.DropsForCluster; GetDiagnosis
scopes both its drop history and the recommendations derived from it. The
per-cluster counters (which carry no resource identity) are additionally cleared
on context switch via ResetMetricsForContextSwitch; process uptime is preserved.

Claude-Session: https://claude.ai/code/session_01XNhMe6Zdqwj5EoBazNDnmW
hisco added 2 commits August 9, 2026 23:31
In-cluster mode stamps and filters with ActiveClusterContext(), which
returns the "in-cluster" sentinel, not an empty string. Comment said
"both \"\"".

Claude-Session: https://claude.ai/code/session_0164KXsqD8R5EyEUDfNkLsNb
Counters, unlike RecentDrops, have no stamp+filter guard, so a straggler
informer draining after a context switch can re-increment them with a few
old-cluster ticks. Accepted (kind names only, no resource identity); make
the residue explicit where the reset happens.

Claude-Session: https://claude.ai/code/session_0164KXsqD8R5EyEUDfNkLsNb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants