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

basehub: add singleuserAdmin.serviceAccountName config and small refactoring #3039

Conversation

consideRatio
Copy link
Contributor

@consideRatio consideRatio commented Aug 28, 2023

I think it could be useful to be able to provide admin users with a different k8s ServiceAccount than for other users, as that would enable us grant them different cloud permissions.

This may be a pre-requisite of resolving #3038 and #2886.

What this PR includes in detail

  1. refactoring to de-nest code for some improved readability
  2. cleanup of not-functional and unused custom.singleuserAdmin.initContainers
  3. refactoring of internal variable name volume to vm (for volume_mount)
  4. adding of not-yet-used custom.singleuserAdmin.serviceAccountName as its a starting point for [Support] cryocloud: Allow bucket access based on hub admin status #2886

@github-actions

This comment was marked as resolved.

@consideRatio consideRatio marked this pull request as ready for review August 28, 2023 08:20
@consideRatio consideRatio requested a review from a team as a code owner August 28, 2023 08:20
@GeorgianaElena
Copy link
Member

There was a similar request from another community a few weeks ago #2886 that this PR might help solve

Copy link
Member

@GeorgianaElena GeorgianaElena left a comment

Choose a reason for hiding this comment

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

@consideRatio, I don't think I understand how this PR as it is right now, could allow admins to map to a different SA. I think this is because it is not clear for me which is the role that KubeSpawner.service_account plays, given our current setup for bucket access.

It is my understanding that right now all user pods map to a K8s SA that in turn, maps to a cloud specific service account. Can you please help me understand what happens if we set through KubeSpawner.service_account another sa for admins? How does this override the infrastructure setup? Where does this new admin sa gets created? do we do it manually?
Thanks and sorry for the confusion.

@consideRatio
Copy link
Contributor Author

consideRatio commented Aug 28, 2023

Thank you for reviewing @GeorgianaElena!! I should have written up more about this beforehand =/

An overview as I understand it currently:

  • A user pod can get a serviceAccountName specified, and based on that get various permissions via env vars or mounted files
  • KubeSpawner.service_account, which can be set via the chart config singleuser.serviceAccountName, influences the user pod's serviceAccountName
  • The jupyterhub chart and the basehub chart defaults to not set this
  • The daskhub chart on the other hand sets this to user-sa
  • The basehub chart includes a chart template to create a k8s ServiceAccount resource with an annotation, making such ServiceAccount used by a pod get relevant credentials from the annotation coupled cloud account on both GCP and AWS thanks to cloud specific stuff

So I think there are reletad work and things to consider:

  • creating separated cloud identities and associated permissions for admin and non-admin users via terraform variables
  • creating separated k8s ServiceAccounts for admin and non-admin users via basehub chart
  • mounting separated k8s ServiceAccounts for admin and non-admin users via basehub chart's singleuserAdmin.serviceAccountName config (this PR)

This PR is meant as an incremental step to offload us from getting everything done at the same time.

@consideRatio consideRatio changed the title basehub: add singleuserAdmin.serviceAccountName config basehub: add singleuserAdmin.serviceAccountName config and small refactoring Oct 18, 2023
@consideRatio consideRatio force-pushed the pr/add-singleuser-admin-serviceaccountname branch from 1425e8d to 1d60dba Compare October 18, 2023 13:54
Copy link
Contributor Author

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

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

@GeorgianaElena I updated the title, could you re-review this?

Comment on lines +600 to +598
if not (self.user.admin and custom_admin):
return super().start(*args, **kwargs)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is opinionated refactoring to have us avoid nesting into if statements and make the code a bit flatter.

@consideRatio consideRatio merged commit e5772f9 into 2i2c-org:master Oct 19, 2023
31 checks passed
@github-actions
Copy link

🎉🎉🎉🎉

Monitor the deployment of the hubs here 👉 https://github.com/2i2c-org/infrastructure/actions/runs/6577624909

@consideRatio
Copy link
Contributor Author

Thank you for reviewing @GeorgianaElena!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done 🎉
Development

Successfully merging this pull request may close these issues.

2 participants