fix: refuse Docker node updates that recreate a running container

Updating any Docker node property destroys and recreates the container
(the route calls DockerVM.update() as soon as the property diff is
non-empty), so it discards everything written inside it. On a running
node that happened silently, as a side effect of an unrelated edit: the
user only saw the node restart.

Refuse the update while the container is up and say what stopping it is
for, like QEMU, IOU, Dynamips, VPCS, VirtualBox and VMware already do
for their own restart-requiring settings. The check runs before anything
is applied — a refused update leaves the node untouched — and only when
the PUT really changes a property, since Web clients send the whole node
back on every edit.

is_running() is not reusable for the question: it reports a paused
container as not running and stops crashed containers as a side effect,
so DockerVM gains is_up() for "running or paused".

gns3server/api/routes/compute/docker_nodes.py

@@ -28,6 +28,7 @@ from fastapi.responses import StreamingResponse
 
 from gns3server import schemas
 from gns3server.compute.docker import Docker
+from gns3server.compute.docker.docker_error import DockerError
 from gns3server.compute.docker.docker_vm import DockerVM
 
 from .dependencies.authentication import compute_authentication, ws_compute_authentication
@@ -154,15 +155,28 @@ async def update_docker_node(node_update: schemas.DockerUpdate, node: DockerVM =
         "cpus",
     ]
 
-    changed = False
     node_data = jsonable_encoder(node_update, exclude_unset=True)
-    for prop in props:
-        # hasattr: startup_config_content only exists on IOLDockerVM
-        if prop in node_data and hasattr(node, prop) and node_data[prop] != getattr(node, prop):
-            setattr(node, prop, node_data[prop])
-            changed = True
+    # hasattr: startup_config_content only exists on IOLDockerVM
+    changed_props = [
+        prop for prop in props if prop in node_data and hasattr(node, prop) and node_data[prop] != getattr(node, prop)
+    ]
+
+    # A property change is applied by destroying and recreating the container,
+    # so it discards whatever was written inside it. Refuse that while the
+    # container is up (like QEMU, IOU, Dynamips, VPCS, VirtualBox and VMware do
+    # for their own restart-requiring settings) instead of resetting a running
+    # node behind the user's back. Nothing is applied before this check: a
+    # refused update must leave the node exactly as it was.
+    if changed_props and await node.is_up():
+        raise DockerError(
+            f'"{node.name}" must be stopped to change {", ".join(sorted(changed_props))}: '
+            "the container is recreated and changes made inside it are lost"
+        )
+
+    for prop in changed_props:
+        setattr(node, prop, node_data[prop])
     # We don't call container.update for nothing because it will restart the container
-    if changed:
+    if changed_props:
         await node.update()
     node.updated()
     return node.asdict()

gns3server/compute/docker/docker_vm.py

@@ -1395,6 +1395,21 @@ class DockerVM(DockerKernelDatapathMixin, BaseNode):
             await self.stop()
         return False
 
+    async def is_up(self):
+        """
+        Checks if the container exists in a running or paused state.
+
+        Both mean the container holds live state that recreating it would
+        discard. is_running() is not usable for that question: it reports a
+        paused container as not running, and it stops a crashed container as
+        a side effect.
+
+        :returns: True or False
+        :rtype: bool
+        """
+
+        return await self._get_container_state() in ("running", "paused")
+
     async def restart(self):
         """
         Restart this Docker container.

tests/api/routes/compute/test_docker_nodes.py

@@ -392,6 +392,71 @@ class TestDockerNodesRoutes:
         assert response.json()["start_command"] is None
         assert response.json()["console_http_path"] == "/"
 
+    async def test_docker_update_refused_while_container_is_up(
+        self, app: FastAPI, compute_client: AsyncClient, vm: dict
+    ) -> None:
+        """
+        A property change is applied by recreating the container, which throws
+        away what was written inside it. While the container is up (running or
+        paused) the update is refused instead of resetting the node behind the
+        user's back, and the error says what stopping it would cost.
+        """
+
+        for state in ("running", "paused"):
+            with asyncio_patch("gns3server.compute.docker.docker_vm.DockerVM.update") as mock:
+                with asyncio_patch(
+                    "gns3server.compute.docker.docker_vm.DockerVM._get_container_state", return_value=state
+                ):
+                    response = await compute_client.put(
+                        app.url_path_for(
+                            "compute:update_docker_node", project_id=vm["project_id"], node_id=vm["node_id"]
+                        ),
+                        json={"console_type": "vnc"},
+                    )
+
+            assert response.status_code == 409
+            assert "must be stopped" in response.json()["message"]
+            assert "console_type" in response.json()["message"]
+            assert not mock.called
+
+        # the refused update left the node exactly as it was
+        response = await compute_client.get(
+            app.url_path_for("compute:get_docker_node", project_id=vm["project_id"], node_id=vm["node_id"])
+        )
+        assert response.json()["console_type"] == "telnet"
+
+    async def test_docker_update_without_change_is_allowed_while_container_is_up(
+        self, app: FastAPI, compute_client: AsyncClient, vm: dict
+    ) -> None:
+        """
+        Clients send the whole node back on every edit: a PUT that changes
+        nothing must not be refused just because the container is running.
+        """
+
+        with asyncio_patch("gns3server.compute.docker.docker_vm.DockerVM._get_container_state", return_value="running"):
+            response = await compute_client.put(
+                app.url_path_for("compute:update_docker_node", project_id=vm["project_id"], node_id=vm["node_id"]),
+                json={"name": "DOCKER-TEST-1", "console_type": "telnet"},
+            )
+        assert response.status_code == 200
+
+    async def test_docker_update_stopped_container_is_recreated(
+        self, app: FastAPI, compute_client: AsyncClient, vm: dict
+    ) -> None:
+        """A stopped container has no live state to lose: the change applies."""
+
+        with asyncio_patch("gns3server.compute.docker.docker_vm.DockerVM.update") as mock:
+            with asyncio_patch(
+                "gns3server.compute.docker.docker_vm.DockerVM._get_container_state", return_value="exited"
+            ):
+                response = await compute_client.put(
+                    app.url_path_for("compute:update_docker_node", project_id=vm["project_id"], node_id=vm["node_id"]),
+                    json={"console_type": "vnc"},
+                )
+        assert response.status_code == 200
+        assert mock.called
+        assert response.json()["console_type"] == "vnc"
+
     async def test_docker_start_capture(self, app: FastAPI, compute_client: AsyncClient, vm: dict) -> None:
 
         url = app.url_path_for(