AlinsRan commented on code in PR #2882:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2882#discussion_r4244456412
##########
internal/adc/translator/tlsroute.go:
##########
@@ -143,18 +141,120 @@ func (t *Translator) TranslateTLSRoute(tctx
*provider.TranslateContext, tlsRoute
}
}
- for _, host := range hosts {
- streamRoutes, err := t.buildL4StreamRoutes(tctx,
tlsRoute.Namespace, tlsRoute.Name, ruleIndex, "TLS", "TLSRoute", labels)
- if err != nil {
- return nil, err
+ // One set of stream routes per rule, each carrying the SNIs of
its own
+ // port, rather than one set per hostname: buildL4StreamRoutes
names them by
+ // rule and port only, so a per hostname loop produced N sets
sharing one ID
+ // and only the last survived.
+ streamRoutes, err := t.buildL4StreamRoutes(tctx,
tlsRoute.Namespace, tlsRoute.Name, ruleIndex, "TLS", "TLSRoute", labels)
+ if err != nil {
+ return nil, err
+ }
+ for _, streamRoute := range streamRoutes {
+ snis := tlsRouteSNIsForPort(tctx.Listeners,
tlsRoute.Spec.Hostnames, streamRoute.ServerPort)
+ if len(snis) == 0 {
+ // Nothing for the stream route to match on.
The listeners a route
+ // attaches to already intersect its hostnames,
so this is a guard
+ // against emitting a route that would answer
every SNI instead.
+ continue
}
- for _, streamRoute := range streamRoutes {
- streamRoute.SNI = host
+ // A single SNI keeps using the singular form: it is
what every
+ // APISIX version understands, and snis only earns its
place once
+ // there is more than one to match.
+ if len(snis) == 1 {
+ streamRoute.SNI = snis[0]
+ } else {
+ streamRoute.SNIs = snis
+ }
+ if tlsPassthroughOnPort(tctx.Listeners,
streamRoute.ServerPort) {
+ streamRoute.TLSPassthrough = ptr.To(true)
}
- service.StreamRoutes = append(service.StreamRoutes,
streamRoutes...)
+ service.StreamRoutes = append(service.StreamRoutes,
streamRoute)
}
result.Services = append(result.Services, service)
}
return result, nil
}
+
+// tlsRouteSNIsForPort returns the SNIs the stream route bound to port matches
on.
+//
+// The set is derived from the listeners on that port alone. Handing every port
+// the route-wide union instead would let a port answer a hostname its own
+// listener never accepted, and would cost a port its catch-all as soon as a
+// listener on another port named a hostname. port 0 means the stream route
+// carries no server_port match, so every matched listener applies.
+//
+// A TLSRoute without hostnames matches everything its listeners accept, so it
+// falls back to the hostnames of the listeners on the port and, when one of
them
+// carries no hostname either, to the catch-all "*". Emitting nothing - which
is
+// what the per hostname loop used to do - left such a route attached but
+// unserved.
+func tlsRouteSNIsForPort(listeners []gatewayv1.Listener, routeHostnames
[]gatewayv1.Hostname, port int32) []string {
+ matched := make([]gatewayv1.Listener, 0, len(listeners))
+ for _, listener := range listeners {
+ if listener.Protocol != gatewayv1.TLSProtocolType {
+ continue
+ }
+ if port != 0 && listener.Port != port {
+ continue
+ }
+ matched = append(matched, listener)
+ }
+
+ // No listener to narrow against: keep what the route itself asks for.
+ if len(matched) == 0 {
+ if len(routeHostnames) == 0 {
+ return []string{"*"}
+ }
+ return hostnamesToSNIs(routeHostnames)
+ }
+
+ if len(routeHostnames) == 0 {
+ hostnames, matchAnyHost := cutils.UnionHostnames(matched)
+ if matchAnyHost {
+ return []string{"*"}
+ }
+ return hostnamesToSNIs(cutils.DedupHostnames(hostnames))
+ }
+
+ snis := make([]gatewayv1.Hostname, 0, len(routeHostnames))
+ for _, hostname := range routeHostnames {
+ snis = append(snis, cutils.NarrowHostname(matched, hostname)...)
+ }
+ return hostnamesToSNIs(cutils.DedupHostnames(snis))
+}
+
+func hostnamesToSNIs(hostnames []gatewayv1.Hostname) []string {
+ snis := make([]string, 0, len(hostnames))
+ for _, hostname := range hostnames {
+ snis = append(snis, string(hostname))
+ }
+ return snis
+}
+
+// tlsPassthroughOnPort reports whether the stream routes bound to port must
+// forward the connection untouched instead of having the gateway terminate it.
+// port 0 means the StreamRoute carries no server_port match, so every matched
+// listener applies.
+//
+// Every matched TLS listener on the port has to agree. Within one Gateway a
+// port carrying both modes is already reported ProtocolConflict and attaches
+// no routes; across Gateways the combination is unrepresentable, since the
+// physical stream listen has a single mode - so the terminating behaviour wins
+// rather than a guess.
+func tlsPassthroughOnPort(listeners []gatewayv1.Listener, port int32) bool {
+ matched := false
+ for _, listener := range listeners {
+ if listener.Protocol != gatewayv1.TLSProtocolType {
+ continue
Review Comment:
You are right about the scope, and I took the second option — the guarantee
is narrowed rather than the check widened. 3b6363d3.
```markdown
<!-- docs/en/latest/concepts/gateway-api.md -->
A single port cannot mix the two modes. Within one Gateway, listeners that
disagree on `tls.mode`
for one port are reported `Accepted=False` with `ProtocolConflict` and carry
no routes, and a route
attached to two Gateways that disagree about the same port falls back to
`Terminate`, because a
physical stream listen has a single mode. Two Gateways that disagree are
each valid on their own, so
when different routes attach to each of them no conflict is reported and
every route is translated
for the mode its own listener declares — only one of which the listen can
honour. The controller
does not configure the stream listen, so it cannot detect or resolve that:
declare one `tls.mode`
per port across Gateways that share a data plane.
```
```go
// internal/adc/translator/tlsroute.go
// Every TLS listener this route matched on the port has to agree. Within one
// Gateway a port carrying both modes is already reported ProtocolConflict
and
// attaches no routes; a route attached to two Gateways that disagree falls
back
// to terminating rather than to a guess, since the physical stream listen
has a
// single mode.
//
// Two *different* routes attached to two disagreeing Gateways are each
// translated for the mode their own listener declares, and only one of
those is
// the mode the listen was configured with. The controller does not
configure the
// stream listen, so it can neither detect that nor resolve it.
```
Why not port-global handling: the mode of the physical listen comes from the
operator's `stream_proxy` config, which the controller neither reads nor writes
— it cannot open or reconfigure a data plane port, which is also why
`spec.listeners[].port` is only partially supported. So a port-global rule
would have to pick a winner between two Gateways that are each individually
valid, and the winner it picks may still be the wrong one for how the listen
was actually configured. It would also mean one Gateway's route silently stops
working because an unrelated Gateway exists, with no condition to report it on:
`ProtocolConflict` is a listener condition inside a Gateway, and there is
nothing in the spec that lets one Gateway invalidate another's listener.
Serving both modes on one listen is `TLSRouteModeMixed`, which this PR
deliberately does not claim.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
