Skip to content

Commit 9ae6a80

Browse files
lexfreilexfrei
committed
fix(gateway): stop describing Cilium 1.19.5 as the shipped release
With Cilium at 1.20.2 the tenant gateway comments, README and one status message still explained behaviour as that of "the pin" at 1.19.5. Several of those claims no longer hold: a Gateway with two routed passthrough ports now gets an Envoy listener per port, Cilium judges a route's kinds against the listener the route names, and the shipped operator accepts an HTTPS and a passthrough listener on one port when the passthrough hostname is strictly narrower. Behaviour that is unchanged is re-anchored on the 1.20.2 source, behaviour that is gone is stated as history, and the refusal of one hostname on two passthrough ports no longer blames a Cilium limitation the shipped release does not have. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
1 parent 966049f commit 9ae6a80

4 files changed

Lines changed: 78 additions & 97 deletions

File tree

‎internal/controller/tenantgateway/reconciler.go‎

Lines changed: 24 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -291,12 +291,10 @@ func (r *Reconciler) runReconcileSteps(ctx context.Context, tgw *gatewayv1alpha1
291291
// listener as ResolvedRefs=False/InvalidRouteKinds rather than
292292
// as a grant.
293293
//
294-
// Nothing else tells the route. CheckGatewayRouteKindAllowed
295-
// (operator/pkg/gateway-api/routechecks/gateway_checks.go,
296-
// v1.19.5) reads each listener's kinds against every route on
297-
// the Gateway rather than against the routes that named it, so
298-
// that same HTTPRoute entry has Cilium report this route
299-
// Accepted, and this condition is the only refusal.
294+
// Nothing else is bound to tell the route. The listener lists
295+
// HTTPRoute among its kinds, so whether the Gateway controller
296+
// refuses the route is left to the implementation, and this
297+
// condition is the refusal that does not depend on it.
300298
//
301299
// Such a ref then leaves the hostname before anything else is
302300
// decided about it, the way a refused TLSRoute leaves the
@@ -1375,9 +1373,10 @@ func (r *Reconciler) renderGateway(tgw *gatewayv1alpha1.TenantGateway, dynHostna
13751373
// route and the http->https redirect, neither of which exists in edge
13761374
// mode, where it would only be a catch-all admitting any hostname.
13771375
// Leaving it out also keeps a hostname-pinned listener at index 0,
1378-
// which is what Cilium's first-listener-wins namespace check reads
1379-
// (cilium#42159, fixed in 1.19.6): with the narrow :80 selector first,
1380-
// an inheriting tenant's route attaches to no listener at all.
1376+
// which is what Cilium's first-listener-wins namespace check read
1377+
// before 1.19.6 (cilium#42159, fixed by cilium#45693): with the narrow
1378+
// :80 selector first, an inheriting tenant's route attached to no
1379+
// listener at all.
13811380
if tgw.Spec.CertMode != gatewayv1alpha1.CertModeEdge {
13821381
listeners = append(listeners, gatewayv1.Listener{
13831382
Name: httpListenerName,
@@ -1588,12 +1587,7 @@ func (r *Reconciler) renderGateway(tgw *gatewayv1alpha1.TenantGateway, dynHostna
15881587
// on the entry's native Port, mode Passthrough, matching the
15891588
// entry's per-engine SNI Hostname, rendered alongside the port-443
15901589
// terminate listeners. What may attach is left to the protocol
1591-
// rather than declared: the port-443 TLSPassthroughServices
1592-
// listeners above must share their allowedRoutes.kinds with the
1593-
// terminate listeners to dodge Cilium's same-port listener collapse
1594-
// (cilium#45559), and these declare no kinds at all, because on the
1595-
// pinned Cilium a declared set is applied to every route on the
1596-
// Gateway rather than to the listener that declares it. A dedicated
1590+
// rather than declared, for the reason on Kinds below. A dedicated
15971591
// (port, SNI) pair still yields exactly one Envoy filter chain that
15981592
// SNI-routes to the attaching TLSRoute's backend. No engine is wired here: the
15991593
// TLSRoute, certificate, and CA plumbing land in later phases.
@@ -1606,30 +1600,21 @@ func (r *Reconciler) renderGateway(tgw *gatewayv1alpha1.TenantGateway, dynHostna
16061600
// of the whole subtree. Narrowing costs nothing while no chart
16071601
// value exposes this field; once something depends on the wide
16081602
// form, narrowing becomes a behaviour change instead.
1609-
// Kinds is left unset on purpose, and the reason is upstream
1610-
// rather than stylistic. CheckGatewayRouteKindAllowed at the
1611-
// pinned v1.19.5 walks every listener on the Gateway with no
1612-
// port and no sectionName filter, skips the ones that declare
1613-
// no kinds, and overwrites the route's Accepted condition on
1614-
// each of the rest, last one winning. A listener declaring
1615-
// TLSRoute alone would therefore reject every HTTPRoute on the
1616-
// Gateway, whatever port it sits on. Declaring nothing keeps
1617-
// this listener out of that loop. What it opens is bounded by
1618-
// the implementation rather than by the spec: Gateway API says
1619-
// only that an absent Kinds derives the set from the listener
1620-
// protocol and leaves the mapping to the implementation, and
1621-
// the conventional table pairs TLS with TCPRoute as well as
1622-
// TLSRoute. Cilium implements neither TCPRoute nor UDPRoute, so
1623-
// on the pinned version TLSRoute is what a TLS listener admits,
1624-
// and the listener pins kubernetes.io/metadata.name to the
1625-
// publishing tenant regardless.
1626-
//
1627-
// This one lifts on a patch bump rather than with the rest of
1628-
// the pin: v1.19.6 skips listeners the parentRef's sectionName
1629-
// or port does not name and returns on the first match, so
1630-
// declaring TLSRoute here would then bind to this listener
1631-
// alone. Spelling it out again is safe from that release on,
1632-
// and pointless, since the protocol already says TLSRoute.
1603+
// Kinds is left unset because the protocol already says
1604+
// TLSRoute. Before v1.19.6, CheckGatewayRouteKindAllowed applied
1605+
// a listener's declared kinds to every route on the Gateway, so
1606+
// declaring TLSRoute here would have rejected every HTTPRoute.
1607+
// From that release it judges a route against the listener its
1608+
// sectionName or port names alone.
1609+
// What an unset Kinds opens is bounded by the implementation
1610+
// rather than by the spec: Gateway API leaves the
1611+
// protocol-to-kind mapping to it, and the conventional table
1612+
// pairs TLS with TCPRoute as well as TLSRoute. Cilium maps the
1613+
// TLS protocol to TLSRoute alone (getSupportedRouteKinds in
1614+
// operator/pkg/gateway-api/helpers.go, v1.20.2), so TLSRoute is
1615+
// what a TLS listener admits, and the listener pins
1616+
// kubernetes.io/metadata.name to the publishing tenant
1617+
// regardless.
16331618
passthroughAllowed := allowedRoutesFromValues([]string{tgw.Namespace})
16341619
listeners = append(listeners, gatewayv1.Listener{
16351620
Name: gatewayv1.SectionName(passthroughListenerPrefix + pl.Name),

‎internal/controller/tenantgateway/reconciler_test.go‎

Lines changed: 33 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -2921,9 +2921,10 @@ func TestReconcile_TLSRouteOnPassthroughListenerTerminatesNothing(t *testing.T)
29212921
// same hostname as the passthrough listener. Gateway API admits that —
29222922
// its uniqueness rule is on (port, protocol, hostname) and the two
29232923
// differ in protocol — and calls for both to be marked Conflicted, so
2924-
// the hostname is served by neither. The pinned v1.19.5 marks nothing
2925-
// and hands Envoy two chains under one server name; v1.19.6 marks the
2926-
// pair from the listener specs alone.
2924+
// the hostname is served by neither. Cilium marks the pair from the
2925+
// listener specs alone and drops it from Envoy, and the operator patch
2926+
// that admits a wildcard beside a narrower passthrough listener keeps
2927+
// that for an identical hostname.
29272928
func TestReconcile_TLSRouteOnPassthroughServiceTerminatesNothing(t *testing.T) {
29282929
s := newScheme(t)
29292930
tgw := &gatewayv1alpha1.TenantGateway{
@@ -3487,9 +3488,9 @@ func TestReconcile_WildcardPassthroughWithdrawsTheNamesBeneathIt(t *testing.T) {
34873488
// terminate listener leaves nothing on the Gateway for the route to
34883489
// attach to, and Accepted=True would then describe a hostname the
34893490
// controller deliberately dropped. Nothing on the Gateway says so
3490-
// instead: the pinned Cilium sets no Conflicted condition, so the pair
3491-
// rendered and left no record of the collision anywhere. That is what
3492-
// makes the route condition the only place to say it.
3491+
// instead: the terminate listener is never rendered, so the Gateway
3492+
// holds no record of the claim. That is what makes the route condition
3493+
// the only place to say it.
34933494
//
34943495
// The reason must differ from HostnameConflict, which states that
34953496
// another route won the same hostname. Nothing won this one.
@@ -3766,8 +3767,8 @@ func TestReconcile_TLSRouteLoserOnAPassthroughHostnameKeepsItsConflict(t *testin
37663767
// accepted exactly when some listener answering its name would take a
37673768
// route from its namespace.
37683769
// noRouteHostnames marks a verdict-table row whose route declares no
3769-
// spec.hostnames at all, the shape TLSRoute v1alpha2 permits and the
3770-
// pinned Cilium serves on the hostname of the listener the route
3770+
// spec.hostnames at all, the shape TLSRoute v1alpha2 permits and
3771+
// Gateway API serves on the hostname of the listener the route
37713772
// selects. Spelled as a name rather than left as a bare "" so a row
37723773
// cannot be read as claiming the empty hostname.
37733774
const noRouteHostnames = ""
@@ -3848,8 +3849,8 @@ func TestReconcile_TLSRouteVerdictTable(t *testing.T) {
38483849
{"section names no listener, tenant namespace", "svc." + apex, "tenant-foo",
38493850
[]string{"svc"}, nil, "tls-absent", 0, false, "tls-absent", ""},
38503851
// A route that declares no hostnames is served on the hostname
3851-
// of the listener it selects: ComputeHosts substitutes it at the
3852-
// pin (operator/pkg/model/helpers.go, v1.19.5), so the claim
3852+
// of the listener it selects: ComputeHosts substitutes it
3853+
// (operator/pkg/model/helpers.go, v1.20.2), so the claim
38533854
// exists and is judged like a declared one. Pinning the listener
38543855
// by sectionName and leaving the hostname to it is the ordinary
38553856
// way to write such a route.
@@ -4264,9 +4265,9 @@ func TestReconcile_HostnameLessTLSRouteEntryIsRetractedWhenItsListenerGoes(t *te
42644265
// TestReconcile_BorrowedHostnamesStopAtListenersTheRouteCanAttachTo
42654266
// pins which listeners lend their hostname to a route that declares
42664267
// none. A native-port listener admits the publishing tenant alone, so
4267-
// the pinned Cilium never puts a route from elsewhere on it and never
4268-
// serves that route on its hostname; a port-443 passthrough listener
4269-
// takes every attached namespace and does lend. Borrowing from both
4268+
// no route from elsewhere attaches to it or is served on its
4269+
// hostname; a port-443 passthrough listener takes every attached
4270+
// namespace and does lend. Borrowing from both
42704271
// hands a route a name its own object does not carry and then refuses
42714272
// it for the namespace, in the same pass that sheds a terminate
42724273
// listener because the very same route is serving the name it did
@@ -4352,11 +4353,9 @@ func TestReconcile_BorrowedHostnamesStopAtListenersTheRouteCanAttachTo(t *testin
43524353
// the native-port listeners declare no kinds at all. The verdict is the
43534354
// same on both.
43544355
//
4355-
// Nor does anything else tell the route: CheckGatewayRouteKindAllowed
4356-
// (operator/pkg/gateway-api/routechecks/gateway_checks.go, v1.19.5)
4357-
// reads each listener's kinds against every route on the Gateway rather
4358-
// than against the routes that named it, so the HTTPRoute entry on the
4359-
// port-443 listeners has Cilium report such a route Accepted.
4356+
// Nothing else is bound to tell the route: the port-443 listeners list
4357+
// HTTPRoute among their kinds, so whether the Gateway controller refuses
4358+
// such a route is left to the implementation.
43604359
//
43614360
// A route claiming the same hostname with no sectionName is refused
43624361
// for the reservation instead, and one naming a listener outside the
@@ -6596,9 +6595,9 @@ func TestValidateTLSPassthroughListeners(t *testing.T) {
65966595
{"duplicate passthrough service", nil,
65976596
[]string{"api", "api"}, apex, true, "duplicate entry"},
65986597
// Gateway API keys listeners by (port, protocol, hostname), so
6599-
// these are distinct and the object is accepted; Cilium routes
6600-
// passthrough by SNI alone (cilium#42898) and silently serves
6601-
// only one of them.
6598+
// these are distinct and the object is accepted; the controller
6599+
// refuses the pair, which Cilium before 1.19.6 served only one
6600+
// of (cilium#42898).
66026601
{"same hostname on different ports", []gatewayv1alpha1.TLSPassthroughListener{
66036602
mk("pg", 5432, "db.foo.example.com"),
66046603
mk("pg2", 5433, "db.foo.example.com"),
@@ -8648,16 +8647,15 @@ func TestReconcile_Port443ListenersShareKinds(t *testing.T) {
86488647
assertPort443Contract := func(t *testing.T, gw *gatewayv1.Gateway) {
86498648
t.Helper()
86508649
// Swept by whether the listener declares kinds at all, not by
8651-
// port, because that is the shape of the check at the pinned
8652-
// v1.19.5: CheckGatewayRouteKindAllowed walks every listener on
8653-
// the Gateway with no port and no sectionName filter, skips the
8654-
// ones whose kinds are empty, and overwrites the route's
8650+
// port, because that was the shape of the check before v1.19.6:
8651+
// CheckGatewayRouteKindAllowed walked every listener on the
8652+
// Gateway with no port and no sectionName filter, skipped the
8653+
// ones whose kinds were empty, and overwrote the route's
86558654
// Accepted condition on each of the rest, so the last listener
8656-
// decides. One listener whose set omits HTTPRoute therefore
8657-
// rejects every HTTPRoute on the Gateway, whatever port it
8658-
// sits on. v1.19.6 narrows the walk to the listener the
8659-
// parentRef names, which makes this sweep stricter than that
8660-
// release requires rather than wrong for it.
8655+
// decided. v1.19.6 narrows the walk to the listener the
8656+
// parentRef names, which makes this sweep stricter than the
8657+
// shipped release requires rather than wrong for it; it goes
8658+
// with the uniform-kinds workaround.
86618659
var declared []gatewayv1.Listener
86628660
for _, l := range gw.Spec.Listeners {
86638661
if l.AllowedRoutes != nil && len(l.AllowedRoutes.Kinds) > 0 {
@@ -8988,10 +8986,10 @@ func TestReconcile_EdgeModeRendersPlainHTTPListeners(t *testing.T) {
89888986
}
89898987
if l.Hostname == nil {
89908988
// A hostname-less listener admits every host, and Cilium
8991-
// reads only the first listener when deciding whether a
8992-
// namespace may attach (cilium#42159), so one sitting at
8993-
// index 0 with the narrow ACME selector detaches every
8994-
// inheriting tenant's route.
8989+
// before 1.19.6 read only the first listener when deciding
8990+
// whether a namespace may attach (cilium#42159), so one
8991+
// sitting at index 0 with the narrow ACME selector detached
8992+
// every inheriting tenant's route.
89958993
t.Errorf("edge mode must render no hostname-less listener, got %q", l.Name)
89968994
continue
89978995
}
@@ -9324,7 +9322,7 @@ func TestReconcile_LeavingEdgeForAWildcardModeClearsTheWithdrawal(t *testing.T)
93249322
// whole-apex pass leaves a route with no spec.hostnames alone, which is
93259323
// not that the controller never judges one. Under http01 it does: the
93269324
// route is judged on the hostname the listener its sectionName names
9327-
// lends it, the way the pinned Cilium serves it. What decides the skip
9325+
// lends it, the way Gateway API serves it. What decides the skip
93289326
// is the leg where that lends nothing. A route pinned to a section this
93299327
// Gateway renders no listener for produces no claim under http01
93309328
// either, so a refusal written on it under edge would have no pass that

‎internal/controller/tenantgateway/renderers.go‎

Lines changed: 16 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -258,11 +258,11 @@ const passthroughListenerPrefix = "tls-"
258258
// Gateway API admits either — its listener uniqueness rule keys on
259259
// (port, protocol, hostname), so differing protocols are distinct — and
260260
// calls for the conflict to surface as Conflicted on both listeners,
261-
// which then serve nothing. That is the specification rather than the
262-
// pinned behaviour: v1.19.5 sets no Conflicted condition anywhere, and
263-
// v1.19.6 adds one in samePortCrossProtocolConflictedListeners, keyed on
264-
// the listener specs alone. Rejecting the entry up front turns either
265-
// into a per-field error on TenantGateway status instead.
261+
// which then serve nothing. Cilium marks such a pair Conflicted from the
262+
// listener specs alone (listenerPairConflict in
263+
// operator/pkg/gateway-api/status_listener.go, v1.20.2). Rejecting the
264+
// entry up front turns either into a per-field error on TenantGateway
265+
// status instead.
266266
//
267267
// A function rather than a package-level set: the ports are a property
268268
// of what renderGateway emits, so nothing should be able to write to
@@ -557,16 +557,15 @@ func validateTLSPassthroughListeners(listeners []gatewayv1alpha1.TLSPassthroughL
557557
// Two listeners sharing a hostname on different ports are
558558
// distinct to Gateway API — listeners are keyed by (port,
559559
// protocol, hostname) — and the object is accepted. Cilium
560-
// routes passthrough by SNI without distinguishing the port
561-
// (cilium#42898, fixed upstream by cilium#44889 and
562-
// backported via cilium#46826 into 1.19.6 — cozystack pins
563-
// 1.19.5 in packages/system/cilium/images/cilium/Dockerfile,
564-
// so revisit this restriction when that pin moves), so only
565-
// one of them works and which one depends on route ordering.
566-
// On a native database port that is a raw stream forwarded to
567-
// the wrong backend, with Accepted and Programmed both true
568-
// and nothing on the status to show for it. Reject the shape
569-
// instead.
560+
// before 1.19.6 routed passthrough by SNI without
561+
// distinguishing the port (cilium#42898), so only one of them
562+
// worked, a raw stream forwarded to the wrong backend with
563+
// Accepted and Programmed both true. From 1.19.6 (cilium#44889,
564+
// backported by cilium#46826) a Gateway with two routed
565+
// passthrough ports gets an Envoy listener per port
566+
// (NeedsPerPortTLSPassthroughListeners in
567+
// operator/pkg/model/model.go), so the shipped 1.20.2 no longer
568+
// needs this refusal; lifting it is tracked in cozystack/cozystack#4765.
570569
//
571570
// Checked after the apex test on purpose: an out-of-apex
572571
// hostname that also happens to overlap should report the apex
@@ -575,7 +574,7 @@ func validateTLSPassthroughListeners(listeners []gatewayv1alpha1.TLSPassthroughL
575574
if !hostnamesOverlap(l.Hostname, claimed.hostname) {
576575
continue
577576
}
578-
return fmt.Errorf("tlsPassthroughListeners: listener %q hostname %q overlaps %s hostname %q; Cilium routes TLS passthrough by SNI alone and cannot distinguish two listeners whose hostnames match the same ClientHello, even on different ports", l.Name, l.Hostname, claimed.source, claimed.hostname)
577+
return fmt.Errorf("tlsPassthroughListeners: listener %q hostname %q overlaps %s hostname %q; two passthrough listeners may not match the same ClientHello SNI, even on different ports", l.Name, l.Hostname, claimed.source, claimed.hostname)
579578
}
580579
seenHostnames = append(seenHostnames, claimedHostname{l.Hostname, fmt.Sprintf("listener %q", l.Name)})
581580
}
@@ -591,8 +590,7 @@ func validateTLSPassthroughListeners(listeners []gatewayv1alpha1.TLSPassthroughL
591590
// does not match "foo.example.com"), which is why the exact leg tests
592591
// for the leading dot.
593592
//
594-
// This matters because the caller rejects overlapping hostnames on the
595-
// premise that Cilium routes passthrough by SNI alone. An exact-match
593+
// This matters because the caller rejects overlapping hostnames. An exact-match
596594
// check would let a single "*.<apex>" entry silently shadow the
597595
// tls-<svc> listeners the chart ships by default (api, vm-exportproxy,
598596
// cdi-uploadproxy all render <svc>.<apex>), which is the exact failure

0 commit comments

Comments
 (0)