Proposal for new Gateway API TCPRoute listener type - #243
Conversation
Signed-off-by: williamhaw <me@williamhaw.com>
scholzj
left a comment
scholzj
left a comment
There was a problem hiding this comment.
Thanks for the proposal. I left some comments. Historically, there was never much interest in the TCP routing because of the complexity of the setup and because most users anyway need to use TLS. So I think we need to carefully think if this is worth it or not.
|
@scholzj thanks for the review comments, I have now simplified the proposal. For additional context, we currently run 16 and counting small clusters (<15 brokers) inside our customers' cloud accounts and expose them behind internal NLBs (only reachable inside the VPC). Clients connect over SASL_PLAINTEXT. Since these Kafka clusters are provisioned in our customers' accounts, it would involve much back-and-forth permission changes to support the DNS record and certificate machinery with |
scholzj
left a comment
scholzj
left a comment
There was a problem hiding this comment.
Thanks. I left some more comments. Sorry, I think in the last review round I might have mislead you about the port configurations.
| A `TLSRoute` is matched by hostname, so all TLS routes can share one gateway listener. | ||
| A `TCPRoute` has nothing to match on, so each route needs its own gateway listener. | ||
| The Gateway API specification is explicit about this: if several `TCPRoute` resources attach to the same listener, all of them are `Accepted` but only the oldest one receives traffic. | ||
| If a `TCPRoute` sets neither `sectionName` nor `port` on its parent reference, it attaches to every TCP listener on the gateway. |
There was a problem hiding this comment.
What if the port it sets is not configured in Gateway/ListenerSet?
| namespace: infra | ||
| bootstrap: | ||
| host: kafka.example.com | ||
| advertisedPortTemplate: "9200 + {nodeId}" |
There was a problem hiding this comment.
Sorry. I might have incorrectly told you that the advertisedPortTemplate is all what is needed. But I don't think so. I think we need to configure it separately - through some port and port template field. Also, we need port configuration for the bootstrap TCPRoute, or?
I'm not sure how to name it. port would be probably too generic. And we want to avoid the name to be too specific such as tcpRoutePort. But we will need something, or?
There was a problem hiding this comment.
I'm thinking of gatewayListenerPort: https://gateway-api.sigs.k8s.io/guides/user-guides/tcp/
There was a problem hiding this comment.
I think we should try to find some neutral name. port might be too confusing as we use port already in the .spec.kafka.listeners. Maybe networkPort? gatewayListenerPort is too specific and cannot be reused. We did that mistake already with nodePort and that is why we cannot reuse that now.
| Those fields configure the hostname in the generated route, and there is no such hostname here. | ||
| Kafka still needs an advertised address, and the broker certificates still need SANs, which is why the existing advertised-address fields are the right ones: | ||
|
|
||
| - `.configuration.bootstrap.host` is required and sets the bootstrap address published to clients, stored in the `Kafka` CR status, and added to the broker certificates. |
There was a problem hiding this comment.
There is nothing like bootstrap address published to clients. I'm not sure how to set the address in the status. But the host field has different semantic. As for SANs, you should use the additional names for that.
There was a problem hiding this comment.
That's a good question. The user will have to get the load balancer address with an out-of-band method. Or if the Gateway has an external-dns annotation they can use that value.
There was a problem hiding this comment.
I guess we can always construct the address for the status from the broker addresses. I don't know. Let's see what other maintainers think.
| - The bootstrap advertised port is the listener `port`. | ||
| Strimzi also uses it as the gateway port on the bootstrap `TCPRoute` parent reference. | ||
| This is the same default as for `type: loadbalancer`. |
There was a problem hiding this comment.
That is not the same. Here, the actual port will likely not be the listener port. As said earlier, we need some field for configuring the ports.
|
Thanks for the comments, I will take some time to refine this proposal. |
|
|
||
| - The cluster uses one gateway, and therefore one cloud load balancer, instead of the N+1 load balancers of `type: loadbalancer`. | ||
| Gateway API implementations back a `Gateway` with a single load balancer that exposes one port per gateway listener. | ||
| Scaling from 3 to 30 brokers adds 27 ports to an existing load balancer instead of provisioning 27 new ones. |
There was a problem hiding this comment.
You make it sound like that happens automatically. But I think you actually need to prepare the ports manually, or?
| | Brokers distinguished by | Hostname (TLS-SNI) | Port | | ||
| | Gateway listeners needed | One, shared by all brokers | One per broker plus one for the bootstrap | | ||
| | DNS records | One per broker, or a wildcard | One, shared | | ||
| | Certificate SANs | One per broker | One, shared | |
There was a problem hiding this comment.
I think typically you would use a wildcard certificate here.
| | Certificate SANs | One per broker | One, shared | | |
| | Certificate SANs | One per broker, or a wildcard | One, shared | |
|
|
||
| - `.configuration.bootstrap.host` is required and sets the bootstrap address published to clients, stored in the `Kafka` CR status, and added to the broker certificates. | ||
| - `.configuration.advertisedHostTemplate` and `.configuration.brokers[].advertisedHost` set the per-broker advertised hosts. | ||
| When neither is configured, the brokers use the bootstrap host, since with `TCPRoute` all brokers are reached through the same gateway address. |
There was a problem hiding this comment.
No, it should definitely not use anything from the bootstrap configuration. It should be required for this listener type and validated in the validation.
| The advertised port and the gateway port are the same value. | ||
| That means this listener cannot advertise a different port than the gateway listens on, which would only matter with port translation in front of the load balancer. | ||
|
|
||
| Unlike `type: tlsroute`, the per-broker advertised ports have no default such as 443, because they have to be distinct from the bootstrap and from each other. |
There was a problem hiding this comment.
They should default to the port used for the TCP Route. So they have a default.
| The implementation also needs a Java model for the `v1` version of the `TCPRoute` API. | ||
| Fabric8 generates its Gateway API model per kind and per API version from a specific Gateway API release, so the `v1` `TLSRoute` support added in Fabric8 7.7.0 does not carry over. | ||
| Fabric8 7.8.0 was released one day before Gateway API 1.6.0, and still pins `sigs.k8s.io/gateway-api` at 1.5.1, where `TCPRoute` exists only as the now-deprecated `v1alpha2`. | ||
| This is being addressed upstream in [fabric8io/kubernetes-client#8032](https://github.com/fabric8io/kubernetes-client/issues/8032) and [fabric8io/kubernetes-client#8033](https://github.com/fabric8io/kubernetes-client/pull/8033), which bump the pin to 1.6.1 and regenerate the model, adding `v1.TCPRoute` and `v1.UDPRoute` while keeping the `v1alpha2` types. | ||
| Once that is released, the implementation needs only a Fabric8 version bump, in the same way the `type: tlsroute` implementation followed the bump to 7.7.0. | ||
| Building on `v1alpha2` instead is rejected, because that version was deprecated in Gateway API 1.6 and will be removed. | ||
| If the Fabric8 release lags, the fallback is for Strimzi to carry the four `TCPRoute` model classes itself, reusing the existing Fabric8 `v1.ParentReference` and `v1.BackendRef` types, since the `v1alpha2` and `v1` schemas are identical. |
There was a problem hiding this comment.
Fabric8 7.9.0 is already out and integrated in Strimzi main. I think it has the new Gateway v1 APIs. But you should probably doublecheck and update this section.
| ### Sharding one listener across multiple gateways | ||
|
|
||
| As described in the limits section, a `type: tcproute` listener is bounded by the number of listeners its gateway supports. | ||
| Spreading the brokers of one listener across several gateways, so that a cluster can exceed that limit, is out of scope. | ||
|
|
||
| The natural unit for such sharding in Strimzi would be the node pool, with different pools attaching to different gateways, and the bootstrap living on any one of them. | ||
| Kafka does not require the bootstrap and the brokers to share an address, so this would work from a protocol point of view. | ||
| It would, however, mean moving part of the listener configuration into the `KafkaNodePool` CR, which is a larger API change than this proposal wants to make, and it should be evaluated on its own merits in a future proposal. | ||
| Until then, clusters that outgrow a single gateway should use a `type: tlsroute` listener. |
There was a problem hiding this comment.
It is worth noting that this can be still done manually by using the cluster-ip listener and managing your own TCPRoutes, right?
2 participants
Type of Change
Select the type of your PR and delete the other items
Description
This proposal introduces a new listener type
tcproutebased on the Gateway API and on TCPRoute and ListenerSet resources. Much inspiration is taken from the existingtlsroutelistener implementation.Checklist
Please go through this checklist and make sure all applicable tasks have been done