From f9c8c36a6b0b5632c51b11aad4effa7aaa30059b Mon Sep 17 00:00:00 2001 From: catborise Date: Thu, 22 Oct 2026 09:07:18 +0300 Subject: [PATCH] fix(nwfilters): a filter is changed in place, also while a VM uses it Every change to a filter (a rule or reference removed or added, the XML edited) undefined it and defined the new XML. libvirt refuses to undefine a filter a running VM uses, so none of these worked on such a filter; on an unused one a refused definition was undone by defining the old XML without its UUID, so the filter came back as another one. The filter is now defined again with its own UUID, which libvirt does in place, while VMs use it too; XML it cannot parse or apply to the running VMs is refused and reported. The XML editor shows the UUID. Co-Authored-By: Claude Opus 5.5 (1M context) --- nwfilters/tests/test_redefine.py | 75 ++++++++++++++++++++++++++++++++ nwfilters/views.py | 35 +++------------ vrtManager/nwfilters.py | 11 ++--- 3 files changed, 84 insertions(+), 37 deletions(-) create mode 100644 nwfilters/tests/test_redefine.py diff --git a/nwfilters/tests/test_redefine.py b/nwfilters/tests/test_redefine.py new file mode 100644 index 00000000..36402c52 --- /dev/null +++ b/nwfilters/tests/test_redefine.py @@ -0,0 +1,75 @@ +"""A filter is changed by defining it again with its own UUID, which libvirt +does in place, also while a running VM uses it (undefining one in use is +refused). A definition libvirt refuses is reported, not undone by an undefine +and another define. libvirt's test driver has no filters, so the connection +is a mock.""" + +from unittest.mock import patch + +from computes.models import Compute +from django.contrib.auth import get_user_model +from django.contrib.messages import get_messages +from django.test import TestCase +from django.urls import reverse +from libvirt import libvirtError +from lxml import etree, html + +UUID = "6d1f7a10-0000-4000-8000-00000000c0de" +XML = (f"{UUID}" + "" + "") + + +class FilterRedefineTests(TestCase): + def setUp(self): + patcher = patch("vrtManager.connection.connection_manager.get_connection") + self.wvm = patcher.start().return_value + self.addCleanup(patcher.stop) + nwfilter = self.wvm.nwfilterLookupByName.return_value + nwfilter.XMLDesc.return_value = XML + nwfilter.name.return_value = "web" + nwfilter.UUIDString.return_value = UUID + self.wvm.listNWFilters.return_value = ["web"] + compute = Compute.objects.create(name="nwf", hostname="localhost", login="", password="", type=4) + self.client.force_login(get_user_model().objects.create_superuser("nwf", "nwf@example.com", "pw")) + self.url = reverse("nwfilter", args=[compute.id, "web"]) + + def defined(self, data): + response = self.client.post(self.url, data) + self.wvm.nwfilterLookupByName.return_value.undefine.assert_not_called() + self.wvm.nwfilterDefineXML.assert_called_once() + return etree.fromstring(self.wvm.nwfilterDefineXML.call_args.args[0]), response + + def test_a_rule_is_removed_in_place(self): + tree, _ = self.defined({"del_nwfilter_rule": "", "action": "drop", "direction": "in", "priority": "900"}) + self.assertEqual(tree.findtext("uuid"), UUID) + self.assertEqual([r.get("action") for r in tree.findall("rule")], ["accept"]) + + def test_a_reference_is_removed_in_place(self): + tree, _ = self.defined({"del_nwfilter_ref": "", "ref": "clean-traffic"}) + self.assertEqual((tree.findtext("uuid"), tree.findall("filterref")), (UUID, [])) + + def test_a_rule_is_added_in_place(self): + rule = "" + tree, response = self.defined({"add_nwfilter_rule": "", "nwfilterrule_xml": rule}) + self.assertEqual(tree.findtext("uuid"), UUID) + self.assertEqual(tree.xpath("rule[@priority='500']/*/@dstportstart"), ["22", "53"]) + self.assertRedirects(response, self.url, fetch_redirect_response=False) + + def test_a_reference_is_added_in_place(self): + tree, response = self.defined({"add_nwfilter_ref": "", "nwfilters_select": "no-spoofing"}) + self.assertEqual(tree.findtext("uuid"), UUID) + self.assertEqual(tree.xpath("filterref/@filter"), ["clean-traffic", "no-spoofing"]) + self.assertRedirects(response, self.url, fetch_redirect_response=False) + + def test_the_editor_holds_the_uuid_and_saves_in_place(self): + page = html.fromstring(self.client.get(self.url).content) + edited = page.get_element_by_id("edit_editor").text + self.assertIn(UUID, edited) + tree, _ = self.defined({"edit_nwfilter": "", "edit_xml": edited}) + self.assertEqual(tree.findtext("uuid"), UUID) + + def test_a_refused_definition_is_reported_not_undone(self): + self.wvm.nwfilterDefineXML.side_effect = libvirtError("XML error: Invalid value for attribute 'action'") + _, response = self.defined({"del_nwfilter_rule": "", "action": "drop", "direction": "in", "priority": "900"}) + self.assertIn("Invalid value for attribute 'action'", " ".join(str(m) for m in get_messages(response.wsgi_request))) diff --git a/nwfilters/views.py b/nwfilters/views.py index d82d30f6..8560ba13 100644 --- a/nwfilters/views.py +++ b/nwfilters/views.py @@ -180,12 +180,7 @@ def nwfilter(request, compute_id, nwfltr): new_xml = request.POST.get("edit_xml", "") if new_xml: - nwfilter.delete() - try: - conn.create_nwfilter(new_xml) - except libvirtError as lib_err: - conn.create_nwfilter(xml) - raise util.OperationError(lib_err) + conn.create_nwfilter(new_xml) if "del_nwfilter_rule" in request.POST: action = request.POST.get("action", "") @@ -193,46 +188,26 @@ def nwfilter(request, compute_id, nwfltr): priority = request.POST.get("priority", "") new_xml = nwfilter.delete_rule(action, direction, priority) - nwfilter.delete() - try: - conn.create_nwfilter(new_xml) - except libvirtError as lib_err: - conn.create_nwfilter(xml) - raise util.OperationError(lib_err) + conn.create_nwfilter(new_xml) if "del_nwfilter_ref" in request.POST: ref_name = request.POST.get("ref") new_xml = nwfilter.delete_ref(ref_name) - nwfilter.delete() - try: - conn.create_nwfilter(new_xml) - except libvirtError as lib_err: - conn.create_nwfilter(xml) - raise util.OperationError(lib_err) + conn.create_nwfilter(new_xml) if "add_nwfilter_rule" in request.POST: rule_xml = request.POST.get("nwfilterrule_xml", "") if not rule_xml: return HttpResponseRedirect(request.get_full_path()) new_xml = nwfilter.add_rule(rule_xml) - nwfilter.delete() - try: - conn.create_nwfilter(new_xml) - except libvirtError as lib_err: - conn.create_nwfilter(xml) - raise util.OperationError(lib_err) + conn.create_nwfilter(new_xml) if "add_nwfilter_ref" in request.POST: ref_name = request.POST.get("nwfilters_select", "") if not ref_name: return HttpResponseRedirect(request.get_full_path()) new_xml = nwfilter.add_ref(ref_name) - nwfilter.delete() - try: - conn.create_nwfilter(new_xml) - except libvirtError as lib_err: - conn.create_nwfilter(xml) - raise util.OperationError(lib_err) + conn.create_nwfilter(new_xml) return HttpResponseRedirect(request.get_full_path()) conn.close() diff --git a/vrtManager/nwfilters.py b/vrtManager/nwfilters.py index 4ad768e8..2d183d3c 100644 --- a/vrtManager/nwfilters.py +++ b/vrtManager/nwfilters.py @@ -37,14 +37,11 @@ class wvmNWFilter(wvmConnect): def get_name(self): return self.nwfilter.name() - def delete(self): - self.nwfilter.undefine() - def get_xml(self): - tree = ElementTree.fromstring(self._XMLDesc(0)) - uuid = tree.find("uuid") - tree.remove(uuid) - return ElementTree.tostring(tree).decode() + """With the UUID: defined again with it, libvirt changes the filter in + place, also while a VM uses it. It refuses XML it cannot parse or + apply to the running VMs' interfaces.""" + return self._XMLDesc(0) def get_filter_refs(self): tree = ElementTree.fromstring(self._XMLDesc(0))