diff --git a/gns3server/controller/project.py b/gns3server/controller/project.py index 72b0bf9f6..bb3f5f103 100644 --- a/gns3server/controller/project.py +++ b/gns3server/controller/project.py @@ -791,7 +791,9 @@ class Project: "tag": marker.get("tag"), "enabled": marker.get("enabled", True), "color": marker.get("color"), + "highlight_duration": marker.get("highlight_duration"), "capture_node_id": marker.get("capture_node_id"), + "direction": marker.get("direction"), } if "link_style" in link_data: await link.update_link_style(link_data["link_style"]) @@ -963,6 +965,21 @@ class Project: """ return self._marker_definitions + def _validate_marker_definition_direction(self, name, direction): + """ + Reject tx/rx on a marker definition: a definition auto-selects its + capture node per link (``_choose_marker_side``), while tx/rx is + interpreted from that node's perspective, so a fixed direction has no + stable meaning project-wide. Only 'both' (the default, = ``None``) is + allowed — use a per-link marker if a directional filter is needed. + """ + if direction in ("tx", "rx"): + raise ControllerError( + f"Marker definition '{name}': direction '{direction}' is not allowed. " + "A definition auto-selects its capture node per link and tx/rx is " + "relative to that node — use 'both' (the default), or a per-link marker." + ) + async def create_marker_definition(self, name, bpf, tag=None, direction=None, color=None, highlight_duration=None): """ Create a project-level marker definition and fan out to every existing @@ -975,6 +992,7 @@ class Project: f"Marker definition '{name}' already exists in this project" ) + self._validate_marker_definition_direction(name, direction) self._marker_definitions[name] = {"bpf": bpf, "tag": tag, "direction": direction, "color": color, "highlight_duration": highlight_duration, "paused": False} await self._apply_def_to_all_links(name) self.dump() @@ -1000,6 +1018,7 @@ class Project: if highlight_duration is not None: d["highlight_duration"] = highlight_duration if direction is not _UNSET: + self._validate_marker_definition_direction(name, direction) d["direction"] = direction # None = clear back to both directions # Sync: update every inherited copy across all links. diff --git a/tests/controller/test_marker.py b/tests/controller/test_marker.py index 73dab47e8..991fef5fa 100644 --- a/tests/controller/test_marker.py +++ b/tests/controller/test_marker.py @@ -24,6 +24,8 @@ Controller-layer tests for the traffic-insight marker feature: * Project.apply_defs_to_new_link and the markers aggregation property. """ +import uuid + import pytest from unittest.mock import MagicMock, patch @@ -286,6 +288,78 @@ async def test_persist_markers_excludes_inherited(project): assert "global-arp" not in persisted +@pytest.mark.asyncio +async def test_load_marker_preserves_direction_and_highlight_duration(project): + """Regression: a private marker's direction + highlight_duration must survive + a close/reopen round-trip through the topology file. + + _create_link_from_topology_data previously restored only bpf/tag/enabled/ + color/capture_node_id, silently dropping direction (→ reverted to "both") + and highlight_duration. + """ + compute = MagicMock() + compute.id = "local" + compute.host = "example.com" + + async def subnet(_other): + return ("192.168.1.1", "192.168.1.2") + + async def udp_cb(path, data={}, **kwargs): + response = MagicMock() + response.json = {"udp_port": 1234} + return response + + compute.get_ip_on_same_subnet.side_effect = subnet + compute.post.side_effect = udp_cb + # Attaching the 2nd node auto-creates the link (NIO round-trips). + compute.put = AsyncioMagicMock() + compute.delete = AsyncioMagicMock() + + node1 = Node(project, compute, "n1", node_type="vpcs") + node1._ports = [EthernetPort("E0", 0, 0, 0)] + node2 = Node(project, compute, "n2", node_type="vpcs") + node2._ports = [EthernetPort("E0", 0, 0, 1)] + # _create_link_from_topology_data resolves nodes via project.get_node(). + project._nodes[node1.id] = node1 + project._nodes[node2.id] = node2 + + capture_node_id = str(uuid.uuid4()) + link_id = str(uuid.uuid4()) + link_data = { + "link_id": link_id, + "nodes": [ + {"node_id": node1.id, "adapter_number": 0, "port_number": 0, "label": "a"}, + {"node_id": node2.id, "adapter_number": 0, "port_number": 1, "label": "b"}, + ], + "markers": { + "icmp": { + "bpf": "icmp", + "direction": "rx", + "highlight_duration": 800, + "tag": 7, + "color": "#ff5722", + "enabled": True, + "capture_node_id": capture_node_id, + } + }, + } + with patch( + "gns3server.controller.project.validate_bpf_syntax", + return_value={"valid": True, "error": None}, + ): + await project._create_link_from_topology_data(link_data) + + # The link survives (2 attached nodes); pull it back from the project. + link = project._links[link_id] + entry = link._markers["icmp"] + assert entry["direction"] == "rx" # dropped before the fix + assert entry["highlight_duration"] == 800 # dropped before the fix + assert entry["tag"] == 7 + assert entry["color"] == "#ff5722" + assert entry["capture_node_id"] == capture_node_id + assert entry["enabled"] is True + + @pytest.mark.asyncio async def test_asdict_markers_runtime_vs_dump(project): """Runtime asdict exposes all markers; topology dump drops inherited ones.""" @@ -410,10 +484,16 @@ async def test_update_marker_preserves_direction_when_omitted(project): @pytest.mark.asyncio async def test_update_marker_definition_clears_direction(project): # Clearing a definition's direction must propagate to every inherited copy. + # New defs can't carry tx/rx, but a legacy def loaded from an old topology + # could — so inject one and confirm a clear syncs every copy. with _valid_bpf(): link1 = await _make_link(project) link2 = await _make_link(project) - await project.create_marker_definition("arp", "arp", direction="tx") + await project.create_marker_definition("arp", "arp") + # Simulate a legacy directional value persisted before the restriction. + project._marker_definitions["arp"]["direction"] = "tx" + await link1.update_marker("global-arp", direction="tx", inherited=True) + await link2.update_marker("global-arp", direction="tx", inherited=True) for link in (link1, link2): assert link.markers["global-arp"]["direction"] == "tx" await project.update_marker_definition("arp", direction=None) @@ -549,3 +629,37 @@ async def test_paused_definition_inherited_as_disabled(project): await project.pause_marker_definition("arp") new_link = await _make_link(project) assert new_link.markers["global-arp"]["enabled"] is False + + +# --------------------------------------------------------------------------- +# Marker definition direction (tx/rx rejected — it is capture-node-relative) +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_create_marker_definition_rejects_directional(project): + # tx/rx is relative to the auto-selected capture node → rejected at the def level. + with pytest.raises(ControllerError): + await project.create_marker_definition("arp", "arp", direction="tx") + with pytest.raises(ControllerError): + await project.create_marker_definition("arp", "arp", direction="rx") + assert "arp" not in project.marker_definitions # nothing created + + +@pytest.mark.asyncio +async def test_create_marker_definition_allows_both(project): + await project.create_marker_definition("arp", "arp") # default both + await project.create_marker_definition("icmp", "icmp", direction=None) + assert project.marker_definitions["arp"]["direction"] is None + assert project.marker_definitions["icmp"]["direction"] is None + + +@pytest.mark.asyncio +async def test_update_marker_definition_rejects_directional(project): + await project.create_marker_definition("arp", "arp") # both + with pytest.raises(ControllerError): + await project.update_marker_definition("arp", direction="tx") + # omitted direction and explicit clear-to-both are both fine + await project.update_marker_definition("arp", color="#ffffff") + await project.update_marker_definition("arp", direction=None) + assert project.marker_definitions["arp"]["direction"] is None