helm charts - ingress.class could be extracted to variable (refactoring)

[sparkoo]

Is your task related to a problem? Please describe.

nginx is used repeatedly as a value for ingress.class in helm. This could be extracted to variable.
examples:
https://github.com/eclipse/che/blob/master/deploy/kubernetes/helm/che/custom-charts/che-jaeger/templates/ingress.yaml#L18
https://github.com/eclipse/che/blob/master/deploy/kubernetes/helm/che/custom-charts/che-keycloak/templates/ingress.yaml#L16

Describe the solution you'd like

Find usages of nginx as an ingress.class and extract it to values.yaml https://github.com/eclipse/che/blob/master/deploy/kubernetes/helm/che/values.yaml

Describe alternatives you've considered

It's refactoring and it does not affect functionality. We don't have to do that.

Additional context

with devfile and plugin registry coming to helm charts and following the pattern, it will be at more places #13890

[metlos]

For this to make sense, the helm chart should also offer the possibility to define the other custom annotations on the ingresses that the given ingress controller (chosen by the ingress.class annotation) requires.

[metlos]

Also note, that we use the hardcoded nginx.class in https://github.com/eclipse/che/blob/master/deploy/kubernetes/helm/che/templates/configmap.yaml#L71 and https://github.com/eclipse/che/blob/master/deploy/kubernetes/helm/che/templates/configmap.yaml#L73

[metlos]

To explain my concerns in more detail.

Currently, we hardcode the ingress.class to nginx and we have a variable called global.ingressAnnotationsPrefix that defaults to nginx. which is used when declaring the ingress annotations (both for the "server" ingresses that make Che server (and Keycloak, Jaeger, et al) work - defined at locations like https://github.com/eclipse/che/blob/master/deploy/kubernetes/helm/che/custom-charts/che-jaeger/templates/ingress.yaml#L18
https://github.com/eclipse/che/blob/master/deploy/kubernetes/helm/che/custom-charts/che-keycloak/templates/ingress.yaml#L16 and for workspace ingresses, defined in https://github.com/eclipse/che/blob/master/deploy/kubernetes/helm/che/templates/configmap.yaml#L71).

This is already bad, because it gives the helm chart user a false idea that somehow they can configure the helm chart to use some other ingress controller (through the global.ingressAnnotationsPrefix variable). They can't because other ingress controllers most probably don't support the same annotations as nginx ingress controller even if the annotation prefix is changed to match the name of the needed ingress controller.

So, by adding the option to configure the ingress.class we're only making that false idea stronger. But it would still be false.

IMHO, there are 2 approaches to this problem:

• keep ingress.class hardcoded (and IMHO actually remove the global.ingressAnnotationsPrefix variable and hardcode it as well) - this would make it clear that the helm chart doesn't support any other ingress controller but nginx
• come up with a way of defining free-form ingress annotations for both the "server" ingresses and "workspace" ingresses, ideally in some "coordinated" manner, so that they cannot diverge too much (they HAVE to differ, because they have different requirements, but they at least should be consistent).

@l0rd, @skabashnyuk, @sleshchenko wdyt?

[sparkoo]

I like the second option. I would investigate option to have multiple ingress configurations prepared and let user choose which one to use. I'm not sure whether it is doable though.

[l0rd]

@metlos makes sense. But I am not sure that server and workspaces ingresses should always be "coordinated". It makes sense for small clusters but on real world clusters with multiple ingress controllers, server side and workspaces side ingress classes may need to be different. What I mean is that users should able specify ingress class and annotations for both server and workspace ingresses. Making them "coordinated" may be gold plating.

[sparkoo]

we've decided to close this one as won't fix and proper solution fixing using various ingress configurations will be investigated in #14230 and fixed in following tasks.