bjw-s / helm-charts

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

Numeric ports must be un-quoted in startupProbe.tcpSocket.port #207

Closed gclawes closed 8 months ago

gclawes commented 9 months ago

Details

What steps did you take and what happened:

Configure an app deployment with a startupProbe:

            probes:
              liveness:
                enabled: false
              readiness:
                enabled: false
              startup:
                enabled: true
                port: 64738
                spec:
                  initialDelaySeconds: 0
                  periodSeconds: 10
                  timeoutSeconds: 1
                  failureThreshold: 3

Attempt to helm upgrade via terraform:

╷
│ Error: cannot patch "mumble-server" with kind Deployment: Deployment.apps "mumble-server" is invalid: [spec.template.spec.containers[0].startupProbe.tcpSocket.port: Invalid value: "64738": must contain at least one letter (a-z), spec.template.spec.containers[1].startupProbe.tcpSocket.port: Invalid value: "9999": must contain at least one letter (a-z)]
│ 
│   with helm_release.mumble-server,
│   on mumble_server.tf line 82, in resource "helm_release" "mumble-server":
│   82: resource "helm_release" "mumble-server" {
│ 
╵

It is presumed this affects all types of probes relying on a port number.

What did you expect to happen:

Helm properly patches the Deployment.

Anything else you would like to add:

Kubernetes API docs specify that tcpSocket.port must be:

Number or name of the port to access on the container. Number must be in the range 1 to 65535. Name must be an IANA_SVC_NAME. [1]

I believe quoting a numeric port results in the kubernetes API interpreting this as a named port, not a number. Because it is impossible to specify a port name in the new 2.0.0 bjw-s/common chart (https://github.com/bjw-s/helm-charts/issues/197), numeric port numbers must always be used.

[1] https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.26/#tcpsocketaction-v1-core

Additional Information:

It would be nice if named ports worked for probes, but fixing this would be sufficient to eliminate the bug.

bjw-s commented 8 months ago

Thanks for reporting this. I've implemented a fix in v2.1.0.

Named ports do actually work for probes btw, but you will need to make sure that you have a ports section set up in your container specification. For example: https://github.com/bjw-s/home-ops/blob/main/kubernetes/cluster-0/apps/monitoring/alertmanager/app/helmrelease.yaml#L40-L42. This would allow you to reference the http port in your probes.