search

Kong / kubernetes-ingress-controller

:gorilla: Kong for Kubernetes: The official Ingress Controller for Kubernetes.
https://docs.konghq.com/kubernetes-ingress-controller/
Apache License 2.0
2.21k stars 590 forks source link

Figure out which Kong specific CRD's should require explicit Ingress Class setting #4673

Open programmer04 opened 1 year ago

programmer04 commented 1 year ago

Is there an existing issue for this?

Problem Statement

KIC is not consistent for which objects setting Ingress Class is required. Sometimes it leads to problems, for instance, when multiple instances of KIC (with different Ingress Class configured) run in a single cluster. Docs are not consistent regarding to it.

Furthermore annotation kubernetes.io/ingress.class is considered deprecated. The dedicated field ingressClassName is a replacement for that annotation, consider adding it to Kong CRDs that benefit from having Ingress Class set.

Proposed Solution

Additional information

Since kubernetes.io/ingress.class annotation was never formally defined, but was widely supported by Ingress controllers KIC should still honor it. But being consistent with the new way (that for instance gateway-api or Ingress v1 uses) - dedicated field ingressClassName would be beneficial too.

Acceptance Criteria

rainest commented 11 months ago

We do have https://docs.konghq.com/kubernetes-ingress-controller/latest/concepts/ingress-classes/ describing the general rule of thumb for whether resources require a class.

https://docs.konghq.com/kubernetes-ingress-controller/latest/references/custom-resources/ sorta used to indicate which individual resources required it, but that looks to have been lost in the transition to generated references.

We never had a functional need to change away from the annotation since we don't need to hold anything other than a string value, and were already using it sorta in the way that ingressClassName was supposed to differ:

The newer ingressClassName field on Ingresses is a replacement for that annotation, but is not a direct equivalent. While the annotation was generally used to reference the name of the Ingress controller that should implement the Ingress, the field is a reference to an IngressClass resource that contains additional Ingress configuration, including the name of the Ingress controller.

We had the notion of separate instances of the same controller fairly early on.

GWAPI doesn't have a direct equivalent of the Ingress (or Kong CRD) relationship to an IngressClass. It binds routes to Gateway resources using ParentReferences. Gateways include a gatewayClassName pointing to a GatewayClass, and GatewayClasses include a controllerName.

https://gateway-api.sigs.k8s.io/references/implementers-guide/?h=#gatewayclass suggests that controllerName is more akin to the old per-implementation model, but it looks like we'd have to make which we match configurable to maintain the ability to run multiple instances in the same cluster, as the spec does not allow matching only some GatewayClasses with a matching controllerName.

Kong's implementation of consumers and global KongClusterPlugins (the CRDs that require class information) doesn't let us easily attach them to a Gateway if we load multiple Gateways into a single instance. The resulting Kong resources will apply to any route, not just the routes attached to a particular Gateway. Consumers could maybe achieve this with automatically-generated ACL plugins, but that would prevent users from defining their own ACL plugins.

ParentRefs can attach to non-Gateway resources, but there's nothing obvious for KongConsumers or global plugins to attach to. A GatewayClass would work for the functional goal of limiting them to a controller configured with a matching controllerName, though IDK if we'd use a ParentReference or a gatewayClassName field like Gateways (ParentReferences want a namespace though--they don't have an indicated way to handle references to cluster-scope resources). Neither these nor the status quo really work great in a world where we support either Ingress or GWAPI (and possibly both at once).

Functionally we don't act currently match the CRDs to any resource, since we're just matching the string from the controller environment. Continuing to do so is a viable option if we don't have any expected interaction between them. We could use a vendor annotation instead to resolve the naming confusion.

rainest commented 11 months ago

A thought: we could make the class annotation (or whatever) optional for resources that don't currently require it. While you would still be able to use the current approach of loading by reference, but if the annotation is present, the controller will honor it and skip non-matching classed resources.

This isn't ideal since it does require additional work over inferring ownership from attached resources, but could provide a simpler stopgap for multi-instance clusters than updating all the controllers to track resources via reference, which probably implies moving them out of the controller generator.