Merge pull request #1246 from Gujiassh/fix/k8s-propagate-pvc-readonly-545

fix(k8s): propagate pvc readonly flag

Author: Pangjiping

server/opensandbox_server/services/k8s/kubernetes_service.py

@@ -60,6 +60,7 @@
 from opensandbox_server.services.k8s.k8s_diagnostics import K8sDiagnosticsMixin
 from opensandbox_server.services.k8s.endpoint_resolver import _attach_egress_auth_headers, _attach_secure_access_headers
 from opensandbox_server.services.k8s.list_helpers import _build_list_sandboxes_response
+from opensandbox_server.services.k8s.volume_helper import ensure_shared_pvc_read_only_policy
 from opensandbox_server.services.k8s.status_helpers import (
     _is_unschedulable_status,
     _normalize_create_status,
@@ -803,6 +804,11 @@ async def create_sandbox(self, request: CreateSandboxRequest) -> CreateSandboxRe
             if has_pool_ref and pool_ref != POOL_AUTO_ASSIGN_REF:
                 await asyncio.to_thread(self._ensure_pool_ref_exists, pool_ref)
 
+            # Kubernetes applies PVC readOnly at the source volume level, so
+            # shared PVC mounts must agree before any auto-provisioning side effect.
+            if request.volumes:
+                ensure_shared_pvc_read_only_policy(request.volumes)
+
             # Auto-create PVCs that don't exist yet
             if request.volumes:
                 managed_pvcs_may_exist = True

server/opensandbox_server/services/k8s/volume_helper.py

@@ -24,6 +24,30 @@
 logger = logging.getLogger(__name__)
 
 
+def _raise_mixed_pvc_read_only_policy(pvc_claim_name: str) -> None:
+    raise ValueError(
+        f"PVC claim '{pvc_claim_name}' is mounted with mixed read_only values. "
+        "All mounts sharing the same PVC must use the same read_only policy."
+    )
+
+
+def ensure_shared_pvc_read_only_policy(volumes: List[Volume]) -> None:
+    """Ensure every mount that references the same PVC uses the same read_only policy."""
+    pvc_to_read_only: Dict[str, bool] = {}
+
+    for vol in volumes:
+        if vol.pvc is None:
+            continue
+
+        pvc_claim_name = vol.pvc.claim_name
+        if pvc_claim_name in pvc_to_read_only:
+            if pvc_to_read_only[pvc_claim_name] != vol.read_only:
+                _raise_mixed_pvc_read_only_policy(pvc_claim_name)
+            continue
+
+        pvc_to_read_only[pvc_claim_name] = vol.read_only
+
+
 def apply_volumes_to_pod_spec(
     pod_spec: Dict[str, Any],
     volumes: List[Volume],
@@ -38,6 +62,8 @@ def apply_volumes_to_pod_spec(
     mounts = main_container.get("volumeMounts", [])
     pod_volumes = pod_spec.get("volumes", [])
 
+    ensure_shared_pvc_read_only_policy(volumes)
+
     existing_volume_names = {v.get("name") for v in pod_volumes if isinstance(v, dict)}
     pvc_to_volume_name: Dict[str, str] = {}
 
@@ -58,6 +84,7 @@ def apply_volumes_to_pod_spec(
                     "name": vol_name,
                     "persistentVolumeClaim": {
                         "claimName": pvc_claim_name,
+                        "readOnly": vol.read_only,
                     },
                 })
                 pvc_to_volume_name[pvc_claim_name] = vol_name
@@ -73,7 +100,11 @@ def apply_volumes_to_pod_spec(
             mounts.append(mount)
 
             logger.info(
-                f"Added PVC volume '{vol_name}' (claim: {pvc_claim_name}) mounted at '{vol.mount_path}' for sandbox"
+                "Added PVC volume '%s' (claim: %s, read_only=%s) mounted at '%s' for sandbox",
+                pvc_to_volume_name[pvc_claim_name],
+                pvc_claim_name,
+                vol.read_only,
+                vol.mount_path,
             )
         elif vol.host is not None:
             host_path = vol.host.path

server/tests/k8s/test_batchsandbox_provider.py

@@ -2698,8 +2698,11 @@ def test_create_workload_with_pvc_volume_readonly(self, mock_k8s_client):
         main_container = pod_spec["containers"][0]
         mounts = main_container.get("volumeMounts", [])
         models_mount = next((m for m in mounts if m["name"] == "models-volume"), None)
+        models_volume = next((v for v in pod_spec["volumes"] if v["name"] == "models-volume"), None)
         assert models_mount is not None
         assert models_mount["readOnly"] is True
+        assert models_volume is not None
+        assert models_volume["persistentVolumeClaim"]["readOnly"] is True
 
     def test_create_workload_with_pvc_volume_subpath(self, mock_k8s_client):
         """
@@ -2961,7 +2964,10 @@ def test_apply_volumes_to_pod_spec_same_pvc_multiple_mounts(self, mock_k8s_clien
         # One volume definition for the shared PVC (first Volume name used)
         assert len(pod_spec["volumes"]) == 1
         assert pod_spec["volumes"][0]["name"] == "skills"
-        assert pod_spec["volumes"][0]["persistentVolumeClaim"]["claimName"] == "oss-pvc-r"
+        shared_volume = next((v for v in pod_spec["volumes"] if v["name"] == "skills"), None)
+        assert shared_volume is not None
+        assert shared_volume["persistentVolumeClaim"]["claimName"] == "oss-pvc-r"
+        assert shared_volume["persistentVolumeClaim"]["readOnly"] is True
 
         # Two volumeMounts, both referencing the same volume name
         mounts = pod_spec["containers"][0]["volumeMounts"]
@@ -2971,3 +2977,67 @@ def test_apply_volumes_to_pod_spec_same_pvc_multiple_mounts(self, mock_k8s_clien
         assert by_path["/path/to/skills"].get("subPath") == "skill-hub/publish"
         assert by_path["/path/to/draft"]["name"] == "skills"
         assert by_path["/path/to/draft"].get("subPath") == "skill-hub/draft"
+
+    def test_apply_volumes_to_pod_spec_same_pvc_multiple_mounts_readwrite(self, mock_k8s_client):
+        """Shared PVC mounts with read_only=False should keep source-level readOnly false."""
+        from opensandbox_server.api.schema import Volume, PVC
+
+        pod_spec = {
+            "containers": [{"name": "main", "volumeMounts": []}],
+            "volumes": [],
+        }
+        volumes = [
+            Volume(
+                name="skills",
+                pvc=PVC(claim_name="oss-pvc-rw"),
+                mount_path="/path/to/skills",
+                sub_path="skill-hub/publish",
+                read_only=False,
+            ),
+            Volume(
+                name="draft",
+                pvc=PVC(claim_name="oss-pvc-rw"),
+                mount_path="/path/to/draft",
+                sub_path="skill-hub/draft",
+                read_only=False,
+            ),
+        ]
+
+        apply_volumes_to_pod_spec(pod_spec, volumes)
+
+        assert len(pod_spec["volumes"]) == 1
+        shared_volume = next((v for v in pod_spec["volumes"] if v["name"] == "skills"), None)
+        assert shared_volume is not None
+        assert shared_volume["persistentVolumeClaim"]["claimName"] == "oss-pvc-rw"
+        assert shared_volume["persistentVolumeClaim"]["readOnly"] is False
+
+        mounts = pod_spec["containers"][0]["volumeMounts"]
+        assert len(mounts) == 2
+        assert all(mount["name"] == "skills" for mount in mounts)
+        assert all(mount["readOnly"] is False for mount in mounts)
+
+    def test_apply_volumes_to_pod_spec_same_pvc_mixed_read_only_raises(self, mock_k8s_client):
+        """Shared PVC mounts with mixed read_only values should fail fast."""
+        from opensandbox_server.api.schema import Volume, PVC
+
+        pod_spec = {
+            "containers": [{"name": "main", "volumeMounts": []}],
+            "volumes": [],
+        }
+        volumes = [
+            Volume(
+                name="skills",
+                pvc=PVC(claim_name="oss-pvc-mixed"),
+                mount_path="/path/to/skills",
+                read_only=False,
+            ),
+            Volume(
+                name="draft",
+                pvc=PVC(claim_name="oss-pvc-mixed"),
+                mount_path="/path/to/draft",
+                read_only=True,
+            ),
+        ]
+
+        with pytest.raises(ValueError, match="mixed read_only values"):
+            apply_volumes_to_pod_spec(pod_spec, volumes)

server/tests/k8s/test_kubernetes_service.py

@@ -1541,6 +1541,56 @@ async def test_pool_ref_with_volumes_rejected_before_pvc_creation(
         k8s_service.k8s_client.create_pvc.assert_not_called()
         k8s_service.workload_provider.create_workload.assert_not_called()
 
+    @pytest.mark.asyncio
+    async def test_shared_pvc_mixed_read_only_rejected_before_pvc_creation(
+        self, k8s_service
+    ):
+        from opensandbox_server.api.schema import (
+            CreateSandboxRequest,
+            ImageSpec,
+            PVC,
+            ResourceLimits,
+            Volume,
+        )
+
+        request = CreateSandboxRequest(
+            image=ImageSpec(uri="python:3.11"),
+            entrypoint=["/bin/bash", "-c", "sleep 3600"],
+            timeout=3600,
+            resourceLimits=ResourceLimits(root={"cpu": "1", "memory": "1Gi"}),
+            volumes=[
+                Volume(
+                    name="shared-rw",
+                    mountPath="/data/rw",
+                    readOnly=False,
+                    pvc=PVC(
+                        claimName="auto-data",
+                        createIfNotExists=True,
+                        deleteOnSandboxTermination=True,
+                    ),
+                ),
+                Volume(
+                    name="shared-ro",
+                    mountPath="/data/ro",
+                    readOnly=True,
+                    pvc=PVC(
+                        claimName="auto-data",
+                        createIfNotExists=True,
+                        deleteOnSandboxTermination=True,
+                    ),
+                ),
+            ],
+        )
+
+        with pytest.raises(HTTPException) as exc_info:
+            await k8s_service.create_sandbox(request)
+
+        assert exc_info.value.status_code == 400
+        assert exc_info.value.detail["code"] == SandboxErrorCodes.INVALID_PARAMETER
+        assert "mixed read_only values" in exc_info.value.detail["message"]
+        k8s_service.k8s_client.create_pvc.assert_not_called()
+        k8s_service.workload_provider.create_workload.assert_not_called()
+
     @pytest.mark.asyncio
     async def test_pvc_cleanup_runs_when_wait_for_ready_rollback_reports_not_found(
         self, k8s_service, mock_workload