Kong / charts

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

Allow multiple `ingress` releases with the same name but in different namespaces #942

Open pmalek opened 10 months ago

pmalek commented 10 months ago

Problem statement

Currently, ingress (through the means of kong subcharts) chart creates a ClusterRole named

https://github.com/Kong/charts/blob/d65e5d1ed44c29bf1ecf896edb056a19614661af/charts/kong/templates/_helpers.tpl#L20

which (when .Values.nameOverride is unspecified) collides since it uses the same name default .Chart.Name .Values.nameOverride.

The purpose of this issue is to allow multiple installations of ingress chart (and possibly kong as well) with the same release name in different namespaces

Acceptance criteria

rainest commented 10 months ago

It does include the release name also, OP has an initial prep variable that's then used in the full template output:

https://github.com/Kong/charts/blob/d65e5d1ed44c29bf1ecf896edb056a19614661af/charts/kong/templates/_helpers.tpl#L20-L21

Resource names and other fields that use fullname are length limited and we have encountered issues with that in the past:

https://github.com/Kong/charts/issues/675 https://github.com/Kong/charts/issues/819

Injecting the namespace in there will make all names longer, forcing other installs to either shorter release names or nameOverride. You'd have to weigh how critical being able to use the same release name across namespaces is against that. AFAIK there's no reason you'd have to use the same release name across namespaces; it's mostly a gotcha.

pmalek commented 10 months ago

If we change

{{- $name := default .Chart.Name .Values.nameOverride -}}

to

{{- $name := default .Release.Namespace .Values.nameOverride -}}

wouldn't that work? This could in theory collide with other ClusterRoles that use this release's namespace and name but 🤷 The probability of a collision is the same using the .Chart.Name (which in ingress is either controller or gateway)

rainest commented 10 months ago

I'm fairly sure that will break stuff in ingress even without multiple releases. The namespace is the same for both subcharts, and stuff like the HPA can be turned on for both and just uses kong.fullname.

I think you'd need to create a separate kong.ns-fullname and use it for Cluster-level resources. That does still run the same overflow risk, but less so--AFAIK most of the problem resources are adding additional suffixes past kong.fullname (the migration jobs with their long -TASK-migrations suffixes are the ones that usually hit the limit). If we only create one of a resource (which I think is the case for most or all cluster resources), we usually just use kong.fullname alone as the resource name.

pmalek commented 10 months ago

The namespace is the same for both subcharts

Ah, right 🤦

I think you'd need to create a separate kong.ns-fullname and use it for Cluster-level resources.

What would be the advantage over reusing kong.fullname?