chore: remove KongIngress support for Gateway API Route objects and Services referenced by those Routes

[pmalek]

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:

• Gateway API *Route objects
• kubernetes Services defined as Gateway API *Route's backends

Which 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:

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

[rainest]

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

kubernetes-ingress-controller/internal/dataplane/parser/ingressrules.go

Lines 58 to 61 in 52c0e00

	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)

to refer to annotations in general--it should have said that already, since it always applied to any annotation, not just override and the attached KongIngress settings.

[pmalek]

Alright so I've added some tests for annotations on Gateway API *Routes and added a warning when that happens.

As for the override annotations on Services referenced by *Routes: it seems that we now have (unreleased yet) support for multiple backend refs in *Routes which is stored/treated(?) on the same level as kong.Service.
So my understanding is that there is a 1 to n mapping between a kong.Service and backendRefs.
These 2 fields in turn are then stored in kongstate.Service which gets its overrides in FillOverrides().

So the question here is:
how does one map the overrides that can happen for any Service referenced by a *Route object (via backendRefs), where the overrides are applied on the kongstate.Service (if I read it right this maps 1-1 to kong.Service, which is what ends up in Admin API)?

[pmalek]

we'll also want to change

kubernetes-ingress-controller/internal/dataplane/parser/ingressrules.go

Lines 58 to 61 in 52c0e00

	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)

to refer to annotations in general--it should have said that already, since it always applied to any annotation, not just override and the attached KongIngress settings.

What specifically would you like to see mentioned here? What other objects can be set as backends to mention them there?

[rainest]

So my understanding is that there is a 1 to n mapping between a kong.Service and backendRefs
[With an HTTPRoute that has two Services in its BackendRef] I get just 1 Service, 1 Upstream and 1 Route in Admin API. What am I missing here?

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 dig example.com DNS hostname or a virtual DNS hostname that only Kong can resolve internally. These virtual entries are Kong upstreams, and our DNS "resolution" for them attempts to map service.host -> upstream.name. If there is an upstream whose name matches the service host, Kong's DNS resolver (the "balancer") will return the upstream's targets instead of the DNS A records you'd get from actual DNS resolution. The balancer is also responsible for load balancing (upstream.algorithm, upstream.hash_on, and target.weight) and healthchecking (upstream.healthchecks), so within Kong it only returns a single address to the router, a healthy address chosen based on the LB algorithm, so we set LB/target weights to implement BackendRef.weight.

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.

[rainest]

What specifically would you like to see mentioned here? What other objects can be set as backends to mention them there?

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:

return fmt.Errorf("the Kubernetes Services %v cannot have different sets of konghq.com annotations" +
"These Services are used in the same Gateway Route BackendRef together to create the Kong Service %s" +
"and must use the same Kong annotations", k8sServices, *service.Name)

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 ingressRules.ServiceNameToParent map that points to a GVK and NamespacedName so that we can:

return fmt.Errorf("the Kubernetes Services %v cannot have different sets of konghq.com annotations" +
"These Services are used in %s together to create the Kong Service %s" +
"and must use the same Kong annotations",
k8sServices, <some string representation of the ingressRules.ServiceNameToParent>, *service.Name)

Finally the way we handle this error is maybe not great. Prior to adding that error, populateServices() always succeeded. If it now fails, that failure bubbles up to the parser build and indicates that it failed completely, and the parser build bubbles that up saying that it also failed completely, invalidating the entire config. That's a bit aggressive for something that only really breaks one Gateway Route--we could proceed with the others fine despite one having invalid Service configuration, we'd just need to skip the broken one (and probably lack the ability to do that easily).

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:

log.WithFields(logrus.Fields{
"parent_gvk": <the parent GVK>,
"parent_namespace": <the parent namespace>,
"parent_name": <the parent name>,
"kong_service": *service.Name,
"kubernetes_service": <CSV of Service namespaced names>, // is there a good way to handle variable arity fields with logrus? it's a map key, so you can't duplicate :(
}).Errorf("services used within a single backend have different konghq.com annotations")

[pmalek]

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

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.

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 ingressRules.ServiceNameToParent should be relatively easy (as this PR was meant to be 🙃 )

[rainest]

For this PR, I'd recommend just changing the message to be about annotations instead of KongIngress, so the

return fmt.Errorf("the Kubernetes Services %v cannot have different sets of konghq.com annotations" +
"These Services are used in the same Gateway Route BackendRef together to create the Kong Service %s" +
"and must use the same Kong annotations", k8sServices, *service.Name)

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.

[pmalek]

Thanks! Changed the error msg. PTAL.

[pmalek]

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