search

Kong / charts

Helm chart for Kong
Apache License 2.0
243 stars 475 forks source link

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

Open randmonkey opened 10 months ago

randmonkey commented 10 months ago

What this PR does / why we need it:

Add kong.router_flavor to set router flavor to:

Which issue this PR fixes

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

Special notes for your reviewer:

Checklist

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

pmalek commented 10 months ago

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 commented 10 months ago

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: https://github.com/helm/helm/pull/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. https://github.com/Kong/charts/pull/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 https://github.com/Kong/charts/pull/923 is the only way we can make it work as we expected.

pmalek commented 10 months ago

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).

https://github.com/Kong/charts/pull/937 shows how it could be used to enforce the router flavor in ingress chart using subcharts values.

czeslavo commented 10 months ago

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 commented 10 months ago

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 https://github.com/Kong/charts/pull/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 commented 10 months ago

@pmalek I came up with another alternative: https://github.com/Kong/charts/pull/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: