feat(gateway): per-Route TLS issuer for client-facing HTTP/2 (gRPC)

[markturansky]

Problem

The reencrypt Route fix (#221) eliminates the CLI's UnknownIssuer error — the OpenShift router terminates with the trusted Let's Encrypt *.apps wildcard — but the openshell CLI still can't connect over gRPC.

Root cause: OpenShift edge/reencrypt Routes advertise client-facing ALPN h2 only when the Route carries its own spec.tls.certificate. Routes riding the shared default *.apps wildcard are deliberately denied h2 (the router's cross-route connection-coalescing protection). gRPC (grpcs://) requires HTTP/2-over-TLS via ALPN, so a default-cert reencrypt Route can't serve gRPC even with ROUTER_DISABLE_HTTP2=false on the IngressController.

Confirmed empirically (openssl -alpn h2 → "No ALPN negotiated"; python ssl.set_alpn_protocols → None; curl --http2 → HTTP/1.1), and via a reversible test: stamping a per-Route cert flips all three to h2 + Verify return code: 0.

Change

New GATEWAY_ROUTE_TLS_ISSUER env knob. When set and termination is reencrypt, the controller:

1. Annotates the gateway Route with cert-manager.io/issuer-name: <issuer> + cert-manager.io/issuer-kind: ClusterIssuer, so the openshift-routes controller requests a cert-manager cert and injects it into Route.spec.tls.{certificate,key}.
2. Preserves that injected cert/key across reconciles. The reconciler's full Update would otherwise clobber the injected fields every reconcile — the two controllers co-own the Route.

No new RBAC: the annotation and cert-preservation reuse the existing route.openshift.io/routes access. When the knob is unset the behavior is unchanged.

Tests

reconciler_test.go — 4 new subtests:

• reencrypt + issuer → Route is annotated
• passthrough ignores the issuer
• reencrypt without issuer → no cert-manager annotation
• reconcile preserves an injected edge certificate

go build ✅ · go test ./internal/gateway/ ✅ · go vet ✅

Docs

• specs/platform/global-architecture.spec.md — clarifies HTTP/2-on-IngressController is necessary-but-not-sufficient; adds a per-gateway-cert requirement with scenarios.
• skills/deploy/ibm-cluster/SKILL.md — documents the h2/ALPN prerequisite and the GATEWAY_ROUTE_TLS_ISSUER knob.

Cluster prerequisites

The ClusterIssuer (letsencrypt-http01) and the openshift-routes injector must exist on the cluster — provisioned via the companion hypershell-gitops PR.

🤖 Generated with Claude Code

[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: 9fdeb342-6af7-431f-abae-f150173facac

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

COMMENT. This is a tight, well-scoped change: the GATEWAY_ROUTE_TLS_ISSUER knob is off by default, the reencrypt-only gating is correct, and the spec/skill docs and four additive tests match the behavior precisely. One robustness gap in the cert-preservation read path is worth addressing before merge, but it is self-healing and not a blocker.

Findings

[Major] readInjectedRouteCert swallows all GET errors, defeating cert preservation on transient failures — Error Handling / Reconciliation (reconciler.go L2128–2131). Confidence: Medium.

The helper returns "", "" for any GET error, including transient API errors, not only IsNotFound. Because reconcileResource does a full Update (replace) of the Route, a transient GET failure during a reconcile causes the injected spec.tls.certificate/key to be omitted and therefore stripped on that write — which flaps ALPN h2 and can tear down active long-lived gRPC streams. That is precisely the failure the preservation logic (and the 3600s router timeout) exist to prevent, and silently dropping the value on error also runs against the "never silently swallow partial failures" convention.

Suggested fix: distinguish k8serrors.IsNotFound(err) (genuinely no cert yet → proceed) from other errors (log and either fail closed by returning an error from reconcileRouteResources so the reconcile retries, or otherwise avoid clobbering the existing cert). A cleaner alternative is to preserve the cert from the existing object that reconcileResource already fetches, avoiding the extra GET entirely.

Cross-PR coordination

Another open pull request (#194) reworks gateway deployment from the imperative manifest/reconcileRouteResources path to a Helm-chart–based deployment, removing the per-resource Route manifest and restructuring the same reconcile flow this PR extends. That is a competing design for how the tenant openshell-gateway Route is produced: this PR embeds the cert-manager issuer annotation and the injected-cert preservation directly in reconcileRouteResources, whereas under the Helm model that logic would have to be re-expressed through chart values and Helm upgrade semantics (where the full-replace clobber concern changes shape). Maintainers should decide the merge order and where the GATEWAY_ROUTE_TLS_ISSUER behavior lives so it is not silently lost or duplicated when the two directions meet.

Findings Summary (ordered by severity, highest first)

1. [Major] readInjectedRouteCert swallows non-NotFound GET errors, stripping the injected edge cert and flapping ALPN h2/gRPC on transient failures — Error Handling / Reconciliation (L2128–2131)

Convention Checklist

Convention	Result
No panic() in production code	Pass
Errors wrapped with fmt.Errorf context	Pass
errors.IsNotFound handled for 404 scenarios	Fail
No secrets in logs or responses	Pass
Input validated	Pass
Reconcile pattern used (not create-or-skip)	Pass
Proper context propagation	Pass
Test Diff Scrutiny (no flipped assertions; tests additive)	Pass
Config separate from code (env knob, default off)	Pass
Conventional commit message	Pass

[jsell-rh]

Verdict

COMMENT. This is a tight, well-scoped change: the GATEWAY_ROUTE_TLS_ISSUER knob is off by default, the reencrypt-only gating is correct, and the spec/skill docs and four additive tests match the behavior precisely. One robustness gap in the cert-preservation read path is worth addressing before merge, but it is self-healing and not a blocker.

Findings

[Major] readInjectedRouteCert swallows all GET errors, defeating cert preservation on transient failures — Error Handling / Reconciliation (reconciler.go L2128–2131). Confidence: Medium.

The helper returns "", "" for any GET error, including transient API errors, not only IsNotFound. Because reconcileResource does a full Update (replace) of the Route, a transient GET failure during a reconcile causes the injected spec.tls.certificate/key to be omitted and therefore stripped on that write — which flaps ALPN h2 and can tear down active long-lived gRPC streams. That is precisely the failure the preservation logic (and the 3600s router timeout) exist to prevent, and silently dropping the value on error also runs against the "never silently swallow partial failures" convention.

Suggested fix: distinguish k8serrors.IsNotFound(err) (genuinely no cert yet → proceed) from other errors (log and either fail closed by returning an error from reconcileRouteResources so the reconcile retries, or otherwise avoid clobbering the existing cert). A cleaner alternative is to preserve the cert from the existing object that reconcileResource already fetches, avoiding the extra GET entirely.

Cross-PR coordination

Another open pull request (#194) reworks gateway deployment from the imperative manifest/reconcileRouteResources path to a Helm-chart–based deployment, removing the per-resource Route manifest and restructuring the same reconcile flow this PR extends. That is a competing design for how the tenant openshell-gateway Route is produced: this PR embeds the cert-manager issuer annotation and the injected-cert preservation directly in reconcileRouteResources, whereas under the Helm model that logic would have to be re-expressed through chart values and Helm upgrade semantics (where the full-replace clobber concern changes shape). Maintainers should decide the merge order and where the GATEWAY_ROUTE_TLS_ISSUER behavior lives so it is not silently lost or duplicated when the two directions meet.

Findings Summary (ordered by severity, highest first)

1. [Major] readInjectedRouteCert swallows non-NotFound GET errors, stripping the injected edge cert and flapping ALPN h2/gRPC on transient failures — Error Handling / Reconciliation (L2128–2131)

Convention Checklist

Convention	Result
No panic() in production code	Pass
Errors wrapped with fmt.Errorf context	Pass
errors.IsNotFound handled for 404 scenarios	Fail
No secrets in logs or responses	Pass
Input validated	Pass
Reconcile pattern used (not create-or-skip)	Pass
Proper context propagation	Pass
Test Diff Scrutiny (no flipped assertions; tests additive)	Pass
Config separate from code (env knob, default off)	Pass
Conventional commit message	Pass

[jsell-rh]

The read swallows every GET error, not just IsNotFound, and returns "", "". Because reconcileResource replaces the whole Route via Update, a transient GET failure here drops the injected spec.tls.certificate/key on that reconcile — stripping the edge cert, flapping ALPN h2, and potentially killing active gRPC streams (the exact failure this preservation is meant to prevent). Distinguish k8serrors.IsNotFound(err) (no cert yet → proceed) from other errors (log and fail closed / avoid clobbering), or preserve from the existing object that reconcileResource already fetches.

[jsell-rh]

Amber review

Status: Complete

Verdict

This is a focused, well-documented change that adds an optional GATEWAY_ROUTE_TLS_ISSUER knob so a reencrypt Route gets its own cert-manager-issued certificate and can advertise ALPN h2 for gRPC, and it correctly preserves the co-owned edge cert across full-replace reconciles. The implementation is additive (default-off, ignored for passthrough), the four new subtests are purely additive with no rewritten assertions, and the spec/skill docs are updated in lockstep; I have no blocking findings, only a minor robustness note and a cross-PR coordination item.

[jsell-rh]

Verdict

This is a focused, well-documented change that adds an optional GATEWAY_ROUTE_TLS_ISSUER knob so a reencrypt Route gets its own cert-manager-issued certificate and can advertise ALPN h2 for gRPC, and it correctly preserves the co-owned edge cert across full-replace reconciles. The implementation is additive (default-off, ignored for passthrough), the four new subtests are purely additive with no rewritten assertions, and the spec/skill docs are updated in lockstep; I have no blocking findings, only a minor robustness note and a cross-PR coordination item.

Amber Assessment

What the PR does well

• Correctly scopes the new behavior: the cert-manager annotations are only applied when termination == reencrypt and the issuer is set, and the reencrypt tlsConfig map is the one that receives the carried-forward certificate/key, so passthrough Routes are untouched.
• The two-controller co-ownership problem (control plane owns termination/destinationCACertificate, openshift-routes owns the edge cert) is called out explicitly and handled by carrying the injected cert forward, avoiding an ALPN h2 flap on every reconcile.
• Tests are additive and match the review's Test Diff Scrutiny bar: no pre-existing assertion was flipped from accept->reject or optional->required; each new subtest asserts a distinct branch (annotated, ignored-for-passthrough, ignored-when-unset, cert preserved).
• Spec (global-architecture.spec.md) and skill (ibm-cluster/SKILL.md) are updated with matching scenarios, and the change adds no new control-plane RBAC (reuses existing Route access).
• No panic(), errors wrapped with context, IsNotFound->create/skip preserved in reconcileResource, no secrets logged. The Route certificate/key are carried in-memory only and not logged.

Minor findings

1. [Minor] readInjectedRouteCert (reconciler.go:2129-2130) collapses all Get errors to "", "", not just IsNotFound. On a transient API error the function reports "no injected cert," and because reconcileResource then does a full replace, the previously injected certificate/key are stripped for that reconcile - the exact ALPN h2/gRPC flap this PR is trying to prevent. It self-heals on the next successful reconcile, but consider distinguishing IsNotFound (expected on first create -> return empty) from other errors (log and treat as "unknown" so the strip is avoided). This also aligns with the project's "never silently swallow partial failures" convention.

2. [Minor / informational] There is a small TOCTOU window: readInjectedRouteCert performs its own Get (reconciler.go:624) before reconcileResource performs a second Get + Update. If openshift-routes injects the cert between those two reads, this reconcile omits it and strips it once before self-healing next pass. Acceptable given the design, but worth a one-line comment acknowledging the window (or reading the cert from the same object reconcileResource fetches).

Cross-PR coordination

There is a material design conflict with the open pull request that adopts the upstream OpenShell Helm chart for gateway deployments (feat(control-plane): adopt upstream OpenShell Helm chart for gateway deployments). That PR removes the large majority of the imperative provisioning in components/control-plane/internal/gateway/reconciler.go (~1000 lines, including the Route path) and its static manifests/gateway/route.yaml, relocating gateway/Route ownership into the Helm chart + values mapping. This PR instead extends the imperative reconcileRouteResources path with the cert-manager annotation and edge-cert preservation logic. These are competing designs for who owns and mints the gateway Route certificate, and they cannot both stand as written.

Maintainers need to decide (a) whether per-Route TLS-issuer ownership lives in the imperative reconciler or is expressed as Helm chart values, and (b) the merge order: if the Helm-adoption PR merges first, this PR's annotation + cert-preservation behavior must be re-expressed in the chart/values path; if this PR merges first, the Helm-adoption PR must carry the cert-manager.io/issuer-* annotation and injected-cert preservation forward into the Helm-based Route so the gRPC/ALPN fix is not lost.

Findings Summary (ordered by severity, highest first):

1. [Minor] readInjectedRouteCert swallows non-IsNotFound Get errors, allowing a transient error to strip the injected edge cert for one reconcile - Error Handling / Reconciliation (L2129)
2. [Minor] TOCTOU window between the cert read and the full-replace update can transiently strip the injected cert - Reconciliation (L624)

Convention Checklist:

Convention	Result
No panic() in production code	Pass
Errors wrapped with fmt.Errorf context	Pass
errors.IsNotFound handled for 404 scenarios	Pass (in reconcileResource)
No secrets in logs or responses	Pass
Reconcile pattern (update-or-create) used	Pass
SecurityContext on pod specs	N/A (no pod spec changed)
Test Diff Scrutiny (no flipped assertions)	Pass
Conventional commit messages	Pass
Config separate from code (env-driven knob)	Pass

[jsell-rh]

[Minor] This collapses all Get errors to "", "", not just IsNotFound. On a transient API error the caller sees "no injected cert", and since reconcileResource then does a full replace, the previously injected certificate/key are stripped for this reconcile - the exact ALPN h2/gRPC flap the PR aims to avoid. It self-heals next pass, but consider distinguishing IsNotFound (expected first-create -> return empty) from other errors (log + treat as unknown so the strip is skipped). Also aligns with the "never silently swallow partial failures" convention. Confidence: Medium.

[jsell-rh]

[Minor / informational] TOCTOU window: this Get runs before reconcileResource does its own Get + full-replace Update. If openshift-routes injects the cert between the two reads, this reconcile omits and strips it once before self-healing. Acceptable given the co-ownership design, but worth a comment noting the window - or read the cert from the same object reconcileResource fetches. Confidence: Medium.