Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 17 commits into from

Conversation

samyuh
Copy link

@samyuh samyuh commented Jun 20, 2024

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

Closes #3442

@samyuh samyuh changed the title feat(daemonset): Add serviceAccount and Annotations feat(daemonset): Add serviceAccount Jun 21, 2024
@samyuh samyuh changed the title feat(daemonset): Add serviceAccount fix(daemonset): Add serviceAccount Jun 23, 2024
@samyuh samyuh changed the title fix(daemonset): Add serviceAccount fix(daemonset): Allow the creation of Service Accounts Jun 23, 2024
@samyuh
Copy link
Author

samyuh commented Jun 25, 2024

Hey @manics could you take a look at this?

jupyterhub/values.yaml Outdated Show resolved Hide resolved
@samyuh samyuh changed the title fix(daemonset): Allow the creation of Service Accounts fix(daemonset): Allow the definition a ServiceAccount in DaemonSets Aug 13, 2024
@samyuh samyuh requested a review from manics August 13, 2024 22:02
@samyuh
Copy link
Author

samyuh commented Aug 22, 2024

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

Copy link
Member

@manics manics left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than a couple of queries this looks good to me, thanks for working on this.

@consideRatio would you mind reviewing this? I'm not fully confident about all the changes.

@samyuh
Copy link
Author

samyuh commented Sep 3, 2024

@consideRatio please take a look when you can :)

@samyuh
Copy link
Author

samyuh commented Sep 30, 2024

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

@consideRatio
Copy link
Member

consideRatio commented Oct 1, 2024

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

@consideRatio
Copy link
Member

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

@samyuh
Copy link
Author

samyuh commented Oct 12, 2024

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

@consideRatio
Copy link
Member

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

Background and overview

There are two "pre puller" machineries:

  • one that runs before a helm upgrade command is run which is temporary - it starts and stops. Its referred to as the hook pre puller (it involves a helm hook to run "before upgrade")
  • one that runs continuously, it never stops, so its called the contiunous pre puller.

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
Copy link
Author

samyuh commented Oct 15, 2024

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not possible to add a ServiceAccount to the Prepuller
3 participants