-
Notifications
You must be signed in to change notification settings - Fork 96
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 service account with allow-app-sharing-role permissions #2917
base: main
Are you sure you want to change the base?
Conversation
...i/stages/kubernetes_services/template/modules/kubernetes/services/keycloak-client/outputs.tf
Show resolved
Hide resolved
I'm seeing some issues right now. The issue is the "jhub-apps-sa" user which creates the startup_apps needs to have logged in before the app server can be started successfully. In the current design, the "jhub-apps-sa" user is meant to act more like a service account and not log in interactively as users do. The issues I've seen so far are in the code that is run by the Spawner to set preferred username and render profiles, but it's possible there are more. In those instances, the auth state for the "jhub-apps-sa" user is None before initial log in so an error is thrown once we try to access info in the auth_state object in those methods. Some possible ideas on how to fix this:
@krassowski any thoughts on how best to do this? |
This might be too radical and not the kind of suggestion you are asking for, but could it be solved on the jhub-apps level? I mean jhub-apps is meant to be auth-provider agnostic so it should be possible to make this work without touching keycloak at all? This might be just my PTSD from figuring out keycloak piping last time speaking. I recall @aktech also had pleasure to work on keycloak integration - he may have better ideas. |
If staying with this approach I would probably try the solutions in that exact order. I am not sure if the last one will work if you need to do anything beyond the configuration being set (i.e. whether server will actually spawn if you do not have |
That would be ideal, indeed. The way we are using the spawner here, expects the user to be logged in (hence populating This (startup apps) works in jhub-apps (without nebari) by default unless the spawner needs We can try to see if we can somehow call the Also, another thing to note here, is supporting this in jhub-apps might be tricky (if its possible), its probably ok to just go for your approach and we can tackle it in jhub-apps later, if time is of essence here. |
I think the ideal solution is to create the startup apps up using
yields
which has the group membership for the service account, preferred_username, and permissions similar to a normal user. |
|
data "keycloak_role" "main-service" { | ||
for_each = toset(var.service-account-roles) | ||
# Get client data for each service account client | ||
data "keycloak_openid_client" "service_clients" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before we only allowed service accounts to get roles from the realm-management client. This PR allows us to set roles by any client. This functionality was needed to be able to set the allow-app-sharing-role on the jupyterhub service account.
@gen.coroutine | ||
def get_username_hook(spawner): | ||
auth_state = yield spawner.user.get_auth_state() | ||
async def get_username_hook(spawner): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flyby: tornado coroutine -> native coroutine. We don't need to use a tornado coroutine.
@@ -23,6 +21,13 @@ def get_username_hook(spawner): | |||
) | |||
|
|||
|
|||
async def pre_spawn_hook(spawner): | |||
# if we are starting a service account pod, set/update auth_state | |||
if spawner.user.name == "service-account-jupyterhub": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make service account name a variable?
@@ -547,14 +546,12 @@ def preserve_envvars(spawner): | |||
return profile | |||
|
|||
|
|||
@gen.coroutine | |||
def render_profiles(spawner): | |||
async def render_profiles(spawner): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flyby: tornado coroutine to native coroutine
@@ -46,15 +84,15 @@ async def update_auth_model(self, auth_model): | |||
user_id = auth_model["auth_state"]["oauth_user"]["sub"] | |||
token = await self._get_token() | |||
|
|||
jupyterhub_client_id = await self._get_jupyterhub_client_id(token=token) | |||
jupyterhub_client_uuid = await self._get_jupyterhub_client_uuid(token=token) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flyby: rename jupyterhub_client_id to jupyterhub_client_uuid so devs don't get confused with jupyterhub's client_id which is different.
The local integration test seems to be failing for unrelated reasons. |
Reference Issues or PRs
In order for startup apps to be used by nebari, we need to create a service account with appropriate permissions to create the apps and share them with others. One issue this caused was the auth state for the service account doesn't get populated and it is needed in our custom Spawner code. This PR updates the service account code during the pre-spawn-hook.
What does this implement/fix?
Put a
x
in the boxes that applyTesting
How to test this PR?
global
namespaceAny other comments?