From fbc6c4976f88188b383bf31141b2c1cf0a097cc2 Mon Sep 17 00:00:00 2001 From: catborise Date: Sat, 3 Oct 2026 17:56:47 +0300 Subject: [PATCH] fix(instances): change the edited NIC instead of adding a new one The edit form posts net-*- fields for NIC n and carries only the new MAC. The backend read index 0 and looked the NIC up by the new MAC, so editing a MAC detached nothing and attached a second NIC, and edits of any NIC but the first were applied with empty values. The form now also posts the current MAC (net-old-mac-). The view reads the submitted NIC's fields, validates the new MAC and reports errors. The backend replaces the NIC with the old MAC, refuses an unknown one, and on a failed attach restores the old NIC and raises instead of reporting success. The view crashed with a TypeError without a Referer header; it now returns to the instance page, and a Referer from another site is ignored. --- .../templates/instances/settings_tab.html | 2 +- instances/test_change_network.py | 57 ++++++++++++++++ instances/tests.py | 42 ++++++++++++ instances/views.py | 68 +++++++++++++------ vrtManager/instance.py | 31 ++++----- vrtManager/test_change_network.py | 56 +++++++++++++++ 6 files changed, 216 insertions(+), 40 deletions(-) create mode 100644 instances/test_change_network.py create mode 100644 vrtManager/test_change_network.py diff --git a/instances/templates/instances/settings_tab.html b/instances/templates/instances/settings_tab.html index 183aecbf..c75cb029 100644 --- a/instances/templates/instances/settings_tab.html +++ b/instances/templates/instances/settings_tab.html @@ -412,7 +412,7 @@
- +
diff --git a/instances/test_change_network.py b/instances/test_change_network.py new file mode 100644 index 00000000..2f97028f --- /dev/null +++ b/instances/test_change_network.py @@ -0,0 +1,57 @@ +from unittest.mock import patch + +from computes.models import Compute +from django.contrib.auth import get_user_model +from django.test import TestCase +from django.urls import reverse +from instances.models import Instance + + +class ChangeNetworkViewTestCase(TestCase): + def setUp(self): + admin = get_user_model().objects.create_superuser("net_admin", "n@example.com", "pw") + self.client.force_login(admin) + compute = Compute.objects.create(name="net-compute", hostname="127.0.0.1", login="", password="", type=4) + self.instance = Instance.objects.create( + compute=compute, name="net-vm", uuid="11111111-2222-3333-4444-555555555555" + ) + self.url = reverse("instances:change_network", args=[self.instance.id]) + + def post(self, data): + with patch("instances.models.wvmInstance") as wvm: + wvm.return_value.get_status.return_value = 5 + self.client.post(self.url, data, HTTP_REFERER="/instances/%d/" % self.instance.id) + return wvm.return_value.change_network + + def test_changes_the_nic_of_the_submitted_form(self): + change = self.post({ + "net-old-mac-1": "52:54:00:00:00:02", + "net-mac-1": "52:54:00:00:00:03", + "net-source-1": "net:default", + "net-nwfilter-1": "", + "net-model-1": "e1000", + }) + change.assert_called_once_with("52:54:00:00:00:02", "52:54:00:00:00:03", "default", "net", "e1000", "") + + def test_invalid_mac_is_rejected(self): + change = self.post({ + "net-old-mac-0": "52:54:00:00:00:01", + "net-mac-0": "52:54:00:00:00:01'/>", + "net-source-0": "net:default", + }) + change.assert_not_called() + + def test_missing_old_mac_is_rejected(self): + change = self.post({"net-mac-0": "52:54:00:00:00:03", "net-source-0": "net:default"}) + change.assert_not_called() + + def test_missing_referer_redirects_to_the_instance(self): + with patch("instances.models.wvmInstance") as wvm: + wvm.return_value.get_status.return_value = 5 + response = self.client.post(self.url, { + "net-old-mac-0": "52:54:00:00:00:01", + "net-source-0": "net:default", + "net-model-0": "virtio", + }) + self.assertEqual(response.status_code, 302) + self.assertEqual(response.url, f"/instances/{self.instance.id}/#network") diff --git a/instances/tests.py b/instances/tests.py index d7a22295..c75b29c6 100644 --- a/instances/tests.py +++ b/instances/tests.py @@ -917,6 +917,7 @@ class InstancesTestCase(TestCase): response = self.client.post( reverse("instances:change_network", args=[self.instance.id]), { + "net-old-mac-0": "52:54:00:a2:3c:e7", "net-mac-0": "52:54:00:a2:3c:e8", "net-source-0": "net:default", "net-nwfilter-0": "", @@ -927,8 +928,49 @@ class InstancesTestCase(TestCase): self.assertEqual(response.status_code, 302) del self.instance.networks + self.assertEqual(len(self.instance.networks), 1) self.assertEqual(self.instance.networks[0]["mac"], "52:54:00:a2:3c:e8") + # The second NIC's form changes only the second NIC + self.client.post( + reverse("instances:add_network", args=[self.instance.id]), + {"add-net-mac": "52:54:00:a2:3c:e9", "add-net-network": "net:default"}, + HTTP_REFERER=reverse("index"), + ) + self.client.post( + reverse("instances:change_network", args=[self.instance.id]), + { + "net-old-mac-1": "52:54:00:a2:3c:e9", + "net-mac-1": "52:54:00:a2:3c:ea", + "net-source-1": "net:default", + "net-nwfilter-1": "", + "net-model-1": "e1000", + }, + HTTP_REFERER=reverse("index"), + ) + del self.instance.networks + self.assertEqual([n["mac"] for n in self.instance.networks], ["52:54:00:a2:3c:e8", "52:54:00:a2:3c:ea"]) + self.assertEqual(self.instance.networks[1]["model"], "e1000") + + # Restore the shared test VM + self.client.post( + reverse("instances:delete_network", args=[self.instance.id]), + {"delete_network": "52:54:00:a2:3c:ea"}, + HTTP_REFERER=reverse("index"), + ) + self.client.post( + reverse("instances:change_network", args=[self.instance.id]), + { + "net-old-mac-0": "52:54:00:a2:3c:e8", + "net-mac-0": "52:54:00:a2:3c:e7", + "net-source-0": "net:default", + "net-model-0": "virtio", + }, + HTTP_REFERER=reverse("index"), + ) + del self.instance.networks + self.assertEqual([n["mac"] for n in self.instance.networks], ["52:54:00:a2:3c:e7"]) + def test_add_delete_network(self): self.assertEqual(len(self.instance.networks), 1) net_mac = self.instance.networks[0]["mac"] diff --git a/instances/views.py b/instances/views.py index 28d8d585..83b70457 100755 --- a/instances/views.py +++ b/instances/views.py @@ -317,6 +317,16 @@ def sshkeys(request, pk): return JsonResponse(instance_keys, safe=False) +def _same_origin_referer(request, default): + """The Referer without its fragment if it is on this site, else default.""" + referer = request.META.get("HTTP_REFERER") + if referer and url_has_allowed_host_and_scheme( + url=referer, allowed_hosts={request.get_host()}, require_https=request.is_secure() + ): + return referer.split("#")[0] + return default + + def get_safe_redirect(request, default=None): referer = request.META.get("HTTP_REFERER") if referer and url_has_allowed_host_and_scheme( @@ -1441,34 +1451,52 @@ def set_video_model(request, pk): @serialize_instance_mutation def change_network(request, pk): instance = get_instance(request.user, pk) + back = _same_origin_referer(request, reverse("instances:instance", args=[pk])) + "#network" - msg = _("Change network") - network_data = {} + # Each NIC has its own form; its fields end with that NIC's index. + nums = [key[len("net-old-mac-"):] for key in request.POST if key.startswith("net-old-mac-")] + if len(nums) != 1: + messages.error(request, _("Select one network interface to change")) + return redirect(back) + num = nums[0] + old_mac = request.POST[f"net-old-mac-{num}"] + mac = request.POST.get(f"net-mac-{num}", "").strip() or old_mac + try: + util.validate_macaddr(mac) + except ValueError as err: + messages.error(request, err) + return redirect(back) - for post in request.POST: - if post.startswith("net-source-"): - (source, source_type) = utils.get_network_tuple(request.POST.get(post)) - network_data[post] = source - network_data[post + "-type"] = source_type + (source, source_type) = utils.get_network_tuple(request.POST.get(f"net-source-{num}", "")) + if source_type == "iface": + iface = wvmInterface( + instance.compute.hostname, + instance.compute.login, + instance.compute.password, + instance.compute.type, + source, + ) + source_type = iface.get_type() - if source_type == "iface": - iface = wvmInterface( - instance.compute.hostname, - instance.compute.login, - instance.compute.password, - instance.compute.type, - source, - ) - network_data[post + "-type"] = iface.get_type() - elif post.startswith("net-"): - network_data[post] = request.POST.get(post, "") + try: + instance.proxy.change_network( + old_mac, + mac, + source, + source_type, + request.POST.get(f"net-model-{num}"), + request.POST.get(f"net-nwfilter-{num}", ""), + ) + except libvirtError as err: + messages.error(request, err) + return redirect(back) - instance.proxy.change_network(network_data) + msg = _("Change network: %(mac)s") % {"mac": old_mac} addlogmsg(request.user.username, instance.compute.name, instance.name, msg) msg = _("Network Device Config is changed. Please shutdown instance to activate.") if instance.proxy.get_status() != 5: messages.success(request, msg) - return redirect(request.META.get("HTTP_REFERER") + "#network") + return redirect(back) @require_POST diff --git a/vrtManager/instance.py b/vrtManager/instance.py index ef9c91e4..c82c4ad5 100644 --- a/vrtManager/instance.py +++ b/vrtManager/instance.py @@ -1707,27 +1707,20 @@ class wvmInstance(wvmConnect): return new_xml return None - def change_network(self, network_data): - net_mac = network_data.get("net-mac-0") - net_source = network_data.get("net-source-0") - net_source_type = network_data.get("net-source-0-type") - net_filter = network_data.get("net-nwfilter-0") - net_model = network_data.get("net-model-0") - - # Remove interface first, but keep network interface XML definition - # If there is an error happened while adding changed one, then add removed one to back. - status = self.delete_network(net_mac) + def change_network(self, old_mac, mac, source, source_type, model, nwfilter): + """Replace the NIC with old_mac; put it back if the new one cannot be added.""" + old_xml = self.delete_network(old_mac) + if old_xml is None: + raise libvirtError(f"No network interface with MAC {old_mac}") try: - self.add_network( - net_mac, net_source, net_source_type, net_model, net_filter - ) + self.add_network(mac, source, source_type, model, nwfilter) except libvirtError: - if status is not None: - if self.get_status() == 1: - self.instance.attachDeviceFlags(status, VIR_DOMAIN_AFFECT_LIVE) - self.instance.attachDeviceFlags(status, VIR_DOMAIN_AFFECT_CONFIG) - if self.get_status() == 5: - self.instance.attachDeviceFlags(status, VIR_DOMAIN_AFFECT_CONFIG) + if self.get_status() == 1: + self.instance.attachDeviceFlags(old_xml, VIR_DOMAIN_AFFECT_LIVE) + self.instance.attachDeviceFlags(old_xml, VIR_DOMAIN_AFFECT_CONFIG) + if self.get_status() == 5: + self.instance.attachDeviceFlags(old_xml, VIR_DOMAIN_AFFECT_CONFIG) + raise def change_network_oldway(self, network_data): """ diff --git a/vrtManager/test_change_network.py b/vrtManager/test_change_network.py new file mode 100644 index 00000000..7da19837 --- /dev/null +++ b/vrtManager/test_change_network.py @@ -0,0 +1,56 @@ +import unittest +from unittest.mock import MagicMock + +from django.conf import settings + +if not settings.configured: + settings.configure(MAC_OUI="52:54:10") + +from libvirt import VIR_DOMAIN_AFFECT_CONFIG, libvirtError +from vrtManager.instance import wvmInstance + +DOMAIN_XML = """ + + +""" + + +def stopped_instance(): + inst = wvmInstance.__new__(wvmInstance) + inst.instance = MagicMock() + inst._XMLDesc = MagicMock(return_value=DOMAIN_XML) + inst.get_status = MagicMock(return_value=5) + return inst + + +class TestChangeNetwork(unittest.TestCase): + def test_replaces_the_nic_with_the_old_mac(self): + inst = stopped_instance() + inst.change_network("52:54:00:00:00:02", "52:54:00:00:00:03", "default", "net", "e1000", "") + + detached = inst.instance.detachDeviceFlags.call_args[0][0] + self.assertIn("52:54:00:00:00:02", detached) + attached = inst.instance.attachDeviceFlags.call_args[0][0] + self.assertIn("52:54:00:00:00:03", attached) + self.assertIn("e1000", attached) + + def test_unknown_old_mac_adds_nothing(self): + inst = stopped_instance() + with self.assertRaises(libvirtError): + inst.change_network("52:54:00:00:00:09", "52:54:00:00:00:03", "default", "net", "virtio", "") + inst.instance.detachDeviceFlags.assert_not_called() + inst.instance.attachDeviceFlags.assert_not_called() + + def test_failed_add_restores_the_old_nic(self): + inst = stopped_instance() + inst.instance.attachDeviceFlags.side_effect = [libvirtError("bad source"), 0] + with self.assertRaises(libvirtError): + inst.change_network("52:54:00:00:00:01", "52:54:00:00:00:03", "missing", "net", "virtio", "") + + restored, flags = inst.instance.attachDeviceFlags.call_args[0] + self.assertIn("52:54:00:00:00:01", restored) + self.assertEqual(flags, VIR_DOMAIN_AFFECT_CONFIG) + + +if __name__ == "__main__": + unittest.main()