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

Add dummy service accounts to hook-image-puller and continuous-image-puller pods #3594

Merged
merged 5 commits into from
Jan 12, 2025

Conversation

samyuh
Copy link
Contributor

@samyuh samyuh commented Jan 10, 2025

This MR adds two separate dummy ServiceAccounts for hook-image-puller and continuous-hook-image-puller.

  • The continuous image puller runs constantly to ensure images are always available.
  • The hook image puller is temporary, used during tasks like upgrades, and is deleted afterward.

Closes #3545

@consideRatio
Copy link
Member

I'm super low on time, but lets try this - do your best to address the review comments, and i'll try get it through to a merge by finishing details this weekend.

Details if you want to try work those as well, from the top of my head would include:

  • ensure lint and validate values can trial configure this if new config is introduced
  • render and check labels etc align with how other continious/hook resources have labels
    I'm not able to really think properly about this now as i'm taking care of my toddler, sneaking in a review effort quickly from my mobile

@samyuh
Copy link
Contributor Author

samyuh commented Jan 10, 2025

Thanks for the feedback, I will address everything today.

@samyuh
Copy link
Contributor Author

samyuh commented Jan 10, 2025

Tests are failing since https://mybinder.org/ is down.

@samyuh samyuh requested a review from consideRatio January 10, 2025 16:05
@consideRatio consideRatio changed the title feat: Add service account to hook-image-puller and continuous-image-puller Add dummy service accounts to hook-image-puller and continuous-image-puller pods Jan 10, 2025
@consideRatio
Copy link
Member

consideRatio commented Jan 10, 2025

@samyuh great work! I pushed two detail commits.

@manics do you have time for a quick gut-check evaluation? This PR adds two dummy service accounts to be used by the the image-puller pods to please various security checks as discussed in #3545 (comment).

Do we make them enabled by default, or not?

I'm leaning towards enabled by default to reduce the hassle for anyone needing to comply with these benchmarks etc, but I'm not fully confident it has no drawbacks for users in other situations.

@samyuh
Copy link
Contributor Author

samyuh commented Jan 10, 2025

@samyuh great work! I pushed two detail commits.

@manics do you have time for a quick gut-check evaluation? This PR adds two dummy service accounts to be used by the the image-puller pods to please various security checks as discussed in #3545 (comment).

Do we make them enabled by default, or not?

I'm leaning towards enabled by default to reduce the hassle for anyone needing to comply with these benchmarks etc, but I'm not fully confident it has no drawbacks for users in other situations.

Thanks for your effort and your review!

@samyuh
Copy link
Contributor Author

samyuh commented Jan 12, 2025

I rebased my commits to keep the history clean and avoid including unnecessary commits.

@consideRatio consideRatio merged commit d94c113 into jupyterhub:main Jan 12, 2025
14 of 15 checks passed
@consideRatio
Copy link
Member

Thank you @samyuh, lets get this merged!!! ❤️ 🎉

consideRatio pushed a commit to jupyterhub/helm-chart that referenced this pull request Jan 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

hook and continuous image pullers' DaemonSets: configuring k8s ServiceAccount - yes or no?
2 participants