feat(reachability): Network-path diagnosis#1037
Conversation
7bbc3ba to
f76a8ce
Compare
0be6378 to
ea9014c
Compare
ea9014c to
e66ff7b
Compare
The probePortServiceMismatch check false-condemned two common healthy patterns: a dedicated health/admin port, and mesh-injected sidecars (e.g. istio-proxy :15021). Even gated on no-ready-endpoints it was too aggressive. Remove it and its helpers, and stop enumerating a Service's ports in the gateway-backend missing-port message. Claude-Session: https://claude.ai/code/session_01EgqPkGNXcvYWz2SUmXRQ2z
- Collapse the shipped verdict to a single coverage-honest value stored on t.Verdict and read by REST, UI, and MCP alike. The in-cluster re-derivation guard only preserves a genuine special-shape unknown (UnknownClass set), so the in-cluster test can still upgrade an apiserver-only trace to healthy. - Diagnose the network path, not app health: any HTTP response (including an app 5xx) reads as reached; only a proxy's own 502/504 (upstream unreachable) or the apiserver failing to reach the backend is a network fault. classifyProxy5xx keeps the laptop and in-cluster vantages in agreement. - worstOutcome now names the failing layer (failedLayer: tcp/tls/http, or "upstream" for a 502/504) so a degraded route says exactly what broke. A valid cert that only expires soon stays reachable (advisory in the detail). - Structured SkipClass on probe results, preferred over reason-text matching. - Copyable reproducers for skips that couldn't self-verify (openssl s_client for a no-SNI TLS listener, grpcurl for a gRPC port; -insecure on TLS gRPC). - A Gateway is condemned unreachable only when every listener was testable. - SSRF guard also denies non-link-local cloud-metadata IPs; the in-cluster probe target is validated (host:port) and its path cleaned. Claude-Session: https://claude.ai/code/session_01EgqPkGNXcvYWz2SUmXRQ2z
- Route rows are pills (Verified · 200 / Reached · 404 / Not tested) with the reason on a hover tooltip; single diagram view (drop the tree/diagram toggle and ReachabilityTree). - Verdict headlines never over- or under-claim: an apiserver-only reach reads "not confirmed", a 502/504 reads "reached, upstream unreachable" (never "app error, check logs"), a cert failure reads "TLS failure", and each degraded route names the layer that failed via failedLayer. - Theme the error/CTA buttons (were washed-out currentColor); read pod counts from the hop that owns the named culprit; don't invent a second failed host from a same-host probe flap. Claude-Session: https://claude.ai/code/session_01EgqPkGNXcvYWz2SUmXRQ2z
| if hasPort { | ||
| return fqdn + ":" + port | ||
| } | ||
| return fqdn |
There was a problem hiding this comment.
Wrong cross-namespace service DNS
Medium Severity
Cross-namespace in-cluster reachability dials backends as name.namespace.svc:port, but cluster DNS expects name.namespace (or the full name.namespace.svc.cluster.local). The extra .svc segment is not a valid Kubernetes service DNS label, so TCP probes can fail with NXDOMAIN or hit the wrong name while the backend is healthy.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 211460f. Configure here.
Mechanical sweep of em-dash to hyphen on this branch's own added lines (comments and user-facing strings) to match the project style. No logic changes; pre-existing main lines are untouched. Claude-Session: https://claude.ai/code/session_01EgqPkGNXcvYWz2SUmXRQ2z
| if disc := GetResourceDiscovery(); disc != nil { | ||
| if gvr, ok := disc.GetGVRWithGroup("Rollout", "argoproj.io"); ok { | ||
| if dc := GetDynamicResourceCache(); dc != nil && !dc.IsSynced(gvr) { | ||
| return false, true |
There was a problem hiding this comment.
Rollout sync softens real outages
Medium Severity
When a Service has zero ready pods but a matching Deployment or StatefulSet still has replicas greater than zero, the new Rollout cache path can still return uncertain, so detection emits a warning instead of the critical no-ready-endpoints finding until the Rollout informer syncs or the list succeeds.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit efecce5. Configure here.
…gSoon TracePanel (the pre-diagram tab component) is no longer rendered - ReachabilityView replaced it - and it dragged a large dead subgraph (VerdictBanner, PathSection, CoverageSection and their private helpers). Remove the component plus everything only it used; keep the shared helpers/subcomponents ReachabilityView and TraceSummary still import (coverageBannerTone, routeOutcomeRank, ReachActions, FindingRow, ...). Drop TracePanel from the package's public API (unused by Hub). TracePanel.tsx: 2009 -> 461 lines. Also remove certExpiringSoon (pkg/probe): both call sites went away when a valid expiring cert stopped being degraded, leaving it dead in prod. The Go<->TS drift guard is repointed at the missing-ref predicate's new home in reachVerdict.ts. Claude-Session: https://claude.ai/code/session_01EgqPkGNXcvYWz2SUmXRQ2z
The cycleN_fixes_test.go files grouped tests by the review round that produced them - opaque names, incoherent grab-bags. Move each test to the topic file for the code it exercises (ApplyInClusterResults -> incluster_results_test.go [new]; coverage headlines / routesByPort / recountCoverage -> coverage_test.go; probe skips -> probes_test.go; verdict -> trace_test.go; netpol -> netpol_test.go; selectedPods -> entries_test.go) and delete the six cycle files. Pure relocation, no test logic changed; the trace suite's RUN count is unchanged. Claude-Session: https://claude.ai/code/session_01EgqPkGNXcvYWz2SUmXRQ2z
Two files drifted out of gofmt in earlier commits (a struct field alignment, and end-of-line comment columns after the em-dash sweep). No code change. Claude-Session: https://claude.ai/code/session_01EgqPkGNXcvYWz2SUmXRQ2z
The in-cluster test spawns a Job/pod, so add a consent dialog naming the cluster + namespace it lands in, with a per-cluster "don't ask again". Permission is already enforced upstream (the button only renders when the capability SSAR allows), so this is a safety confirm, not authz. Reuses ConfirmDialog and follows the ForceDeleteConfirmDialog wrapper pattern; consent is a pure k8s-ui helper (localStorage, per-cluster keyed) with state + dialog wired in web/. Claude-Session: https://claude.ai/code/session_01EgqPkGNXcvYWz2SUmXRQ2z
The Pods hop showed only "N of M ready" plus one culprit. Add a per-pod grid in the Pods node panel: each pod's Kubernetes readiness and whether it answered when knocked on directly (per-node reachability), so the operator sees WHICH pod is broken and which shape - crashing (not-ready, "not tested") vs ready-but-not- serving (Ready, but the probe refused). Surfaces the per-pod probe that already runs (apiserver = indirect / in-cluster = real, labeled honestly); no new probing, no Job runner. Backend emits a PodStatus roster: culprits (not-ready) are always included so they are never sampled out, ready pods are capped to the probe sample, and PodTotal drives an honest "showing N of total". Verified on kind across both vantages, sampling, the amber ready-but-unreachable case, a mixed culprit+ready fleet, and pod-nav. Claude-Session: https://claude.ai/code/session_01EgqPkGNXcvYWz2SUmXRQ2z
| // Optional operator-chosen HTTP path (e.g. /healthz) for the L7 probes; | ||
| // defaults to "/" inside the trace package. | ||
| ProbePath: r.URL.Query().Get("path"), | ||
| } |
There was a problem hiding this comment.
Unsanitized HTTP probe paths
Low Severity
The trace API accepts a custom HTTP probe path via ?path= and the in-cluster trace body, but unlike handleProbeInCluster it never normalizes that value with path.Clean. httpPath only trims and adds a leading slash, so segments like .. can reach the probe Job and proxy requests as a different path than the operator intended.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 4280fb3. Configure here.
The passive trace's ?path= (and the in-cluster trace body path) flowed through httpPath, which trimmed and added a leading slash but never path.Clean'd it, so a segment like .. reached the L7 probe as a different path than the operator intended. Clean it centrally in httpPath, mirroring handleProbeInCluster. Claude-Session: https://claude.ai/code/session_01EgqPkGNXcvYWz2SUmXRQ2z
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 4 total unresolved issues (including 3 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7785c6a. Configure here.
… branch) The ready==0 scale-to-zero branch said "Deployment/StatefulSet" while scaledToZeroBackingWorkload also matches Argo Rollouts (the selected==0 branch already says so). Align the two so a Rollout-backed Service reads accurately. Claude-Session: https://claude.ai/code/session_01EgqPkGNXcvYWz2SUmXRQ2z
…ng them A per-route in-cluster failure (Job couldn't start / timed out / RBAC) returns HTTP 200 with an error status + copyable fallbackCommand inside inClusterTests, but the hook destructured only `trace` and dropped it - and with no in-cluster result the matrix collapsed to one column and self-hid, so the failure vanished as if nothing ran. Capture it and surface a warning AlertBanner with the reason plus the copyable kubectl command to run the probe by hand. Claude-Session: https://claude.ai/code/session_01EgqPkGNXcvYWz2SUmXRQ2z


Reachability · Network Path — honest, DevOps-grade path diagnosis
TL;DR
A new Reachability view for the kinds that carry traffic —
Service,Ingress,HTTPRoute,GRPCRoute,Gateway— that answers one operator question: "can traffic get to my app, and if not, exactly where and why does it break?" It reads like a DevOps engineer built it for someone who isn't a Kubernetes expert, and its defining constraint is honesty: it never says "broken" when traffic flows, and never says "healthy/reachable" for something it couldn't actually verify.Two modes: Config check (passive, zero traffic - reads the declared path + config breaks) and Live test (on demand - DNS/TCP/TLS/HTTP probes, reported per-route and per-vantage). Surfaces: a full Reachability tab (a node/edge Diagram + a per-route/per-vantage matrix), a passive drawer glance, and the MCP
diagnosetool extended to network kinds.How to review this (suggested reading order)
It's a large diff but it's one feature with a clear spine. Read in this order:
internal/trace/coverage.go— the honesty spine.RouteResult{Outcome, Confidence},CoverageVerdict,CoverageHeadline. Understandrealvsindirectconfidence here and the rest follows.pkg/probe/probe.go— the probe primitives + the two vantages (real path vs apiserver-proxy) +proxyResult/isClusterUnreachable(couldn't-reach ≠ unreachable).internal/trace/trace.go—computeVerdict/reviseVerdictWithProbes(how findings + probes become healthy/degraded/broken/unknown).internal/trace/entries.go— how each subject's static path graph (hops + findings) is built.packages/k8s-ui/src/components/trace/reachVerdict.ts(operator-facing verdict),traceToSubgraph.ts(node/edge coloring),TraceSummary.tsx(drawer glance).What to scrutinize: the honesty invariants below — each is load-bearing and each has a test that pins it. If you can find a case that false-condemns (says broken when traffic flows) or overclaims (says reachable/healthy without real verification), that's the bug that matters most here.
High-level architecture
The two ideas that do the heavy lifting
confidence. Every probe is tagged by how it learned a result. real = tested the way traffic flows (direct dial / in-cluster). indirect = reached only via the API-server proxy — it localizes a problem but never sets the headline and never condemns the real path (the proxy bypasses NetworkPolicy and can fail for vantage-only reasons). The one exception: an authoritative cache fact (e.g. 0 ready endpoints) is definitive regardless of vantage and is promoted to a real failure.Honesty invariants (each pinned by a test)
trace.gohopReachSeverity;traceToSubgraph.tsTestComputeVerdict_PartialReadyBackendIsDegradedNotBroken; traceToSubgraph "partial-ready backend → amber"coverage.goupgradeDefinitiveBackendDownTestUpgradeDefinitiveBackendDown(incl. multi-port + uncertain-warning-stays-soft)pkg/probe/probe.goisClusterUnreachableTestIsClusterUnreachabletrace.goanyProbeReachedTestAnyProbeReached_FailedProbeIsNotReachedcoverage.gocoverageBannerTone,singleRouteHeadlinereachVerdict.tsbackendDowncoverage.go,TraceSummary.tsxtraceToSubgraph.tsRefinements from review
Later review rounds tightened the honesty model (all test-pinned):
verdictis collapsed to a single coverage-honest value stored on the trace and read identically by REST, the UI, and MCP - no per-surface drift. The in-cluster test still upgrades an apiserver-only ("unknown") trace to real once it confirms the data path.failedLayer(tcp/tls/http, orupstreamfor a 502/504) instead of a generic "server error".Verified · 200/Reached · 404/Not tested) with the reason on a hover tooltip; a single diagram view (the tree/diagram toggle andReachabilityTreewere removed).Screenshots
2. Coverage matrix + the honest verdict. Each route ×
FROM RADAR · YOUR MACHINE/VIA API SERVER/IN-CLUSTER, with per-route pills (Verified · 200). Reached only via the API-server proxy is never a confident green - the verdict reads "Backend answered via API server - the real network path isn't confirmed".3. Per-port truth. A multi-port Service is reported per port -
:80verified,:9090unreachable (a targetPort mismatch - nothing listens on 9090) - instead of collapsing to one verdict.4. HTTP 404 is reached, not broken. We diagnose the network path, not app health: any HTTP response means traffic reached the app, so a 404 reads green-dashed "reached", never a false "unreachable".
5. Resource drawer. The passive glance in the resource drawer surfaces the operational issue ("No endpoints - 0/1 ready") with one CTA into the live test. It never nags.
File map (80 files)
internal/trace/(24) — the engine:trace.go(verdict),coverage.go(route projection),entries.go(path graph),probes.go(probe orchestration),findings.go,netpol.go(NetworkPolicy evaluator),ingress_controller.go(controller tier),egress.go. ThecycleN_fixes_test.gofiles are test-only regression suites from review rounds.pkg/probe/(2) — probe primitives (DNS/TCP/TLS/HTTP, apiserver-proxy) + classification.internal/reachability/(4) — the in-cluster probe runner (short-lived Jobs).packages/k8s-ui/src/components/trace/(21) - the UI:ReachabilityView(the node/edge diagram),ReachabilityExplainer(per-route/per-vantage matrix + pills),reachVerdict,traceToSubgraph,TraceSummary(drawer glance),probe-display. (ReachabilityTreewas removed - single diagram view;TracePanelnow provides shared helpers, not the rendered tab.)internal/mcp/(5),internal/server/(4) — MCPdiagnoseextension + the/api/traceendpoint.internal/k8s/(6) — cache wiring for the kinds the trace reads.web/,cmd/,docs/,deploy/helm, README — wiring, flags, docs.Test plan
go test ./...(incl.internal/trace,pkg/probe) — green.tscclean, full vitest suite green (incl.reachVerdict/traceToSubgraph/TraceSummary/probe-display).Known limitations (deliberately deferred)
Surfaced by an independent critic pass and consciously left for a follow-up — each would be net-negative or out-of-scope to fix now:
NetworkPolicyonly. A path a CNI CRD policy allows could still show an amber "a rule would block traffic". Not capped: core NP is enforced on those CNIs too, so suppressing it would under-report a real deny — and the finding is hedged + clearable by the in-cluster test (subject to the real CNI).IsNotFoundduring informer sync. A just-applied or cross-scope backend can briefly read as a missing-ref break until the cache syncs. Narrow + self-healing; a scope-aware guard risks a worse false-clear.Scope / risk notes
create jobs+list/get pods.GET; per-request headers deferred).Note
High Risk
Adds mutating in-cluster probe Jobs, broad REST/MCP surface, and verdict logic where proxy-only failures must not false-condemn paths—security and honesty regressions are the main risk.
Overview
Introduces network path diagnosis for
Service,Ingress,HTTPRoute,GRPCRoute, andGateway: a static hop chain (upstream / subject / downstream) built from the informer cache, with existing detections attached per hop and coverage-style verdicts that distinguish real datapath results from indirect API-server-proxy probes.API & MCP:
GET /api/trace/{kind}/{ns}/{name}with optional?probe=true(andpath); POST routes for single-target and whole-subject in-cluster tests plus a capability check. MCPdiagnosegains the same shape for network kinds (probe, mutatinginCluster), with Cloud Member gating aligned to REST.Probing: New
radar probesubcommand (pkg/probe) andinternal/reachabilityrunner that creates restricted, short-lived Jobs under the caller’s RBAC; image resolution via--reachability-image/ config, self-read (MY_POD_NAME),RADAR_IMAGE(Helm), or version-tagged default.make kind-load-probesupports local kind dev.Detection tweaks: Scale-to-zero recognition extended to Argo Rollouts (with
ErrDynamicNotReadyfor ambiguous dynamic cache state); Service “0 ready” stays critical when Rollout lookup is uncertain.Reviewed by Cursor Bugbot for commit d1face5. Bugbot is set up for automated code reviews on this repo. Configure here.