From 7ed4eeab298142fb0675e7790f51da8f9941bebc Mon Sep 17 00:00:00 2001 From: YueGuobin Date: Wed, 5 Aug 2026 01:40:22 +0800 Subject: [PATCH] mcp: drop direction from marker_definition tool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A marker definition fans out to every link and auto-selects its capture node on each, so tx/rx is relative to a node that varies per link — the controller already rejects it (409). Exposing direction on the MCP definition tool let an agent ask for something that could only fail. Remove the parameter and the handler's direction handling; the docstring now points to encoding direction in the BPF (e.g. 'icmp and icmp[icmptype]==8'). Per-link link_marker keeps direction, where the capture node is fixed. --- gns3server/api/routes/mcp/__init__.py | 11 ++++-- gns3server/api/routes/mcp/links.py | 14 ++----- tests/api/routes/mcp/test_handlers.py | 55 +++++++++++++-------------- 3 files changed, 39 insertions(+), 41 deletions(-) diff --git a/gns3server/api/routes/mcp/__init__.py b/gns3server/api/routes/mcp/__init__.py index 3f2425b00..2369fe658 100644 --- a/gns3server/api/routes/mcp/__init__.py +++ b/gns3server/api/routes/mcp/__init__.py @@ -1052,7 +1052,6 @@ async def marker_definition( tag: Annotated[int | None, Field(description="Numeric tag for packet correlation")] = None, color: Annotated[str | None, Field(description="Hex color for UI highlight, e.g. '#ff5722'")] = None, highlight_duration: Annotated[int | None, Field(description="UI highlight duration in milliseconds")] = None, - direction: Annotated[str | None, Field(description="Direction filter: 'tx' (capture node sending only), 'rx' (receiving only), or 'both' (no filter — on update this clears a previously set direction). Omit to leave unchanged on update.")] = None, ) -> list[dict[str, Any]]: """Manage project-level marker definitions — traffic-insight rules that apply to ALL links. @@ -1061,14 +1060,20 @@ async def marker_definition( On delete, 'global-{name}' is removed from every link. Create requires: project_id, action='create', bpf - Update requires: project_id, action='update', def_name, and at least one of (bpf, tag, direction, color, highlight_duration) + Update requires: project_id, action='update', def_name, and at least one of (bpf, tag, color, highlight_duration) Delete requires: project_id, action='delete', def_name List requires: project_id, action='list' + A definition has NO direction (tx/rx): it fans out to every link and auto-selects + its capture node on each, so a fixed direction has no consistent meaning. Encode + the direction you want in the BPF instead (e.g. 'icmp and icmp[icmptype]==8' for + echo requests only). For a capture-node-relative direction on a single link, use + the per-link `link_marker` tool. + Common BPF examples: 'arp', 'icmp', 'ospf', 'tcp port 22', 'udp port 53' """ params = {"project_id": project_id, "action": action} - for opt in ("bpf", "def_name", "name", "tag", "direction", "color", "highlight_duration"): + for opt in ("bpf", "def_name", "name", "tag", "color", "highlight_duration"): val = locals().get(opt) if val is not None: params[opt] = val diff --git a/gns3server/api/routes/mcp/links.py b/gns3server/api/routes/mcp/links.py index 9ddf2b377..7e1afc6a2 100644 --- a/gns3server/api/routes/mcp/links.py +++ b/gns3server/api/routes/mcp/links.py @@ -460,9 +460,9 @@ def marker_definition_handler(params: dict[str, Any], gns3_ctx: dict[str, Any]) for opt in ("name", "tag", "color", "highlight_duration"): if params.get(opt) is not None: body[opt] = params[opt] - # direction: "tx"/"rx" set a one-way filter; "both"/omitted = no filter. - if params.get("direction") in ("tx", "rx"): - body["direction"] = params["direction"] + # No direction: a definition fans out to every link and auto-selects its + # capture node on each, so tx/rx (which is relative to that node) has no + # consistent meaning. Encode direction in the BPF instead. return conn.http_call("post", base, json_data=body).json() def_name = params.get("def_name") @@ -476,14 +476,8 @@ def marker_definition_handler(params: dict[str, Any], gns3_ctx: dict[str, Any]) for opt in ("bpf", "tag", "color", "highlight_duration"): if params.get(opt) is not None: body[opt] = params[opt] - # direction tri-state: omitted=preserve, "tx"/"rx"=set, "both"=clear (→ null). - direction = params.get("direction") - if direction == "both": - body["direction"] = None - elif direction in ("tx", "rx"): - body["direction"] = direction if not body: - return {"error": "At least one update field is required (bpf, tag, direction, color, highlight_duration)"} + return {"error": "At least one update field is required (bpf, tag, color, highlight_duration)"} return conn.http_call("put", url, json_data=body).json() # action == "delete" diff --git a/tests/api/routes/mcp/test_handlers.py b/tests/api/routes/mcp/test_handlers.py index 5cd8a8bc0..fc8cd00d2 100644 --- a/tests/api/routes/mcp/test_handlers.py +++ b/tests/api/routes/mcp/test_handlers.py @@ -441,39 +441,44 @@ class TestLinkMarker: class TestMarkerDefinition: - """marker_definition_handler direction tri-state (same semantics as link markers).""" + """marker_definition_handler build create/update bodies. + + A definition has NO direction: it fans out to every link and auto-selects its + capture node on each, so tx/rx (relative to that node) has no consistent + meaning — any direction passed is ignored, never reaching the request body. + """ mod = "links" - def test_update_direction_both_clears(self, ctx): + def test_create_builds_body(self, ctx): from gns3server.api.routes.mcp.links import marker_definition_handler with patch(f"{BASE}.{self.mod}._get_connector") as m: conn = _mock_conn({"name": "arp"}) m.return_value = conn marker_definition_handler( - {"project_id": "p", "action": "update", - "def_name": "arp", "direction": "both"}, ctx, + {"project_id": "p", "action": "create", + "bpf": "arp", "tag": 1, "color": "#fff"}, ctx, ) conn.http_call.assert_called_with( - "put", "http://192.168.1.3:3080/v3/projects/p/marker-definitions/arp", - json_data={"direction": None}, + "post", "http://192.168.1.3:3080/v3/projects/p/marker-definitions", + json_data={"bpf": "arp", "tag": 1, "color": "#fff"}, ) - def test_update_direction_tx_sets(self, ctx): + def test_create_ignores_direction(self, ctx): from gns3server.api.routes.mcp.links import marker_definition_handler with patch(f"{BASE}.{self.mod}._get_connector") as m: conn = _mock_conn({"name": "arp"}) m.return_value = conn marker_definition_handler( - {"project_id": "p", "action": "update", - "def_name": "arp", "direction": "tx"}, ctx, + {"project_id": "p", "action": "create", + "bpf": "arp", "direction": "tx"}, ctx, ) conn.http_call.assert_called_with( - "put", "http://192.168.1.3:3080/v3/projects/p/marker-definitions/arp", - json_data={"direction": "tx"}, + "post", "http://192.168.1.3:3080/v3/projects/p/marker-definitions", + json_data={"bpf": "arp"}, ) - def test_update_direction_omitted_preserved(self, ctx): + def test_update_builds_body(self, ctx): from gns3server.api.routes.mcp.links import marker_definition_handler with patch(f"{BASE}.{self.mod}._get_connector") as m: conn = _mock_conn({"name": "arp"}) @@ -487,30 +492,24 @@ class TestMarkerDefinition: json_data={"tag": 1}, ) - def test_create_direction_both_omitted(self, ctx): + def test_update_ignores_direction(self, ctx): from gns3server.api.routes.mcp.links import marker_definition_handler with patch(f"{BASE}.{self.mod}._get_connector") as m: conn = _mock_conn({"name": "arp"}) m.return_value = conn marker_definition_handler( - {"project_id": "p", "action": "create", - "bpf": "arp", "direction": "both"}, ctx, + {"project_id": "p", "action": "update", + "def_name": "arp", "tag": 1, "direction": "rx"}, ctx, ) conn.http_call.assert_called_with( - "post", "http://192.168.1.3:3080/v3/projects/p/marker-definitions", - json_data={"bpf": "arp"}, + "put", "http://192.168.1.3:3080/v3/projects/p/marker-definitions/arp", + json_data={"tag": 1}, ) - def test_create_direction_tx(self, ctx): + def test_update_requires_a_field(self, ctx): from gns3server.api.routes.mcp.links import marker_definition_handler - with patch(f"{BASE}.{self.mod}._get_connector") as m: - conn = _mock_conn({"name": "arp"}) - m.return_value = conn - marker_definition_handler( - {"project_id": "p", "action": "create", - "bpf": "arp", "direction": "tx"}, ctx, - ) - conn.http_call.assert_called_with( - "post", "http://192.168.1.3:3080/v3/projects/p/marker-definitions", - json_data={"bpf": "arp", "direction": "tx"}, + with patch(f"{BASE}.{self.mod}._get_connector"): + result = marker_definition_handler( + {"project_id": "p", "action": "update", "def_name": "arp"}, ctx, ) + assert "error" in result