Repository navigation
Name the NetworkPolicy that denies Radar its Prometheus candidate - #1737
Merged
Merged
Conversation
PR Summary by QodoAttribute Prometheus failures to blocking NetworkPolicies
AI Description
Diagram
High-Level Assessment
Files changed (13)
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit bb1af2a. Configure here.
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+43
to
+45
| selfName, selfNs := os.Getenv("MY_POD_NAME"), os.Getenv("MY_POD_NAMESPACE") | ||
| if selfName == "" || selfNs == "" || c.k8sClient == nil { | ||
| return nil |
There was a problem hiding this comment.
4. Staging misses policy attribution 🔗 Cross-repo conflict ≡ Correctness
attributeNetworkPolicyBlock returns without evaluating policies when MY_POD_NAME or MY_POD_NAMESPACE is unset. The deployment repo's staging Radar container does not inject these downward-API variables, so transport failures there still reach the generic unreachable path instead of the new attribution.
Agent Prompt
## Issue description
NetworkPolicy attribution requires `MY_POD_NAME` and `MY_POD_NAMESPACE`, but the staging chart consumed by the deployment repository does not inject them into the Radar container.
## Fix Focus Areas
- deploy/helm/radar/templates/deployment.yaml[70-100]
- internal/prometheus/netpol_attribution.go[42-45]
## Recommended Fix
Add `MY_POD_NAME` and `MY_POD_NAMESPACE` to the Radar container environment using downward-API `fieldRef` values for `metadata.name` and `metadata.namespace`. Regenerate or synchronize the vendored staging chart in `skyhook-dev/deployment` so its rendered deployment carries the same variables.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
nadaverell
added a commit
that referenced
this pull request
Sep 13, 2026
…e headers without a URL (#1736) ## Why Users saw a **Discover Prometheus** button even though discovery is automatic. Two gaps: 1. Discovery already ran in the background at boot, but `prom.Status` had no in-flight state, so any metrics view opened during the 5–10 s run (4 s direct-probe budget + port-forward off-cluster) rendered the manual CTA. Clicking it only joined the run already underway. 2. After a context switch nothing started discovery at all — `Reinitialize` builds a fresh client and stops there. Separately: `--prometheus-header` values were sent to *every* auto-discovered candidate during probing. A mislabelled or hostile Service that looks like Prometheus would receive the operator's bearer token. ## What **Discovery status.** `prom.Status` gains `discovering`, owned by the detached discovery run itself — waiters come and go on their own deadlines, the flag follows the run (gate wait included). The last run's outcome, including a timeout, is reported as `error` when idle, so charts and Settings say *why* there is no connection instead of showing a bare button. Both are generation-keyed: an older run can never describe a newer configuration, and a cached-endpoint probe that finishes after a configuration change neither returns the superseded endpoint nor tears down what the new generation connected. **Pre-warm everywhere a cluster comes up.** `Prewarm()` runs at boot and from the context-switch and namespace-rescope callbacks. A cluster with no Prometheus no longer records an errorlog warning on every start; the status carries the reason. **Headers require a URL.** With headers configured and no URL, discovery refuses to run — in the Prometheus client and the Caretta traffic source — the status names the rule, and boot logs a warning (not fatal). `PUT /api/integrations/prometheus` rejects saving that pair, checked against what will actually be in effect: the submitted headers, else the running client's (flags, env and file already folded in), plus env-sourced headers on disk that re-resolve at the next start. The Helm chart's `values.schema.json` enforces the same dependency so `helm install`/`lint` fails on `traffic.prometheusHeaders*` without `traffic.prometheusUrl`. **Atomic live configuration.** A URL + header change applies in one step with the cached connection dropped alongside it (`prometheus.Configure`, `traffic.SetMetricsConfig`), and only the forward that served the dropped endpoint is stopped. The traffic subsystem is rebuilt on such a change — serialized with context operations — because a live source copies URL and headers at construction; without the rebuild, rotated or cleared credentials would keep being used until the next context switch. **Frontend.** Status polls every 2 s while a run is in flight or before the server's run has reported (a status read before the boot/switch pre-warm starts is transient — every run ends in `connected` or `error`); both chart CTA sites render "Discovering Prometheus…" instead of the button; `useAutoPromConnect` marks discovering optimistically before its POST and undoes it if the request itself fails, so an unreachable server can't leave the spinner up. Settings → Overview → Metrics shows Discovering / Connected / the failure reason. `@skyhook-io/k8s-ui` `PrometheusChartsView` gains an optional `discovering` prop (additive). ## Testing - **Go** (`internal/prometheus`, `internal/server`, `internal/traffic`, `pkg/prom`): status before / during / after a run; discovering follows the flight when every waiter cancels; failure outcome surfaces when idle; timeout and generation rules; headers gate never touches the cluster (fake client asserts no list); new credentials never reach the previous endpoint through `Configure`; a stale cached probe cannot tear down a newer connection; apply-handler 400 matrix (submitted / live / env-on-disk headers). **Vitest**: poll cadence. `helm lint` with and without headers/URL. - **Live, local Radar against an EKS cluster** with `monitoring/prometheus-operated`: opening a Pod drawer inside the boot window shows "Discovering Prometheus…" and flips to populated charts ~1 s later with no click; a header context switch to a cluster without Prometheus shows *Not connected — no Prometheus service found in cluster* in Settings, and switching back pre-warms and connects (~6 s) while the open drawer waits on the same run. Started with `--prometheus-header` and no URL: warning logged, no enumeration or probe, the drawer and Settings carry the rule, `PUT` with an empty URL returns 400, and clearing the headers in the same `PUT` discovers and connects. ## Notes - Local and in-cluster both pre-warm; in-cluster this is a `list services` plus in-cluster GETs, no port-forward. - A context switch performed server-side (API, CAPI connect) reaches an already-open tab on the regular 60 s status cadence; the header switcher invalidates queries itself. Unchanged by this PR. - Stacked follow-up: #1737 names the NetworkPolicy behind an in-cluster unreachable candidate. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes metrics discovery and how Prometheus auth headers are applied; misconfigurations that relied on headers without a URL will stop discovering until fixed, but the change reduces credential leakage to unintended endpoints. > > **Overview** > Improves **Prometheus metrics UX** and **credential safety** when Radar auto-discovers metrics backends. > > **Discovery progress** — `prom.Status` now exposes `discovering` (tied to the in-flight discovery run, not individual HTTP waiters) and surfaces the last failure (including timeouts) as `error` when idle. Boot and **context/namespace switches** call `Prewarm()` so discovery starts automatically; the UI polls every 2s while unsettled and shows “Discovering Prometheus…” instead of a manual CTA. > > **Headers require an explicit URL** — Shared rule via `pkg/prom.HeadersRequireURL` / `ErrHeadersRequireURL`: with auth headers but no `prometheusUrl`, discovery is skipped (Prometheus client and Caretta traffic), Settings/`PUT /api/integrations/prometheus` reject saving that combo, and Helm `values.schema.json` enforces `traffic.prometheusUrl` when headers are set. Docs call out that credentials are never sent to auto-discovered endpoints. > > **Safer live config** — URL and headers apply atomically (`Configure`, `SetMetricsConfig`) with the cached connection dropped in the same step; live Prometheus integration changes **restart the traffic subsystem** so rotated headers cannot keep hitting an old endpoint. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 52216a7. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
In-cluster, a Prometheus candidate that does not answer its direct probe used to end as a bare "no Prometheus service reachable in cluster", which looks identical whether the pod is down or a NetworkPolicy walls off the monitoring namespace. Discovery now records why each probe was rejected, and for a network-level failure evaluates the core NetworkPolicy objects it can see against Radar's own pod and the candidate's published endpoints. The verdict is positive evidence only. pkg/netpol is pure and three-valued: Denied requires that the source is egress-isolated with no admitting rule, or that every ready backend is ingress-isolated with no rule admitting the source — namespaceSelector on real Namespace labels, podSelector, both, ipBlock with except, ports incl. named ports and endPort. Anything the static model cannot decide is Unknown and never reported: hostNetwork pods, a Namespace the caller could not read, egress ipBlock rules, malformed selectors. The wiring in internal/prometheus adds its own preconditions before trusting the cache — synced and covering both namespaces, Radar's pod identity from the downward API, backends from EndpointSlices with every endpoint resolved to a cached Pod — and otherwise says nothing. The policy-type and port-matching primitives move out of internal/trace into pkg/netpol so the two evaluators share one definition of the semantics.
A probe that got an HTTP error reached its endpoint, so it is no longer a transport failure. Backends are tied to their cached pod by UID and address, judged under their own namespace's policies, matched on the TCP port the probe used, and include terminating-but-serving endpoints that kube-proxy may still route to. The evaluator matches ipBlock rules against the pod's addresses of the block's own family and never lets a pod deny itself. A run cancelled while attribution reads the API ends as cancelled rather than being recorded as a cluster without Prometheus.
… address A body-read failure or an oversized response arrives after the endpoint answered, so the transport now marks those as post-response errors and the probe reports them as HTTP errors rather than transport failures. Endpoint validation compares the pod against the first published address — the one consumers route to — instead of any address in the list.
A deferred kind's lister is nil for a moment after its informer syncs; attribution now requires the kind to be ready as well as synced, and nil-checks every lister it uses, so a cache that is still warming leaves the generic message in place instead of panicking.
…lock match as evidence A policy that drops packets makes the probe hang until the pass deadline; those probes now count as transport failures rather than being left unexplained by the snapshot. An ingress ipBlock miss no longer denies — the address a policy sees may have been rewritten by the network plugin — while a match still admits. The egress remedy names the resolved backends' namespaces and pod ports, which a targetPort or a manually managed EndpointSlice can make differ from the Service's.
nadaverell
force-pushed
the
feat/prometheus-netpol-attribution
branch
from
September 13, 2026 12:31
ca2fb81 to
d28678d
Compare
… trace A Service was reported reachable when egress admitted one backend and ingress admitted a different one; each backend is now judged on both legs of its own path, and a denial no single direction accounts for names both policy sets. Policy-to-pod selection is exported from pkg/netpol and trace uses it in place of its own copies, so the two evaluators agree on which policies apply to a pod. RuleMatchesPort normalizes the caller's protocol the way it already normalized the rule's.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
In-cluster, a Prometheus candidate whose direct probe fails ends as a bare no Prometheus service reachable in cluster (Radar deliberately doesn't fall back to port-forwarding there). That message looks identical whether the pod is down or a NetworkPolicy walls off the monitoring namespace — and the second is the common one on locked-down clusters, where the fix is a one-line ingress rule the user can't see they need.
What
When a probe fails at the network level, discovery evaluates the core
NetworkPolicyobjects it can see against Radar's own pod and the candidate's published endpoints, and names the policies only when they deny the connection outright:An egress denial on Radar's own side is reported the same way with an egress remedy. The error unwraps to the existing sentinels, so every
errors.Iscaller and the Availability classification are unchanged; it surfaces throughprom.Status.error(from #1736) into the charts and Settings.Positive evidence only.
pkg/netpolis pure and three-valued (Denied/Allowed/Unknown); onlyDeniedis acted on. It requires that the source is egress-isolated with no admitting rule, or that every backend is ingress-isolated with no rule admitting the source —policyTypesdefaulting, additive policies,namespaceSelectoron real Namespace labels,podSelector, both,ipBlockwithexceptmatched per address family, ports incl. named ports andendPort, and the self-traffic exception. Everything the static model can't decide isUnknownand reported as nothing: hostNetwork pods, a Namespace object the caller couldn't read, egressipBlockrules (cluster-IP DNAT is CNI-defined), malformed selectors.The wiring in
internal/prometheusadds its own preconditions before trusting the cache:MY_POD_NAME/MY_POD_NAMESPACEfrom the downward API; informers synced andKindCoversNamespacefor Pods/Services/NetworkPolicies in every namespace involved; backends resolved from EndpointSlices (real membership + resolved pod port, TCP only, terminating-but-serving included) with each endpoint tied to its cached Pod by UID and routed address; and the probe's rejection reason recorded per candidate so only a true transport failure — no HTTP response at all — is attributed. An endpoint that answered 500, refused credentials, or stalled mid-body was reached.pkg/netpolis meant to be the single home for core NetworkPolicy semantics. The policy-type defaulting, port matching and policy-to-pod selection (EffectivePolicyTypes,RuleMatchesPort,Selects) move out ofinternal/trace/netpol.goand trace now calls them, so the trace and attribution evaluators cannot disagree on which policies apply to a pod or which ports a rule covers. Their verdict layers stay separate on purpose: trace answers for an unspecified client and can only assert what holds for every source, while attribution knows the exact source pod. Each backend is judged on its whole path — the source's egress and the backend's ingress — so a Service is never called reachable because egress admits one backend and ingress a different one.Limits (documented in
docs/in-cluster.md)NetworkPolicyonly — Cilium/Calico CRD policies are not evaluated.Testing
pkg/netpol: 35 table-driven cases — every rule shape above, the uncertainty cases (hostNetwork, missing Namespace, ipBlock miss, egress ipBlock, malformed selector), self-traffic, dual-stack address families, unset protocol, and a Service whose egress admits one backend while ingress admits another (no complete path → denied on both directions).internal/prometheus: attribution wiring against a standalone cache + fake clientset — ingress and egress attribution; admitting policy, refused/answered probe, missing pod identity, namespace-scoped policy cache, not-yet-ready deferred informer, unresolvable / stale-UID / mis-addressed endpoints, UDP twin port, serving-terminating endpoint, cross-namespace backend → silence; egress remedy names the resolved backend; probes that hang to the pass deadline count as transport failures.pkg/prom: probe reasons (HTTP error vs transport vs oversized body).internal/trace: its suite is unchanged and green with policy selection routed throughpkg/netpol.Selects.Live, in-cluster on kind + Cilium (an enforcing CNI), Radar built from this branch and installed with the chart's default values. A Prometheus in
monitoringbehind adefault-deny-ingresspolicy: the probe hangs to the 3 s deadline and discovery — the boot pre-warm itself, deferred informers having synced in ~100 ms — reportsVerified in the UI through the in-cluster instance: the Pod drawer's Metrics section and Settings → Overview → Metrics both show that message; adding an allow-from-
radarrule on 9090 flips the drawer to charts with no click (connected directly at the Service address); changing that rule to port 8080 denies again and names both policies. With local Radar against the same cluster, the Service's Reachability view still shows trace's ownwould-denyfinding naming the policy, and its in-cluster probe fails to get through — the two evaluators agree. The egress direction is unit-tested only: a core egress policy that still lets Radar reach the API server needs anipBlockrule, which is deliberatelyUnknown.Follow-ups (not this PR)
internal/server/network_policy.go(the/api/network-policies/evaluateendpoint behind the Traffic flow list) still has its own evaluator, and it diverges frompkg/netpol: it treats a policy withoutpolicyTypesas isolating nothing, ignores rule ports, matchesnamespaceSelectorwithout the Namespace's labels, and never matchesipBlock. It should be rebased onpkg/netpolbehind an adapter that keeps its per-policyeffect/reasonresponse and adds the port the UI doesn't yet send.evaluateNetpol/evaluatePodPort) could be expressed onpkg/netpolonce it grows a per-backend, per-direction API that accepts an unknown source; today only the primitives are shared.NetworkPolicyRenderer/NetworkPolicyDiagramin k8s-ui have presentation gaps unrelated to this PR: a peer with both selectors shows onlypodSelector,matchExpressionsrender as "all",policyTypesdefaulting isn't applied, and the rule list omitsendPort.skyhook-dev/deploymentis vendored from an older Radar and does not injectMY_POD_NAME/MY_POD_NAMESPACE; this repo's chart does. Re-vendor there for attribution to run on staging.Note
Medium Risk
Changes Prometheus discovery failure paths and adds API/cache-dependent NetworkPolicy evaluation; mis-attribution is mitigated by conservative silence, but operators may act on policy messages that reflect declared intent rather than live CNI behavior.
Overview
When in-cluster Prometheus discovery finds candidates but none answer a direct Service probe, Radar no longer stops at the generic no Prometheus service reachable in cluster whenever it can prove a core
NetworkPolicyblocks the path. Probes now record per-candidate rejection reasons; only true transport failures (no HTTP response, including probes still hanging when the pass ends) trigger static policy evaluation from Radar’s pod to the candidate’s EndpointSlice backends.A new shared
pkg/netpolpackage holds the evaluator and port/policy-type helpers (trace’s netpol code now imports it). Attributed failures return a detailed message naming the isolating policy and a suggested ingress or egress fix, while still unwrapping to the existing unreachable sentinel soerrors.Isand UI classification stay the same. Attribution stays silent when evidence is incomplete (missingMY_POD_NAME/MY_POD_NAMESPACE, partial cache, answered-but-refused probes, etc.).pkg/promaddsProbeReasonHTTPErrorand distinguishes HTTP responses from dial/timeouts so network-path reasoning is not confused with auth or 5xx answers.docs/in-cluster.mddocuments the new message and evaluation limits (core NP only, declared intent vs CNI enforcement).Reviewed by Cursor Bugbot for commit d28678d. Bugbot is set up for automated code reviews on this repo. Configure here.