-
Notifications
You must be signed in to change notification settings - Fork 97
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
Refactor shared group mounting using RBAC #2593
Changes from 8 commits
9e38ab1
ff74a0d
5ca8dbc
6a8e000
bb7a509
fcbd4b6
8025f0c
afca632
c5f651c
7bc8a04
b1336f8
a100a44
8f05abb
7dd72cd
3324d30
5713783
742f7fe
c955e6a
fc349d9
d2cbb94
53cfcd3
655b743
6f0ad66
e448f17
c1e0a95
0c5d9aa
3ce8e88
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -82,11 +82,11 @@ def base_profile_home_mounts(username): | |
} | ||
|
||
|
||
def base_profile_shared_mounts(groups): | ||
def base_profile_shared_mounts(groups, group_roles): | ||
"""Configure the group directory mounts for user. | ||
|
||
Ensure that {shared}/{group} directory exists and user has | ||
permissions to read/write/execute. Kubernetes does not allow the | ||
Ensure that {shared}/{group} directory exists based on the scope availability | ||
and if user has permissions to read/write/execute. Kubernetes does not allow the | ||
same pvc to be a volume thus we must check that the home and share | ||
pvc are not the same for some operation. | ||
|
||
|
@@ -103,40 +103,50 @@ def base_profile_shared_mounts(groups): | |
{"name": "shared", "persistentVolumeClaim": {"claimName": shared_pvc_name}} | ||
) | ||
|
||
extra_container_config = { | ||
"volumeMounts": [ | ||
{ | ||
"mountPath": pod_shared_mount_path.format(group=group), | ||
"name": "shared" if home_pvc_name != shared_pvc_name else "home", | ||
"subPath": pvc_shared_mount_path.format(group=group), | ||
} | ||
for group in groups | ||
] | ||
} | ||
groups_to_volume_mount = [] | ||
for group in groups: | ||
for roles in group_roles.get(group, []): | ||
# Check if the group has a role that has a shared-directory scope | ||
if "shared-directory" in roles.get("attributes", {}).get("component", []): | ||
groups_to_volume_mount.append(group) | ||
break | ||
|
||
extra_container_config = {"volumeMounts": []} | ||
|
||
MKDIR_OWN_DIRECTORY = "mkdir -p /mnt/{path} && chmod 777 /mnt/{path}" | ||
command = " && ".join( | ||
[ | ||
MKDIR_OWN_DIRECTORY.format(path=pvc_shared_mount_path.format(group=group)) | ||
for group in groups | ||
for group in groups_to_volume_mount | ||
] | ||
) | ||
|
||
init_containers = [ | ||
{ | ||
"name": "initialize-shared-mounts", | ||
"image": "busybox:1.31", | ||
"command": ["sh", "-c", command], | ||
"securityContext": {"runAsUser": 0}, | ||
"volumeMounts": [ | ||
{ | ||
"mountPath": f"/mnt/{pvc_shared_mount_path.format(group=group)}", | ||
"name": "shared" if home_pvc_name != shared_pvc_name else "home", | ||
"subPath": pvc_shared_mount_path.format(group=group), | ||
} | ||
for group in groups | ||
], | ||
"volumeMounts": [], | ||
} | ||
] | ||
|
||
for groups in groups_to_volume_mount: | ||
viniciusdc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
extra_container_config["volumeMounts"].append( | ||
{ | ||
"mountPath": pod_shared_mount_path.format(group=groups), | ||
"name": "shared" if home_pvc_name != shared_pvc_name else "home", | ||
"subPath": pvc_shared_mount_path.format(group=groups), | ||
} | ||
) | ||
init_containers[0]["volumeMounts"].append( | ||
{ | ||
"mountPath": f"/mnt/{pvc_shared_mount_path.format(group=groups)}", | ||
"name": "shared" if home_pvc_name != shared_pvc_name else "home", | ||
"subPath": pvc_shared_mount_path.format(group=groups), | ||
} | ||
) | ||
|
||
return { | ||
"extra_pod_config": extra_pod_config, | ||
"extra_container_config": extra_container_config, | ||
|
@@ -475,7 +485,7 @@ def profile_conda_store_viewer_token(): | |
} | ||
|
||
|
||
def render_profile(profile, username, groups, keycloak_profilenames): | ||
def render_profile(profile, username, groups, keycloak_profilenames, group_roles): | ||
"""Render each profile for user. | ||
|
||
If profile is not available for given username, groups returns | ||
|
@@ -513,7 +523,7 @@ def render_profile(profile, username, groups, keycloak_profilenames): | |
deep_merge, | ||
[ | ||
base_profile_home_mounts(username), | ||
base_profile_shared_mounts(groups), | ||
base_profile_shared_mounts(groups, group_roles), | ||
profile_conda_store_mounts(username, groups), | ||
base_profile_extra_mounts(), | ||
configure_user(username, groups), | ||
|
@@ -543,13 +553,35 @@ def preserve_envvars(spawner): | |
return profile | ||
|
||
|
||
def parse_roles(data): | ||
parsed_roles = {} | ||
|
||
for role in data: | ||
for group in role["groups"]: | ||
# group = str(group).replace("/", "") | ||
group_name = Path(group).name | ||
if group_name not in parsed_roles: | ||
parsed_roles[group_name] = [] | ||
|
||
role_info = { | ||
"description": role["description"], | ||
"name": role["name"], | ||
"attributes": role["attributes"], | ||
} | ||
|
||
parsed_roles[group_name].append(role_info) | ||
|
||
return parsed_roles | ||
|
||
|
||
@gen.coroutine | ||
def render_profiles(spawner): | ||
# jupyterhub does not yet manage groups but it will soon | ||
# so for now we rely on auth_state from the keycloak | ||
# userinfo request to have the groups in the key | ||
# "auth_state.oauth_user.groups" | ||
auth_state = yield spawner.user.get_auth_state() | ||
group_roles = parse_roles(spawner.authenticator.group_roles_map) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is going to cause problem in jhub-apps: https://github.com/nebari-dev/jhub-apps/blob/657f7c871003e5f678d865b2b4aad1845e3b5bf7/jhub_apps/service/utils.py#L53 Since that's a downstream problem, it's not blocking this but I would need to think of an alternative approach in jhub-apps to get the spawner with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can add the extra information to the auth_state object (probably), this way you don't need to customize the above mock spawner, and we will not need to rely on Though, the reason I didn't go for it was that I was skeptical of changing it directly as
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will have a try on that, and check how it goes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll check if there is a less hacky way to do this on the jhub-apps. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually there isn't a non-hacky way to do this in jhub-apps. In native JupyterHub the spawner object is magically available due to the huge initialisation done at the startup. Ref:
We do need to set the groups via |
||
|
||
username = auth_state["oauth_user"]["preferred_username"] | ||
# only return the lowest level group name | ||
|
@@ -566,7 +598,7 @@ def render_profiles(spawner): | |
filter( | ||
None, | ||
[ | ||
render_profile(p, username, groups, keycloak_profilenames) | ||
render_profile(p, username, groups, keycloak_profilenames, group_roles) | ||
for p in profile_list | ||
], | ||
) | ||
|
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.
Is there any reason we need to loop through groups to create a list, then loop through that list to create a list of volume mounts and a list of commands and a list of init containers??
Couldn't we just generate the list of volume mounts, commands and init containers in the original loop and just use them later? That would be more readable and reduce the loops from 4 to 1
It may even be worth extracting that loop into a seperate function that takes a list of groups and group roles in and returns the 3 lists that you need further in this function.
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.
I complelty agree; though most of this was already here.If possible, I would prefer to include enhancements in a separate PR. No strong opinions, if you also consider this would suit this PR I can change that as well 😄