fix(kind): expose gateway metrics and fix control plane connectivity - #211
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 VerdictREQUEST_CHANGES (advisory Hi — Amber here. I reviewed the diff against the HyperShell conventions (CLAUDE.md, security + control-plane specs) and the review checklist. Summary: the local-dev connectivity/metrics fixes are correct and useful, but a substantial amount of the added Prometheus infrastructure and dashboard UI is orphaned (not referenced by any kustomization/script) or points at endpoints that don't exist yet, so it can't work end-to-end in its current form. BlockersNone. Major1. The Prometheus infrastructure and ServiceMonitor are never deployed (orphaned resources).
2. Duplicate ServiceMonitor definition. 3. The gateway-metrics dashboard is non-functional as shipped. 4. Scope and description mismatch. Minor
Cross-PR coordinationTwo open pull requests introduce the same new file 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
REQUEST_CHANGES (advisory COMMENT event). The two genuine local-dev fixes (metrics bind address, controller egress) are sound, but the PR also carries a large, undocumented gateway-metrics feature whose Kubernetes wiring and backend/UI endpoints are not actually connected, so the metrics pipeline this PR claims to enable does not function as shipped.
Hi — Amber here. I reviewed the diff against the HyperShell conventions (CLAUDE.md, security + control-plane specs) and the review checklist. Summary: the local-dev connectivity/metrics fixes are correct and useful, but a substantial amount of the added Prometheus infrastructure and dashboard UI is orphaned (not referenced by any kustomization/script) or points at endpoints that don't exist yet, so it can't work end-to-end in its current form.
Blockers
None.
Major
1. The Prometheus infrastructure and ServiceMonitor are never deployed (orphaned resources).
deploy/base/prometheus/ (its kustomization.yaml, prometheus.yaml, rbac.yaml, servicemonitor.yaml), deploy/base/servicemonitor.yaml, and the 75,414-line deploy/kind/infrastructure/prometheus-operator-bundle.yaml are not referenced by any parent kustomization or script:
deploy/base/kustomization.yamlis unchanged (does not listservicemonitor.yamlorprometheus/), which contradicts the PR body's claim that the ServiceMonitor is "registered in the base kustomization."deploy/kind/infrastructure/kustomization.yamlis unchanged, soprometheus-operator-bundle.yamlis never applied — the Prometheus Operator CRDs (monitoring.coreos.com/v1) are never installed, which means thePrometheus/ServiceMonitorCRs would fail to apply even if they were wired in.
Net effect: nothing scrapes the API server, so the metrics pipeline the PR describes is not operational. Either wire these resources into the appropriate overlays (guarded so the CRDs exist first) or drop them from this PR.
2. Duplicate ServiceMonitor definition.
deploy/base/servicemonitor.yaml and deploy/base/prometheus/servicemonitor.yaml are byte-for-byte the same object (ServiceMonitor/hypershell-api-server in hypershell-system). If both are ever pulled into the same overlay they collide on name+namespace. Keep exactly one.
3. The gateway-metrics dashboard is non-functional as shipped.
GatewayMetricsDashboard and dashboardOperations are exported but never rendered by any route or wired into the app shell in components/web-console/app. Additionally, fetchGatewayMetrics calls GET /api/metrics/gateways, an endpoint that does not exist in the API server — there is no BFF proxy route and no hypershell_gateways_total Collector code in this PR (the accompanying spec DASH-01/DASH-04 describes both, but the implementation is absent). As written, the dashboard would always land in its error state. Please either land the backend Collector + BFF route and mount the dashboard on a route, or split the UI/spec out until the backend exists.
4. Scope and description mismatch.
The PR body describes "two local development fixes plus the ServiceMonitor from #202," but the diff also introduces a whole gateway-metrics dashboard feature (shared UI component, web-console adapter/composition, i18n, a 247-line spec) and a 75k-line vendored operator bundle. Undocumented scope is hard to review and mixes a self-contained dev fix with an incomplete feature. Please describe the full scope or split the feature into its own PR once it's functional.
Minor
- Vendored operator bundle vs. URL reference. Other infra dependencies in
deploy/kind/infrastructure/kustomization.yaml(gateway-api, cert-manager, cnpg, agent-sandbox) are referenced by release URL. The 75,414-line inlineprometheus-operator-bundle.yamlbloats the repo and diverges from that pattern; prefer referencing the upstream release. - Hostname naming inconsistency. The PR body/testing notes mention
metrics.hypershell.localhost, but the code usesobservability.hypershell.localhost(METRICS_HOSTNAME). Align the docs/testing notes with the actual host. - Prometheus pod securityContext.
deploy/base/prometheus/prometheus.yamlsetsrunAsNonRoot/runAsUser/fsGroupbut noseccompProfile: RuntimeDefault. It's operator-managed so low priority, but adding it keeps it consistent with the control-plane SecurityContext convention. - Base NetworkPolicy egress. Declaring
Egress+egress: [{}]indeploy/base/networkpolicies.yamlis functionally a no-op on spec-compliant CNIs (a policy withoutEgressinpolicyTypesalready leaves egress unrestricted) and correctly works around kindnet. That's fine; just note the allow-all now lives inbase, so if per-controller egress restriction is ever desired it must be reintroduced deliberately.
Cross-PR coordination
Two open pull requests introduce the same new file components/web-console/app/composition/dashboard-composition.ts and the same dashboardOperations export as this PR, but with an incompatible design: they build the operational dashboard from a new packages/operational-dashboard-ui package fed by a mock control-plane adapter, whereas this PR wires an api adapter in packages/gateway-management-ui that fetches live gateway-phase counts. These are competing implementations of the same dashboard surface (both claim the composition entry point and the dashboardOperations symbol), so only one can own that file. Maintainers should decide which dashboard package and adapter model is canonical, and in what order these land, before any of them merges — see #209 and #214.
Findings Summary (ordered by severity, highest first)
- [Major] Prometheus infra + ServiceMonitor + operator bundle are orphaned/unreferenced; metrics pipeline is not actually deployed - Spec Consistency / Config (
deploy/base/prometheus/kustomization.yaml,deploy/base/servicemonitor.yaml,deploy/kind/infrastructure/prometheus-operator-bundle.yaml) - [Major] Duplicate
ServiceMonitor/hypershell-api-serverin two files - Config (deploy/base/servicemonitor.yaml,deploy/base/prometheus/servicemonitor.yaml) - [Major] Dashboard unwired to any route and calls non-existent
/api/metrics/gateways; no Collector/BFF route in PR - API Design / Completeness (packages/gateway-management-ui/src/metrics/gateway-metrics-data.ts) - [Major] PR scope far exceeds description (full dashboard feature + 75k-line bundle undocumented) - Reviewability
- [Minor] Vendored 75k-line operator bundle instead of URL reference - Maintainability
- [Minor] Hostname mismatch:
metrics.(docs) vsobservability.(code) - Docs - [Minor] Prometheus pod securityContext missing
seccompProfile- Security
Convention Checklist
| Convention | Result |
|---|---|
| Image references consistent across manifests | Fail (metrics pipeline manifests unreferenced; can't be applied consistently) |
| SecurityContext on pod specs | Partial (Prometheus CR missing seccompProfile) |
| Config wired into kustomize overlays | Fail |
| Conventional commit / PR title | Pass |
| Input validated | N/A |
| No secrets in logs/responses | Pass (metrics endpoint intentionally unauthenticated per spec) |
| @@ -0,0 +1,16 @@ | |||
| apiVersion: monitoring.coreos.com/v1 | |||
| kind: ServiceMonitor | |||
There was a problem hiding this comment.
This ServiceMonitor/hypershell-api-server is duplicated verbatim in deploy/base/prometheus/servicemonitor.yaml, and neither file is referenced by deploy/base/kustomization.yaml. Result: the ServiceMonitor is never deployed (contradicting the PR body's "registers it in the base kustomization"), and if both copies are ever wired into one overlay they collide on name+namespace. Keep one copy and add it to a kustomization that only loads after the Prometheus Operator CRDs exist.
| @@ -0,0 +1,6 @@ | |||
| apiVersion: kustomize.config.k8s.io/v1beta1 | |||
| kind: Kustomization | |||
| resources: | |||
There was a problem hiding this comment.
This prometheus/ kustomization (and the Prometheus CR + RBAC it aggregates) is not referenced by any parent kustomization, so it is never applied. It also depends on monitoring.coreos.com/v1 CRDs from the operator bundle, which is itself unreferenced (see deploy/kind/infrastructure/prometheus-operator-bundle.yaml). Wire the operator install first, then this overlay, or drop these from the PR.
| export async function fetchGatewayMetrics( | ||
| signal?: AbortSignal, | ||
| ): Promise<GatewayPhaseCounts> { | ||
| const response = await fetch("/api/metrics/gateways", { |
There was a problem hiding this comment.
/api/metrics/gateways does not exist anywhere in the API server or the web-console BFF in this PR (spec DASH-04 describes it, but no route/Collector is implemented). Combined with GatewayMetricsDashboard not being mounted on any route, the dashboard always resolves to its error state. Land the BFF proxy + hypershell_gateways_total Collector and mount the dashboard, or split this UI out until the backend exists.
| } | ||
|
|
||
| export const dashboardOperations: DashboardOperations = { | ||
| controlPlane: createDashboardControlPlaneAdapter(), |
There was a problem hiding this comment.
This new dashboard-composition.ts / dashboardOperations export collides with a competing design in other open dashboard PRs that create the same file from a different package (mock-adapter based). Only one can own this composition entry point; the canonical dashboard package/adapter and merge order need a maintainer decision before this lands.
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
REQUEST_CHANGES. The Kind connectivity fix and the metrics wiring are solid and were clearly validated in a live cluster, but the new GET /api/hypershell/v1/metrics/gateways endpoint returns platform-wide gateway phase counts with no per-caller RBAC scoping, which contradicts the Gateway Access Isolation rule in security.spec.md. That tenancy gap should be resolved before merge; the remaining items are minor.
Summary
This PR fixes control-plane egress under kindnet and exposes an end-to-end gateway metrics pipeline (Prometheus operator + ServiceMonitor + a JSON count endpoint + a web-console dashboard). The wiring is coherent and was verified in Kind, but the user-facing count endpoint aggregates across all tenants without honoring the caller's RoleBindings.
Findings
Major
Cross-tenant aggregate disclosure on /api/hypershell/v1/metrics/gateways — MetricsGateways (components/api-server/plugins/gateways/handler.go:320) calls CountByPhase, whose DAO query (dao.go CountByPhase) counts every gateway row with no filter on the caller's bindings. AuthorizeApi treats this path as resource=="gateways", gatewayID=="", which resolves to hasPlatformAdmin(bindings) || len(bindings) > 0 — so any user holding a single gateway:viewer binding on one gateway receives platform-wide phase totals across all tenants. security.spec.md (Gateway Access Isolation) requires that "all gateway queries MUST filter by the caller's RoleBindings." Either scope the counts to the caller's bound gateways, or restrict this endpoint to platform:admin only. Confidence: High.
Minor
- Raw internal error returned to the client (
handler.go:323):http.Error(w, svcErr.Error(), ...)forwards the underlying service/DB error string to the caller. Return a generic message and log the detail server-side (matching the framework error handling used by the other handlers in this plugin). Confidence: High. - Untyped response body (
handler.go:327):map[string]interface{}{"counts": counts}makes the wire contract implicit. A small typed struct documents the response shape and keeps the TS client (gateway-metrics-data.ts) and Go handler in sync. This endpoint is also not represented in the OpenAPI spec. Confidence: Medium. - Kind-specific network workaround placed in
deploy/base/(deploy/base/networkpolicies.yaml:16): the allow-allegress: [{}]addresses a kindnet implicit-egress-block behavior, but it lands in the shared base overlay that all environments inherit. Consider scoping the kindnet workaround to the Kind overlay so the base network posture stays explicit and tight. (The--metrics-server-bindaddressaddition in base is fine — it feeds the ServiceMonitor.) Confidence: Medium. - No navigation entry for the new dashboard (
components/web-console/app/routes.ts:16):/metricsis routed but there is no link in the application shell, so the dashboard is only reachable by typing the URL. Confidence: Medium. - No test coverage for the new endpoint: neither
MetricsGatewaysnor theCountByPhaseservice path has a unit/integration test. A small test (including the RBAC-scoping behavior once fixed) would lock in the contract. Confidence: Medium.
Cross-PR coordination
Another open pull request (#214, "Hypershell-112 UI with adjustments") builds a gateway operational/status dashboard in a separate new package (packages/operational-dashboard-ui/) on a /dashboard route, with its own gateway status-count charts and metrics-data fetch layer. This PR independently adds a second gateway dashboard (GatewayMetricsDashboard in packages/gateway-management-ui/src/metrics/) on a /metrics route, backed by the new JSON count endpoint. These are two competing solutions for essentially the same product surface — visualizing gateway phase/status distribution in the web console — using different owning packages, routes, and metrics data sources, and both edit the same shared entry points (app/routes.ts, bff/src/app.ts, route-contract.json, locales/en.json). Maintainers should decide which dashboard is canonical (or how the two compose), which package owns gateway metrics UI, and the single metrics data source, before both land, to avoid two divergent dashboards and conflicting route/BFF/contract wiring.
Findings Summary (ordered by severity, highest first)
- [Major]
/metrics/gatewaysreturns cross-tenant gateway counts unscoped by caller RoleBindings — Security / Spec Consistency (handler.go:320) - [Minor] Raw internal error string returned to client — Security / Error Handling (handler.go:323)
- [Minor] Untyped
map[string]interface{}response; endpoint absent from OpenAPI — API Design (handler.go:327) - [Minor] Kindnet allow-all egress workaround placed in shared
deploy/base/— Ownership Boundary (networkpolicies.yaml:16) - [Minor]
/metricsroute has no navigation entry — UX (routes.ts:16) - [Minor] No test for
MetricsGateways/CountByPhase— Test Coverage
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf/service context |
Pass (service); Fail (handler leaks raw error) |
| Gateway queries scoped by caller RBAC | Fail |
| No secrets in logs or responses | Pass |
| SecurityContext on pod specs (Prometheus CR) | Pass |
| Image references consistent across manifests | Pass |
| Conventional commit message | Pass |
| handlers.HandleDelete(w, r, cfg, http.StatusNoContent) | ||
| } | ||
|
|
||
| func (h gatewayHandler) MetricsGateways(w http.ResponseWriter, r *http.Request) { |
There was a problem hiding this comment.
[Major] Cross-tenant aggregate disclosure. CountByPhase counts every gateway row regardless of the caller's RoleBindings, and AuthorizeApi resolves this path (resource=="gateways", gatewayID=="") to hasPlatformAdmin || len(bindings) > 0. A user with a single gateway:viewer binding therefore receives platform-wide phase totals across all tenants, which contradicts security.spec.md Gateway Access Isolation ("all gateway queries MUST filter by the caller's RoleBindings"). Either scope the counts to the caller's bound gateways or restrict this endpoint to platform:admin.
| func (h gatewayHandler) MetricsGateways(w http.ResponseWriter, r *http.Request) { | ||
| counts, svcErr := h.gateway.CountByPhase(r.Context()) | ||
| if svcErr != nil { | ||
| http.Error(w, svcErr.Error(), http.StatusInternalServerError) |
There was a problem hiding this comment.
[Minor] Returning svcErr.Error() forwards the underlying service/DB error string to the client. Return a generic message here and log the detail server-side, matching the framework error handling the other handlers in this plugin use.
| return | ||
| } | ||
| w.Header().Set("Content-Type", "application/json") | ||
| if err := json.NewEncoder(w).Encode(map[string]interface{}{"counts": counts}); err != nil { |
There was a problem hiding this comment.
[Minor] The response uses an untyped map[string]interface{}, leaving the wire contract implicit and keeping this endpoint out of the OpenAPI spec. A small typed struct documents the shape and keeps the Go handler and the TS client (gateway-metrics-data.ts) in sync.
| policyTypes: | ||
| - Ingress | ||
| - Egress | ||
| egress: |
There was a problem hiding this comment.
[Minor] This allow-all egress: [{}] fixes a kindnet implicit-egress-block behavior, but it lands in the shared base overlay inherited by every environment. Consider scoping the kindnet workaround to the Kind overlay so the base network posture stays explicit.
| index("./routes/home.tsx"), | ||
| route(routeContract.gatewayNew, "./routes/gateway-new.tsx"), | ||
| route(routeContract.gatewayDetail, "./routes/gateway.tsx"), | ||
| route(routeContract.metrics, "./routes/metrics.tsx"), |
There was a problem hiding this comment.
[Minor] The /metrics route is registered but there's no corresponding link in the application shell nav, so the dashboard is only reachable by typing the URL directly.
- 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>
Brings in the ServiceMonitor from #202, wiring Prometheus Operator scraping to the metrics port (4433) already exposed by the API server. Co-Authored-By: Crush <crush@charm.land>
Co-Authored-By: Crush <crush@charm.land>
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>
…cture 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>
- 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>
Co-Authored-By: Crush <crush@charm.land>
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>
41ead38 to
dcdcd88
Compare
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
The local-dev connectivity and metrics-port fixes are sound and well explained, and the dashboard UI is clean and fully localized. However, this PR pivoted the metrics data path away from the design its own spec still documents, and the new /metrics/gateways API endpoint returns platform-wide gateway counts to any authenticated caller without the per-gateway RBAC scoping the List handler and the security spec require.
Blocker / Major findings
[Major - Security] Metrics endpoint is not RBAC-scoped (components/api-server/plugins/gateways/handler.go:317, plugin.go:156)
MetricsGateways calls CountByPhase over the entire gateways table and returns the aggregate to any caller who passes AuthenticateAccountJWT + AuthorizeApi. The List handler (handler.go:177-203) deliberately scopes results to AccessibleGatewayIDs for non-platform-admins, and security.spec.md (Gateway Access Isolation) states that all gateway queries MUST filter by the caller's RoleBindings. As written, a non-admin with a binding on a single gateway learns the platform-wide count of every other tenant's gateways. Either gate this endpoint to platform admins or compute counts over the caller's accessible gateway IDs.
[Major - Spec Consistency] Implementation contradicts the spec added in this same PR (specs/platform/gateway-metrics-dashboard.spec.md)
The new spec documents a design the code does not implement:
- DASH-05 says the browser reaches metrics through a BFF proxy route
GET /api/metrics/gatewaysthat queries Prometheus and never contacts the API server. The code instead fetches/api/hypershell/v1/metrics/gatewaysdirectly from the API server's DB-backed handler; no BFF Prometheus proxy exists in the diff. - DASH-03 says the Prometheus Operator SHALL be vendored as
deploy/kind/infrastructure/prometheus-operator-bundle.yaml(pinned). The code references the upstreambundle.yamlURL instead. - DASH-07 says the route is
/dashboardwith a"dashboard"route-contract entry and sidebar nav; the code adds a/metricsroute with a"metrics"entry and no shell nav wiring. - DASH-08's
PROMETHEUS_URLweb-console patch is not present.
Net effect: a full Prometheus stack (operator, CR, RBAC, ServiceMonitor) is deployed but the dashboard bypasses it, reading the DB directly. Please reconcile the spec with the implementation (or vice versa) so they describe one coherent data path; right now a reviewer cannot tell which design is authoritative.
Minor findings
- [Minor - Security] Raw service error leaked to client (
handler.go:320):http.Error(w, svcErr.Error(), ...)writes the wrapped DB error text into the HTTP response and bypasses the framework error presenter used elsewhere. Return a generic message and log the detail server-side. The response also uses an untypedmap[string]interface{}and sets noCache-Control. - [Minor - Tests] No coverage for the new endpoint: neither
sqlGatewayService.CountByPhasenorgatewayHandler.MetricsGatewayshas a unit/integration test in this PR. - [Minor - UX] Error copy references the wrong dependency (
packages/gateway-management-ui/src/metrics/gateway-metrics-dashboard.tsx:35): the error state tells users to check that "Prometheus is running and reachable," but the dashboard now reads from the API server, so the guidance is misleading. - [Minor] Dead nav string / spec gap:
app.nav.metricsis defined but no sidebar nav entry is wired in (application-shell.tsxuntouched), so the route is reachable only by direct URL and DASH-07's navigation requirement is unmet. - [Minor - Config hygiene] Kind-only NetworkPolicy workaround lives in
deploy/base(deploy/base/networkpolicies.yaml:16): the allow-allegress: [{}]is a kindnet-specific fix. It is a no-op on CNIs that already leave egress unrestricted, but a shared base allow-all forecloses any future egress tightening. Consider scoping egress to DNS + the API server, or moving the workaround to the kind overlay. - [Minor] Hardcoded operator namespace (
scripts/kind/up.sh): the readiness wait targetsdeployment/prometheus-operator -n default; this assumes the upstream bundle's default namespace and will silently break if that changes.
Cross-PR coordination
PR #214 introduces a competing dashboard: a separate operational-dashboard-ui package mounted at /dashboard, admin-gated to hypershell-admins/platform:admin, wired into the app shell nav, and served from a mock control-plane adapter. This PR adds a different GatewayMetricsDashboard (in gateway-management-ui) at /metrics, ungated, served from a new API-server endpoint - and its own spec (DASH-07) asks for the /dashboard route and sidebar nav that #214 actually builds. The two PRs also both edit route-contract.json, app/routes.ts, bff/src/app.ts, app/i18n/messages.ts, and locales/en.json, and #214 owns the dashboard-composition.ts / dashboard-control-plane.ts entry point this PR removed. Maintainers need to decide which dashboard is canonical (or how they coexist), reconcile the route/navigation and access-control model, and sequence the merges accordingly.
Findings Summary (ordered by severity, highest first):
- [Major] Metrics endpoint returns platform-wide gateway counts without RBAC scoping - Security (handler.go L317, plugin.go L156)
- [Major] Spec added in this PR contradicts the implemented data path (DASH-03/05/07/08) - Spec Consistency (gateway-metrics-dashboard.spec.md)
- [Minor] Raw service error written to HTTP response - Security / Error Handling (handler.go L320)
- [Minor] No tests for
CountByPhase/MetricsGateways- Test Coverage - [Minor] Dashboard error copy references Prometheus instead of the API server - UX (gateway-metrics-dashboard.tsx L35)
- [Minor]
app.nav.metricsdefined but no nav wiring; DASH-07 nav unmet - Spec Completeness - [Minor] Kind-specific allow-all egress placed in shared base manifest - Config Hygiene (networkpolicies.yaml L16)
- [Minor] Hardcoded
-n defaultoperator wait - Robustness (scripts/kind/up.sh)
Convention Checklist:
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
| No secrets in logs or responses | Pass |
| Gateway queries RBAC-scoped to caller | Fail |
| Internal error detail not leaked to clients | Fail |
| SecurityContext on pod specs (Prometheus CR) | Pass |
| Image/version references pinned | Pass |
| Spec matches implementation | Fail |
| Tests accompany new endpoints | Fail |
| Conventional commit messages | Pass |
| } | ||
|
|
||
| func (h gatewayHandler) MetricsGateways(w http.ResponseWriter, r *http.Request) { | ||
| counts, svcErr := h.gateway.CountByPhase(r.Context()) |
There was a problem hiding this comment.
[Major - Security] This returns CountByPhase over all gateways to any authenticated caller. The List handler (this file, ~L177-203) scopes results to AccessibleGatewayIDs for non-platform-admins, and security.spec.md (Gateway Access Isolation) requires every gateway query to filter by the caller's RoleBindings. As written, a non-admin learns the platform-wide gateway count across all tenants. Gate this to platform admins or count over the caller's accessible IDs.
| func (h gatewayHandler) MetricsGateways(w http.ResponseWriter, r *http.Request) { | ||
| counts, svcErr := h.gateway.CountByPhase(r.Context()) | ||
| if svcErr != nil { | ||
| http.Error(w, svcErr.Error(), http.StatusInternalServerError) |
There was a problem hiding this comment.
[Minor - Security/Error handling] svcErr.Error() writes the wrapped DB error text straight into the HTTP response and bypasses the framework's error presenter used by the other handlers. Return a generic message to the client and log the detail server-side. The response body also uses an untyped map[string]interface{} and sets no Cache-Control.
| gatewaysRouter.Use(authzMiddleware.AuthorizeApi) | ||
|
|
||
| metricsRouter := apiV1Router.PathPrefix("/metrics").Subrouter() | ||
| metricsRouter.HandleFunc("/gateways", gatewayHandler.MetricsGateways).Methods(http.MethodGet) |
There was a problem hiding this comment.
[Major - Security] The /metrics/gateways route is protected only by AuthenticateAccountJWT + AuthorizeApi; it applies none of the per-gateway visibility filtering the gateways subrouter relies on for the List path. Any authorized API user gets platform-wide counts. Decide whether this is admin-only (add an admin gate) or per-caller scoped (filter counts by accessible gateway IDs).
|
|
||
| ### Requirement: DASH-05 -- BFF Metrics Proxy Route | ||
|
|
||
| The web-console BFF SHALL expose `GET /api/metrics/gateways` as a same-origin proxy route that queries Prometheus and returns the `hypershell_gateways_total` phase counts as JSON. The browser SHALL never contact Prometheus directly. |
There was a problem hiding this comment.
[Major - Spec Consistency] DASH-05 specifies a BFF proxy route GET /api/metrics/gateways that queries Prometheus, with the browser never contacting the API server. The implementation in this PR instead fetches /api/hypershell/v1/metrics/gateways directly from the API server's DB-backed handler, and no BFF Prometheus proxy appears in the diff. The spec and code describe two different data paths - please reconcile them.
|
|
||
| Every cluster to which HyperShell is deployed SHALL include a Prometheus Operator and a Prometheus instance capable of scraping the `hypershell_gateways_total` metric without requiring cluster-specific manual configuration. | ||
|
|
||
| The Prometheus Operator SHALL be vendored as `deploy/kind/infrastructure/prometheus-operator-bundle.yaml` (pinned to a specific release) and applied as a cluster-level infrastructure dependency before application manifests, in the same layer as cert-manager and CNPG. |
There was a problem hiding this comment.
[Major - Spec Consistency] DASH-03 says the operator SHALL be vendored as deploy/kind/infrastructure/prometheus-operator-bundle.yaml (pinned), but the kustomizations reference the upstream bundle.yaml URL. Similarly DASH-07 asks for a /dashboard route + sidebar nav while the code adds /metrics, and DASH-08's PROMETHEUS_URL web-console patch is absent. Update the spec (or the code) so they agree.
| export async function fetchGatewayMetrics( | ||
| signal?: AbortSignal, | ||
| ): Promise<GatewayPhaseCounts> { | ||
| const response = await fetch("/api/hypershell/v1/metrics/gateways", { |
There was a problem hiding this comment.
[Spec Consistency] This fetches the API server endpoint directly. The spec (DASH-05) describes reaching metrics through a same-origin BFF proxy that queries Prometheus. If the direct-to-API-server path is the intended design, the spec needs to be rewritten to match; as-is the whole Prometheus stack deployed by this PR is unused by the dashboard.
| loadErrorBody: { | ||
| id: "app.metrics.dashboard.loadErrorBody", | ||
| defaultMessage: | ||
| "Check that Prometheus is running and reachable, then refresh.", |
There was a problem hiding this comment.
[Minor - UX] The error guidance tells users to check that Prometheus is reachable, but the dashboard now reads from the API server (/api/hypershell/v1/metrics/gateways), not Prometheus. Update this copy so the recovery advice points at the actual dependency.
| policyTypes: | ||
| - Ingress | ||
| - Egress | ||
| egress: |
There was a problem hiding this comment.
[Minor - Config hygiene] This allow-all egress: [{}] is a kindnet-specific workaround (per the PR description) but lives in the shared deploy/base manifest. It is effectively a no-op on CNIs that already leave egress open, yet a base allow-all forecloses future egress tightening. Consider scoping egress to DNS + the API server, or moving this to the kind overlay.
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
REQUEST_CHANGES. The connectivity fix and the metrics endpoint work are solid and well-tested locally, but the new gateway-metrics-dashboard.spec.md shipped in this PR describes a design (BFF-proxied, Prometheus-sourced, base/every-cluster deployment) that the code in the same PR does not implement (dashboard reads the API server directly; the Prometheus stack is wired kind-only). That internal spec/code contradiction plus a base-layer NetworkPolicy that now allows all egress should be reconciled before merge.
Amber Assessment
This PR does three good things: it fixes the controller egress/DNS bug that left gateways with a blank phase, exposes the API server metrics port for scraping, and delivers a working gateway-phase dashboard end to end. My concern is not the runtime behavior — which the author verified in Kind — but the mismatch between the specification this PR adds and what the code actually does, and one security-posture change that lands in the shared base layer rather than the Kind overlay.
Blocker
None.
Critical
None.
Major
-
Spec describes a Prometheus/BFF data flow the code does not implement.
gateway-metrics-dashboard.spec.mdDASH-05 states the browser reaches metrics through a BFF proxy routeGET /api/metrics/gatewaysthat queries Prometheus, and "the browser SHALL never contact Prometheus directly." The implementation instead adds a direct DB-backed JSON endpoint on the API server (GET /api/hypershell/v1/metrics/gateways,MetricsGateways->CountByPhase) and the dashboard fetches that endpoint directly. As a result the Prometheus Operator +PrometheusCR +ServiceMonitordeployed by this PR are never consumed by the dashboard (thehypershell_gateways_totalcollector is scraped, but the UI does not read from Prometheus). Either the spec or the data path should be corrected so the shipped "desired state" matches the code. (Spec Consistency) -
Spec claims base/every-cluster deployment; code wires the stack Kind-only. DASH-03 says the
PrometheusCR/RBAC "SHALL be declared indeploy/base/prometheus/and applied as application-level resources" on "every cluster," and DASH-04's scenario assertsdeploy/base/kustomization.yamlreferences theServiceMonitor. In factdeploy/base/kustomization.yamldoes not referenceprometheus/or the ServiceMonitor; the stack is applied only byscripts/kind/up.sh, and the previousdeploy/base/servicemonitor.yamlwas removed. On non-Kind overlays (hub/OpenShift) no Prometheus, ServiceMonitor, or scrape config is deployed. The spec scenarios are therefore false against this implementation. (Spec Consistency) -
Base NetworkPolicy now allows all egress.
deploy/base/networkpolicies.yamladdsEgresstopolicyTypeswith an allow-allegress: [{}]on the controller pod. This is the shared base manifest inherited by all overlays, not the Kind overlay. The connectivity bug it fixes is real (kindnet implicitly denies egress once a pod is selected), but allow-all egress in base weakens the posture for production clusters that may run default-deny. Prefer scoping egress to what the controller needs (DNS53/UDP+TCPto CoreDNS + the API server on9000), or move the broad rule into the Kind overlay. (Security / Least Privilege)
Minor
- Metrics handler returns internal error text and an untyped body. In
handler.go,MetricsGatewaysdoeshttp.Error(w, svcErr.Error(), 500), surfacing the wrapped DB error string ("Unable to count gateways by phase: ...") to the caller instead of a generic message + server-side log, which is the pattern used elsewhere. It also encodesmap[string]interface{}{"counts": counts}; a typed response struct would be clearer and matches the presenter convention. (Error Handling / API Design)
Cross-PR coordination
Another open pull request introduces a competing operational dashboard for the web console: it adds a separate operational-dashboard-ui package, a /dashboard SPA route with nav entry, a mock control-plane data adapter, and an admin (platform:admin / hypershell-admins) access gate, and it renders gateway status/phase counts — the same underlying data this PR surfaces. This PR instead adds a /metrics route backed by gateway-management-ui reading live data from the API server with no admin gate. The two overlap on goal (gateway health at a glance), on the same web-console entry points (route-contract.json, routes.ts, bff/src/app.ts, i18n/messages.ts, locales/en.json, shell nav), and on data source and access-control decisions. Maintainers should decide which dashboard is canonical (or how they compose) — including the route name, the package that owns the dashboard, whether metrics come from Prometheus/BFF or the API server directly, and whether the view is admin-gated — before both merge, to avoid shipping two divergent dashboards. That PR is #214.
Findings Summary (ordered by severity, highest first)
- [Major] Dashboard reads the API server directly while the spec mandates a BFF/Prometheus data flow; deployed Prometheus stack is unused by the UI - Spec Consistency (gateway-metrics-data.ts L20, spec L121)
- [Major] Spec claims base/every-cluster deployment but the Prometheus stack + ServiceMonitor are wired Kind-only - Spec Consistency (spec L71, L96)
- [Major] Base NetworkPolicy grants allow-all egress on the controller pod - Security (networkpolicies.yaml L16)
- [Minor] Metrics handler leaks internal error text and returns an untyped body - Error Handling (handler.go L320, L324)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound handled for 404 scenarios |
N/A (aggregate count) |
| No secrets in logs or responses | Pass |
| Internal error detail withheld from clients | Fail |
| SecurityContext on pod specs (Prometheus CR) | Pass |
| Restricted egress (least privilege) | Fail |
| Image/manifest references consistent across the stack | Pass |
| Spec matches implementation | Fail |
| Conventional commit messages | Pass |
| policyTypes: | ||
| - Ingress | ||
| - Egress | ||
| egress: |
There was a problem hiding this comment.
[Major] Allow-all egress in the shared base layer. egress: [{}] with policyTypes: [Ingress, Egress] removes all egress restriction for the controller pod, and this is deploy/base, inherited by every overlay (including production hub/OpenShift), not just Kind.
The fix for the kindnet implicit-deny is correct in spirit, but prefer scoping egress to what the controller actually needs so default-deny production clusters stay tight:
egress:
- to:
- namespaceSelector: {}
podSelector:
matchLabels:
k8s-app: kube-dns
ports:
- {protocol: UDP, port: 53}
- {protocol: TCP, port: 53}
- to:
- podSelector:
matchLabels:
app: hypershell-api-server
ports:
- {protocol: TCP, port: 9000}Alternatively, keep allow-all but move it into the Kind overlay so it doesn't broaden the base posture.
| func (h gatewayHandler) MetricsGateways(w http.ResponseWriter, r *http.Request) { | ||
| counts, svcErr := h.gateway.CountByPhase(r.Context()) | ||
| if svcErr != nil { | ||
| http.Error(w, svcErr.Error(), http.StatusInternalServerError) |
There was a problem hiding this comment.
[Minor] Internal error text is returned to the caller. svcErr.Error() surfaces the wrapped message ("Unable to count gateways by phase: <db error>") to the client. Log the detail server-side and return a generic message, matching the presenter/error pattern used by the other handlers:
if svcErr != nil {
glog.Errorf("gateway metrics: count by phase: %v", svcErr)
http.Error(w, "failed to compute gateway metrics", http.StatusInternalServerError)
return
}Also consider a typed response struct instead of map[string]interface{}{"counts": counts} (L324) for clarity and consistency.
| export async function fetchGatewayMetrics( | ||
| signal?: AbortSignal, | ||
| ): Promise<GatewayPhaseCounts> { | ||
| const response = await fetch("/api/hypershell/v1/metrics/gateways", { |
There was a problem hiding this comment.
[Major] This contradicts DASH-05 in the spec added by this same PR. The dashboard fetches the API server directly (/api/hypershell/v1/metrics/gateways), but gateway-metrics-dashboard.spec.md DASH-05 says the browser must go through a BFF proxy route /api/metrics/gateways that queries Prometheus, and "the browser SHALL never contact Prometheus directly."
Because of this, the Prometheus Operator + Prometheus CR + ServiceMonitor this PR deploys are never consumed by the UI (the collector is scraped, but nothing reads it back). Please reconcile: either implement the BFF/Prometheus path the spec describes, or update the spec to document the direct API-server data flow and justify keeping the Prometheus stack (e.g. alerting/history only).
|
|
||
| ### Requirement: DASH-04 -- ServiceMonitor Scrape Configuration | ||
|
|
||
| A `ServiceMonitor` resource named `hypershell-api-server` SHALL exist in `hypershell-system` and SHALL be included in the base kustomization so it is applied to every cluster. The `ServiceMonitor` SHALL: |
There was a problem hiding this comment.
[Major] Spec/implementation mismatch. DASH-04 states the ServiceMonitor "SHALL be included in the base kustomization so it is applied to every cluster," and DASH-03 (L67-71) says the Prometheus CR/RBAC are applied via deploy/base. In this PR deploy/base/kustomization.yaml does not reference prometheus/ or a ServiceMonitor (the old deploy/base/servicemonitor.yaml was removed), and the stack is applied only by scripts/kind/up.sh. On non-Kind overlays nothing is deployed. Either add prometheus/ to the base kustomization or revise DASH-03/04 to describe the Kind-only wiring.
|
|
||
| Every cluster to which HyperShell is deployed SHALL include a Prometheus Operator and a Prometheus instance capable of scraping the `hypershell_gateways_total` metric without requiring cluster-specific manual configuration. | ||
|
|
||
| The Prometheus Operator SHALL be vendored as `deploy/kind/infrastructure/prometheus-operator-bundle.yaml` (pinned to a specific release) and applied as a cluster-level infrastructure dependency before application manifests, in the same layer as cert-manager and CNPG. |
There was a problem hiding this comment.
[Minor] Spec/implementation mismatch. DASH-03 says the operator "SHALL be vendored as deploy/kind/infrastructure/prometheus-operator-bundle.yaml (pinned to a specific release)." The implementation instead references the upstream bundle URL (.../v0.93.1/bundle.yaml) in the infrastructure kustomizations. Update the spec text to match the URL-reference approach.
2 participants
Summary
Two local development fixes plus a fully wired gateway metrics pipeline — all addressed after the Amber review on the original submission.
Changes
Fix: control plane losing connectivity to the API server
The
hypershell-service-account-provisionerNetworkPolicy selected the controller pod but only declaredIngressinpolicyTypes. kindnet implicitly blocks all egress from any pod selected by a NetworkPolicy, so the control plane could not reach CoreDNS and therefore could not connect to the API server via gRPC, leaving all gateways with a blank phase.Fix: explicitly declare
EgressinpolicyTypeswith an allow-allegress: [{}]rule. Also adds fully-qualified service names anddnsConfig.use-vc(TCP DNS) to the Kind controller overlay.Fix: expose gateway metrics port in the Kind cluster
The API server's metrics server was binding to
localhost:4433inside the pod. Adds--metrics-server-bindaddress=0.0.0.0:4433to the base manifest and an HTTPRoute forobservability.hypershell.localhostrouting to port 4433.Implement gateway metrics pipeline (end-to-end)
Prometheus infrastructure
v0.93.1) in bothinfrastructure/andinfrastructure-no-cnpg/kustomizations, replacing the 75k-line vendored copydeploy/base/prometheus/(Prometheus CR, RBAC, ServiceMonitor) applied inscripts/kind/up.shafter the main component rollout;kind-upwaits for the operator before proceedingdeploy/base/prometheus/servicemonitor.yaml(duplicatedeploy/base/servicemonitor.yamlremoved)seccompProfile: RuntimeDefaultadded to Prometheus CRsecurityContextAPI server
CountByPhaseadded toGatewayServiceinterface andsqlGatewayServiceMetricsGatewaysHTTP handler registered asGET /api/hypershell/v1/metrics/gateways(auth-gated)Web console
gateway-metrics-data.ts:/api/metrics/gatewaysto/api/hypershell/v1/metrics/gateways/metricsroute toroute-contract.json,routes.ts,bff/src/app.ts, and newapp/routes/metrics.tsxmountingGatewayMetricsDashboarddashboard-composition.tsanddashboard-control-plane.tsto avoid colliding with PRs HYPERSHELL-112 Dashboard UI #209 and Hypershell-154 UI with adjustments #214 which own that composition entry pointTesting
Verified in a local Kind cluster (
LOCAL_IMAGES=true make kind-up):prometheus-hypershell-0pod runs 2/2 afterkind-uphttp://localhost:4433/metricsreturnshypershell_gateways_total{phase="Running"} 0(all four phases present)GET /api/hypershell/v1/metrics/gatewaysreturns 401 without a JWT (route registered, auth-gated correctly)Closes #202