feat: set default router flavor to expressions if KIC >=3.0 and traditional_compatible otherwide #935

randmonkey · 2023-11-09T07:48:50Z

What this PR does / why we need it:

Add kong.router_flavor to set router flavor to:

values.env.router_flavor if specified
expressions if KIC is enabled and image version >= 3.0.0
traditional_compatible otherwise (REVIEW: is it better to leave it empty to let Kong gateway to use the default value)?

Which issue this PR fixes

(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged)

fixes Chart uses the expression router by default kubernetes-ingress-controller#5129

Special notes for your reviewer:

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

PR is based off the current tip of the main branch.
Changes are documented under the "Unreleased" header in CHANGELOG.md
New or modified sections of values.yaml are documented in the README.md
Commits follow the Kong commit message guidelines

…tional_compatible otherwise

czeslavo · 2023-11-09T09:24:15Z

charts/kong/templates/_helpers.tpl

+    {{- if (and  .Values.ingressController.enabled  (semverCompare ">= 3.0.0" (include "kong.effectiveVersion" .Values.ingressController.image))) -}}
+        expressions
+    {{- else -}}
+        traditional_compatible


As I understand this also changes the default when KIC is < 3.0.0 from traditional to traditional_compatible, is this intended?

👍 on keeping whatever was the default - traditional for KIC <3.0.

pmalek · 2023-11-09T09:38:54Z

REVIEW: is it better to leave it empty to let Kong gateway to use the default value

On one hand setting this in chart makes this predictable for us. Regardless of Kong Gateway version we use as default, we'll always get the expected behavior. On the other hand though, users might be expecting the Gateway defaults to translate into corresponding chart settings.

We've had a long history of setting the router flavor so I'd say we retain this and keep setting it in the chart. If there'a debate we'd like to have on this I suggest we consider pros and cons of that in a separate issue.

czeslavo

As mentioned on the team sync, we have to keep in mind this won't work as expected with kong/ingress chart. gateway part of the release doesn't know about the controller's ingressController.image. The condition will be executed based on the gateway's ingressController.image (which will be the default from values.yaml - so we accidentally get KIC 3.0 there always if a user for some weird reason didn't change gateway.ingressController.image to something else).

As an example, this change will break Konnect values.yaml where we still want to keep using traditional_compatible router with KIC 2.11/2.12:

controller:
  ingressController:
    image:
      tag: "2.11"
...

gateway:
  image:
    repository: kong/kong-gateway
    tag: "3.4"
...

~~As this will pick expressions based on the default value of gateway.ingressController.image which is 3.0 now.~~

EDIT after checking actual results: As in the template there's .Values.ingressController.enabled condition required to set router flavor to expressions, for kong/ingress chart this will always default to traditional_compatible. The problem persists though because the router flavor is not changed based on the KIC version as intended. So, if we set controller.ingressController.image.tag to 3.0 to start using KIC 3.0 in kong/ingress, Gateway's router flavor will still default to traditional_compatible.

I think we have to either:

Wait for @rainest's Split the Deployment #923
Add some templating to kong/ingress that will somehow override gateway.ingressController.image to what is set in controller.ingressController.image.

czeslavo · 2023-11-09T13:24:35Z

I was trying to come up with some workaround that would make this work with kong/ingress, but I couldn't achieve anything that would satisfy the requirement of helm install kong kong/ingress just setting the expression router based on controller image tag. 😞

I was trying to use .Values.global or templates to override values of the gateway subchart using values of the controller, but that is not possible (there was a proposal in Helm charts repo implementing that, but it was rejected: helm/helm#6876).

The only more-or-less reasonable way I came up with was adding a new variable in kong/kong values (deployment.kong.alignWithControllerVersion) that could allow users to explicitly tell to auto-align gateway's config based on a given controller version. #936 But of course, it has drawbacks, e.g. requires setting an additional value that also should match ingress.ingressController.image.tag to not blow up (that could be validated).

I think I just assured myself that #923 is the only way we can make it work as we expected.

pmalek · 2023-11-09T13:36:57Z

I couldn't find the helm doc page on this but .Subcharts seems to be the thing that we're looking for (at least as a workaround until/if we get to the split Deployment in kong chart).

#937 shows how it could be used to enforce the router flavor in ingress chart using subcharts values.

czeslavo · 2023-11-09T13:42:37Z

I couldn't find the helm doc page on this but .Subcharts seems to be the thing that we're looking for (at least as a workaround until/if we get to the split Deployment in kong chart).

#937 shows how it could be used to enforce the router flavor in ingress chart using subcharts values.

As I understand that would prevent using anything else but expressions with KIC >= 3.0 which might be too rigorous 🤔

Sidenote: If I'm not mistaken, .Subcharts is an equivalent of getting these values from .Values.controller or .Values.gateway.

pmalek · 2023-11-09T13:52:33Z

As I understand that would prevent using anything else but expressions with KIC >= 3.0 which might be too rigorous 🤔

Hmm, yes. We'd like to set this for the user as in perform the defaulting in template code rather than limit the possibilities as #937 is trying to do.

In this case I'm not sure this is possible this way (or at least without massive rewrite of kong chart into something like a gigantic template which would be rendered in ingress using provided subchart values 🤯 )

Sidenote: If I'm not mistaken, .Subcharts is an equivalent of getting these values from .Values.controller or .Values.gateway.

.Subcharts is an object holding fields like .Files, .Values etc.

.Subcharts.controller.Values and .Subcharts.gateway.Values can be used in ingress chart to get values from subcharts.

czeslavo · 2023-11-09T14:33:21Z

@pmalek I came up with another alternative: #939. It sets the router flavor to expressions in kong/ingress and prevents using this kind of flavor with KIC < 3.0. It has a drawback that would make it break when somebody:

has kong/ingress release installed with KIC version pinned to one lower than 3.0
has router flavor set to expressions already
wants to keep using the pinned KIC with the new chart version

feat: set default router flavor to expressions if KIC >=3.0 and tradi…

2eaf90c

…tional_compatible otherwise

randmonkey requested a review from a team as a code owner November 9, 2023 07:48

update changelog

f6119b0

randmonkey self-assigned this Nov 9, 2023

randmonkey added the enhancement New feature or request label Nov 9, 2023

czeslavo reviewed Nov 9, 2023

View reviewed changes

czeslavo requested changes Nov 9, 2023

View reviewed changes

czeslavo mentioned this pull request Nov 9, 2023

add alignWithControllerVersion to allow auto-adjustment of GW settings #936

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: set default router flavor to expressions if KIC >=3.0 and traditional_compatible otherwide #935

feat: set default router flavor to expressions if KIC >=3.0 and traditional_compatible otherwide #935

randmonkey commented Nov 9, 2023 •

edited

Loading

czeslavo Nov 9, 2023

pmalek Nov 9, 2023

pmalek commented Nov 9, 2023

czeslavo left a comment •

edited

Loading

czeslavo commented Nov 9, 2023 •

edited

Loading

pmalek commented Nov 9, 2023

czeslavo commented Nov 9, 2023

pmalek commented Nov 9, 2023

czeslavo commented Nov 9, 2023 •

edited

Loading

feat: set default router flavor to expressions if KIC >=3.0 and traditional_compatible otherwide #935

Are you sure you want to change the base?

feat: set default router flavor to expressions if KIC >=3.0 and traditional_compatible otherwide #935

Conversation

randmonkey commented Nov 9, 2023 • edited Loading

What this PR does / why we need it:

Which issue this PR fixes

Special notes for your reviewer:

Checklist

czeslavo Nov 9, 2023

Choose a reason for hiding this comment

pmalek Nov 9, 2023

Choose a reason for hiding this comment

pmalek commented Nov 9, 2023

czeslavo left a comment • edited Loading

Choose a reason for hiding this comment

czeslavo commented Nov 9, 2023 • edited Loading

pmalek commented Nov 9, 2023

czeslavo commented Nov 9, 2023

pmalek commented Nov 9, 2023

czeslavo commented Nov 9, 2023 • edited Loading

randmonkey commented Nov 9, 2023 •

edited

Loading

czeslavo left a comment •

edited

Loading

czeslavo commented Nov 9, 2023 •

edited

Loading

czeslavo commented Nov 9, 2023 •

edited

Loading