Updated helm chart practices - PRs wanted?

[consideRatio]

Hi @yuvipanda! Would you like PRs for the following kinds of changes for this repo at this point?

1. Label conventions and such are outdated (component instead of app.kubernetes.io/component etc), would you like them to be updated? This is a breaking change because it changes matchLabels, so helm3 will struggle making such change while helm2 --force can manage them okay.

   Done in commit helm: modern name/labels/matchLabels

2. The helm Chart.yaml is a v1 kind supporting helm2 still, do you want to stay with that and migrate later or upgrade right away to helm3 native v2 Chart.yaml specification? No real benefit afaik about this though other than staying up to date, but it will remove helm2 support by doing it.

   Done in commit helm: use Chart.yaml v2 and add myself as a maintainer.

3. There are general indentation practices that i have managed to make helm official that are broken which i could also update, like using nindent along with toYaml.

   Done in commit helm: modern indentation and whitespace chomping.

4. Helm template helpers are named ..fullname etc rather than something globally unique, like jupyterhub-ssh.fullname. This can cause a conflict with other charts doing the same thing with 'helm create .' for example, and breaks a Helm best practice. Would you like to have them made globally unique?

   Done in commit helm: modern name/labels/matchLabels

5. The Helm chart deployment starts the pod with container port 22 instead of something above 1000 which makes it require more privileges. I suggest to use a 1000+ port like 8022 instead, name it to ssh, and let the k8s Service expose port 22 and point to the targetPort of ssh. Benefits: easier networkpolicy configuration in the future and less issues with PodSecurityPolicies.

   Done in commit helm: ports for deployments in 1000+ range & services ref named ports. Note though that since the Dockerfile for the sftp-server wasn't in the repo, I could not ensure that it now started listening on port 8022 instead of port 22, so that needs to be fixed separately.

6. The configmap and secret are kept separate, but in practice, you put secret content in the configmap currently. This is a practice we stopped following in z2jh due to the complexity added. I suggest to have only a k8s secret and put everything there. If you care about readability, we can use stringData instead of data in the k8s secret to avoid needing to base64 encode.

   Done in commit helm: don't split config into secret/configmap

7. I think hubUrl is required, right?

   Done in commit helm: add NOTES.txt requesting hubUrl config if unset and I also added it as a blank value to values.yaml in a previous commit.

8. I noticed the annotations to restart the pods were a bit off, because they only checked if the templates themselves changed, not the rendered templates, which is probably what was wanted.

   Done in commit helm: fix logic failure with annotated config checksum

9: I noticed there was configuration about labels, but not implemented, so I implemented podLabels, podAnnotations, serviceLabels, serviceAnnotations - things I have seen requested in z2jh and know to be relevant from that experience.

Done in commit helm: fix and support pod&service labels&annotations.

10. I figured it would also make sense to support imagePullSecrets, so I added such support.

    Done in commit helm: support imagePullSecrets config.

11. I did some small stuff like falling back to .Chart.AppVersion as image tag if ..image.tag wasn't specified, and added a note about that we should use a statefulset if we think we want a PVC for the SFTP server I think.

[yuvipanda]

Extremely excited when I saw you open this, @consideRatio :)

1. Label conventions and such are outdated (component instead of app.kubernetes.io/component etc), would you like them to be updated? This is a breaking change because it changes matchLabels, so helm3 will struggle making such change while helm2 --force can manage them okay.

w00p, would love that! Nobody's using this other than me right now so that's ok :)

2\. The helm Chart.yaml is a v1 kind supporting helm2 still, do you want to stay with that and migrate later or upgrade right away to helm3 native v2 Chart.yaml specification? No real benefit afaik about this though other than staying up to date, but it will remove helm2 support by doing it.

Don't think we have to support helm2, so let's drop it!

3\. There are general indentation practices that i have managed to make helm official that are broken which i could also update, like using nindent along with toYaml.

yes please!

5\. The Helm chart deployment starts the pod with container port 22 instead of something above 1000 which makes it require more privileges. I suggest to use a 1000+ port like 8022 instead, name it to ssh, and let the k8s Service expose port 22 and point to the targetPort of `ssh`. Benefits: easier networkpolicy configuration in the future and less issues with PodSecurityPolicies.

See #16. I just moved the ports, but the sftp daemon at least still needs to run as root with privileged: true

6\. The configmap and secret are kept separate, but in practice, you put secret content in the configmap currently. This is a practice we stopped following in z2jh due to the complexity added. I suggest to have only a k8s secret and put everything there. If you care about readability, we can use `stringData` instead of `data` in the k8s secret to avoid needing to base64 encode.

This sounds great too!

[yuvipanda]

4\. Helm template helpers are named ..fullname etc rather than something globally unique, like jupyterhub-ssh.fullname. This can cause a conflict with other charts doing the same thing with 'helm create .' for example, and breaks a Helm best practice. Would you like to have them made globally unique?

I actually moved away from prefixing object names with release names, both to match z2jh and because I find it cleaner (vs repeating the name of namespace + release name). From a practical perspective, this makes life much easier when you need to integrate with other charts - in this case, z2jh itself. Check out https://github.com/berkeley-dsep-infra/datahub/blob/staging/hub/values.yaml#L83 - I have to tell traefik where to route to, and I can consistently just specify this all the time. If we instead prefixed chart objects with their release names, this mostly becomes impossible to do.

This is not a super strongly held belief though, so can be convinced otherwise :) But doesn't seem a priority...

Very excited to have you be interested in this, @consideRatio!

[consideRatio]

@yuvipanda "Would you like to have them made globally unique?" regarded the names of the template helpers for the Helm chart, if they are not made globally unique, and you have this Helm chart as a dependency to another Helm chart, they may collide. This kind of change does not influence anything rendered by the Helm chart. Helm describes it as a best practice to always define helper functions with like "."

But, you are considering about the naming convention of the resources themselves, and with what I will introduce, you can achieve that by declaring fullnameOverride: jupyterhub-ssh in provided Helm values. From experience, I'm strongly opinionated to support the inclusion of the release name, but not strongly opinionated of including it by default, so we can can certainly let fullnameOverride: jupyterhub-ssh by default if you think that makes sense. It is a deviation of the common practice, but a very unproblematic one and I think it would help us document things better by providing easy examples that work unless someone has included the release name.

[yuvipanda]

@consideRatio ah, if I understand this correctly, we can have a service name that is by default jupyterhub-ssh say, but lets folks override it to be my-release-name-jupyterhub-ssh. Is that correct? If we can have the default not have prefix, that sounds awesome!

[consideRatio]

@yuvipanda yes close to that I think!

• If you fullnameOverride is set to jupyterhub-ssh, then resources will be named jupyterhub-ssh no matter what, except for I avoid naming collisions of sftp / ssh server by appending -sftp-server and -ssh-server for example.
• If fullnameOverride isn't set, then resources will get resource names with the release name included by default with jupyter-ssh appended to it. There is an exception, which is that if they named the release something-with-jupyterhub-ssh-in-its-name then it will just be whatever it was without jupyterhub-ssh appended to it.

[yuvipanda]

@consideRatio do you think this can be closed now that #19 is merged?

[consideRatio]

@yuvipanda absolutely! I wrote "closing #17" within the PR text, but I needed to write "fixes" or "closes" :)