feat(web-console): add operational dashboard with live metrics - #241
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: Advanced 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 |
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.
bdc836e to
7d122e3
Compare
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
APPROVE-leaning COMMENT. This is a large, well-structured, and heavily-tested feature (new reusable dashboard package, BFF metrics proxy routes, users API, RBAC extension, and deploy manifests). Conventions are respected: SecurityContext is correct on every new pod spec, Prometheus queries use fixed PromQL (no injection), secrets are never logged, unauthorized user-by-id GETs return an opaque 404, and CI/component registration is complete. My one notable concern is a resilience/consistency issue in the dashboard data adapter; the rest are minor.
Summary
The operational dashboard, users inventory API, and dashboard-admin authorization are cohesive and thoroughly tested (BFF route tests, adapter tests, RBAC unit + integration tests, Storybook fixtures). The refactor of isAuthorized to take jwtRoles is a pure signature addition in pre-existing tests (mechanical nil argument), not a removed guarantee, and the /metrics access tightening is intentional and documented in gateway-metrics-dashboard.spec.md DASH-07.
Findings
[Major] Dashboard adapter fails the entire dashboard when a single cluster-metric source is unavailable - Resilience / Spec Consistency
getOperationalMetrics fetches memory/CPU/pods/nodes with Promise.all, so a transient failure of any one source (e.g. a 502 from /api/metrics/cluster-memory) rejects the whole call, which surfaces as showInitialLoadError/showRefreshError and blanks every widget, including healthy ones. The provision-time fetch, by contrast, is wrapped to return undefined and degrade to a per-widget unavailable state. operational-dashboard.spec.md states (lines 8, 221) that "widgets without a connected source remain on the page and render a localized unavailable state ... instead of failing the entire dashboard." For a "fleet health at a glance" surface, one dependency outage hiding all data is a poor operational outcome. Consider Promise.allSettled (or per-metric try/catch returning undefined, mirroring provision-time) so a single source outage only marks that widget unavailable. The adapter tests currently codify the whole-dashboard-fails behavior, so please confirm the intended contract and align spec + tests + code. Confidence: Medium.
[Minor] Users list returns full PII though only total is consumed - Security / Data minimization
The dashboard's registered-users widget only reads userList.total, but GET /api/hypershell/v1/users returns every user's email and name. The endpoint is admin-restricted so this is not a leak, but consider whether email PII needs to be in this response or whether a lean count/summary shape would be preferable. Confidence: Medium.
[Minor] Redundant role re-export indirection - Maintainability
bff/src/roles.ts re-exports from bff/src/dashboard-roles.ts, which re-exports from shared/dashboard-roles.js. Two BFF shim files that both only re-export the same three symbols could collapse to one import site. Low priority. Confidence: High.
Cross-PR coordination
No material cross-PR coordination issue requires maintainer action.
Findings Summary (ordered by severity, highest first)
- [Major] Single cluster-metric outage blanks the whole dashboard instead of one widget - Resilience / Spec Consistency
- [Minor] Users list returns full PII (
email,name) though onlytotalis used - Security / Data minimization - [Minor] Redundant BFF role re-export shim files - Maintainability
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
| Errors wrapped / propagated (no silent swallow) | Pass |
| No secrets in logs or responses | Pass |
| Input validated (fixed PromQL, no injection) | Pass |
| Opaque 404 for unauthorized resource-by-id | Pass |
| SecurityContext on all new pod specs | Pass |
| Image references pinned by digest | Pass |
| OpenAPI client generated (not hand-edited) | Pass |
| CI / component registration for new package | Pass |
| Test Diff Scrutiny (modified assertions justified) | Pass |
| Conventional commit / squash before merge | Review (rebase commits present) |
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-structured, and heavily-tested vertical slice: it lands the operational dashboard, a users list API with proper RBAC concealment, dashboard-admin enforcement on /dashboard and /metrics, and hardened Prometheus proxy routes with fixed PromQL and no secret leakage. I found no blockers; the main item to confirm is a role-extraction inconsistency between the API server and the BFF that could break the registered-users widget for the exact admin persona it targets.
What looks good
- RBAC: the new
usersresource requires dashboard-operator access (platform:adminbinding orhypershell-adminsJWT role), and deniedGET /users/{id}correctly returns404to conceal existence (matchessecurity.spec.md). New tests are additive and cover the allow/deny/conceal paths. - Test diff scrutiny: the
authorization_test.gochanges are mechanical (isAuthorized(...)gains ajwtRolesparameter, passednil); no pre-existing assertion flips from allow to deny. No removed guarantees. - BFF
/api/metrics/*routes are gated byrequireDashboardMetricsAccess(401/403), useCache-Control: no-store, build PromQL from fixed constants (no injection), enforce a bounded timeout viaAbortController, and return generic502 { "error": "Metrics unavailable" }without leaking Prometheus detail. - Deploy manifests (node-exporter, kube-state-metrics, otel-collector) all set restrictive
securityContext(runAsNonRoot,allowPrivilegeEscalation: false, dropALL) and resource requests/limits. - No
panic(), nocontext.TODO(), no secrets in logs, and no em dashes in changed files.
Findings
[Major] API-server JWT role extraction diverges from the BFF, which can deny the registered-users widget to hypershell-admins. The dashboard admin gate has two independent implementations. The BFF (auth.ts extractRealmRoles) reads roles from roles, then the top-level groups claim (the documented oidc-usermodel-realm-role-mapper mapping), then realm_access.roles, and normalizes a leading / (/hypershell-admins -> hypershell-admins). The API server (user_provisioning.go extractJWTRoles) reads only realm_access.roles and does no leading-slash normalization. If the bearer/access token presented to the API server carries the realm role only via groups/group-path form (the mapping the PR itself documents for ID tokens), then HasHypershellAdminRole returns false and GET /api/hypershell/v1/users returns 403, even though the BFF has already granted the user the /dashboard page. The result is a broken "Registered users" widget for exactly the hypershell-admins persona (users with a platform:admin binding are unaffected because that path goes through bindings). Please align the API server role extraction with the BFF (accept groups and normalize the leading slash), or document the required Keycloak token/claim mapping and add a test asserting a hypershell-admins-only token is authorized for users. Confidence: Medium (depends on Keycloak token/claim configuration, which I cannot verify here).
[Minor] users list field-filter drops the list envelope. In handler.go List, when listArgs.Fields is set the handler returns the raw filteredItems slice instead of the UserList envelope, so kind/page/size/total are omitted for field-filtered responses. The dashboard relies on total for the registered-user count; a caller passing fields= would lose it. If this matches the established list-handler pattern in the repo it is acceptable, but worth confirming the dashboard never sends fields. Confidence: Medium.
Cross-PR coordination
See the top-level Cross-PR coordination section below.
Cross-PR coordination
One material coordination item requires maintainer attention.
deploy/gitops-base GitOps fleet layer (PR #251). This PR adds a hub-cluster metrics-collection stack to deploy/base (a node-exporter DaemonSet with hostPath mounts, kube-state-metrics with a cluster-scoped ClusterRole/ClusterRoleBinding, an otel-collector Deployment, and ServiceMonitors) and wires prometheus/ into deploy/base/kustomization.yaml. PR #251 introduces deploy/gitops-base, whose wrapper composes deploy/hub (which inherits deploy/base) and layers fleet-wide NetworkPolicy, name-references, and RBAC/prefix transforms. Because the new base resources transitively flow into the GitOps fleet render, the maintainers must decide, and the two PRs must coordinate on: (a) whether cluster-scoped/host-level monitoring (DaemonSet + cluster RBAC) belongs in the fleet-wide base render or must be excluded/patched per environment; (b) whether the wrapper's network policies permit scraping of the new node-exporter/kube-state-metrics/otel-collector endpoints; and (c) whether the name-reference/RBAC-prefix transforms need to account for the new ServiceAccount and ClusterRoleBinding subject. This is an ordering/design decision, not a text merge conflict, and should be resolved before whichever of the two merges second.
Findings Summary (ordered by severity, highest first)
- [Major] API-server JWT role extraction (
realm_access.rolesonly, nogroups/leading-slash handling) diverges from the BFF, risking403onGET /usersforhypershell-admins- Security / Consistency (user_provisioning.go, authorization.go) - [Minor]
userslist field-filter path returns a bare slice, dropping the list envelope (total) - API Design (handler.go)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
| Errors wrapped / framework error passthrough | Pass |
errors.IsNotFound / 404 concealment handled |
Pass |
| No secrets in logs or responses | Pass |
| Input validated (fixed PromQL, no injection) | Pass |
| SecurityContext + limits on all pod specs | Pass |
| Reconcile pattern (N/A - no reconciler changes) | N/A |
| Image references pinned by digest | Pass |
| OpenAPI client generated, not hand-edited | Pass |
| Test diff scrutiny (no flipped guarantees) | Pass |
| No em dashes | Pass |
766e596 to
317b79f
Compare
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-structured feature that adds an operational dashboard, a registered-users API surface, dashboard-admin authorization, and Prometheus-backed hub metrics. The change is solid: security contexts are present on every new pod spec, BFF metrics routes use fixed PromQL (no injection) with bounded timeouts and generic 502s, error paths are wrapped, and access is enforced in depth (BFF redirect + SPA guard + API-server RBAC). I have only minor, non-blocking notes.
Strengths
- Restricted
SecurityContext(runAsNonRoot, drop ALL, seccomp RuntimeDefault, readOnlyRootFilesystem) and resource requests/limits on the new kube-state-metrics, node-exporter, and otel-collector manifests. - BFF Prometheus queries are constant strings with
AbortControllertimeouts (configurable viaPROMETHEUS_QUERY_TIMEOUT_MS), and failures return an opaque502without leaking upstream detail. - API-server
usersRBAC gates the endpoint behind platform-admin or thehypershell-adminsrealm role, returns an opaque404for denied by-ID reads, and JWT role extraction was deliberately aligned between the API server and the BFF (extractRealmRolesFromClaims/extractRealmRoles). - Test coverage is strong across authorization, JWT role parsing, users integration, and the BFF metrics routes.
Test Diff Scrutiny
The modified assertions in authorization_test.go are purely mechanical: every existing isAuthorized(...) call gained a trailing nil for the new jwtRoles parameter. No pre-existing assertion flipped its guarantee (accepted -> rejected, optional -> required), and new cases were added additively for the users resource. No removed guarantees.
Minor findings
- [Minor] The
userslist/get responses includeemailandname(PII) for every registered user, but the operational dashboard only consumestotal. Consider data minimization on this new surface (e.g. return identity fields only where a consumer needs them) or confirm exposing full identity to every dashboard-admin is intended. Security / API design (low confidence). - [Minor] In the users
Listhandler, when thefieldsquery parameter is present the handler returns the filteredItemsslice and drops theTotal/paging envelope. The dashboard's count query does not passfields, so this is latent, but any caller combiningfieldswith a need fortotalwould get an inconsistent shape. API design (low confidence). - [Minor] Access to
/metrics(Gateway Metrics Dashboard) is tightened from any authenticated user to dashboard-admins only. This is intentional and documented (DASH-07), noting here only so it is not lost as a behavior change for existing non-admin users.
Cross-PR coordination
No material cross-PR coordination issue requires maintainer action.
Summary
Findings Summary (ordered by severity, highest first):
- [Minor] Users API returns email/name PII while the dashboard only needs a count - consider data minimization - Security / API design (presenter.go L17)
- [Minor]
fieldsquery param drops the pagination/Totalenvelope in the users list handler - API design (handler.go L58) - [Minor]
/metricsaccess tightened to dashboard-admins only (intentional, documented) - Behavior change (metrics.tsx L14)
Convention Checklist:
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound / opaque 404 for denied by-ID reads |
Pass |
| No secrets in logs or responses | Pass |
| Input validated / no PromQL injection (constant queries) | Pass |
| SecurityContext on all new pod specs | Pass |
| Resource limits/requests on new containers | Pass |
| Image references pinned by digest | Pass |
| OpenAPI client generated (not hand-edited) | Pass |
| Conventional commit messages | Pass |
…ter data Deliver the operational dashboard with widgets for gateways, registered users, and hub cluster resources (memory, CPU, pods, nodes, provision time). Restrict dashboard access to admin roles, wire live data through the BFF, and align JWT role extraction between API server and BFF. Co-authored-by: Cursor <cursoragent@cursor.com>
317b79f to
403475e
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 well-structured, defense-in-depth feature (BFF redirect + SPA guard + API-server RBAC), with proper security contexts on the new deploy manifests, CI registration for the new package, timeouts on all Prometheus fetches, and generic error responses that do not leak upstream detail. The main item to confirm before merge is a security-relevant change to JWT role-claim precedence that also affects the existing role-sync path; the remaining findings are minor robustness/traceability notes.
What looks good
- Layered authorization for the new users inventory: BFF
requiresDashboardAdminAccess/requireDashboardMetricsAccess, theRequireDashboardAdminSPA guard, and API-serverhasUsersInventoryAccessall agree onplatform:admin/hypershell-admins. - Denied
GET /users/{id}returns an opaque404(matchessecurity.spec.md"return 404 not 403 to avoid revealing existence"). - All Prometheus helpers use an
AbortControllertimeout, validatestatus/sample shape, reject non-finite/negative values, and the routes downgrade failures to502 Metrics unavailablewithout echoing upstream errors. deploy/base/prometheus/*pods setrunAsNonRoot, dropALLcaps,allowPrivilegeEscalation: false, and seccompRuntimeDefault.- New
operational-dashboard-uicomponent is registered in.github/component-paths.jsonandlint.yml, and OpenAPI is wired viamake generate(not hand-edited).
Findings
[Major] JWT role-claim precedence is prefer-one, and it also governs role sync (security-relevant).
extractRealmRolesFromClaims now returns the roles claim if present, else groups, else realm_access.roles - and jwt_roles_test.go explicitly asserts realm_access is dropped when roles is present. This same extractor feeds SyncJWTRoles, which previously read only realm_access.roles. If a token presented to the API server carries a groups/roles claim that does not enumerate platform:admin/gateway:creator alongside hypershell-admins, those bindings will silently stop syncing and authorization will regress. Please either union the claim sources or document/guarantee (and test) that the mapped groups claim on tokens reaching the API server always contains the full realm role set. Confidence: Medium.
[Minor] metrics-cluster-pods.ts uses exact equality for two independently-scraped series.
phaseTotal !== used_pods throws Inconsistent cluster pod phase samples, which fails the whole pods widget when count(kube_pod_info) and sum(kube_pod_status_phase{...}) momentarily skew across scrapes. CPU already applies a tolerance; pods should apply a small tolerance/clamp for consistency. Confidence: Medium.
[Minor] Provision-time widget depends on an emitter not present in this diff.
The BFF queries gateway_provision_duration_seconds_*, and the PR body claims a control-plane gateway.provision.duration OTLP histogram, but the diff contains no control-plane source change (only specs/platform/control-plane-observability.spec.md). Confirm the emitter already exists or land it first; otherwise the widget is permanently "unavailable." Confidence: Medium.
Cross-PR coordination
Coordinate with #251 (GitOps deployment layer) before merging. This PR makes HyperShell bundle its own monitoring stack inside deploy/base (a Prometheus CR plus hypershell-api-server and exporter ServiceMonitors, selected by label hypershell.redhat.io/prometheus-scrape: "true" and scoped to hypershell-system), wired into deploy/base/kustomization.yaml. #251 moves a GitOps layer that defines its own app ServiceMonitor (hypershell-metrics, selector app.kubernetes.io/name: hypershell, no scrape label) and assumes platform/central Prometheus. These are competing ownership models: an app-owned in-cluster Prometheus versus a GitOps/central-monitoring model, plus divergent ServiceMonitor naming/label conventions that would not be scraped by each other. Maintainers need to decide which layer owns the monitoring stack, standardize the ServiceMonitor selector/labels, and settle merge order so the two do not ship duplicate/incompatible scrape definitions.
Findings Summary (ordered by severity, highest first)
- [Major] JWT role-claim precedence prefers
roles/groupsand dropsrealm_access.roles, silently changing the existing role-sync path - Security / RBAC (jwt_roles.go) - [Minor] Cluster-pods metric uses exact equality across two independent series, failing the widget on transient scrape skew - Robustness (metrics-cluster-pods.ts)
- [Minor] Provision-time widget queries a control-plane histogram not emitted in this diff - Traceability / Scope (metrics-gateway-provision-duration.ts)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
| Errors wrapped / no secret leakage in logs or responses | Pass |
| Input validated (config bounds, claim parsing, fixed PromQL) | Pass |
| SecurityContext on all new pod specs | Pass |
| Denied read returns opaque 404 | Pass |
| OpenAPI client generated, not hand-edited | Pass |
| Component registered in CI | Pass |
| Conventional commit message | Pass |
| Optional -> required change has fallback/backfill (role-sync claim precedence) | Needs confirmation |
| // | ||
| // This mirrors components/web-console/bff/src/auth.ts extractRealmRoles so the API | ||
| // server and BFF agree on dashboard-operator access for the same bearer token. | ||
| func extractRealmRolesFromClaims(claims jwt.MapClaims) []string { |
There was a problem hiding this comment.
[Major] Role-claim precedence is prefer-one, and it also drives role sync.
extractRealmRolesFromClaims returns roles if present, else groups, else realm_access.roles, and jwt_roles_test.go confirms realm_access is dropped when roles exists. Because this same function feeds UserProvisioningMiddleware -> SyncJWTRoles (which previously read only realm_access.roles), any token that carries a groups/roles claim not enumerating platform:admin/gateway:creator will silently stop syncing those bindings, regressing authorization.
Please union the claim sources, or document and test the guarantee that the mapped groups claim on tokens reaching the API server always contains the complete realm role set (not just hypershell-*).
| phase_failed_pods + | ||
| phase_unknown_pods; | ||
|
|
||
| if (phaseTotal !== used_pods) { |
There was a problem hiding this comment.
[Minor] Exact equality across two independently-scraped series makes the widget brittle.
count(kube_pod_info) and sum(kube_pod_status_phase{...}) are separate queries that can momentarily skew between scrapes, so phaseTotal !== used_pods will throw and blank the entire pods widget under normal conditions. metrics-cluster-cpu.ts already tolerates a small delta; consider the same tolerance/clamp here.
| ), | ||
| ); | ||
|
|
||
| if (observation_count === 0) { |
There was a problem hiding this comment.
[Minor] This widget depends on an emitter not present in this PR.
The BFF queries gateway_provision_duration_seconds_*, and the PR description claims a control-plane gateway.provision.duration OTLP histogram, but there is no control-plane source change in this diff (only the observability spec). Please confirm the emitter already ships, or land it before/with this so the widget is not permanently "unavailable."
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)