Repository navigation
Attribute pods to workloads by ownership, never by name prefix - #1694
Merged
Merged
Conversation
PR Summary by QodoAttribute workload pods by controller ownership
AI Description
Diagram
High-Level Assessment
Files changed (29)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
nadaverell
added a commit
that referenced
this pull request
Sep 9, 2026
## Summary With auth enabled, every metrics surface ran with Radar's own Prometheus access rather than the caller's. A user limited to one namespace could read another namespace's charts, and with raw PromQL could read anything the Prometheus backend holds — including `kube-system` and node-level series. This gates each surface on the Kubernetes permission that corresponds to what it exposes. Local no-auth mode is unchanged. The response-bounding and availability helpers this builds on are already on `main` (#1678). ## What changed Two tiers. A curated surface is gated on the same read grant as the object it charts. A surface whose scope cannot be determined by inspection — raw PromQL, metric discovery, alert rules, the cluster aggregate — is gated on being able to list pods in every namespace, because a PromQL expression cannot be namespace-filtered by reading it. | Surface | Gate | |---|---| | `GET /prometheus/resources/{kind}/{ns}/{name}` | `get` on that resource (group/resource derived from the kind) | | `GET /prometheus/resources/Node/{name}`, and Node on the namespaced route | `get` on the Node, always cluster-scoped | | `GET /prometheus/namespace/{ns}` | `list pods` in that namespace | | `GET /prometheus/cluster`, `GET /prometheus/query`, MCP `query_prometheus` / `discover_metrics` / `get_prometheus_rules` | any-namespace `list pods`; 403 or tool error `cluster-wide metrics access requires permission to list pods across all namespaces` | | `GET /prometheus/hpa/{ns}/{name}` (new) | `get` on the HPA; returns the current and desired replica series | - `internal/prometheus/auth.go` — `canReadClusterWideMetrics` and `canReadMetricsResource`. An unmapped kind fails closed, and a cluster-scoped kind forces `namespace=""` so a namespaced RoleBinding cannot pass a Node read. - `internal/prometheus/handlers.go` — the gate on every route; `handleRawQuery` now applies `MaxResponseBytes` / `SummarizeLargeResult` and returns `truncated` + `summary` the way the MCP tool already did, so an oversized result is never served as a partial one; `handleHPAMetrics` shares the range handling of resource metrics. - `internal/server/server.go` — the inline gate closure becomes `Server.prometheusAuthGate`, which makes the two checks testable in isolation. - `pkg/prom/queries.go` — `BuildHPAReplicasQueries` builds the two kube-state-metrics series server-side, with label sanitizing, instead of the client assembling PromQL. - `web/src/api/client.ts`, `HPACharts.tsx` — the HPA chart reads the curated endpoint instead of raw PromQL. A 403 renders an explicit access banner and stops polling rather than showing an empty chart, which would have read as "this workload never scaled". - `docs/mcp.md` and `CLAUDE.md` describe the two tiers. **Worth a reviewer's attention:** the gate authorizes the *requested* kind, not the object the query resolves to. See the first tradeoff below. ## Testing - `go test ./...` (both modules), `make tsc`, `make build` — pass. - `packages/k8s-ui` — 174 files, 3,582 passed. `web` — 85 files, 912 passed. - `internal/prometheus/handlers_auth_test.go` — every row of the matrix: no-gate passthrough, an authorized user (asserting the exact tuple the gate is asked for), and a namespace-scoped user (403 with the right message, Prometheus never queried). Plus the HPA endpoint's range/step handling and raw-query truncation. - `internal/mcp/tools_prometheus_auth_test.go` — the three unbounded tools for no user, a namespace-scoped user (denied, no Prometheus probe), a cluster-wide user (allowed, one memoized SAR), and a SAR error (fails closed, not cached). - `internal/server/prometheus_auth_test.go` — `prometheusAuthGate` against a fake apiserver: no-auth passthrough, a namespace-scoped user failing the all-namespaces review and cross-namespace/Node reads, an SAR-allowed namespace outside the allow-list refused, and an SAR error failing closed without caching. Visual test: skipped. The 403 banner needs auth-enabled mode with a namespace-restricted user, which no available cluster is configured for; the banner is covered by `HPACharts.render.test.tsx` instead. `web/src/api/client.authRedirect.test.ts` is flaky in this repo (a timing-sensitive 401 single-flight test, unrelated to this change). It passed in the run above and has failed in others; a rerun clears it. ## Tradeoffs - **The gate authorizes the requested kind, but the query does not resolve one.** `buildWorkloadQuery` takes only namespace and name and matches `pod=~"<name>-.*"`, discarding the kind — so a user who can `get` Deployments but not StatefulSets in a namespace can request `Deployment/ns/db` and receive StatefulSet `db`'s pods. This is a real gap in the curated tier while it stands. It is closed by #1694 later in this series, which resolves a workload's pods by controller ownership and returns nothing for a kind/name pair that owns none; the gate itself does not need to change, because the query can then only return pods the requested object actually owns. - **Object-scoped RBAC (`resourceNames`) is not consulted.** The gate carries group, resource, namespace and verb, the same shape every other read gate in Radar uses. It fails closed, so such a user sees less rather than more. - **Radar queries Prometheus with its own credentials**; only the authorization is per user. Where several tenants share one Prometheus, that backend needs its own isolation. - **Hosted consumers need an in-cluster Radar at or above this version for the HPA chart.** `@skyhook-io/radar-app` renders `HPACharts` inside `RadarApp`, and against an older agent the new `/prometheus/hpa` route returns 404, so the chart hides itself. There is deliberately no fallback to the raw endpoint, which an older agent would serve ungated. Release note, not a code path. - Per-resource denials return the same `forbidden` body that PVC and rightsizing already use; only the cluster-wide tier carries the message naming the grant it needs. - `usePromQLRange` was removed rather than deprecated: it was not part of the library's public surface in `web/src/index.ts` and had one caller. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes authorization for all Prometheus and related MCP entry points; mis-gating could block legitimate operators or leave a gap, though behavior is fail-closed with extensive test coverage. > > **Overview** > With auth enabled, Prometheus REST routes and MCP tools now enforce **two authorization tiers** instead of running every query with Radar’s backend credentials. > > **Curated surfaces** (per-resource charts, namespace aggregates, new `GET /prometheus/hpa/{ns}/{name}`) require the same Kubernetes read as the object they expose—e.g. `get` on the workload/HPA/Node, or `list pods` in that namespace. **Unbounded surfaces**—`GET /prometheus/cluster`, `GET /prometheus/query`, and MCP `query_prometheus` / `discover_metrics` / `get_prometheus_rules`—require an any-namespace **`list pods`** SubjectAccessReview and return a message naming that grant on denial. Local no-auth mode still passes through. > > `Server.prometheusAuthGate` centralizes SAR plus namespace allow-list checks (same semantics as the main resource API). Raw PromQL responses now use **`MaxResponseBytes` / `SummarizeLargeResult`** (`truncated` + `summary`) like the MCP tool. The HPA detail chart calls the curated HPA endpoint instead of client-built PromQL, shows an explicit **access banner** on 403, and stops polling denials. > > Docs (`CLAUDE.md`, `docs/mcp.md`) and broad handler/MCP/server tests document and lock in the matrix. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit f15397f. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
Contributor
Author
|
Note for the rebase onto main (once #1679 lands, not a change request). #1679 gained four things that live in the same regions this PR rewrites, so the rebase will conflict in
Resolving the first one toward this branch drops the switch. CI catches all four — |
nadaverell
added a commit
that referenced
this pull request
Sep 9, 2026
## Summary
`diagnose` now returns a `metrics` field alongside the rest of the
bundle: CPU, memory and restarts for Pod, Deployment, StatefulSet,
DaemonSet and Argo Rollout targets, captured inside the same call over
the pods the workload's selector matches.
The reason it is captured rather than left to the agent is that an agent
which has to ask for resource data can silently never ask. A healthy
verdict reached without looking at memory is indistinguishable,
afterwards, from one where memory was fine — and the Findings pane
cannot chart what the bundle does not carry, because the evidence ledger
is built from the tool results the agent received. Capturing makes "did
anyone check?" answerable, and gives the chart, the agent and the ledger
the same numbers over the same window and the same pods.
## What changed
**The vitals branch** (`internal/mcp/tools_diagnose_metrics.go`). Runs
concurrently with the rest of diagnose — launched before the bundle is
assembled, joined immediately before the response is returned — and the
three categories fan out within it, so all of this is one Prometheus
round trip wide. Gated in order: a per-kind `get` SubjectAccessReview on
the diagnosed resource (diagnose otherwise only checks namespace access
before reading the shared cache), then availability. A denial omits the
field without contacting Prometheus; an absent Prometheus omits it; a
configured but unreachable one sets `error`.
**Shape.** `metrics: {window: {start, end, step}, pods, partial?,
omittedPods?, series: [{category, unit, query, observedSamples,
series}], error?}`. Each entry carries the PromQL that produced it, so
an empty `series` with no `error` reads as "the window holds no
samples", not "nothing was asked".
**Bounds.** Pod set cut to the first 50 by name (`partial` +
`omittedPods` when cut). Step chosen for at most 60 points per series
inside an 8 KiB sample budget, with a 24 KiB backstop over the whole
field including query text. Values rounded to four significant digits. A
3s budget covers the branch including the availability probe. On a time
or size breach, series are dropped and `error` names the limit; a single
failing category is left out and named while the others stand.
**Flat stretches are compressed.** The interior of an evenly spaced run
of equal values is dropped, keeping the points that open and close it. A
workload sitting still costs two points per category instead of sixty.
Each category reports `observedSamples`, the count Prometheus actually
returned before compression — without it a two-point series is ambiguous
between an hour of agreeing samples and two lonely ones, and
`window`/`step` cannot settle it, because a range query returns nothing
for an evaluation with no data. Values are compared after rounding, so
movement below the four significant digits the field already reports
counts as flat; a run must be evenly spaced to compress, so a gap in the
series survives.
**One switch.** `DiagnoseMetricsEnabled` in `internal/mcp` is the single
place the decision lives, so a later release can change the default by
flipping it: the field is omitted and the rest of the bundle and
`query_prometheus` are untouched.
**`pkg/prom`.** `BuildPodSetQuery` / `BuildPodSetQueryNoContainerFilter`
aggregate one category over an explicit pod list as `sum(...)` — one
series per category regardless of pod count, never one per pod. #1694
supersedes their call site with the ownership join; see below.
## What a reviewer should focus on
**Membership and the query form are both transitional here, and #1694
replaces both.** Two separate layers:
| | this PR | #1694 |
|---|---|---|
| which pods count as the workload's | the workload's **label selector**
| **controller ownership** |
| how the query says it | enumerate every pod name | `kube_pod_owner`
join on the workload |
On membership, this PR is an improvement over the `pod=~"<name>-.*"`
prefix the older chart queries use — a prefix makes a Deployment named
`api` absorb `api-worker`'s pods. It is not yet correct: a selector also
matches bare pods, a sibling controller's pods during a Rollout
`workloadRef` migration, and anything else sharing its labels. Ownership
is what kube-state-metrics itself records, so it is also what makes
history and the present agree on membership.
On the query form, enumeration is exact but does not scale: 525 B of
query text at one pod, 2.2 KB at ten, 9.9 KB at fifty across the three
categories — and `pkg/prom` issues queries by GET, so a wide workload
puts ~3.3 KB into a URL per category, which some ingresses and auth
proxies will refuse. The 50-pod cap is what bounds that. The ownership
join names the workload once and is constant-length at any pod count.
No release happens between this PR and #1694, so neither the selector
membership nor the enumerated query reaches a user. Reviewing this on
the understanding that both are stepping stones is the right frame.
**Restarts use `sum(round(increase(...[1h])))`, not the `changes()` the
older per-pod and workload queries use.** `changes()` counts sampled
value transitions, so restarts landing between two scrapes — what
crash-loop backoff produces in its first minute at 10s, 20s, 40s —
collapse into one for the whole hour. `increase()` reads the counter
delta and handles resets; its extrapolation yields fractions, so
`round()` settles each pod's value before the sum. The older builders
keep `changes()` because `RestartEventLane` (`RestartChart.tsx`) emits a
marker on every positive delta between consecutive rolling counts and
depends on integer semantics; changing them needs verification against a
crash-looping cluster and is left for a follow-up. Both call sites now
carry a comment saying which is which.
## Testing
`go test ./...` (both modules), `make tsc`, `make build`, `git diff
--check` — all pass.
New tests cover the three series over the exact pod set within the byte
budget against a step-faithful fake; `since` and `kind=pod`; the 55-pod
set capped to 50 with lowered resolution; Prometheus absent,
unreachable, and read-gate denied; the time and size budget breaches; a
single failing category and a failed container-filter fallback; rounding
and error bounding; the compression rule (full collapse, step
boundaries, gap boundaries preserved, varying series untouched, NaN and
±Inf never collapsed, 0/1/2-point inputs); the end-to-end byte
reduction; and the switch, including that turning it off leaves the rest
of the bundle intact.
The fake Prometheus gains a `rangeDelay` so the time budget can be
breached deterministically (slept outside the lock so concurrent callers
are not serialized) and a `rangeBodyFunc` that renders the range body
from the request parameters, which is what makes a step-faithful
response possible. Fixtures that measure point counts or serialized size
now vary their values above the reported precision — a flat fixture
compresses to two points and would stop testing what it was for.
**Verified against a live GKE cluster with a real Prometheus**, driving
the MCP `diagnose` tool directly:
- A crash-looping Deployment (874 restarts) and a three-pod StatefulSet
returned all three categories with correct units, whole-number restarts,
and the exact pod set in the query.
- Compression on a Deployment blocked by a missing Secret: the field
went from 7.7 KB (50.4% of the bundle) to 1.2 KB (13.6%), all three
categories collapsing to two points with `observedSamples: 56`. The
crash-looping workload barely moved — its CPU and restarts genuinely
vary — which is the intended shape.
- `observedSamples` earns its place there: the crash-looping workload's
memory series has six samples, not fifty-six, because the pod keeps
dying. Three retained points with `observedSamples: 6` is a sparse
series; three with `observedSamples: 56` is a flat one.
- Latency: diagnose without the metrics branch returns in ~130 ms, with
it in ~650–840 ms, and a single Prometheus range query takes ~620 ms.
The branch is concurrent, but it is the critical path — nothing else
diagnose does takes long enough to hide it. `docs/mcp.md` states this so
the default can be revisited against numbers.
Visual test: skipped, Go only. The frontend adapter that turns this into
a chart card follows on Track B.
The caps and failure paths — the 50-pod cut, the byte budgets, the 3s
breach, an unreachable Prometheus — are covered against the fake rather
than live, since reaching them means degrading a real cluster.
## Tradeoffs
- The 8 KiB budget covers samples and envelope only; the 24 KiB backstop
is what bounds query text. Values are rounded to four significant digits
so three series fit.
- Metrics from a partial pod set are marked `partial` with the omitted
count, so the agent does not read them as the whole workload.
- Collecting on every diagnose costs one Prometheus round trip on the
critical path. That is the price of the guarantee above; the switch is
there for when it stops being worth paying.
This was referenced Sep 9, 2026
Four surfaces answered "which pods are this workload's" four different ways for Prometheus. The workload page's charts matched pod names against "<name>-.*", so a Deployment named api charted api-worker's pods too; the diagnose vitals and the workload log reads used the label selector, which also matches a bare debug pod and, during a Rollout workloadRef migration, the referenced Deployment's pods; rightsizing used controller ownership with a kube-state-metrics history join; and the Findings classifier accepted the same name prefix, so a sibling's chart was promoted as target evidence and drew the target's change markers. Membership is now established, never inferred. k8s.WorkloadPods walks the controller chain the topology and kube-state-metrics both use (pod → ReplicaSet → Deployment or Rollout, pod → Job → CronJob, direct for the rest) and reports denied listers instead of an empty set. Where a window matters, internal/prometheus.ResolvePodScope asks kube-state-metrics whether it attributed pods to the workload across it, so pods replaced mid-window still count, and falls back to the pods running now when there is no such history. Both ownership edges stay inside the query and are evaluated at every step, so a ReplicaSet adopted by another Deployment contributes only for the steps it actually belonged to this one. The response and the diagnose bundle say which source they got, and the chart carries one line about it, because "current pods only" and "every pod in the window" are different claims. A resolved scope is cached for two minutes against the cluster, the Prometheus connection and the window, and dropped outright on a context switch. pkg/prom.BuildScopedQuery renders either form for every category and keeps aggregation a separate axis: the workload page keeps one series per pod for its legends and per-pod overlays, the vitals keep one total. The ownership lookback covers what each category's function reads, so a pod replaced inside the window does not lose the tail of its own rate. The diagnose bundle now lists podNames, the prompt tells the agent to scope its own queries to them, and the Findings classifier proves a pod matcher against that set, gathered before anything is classified so the order the agent ran its tools in cannot change a verdict. An exact name or an anchored set of established pods is the target's; a prefix, an unescaped dot, or a workload_type naming another kind is not. That is what stops a sibling's chart from inheriting the target's change markers. Two selector shapes move with it: a namespace-only query and a container-only query are now broader rather than related, per the plan's scope rule. Radar degrades rather than fails where it used to do neither. A workload scaled to zero still charts from ownership history. An install that cannot list pods gets the rest of the diagnose bundle with a warning saying why the pod-level parts are empty, instead of a failed tool call. A pod scope that cannot be resolved, and a Prometheus discovery probe that times out, both report themselves instead of omitting the metrics field as if the workload had none. Rightsizing keeps its own seven-day statistics and probe, which are about retention rather than membership, and shares the ownership resolver. A Pod target is still its own exact scope and a Node still has none.
ReplicaSets sync in the deferred phase, and their lister is non-nil the whole time it is filling. A Deployment or Rollout resolved during that window found no ReplicaSet for any pod, so ownership matched nothing and the workload came back with zero pods and no error: diagnose bundles, workload logs and pod-scoped charts all reported a workload that has no pods, silently, until the cache finished. The pod set is now refused while the deferred informers are loading. That needs its own sentinel rather than the existing denial, because the two are not the same answer — reporting a warming cache as a denial sends an operator, or the agent, after an RBAC problem that does not exist. Its HTTP mapping stays 503, the diagnose bundle degrades with a warning naming the wait rather than permissions, and the MCP workload tools say to retry instead of "forbidden". The warming branch has no unit test: the cache harness constructs a fully synced cache, and expressing a half-loaded one needs a test seam in the shared package that is out of proportion to a four-line guard. The synced path stays covered by the existing ownership tests. Found by Cursor Bugbot on this PR.
The pod-coverage note rendered only when the chart had series, so the one case where the reader most needs it — an empty chart — said "No data for this time range" and nothing about which pods were read. That is the difference between "these pods reported nothing" and "we charted the wrong pods", and the reader could not tell which. describePodCoverage was exported and untested; its wording is now pinned, including the current-pods caveat that a pod replaced inside the window is missing, and the ownership-read failure that explains the fallback. Raised independently by Qodo and Cursor Bugbot on this PR.
…checked Three ways this branch could still hand back an answer it had not established. The warming guard asked whether every deferred informer had synced. That flag also stays false when an unrelated informer permanently fails, so one broken kind could have turned Deployment and Rollout pod resolution into a permanent 503. It now asks about the one informer it is about to walk, through a new per-kind accessor that separates "not synced yet" from "never watched" so an unwatched kind is not treated as a failure. The CronJob hop through Jobs gets the same guard: a critical informer promoted after a sync timeout warms exactly the same way. A metrics selector excluded the target only when workload_type was written with `=`. Prometheus anchors regex matchers, so `workload_type=~"statefulset"` excludes a Deployment just as plainly, and reading only `=` let a sibling workload's chart take the target's identity and its change markers. An exact set that includes the target kind is not a conflict, and an inexact regex still says nothing either way. A configured Prometheus that is not answering left the querier nil. If the workload also had no pods right now, ownership history — the source that would have had the answer for a workload scaled to zero or freshly replaced — was never consulted, and the metrics field vanished, which reads as "no metrics" rather than "Radar could not look". All three found by Codex in the merge sign-off review.
Two gaps in the fixes above, both the same shape as what they were fixing. The empty-scope branch treated only an absent querier as "Radar could not look". A connected Prometheus whose ownership probe fails also leaves the selection empty, and that path still returned nil — so a workload scaled to zero lost the metrics field entirely and read as having no metrics rather than a failed history read. Both causes now say which one it was. The warming warning named ReplicaSets. The guard now also covers the Jobs informer a CronJob is resolved through, so diagnosing a CronJob during that wait pointed the reader at the wrong resource — the exact mix-up the separate warming sentinel exists to prevent. It now names what it is actually waiting for. Both found by Cursor Bugbot.
…apped Two attribution details. The set that proves a Prometheus selector is about the target was built from any diagnose result that had not explicitly failed. Everything else in the projection takes confirmed success — status done and isError false — and a result that never said it succeeded should not put pods into the set that decides whether a chart is the target's. A capped pod list was labelled "partial pod set" in every mode. With ownership history the query still covers every pod the workload owned in the window; the cap applies to the name list the caller reports, which is what the resolver's own comment says. The card was contradicting the count beside it, so that mode now says the list was capped and the current-pods mode keeps the stronger wording it has earned. Both found by Qodo and Cursor Bugbot.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125QDX24TNuod7FKLwTDhvW
nadaverell
force-pushed
the
feat/pod-attribution
branch
from
September 10, 2026 21:40
4787611 to
adcafdc
Compare
An informer that is still filling its initial sync, or is scoped to other namespaces, returns an empty list rather than an error. Reading that as "this workload controls no pods" states a fact the cache never established, and KindCoversNamespace says so itself: a miss is not zero objects. Every kind reaches its pods through the pod lister, so it is gated alongside the ReplicaSet and Job hops rather than trusted because a hop was checked. The ownership vector's own claim is corrected too. Its two edges are read by separate last_over_time selectors, which take each series' most recent sample independently and so cannot establish that both edges held at one instant. A ReplicaSet transferred between Deployments is attributed to both for the length of the lookback; the comment previously claimed the opposite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125QDX24TNuod7FKLwTDhvW
Membership is now one thing: the pods the cluster says the workload controls, by controller ownership. A query names them exactly, and a workload whose pods cannot be established is an error rather than a chart of whatever the namespace happens to hold. The kube-state-metrics ownership-history layer is removed from this change. Reading a workload's pods from KSM over a window answers a question the current population cannot — a workload scaled to zero, or whose pods were replaced mid-window — but its two ownership edges are read by independent selectors that cannot establish the edges held at the same instant, so a transferred ReplicaSet is attributed to both owners for the length of the lookback. A coverage field that says which source answered cannot also say whether that source was complete, and neither question has a good answer while attribution can name a pod the workload never owned. It returns as its own change, on feat/pod-attribution-history. What that costs is stated where it shows: the chart caption says the line covers the pods running now and that replaced pods are not included, because the series still spans the whole window. Removing the history probe removes the only Prometheus call membership needed, so resolution is a local lister read: no probe, no cache keyed on the connection, no singleflight, and no resolved scope that can go stale between the pod list a bundle reports and the pods its charts query. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125QDX24TNuod7FKLwTDhvW
A metrics query naming a pod is the target's evidence when a producer established that pod as the workload's own. Two adapters ask that question, and both asked it against the projection as it was being built, so the answer depended on the order the agent called its tools in: a Prometheus query that ran before the workload-logs read naming the same pod was demoted, while the same two results in the other order were not. Both now run after every other tool in the transcript, against the membership the whole transcript established, with their source order captured before they are queued so deferring the classification does not reorder the evidence. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125QDX24TNuod7FKLwTDhvW
The diagnose vitals adapter still read coverage, observedPods and scopeError and built captions and a limitation row from them. The bundle stopped sending those fields with the ownership-history layer, so the branches were unreachable and the strings they composed could never appear. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125QDX24TNuod7FKLwTDhvW
The membership-dependent adapters are adapted after every other tool, so the limitations they record were appended after the rest regardless of when their step ran. They are ordered by the transcript position each already carries. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125QDX24TNuod7FKLwTDhvW
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1758949. Configure here.
InformerSynced read the Synced field a per-informer tracking goroutine writes, which lags the informer's own HasSynced by about 10ms. That is harmless for the diagnostics the field was built for and wrong for a caller deciding whether it may trust an empty list: for those few milliseconds after a cache is ready, a workload's pods read as "still loading" and the read is refused. The informer's HasSynced is kept beside each status and answers instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125QDX24TNuod7FKLwTDhvW
A chart whose pods cannot be listed was answered with 503 whether the cache was still loading or the identity may not read them. Only the first is worth retrying. Every path to ErrWorkloadAccessDenied is a permission one — a lister the identity does not have, or a namespace its informer was scoped away from — so it answers 403, and 503 keeps the case that resolves on its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125QDX24TNuod7FKLwTDhvW
Contributor
Author
|
Reviewer triage. Qodo's review ran against
One CI failure was real and is fixed in |
Two things a reader and an agent could not get to. A workload that controls no pods serialized no pod count at all, because zero and "this kind has no pod scope" shared one omitted field. The caption that tells an empty chart apart from a Node could never fire, so a scaled-to-zero workload showed only "No data for this time range". Zero is an answer and now travels as one. A Pod target accepted `pod=~"name"` but not `pod=~"^(name)$"`, which is the form the diagnose prompt asks the agent to write. An agent following the instruction had the pod's own series classified as merely related, losing the target evidence and its change markers. The anchored set is accepted when it names that pod and nothing else. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125QDX24TNuod7FKLwTDhvW
Contributor
Author
|
Both Bugbot findings on Zero-pod charts hide coverage — correct, and it defeated the caption this PR added. My unit test for that branch passed the whole time because it built the result object directly and never went through JSON. The new test asserts Pod target rejects documented matcher — also correct. Both tests were checked against their defects before being kept. |
The empty-chart caption became reachable once a zero pod count could survive the wire, and it read as a failure to establish membership. A workload whose pods Radar cannot resolve reports an error instead of a count, so zero here is only ever the workload's own state. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125QDX24TNuod7FKLwTDhvW
The line naming the charted pods reached the drawer's single chart but not the expanded workload page, which renders its own grid and asks for the metrics itself. That page is where a reader compares six charts at once and where a chart covering the wrong pods was hardest to notice. Every chart in the grid covers the same pods, so it is said once beneath them rather than repeated under each. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125QDX24TNuod7FKLwTDhvW
This was referenced Sep 11, 2026
nadaverell
added a commit
that referenced
this pull request
Sep 11, 2026
…self (#1695) ## Summary Seven places where an investigation claimed more than it had checked: a firing alert that could not contradict an all-clear, a chart axis with a unit nothing earned, an empty read reported as a clean one, an agent note that vanished rather than being counted. Each is small on its own; together they are the difference between Findings stating what Radar established and Findings stating what it assumed. Follows the pod-attribution work that merged as #1694. ## What changed **Per-kind behaviour is decided once.** Whether a card can qualify a model's "no active problem" was one of eight hand-maintained per-kind lists, and the round that added alert and Helm cards updated seven of them — so a firing alert naming the workload sat under a green banner. Those lists are one record over the evidence kinds now (`investigationEvidenceKinds.ts`), holding adverse, focused, full-row, icon and caveat with no optional fields, so the compiler refuses a new kind until each is decided. The rule also counts Radar's intermediate `alert` tone, which is where every Go `high` severity lands. **A note can only explain what its assessment could see.** The healthy-verdict banner is reframed when the agent marked every conflicting card `benign`, and that note must clear five conditions: it belongs to the displayed assessment, it carries text, it sits on the card's current state rather than a superseded read, its role is `benign`, and **the reading it explains was not captured after the assessment ran**. That last one matters because `observe` splits one log stream read by two calls into separate groups, so the check deliberately matches a note across groups sharing a kind and identity — without a bound in time, a note about an early warmup error would also soften the banner over a different failure the same stream reported in a later turn. **A chart carries a unit only when it earned one.** Unit inference was fail-open, so a boolean, a presence flag and a variance in bytes squared all got a bytes axis. It works from an allow-list; arithmetic, comparisons, set operators, a second rate, an unlisted function and a histogram's counting parts all yield no unit. A sort keeps its operand's unit, an angle drops it. Strings and comments are recognised in one left-to-right pass, because scanning for comments first let a `#` inside a label value swallow the rest of the query. **A chart is named by what it measures.** A recognised metric family titles the card — "Memory working set · Deployment shop/api" — and the metric name moves into the summary so it stays visible. The table is deliberately small: an unrecognised metric keeps its own name rather than being handed a meaning Radar cannot derive from it. The scope suffix is added only where Radar already concluded the query is about the target, so the title never widens or narrows what the relevance check decided. **A narrowed read says so.** A cluster-wide `get_events` by a namespace-scoped caller is narrowed to what they may see; with nothing to mark it, an empty result became a receipt claiming the cluster was checked. The response carries `partialScope` and `scopeNamespaces` naming what was actually read. Likewise a chart's empty change-marker list meant "nobody looked" as often as "nothing happened" — it now reads "None of the changes Radar read fall in this window", and stays silent when no change lookup ran in the turn. **A malformed note is counted, not swallowed.** Empty claims are rejected, and a reference pasted inside its marker wrapper is unwrapped — but only when the field is exactly one marker, because taking the first of two would be choosing what the agent meant. Nothing else is repaired. What was dropped is counted through to the pane, and an `evidence` field that is not a list at all is reported as its own fact, since no number describes it. **One vocabulary for the gaps, and one line where one line does.** A receipt card carries a title and the scope it covered, so it stops there unless a second line adds the reason the answer is what it is, or the limit of what it proves — the container status behind "no previous logs expected", the namespaces an events read actually reached and what a clean result there does not clear. Receipts read as plain sentences and no longer sit beside each other saying the same thing differently; a producer that cut its results short says "returned part of"; a permissions gap says what was not readable. Internals like "resolved pods", "no finite values" and "narrowed to keep this investigation bounded" are gone. The sections are deliberately not split. **And the duplication behind several of these.** The HPA diagnosis block existed three times with a redundancy rule that had already drifted; it is one shared `HPADiagnosisSummary` now, resolved to the superset the HPA page was missing. It leads with Radar's reading of the condition and quotes the controller's own sentence under it — named as Kubernetes only where the reason was built from a condition, since elsewhere that field holds Radar's own text. The GVR resolver duplicated across `internal/mcp` and `internal/server` moved to one helper in `internal/k8s`. The metrics authorization table stopped hand-rolling kind mappings while keeping its allowlist, which is load-bearing: the generic GVR helpers alone would have widened the gate from eight chartable kinds to every builtin. The HPA lookup is keyed rather than scanned per reference. The container-filter fallback has one behaviour instead of surfacing its error on one path and swallowing it on the other. The Go and TypeScript role and category lists name each other. ## Scope **The `investigationEvidence.ts` refactor is not in this PR.** Splitting that 5,475-line file into a layered directory is 5,864 lines of movement, and its correctness rests on a line-by-line audit against the file it is derived from. It follows separately, derived against current `main`, so neither change has to be reviewed through the other. **The rule that an old note cannot explain a verdict it never saw already ships**, in the stricter form described above — `benign` role, current card state, displayed assessment, and now the capture-order bound. ## Testing `go build ./...` and `go test ./...` in both modules (`pkg/` is separate), `npx tsc`, and the full vitest suites for `web` (1,103) and `packages/k8s-ui` (3,627). **Visual test: run**, against a live GKE cluster, on the shared HPA diagnosis block — the one rendered surface this adds. An HPA whose target is missing reads "HPA cannot read or update the target scale · 3/3 replicas, bounds 3-5 · Unable to scale", and the controller's own `FailedGetScale` and `FailedGetResourceMetric` sentences quoted beneath it. A KEDA-managed HPA clamped at its floor reads "HPA wants fewer replicas but is held at minReplicas=2 · At min". Everything else this touches is copy inside existing cards, which the tests pin. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches per-user RBAC semantics (events partial scope, metrics kind gating, centralized GVR/SAR resolution) and diagnosis API fields consumed by the UI; behavior is mostly fail-closed and copy-honesty, but wrong gating would hide data or overstate coverage. > > **Overview** > Tightens how investigations **state what Radar actually checked** instead of implying more: per-kind rules live in one `investigationEvidenceKinds` registry (adverse vs context, citation focus, icons/caveats), receipt and limitation copy is plainer, Prometheus metric cards get human titles and change-window messaging only when changes were looked up, and cluster-wide `get_events` / MCP responses expose **`partialScope` + `scopeNamespaces`** so narrowed RBAC reads cannot read as a clean cluster. > > **Agent diagnosis “case” binding** is stricter and more honest: empty or over-long claims and unknown roles are dropped (not truncated or remapped), whole-marker evidence refs unwrap, cap cuts and bind failures roll up as **`unlinkedEvidence`** / **`evidenceMalformed`**, and the UI surfaces those counts. > > **Shared infrastructure**: `k8s.LookupResourceGVR` replaces duplicated MCP/server resolvers (static builtins when discovery is cold); Prometheus chart auth maps kinds via shared GVR catalogues but still **`prom.SupportedKinds`**; container-filter query fallback is one helper with consistent error handling; HPA diagnosis UI is consolidated in **`HPADiagnosisSummary`** with clearer Kubernetes vs Radar attribution; HPA context builds use an indexed lookup. > > Docs add Prometheus metrics RBAC notes and document partial-scope events behavior for MCP. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 93eafb5. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
nadaverell
added a commit
that referenced
this pull request
Oct 14, 2026
…2005) After a Pod is replaced with the same name, retained kubelet warnings can make its healthy replacement look as though it still has an image-pull failure. This affects agents using `get_resource(include=events)` or workload `diagnose`, including ordinary StatefulSet Pod replacement. Current warnings match the resource or resolved Pods already observed, including their UIDs. Known UID mismatches are excluded; UID-less warnings remain identity-matched evidence without proof of incarnation. Node producers using the Node name as UID and mirror Pods using their observed static UID remain supported. This is the incarnation filtering `kubectl describe` applies (it selects events by `involvedObject.uid`, with the same Node and mirror-Pod exceptions), and the same fix k8sgpt shipped in v0.4.40. When warnings were excluded this way, `diagnose` and `get_resource(include=events)` add one entry to their existing `warnings` array saying how many were left out, that they may still explain what happened before (such as why a Pod was replaced), and that `get_events` has them, so an empty list is not read as "nothing ever failed". One shared current-workload resolver validates controller API group, kind, name and UID at the root and each ReplicaSet/Job hop. Diagnose supplies its observed root UID and reuses the resolved Pod population for warnings, logs and vitals; single-workload rightsizing likewise uses the root it already read. Existing workload log reads and generic current metric scope resolve the current root through the same helper. Current membership requires Radar to be able to list Pods in the namespace and the root kind as well as its controller inventories. Typed roots use ready, covering listers. A watched Rollout/Workflow root is read from the store; a cold one starts the existing on-demand acquisition and is waited for at most the request context or 3s (250ms for the restart warning, which runs before the restart), past which the answer is "still loading" while acquisition continues. If a Rollout or Workflow informer exists but never syncs (for example a failing CRD conversion webhook), those reads keep answering "still loading" rather than falling back to name matching. A confirmed missing root is reported as not found: `get_workload_logs` answers "resource not found" with name suggestions, while charts and the Metrics tab still read it as no current Pods. Denied or warming inventories remain errors. ## Current membership vs history Two different questions get two different answers by design: - **Which Pods does this workload control now** (diagnose, logs, restart warning, current-Pod tables, live OOM evidence, warnings presented as current evidence): the observed incarnation, by full controller UID chain. - **What ran under this workload's name over a window** (Metrics tab history, rightsizing recommendations, timeline/workload history, `get_events`): logical identity spanning rollouts and recreation. Unchanged here. #1694 matched live ownership by name so it would agree with kube-state-metrics, which records no owner UID. Since the Metrics tab's history deliberately spans incarnations (`docs/workload-metrics.md`), live membership no longer needs to match that history, and UID matching removes Pods left behind by a deleted-and-recreated root. Historical `get_events`, resource history, timeline, GitOps, selector-based HTTP workload logs, bulk rightsizing and workload-level issue/admission aggregation retain their existing behavior. Generic PromQL remains name-based: an older UID with the same selected Pod name may contribute samples within the query window. ## Also in this PR - `/api/applications` joins a Warning event into a workload's events only when it describes the observed workload or one of its current Pods (same rule, one shared predicate). Before, a replaced same-name Pod's events joined and were summed into the current Pod's counts. Radar Hub's application page lists these events as current issues, and OSS's history summary headlined them as "Current incident" right after a repair. Response shape is unchanged, so Hub picks this up as clusters upgrade. Pod membership there is still selector-based, so a sibling controller's Pods with an overlapping selector still contribute; that is a whole-row membership question (health and restarts use the same Pods), left for the follow-up. - The issues engine no longer relabels a same-name replacement Pod with its predecessor's probe failure (the UID guard the restart-loop evidence already had). Both guards now accept a static Pod's static UID, so control-plane Pods such as kube-apiserver keep their current probe failures. - `get_workload_logs` on a workload that does not exist answers "resource not found" instead of "pods: 0" with a scheduling hint. - An uninstalled Rollout/Workflow kind reads as "not installed" (REST charts 400, explicit MCP message) instead of 502 / "unknown resource kind". The REST 403 says Pod membership could not be established rather than blaming the Pod lister when the root kind is the unreadable one. No new endpoint, field or response shape. Error messages and the REST status for an uninstalled kind change. ## Testing - Go build, full `make test` and the shared `pkg` module. Regression coverage includes stale root/intermediate UIDs, follow-up log requests, generic metric scope, rightsizing tied to the observed root, dynamic-root cold starts, a stalled acquisition answering "still loading" within budget, cancellation, a denied dynamic root, a root informer or namespace outside the watched scope, denied/warming/deleted roots, cache-only warm traversals, the withheld-history warning (including when every retained warning is historical), a replacement Pod not inheriting its predecessor's probe failure, and a static Pod keeping its own. New tests were checked to fail without their fix. - Visual testing skipped: no UI delta. - Live nonprod StatefulSet image repair: real kubelet warnings for the previous Pod disappear from current resource/Diagnose evidence while historical warnings remain. A separate controlled stale-owner fixture checks Diagnose, follow-up workload-log fan-out and generic metric query membership; it demonstrates handling, not frequency. The local query stub verifies selection, not measured metric values. Owned fixtures and comparison servers are cleaned up. ## Known follow-ups (same bug class, not in this PR) - Next PR, stacked on this branch: show earlier-incarnation history labelled rather than dropped — the Applications history banner (needs a flagged field, and radar-hub must pass it through its own event DTO), the resource drawer's Recent Events and "Recent activity". - Applications Pod membership is selector-based for the whole row (health, restarts, events); move it to ownership together. - Helm hook evidence, admission FailedCreate, PVC pending cause and GitOps RecentEvents attribute events by name. - Bulk rightsizing's live OOM membership still follows owner names; history-aware diagnose vitals.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
Four surfaces answered "which pods belong to this workload" four different ways for Prometheus.
<name>-.*apichartsapi-workerandapi-migratepodsworkloadRefmigration the referenced Deployment's podsThis is not hypothetical. On a live cluster running stock components, eight Deployments have a name that is a prefix of a sibling's —
cert-managerovercert-manager-cainjectorandcert-manager-webhook,external-secretsover its webhook and cert-controller,keda-operatoroverkeda-operator-metrics-apiserver,kube-dnsoverkube-dns-autoscaler. Measured against that cluster's Prometheus, the CPU those charts report is overstated by 2.3× to 3.2×.What this does
Membership is established, never inferred.
k8s.WorkloadPodswalks the controller chain that the topology and kube-state-metrics both use: pod → ReplicaSet → Deployment or Rollout, pod → Job → CronJob, direct for StatefulSet, DaemonSet, ReplicaSet, Job and Workflow. Queries name that set exactly (pod=~'^(a|b)$'), andprom.BuildQueryno longer builds a workload query from a name at all.A cache that cannot answer says so. A denied lister, an informer still filling its initial sync, and an informer scoped to other namespaces all return an empty list rather than an error, and "this workload controls no pods" is a claim none of them support —
KindCoversNamespacesays as much itself. All three are refused, for the pod lister every kind reads and for the ReplicaSet and Job hops. A workload whose pods cannot be established is reported, never charted as empty, and a selection that names no pod matches nothing rather than falling back to its namespace.The two answers are told apart where it counts: a permission failure is
403and will not change on retry, while a cache still loading is503and will. Zero pods is itself an answer and travels as one, so a scaled-to-zero workload reads differently from a Node, which has no pod scope at all.Aggregation stays a separate axis from membership.
pkg/prom.BuildScopedQueryrenders either form for every category: the workload page keeps one series per pod for its legends, per-pod limit overlays and restart deltas; the diagnose vitals keep one total.The agent gets the pod list and a rule. The diagnose bundle carries
podNames, the prompt tells the agent to scope its own queries to them, and the Findings classifier proves a pod matcher against that set. An exact name or an anchored set of established pods is the target's — including on a Pod target, which recognises its own name in the^(name)$form the prompt asks for. A prefix, an unescaped dot (^(api.v2-0)$also selectsapi-v2-0), or aworkload_typenaming another kind is not.Classification does not depend on the order the agent worked in. Two adapters ask which pods a producer established. Both now run after every other tool in the transcript, so a Prometheus query that ran before the read naming the same pod reaches the same verdict as the reverse order.
What this charts, and what it does not
Membership is the pods a workload controls at load time. A chart's line spans the whole window, but a pod replaced earlier in that window is not in it, and a workload scaled to zero has nothing to chart. The caption under each workload chart says so rather than leaving the reader to assume the line covers everything that ran.
Reading membership from kube-state-metrics over a window answers exactly those cases, and that layer is deliberately not part of this change. Its two ownership edges are read by independent selectors that cannot establish both edges held at the same instant, so a ReplicaSet transferred between Deployments is attributed to both for the length of the lookback; and a field saying which source answered cannot also say whether that source was complete. Neither question has a good answer while attribution can name a pod the workload never owned. It continues on
feat/pod-attribution-history.Because membership needs no Prometheus call, resolving it is a local lister read — no probe, no cache keyed on the connection, and no resolved scope that can go stale between the pod list a bundle reports and the pods its charts query.
Behaviour changes worth knowing
apiandapi-worker) drop the sibling's pods. That is the fix, and it will look like a drop in reported usage on those workloads.broaderrather thanproducer-related.prom.BuildQueryreturns""for workload kinds; callers resolve membership first and useBuildScopedQuery.podsandpodsTotalon the REST response;podNamesandpodNamesTruncatedon the diagnose bundle.403where it previously answered503.Where different logic is deliberate
Rightsizing keeps its own seven-day statistics and its own kube-state-metrics probe, which are about sample retention rather than membership. A Pod target is its own exact scope. A Node has no pod scope.
Ownership is matched by controller kind and name, not UID, so a workload deleted and recreated while its old pods are still terminating claims them. kube-state-metrics joins on
owner_nameand exposes no owner UID, so a UID-strict live path would disagree with every ownership source Radar reads.Verification
go build ./...,go test ./...in both modules (pkg/is separate),npx tsc, and the full vitest suites forweb(1085) andpackages/k8s-ui(3606).Tests cover the ownership walk per kind including the Rollout
workloadRefsplit and cross-namespace safety, refusing a warming or differently-scoped informer rather than reporting an empty workload, the query shape for every category and both aggregates with a paren-balance guard, a cut pod list, the classifier's ordering independence across both consumers and across turns, itsworkload_typeveto and wildcard-dot rejection, the anchored set form on a Pod target, and a zero pod count surviving JSON rather than being omitted with it.Verified live against a GKE cluster with kube-state-metrics and Prometheus: queries generated by the shipping code, run against that Prometheus, reproduce the corrected numbers on all three badly-affected collisions.
Note
Medium Risk
Touches diagnose critical path, Prometheus query shapes, and evidence classification across backend and UI; behavior changes on name-colliding workloads are intentional but will shift reported metrics and relevance tiers.
Overview
Unifies which pods count as a workload across diagnose, workload logs, Prometheus charts/vitals, and Findings: membership comes from controller ownership (
WorkloadPods/ResolvePodScope) and PromQL uses exact pod sets (pod=~'^(a|b)$') viaBuildScopedQuery, instead of label selectors or<workload-name>-.*prefixes that pulled in siblings and debug pods.Diagnose exposes
podNames(capped like metrics) for follow-upquery_prometheusscoping; pod resolution failures degrade with explicit warnings (RBAC vs cache warming) rather than silent empty bundles. Metrics paths surface errors when pods cannot be established or Prometheus discovery times out instead of omitting the field.Investigation UI proves target relevance against pods established in a successful target
diagnose(and defersquery_prometheusclassification until membership is known so tool order does not change verdicts). Charts showpods/podsTotaland a coverage caption for current pods vs the time window.Supporting changes: per-informer
InformerSynced, scoped test caches, docs inmcp.md, and strictermetricsScope(no prefix/dot wildcards;workload_typeveto).Reviewed by Cursor Bugbot for commit e9fd5b0. Bugbot is set up for automated code reviews on this repo. Configure here.