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))