Repository navigation
fix(istio): stop policy rows asserting a security posture no single object establishes - #1649
Conversation
…ablishes
An AuthorizationPolicy's action is a declaration, not a verdict. The
request decision is made across every policy selecting the workload, so
one object cannot establish it — yet ALLOW rendered green and DENY red,
which made Istio's documented deny-all (`spec: {}`, where the action
defaults to ALLOW and rules are alternatives, so zero match nothing) the
greenest row on the page, and made a DENY control doing its job look like
a failure. Every action now reads neutral in the table, the detail page,
and the status accessor, and the one shape worth marking — an ALLOW with
no rules — carries an amber count in the default-visible Rules cell.
The Applies-to column was dead: AuthorizationPolicyCell had no selector
case, so it rendered '-' for every policy in every cluster. Its accessor
was also wrong twice. A policy attached through targetRef or targetRefs,
as waypoints and ambient Gateways are, has no selector and was reported as
namespace-scoped; and an empty selector in the mesh root namespace is
mesh-wide, not namespace-wide. The cell now renders attachments as
kind/name and names both possible scopes when nothing is declared, because
MeshConfig.rootNamespace is not readable from the object and the
conventional namespace name is not evidence.
DestinationRule's status restated the subset count shown two cells over as
a green badge, or called a declared traffic policy Configured. It keeps
the one failure it can see — a rule with no host applies to nothing — and
reports the rest as not assessed. A Client TLS column now shows the
declared mode: DISABLE there turns off client mTLS for the host even where
a PeerAuthentication requires it, and both rows read as configured while
it happens.
Both status columns default to hidden, so the neutral action badge and the
Rules count are what an operator sees without opting in.
The expanded view said an ALLOW policy with no rules means no traffic is allowed. Another ALLOW policy selecting the same workload can permit requests, so default-deny-plus-exceptions — an ordinary configuration — produced a false outage claim, and it appeared exactly when an operator opened the row to investigate the table's more careful wording. The banner now says the policy contributes no permitted matches and that other policies may still permit traffic. The amber count in the table carries the same explanation on the badge itself, rather than depending on the reader finding the column header's tooltip.
… otherwise A DENY with no rules was shown as a red "Deny All" asserting that it blocks all traffic to the workload. Istio specifies the opposite: rules are alternatives and an unset list never matches, so such a policy matches no requests and denies none. That is the dangerous direction of the error — it tells an operator that an ineffective blocking policy is protecting them, and misdirects an outage investigation toward a policy doing nothing. Both empty-rule cases now come from one helper, since they are the same fact about the same field: a DENY with no rules is reported as inert, an ALLOW with no rules keeps its warning because a workload with any ALLOW policy admits only what one of them matches. Neither is stated as an outcome for the workload, because every policy selecting it takes part in the decision. Extracting the helper also makes both reachable from tests without rendering the drawer, which is how the empty-rules reading went unchecked.
…xceptions from A DENY rule with no conditions matches every request. That is not a baseline other policies add exceptions to: DENY is evaluated before ALLOW, so no ALLOW policy can re-permit what it matches, and the workloads it selects are blocked outright. The drawer now says so, neutrally, since the configuration is often deliberate.
PR Summary by QodoCorrect Istio policy status and scope semantics
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo
1.
|
A client-side DISABLE was described as turning off mTLS where the server requires it. Against a STRICT PeerAuthentication that is not what happens: the server rejects the plaintext and the requests fail. The unencrypted case is the PERMISSIVE one, where the connection succeeds without TLS. Both outcomes depend on an object this row cannot see, which is the reason the column reports the declaration and stays neutral about the posture.
…tection The Client TLS and Applies-to columns had no filter reader, so the dropdown stayed empty for values the table was visibly showing. Client TLS also needed its own key: 'tls' already belongs to Traefik's boolean column and sits in SKIP_FILTER_COLUMNS, which would have made the reader unreachable. A DENY rule counts as unconditional when it carries no conditions, which includes from, to and when serialized as empty lists rather than omitted — those still match every request the policy selects. An unrecognised field is treated as a condition, so a future matcher cannot be misread as the absence of one. The load-balancer tooltip described the unset case while being attached to the whole column, so hovering above a rule declaring ROUND_ROBIN explained the opposite of that rule. It now describes the column and names '-' as the unset value.
…ncode Both maps mapped all four actions to the same tone, and both call sites read map[action] ?? 'neutral', so the expression was unconditionally neutral. One was exported solely so a test could assert every value in it was neutral — a test that could not have caught the regression it named, since a different code path setting a tone would pass it unchanged. The badge is now neutral at the two sites, with the reasoning as a comment where the map's name used to carry it.
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* under `status.bindings[].conditions`, the only condition type is `Accepted`, and it writes any of it only when the `StatusForConfigurationResources` feature 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 `Accepted` only when every workload that reported has accepted it: one rejection among several shows partial acceptance, a workload still reporting `Unknown` keeps the badge off green, an outright rejection surfaces its reason, and nothing reported reads `Not assessed`. A condition whose `observedGeneration` predates the object's is skipped rather than read as current. Worth being precise about the ceiling: `Accepted` means 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 `Ready` condition; declaring rules is not the controller accepting them. The fallback now reports `Not assessed`, which also covers `Ready=Unknown` and a conditions list with no `Ready` at all. **Istio ServiceEntry** rendered `spec.location` in green. `MESH_EXTERNAL` / `MESH_INTERNAL` describe 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. `getServiceEntryStatus` and `getServiceEntryLocation` read `spec.location` with the same default and map it to the same two words. Their only divergence is `No Hosts`, and that state cannot exist: Istio's CRD declares `hosts` both `required` and `minItems: 1` in 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 mTLS` beside `STRICT`). 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 is `Defined`. 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 `RecoveryWindow` is 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 from `level`. Each change moves both; changing `level` alone would have left every row green. Vocabulary follows the two prior PRs rather than inventing any: `Not assessed` / `unknown` where nothing confirmed anything (`getDestinationRuleStatus`), and `neutral` for a classification that is not a health claim (`getIstioGatewayStatus`'s `Defined`). ## Deliberately not in scope - `pkg/topology/builder.go` hardcodes `StatusHealthy` for ServiceMonitor and PodMonitor nodes and treats any routed VirtualService as healthy. Same overclaim, different layer and language — it wants its own change. - Trivy `ConfigAuditReport` treats an absent `summary` as all-zero and reports "Pass". Upstream marks `summary` optional, so this is reachable in principle, but I have not established the standard scanner emits it. - `getCronJobStatus` returns green "Scheduled" for any unsuspended CronJob, which establishes that scheduling is enabled rather than working. - The ServiceEntry detail page still shows a status badge above a Location property — the same restatement, but detail pages have the room and a leading status badge is the convention there. Checked and deliberately left alone: Trivy `VulnerabilityReport` (all five severity counts are read before "Clean"), Knative Revision (its healthy branches all require `Ready=True`, and upstream excludes `Active` from the readiness set), and CAPI's shared phase helper (already falls through to `Unknown`). ## Testing `make tsc` clean. 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 stale `observedGeneration`, 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-status` is grey rather than green, `sm-rejected` is red, and the hostless VirtualService is grey rather than red. The two `Accepted by 1/2` rows 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 `reason` is arbitrary-length and these kinds could previously only ever render `Active`, 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 the `w-24` Status column could not fit the new vocabulary — `Accepted by 1/2` ellipsised to `Accepted by…`, losing the count — so the three Prometheus kinds move to `w-36`. Kyverno's status cell renders a `reason` the same way and gets the same guard. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!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`), respecting `observedGeneration`, 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 `title` tooltip. New **`status-overclaim.test.ts`** pins these contracts. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 38e36ae. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
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 -->
Summary
An Istio AuthorizationPolicy's action is a declaration, not a verdict. The request decision is made across every policy selecting a workload — Istio evaluates
CUSTOM, thenDENY, thenALLOW— so no single object can establish the outcome. Radar's rows did anyway, and got it backwards in the cases that matter most.spec: {}is Istio's documented deny-all: the action defaults toALLOW, rules are alternatives, and "if not set, the match will never occur… equivalent to setting a default of deny." Radar rendered it green. Meanwhile aDENYpolicy doing its job rendered red, and aDENYwith no rules — which matches nothing and therefore denies nothing — carried a red "Deny All" banner telling an operator that an inert policy was protecting them.Two neighbouring rows lied in the same way: the Applies-to column was blank for every policy in every cluster, and DestinationRule restated the subset count beside it as a green badge.
AuthorizationPolicy
Actions read neutral everywhere — table, detail page, status accessor. An action is what the policy declares; a tone would assert a decision this object does not make.
The one shape worth marking is an
ALLOWwith no rules, since a workload with anyALLOWpolicy admits only what one of them matches. It carries an amber count in the default-visible Rules cell, with the explanation on the badge itself rather than behind the column header. The Status column defaults to hidden, so this is what an operator sees without opting in.Applies-to now renders.
AuthorizationPolicyCellhad noselectorcase, so the column showed-for every policy. Its accessor also read onlyspec.selector.matchLabels, which is wrong twice: a policy attached throughtargetRef/targetRefs— as waypoints and ambient Gateways are — has no selector at all, and an empty selector in the mesh root namespace applies mesh-wide, not namespace-wide. Attachments render askind/name; with nothing declared the cell names both possible scopes, because the root namespace is configurable and Radar cannot readMeshConfig.rootNamespacefrom this object.The drawer's rule banners describe what the policy contributes, not an outcome for the workload:
DENYwith no rules → informational: it matches no requests and denies no traffic; other policies may still deny.ALLOWwith no rules → warning: it permits nothing on its own; otherALLOWpolicies may still permit. A default-deny-plus-exceptions setup looks exactly like this.DENYwhose rule carries no conditions → informational: it matches everything it selects, and sinceDENYis evaluated beforeALLOW, nothing can carve an exception from it.A rule counts as unconditional when
from,toandwhenare all absent or empty — serialized empty lists impose nothing. An unrecognised field is treated as a condition, so a future matcher is never misread as the absence of one.DestinationRule
Status keeps the one failure the object can show — a rule with no host applies to nothing — and reports everything else as not assessed, rather than restating the subset count or calling a declared traffic policy
Configured. It also defaults to hidden.A Client TLS column shows the declared mode, because what it produces depends on an object this row cannot see.
DISABLEis "do not setup a TLS connection to the upstream endpoint"; against aPERMISSIVEPeerAuthentication ("either plaintext or mTLS tunnel") the traffic simply goes unencrypted, against aSTRICTone ("an mTLS tunnel… must be presented") the requests fail. All values are neutral, and subset and port policies can override the mode besides.Reviewer notes
The column key is
tlsMode, nottls: Traefik's IngressRoute already ownstls, and that key sits inSKIP_FILTER_COLUMNS, which would have left this column's filter permanently empty.Both the new and the repaired columns have
getCellFilterValuebranches — the generic fallback cannot reachspec.trafficPolicy.tls.mode, and the existing selector branch served only Istio Gateways — so the filter dropdown offers the values the table is showing.Scope
PeerAuthentication is deliberately untouched. Its findings need a verified
MeshConfig.rootNamespaceto distinguish a namespace default from a mesh default, and to tell whether a selector renders a root-namespace policy inert. That is data the frontend does not have, and inferring it from the conventional namespace name is the guess this PR removes elsewhere.Testing
make tscclean.packages/k8s-ui: 3456 passed, 1 skipped — the one failing file (resource-utils-hpa.test.ts) fails identically onmain, a fixture path resolved from the runner's cwd.26 tests where these accessors had none: the deny-all idiom and its explicit-empty-array twin,
DENYandALLOWinertness, blanketDENYwritten as{}or as empty condition lists, an unrecognised rule field,AUDIT/CUSTOM, unknown actions,targetRefandtargetRefsattachment, the both-scopes literal, subset-count and traffic-policy restatement, and everytls.modecase.Visually verified at 1920 against a cluster with the Istio CRDs installed and fixtures covering each path — Radar renders from the API objects, so no control plane is needed.
AuthorizationPolicy: a
spec: {}policy shows a neutralALLOWbeside an amber0, and aDENYwith real rules shows a neutralDENY. The Applies-to column rendersGateway/waypointfor atargetRefsattachment andNamespace / mesh scopewhere nothing is declared, on a column that previously rendered-for every policy. The amber badge fits thew-20column without overflow and carries dark-mode variants.DestinationRule: Client TLS renders
ISTIO_MUTUAL,DISABLEand-; a rule with two subsets shows the count without a green badge restating it. Table minimum width is 969px, so the added column introduces no horizontal scroll at 1280.Long values in Applies-to truncate at
w-48and are recoverable on hover, consistent with the other text columns.Semantics are quoted from the Istio authorization-policy reference, the security-concepts page, and the
destination_rule/peer_authenticationprotos.🤖 Generated with Claude Code
Note
Medium Risk
Changes how operators interpret Istio security policy tables and detail banners; semantics are safer but could surprise users accustomed to the old green/red cues.
Overview
AuthorizationPolicy rows and detail views no longer use green/red action badges or status text that implied a workload was permitted or blocked. Actions render neutral; only ALLOW with zero rules (Istio’s deny-all idiom) gets a degraded status and an amber 0 rules badge with tooltip. The Applies to column actually renders scope via
getAuthorizationPolicySelectorString(targetRef/targetRefs, labels, or “Namespace / mesh scope”), and drawer banners come fromgetAuthorizationPolicyRuleNotice—fixing the old “Deny All” on empty DENY and overstated “Allow Nothing” messaging.DestinationRule status no longer treats subset count or traffic policy as healthy; only missing host is unhealthy, otherwise Not assessed. A new Client TLS column shows declared
trafficPolicy.tls.modeneutrally, with column tooltips and Status hidden by default on both resource types.Filter dropdowns now match table cells for auth policy selector and destination rule
tlsMode.istio-policy-accessors.test.tslocks the new accessor semantics.Reviewed by Cursor Bugbot for commit a184b24. Bugbot is set up for automated code reviews on this repo. Configure here.