feat(web-console): add operational dashboard with live metrics - #241
feat(web-console): add operational dashboard with live metrics#241kdoberst wants to merge 34 commits into
Conversation
|
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: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
REQUEST_CHANGES
The operational dashboard work is well-structured, thoroughly tested, and the API-server users RBAC (platform:admin / hypershell-admins, opaque 404s) is solid. The blocking concern is an authorization asymmetry: the dashboard pages are admin-gated, but the BFF metrics data endpoints that feed them are only authentication-gated, so any signed-in non-admin user can read cluster-wide metrics and aggregate gateway phase counts.
Findings
[Major] BFF metrics routes are not admin-gated (authorization inconsistency) — Security
components/web-console/bff/src/app.ts L441-517. The five /api/metrics/* handlers only check config.oidcIssuer && !request.session.get("accessToken") (authentication), while the /dashboard page and dashboard host / are gated with hasDashboardAdminRole(...). Any authenticated non-admin user can therefore call /api/metrics/cluster-cpu|memory|pods|nodes and /api/metrics/gateways directly and obtain the same fleet/cluster data the UI restricts to admins. This contradicts the PR's stated access model ("restricted to users with the hypershell-admins or platform:admin realm role"). Additionally, /api/metrics/gateways returns aggregate gateway phase counts across the whole fleet without filtering by the caller's RoleBindings, which is at odds with the Gateway Access Isolation rule in security.spec.md (gateway queries MUST filter by the caller's bindings). Fix: apply the same hasDashboardAdminRole check to each metrics route (guard once via a shared preHandler/helper), consistent with the page gating already in place.
[Minor] Duplicated, drift-prone gateway phase list in the BFF — Spec Consistency
components/web-console/bff/src/metrics-gateways.ts L1-5 hardcodes ["Running","Provisioning","Degraded","Failed"] and omits Pending. Any gateway reported in a Pending phase by hypershell_gateways_total will be silently dropped from the count. Prefer deriving the phase set from a single canonical source rather than a local literal (see Cross-PR coordination).
[Minor] Missing resource requests/limits on new workloads — Convention
deploy/base/prometheus/node-exporter.yaml (container ~L29) and deploy/base/prometheus/kube-state-metrics.yaml (container ~L109) define no resources.requests/limits. SecurityContexts are correct (runAsNonRoot, drop ALL, seccomp RuntimeDefault). Add modest requests/limits so these cluster-wide DaemonSet/Deployment pods are schedulable and bounded.
Cross-PR coordination
A material conflict exists with the pull request that standardizes the gateway health/phase vocabulary ([HYPERSHELL-178]). That PR establishes a single canonical phase set (Pending, Provisioning, Running, Degraded, Failed), fixes hypershell_gateways_total to emit all of those phases (specifically adding the previously-omitted Pending), and consolidates the console phase list in the shared gateway-management-ui gateway-data.ts. This PR introduces a new hardcoded phase list in components/web-console/bff/src/metrics-gateways.ts that consumes the same hypershell_gateways_total metric but omits Pending — reintroducing exactly the magic-literal drift the other PR removes. Maintainers should decide the canonical source of the phase vocabulary and the merge order: if the vocabulary-standardization PR merges first, this PR's BFF list will undercount by dropping Pending gateways and should be updated to derive from the shared vocabulary; if this PR merges first, the other PR must also reconcile the BFF list. This needs a coordinated decision rather than an independent merge of both.
Findings Summary (ordered by severity, highest first)
- [Major] BFF metrics endpoints require only authentication, not the dashboard admin role; aggregate gateway metrics also bypass per-gateway RBAC filtering - Security (app.ts L441-517)
- [Minor] Hardcoded BFF gateway phase list omits
Pendingand duplicates the canonical vocabulary - Spec Consistency (metrics-gateways.ts L1-5) - [Minor] New node-exporter / kube-state-metrics pods lack resource requests/limits - Convention (node-exporter.yaml L29, kube-state-metrics.yaml L109)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound / opaque 404 handling |
Pass |
| No secrets in logs or responses | Pass |
| Input validated | Pass |
| Authorization enforced consistently | Fail |
| SecurityContext on all pod specs | Pass |
| Resource limits/requests on containers | Fail |
| Image references pinned/consistent | Pass |
| OpenAPI client not manually edited | Pass |
| Test diff scrutiny (no silent guarantee removal) | Pass |
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
REQUEST_CHANGES. This is a large, well-structured and heavily-tested vertical slice (users API + RBAC, BFF metrics proxying, operational dashboard package, deploy manifests), and the Go RBAC path, opaque 404 concealment, security contexts, and sha-pinned images are all solid. The one thing that should change before merge is a least-privilege issue in the new kube-state-metrics ClusterRole; there is also a cross-PR coordination decision the maintainers need to make about who owns the canonical gateway phase vocabulary.
Summary
The dashboard slice is careful: /api/metrics/* routes are gated by requireDashboardMetricsAccess (401/403/dev-mode-open), the users list/get enforce hypershell-admins/platform:admin and return an opaque 404 for unauthorized single-user reads, secrets are never logged (only generic error strings), and the new components are registered in CI (component-paths.json, lint.yml). Integration and unit coverage for the new authz paths is thorough and the modified RBAC tests are mechanical (adding a jwtRoles argument), not weakened contracts.
Findings
[Major] Least privilege - kube-state-metrics ClusterRole grants cluster-wide read of Secrets/ConfigMaps (deploy/base/prometheus/kube-state-metrics.yaml)
The new hypershell-kube-state-metrics ClusterRole grants list/watch on secrets, configmaps, and many other resources cluster-wide. list/watch on secrets return the full objects (including secret data) to the kube-state-metrics ServiceAccount token, so this is a meaningful expansion of secret exposure for a component whose dashboard only needs node and pod metrics. security.spec.md emphasizes minimizing secret exposure. Please scope this down: pass --resources=pods,nodes,... (only what the dashboard scrapes) to the container and trim the ClusterRole to those resources, removing secrets (and configmaps/others you do not surface). This is the standard upstream default, but the default is broader than this feature requires.
[Minor] Hardcoded gateway phase set can silently drop phases (components/web-console/bff/src/metrics-gateways.ts)
gatewayMetricPhases enumerates exactly Running/Provisioning/Degraded/Failed and isGatewayMetricPhase drops any other value. If the API-server collector's canonical phase vocabulary grows (e.g. a Pending phase), gateways in the new phase would be silently excluded from the donut rather than surfaced. Prefer deriving this list from the single shared vocabulary rather than re-declaring it here. See the Cross-PR section below.
[Minor] Duplicated dashboard-admin role logic/constants (components/web-console/bff/src/roles.ts, components/web-console/app/lib/session-roles.ts)
HYPERSHELL_ADMIN_ROLE/PLATFORM_ADMIN_ROLE/hasDashboardAdminRole are byte-for-byte duplicated across the BFF and the SPA lib (and the role string is also defined in Go as HypershellAdminRole). The BFF/browser bundle split makes some duplication unavoidable, but consider a single shared source for the role names to avoid drift if the admin-role set changes.
[Minor] PR title is not a conventional-commit subject (commit discipline)
The PR title HYPERSHELL-276 initial dashboard with data (used as the squash subject) lacks a type(scope): description prefix, and the branch commits (Changes requested from amber review, Spec changes based on amber review) are non-conventional. Please set a conventional squash subject (e.g. feat(web-console): add operational dashboard with live data) on merge.
Cross-PR coordination
Another open pull request standardizes the Gateway phase/status vocabulary into a single canonical source of truth (introducing a shared Go gatewayhealth package used by the API server and control plane, adding Pending as a first-class phase, and deriving hypershell_gateways_total from that vocabulary), and it edits packages/gateway-management-ui/src/gateways/gateway-data.ts. This PR also edits that same file to add the phase->display-bucket mapping, and its gateway-metrics-dashboard.spec.md change declares @openshift-online/hypershell-gateway-management-ui as the "single source of truth" for the phase vocabulary. These two efforts assert competing ownership of the canonical phase vocabulary and make interdependent assumptions about the phase set (notably Pending): if the vocabulary PR merges first, this PR's BFF metrics-gateways.ts filter and the hypershell_gateways_total consumers will drop the new Pending phase; if this PR merges first, the vocabulary PR must reconcile with the display-bucket model added here. Maintainers should decide which package/module owns the canonical phase vocabulary and the merge order before either lands. Affected PR: #239.
Findings Summary (ordered by severity, highest first)
- [Major] kube-state-metrics ClusterRole grants cluster-wide read of Secrets/ConfigMaps - Security / Least Privilege (kube-state-metrics.yaml L19)
- [Minor] Hardcoded gateway phase set can silently drop future phases - Robustness / Spec Consistency (metrics-gateways.ts L1)
- [Minor] Duplicated dashboard-admin role constants/logic across BFF and SPA - Maintainability (roles.ts, session-roles.ts)
- [Minor] PR title/commits not conventional-commit format - Commit Discipline
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound / opaque 404 handling |
Pass |
| No secrets in logs or responses | Pass |
| Input validated | Pass |
| SecurityContext on all pod specs | Pass |
| Image references pinned/consistent (sha-pinned) | Pass |
| OpenAPI client generated, not hand-edited | Pass |
| Test assertions not silently weakened | Pass |
| Component registered in CI | Pass |
| Restricted RBAC / least privilege | Fail |
| Conventional commit subject | Fail |
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This is a large, well-engineered feature: the operational dashboard, its BFF metrics routes, the new users API, and the RBAC/admin-role plumbing are cohesive, tested, and follow HyperShell conventions (restricted SecurityContexts on the new pods, sha-pinned images, CI component registration, no secrets in logs). The main items to resolve before merge are an undocumented access-restriction change to the existing /metrics page and cross-PR coordination on work this PR duplicates.
Summary
The change adds a widgetized admin dashboard with live data sourced from a new users list API, paginated gateways, and five Prometheus-backed BFF metrics routes, all gated behind hypershell-admins/platform:admin. Code quality is high and defense-in-depth (BFF enforcement + client RequireDashboardAdmin) is done correctly; my findings are one behavioral regression to confirm plus minor polish.
Findings
[Major] /metrics (the existing gateway metrics dashboard) is silently restricted to dashboard admins.
requiresDashboardAdminAccess() in components/web-console/bff/src/app.ts (L100) and the RequireDashboardAdmin wrapper in components/web-console/app/routes/metrics.tsx (L13) now gate the pre-existing /metrics route behind hypershell-admins/platform:admin. Previously any authenticated user could open it. This is an access regression for non-admin users of an existing feature and is not mentioned in the PR description. Please confirm it is intentional and call it out in the description (and specs), or scope the new admin gate to /dashboard and the dashboard host only. Confidence: Medium.
[Minor] Prometheus query timeout is a hardcoded magic number.
Each metrics route in components/web-console/bff/src/app.ts passes a literal 10_000 ms timeout. Per the "separate configuration from code" convention, consider sourcing this from ServerConfig (like prometheusUrl) so operators can tune it without a code change. Confidence: High.
Cross-PR coordination
This PR re-introduces, verbatim, the shared gateway phase/status vocabulary that is the entire subject of another open pull request: the new components/api-server/pkg/gatewayhealth package (and its test), the plugins/gateways/metrics.go rewrite that derives phases from gatewayhealth.PhaseStrings(), and a specs/platform/gateway-phase-vocabulary.spec.md (with differing content). Maintainers must decide ownership and merge order: pull request #239 is the dedicated owner of this vocabulary, so it should land first and this PR should rebase to consume it, rather than both adding the same package/spec and colliding. This is a duplicate-solution/ownership decision, not a routine merge conflict.
This PR also introduces its own definition of "gateway provision time" — a new specs/platform/gateway-provision-time.spec.md and a dashboard "mean provision time" widget computed client-side from the gateway list. Pull request #243 measures the same concept server-side as an OTLP gateway.provision.duration histogram in the control plane. These are competing data models for one metric; maintainers should decide whether the dashboard consumes #243's histogram as the source of truth instead of independently deriving provision time, to avoid two divergent definitions.
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped / typed ServiceError with context |
Pass |
| No secrets in logs or responses | Pass |
| Input validated | Pass |
| Restricted SecurityContext on new pod specs | Pass |
| Resource requests/limits on new containers | Pass |
| Image references sha-pinned and consistent | Pass |
| CI component registration for new package | Pass |
| Test diff scrutiny (no silently flipped assertions) | Pass |
| Config separated from code | Fail (hardcoded metrics timeout) |
Findings Summary (ordered by severity, highest first):
- [Major]
/metricsexisting route silently restricted to dashboard admins - Access Control / Scope (app.ts L100, metrics.tsx L13) - [Minor] Hardcoded 10s Prometheus timeout should be configurable - Config vs Code (app.ts)
adb99b4 to
886c4ad
Compare
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
COMMENT — This is a large, well-structured operational-dashboard slice (API users list, BFF Prometheus-backed metrics routes, reusable dashboard package, deploy manifests) with solid security hygiene: dashboard-admin gating on every metrics route, secret-free logging, pinned image digests, and restricted SecurityContexts on the new exporters. Two design-level concerns (whole-dashboard failure on empty/all-provisioning fleets, and a provision-time proxy that drifts over a gateway's lifetime) plus cross-PR coordination need maintainer attention before merge.
What looks good
- Every
/api/metrics/*BFF route sits behindrequireDashboardMetricsAccess(401 reauth / 403 non-admin), and browser navigations to/dashboard,/metrics, and the dashboard host are guarded consistently.PROMETHEUS_URLis server-side config validated as an origin, so there is no user-controlled SSRF surface. - API-server
userslist/get is gated to platform-admin binding or thehypershell-adminsrealm role, with an opaque404on unauthorized GET-by-id (matches the security spec's existence-hiding rule). Good integration coverage. - New
kube-state-metricsandnode-exportermanifests setrunAsNonRoot,allowPrivilegeEscalation: false,drop: ["ALL"], seccompRuntimeDefault, and pin images by digest. CI registration (component-paths.json,lint.yml) andCLAUDE.mdwere updated for the new package. - No
panic()in production paths; Go errors are returned through the framework; metrics routes fail closed to502with redacted logs.
Findings
[Major] Empty / all-provisioning fleet takes the whole dashboard down. averageGatewayProvisionMinutes throws when there are zero Running gateways, and it is awaited inside aggregateGatewayList before the Promise.all that loads memory/CPU/pods/nodes/users. A single missing sample therefore rejects getOperationalMetrics entirely, so unrelated cluster-infrastructure widgets go dark on a fresh install or any fleet where nothing has reached Running yet. This is codified in the spec (GPT-07), so it is self-consistent — but the design choice of coupling an optional application metric to the availability of the entire operational dashboard is worth a maintainer decision. Recommend degrading only the provision-time row (omit it or mark it unavailable) rather than failing the whole payload. Confidence: High.
[Major] Provision-time proxy drifts for the lifetime of the gateway. The metric uses updated_at - created_at for Running gateways as the provision duration. updated_at is bumped by every subsequent write to the row (status/phase heartbeats from the control-plane health loop), so a long-lived healthy gateway reports an ever-growing "provision time" that is really its age, not its time-to-Running. The spec labels this a v1 proxy (GPT-03), but the number will be materially wrong in steady state. Please confirm the intended semantics with maintainers and see the Cross-PR section for a more precise measurement already in flight. Confidence: Medium.
[Minor] Squash-merge title is not a conventional commit. The PR title HYPERSHELL-276 initial dashboard with data (and several intermediate commits such as "code change for amber review", "Final linting", "Update mock data") lacks a type(scope): description prefix. Since merges squash to the PR title on main, please reword to e.g. feat(web-console): add operational dashboard with live metrics. Confidence: High.
Cross-PR coordination
This PR bundles, verbatim, the gateway health/phase vocabulary standardization that another open pull request owns as its sole purpose: an identical components/api-server/pkg/gatewayhealth package (same Phase constants, PhaseStrings/IsValidPhase, StatusHealthy), the same new specs/platform/gateway-phase-vocabulary.spec.md, and the same plugins/gateways/metrics.go rewrite that derives the phase set from that package. Maintainers must decide ownership and merge order: land #239 first and have #241 drop/rebase onto the shared package (and its web-console mirror), or explicitly reassign that vocabulary work to #241 and close it out of #239. Merging both as-is will duplicate the source of truth and collide on those files.
Separately, the provision-time capability here (new specs/platform/gateway-provision-time.spec.md plus the dashboard-adapter updated_at - created_at average) competes with #243, which measures gateway provision duration in the control plane as an OTLP histogram anchored on the created_at→Running transition. These are two different definitions of the same concept with different accuracy characteristics. Maintainers should choose one canonical measurement (or explicitly scope each to its layer) so the platform does not ship two divergent "provision time" numbers.
Findings Summary (ordered by severity, highest first)
- [Major] Empty/all-provisioning fleet fails the entire operational dashboard, not just the provision-time row — Robustness / API design (dashboard-control-plane.ts L215, L289)
- [Major]
updated_at - created_atprovision-time proxy drifts with post-Running writes — Correctness (dashboard-control-plane.ts L179, L211) - [Minor] Squash-merge title / commits not conventional-commit form — Commit discipline
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
| Errors wrapped / propagated with context | Pass |
errors.IsNotFound / opaque 404 handling |
Pass |
| No secrets in logs or responses | Pass |
| Input validated (roles, phase set, Prometheus origin) | Pass |
| SecurityContext on all pod specs | Pass |
| Image references pinned/consistent | Pass |
| OpenAPI client not hand-edited (generated) | Pass |
| Component registered in CI | Pass |
| Conventional commit message | Fail |
Introduce the operational-dashboard-ui package and wire it into the web console with gateway status, utilization, and summary widgets, layout persistence, mock metrics support, and dashboard host routing. Co-authored-by: Cursor <cursoragent@cursor.com>
Gate the operational dashboard behind hypershell-admins and platform:admin roles in the BFF and browser, with access-denied UI and improved OIDC role claim extraction for Keycloak group mappings. Co-authored-by: Cursor <cursoragent@cursor.com>
restore_web_console_from_overlay() must use kustomize's LoadRestrictionsNone flag so deploy/kind builds on kustomize 5.x. Without it the overlay fails and the script falls back to deploy/base/web-console.yaml, stripping OIDC env vars and breaking /auth/session and API proxy auth. Co-authored-by: Cursor <cursoragent@cursor.com>
Add the operational dashboard spec, clarify its separation from the Prometheus gateway metrics pipeline, and harden verification with adapter and package tests. Co-authored-by: Cursor <cursoragent@cursor.com>
Expose read-only user inventory (List/Get) with admin-only RBAC, regenerate Go and TypeScript SDK clients, and wire the registered-users dashboard widget through the control-plane adapter per registered-users.spec.md. Co-authored-by: Cursor <cursoragent@cursor.com>
Define hub-cluster used and capacity memory via BFF Prometheus proxy, register the spec in the index, and mark registered-users connected in OP-DASH-08. Co-authored-by: Cursor <cursoragent@cursor.com>
Implement CM-W1–W3 from cluster-memory.spec.md: node-exporter scrape targets, BFF GET /api/metrics/cluster-memory (Prometheus instant queries), and dashboard adapter mapping to the memory utilization widget. Refresh registered-users reconcile state to 8/8 present after eb99f6b. Co-authored-by: Cursor <cursoragent@cursor.com>
Define hub-cluster CPU capacity, used, and available cores via Prometheus node-exporter, BFF route, and dashboard adapter mapping to the existing cpu widget. Co-authored-by: Cursor <cursoragent@cursor.com>
Add BFF Prometheus route and dashboard adapter for hub-cluster CPU cores, re-sync locales/en.json key ordering from prior manual edits, and update reconcile state for CC-W1–W3. Co-authored-by: Cursor <cursoragent@cursor.com>
Define hub-cluster pod capacity and utilization via kube-state-metrics, BFF route, and dashboard adapter requirements; register the spec and link OP-DASH-08 pods row. Used pod count includes all phases (Failed and Succeeded while objects still exist). Co-authored-by: Cursor <cursoragent@cursor.com>
Deploy kube-state-metrics for pod capacity/usage PromQL, expose BFF GET /api/metrics/cluster-pods, and connect the dashboard pods widget. Co-authored-by: Cursor <cursoragent@cursor.com>
Add cluster-nodes spec and connect node inventory via kube-state-metrics, BFF GET /api/metrics/cluster-nodes, and gateway-style summary status. Co-authored-by: Cursor <cursoragent@cursor.com>
Add gateway-provision-time spec and compute mean Running gateway duration from the paginated gateway list for the provision-time metric. Co-authored-by: Cursor <cursoragent@cursor.com>
Extract StatusDonutChart for gateway and node inventory metrics, wire a nodes widget with compact sizing, and document the presentation in specs. Co-authored-by: Cursor <cursoragent@cursor.com>
Extract new dashboard message IDs into web-console locales, fix OP-DASH-16 scenario drift, and refresh RECONCILE checkpoint for OP-DASH-16 and layout v17. Co-authored-by: Cursor <cursoragent@cursor.com>
Expose per-phase pod counts from the BFF and render a capacity donut on the pods widget with phase segments plus an Unused segment for available slots. Co-authored-by: Cursor <cursoragent@cursor.com>
Replace placeholder-era description with a summary of fleet health, cluster capacity, and usage metrics plus the 15-minute refresh policy. Co-authored-by: Cursor <cursoragent@cursor.com>
Treat omitted active_sandbox_count as zero when summing gateways so the adapter never emits NaN, and show a localized fallback for any non-finite metric value in cards, summaries, and charts (OP-DASH-18). Co-authored-by: Cursor <cursoragent@cursor.com>
kdoberst
changed the title
3585630 to
0043910
Compare
Amber reviewStatus: Stopped The pull request head changed before Amber posted the review. A later job can review the new head. |
0043910 to
bdc836e
Compare
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
COMMENT — This is a large, cohesive, and unusually well-tested feature (operational dashboard + supporting API/BFF/deploy plumbing). The code follows HyperShell conventions: no panic() in production paths, errors are wrapped/handled, secrets are not logged, pod specs carry restricted SecurityContexts, the new UI package is registered in CI, and generated OpenAPI/SDK artifacts are regenerated rather than hand-edited. I found no Blocker/Critical defects in this PR's own code; the most important item is a cross-PR coordination decision (see below), plus a couple of minor polish points.
The Prometheus-backed metrics contract is internally consistent: the control-plane histogram gateway.provision.duration (unit s) exports as gateway_provision_duration_seconds_{bucket,count,sum}, which is exactly what the BFF queries; hypershell_gateways_total{phase=...} matches the API-server gauge; and PROMETHEUS_URL is a validated, non-user-controlled origin, so the BFF query fan-out is not an SSRF vector. Admin enforcement is server-side in the BFF (route + /api/metrics/* preHandler), with the SPA guard as defense-in-depth.
Strengths
- Solid test coverage across BFF routes/adapters, RBAC authorization, and the users plugin integration.
- The added RBAC test changes are mechanical (
isAuthorized(..., nil)signature update) plus genuinely additive new cases — no pre-existing assertion was flipped from allow→deny, so no silent guarantee was removed. - Metrics pod specs (node-exporter, kube-state-metrics, otel-collector) all set
runAsNonRoot, dropALLcaps,readOnlyRootFilesystem, seccomp, and resource limits; host mounts are read-only.
Findings
- [Minor] User list endpoint returns full PII when the dashboard only needs a count.
GET /api/hypershell/v1/usersreturns eachUserincludingemailandname, but the registered-users widget consumes onlytotalfrom the paginated envelope. The endpoint is admin-restricted so this is not a leak, but returning per-user email/name to satisfy a count is more surface than necessary. Consider a lighter count-only projection or documenting why the full list is exposed. (components/api-server/plugins/users/handler.go,presenter.go) - [Minor] Duplicated Prometheus instant-query helper.
queryPrometheusInstant/queryPrometheusInstantNumber(AbortController + timeout + status/finite validation) is copy-pasted acrossmetrics-cluster-cpu.ts,metrics-cluster-memory.ts,metrics-cluster-nodes.ts,metrics-cluster-pods.ts, andmetrics-gateway-provision-duration.ts. Extracting one shared helper would reduce drift risk as these routes evolve. - [Minor] Access scope for
/metrics(Gateway Metrics Dashboard) is tightened from any authenticated user to dashboard-admins only. This is intentional and documented (spec DASH-07), so no change is required — flagging it so reviewers/operators are aware it is a behavioral change, not just an additive feature.
Cross-PR coordination
Another open pull request independently introduces the same new shared package components/api-server/pkg/gatewayhealth (identical Phase constants, PhaseStrings()/IsValidPhase(), StatusHealthy) and the same new spec file specs/platform/gateway-phase-vocabulary.spec.md, and it also edits components/api-server/plugins/gateways/metrics.go to source phases from that package. That PR is the dedicated "standardize gateway health/readiness vocabulary" change (it additionally adds write-time phase validation and rewires the control-plane consumers), whereas this PR needs the package only for the per-phase metric and the BFF/console dashboard vocabulary. This is a duplicate solution, not a mere file overlap: whichever merges second will re-add an already-existing package and spec and must be reworked. Maintainers should decide which PR owns gatewayhealth and gateway-phase-vocabulary.spec.md, merge that one first, and rebase the other to consume it (dropping its duplicate copies). Please coordinate with the owner of that PR before merging either.
Findings Summary (ordered by severity, highest first):
- [Minor] User list endpoint returns full PII (email/name) when only a count is consumed - API Design / Data Minimization (
plugins/users/handler.go,presenter.go) - [Minor] Duplicated Prometheus instant-query helper across five BFF metrics modules - Maintainability (
bff/src/metrics-*.ts) - [Minor]
/metricsaccess tightened to dashboard-admins only (intentional/documented) - Behavioral Change Awareness (app/routes/metrics.tsx,bff/src/app.ts)
Convention Checklist:
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf / handled |
Pass |
errors.IsNotFound/404 concealment handled |
Pass |
| No secrets in logs or responses | Pass |
| Input validated (roles, phases, numeric samples) | Pass |
| SecurityContext on all pod specs | Pass |
| Reconcile pattern (not create-or-skip) | N/A |
| Image references pinned/consistent | Pass |
| Component registered in CI | Pass |
| OpenAPI/SDK generated, not hand-edited | Pass |
| Test Diff Scrutiny (no flipped assertions) | Pass |
| } | ||
|
|
||
| for _, user := range users { | ||
| converted := PresentUser(&user) |
There was a problem hiding this comment.
The dashboard's registered-users widget only consumes total from this list envelope, yet each item is presented with full PII (email, name) via PresentUser. The route is admin-restricted so this is not a leak, but consider a count-only projection (or field filtering by default) so the endpoint returns no more than the dashboard needs. Minor / data minimization.
| }; | ||
| } | ||
|
|
||
| async function queryPrometheusInstant( |
There was a problem hiding this comment.
This queryPrometheusInstant helper (AbortController + timeout + status/finite validation) is duplicated near-verbatim across metrics-cluster-cpu.ts, metrics-cluster-memory.ts, metrics-cluster-nodes.ts, this file, and metrics-gateway-provision-duration.ts. Extracting a single shared helper would prevent the five copies from drifting as validation rules evolve. Minor / maintainability.
2 participants
Summary
Closes HYPERSHELL-276.
Closes HYPERSHELL-154.
Adds an Operational Dashboard to the HyperShell web console — a widgetized fleet health overview for administrators. The dashboard is available at
/dashboard.Access is restricted to users with the
hypershell-adminsorplatform:adminrealm role. Non-admins are redirected to the main page (gateway list).The existing Gateway Metrics Dashboard at
/metricsis intentionally restricted to the same dashboard-admin roles (BFF redirect +RequireDashboardAdminSPA guard). This aligns Prometheus-sourced fleet phase counts with the operational dashboard access model; seeplatform/gateway-metrics-dashboard.spec.mdDASH-07.New package
Introduces
@openshift-online/hypershell-operational-dashboard-ui, a reusable PatternFly widgetized-dashboard package with:OperationalDashboardPageand a default four-column layout (usage summary, gateway status donut, sandboxes, memory, CPU, pods, nodes, system summary)DashboardControlPlaneport,createDashboardOperations, workflow probes)Live data (v1)
GET /api/hypershell/v1/usersAPI (totalfrom paginated list)GET /api/hypershell/v1/gateways— healthy / provisioning / degraded / failed breakdownactive_sandbox_countacross gatewaysGET /api/metrics/cluster-memory(Prometheus node-exporter)GET /api/metrics/cluster-cpu(Prometheus node-exporter)GET /api/metrics/cluster-pods(kube-state-metrics) — capacity, phases, unusedGET /api/metrics/cluster-nodes(kube-state-metrics) — ready vs not readyGET /api/metrics/gateway-provision-duration(control-plane OTLP histogram: mean, P50, P95)Supporting changes
/dashboard,/metrics, dashboard-host/, and all/api/metrics/*routes; Prometheus-backed cluster and provision-duration metrics routes with configurablePROMETHEUS_QUERY_TIMEOUT_MSgateway.provision.durationOTLP histogram for provision-time metricsspecs/web-console/operational-dashboard.spec.mdplus platform specs for cluster CPU/memory/nodes/pods, registered users, and gateway provision timeScreenshot
Test plan
hypershell-adminsorplatform:adminand open/dashboard— dashboard loads with live metrics/dashboard— access denied empty state; BFF redirects on direct navigation/metrics— redirected away (admin-only)pnpm --filter @openshift-online/hypershell-operational-dashboard-ui checkcomponents/web-console/bff/test/*,dashboard-control-plane.test.ts)components/api-server/plugins/users/integration_test.go)