Kong chart: scope IngressClass controller name by ingress class - #1519
Kong chart: scope IngressClass controller name by ingress class#1519vkobets wants to merge 3 commits into
Conversation
IngressClass.spec.controller stayed fixed to ingress-controllers.konghq.com/kong even when ingressController.ingressClass was different. Set spec.controller and CONTROLLER_GATEWAY_API_CONTROLLER_NAME from ingressController.ingressClass so the controller matches that class.
There was a problem hiding this comment.
Pull request overview
This PR updates the Helm chart’s controller identity wiring so that multi-instance Kong Ingress Controller deployments can use distinct identities per ingressController.ingressClass, improving correctness for both IngressClass-based routing and Gateway API reconciliation.
Changes:
- Set the chart-managed
IngressClass.spec.controllertoingress-controllers.konghq.com/{{ .Values.ingressController.ingressClass }}. - Add
CONTROLLER_GATEWAY_API_CONTROLLER_NAMEenv var derived fromingressController.ingressClass. - Document the change under the CHANGELOG “Unreleased” section.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| charts/kong/templates/ingress-class.yaml | Makes the managed IngressClass controller identifier depend on the configured ingress class. |
| charts/kong/templates/_helpers.tpl | Injects a Gateway API controller name env var based on the ingress class. |
| charts/kong/CHANGELOG.md | Adds an Unreleased note describing the alignment behavior change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| spec: | ||
| controller: ingress-controllers.konghq.com/kong | ||
| controller: ingress-controllers.konghq.com/{{ .Values.ingressController.ingressClass }} |
There was a problem hiding this comment.
IngressClass.spec.controller is immutable. For any existing chart-managed IngressClass created with the old fixed controller value (ingress-controllers.konghq.com/kong), upgrading with a non-default ingressController.ingressClass will make Helm try to patch this field and the upgrade will fail unless the IngressClass is deleted/recreated. Consider adding template logic to detect an existing IngressClass with a different .spec.controller and fail with a clear message (or otherwise document/handle the required delete-and-recreate flow).
| {{- $_ := set $autoEnv "CONTROLLER_KONG_ADMIN_TLS_SKIP_VERIFY" true -}} | ||
| {{- $_ := set $autoEnv "CONTROLLER_PUBLISH_SERVICE" ( include "kong.controller-publish-service" . ) -}} | ||
| {{- $_ := set $autoEnv "CONTROLLER_INGRESS_CLASS" .Values.ingressController.ingressClass -}} | ||
| {{- $_ := set $autoEnv "CONTROLLER_GATEWAY_API_CONTROLLER_NAME" (printf "ingress-controllers.konghq.com/-%s" .Values.ingressController.ingressClass) -}} |
There was a problem hiding this comment.
Setting CONTROLLER_GATEWAY_API_CONTROLLER_NAME to ingress-controllers.konghq.com/-%s changes the controller identity for Gateway API reconciliation and can break existing GatewayClass objects that use the long-standing default konghq.com/kic-gateway-controller (the chart’s own helm-test GatewayClass uses that value in templates/tests/test-resources.yaml). Unless the KIC default has changed, this is a breaking behavior change on upgrade. Consider preserving the default when ingressController.ingressClass is the default class (or only setting this env var when the user explicitly opts in / when ingressClass != "kong"), and/or using the konghq.com/kic-gateway-controller[-<class>] naming scheme to stay compatible with existing manifests.
| {{- $_ := set $autoEnv "CONTROLLER_KONG_ADMIN_TLS_SKIP_VERIFY" true -}} | |
| {{- $_ := set $autoEnv "CONTROLLER_PUBLISH_SERVICE" ( include "kong.controller-publish-service" . ) -}} | |
| {{- $_ := set $autoEnv "CONTROLLER_INGRESS_CLASS" .Values.ingressController.ingressClass -}} | |
| {{- $_ := set $autoEnv "CONTROLLER_GATEWAY_API_CONTROLLER_NAME" (printf "ingress-controllers.konghq.com/-%s" .Values.ingressController.ingressClass) -}} | |
| {{- $gatewayAPIControllerName := "konghq.com/kic-gateway-controller" -}} | |
| {{- if ne .Values.ingressController.ingressClass "kong" -}} | |
| {{- $gatewayAPIControllerName = printf "konghq.com/kic-gateway-controller-%s" .Values.ingressController.ingressClass -}} | |
| {{- end -}} | |
| {{- $_ := set $autoEnv "CONTROLLER_KONG_ADMIN_TLS_SKIP_VERIFY" true -}} | |
| {{- $_ := set $autoEnv "CONTROLLER_PUBLISH_SERVICE" ( include "kong.controller-publish-service" . ) -}} | |
| {{- $_ := set $autoEnv "CONTROLLER_INGRESS_CLASS" .Values.ingressController.ingressClass -}} | |
| {{- $_ := set $autoEnv "CONTROLLER_GATEWAY_API_CONTROLLER_NAME" $gatewayAPIControllerName -}} |
|
|
||
| * Align `IngressClass.spec.controller` and Gateway API | ||
| `CONTROLLER_GATEWAY_API_CONTROLLER_NAME` with `ingressController.ingressClass` | ||
| when using multiple controllers or a non-default ingress class. |
There was a problem hiding this comment.
This changelog entry describes the alignment change, but it doesn’t call out the operational upgrade impact: IngressClass.spec.controller is immutable, so users with an existing chart-managed IngressClass and a non-default ingressController.ingressClass will need to delete/recreate the IngressClass before upgrading. Similarly, changing the Gateway API controller name may require updating existing GatewayClass controllerName values. Please add explicit upgrade notes here (and ideally in UPGRADE.md) to prevent failed upgrades / unexpected controller non-reconciliation.
| when using multiple controllers or a non-default ingress class. | |
| when using multiple controllers or a non-default ingress class. | |
| Upgrade note: `IngressClass.spec.controller` is immutable. If you have an | |
| existing chart-managed `IngressClass` and use a non-default | |
| `ingressController.ingressClass`, you may need to delete and recreate the | |
| `IngressClass` before upgrading. If you use Gateway API resources, you may | |
| also need to update existing `GatewayClass.spec.controllerName` values to | |
| match the new controller name after upgrade, otherwise the controller may | |
| not reconcile those resources. |
pmalek
left a comment
There was a problem hiding this comment.
I'm OK with change in charts/kong/templates/ingress-class.yaml but for the other one (re Gateway API controller): I'd not go this route since those are 2 different things, IngressClass and Gateway API controller name.
| {{- include "kong.metaLabels" . | nindent 4 }} | ||
| spec: | ||
| controller: ingress-controllers.konghq.com/kong | ||
| controller: ingress-controllers.konghq.com/{{ .Values.ingressController.ingressClass }} |
There was a problem hiding this comment.
After reading this second time I need to take back my comment: this is not about the ingress class here but about the controller. While we could possibly link these 2 (like you did) I'd prefer not to do this.
What I think could be a good way forward and not introducing a breaking change for users is to add a new field in values.yaml (possibly: .ingressController.controllerName, defaulting to kong) and use that here.
This would also prevent what copilot mentioned about controller being immutable (by setting the default to what was already hardcoded in the template).
What this PR does / why we need it:
The chart previously set
IngressClass.spec.controllerto a fixed value (ingress-controllers.konghq.com/kong). That makes every deployment advertise the same controller identity, which breaks expectations when multiple Kong Ingress Controller instances use differentingressController.ingressClassvalues.This PR:
spec.controlleron the managedIngressClasstoingress-controllers.konghq.com/{{ .Values.ingressController.ingressClass }}so it matches the configured ingress class.An Ingress resource that targets a given ingress class resolves to the correct controller instance.
Which issue this PR fixes
ingressController.ingressClass)Special notes for your reviewer:
Checklist
mainbranch.