Hypershell-154 UI with adjustments - #214
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 VerdictCOMMENT — the operational-dashboard feature is well-structured (clean hexagonal boundary, domain probes, correlation IDs, thorough component/CI registration, additive tests) and I found no blocker. However, this PR overlaps almost entirely with PR #209 and takes a dashboard-plumbing design that conflicts with PR #211, so maintainers must decide which PR owns this feature before merge. A stray unrelated planning file and an authorization-claim behavior change also warrant attention. Hi team — Amber here. This is a large, mostly-new web-console feature adding an operational dashboard behind a |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
COMMENT — the operational-dashboard feature is well-structured (clean hexagonal boundary, domain probes, correlation IDs, thorough component/CI registration, additive tests) and I found no blocker. However, this PR overlaps almost entirely with PR #209 and takes a dashboard-plumbing design that conflicts with PR #211, so maintainers must decide which PR owns this feature before merge. A stray unrelated planning file and an authorization-claim behavior change also warrant attention.
Hi team — Amber here. This is a large, mostly-new web-console feature adding an operational dashboard behind a packages/operational-dashboard-ui package, an admin gate (client + BFF), and full CI/registration wiring. Overall quality is good; my concerns are concentrated in cross-PR coordination, one accidental file, and authorization-claim semantics.
Cross-PR coordination
I compared this PR against all other open PRs in openshift-online/hypershell. Open PRs at review time: #73, #75, #109, #135, #148, #150, #151, #179, #182, #185, #188, #189, #194, #200, #201, #206, #207, #208, #209, #210, #211, #212, #216.
Two material conflicts need a maintainer decision:
-
PR #209 "HYPERSHELL-112 Dashboard UI" — duplicate solution. #209 and this PR (#214 "Hypershell-112 UI with adjustments") implement the same HYPERSHELL-112 feature. The changed-file lists are near-identical (same
packages/operational-dashboard-uipackage, same web-console shell/route/BFF/role wiring, same i18n and CI registration). #214 is a superset — it addsgateway-status-chart.tsx,gateway-exception-status-counts.ts,metric-trend-change.ts, and an error-state Storybook story. Decision needed: these cannot both merge. Maintainers should pick #214 as the successor and close #209 (or explicitly rebase one onto the other), otherwise the second to merge will collide on every shared file. -
PR #211 "fix(kind): expose gateway metrics and fix control plane connectivity" — incompatible dashboard data-plumbing design. Both PRs create
components/web-console/app/composition/dashboard-composition.tsas a new file with divergent, incompatible contents:- This PR:
dashboardOperations = createDashboardOperations({ controlPlane: createMockDashboardControlPlane() }), importingcreateDashboardOperationsfrom the new@openshift-online/hypershell-operational-dashboard-uipackage and a mock adapter underapp/adapters/mock/. - PR #211: defines its own local
DashboardOperationsinterface and wires a live adaptercreateDashboardControlPlaneAdapter()fromapp/adapters/api/dashboard-control-plane.ts, backed by Prometheus + agateway-management-uimetrics dashboard — it does not use the newoperational-dashboard-uipackage at all.
This is not a mere text merge conflict: the two PRs assume different owning package, different composition interface, and different data source (mock vs live Prometheus). This PR's body says "live API integration can follow in a later PR," which suggests #211 is meant to supply the live data — but as written they disagree on the composition contract and adapter location. Decision needed: agree on the canonical dashboard composition/adapter contract and a merge order (which package owns the dashboard; whether #211 layers its live adapter onto #214'screateDashboardOperationsport rather than replacing the composition).
- This PR:
No material conflict was found with the other open PRs (dependency bumps #73/#75/#135/#188/#189; spec/docs #148/#151/#185/#200; control-plane/API #179/#182/#194/#201/#207/#216; CLI #206; e2e #212; security tooling #109). PR #208/#210 touch gateway-management-ui gateway-connection files but do not overlap this PR's changes.
Findings
Major
-
Stray unrelated file
packages/gateway-management-ui/plan.md(187 lines). Its content is an implementation plan for "Update Daily Note - Jira" (branch004-daily-jira-items,.agents/skills/, Jira REST API) — it has nothing to do with HyperShell or this feature and appears to be an accidental commit. It lands inside the sharedgateway-management-uipackage (included in that component's lint/path scope). Please delete it. Confidence: High. -
extractRealmRolesnow falls back to thegroupsclaim for authorization (bff/src/auth.ts:69-79). The admin gate matches exact literalshypershell-admins/platform:admin. Keycloak group memberships are commonly emitted as paths with a leading slash (e.g./hypershell-admins), which would silently fail to match, and conflating group names with realm-role names for a privilege decision can also over-grant if group and role namespaces differ. Since this feeds the dashboard admin gate, please confirm the exact Keycloak claim/format this targets and normalize (or scope the fallback) so the authorization decision is deterministic. Confidence: Medium.
Minor
-
Client-side admin gate falls through for unauthenticated/undefined sessions (
app/features/dashboard/require-dashboard-admin.tsx:32). The deny branch only triggers whensession?.authenticatedis true; an unauthenticated or failed-load session returnschildren. Server-side BFF enforcement covers the real boundary, so this is defense-in-depth only, but consider an explicit not-authorized/redirect for the non-authenticated case rather than rendering the dashboard shell. Confidence: High. -
domain-probespackage publictypesentry repointed fromdist/*.d.tstosrc/*.ts(components/web-console/domain-probes/package.json:8,11,16). This changes type resolution for every consumer of a shared package and is bundled into a feature PR. It's likely intentional (so the new package type-checks against source without a prior build), but call it out explicitly and confirm it doesn't break published-artifact consumers or build ordering. Confidence: Medium. -
Admin gate is only enforced when
config.oidcIssueris set (bff/src/app.ts:379). In no-auth dev mode the dashboard is open to everyone — acceptable for local dev, but worth a comment so it isn't mistaken for production behavior. Confidence: High.
What looks good
- New package respects the narrow hexagonal boundary: control-plane access is behind a
DashboardControlPlaneport, workflow effects go through aDashboardProbePublisherfan-out, correlation IDs are generated per invocation, and there are no rawconsole.*/telemetry calls in the package source. - Component registration is complete and consistent:
component-paths.json,lint.yml(job + aggregate gate),pnpm-workspace.yaml, rootpackage.jsonbuild/check scripts,swap-component.sh, andCLAUDE.mdall updated together. - Test changes are additive — no pre-existing assertion was weakened or flipped. New BFF tests cover non-admin redirect and admin allow for both roles; the existing unauthenticated-redirect test was extended (not rewritten) to include
/dashboard.
Findings Summary (ordered by severity, highest first):
- [Major] Cross-PR: duplicate of #209 and incompatible dashboard composition vs #211 — needs maintainer decision — Cross-PR Coordination
- [Major] Stray unrelated
packages/gateway-management-ui/plan.mdaccidentally committed — Repo Hygiene - [Major]
extractRealmRolesgroups-claim fallback may misfire authorization (Keycloak group path format) — Security / AuthZ (auth.ts L69) - [Minor] Client admin gate falls through for unauthenticated sessions — Defense in Depth (require-dashboard-admin.tsx L32)
- [Minor]
domain-probestypesrepointed to source.ts— shared-package contract change — API Surface (package.json L8) - [Minor] Dashboard admin gate bypassed when OIDC disabled (dev mode) — Observability/Docs (app.ts L379)
Convention Checklist (only evaluated rows):
| Convention | Result |
|---|---|
No panic() / no raw console.* in production code |
Pass |
| No secrets in logs or responses | Pass |
| Input validated (auth roles/claims) | Fail |
| Narrow hexagonal UI boundary (ports for external effects) | Pass |
| Domain probes for UI observability | Pass |
| Reuse PatternFly / no duplicate UI components | Pass |
| Register every component in CI | Pass |
| Image references consistent across manifests | N/A |
| Test Diff Scrutiny (no weakened assertions) | Pass |
a4ecf2d to
8393fd2
Compare
Amber reviewStatus: Complete VerdictCOMMENT — This is a well-structured, hexagonally-boundaried operational dashboard package with solid BFF+client admin gating and good test coverage; the concerns are around shipping mock data as if it were live, a dev-only hardcoded hostname, and a few observability/brand nits. Nothing here rises to a blocker, but the mock-data and hostname items warrant a maintainer decision before this becomes visible to admins. The new Findings[Major] Mock metrics are wired as the production dependency. [Major] Dev-only hardcoded hostname in [Minor] Raw adapter error message rendered in the UI. [Minor] Declared [Minor] Hardcoded hex color literals in the status chart. Cross-PR coordinationNo material cross-PR coordination issue requires maintainer action. Findings Summary (ordered by severity, highest first)
Convention Checklist
|
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
COMMENT — This is a well-structured, hexagonally-boundaried operational dashboard package with solid BFF+client admin gating and good test coverage; the concerns are around shipping mock data as if it were live, a dev-only hardcoded hostname, and a few observability/brand nits. Nothing here rises to a blocker, but the mock-data and hostname items warrant a maintainer decision before this becomes visible to admins.
The new @openshift-online/hypershell-operational-dashboard-ui package is cleanly layered (application ports, probes, presentation), CI registration is complete (component-paths, lint.yml, aggregate gate, Dockerfile, .dockerignore, workspace), and the admin gate is enforced in both the BFF (/dashboard + dashboard. host) and the client (RequireDashboardAdmin) with additive, well-targeted tests. Test diff scrutiny passed: the modified assertions in session-adapter.test.ts and bff/test/auth.test.ts are additive (new authEnabled field, new admin-gate cases) and do not flip or delete any prior guarantee.
Findings
[Major] Mock metrics are wired as the production dependency.
components/web-console/app/composition/dashboard-composition.ts injects createMockDashboardControlPlane() as the real controlPlane, so authenticated admins are shown fabricated numbers (with an artificial 2s delay) presented as live operational metrics. The PR body acknowledges this is interim, but there is no visible "sample data" indicator or feature flag. Admins could make operational judgments on fake data. Recommend gating the route behind a feature flag or rendering an unmistakable "sample data" banner until the live adapter lands.
[Major] Dev-only hardcoded hostname in home.tsx diverges from the BFF host model.
components/web-console/app/routes/home.tsx:22 decides whether / renders the dashboard via globalThis.location.hostname === "dashboard.hypershell.localhost", while the BFF (bff/src/app.ts) uses a generic dashboard. prefix (isDashboardHost). In any non-Kind environment (e.g. dashboard.<prod-domain>) the SPA at / on the dashboard host will render the Gateways page instead of the dashboard, so client behavior silently disagrees with the BFF gate. This violates "Separate configuration from code." Derive the check from the same dashboard. prefix logic or from runtime config rather than a literal localhost.
[Minor] Raw adapter error message rendered in the UI.
packages/operational-dashboard-ui/src/pages/operational-dashboard-page.tsx:472 and :482 render metricsQuery.error.message directly into the Alert. Once a live control-plane adapter exists, this can surface internal/backend detail to the browser and is unlocalized. Prefer the localized loadErrorBody/refreshErrorBody for display and route the raw message to a probe/log.
[Minor] Declared dashboard.layout.template.invalid probe is never emitted.
operational-dashboard-page.tsx:322 silently falls back to the default template when isValidSavedTemplate fails (and the surrounding catch also swallows parse errors), yet the probe contract declares dashboard.layout.template.invalid. Emit that probe on the invalid/parse-failure paths so the declared observability signal is actually produced, or drop the unused probe name.
[Minor] Hardcoded hex color literals in the status chart.
packages/operational-dashboard-ui/src/dashboard/gateway-status-chart.tsx:20-25 hardcodes hex values. They do map to approved palette swatches and correctly avoid using red for failure (failure uses danger-orange), which is good — but UI-BRAND-02 asks for named design tokens and flags hard-coded duplicates. Prefer PatternFly chart theme tokens over literal hex.
Cross-PR coordination
No material cross-PR coordination issue requires maintainer action.
Findings Summary (ordered by severity, highest first)
- [Major] Mock control-plane adapter wired as the production dependency; admins see fabricated metrics without a "sample data" indicator - Trust / Product (dashboard-composition.ts L6, mock/dashboard-control-plane.ts L12)
- [Major] Hardcoded
dashboard.hypershell.localhostinhome.tsxdiverges from the BFF's genericdashboard.host model; dashboard won't render on/in non-Kind envs - Config-from-code (home.tsx L22) - [Minor] Raw, unlocalized
error.messagerendered in dashboard Alerts - Observability / i18n (operational-dashboard-page.tsx L472, L482) - [Minor] Declared
dashboard.layout.template.invalidprobe never emitted on the invalid/parse-failure path - Observability (operational-dashboard-page.tsx L322) - [Minor] Hardcoded hex chart colors instead of named design tokens - Brand color (gateway-status-chart.tsx L20-25)
Convention Checklist
| Convention | Result |
|---|---|
No panic() / no raw console in production code |
Pass |
| Errors handled, not silently swallowed | Pass (probe-backed) |
| No secrets in logs or responses | Pass |
| Restricted admin access enforced server-side (BFF) and client-side | Pass |
| Test Diff Scrutiny (no flipped/removed guarantees) | Pass |
| Component registered in CI (paths, lint job, aggregate, Docker) | Pass |
| Separate configuration from code | Fail (hardcoded dashboard hostname) |
| Narrow hexagonal UI boundary (ports for effects) | Pass |
| Domain probes for observability (no raw telemetry) | Pass |
| PatternFly reuse / named brand tokens | Fail (hardcoded hex) |
| i18n for user-facing strings | Partial (raw error.message unlocalized) |
| import { createMockDashboardControlPlane } from "../adapters/mock/dashboard-control-plane"; | ||
|
|
||
| export const dashboardOperations = createDashboardOperations({ | ||
| controlPlane: createMockDashboardControlPlane(), |
There was a problem hiding this comment.
[Major] The mock control-plane adapter is wired here as the production dependency, so authenticated admins are shown fabricated metrics (see the artificial 2s delay in mock/dashboard-control-plane.ts) presented as live data. The PR body notes this is interim, but there's no visible "sample data" indicator or feature flag. Consider gating the route behind a flag or rendering an unmistakable sample-data banner until the live adapter lands, so admins don't act on fake numbers.
| const navigate = useNavigate(); | ||
| const [searchParameters, setSearchParameters] = useSearchParams(); | ||
| const hostname = globalThis.location.hostname; | ||
| if (hostname === "dashboard.hypershell.localhost") { |
There was a problem hiding this comment.
[Major] This hardcodes the Kind-only hostname dashboard.hypershell.localhost, while the BFF (bff/src/app.ts isDashboardHost) uses a generic dashboard. prefix. In any non-Kind environment (e.g. dashboard.<prod-domain>) the SPA at / on the dashboard host will render the Gateways page instead of the dashboard, so client behavior silently disagrees with the BFF gate. This violates "Separate configuration from code" - derive the check from the same dashboard. prefix logic or runtime config.
| variant="danger" | ||
| > | ||
| {metricsQuery.error instanceof Error | ||
| ? metricsQuery.error.message |
There was a problem hiding this comment.
[Minor] Rendering metricsQuery.error.message verbatim (here and at ~L482) can surface internal/backend detail to the browser once a live adapter exists, and it's unlocalized. Prefer the localized loadErrorBody/refreshErrorBody for display and route the raw message to a probe/log.
| } | ||
|
|
||
| const parsed = JSON.parse(rawTemplate) as ExtendedTemplateConfig; | ||
| if (!isValidSavedTemplate(parsed)) { |
There was a problem hiding this comment.
[Minor] When a saved template is invalid this falls back to default silently (and the surrounding catch also swallows parse errors), yet the probe contract declares dashboard.layout.template.invalid. Emit that probe on the invalid/parse-failure paths so the declared observability signal is actually produced, or drop the unused probe name.
| * | ||
| * @see https://www.patternfly.org/components/alert | ||
| * @see https://www.patternfly.org/components/label | ||
| */ |
There was a problem hiding this comment.
[Minor] These hex literals map to approved palette swatches and correctly avoid red for failure (uses danger-orange), which is good - but UI-BRAND-02 asks for named design tokens and flags hard-coded duplicates. Prefer PatternFly chart theme tokens over literal hex.
…us wiring - Remove duplicate ServiceMonitor (deploy/base/servicemonitor.yaml); keep single canonical copy under deploy/base/prometheus/ - Replace 75k-line vendored prometheus-operator-bundle.yaml with an upstream URL reference; add to both infrastructure/ and infrastructure-no-cnpg/ kustomizations so all Kind DB modes work - Wire Prometheus stack into kind-up: wait for prometheus-operator after infrastructure apply, then apply deploy/base/prometheus/ after the main component rollout (CRDs exist, namespace already present) - Implement GET /api/hypershell/v1/metrics/gateways: add CountByPhase to GatewayService interface + sqlGatewayService, MetricsGateways handler, and auth-gated route; fix fetch URL in gateway-metrics-data.ts - Mount GatewayMetricsDashboard on /metrics route in web-console: add route to route-contract.json, routes.ts, new metrics.tsx, and BFF isApplicationRoute; add i18n keys to locales/en.json - Add seccompProfile: RuntimeDefault to Prometheus CR securityContext - Remove dashboard-composition.ts and dashboard-control-plane.ts to avoid collision with PRs #209 and #214 which own that entry point - Fix index-based JSON6902 patch in database-deployment component: --metrics-server-bindaddress shifted the serve command array by one, so the sslmode replace op index increments from 12 to 13 Co-Authored-By: Crush <crush@charm.land>
…us wiring - Remove duplicate ServiceMonitor (deploy/base/servicemonitor.yaml); keep single canonical copy under deploy/base/prometheus/ - Replace 75k-line vendored prometheus-operator-bundle.yaml with an upstream URL reference; add to both infrastructure/ and infrastructure-no-cnpg/ kustomizations so all Kind DB modes work - Wire Prometheus stack into kind-up: wait for prometheus-operator after infrastructure apply, then apply deploy/base/prometheus/ after the main component rollout (CRDs exist, namespace already present) - Implement GET /api/hypershell/v1/metrics/gateways: add CountByPhase to GatewayService interface + sqlGatewayService, MetricsGateways handler, and auth-gated route; fix fetch URL in gateway-metrics-data.ts - Mount GatewayMetricsDashboard on /metrics route in web-console: add route to route-contract.json, routes.ts, new metrics.tsx, and BFF isApplicationRoute; add i18n keys to locales/en.json - Add seccompProfile: RuntimeDefault to Prometheus CR securityContext - Remove dashboard-composition.ts and dashboard-control-plane.ts to avoid collision with PRs #209 and #214 which own that entry point - Fix index-based JSON6902 patch in database-deployment component: --metrics-server-bindaddress shifted the serve command array by one, so the sslmode replace op index increments from 12 to 13 Co-Authored-By: Crush <crush@charm.land>
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>
8393fd2 to
a4eaf4b
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, well-tested feature PR that adds an operational dashboard as a new private React package and wires it into the web console with an admin-only access gate. The code follows the project's hexagonal-boundary and error-handling conventions cleanly; my findings are all Minor (observability wiring, a header-derived redirect, and duplicated security constants) with no blockers.
I reviewed the security-critical BFF auth/role changes, the admin gate, the new package's application layer, and the modified test files against the HyperShell security, control-plane, and review conventions.
What works well
- Admin gate is defense-in-depth. Access is enforced server-side in the BFF (
onRequesthook redirects non-admins away from/dashboard, andsendApplicationre-checks) and again client-side viaRequireDashboardAdmin. Enforcement is correctly gated onconfig.oidcIssuerso no-auth dev mode stays open, which is documented in-line. extractRealmRolesis robust and well-tested. It handles theroles/groups/realm_access.rolesprecedence, normalizes leading-slash group paths, and ignores non-string entries, with matching unit tests.- Test diff scrutiny: clean. The only modified assertions (
session-adapter.test.ts,bff/test/auth.test.ts) are strictly additive - they add the newauthEnabledfield and new admin/non-admin route cases. No pre-existing guarantee was flipped or removed. - Error handling / observability in the package.
localStorageread/write andJSON.parseare guarded; parse/persist failures emit typed probes instead of throwing. The workflow layer publishes started/completed/cancelled/failed probes with correlation IDs. - CI + config registration is complete. New package registered in
component-paths.json, a dedicatedlint-operational-dashboard-uijob added and included in the aggregate gate, plus Dockerfile,.dockerignore,pnpm-workspace.yaml,CLAUDE.md, and i18n extraction globs all updated consistently.
Findings (all Minor)
- Dashboard domain probes are wired to the default no-op publisher in production composition (
dashboard-composition.ts+DashboardUiProviderinapplication-shell.tsx), so the typed workflow/layout facts are dropped. The gateway composition wires a real fan-out publisher; the dashboard should follow. See inline comment. consoleRedirectForDashboardHostbuilds its redirect target from the client-controlledHostheader. Low risk (self-affecting), but prefer trusted config or an allowlist. See inline comment.- Admin role constants and helper are duplicated across the BFF and frontend with no drift protection. These are security-relevant; consider a shared module or a sync test. See inline comment.
Cross-PR coordination
No material cross-PR coordination issue requires maintainer action.
Findings Summary (ordered by severity, highest first):
- [Minor] Dashboard domain probes never wired into the fan-out publisher (defaults to no-op) - Observability / Convention (dashboard-composition.ts L5, application-shell.tsx L149)
- [Minor] Redirect target derived from client-controlled
Hostheader - Security (app.ts L73) - [Minor] Duplicated admin-role constants/helper across BFF and frontend with no drift guard - Maintainability / Security (roles.ts L1)
Convention Checklist:
| Convention | Result |
|---|---|
| No secrets in logs or responses | Pass |
| Input validated (roles, host, JSON parse guarded) | Pass |
| Server-side authorization enforced (admin gate) | Pass |
| Error paths propagate / no silent swallowing | Pass |
| Domain probes published through fan-out port | Fail (defaults to no-op in production) |
| Test diff scrutiny (no flipped/removed guarantees) | Pass |
| CI component registration complete | Pass |
| Image/config references consistent across stack | Pass |
| Conventional commit messages | Pass |
|
|
||
| import { createMockDashboardControlPlane } from "../adapters/mock/dashboard-control-plane"; | ||
|
|
||
| export const dashboardOperations = createDashboardOperations({ |
There was a problem hiding this comment.
[Minor] Dashboard domain probes are never wired into the fan-out publisher in production. createDashboardOperations is called with only controlPlane, so probes defaults to noopDashboardProbePublisher. The dashboard already emits typed workflow probes (dashboard.workflow.started/completed) and layout probes (dashboard.layout.template.*), but with a no-op publisher these facts are silently dropped in the browser. Compare gateway-composition.ts, which wires probes: gatewayObservability.probes into the fan-out port. CLAUDE.md's "Domain probes for UI observability" convention expects these facts to be published through the fan-out port. Since metrics are still mock data, this is Minor, but please wire the probe publisher (here and in the DashboardUiProvider below) before/with the live-API follow-up so dashboard observability isn't a no-op. Confidence: High.
| <DashboardUiProvider | ||
| dashboard={dashboardOperations} | ||
| navigation={dashboardNavigation} | ||
| usesSampleData |
There was a problem hiding this comment.
[Minor] DashboardUiProvider omits probes. The provider accepts an optional probes publisher but none is passed here, so any probe consumer resolved via useDashboardUi() gets undefined and dashboard observability stays a no-op in production. Pass the same fan-out publisher used for gateways (see gateway-composition.ts). Confidence: High.
| return host.startsWith(DASHBOARD_HOST_PREFIX); | ||
| } | ||
|
|
||
| function consoleRedirectForDashboardHost(request: FastifyRequest): string { |
There was a problem hiding this comment.
[Minor] Redirect target is derived from the client-controlled Host header. consoleRedirectForDashboardHost builds the Location from request.headers.host, swapping the dashboard. prefix for console.. Because it only reshapes the prefix and prepends console., this is low risk (a spoofed Host only affects the spoofing client), but it is still an externally-influenced redirect. Consider deriving the console host from trusted config rather than the request header, or validating the host against an allowlist. Confidence: Medium.
| @@ -0,0 +1,14 @@ | |||
| /** Keycloak realm role for HyperShell administrators. */ | |||
There was a problem hiding this comment.
[Minor] Role constants and hasDashboardAdminRole are duplicated between bff/src/roles.ts and app/lib/session-roles.ts (and isDashboardHost between bff/src/app.ts and app/lib/dashboard-host.ts). Duplication across the process boundary is understandable, but these are security-relevant constants (admin role names) that must not drift. Consider a shared workspace module or a test that asserts the two definitions stay in sync. Confidence: Medium.
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>
a4eaf4b to
5184bc0
Compare
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This PR adds a well-structured, admin-gated operational dashboard with proper defense-in-depth (server-side role enforcement in the BFF plus a client-side gate), thorough additive tests, complete CI registration, and a clean hexagonal/probe architecture. No blockers or security regressions were found; the items below are minor, plus one cross-PR coordination point that needs a maintainer decision.
What's good
- Access control is enforced server-side, not just in React.
bff/src/app.tsgates/dashboard(and the dashboard-host root) in both theonRequesthook andsendApplication, andRequireDashboardAdminadds a client-side gate. No-auth dev mode is handled consistently via the newauthEnabledflag. - Test changes are additive and honest. The modified assertions in
session-adapter.test.tsandauth.test.tsadd the newauthEnabledfield / new dashboard cases rather than flipping an existing guarantee. The 404-vs-401 distinction in the session adapter (no-auth vs unauthenticated) is a genuine improvement and is covered. - CI and packaging are fully wired.
.github/component-paths.json,lint.yml(newlint-operational-dashboard-uijob + aggregate gate),.dockerignore,Dockerfile,pnpm-workspace.yaml, i18n extraction globs, andCLAUDE.mdwere all updated together.
Minor findings
- [Minor]
dashboard-control-plane.tsships an artificialsetTimeout(…, 2000)("just for demos") in the composition path wired into the real app shell, and the delay is not abort-aware (onlythrowIfAborted()runs before it). Remove it or confine it to Storybook/fixtures. Observability/UX - [Minor] Role/host logic is duplicated across the client/server boundary:
bff/src/roles.tsandapp/lib/session-roles.tsare byte-identical, andapp/lib/dashboard-host.tsre-encodes thedashboard.prefix logic embedded inbff/src/app.ts. Duplication across module systems is acceptable, but the admin role names and host prefix are now defined in multiple places and can drift; consider a shared source or a cross-check test. Maintainability - [Minor]
operational-dashboard-ui/package.jsondepends on@patternfly/widgetized-dashboard@1.0.0-prerelease.6, a prerelease pulled into a shipped package. Confirm this is intentional and pinned; a prerelease can introduce breaking changes without a semver signal. Dependency risk - [Minor / forward-looking] Server-side enforcement currently gates the HTML route and nav only; the
/api/*proxy has no dashboard-admin check. That is fine today because metrics are mock/client-side, but when live metrics replace the mock adapter the backing endpoint must add server-side role enforcement so non-admins cannot fetch the data directly. Security (future)
Cross-PR coordination
A competing/overlapping dashboard effort needs a maintainer decision with #211. That PR introduces a GatewayMetricsDashboard (real Prometheus-backed pipeline) inside the existing gateway-management-ui package with a /metrics route, while this PR introduces a separate operational dashboard in a new operational-dashboard-ui package with a /dashboard route backed by mock data. Two decisions are required: (a) whether these are complementary surfaces or a duplicated dashboard direction, and specifically whether this PR's operational dashboard should consume #211's real metrics pipeline instead of the mock adapter rather than each building its own data path; and (b) the shared BFF route allowlist (isApplicationRoute in bff/src/app.ts), app/routes.ts, and route-contract.json are extended by both PRs, so whichever merges second must reconcile the allowlist to include both routes (a dropped entry causes the other route to 404/misroute) — this is an ordering/integration constraint, not just a text merge.
Findings Summary (ordered by severity, highest first)
- [Minor] Artificial 2s (non-abort-aware) delay in the mock dashboard adapter on the production composition path - Observability/UX
- [Minor] Duplicated admin-role/host constants across BFF and app (drift risk) - Maintainability
- [Minor] Prerelease
@patternfly/widgetized-dashboarddependency in a shipped package - Dependency risk - [Minor]
/api/*proxy lacks dashboard-admin enforcement for the eventual live metrics endpoint - Security (future)
Convention Checklist
| Convention | Result |
|---|---|
| No secrets in logs or responses | Pass |
| Input validated (correlation id, role filtering) | Pass |
| Register every component in CI | Pass |
| Conventional commit messages | Pass |
| Domain probes for UI observability (no raw console/telemetry) | Pass |
| PatternFly 6 reuse / no duplicate UI primitives | Pass |
| Narrow hexagonal UI boundary (ports for effects) | Pass |
| Test Diff Scrutiny (modified assertions are additive, not removed guarantees) | Pass |
| return { | ||
| async getOperationalMetrics(context: DashboardInvocationContext) { | ||
| context.signal?.throwIfAborted(); | ||
|
|
There was a problem hiding this comment.
This 2s artificial delay ("just for demos") sits in the composition wired into the real application shell, so it ships in the actual web console, not just Storybook. It is also not abort-aware: throwIfAborted() runs before the wait but a cancellation during the 2s is ignored, so a navigation-away still resolves the promise. Remove the delay or move this artificial latency into a Storybook/fixture-only adapter.
| PLATFORM_ADMIN_ROLE, | ||
| ]); | ||
|
|
||
| export function hasDashboardAdminRole(roles: readonly string[]): boolean { |
There was a problem hiding this comment.
This file is byte-identical to bff/src/roles.ts, and the dashboard. host prefix is likewise defined both here-adjacent (dashboard-host.ts) and inline in bff/src/app.ts. Duplication across the client/server module boundary is understandable, but the admin role names and host prefix are now security-relevant constants defined in multiple places that can silently drift. Consider a single shared source or a test that asserts the two lists stay in sync.
| "@patternfly/react-charts": "8.6.1", | ||
| "@patternfly/react-core": "6.6.0", | ||
| "@patternfly/react-icons": "6.6.0", | ||
| "@patternfly/widgetized-dashboard": "1.0.0-prerelease.6", |
There was a problem hiding this comment.
@patternfly/widgetized-dashboard@1.0.0-prerelease.6 is a prerelease being pulled into a shipped package. Prereleases can ship breaking changes without a semver bump. Please confirm this is intentional and note the plan to move to a stable release before this dashboard graduates from mock data.
| return; | ||
| } | ||
| if ( | ||
| requiresDashboardAdminAccess(pathname, request.headers.host) && |
There was a problem hiding this comment.
Nice defense-in-depth on the HTML route here. Forward-looking note: enforcement currently covers only the application/HTML routes and nav; the /api/* proxy below has no dashboard-admin check. That is fine while metrics are mock/client-side, but when the mock adapter is replaced with a live metrics endpoint, the data path must add server-side role enforcement so a non-admin cannot fetch dashboard data directly by calling the API through the proxy.
…penshift-online#211) * fix(kind): expose gateway metrics and fix control plane connectivity - Add HTTPRoute for metrics.hypershell.localhost routing to the API server's dedicated metrics port (4433), making Prometheus-format gateway metrics browsable in the local Kind cluster - Bind the metrics server to 0.0.0.0:4433 (was localhost-only) so the gateway can reach it from outside the pod - Fix the controller NetworkPolicy to explicitly allow all egress; kindnet's implementation implicitly blocks all egress when any NetworkPolicy selects a pod, which prevented the control plane from reaching CoreDNS and caused gateways to remain in a blank phase - Use fully-qualified service names for the control plane's gRPC and HTTP API server addresses in the Kind overlay to avoid DNS search domain ambiguity - Print the metrics URL in the kind-up summary alongside the other service URLs Co-Authored-By: Crush <crush@charm.land> * feat(deploy): add ServiceMonitor for Prometheus scraping Brings in the ServiceMonitor from openshift-online#202, wiring Prometheus Operator scraping to the metrics port (4433) already exposed by the API server. Co-Authored-By: Crush <crush@charm.land> * fix(kind): rename metrics hostname to observability.hypershell.localhost Co-Authored-By: Crush <crush@charm.land> * fix(e2e): move ServiceMonitor out of base to avoid CRD-not-found failure The ServiceMonitor was included in deploy/base/kustomization.yaml which is applied unconditionally, causing kind-up to fail in environments without Prometheus Operator installed (including CI). Moves it into deploy/base/prometheus/ alongside the existing Prometheus resources, where it is only applied by overlays that have Prometheus Operator available. Co-Authored-By: Crush <crush@charm.land> * feat(metrics): add gateway metrics dashboard and Prometheus infrastructure Introduces a gateway metrics dashboard in the management UI backed by a Prometheus stack wired into the Kind dev cluster. Includes RBAC, scrape config, the Prometheus operator bundle, a new metrics data layer, and the platform spec describing the desired dashboard state. Co-Authored-By: Crush <crush@charm.land> * fix(ci): resolve lint and policy failures from metrics dashboard files - Replace import from non-existent operational-dashboard-ui package with locally defined types in the web-console adapter, eliminating all unsafe-assignment and unsafe-call ESLint errors - Export metrics symbols from the gateway-management-ui public index so the web-console adapter can resolve them with full type safety - Replace em dashes in the metrics dashboard spec with hyphens to satisfy the repository forbidden-terms policy check Co-Authored-By: Crush <crush@charm.land> * fix(ci): apply Prettier formatting to dashboard control plane adapter Co-Authored-By: Crush <crush@charm.land> * fix(ci): sync i18n catalog with new gateway metrics dashboard messages Co-Authored-By: Crush <crush@charm.land> * fix(metrics): address review blockers on gateway metrics and Prometheus wiring - Remove duplicate ServiceMonitor (deploy/base/servicemonitor.yaml); keep single canonical copy under deploy/base/prometheus/ - Replace 75k-line vendored prometheus-operator-bundle.yaml with an upstream URL reference; add to both infrastructure/ and infrastructure-no-cnpg/ kustomizations so all Kind DB modes work - Wire Prometheus stack into kind-up: wait for prometheus-operator after infrastructure apply, then apply deploy/base/prometheus/ after the main component rollout (CRDs exist, namespace already present) - Implement GET /api/hypershell/v1/metrics/gateways: add CountByPhase to GatewayService interface + sqlGatewayService, MetricsGateways handler, and auth-gated route; fix fetch URL in gateway-metrics-data.ts - Mount GatewayMetricsDashboard on /metrics route in web-console: add route to route-contract.json, routes.ts, new metrics.tsx, and BFF isApplicationRoute; add i18n keys to locales/en.json - Add seccompProfile: RuntimeDefault to Prometheus CR securityContext - Remove dashboard-composition.ts and dashboard-control-plane.ts to avoid collision with PRs openshift-online#209 and openshift-online#214 which own that entry point - Fix index-based JSON6902 patch in database-deployment component: --metrics-server-bindaddress shifted the serve command array by one, so the sslmode replace op index increments from 12 to 13 Co-Authored-By: Crush <crush@charm.land> --------- Co-authored-by: Crush <crush@charm.land>
kdoberst
changed the title
2 participants
Summary
@openshift-online/hypershell-operational-dashboard-uipackage (widgetized layout, utilization charts, summary metrics).hypershell-adminsorplatform:adminrealm roles.Changes
packages/operational-dashboard-uipackage with dashboard page, charts, and mock metrics fixture.RequireDashboardAdmin).Metrics are served from a mock control plane adapter for now; live API integration can follow in a later PR.
Test plan
pnpm --filter @openshift-online/hypershell-operational-dashboard-ui checkpnpm --filter @openshift-online/hypershell-web-console test:run(or the web-console unit test command you normally use)make kind-web-console-up— and go to: https://console.hypershell.localhost/dashboard OR run story book:cd components/web-console && pnpm run storybook