web and websecure entrypoints shouldn't be present in default values.yaml

[Issif]

Hi,

We use this helm chart as a dependency:

dependencies:
- name: traefik
  version: 8.8.1
  repository: https://containous.github.io/traefik-helm-chart

It means the default values.yaml which is present in this repository is applied and web and websecure entrypoints are created whatever we set in our own values.yaml. See here.

This is annoying because we want to use port 8000 for another purpose. We can override the port for web (and websecure) but these entrypoints are there, no matter what and we do not want useless and confusant config.

I understand the aim behind, which is offering a valid and functionnal chart, but it leads to a chart helm which is rigid and not 100% modular.

We can keep those parts as commented for example, but not force them to be set.

[Daverkex]

Hi @Issif,
Disable web and websecure entry is enough for you requested configuration?

web: {}
websecure: {}

[Issif]

I'll give a try. Thanks for the tip.

[Issif]

@Daverkex We tried, it didn't worked. I think we'll have to override the port.

[dirtycajunrice]

@Issif Correct. Entrypoints are defined directly from the ports here
https://github.com/containous/traefik-helm-chart/blob/98f4575d75b91c6e739ebc0e75c720c998f7dd38/traefik/templates/deployment.yaml#L123-L125
but as they are designed to only exist if they are used, leaving the name will cause an error as it will iterate over name with no config. This could be addressed with a change to

{{- range $name, $config := .Values.ports }}
{{- if $config }}
- "--entryPoints.{{$name}}.address=:{{ $config.port }}/{{ default "tcp" $config.protocol | lower }}"
{{- end }}
{{- end }}

that would also need to be fixed in a similar manner here:
https://github.com/containous/traefik-helm-chart/blob/98f4575d75b91c6e739ebc0e75c720c998f7dd38/traefik/templates/deployment.yaml#L88-L99
@SantoDE can you put a bug label on this and ill grab it in the next day or so?

[SantoDE]

@dirtycajunrice done. Happy to see you grabbing this

[BlackDark]

Hi @dirtycajunrice
it is still not enough to just remove the ports from a custom values file

Following does not work and still include for example the websecure port:

# Not working. Websecure entrypoint still created
websecure: {}

# Not working. Websecure entrypoint still created
# Comment or remove the port
# websecure: {}

This works and removes the websecure entrypoint for generated yaml:

# Just keep it empty
websecure:

# Set it to null
websecure: null

The default values.yaml is probably merged with the custom provided and if the websecure port is not removed or set to null directly it will remain.

Maybe this could be documented? Or the default values.yaml should not contain any ports at all? (Except the traefik port)

[SantoDE]

Ports need to be there for the k8s service. However, documenting it could be a good idea :)

[dirtycajunrice]

@BlackDark Morning! I am always for more docs! That said I am not sure if "explaining helm" is in scope. Since sometime back in 2017 Helm has used "deep merging" which will inherently merge any dictionaries of any depth.

What that means is websecure: {} is a dictionary, and therefore will merge with the default values.yaml. Yaml has a special character for null which is ~ and helm has an explicit that you showed called null. These are designed to change the default merging when needed.

Before the MR, if you had done websecure: null the chart would have broken, but now will not!

So the question remains: How far do you think a repo should go in the inline docs before expecting someone to read the helm docs? If this is a "special" circumstance lemme know ill add the docs to my next MR :)

[BlackDark]

@dirtycajunrice i see your point.
Not sure what the best is. With many charts my experiences are that they do only the most necessary things with the default values and provide the user the options to activate stuff as needed (for example elastic stacks: https://github.com/elastic/helm-charts).

Changing the default to not include those port would probably also result in problems for many peoples.
@SantoDE what do mean they are needed for the k8s service? If i remove both (web + secure) all is fine :)

But explaining helm features is sure nothing we need :D

[SantoDE]

The ports are for example used, for the service created by the helm-chart (https://github.com/containous/traefik-helm-chart/blob/c174d5bb241bb567beb32022382b3eaf2a1e2c18/traefik/templates/service.yaml#L89).

Therefore, I'm not sure what you mean by removing and all is fine. But I guess, we're dragging a bit off topic :)