Repository navigation
Stop status badges reporting health nothing verified - #1659
Merged
Merged
Conversation
PR Summary by QodoPrevent unverified Kubernetes status badges from claiming health
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo
1. Unknown workload verdicts look accepted
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| }) | ||
|
|
||
| it(`${name} ignores a status.conditions that these kinds never have`, () => { | ||
| // Reading this path was the original bug — it is not part of the schema. |
There was a problem hiding this comment.
1. Maintainers inherit bug history 📘 Rule violation ⚙ Maintainability
The getServiceMonitorStatus test comment calls the invalid status.conditions read `the original bug`, embedding change history in source. Anyone maintaining this regression test encounters historical PR context that can drift independently of the schema behavior documented by the assertion.
Agent Prompt
## Issue description
Remove the historical bug reference from the test comment while retaining the relevant schema constraint.
## Issue Context
The assertion should explain that `status.conditions` is not part of these resource schemas without describing prior implementation history.
## Fix Focus Areas
- packages/k8s-ui/src/components/resources/status-overclaim.test.ts[30-30]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+29
to
+32
| if (cond.status === 'True') verdicts.push(true) | ||
| else if (cond.status === 'False') { | ||
| verdicts.push(false) | ||
| if (!rejection) rejection = cond.reason || '' |
There was a problem hiding this comment.
2. Unknown workload verdicts look accepted 🐞 Bug ≡ Correctness
getConfigResourceStatus records only Accepted=True and Accepted=False, excluding current Accepted=Unknown conditions from the aggregate. When one selecting workload reports True while another reports Unknown, the resource receives a healthy Accepted badge even though not every workload has accepted it.
Agent Prompt
## Issue description
Prometheus configuration status aggregation ignores current `Accepted=Unknown` reports. This allows one `True` report to produce a healthy badge despite another selecting workload remaining unassessed.
## Issue Context
The Prometheus Operator permits `Accepted` condition statuses of `True`, `False`, or `Unknown`. Count every current binding report in the aggregate, while preserving explicit rejection and stale-generation handling; add coverage for mixed `True`/`Unknown` and all-`Unknown` states.
## Fix Focus Areas
- packages/k8s-ui/src/components/resources/resource-utils-prometheus.ts[17-52]
- packages/k8s-ui/src/components/resources/status-overclaim.test.ts[10-67]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // evidence of a fault. | ||
| if (hosts.length === 0) { | ||
| return { text: 'No Hosts', color: healthColors.unhealthy, level: 'unhealthy' } | ||
| return { text: 'No Hosts', color: healthColors.unknown, level: 'unknown' } |
There was a problem hiding this comment.
3. Empty traffic rules lose their warning 🐞 Bug ≡ Correctness
getVirtualServiceStatus now returns unknown for missing hosts before evaluating totalRoutes. A hostless object with no HTTP, TCP, or TLS routes therefore bypasses the existing unhealthy `No Routes` result, although only hostless objects that actually define delegated routes justify the new exception.
Agent Prompt
## Issue description
The hostless VirtualService exception executes before the route-count validation. Empty hostless objects consequently lose the existing `No Routes` warning.
## Issue Context
Keep valid delegated VirtualServices neutral or unknown when they omit hosts, but continue marking every VirtualService without HTTP, TCP, or TLS routes unhealthy. Add a route-less hostless test alongside the valid hostless delegate case.
## Fix Focus Areas
- packages/k8s-ui/src/components/resources/resource-utils-istio.ts[24-40]
- packages/k8s-ui/src/components/resources/status-overclaim.test.ts[101-118]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
nadaverell
force-pushed
the
fix/status-badges-claiming-unverified-health
branch
2 times, most recently
from
September 7, 2026 11:10
361df2f to
38e36ae
Compare
Six status cells painted a row green, or red, on evidence the object does not carry. ServiceMonitor, PodMonitor and PrometheusRule read `status.conditions`, a path their schema does not define. The operator reports acceptance per selecting workload under `status.bindings[].conditions`, the only condition type is `Accepted`, and it writes any of it only when the StatusForConfigurationResources feature gate is on. The read therefore never matched and every row fell through to a green "Active" — including a configuration a Prometheus had explicitly rejected. They now read the real path, report the rejection, distinguish workloads that disagree, skip a condition observed against an older generation, and say nothing when nothing reported. A Kyverno policy counted its own declared rules as readiness. Kyverno writes a Ready condition; declaring rules is not the controller accepting them. Istio ServiceEntry rendered `spec.location` in green. External and Internal describe where a host sits relative to the mesh, not whether it resolves or answers. VirtualService called any object with hosts and routes "Active", and called every hostless one broken — but a delegate VirtualService is required to declare no hosts, and nothing on the object says whether a parent delegates to it. A CNPG ObjectStore reported "Recoverable" from the presence of a server entry. Every timestamp on a recovery window is optional, so a server can be listed while holding nothing restorable. Badges take their colour from the returned `color`, so each of these changes the colour alongside the level; changing the level alone would have left the row green.
nadaverell
force-pushed
the
fix/status-badges-claiming-unverified-health
branch
from
September 7, 2026 11:29
38e36ae to
7b527b5
Compare
nadaverell
added a commit
that referenced
this pull request
Sep 7, 2026
A topology node's status is not just a dot in the graph. MCP builds its `get_topology` problem list from degraded and unhealthy nodes (`internal/mcp/tools.go:1645`), and GitOps derives managed-resource health from it (`pkg/gitops/tree/graph.go:21`). A green node is Radar answering *"nothing is wrong"* — to an operator scanning the graph, and to an AI asked whether the cluster is healthy. Three places were answering that without checking. This continues #1645/#1649/#1659, which fixed the TypeScript accessors; `pkg/topology` computes health independently in Go and had drifted from them. ## What changed **Gateway API routes were judged on `Accepted` alone.** A gateway accepting a route says nothing about whether the backends it forwards to exist — so an HTTPRoute whose `backendRef` named a deleted Service reported healthy. That is precisely the failure an operator opens the graph to find, and it is the same defect #1645 fixed on the TS side. `getRouteHealth` now reads `ResolvedRefs` alongside `Accepted`. A route reads healthy only when every parent reports both `Accepted=True` and `ResolvedRefs=True`. A parent that reports nothing — no conditions, or only ones observed against an earlier `generation` — is counted as unassessed and blocks both the healthy and the unhealthy verdict, so neither a green node nor a total failure is claimed while a gateway is still silent. Two deliberate differences from the TS accessor, since this is otherwise meant to match it: the `observedGeneration` staleness check is an addition (`speaksToCurrentSpec` in `resource-utils.ts:1953` only tests whether the relevant conditions exist), and a route whose parents all report nothing reads `unknown` here where TS reads `Pending`/degraded — claiming a fault we have not established seemed the wrong direction for this change. Both are candidates for reconciling in TS rather than divergences to keep. **HorizontalPodAutoscalers were hardcoded healthy** while the builder read `hpa.Status.CurrentReplicas` three lines below. `pkg/hpadiag` already diagnoses HPAs for the detail drawer, so the node now uses it and the graph stops disagreeing with the drawer. An autoscaler the controller has not written a condition for reads unknown rather than healthy: `hpadiag.Analyze` reports `ok` for a conditionless HPA, so absence of evidence would otherwise arrive as health. States the vocabulary calls intentionally idle — scaled to zero, disabled, pinned, stabilised, at minimum — map to neutral, matching the shipped TS mapping. Otherwise the mapping only ever *downgrades* on negative evidence — unreachable metrics and inability to scale become unhealthy, capped/incomplete metrics degraded, everything else stays healthy. Notably `scaling_up`/`scaling_down` remain healthy: an HPA doing its job is not a problem, and since degraded nodes become MCP-reported problems, promoting routine autoscaling would manufacture false ones. This differs from `hpaStateLevel` in `resource-utils-hpa.ts:68`, which calls both scaling directions degraded — worth reconciling, but in both places deliberately rather than by divergence here. **Four Secret nodes are stubs synthesised from a reference** — a TLS or CA secret named by a Traefik or HTTPProxy resource that the Secret listing never produced, either because it does not exist or because it was not listable. They were built green, so a misspelled `certificateRef` rendered as a healthy Secret. They now report unknown. ## Verification The HPA fix is verified end to end against a real controller failure rather than a hand-written fixture: a `kind` cluster has no metrics-server, so an HPA there genuinely gets `ScalingActive=False / FailedGetResourceMetric` from the HPA controller. With that in the cluster, the topology API returns `status: "unhealthy"` for the node, and `get_topology` over MCP returns the same — where both previously said healthy. Route health is pinned by table-driven tests covering unresolved backendRefs, full acceptance, partial and total rejection, absent reports, a stale `observedGeneration`, and the mixed cases where one parent is current and healthy while another is stale or silent. I confirmed these actually fail against the previous algorithm rather than passing either way: for a route with `Accepted=True, ResolvedRefs=False` the old code returns `healthy` and the new code returns `degraded`. `go build` and `go test ./...` clean in both the root and `pkg` modules. ## Scope Deliberately narrow. A survey found 22 hardcoded `Status: StatusHealthy` node constructions, but most are not defects worth changing: - **Recolouring config objects** (ConfigMap, listed Secrets, ServiceAccount, the synthetic Internet node) is not done. `StatusNeutral` means *intentionally idle* and paints a sky-blue outline (`K8sResourceNode.tsx:275`), so it would add emphasis to the least interesting nodes rather than remove a claim. That needs a deliberate "health not applicable" presentation instead. - **NetworkPolicy and the spec-presence-implies-healthy sites** (VirtualService, Istio Gateway, Traefik routes) are consistency gaps, not defects anyone is misled by. - **Ingress `loadBalancer` and Service endpoints** would be *new* health signals rather than removing a false one. Given that a wrong status propagates into MCP problems and GitOps degraded counts, a false alarm costs more than a missing one here, and Service semantics already live in `internal/k8s/detect.go:1151`. Known and not addressed: `TraefikService` is marked unhealthy on zero backends in the resources view (`builder.go:2274`) but always healthy in the traffic view (`:6472`); Cilium and ClusterNetworkPolicy expose conditions the builder ignores; VPA conditions are readable and `ConfigUnsupported` currently renders green; and the generic condition helper (`builder.go:7951`) returns on the first Ready/Available/Succeeded it sees. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes how topology health is computed for routes, HPAs, and secrets, which can alter MCP-reported problems and GitOps degraded counts; logic is conservative but user-visible status may shift. > > **Overview** > Topology nodes were marking **healthy** without checking real status, which flows into MCP problem lists and GitOps health. This PR tightens three cases in `pkg/topology/builder.go`. > > **Gateway API routes** — `getRouteHealth` no longer treats `Accepted=True` as fully healthy. It also requires current `ResolvedRefs` (and ignores conditions on stale `observedGeneration`), counts silent parents as unassessed, and only returns healthy when every reporting parent accepts and resolves refs. > > **HPAs** — HPA graph nodes call new `hpaNodeHealth`, which maps `hpadiag.Analyze` results into graph status (unhealthy/degraded for scale/metrics failures, neutral for intentional idle states, unknown when unreconciled). Routine scaling stays non-problematic so MCP does not get false alarms. > > **Referenced Secret stubs** (Traefik/Contour TLS/CA refs not in the Secret list) switch from **healthy** to **unknown** so missing secrets are not shown as OK. > > Adds `node_health_test.go` table tests for route and HPA health behavior. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit edf6a40. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
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.
Six status cells painted a row green — or red — on evidence the object does not carry. This continues #1645 and #1649; a census of the remaining status accessors found the class had survived in five more places, and one of them was reading a field that does not exist.
What changed
Prometheus configuration resources (ServiceMonitor, PodMonitor, PrometheusRule). These read
status.conditions, a path their schema does not define. The operator reports acceptance per selecting workload understatus.bindings[].conditions, the only condition type isAccepted, and it writes any of it only when theStatusForConfigurationResourcesfeature gate is enabled. The read never matched, so every row fell through to a green "Active" — including a configuration a Prometheus had explicitly rejected.They now read the real path and report what it says. A resource is
Acceptedonly when every workload that reported has accepted it: one rejection among several shows partial acceptance, a workload still reportingUnknownkeeps the badge off green, an outright rejection surfaces its reason, and nothing reported readsNot assessed. A condition whoseobservedGenerationpredates the object's is skipped rather than read as current.Worth being precise about the ceiling:
Acceptedmeans the operator accepted and wrote the configuration. It does not mean a target exists or that scraping succeeds, and this badge does not claim it does.Kyverno policies counted their own declared rules as readiness. Kyverno writes a
Readycondition; declaring rules is not the controller accepting them. The fallback now reportsNot assessed, which also coversReady=Unknownand a conditions list with noReadyat all.Istio ServiceEntry rendered
spec.locationin green.MESH_EXTERNAL/MESH_INTERNALdescribe where a host sits relative to the mesh, not whether it resolves or answers — the text stays, the tone becomes neutral.ServiceEntry and PeerAuthentication lose their Status column entirely, because in both cases it could only ever restate the column beside it.
getServiceEntryStatusandgetServiceEntryLocationreadspec.locationwith the same default and map it to the same two words. Their only divergence isNo Hosts, and that state cannot exist: Istio's CRD declareshostsbothrequiredandminItems: 1in all three served versions, so the API server rejects a ServiceEntry without them. Status was therefore identical to Location on every row.PeerAuthentication is the same shape with no escape clause at all — both columns read
spec.mtls.mode || 'UNSET'and encode the same four severities, differing only in wording (Strict mTLSbesideSTRICT).Location and mTLS Mode are the honest primary representations and are kept. Both accessors stay: the
./components/resources/*export makes them importable, and the detail pages still use them.Istio VirtualService called any object with hosts and routes "Active", and called every hostless one broken. Istio requires a delegate VirtualService to declare no hosts, and nothing on the object says whether a parent delegates to it, so red was a false alarm on a documented pattern. Hostless is now
unknown; a declaration with routes isDefined. A VirtualService with no routes stays a real defect either way — a delegate omits hosts, but it exists to supply routes.CNPG ObjectStore reported "Recoverable" from the presence of a server entry. Every timestamp on a
RecoveryWindowis optional, so a server can be listed while holding nothing restorable —{ pg: {} }was green. It now requires an actual recovery point.Reviewer note
Badges take their colour from the returned
color, not fromlevel. Each change moves both; changinglevelalone would have left every row green.Vocabulary follows the two prior PRs rather than inventing any:
Not assessed/unknownwhere nothing confirmed anything (getDestinationRuleStatus), andneutralfor a classification that is not a health claim (getIstioGatewayStatus'sDefined).Deliberately not in scope
pkg/topology/builder.gohardcodesStatusHealthyfor ServiceMonitor and PodMonitor nodes and treats any routed VirtualService as healthy. Same overclaim, different layer and language — it wants its own change.ConfigAuditReporttreats an absentsummaryas all-zero and reports "Pass". Upstream markssummaryoptional, so this is reachable in principle, but I have not established the standard scanner emits it.getCronJobStatusreturns green "Scheduled" for any unsuspended CronJob, which establishes that scheduling is enabled rather than working.Checked and deliberately left alone: Trivy
VulnerabilityReport(all five severity counts are read before "Clean"), Knative Revision (its healthy branches all requireReady=True, and upstream excludesActivefrom the readiness set), and CAPI's shared phase helper (already falls through toUnknown).Testing
make tscclean. 1707 tests pass (77 files), 26 of them new — each pinned to the upstream contract rather than to the implementation: the bindings path and its absence, an explicit rejection, disagreeing workloads, a staleobservedGeneration, Kyverno's three not-ready shapes, the delegate VirtualService, and an empty CNPG recovery window.Visually verified against a live cluster, with the CRDs and fixtures installed to reach every branch: a ServiceMonitor with no status, one accepted, one rejected, one selected by two workloads that disagree, and one where a second workload is still undecided; both VirtualService shapes, both ServiceEntry locations, and three PeerAuthentication modes.
The render confirmed the point of the change —
sm-no-statusis grey rather than green,sm-rejectedis red, and the hostless VirtualService is grey rather than red. The twoAccepted by 1/2rows are visibly different tones (amber for a rejection, grey for an undecided workload) despite identical text.It also surfaced two things code review did not. A controller
reasonis arbitrary-length and these kinds could previously only ever renderActive, so a rejection reason overflowed its cell by 28px into the next column; the status badge is now bounded to the cell with the full value on hover, matching the pattern the CAPI and Karpenter cells already use. And thew-24Status column could not fit the new vocabulary —Accepted by 1/2ellipsised toAccepted by…, losing the count — so the three Prometheus kinds move tow-36. Kyverno's status cell renders areasonthe same way and gets the same guard.🤖 Generated with Claude Code
Note
Low Risk
Display-only status logic in k8s-ui with regression tests; no API or auth changes, though operators may see more unknown/neutral badges instead of false greens.
Overview
Status badges no longer infer “healthy” from spec shape or wrong status paths across Prometheus Operator monitors/rules, Kyverno policies, Istio VirtualService/ServiceEntry, and CNPG ObjectStores.
Prometheus ServiceMonitor, PodMonitor, and PrometheusRule now derive status from
status.bindings[].conditions(Accepted), respectingobservedGeneration, partial/multi-workload outcomes, and explicit rejections; missing operator status shows Not assessed instead of green Active. Kyverno drops the “has rules ⇒ Ready” fallback. Istio ServiceEntry location labels are neutral; VirtualService treats hostless delegates as unknown, valid declarations as Defined, and still flags missing routes. CNPG ObjectStore requires a real recovery timestamp before Recoverable.UI tweaks widen Prometheus status columns and truncate long badge text with a
titletooltip. Newstatus-overclaim.test.tspins these contracts.Reviewed by Cursor Bugbot for commit 38e36ae. Bugbot is set up for automated code reviews on this repo. Configure here.