mirror of
https://github.com/GNS3/gns3-server.git
synced 2026-08-29 05:20:14 +03:00
fix: reconcile marker filters in _ubridge_apply_markers (delete/update)
The batch marker-def fan-out (PR #2848) routed create/update/delete marker_definition through memory_only + a batch PUT /nios/batch that re-applies markers via _ubridge_apply_markers. But _ubridge_apply_markers was strictly add-only: it skipped any (name, link_id) already in _marker_filter_bridges, and reset_packet_filters preserves mark filters (contract). So: * delete_marker_definition left the deleted marker's filter alive in uBridge (still matching / signalling / writing pcap) until node restart. * update_marker_definition (bpf/tag/direction change) never reached uBridge — the live filter kept the old expression until node restart. Make _ubridge_apply_markers a real reconcile against the desired nio.markers: - installed but no longer desired → delete_packet_filter + unlink pcap + unregister - desired with changed filter field → rebuild (delete + re-add) - desired with only enabled changed → instant toggle (pcap preserved) - desired and unchanged → skip - desired and new → add Track installed specs in a parallel _marker_specs dict so changes can be detected. Both base_node and the IOU iol_bridge override are updated. Added tests for the delete-removed and rebuild-changed-bpf paths.
This commit is contained in:
YueGuobin
parent
f045cde0da
commit
958c45b6fc
@ -103,6 +103,10 @@ class BaseNode:
|
||||
# marker filter name -> uBridge bridge_name (recorded at apply time so
|
||||
# _ubridge_set_marker_filter_state can toggle on/off without an NIO rebuild).
|
||||
self._marker_filter_bridges = {}
|
||||
# Parallel store of the installed marker spec (bpf/tag/direction/enabled/...)
|
||||
# keyed by (name, link_id) so _ubridge_apply_markers can reconcile: detect
|
||||
# deletions and field changes instead of being add-only.
|
||||
self._marker_specs = {}
|
||||
|
||||
if self._console is not None:
|
||||
# use a previously allocated console port
|
||||
@ -994,6 +998,7 @@ class BaseNode:
|
||||
# gone too — clear the map so the next apply re-installs them all rather
|
||||
# than skipping them as "already installed".
|
||||
self._marker_filter_bridges.clear()
|
||||
self._marker_specs.clear()
|
||||
|
||||
async def add_ubridge_udp_connection(self, bridge_name, source_nio, destination_nio):
|
||||
"""
|
||||
@ -1176,6 +1181,7 @@ class BaseNode:
|
||||
if nio is not None and getattr(nio, "markers", None):
|
||||
nio.markers.pop(name, None)
|
||||
bridge_name = self._marker_filter_bridges.pop((name, link_id), None)
|
||||
self._marker_specs.pop((name, link_id), None)
|
||||
if bridge_name is not None:
|
||||
await self._ubridge_delete_marker_filter(bridge_name, name)
|
||||
try:
|
||||
@ -1223,34 +1229,71 @@ class BaseNode:
|
||||
|
||||
async def _ubridge_apply_markers(self, bridge_name, nio):
|
||||
"""
|
||||
Install the traffic-insight markers carried by *nio* onto bridge
|
||||
*bridge_name* that aren't already there. uBridge's ``reset_packet_filters``
|
||||
preserves mark filters (contract), so on an NIO update we add only the new
|
||||
ones — re-adding an existing marker would either duplicate it or
|
||||
close/reopen its pcap. Called from ``add_ubridge_udp_connection`` (fresh
|
||||
bridge, empty map → installs all) and ``update_ubridge_udp_connection``
|
||||
(incremental).
|
||||
Reconcile the traffic-insight markers carried by *nio* onto bridge
|
||||
*bridge_name* with what is already installed there.
|
||||
|
||||
uBridge's ``reset_packet_filters`` preserves mark filters (contract), so
|
||||
a plain re-add would duplicate them; instead this diffs the desired
|
||||
``nio.markers`` against the installed ``_marker_specs``:
|
||||
|
||||
* installed but no longer desired → delete filter + unlink pcap
|
||||
* desired with changed bpf/tag/direction/data_link_type → rebuild
|
||||
(delete + add; the marker's own pcap reopens for the new BPF)
|
||||
* desired with only ``enabled`` changed → instant on/off toggle
|
||||
(sibling and own pcap stay open)
|
||||
* desired and unchanged → skip
|
||||
* desired and new → add
|
||||
|
||||
Called from ``add_ubridge_udp_connection`` (fresh bridge, empty maps →
|
||||
installs all) and ``update_ubridge_udp_connection`` / the batch NIO
|
||||
update path (incremental reconcile).
|
||||
"""
|
||||
from gns3server.compute.marker.marker_manager import MarkerManager
|
||||
|
||||
markers = nio.markers if hasattr(nio, 'markers') else {}
|
||||
if not markers:
|
||||
return
|
||||
|
||||
manager = MarkerManager.instance()
|
||||
markers_dir = self.project.markers_working_directory()
|
||||
for name, spec in markers.items():
|
||||
link_id = spec.get("link_id", "")
|
||||
# Incremental: skip markers already on this bridge. uBridge keeps mark
|
||||
# filters across reset_packet_filters, so re-adding would duplicate (or
|
||||
# reopen the pcap). A fresh bridge has an empty map → installs all.
|
||||
if (name, link_id) in self._marker_filter_bridges:
|
||||
continue
|
||||
desired = {(name, spec.get("link_id", "")): spec for name, spec in markers.items()}
|
||||
|
||||
# 1. Remove installed markers that are no longer desired (marker/def delete).
|
||||
for key in list(self._marker_filter_bridges):
|
||||
if key not in desired:
|
||||
mname, link_id = key
|
||||
installed_bridge = self._marker_filter_bridges.pop(key)
|
||||
self._marker_specs.pop(key, None)
|
||||
await self._ubridge_delete_marker_filter(installed_bridge, mname)
|
||||
try:
|
||||
os.remove(os.path.join(markers_dir, f"{self._id}_{link_id}_{mname}.pcap"))
|
||||
except FileNotFoundError:
|
||||
pass
|
||||
except OSError as e:
|
||||
log.warning("Could not remove marker pcap for '%s' on link %s: %s", mname, link_id, e)
|
||||
manager.unregister(self._id, mname)
|
||||
|
||||
# 2. Add newly-desired markers; rebuild ones whose filter fields changed.
|
||||
rebuild_fields = ("bpf", "tag", "direction", "data_link_type")
|
||||
for (name, link_id), spec in desired.items():
|
||||
bpf = spec.get("bpf", "")
|
||||
tag = spec.get("tag")
|
||||
pcap_path = os.path.join(
|
||||
markers_dir, f"{self._id}_{link_id}_{name}.pcap"
|
||||
)
|
||||
enabled = spec.get("enabled", True)
|
||||
if (name, link_id) in self._marker_filter_bridges:
|
||||
installed_spec = self._marker_specs.get((name, link_id))
|
||||
if installed_spec is None:
|
||||
# Installed but no recorded spec (legacy / pre-reconcile state):
|
||||
# cannot diff, skip to avoid a duplicate add.
|
||||
continue
|
||||
if any(installed_spec.get(f) != spec.get(f) for f in rebuild_fields):
|
||||
# A filter field changed → rebuild (delete + re-add).
|
||||
installed_bridge = self._marker_filter_bridges.get((name, link_id))
|
||||
await self._ubridge_delete_marker_filter(installed_bridge, name)
|
||||
elif installed_spec.get("enabled", True) != enabled:
|
||||
# Only the on/off state changed → instant toggle, pcap preserved.
|
||||
await self._ubridge_set_marker_filter_state(name, enabled)
|
||||
self._marker_specs[(name, link_id)] = spec
|
||||
continue
|
||||
else:
|
||||
continue # unchanged
|
||||
pcap_path = os.path.join(markers_dir, f"{self._id}_{link_id}_{name}.pcap")
|
||||
try:
|
||||
await self._ubridge_add_marker_filter(bridge_name, name, bpf, pcap_path, tag, link_id,
|
||||
direction=spec.get("direction"),
|
||||
@ -1268,22 +1311,19 @@ class BaseNode:
|
||||
# A disabled marker is installed but turned off (a paused tap), not
|
||||
# dropped — so the UI can flip it back on instantly with
|
||||
# enable_packet_filter, no NIO rebuild (ubridge contract §3.2).
|
||||
if not spec.get("enabled", True):
|
||||
if not enabled:
|
||||
try:
|
||||
await self._ubridge_send(f"bridge enable_packet_filter {bridge_name} {name} off")
|
||||
except UbridgeError as e:
|
||||
# Old ubridge without enable_packet_filter: leave it installed
|
||||
# (on) rather than fail the whole link/marker apply.
|
||||
log.warning(f"Could not turn marker '{name}' off on {bridge_name}: {e}")
|
||||
manager.register(
|
||||
str(self.project.id), self._id, name, link_id, tag
|
||||
)
|
||||
manager.register(str(self.project.id), self._id, name, link_id, tag)
|
||||
# Remember which bridge hosts this filter so an instant on/off toggle
|
||||
# (no NIO rebuild) can resolve it by name alone.
|
||||
# keyed (name, link_id) so a node that hosts markers for several links
|
||||
# (e.g. IOU with one IOL-BRIDGE and many bays/units) records each
|
||||
# copy independently — toggle below iterates all matching entries.
|
||||
# (no NIO rebuild) can resolve it by name alone, and keep the spec so
|
||||
# the next reconcile can detect changes.
|
||||
self._marker_filter_bridges[name, link_id] = bridge_name
|
||||
self._marker_specs[name, link_id] = spec
|
||||
|
||||
async def _ubridge_set_marker_filter_state(self, name, enabled):
|
||||
"""
|
||||
|
||||
@ -1266,7 +1266,9 @@ class IOUVM(BaseNode):
|
||||
|
||||
async def _ubridge_apply_markers(self, adapter_number, port_number, nio):
|
||||
"""
|
||||
(Re-)apply traffic-insight markers to the IOL bridge.
|
||||
Reconcile traffic-insight markers on the IOL bridge (diff desired
|
||||
``nio.markers`` against installed ``_marker_specs``): delete removed,
|
||||
rebuild changed, toggle on/off-only changes, add new, skip unchanged.
|
||||
|
||||
IOU uses ``iol_bridge`` (not ``bridge``) and the ``add_packet_filter``
|
||||
command carries extra ``{bay} {unit}`` positional arguments between the
|
||||
@ -1280,41 +1282,55 @@ class IOUVM(BaseNode):
|
||||
from gns3server.compute.marker.marker_manager import MarkerManager
|
||||
|
||||
markers = nio.markers if hasattr(nio, 'markers') else {}
|
||||
if not markers:
|
||||
return
|
||||
|
||||
manager = MarkerManager.instance()
|
||||
markers_dir = self.project.markers_working_directory()
|
||||
bridge_name = f"IOL-BRIDGE-{self.application_id + 512}"
|
||||
location = "{bridge_name} {bay} {unit}".format(
|
||||
bridge_name=bridge_name, bay=adapter_number, unit=port_number
|
||||
)
|
||||
for name, spec in markers.items():
|
||||
link_id = spec.get("link_id", "")
|
||||
# Incremental: skip markers already installed on this port. A NIO
|
||||
# update carries EVERY marker on the port (e.g. an inherited
|
||||
# global-* copy plus a newly added private one); uBridge's
|
||||
# add_packet_filter rejects a duplicate filter name (packet_filter.c),
|
||||
# so we must not re-add one already here — mirrors the generic
|
||||
# _ubridge_apply_markers guard. A fresh bridge has an empty map
|
||||
# (cleared on _stop_ubridge) so all are installed.
|
||||
if (name, link_id) in self._marker_filter_bridges:
|
||||
continue
|
||||
desired = {(name, spec.get("link_id", "")): spec for name, spec in markers.items()}
|
||||
|
||||
# 1. Remove installed markers that are no longer desired.
|
||||
for key in list(self._marker_filter_bridges):
|
||||
if key not in desired:
|
||||
mname, link_id = key
|
||||
installed_location = self._marker_filter_bridges.pop(key)
|
||||
self._marker_specs.pop(key, None)
|
||||
await self._ubridge_delete_marker_filter(installed_location, mname)
|
||||
try:
|
||||
os.remove(os.path.join(markers_dir, f"{self._id}_{link_id}_{mname}.pcap"))
|
||||
except FileNotFoundError:
|
||||
pass
|
||||
except OSError as e:
|
||||
log.warning("Could not remove marker pcap for '%s' on link %s: %s", mname, link_id, e)
|
||||
manager.unregister(self._id, mname)
|
||||
|
||||
# 2. Add / reconcile desired markers.
|
||||
rebuild_fields = ("bpf", "tag", "direction", "data_link_type")
|
||||
for (name, link_id), spec in desired.items():
|
||||
bpf = spec.get("bpf", "")
|
||||
tag = spec.get("tag")
|
||||
pcap_path = os.path.join(
|
||||
markers_dir, f"{self._id}_{link_id}_{name}.pcap"
|
||||
)
|
||||
# Build the iol_bridge marker filter command:
|
||||
# iol_bridge add_packet_filter {br} {bay} {unit} {name} mark "{bpf}" [tag {id}] pcap "{path}"
|
||||
enabled = spec.get("enabled", True)
|
||||
if (name, link_id) in self._marker_filter_bridges:
|
||||
installed_spec = self._marker_specs.get((name, link_id))
|
||||
if installed_spec is None:
|
||||
continue # installed (legacy, no spec) — skip to avoid dup
|
||||
if any(installed_spec.get(f) != spec.get(f) for f in rebuild_fields):
|
||||
installed_location = self._marker_filter_bridges.get((name, link_id))
|
||||
await self._ubridge_delete_marker_filter(installed_location, name)
|
||||
elif installed_spec.get("enabled", True) != enabled:
|
||||
await self._ubridge_set_marker_filter_state(name, enabled)
|
||||
self._marker_specs[(name, link_id)] = spec
|
||||
continue
|
||||
else:
|
||||
continue
|
||||
pcap_path = os.path.join(markers_dir, f"{self._id}_{link_id}_{name}.pcap")
|
||||
# iol_bridge add_packet_filter {br} {bay} {unit} {name} mark "{bpf}" [tag {id}] pcap "{path}"
|
||||
cmd = 'iol_bridge add_packet_filter {loc} {name} mark "{bpf}"'.format(
|
||||
loc=location, name=name, bpf=bpf
|
||||
)
|
||||
if tag is not None:
|
||||
cmd += f" tag {tag}"
|
||||
# IOU uses one per-node IOL-BRIDGE for every link, so bridge+filter
|
||||
# are identical across this node's links — `link` is the only way the
|
||||
# controller can tell their signals apart (contract §3.2).
|
||||
if link_id:
|
||||
cmd += f" link {link_id}"
|
||||
direction = spec.get("direction")
|
||||
@ -1333,16 +1349,14 @@ class IOUVM(BaseNode):
|
||||
self.project.emit("log.warning", {"message": message})
|
||||
continue
|
||||
raise
|
||||
if not spec.get("enabled", True):
|
||||
if not enabled:
|
||||
try:
|
||||
await self._ubridge_send(f"iol_bridge enable_packet_filter {location} {name} off")
|
||||
except UbridgeError as e:
|
||||
log.warning(f"Could not turn marker '{name}' off on {location}: {e}")
|
||||
manager.register(
|
||||
str(self.project.id), self._id, name, link_id, tag
|
||||
)
|
||||
# Record name -> location (bridge bay unit) for instant toggle.
|
||||
manager.register(str(self.project.id), self._id, name, link_id, tag)
|
||||
self._marker_filter_bridges[name, link_id] = location
|
||||
self._marker_specs[name, link_id] = spec
|
||||
|
||||
async def _ubridge_set_marker_filter_state(self, name, enabled):
|
||||
"""IOU override: toggle every (name, link_id) entry via ``iol_bridge``."""
|
||||
|
||||
@ -367,6 +367,49 @@ async def test_apply_markers_skips_already_installed(compute_project, manager):
|
||||
assert not any("add_packet_filter" in c for c in cmds) # skipped, not re-added
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_apply_markers_deletes_removed(compute_project, manager):
|
||||
# Reconcile: a marker no longer in nio.markers is deleted from uBridge
|
||||
# (filter + registry), not left as an orphan still matching packets.
|
||||
node = VPCSVM("test", "00010203-0405-0607-0809-0a0b0c0d0e0f", compute_project, manager)
|
||||
node._ubridge_send = AsyncioMagicMock()
|
||||
node._ubridge_hypervisor = MagicMock()
|
||||
node._ubridge_hypervisor.is_running.return_value = True
|
||||
node._marker_filter_bridges["m", "L1"] = "VPCS-10"
|
||||
node._marker_specs["m", "L1"] = {"bpf": "icmp", "enabled": True}
|
||||
nio = NIOUDP(1234, "127.0.0.1", 4321)
|
||||
nio.markers = {} # marker removed
|
||||
with patch("gns3server.compute.marker.marker_manager.MarkerManager") as mm:
|
||||
mm.instance.return_value.unregister = MagicMock()
|
||||
await node._ubridge_apply_markers("VPCS-10", nio)
|
||||
cmds = [c.args[0] for c in node._ubridge_send.call_args_list]
|
||||
assert any("delete_packet_filter VPCS-10 m" in c for c in cmds)
|
||||
assert ("m", "L1") not in node._marker_filter_bridges
|
||||
assert ("m", "L1") not in node._marker_specs
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_apply_markers_rebuilds_changed_bpf(compute_project, manager):
|
||||
# Reconcile: a marker whose bpf changed is rebuilt (delete + re-add), so
|
||||
# uBridge ends up with the new expression — not the stale original.
|
||||
node = VPCSVM("test", "00010203-0405-0607-0809-0a0b0c0d0e0f", compute_project, manager)
|
||||
node._ubridge_send = AsyncioMagicMock()
|
||||
node._ubridge_hypervisor = MagicMock()
|
||||
node._ubridge_hypervisor.is_running.return_value = True
|
||||
node._marker_filter_bridges["m", "L1"] = "VPCS-10"
|
||||
node._marker_specs["m", "L1"] = {"bpf": "icmp", "tag": None, "direction": None,
|
||||
"data_link_type": None, "enabled": True}
|
||||
nio = NIOUDP(1234, "127.0.0.1", 4321)
|
||||
nio.markers = {"m": {"bpf": "tcp", "tag": None, "link_id": "L1",
|
||||
"direction": None, "enabled": True}}
|
||||
with patch("gns3server.compute.marker.marker_manager.MarkerManager") as mm:
|
||||
mm.instance.return_value.register = MagicMock()
|
||||
await node._ubridge_apply_markers("VPCS-10", nio)
|
||||
cmds = [c.args[0] for c in node._ubridge_send.call_args_list]
|
||||
assert any("delete_packet_filter VPCS-10 m" in c for c in cmds) # old removed
|
||||
assert any("add_packet_filter VPCS-10 m mark" in c and "tcp" in c for c in cmds) # new added
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_stop_ubridge_clears_marker_bridges(compute_project, manager):
|
||||
# uBridge stopping drops every marker filter — the map must clear so the next
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user