-
Notifications
You must be signed in to change notification settings - Fork 590
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
chore: remove KongIngress support for Gateway API Route objects and Services referenced by those Routes #2554
Conversation
d4e6ca8
to
06a0c61
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.
Based on
09:42:36-0700 esenin $ cd src/gateway-api
09:42:41-0700 esenin $ grep -r '"UDPRoute"'
pkg/client/clientset/versioned/typed/apis/v1alpha2/fake/fake_udproute.go:var udproutesKind = schema.GroupVersionKind{Group: "gateway.networking.k8s.io", Version: "v1alpha2", Kind: "UDPRoute"}
it does not appear there's a constant. Is that typically expected? Briefly looking through the Kubernetes core codebase, it looks like they do use the hardcoded string throughout, and that seems okay since any change to the kind name would mean that it's a different kind entirely.
The warning would be good--arguably mandatory if we're breaking the existing contract around them applying to Services always.
It is true that FillOverrides() does not have its own information on what ingresslike kind links to a Service, and that we cannot disable KongIngress use with Gateway Route-attached Services from there as-is. To provide that information we probably need to modify the kongstate Service type to include the parent kind, which we'd set from the various translate functions.
From there we could add cases to continue
in the FillOverrides()
service and upstream loops.
Edit: we'll also want to change
return fmt.Errorf("the following Kubernetes services were all configured together as the backends "+ | |
"for a Kong Service named %s: %+v. These services had disparate KongIngress overrides which is not allowed: "+ | |
" when configuring multiple Kubernetes Services as backends (e.g. backendRefs in HTTPRoutes) it is required "+ | |
" that all of them have matching KongIngress override annotations", *service.Name, k8sServices) |
06a0c61
to
dbbe000
Compare
Alright so I've added some tests for annotations on Gateway API As for the override annotations on Services referenced by So the question here is: |
dbbe000
to
e00aa05
Compare
What specifically would you like to see mentioned here? What other objects can be set as backends to mention them there? |
e00aa05
to
77fd772
Compare
We had to fit multiple backendRefs into our model a bit differently than you might immediately expect based on the names. Kong routes can point to one Kong service and one Kong service only, so we have to fit multiple Kubernetes Services into a single Kong service. We use our services more to indicate connection and request characteristics (connect timeout, upstream protocol, upstream hostname, etc.), and then the upstream endpoint, but that endpoint can take several forms. A Kong service host can indicate either a regular, resolves with Kong's model has the upstream's targets hold the actual individual endpoints/addresses, so to satisfy the HTTPRoute, we create the single Kong service to hold service settings and an upstream whose targets are the collection of endpoints from each Kubernetes service (or each Kubernetes service hostname with service-upstream). If you wanted to see the multiple Kubernetes Services represented, you'd want to look at https://docs.konghq.com/gateway/2.8.x/admin-api/#list-targets in the admin API. The KongIngress (and annotation) overrides apply to the single Kong service and single Kong upstream we create for the route (targets have no tunables other than the per-target weight). There's no way for us to handle multiple Kubernetes Services within a BackendRef having different overrides because we create only one Kong upstream and service for the entire BackendRef, so we enforce all Services having the same overrides. |
Specifically I was looking at removing the now irrelevant KongIngress mention and indicating that it's any of our annotations. May as well try and simplify the prose in general and change the info about where this matters:
This issue actually only affects Gateway Routes (not an official term, looking for consensus in SIG chat); we wrote it for a potential future where we may have other types of config that can use multiple Services. To reduce noisiness we'd want to use a Condition on the object itself. There isn't an appropriate standard one, but we can add our own RouteConditionReason false Accepted Condition. However, because this fails immediately without unblocking the build (see below), we'll only mark one problem Route at a time. Note that the parser cannot set resource status information directly: only the controller reconcilers can do so. To communicate information back from the parser to the controllers, other controllers use a StatusQueue watch that reads address status information. As best I can tell, the ones we use for Ingress and such read static information, and the parser queue event only triggers that read (@shaneutt can explain this system better), so a Condition setter that'd be able to handle dynamic info from the parser would be fairly different. Since we don't have a drop-in solution, it may make sense to defer that til we can handle it for more than one Route at a time. And now, scope creep! IMO that language may be a bit confusing if you're looking at the error and trying to figure out what generated it, and while my version narrows the scope, it doesn't really help you find the precise object--the user will want to look at the specific broken HTTPRoute or whatever, not scan through all Gateway Routes. This is separate since it's not truly needed--we already provide the Services, and that's all you need to fix, but it's probably useful to know what's using them rather than changing them blindly--you won't necessarily know what annotations are appropriate to add or remove without knowing what they're being used from: #2566 To indicate that we probably actually want to populate an additional
Finally the way we handle this error is maybe not great. Prior to adding that error, The actual end state we probably want to reach is that those errors just invalidate the one Gateway Route and log as much (#2565). Failing piecemeal would then let us just log each, and bonus points use structured logs correctly:
|
Thanks for those extensive comments and reporting those other issues. That helps a lot! Now in order to minimize the scope of this PR - since we already have those other issue that you've reported and which are pretty specific - what do you suggest we cover in this PR from the issues that you've enlisted? I reckon we need to set the Condition on the - if I understood correctly - Route object? Yet you mentioned as well that
In which case: what would you suggest to cover in this PR :)? I believe adding the parent obj to the log via an addition of |
For this PR, I'd recommend just changing the message to be about annotations instead of KongIngress, so the
The conditions would be good and more aligned with the wants of the Gateway spec, but given the need to build out a new system to shuttle them to the reconciler and the limitation around marking only one route, more work than I think we want here for limited benefit. I've edited #2566 to mention that, since it's what will enable setting conditions for all problem Routes, not just the first one the parser encounters. |
77fd772
to
367030f
Compare
Thanks! Changed the error msg. PTAL. |
367030f
to
705ca33
Compare
Removed the switches (as it makes sense after realizing that only *Route object will/should trigger those code sections) and changed the wording of logs. Thanks! And PTAL @rainest |
What this PR does / why we need it:
In order to prevent users on relying on
KongIngress
annotation overrides this PR disables the ability to do so for:Route
objectsService
s defined as Gateway API *Route
's backendsWhich issue this PR fixes:
fixes #2505
Special notes for your reviewer:
N/A
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