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

Implement Gateaway Listener allowedRoutes.namespaces #2389

Merged
merged 7 commits into from
Apr 18, 2022

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Apr 4, 2022

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:

  • Only call r.determineListenersFromDataPlane() on creation. Allow free manipulation of Listeners after a Gateway has passed through our initial managed Gateway mutation. If you wish to add allowedRoutes, 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.
  • Attempt to "merge" as shown in this initial draft. Choose some criteria (here, protocol) used to match user Listeners to a generated Listener, then cut the 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.
  • Annotate something we use to build the generated Listener to indicate the appropriate 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 and kinds. 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 than gateway.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 allows gateway.networking.k8s.io/HTTPRoute, or get really crazy and say it allows configuration.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:

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

@rainest rainest temporarily deployed to Configure ci April 4, 2022 22:52 Inactive
@rainest rainest temporarily deployed to Configure ci April 4, 2022 23:17 Inactive
@rainest rainest temporarily deployed to Configure ci April 4, 2022 23:54 Inactive
@rainest rainest temporarily deployed to Configure ci April 4, 2022 23:54 Inactive
@rainest rainest temporarily deployed to Configure ci April 5, 2022 00:11 Inactive
@rainest rainest temporarily deployed to Configure ci April 8, 2022 22:54 Inactive
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.
@rainest rainest temporarily deployed to Configure ci April 11, 2022 20:26 Inactive
@rainest rainest marked this pull request as ready for review April 11, 2022 20:26
@rainest rainest requested a review from a team as a code owner April 11, 2022 20:26
@rainest rainest temporarily deployed to Configure ci April 11, 2022 20:26 Inactive
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.
@rainest rainest temporarily deployed to Configure ci April 11, 2022 20:32 Inactive
@rainest rainest temporarily deployed to Configure ci April 11, 2022 20:33 Inactive
@rainest
Copy link
Contributor Author

rainest commented Apr 11, 2022

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.

@rainest rainest temporarily deployed to Configure ci April 11, 2022 20:52 Inactive
@shaneutt shaneutt self-assigned this Apr 12, 2022
CHANGELOG.md Show resolved Hide resolved
internal/controllers/gateway/gateway_controller.go Outdated Show resolved Hide resolved
internal/controllers/gateway/gateway_controller.go Outdated Show resolved Hide resolved
internal/controllers/gateway/gateway_controller.go Outdated Show resolved Hide resolved
test/integration/gateway_test.go Outdated Show resolved Hide resolved
test/integration/gateway_test.go Outdated Show resolved Hide resolved
test/integration/gateway_test.go Outdated Show resolved Hide resolved
test/integration/gateway_test.go Outdated Show resolved Hide resolved
test/integration/gateway_test.go Outdated Show resolved Hide resolved
@rainest rainest temporarily deployed to Configure ci April 15, 2022 18:05 Inactive
Co-authored-by: Shane Utt <shaneutt@linux.com>
@rainest rainest temporarily deployed to Configure ci April 15, 2022 18:35 Inactive
@rainest rainest temporarily deployed to Configure ci April 15, 2022 18:35 Inactive
@rainest rainest temporarily deployed to Configure ci April 15, 2022 18:53 Inactive
@shaneutt shaneutt self-requested a review April 18, 2022 18:08
shaneutt
shaneutt previously approved these changes Apr 18, 2022
Copy link
Contributor

@shaneutt shaneutt left a 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).

@rainest rainest temporarily deployed to Configure ci April 18, 2022 18:42 Inactive
@rainest rainest temporarily deployed to Configure ci April 18, 2022 18:42 Inactive
@rainest rainest enabled auto-merge (squash) April 18, 2022 18:43
@rainest rainest requested a review from shaneutt April 18, 2022 18:44
@github-actions
Copy link

Licenses differ between commit 9c08059 and base:

+++ pr_licenses.csv	2022-04-18 18:44:35.335112165 +0000
@@ -11,7 +11,7 @@
 github.com/containerd/containerd,https://github.com/containerd/containerd/blob/v1.6.1/LICENSE,Apache-2.0
 github.com/davecgh/go-spew/spew,https://github.com/davecgh/go-spew/blob/v1.1.1/LICENSE,ISC
 github.com/docker/distribution,https://github.com/docker/distribution/blob/v2.8.0/LICENSE,Apache-2.0
-github.com/docker/docker,https://github.com/docker/docker/blob/v20.10.14/LICENSE,Apache-2.0
+github.com/docker/docker,https://github.com/docker/docker/blob/v20.10.13/LICENSE,Apache-2.0
 github.com/docker/go-connections,https://github.com/docker/go-connections/blob/v0.4.0/LICENSE,Apache-2.0
 github.com/docker/go-units,https://github.com/docker/go-units/blob/v0.4.0/LICENSE,Apache-2.0
 github.com/evanphx/json-patch,https://github.com/evanphx/json-patch/blob/v4.12.0/LICENSE,BSD-3-Clause```

@rainest
Copy link
Contributor Author

rainest commented Apr 18, 2022

Added minor comment updates to clarify TODOs and link to issues before merging this.

@rainest rainest temporarily deployed to Configure ci April 18, 2022 19:04 Inactive
@rainest rainest merged commit 0d52516 into main Apr 18, 2022
@rainest rainest deleted the feat/gateway-namespace branch April 18, 2022 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gateway Controller Namespacing
2 participants