Merge pull request #166 from lsst-sqre/tickets/DM-36093

[DM-36093] Update GID and group handling for Gafaelfawr changes
lsst-sqre · Sep 6, 2022 · f36219e · f36219e
2 parents 1826ed2 + a55bb9c
commit f36219e
Show file tree

Hide file tree

Showing 3 changed files with 33 additions and 10 deletions.
diff --git a/src/nublado2/hooks.py b/src/nublado2/hooks.py
@@ -35,8 +35,13 @@ async def pre_spawn(self, spawner: Spawner) -> None:
             spawner.gid = auth_state["gid"]
         else:
             spawner.gid = spawner.uid
+
+        # Not all groups may have GIDs.  Only those that do contribute to the
+        # supplemental GIDs.
         spawner.supplemental_gids = [
-            g["id"] for g in auth_state["groups"] if g["id"] != spawner.gid
+            g["id"]
+            for g in auth_state["groups"]
+            if "id" in g and g["id"] != spawner.gid
         ]
 
         # Since we will create a serviceaccount in the user resources,

diff --git a/src/nublado2/resourcemgr.py b/src/nublado2/resourcemgr.py
@@ -194,15 +194,21 @@ async def _build_template_values(
         auth_state = await spawner.user.get_auth_state()
         groups = auth_state["groups"]
 
-        # Build a comma separated list of group:gid
-        # ex: group1:1000,group2:1001,group3:1002
-        external_groups = ",".join([f'{g["name"]}:{g["id"]}' for g in groups])
+        # Build a comma-separated list of groups used to populate the
+        # /etc/groups file.  Only groups with GIDs can be added, so skip
+        # groups without GIDs.
+        #
+        # Example: group1:1000,group2:1001,group3:1002
+        external_groups = ",".join(
+            [f'{g["name"]}:{g["id"]}' for g in groups if "id" in g]
+        )
 
         # Define the template variables.
         template_values = {
             "user_namespace": spawner.namespace,
             "user": spawner.user.name,
             "uid": auth_state["uid"],
+            "gid": auth_state.get("gid"),
             "token": auth_state["token"],
             "groups": groups,
             "external_groups": external_groups,

diff --git a/tests/resourcemgr_test.py b/tests/resourcemgr_test.py
@@ -33,9 +33,12 @@
     name: group
     namespace: "{{ user_namespace }}"
   data:
+    user: |
+      {{user}}:x:{{uid}}:{{gid if gid else uid}}::/home/{{ user }}:/bin/bash
     group: |
-      {{user}}:x:{{uid}}:{% for group in groups %}
-      {{ group.name }}:x:{{ group.id }}:{{ user }}{% endfor %}
+      {%- for group in groups %}{% if "id" in group %}
+      {{ group.name }}:x:{{ group.id }}:\
+{{ user if group.id != gid else ""}}{% endif %}{% endfor %}
 - apiVersion: v1
   kind: ConfigMap
   metadata:
@@ -104,6 +107,7 @@ def config_mock() -> Iterator[None]:
             "FIREFLY_ROUTE": "/portal/app",
             "HUB_ROUTE": "{{ nublado_base_url }}",
             "EXTERNAL_GROUPS": "{{ external_groups }}",
+            "EXTERNAL_GID": "{{ gid }}",
             "EXTERNAL_UID": "{{ uid }}",
             "ACCESS_TOKEN": "{{ token }}",
             "IMAGE_DIGEST": "{{ options.image_info.digest }}",
@@ -151,7 +155,13 @@ async def test_create_kubernetes_resources(
     auth_state = {
         "token": "user-token",
         "uid": 1234,
-        "groups": [{"name": "foo", "id": 1235}, {"name": "bar", "id": 4567}],
+        "gid": 1551,
+        "groups": [
+            {"name": "foo", "id": 1235},
+            {"name": "primary", "id": 1551},
+            {"name": "bar", "id": 4567},
+            {"name": "baz"},
+        ],
     }
     pod_manifest = V1Pod(
         api_version="v1",
@@ -235,11 +245,12 @@ async def test_create_kubernetes_resources(
                 "labels": spawner.extra_labels,
             },
             "data": {
+                "user": "someuser:x:1234:1551::/home/someuser:/bin/bash\n",
                 "group": (
-                    "someuser:x:1234:\n"
                     "foo:x:1235:someuser\n"
+                    "primary:x:1551:\n"
                     "bar:x:4567:someuser\n"
-                )
+                ),
             },
         },
         {
@@ -255,7 +266,8 @@ async def test_create_kubernetes_resources(
                 "EXTERNAL_INSTANCE_URL": "https://data.example.com/",
                 "FIREFLY_ROUTE": "/portal/app",
                 "HUB_ROUTE": "/nb/hub/",
-                "EXTERNAL_GROUPS": "foo:1235,bar:4567",
+                "EXTERNAL_GID": "1551",
+                "EXTERNAL_GROUPS": "foo:1235,primary:1551,bar:4567",
                 "EXTERNAL_UID": "1234",
                 "ACCESS_TOKEN": "user-token",
                 "IMAGE_DIGEST": "sha256:123456789abcdef",