fix(console): support OpenShift Route ingress

[jsell-rh]

Summary

• Use the effective gateway ingress mode for the gateway console.
• Create an edge-terminated OpenShift Route in Route mode.
• Check Route admission before publishing the console address.
• Remove inactive console exposure resources and support Route-mode cleanup.
• Grant only create and update access to routes/custom-host.
• Clear a stale console address when GATEWAY_API_BASE_DOMAIN is unavailable.
• Update the console specification and reconciliation checkpoint.

Verification

• go test -count=1 ./... in components/control-plane
• make lint-control-plane
• go test -race -count=1 ./internal/gateway ./internal/reconciler
• Kustomize renders for base, OpenShift, and IBM overlays

Live verification

Validated on the hysh-ibm-01 OpenShift cluster. The console Route became admitted, the stale HTTPRoute was removed, the console address was published, and the HTTPS endpoint returned the expected OAuth redirect.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: 72b5a068-ff84-4369-9800-9ed508c84222

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

_{Comment @coderabbitai help to get the list of available commands.}

[jsell-rh]

Amber review

Status: Complete

Verdict

Approve with minor nits (posted as COMMENT). This PR cleanly generalizes the per-gateway console from a Gateway-API-only HTTPRoute to a mode-selected exposure (HTTPRoute in gateway-api mode, edge-terminated OpenShift Route in route mode), and it threads a single resolved ingressMode through provisioning, readiness, self-heal, and teardown so the emitted and observed resources cannot diverge. Error handling, IsNotFound semantics, idempotent reconcile, additive test coverage, and the spec update are all solid; the only substantive issue is a redundant RBAC rule, plus some cross-PR coordination the maintainers should resolve.

Amber here. Two-sentence summary: the change is well-structured, backward-compatible for existing route configs, and the test diff scrutiny passes (contract changes are additive, not silent flips). I recommend approval once the redundant RBAC grant is trimmed and the cross-PR overlaps with #194 and #151 are acknowledged.

What this PR does well

• Single source of truth for ingress mode. IngressMode(hasGatewayAPI, isOpenShift) is resolved once in both GatewayReconciler and GatewayHealthReconciler, and the health loop now uses h.ingressMode instead of the exposure != nil proxy. This removes the class of bug where a Route-exposed gateway was observed through the Gateway API adapter.
• Readiness is exposure-correct. ConsoleExposureReady gates on Accepted+ResolvedRefs for HTTPRoutes and on Admitted=True from at least one router for Routes, so console_address is only published against a live public path. consoleOpenShiftRouteReady correctly prefers a specific rejection reason and short-circuits to ready on any admitted router (both covered by tests).
• Cleanup converges. reconcileConsoleExposure deletes the inactive exposure before creating the active one (prevents two controllers claiming the same host across a mode change), RouteResourcesAbsent probes both exposure kinds, and DeleteRouteResources now joins errors instead of logging-and-swallowing — matching the "never silently swallow partial failures" convention.
• Error wrapping / 404 handling. All new dynamic-client calls wrap with fmt.Errorf("...: %w", err) and treat IsNotFound as converged/absent. No panic(), no secrets in logs, no context.TODO().

Test Diff Scrutiny

I checked the modified assertions in pre-existing tests:

• TestIsRoutedGateway is additive: the pre-existing empty object {} -> true and {"enabled":true} -> true cases are unchanged; only {"enabled":false} -> false and host-only -> true are added. Combined with parseGatewayRouteConfig defaulting Enabled: true for any present, non-null route object, existing empty/host-only route records keep behaving as routed. This is the correct, backfilled way to move enabled from ignored to honored — no removed guarantee. It actually fixes a latent inconsistency where a host-only/enabled:false config made isRoutedGateway report routed while ReconcileGateway skipped creating route resources.
• TestConsoleRouteReady -> TestConsoleExposureReadyHTTPRoute and the selfHealConsole "no exposure port" -> "no selected ingress" cases are renames preserving the same guarantee against the renamed API (ConsoleRouteReady->ConsoleExposureReady, exposure->ingressMode), with new sibling tests for the Route path. No coverage lost.

Findings

• [Minor] Redundant RBAC rule in deploy/base/controller-rbac.yaml. The new routes/custom-host (create,update) rule is already fully covered by the pre-existing rule on ["routes", "routes/custom-host"] with ["get","list","watch","create","update","patch","delete"]. The extra stanza (and its mirror in the spec) grants nothing new and adds config noise. Fold the comment into the existing rule or drop the duplicate. (No security impact — it is a subset of an existing grant.)
• [Minor / behavior note] syncConsoleAddress now clears console_address when the base domain is unset (previously it returned early leaving the field untouched). This is an improvement (retracts a stale dead link) and is covered by TestSyncConsoleAddressClearsAddressWithoutBaseDomain, but it is a behavior change worth a line in the PR body for reviewers deploying without GATEWAY_API_BASE_DOMAIN.

[jsell-rh]

Verdict

Approve with minor nits (posted as COMMENT). This PR cleanly generalizes the per-gateway console from a Gateway-API-only HTTPRoute to a mode-selected exposure (HTTPRoute in gateway-api mode, edge-terminated OpenShift Route in route mode), and it threads a single resolved ingressMode through provisioning, readiness, self-heal, and teardown so the emitted and observed resources cannot diverge. Error handling, IsNotFound semantics, idempotent reconcile, additive test coverage, and the spec update are all solid; the only substantive issue is a redundant RBAC rule, plus some cross-PR coordination the maintainers should resolve.

Amber here. Two-sentence summary: the change is well-structured, backward-compatible for existing route configs, and the test diff scrutiny passes (contract changes are additive, not silent flips). I recommend approval once the redundant RBAC grant is trimmed and the cross-PR overlaps with #194 and #151 are acknowledged.

What this PR does well

• Single source of truth for ingress mode. IngressMode(hasGatewayAPI, isOpenShift) is resolved once in both GatewayReconciler and GatewayHealthReconciler, and the health loop now uses h.ingressMode instead of the exposure != nil proxy. This removes the class of bug where a Route-exposed gateway was observed through the Gateway API adapter.
• Readiness is exposure-correct. ConsoleExposureReady gates on Accepted+ResolvedRefs for HTTPRoutes and on Admitted=True from at least one router for Routes, so console_address is only published against a live public path. consoleOpenShiftRouteReady correctly prefers a specific rejection reason and short-circuits to ready on any admitted router (both covered by tests).
• Cleanup converges. reconcileConsoleExposure deletes the inactive exposure before creating the active one (prevents two controllers claiming the same host across a mode change), RouteResourcesAbsent probes both exposure kinds, and DeleteRouteResources now joins errors instead of logging-and-swallowing — matching the "never silently swallow partial failures" convention.
• Error wrapping / 404 handling. All new dynamic-client calls wrap with fmt.Errorf("...: %w", err) and treat IsNotFound as converged/absent. No panic(), no secrets in logs, no context.TODO().

Test Diff Scrutiny

I checked the modified assertions in pre-existing tests:

• TestIsRoutedGateway is additive: the pre-existing empty object {} -> true and {"enabled":true} -> true cases are unchanged; only {"enabled":false} -> false and host-only -> true are added. Combined with parseGatewayRouteConfig defaulting Enabled: true for any present, non-null route object, existing empty/host-only route records keep behaving as routed. This is the correct, backfilled way to move enabled from ignored to honored — no removed guarantee. It actually fixes a latent inconsistency where a host-only/enabled:false config made isRoutedGateway report routed while ReconcileGateway skipped creating route resources.
• TestConsoleRouteReady -> TestConsoleExposureReadyHTTPRoute and the selfHealConsole "no exposure port" -> "no selected ingress" cases are renames preserving the same guarantee against the renamed API (ConsoleRouteReady->ConsoleExposureReady, exposure->ingressMode), with new sibling tests for the Route path. No coverage lost.

Findings

• [Minor] Redundant RBAC rule in deploy/base/controller-rbac.yaml. The new routes/custom-host (create,update) rule is already fully covered by the pre-existing rule on ["routes", "routes/custom-host"] with ["get","list","watch","create","update","patch","delete"]. The extra stanza (and its mirror in the spec) grants nothing new and adds config noise. Fold the comment into the existing rule or drop the duplicate. (No security impact — it is a subset of an existing grant.)
• [Minor / behavior note] syncConsoleAddress now clears console_address when the base domain is unset (previously it returned early leaving the field untouched). This is an improvement (retracts a stale dead link) and is covered by TestSyncConsoleAddressClearsAddressWithoutBaseDomain, but it is a behavior change worth a line in the PR body for reviewers deploying without GATEWAY_API_BASE_DOMAIN.

Cross-PR coordination

I reviewed the other 23 open PRs and compared goals, ownership boundaries, data models, interfaces, and change order against #216. Material items maintainers should decide:

• #194 feat(control-plane): adopt upstream OpenShell Helm chart for gateway deployments — HIGH, competing design over the same code. #194 restructures how gateways (and their ingress/console/NetworkPolicy resources) are produced, moving from Go-emitted SSA manifests to a runtime Helm install, and it edits the exact functions #216 changes: internal/gateway/reconciler.go (RouteResourcesAbsent, deleteRouteResources, reconcileConsole), internal/reconciler/{reconciler.go,health.go}, and openshell-gateway-routing.spec.md. Two direct conflicts: (1) incompatible RouteResourcesAbsent signatures — #216 adds an ingressMode parameter and a second console-exposure probe, while #194 keeps the current 5-arg form; (2) opposite NetworkPolicy decision — #194 removes the console/router NetworkPolicies entirely ("no NetworkPolicies" design decision), whereas #216 keeps them and adds absence probes for them. Maintainers need to decide the merge order and whether #216's Route-mode console exposure is re-expressed as Helm chart values or retained as Go-emitted resources; merging both without a decision will silently reintroduce or drop the console exposure/NetworkPolicies.
• #151 spec(control-plane): gate gateway re-provisioning on desired-state convergence — MEDIUM, conflicting assumption about provisioning re-entry. #151 replaces the phase gate in GatewayReconciler.Handle (skip when Running/Provisioning/Degraded) with a convergence gate (skip only when observed_generation == generation). #216 newly invokes ReconcileConsole inside the provisioning path's Route branch and repeatedly documents the assumption that "once the gateway reaches Running the provisioning path never runs again (phase gate); the health loop owns the console." Under #151 a spec change re-enters provisioning regardless of phase, so console reconciliation would run from both the provisioning path and the health loop. That is likely benign (both are idempotent) but the ownership assumption #216 relies on no longer holds — the maintainers should confirm the console self-heal/teardown split still behaves as intended once the gate changes.
• #179 fix(control-plane): reconcile existing Keycloak clients on gated gateways — LOW/overlap, coordinate merge. #179 edits the same reconcileGatewayHealth function (the phase/status write near the end) while #216 edits the earlier console-sync/self-heal/teardown hunks and the selfHealConsole guard. The concerns are orthogonal (Keycloak client status preservation vs. ingress-mode selection), so this is a mechanical merge overlap rather than a design conflict, but both restructure the health tick and should be rebased in a known order.

No other open PR (image updates #201, trace correlation #207, kind metrics #211, web-console #208/#210/#214/#209, hsctl #206, spec/docs #185/#200/#148/#150, dependency bumps) touches #216's ownership area in a way that requires a design decision.

Findings Summary (ordered by severity, highest first)

1. [Minor] Redundant routes/custom-host RBAC rule duplicates an existing broader grant - Config / RBAC hygiene (controller-rbac.yaml L33-36)
2. [Minor] syncConsoleAddress now clears console_address when the base domain is unset (behavior change, tested) - Behavior clarity (reconciler.go L1819-1840)

Convention Checklist

Convention	Result
No panic() in production code	Pass
Errors wrapped with fmt.Errorf context	Pass
errors.IsNotFound handled for 404 scenarios	Pass
Never silently swallow partial failures (errors.Join in teardown)	Pass
No secrets in logs or error messages	Pass
Reconcile pattern (not create-or-skip)	Pass
Proper context propagation (no context.TODO())	Pass
Image references consistent across manifests	N/A (no image changes)
Test Diff Scrutiny (modified assertions justified / additive)	Pass
SecurityContext on pod specs	N/A (no pod specs changed)
Spec updated to match behavior	Pass
Least-privilege RBAC (no redundant grants)	Fail (redundant duplicate rule)

[jsell-rh]

Amber review

Status: Complete

Verdict

This PR cleanly extends the per-gateway console to support OpenShift Route ingress alongside Gateway API, gating consoleAddress on Route Admitted=True, unifying teardown/absence probes by ingress mode, and narrowing the routes/custom-host RBAC to create/update. The change is well-structured, well-tested, and spec-aligned; I recommend COMMENT with a few minor items to confirm before merge.

Amber Analysis

The design correctly funnels both the gateway and its console through the single IngressMode resolver, so the emitted exposure and the observed exposure cannot diverge. Error handling wraps context throughout, cleanup joins partial errors instead of swallowing them, no secrets are logged, and the modified test assertions are additive (renames + new mode cases) rather than flipped guarantees. The remaining notes are about a subtle route-parsing behavior change for pre-existing data and cross-API robustness on non-OpenShift clusters.

Findings

[Minor] Route semantics change for enabled-less route objects.
parseGatewayRouteConfig now defaults Enabled: true for any present, non-empty route object ({}, host-only). Previously Handle unmarshalled directly, so nsConfig.Gateway.Route.Enabled was the JSON zero value (false) for those objects, and the provisioning switch took the delete branch. This flips such gateways to "routed/provisioned". It is documented in the console spec as an intentional compatibility default and actually removes a prior inconsistency (isRoutedGateway already returned true for {}), so it is likely a fix — but please confirm no running gateway persists {}/host-only route JSON with the intent of "not routed". Confidence: Medium.

[Minor] Cross-API robustness of the inactive/console Route probes on non-OpenShift clusters.
RouteResourcesAbsent unconditionally Gets the route.openshift.io/v1 console Route, and reconcileConsoleExposure unconditionally Deletes the inactive exposure (the OpenShift Route in gateway-api mode). On a Gateway-API-only cluster that lacks the Route CRD, these calls normally return a Kubernetes NotFound (tolerated). If instead the API surfaces a discovery/no-match error, RouteResourcesAbsent returns an error and teardown never confirms absence (retries indefinitely), and console reconcile logs a WARN every pass. Consider gating the inactive-mode probe/delete by cluster capability, or confirm the client returns IsNotFound for an unregistered GVR. Confidence: Low.

[Minor/Positive] RBAC least-privilege.
Splitting routes/custom-host into its own rule scoped to create/update is the correct narrowing for the admission check that runs when the controller sets spec.host. Good change.

Cross-PR coordination

One other open pull request adopts an upstream OpenShell Helm chart as the mechanism for provisioning gateway resources: it deletes the exact switch ingressMode block in ReconcileGateway, removes reconcileRouteResources/deleteRouteResources, and replaces the teardown path with a Helm uninstall, moving Deployment/Service/GRPCRoute/BackendTLSPolicy/Route provisioning into chart values. This PR instead deepens the direct dynamic-client reconcile path in the same region (route-mode console exposure, ReconcileConsole from the route branch, exported DeleteRouteResources, ingress-mode-aware RouteResourcesAbsent, mode-selected teardownRoute). These are competing designs for the same provisioning and teardown surface, and they raise an unresolved ownership question — whether console/Route exposure belongs in the Helm chart or in the control plane's direct reconcile. Maintainers should decide a design and a merge order: whichever lands first, the other must be re-implemented against the chosen substrate (Helm values vs direct reconcile) rather than merged as-is.

Findings Summary (ordered by severity, highest first)

1. [Minor] Route object without enabled now defaults to enabled (provisioning behavior change for pre-existing data) - Test Diff Scrutiny / Migration (reconciler.go L1692-L1717)
2. [Minor] Inactive/console OpenShift Route probe + delete run on non-OpenShift clusters; non-NotFound errors could stall teardown - Reconciliation Robustness (reconciler.go L480, console.go L814)
3. [Minor] routes/custom-host narrowed to create/update - Security (positive) (controller-rbac.yaml L34-L36)

Convention Checklist

Convention	Result
No panic() in production code	Pass
Errors wrapped with fmt.Errorf context	Pass
errors.IsNotFound handled for 404 scenarios	Pass
No secrets in logs or responses	Pass
Reconcile pattern (not create-or-skip)	Pass
Never silently swallow partial failures	Pass
Image references consistent	N/A
SecurityContext on pod specs	N/A (no pod specs changed)
Test diff scrutiny (no flipped guarantees)	Pass
Spec updated to match code	Pass
RBAC least-privilege	Pass

[jsell-rh]

Verdict

This PR cleanly extends the per-gateway console to support OpenShift Route ingress alongside Gateway API, gating consoleAddress on Route Admitted=True, unifying teardown/absence probes by ingress mode, and narrowing the routes/custom-host RBAC to create/update. The change is well-structured, well-tested, and spec-aligned; I recommend COMMENT with a few minor items to confirm before merge.

Amber Analysis

The design correctly funnels both the gateway and its console through the single IngressMode resolver, so the emitted exposure and the observed exposure cannot diverge. Error handling wraps context throughout, cleanup joins partial errors instead of swallowing them, no secrets are logged, and the modified test assertions are additive (renames + new mode cases) rather than flipped guarantees. The remaining notes are about a subtle route-parsing behavior change for pre-existing data and cross-API robustness on non-OpenShift clusters.

Findings

[Minor] Route semantics change for enabled-less route objects.
parseGatewayRouteConfig now defaults Enabled: true for any present, non-empty route object ({}, host-only). Previously Handle unmarshalled directly, so nsConfig.Gateway.Route.Enabled was the JSON zero value (false) for those objects, and the provisioning switch took the delete branch. This flips such gateways to "routed/provisioned". It is documented in the console spec as an intentional compatibility default and actually removes a prior inconsistency (isRoutedGateway already returned true for {}), so it is likely a fix — but please confirm no running gateway persists {}/host-only route JSON with the intent of "not routed". Confidence: Medium.

[Minor] Cross-API robustness of the inactive/console Route probes on non-OpenShift clusters.
RouteResourcesAbsent unconditionally Gets the route.openshift.io/v1 console Route, and reconcileConsoleExposure unconditionally Deletes the inactive exposure (the OpenShift Route in gateway-api mode). On a Gateway-API-only cluster that lacks the Route CRD, these calls normally return a Kubernetes NotFound (tolerated). If instead the API surfaces a discovery/no-match error, RouteResourcesAbsent returns an error and teardown never confirms absence (retries indefinitely), and console reconcile logs a WARN every pass. Consider gating the inactive-mode probe/delete by cluster capability, or confirm the client returns IsNotFound for an unregistered GVR. Confidence: Low.

[Minor/Positive] RBAC least-privilege.
Splitting routes/custom-host into its own rule scoped to create/update is the correct narrowing for the admission check that runs when the controller sets spec.host. Good change.

Cross-PR coordination

One other open pull request adopts an upstream OpenShell Helm chart as the mechanism for provisioning gateway resources: it deletes the exact switch ingressMode block in ReconcileGateway, removes reconcileRouteResources/deleteRouteResources, and replaces the teardown path with a Helm uninstall, moving Deployment/Service/GRPCRoute/BackendTLSPolicy/Route provisioning into chart values. This PR instead deepens the direct dynamic-client reconcile path in the same region (route-mode console exposure, ReconcileConsole from the route branch, exported DeleteRouteResources, ingress-mode-aware RouteResourcesAbsent, mode-selected teardownRoute). These are competing designs for the same provisioning and teardown surface, and they raise an unresolved ownership question — whether console/Route exposure belongs in the Helm chart or in the control plane's direct reconcile. Maintainers should decide a design and a merge order: whichever lands first, the other must be re-implemented against the chosen substrate (Helm values vs direct reconcile) rather than merged as-is.

Findings Summary (ordered by severity, highest first)

1. [Minor] Route object without enabled now defaults to enabled (provisioning behavior change for pre-existing data) - Test Diff Scrutiny / Migration (reconciler.go L1692-L1717)
2. [Minor] Inactive/console OpenShift Route probe + delete run on non-OpenShift clusters; non-NotFound errors could stall teardown - Reconciliation Robustness (reconciler.go L480, console.go L814)
3. [Minor] routes/custom-host narrowed to create/update - Security (positive) (controller-rbac.yaml L34-L36)

Convention Checklist

Convention	Result
No panic() in production code	Pass
Errors wrapped with fmt.Errorf context	Pass
errors.IsNotFound handled for 404 scenarios	Pass
No secrets in logs or responses	Pass
Reconcile pattern (not create-or-skip)	Pass
Never silently swallow partial failures	Pass
Image references consistent	N/A
SecurityContext on pod specs	N/A (no pod specs changed)
Test diff scrutiny (no flipped guarantees)	Pass
Spec updated to match code	Pass
RBAC least-privilege	Pass

[jsell-rh]

Behavior change worth confirming: a present, non-empty route object that omits enabled (e.g. {} or host-only) now defaults to Enabled: true. Before this PR Handle unmarshalled the route string directly, so nsConfig.Gateway.Route.Enabled was the JSON zero value (false) for those objects and the provisioning switch took the delete branch. This flips such gateways to provisioned/routed. It's documented in the console spec and removes a prior inconsistency (isRoutedGateway already treated {} as routed), so it's likely a fix — but please confirm no running gateway stores {}/host-only route JSON meaning "not routed".

[jsell-rh]

This probes the route.openshift.io/v1 console Route unconditionally, including in gateway-api mode on a non-OpenShift cluster that lacks the Route CRD. That's fine if the client returns a Kubernetes NotFound for an unregistered GVR (tolerated below). If instead it returns a discovery/no-match error, RouteResourcesAbsent returns an error and teardown can never confirm absence (retries indefinitely). Consider gating the inactive-mode probe by cluster capability, or confirm IsNotFound holds for a missing GVR. Same concern applies to the unconditional inactive-exposure delete in reconcileConsoleExposure.

[jsell-rh]

Good least-privilege change: routes/custom-host is the subresource OpenShift admission checks when the controller sets an explicit spec.host, and create/update are the correct verbs. Narrowing this from the full verb set is the right call.