[Feature]: Helm — Independently configure container and service ports

[EternalDeiwos]

Description

I find the template for configuring Nessie's ports odd since there is all this logic going into checking the ports between the service and deployment are synchronized when they don't have to be.

Its best practice to not bind privileged ports inside containers and expose them on whatever port after the fact; doing so will also simplify the helm templates and make them easier to maintain.

nessie/helm/nessie/templates/_helpers.tpl

Lines 428 to 468 in 9954c6e

	{{/*
	Prints the ports section of the container spec. Also validates all port names and numbers to ensure
	that they are consistent and that there are no overlaps.
	*/}}
	{{- define "nessie.containerPorts" -}}
	{{- $ports := dict -}}
	{{- range $i, $port := .Values.service.ports -}}
	{{- if hasKey $ports $port.name -}}
	{{- fail (printf "service.ports[%d]: port name already taken: %v" $i $port.name) -}}
	{{- end -}}
	{{- if has $port.number (values $ports) -}}
	{{- fail (printf "service.ports[%d]: port number already taken: %v" $i $port.number) -}}
	{{- end -}}
	{{- $_ := set $ports $port.name $port.number -}}
	{{- end -}}
	{{- if hasKey $ports .Values.managementService.portName -}}
	{{- fail (print "managementService.portName: port name already taken: " .Values.managementService.portName ) -}}
	{{- end -}}
	{{- if has .Values.managementService.portNumber (values $ports) -}}
	{{- fail (print "managementService.portNumber: port number already taken: " .Values.managementService.portNumber) -}}
	{{- end -}}
	{{- $_ := set $ports .Values.managementService.portName .Values.managementService.portNumber -}}
	{{- range $i, $svc := .Values.extraServices -}}
	{{- range $j, $port := $svc.ports -}}
	{{- if hasKey $ports $port.name -}}
	{{- if ne $port.number (get $ports $port.name) -}}
	{{- fail (printf "extraServices[%d].ports[%d]: wrong port number for port %s, expected %v, got %v" $i $j $port.name (get $ports $port.name) $port.number) -}}
	{{- end -}}
	{{- else if has $port.number (values $ports) -}}
	{{- fail (printf "extraServices[%d].ports[%d]: port number already taken: %v" $i $j $port.number) -}}
	{{- end -}}
	{{- $_ := set $ports $port.name $port.number -}}
	{{- end -}}
	{{- end -}}
	ports:
	{{ range $portName, $portNumber := $ports -}}
	- name: {{ $portName }}
	containerPort: {{ $portNumber }}
	protocol: TCP
	{{ end -}}
	{{ end -}}

Put another way, the Kubernetes API exposes a very well defined, well understood way for specifying connections between Service ports and container ports; why not expose it directly?

Expected Use Cases

I would like to expose Nessie from my service on ports 80/443 respectively but still have the container bind on whatever is the defaults e.g. 19120/19121.

Requested Changes in public API

N/A

[adutra]

Your suggestion makes totally sense. I guess that the only reason why we don't have it is because it has never been asked before :-)

I would suggest something like this:

service:
  ports:
    - name: nessie-http
      port: 80
      targetPort: 19120
    - name: nessie-https
      port: 443
      targetPort: 19121

managementService:
  port:
    name: nessie-mgmt
    port: 9000
    targetPort: 9000

extraServices:
  ports:
    - name: nessie-http-headless
      port: 19120
      targetPort: 19120
      clusterIP: None

It's not a difficult change per se, but we need to account for old fields (service.ports[0].number or managementService.portNumber for instance), so that users can upgrade smoothly.

WDYT?

[EternalDeiwos]

Supporting migration is necessary; even if it makes things more complicated now, it will allow the older fields to be deprecated and removed for an overall cleaner solution later.

Regarding your suggestion for extraServices, I'd advise mirroring the structure of a service; so you'd have this instead:

extraServices:
  - name: nessie-ext
    # Or this:
    # nameSuffix: -ext
    clusterIP: None
    ports:
      - name: nessie-http-headless
        port: 19120
        targetPort: 19120

Similarly, managementService should also use the same API (i.e. ports: ServicePort[]) even if only one port is defined. This will make your service definitions cleaner and more predictable.

[adutra]

Regarding your suggestion for extraServices, I'd advise mirroring the structure of a service; so you'd have this instead

Absolutely, the structure here is meant to be ServicePort, my snippet above had an indentation error.

also use the same API (i.e. ports: ServicePort[]) even if only one port is defined.

Hmm here I'm not so sure. I could easily see people trying to enter more than one port for the management service, which doesn't make sense (or does it?). I understand what you have in mind: you would like the whole thing to be of type ServiceSpec right? Why not, but I'm not entirely sold on that.

[adutra]

Another problem with using ServiceSpec is that it contains the selector field, which should not be populated by users. I guess we could add a runtime check for that though.

[EternalDeiwos]

I'm not suggesting you allow all fields; just the fields that you do include should mirror the structure defined by ServiceSpec. I absolutely agree that selector and many other fields have no place being modified by the user as it would defeat the purpose of using a Helm chart in the first place. This was also why I had initially left internalTrafficPolicy out of #10011, because I thought it was not necessary to be specified by the user.