AlinsRan commented on code in PR #2804:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2804#discussion_r3635519602
##########
internal/controller/utils.go:
##########
@@ -383,6 +383,10 @@ func ParseRouteParentRefs(
}
if ok, _ := routeMatchesListenerType(route, listener);
!ok {
+ // The listener exists but its protocol cannot
carry this route kind,
+ // which the spec reports as
NotAllowedByListeners. A parentRef that
+ // matches no listener at all stays
NoMatchingParent.
+ reason =
gatewayv1.RouteReasonNotAllowedByListeners
Review Comment:
Fixed in d7a05234. `ParseRouteParentRefs` now aggregates `hostnameMismatch`
/ `notAllowed` across all listeners and selects the reason after the loop
(`hostnameMismatch` > `notAllowed` > `NoMatchingParent`), so the result is
independent of listener order and a compatible-but-hostname-mismatched listener
keeps `NoMatchingListenerHostname`. Added a test that runs both listener orders
and asserts the same reason.
##########
internal/adc/translator/translator.go:
##########
@@ -66,6 +66,27 @@ func hasExplicitListenerTarget(parentRefs
[]gatewayv1.ParentReference) bool {
return false
}
+// collectServerPortMatchPorts returns the set of listener ports that should be
+// enforced through a server_port var.
+//
+// Listeners carrying a hostname are isolated by that hostname (service.hosts),
+// which is the correct discriminator when several listeners share a single
port.
+// A server_port var adds no isolation for them and actively breaks routing: it
+// pins the route to the Gateway's declared listener port, which need not equal
+// the port APISIX actually accepts the connection on (node_listen), turning
+// every request into a 404. Only hostname-less listeners rely on port-based
+// isolation, so only their ports contribute here.
+func collectServerPortMatchPorts(listeners []gatewayv1.Listener)
map[int32]struct{} {
+ ports := make(map[int32]struct{})
+ for _, listener := range listeners {
+ if listener.Hostname != nil && *listener.Hostname != "" {
Review Comment:
Correction to my earlier reply here — I described a different branch. On
this PR the server_port handling was reworked in a5e18fff:
`collectServerPortMatchPorts` collects ports only from hostname-*less*
listeners, and hostname-carrying listeners are isolated by `service.hosts`
instead, so a hostname listener is no longer pinned to a `server_port` var.
For the exact multi-ParentRef case you raised (one route targeting both a
hostname and a hostname-less listener), the route can still end up with both
`service.hosts` (from the hostname listener) and a `server_port` var (from the
hostname-less listener) on the same object, which over-constrains it. So your
per-listener split suggestion may still be warranted there. I did not touch
this path in the current change — happy to take the split as a follow-up if
you'd like.
##########
internal/adc/translator/translator.go:
##########
@@ -66,6 +66,27 @@ func hasExplicitListenerTarget(parentRefs
[]gatewayv1.ParentReference) bool {
return false
}
+// collectServerPortMatchPorts returns the set of listener ports that should be
+// enforced through a server_port var.
+//
+// Listeners carrying a hostname are isolated by that hostname (service.hosts),
+// which is the correct discriminator when several listeners share a single
port.
+// A server_port var adds no isolation for them and actively breaks routing: it
+// pins the route to the Gateway's declared listener port, which need not equal
+// the port APISIX actually accepts the connection on (node_listen), turning
+// every request into a 404. Only hostname-less listeners rely on port-based
+// isolation, so only their ports contribute here.
+func collectServerPortMatchPorts(listeners []gatewayv1.Listener)
map[int32]struct{} {
+ ports := make(map[int32]struct{})
+ for _, listener := range listeners {
+ if listener.Hostname != nil && *listener.Hostname != "" {
Review Comment:
This looks already handled on the current branch. `addServerPortVars` emits
`server_port in (p1, p2, …)` collecting ports from all of `tctx.Listeners` (so
multiple explicit ParentRefs are preserved), not a single-port `==`. A hostname
listener's port is dropped only when the route's hostnames don't intersect it,
in which case that listener legitimately doesn't accept the route. Regression
coverage is in `TestTranslateHTTPRouteServerPortVarsByMode` (the multi-port
`in` cases, including colliding listener names across gateways).
Separately, I noticed a possible *host* collapse in `grpcroute.go`
(`service.Hosts` narrowed to the hostname listener when a hostname-less
parentRef also matches) — I left it out of this change; happy to track it as
its own issue if you agree it's real.
--
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]
