feat(gateway)!: add TLS-passthrough listeners to TenantGateway by lexfrei · Pull Request #3342 · cozystack/cozystack

Aleksei Sviridkin (lexfrei) · 2026-07-17T19:26:34Z

What this PR does

Phase 1 of external database exposure via Gateway API TLS-passthrough (#2815): adds a tlsPassthroughListeners field to TenantGateway and renders one layer-4 mode: Passthrough listener per entry, on a native database port with a per-engine subdomain hostname, alongside the Gateway's terminate listeners. No engine is wired: no TLSRoute, no certificate or CA changes. The existing layer-7 tlsPassthroughServices field gains a 62-entry count bound, an item length and a DNS-1123 item pattern, and a repeated entry now fails the reconcile with Ready=False instead of rendering the same listener twice. spec.apex gains a 253-character bound and nothing else: the CEL rules concatenate it, so the install-time cost estimator refuses the CRD without a declared bound. It gains no format pattern, and that is not a claim that a mixed-case host works. Such a host reaches the field verbatim from a namespace label, the renderer then composes listener hostnames Gateway API refuses, and that tenant has no Gateway either way; what a pattern would change is only that the write is refused, which fails this HelmRelease where the same fault costs the one Gateway. An apex no listener hostname can sit within is reported by the controller instead, and only when tlsPassthroughListeners is in use. The constraints and what they do to objects that already exist are described further down. The bound on an entry is what the schema can state on its own. An entry also becomes a listener name and a hostname built from the apex, so the controller checks both composed values against the 253 characters Gateway API allows and names whichever one overflows.

Validation rejects duplicate listener names or ports, ports reserved by the platform Gateway (80 and 443), out-of-range ports, invalid DNS-1123 names, and hostnames outside the tenant apex (mirroring the Gateway hostname admission policy so an invalid entry cannot make the whole Gateway rejected).

The SNI-overlap rule refuses two passthrough listeners that answer the same name on different ports. That is what cilium#42898 reports: only one of the two routes ever works. It does not extend to a terminate listener and a passthrough listener sharing a name across ports. No failure is reported for that shape, and Cilium counts a terminate/passthrough pair as conflicting only when they share a port.

The per-engine subdomain hostname scheme follows the phase-1 gate. The flat-hostname alternative (pending the Cilium listener-isolation fix targeting 1.20) is a later scheduling decision that does not change this API field.

One shape differs from #2815 on purpose. The ticket has hostname optional with a *.<name>.<apex> default; here it is required. Deriving the SNI from the listener name hides the one value an operator has to get right, and the two drift apart the moment the naming scheme changes.

Found while checking that pairing, unrelated to this field: a stock dns01 gateway already renders tls-api (passthrough, 443) beside https (terminate, 443, *.<apex>), one port with overlapping SNI, which is the shape Cilium 1.19.6 marks Conflicted. That belongs to the Cilium bump and is recorded on #3343.

Also named rather than fixed: the controller-side rules on this field stop the whole reconcile when they refuse a spec. renderGateway returns an error, so the Gateway is not updated at all, and the tenant sees Ready=False with ReconcileError while new app listeners and every certificate the controller would issue or amend wait behind it. Certificates already issued go on renewing, since cert-manager drives that from the objects already in the cluster and an aborted reconcile writes nothing to them. Dropping only the conflicting listener and reporting it on status would be the better shape, but that changes the error model of the whole renderer rather than one rule, so it is out of scope here. The current behaviour is at least observable: Ready=False is a signal, not silent drift.

On HTTP-01, a hostname a passthrough listener answers earns no HTTPS-terminate listener once a TLSRoute claiming that hostname can be served on the listener, matched by SNI overlap so a wildcard entry reaches a name beneath it as readily as the name written in the spec, and the field itself is accepted under certMode http01 alone. That refusal is stated as the set of modes that can carry a passthrough listener rather than the set that cannot, because a mode added to the enum would otherwise be admitted by default: dns01 and existingSecret serve the whole apex from one wildcard terminate listener the pinned Cilium cannot keep apart from a passthrough listener beneath it, and edge ends TLS at the class provider and renders no TLS listener for one to sit beside. The route half of that condition is load-bearing rather than incidental. On the pinned v1.19.5, tlsPassthroughFilterChains walks a listener's routes and skips one with no backends, so a declared passthrough listener nothing attaches to contributes no filter chain and matches no ClientHello, while the terminate listener's chain is built from its own Secret and carries the name alone. Keying the withdrawal on the declaration would therefore take a name that is being served and hand it to a listener that forwards nowhere, and that is the ordinary shape rather than a corner: the chart puts api, vm-exportproxy and cdi-uploadproxy on every tenant Gateway, while there is one platform TLSRoute per name cluster-wide and each names one tenant's Gateway as its parent, so every other tenant renders all three entries with a route on none. The backend half is that same rule reached by a different road: the chain is built per route out of the backends that survived resolution, so a route that attached and forwards nowhere puts no chain on the name either, a backendRef naming a Service that does not exist yet because manifests applied out of order, one deleted since, a name with a typo, or one in another namespace with no ReferenceGrant, and without that half an ordinary backend mistake takes a live endpoint offline while Cilium builds nothing to replace it. Resolution asks what v1.19.5 asks and no more: the reference allowed, the Service present, and neither its ports nor its endpoints read, because the translator reads neither. Services and ReferenceGrants are watched alongside routes, so both halves are deferrals rather than exemptions and the withdrawal lands on the pass that sees the missing piece arrive. The withdrawal reaches HTTP-01 alone, because collectHostnameClaims returns nothing under the other two modes and they render no per-hostname listener to withdraw. A TLSRoute that declares no spec.hostnames claims the hostname of each rendered passthrough listener its parentRef selects, because that is the name Cilium serves it on: ComputeHosts at v1.19.5 substitutes the listener hostname for a route that names none, so such a route takes the name over exactly as one that spells it out does. The tenant-namespace hostname policy requires a route there to name its hostnames, so this shape reaches a Gateway from the system namespaces where the platform routes live, or from an administrator, not from a tenant chart. A claim broader than the listener's counts the same way from the other side: a TLSRoute claiming *.<apex> is served on api.<apex> rather than on the wildcard, because ComputeHosts returns the listener's own hostname for a route hostname covering it, so a claim covering a reserved name is judged as a claim on that name while the route keeps one entry under the key its owner wrote. Both restrictions are tied to the Cilium pin, currently v1.19.5. That version translates the Gateway into a single Envoy listener whose filter chains match on SNI without the port, so a terminate chain and a passthrough chain carrying one name collide regardless of which Gateway port declared them; upstream states the consequence above NeedsCrossProtocolSplit in operator/pkg/model/model.go as of v1.19.6, the release that starts splitting Envoy listeners per port when NeedsPerPortListeners holds. That is a disjunction rather than a blanket change, and the passthrough side of every one of its three clauses is counted from attached routes, so a listener with no TLSRoute behind it contributes to none of them: the split arrives with the route, not with the bump.

The same single-listener translation has a consequence that is not a restriction: the declared port is where the listener is published, not a bound on reach, because the backend answers its hostname's SNI on every port the Gateway exposes. Firewalling a tenant address down to 80 and 443 does not put a database out of reach; the hostname is the access surface, and the field's own doc and the chart README now say so.

Both restrictions are validation-only, and they lift together on the pin plus a configuration the split actually covers rather than on the bump alone.

A route whose hostname is withdrawn this way is told so under this controller's own name: Accepted=False with NoMatchingListenerHostname, and a message naming the passthrough hostname that answers it, which for a wildcard entry the route cannot derive on its own. It matters most there. A wildcard entry can withdraw the listener from several published names beneath it at once, each one a TLSRoute claims and can be served on, and the certificates are garbage-collected with them, and a rollback is cheaper than that sounds: cert-manager here runs with enableCertificateOwnerRef: false, so deleting a Certificate leaves its Secret behind and a re-created Certificate adopts a still-valid one rather than placing an order. Before this the only signal was Cilium's own RouteParentStatus entry under a different controllerName. A TLSRoute whose hostname no passthrough listener answers is told so as well, on the same condition and for the same reason: before this change that hostname got a terminate listener Cilium refused with ResolvedRefs=False, so withdrawing the listener would otherwise have removed the only object saying the route went nowhere.

Two things stay named rather than fixed. A tlsPassthroughListeners hostname of *.<apex> is refused only indirectly: the overlap rule rejects it while tlsPassthroughServices carries its defaults, because the wildcard covers api., so emptying that list is enough to let it through, and the terminate listener then goes from every published name a TLSRoute claims under the wildcard. The README states this; refusing the apex and its wildcard on their own terms is a one-line rule worth adding when the field is wired up to chart values.

A wildcard hostname on an attached HTTPRoute used to render a listener name the shipped Gateway CRD refuses, https-*-<hash>, which failed the whole Gateway write while the README said the shape worked. Listener and Certificate names now spell a leading * as wildcard, with the hash still covering the whole hostname, and a test reads both the listener-name and the hostname patterns off the shipped CRD bundle and renders wildcards through them; every name that was already admissible is pinned byte for byte, so nothing is renamed on upgrade. Under http01 an HTTPRoute wildcard claim is refused before the race and the overlap are read, with NoMatchingListenerHostname, because the HTTP-01 challenge issues no wildcard certificate and a terminate listener could never have carried the name.

A route status write that fails no longer leaves the route carrying its previous condition: updateRouteStatuses and the whole-apex pass collect every failed write and return them after the loop, so the reconcile fails, the TenantGateway reports Ready=False with ReconcileError and controller-runtime requeues, while the loop still runs to the end so one route's conflict does not hold the others back. Conditions this controller wrote under http01 are retracted when the tenant moves to dns01, existingSecret or edge, for HTTPRoutes and hostname-less TLSRoutes alike, so a refusal does not outlive the mode that produced it. And a TLSRoute claiming a wildcard passthrough entry itself leaves the names beneath the wildcard alone, since Envoy ranks an exact server name above the wildcard and nothing collides.

One thing this does not cover yet: Accepted is computed from the hostname race and the withdrawals above, so a route is still told it is fine when an HTTPRoute holds the name and the TLSRoute claiming it cannot attach to the terminate listener that answers it. That one predates this change. Two neighbours of it no longer fall in the gap. A parentRef port that names no listener answering the hostname is compared alongside the sectionName, since Gateway API requires every key a route pins itself with to match the listener it selects. An HTTPRoute's parentRef.port is judged the same way: the terminate listener sits on 443, so any other pinned port earns no listener and no certificate through that parentRef and reads the port-mismatch cause; port 80, or sectionName: http, from a namespace the plain-HTTP listener admits (the publishing tenant and cozy-cert-manager) is served plain with no terminate listener and no refusal, from any other namespace it is refused with NotAllowedByListeners, and a sectionName and a port that select different listeners select none and read NoMatchingParent. A plain-served route stays in the hostname race on the same footing as a terminating one, in every recount as well as the first count. And a sectionName naming a tls-<name> listener this Gateway does not render now reports Accepted=False with NoMatchingParent, Gateway API's reason for a parentRef matching no listener: the controller renders every listener in that namespace, so an absent name there is a fact rather than a guess, while a sectionName naming the plain-HTTP listener is judged like port 80 below, and any other name outside the prefix is written Accepted like an attached route, with the class controller's own status entry answering for the listener it named.

The namespace case is off that list. Native-port listeners admit the publishing tenant alone, and a TLSRoute elsewhere on one of their hostnames is refused with NotAllowedByListeners. An HTTPRoute that pins itself by sectionName to a rendered tls-<name> passthrough listener is refused with the same reason: that listener forwards the stream it matched by SNI and terminates nothing, so it takes TLSRoute alone by protocol and cannot serve the route whatever answers its hostname. Its claim also leaves the hostname race before ownership is ranked, so the TLSRoute that can be served is not told it lost to a route that could never attach. Cilium does not report this refusal at the pin. The port-443 passthrough listeners name HTTPRoute in allowedRoutes.kinds to keep the port-443 set uniform (cilium#45559), the Gateway reports that entry back as ResolvedRefs=False/InvalidRouteKinds on the listener, and Cilium's route-side kind check reads a listener's kinds against every route on the Gateway rather than against the routes that named it, so it reports the route Accepted and the condition this controller writes is the only refusal the route gets. A hostname whose every claimant is refused this way earns no HTTPS-terminate listener and no certificate, where before it earned both. The refusal is decided per route rather than per hostname, which matters once a claim overlaps more than one reserved name: a wildcard covering both a port-443 service hostname and a native-port one is attachable through the first and refused through the second, so the answer comes from the listener the parentRef names, its own hostname and its own attach set. The row that pinned a sectionName naming no rendered listener as a known gap now pins the verdict instead. On the port-443 listeners the two namespace sets agree by construction, since collectHostnameClaims and the listeners' allowedRoutes selector run off the same list.

And the justification for the pin is written out at every site that depends on it rather than once, so the day the pin moves they all have to move together and the one that does not is a false statement no diff can catch. That is not hypothetical here: three claims of exactly that shape were wrong in this branch and were caught by reading the source they described rather than by any diff, so the volume of restated reasoning is itself what produces them. One canonical block with references to it would carry the same knowledge with one place to update, and rewriting the narrative under review would be a documentation change rather than a fix, so it is tracked on #3842 instead.

The gateway chainsaw suite now covers the withdrawal, which it did not before: a tenant owning its own Gateway renders the shipped passthrough entries with nothing routed to them, an HTTPRoute on api. keeps its terminate listener, its certificate and Accepted=True there, and attaching a TLSRoute to the same listener, pointed at a Service in the tenant's own namespace, since a route that forwards nowhere is the case the first phase already measures, withdraws all three and moves the route to NoMatchingListenerHostname. A third phase patches in a native-port entry and reads the per-listener readiness flag, which is the claim no test against a fake client can settle. The first two phases are one measurement: withdrawing on demand is what dates the earlier assertion to settled state rather than to a withdrawal that had not arrived yet.

The Require API owner review for sizeable API changes check is red on purpose and does not clear with a code change. A new field plus tightened validation on already-shipped fields counts as a sizeable API change, so .github/workflows/api-review-gate.yaml holds the PR until an API owner approves it. The gate counts each owner's latest review on the current head commit, so the approval has to land on the head as pushed and any later push invalidates it; the job re-runs on review submission, so no push is needed to flip it. Reading that red as "not ready" is the mistake this paragraph exists to prevent.

Part of #2815.

Release note

feat(gateway): TenantGateway supports layer-4 TLS-passthrough listeners on native database ports for SNI-based external exposure. On HTTP-01 gateways a hostname a TLS-passthrough listener answers, including any name beneath a wildcard entry, no longer earns an HTTPS-terminate listener or a Gateway-issued certificate once a TLSRoute claiming that hostname can be served on the listener and forwards to a backend that resolves; a hostname claimed only by a TLSRoute stops earning one too. A TLSRoute with no spec.hostnames counts as claiming the hostname of each passthrough listener its parentRef selects, and one whose hostname covers a listener's counts as claiming that listener's name, both being the name Cilium serves it on. A declared passthrough listener with no route on it changes nothing, because it emits no filter chain and the terminate listener is what answers the name, and neither does a route whose backendRefs resolve to nothing, a Service that does not exist, or one in another namespace with no ReferenceGrant, since such a route emits no filter chain either. The controller watches Services and ReferenceGrants, so the withdrawal it defers lands once the backend appears. So the tenant whose Gateway the platform TLSRoutes name, tenant-root unless the cluster's expose-ingress says otherwise, sheds the terminate listeners and certificates for api, vm-exportproxy and cdi-uploadproxy on upgrade (those TLSRoutes ship with the cozystack-api, kubevirt and kubevirt-cdi packages, each rendered only where the Gateway integration is enabled and the service is among the platform's exposed services), and a tenant that renders the same three entries with no route on them keeps what it has. The shed certificates leave their TLS Secrets behind: cert-manager here ships enableCertificateOwnerRef: false, so each Secret stays in the namespace with no owner, nothing renewing it and nothing deleting it. That cuts both ways, as stale Secrets to remove by hand and as a cheap way back, since a re-created Certificate adopts a still-valid Secret instead of placing an order. A route the controller renders no listener it can attach to now carries Accepted=False from this controller instead of Accepted=True. Five shapes report NoMatchingListenerHostname and the message tells them apart: the HTTPRoute whose hostname a passthrough listener answers, the TLSRoute with no passthrough listener behind it, the route whose parentRef names a listener that answers a different hostname, the route whose parentRef names a port no listener answering its hostname is published on, and the HTTPRoute claiming a wildcard hostname under http01, which the HTTP-01 challenge cannot certify. A route pinned by sectionName to a tls-<name> listener the Gateway does not render carries NoMatchingParent instead, which is what Gateway API calls a parentRef matching no listener, and so does an HTTPRoute whose sectionName and parentRef port select different listeners. A TLSRoute outside the publishing tenant's namespace claiming a native-port hostname carries NotAllowedByListeners instead, since those listeners admit the tenant alone. So does an HTTPRoute pinned by sectionName to a rendered tls-<name> passthrough listener, which cannot serve its kind, and an HTTPRoute selecting the plain-HTTP listener from a namespace it does not admit; a refused route also stops counting in the hostname race, so a hostname whose every claimant is refused earns no terminate listener and no certificate. Listener and Certificate names derived from a wildcard hostname are admissible now, names that were already admissible do not change, a failed route-status write fails the reconcile instead of being logged away, and conditions written under http01 are retracted when certMode leaves it. The dns01 and existingSecret cert modes shed nothing, because neither renders per-hostname listeners at all, and tlsPassthroughListeners is accepted under http01 alone: those two serve the whole apex from one wildcard terminate listener, and edge terminates TLS upstream and renders no TLS listener a passthrough entry could sit beside. The withdrawal does not take a working path away: on the pinned Cilium both port-443 filter chains already carry these hostnames and nothing on the objects says which one answers a connection, so a race that was previously undetermined and unobservable now resolves the same way every time. Treat this as a change of serving path rather than the removal of something idle: today each of those three hostnames has a terminate listener and a passthrough listener on port 443, both filter chains carry the SNI, and nothing on the objects says which one answers a connection. Gateway API calls for both listeners to report Conflicted; the pinned v1.19.5 has no Conflicted condition at all, so there is nothing to read, and you cannot tell from the cluster whether api.<apex> is terminated by the Gateway or passed through. Afterwards it is passed through, always. Read the Gateway's listener conditions before upgrading to see what those listeners were serving. Before upgrading, confirm the management apiserver serving certificate carries api.<apex> in its SANs, because once the ACME certificate is withdrawn that is the only certificate left on the name; a cluster that came from the ingress-nginx path already meets that, a cluster served by the Gateway's ACME certificate since it moved does not necessarily and loses the name until the SAN is added. If something depends on the terminate path for one of those names, the package README lists three ways forward and what each costs. Moving the consumer to another hostname under the apex is the only one that gives up nothing.

Downstream repositories

Walked the trigger map in docs/agents/contributing.md against this diff: api/gateway/v1alpha1/{tenantgateway_types.go,zz_generated.deepcopy.go}, internal/controller/tenantgateway/{reconciler,renderers,conflict,mapper,status,backends,reconciler_test,crdcel_test,mapper_test,backends_test}.go, internal/controller/tenantgateway/rbac_test.go, cmd/cozystack-controller/main.go, hack/e2e-chainsaw/gateway/chainsaw-test.yaml, packages/system/cozystack-controller/definitions/gateway.cozystack.io_tenantgateways.yaml, packages/system/cozystack-controller/templates/rbac.yaml, packages/core/platform/values.yaml, and packages/extra/gateway/README.md. No trigger fires, and the two repositories worth ruling out explicitly are below rather than waved off.

cozystack/website: no package under packages/apps/ or packages/extra/ is added, renamed or removed. packages/extra/gateway/README.md is edited in place. The docs generator regenerates a reference page from a package README only for the apps hardcoded in the website Makefile, and gateway is in none of APPS, K8S, VMS, NETWORKING or SERVICES, so there is no generated page for this package to drift out of step with. That gap is the same shape the trigger map already records for packages/apps/opensearch and packages/apps/bucket; it predates this change and is neither created nor widened here. None of the other website triggers apply: packages/core/platform/values.yaml changes only in a comment, which now says that api, vm-exportproxy and cdi-uploadproxy render a TLSRoute rather than an HTTPRoute, with no key added, moved or given a new default, no platform variant or component moves, no release asset is renamed, ApplicationDefinition semantics are unchanged, there is no Talos bump and no developer-tooling change.

cozystack/terraform-provider-cozystack: this adds an API field, so the field-level triggers are the ones to check, and they are keyed to app charts rather than to this CRD. No app is added under packages/apps/; no values.schema.json gains, loses or renames a field; no version enum, values.yaml default, kind, plural, release.prefix or output Secret/Service name changes. TenantGateway is not among the CRDs the provider types by hand. Those are Package under cozystack.io and Plan/RestoreJob under backups.cozystack.io, and the provider carries no reference to gateway.cozystack.io at all. It therefore falls under the map's last bullet: the spec travels as opaque JSON, so the provider keeps working and simply offers no typed access to tlsPassthroughListeners, which is the position it already holds for every other field it does not model, and not a break. The provider's new-top-level-field test guard is scoped to api/apps/v1alpha1, which this diff does not touch.

cozystack/ansible-cozystack: nothing in reach. packages/core/installer/values.yaml and the cozy-system/PSA contract are untouched, no platform variant is added or renamed, none of the packages/core/platform/values.yaml keys the role sets are involved (the file changes in a comment alone), no object the role waits on is renamed, and hack/e2e-prepare-cluster.bats node prerequisites are unchanged.

The schema constraints this diff adds to already-shipped fields all reach objects that already exist, and every one of them is a kind the apiserver ratchets. spec.apex gains a 253-character limit (no pattern: a mixed-case apex is admitted and refused by the controller only when passthrough listeners are declared), and spec.tlsPassthroughServices gains a DNS-1123 item pattern and a length bound. A stored value that violates one still passes any write that leaves it unchanged, so the rule bites only once the value itself changes. A repeated tlsPassthroughServices entry is reported by the controller as Ready=False rather than refused at admission, and the reason is not the one this section used to give. Ratcheting does reach a list-type error: customresource.ValidateUpdate validates an incoming object's list-type semantics only when the stored object passes them, so a stored duplicate is ratcheted through rather than blocking every write. What makes the marker the wrong tool is the grain of that ratchet, which is the whole object rather than the field, so one stored duplicate would switch list-type checking off for every list on the TenantGateway at once and say nothing about it. Documented in packages/extra/gateway/README.md, and pinned by TestTightenedConstraintsOnExistingObjects together with TestListTypeSetWouldNotRefuseAStoredDuplicate, which compiles a copy of the shipped schema carrying the marker to show what the apiserver actually does with one. Shipped defaults conform and the chart already fails on an empty host, so this is a no-op in practice.

No downstream repository is affected by this change
cozystack/website - follow-up:
cozystack/terraform-provider-cozystack - follow-up:
cozystack/ansible-cozystack - follow-up:
cozystack/ccp - follow-up:
cozystack/talm - follow-up:
cozystack/cozyhr - follow-up:

Summary by CodeRabbit

New Features
- Added layer-4 TLS passthrough listeners (tlsPassthroughListeners) with native port + SNI hostname, rendered as passthrough listeners and limited to TLSRoute attachment.
Bug Fixes
- Added stricter admission-time and controller validation (reserved ports 80/443, distinct listener ports, tenant-apex hostname rules, and collision prevention with passthrough services).
- Improved listener rendering safety: prevents exceeding the Gateway listener cap and ensures AllowedRoutes are isolated per listener.
Documentation
- Updated gateway docs with listener-cap considerations and layer-4 passthrough usage details.
Tests
- Added CRD/CEL and reconciliation coverage for valid/invalid configurations and rendering behavior.

gemini-code-assist · 2026-07-17T19:26:37Z

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

coderabbitai · 2026-07-17T19:26:42Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

📝 Walkthrough

Walkthrough

TenantGateway gains structured TLS passthrough listeners with CRD and controller validation, Gateway rendering, listener-count limits, route selector isolation, generated deepcopy support, comprehensive tests, and updated gateway documentation.

Changes

Tenant TLS passthrough listeners

Layer / File(s)	Summary
Listener API contract `api/gateway/v1alpha1/...`, `packages/system/cozystack-controller/definitions/...`	Adds `TLSPassthroughListener`, bounded name-keyed configuration, strengthened schema/CEL rules, and deepcopy support.
Validation and Gateway rendering `internal/controller/tenantgateway/renderers.go`, `internal/controller/tenantgateway/reconciler.go`, `internal/controller/tenantgateway/conflict.go`	Validates listener names, ports, hostnames, and collisions; renders TLS-only listeners with isolated route policies, updates hostname selection, and enforces the Gateway listener cap.
Listener behavior coverage `internal/controller/tenantgateway/*_test.go`, `packages/extra/gateway/README.md`	Tests admission/controller parity, rendering, route selectors, aliasing, rejection cases, and listener limits; documents passthrough behavior and namespace selection.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant TenantGateway
  participant Admission
  participant Reconciler
  participant Gateway
  TenantGateway->>Admission: submit listener configuration
  Admission->>Admission: apply structural schema and CEL rules
  Admission-->>TenantGateway: accept or reject specification
  TenantGateway->>Reconciler: reconcile accepted specification
  Reconciler->>Reconciler: validate and render listeners
  Reconciler->>Gateway: publish TLS passthrough listeners and route policies

Possibly related issues

cozystack/cozystack#2478 — Covers end-to-end testing for TLS passthrough listeners and TLSRoutes introduced by this change.

Suggested reviewers: androndo

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: adding TLS passthrough listeners to TenantGateway.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch feat/tenant-gateway-tls-passthrough-listeners

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

gemini-code-assist · 2026-07-17T19:27:34Z

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

IvanHunters

Reviewed hermetically. LGTM with non-blocking notes.

Verified:

go build/vet/test green; zz_generated.deepcopy.go is committed and matches the new TLSPassthroughListener type and CRD markers (no make generate staleness).
Tests are non-vacuous: neutralising the reserved-port guard turns the port-collision subtests red. Negative cases cover duplicate name/port, reserved ports, DNS-1123, malformed/wildcard hostname, out-of-apex, service-name collision.
Validation errors surface on status (Ready=False, Reason=ReconcileError), not just logs.
hostnameWithinApex matches the cozystack-gateway-hostname-policy VAP CEL; it is a defensive pre-check, the VAP stays authoritative.
Upgrade: additive +optional/omitempty field, no default flip, no migration.

Non-blocking:

Phased feature: a rendered passthrough listener has no TLSRoute/certificate wiring yet (later phases), so a tenant setting tlsPassthroughListeners gets a listener that does nothing until they supply their own TLSRoute and backend. Worth a docs note.
One-port-per-listener (duplicate port rejected) is enforced by design rather than SNI-multiplexing; flagging.

Aleksei Sviridkin (lexfrei) · 2026-07-19T21:34:07Z

Both notes were right, thanks.

The phased-feature note went further than a docs line: the field description now states that the listener is all it creates, that nothing routes until a TLSRoute attaches by sectionName: tls-<name>, and that the backend rather than the Gateway holds the certificate for that hostname. The package README gained a matching section — its listener-budget arithmetic had not counted these listeners either.

On one-port-per-listener you read it correctly: it is enforced, not inherent. Gateway API keys TLS listeners by (port, protocol, hostname), so same-port SNI selection is legal and the port-443 passthrough listeners already rely on it. It stays for this phase because the field's whole point is the engine's native port, where a second listener means two engines behind one port with SNI deciding, and there is no routing or certificate plumbing yet to make that shape testable. Lifting it is deleting a check — no API or schema change — so it remains available later. That reasoning is now in the code instead of implied.

Two things the follow-up pass turned up that are worth flagging: the validation rules initially pushed the CRD past the apiserver's install-time CEL cost budget, making it un-installable on both create and upgrade while every per-request test stayed green — there is now a test running the real CRD validation; and two passthrough listeners could match the same SNI, including a wildcard covering the api.<apex> listener the chart ships by default, which the pinned Cilium cannot disambiguate.

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/controller/tenantgateway/crdcel_test.go`:
- Around line 327-340: Add an explicit version-found guard to both CRD schema
validation loops in the relevant tests, including
TestPassthroughListenerCapFitsGatewayAPI. Track whether v1alpha1 was encountered
and fail the test when it is absent, while preserving the existing assertions
for the located version.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 7e2120f5-f9fb-4f81-84f3-1387e95dd346

📥 Commits

Reviewing files that changed from the base of the PR and between acaffb4 and 3958dd1.

📒 Files selected for processing (8)

api/gateway/v1alpha1/tenantgateway_types.go
api/gateway/v1alpha1/zz_generated.deepcopy.go
internal/controller/tenantgateway/crdcel_test.go
internal/controller/tenantgateway/reconciler.go
internal/controller/tenantgateway/reconciler_test.go
internal/controller/tenantgateway/renderers.go
packages/extra/gateway/README.md
packages/system/cozystack-controller/definitions/gateway.cozystack.io_tenantgateways.yaml

🚧 Files skipped from review as they are similar to previous changes (6)

internal/controller/tenantgateway/renderers.go
packages/system/cozystack-controller/definitions/gateway.cozystack.io_tenantgateways.yaml
internal/controller/tenantgateway/reconciler.go
packages/extra/gateway/README.md
api/gateway/v1alpha1/tenantgateway_types.go
internal/controller/tenantgateway/reconciler_test.go

myasnikovdaniil

NOT LGTM — the mechanism is sound and phase 1 is unusually well hedged, but this PR adds documentation that the controller does not honor, and the TLSRoute path it exists to enable has no test coverage at all. Separately, it is hard-blocked by the API-owner gate regardless of technical verdict.

Business context: phase 1 of #2815 (external database exposure via SNI passthrough) — adds the tlsPassthroughListeners API field and renders one layer-4 mode: Passthrough listener per entry on a native DB port, with no engine, TLSRoute, or certificate wiring yet.

Process gate — needs an API owner, nothing in the diff can satisfy it

.github/workflows/api-review-gate.yaml:36 sets API_OWNERS: "lllamnyp kvaps", and the approval query at :139-151 counts only each reviewer's latest review on the current head commit (select(.commit_id == $head)) with state APPROVED. So this needs Timofei Larkin (@lllamnyp) or Andrei Kvapil (@kvaps) to approve head 3958dd163 directly; any new push invalidates it. The job re-runs on pull_request_review, so an approval flips the check without needing a push. Flagging so it is not mistaken for something the diff can fix.

Also worth knowing before anyone reads the check list as a code signal: gh pr checks reports E2E Tests as fail, but the API reports "conclusion": "cancelled" for job 88245594808 after 1h2m. That is a concurrency cancellation, not a test failure.

Blockers

B1: the field's documented contract is falsified by the controller once a TLSRoute attaches

api/gateway/v1alpha1/tenantgateway_types.go:331 and :337, internal/controller/tenantgateway/reconciler.go:907

Evidence — verified by execution, not by reading. HTTP-01 TenantGateway, one entry {postgres, 5432, postgres.foo.example.com}, plus a TLSRoute in tenant-foo with sectionName: tls-postgres and spec.hostnames: [postgres.foo.example.com]. Rendered result:

LISTENER name=http                    port=80    proto=HTTP
LISTENER name=https-postgres-715667a8 port=443   proto=HTTPS  host=postgres.foo.example.com
LISTENER name=tls-postgres            port=5432  proto=TLS    host=postgres.foo.example.com
CERT     name=cozystack-postgres-715667a8-tls   dnsNames=[postgres.foo.example.com]

collectHostnameClaims (reconciler.go:343-367) folds TLSRoute hostnames into the same claims map as HTTPRoute hostnames; those become winners → dynHostnames (:136-142), which drives both per-app HTTPS listener rendering and reconcilePerListenerCertificates (:175). It never consults sectionName or listener protocol.

Three consequences, each contradicting text this PR adds:

:331 says "the backend must present its own certificate … the Gateway neither holds nor issues that certificate". It does issue one — a real Let's Encrypt order against a hostname that needs no Gateway certificate, consuming LE rate limit.
:337 says the field costs "one per entry here", and :346 caps MaxItems=62. A routed entry costs two listener slots, so the maxGatewayListeners guard at reconciler.go:937 can trip at roughly half the documented headroom.
A stray HTTPS-terminate listener appears for the database hostname on :443 and will never serve anything useful.

Important qualifier: the underlying defect is pre-existing, not introduced here. I re-ran the identical probe using TLSPassthroughServices — the field already on main, with collectHostnameClaims untouched by this PR — and reproduced the same leak. There it is worse, producing https-postgres-… and tls-postgres both on port 443 with the same hostname, which is a Gateway API protocol conflict.

So this is not "your new code is broken". It is that the PR adds prose asserting a property the codebase does not have, in the exact area phase 2 will build on.

Minimum fix: correct the two claims at :331 and :337 and file a tracking issue — phase 2 is precisely when TLSRoutes get wired and this fires for real. Better fix: filter routeKindTLS refs out of the dynHostnames computation while keeping them in allRefs for conflict and status purposes.

Raised independently by automated review as its sole finding; the pre-existing nature is my addition.

B2: no TLSRoute appears anywhere in the test suite

internal/controller/tenantgateway/reconciler_test.go

Evidence: rg -n "TLSRoute\{" internal/controller/tenantgateway/reconciler_test.go returns zero hits across ~4,500 lines. There is an httpRouteAttached helper (:1648) with no TLSRoute counterpart. The entire TLSRoute branch of collectHostnameClaims is unexercised — which is why B1 survived 22 commits unnoticed.

Given this PR's whole purpose is to make TLSRoute attachment possible, and the area has a substantial existing test convention, shipping the enabling change with no test of the thing being enabled is the gap worth closing before merge.

Fix: add a tlsRouteAttached helper and one test pinning what a TLSRoute attached to a passthrough listener should and should not produce. That test is also what would have caught B1.

Action items (non-blocking)

1. The route-hostname VAP has a hostname-less escape. packages/system/cozystack-basics/templates/route-hostname-policy.yaml:38 reads !has(object.spec.hostnames) || object.spec.hostnames.all(…). A TLSRoute that omits hostnames passes the VAP and, per Gateway API ([email protected]/apis/v1alpha2/tlsroute_types.go:65-67), inherits all the listener's hostnames. Combined with the attach set — packages/apps/tenant/templates/namespace.yaml stamps namespace.cozystack.io/gateway on every inheriting child tenant namespace — a child could in principle attach to a parent's passthrough listener.

Materially downgraded by the README's own note that "tenants in Cozystack do not hold gateway.networking.k8s.io/* RBAC by design, so this is not a tenant-user defense" (packages/extra/gateway/README.md:110), so this is defense-in-depth only and pre-existing. But on a native DB port the failure mode has teeth: PostgreSQL sslmode=require does not verify the server certificate, so a hijacking backend could harvest credentials. Worth closing before phase 2 rather than after.

2. The open nit on crdcel_test.go:327-340 (add a version-found guard to the CRD schema loops) is legitimate and cheap.

3. The renderWildcardCertificate gofmt-only hunk in renderers.go (the seen map realignment) is unrelated noise in an already-oversized diff.

Phase-1 risk assessment: safe, and better hedged than most

Worth stating plainly since it is the main thing a reviewer will worry about — the field is not reachable through any supported path:

packages/extra/gateway/templates/tenantgateway.yaml plumbs attachedNamespaces and tlsPassthroughServices but has no tlsPassthroughListeners stanza, and the README says so outright: "the chart exposes no corresponding value yet … a kubectl edit adding tlsPassthroughListeners is drift that Flux reverts on its next reconcile."
Reaching the field therefore requires cluster-admin RBAC on TenantGateway plus a suspended HelmRelease.
If someone does set it, validation is genuinely thorough. validateTLSPassthroughListeners (renderers.go:233) rejects bad DNS-1123 names, duplicate names, collisions with tlsPassthroughServices, out-of-range ports, reserved 80/443, duplicate ports, malformed hostnames, out-of-apex hostnames, and wildcard-aware SNI overlap (hostnamesOverlap, :341). Four are additionally enforced at admission via CEL. Failures surface as Ready=False on TenantGateway status rather than a wholesale Gateway rejection — the stated design goal, and it is met.
The listener renders pointing at nothing until a TLSRoute attaches: inert, not half-working — except for B1, which fires only once a TLSRoute exists.

Reversibility is adequate without a feature gate: v1alpha1, +optional, omitempty, no default, chart-unreachable. If phase 2 invalidates the design the field can be removed with no user-visible break.

Dismissed after investigation

Listener collapse (cilium#45559) regression — the new listeners sit on distinct native ports, so same-port collapse cannot apply, and they correctly carry Kinds = [TLSRoute] alone rather than the shared port443Kinds (reconciler.go:907-921), with the reasoning in-code. TestReconcile_Port443ListenersShareKinds filters on l.Port == 443 (reconciler_test.go:4227), so the new listeners are correctly outside its contract and the guard is intact. Bonus: Kinds=[TLSRoute] means len(supportedKinds) == len(AllowedRoutes.Kinds), so these listeners will not surface the cosmetic ResolvedRefs=False/InvalidRouteKinds that the :443 ones do on Cilium 1.19.x.
Weakening the GRPCRoute/TCPRoute/UDPRoute exclusion — reconciler_test.go:4262-4266 still asserts the forbidden-kind set for :443, and TestReconcile_TLSPassthroughListenerObjects (:2156-2159) asserts the new listeners carry exactly [TLSRoute] plus the namespace selector. Strictly tighter than the existing bar.
Stale generated artifacts — zz_generated.deepcopy.go carries the new type, the CRD YAML matches every marker, and Verify generated code is up to date passes. go build ./... and go test ./internal/controller/tenantgateway/... both green locally.
"No negative-path tests" — disproven for the validation layer: eight dedicated rejection-path test functions (reconciler_test.go:2451, 2480, 2509, 2539; crdcel_test.go:318, 354, 415, 455), including TestSpecCELMatchesControllerValidation for CEL/controller parity and TestCRDPassesInstallTimeValidation for the CEL cost budget — the latter a genuinely good catch. The gap is narrower than "no tests": it is specifically the TLSRoute interaction, which is B2.
The aliasing changes (slices.Clone(port443Kinds), DeepCopy()) fix a real latent hazard — the old passthroughAllowed := *allowedRoutes shared both the Namespaces pointer and the Kinds slice across every listener. Nothing mutated them, so it was latent rather than active; the fix is correct and covered by TestReconcile_ListenerAllowedRoutesNotAliased (:2238).

Scope drift

The advertised +571/-1 across 6 files was exact at commit 3a60cc72a, the 2nd of 22 — git diff --stat origin/main...3a60cc72a reports precisely that. Current head is +1662/-38 across 8 files, so 20 subsequent commits added roughly 3×:

Category	Weight
New CEL admission layer + cost-budget guard	`crdcel_test.go` (466 lines, new file) + ~100 lines of markers
Wildcard-aware SNI overlap matching	`hostnamesOverlap` / `hostnameCovers` in `renderers.go`
AllowedRoutes aliasing fixes	`reconciler.go` — touches pre-existing :443 and wildcard listeners
Listener-cap guard	`reconciler.go:937`
Docs	README + large doc-comment blocks

Two observations worth putting on the record rather than blocking on: the aliasing fixes and the tlsPassthroughServices tightening (MaxItems, listType=set, item pattern) are behavior changes to pre-existing fields that the title does not advertise and that the PR body itself notes affect already-deployed objects; and a large share of the added lines is prose inside Go doc comments, with the TenantGatewaySpec block alone running ~35 lines of design rationale. The latter is defensible — the CEL-cost reasoning is genuinely non-obvious — but it inflates the diff against the size/XXL label. Splitting the pre-existing-field hardening into its own PR would have kept phase 1 near its advertised size and made both halves easier to review.

coderabbitai

🧹 Nitpick comments (1)

internal/controller/tenantgateway/crdcel_test.go (1)
352-352: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Avoid duplicating the listener cap magic number.

gatewayAPIListenerCap hardcodes 64 locally instead of referencing the actual maxGatewayListeners constant checked in renderGateway (reconciler.go). If that constant ever changes, this test silently stops verifying the real relationship it's meant to pin.
♻️ Proposed fix
 func TestPassthroughListenerCapFitsGatewayAPI(t *testing.T) {
-	const gatewayAPIListenerCap = 64
+	const gatewayAPIListenerCap = maxGatewayListeners
 
 	field := v1alpha1SpecSchema(t).Properties["tlsPassthroughListeners"]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/controller/tenantgateway/crdcel_test.go` at line 352, Update the
test constant gatewayAPIListenerCap to reuse the maxGatewayListeners constant
used by renderGateway in reconciler.go instead of defining a separate value of
64, so the test remains aligned with the production listener limit.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@internal/controller/tenantgateway/crdcel_test.go`:
- Line 352: Update the test constant gatewayAPIListenerCap to reuse the
maxGatewayListeners constant used by renderGateway in reconciler.go instead of
defining a separate value of 64, so the test remains aligned with the production
listener limit.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 4f93ae2c-6ccf-4a9c-b9a8-448693169592

📥 Commits

Reviewing files that changed from the base of the PR and between 3958dd1 and 78f8d7e.

📒 Files selected for processing (5)

internal/controller/tenantgateway/crdcel_test.go
internal/controller/tenantgateway/reconciler.go
internal/controller/tenantgateway/reconciler_test.go
internal/controller/tenantgateway/renderers.go
packages/extra/gateway/README.md

🚧 Files skipped from review as they are similar to previous changes (2)

internal/controller/tenantgateway/renderers.go
packages/extra/gateway/README.md

Aleksei Sviridkin (lexfrei) · 2026-07-27T21:22:29Z

myasnikovdaniil You are right on both, and the reproduction saved me finding it.

B1 went the second way you offered, 9d2356716, and came out wider than either option. The collision does not need a TLSRoute at all, so filtering on route kind would have left half of it open. Detail in the thread on tenantgateway_types.go. Since it is fixed rather than documented there is nothing left to track.

B2 is closed. There is a tlsRouteAttached helper and six tests building real TLSRoutes and HTTPRoutes against passthrough listeners. I checked each one is load-bearing by removing a single clause of the rule at a time and seeing which test reddens.

Action item 2 is in 1975bdd7f, with the version lookup routed through one helper that fails on absence.

Action item 3 I am keeping. gofmt -l flags renderers.go as it stands on main, and that realignment is what gofmt emits, so reverting it puts an unformatted file back on the branch.

Action item 1 gets its own PR. It changes a VAP that gates every route in the cluster, and your scope-drift point is the reason not to fold it in here.

Going the other way, "the Gateway API rejects it wholesale" was wrong in this PR, in a few comments and in the reservedGatewayPorts rationale. The uniqueness CEL keys on (port, protocol, hostname), so an HTTPS/TLS pair on 443 and an HTTP/TLS pair on 80 are both admitted, and the conflict shows up as Conflicted on the listeners. The guards stay, a Conflicted listener is no better than a refused one. Fixed in 78f8d7e4c.

The gate stays red until an API owner approves the head commit.

coderabbitai

🧹 Nitpick comments (1)

internal/controller/tenantgateway/crdcel_test.go (1)
330-330: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Nit: local field shadows the imported field package.

Harmless today (neither function uses field.NewPath), but a future assertion needing field.NewPath here would fail to compile confusingly. Consider prop or listenersSchema.

Also applies to: 353-353
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/controller/tenantgateway/crdcel_test.go` at line 330, Rename the
local field variable in the affected test cases around v1alpha1SpecSchema,
including the occurrence near the additional referenced line, to avoid shadowing
the imported field package. Use a descriptive name such as listenersSchema or
prop and update all references within those test scopes.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@internal/controller/tenantgateway/crdcel_test.go`:
- Line 330: Rename the local field variable in the affected test cases around
v1alpha1SpecSchema, including the occurrence near the additional referenced
line, to avoid shadowing the imported field package. Use a descriptive name such
as listenersSchema or prop and update all references within those test scopes.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 862767f8-9c7e-4055-8f3c-49c13e7f9cf3

📥 Commits

Reviewing files that changed from the base of the PR and between 78f8d7e and a8a1ae2.

📒 Files selected for processing (8)

api/gateway/v1alpha1/tenantgateway_types.go
internal/controller/tenantgateway/conflict.go
internal/controller/tenantgateway/crdcel_test.go
internal/controller/tenantgateway/reconciler.go
internal/controller/tenantgateway/reconciler_test.go
internal/controller/tenantgateway/renderers.go
packages/extra/gateway/README.md
packages/system/cozystack-controller/definitions/gateway.cozystack.io_tenantgateways.yaml

🚧 Files skipped from review as they are similar to previous changes (6)

internal/controller/tenantgateway/renderers.go
packages/extra/gateway/README.md
packages/system/cozystack-controller/definitions/gateway.cozystack.io_tenantgateways.yaml
api/gateway/v1alpha1/tenantgateway_types.go
internal/controller/tenantgateway/reconciler.go
internal/controller/tenantgateway/reconciler_test.go

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

internal/controller/tenantgateway/renderers.go (1)
356-371: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Comment sits above the wrong check.

Lines 356-368 explain SNI-indistinguishable hostnames across ports — that's the rationale for the overlap loop at Line 376, not for the apex-containment check at Line 369, which has its own unrelated (VAP) justification in the error string. Moving it down to the overlap block would keep each rationale next to the code it defends.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/controller/tenantgateway/renderers.go` around lines 356 - 371, The
explanatory comment about listeners sharing hostnames across ports is positioned
above the unrelated hostname apex validation. Move that comment so it
immediately precedes the overlap-detection loop or check that rejects duplicate
hostnames across different ports, while keeping the existing VAP-related
rationale adjacent to the hostnameWithinApex validation.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@api/gateway/v1alpha1/tenantgateway_types.go`:
- Around line 301-315: The validation markers on TLSPassthroughServices must
preserve updates to existing invalid values through ratcheting rules. Replace
the direct MaxItems, item MaxLength, item Pattern, and set validation with CEL
ratcheting expressions that allow self == oldSelf while enforcing the new
constraints for changed values, preserving duplicate and legacy entries on
unrelated updates.

---

Nitpick comments:
In `@internal/controller/tenantgateway/renderers.go`:
- Around line 356-371: The explanatory comment about listeners sharing hostnames
across ports is positioned above the unrelated hostname apex validation. Move
that comment so it immediately precedes the overlap-detection loop or check that
rejects duplicate hostnames across different ports, while keeping the existing
VAP-related rationale adjacent to the hostnameWithinApex validation.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 9f046279-bc6e-4469-a718-4a14d387d3fa

📥 Commits

Reviewing files that changed from the base of the PR and between a8a1ae2 and a467c51.

📒 Files selected for processing (8)

api/gateway/v1alpha1/tenantgateway_types.go
internal/controller/tenantgateway/conflict.go
internal/controller/tenantgateway/crdcel_test.go
internal/controller/tenantgateway/reconciler.go
internal/controller/tenantgateway/reconciler_test.go
internal/controller/tenantgateway/renderers.go
packages/extra/gateway/README.md
packages/system/cozystack-controller/definitions/gateway.cozystack.io_tenantgateways.yaml

🚧 Files skipped from review as they are similar to previous changes (6)

internal/controller/tenantgateway/crdcel_test.go
internal/controller/tenantgateway/conflict.go
packages/system/cozystack-controller/definitions/gateway.cozystack.io_tenantgateways.yaml
internal/controller/tenantgateway/reconciler.go
packages/extra/gateway/README.md
internal/controller/tenantgateway/reconciler_test.go

Andrei Kvapil (kvaps)

Verdict

NOT LGTM — one blocking item, and it is not in the code. The design is settled, the implementation matches it, and everything the previous two rounds raised is closed at this head. What is missing is the exit gate the accepted design wrote for this exact phase.

Reviewed at head 20abf8768, diffed against merge-base d3559d48e. A note on the review state before anything else: the standing CHANGES_REQUESTED is IvanHunters' from 2026-09-08 05:35Z and this head was pushed 2026-09-08 21:52Z, so the decision field predates the work that answers it. Its MAJOR — the wildcard listener name the Gateway CRD refuses — is addressed here.

Why this exists, and whether the mechanism fits

Taking the design question first, because that is the one an API owner is being asked to sign.

The problem is real and its shape is not this PR's to invent. design-proposals/external-database-exposure in cozystack/community is Accepted, revised 2026-09-03, and it specifies this field. The Go block in the proposal carries the same field names, the same types, Hostname required rather than optional, and the same four list-level markers — +listType=map, +listMapKey=name, MaxItems=62 — that this PR ships. So the API surface is not a new proposal arriving inside an implementation PR; it is the accepted one, and the earlier #2815 sketch with an optional defaulted hostname is superseded rather than departed from. The two passthrough fields staying independent is a recorded decision too: the proposal states that tlsPassthroughServices is too weak for this design and that neither field replaces the other.

The mechanism is proportionate. One mode: Passthrough listener per entry, SNI-matched, on the engine's native port, consuming O(engine types) of the 64-listener budget rather than O(instances). No new CRD, no new controller, no new cross-cutting abstraction — a field on an object that already exists, plus an upstream TLSRoute with no wrapper around it. For a feature whose entire point is that the Gateway must not hold the key, not terminating is the smallest thing that can work.

The breaking half is inside the approved scope as well, which is worth stating because the diff makes it look like scope creep. Lines 183 and 222 of the proposal describe the terminate-listener withdrawal explicitly, including that it is "a behaviour change that reaches clusters which never declare the new field at all".

The blocking item

The proposal's Rollout section gives Phase 1 an exit gate:

Gate: on the deployed Cilium (1.19.x), stand up a mode: Passthrough listener on a native database port (for example 5432) with a per-engine subdomain hostname (*.postgres.<apex>) alongside the Gateway's terminate listeners, and confirm it SNI-routes to the right backend with exactly one Envoy filter chain.

That has not been done, and nothing in the PR can do it. The new chainsaw phases settle the object-level facts — the listener reaches Programmed, the withdrawal happens on demand, the route conditions read correctly — and the third phase reads the per-listener readiness flag, which is genuinely the part no fake client could answer. None of that is a handshake. No test in this change opens a TLS connection carrying a passthrough SNI and confirms which backend answers it, or how many filter chains carry the name.

The whole design rests on one datapath fact: that Cilium 1.19.5 folds the Gateway into a single Envoy listener whose filter chains match on SNI without the port. Three load-bearing claims descend from it — the certMode restriction, the README's statement that the declared port is not an access boundary, and the firewall advice in the field doc.

To be fair to the work: I had the reasoning checked independently against the v1.19.5 source rather than taking the PR's word for it, and all of it holds — the single listener (operator/pkg/model/translation/envoy_listener.go:207-239), toFilterChainMatch carrying no destination port (:589-600, shared by both the terminate and passthrough chain builders), the skip for a route with no backends (:408-411), and ComputeHosts substituting the listener hostname for a route that declares none (operator/pkg/model/helpers.go:39-44). So this is not a doubt about the reasoning. What is left is the empirical half — the half the accepted design chose to gate on, presumably because translation, xDS and the datapath have more steps in them than the translator source shows.

It matters more here than it would for an inert field, because the withdrawal reaches clusters that never touch the new field. Closing it is minutes on a disposable cluster: one entry, one TLSRoute, openssl s_client -connect <gw-ip>:443 -servername <hostname>, and a look at the Envoy config dump for the chain count. Given this is the breaking half of a breaking change, and given the gate is one you wrote, I would rather have it before merge than after.

A second one, and the direction of the change is right

Making a failed route-status write fail the reconcile is the correct instinct — logging it away was worse. But the new path does not tell a retryable write apart from one that can never succeed, and it sits ahead of two steps that do not depend on it.

updateRouteStatuses and reconcileWholeApexRouteStatuses now collect their write errors and return them (conflict.go:718, :730, :733), and runReconcileSteps returns on both (reconciler.go:667-672) — where the merge base logged and carried on (git show d3559d48e:internal/controller/tenantgateway/conflict.go, the logger.Error calls at :141 and :153, function returning nil). The two steps that never run after that return are reconcileHTTPToHTTPSRedirect (reconciler.go:672) and reconcileStatus (:677).

For a transient conflict that is fine — one failed pass, a requeue, done. The case that does not resolve is the ceiling: RouteStatus.Parents is MaxItems=32 and parentRefs is MaxItems=32 (gateway-api v1.4.1, apis/v1/shared_types.go:597 and :245), and both this controller and Cilium write one entry per parentRef. A single route carrying seventeen or more parentRefs to the shared Gateway therefore produces a status object the apiserver rejects as Invalid, every pass, forever. The Gateway and the certificates still reconcile — those steps run before the return — but the TenantGateway is pinned at Ready=False, and on a fresh install the http-to-https redirect route is never created at all, which is the one object that guard exists to provide.

The Gateway is shared across the inheriting subtree and the attached cozy-* namespaces, so the route that does this need not belong to whoever notices. Two small changes cover it: treat IsConflict/IsNotFound on a route status as skip-and-requeue rather than fatal, and move the status pass after reconcileHTTPToHTTPSRedirect so a status problem cannot withhold the redirect. Batching a route's refs into one read-modify-write would also cut the conflict rate, since updateRouteParentStatus (conflict.go:915-942) currently re-reads a cached object once per attaching parentRef. Unit tests cannot see any of this — the fake client has no cache lag and no second writer.

What the withdrawal actually changes

Two facts bound this, and the description does not lead with either.

gateway.enabled is false by default (packages/core/platform/values.yaml:479), and all three platform TLSRoutes render only under gateway-enabled=true with the name present in expose-services. So the withdrawal reaches clusters that opted into the Gateway integration, not the installed base.

And on the path those clusters came from, api.<apex> was already passed through. packages/system/cozystack-api/templates/api-ingress.yaml:10-12 sets ssl-passthrough: "true" with backend-protocol: HTTPS to Service kubernetes:443, rendered exactly when gateway-enabled != true, and packages/system/ingress-nginx/values.yaml:4 turns the feature on. The two KubeVirt proxies are the same shape. So this change does not invent passthrough for these names; it makes the Gateway path agree with the ingress-nginx path the platform has always shipped. Meanwhile packages/extra/info/templates/kubeconfig.yaml:21 embeds certificate-authority-data from kube-root-ca, so the kubeconfig Cozystack hands a tenant already validates against the cluster CA — passthrough is what that client expects. Before this change, which of the two port-443 filter chains answered was undetermined and unobservable, since the pinned Cilium has no Conflicted condition to read. Making it deterministic is the fix, not the risk. That framing belongs near the top of the description; right now a reader meets the shed before they meet the reason it is an improvement.

A precondition the release note should carry

Once the ACME certificate is gone, the only certificate on api.<apex> is the kube-apiserver's serving certificate, so it must carry api.<apex> in its certSANs. On the ingress-nginx path that was already true, so most clusters will satisfy it — but a cluster that moved to the Gateway path and has been served by the ACME certificate since will not, and it loses api.<apex> on upgrade. Nothing in this repository states the requirement; the e2e clusters do not meet it either (hack/e2e-container-up.sh:263 and hack/e2e-prepare-cluster.bats:239 list IP SANs only), which is a second reason the suite cannot reach the datapath question above.

Please add one line to the release note and to the README's upgrade section: before upgrading, confirm the management apiserver's serving certificate carries api.<apex>; if it does not, add the SAN, or take the migration path that moves the consumer to another hostname, which the README already names as the one that costs nothing.

While you are in that section — the second migration path reads as more than it delivers. "Switching certMode to dns01 or existingSecret keeps the pre-upgrade shape wholesale" is literally true, and the pre-upgrade shape is the ambiguity: rendersTLSPassthrough (reconciler.go:94-96) excludes edge alone, and both passthrough loops sit outside the certMode switch that ends at reconciler.go:1585, so under dns01 the tls-<svc> listeners still stand beside a wildcard terminate listener covering the same names. Someone picking that path to get termination back gets the undetermined race back instead. Worth saying so where the path is offered, since the README explains elsewhere that the shape is unobservable.

No migration ships with this change and I do not think one is needed given the above — but say that in the release note rather than leaving it inferred.

Notes, none of them blocking

The certMode wedge is real, already documented, and I agree with IvanHunters that it should not block. A stored tlsPassthroughListeners refuses the write that would move certMode, because the CEL rules sit on the spec node and any change there un-suppresses them, and packages/apps/tenant/templates/gateway.yaml:46,52 sets retries: -1, so the gateway release retries the refusal indefinitely. What keeps it out of blocker territory is that the CEL message names the field: the exit is one kubectl edit. The proposal already carries it as an open question under who declares the entry, which is the right place. Worth revisiting when the chart grows a value for the field — an admission rule that can reject a write the platform makes automatically, and that the tenant cannot influence, is the shape to avoid.

tlsRouteForwards (backends.go:54-80) reads Service existence and stops there, which is the right call and reads as deliberate. Worth noting the consequence for the two KubeVirt names: their Services are created by virt-operator and cdi-operator rather than by this tree, so on a cluster where one has not appeared yet the terminate listener simply stays. That is the safe direction.

The new chainsaw phases hand-roll poll() loops with sleep 3 where docs/agents/e2e-testing.md §2 and checklist items 2–3 call for a Chainsaw assert, which already polls to its own timeout and reports a structured diff on failure. The trap cleanup EXIT is fine — self-contained inside one script step, which checklist item 4 permits explicitly. Either move the presence and absence checks onto assert/error operations, or leave a comment saying why the imperative form is needed; the reasoning about not reading a failed kubectl as "absent" is good and belongs beside the code that needs it.

Two tests assert less than the text around them claims, and both are one assertion away from closing.

TestListTypeSetWouldNotRefuseAStoredDuplicate (crdcel_test.go:648-690) is cited for the decision not to put +listType=set on tlsPassthroughServices, and the argument for that decision is the interesting part: that the apiserver's list-type check is gated on the stored object passing it, at whole-object granularity, so one stored duplicate would switch the check off for every list on the TenantGateway at once. The test shows only that the marker refuses a duplicate over a clean stored object, and that a stored duplicate does not block an unrelated write. It never demonstrates the spillover, which is the whole point. I had it checked against the shipped schema and the spillover is real — a stored duplicate in tlsPassthroughServices lets a duplicate name key through in the listType=map list on the same write, guard at k8s.io/[email protected]/pkg/registry/customresource/strategy.go:299-302. The object already carries a listType=map list, so pinning it is one more assertion in a test that is already standing there.

TestCRDPassesInstallTimeValidation (crdcel_test.go:476-497) has no negative control, so it passes whether or not the bounds it exists to justify are doing anything. The claim itself holds — removing MaxLength from apex puts the estimate 3.9x over budget. But the field comment attributes the need to apex, and that undersells it: removing MaxItems from tlsPassthroughServices costs 6.5x and removing it from tlsPassthroughListeners costs over 100x plus the schema's total limit. Worth correcting the comment, since the next person to touch a bound will read it.

The new Service and ReferenceGrant watches map correctly and I found no staleness path — both branches match what Cilium resolves, and the ReferenceGrant branch deliberately over-enqueues rather than under-enqueues. One efficiency note: mapper.go:157-166 lists all TLSRoutes and all TenantGateways on every accepted event, and lists TenantGatewayList even when there are no TLSRoutes at all. serviceExistenceChanged() (mapper.go:126-130) overrides only UpdateFunc, so Creates pass — and controller-runtime replays the whole store as Creates when a handler registers on a running informer, which means every controller start and every leader change runs the mapper once per Service with two full lists each. It is all served from the manager cache rather than the apiserver, and it is tidier than what WorkloadMonitorReconciler already does, so this is not a blocker. An early return when len(routes.Items) == 0, and moving the TenantGateway list inside the match branch, costs nothing.

conflict.go:819 writes the literal "NoMatchingParent" where every other site uses the upstream constant. gatewayv1.RouteReasonNoMatchingParent exists — gateway-api v1.4.1, apis/v1/shared_types.go:374.

Not a finding, but worth recording since this PR touches the file: the two rules added to packages/system/cozystack-controller/templates/rbac.yaml grant nothing today, because the same ClusterRole already ends in apiGroups: ['*'], resources: ['*'], verbs: ["get","list","watch"] (:71-73). Naming the reads is the right thing to do — it is what makes narrowing that catch-all possible later, and rbac_test.go pins it. Narrowing it is separate work and worth scheduling: a cluster-wide read of everything, alongside cluster-wide write on Secrets, is a lot of standing authority for a platform controller in a multi-tenant product.

On IvanHunters' second MINOR: parentRefAttachesTo (reconciler.go:1019-1036) still does not read ref.Port, so the asymmetry he named is still there in that function. The consequence he predicted is not, because judgeHTTPRouteClaim (conflict.go:278-305) now judges the port and refuses anything that is neither the plain-HTTP nor the HTTPS port before it can reach dynHostnames. So the symptom is closed and the trap is not: a helper called parentRefAttachesTo that returns true for a route pinned to a port the Gateway does not publish there will mislead whoever calls it next. Worth a comment on the function at minimum.

Two things I could not settle myself and would rather name than assert. First, the withdrawal is refused for a TLSRoute reaching a native-port listener from a namespace that listener does not admit, so this controller keeps the terminate listener — but if Cilium's TLSRoute ingestion does not consult allowedRoutes and the permissive port-443 listener answering the same hostname satisfies its namespace check, it would build the passthrough chain regardless, and two chains carrying one server name in one Envoy listener is not a graceful failure. It needs a hand-written entry to reach, so nothing in a shipped configuration does it, but it is the same experiment as the Phase 1 gate and worth adding to that run. Second, updateRouteParentStatus merges on (ControllerName, ParentRef) while Cilium's merge keys on the ParentRef alone, and this controller writes ObservedGeneration: 0 where Cilium writes the route's generation — if that means the two rewrite each other's entry indefinitely it would make the conflicts in the section above routine rather than rare. Both predate this change; both are worth ten minutes on the cluster you use for the gate.

One dependency worth a line somewhere, because the rollback story rests on it: the "a re-created Certificate adopts a still-valid Secret rather than ordering" argument holds only while cert-manager runs with enableCertificateOwnerRef: false. It does — packages/system/cert-manager/charts/cert-manager/values.yaml:268, with no override anywhere under packages/ — but that is a chart default this design now depends on, and flipping it turns every backend flap into a fresh order against Let's Encrypt's duplicate-certificate limit. Say so where the rollback is described.

Things I checked and found clean, so nobody re-derives them: the wildcard rename preserves every previously-admissible listener and Certificate name byte for byte (the hash covers the whole hostname including the asterisk, and TestPerListenerName_KeepsEveryNameThatWasAlreadyAdmissible pins the old values rather than recomputing them); the generated CRD is byte-identical to what the pinned controller-gen produces from the markers; packages/core/platform/values.yaml really does change in comments only; and packages/extra/gateway is not a vendored chart, so the patches/*.diff rule does not apply here — cozyvalues-gen regenerates only the ## Parameters table, and all six README hunks sit outside it.

On the description, as a request rather than a finding: +11955/-2088 is misleading in a way that costs reviewers. The production surface is roughly 2,300 lines across six files; reconciler_test.go alone accounts for 9,611 of the rest. Saying that in the first paragraph would set a truer expectation. Relatedly, the body is long enough that its own conclusions are hard to find — #3842 already tracks collapsing the restated Cilium reasoning into one canonical block, and the same treatment would help the body itself. A reader who has to reach the fortieth paragraph to learn that the red check is deliberate is a reader who stopped before forty.

Process

The red Require API owner review for sizeable API changes is the gate working as designed, and no code change clears it: it needs an approving review from Timofei Larkin (@lllamnyp) or Andrei Kvapil (@kvaps) on the current head, and any later push invalidates it. Everything else on this head is green, including the full E2E lane at 2h2m — hack/select-e2e.sh:109 escalates api/, cmd/, internal/ and packages/core/ to the whole suite, so the gateway suite with the new withdrawal phases did run.

Close the Phase 1 gate, add the certSANs line, and correct the second migration path, and I have nothing else.

Andrei Kvapil (kvaps) · 2026-09-09T21:12:02Z

Two more from a second pass over reconciler.go, both non-blocking and both smaller than anything in the review above. Neither changes the verdict; I am adding them because they are cheap to fix while you are in this code and expensive to rediscover later.

The first is a pre-existing false conflict that this PR builds the machinery to fix and then declines to run. recount (reconciler.go:325-337) re-ranks ownership of a hostname over the plain routes plus whichever field it is given, and it is gated on if len(refused) > 0 (:341-344) — so an ordinary race with no refusal keeps the original ranking. That original ranking is wrong for one shipped case: cert-manager's HTTP-01 solver route is published in the Gateway's namespace with sectionName: http (renderers.go:823-825), so it is classified as served-plain and enters the race; rankRouteRefs orders by namespace, and tenant-a sorts before tenant-a-child. A child tenant publishing a hostname therefore loses the race to the solver for the duration of every ACME challenge and reads Accepted=False with HostnameConflict, while its terminate listener is rendered and Cilium is serving it happily. Same outcome on the merge base, so this is not a regression — but the fix is now one condition away, since recount already does exactly the right thing. Either drop the len(refused) > 0 gate for this case or say in a comment why the solver is allowed to win.

The second is new and small: the withdrawal reads namespace state one pass before it establishes it. collectHostnameClaims runs at reconciler.go:187 and ensureNamespaceLabels at :652, so on the first reconcile after an entry is added to attachedNamespaces a route in that namespace is judged attachable while the Gateway's own selector does not yet admit it. It converges on the next pass, so the worst case is one pass of a status condition that is briefly wrong rather than anything served incorrectly — but a withdrawal decision taken from state the same function has not yet written is worth either reordering or naming in a comment.

While I am here, one correction to sourcing rather than to substance. The three Cilium behaviours I cited in the review — the single Envoy listener, toFilterChainMatch carrying no destination port, and the skip for a route with no backends — were read against v1.19.5 specifically, which is what packages/system/cilium/charts/cilium/Chart.yaml pins. The two I flagged as unsettled at the end of that review (toTLSRoutes not consulting allowedRoutes, and mergeStatusConditions keying on the parentRef alone) were read against the v1.19 branch rather than that tag, which is exactly why I named them as things to confirm on a cluster rather than asserting them.

Andrei Kvapil (kvaps)

Approving, and being explicit about what that covers

Approving as API owner on 20abf8768, which clears Require API owner review for sizeable API changes. The release is what is driving the timing, and the remaining items below are being carried as follow-ups rather than treated as closed. I would rather write that down than let the approval imply they went away.

On the API surface itself I have no reservations, and did not have any in the previous review. design-proposals/external-database-exposure in cozystack/community is Accepted, revised 2026-09-03, and the Go block there matches this PR field for field — same names, same types, Hostname required, and the same four list-level markers (+listType=map, +listMapKey=name, MaxItems=62). The two passthrough fields staying independent is a recorded decision in that document too. So this is the approved shape arriving as an implementation, not a new proposal arriving inside one, and v1alpha1 plus no chart value writing the field keeps the cost of being wrong low.

On the Phase 1 exit gate: I understand from the maintainer that you did exercise this on your own cluster. That closes it on the merits and I withdraw it as a blocking item. The one thing I would still ask for is that the result goes into the thread or the body — what you stood up, what SNI you dialled, and the filter-chain count — because the design named it as the gate for this phase, and right now nothing in the PR records that it was met. The next person to touch the flat-hostname phase has to re-run the same check against a different shape, and starting from your notes is much cheaper than starting from the design text.

Carried as follow-ups

Listed in the order I would do them, not in the order they appear in the review above.

1. The fatal route-status write is a regression this PR introduces, and it is reachable by a tenant. updateRouteStatuses and reconcileWholeApexRouteStatuses now return their write errors (conflict.go:718, :730, :733) and runReconcileSteps returns on them (reconciler.go:667-672), where the merge base logged and carried on. Failing rather than swallowing is the right instinct; the gap is that a retryable conflict and a permanently invalid write are treated alike. RouteStatus.Parents is MaxItems=32 and both this controller and Cilium write one entry per parentRef, so a single route with seventeen or more parentRefs to the shared Gateway makes the status permanently unwritable, and from then on reconcileHTTPToHTTPSRedirect and reconcileStatus never run — on a fresh install the http-to-https redirect is then never created at all. The Gateway is shared across the inheriting subtree and the attached cozy-* namespaces, so whoever triggers it need not be whoever notices. Treating IsConflict/IsNotFound as skip-and-requeue and moving the status pass after the redirect covers it. This is the one I would not let sit long.

2. The certSANs precondition belongs in the release note. After the shed, the only certificate on api.<apex> is the kube-apiserver's serving certificate, so it must carry api.<apex> in its SANs. The ingress-nginx path already passed through, so most clusters satisfy it — but a cluster that moved to the Gateway path and has been served by the ACME certificate since does not, and loses api.<apex> on upgrade. Nothing in this repository states the requirement today.

3. The second migration path in the README promises more than it delivers. "Switching certMode to dns01 or existingSecret keeps the pre-upgrade shape wholesale" is literally true, and the pre-upgrade shape is the ambiguity: rendersTLSPassthrough excludes edge alone and both passthrough loops sit outside the certMode switch, so under dns01 the tls-<svc> listeners still stand beside a wildcard terminate listener covering the same names. Someone picking that path to get termination back gets the undetermined race back instead.

4. The two small ones from my follow-up comment — the recount gate that declines to fix the ACME-solver false conflict, and the withdrawal reading namespace state a pass before ensureNamespaceLabels writes it.

5. The two Cilium-source items I flagged as unsettled rather than asserted: whether toTLSRoutes consults allowedRoutes during ingestion, and whether mergeStatusConditions keying on the parentRef alone produces a status ping-pong with this controller. Both are cheap to answer on the cluster you already have standing.

Not merging

I am clearing the gate, not pressing the button — the merge decision is not mine to take here, and E2E Tests should be looked at on the current head before anyone does.

A tlsPassthroughServices entry publishes a hostname on port 443, which suits a service whose clients speak HTTP and mis-serves one whose clients expect a database on its own port. The new field takes a name, a native port and the SNI hostname to answer on it, so an engine can be reached where its drivers already look. The cross-field rules are enforced at admission as well as in the controller. renderGateway is the first thing that renders, so a spec the apiserver accepts and the renderer refuses aborts everything behind it, and one mistyped hostname would freeze the Gateway and every route status where they stand. The controller keeps its own copy because it and the CRD roll out separately. Rules with no admission form, and why each one has none, are named on the type. tlsPassthroughServices and apex gain bounds at the same time, for the CEL cost estimator rather than for their own sake: the new rules concatenate apex and scan the services list, and an unbounded string or list makes the install-time estimate exceed the per-CRD budget and the apiserver refuse the whole definition. Every bound is a kind the apiserver ratchets, so a stored value that violates one keeps accepting writes that leave it alone. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>

Andrei Kvapil (kvaps) · 2026-09-10T11:24:24Z

Recording the follow-ups from the approval above as issues, so that "carried as a follow-up" means something a month from now rather than a sentence nobody can act on.

tenant Gateway: a failed route-status write can freeze the shared Gateway for every tenant on it #4195 — a failed route-status write can freeze the shared Gateway for every tenant on it. This is the regression this PR introduces, and it is the one I would take first.
gateway: document the api.<apex> certSANs precondition for the TLS-passthrough shed #4196 — document the api.<apex> certSANs precondition for the shed, in the release note and in the package README.
gateway: the dns01 migration path in the README restores the ambiguity, not termination #4197 — the dns01 migration path in the README restores the ambiguity rather than termination.
tenant Gateway: two small ordering and gating items in the reconciler #4198 — the recount gate that declines to fix the ACME-solver false conflict, and the withdrawal reading namespace state a pass before ensureNamespaceLabels writes it.
gateway: check this controller's route-status writer against a live Cilium #4199 — confirm the two Cilium v1.19.5 assumptions I flagged as unsettled rather than asserted; the Phase 1 gate run is the natural place to answer both.

For the record, because the shape of this thread will otherwise be hard to read later: this PR carried a NOT LGTM from me yesterday, and the approval above is not that verdict being withdrawn on the merits. Every item in it is still open — none was answered by a code change, and the head has not moved since. What changed is the release timing, and the judgement that the API surface is the approved design and that the remaining items are better carried as tracked issues than as a held PR. The Phase 1 gate is the one exception: I understand from the maintainer that it was exercised on your own cluster, which closes it on the merits, and the only thing still missing there is the record of it.

If any of the five turns out to be wrong on closer reading, say so on its issue and I will close it — I would rather have five issues where one is mistaken than four where the fifth was forgotten.

Andrei Kvapil (kvaps)

Re-approving on `8af986398`

The branch was force-pushed while I was writing the previous approval, so that one sits on 20abf8768 and the gate correctly does not count it. Re-approving here, and the head moving turned out to be worth the round trip: this is not a rebase, it answers the review.

Closed by this push, verified in the delta rather than taken on trust:

The route-status regression is fixed the way I hoped and then some. retryableRouteWrite (conflict.go) classifies IsConflict and IsNotFound as survivable and everything else — an Invalid the apiserver will refuse on every pass, a Forbidden — as fatal, and it recurses into a joined error so one fatal leaf fails the whole batch instead of being retried into silence. routeStatusWriteError tags the failure so Reconcile can tell it from a desired-state failure without matching on message text, and a retryable one now requeues rather than pinning Ready=False on the tenant. reconcileHTTPToHTTPSRedirect moved ahead of the status pass and reconcileStatus now runs before the collected errors are returned, so a route status the apiserver refuses can no longer withhold the redirect or freeze the TenantGateway's own status. That was my one blocking-grade item and it is gone. Closing #4195.

The api.<apex> certSANs precondition is in the README now, ahead of the three migration paths and phrased as a pre-upgrade check. Closing #4196.

conflict.go uses gatewayv1.RouteReasonNoMatchingParent instead of the string literal, and parentRefAttachesTo carries the comment explaining why it answers identity alone and leaves the port to the serving verdict. Both were nits and both are done.

Still open, and still carried as follow-ups rather than as blockers:

#4197 — the dns01 migration path still reads "keeps the pre-upgrade shape wholesale" with no note that the pre-upgrade shape is the ambiguity itself.
#4198 — recount is still gated on len(refused) > 0, and collectHostnameClaims still runs at :208 while ensureNamespaceLabels runs at :673.
#4199 — the two Cilium v1.19.5 assumptions I flagged as read against the branch rather than the pinned tag.

Same framing as before, since the record should stay readable: this PR carried a NOT LGTM from me yesterday, and the approval is not that verdict collapsing on the merits. The API surface is the accepted design and was never the question; what moved is that the one item I would have held on is now fixed in the code, the Phase 1 gate is reported as exercised on your own cluster, and the rest is tracked. If any of the three remaining issues is wrong on closer reading, say so on it and I will close it.

Not merging — that is not my button here, and E2E Tests wants looking at on this head first.

Aleksei Sviridkin (lexfrei) · 2026-09-10T11:27:27Z

Closed the gate on a live cluster, Cilium 1.19.5: terminate listeners on 443, a passthrough listener on 5432, one TLSRoute per name, backends presenting their own certificates. Cilium folds the whole Gateway into a single Envoy listener and builds exactly one filter chain per passthrough name, matching on serverNames plus transportProtocol with no destination port, tcp_proxy straight into the backend EDS cluster. openssl s_client against the load balancer address returns the backend certificate for the passthrough name and the gateway certificate for the terminate name, on either port, which is the datapath half of the claim that the declared port is not an access boundary.

Your first caveat does not reproduce. A TLSRoute reaching the native-port listener from a namespace that listener does not admit gets no filter chain at all, and the name goes on answering with the terminate certificate. Pinned to the listener it reads Accepted=False with NotAllowedByListeners; unpinned it reads Accepted=True, which is the status bug you would expect there, but nothing is built either way. The second one I could only half measure: Cilium preserves a foreign controller's entry verbatim across generation bumps, no rewrite war, but this branch's controller is not deployed on that cluster so its own writer against a live Cilium is still untested.

Both route-status changes are in. Retryable and permanent are now separated by error type rather than by message text, a retryable batch requeues without touching Ready, and one permanent leaf still fails its whole batch so an Invalid cannot ride to silence on a conflict's retry. The order needed a test of its own: collecting the errors already delivers the redirect, so the redirect assertions stay green under a reorder and only an explicit ordering assertion catches a later status step that returns on the spot. updateRouteParentStatus still reads per parentRef; batching a route's refs into one read-modify-write is a separate change and I would rather it land alone.

The precondition is in the README paragraph about what the upgrade sheds and in the release note. A cluster that came from ingress-nginx already meets it, since that path passes the same name through to the same Service; a cluster served by the Gateway's ACME certificate since it moved does not necessarily, and loses the name until the SAN is added. The second migration path now says what it costs instead of implying a clean rollback: under dns01 or existingSecret the tls-<svc> listeners still stand beside the wildcard terminate listener covering their names, both port-443 chains carry the same SNI again, and nothing on the objects says which one answers.

The smaller notes are all in. One number worth quoting since it changes which bound is the load-bearing one: removing each in turn, apex MaxLength puts the containment rule 3.9x over budget, tlsPassthroughServices MaxItems puts the collision rule 6.5x over, and tlsPassthroughListeners MaxItems puts the distinct-port rule over 100x over and takes the whole-schema total with it. The list counts are what to defend, not the string length. The chainsaw loops stay imperative, with the reason next to them: the apex every polled hostname is built from is read off the tenant namespace label inside the script, and the cleanup trap has to be step-level, so the work and the teardown of what it creates cannot be split.

Andrei Kvapil (kvaps) · 2026-09-10T11:57:54Z

Gate report is what I was after, thanks.

Checked the rest against the tree at 8af98639. The dns01 path now says the ambiguity comes back with it, packages/extra/gateway/README.md:109, so #4197 is closed.

Still open, both in reconciler.go: recount is gated on len(refused) > 0 at :363, and collectHostnameClaims runs at :208 while ensureNamespaceLabels runs at :673. That is #4198. Neither is urgent and neither blocks anything.

Your first caveat not reproducing is the useful half of #4199: no filter chain built at all, and the name keeps answering with the terminate certificate. I narrowed the issue to the part you could not measure, your own controller writing route status against a live Cilium.

The withdrawal case in the suite pins the rule rather than a neighbour: the TLSRoute it attaches has a backend that resolves, and after it the terminate hostname is gone from spec.listeners and from the per-listener Certificate while tls-api stays, with api-probe moving to NoMatchingListenerHostname.

Approval stands on this head.

Each tlsPassthroughListeners entry renders a mode=Passthrough listener on its own port, answering the entry's hostname by SNI and admitting TLSRoutes from the publishing tenant's namespace alone. It declares no allowedRoutes.kinds: at the pinned Cilium v1.19.5 a listener that spells its kinds out has them applied to every route on the Gateway rather than to itself, so naming TLSRoute here would reject every HTTPRoute on the Gateway. A hostname a passthrough listener answers no longer earns an HTTPS-terminate listener or a Gateway-issued certificate. That is the break, and it is keyed on service rather than on declaration: the same Cilium builds a passthrough filter chain per route out of the backends that survived resolution, so a listener no TLSRoute attaches to, and a route whose backendRefs resolve to nothing, both match no ClientHello and leave the terminate listener holding the name. Eligibility asks what v1.19.5 asks and no more, the reference allowed and the Service present, and fails closed on a read it cannot complete. Services and ReferenceGrants are watched, so a withdrawal deferred on a missing backend lands on the pass that sees it arrive, and the Service watch takes only the creates and deletes, existence being all that is read. Which hostname a route claims is answered the same way. spec.hostnames is optional on a TLSRoute, and ComputeHosts substitutes the hostname of each TLS listener the route's parentRefs select for a route that declares none, so such a route carries the SNI and takes the name over exactly as one that spells it out. A declared hostname broader than the listener's is that substitution from the other side: the route is served on the listener's name and not on the wildcard it wrote, so a claim covering a reserved name counts as a claim on it. The substitution reaches only the listeners that would admit the route. A native-port listener takes the publishing tenant's namespace alone, so Cilium never puts a route from elsewhere on it and never serves that route on its hostname; borrowing the name anyway refuses the route for a namespace, over a hostname its own object does not carry, in the same pass that sheds a terminate listener because that same route is serving a name it borrowed legitimately. A route that names the listener in its sectionName keeps the borrow, since there the refusal is the answer to what it asked for. A route the Gateway renders no listener for hears it from this controller instead of from Cilium alone: the hostname a passthrough listener took, the hostname nothing answers, the sectionName or port naming a listener that answers something else, the sectionName naming no rendered passthrough listener, and the native-port listener that admits the tenant alone, each with its own Gateway API reason and a message naming the hostnames that lost. An HTTPRoute pinning a rendered passthrough listener by sectionName is refused there too, since such a listener forwards a stream it never terminates and takes TLSRoute alone by protocol. Nothing else tells that route: the port-443 listeners name HTTPRoute in their kinds to keep every port-443 set uniform, which the Gateway reports back as InvalidRouteKinds rather than as a grant, and the pin reads a listener's kinds against every route on the Gateway, so it reports the route accepted. The field is confined to certMode http01, stated as the modes that can carry it so one added later is refused until someone rules on it. The wildcard modes cover the whole apex with one terminate listener the pinned Cilium cannot keep apart from a passthrough listener beneath it, and edge terminates TLS at the class provider and renders no TLS listener at all. An HTTPRoute claiming a wildcard is refused under http01 before the race and the overlap are read. The HTTP-01 challenge issues no wildcard certificate, so the terminate listener rendered for the claim could never carry the name, and the listener name built from the hostname's first label carried the asterisk, which the Gateway CRD admits in a hostname and refuses in a name, so the apiserver refused the whole Gateway write and every other listener on it froze where it stood. The renderer now writes that label as "wildcard", so every hostname the CRD admits yields a name it admits, and no name that was already admissible moves. That refusal is a statement about the terminate listener, so it is judged where that listener is decided rather than ahead of everything else. The port-80 listener carries no hostname and serves a wildcard claim as readily as any other, so a route that selects it, by pinning the port or by naming the listener, never needed the listener HTTP-01 cannot supply and is served rather than refused. Judged first, the refusal reached that route too and reported it unserved while Cilium was serving it in plaintext. A parentRef port on an HTTPRoute is read the way servableOn reads it for a TLSRoute. The terminate listener sits on 443, so a route pinning another port attaches to no listener the render would give it, and it used to earn a listener and an ACME certificate for a route that attached to nothing. Port 80 is the plain-HTTP listener, which admits the publishing tenant alone: a route from there pinning it is served plain and earns neither a listener nor a refusal. A route status write that fails is told apart by kind. A conflict or a route that vanished between the list and the write is a race between the two status writers this Gateway has: the reconcile requeues and retries the write without pinning Ready=False on the tenant, and the status pass runs after the http-to-https redirect, so such a race cannot withhold the one object that guard exists to provide. A write the apiserver will refuse on every pass still fails the reconcile, after every other route has been written, because the condition is the only object saying why a hostname is not served, and a pass that reported success over a lost write left the route reading Accepted=True until the resync. The retryable half is logged on the way out. Declining to pin Ready=False leaves the requeue as the only thing acting on it, and the requeue says nothing: the tenant goes on reading Ready, no event is recorded, and returning no error opts out of the backoff that would widen the interval, so a write failing on every pass retries at a fixed period indefinitely with nothing anywhere to show for it. A TLSRoute claiming a hostname broader than the listener's is pulled into the eligibility question for each name it covers, because the pinned Cilium serves it on the listener's own name. That is right only where the listener answering a name is named exactly that name. Under a wildcard entry the substitution hands such a route the wildcard, so its chain carries "*.db.<apex>" while the terminate chain carries the exact name beneath, and Envoy matches an exact server name ahead of a wildcard, so nothing collides; pulling the route in there withdrew a served endpoint and handed its name to the database behind the passthrough listener. The pull-in is now keyed on an exact listener match. Every condition this controller writes on an HTTPRoute, and the one it writes on a TLSRoute declaring no hostnames, is written under http01, and the whole-apex modes collect no claims, so a switch of certMode left those verdicts standing over listeners the new mode does not render. The whole-apex pass now drops this controller's entry from HTTPRoutes and from hostname-less TLSRoutes; the edge refusal stays confined to routes that declare hostnames, since a refusal written on a lent hostname would have no path back. Within http01 the same verdicts outlive their listeners for a second reason, and no mode change is needed to reach it. The status pass walks the routes that attached rather than the ones that claimed a hostname, and retracts its entry from the difference, because a claim set is not a route's own property once a hostname can be borrowed: a TLSRoute declaring none takes the hostname of every passthrough listener its parentRef selects, so removing that listener empties the claim set while the route stands unchanged, and an HTTPRoute reaches the same state by dropping its hostnames. Judged on claims alone such a route keeps whatever the last pass that did judge it left behind, describing a listener the Gateway no longer has. An HTTPRoute pinned by sectionName to a passthrough listener this Gateway does not render is refused with NoMatchingParent, the reading a TLSRoute in the same shape already got, instead of earning a terminate listener it could never attach to. A route served on the port-80 listener, by parentRef port or by naming it, earns no terminate listener and is refused nothing, but it publishes the name from its namespace all the same, so it stays in the hostname race. Every recount now ranks it with the field it recounts, because a recount that left it out crowned the namespace that had lost to it and dropped a loss the first count recorded: a third route refused in a third namespace, or a passthrough entry declared for the name, flipped the loser to Accepted=True. Selecting the port-80 listener by name is judged as selecting it by port: from a namespace the listener admits the route is served plain, from anywhere else the listener turns it away and the route reads NotAllowedByListeners, and a sectionName and a parentRef port that select different listeners select none and read NoMatchingParent, which is what Gateway API requires when both are given. Before this a route naming the listener from a refused namespace earned a terminate listener and a certificate it could never attach to. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>

Deciding whether a TLSRoute forwards anywhere reads the backend Service and the ReferenceGrant admitting a cross-namespace reference to it, and both are watched rather than fetched on demand. The read-everything rule at the foot of the ClusterRole covers them today, so nothing is broken. Naming them keeps the feature working the day that rule is narrowed, where an informer starting without a grant fails with Forbidden and takes the manager with it. The kubebuilder markers in the package generate nothing: controller-gen runs over ./api/... alone and writes no rbac artifact, so the shipped role is hand-written and only a test connects the two. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>

Three phases on a tenant that owns its Gateway. The shipped passthrough entries render with nothing routed to them and an HTTPRoute on api.<apex> keeps its terminate listener, its certificate and Accepted=True, which is the state a withdrawal keyed on the declaration alone would take away. Attaching a TLSRoute to that listener, pointed at a Service in the tenant's own namespace so it resolves, withdraws all three and moves the route to NoMatchingListenerHostname. A third phase patches in a native-port entry and reads the per-listener readiness flag, which is the claim no test against a fake client can settle. The first two phases are one measurement rather than two: withdrawing on demand is what dates the earlier assertion to settled state instead of to a withdrawal that had not arrived yet. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>

The chart README gains the native-port field, the three limitations it carries and where each comes from, the one that is a phase-1 narrowing rather than a consequence of the Cilium pin, and the three ways forward for anything that depended on a terminate listener the change takes away. The withdrawal is stated as what it is, a change of serving path rather than the removal of something idle, because on the pinned version both filter chains carry the SNI today and nothing on the objects says which one answers. The layers section is brought back in step with the listeners: which label each listener class selects on, that the attach set is the whole inheriting tenant subtree rather than the tenant plus the admin list, and the route conditions an operator reads, including the one that says the sectionName itself names nothing. The platform values comment on gateway.enabled said every exposed service renders an HTTPRoute, which the chart value the withdrawal reads contradicts. It now separates them: the services whose TLS the Gateway terminates get an HTTPRoute, while api, vm-exportproxy and cdi-uploadproxy get a TLSRoute pinned to their passthrough listener, one each from the packages that own those services. The overlap paragraph no longer describes an HTTPRoute wildcard as keeping its terminate listener. Such a claim is refused under HTTP-01, which issues no wildcard certificate, and the paragraph says so and what the render used to do with it. The route-condition list gains that shape and the port shapes an HTTPRoute now reaches, and the per-app naming bullets carry the hash suffix the rendered names have. Two sentences that were false against the code are corrected: a TLSRoute on a wildcard entry itself leaves the names beneath it their terminate listeners, and an unusable apex is refused by the containment rule at admission before the controller's own check is reached. The edge section says what happens to the entries the HTTP-01 pass wrote once the mode moves. The NoMatchingParent sentence now separates the two kinds: an HTTPRoute pinned to an absent passthrough section is refused whatever answers its hostname, while a TLSRoute is judged where a passthrough listener answers it. A sectionName outside the passthrough prefix is written Accepted like any attached route rather than left without a verdict, and the text says so; the port-80 listener's refusal joins the NotAllowedByListeners shapes and the name-and-port disagreement the NoMatchingParent ones. The attachedNamespaces comment described the list as reaching every listener, which the native-port ones do not: they select on kubernetes.io/metadata.name and name the publishing tenant alone, so a namespace listed there cannot attach to a database port. It also credited the wrong label with keeping the list honest; what a listed entry gets is the namespace.cozystack.io/gateway label, and what keeps a tenant from stamping it on itself is that no tenant role carries patch on Namespace. The class reference is brought along. Its certificate-mode table promised one Certificate per published hostname under http01, which a hostname served by a passthrough listener no longer earns, and its edge table said every attached TLSRoute is refused, where only a route declaring hostnames is and the rest have this controller's entry removed instead. The refusal-shape list is corrected against the code, without a count of its own so the next correction does not have to move one. A route the plain-HTTP listener turns away on its namespace selector was listed under NoMatchingListenerHostname as well as under NotAllowedByListeners; the controller reports the second. The port shape standing beside it swept port 80 back in by naming every port but 443, where a route selecting the plain-HTTP listener is answered by the namespace shapes and never reaches the port refusal at all. And a TLSRoute pinned to an absent section is said to carry NoMatchingListenerHostname where no passthrough listener answers its hostname, which holds only while no HTTPRoute claims that hostname: with one there a terminate listener stands on the name, and the route is judged like any other attached route. The wildcard shape gains the same qualifier. A wildcard claim is refused because no terminate listener can carry a certificate HTTP-01 will not issue, so the refusal reaches a route that wanted a terminate listener and not one that asked for the plain-HTTP listener, which carries no hostname and serves the claim there. The Helm merge paragraph named the two-way path as what happens to a custom resource Helm has no Go type for. That path is one of two in createPatch, the version deciding which runs, and the version comes from helm-controller rather than from this tree, so the claim was not checkable where it was written. It now names both and says the outcome it rests on holds either way. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>

Aleksei Sviridkin (lexfrei) · 2026-09-10T16:37:09Z

Head is ae5b626c8 now. I said the refusal shapes were precise and that was too early: two of them refused routes that were being served.

A TLSRoute with no spec.hostnames takes the hostname of every passthrough listener its parentRef selects, and it was taking them from the native-port listeners too. Those admit the publishing tenant's namespace only, so a route in an attached system namespace got NotAllowedByListeners for a hostname it never names, in the same pass that dropped a terminate listener because that same route was serving a name it had borrowed legitimately. Separately, the wildcard refusal ran before the parentRef was read. It is about the terminate listener, which HTTP-01 has no wildcard certificate for, but the port-80 listener carries no hostname and serves a wildcard route like any other, so a route pinning port 80 read Accepted=False while Cilium served it in plaintext. Both now key on the listener the route can actually attach to.

Two smaller ones went in with them. A route that attached and then stopped claiming any hostname kept its last verdict forever, since the status pass walked only what claimed; it walks what attached now. And the retryable status-write path had lost its log line, so a write failing every pass retried at a fixed interval with nothing anywhere to show for it.

Four sentences this branch adds still misstate the code and I left them in rather than spend another round: the dns01 refusal says one apex listener where two render, the README names the wrong source for spec.apex, the tlsPassthroughListeners godoc makes sectionName sound required for routing when it is not, and a renderGateway comment says "owned" where the code reads "claimed". The godoc one ships into the CRD description. There is also a describeWithdrawn message that can name a listener nobody rendered. Prose or one string each, no logic behind any of them, and they are tracked so they do not get lost.

Andrei Kvapil (kvaps)

LGTM as of ae5b626c8.

Read the two refusals you pulled back, and both fixes are the narrow ones.

tlsRouteClaims now skips a tenantOnly listener when the route names no section and lives outside the tenant namespace, and keeps the borrow when the route did name it. That second half is the part worth having: a route that asked for the listener still gets an answer to what it asked, instead of falling out with no verdict at all.

The wildcard refusal moved into the !hasPort || port == httpsListenerPort branch, so a route pinned to port 80 or to sectionName: http reaches the plain-served branch first and is not refused for a certificate it never needed. I went looking for a third false refusal the move could have created and did not find one: a wildcard with no port and no section still lands on the terminate branch and is refused, and a wildcard on some other port still reads the port mismatch.

Walking attached instead of what claimed, and retracting the entry for a ref that attached and claims nothing, is right for the same reason. Claiming nothing is not stable, and a stale verdict over a listener set that no longer exists is worse than saying nothing. Both new tests fail on a revert of their own change, and go test ./internal/controller/tenantgateway/... is green here.

The log line on the retryable path matters more than it looks: a nil error opts out of the backoff, so a write failing every pass would have retried at a fixed period with nothing recorded anywhere.

On the five statements in #4207, the godoc one is the one I would rather see fixed in this PR than after it. "Nothing is routed until a TLSRoute attaches to it by sectionName tls-<name>" is not what the code does, since a hostname-less route with no sectionName attaches to every listener it may attach to, and that sentence ships into the CRD description, so it is what kubectl explain prints on every cluster. If you push again for any other reason, fold it in. The other four are internal prose and I am fine with them riding #4207.

Still open from my side, and neither blocks: #4198 for the recount gate and the namespace-label ordering, #4199 for this controller's status writer against a live Cilium, which your run could only half answer.

Approval is on ae5b626c8 alone, and the gate counts it only on that commit.

Aleksei Sviridkin (lexfrei) · 2026-09-11T09:03:17Z

Merged at ae5b626c8 with your change request still standing, so here is what it was resting on rather than leaving you to reconstruct it.

Every item from your round on 491beb6c6 is closed and each one is answered in the thread above with the mechanism: the wildcard listener name the CRD refuses, the swallowed route-status write, and parentRef.port on HTTPRoutes. Two rounds went past that. The second found that a route which stopped claiming a hostname kept its last verdict forever, and that a route pinning port 80 was told it was unserved while Cilium served it in plaintext; both are fixed with tests. kvaps reviewed the current head and approved it.

The red check is redis-2-backup-roundtrip, a BackupJob that never left Running. This branch touches twenty-two files and none of them is redis, a backup path or velero.

Five statements the branch adds still misstate the code, listed in the comment above and tracked as #4207, one of which ships into the CRD description. If you would rather any of them had been fixed before merge rather than after, say so and I will treat that as the rule next time.

github-actions Bot added area/networking Issues or PRs related to networking (ingress, gateway, vpn, metallb, kube-ovn) kind/feature Categorizes issue or PR as related to a new feature size/XL This PR changes 500-999 lines, ignoring generated files labels Jul 17, 2026

Aleksei Sviridkin (lexfrei) marked this pull request as ready for review July 17, 2026 19:27

Aleksei Sviridkin (lexfrei) requested review from IvanHunters, Andrey Kolkov (androndo), Andrei Kvapil (kvaps), Timofei Larkin (lllamnyp) and Kirill Ilin (sircthulhu) as code owners July 17, 2026 19:27

Aleksei Sviridkin (lexfrei) requested a review from myasnikovdaniil as a code owner July 17, 2026 19:27

dosubot Bot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Jul 17, 2026

IvanHunters reviewed Jul 18, 2026

View reviewed changes

github-actions Bot added size/XXL This PR changes 1000+ lines, ignoring generated files and removed size/XL This PR changes 500-999 lines, ignoring generated files labels Jul 19, 2026

Aleksei Sviridkin (lexfrei) force-pushed the feat/tenant-gateway-tls-passthrough-listeners branch from acaffb4 to 3958dd1 Compare July 19, 2026 21:33

coderabbitai Bot reviewed Jul 19, 2026

View reviewed changes

Comment thread internal/controller/tenantgateway/crdcel_test.go Outdated

Aleksei Sviridkin (lexfrei) self-assigned this Jul 20, 2026

myasnikovdaniil requested changes Jul 20, 2026

View reviewed changes

Comment thread api/gateway/v1alpha1/tenantgateway_types.go Outdated

Andrei Kvapil (kvaps) mentioned this pull request Jul 27, 2026

design-proposals: fold extra into apps — tenant modules as apps with declarative capabilities cozystack/community#39

Open

coderabbitai Bot reviewed Jul 27, 2026

View reviewed changes

Aleksei Sviridkin (lexfrei) force-pushed the feat/tenant-gateway-tls-passthrough-listeners branch from 78f8d7e to a8a1ae2 Compare July 27, 2026 22:37

coderabbitai Bot reviewed Jul 27, 2026

View reviewed changes

Aleksei Sviridkin (lexfrei) force-pushed the feat/tenant-gateway-tls-passthrough-listeners branch from a8a1ae2 to a467c51 Compare July 27, 2026 23:54

coderabbitai Bot reviewed Jul 27, 2026

View reviewed changes

Comment thread api/gateway/v1alpha1/tenantgateway_types.go Outdated

Aleksei Sviridkin (lexfrei) force-pushed the feat/tenant-gateway-tls-passthrough-listeners branch from a467c51 to 578c355 Compare August 7, 2026 17:20

Aleksei Sviridkin (lexfrei) requested a review from IvanHunters September 9, 2026 06:44

Andrei Kvapil (kvaps) requested changes Sep 9, 2026

View reviewed changes

Andrei Kvapil (kvaps) previously approved these changes Sep 10, 2026

View reviewed changes

Aleksei Sviridkin (lexfrei) dismissed Andrei Kvapil (kvaps)’s stale review via 8af9863 September 10, 2026 11:22

Aleksei Sviridkin (lexfrei) force-pushed the feat/tenant-gateway-tls-passthrough-listeners branch from 20abf87 to 8af9863 Compare September 10, 2026 11:22

Aleksei Sviridkin (lexfrei) requested a review from Timur Tukaev (tym83) as a code owner September 10, 2026 11:22

Andrei Kvapil (kvaps) previously approved these changes Sep 10, 2026

View reviewed changes

Aleksei Sviridkin (lexfrei) added 4 commits September 10, 2026 18:47

This was referenced Sep 10, 2026

gateway: five statements in the TenantGateway package describe the code incorrectly, one of them in the CRD #4207

Open

gateway: two tenants with the Gateway integration overwrite each other's namespace attach label #4208

Closed

Andrei Kvapil (kvaps) approved these changes Sep 10, 2026

View reviewed changes

Aleksei Sviridkin (lexfrei) mentioned this pull request Sep 30, 2026

fix(gateway): copy allowed routes for the edge listener #4167

Open

1 of 12 tasks

Conversation

Aleksei Sviridkin (lexfrei) commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does

Release note

Downstream repositories

Summary by CodeRabbit

Uh oh!

gemini-code-assist Bot commented Jul 17, 2026

Uh oh!

coderabbitai Bot commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

gemini-code-assist Bot commented Jul 17, 2026

Uh oh!

IvanHunters left a comment

Choose a reason for hiding this comment

Uh oh!

Aleksei Sviridkin (lexfrei) commented Jul 19, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

myasnikovdaniil left a comment

Choose a reason for hiding this comment

Process gate — needs an API owner, nothing in the diff can satisfy it

Blockers

B1: the field's documented contract is falsified by the controller once a TLSRoute attaches

B2: no TLSRoute appears anywhere in the test suite

Action items (non-blocking)

Phase-1 risk assessment: safe, and better hedged than most

Dismissed after investigation

Scope drift

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Aleksei Sviridkin (lexfrei) commented Jul 27, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Andrei Kvapil (kvaps) left a comment

Choose a reason for hiding this comment

Verdict

Why this exists, and whether the mechanism fits

The blocking item

A second one, and the direction of the change is right

What the withdrawal actually changes

A precondition the release note should carry

Notes, none of them blocking

Process

Uh oh!

Andrei Kvapil (kvaps) commented Sep 9, 2026

Uh oh!

Andrei Kvapil (kvaps) left a comment

Choose a reason for hiding this comment

Approving, and being explicit about what that covers

Carried as follow-ups

Not merging

Uh oh!

Andrei Kvapil (kvaps) commented Sep 10, 2026

Uh oh!

Andrei Kvapil (kvaps) left a comment

Choose a reason for hiding this comment

Re-approving on 8af986398

Uh oh!

Aleksei Sviridkin (lexfrei) commented Sep 10, 2026

Uh oh!

Andrei Kvapil (kvaps) commented Sep 10, 2026

Aleksei Sviridkin (lexfrei) commented Jul 17, 2026 •

edited

Loading

coderabbitai Bot commented Jul 17, 2026 •

edited

Loading

Re-approving on `8af986398`