Repository navigation
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 |
amber-review-bot
left a comment
amber-review-bot
left a comment
There was a problem hiding this comment.
Verdict
This PR cleanly shifts the operational dashboard's aggregate widgets from paginated REST list calls to Prometheus-backed API-server collectors and gated BFF routes, and the collector/route/adapter wiring is consistent, well-tested, and follows the established secure-metrics patterns. I found no blocking correctness or security issues; the notes below are non-blocking, and the main item for maintainers is a cross-PR design overlap on the registered-users dashboard widget.
What looks good
- Collectors emit
prometheus.NewInvalidMetricon DB errors and always emit the scalar totals, so an empty fleet correctly reports0rather than a 502, while a truly missing series maps to 502 as intended. - BFF routes reuse
fetchMetrics/namespaceSelectorand dedup API replica samples viamax(...)/max by (...)PromQL plusMath.maxin parsing. - Mocks and interfaces are updated in lockstep (
SumActiveSandboxCount,CountRegistered,InventorySnapshot), andstrPtrhelpers do not collide with existing package declarations. - The flipped assertions in
roles.test.tsandauth.test.ts(hypershell-admins no longer reaching the dashboard) are an intentional authorization tightening that is explicitly documented in the PR body's Compatibility notes and paired with the dev Keycloak realm update, so they are justified rather than a silently removed guarantee.
Non-blocking findings
- [Minor] Full-table load per scrape for inventory collectors.
managedClustersandmanagedDatabasesinventory snapshots calld.All(ctx)and aggregate in Go on every scrape, unlike the gateway collector which aggregates in SQL (CountByPhase). At current fleet scale this is fine, but it is inconsistent with the same PR's other collector and re-introduces a load-everything pattern the PR is otherwise trying to move away from. Consider aGROUP BYaggregate if inventory row counts grow. - [Minor] Dashboard access tightening is a deploy-time compatibility step. Removing
hypershell-adminsfromDASHBOARD_ADMIN_ROLESmeans production operators who hold onlyhypershell-adminslose dashboard and fleet-wide gateway list access. This is documented, but ensure production Keycloak realms grantplatform:adminto the intended operators before/with rollout.
Cross-PR coordination
Another open pull request (#264, same author) also redefines the operational dashboard's Registered users widget and rewrites the same specs/platform/registered-users.spec.md, but with an incompatible data-source design: it adds a REST GET /api/hypershell/v1/users/stats endpoint returning a richer UserActivityStats payload (registration/active trends) consumed directly through the SDK, whereas this PR sources the widget from a Prometheus gauge (hypershell_users_registered_total) via a new BFF GET /api/metrics/registered-users route. The two PRs make competing edits to registered-users.spec.md, components/web-console/app/adapters/api/dashboard-control-plane.ts, packages/operational-dashboard-ui/DATA_SOURCES.md, packages/operational-dashboard-ui/src/application/dashboard-types.ts, and components/api-server/plugins/users/{dao.go,plugin.go}. Maintainers need to decide which registered-users data source is canonical (Prometheus aggregate vs REST stats endpoint) and in what order the two land, since merging both as-is would leave contradictory spec requirements and two divergent adapter/DAO implementations for the same widget.
Findings Summary (ordered by severity, highest first)
- [Minor] Inventory collectors load entire tables per scrape instead of aggregating in SQL - Performance / Consistency (managedClusters/dao.go L97, managedDatabases/dao.go L121)
- [Minor] Dashboard/fleet-list access tightened to
platform:admin; requires production Keycloak grant - Access Control / Compatibility (shared/dashboard-roles.ts L7)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
| Errors wrapped / propagated (no swallowed failures) | Pass |
| No secrets in logs or responses | Pass |
| Input validated (bucket normalization, PromQL sample validation) | Pass |
Reconcile pattern / idempotent metric registration (sync.Once) |
Pass |
| Interfaces and mocks updated together | Pass |
| Test Diff Scrutiny (flipped assertions justified + documented) | Pass |
| Image references consistent across manifests | N/A |
| Conventional commit / PR hygiene | Pass |
| return &managedCluster, nil | ||
| } | ||
|
|
||
| func (d *sqlManagedClusterDao) InventorySnapshot(ctx context.Context, evaluationTime time.Time) (*ClusterInventorySnapshot, error) { |
There was a problem hiding this comment.
InventorySnapshot calls d.All(ctx) and buckets in Go on every Prometheus scrape. The gateway collector in this same PR aggregates in SQL (CountByPhase); doing the same here (GROUP BY status, provider, region plus a windowed created-count) would avoid loading the full table each scrape and keep the collectors consistent. Not blocking at current scale.
| return count > 0, nil | ||
| } | ||
|
|
||
| func (d *sqlManagedDatabaseDao) InventorySnapshot(ctx context.Context) (*DatabaseInventorySnapshot, error) { |
There was a problem hiding this comment.
Same note as the managed-cluster collector: this loads every row via All(ctx) and aggregates in Go on each scrape. A GROUP BY status count would be cheaper and matches the gateway collector's SQL-side aggregation. Minor / non-blocking.
| HYPERSHELL_ADMIN_ROLE, | ||
| PLATFORM_ADMIN_ROLE, | ||
| ]); | ||
| const DASHBOARD_ADMIN_ROLES = new Set([PLATFORM_ADMIN_ROLE]); |
There was a problem hiding this comment.
Dropping HYPERSHELL_ADMIN_ROLE from the dashboard-admin set is an intentional, documented tightening. Flagging as a deploy dependency: operators who currently hold only hypershell-admins will lose dashboard and fleet-wide gateway list access, so production Keycloak realms must grant platform:admin before this ships. The dev realm is updated in deploy/base/keycloak/keycloak.yaml, but confirm the production realm coordination.
0ca2048 to
0b5d435
Compare
Amber reviewStatus: Complete |
amber-review-bot
left a comment
amber-review-bot
left a comment
There was a problem hiding this comment.
Verdict
The Prometheus-first dashboard refactor is well structured, follows the existing collector/route patterns, and ships genuinely additive test coverage (partial-failure and abort-propagation cases included). The two things that need attention are a documented but real breaking RBAC change and a material cross-PR design conflict over the "registered users" dashboard widget - neither is a code defect, but both need a maintainer decision before merge.
Summary
This PR shifts operational-dashboard aggregates from paginated REST list calls to Prometheus-backed BFF routes fed by new API-server inventory collectors, and tightens dashboard/fleet-wide-gateway-list access to platform:admin only. The code is clean and convention-compliant; the primary risks are an authorization contract change and coordination with a competing dashboard PR.
Findings
[Major] Dashboard access silently removed from hypershell-admins (authz contract change) - Security / Test Diff Scrutiny
components/web-console/shared/dashboard-roles.ts:7 drops HYPERSHELL_ADMIN_ROLE from DASHBOARD_ADMIN_ROLES, and several pre-existing tests flip from allow to deny (roles.test.ts:6, session-roles.test.ts, require-dashboard-admin.test.tsx, bff/test/auth.test.ts). This is a deliberate tightening: operators who hold only hypershell-admins in an already-running environment lose /dashboard, /metrics, and all /api/metrics/* access with no automatic fallback or backfill - the only remediation is a manual Keycloak grant of platform:admin. This clears the Test Diff Scrutiny bar because it is called out in the PR body's Compatibility notes, the platform:admin companion tests remain, and the assertions were renamed rather than silently mutated. It is flagged here so maintainers consciously accept the operator-facing migration (the dev overlay is updated in deploy/base/keycloak/keycloak.yaml, but production realms are not). Confidence: High.
[Minor] Unbounded label cardinality on cluster inventory gauge - Observability
components/api-server/plugins/managedClusters/metrics.go:50-53 emits hypershell_managed_clusters_inventory_total{status,provider,region}. If provider/region can hold free-form values, this can grow Prometheus series cardinality on every scrape. Please confirm these are drawn from a bounded/validated value set (the unknown bucketing helps, but does not bound distinct valid values). Confidence: Medium.
Cross-PR coordination
PR #264 (HYPERSHELL-279, "user registrations") and this PR both redesign the operational dashboard's Registered users widget with incompatible data-source architectures. This PR sources the widget from a Prometheus gauge (hypershell_users_registered_total via GET /api/metrics/registered-users, adapter fetches the BFF route), while #264 sources it from a new REST activity-stats API (GET /api/hypershell/v1/users/stats / client.users.activityStats(), with login recording and sparklines). Both edit the same authoritative surfaces: specs/platform/registered-users.spec.md, specs/web-console/operational-dashboard.spec.md, the registered-users dashboard metric source in dashboard-control-plane.ts, and components/api-server/plugins/users/{dao.go,plugin.go}. These are competing solutions for the same widget and cannot both land as written. Maintainers need to decide which data-source model wins (Prometheus gauge vs REST activity-stats), reconcile the two spec rewrites, and define a merge order for the shared users DAO/plugin and adapter changes.
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
| Errors wrapped/propagated (consistent with existing DAO style) | Pass |
| No secrets in logs or responses | Pass |
Metrics routes require auth + Cache-Control: no-store |
Pass |
| Reconcile/collector patterns consistent with existing metrics | Pass |
| Test Diff Scrutiny (modified assertions justified) | Pass (documented) |
| Prometheus metric label cardinality bounded | Verify |
Findings Summary (ordered by severity, highest first):
- [Major] Dashboard/metrics access removed from
hypershell-admins; operator migration required - Security / Test Diff Scrutiny (dashboard-roles.ts:7) - [Minor] Potential unbounded label cardinality on
hypershell_managed_clusters_inventory_total- Observability (managedClusters/metrics.go:50)
3 participants
Summary
Shift operational dashboard aggregate widgets from paginated HyperShell REST list APIs to Prometheus-backed BFF routes, with new API server inventory collectors emitting fleet-wide gauges on
/metrics.Restrict operational dashboard and fleet-wide gateway list visibility to
platform:adminonly. New Prometheus query paths follow the secure metrics patterns from HYPERSHELL-329 (fetchMetrics, namespace filtering, missing-series handling, replica deduplication).Fixes HYPERSHELL-332
Motivation
The operational dashboard was fanning out across paginated
gateways.list,users.list,managedClusters.list, andmanagedDatabases.listcalls to compute totals and breakdowns. That pattern is slow at scale, duplicates work the API server already does for Prometheus scrapes, and couples dashboard refresh to REST pagination semantics.Prometheus-first aggregates align with the direction established in the spec reconciliation work and with existing dashboard metrics (gateway phases, cluster CPU/memory/pods/nodes, provision duration).
Changes
API server - Prometheus collectors
New collectors query the database on each scrape and emit:
hypershell_gateways_active_sandboxes_totalactive_sandbox_countacross gatewayshypershell_users_registered_totalCountRegisteredfrom users DAOhypershell_managed_clusters_totalhypershell_managed_clusters_created_last_30_days_totalhypershell_managed_clusters_inventory_total{status,provider,region}hypershell_managed_databases_totalhypershell_managed_databases_inventory_total{status}Collectors emit
prometheus.NewInvalidMetricon database errors.BFF - new metrics routes
GET /api/metrics/gateway-sandboxeshypershell_gateways_active_sandboxes_totalGET /api/metrics/platform-inventoryhypershell_managed_clusters_*,hypershell_managed_databases_*GET /api/metrics/registered-usershypershell_users_registered_totalAll three routes:
platform:adminwhen OIDC is enabled (same gate as other dashboard metrics)config.prometheusUrl(application metrics endpoint, not cluster Thanos)config.prometheusNamespacefor instance-scoped queries on OpenShiftprometheus-instant-query.ts, which routes throughfetchMetrics(TLS credentials, 4 MiB response limit, timeout handling)max()/max by (...)PromQL andMath.maxin result parsingWeb console - dashboard adapter
dashboard-control-plane.tsno longer paginates REST list APIs for:It fetches the new BFF routes instead. Gateway phase display-status mapping still uses
gatewayPhaseCountsToDisplayStatusCountsfrom@openshift-online/hypershell-gateway-management-ui.Access control
/dashboard,/metrics, allGET /api/metrics/*):platform:adminonly (removedhypershell-admins)GET /api/hypershell/v1/gateways):platform:adminonly; noHasFleetWideGatewayListAccessexpansion tohypershell-adminshypershell-adminsretains REST list access for users, managed clusters, and managed databases per PI-03 / RU-03Dev Keycloak:
adminuser grantedplatform:adminsoadmin/adminstill reaches the dashboard in Kind.Specs
Updated operational dashboard, platform inventory, registered users, gateway sandbox count, and related specs to document Prometheus-first data sources and
platform:admindashboard-operator authorization.Compatibility notes
hypershell-admins(withoutplatform:admin) lose dashboard and fleet-wide gateway list access. Grantplatform:adminin Keycloak for operators who need the operational dashboard./metricsendpoint remain cluster-internal (ServiceMonitor). BFF routes enforce auth before exposing aggregates to the browser.degradedvs the gateway table (documented trade-off inoperational-dashboard.spec.md).Test plan
pnpm --filter @openshift-online/hypershell-web-console exec vitest runincomponents/web-console/bff(metrics-source, platform-inventory, auth, cluster route tests)pnpm --filter @openshift-online/hypershell-web-console testfor dashboard adapter andRequireDashboardAdmintestscd components/api-server && make testfor new collector and inventory unit testsadmin/admin, open/dashboard, verify gateway counts, sandboxes, registered users, and platform inventory widgets loaddeveloper/developer, confirm/dashboardredirects and metrics routes return 403admin, confirm gateway list shows fleet-wide results; sign in asdeveloper, confirm gateway list is RBAC-filteredPROMETHEUS_NAMESPACEscopes application metrics to the instance namespace