Merge pull request #3005 from yueguobin/fix/node-update-while-running

fix: refuse Docker node updates that would recreate a running container

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
-        # hasattr: startup_config_content only exists on IOLDockerVM
+    changed_props = [
-        if prop in node_data and hasattr(node, prop) and node_data[prop] != getattr(node, prop):
+        prop for prop in props if prop in node_data and hasattr(node, prop) and node_data[prop] != getattr(node, prop)
-            setattr(node, prop, node_data[prop])
+    ]
-            changed = True
+
+    # 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.

gns3server/controller/node.py

@@ -711,6 +711,9 @@ class Node:
         old_properties = copy.deepcopy(self._properties)
         old_custom_adapters = copy.deepcopy(self._custom_adapters)
         old_missing_images = copy.deepcopy(self._missing_images)
+        # Values applied locally below, kept to roll the node back if the
+        # compute refuses the update (see the ComputeConflictError handler).
+        old_prop_values = {prop: getattr(self, prop) for prop in kwargs if prop != "properties"}
 
         compute_properties = None
         # Update node properties with additional elements
@@ -766,9 +769,20 @@ class Node:
             try:
                 response = await self.put(None, data=data)
             except ComputeConflictError:
+                # The compute refused the update: roll back everything applied
+                # locally. Keeping the requested values would leave the
+                # controller describing a node the compute never accepted, and
+                # the next project dump persists them — a refused change would
+                # then silently land the next time the node is created.
+                for prop, old_value in old_prop_values.items():
+                    setattr(self, prop, old_value)
                 if old_name != self.name:
                     # special case when the new name is already updated on controller but refused by the compute
                     self.name = old_name
+                self._properties = old_properties
+                self._custom_adapters = old_custom_adapters
+                self._missing_images = old_missing_images
+                self._list_ports()
                 raise
             await self.parse_node_response(response.json)
         elif old_json != self.asdict():

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(

tests/controller/test_node.py

@@ -811,6 +811,36 @@ async def test_update_properties(node, compute, controller):
     # controller._notification.emit.assert_called_with("node.updated", node_notif)
 
 
+@pytest.mark.asyncio
+async def test_update_rolls_back_when_compute_refuses(node, compute, project, controller):
+    """
+    Values applied locally before the PUT must be rolled back when the compute
+    refuses the update: kept in memory they describe a node the compute never
+    accepted, and the next project dump persists them — the refused change
+    would then silently land the next time the node is created.
+    """
+
+    async def resp(*args, **kwargs):
+        raise ComputeConflictError(
+            f"/projects/{project.id}/vpcs/nodes/{node.id}",
+            {"message": '"demo" must be stopped to change the console type to telnet'},
+        )
+
+    compute.put = AsyncioMagicMock(side_effect=resp)
+    controller._notification = AsyncioMagicMock()
+    project.dump = MagicMock()
+    old_json = node.asdict()
+
+    with pytest.raises(ComputeConflictError):
+        await node.update(x=42, console_type="telnet", name="renamed", properties={"startup_script": "changed"})
+
+    assert node.name == "demo"
+    assert node.console_type == "vnc"
+    assert node.properties == {"startup_script": "echo test"}
+    assert node.asdict() == old_json
+    assert not project.dump.called
+
+
 @pytest.mark.asyncio
 async def test_update_only_controller(node, compute):
     """