Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(gateway) support UDP services #3832

Merged
merged 6 commits into from
Apr 5, 2023
Merged

feat(gateway) support UDP services #3832

merged 6 commits into from
Apr 5, 2023

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Mar 30, 2023

What this PR does / why we need it:

Add Gateway controller support for a separate UDP publish service in addition to the existing TCP/TLS/HTTP publish service. Gateways can now properly validate UDP Listeners and indicate if they are ready or not ready due to the presence or absence of corresponding Kong Service and listen configuration.

Remove the UDPRoute blanket accept. Previously, the UDPRoute controller overrode the parentRef validity checks and accepted any UDPRoute that indicated a matched Gateway as its parent. The controller now requires that the indicated parent corresponds to a matching, ready Listener.

Expand UDPRoute test to confirm that a mismatched parent port excludes the UDPRoute from configuration.

Which issue this PR fixes:

Fix #3237

Special notes for your reviewer:

This change may break some currently active UDPRoutes. Those routes were never spec-compliant, but were accepted because we lacked the means to check their validity. We would previously accept:

  • UDPRoutes that used a port not exposed in Kong's listen/Service configuration. These shouldn't really matter, because they would have been impossible to use.
  • UDPRoutes that matched a Kong listen that had no accompanying Gateway Listener. These routes would have been usable.

#2544 missed a rather important bit for unblocking #3237: adding the ability to set a UDP publish service doesn't do much if the Gateway controller is entirely unaware of it. As such, this PR goes beyond just modifying UDPRoute.

I found a minor oddity while writing this: #3832 (comment)

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

@rainest rainest requested a review from a team as a code owner March 30, 2023 21:59
@rainest rainest added the area/gateway-api Relating to upstream Kubernetes SIG Networking Gateway API label Mar 30, 2023
@rainest rainest force-pushed the feat/udp-port-matching branch 2 times, most recently from ee855b0 to 9a2b9a3 Compare March 30, 2023 22:15
@rainest
Copy link
Contributor Author

rainest commented Mar 30, 2023

Though not directly related to the task at hand, I did encounter an oddity while writing this. We use the gatewayclass-unmanaged annotation on both GatewayClasses and Gateways, differently.

GatewayClasses use a simple indication that they are unmanaged:

konghq.com/gatewayclass-unmanaged: true

This appears to be the original intent of this annotation, given the name and it making a boolean statement.

Gateways reuse this to indicate their publish service. The controller adds this annotation once it has associated them with a GatewayClass and sees that they don't currently have the annotation:

konghq.com/gatewayclass-unmanaged: kong-system/ingress-controller-kong-proxy,kong-system/ingress-controller-kong-udp-proxy

Prior to this PR they would only contain a single service. After they contain a CSV with multiple.

Any idea why we've overloaded it like this? It seems like the Gateways should have a separate annotation, or have no annotation at all (it's not clear they really need this, since the controller has information about those publish services available internally).

The reuse caused some annoyance parsing this (I originally handled the CSV in the annotation's utility functions, which broke the GatewayClass usage), but outside changes here it appears benign, if weird, so I left it alone after I fixed my parsing.

@codecov
Copy link

codecov bot commented Mar 30, 2023

Codecov Report

Patch coverage: 2.9% and project coverage change: -0.1 ⚠️

Comparison is base (8690e86) 59.1% compared to head (d8b573b) 59.0%.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #3832     +/-   ##
=======================================
- Coverage   59.1%   59.0%   -0.1%     
=======================================
  Files        138     138             
  Lines      15912   15949     +37     
=======================================
+ Hits        9417    9423      +6     
- Misses      5856    5887     +31     
  Partials     639     639             
Impacted Files Coverage Δ
internal/controllers/gateway/gateway_utils.go 44.1% <ø> (ø)
...nternal/controllers/gateway/udproute_controller.go 8.6% <0.0%> (-0.6%) ⬇️
internal/controllers/gateway/gateway_controller.go 11.4% <2.2%> (-0.3%) ⬇️
internal/manager/controllerdef.go 98.7% <100.0%> (+<0.1%) ⬆️

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@czeslavo czeslavo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the comment on the annotation, I think we can take it asynchronously to this PR.

test/integration/gateway_test.go Outdated Show resolved Hide resolved
@rainest
Copy link
Contributor Author

rainest commented Mar 31, 2023

Additional force push, apologies. My pseudo-IDE usually adds stdlib packages automatically when requested somewhere in code, but apparently that breaks when there are build tags. Curse you vim-go and your many oddities.

@rainest rainest requested a review from czeslavo March 31, 2023 16:54
@rainest rainest force-pushed the feat/udp-port-matching branch 2 times, most recently from 938d91b to 8eaf1b6 Compare April 1, 2023 00:35
Add Gateway controller support for a separate UDP publish service in
addition to the existing TCP/TLS/HTTP publish service. Gateways can now
properly validate UDP Listeners and indicate if they are ready or not
ready due to the presence or absence of corresponding Kong Service and
listen configuration.

Remove the UDPRoute blanket accept. Previously, the UDPRoute controller
overrode the parentRef validity checks and accepted any UDPRoute that
indicated a matched Gateway as its parent. The controller now requires
that the indicated parent corresponds to a matching, ready Listener.

Expand UDPRoute test to confirm that a mismatched parent port excludes
the UDPRoute from configuration.
Use a different name for the second UDPRoute test container. The
Deployment generator uses the container name for the app label value, so
using the same name for two makes the associated Services point to
either Deployment.

This results in a duplicate set of targets, which Kong will reject in
database mode.
@rainest
Copy link
Contributor Author

rainest commented Apr 3, 2023

Force push to drop the extra debug logging after finding the issue causing the Postgres failures.

The original last step of the UDPRoute test had always been broken, and adding this additional step after surfaced the failure.

Both of the test containers used the same name, causing them to use the same app label (our Deployment generator uses the container name as the app label). This associated both Deployments' endpoints with both Services.

Adding the second Service into the route backend thus added both endpoints twice, which broke configuration due to the duplicate targets. While KIC was never able to apply this broken configuration, the test validation after still passed, as the original config was already sending traffic to both Deployments.

@rainest rainest requested a review from pmalek April 3, 2023 23:38
@pmalek
Copy link
Member

pmalek commented Apr 4, 2023

Though not directly related to the task at hand, I did encounter an oddity while writing this. We use the gatewayclass-unmanaged annotation on both GatewayClasses and Gateways, differently.

GatewayClasses use a simple indication that they are unmanaged:

konghq.com/gatewayclass-unmanaged: true

This appears to be the original intent of this annotation, given the name and it making a boolean statement.

Gateways reuse this to indicate their publish service. The controller adds this annotation once it has associated them with a GatewayClass and sees that they don't currently have the annotation:

konghq.com/gatewayclass-unmanaged: kong-system/ingress-controller-kong-proxy,kong-system/ingress-controller-kong-udp-proxy

Prior to this PR they would only contain a single service. After they contain a CSV with multiple.

Any idea why we've overloaded it like this? It seems like the Gateways should have a separate annotation, or have no annotation at all (it's not clear they really need this, since the controller has information about those publish services available internally).

The reuse caused some annoyance parsing this (I originally handled the CSV in the annotation's utility functions, which broke the GatewayClass usage), but outside changes here it appears benign, if weird, so I left it alone after I fixed my parsing.

It feels to me that this might be an artifact of early phases of Gateway API support. I suggest (as I feel is the intent behind your comment as well) that we change Gateway publish service annotation sooner rather than later. That is: let's create an issue for this and try to tackle it in the upcoming (?) release. WDYT @rainest ?

@pmalek pmalek added this to the KIC v2.10.0 milestone Apr 4, 2023
@rainest rainest requested a review from pmalek April 4, 2023 18:50
Plumb a bunch of contexts to callback generators to avoid their use of
context.Background().

Ironically, all the plumbed contexts are also context.Background().
@pull-request-size pull-request-size bot added size/XL and removed size/L labels Apr 4, 2023
@rainest rainest merged commit 7ae5aeb into main Apr 5, 2023
@rainest rainest deleted the feat/udp-port-matching branch April 5, 2023 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/gateway-api Relating to upstream Kubernetes SIG Networking Gateway API size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GEP-957: Gateway port matching for Gateway API UDPRoute
3 participants