bjw-s / helm-charts

A collection of Helm charts
https://bjw-s.github.io/helm-charts/
Apache License 2.0
582 stars 106 forks source link

"ports" property missing under main container in new version 2.0.1 #197

Closed runenielsen closed 1 year ago

runenielsen commented 1 year ago

Details

This is primarily a question to know ensure that the change below was intentional and hear the reasoning, not necessarily a bug.

I upgraded my Helm chart from using version 1.5.1 to 2.0.1 and compared the rendered result.

Previously I automatically got a ports property under the main container in the rendered result, like this:

    spec:
...
      containers:
        - name: ...
...
          ports:
            - name: http
              containerPort: 8080
              protocol: TCP

... in the new version I have to add it myself manually in my values-file, like this:

controllers:
  main:
    containers:
      main:
...
        ports:
          - containerPort: 8080
            name: http
            protocol: TCP

... otherwise it is not rendered. Was this change on purpose, and what is the reason?

What did you expect to happen:

I expected the new version to render the same as the old version for ports, but I'm no charting expert, so there is probably a reason for the change, that I am not aware of.

Anything else you would like to add:

No.

Additional Information:

No.

bjw-s commented 1 year ago

You are correct that this is a change that I forgot to put in the release notes. The container ports section relied heavily on some magic templating that was "impossible" to recreate with the deprecation of primary services.

Based on what I could read here, there is actually not much benefit to setting them: https://matthewpalmer.net/kubernetes-app-developer/articles/kubernetes-ports-targetport-nodeport-service.html

This array, defined in pod.spec.containers[].ports, provides a list of ports that get exposed by the container. You don’t really need to specify this list—even if it’s empty, as long as your containers are listening on the port, they’ll still be available for network access. This just provides some extra information to Kubernetes.

The only use case I've found for explicitly adding named container ports has been with custom probes. In the end I've just resorted to configuring them with the port number instead of the named port.

runenielsen commented 1 year ago

@bjw-s : Thanks for the explanation - this was also my suspicion based on another link I found, that said something similar. Just wanted to make sure this was intentional, and that you agreed 🙂