Merge pull request #2868 from yueguobin/feat/template-delete-usage-guard

feat: forbid deleting templates and images still used by projects

gns3server/api/routes/controller/images.py

@@ -197,11 +197,16 @@ async def prune_images(
     """
     Prune images not attached to any template.
 
+    Images referenced by a node in any project (opened or closed) are kept.
+
     Required privilege: Image.Allocate
     """
 
     skip_images = get_builtin_disks()
-    await images_repo.prune_images(skip_images)
+    # a single pass over all projects' node properties protects every
+    # referenced file name at once
+    referenced_filenames = Controller.instance().collect_referenced_image_filenames()
+    await images_repo.prune_images(list(skip_images) + list(referenced_filenames))
 
 
 @router.post(
@@ -309,6 +314,10 @@ async def delete_image(
         template_names = ", ".join([template.name for template in templates])
         raise ControllerError(f"Image '{image_path}' is used by one or more templates: {template_names}")
 
+    project_names = Controller.instance().find_projects_using_image(image.filename)
+    if project_names:
+        raise ControllerError(f"Image '{image_path}' is used by one or more projects: {', '.join(project_names)}")
+
     try:
         os.remove(image.path)
     except OSError:

gns3server/api/routes/controller/templates.py

@@ -35,6 +35,7 @@ from gns3server.db.repositories.templates import TemplatesRepository
 from gns3server.services.templates import TemplatesService
 from gns3server.db.repositories.rbac import RbacRepository
 from gns3server.db.repositories.images import ImagesRepository
+from gns3server.controller import Controller
 from gns3server.controller.controller_error import ControllerError, ControllerBadRequestError
 from gns3server.utils.images import get_builtin_disks
 
@@ -135,28 +136,49 @@ async def delete_template(
     Required privilege: Template.Allocate
     """
 
+    controller = Controller.instance()
     images = await templates_repo.get_template_images(template_id)
-    await TemplatesService(templates_repo).delete_template(template_id)
-    await rbac_repo.delete_all_ace_starting_with_path(f"/templates/{template_id}")
+
+    # Run every usage check before mutating anything, so a refused deletion
+    # cannot leave the template gone while its images survive.
+    images_to_prune = []
     if prune_images and images:
         skip_images = get_builtin_disks()
+        # collected lazily: a single pass over all projects' node properties
+        referenced_filenames = None
         for image in images:
             if image.filename in skip_images:
                 continue
-            templates = await images_repo.get_image_templates(image.image_id)
-            if templates:
-                template_names = ", ".join([template.name for template in templates])
+            # the template being deleted is still in the database at this
+            # point, exclude it from the other-templates check
+            other_templates = [
+                template for template in await images_repo.get_image_templates(image.image_id)
+                if str(template.template_id) != str(template_id)
+            ]
+            if other_templates:
+                template_names = ", ".join([template.name for template in other_templates])
                 raise ControllerError(f"Image '{image.path}' is used by one or more templates: {template_names}")
 
-            try:
-                os.remove(image.path)
-            except OSError:
-                log.warning(f"Could not delete image file {image.path}")
+            if referenced_filenames is None:
+                referenced_filenames = controller.collect_referenced_image_filenames()
+            if image.filename in referenced_filenames:
+                project_names = controller.find_projects_using_image(image.filename)
+                raise ControllerError(f"Image '{image.path}' is used by one or more projects: {', '.join(project_names)}")
+            images_to_prune.append(image)
 
-            print(f"Deleting image '{image.path}'")
-            success = await images_repo.delete_image(image.path)
-            if not success:
-                raise ControllerError(f"Image '{image.path}' could not removed from the database")
+    await TemplatesService(templates_repo).delete_template(template_id)
+    await rbac_repo.delete_all_ace_starting_with_path(f"/templates/{template_id}")
+
+    for image in images_to_prune:
+        try:
+            os.remove(image.path)
+        except OSError:
+            log.warning(f"Could not delete image file {image.path}")
+
+        log.info(f"Deleting image '{image.path}'")
+        success = await images_repo.delete_image(image.path)
+        if not success:
+            raise ControllerError(f"Image '{image.path}' could not removed from the database")
 
 
 @router.get(

gns3server/controller/__init__.py

@@ -38,6 +38,7 @@ from ..utils.images import default_images_directory
 from ..utils.asyncio import wait_run_in_executor
 
 from .project import Project
+from .node import Node
 from .appliance import Appliance
 from .appliance_manager import ApplianceManager
 from .compute import Compute, ComputeError
@@ -91,6 +92,35 @@ class _ProjectsDirectoryEventHandler(FileSystemEventHandler):
                     return
 
 
+def _iter_string_values(value):
+    """
+    Yield every string contained in value, recursing into dicts and lists.
+    """
+
+    if isinstance(value, str):
+        yield value
+    elif isinstance(value, dict):
+        for child in value.values():
+            yield from _iter_string_values(child)
+    elif isinstance(value, (list, tuple, set)):
+        for child in value:
+            yield from _iter_string_values(child)
+
+
+def _image_referenced(node_properties, image_filename: str) -> bool:
+    """
+    Whether a node's properties reference an image by file name.
+
+    Image references live under type-specific property keys
+    (hda_disk_image, hda_disk_image_backing_file, image, initrd, path...)
+    and are stored either as bare file names or as absolute paths
+    depending on topology age, so every string value is matched on its
+    base name instead of walking a fixed key list.
+    """
+
+    return any(os.path.basename(string) == image_filename for string in _iter_string_values(node_properties))
+
+
 class Controller:
     """
     The controller is responsible to manage one or more computes.
@@ -811,6 +841,82 @@ class Controller:
                 raise ControllerError("A project name could not be allocated (node limit reached?)")
         return new_name
 
+    def _iter_project_nodes(self):
+        """
+        Iterate over (project, node) for every known project, opened or closed.
+
+        Opened projects yield controller Node objects; closed projects yield
+        the raw node dicts read from their .gns3 file. A project whose
+        topology cannot be read is skipped — it must not block the usage
+        checks that rely on this iterator.
+        """
+
+        for project in self._projects.values():
+            try:
+                nodes = project.nodes.values()
+            except ControllerError:
+                continue
+            for node in nodes:
+                yield project, node
+
+    def find_projects_using_template(self, template_id) -> list:
+        """
+        Return the names of the projects with at least one node created
+        from this template.
+
+        Used to forbid template deletion while any project still
+        references it.
+        """
+
+        template_id = str(template_id)
+        project_names = []
+        for project, node in self._iter_project_nodes():
+            if isinstance(node, Node):
+                node_template_id = node.template_id
+            else:
+                node_template_id = node.get("template_id")
+            if node_template_id and str(node_template_id) == template_id and project.name not in project_names:
+                project_names.append(project.name)
+        return project_names
+
+    def find_projects_using_image(self, image_filename: str) -> list:
+        """
+        Return the names of the projects with at least one node whose
+        properties reference this image file.
+
+        Used to forbid image deletion while any project still uses it.
+        """
+
+        project_names = []
+        for project, node in self._iter_project_nodes():
+            if isinstance(node, Node):
+                node_properties = node.properties
+            else:
+                node_properties = node.get("properties") or {}
+            if _image_referenced(node_properties, image_filename) and project.name not in project_names:
+                project_names.append(project.name)
+        return project_names
+
+    def collect_referenced_image_filenames(self) -> set:
+        """
+        Return every file name referenced by a node property across all
+        projects (opened or closed).
+
+        The set deliberately over-approximates (any property string counts):
+        it protects images from pruning, so a false positive only keeps a
+        file alive. Used as a single-pass skip list for image pruning.
+        """
+
+        filenames = set()
+        for _project, node in self._iter_project_nodes():
+            if isinstance(node, Node):
+                node_properties = node.properties
+            else:
+                node_properties = node.get("properties") or {}
+            for string in _iter_string_values(node_properties):
+                filenames.add(os.path.basename(string))
+        return filenames
+
     @property
     def projects(self):
         """

gns3server/controller/node.py

@@ -162,6 +162,10 @@ class Node:
     def status(self):
         return self._status
 
+    @property
+    def template_id(self):
+        return self._template_id
+
     @property
     def name(self):
         return self._name

gns3server/services/templates.py

@@ -341,6 +341,15 @@ class TemplatesService:
 
         if self.get_builtin_template(template_id):
             raise ControllerForbiddenError(f"Template '{template_id}' cannot be deleted because it is built-in")
+
+        template = await self.get_template(template_id)
+        project_names = self._controller.find_projects_using_template(template_id)
+        if project_names:
+            raise ControllerError(
+                f"Template '{template['name']}' cannot be deleted because it is used by "
+                f"one or more projects: {', '.join(project_names)}"
+            )
+
         if await self._templates_repo.delete_template(template_id):
             self._controller.notification.controller_emit("template.deleted", {"template_id": str(template_id)})
         else:

tests/api/routes/controller/test_images.py

@@ -16,6 +16,8 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import os
+import json
+import uuid
 import pytest
 import hashlib
 
@@ -282,6 +284,99 @@ class TestImageRoutes:
         images_in_db = await images_repo.get_images()
         assert len(images_in_db) == 0
 
+    @staticmethod
+    async def _add_closed_project_with_node(controller: Controller, name: str, node: dict):
+        """
+        Register a closed project whose .gns3 contains a single node,
+        mirroring how the controller picks up existing projects from disk.
+        """
+
+        project_dir = os.path.join(controller.projects_directory(), name)
+        os.makedirs(project_dir)
+        project_id = str(uuid.uuid4())
+        topology = {
+            "name": name,
+            "project_id": project_id,
+            "topology": {"computes": [], "links": [], "drawings": [], "nodes": [node]},
+        }
+        with open(os.path.join(project_dir, f"{name}.gns3"), "w+") as f:
+            json.dump(topology, f)
+        return await controller.add_project(
+            project_id=project_id, name=name, path=project_dir,
+            filename=f"{name}.gns3", status="closed",
+        )
+
+    async def test_image_delete_used_by_project(
+            self,
+            app: FastAPI,
+            client: AsyncClient,
+            controller: Controller,
+            db_session: AsyncSession,
+            tmpdir: str,
+    ) -> None:
+        """
+        An image referenced by a node in a project must not be deleted,
+        even when the project is closed and no template uses the image.
+        """
+
+        image_path = os.path.join(tmpdir, "used.qcow2")
+        with open(image_path, "wb+") as f:
+            f.write(b'\x42\x42\x42\x42')
+
+        images_repo = ImagesRepository(db_session)
+        await images_repo.add_image("used.qcow2", "qemu", 42, image_path, "e342eb86c1229b6c154367a5476969b5", "md5")
+
+        guarded_project = await self._add_closed_project_with_node(
+            controller, "Guarded",
+            {"node_id": str(uuid.uuid4()), "node_type": "qemu", "compute_id": "local",
+             "name": "n1", "properties": {"hda_disk_image_backing_file": "used.qcow2"}},
+        )
+
+        response = await client.delete(app.url_path_for("delete_image", image_path="used.qcow2"))
+        assert response.status_code == status.HTTP_409_CONFLICT
+        assert "Guarded" in response.json()["message"]
+        assert os.path.exists(image_path)
+
+        # once no project uses it anymore the deletion goes through
+        controller.remove_project(guarded_project)
+        response = await client.delete(app.url_path_for("delete_image", image_path="used.qcow2"))
+        assert response.status_code == status.HTTP_204_NO_CONTENT
+        assert not os.path.exists(image_path)
+
+    async def test_prune_images_keeps_project_referenced(
+            self,
+            app: FastAPI,
+            client: AsyncClient,
+            controller: Controller,
+            db_session: AsyncSession,
+            tmpdir: str,
+    ) -> None:
+        """
+        Pruning must keep images referenced by a project node while
+        removing the unreferenced ones.
+        """
+
+        images_repo = ImagesRepository(db_session)
+        for filename in ("used.qcow2", "unused.qcow2"):
+            image_path = os.path.join(tmpdir, filename)
+            with open(image_path, "wb+") as f:
+                f.write(b'\x42\x42\x42\x42')
+            await images_repo.add_image(filename, "qemu", 42, image_path, "e342eb86c1229b6c154367a5476969b5", "md5")
+
+        await self._add_closed_project_with_node(
+            controller, "Guarded",
+            {"node_id": str(uuid.uuid4()), "node_type": "qemu", "compute_id": "local",
+             "name": "n1", "properties": {"hda_disk_image_backing_file": "used.qcow2"}},
+        )
+
+        response = await client.delete(app.url_path_for("prune_images"))
+        assert response.status_code == status.HTTP_204_NO_CONTENT
+
+        assert await images_repo.get_image(os.path.join(tmpdir, "used.qcow2")) is not None
+        assert os.path.exists(os.path.join(tmpdir, "used.qcow2"))
+        assert await images_repo.get_image(os.path.join(tmpdir, "unused.qcow2")) is None
+        assert not os.path.exists(os.path.join(tmpdir, "unused.qcow2"))
+
     async def test_image_upload_create_appliance(
             self, app: FastAPI,
             client: AsyncClient,

tests/api/routes/controller/test_templates.py

@@ -16,6 +16,7 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import os
+import json
 import shutil
 
 import pytest
@@ -270,6 +271,128 @@ class TestTemplateRoutes:
         images = await images_repo.get_images()
         assert len(images) == 0
 
+    @staticmethod
+    async def _add_closed_project_with_node(controller: Controller, name: str, node: dict):
+        """
+        Register a closed project whose .gns3 contains a single node,
+        mirroring how the controller picks up existing projects from disk.
+        """
+
+        project_dir = os.path.join(controller.projects_directory(), name)
+        os.makedirs(project_dir)
+        project_id = str(uuid.uuid4())
+        topology = {
+            "name": name,
+            "project_id": project_id,
+            "topology": {"computes": [], "links": [], "drawings": [], "nodes": [node]},
+        }
+        with open(os.path.join(project_dir, f"{name}.gns3"), "w+") as f:
+            json.dump(topology, f)
+        # an explicit project_id marks this as an existing on-disk project
+        # (same path load_project takes at startup)
+        return await controller.add_project(
+            project_id=project_id, name=name, path=project_dir,
+            filename=f"{name}.gns3", status="closed",
+        )
+
+    async def test_template_delete_used_by_project(
+            self,
+            app: FastAPI,
+            client: AsyncClient,
+            controller: Controller,
+    ) -> None:
+        """
+        A template referenced by a node in a project must not be deleted,
+        even when the project is closed.
+        """
+
+        template_id = str(uuid.uuid4())
+        params = {"template_id": template_id,
+                  "name": "VPCS_GUARDED",
+                  "compute_id": "local",
+                  "template_type": "vpcs"}
+
+        response = await client.post(app.url_path_for("create_template"), json=params)
+        assert response.status_code == status.HTTP_201_CREATED
+
+        guarded_project = await self._add_closed_project_with_node(
+            controller, "Guarded",
+            {"node_id": str(uuid.uuid4()), "node_type": "vpcs", "compute_id": "local",
+             "name": "n1", "template_id": template_id, "properties": {}},
+        )
+
+        response = await client.delete(app.url_path_for("delete_template", template_id=template_id))
+        assert response.status_code == status.HTTP_409_CONFLICT
+        assert "Guarded" in response.json()["message"]
+
+        # the template survived the refused deletion
+        response = await client.get(app.url_path_for("get_template", template_id=template_id))
+        assert response.status_code == status.HTTP_200_OK
+
+        # once no project uses it anymore the deletion goes through
+        controller.remove_project(guarded_project)
+        response = await client.delete(app.url_path_for("delete_template", template_id=template_id))
+        assert response.status_code == status.HTTP_204_NO_CONTENT
+
+    async def test_template_delete_with_prune_images_used_by_project(
+            self,
+            app: FastAPI,
+            client: AsyncClient,
+            controller: Controller,
+            db_session: AsyncSession,
+            tmpdir: str,
+    ) -> None:
+        """
+        Pruning an image still referenced by a project node must be refused
+        before any mutation: the template and the image file both survive.
+        """
+
+        image_path = os.path.join(tmpdir, "used.qcow2")
+        with open(image_path, "wb+") as f:
+            f.write(b'\x42\x42\x42\x42')
+
+        images_repo = ImagesRepository(db_session)
+        await images_repo.add_image("used.qcow2", "qemu", 42, image_path, "e342eb86c1229b6c154367a5476969b5", "md5")
+
+        template_id = str(uuid.uuid4())
+        params = {"template_id": template_id,
+                  "name": "QEMU_GUARDED",
+                  "compute_id": "local",
+                  "hda_disk_image": "used.qcow2",
+                  "template_type": "qemu"}
+
+        response = await client.post(app.url_path_for("create_template"), json=params)
+        assert response.status_code == status.HTTP_201_CREATED
+
+        # the node references the image but not the template: the template
+        # check passes, the image check must still refuse the prune
+        guarded_project = await self._add_closed_project_with_node(
+            controller, "Guarded",
+            {"node_id": str(uuid.uuid4()), "node_type": "qemu", "compute_id": "local",
+             "name": "n1", "properties": {"hda_disk_image_backing_file": "used.qcow2"}},
+        )
+
+        response = await client.delete(
+            app.url_path_for("delete_template", template_id=template_id),
+            params={"prune_images": True}
+        )
+        assert response.status_code == status.HTTP_409_CONFLICT
+        assert "Guarded" in response.json()["message"]
+
+        # neither the template nor the image file was touched
+        response = await client.get(app.url_path_for("get_template", template_id=template_id))
+        assert response.status_code == status.HTTP_200_OK
+        assert os.path.exists(image_path)
+
+        controller.remove_project(guarded_project)
+        response = await client.delete(
+            app.url_path_for("delete_template", template_id=template_id),
+            params={"prune_images": True}
+        )
+        assert response.status_code == status.HTTP_204_NO_CONTENT
+        assert not os.path.exists(image_path)
+        assert await images_repo.get_image(image_path) is None
+
     # async def test_create_node_from_template(self, controller_api, controller, project):
     #
     #     id = str(uuid.uuid4())

tests/controller/test_controller.py

@@ -534,3 +534,99 @@ async def test_autoidlepc(controller):
         await controller.autoidlepc("local", "c7200", "test.bin", 512)
     assert node_mock.dynamips_auto_idlepc.called
     assert len(controller.projects) == 0
+
+
+@pytest.mark.asyncio
+async def test_find_projects_using_template_and_images(controller):
+    """
+    The template/image usage checks must see nodes of opened projects
+    (in-memory Node objects) as well as nodes of closed projects (raw
+    node dicts read back from the .gns3 file).
+    """
+
+    compute = MagicMock()
+    response = MagicMock()
+    response.json = {"console": 2048}
+    compute.post = AsyncioMagicMock(return_value=response)
+
+    project1 = await controller.add_project(name="Test1")
+    project2 = await controller.add_project(name="Test2")
+
+    template_id = str(uuid.uuid4())
+    await project1.add_node(
+        compute,
+        "n1",
+        None,
+        node_type="vpcs",
+        template_id=template_id,
+        properties={"hda_disk_image": "/tmp/images/disk.qcow2", "hda_disk_image_backing_file": "base.qcow2"},
+    )
+    await project2.add_node(compute, "n2", None, node_type="vpcs", properties={})
+
+    # opened projects
+    assert controller.find_projects_using_template(template_id) == ["Test1"]
+    assert controller.find_projects_using_template(str(uuid.uuid4())) == []
+    # image references are matched on file name whether stored as a bare
+    # name or as an absolute path
+    assert controller.find_projects_using_image("base.qcow2") == ["Test1"]
+    assert controller.find_projects_using_image("disk.qcow2") == ["Test1"]
+    assert controller.find_projects_using_image("unknown.qcow2") == []
+    referenced = controller.collect_referenced_image_filenames()
+    assert "base.qcow2" in referenced
+    assert "disk.qcow2" in referenced
+
+    # a second node from the same template in the same project must not
+    # list the project twice
+    await project1.add_node(
+        compute,
+        "n1-bis",
+        None,
+        node_type="vpcs",
+        template_id=template_id,
+        properties={},
+    )
+    assert controller.find_projects_using_template(template_id) == ["Test1"]
+
+    # a second project using the same template and image must be listed too
+    await project2.add_node(
+        compute,
+        "n2-bis",
+        None,
+        node_type="vpcs",
+        template_id=template_id,
+        properties={"hda_disk_image_backing_file": "base.qcow2"},
+    )
+    assert controller.find_projects_using_template(template_id) == ["Test1", "Test2"]
+    assert controller.find_projects_using_image("base.qcow2") == ["Test1", "Test2"]
+    assert controller.find_projects_using_image("disk.qcow2") == ["Test1"]
+
+    # closed projects: same answers from the .gns3 file on disk
+    await project1.close()
+    await project2.close()
+    assert controller.find_projects_using_template(template_id) == ["Test1", "Test2"]
+    assert controller.find_projects_using_image("base.qcow2") == ["Test1", "Test2"]
+    assert controller.find_projects_using_image("disk.qcow2") == ["Test1"]
+
+
+@pytest.mark.asyncio
+async def test_find_projects_skips_unreadable_topology(controller):
+    """
+    A project whose topology cannot be read must not break the usage checks.
+    """
+
+    compute = MagicMock()
+    response = MagicMock()
+    response.json = {"console": 2048}
+    compute.post = AsyncioMagicMock(return_value=response)
+
+    project = await controller.add_project(name="Broken")
+    await project.add_node(
+        compute, "n1", None, node_type="vpcs",
+        template_id=str(uuid.uuid4()), properties={"hda_disk_image": "lost.qcow2"},
+    )
+    await project.close()
+    os.remove(project.topology_file)
+
+    assert controller.find_projects_using_template(str(uuid.uuid4())) == []
+    assert controller.find_projects_using_image("lost.qcow2") == []
+    assert controller.collect_referenced_image_filenames() == set()