fix(e2e): unify OpenShift driver with Kind, dynamic namespace GC timing

[squizzi]

Summary

Makes the OpenShift e2e driver feature-complete with Kind by adding console host discovery and dynamic namespace GC timing, while moving GC timing from static overlay to runtime patches so Kind remains production-representative.

Highlights

• Console discovery: Added discover_console_host() to both drivers; OpenShift Routes must be discovered independently rather than derived from API hostname
• Dynamic GC timing: Removed hardcoded 30s GC intervals from Kind overlay; all drivers now patch controller deployment at runtime for e2e and restore production defaults (5m/10m) afterward
• Unified OIDC logic: Consolidated per-gateway client token acquisition to run on both Kind and OpenShift (removed driver-specific branches)
• Better failure visibility: Enhanced print_results() to report which area the run aborted in, so truncated logs are interpretable
• Test robustness: Improved detect_infra_driver() error handling, added console and GC timing assertions to OpenShift driver test

Scope

E2E testing only — no API or control plane changes. Fixes OpenShift parity and makes namespace GC assertions runnable without long production wait times.

[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: Team

Run ID: 6ef490b7-31b7-4416-afe0-cd112fb43f7d

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

View the submitted review.

[jsell-rh]

Verdict

COMMENT — This is a clean, well-reasoned e2e-only change that brings the OpenShift driver to parity with Kind and, sensibly, moves shortened namespace-GC timing out of the Kind overlay into a runtime patch/restore pair so Kind stays representative of a vanilla deployment. The code is correct and the new behavior is covered by additive driver-test assertions; the main thing to resolve before merge is a set of now-contradictory statements left behind in the very spec this PR edits.

What I checked

• Test Diff Scrutiny: openshift_driver_test.sh changes are purely additive (new console-discovery and GC-timing assertions). No pre-existing assertion was flipped from accept→reject or optional→required. No removed guarantees.
• No production code touched: API server and control plane are untouched; this is tests/e2e/ plus one deploy overlay and the e2e spec.
• Naming consistency: the runtime patch targets deployment/hypershell-controller -c controller, which matches deploy/base/controller.yaml; hypershell-web-console Route and console.hypershell.localhost HTTPRoute both exist in the deploy manifests, so the new discovery helpers rest on real resources.
• Cleanup/exit semantics: moving print_results into the EXIT trap preserves the script's exit code (bash restores $? around a non-exiting EXIT trap), and the main path no longer double-prints. Good.

Findings

[Major] Spec left self-contradictory — specs/platform/e2e-testing.spec.md

This PR removes the shortened GC env from the Kind overlay and rewrites line 230 to say "no overlay bakes in shortened e2e timing" and that a run SHALL call configure_namespace_gc_timing. But three other sections of the same file still assert the opposite as normative requirements:

• Lines 382–386: "the Kind overlay SHALL patch the control-plane deployment with GATEWAY_NAMESPACE_GC_INTERVAL and GATEWAY_NAMESPACE_GC_GRACE_PERIOD…" — now false.
• Lines 423–424 (Scenario GIVEN): "GIVEN the Kind overlay has shortened GATEWAY_NAMESPACE_GC_INTERVAL and GATEWAY_NAMESPACE_GC_GRACE_PERIOD…" — precondition no longer holds.
• Line 622: lists "shortened namespace GC timing (GATEWAY_NAMESPACE_GC_INTERVAL … for example 30s)" among the Kind overlay's Kustomize patches.

In a spec-driven repo these contradictory SHALL/GIVEN statements are actionable. Update them to reference the runtime configure_namespace_gc_timing/restore_namespace_gc_timing mechanism, matching the rewritten line 230.

[Minor] E2E_COMPLETED=1 in the performance script is inert — tests/e2e/e2e-performance.sh:683

The abort-visibility flag only does anything when print_results runs on an aborted path. In this script print_results is called once, on the success path (line 684), right after E2E_COMPLETED=1; perf_cleanup never calls print_results. So an aborted performance run prints no "aborted" banner and the added line/comment is effectively dead. Either wire print_results into perf_cleanup (mirroring e2e-openshell.sh) or drop the line so it doesn't imply coverage that isn't there.

Cross-PR coordination

No material cross-PR coordination issue requires maintainer action.

---

Findings Summary (ordered by severity, highest first):

1. [Major] Spec file left with contradictory normative statements about Kind-overlay GC timing after the overlay was removed - Spec Consistency (spec L230 vs L382–386, L423–424, L622)
2. [Minor] E2E_COMPLETED abort banner not wired into the performance script's cleanup, so the flag is inert there - Observability (perf L683)

Convention Checklist:

Convention	Result
Test Diff Scrutiny (no flipped assertions / removed guarantees)	Pass
No secrets in logs or error messages	Pass
Image references consistent across the stack	Pass
Spec kept internally consistent with code	Fail
Conventional commit message	Pass

[jsell-rh]

This rewrite correctly says no overlay bakes in shortened GC timing and that a run SHALL call configure_namespace_gc_timing. However, three other sections of this same file still assert the removed overlay behavior as normative:

• L382-386: "the Kind overlay SHALL patch the control-plane deployment with GATEWAY_NAMESPACE_GC_INTERVAL and GATEWAY_NAMESPACE_GC_GRACE_PERIOD…"
• L423-424 (Scenario GIVEN): "GIVEN the Kind overlay has shortened GATEWAY_NAMESPACE_GC_INTERVAL and GATEWAY_NAMESPACE_GC_GRACE_PERIOD…"
• L622: lists "shortened namespace GC timing (GATEWAY_NAMESPACE_GC_INTERVAL … for example 30s)" among the Kind overlay's Kustomize patches.

These now contradict both the code (env removed from deploy/kind/kustomization.yaml) and this line. Please update them to reference the runtime configure_namespace_gc_timing/restore_namespace_gc_timing mechanism. [Major]

[jsell-rh]

This flag is inert in this script. E2E_COMPLETED only affects output when print_results runs on an aborted path, but here print_results is called exactly once, on the success path immediately below, and perf_cleanup never calls it. So an aborted performance run prints no "aborted" banner and this line/comment implies coverage that doesn't exist. Either call print_results from perf_cleanup (as e2e-openshell.sh now does via its EXIT trap) or drop this line. [Minor]

[jsell-rh]

Amber review

Status: Stopped

The pull request head changed before Amber posted the review. A later job can review the new head.

[jsell-rh]

Amber review

Status: Complete

View the submitted review.

[jsell-rh]

Verdict

This PR does a genuinely good job of bringing the OpenShift e2e driver to parity with Kind: console-host discovery, runtime GC-timing patching (keeping Kind production-representative), unified OIDC logic, and a broad set of new driver/unit tests. My main concern is a latent shell bug where effective_database_provider's exit 1 is swallowed inside a command substitution, which turns an intended fatal validation into a silent (and in one path destructive) fallthrough; everything else is minor.

Findings

[Major] effective_database_provider aborts only its own subshell when invoked via $(...) - scripts/cluster/drivers/openshift.sh
effective_database_provider() uses exit 1 for its two error paths (unknown DATABASE_PROVIDER, and DATABASE_PROVIDER=cnpg on a cluster without CNPG). But it is called almost everywhere through command substitution - [[ "$(effective_database_provider)" != "cnpg" ]] and, critically, cutover_database_provider "$(effective_database_provider)" at line 1183. exit 1 inside $(...) only terminates the subshell, and set -e does not propagate a command-substitution failure in argument or [[ ]] position, so the run continues. I reproduced both behaviors:

• In the [[ ]] guards the empty result is treated as "not cnpg", so the intended abort is silently downgraded to the deployment path.
• In cluster_up, cutover_database_provider "" receives an empty target; both [[ "" != "cnpg" ]] and [[ "" != "deployment" ]] are true, so the cutover deletes both the CNPG Cluster/PVCs and the bundled Deployment/Service. That is the opposite of the guard's intent (fail clearly), and it is destructive.

This is dev/e2e infra (ephemeral, as the code notes), and it only triggers on an explicit misconfiguration (DATABASE_PROVIDER=cnpg without CNPG, or an unknown value), so it is Major rather than Blocker. Fix: resolve the provider once into a plain (non-local) global assignment where set -e does honor the failure, e.g. TARGET_DB_PROVIDER="$(effective_database_provider)" on its own line, then pass/reference $TARGET_DB_PROVIDER; and/or have effective_database_provider return 1 with callers checking. Also guard cutover_database_provider against an empty target so it can never delete both providers.

[Minor] restore_namespace_gc_timing in the cleanup trap is not failure-guarded - tests/e2e/e2e-openshell.sh:89
The whole point of moving print_results into cleanup() is that the summary always prints. But restore_namespace_gc_timing is the first statement in the trap and is not || true like the surrounding cleanup calls. On the OpenShift driver it runs _openshift_require_config first, which can return 1; under set -e a non-zero return from the first trap command can stop the trap before print_results runs. Recommend restore_namespace_gc_timing || true for consistency with the other best-effort cleanup lines.

[Minor] PR scope says "no control plane changes," but there is one - components/control-plane/internal/gateway/reconciler.go:2252
The GRPCRoute sectionName changes from the hardcoded "grpc" to sharedGatewayListenerName(), and console.go renames the helper. This is intentional and matches the updated openshift-development.spec.md, but it is behavior-affecting: the Go default is "https", so on any deployment that previously relied on the hardcoded "grpc" without setting GATEWAY_API_HTTP_LISTENER_NAME, the GRPCRoute would now target a different listener. All in-tree overlays set the env var to grpc, so shipped behavior is unchanged, but the "E2E testing only" scope line understates the change and may cause reviewers to skip scrutinizing a reconciler edit. Please correct the description.

[Minor] .kind-swaps format change is not backward-compatible with existing files
The ledger format changes from component to component<TAB>image, and is_swapped now requires the trailing tab (^${component}[[:space:]]). A pre-existing single-column .kind-swaps from before this change will no longer be recognized. It is a local, gitignored dev-state file, so impact is negligible, but a one-line note in the spec/PR (or tolerating the old format on read) would save a confusing "why is my swap gone" moment.

Cross-PR coordination

Two open pull requests have material conflicts with this one that need a maintainer decision:

• A pull request adopting the upstream OpenShell Helm chart for gateway deployments removes the hand-built GRPCRoute block in components/control-plane/internal/gateway/reconciler.go (moving GRPCRoute creation into the chart) at the exact location this PR edits to make the GRPCRoute sectionName derive from the shared listener name. These are competing designs for the same routing responsibility. Maintainers should decide merge order and ensure the discovered/overridden listener name (GATEWAY_API_HTTP_LISTENER_NAME / sharedGatewayListenerName()) flows into that chart's GRPCRoute sectionName; otherwise the listener-mismatch fix in this PR is silently dropped when the Helm-based reconciler lands.

• A pull request adding an external mode to DATABASE_PROVIDER conflicts with this PR's new OpenShift effective_database_provider, which validates DATABASE_PROVIDER against a strict {cnpg, deployment} allowlist and rejects anything else. If both land, make openshift-up DATABASE_PROVIDER=external will be rejected. Maintainers need to decide whether external is a supported provider for openshift-up's platform database (and extend the case/cutover logic accordingly) or explicitly scope external as a non-OpenShift option.

Findings Summary (ordered by severity, highest first)

1. [Major] effective_database_provider exit 1 swallowed inside $(...), causing a silent fallthrough and a destructive empty-target cutover - Error Handling / Reconcile Safety (openshift.sh L571-589, L1183)
2. [Minor] Cleanup trap's restore_namespace_gc_timing not || true; can skip print_results on failure - Robustness (e2e-openshell.sh L89)
3. [Minor] PR description claims "no control plane changes" despite a behavior-affecting GRPCRoute sectionName change - Description Accuracy (reconciler.go L2252)
4. [Minor] .kind-swaps new format not backward-compatible with pre-existing files - Compatibility (kind/lib.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
Input validated	Pass
SecurityContext on pod specs	N/A (no pod specs changed)
Reconcile pattern (not create-or-skip)	Pass
Image references consistent across manifests	Pass
Conventional commit messages	Pass
OpenAPI client not manually edited	N/A
Test Diff Scrutiny (modified assertions)	Pass (existing assertions replaced by equivalent/stronger checks; changes are additive or track renamed helpers)
Shell error propagation (set -e / no swallowed exits)	Fail

[jsell-rh]

[Major] effective_database_provider uses exit 1 for its error paths, but here it runs inside $(...). exit 1 only terminates the command-substitution subshell, and set -e does not propagate a substitution failure in argument position, so the run continues. When DATABASE_PROVIDER=cnpg is set on a cluster without CNPG (or an unknown value is given), this passes an empty target to cutover_database_provider; both [[ "" != "cnpg" ]] and [[ "" != "deployment" ]] are true, so it deletes BOTH provider stacks (CNPG Cluster/PVCs and the bundled Deployment/Service) instead of aborting with the intended clear error. Resolve the provider once into a plain global assignment (TARGET_DB_PROVIDER="$(effective_database_provider)" on its own line, where set -e honors the failure) and pass the variable; also guard cutover_database_provider against an empty target.

[jsell-rh]

These exit 1 guards are ineffective at every call site that invokes this function via command substitution (the [[ "$(effective_database_provider)" != "cnpg" ]] guards and the cutover_database_provider "$(...)" call). Consider return 1 here with callers checking, or compute the value once via a plain assignment so set -e can act on the failure.

[jsell-rh]

[Minor] print_results was moved into this trap so the summary always prints, but restore_namespace_gc_timing is the first statement and is not || true like the other cleanup lines. On the OpenShift driver it calls _openshift_require_config, which can return 1; under set -e a non-zero return from the first trap command can stop the trap before print_results runs. Suggest restore_namespace_gc_timing || true.

[jsell-rh]

[Minor] This changes the GRPCRoute sectionName from a hardcoded "grpc" to sharedGatewayListenerName(), whose Go default is "https". It matches the updated spec and all in-tree overlays set GATEWAY_API_HTTP_LISTENER_NAME=grpc, so shipped behavior is unchanged - but this is a control-plane behavior change, which the PR body's "no API or control plane changes" scope line contradicts. Please correct the description so this reconciler edit gets appropriate review.