UDPRoute Support

[shaneutt]

Problem Statement

The purpose of this issue is to add support for UDPRoute from Gateway API.

Proposed Solution

• add a controller for UDPRoute (use HTTPRoute as general reference)
• add pkg/parser/translate.go data-plane translation support for UDPRoute
• add integration tests which cover all options

Acceptance Criteria

• as a user I can use UDPRoute objects with KIC

[rainest]

Some questions from implementation:

• Offhand, I can't think of anything that will differ in the controller other than the type--I just did an s/HTTPRoute/UDPRoute/g on a copy of httproute_controller.go to make udproute_controller.go. AFAIK while they have the same Gateway status links that differentiate them from other controllers, they don't have anything protocol-specific. Do we want to make another generator for the Gateway *Route controllers?
• At present, we add all Gateway controllers based on the feature gate alone. We have none of the standard flags to enable/disable controllers. Do we want to add these (presumably as no-ops if the feature gate is off) yet? Disabling individual controllers isn't something I expect is used often outside permission-limited instances, and it's probably fine to just leave them as a set, but it's a bit inconsistent now that the Gateway feature flag encompass more than the minimal set needed to support HTTPRoute only.
• Store tests for Ingresses filter by class. Should we test this for Gateway routes? Probably no--for those I think we only filter at the controller level (the store isn't aware of Gateways, so it can't check the parentRefs). More indication that we should just jettison the store filters.
• Our function names for HTTPRoute (and, following the same pattern, UDPRoute) don't mention the version. We've gotten away with this for most resources because we only handle one version. We're almost certainly going to add a version after v1alpha2. I wonder if we couldn't make this cleaner with interfaces and types (with a type per version) for the various things we translate and parse (for Ingress-likes, they pretty much all do the same "list", "translate into rules", etc. things) rather than stuffing the version in the function name.

[shaneutt]

Offhand, I can't think of anything that will differ in the controller other than the type--I just did an s/HTTPRoute/UDPRoute/g on a copy of httproute_controller.go to make udproute_controller.go. AFAIK while they have the same Gateway status links that differentiate them from other controllers, they don't have anything protocol-specific. Do we want to make another generator for the Gateway *Route controllers?

It sounds like we have almost no permutations so that makes me kinda want to avoid generators, but given that we have to deal a lot with identical object attributes I think creating a generator for this for now will be fine. This is the way we've done it historically and it will be fast since it's basically just going to be a copy/paste and some templating. We should probably be looking at alternative solutions down the road.

At present, we add all Gateway controllers based on the feature gate alone. We have none of the standard flags to enable/disable controllers. Do we want to add these (presumably as no-ops if the feature gate is off) yet? Disabling individual controllers isn't something I expect is used often outside permission-limited instances, and it's probably fine to just leave them as a set, but it's a bit inconsistent now that the Gateway feature flag encompass more than the minimal set needed to support HTTPRoute only.

My position is that we keep this all behind the feature gate for the moment and just leave it as an all-on or all-off kind of situation until we're ready to graduate the project to beta status, primarily because this keeps things simple for now and the cost of an idle controller is negligible.

Store tests for Ingresses filter by class. Should we test this for Gateway routes? Probably no--for those I think we only filter at the controller level (the store isn't aware of Gateways, so it can't check the parentRefs). More indication that we should just jettison the store filters.

Agreed.

[rainest]

It sounds like we have almost no permutations so that makes me kinda want to avoid generators

We have a lot of type-specific code (because they're FooReconciler methods) for GWAPI routes around linking them to Gateway resources that may potentially need to change, and the goal of the template would be to avoid duplicating those changes across each method manually. The find/replace approach works fine for initially creating the controller, but less so for modifying like methods after.

More questions after finishing a viable, if limited implementation in #2363:

1. UDPRouteRule has a Matches field, but it's experimental, so it was only added to the experimental CRD channel. We currently use the stable channel in tests. Should we switch to experimental and start implementing those immediately, or wait for a new API version to include a stable version of them?
2. Absent Matches, UDPRouteRule includes only a BackendRef, and those only include the target port of the upstream Service. This appears to indicate that, at present, UDPRouteRules should only match traffic inbound to the upstream Service's target port: PNAT, though supported (and usually used) in L4 Kong routes, is not supported in UDPRoute. Digging around indicates that supporting this was under discussion, but address matching was implemented first, and port matching was deferred til later, ultimately getting handled as part of the route ParentRef. As this applies across the route, we're probably de facto limited to a single rule per route unless they furthermore specify destination address matches.
3. Though not yet implemented, neither AddressRouteMatches documentation nor the related GEP indicate relationships between matches as I read them. I'd expect that it's an AND of the source/destination matches, and then an OR of the AddressMatches inside, but we'd need to confirm and update upstream as such.
4. On a related note, should an IPAddress AddressMatch support CIDR? I'd expect it to, and it's a raw string, so we can get that for free with Kong's implementation of route.sources/destinations, but the examples don't include any.

Non-questions:

1. It looks like there are some non-template DRY opportunities to share code between all route types. However, the most obvious one is around BackendRef handling, and I think it's best to defer that til after HTTPRoute - Multiple BackendRefs Support #2166 even if they probably don't absolutely require it (they create the Service rather than its upstreams). For now, I just copy-pasted and modified the HTTPRoute functions.
2. If we do switch to experimental, I'm not sure how we handle AddressMatch tests, as our test environment does not use static addresses.
3. ParentRef port handling (which I didn't implement yet because I only learned about it late in the day) de jure applies to all route types, though I don't think Kong's implementation allows this--pretty sure you can't use destinations for HTTP routes (maybe you should be able to, it'd solve the long-standing problem that it's impossible to group routes by listener).
4. The NamedAddress AddressMatch type could kinda sorta let us implement per-rule port matches, insofar as it's technically a vendor-specific spooky zone, but it arguably isn't intended for this purpose.

[shaneutt]

UDPRouteRule has a Matches field, but it's experimental, so it was only added to the experimental CRD channel. We currently use the stable channel in tests. Should we switch to experimental and start implementing those immediately, or wait for a new API version to include a stable version of them?

I feel strongly we should only be implementing stable unless an end-user specifically requests experimental features. This will help us reduce time waste for experimental features which are significantly changed, or removed entirely in upcoming releases.

Absent Matches, UDPRouteRule includes only a BackendRef, and those only include the target port of the upstream Service. This appears to indicate that, at present, UDPRouteRules should only match traffic inbound to the upstream Service's target port: PNAT, though supported (and usually used) in L4 Kong routes, is not supported in UDPRoute. Digging around indicates that supporting this was under discussion, but address matching was implemented first, and port matching was deferred til later, ultimately getting handled as part of the route ParentRef. As this applies across the route, we're probably de facto limited to a single rule per route unless they furthermore specify destination address matches.

✔️

Though not yet implemented, neither AddressRouteMatches documentation nor the related GEP indicate relationships between matches as I read them. I'd expect that it's an AND of the source/destination matches, and then an OR of the AddressMatches inside, but we'd need to confirm and update upstream as such.

Yes we need to make this more clear. As I last remember the intention was an OR of AddressRouteMatches (which is somewhat obvious), an OR of the AddressMatches, and then as you said Source AND Destination therein.

On a related note, should an IPAddress AddressMatch support CIDR? I'd expect it to, and it's a raw string, so we can get that for free with Kong's implementation of route.sources/destinations, but the examples don't include any.

https://github.com/kubernetes-sigs/gateway-api/blob/master/site-src/geps/gep-735.md#cidr-addresstype

It looks like there are some non-template DRY opportunities to share code between all route types. However, the most obvious one is around BackendRef handling, and I think it's best to defer that til after HTTPRoute - Multiple BackendRefs Support #2166 even if they probably don't absolutely require it (they create the Service rather than its upstreams). For now, I just copy-pasted and modified the HTTPRoute functions.

Sounds good.

If we do switch to experimental, I'm not sure how we handle AddressMatch tests, as our test environment does not use static addresses.

I would imagine that for source traffic these will all come from the container bridge network and so the IP will be predictable. For destination it's kinda fuzzy when that's even going to be useful but we'll just need to dig around a bit and we should be able to dynamically list all the destination IPs for any particular UDP traffic.

ParentRef port handling (which I didn't implement yet because I only learned about it late in the day) de jure applies to all route types, though I don't think Kong's implementation allows this--pretty sure you can't use destinations for HTTP routes (maybe you should be able to, it'd solve the long-standing problem that it's impossible to group routes by listener).

🤔

The NamedAddress AddressMatch type could kinda sorta let us implement per-rule port matches, insofar as it's technically a vendor-specific spooky zone, but it arguably isn't intended for this purpose.

🤔

[rainest]

Hey, go figure, ParentRef.Port is also experimental, so as of yet, there is no means of choosing the proxy port. Did a pass at parentRef before realizing this, so parent.diff.txt works but only if you use the experimental CRDs.