prefect-server helm chart enhancement to support multi-port service

[baisystems]

prefect-server helm chart enhancement to support multi-port service.

• service template creates service name and ports based on the ports array from values.yaml file
• update deployment template to use the server port extracted from the ports array
• add a service port to ingress template and values
• updated helm chart validation schema

closes #314

[jamiezieziula]

Hi @baisystems, thanks for the PR! Is there a reason we can't accomplish this functionality by allowing for additionalPorts to be passed as an optional array[dictionary] and leaving the current handling of the required service ports as is?

[baisystems]

Hi @jamiezieziula, there is no additionalPorts currently. If I understand you correctly, you are proposing to add a new extraPorts field under service in values.yaml file and use it like this in service template:

ports:
    - name: server-svc-port
      port: {{ .Values.service.port }}
      ...
    {{- range .Values.service.extraPorts }}
    - name: {{ .name }}
      port: {{ .port }}
      ...
    {{- end }}

If this is the case, my answer is Yes, we can accomplish this functionality with extraPorts. I do see the merits of your proposal: minimal changes to current code base as I don't need to change deployment.yaml and I don't need to add a new function tartget-port.yaml.

Your proposal also agree well with your current helm chart pattern extra ..., for example, extraContainers for server pod and extraPaths for server ingress.

I actually like your idea and can tailor my pr to implement the extraPorts field as I illustrated above.

The second feature of this PR is its introduction of a new field servicePort to ingress. This will make the ingress service port configurable to accommodate the case where the ingress is serving a port other than server-svc-port. What do you think about this change?

[jamiezieziula]

@baisystems thanks for the feedback! I look forward to reviewing your changes :)

I like the idea of the customizability of the ingress service port!

[baisystems]

@jamiezieziula I pushed a new commit which implemented the extraPorts field as discussed. Please review.

[jamiezieziula]

I don't think this is neccessary?

[baisystems]

I need to define this variable to hold the value of serviceType. The variable is used inside the loop of {{- range .Values.service.extraPorts }} to set nodePort. While inside the loop, it can't access the .Values.service.type due to context being changed. Is there better way to handle this situation?

[jamiezieziula]

ah i see - yes i think this makes sense!

[mitchnielsen]

You should be able to access the value with $.Values.service.type (notice the $. instead of just .).

There's an example at the bottom of https://helm.sh/docs/chart_template_guide/variables.

[baisystems]

@mitchnielsen Thank you for the tip. It works. $ - this variable will always point to the root context. Great to learn this.

[jamiezieziula]

A few minor comments - thanks!

[baisystems]

@jamiezieziula I pushed a new commit which default the extraPorts value to []. Any other issue for this pr?

[jamiezieziula]

@baisystems - can you install precommit and run it? linters are failing.

[baisystems]

@jamiezieziula I pushed a new commit. All the lint errors are fixed and the build is now passed. Could you please take another look?

[jamiezieziula]

Hi @baisystems - thanks! All linting looks healthy now. Sorry to do this, but one last thing - see @mitchnielsen's comment above.

[baisystems]

@jamiezieziula I tested @mitchnielsen's suggestion, helm install succeed with it. I will push an update commit soon.

[baisystems]

@jamiezieziula I pushed a new commit, the 5th. Please review again.