Skip to content

fix(istio): stop policy rows asserting a security posture no single object establishes - #1649

Merged
nadaverell merged 8 commits into
mainfrom
fix/istio-policy-rows-stop-asserting-posture
Sep 6, 2026
Merged

nadaverell merged 8 commits into
mainfrom
fix/istio-policy-rows-stop-asserting-posture

Conversation

@nadaverell

@nadaverell nadaverell commented Sep 6, 2026 •

Copy link
Copy Markdown
Contributor

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, then DENY, then ALLOW — 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 to ALLOW, rules are alternatives, and "if not set, the match will never occur… equivalent to setting a default of deny." Radar rendered it green. Meanwhile a DENY policy doing its job rendered red, and a DENY with 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 ALLOW with no rules, since a workload with any ALLOW policy 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. AuthorizationPolicyCell had no selector case, so the column showed - for every policy. Its accessor also read only spec.selector.matchLabels, which is wrong twice: a policy attached through targetRef/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 as kind/name; with nothing declared the cell names both possible scopes, because the root namespace is configurable and Radar cannot read MeshConfig.rootNamespace from this object.

The drawer's rule banners describe what the policy contributes, not an outcome for the workload:

  • DENY with no rules → informational: it matches no requests and denies no traffic; other policies may still deny.
  • ALLOW with no rules → warning: it permits nothing on its own; other ALLOW policies may still permit. A default-deny-plus-exceptions setup looks exactly like this.
  • DENY whose rule carries no conditions → informational: it matches everything it selects, and since DENY is evaluated before ALLOW, nothing can carve an exception from it.

A rule counts as unconditional when from, to and when are 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. DISABLE is "do not setup a TLS connection to the upstream endpoint"; against a PERMISSIVE PeerAuthentication ("either plaintext or mTLS tunnel") the traffic simply goes unencrypted, against a STRICT one ("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, not tls: Traefik's IngressRoute already owns tls, and that key sits in SKIP_FILTER_COLUMNS, which would have left this column's filter permanently empty.

Both the new and the repaired columns have getCellFilterValue branches — the generic fallback cannot reach spec.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.rootNamespace to 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 tsc clean. packages/k8s-ui: 3456 passed, 1 skipped — the one failing file (resource-utils-hpa.test.ts) fails identically on main, 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, DENY and ALLOW inertness, blanket DENY written as {} or as empty condition lists, an unrecognised rule field, AUDIT/CUSTOM, unknown actions, targetRef and targetRefs attachment, the both-scopes literal, subset-count and traffic-policy restatement, and every tls.mode case.

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 neutral ALLOW beside an amber 0, and a DENY with real rules shows a neutral DENY. The Applies-to column renders Gateway/waypoint for a targetRefs attachment and Namespace / mesh scope where nothing is declared, on a column that previously rendered - for every policy. The amber badge fits the w-20 column without overflow and carries dark-mode variants.

DestinationRule: Client TLS renders ISTIO_MUTUAL, DISABLE and -; 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-48 and 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_authentication protos.

🤖 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 from getAuthorizationPolicyRuleNotice—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.mode neutrally, 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.ts locks the new accessor semantics.

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

…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.
@nadaverell
nadaverell requested a review from hisco as a code owner September 6, 2026 19:30
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Correct Istio policy status and scope semantics

🐞 Bug fix ✨ Enhancement 🧪 Tests 🕐 20-40 Minutes

Grey Divider

AI Description

• Render AuthorizationPolicy actions neutrally and explain empty or unconditional rule behavior.
• Show policy selectors, target attachments, and inherited scope in the Applies-to column.
• Stop inferring DestinationRule health; expose declared client TLS mode with focused tests.
Diagram

graph TD
  AP["Authorization Policy"] --> Utils["Istio utilities"] --> Cells["Table cells"] --> Table["Resource table"]
  DR["Destination Rule"] --> Utils --> Drawer["Policy drawer"]
  Tests["Accessor tests"] --> Utils
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Compute effective workload posture
  • ➕ Could present an actual authorization verdict instead of object-level declarations.
  • ➕ Could identify interactions across ALLOW, DENY, CUSTOM, and attachment scopes.
  • ➖ Requires resolving all policies, workloads, namespaces, mesh configuration, and evaluation precedence.
  • ➖ Does not fit reliably within an individual resource row.
  • ➖ Introduces substantially greater complexity and risk than correcting misleading object-level status.

Recommendation: Use the PR’s neutral, declaration-focused presentation for individual policy rows. Effective posture computation is valuable as a separate workload-level analysis feature, but individual resources lack enough context to support that verdict accurately.

Files changed (5) +373 / -64

Enhancement (1) +9 / -8
ResourcesView.tsxClarify Istio columns and expose client TLS declarations +9/-8

Clarify Istio columns and expose client TLS declarations

• Hides unassessed status columns by default, renames AuthorizationPolicy selector to Applies to, and adds semantic tooltips. Adds a DestinationRule Client TLS column while clarifying that subsets and traffic policies do not establish effective posture.

packages/k8s-ui/src/components/resources/ResourcesView.tsx

Bug fix (3) +174 / -56
IstioAuthorizationPolicyRenderer.tsxReplace misleading policy banners with shared notices +13/-19

Replace misleading policy banners with shared notices

• Makes all action badges neutral and delegates rule-list explanations to the shared notice helper. Removes incorrect claims that an empty DENY blocks all traffic or an empty ALLOW proves a workload outage.

packages/k8s-ui/src/components/resources/renderers/IstioAuthorizationPolicyRenderer.tsx

istio-cells.tsxRender neutral policy actions, scopes, warnings, and TLS +40/-6

Render neutral policy actions, scopes, warnings, and TLS

• Adds DestinationRule client TLS rendering and a functional AuthorizationPolicy Applies-to cell. Policy actions become neutral, while rule-less ALLOW policies receive an amber count with an explanatory tooltip.

packages/k8s-ui/src/components/resources/renderers/istio-cells.tsx

resource-utils-istio.tsCorrect Istio status, scope, and rule interpretation +121/-31

Correct Istio status, scope, and rule interpretation

• Stops treating configured DestinationRules and policy actions as health verdicts, while preserving missing-host failure detection. Adds client TLS extraction, targetRef-aware scope formatting, and centralized notices for empty or unconditional AuthorizationPolicy rules.

packages/k8s-ui/src/components/resources/resource-utils-istio.ts

Tests (1) +190 / -0
istio-policy-accessors.test.tsCover Istio policy semantics and presentation states +190/-0

Cover Istio policy semantics and presentation states

• Adds tests for neutral AuthorizationPolicy actions, empty and unconditional rules, target attachments, inherited scope, DestinationRule status, and declared TLS modes. The cases prevent object-level declarations from being presented as effective mesh posture.

packages/k8s-ui/src/components/resources/istio-policy-accessors.test.ts

@qodo-code-review

qodo-code-review Bot commented Sep 6, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Status docs preserve prior output logic ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
The getDestinationRuleStatus documentation says restating the subset count said nothing,
describing output that this change removes. Maintainers of the accessor therefore inherit a diff
narrative instead of a timeless explanation that only a missing host can be assessed locally.
Code

packages/k8s-ui/src/components/resources/resource-utils-istio.ts[R129-132]

+ * A rule without a host applies to nothing. Everything else it declares —
+ * subsets, a traffic policy — describes configuration, and restating the count
+ * of subsets already shown two columns over said nothing about whether the
+ * host is reachable or the policy is in effect.
Relevance

●●● Strong

Recent precedent accepts removing superseded behavior from documentation comments.

PR-#1642

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Compliance rule 3036538 forbids explicit change history in comments. The documentation recounts a
removed status presentation by referring to the subset count that was previously restated.

Rule 3036538: Disallow references to tickets or PR history in code comments
packages/k8s-ui/src/components/resources/resource-utils-istio.ts[129-132]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The added documentation describes why the former subset-count status output was unhelpful.

## Issue Context
Document the current assessment boundary directly: a missing host is locally observable, while reachability and policy effectiveness are not.

## Fix Focus Areas
- packages/k8s-ui/src/components/resources/resource-utils-istio.ts[126-132]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Tests preserve old selector behavior ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
The target attachment test comment says reading only the selector called these namespace-wide,
documenting behavior from before the accessor change. Future test maintainers encounter that
historical implementation detail even though the attachment constraint can be explained as a
present-tense semantic distinction.
Code

packages/k8s-ui/src/components/resources/istio-policy-accessors.test.ts[R55-57]

+  it('reports a targetRef attachment instead of claiming namespace scope', () => {
+    // Waypoint and Gateway attachment in ambient mode uses targetRefs, not
+    // labels; reading only the selector called these namespace-wide.
Relevance

●●● Strong

Recent precedent accepts rewriting comments from implementation history into timeless semantic
explanations.

PR-#1642

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Compliance rule 3036538 prohibits explicit prior-behavior history in comments. The cited test
comment directly states what the selector-only implementation previously called these policies.

Rule 3036538: Disallow references to tickets or PR history in code comments
packages/k8s-ui/src/components/resources/istio-policy-accessors.test.ts[55-57]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The test comment records how the former selector-only implementation classified target attachments.

## Issue Context
Explain that target attachments are distinct from namespace scope without referring to the replaced implementation.

## Fix Focus Areas
- packages/k8s-ui/src/components/resources/istio-policy-accessors.test.ts[55-57]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Tests preserve old color behavior ⊘ Outdated 📘 Rule violation ⚙ Maintainability
Description
The action badge test comment says coloring actions green and red made rows communicate incorrect
outcomes, preserving the behavior replaced by this change. The note remains in the test whenever its
invariant is maintained, although the reason for uniform tones can be stated without recounting the
former presentation.
Code

packages/k8s-ui/src/components/resources/istio-policy-accessors.test.ts[R132-135]

+    // The action is a declaration, not a verdict. Colouring ALLOW green made a
+    // deny-all policy the greenest row on the page; colouring DENY red made a
+    // control doing its job look like a failure. This is also the badge that
+    // stays visible when the Status column is off by default.
Relevance

●●● Strong

Recent precedent accepts removing past-rendering history from newly added comments.

PR-#1642

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Compliance rule 3036538 disallows explicit change history in added comments. The cited lines
describe the consequences of the prior green and red badge behavior in past tense.

Rule 3036538: Disallow references to tickets or PR history in code comments
packages/k8s-ui/src/components/resources/istio-policy-accessors.test.ts[132-135]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The test comment recounts the effects of the superseded action badge colors.

## Issue Context
Keep the explanation that action declarations are not health verdicts, but remove the historical rendering narrative.

## Fix Focus Areas
- packages/k8s-ui/src/components/resources/istio-policy-accessors.test.ts[132-135]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View medium (7)
4. Policy scope cannot be filtered ✓ Resolved 🐞 Bug ≡ Correctness
Description
The new selector cell displays getAuthorizationPolicySelectorString(resource), but the existing
filter branch returns a value only for Istio Gateways and returns empty for AuthorizationPolicies.
Once the repaired Applies-to column shows labels or target attachments, none of those visible values
can populate or match its table filter.
Code

packages/k8s-ui/src/components/resources/renderers/istio-cells.tsx[R225-230]

+    case 'selector': {
+      const scope = getAuthorizationPolicySelectorString(resource)
+      return (
+        <Tooltip content={scope}>
+          <span className="text-sm text-theme-text-secondary truncate block">{scope}</span>
+        </Tooltip>
Relevance

●●● Strong

Accepted precedent requires curated displayed text to have a corresponding filter accessor branch.

PR-#1369

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The new cell visibly renders the policy scope, but the common filter path explicitly handles only
Istio Gateways for the same column key.

packages/k8s-ui/src/components/resources/renderers/istio-cells.tsx[225-230]
packages/k8s-ui/src/components/resources/resource-utils.ts[2321-2323]
packages/k8s-ui/src/components/resources/ResourcesView.tsx[5190-5196]
packages/k8s-ui/src/components/resources/ResourcesView.tsx[5571-5576]
PR-#1369

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
AuthorizationPolicy Applies-to cells now display derived selectors and target attachments, but the shared selector filter accessor still excludes this resource kind.

## Issue Context
The `selector` accessor currently handles only Istio Gateways, so AuthorizationPolicy values are always empty during filter option collection and matching.

## Fix Focus Areas
- packages/k8s-ui/src/components/resources/resource-utils.ts[2321-2323]
- packages/k8s-ui/src/components/resources/resource-utils-istio.ts[396-415]
- packages/k8s-ui/src/components/resources/istio-policy-accessors.test.ts[54-95]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


5. Encryption modes cannot be filtered ✓ Resolved 🐞 Bug ≡ Correctness
Description
The new tls column renders getDestinationRuleTlsMode(resource), but getCellFilterValue has no
matching branch and its generic fallback cannot read spec.trafficPolicy.tls.mode. Any
DestinationRule table containing declared modes therefore omits them from the filter dropdown and
cannot filter rows by the values visibly shown.
Code

packages/k8s-ui/src/components/resources/ResourcesView.tsx[2096]

+    { key: 'tls', label: 'Client TLS', width: 'w-28', tooltip: 'Declared client-side TLS mode for this host at the rule level. Subset and port policies can override it; this does not establish the effective mTLS posture.' },
Relevance

●●● Strong

Accepted precedent requires displayed values to be wired into the shared filter accessor.

PR-#1369

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The cell reads the nested TLS mode, while filter collection and matching exclusively use an accessor
whose fallback skips that path and returns empty for objects.

packages/k8s-ui/src/components/resources/renderers/istio-cells.tsx[97-101]
packages/k8s-ui/src/components/resources/resource-utils-istio.ts[165-167]
packages/k8s-ui/src/components/resources/resource-utils.ts[2456-2463]
packages/k8s-ui/src/components/resources/ResourcesView.tsx[5571-5576]
PR-#1369

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new DestinationRule Client TLS column displays a nested derived value, but the shared table filter accessor returns an empty string for it.

## Issue Context
`getDestinationRuleTlsMode` reads `spec.trafficPolicy.tls.mode`, while the generic accessor only checks direct status, spec, and resource properties.

## Fix Focus Areas
- packages/k8s-ui/src/components/resources/resource-utils.ts[2172-2463]
- packages/k8s-ui/src/components/resources/resource-utils-istio.ts[165-167]
- packages/k8s-ui/src/components/resources/istio-policy-accessors.test.ts[113-128]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


6. Selector docs preserve prior behavior ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
The getAuthorizationPolicySelectorString documentation says reading only the selector reported
attached policies as namespace-scoped, recording the accessor behavior this change replaces. Anyone
modifying scope resolution encounters that history even though the distinction between selectors,
attachments, and inherited scope can be explained entirely in present tense.
Code

packages/k8s-ui/src/components/resources/resource-utils-istio.ts[R388-391]

+ * A policy can attach by targetRef instead of by labels — waypoints and
+ * Gateways in ambient mode do — and reading only the selector reported those as
+ * scoped to the namespace, which is a different and much wider claim. With
+ * neither, scope is inherited: the namespace, or the whole mesh when the policy
Relevance

●●● Strong

Recent precedent accepts rewriting accessor documentation around current invariants rather than
prior behavior.

PR-#1642

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Compliance rule 3036538 disallows comments that preserve explicit prior implementation behavior. The
cited documentation states in past tense that selector-only reading reported attached policies as
namespace-scoped.

Rule 3036538: Disallow references to tickets or PR history in code comments
packages/k8s-ui/src/components/resources/resource-utils-istio.ts[388-391]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The added documentation records how the previous selector-only implementation classified attached policies.

## Issue Context
Preserve the semantic warning that attachments are narrower than inherited namespace scope, but phrase it as a current invariant.

## Fix Focus Areas
- packages/k8s-ui/src/components/resources/resource-utils-istio.ts[385-394]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


7. Maintainers inherit old color notes ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
The comment above AUTHORIZATION_POLICY_ACTION_SEVERITY says the former green and red tones made
policy rows communicate incorrect outcomes, recording the implementation this mapping replaces.
Readers of the shared cell renderer therefore receive change history where a present-tense
explanation of why all actions are neutral would suffice.
Code

packages/k8s-ui/src/components/resources/renderers/istio-cells.tsx[R39-41]

+  // Deliberately uniform: an action is a declaration, not a verdict. Colouring
+  // ALLOW green and DENY red made a deny-all policy the greenest row on the
+  // page, and a DENY doing its job look like a failure.
Relevance

●●● Strong

Recent precedent accepts rewriting shared renderer comments to avoid implementation-history wording.

PR-#1642

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Compliance rule 3036538 disallows explicit change history in comments. These lines recount the
effects of the former color mapping rather than only documenting the current invariant.

Rule 3036538: Disallow references to tickets or PR history in code comments
packages/k8s-ui/src/components/resources/renderers/istio-cells.tsx[39-41]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The added comment describes the outcome of the color mapping that this PR supersedes.

## Issue Context
Retain the semantic reason for using neutral action tones without documenting the prior rendering.

## Fix Focus Areas
- packages/k8s-ui/src/components/resources/renderers/istio-cells.tsx[39-41]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


8. Maintainers inherit old rendering notes ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
The comment above actionSeverity says the former green and red rendering made policies appear
successful or failed, embedding superseded behavior beside the current mapping. Anyone maintaining
the renderer encounters this change history even though the neutral-tone rationale can be expressed
without describing the previous implementation.
Code

packages/k8s-ui/src/components/resources/renderers/IstioAuthorizationPolicyRenderer.tsx[R12-14]

+  // Deliberately uniform: an action is a declaration, not a verdict. Colouring
+  // ALLOW green and DENY red made a deny-all policy the greenest row on the
+  // page, and a DENY doing its job look like a failure.
Relevance

●●● Strong

Recent precedent accepts replacing old behavior narratives with present-tense rationale.

PR-#1642

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Compliance rule 3036538 prohibits added comments containing explicit change history. The cited
comment describes the superseded green and red rendering in past tense.

Rule 3036538: Disallow references to tickets or PR history in code comments
packages/k8s-ui/src/components/resources/renderers/IstioAuthorizationPolicyRenderer.tsx[12-14]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The added comment records the behavior and consequences of the rendering that this change replaced.

## Issue Context
Preserve the reason for neutral tones, but express it as a timeless invariant rather than implementation history.

## Fix Focus Areas
- packages/k8s-ui/src/components/resources/renderers/IstioAuthorizationPolicyRenderer.tsx[12-14]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


9. Configured balancing looks unset ✓ Resolved 🐞 Bug ≡ Correctness
Description
The loadBalancer header tooltip says no algorithm is set even though it is attached to the column
globally rather than only to cells displaying -. Hovering the header above a rule whose cell shows
ROUND_ROBIN or ConsistentHash therefore gives operators the opposite explanation from that
rule's configuration.
Code

packages/k8s-ui/src/components/resources/ResourcesView.tsx[2097]

+    { key: 'loadBalancer', label: 'LB Policy', width: 'w-28', tooltip: "No algorithm set on this rule — Istio applies its mesh default (LEAST_REQUEST since Istio 1.14, overridable per mesh). Subset and port policies can differ." },
Relevance

●● Moderate

Tooltip scope is subjective; no closely matching historical acceptance or rejection precedent was
found.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The tooltip is rendered on the shared header, while the cell accessor explicitly returns configured
algorithms for some rows.

packages/k8s-ui/src/components/resources/ResourcesView.tsx[2093-2098]
packages/k8s-ui/src/components/resources/ResourcesView.tsx[6409-6415]
packages/k8s-ui/src/components/resources/resource-utils-istio.ts[178-183]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The load-balancer column header categorically describes the unset state, including when rows in the column contain an explicitly configured algorithm.

## Issue Context
Column tooltips are rendered once on the header, while `getDestinationRuleLoadBalancer` can return configured algorithms or `-` on a per-row basis.

## Fix Focus Areas
- packages/k8s-ui/src/components/resources/ResourcesView.tsx[2097-2097]
- packages/k8s-ui/src/components/resources/ResourcesView.tsx[6409-6415]
- packages/k8s-ui/src/components/resources/resource-utils-istio.ts[178-183]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


10. Operators miss blanket deny rules ✓ Resolved 🐞 Bug ≡ Correctness
Description
getAuthorizationPolicyRuleNotice recognizes an unconditional rule only when its object has no
keys, even though fields such as from: [], to: [], and when: [] provide no effective
conditions. A DENY containing one of those serialized empty lists still matches every request it
selects, but the drawer returns no blanket-deny notice.
Code

packages/k8s-ui/src/components/resources/resource-utils-istio.ts[R436-439]

+    const matchesEverything = rules.some(
+      (r: any) => r && typeof r === 'object' && Object.keys(r).length === 0,
+    )
+    if (action === 'DENY' && matchesEverything) {
Relevance

●● Moderate

The semantic edge case is plausible, but no closely matching accepted or rejected precedent was
found.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The implementation tests only Object.keys(r).length === 0; Istio documents that an empty rule
always matches and that unset from, to, and when fields allow any corresponding value.

packages/k8s-ui/src/components/resources/resource-utils-istio.ts[432-445]
packages/k8s-ui/src/components/resources/renderers/IstioAuthorizationPolicyRenderer.tsx[74-75]
packages/k8s-ui/src/components/resources/renderers/IstioAuthorizationPolicyRenderer.tsx[105-106]
🌐 Istio states that an empty rule always matches and that unset source, operation, and condition lists allow any corresponding request value.

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The blanket-DENY notice detects only a literally empty rule object and misses rules whose optional repeated condition fields are present as empty arrays.

## Issue Context
Istio treats absent source, operation, and condition lists as unrestricted, and repeated empty lists carry the same effective lack of constraints.

## Fix Focus Areas
- packages/k8s-ui/src/components/resources/resource-utils-istio.ts[432-445]
- packages/k8s-ui/src/components/resources/istio-policy-accessors.test.ts[155-166]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
✅ Compliance rules (platform): 41 rules
✅ Web pages:
  +2 more
Review mode: ⚖️ Balanced: Downgraded extended -> standard: change is below the extended eligibility bar (hunks 15/18, lines 437/200; both must reach the floor). Router rationale: This changes behavior across multiple Istio policy renderers, accessors, status semantics, scope resolution, and TLS display paths, with substantial new logic and many independent edge cases that warrant redundant review.

Grey Divider

Tip of the day
💡 Did you know, you can show, collapse, or hide each part of a finding: code, evidence, and all

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread packages/k8s-ui/src/components/resources/renderers/istio-cells.tsx Outdated
Comment thread packages/k8s-ui/src/components/resources/istio-policy-accessors.test.ts Outdated
Comment thread packages/k8s-ui/src/components/resources/resource-utils-istio.ts Outdated
Comment thread packages/k8s-ui/src/components/resources/resource-utils-istio.ts Outdated
Comment thread packages/k8s-ui/src/components/resources/ResourcesView.tsx Outdated
Comment thread packages/k8s-ui/src/components/resources/renderers/istio-cells.tsx
Comment thread packages/k8s-ui/src/components/resources/ResourcesView.tsx Outdated
Comment thread packages/k8s-ui/src/components/resources/resource-utils-istio.ts Outdated
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.
@nadaverell nadaverell changed the title fix(istio): stop policy rows asserting a posture no single object establishes fix(istio): stop policy rows asserting a security posture no single object establishes Sep 6, 2026
…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.
@nadaverell
nadaverell merged commit c38475e into main Sep 6, 2026
9 checks passed
@nadaverell
nadaverell deleted the fix/istio-policy-rows-stop-asserting-posture branch September 6, 2026 22:48
nadaverell added a commit that referenced this pull request Sep 7, 2026
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 -->
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 -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant