-
Notifications
You must be signed in to change notification settings - Fork 594
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
Implement Gateaway Listener allowedRoutes.namespaces #2389
Conversation
40042a9
to
9640a39
Compare
9640a39
to
aa1aa5f
Compare
Implement the allowedRoutes.RouteNamespaces filters, which instruct listens to only accept routes in a specific set of namespaces.
Remove --watch-namespaces from the test arguments, to allow tests that create additional namespaces beyond their standard test namespace.
Merge the allowedRoutes configuration for Gateway listeners into the generated managed Gateway listeners. If a user listen has the same protocol as a generated listener, add the user listener's allowedRoutes section into the generated listener.
When looking for a supported gateway, only check the listeners whose protocol matches the route type.
92defd0
to
ef2fb87
Compare
Check that AllowedRoutes from user-provided Listeners meet the criteria necessary for Kong: Listeners for the same protocol must not specify different filter requirements because Kong combines all routes into a single proxy configuration.
ef2fb87
to
ec9b0c1
Compare
Per discussion with Shane we'll go with the merge approach with validation to ensure we can actually merge and honor the user intent (in ec9b0c1) within the constraints of what Kong configuration can handle. Listens of the same protocol must use the same filter, because they aren't actually separated by port: Kong serves all routes for a given protocol on any listen that handles that protocol; it cannot serve a subset of routes based on the proxy destination port. |
Co-authored-by: Shane Utt <shaneutt@linux.com>
e335215
to
d1e789a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving as I have no further comments (but there are a few remaining unresolved comments that should be resolved before merging).
Licenses differ between commit 9c08059 and base:
|
Added minor comment updates to clarify TODOs and link to issues before merging this. |
What this PR does / why we need it:
Implements the https://gateway-api.sigs.k8s.io/v1alpha2/references/spec/#gateway.networking.k8s.io/v1alpha2.RouteNamespaces filter. This allows a Gateway Listener to filter the routes it will accept by namespace (any namespace, same namespace as Gateway, or namespaces identified by a selector).
Merges allowedRoutes section from user Listener sections into the generated managed Gateway Listeners. An allowedRoute in a user Listener whose procotol matches a generated Listener will be copied into that generated Listener.
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close that issue when PR gets merged): fixes #2080. Some tasks were moved to separate issues.I move to separate the BackendRef namespace filtering (see https://gateway-api.sigs.k8s.io/v1alpha2/references/spec/#gateway.networking.k8s.io/v1alpha2.BackendObjectReference) into its own issue, as it works with a completely different set of objects (BackendRefs and ReferencePolicys versus Gateways and XRoutes) in a different part of the code (mostly in the parser translate functions, not in the Gateway controller).
Special notes for your reviewer:
Test changes
Removed
--watch-namespaces
from integration without a replacement. Tests for this require creating a custom test namespace, which wouldn't be watched because it didn't exist at start time. We could create an E2E test for this, but it doesn't seem particularly worth the extra time cost: we just stuff the contents of a flag into a controller-runtime config option, so regression on our side is unlikely.Managed Gateway throws a wrench in this
From initial review, go with the merge approach, with some additional validation.
Dealing with config that lives inside a Listener is a difficult problem for our managed gateway approach. Currently, we ignore the original Listener configuration altogether and infer the entire correct Listener set from the proxy Service/Deployment, which would clobber any
allowedRoutes
section a user created. There are a couple options I can think of for handling this, all bad:r.determineListenersFromDataPlane()
on creation. Allow free manipulation of Listeners after a Gateway has passed through our initial managed Gateway mutation. If you wish to addallowedRoutes
, you must wait until we populate Listeners and edit them in after. Simple, but I'm unsure if there's anything that relies on our currently updating Listeners on every change to the Gateway.allowedRoutes
out of the user Listener and stuff it into the generated Listener before discarding the original user Listener. This works for the test example chosen, but feels like it opens a potential world of pain for cases that the merger can't handle correctly for some reason. It's also semi-hidden weird vendor behavior, which is not great.allowedRoutes
configuration. Didn't explore much since you can't annotate individual ports, so this would presumably be limited to using the same thing across a Service.No kinds implementation
Per initial review with Shane, there's no immediate use for this. Deferred to #2408 whenever we add a route type that warrants it.
AllowedRoutes provides both
namespaces
andkinds
. The latter lets you filter by Group+Kind combinations and is not implemented here (no problem! #2080 didn't ask for it!). I wasn't quite sure what the intended use of this was: it seems like you wouldn't be able to do much with it, since when would your UDP listener handle anything other thangateway.networking.k8s.io/UDPRoute
and/or why would you want to write a filter that says it can't? If I write a condition saying that my UDP listen allowsgateway.networking.k8s.io/HTTPRoute
, or get really crazy and say it allowsconfiguration.konghq.com/KongConsumer
, what the heck does that even mean? It won't be able to do anything with them after.Was this maybe intended for later, when you have say, an HTTPS listen that you only want to allow a hypothetical GRPCRoute through, not HTTPRoute? If that's indeed the case, do we know of anything that'd make use of this currently, and if not should we defer this?
The draft does de facto implement this sort of, albeit forcibly: it skips Listens whose protocols don't match the route when finding a matching Gateway, as that's necessary to ensure that you don't ingest an HTTPRoute on a Gateway that has an HTTP Listen that filters out its namespace, but also has a TCP Listen that allows any namespace.
PR Readiness Checklist:
Complete these before marking the PR as
ready to review
:CHANGELOG.md
release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR