feat(session): fail when injecting env vars that exist in session (#1396

)

Co-authored-by: Tasko Olevski <olevski90@gmail.com>

renku_notebooks/api/amalthea_patches/jupyter_server.py

@@ -1,12 +1,13 @@
 from pathlib import Path
 from typing import TYPE_CHECKING, Any, Dict, List
 
+from renku_notebooks.errors.user import OverriddenEnvironmentVariableError
+
 if TYPE_CHECKING:
     from renku_notebooks.api.classes.server import UserServer
 
 
 def env(server: "UserServer"):
-    patches = []
     # amalthea always makes the jupyter server the first container in the statefulset
     patch_list = [
         {
@@ -66,8 +67,15 @@ def env(server: "UserServer"):
         },
     ]
 
+    env_vars = {p["value"]["name"]: p["value"]["value"] for p in patch_list}
+
     if server.environment_variables:
         for key, value in server.environment_variables.items():
+            if key in env_vars and value != env_vars[key]:
+                raise OverriddenEnvironmentVariableError(
+                    message=f"Cannot override environment variable '{key}'"
+                )
+
             patch_list.append(
                 {
                     "op": "add",

renku_notebooks/api/classes/server.py

@@ -16,7 +16,7 @@
 from ..amalthea_patches import inject_certificates as inject_certificates_patches
 from ..amalthea_patches import jupyter_server as jupyter_server_patches
 from ...config import config
-from ...errors.programming import ConfigurationError
+from ...errors.programming import ConfigurationError, DuplicateEnvironmentVariableError
 from ...errors.user import MissingResourceError
 from .k8s_client import K8sClient
 from .cloud_storage import ICloudStorageRequest
@@ -282,6 +282,9 @@ def _get_session_manifest(self):
                 inject_certificates_patches.proxy(self),
             )
         )
+
+        self._check_environment_variables_overrides(patches)
+
         # Storage
         if config.sessions.storage.pvs_enabled:
             storage = {
@@ -364,6 +367,30 @@ def _get_session_manifest(self):
         }
         return manifest
 
+    @staticmethod
+    def _check_environment_variables_overrides(patches_list):
+        """Check if any patch overrides server's environment variables with a different value,
+        or if two patches create environment variables with different values."""
+        env_vars = {}
+
+        for patch_list in patches_list:
+            patches = patch_list["patch"]
+
+            for patch in patches:
+                path = patch["path"].lower()
+                if path.endswith("/env/-"):
+                    name = patch["value"]["name"]
+                    value = patch["value"]["value"]
+                    key = (path, name)
+
+                    if key in env_vars and env_vars[key] != value:
+                        raise DuplicateEnvironmentVariableError(
+                            message=f"Environment variable {path}::{name} is being overridden by "
+                            "multiple patches"
+                        )
+                    else:
+                        env_vars[key] = value
+
     def start(self) -> Optional[Dict[str, Any]]:
         """Create the jupyterserver resource in k8s."""
         error = []

renku_notebooks/api/notebooks.py

@@ -169,7 +169,7 @@ def launch_notebook(
     image,
     server_options,
     environment_variables,
-    cloudstorage=[],
+    cloudstorage=None,
 ):
     """
     Launch a Jupyter server.
@@ -217,7 +217,7 @@ def launch_notebook(
         image,
         server_options,
         environment_variables,
-        cloudstorage,
+        cloudstorage or [],
         config.k8s.client,
     )
 

renku_notebooks/errors/programming.py

@@ -39,3 +39,11 @@ class FilteringResourcesError(ProgrammingError):
 
     message: str
     code: int = ProgrammingError.code + 2
+
+
+@dataclass
+class DuplicateEnvironmentVariableError(ProgrammingError):
+    """Raised when amalthea patches are overriding an env var with different values."""
+
+    message: str
+    code: int = ProgrammingError.code + 3

renku_notebooks/errors/user.py

@@ -68,3 +68,11 @@ class InvalidCloudStorageUrl(UserInputError):
 
     message: str = "The url for the cloud storage endpoint is not valid."
     code: int = UserInputError.code + 3
+
+
+@dataclass
+class OverriddenEnvironmentVariableError(UserInputError):
+    """Raised when an amalthea patch is overriding a session's env var with a different value."""
+
+    message: str
+    code: int = UserInputError.code + 4

tests/unit/test_server_class/test_manifest.py

@@ -1,7 +1,29 @@
+from typing import Any, Dict
+
 import pytest
 
 from renku_notebooks.api.classes.k8s_client import K8sClient
 from renku_notebooks.api.classes.server import UserServer
+from renku_notebooks.errors.programming import DuplicateEnvironmentVariableError
+from renku_notebooks.errors.user import OverriddenEnvironmentVariableError
+
+BASE_PARAMETERS = {
+    "namespace": "test-namespace",
+    "project": "test-project",
+    "image": None,
+    "server_options": {
+        "lfs_auto_fetch": 0,
+        "defaultUrl": "/lab",
+        "cpu_request": "100",
+        "mem_request": "100",
+        "disk_request": "100",
+    },
+    "branch": "master",
+    "commit_sha": "abcdefg123456789",
+    "notebook": "",
+    "environment_variables": {},
+    "cloudstorage": [],
+}
 
 
 # TODO: Add more tests https://github.com/SwissDataScienceCenter/renku-notebooks/issues/1145
@@ -23,33 +45,70 @@ def test_session_manifest(
     mocker,
 ):
     """Test that session manifest can be created correctly."""
-
-    user = user_with_project_path("namespace/project")
     with app.app_context():
-        mock_k8s_client = mocker.MagicMock(K8sClient)
-        base_parameters = {
-            "user": user,
-            "namespace": "test-namespace",
-            "project": "test-project",
-            "image": None,
-            "server_options": {
-                "lfs_auto_fetch": 0,
-                "defaultUrl": "/lab",
-                "cpu_request": "100",
-                "mem_request": "100",
-                "disk_request": "100",
-            },
-            "branch": "master",
-            "commit_sha": "abcdefg123456789",
-            "notebook": "",
-            "environment_variables": {},
-            "cloudstorage": [],
-            "k8s_client": mock_k8s_client,
-        }
+        base_parameters = BASE_PARAMETERS.copy()
+        base_parameters["user"] = user_with_project_path("namespace/project")
+        base_parameters["k8s_client"] = mocker.MagicMock(K8sClient)
 
         server = UserServer(**{**base_parameters, **parameters})
         server.image_workdir = ""
 
         manifest = server._get_session_manifest()
 
     assert expected in str(manifest)
+
+
+def test_session_env_var_override(
+    patch_user_server, user_with_project_path, app, mocker
+):
+    """Test that when a patch overrides session env vars an error is raised."""
+    with app.app_context():
+        parameters: Dict[str, Any] = BASE_PARAMETERS.copy()
+        parameters["user"] = user_with_project_path("namespace/project")
+        parameters["k8s_client"] = mocker.MagicMock(K8sClient)
+        # NOTE: NOTEBOOK_DIR is defined in ``jupyter_server.env`` patch
+        parameters["environment_variables"] = {"NOTEBOOK_DIR": "/some/path"}
+
+        server = UserServer(**parameters)
+        server.image_workdir = ""
+
+        with pytest.raises(OverriddenEnvironmentVariableError):
+            server._get_session_manifest()
+
+
+def test_patches_env_var_override(
+    patch_user_server, user_with_project_path, app, mocker
+):
+    """Test that when multiple patches define the same env vars with different values an error is
+    raised."""
+    general_patches = mocker.patch(
+        "renku_notebooks.api.classes.server.general_patches.oidc_unverified_email",
+        autospec=True,
+    )
+    # NOTE: Override ``jupyter_server.env::RENKU_USERNAME`` env var with a different value
+    general_patches.return_value = [
+        {
+            "type": "application/json-patch+json",
+            "patch": [
+                {
+                    "op": "add",
+                    "path": "/statefulset/spec/template/spec/containers/0/env/-",
+                    "value": {
+                        "name": "RENKU_USERNAME",
+                        "value": "some-different-value",
+                    },
+                },
+            ],
+        }
+    ]
+
+    with app.app_context():
+        parameters = BASE_PARAMETERS.copy()
+        parameters["user"] = user_with_project_path("namespace/project")
+        parameters["k8s_client"] = mocker.MagicMock(K8sClient)
+
+        server = UserServer(**parameters)
+        server.image_workdir = ""
+
+        with pytest.raises(DuplicateEnvironmentVariableError):
+            server._get_session_manifest()