Copilot commented on code in PR #2882:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2882#discussion_r4244240757
##########
internal/adc/translator/tlsroute.go:
##########
@@ -143,18 +141,90 @@ 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 carrying every SNI, not
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 {
+ // 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
}
Review Comment:
The same route-wide SNI list is copied to every per-port StreamRoute. If a
hostname-less TLSRoute attaches to `a.example.com` on port 9110 and
`b.example.com` on 9120, both generated routes accept both names, so each
listener serves a hostname it does not accept. Derive the SNI set from the
listeners matching each `ServerPort` (including a per-port hostname-less
catch-all) instead of assigning the global union.
This issue also appears on line 199 of the same file.
##########
internal/controller/utils.go:
##########
@@ -1392,30 +1392,56 @@ func FullTypeName(a any) string {
// filterHostnames accepts a list of gateways and an HTTPRoute, and returns a
copy of the HTTPRoute with only the hostnames that match the listener hostnames
of the gateways.
// If the HTTPRoute hostnames do not intersect with the listener hostnames of
the gateways, it returns an ErrNoMatchingListenerHostname error.
func filterHostnames(gateways []RouteParentRefContext, httpRoute
*gatewayv1.HTTPRoute) (*gatewayv1.HTTPRoute, error) {
- filteredHostnames := make([]gatewayv1.Hostname, 0)
+ hostnames, err := intersectRouteHostnames(gateways,
httpRoute.Spec.Hostnames)
+ if err != nil {
+ return httpRoute, err
+ }
+ httpRoute.Spec.Hostnames = hostnames
+ return httpRoute, nil
+}
+
+// filterTLSRouteHostnames is filterHostnames for a TLSRoute. Its hostnames
become
+// the SNIs the stream routes match on, so a route left carrying its own
broader
+// hostname would serve names the listener it attached to never accepted -
and, on
+// a single shared stream listen, would take them from the route that should
have.
+func filterTLSRouteHostnames(gateways []RouteParentRefContext, tlsRoute
*gatewayv1.TLSRoute) (*gatewayv1.TLSRoute, error) {
+ hostnames, err := intersectRouteHostnames(gateways,
tlsRoute.Spec.Hostnames)
Review Comment:
`intersectRouteHostnames` keeps only the first listener intersection for
each route hostname because `getMinimumHostnameIntersection` returns on its
first match. For example, a TLSRoute with `*.example.com` attached to same-port
listeners for `a.example.com` and `b.example.com` is emitted only for the first
name, so traffic accepted by the second listener is dropped. Collect and
deduplicate every intersection across the accepted matched listeners for
TLSRoute.
##########
test/conformance/conformance_test.go:
##########
@@ -62,6 +51,18 @@ var skippedTestsForKnownGaps = []string{
// A single HTTPRoute attached to several Gateways is not served from
each
// parent independently.
tests.HTTPRouteMultipleGateways.ShortName,
+
+ // The same limitation for TLSRoute, and not something the translator
can
+ // fix. The test stands four Gateways up on port 443 with different
listener
+ // hostnames; every Gateway resolves to the one data plane address and
the one
+ // physical stream listen, so their SNI namespaces are shared. The
Gateway
+ // whose listener carries no hostname keeps its route's "*.com"
verbatim -
+ // correctly, and its own subtest depends on it - which then also
answers
+ // "non.matching.com" on the address of the Gateway that should have
rejected
+ // it. Which Gateway a connection was addressed to is not on the wire,
so
+ // there is nothing left to discriminate on. Every other assertion in
this
+ // test passes, including the hostname intersections themselves.
+ tests.TLSRouteHostnameIntersection.ShortName,
Review Comment:
This remains in the skip list, contradicting the PR description's claim that
all four Passthrough tests, including `TLSRouteHostnameIntersection`, are no
longer skipped. Either make the test pass and remove this entry or update the
stated conformance scope.
##########
docs/en/latest/concepts/gateway-api.md:
##########
@@ -87,7 +87,7 @@ The fields below are specified in the Gateway API
specification but are either p
| `spec.listeners[].port` | Partially supported | Controls
`server_port` route-var injection; off by default and enabled via
[`listener_port_match_mode`](../reference/configuration-file.md) (`auto` /
`explicit`). The controller cannot dynamically open data plane ports, so APISIX
must already listen on the specified port. |
| `spec.listeners[].tls.certificateRefs[].group` | Partially supported | Only
`""` is supported; other group values cause validation failure. |
| `spec.listeners[].tls.certificateRefs[].kind` | Partially supported
| Only `Secret` is supported.
|
-| `spec.listeners[].tls.mode` | Partially supported
| `Terminate` is implemented; `Passthrough` is effectively unsupported for
Gateway listeners. |
+| `spec.listeners[].tls.mode` | Partially supported
| `Terminate` and `Passthrough` are both implemented. A `Passthrough` listener
needs APISIX to be listening on that port with
[`tls_passthrough`](https://apisix.apache.org/docs/apisix/stream-proxy/); the
controller cannot open data plane ports itself. A single port cannot mix the
two modes: listeners that disagree on `tls.mode` for one port are reported
`Accepted=False` with `ProtocolConflict`. |
Review Comment:
This says all same-port disagreements are reported as `ProtocolConflict`,
but that check is scoped to listeners within one Gateway. Across Gateways, the
translator instead leaves `tls_passthrough` unset. Document that distinction so
operators do not expect a rejection that never occurs.
--
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]
