Repository navigation
fix(web-console): query OpenShift dashboard metrics securely - #270
Conversation
Track HYPERSHELL-329. Separate cluster and application metrics, read rotating credentials, and filter gateway metrics by instance.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Amber reviewStatus: Complete |
amber-review-bot
left a comment
amber-review-bot
left a comment
There was a problem hiding this comment.
Verdict
COMMENT. This is a well-constructed, security-conscious fix: the new fetchMetrics helper keeps cluster-monitoring credentials server-side, verifies TLS, refuses redirects, re-reads projected files per request for rotation, bounds the response body, and validates the token and namespace before use. I found no blockers or convention violations; the notes below are minor hardening/observability points plus one cross-PR coordination decision for maintainers.
Strengths
- Credentials never reach the browser or the application-metrics request path; test asserts
browserRuntimeConfigexcludes the cluster token and URL. - Redirects cannot receive the bearer token (
redirect: "error"on the unauthenticated path;node:httpsrequest does not follow redirects and the token-before-TLS-trust ordering is covered by a test). - Config fails closed: credential files require an explicit
https://cluster URL, and the namespace is regex-validated (in config and again innamespaceSelector) before being interpolated withJSON.stringify, preventing PromQL selector injection. - Error messages are generic and do not echo token/CA contents.
Test Diff Scrutiny
The four pre-existing cluster tests (metrics-cluster-cpu/memory/nodes/pods) change the expected error string from a source-specific message (e.g. "No cluster CPU capacity data") to "Prometheus query returned no samples". This is a message change, not a flipped contract: the "missing data throws" guarantee is preserved, and the intent (never surface zero for absent samples) is reinforced by the new does not report zero CPU use or gateway counts when samples are missing test. No removed guarantee.
Minor findings
-
[Minor]
metrics-source.ts:22- the unauthenticated path returnsfetch(...)directly and relies on the caller'sresponse.json(), so it does not apply the 4 MiB body cap that the HTTPS path enforces (metrics-source.ts:71). For the trusted in-cluster app endpoint this is acceptable, but the asymmetry means a misbehaving/compromised app Prometheus could return an unbounded body. Consider a consistent size bound or a short note on why the app path is exempt. Confidence: Medium. -
[Minor]
metrics-source.ts:44- thecatch {}collapses distinct credential failures (missing file, empty value, malformed token) into one generic"Could not read metrics credentials". Keeping secrets out of errors is correct, but a redacted debug log (e.g. which file,len=, failure kind) would materially help operators diagnose token-rotation/mount problems without leaking the value. Confidence: Medium.
Cross-PR coordination
PR 270 introduces new required deployment wiring for the web console (a cluster-monitoring-metrics-api RoleBinding in openshift-monitoring, a projected service-account token volume, a CA ConfigMap mount, and the CLUSTER_PROMETHEUS_* / PROMETHEUS_NAMESPACE env vars) and states this configuration is delivered through the external hypershell-gitops repository. PR #251 moves the GitOps deployment layer into this repository (deploy/gitops-base) on the stated principle that "the app owns its deployment definition," and it already owns the web-console Deployment patches and NetworkPolicies. These two efforts make opposite assumptions about where the console's deployment configuration is authored. Maintainers should decide the canonical home for this PR's console monitoring-access wiring and the merge order: if #251 establishes in-repo ownership of the web-console deployment surface, PR 270's RBAC/token-mount/env configuration should target deploy/gitops-base rather than the external repo, to avoid drift or one source overwriting the other.
Convention Checklist
| Convention | Result |
|---|---|
No panic() / fail-fast in production code |
Pass |
| Errors carry context, no secret leakage in errors/logs | Pass |
| No secrets in responses or browser config | Pass |
| Input validated (namespace selector, token charset, HTTPS-only creds) | Pass |
| Redirects refused; TLS verification enforced | Pass |
| Config separated from code (env-driven) | Pass |
| Test Diff Scrutiny (modified assertions justified) | Pass |
| No em dashes | Pass |
Findings Summary (highest severity first):
- [Minor] Unauthenticated metrics path lacks the 4 MiB body cap the HTTPS path enforces - Hardening (metrics-source.ts:22)
- [Minor] Generic credential-read error hides failure cause from operators; add a redacted debug log - Observability (metrics-source.ts:44)
Apply the response limit to both clients, report credential failure types without secret values, and provide the reusable OpenShift deployment component. Fix README formatting for CI.
|
Addressed Amber's review in d04466a and resolved both inline threads. Both clients now enforce a 4 MiB response limit, including decoded application responses. Credential errors identify the configuration variable and failure type without file paths or credential values. The README formatting failure is fixed. The full For deployment ownership, the reusable component is now in PR #119 remains a draft until #270 merges and its console image is published and pinned. No live configuration changed. CI is running on the new commit. |
Amber reviewStatus: Complete |
amber-review-bot
left a comment
amber-review-bot
left a comment
There was a problem hiding this comment.
Verdict
This PR cleanly separates cluster metrics (authenticated, TLS-verified, per-request token rotation) from application metrics, bounds both response paths to 4 MiB, and keeps credentials off redirects and out of logs. The security posture is solid; my remaining notes are verification items and one behavioral change worth confirming, so this is a COMMENT rather than an approval-blocking review.
Summary
The design is careful: fetchMetrics refuses to attach a bearer token to a non-HTTPS URL or a URL carrying inline credentials, uses redirect: "error" on the plain path, and the raw https.request path never follows redirects, so the token cannot leak to a redirect target. readCredential reports only a fixed variable name plus a coarse failure type (missing/unreadable/empty/invalid format), never the path, value, or underlying fs error. Namespace selectors are regex-validated and quoted, so PromQL injection is not possible. Both response readers enforce the 4 MiB cap, and the plain path applies it after undici decompression. Test coverage is broad (size limits on HTTP/HTTPS, compressed bodies, stream cancellation, token rotation, redirect rejection, TLS, timeout, source selection, namespace filtering).
The notes below are Minor.
Findings
-
[Minor] Namespace label names differ between metric families (
namespaceforhypershell_gateways_total,k8s_namespace_nameforgateway_provision_duration_seconds_*). If either label does not match the label the metric actually carries in the target Prometheus, the query returns no samples, which the new code now surfaces as a hard error (HTTP 502) rather than an empty chart. Please confirm both labels against the live metric schema. Correctness (Confidence: Medium) -
[Minor] The empty-sample contract for cluster queries changed from "return 0" to "throw". This is intentional per the description, and the updated existing tests still assert rejection (guarantee preserved, message refined). Worth confirming that no cluster subquery (for example a genuinely idle "used" series that resolves to an empty result set rather than a
0sample) can now 502 a previously-rendering dashboard. Behavior Change (Confidence: Medium) -
[Minor] In the authenticated
datahandler,reject(...)is called beforereq.destroy()on the over-limit branch. The Promise ignores the later settlement so this is harmless, but destroying first is slightly cleaner. Style (Confidence: High)
Cross-PR coordination
No material cross-PR coordination issue requires maintainer action.
Findings Summary (ordered by severity, highest first):
- [Minor] Namespace label names differ between gateway metric families; verify they match the live metric schema or empty results become 502s - Correctness (metrics-gateway-provision-duration.ts:85, metrics-gateways.ts:41)
- [Minor] Cluster empty-sample handling changed from return-0 to throw; confirm no legitimately-empty series 502s a working dashboard - Behavior Change (metrics-cluster-cpu.ts:53)
- [Minor]
rejectbeforereq.destroy()on the over-limit branch - Style (metrics-source.ts:129-130)
Convention Checklist
| Convention | Result |
|---|---|
| No secrets in logs or error messages | Pass |
| Secret references, not inline secrets (projected token/CA files) | Pass |
| Input validated (namespace regex, token format, HTTPS enforcement) | Pass |
| No panic / explicit error propagation | Pass |
| Config separated from code (env-driven, GitOps component) | Pass |
| Test diff scrutiny (modified assertions still assert rejection) | Pass |
| SecurityContext on pod specs (patch adds only env/SA/volumes to existing spec) | N/A |
Confirm live namespace labels, document missing observations, cover valid zero-valued cluster samples, and destroy oversized requests before rejecting them.
Amber reviewStatus: Complete |
amber-review-bot
left a comment
amber-review-bot
left a comment
There was a problem hiding this comment.
Verdict
APPROVE. This change routes cluster metrics through a credentialed, TLS-verified server-side client while keeping gateway metrics on the application store, and it is backed by unusually thorough tests covering size limits, token rotation, redirects, credential-failure redaction, and zero-vs-missing sample handling. I found no blocker, critical, or major issues; the notes below are minor and non-blocking.
Amber Analysis
The BFF now fetches OpenShift cluster metrics from a dedicated HTTPS endpoint using a rotating projected token and a service-CA bundle, while gateway counts and provision duration stay on the application Prometheus with a server-fixed instance namespace. The security posture is strong: TLS verification stays on, redirects are rejected before the bearer token is attached, credential-read failures name only the env var and failure type (no paths or values), namespace input is regex-validated and quoted via JSON.stringify to block PromQL injection, and both fetch paths cap responses at 4 MiB.
Test Diff Scrutiny. Four pre-existing cluster tests flip their expected message from a specific "No cluster X data" string to the generic "Prometheus query returned no samples". This is not a removed guarantee: queryPrometheusInstant changed from returning 0 on an empty result to throwing, and the new metrics-source.test.ts explicitly proves that samples carrying a literal value of 0 (idle CPU, zero ready nodes, empty pod phases) are still accepted, while only a truly absent series errors. This matches the stated intent that missing cluster samples surface as an error (HTTP 502) rather than a fabricated zero, and it is well covered.
Security review. No secrets are logged or returned to the browser; browserRuntimeConfig is asserted to exclude the cluster token and URL. Credential files are read per request to honor rotation. The deployment component disables SA token automounting and mounts a short-lived projected token. Container SecurityContext is unchanged by this PR and is not in scope.
Full findings
Minor - observability asymmetry (metrics-gateways.ts). With PROMETHEUS_NAMESPACE set (the production deployment path), an empty gateway result throws "No gateway metrics for the configured namespace" and returns 502. Without a namespace (dev / dedicated-store), an empty result silently returns all-zero phase counts, which could mask a metrics outage as "0 gateways". This looks intentional as a single-source dev fallback, but the two paths now disagree with the cluster-metrics "missing means error" convention. Confirm this is deliberate; if so, no change needed.
Nit. The authenticated HTTPS path in fetchMetrics builds the response with an implicit 200 status and does not negotiate/decompress content encoding (Node's http.request sends no Accept-Encoding, so the 4 MiB cap is over uncompressed bytes today). Fine as written; worth a comment if compression negotiation is ever added, since the cap would then apply to compressed bytes.
Cross-PR coordination
No material cross-PR coordination issue requires maintainer action.
Findings Summary
(ordered by severity, highest first)
- [Minor] Asymmetric missing-data handling: namespaced gateway query errors, non-namespaced returns zeros silently - Observability (metrics-gateways.ts L68)
- [Minor] Authenticated HTTPS path does not decompress; 4 MiB cap applies to raw bytes only - Robustness (metrics-source.ts L104-155)
Convention Checklist
| Convention | Result |
|---|---|
| No secrets in logs or responses | Pass |
| Input validated (namespace regex, token format, HTTPS-only creds) | Pass |
| Secret references / files, not inline secrets | Pass |
| TLS verification enabled; redirects reject credentials | Pass |
| Errors surfaced, partial failures not silently swallowed | Pass |
| Config separated from code (env vars, kustomize component) | Pass |
| Test diff scrutiny (modified assertions justified + backfilled by new tests) | Pass |
| Container SecurityContext / SA token hardening | Pass |
| No em dashes | Pass |
| Conventional commit messages | Pass |
The web dashboard returns HTTP 502 on hypc-aws-00 because its metrics client defaults to localhost. OpenShift cluster metrics and HyperShell gateway metrics are also in separate stores.
Add a separate cluster metrics endpoint with a rotating token file and trusted CA bundle. Cluster charts use that endpoint; gateway counts and provision duration use application Prometheus with a server-configured instance namespace. Repeated API replica gauges do not add duplicate counts. Missing cluster samples return an error. Existing single-source development settings still work.
Both clients limit responses to 4 MiB. The application client applies the limit after decompression. Credential errors identify the configuration variable and failure type without exposing file paths or values. TLS verification, redirect rejection, browser credential isolation, and existing dashboard authorization stay in place.
The reusable OpenShift deployment component is owned here at
deploy/gitops-base/components/openshift-dashboard-metrics. It contains the console patch, service account, service CA, and projected token mount. GitOps PR #119 consumes a pinned component and supplies its application endpoint and instance RoleBinding. This component works with bothdeploy/huband the wrapper from PR #251. It is optional, so #251 can merge in either order.Tracking: HYPERSHELL-329. Deployment configuration is tracked by HYPERSHELL-330.
Validation: the full
pnpm check:webcommand passes, including 187 backend tests, formatting, lint, type checks, the production build, and Storybook. Tests cover HTTP/HTTPS size limits, chunked stream cancellation, compressed responses, credential failure types, TLS, token rotation, redirects, timeout, source selection, and namespace filtering. Repository policy checks pass. The component builds with the existing hub base and PR #251's wrapper in both hyp0 and hyp1. Read-only queries confirmed the live platform metrics, zero-valued pod phases, the metrics API role, and both gateway namespace label schemas. A regression test covers valid zero-valued cluster samples. The README describes missing data and rate history requirements.Rollout: merge this source change, publish its console image, then update the component revision and console image pins in the companion GitOps PR before merge. No live configuration has changed.