search

jupyterhub / zero-to-jupyterhub-k8s

Helm Chart & Documentation for deploying JupyterHub on Kubernetes
https://zero-to-jupyterhub.readthedocs.io
Other
1.56k stars 799 forks source link

fix(daemonset): Allow the definition a ServiceAccount in DaemonSets #3441

Closed samyuh closed 1 month ago

samyuh commented 5 months ago

This pull request introduces the option to define a ServiceAccount in DaemonSets used in the prepuller.

Closes https://github.com/jupyterhub/zero-to-jupyterhub-k8s/pull/3442

samyuh commented 4 months ago

Hey @manics could you take a look at this?

samyuh commented 3 months ago

@manics check this again when you can, everything is working as expected

samyuh commented 2 months ago

@consideRatio please take a look when you can :)

samyuh commented 1 month ago

@manics can you push this to have the second review and be merged?

consideRatio commented 1 month ago

I'll make time to review this this PR this week

consideRatio commented 1 month ago

I failed to make time for this, but it remains on my todo list.

samyuh commented 1 month ago

Thanks! Please don't forget to review this since it's open since June :)

consideRatio commented 1 month ago

Thank you for engaging and trying to figure things out here @samyuh!!

Background and overview

There are two "pre puller" machineries:

Both involve a daemonset to ensure each node gets images pulled, but the hook pre puller needs to stop, so it includes a k8s job that starts and finishes. The k8s job pod needs k8s api permissions, because it needs to read the state of the daemonset that was created, so it can say if all its pods are ready etc. Its just waiting for that to happen and then shuts down.

Anyhow, this is why the daemonset pods doesn't have a service account, they haven't needed one. The one used for the hook pre puller's k8s job pod is specifically for that k8s job pod and comes with relevant permissions, re-using it for daemonset's pods won't be the correct call no matter what as it provides irrelevant permissions to those pods.

Decision

I'll opt to close this pull request in its current form for now as I don't want to have the hook pre puller machinery's k8s job's service account re-used for the daemonsets' pods, however:

If there a need to configure the pre-puller pods with a k8s Service Account, and possibly also have it be created by the chart as well, it could be considered. For this, please describe what is to be accomplished by configuring the daemonset pod's serviceAccountName and optionally also if there is value in getting a blank k8s ServiceAccount created by the chart.

samyuh commented 1 month ago

Hey @consideRatio, thanks for your detailed response.

If there a need to configure the pre-puller pods with a k8s Service Account, and possibly also have it be created by the chart as well, it could be considered. For this, please describe what is to be accomplished by configuring the daemonset pod's serviceAccountName and optionally also if there is value in getting a blank k8s ServiceAccount created by the chart.

We really need to have every component in our deployment with a specific service account due to security compliance. In this merge request, I used the existing pre-puller service account as I thought there would be no problem. We can just go with the option of setting the serviceAccountName with a value from the chart. Are you ok with that alternative? Can you reopen this merge request if you're okay with these changes?

Thanks again :)