Copilot commented on code in PR #2882:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2882#discussion_r4244441240
##########
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:
This checks only listeners matched by the current TLSRoute's own parentRefs,
not every route/Gateway sharing the physical port. If separate routes attach to
two Gateways with conflicting modes, the Passthrough route still gets
`tls_passthrough: true` while the Terminate route leaves it unset, so the
documented cross-Gateway “routes on that port fall back to Terminate” behavior
never occurs. Either implement port-global conflict handling or narrow the
documented guarantee to a single route attached to disagreeing parents.
##########
Makefile:
##########
@@ -29,7 +29,7 @@ KIND_NAME ?= apisix-ingress-cluster
CLOUD_PROVIDER_KIND_VERSION ?= v0.6.0
CLOUD_PROVIDER_KIND_PID ?= /tmp/cloud-provider-kind.pid
-ADC_VERSION ?= 0.30.5
+ADC_VERSION ?= 0.31.0
Review Comment:
The checked-in sidecar image is still pinned to ADC 0.30.5 in
`config/manager/kustomization.yaml:20`. Building `config/default` directly
therefore deploys a strict schema that rejects the new `snis` and
`tls_passthrough` fields; only Makefile targets that rewrite the image avoid
the failure. Please bump that tracked pin to 0.31.0 as well.
--
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]
