diff --git a/interfaces/api/viewsets.py b/interfaces/api/viewsets.py index 8d8189f5..a76445a8 100644 --- a/interfaces/api/viewsets.py +++ b/interfaces/api/viewsets.py @@ -3,7 +3,7 @@ from django.shortcuts import get_object_or_404 from computes.models import Compute from rest_framework import viewsets from rest_framework.response import Response -from vrtManager.interface import wvmInterfaces, wvmInterface +from vrtManager.interface import wvmInterfaces from .serializers import InterfacesSerializer @@ -17,15 +17,10 @@ class InterfaceViewSet(viewsets.ViewSet): serializer_class = InterfacesSerializer def list(self, request, compute_pk=None): - queryset = [] compute = get_object_or_404(Compute, pk=compute_pk) conn = wvmInterfaces(compute.hostname, compute.login, compute.password, compute.type) - ifaces = conn.get_ifaces() - - for iface in ifaces: - interf = wvmInterface(compute.hostname, compute.login, compute.password, compute.type, iface) - queryset.append(interf.get_details()) + queryset = conn.get_ifaces_info() serializer = InterfacesSerializer(queryset, many=True, context={'request': request}) diff --git a/interfaces/forms.py b/interfaces/forms.py index 2fb54c25..27282b10 100644 --- a/interfaces/forms.py +++ b/interfaces/forms.py @@ -32,12 +32,14 @@ class AddInterface(forms.Form): stp = forms.ChoiceField(required=False, choices=(("on", "on"), ("off", "off"))) delay = forms.IntegerField(required=False, min_value=0) - def __init__(self, *args, netdevs=(), defined=(), **kwargs): - # The host's network devices: a bridge member, or the device an - # ethernet interface configures, must be one of them. The interfaces - # the host defines: defining one again would replace its configuration. + def __init__(self, *args, netdevs=(), members=(), defined=(), **kwargs): + # The host's network devices. members: those a bridge can take + # (wvmInterfaces.get_member_candidates); an ethernet interface + # configures one of them too. The interfaces the host defines: + # defining one again would replace its configuration. super().__init__(*args, **kwargs) self.netdevs = set(netdevs) + self.members = set(members) self.defined = set(defined) def clean_ipv4_addr(self): @@ -122,8 +124,10 @@ class AddInterface(forms.Form): self.add_error("netdev", _("A bridge needs a device")) elif netdev and netdev not in self.netdevs: self.add_error("netdev", _("The device is not on this host")) + elif netdev and netdev not in self.members: + self.add_error("netdev", _("The device is a bridge, bond or VLAN, a member of one, or a VM's")) elif netdev and netdev == name: self.add_error("netdev", _("A bridge cannot contain itself")) - elif cleaned.get("itype") == "ethernet" and name and name not in self.netdevs: - self.add_error("name", _("An ethernet interface configures a device of this host")) + elif cleaned.get("itype") == "ethernet" and name and name not in self.members: + self.add_error("name", _("An ethernet interface configures a device of this host in no bridge, bond or VLAN")) return cleaned diff --git a/interfaces/templates/interface.html b/interfaces/templates/interface.html index b133cd9a..d27e0b96 100644 --- a/interfaces/templates/interface.html +++ b/interfaces/templates/interface.html @@ -16,44 +16,42 @@ {% if changeable is False %}{% include 'interfaces_unchangeable.html' %}{% endif %}
+ {% if info %}
{% trans "Interface" %}
{{ iface }}
-
{% trans "IPv4" %} ({% if ipv4 == None %}{% trans 'None' %}{% else %}{{ ipv4_type }}{% endif %})
-
{{ ipv4 }}
-
{% trans "IPv6" %} ({% if ipv6 == None %}{% trans 'None' %}{% else %}{{ ipv6_type }}{% endif %})
-
{{ ipv6 }}
+
{% trans "IPv4" %}{% if info.ipv4.method %} ({{ info.ipv4.method }}){% endif %}
+
{{ info.ipv4.addresses|join:", "|default:"–" }}
+
{% trans "IPv6" %}{% if info.ipv6.method %} ({{ info.ipv6.method }}){% endif %}
+
{{ info.ipv6.addresses|join:", "|default:"–" }}
{% trans "MAC" %}
-
{{ mac }}
+
{{ info.mac|default:"–" }}
{% trans "Interface Type" %}
-
{{ itype }}
- {% if itype == 'bridge' %} -
{% trans "Bridge Device" %}
-
{{ bridge }}
- {% endif %} +
{{ info.type }}
{% trans "Boot Mode" %}
-
{{ start_mode }}
+
{{ info.start_mode|default:"–" }}
{% trans "State" %}
- {% if changeable is False %} - {% if state == 0 %}{% trans "Inactive" %}{% else %}{% trans "Active" %}{% endif %} - {% else %} + {% if changeable %}
{% csrf_token %} {% if state == 0 %} - + {% else %} {% endif %}
+ {% else %} + {% if state == 0 %}{% trans "Inactive" %}{% else %}{% trans "Active" %}{% endif %} {% endif %}
+ {% endif %}
+ {% if info.members %}
-
{% trans 'Slaves' %}
- {% if itype == 'bridge' %} +
{% trans 'Members' %}
@@ -66,20 +64,20 @@ - {% for iface in slave_ifaces %} + {% for member in info.members %} - - - - - + + + + + {% endfor %}
{{ iface.mac }}{{ iface.name }}{{ iface.type }}{{ iface.speed }}{{ iface.state }}{{ member.mac|default:"–" }}{{ member.name }}{{ member.type }}{{ member.speed|default:"–" }}{{ member.link|default:"–" }}
- {% endif %}
+ {% endif %}
{% endblock %} \ No newline at end of file diff --git a/interfaces/tests/test_audit_rows.py b/interfaces/tests/test_audit_rows.py index 5a428695..3e4e00b1 100644 --- a/interfaces/tests/test_audit_rows.py +++ b/interfaces/tests/test_audit_rows.py @@ -21,7 +21,8 @@ class InterfaceRowTests(TestCase): def test_interface_actions(self, ifaces, iface): ifaces.return_value.can_change_interfaces.return_value = True ifaces.return_value.get_net_devices.return_value = ["eth0"] - ifaces.return_value.get_ifaces.return_value = [] + ifaces.return_value.get_member_candidates.return_value = ["eth0"] + ifaces.return_value.get_ifaces_info.return_value = [] iface.return_value.can_change_interfaces.return_value = True self.client.post(reverse("interfaces", args=[self.compute.id]), { "create": "", "name": "br1", "itype": "bridge", "start_mode": "onboot", "netdev": "eth0", diff --git a/interfaces/tests/test_delete.py b/interfaces/tests/test_delete.py index 9222791e..e2a93eab 100644 --- a/interfaces/tests/test_delete.py +++ b/interfaces/tests/test_delete.py @@ -1,23 +1,33 @@ -"""Deleting a host interface: netcf removes a bridge's members with the -bridge, so they are defined again on their own (as virsh iface-unbridge).""" +"""Deleting a host interface: a stopped one only; netcf removes a bridge's +members with the bridge, so they are defined again on their own (as virsh +iface-unbridge), all in one change of the host's configuration, rolled back +on a failure; a bridge with addresses and several members is refused.""" from unittest.mock import MagicMock +import libvirt from django.test import SimpleTestCase from lxml import etree from vrtManager.interface import wvmInterface +from vrtManager.util import OperationError ADDRESS = "" BOND = "" -def delete(xml): +def interface(xml, active=False): conn = wvmInterface.__new__(wvmInterface) - conn.iface = MagicMock() - conn.iface.XMLDesc.return_value = xml + conn.iface = MagicMock(**{"XMLDesc.return_value": xml, "isActive.return_value": active, "name.return_value": "br1"}) conn.wvm = MagicMock() + return conn + + +def delete(xml): + conn = interface(xml) conn.delete_iface() conn.iface.undefine.assert_called_once_with() + conn.wvm.changeBegin.assert_called_once_with(0) + conn.wvm.changeCommit.assert_called_once_with(0) return [etree.fromstring(call.args[0]) for call in conn.wvm.interfaceDefineXML.call_args_list] @@ -30,9 +40,9 @@ class DeleteInterfaceTestCase(SimpleTestCase): self.assertEqual(bond.xpath("start/@mode"), ["onboot"]) self.assertEqual(bond.xpath("protocol/ip/@address"), ["192.0.2.10"]) - def test_several_members_take_the_start_mode_but_no_address(self): + def test_several_members_take_the_start_mode(self): members = delete( - f"{ADDRESS}" + "" "" ) self.assertEqual([m.get("name") for m in members], ["eth1", "eth2"]) @@ -40,5 +50,43 @@ class DeleteInterfaceTestCase(SimpleTestCase): self.assertEqual(member.xpath("start/@mode"), ["onboot"]) self.assertEqual(member.xpath("protocol"), []) + def test_a_bridge_with_addresses_and_several_members_is_refused(self): + # which member would take the addresses is the user's choice + conn = interface(f"{ADDRESS}" + "") + with self.assertRaisesRegex(OperationError, "has addresses and several members"): + conn.delete_iface() + conn.iface.undefine.assert_not_called() + + def test_an_active_interface_is_refused(self): + conn = interface("", active=True) + with self.assertRaisesRegex(OperationError, "Stop interface br1 before deleting it"): + conn.delete_iface() + conn.iface.undefine.assert_not_called() + + def test_a_member_libvirt_refuses_rolls_the_whole_change_back(self): + conn = interface(f"{BOND}") + conn.wvm.interfaceDefineXML.side_effect = libvirt.libvirtError("netcf: cannot write") + with self.assertRaises(Exception): + conn.delete_iface() + conn.wvm.changeRollback.assert_called_once_with(0) + conn.wvm.changeCommit.assert_not_called() + + def test_a_commit_libvirt_refuses_is_rolled_back(self): + conn = interface("") + conn.wvm.changeCommit.side_effect = libvirt.libvirtError("netcf: cannot commit") + with self.assertRaises(Exception): + conn.delete_iface() + conn.wvm.changeRollback.assert_called_once_with(0) + def test_an_interface_that_is_not_a_bridge_is_only_undefined(self): self.assertEqual(delete(f"{ADDRESS}"), []) + + def test_a_rollback_that_fails_too_tells_both(self): + conn = interface("") + conn.wvm.changeCommit.side_effect = OperationError("netcf: cannot commit") + conn.wvm.changeRollback.side_effect = OperationError("netcf: cannot roll back") + with self.assertRaises(OperationError) as raised: + conn.delete_iface() + self.assertIn("netcf: cannot commit", str(raised.exception)) + self.assertIn("netcf: cannot roll back", str(raised.exception)) diff --git a/interfaces/tests/test_forms.py b/interfaces/tests/test_forms.py index b201f9a8..893ed208 100644 --- a/interfaces/tests/test_forms.py +++ b/interfaces/tests/test_forms.py @@ -25,7 +25,7 @@ NETDEVS = ["eth0", "eth1", "enp129s0f0np0", "eth0.100", "bond-lan"] def form(**values): - return AddInterface(data={**BASE, **values}, netdevs=NETDEVS) + return AddInterface(data={**BASE, **values}, netdevs=NETDEVS, members=NETDEVS) class DefinedNameTestCase(SimpleTestCase): @@ -33,9 +33,9 @@ class DefinedNameTestCase(SimpleTestCase): defined = ["br0", "bond-lan"] # as libvirt lists them for values in ({"name": "br0"}, {"name": "bond-lan", "itype": "ethernet", "netdev": ""}): with self.subTest(**values): - f = AddInterface(data={**BASE, **values}, netdevs=NETDEVS, defined=defined) + f = AddInterface(data={**BASE, **values}, netdevs=NETDEVS, members=NETDEVS, defined=defined) self.assertEqual(f.errors["name"], [f"The host already defines interface {values['name']}"]) - self.assertTrue(AddInterface(data={**BASE, "ipv6_type": "none"}, netdevs=NETDEVS, defined=defined).is_valid()) + self.assertTrue(AddInterface(data={**BASE, "ipv6_type": "none"}, netdevs=NETDEVS, members=NETDEVS, defined=defined).is_valid()) class Ipv6FormTestCase(SimpleTestCase): @@ -74,6 +74,7 @@ class Ipv6FormTestCase(SimpleTestCase): class Ipv6XmlTestCase(SimpleTestCase): def xml(self, gw): conn = wvmInterfaces.__new__(wvmInterfaces) + conn.wvm = MagicMock() conn.define_iface = MagicMock() conn.get_iface = MagicMock() conn.get_iface.return_value.XMLDesc.return_value = "" @@ -158,6 +159,7 @@ class InterfaceNameTestCase(SimpleTestCase): class Ipv4XmlTestCase(SimpleTestCase): def xml(self, addr, gw): conn = wvmInterfaces.__new__(wvmInterfaces) + conn.wvm = MagicMock() conn.define_iface = MagicMock() conn.get_iface = MagicMock() conn.get_iface.return_value.XMLDesc.return_value = "" @@ -192,6 +194,13 @@ class HostDeviceTestCase(SimpleTestCase): self.assertFalse(f.is_valid()) self.assertIn("name", f.errors) + def test_an_ethernet_interface_is_no_device_of_another_interface(self): + # netcf lists a bridge, not its members: eth1 is in br0 here + f = AddInterface(data={**BASE, "itype": "ethernet", "name": "eth1", "ipv6_type": "none"}, + netdevs=NETDEVS, members=["eth0"], defined=["br0"]) + self.assertFalse(f.is_valid()) + self.assertIn("name", f.errors) + def no_interface(name): err = libvirt.libvirtError("Interface not found: no interface with matching name '%s'" % name) @@ -205,6 +214,7 @@ class BridgeMemberXmlTestCase(SimpleTestCase): def member(self, host_xml): conn = wvmInterfaces.__new__(wvmInterfaces) + conn.wvm = MagicMock() conn.define_iface = MagicMock() def get_iface(name): @@ -253,6 +263,7 @@ class BridgeMemberXmlTestCase(SimpleTestCase): def test_other_lookup_errors_are_not_hidden(self): conn = wvmInterfaces.__new__(wvmInterfaces) + conn.wvm = MagicMock() conn.define_iface = MagicMock() err = libvirt.libvirtError("cannot recv data") err.err = (libvirt.VIR_ERR_SYSTEM_ERROR, libvirt.VIR_FROM_RPC, str(err), 2, None, None, None, 0, 0) @@ -261,14 +272,17 @@ class BridgeMemberXmlTestCase(SimpleTestCase): conn.create_iface("br1", "bridge", "onboot", "bond0", "none", "", "", "none", "", "", "on", 0) self.assertFalse(conn.define_iface.called) - def test_an_interface_that_does_not_start_is_undefined(self): + def test_an_interface_that_does_not_start_is_rolled_back(self): + # the definition, and the member's own configuration it changed, as they were conn = wvmInterfaces.__new__(wvmInterfaces) + conn.wvm = MagicMock() conn.define_iface = MagicMock() iface = conn.define_iface.return_value iface.create.side_effect = libvirt.libvirtError("failed to bring up br1") with self.assertRaises(libvirt.libvirtError): conn.create_iface("br1", "ethernet", "onboot", "", "none", "", "", "none", "", "", "on", 0) - iface.undefine.assert_called_once_with() + conn.wvm.changeRollback.assert_called_once_with(0) + conn.wvm.changeCommit.assert_not_called() class BridgeSettingsTestCase(SimpleTestCase): @@ -310,9 +324,10 @@ class CreateViewTestCase(TestCase): for netdev, created in (("eth9", False), ("eth0", True)): with self.subTest(netdev=netdev), patch("interfaces.views.wvmInterfaces") as conn_cls: conn = conn_cls.return_value - conn.get_ifaces.return_value = [] + conn.get_ifaces_info.return_value = [] conn.can_change_interfaces.return_value = True conn.get_net_devices.return_value = ["eth0", "eth1"] + conn.get_member_candidates.return_value = ["eth0", "eth1"] self.client.post( reverse("interfaces", args=[compute.id]), {**BASE, "netdev": netdev, "ipv6_type": "none", "create": ""}, @@ -325,7 +340,7 @@ class CreateViewTestCase(TestCase): self.client.force_login(admin) with patch("interfaces.views.wvmInterfaces") as conn_cls: conn = conn_cls.return_value - conn.get_ifaces.return_value = [] + conn.get_ifaces_info.return_value = [] conn.can_change_interfaces.return_value = True conn.get_net_devices.side_effect = RuntimeError("nodedev driver not running") response = self.client.post( @@ -351,7 +366,7 @@ class UnchangeableHostTestCase(TestCase): def interfaces_page(self, changeable, post=None): with patch("interfaces.views.wvmInterfaces") as conn_cls: conn = conn_cls.return_value - conn.get_ifaces.return_value = [] + conn.get_ifaces_info.return_value = [] conn.can_change_interfaces.return_value = changeable conn.get_net_devices.return_value = ["eth0", "eth1"] url = reverse("interfaces", args=[self.compute.id]) @@ -377,7 +392,6 @@ class UnchangeableHostTestCase(TestCase): conn = conn_cls.return_value conn.can_change_interfaces.return_value = changeable conn.is_active.return_value = state - conn.get_bridge_slave_ifaces.return_value = [] url = reverse("interface", args=[self.compute.id, "br0"]) response = self.client.post(url, post) if post else self.client.get(url) return conn, response @@ -395,11 +409,12 @@ class UnchangeableHostTestCase(TestCase): _conn, response = self.interface_page(True, 0) self.assertIn('name="start"', response.content.decode()) - def test_a_host_that_could_not_be_read_keeps_the_page_as_it_was(self): + def test_a_host_that_could_not_be_read_offers_nothing(self): + # not knowing whether the host can change interfaces is not a yes with patch("interfaces.views.wvmInterface", side_effect=libvirt.libvirtError("unreachable")): page = self.client.get(reverse("interface", args=[self.compute.id, "br0"])).content.decode() self.assertNotIn(self.NOTE, page) - self.assertIn("Interface start/stop/delete form", page) + self.assertNotIn("Interface start/stop/delete form", page) def test_start_stop_and_delete_posts_change_nothing(self): for action in ("start", "stop", "delete"): diff --git a/interfaces/tests/test_reads.py b/interfaces/tests/test_reads.py new file mode 100644 index 00000000..367f0159 --- /dev/null +++ b/interfaces/tests/test_reads.py @@ -0,0 +1,92 @@ +"""What the panel reads of the host's interfaces: each one's XML once, its +link as reported (an unknown speed is none), its members; which devices a new +bridge can take; how each family is addressed.""" + +from unittest.mock import MagicMock + +import libvirt +from django.test import SimpleTestCase +from vrtManager.interface import wvmInterface, wvmInterfaces + +BRIDGE = ("" + "" + "" + "") +ETHERNET = "" + + +def iface(xml, active=1): + name = xml.split("name='")[1].split("'")[0] + mac = xml.split("mac address='")[1].split("'")[0] + return MagicMock(**{"name.return_value": name, "MACString.return_value": mac, "XMLDesc.return_value": xml, + "isActive.return_value": active}) + + +def error(code): + err = libvirt.libvirtError("refused") + err.get_error_code = lambda: code + return err + + +class ReadTests(SimpleTestCase): + def interfaces(self, *xmls, networks=()): + conn = wvmInterfaces.__new__(wvmInterfaces) + conn.wvm = MagicMock(**{"listAllInterfaces.return_value": [iface(x) for x in xmls], + "listAllNetworks.return_value": [MagicMock(**{"XMLDesc.return_value": n}) for n in networks]}) + return conn + + def test_each_interface_is_read_once(self): + conn = self.interfaces(BRIDGE, ETHERNET) + br0, eno2 = conn.get_ifaces_info() + self.assertEqual((br0["type"], br0["link"], br0["speed"], br0["mtu"], br0["mac"]), + ("bridge", "up", None, "1500", "52:54:00:aa:01:02")) + self.assertEqual(br0["members"], [{"name": "eno1", "type": "ethernet", "mac": "52:54:00:aa:01:03", + "link": "up", "speed": "1000"}]) + self.assertEqual((eno2["link"], eno2["members"]), ("down", [])) + for listed in conn.wvm.listAllInterfaces.return_value: + # name and MAC are held by libvirt's object: no call to the host + self.assertEqual([c[0] for c in listed.mock_calls], ["XMLDesc", "name", "MACString", "isActive"]) + + def test_an_interface_libvirt_cannot_write_is_read_as_saved(self): + # netcf writes a link speed of -1 for a virtio card, then refuses that XML + listed = iface(ETHERNET) + listed.XMLDesc.side_effect = lambda flags=0: ETHERNET if flags else (_ for _ in ()).throw(error(libvirt.VIR_ERR_XML_ERROR)) + conn = self.interfaces() + conn.wvm.listAllInterfaces.return_value = [listed] + (eno2,) = conn.get_ifaces_info() + self.assertEqual(eno2["name"], "eno2") + + def test_a_bridge_takes_no_bridge_member_network_bridge_or_tap(self): + # as configured: a member the running bridge does not have yet is taken too + conn = self.interfaces(BRIDGE, ETHERNET, networks=[""]) + self.assertEqual(conn.get_member_candidates(["br0", "eno1", "eno2", "lo", "virbr0", "vnet3", "macvtap0"]), ["eno2"]) + for listed in conn.wvm.listAllInterfaces.return_value: + listed.XMLDesc.assert_called_with(libvirt.VIR_INTERFACE_XML_INACTIVE) + + def test_a_device_in_a_bond_of_a_bridge_is_taken(self): + nested = ("" + "" + "") + conn = self.interfaces(nested) + self.assertEqual(conn.get_member_candidates(["br1", "bond0", "eno3", "eno4", "eno5"]), ["eno5"]) + + def test_each_family_says_how_it_is_addressed(self): + conn = wvmInterface.__new__(wvmInterface) + saved = ("" + "" + "") + running = ("" + "") + conn.iface = MagicMock(**{"name.return_value": "eth0", "MACString.return_value": "52:54:00:00:00:01", + "XMLDesc.side_effect": lambda flags=0: saved if flags else running}) + info = conn.get_info() + self.assertEqual(info["start_mode"], "onboot") + self.assertEqual(info["ipv4"], {"method": "static", "addresses": ["198.51.100.10/24"]}) + self.assertEqual(info["ipv6"], {"method": "autoconf", "addresses": ["2001:db8::5/64", "2001:db8::6/64"]}) + self.assertEqual(conn.iface.XMLDesc.call_count, 2) # the running and the saved definition, once each + + def test_an_address_without_a_prefix_is_shown_without_one(self): + conn = wvmInterface.__new__(wvmInterface) + xml = "" + conn.iface = MagicMock(**{"name.return_value": "eth0", "MACString.return_value": "", "XMLDesc.return_value": xml}) + self.assertEqual(conn.get_info()["ipv4"]["addresses"], ["192.0.2.10"]) diff --git a/interfaces/views.py b/interfaces/views.py index 07fb30ca..a3abee67 100644 --- a/interfaces/views.py +++ b/interfaces/views.py @@ -13,70 +13,41 @@ from admin.decorators import superuser_only @superuser_only def interfaces(request, compute_id): - """ - :param request: - :param compute_id: - :return: - """ - - ifaces_all = [] + """The host's interfaces, and a new one where its libvirt can define them.""" compute = get_object_or_404(Compute, pk=compute_id) try: - conn = wvmInterfaces( - compute.hostname, - compute.login, - compute.password, - compute.type - ) - ifaces = conn.get_ifaces() + conn = wvmInterfaces(compute.hostname, compute.login, compute.password, compute.type) + ifaces_all = conn.get_ifaces_info() changeable = conn.can_change_interfaces() # the host's devices are only the create form's choices - netdevs, netdev_error = [], None + netdevs, members, netdev_error = [], [], None if changeable: try: netdevs = conn.get_net_devices() - except Exception as err: + members = conn.get_member_candidates(netdevs) + except Exception as err: # a host without the node device driver too netdev_error = err messages.error(request, _("The network devices of this host could not be read: %(error)s") % {"error": err}) - for iface in ifaces: - interf = wvmInterface( - compute.hostname, - compute.login, - compute.password, - compute.type, - iface - ) - ifaces_all.append(interf.get_details()) - - if request.method == "POST": - # the page says why nothing is created on a host that cannot change - # interfaces; without the host's devices nothing can be checked - if "create" in request.POST and changeable and netdev_error is None: - form = AddInterface(request.POST, netdevs=netdevs, defined=ifaces) - if form.is_valid(): - data = form.cleaned_data - with audit(request, compute, f"Interface {data['name']}", "add"): - conn.create_iface( - data["name"], - data["itype"], - data["start_mode"], - data["netdev"], - data["ipv4_type"], - data["ipv4_addr"], - data["ipv4_gw"], - data["ipv6_type"], - data["ipv6_addr"], - data["ipv6_gw"], - data["stp"], - data["delay"], - ) - return HttpResponseRedirect(request.get_full_path()) - else: - for msg_err in form.errors.values(): - messages.error(request, msg_err.as_text()) + # the page says why nothing is created on a host that cannot change + # interfaces; without the host's devices nothing can be checked + if request.method == "POST" and "create" in request.POST and changeable and netdev_error is None: + form = AddInterface(request.POST, netdevs=netdevs, members=members, + defined=[iface["name"] for iface in ifaces_all]) + if form.is_valid(): + data = form.cleaned_data + with audit(request, compute, f"Interface {data['name']}", "add"): + conn.create_iface( + data["name"], data["itype"], data["start_mode"], data["netdev"], + data["ipv4_type"], data["ipv4_addr"], data["ipv4_gw"], + data["ipv6_type"], data["ipv6_addr"], data["ipv6_gw"], + data["stp"], data["delay"], + ) + return HttpResponseRedirect(reverse("interfaces", args=[compute_id])) + for msg_err in form.errors.values(): + messages.error(request, msg_err.as_text()) conn.close() except libvirtError as lib_err: messages.error(request, lib_err) @@ -86,50 +57,36 @@ def interfaces(request, compute_id): @superuser_only def interface(request, compute_id, iface): - """ - :param request: - :param compute_id: - :param iface: - :return: - """ - - ifaces_all = [] + """An interface of the host; started, stopped or deleted where the host's libvirt can.""" compute = get_object_or_404(Compute, pk=compute_id) try: - conn = wvmInterface( - compute.hostname, - compute.login, - compute.password, - compute.type, - iface - ) - start_mode = conn.get_start_mode() - state = conn.is_active() - mac = conn.get_mac() - itype = conn.get_type() - ipv4 = conn.get_ipv4() - ipv4_type = conn.get_ipv4_type() - ipv6 = conn.get_ipv6() - ipv6_type = conn.get_ipv6_type() - bridge = conn.get_bridge() - slave_ifaces = conn.get_bridge_slave_ifaces() + conn = wvmInterface(compute.hostname, compute.login, compute.password, compute.type, iface) changeable = conn.can_change_interfaces() # the page says why nothing is done on a host that cannot change interfaces if request.method == "POST" and changeable: - if "stop" in request.POST: - with audit(request, compute, f"Interface {iface}", "stop"): - conn.stop_iface() - return HttpResponseRedirect(request.get_full_path()) - if "start" in request.POST: - with audit(request, compute, f"Interface {iface}", "start"): - conn.start_iface() - return HttpResponseRedirect(request.get_full_path()) - if "delete" in request.POST: - with audit(request, compute, f"Interface {iface}", "delete"): - conn.delete_iface() - return HttpResponseRedirect(reverse("interfaces", args=[compute_id])) + here = reverse("interface", args=[compute_id, iface]) + subject = f"Interface {iface}" + try: + if "stop" in request.POST: + with audit(request, compute, subject, "stop"): + conn.stop_iface() + return HttpResponseRedirect(here) + if "start" in request.POST: + with audit(request, compute, subject, "start"): + conn.start_iface() + return HttpResponseRedirect(here) + if "delete" in request.POST: + with audit(request, compute, subject, "delete"): + conn.delete_iface() + return HttpResponseRedirect(reverse("interfaces", args=[compute_id])) + except libvirtError as err: # OperationError too: an active one, a bridge whose addresses go nowhere + messages.error(request, err) + return HttpResponseRedirect(here) + + info = conn.get_info() + state = conn.is_active() conn.close() except libvirtError as lib_err: messages.error(request, lib_err) diff --git a/vrtManager/connection.py b/vrtManager/connection.py index bb2a03fc..5dd5eb8b 100644 --- a/vrtManager/connection.py +++ b/vrtManager/connection.py @@ -558,15 +558,8 @@ class wvmConnect(object): return virtnet def get_ifaces(self): - """ - :return: list of host interfaces - """ - interface = [] - for inface in self.wvm.listInterfaces(): - interface.append(inface) - for inface in self.wvm.listDefinedInterfaces(): - interface.append(inface) - return interface + """The names of the host's interfaces, in every state.""" + return [iface.name() for iface in self.wvm.listAllInterfaces(0)] def get_nwfilters(self): """ @@ -1052,14 +1045,19 @@ class wvmConnect(object): return instance def can_change_interfaces(self): - """False when the host's libvirt cannot define interfaces (the udev - backend only reads them). The definition sent is invalid, so nothing is - defined: a backend without define refuses it with VIR_ERR_NO_SUPPORT, - one with define with an XML error.""" + """Whether the host's libvirt can define interfaces: the udev backend + only reads them. The definition sent is invalid, so nothing is + defined: a backend without define refuses it as unsupported, one with + define as bad XML. Any other refusal, of the connection say, is not an + answer and is raised; one the host's access rules give is a no.""" try: self.wvm.interfaceDefineXML("", 0) except libvirtError as err: - return err.get_error_code() != libvirt.VIR_ERR_NO_SUPPORT + if err.get_error_code() in (libvirt.VIR_ERR_NO_SUPPORT, libvirt.VIR_ERR_OPERATION_DENIED): + return False + if err.get_error_code() in (libvirt.VIR_ERR_XML_ERROR, libvirt.VIR_ERR_XML_DETAIL): + return True + raise return True def get_net_devices(self): diff --git a/vrtManager/interface.py b/vrtManager/interface.py index 29382763..a93a6126 100644 --- a/vrtManager/interface.py +++ b/vrtManager/interface.py @@ -1,12 +1,106 @@ +import contextlib +from functools import cached_property from xml.etree import ElementTree -from libvirt import VIR_ERR_NO_INTERFACE, VIR_INTERFACE_XML_INACTIVE, libvirtError +from libvirt import ( + VIR_ERR_NO_INTERFACE, + VIR_ERR_XML_ERROR, + VIR_INTERFACE_XML_INACTIVE, + libvirtError, +) from vrtManager import util from vrtManager.connection import wvmConnect +# a link speed the kernel gives when it does not know one +UNKNOWN_SPEEDS = ("0", "4294967295", "-1") + + +@contextlib.contextmanager +def changes(conn): + """The host's interface configuration changed as one: what fails in the + block, or its commit, is rolled back (libvirt's interface change + transaction). A rollback that fails too is told with the failure.""" + conn.changeBegin(0) + try: + yield + conn.changeCommit(0) + except BaseException as err: + try: + conn.changeRollback(0) + except libvirtError as rollback_err: + raise util.OperationError(f"{err}; rolling the change back failed too, " + f"the host's configuration may be changed: {rollback_err}") from err + raise + + +def running_xml(iface): + """The interface's running XML, or its saved one when libvirt cannot give + that: netcf writes a link speed of -1 for a device that has none (a + virtio card), then refuses its own XML.""" + try: + return iface.XMLDesc(0) + except libvirtError as err: + if err.get_error_code() != VIR_ERR_XML_ERROR: + raise + return iface.XMLDesc(VIR_INTERFACE_XML_INACTIVE) + + +def link_of(element): + """An interface's or member's link state and speed in Mb/s, as libvirt + reports them: None for what it does not.""" + link = element.find("link") + if link is None: + return None, None + speed = link.get("speed") + return link.get("state"), None if speed in (None, *UNKNOWN_SPEEDS) else speed + + +def attribute(element, path, name): + """The attribute name of the child at path, or None.""" + child = element.find(path) + return child.get(name) if child is not None else None + + +def interface_info(iface, tree): + """What an interface (its libvirt object, which holds its name and MAC) + and its XML tell: type, MAC, MTU, link state and speed, and the members + of a bridge or bond, or a VLAN's device, as reported.""" + state, speed = link_of(tree) + members = [] + for member in tree.findall("bridge/interface") + tree.findall("bond/interface") + tree.findall("vlan/interface"): + member_state, member_speed = link_of(member) + members.append({"name": member.get("name"), "type": member.get("type"), "mac": attribute(member, "mac", "address"), + "link": member_state, "speed": member_speed}) + return {"name": iface.name(), "type": tree.get("type"), "mac": iface.MACString() or None, + "mtu": attribute(tree, "mtu", "size"), "link": state, "speed": speed, "members": members} + class wvmInterfaces(wvmConnect): + def get_ifaces_info(self): + """Every interface of the host (interface_info) and whether libvirt + has it active: one listing, two calls per interface.""" + ifaces = [] + for iface in self.wvm.listAllInterfaces(0): + info = interface_info(iface, ElementTree.fromstring(running_xml(iface))) + info["state"] = iface.isActive() + ifaces.append(info) + return sorted(ifaces, key=lambda iface: iface["name"]) + + def get_member_candidates(self, netdevs): + """The devices of netdevs a new bridge can take: not the loopback, a + bridge, bond or VLAN of the host nor a device in one as configured, at + any depth (a bridge's bond's devices), not the bridge of a libvirt + network and not a VM's tap (libvirt names them vnetN and macvtapN).""" + taken = {"lo"} + for iface in self.wvm.listAllInterfaces(0): + tree = ElementTree.fromstring(iface.XMLDesc(VIR_INTERFACE_XML_INACTIVE)) + if tree.get("type") in ("bridge", "bond", "vlan"): + taken.update(element.get("name") for element in tree.iter("interface")) + for net in self.wvm.listAllNetworks(0): + taken.add(util.get_xml_path(net.XMLDesc(0), "/network/bridge/@name")) + return [dev for dev in netdevs if dev not in taken and not dev.startswith(("vnet", "macvtap"))] + def define_iface(self, xml, flag=0): return self.wvm.interfaceDefineXML(xml, flag) @@ -69,131 +163,56 @@ class wvmInterfaces(wvmConnect): {self.bridge_member_xml(netdev)} """ xml += """""" - # the form refuses a name the host defines, so nothing is replaced - iface = self.define_iface(xml) - try: - iface.create() - except libvirtError: # the name stays free for another try - iface.undefine() - raise + # the form refuses a name the host defines, so nothing is replaced; + # one that does not start leaves the host as it was, its member's + # own configuration too + with changes(self.wvm): + self.define_iface(xml).create() class wvmInterface(wvmConnect): + """An interface of the host. A page reads its running definition (tree) + and its saved one (saved) once each.""" + def __init__(self, host, login, passwd, conn, iface): wvmConnect.__init__(self, host, login, passwd, conn) self.iface = self.get_iface(iface) - def _XMLDesc(self, flags=0): - return self.iface.XMLDesc(flags) + @cached_property + def tree(self): + return ElementTree.fromstring(running_xml(self.iface)) - def get_start_mode(self): - try: - xml = self._XMLDesc(VIR_INTERFACE_XML_INACTIVE) - return util.get_xml_path(xml, "/interface/start/@mode") - except Exception: - return None + @cached_property + def saved(self): + """The saved definition: its start mode and addressing.""" + return ElementTree.fromstring(self.iface.XMLDesc(VIR_INTERFACE_XML_INACTIVE)) def is_active(self): return self.iface.isActive() - def get_mac(self): - mac = self.iface.MACString() - return mac or None - def get_type(self): - xml = self._XMLDesc() - return util.get_xml_path(xml, "/interface/@type") + return self.tree.get("type") - def get_ipv4_type(self): - try: - xml = self._XMLDesc(VIR_INTERFACE_XML_INACTIVE) - ipaddr = util.get_xml_path( - xml, "/interface/protocol[@family='ipv4']/ip/@address" - ) - return "static" if ipaddr else "dhcp" - except Exception: - return None - - def get_ipv4(self): - xml = self._XMLDesc() - int_ipv4_ip = util.get_xml_path( - xml, "/interface/protocol[@family='ipv4']/ip/@address" - ) - int_ipv4_mask = util.get_xml_path( - xml, "/interface/protocol[@family='ipv4']/ip/@prefix" - ) - if not int_ipv4_ip or not int_ipv4_mask: - return None - else: - return f"{int_ipv4_ip}/{int_ipv4_mask}" - - def get_ipv6_type(self): - try: - xml = self._XMLDesc(VIR_INTERFACE_XML_INACTIVE) - ipaddr = util.get_xml_path( - xml, "/interface/protocol[@family='ipv6']/ip/@address" - ) - return "static" if ipaddr else "dhcp" - except Exception: - return None - - def get_ipv6(self): - xml = self._XMLDesc() - int_ipv6_ip = util.get_xml_path( - xml, "/interface/protocol[@family='ipv6']/ip/@address" - ) - int_ipv6_mask = util.get_xml_path( - xml, "/interface/protocol[@family='ipv6']/ip/@prefix" - ) - if not int_ipv6_ip or not int_ipv6_mask: - return None - else: - return f"{int_ipv6_ip}/{int_ipv6_mask}" - - def get_bridge(self): - bridge = None - if self.get_type() != "bridge": - return None - bridge = util.get_xml_path(self._XMLDesc(), "/interface/bridge/interface/@name") - for iface in self.get_bridge_slave_ifaces(): - if iface.get("state") == "up" and iface.get("speed") != "unknown": - bridge = iface.get("name") - return bridge - return bridge - - def get_bridge_slave_ifaces(self): - if self.get_type() != "bridge": - return None - - ifaces = [] - tree = ElementTree.fromstring(self._XMLDesc()) - for iface in tree.findall("./bridge/"): - address = state = speed = None - name = iface.get("name") - if_type = iface.get("type") - link = iface.find("link") - if link is not None: - state = link.get("state") - speed = link.get("speed") - mac = iface.find("mac") - if mac is not None: - address = mac.get("address") - ifaces.append( - { - "name": name, - "type": if_type, - "state": state, - "speed": speed, - "mac": address, - } - ) - return ifaces - - def get_details(self): - mac = self.get_mac() - itype = self.get_type() - state = self.is_active() - return {"name": self.iface.name(), "type": itype, "state": state, "mac": mac} + def get_info(self): + """interface_info, with its start mode and, per family, how it is + addressed (dhcp, autoconf, static or None) and its addresses.""" + info = interface_info(self.iface, self.tree) + info["start_mode"] = attribute(self.saved, "start", "mode") + for family in ("ipv4", "ipv6"): + config = self.saved.find(f"protocol[@family='{family}']") + if config is None: + method = None + elif config.find("dhcp") is not None: + method = "dhcp" + elif config.find("autoconf") is not None: + method = "autoconf" + else: + method = "static" + # the prefix is optional in libvirt's XML + addresses = [ip.get("address") + (f"/{ip.get('prefix')}" if ip.get("prefix") else "") + for ip in self.tree.findall(f"protocol[@family='{family}']/ip")] + info[family] = {"method": method, "addresses": addresses} + return info def stop_iface(self): self.iface.destroy() @@ -202,12 +221,21 @@ class wvmInterface(wvmConnect): self.iface.create() def delete_iface(self): - """Undefine the interface. netcf removes a bridge's members with it, so - each member is defined again on its own, as virsh iface-unbridge does: - with the bridge's start mode and, when it is the only one, addresses.""" - tree = ElementTree.fromstring(self._XMLDesc(VIR_INTERFACE_XML_INACTIVE)) + """Undefine the interface, stopped first. netcf removes a bridge's + members with it, so each member is defined again on its own, as virsh + iface-unbridge does: with the bridge's start mode and, when it is the + only one, addresses; a bridge with addresses and several members is + refused, which of them takes the addresses being the user's choice. + All of it is one change of the host's configuration.""" + if self.iface.isActive(): + raise util.OperationError(f"Stop interface {self.iface.name()} before deleting it") + tree = ElementTree.fromstring(self.iface.XMLDesc(VIR_INTERFACE_XML_INACTIVE)) members = tree.findall("bridge/interface") - self.iface.undefine() - for member in members: - member.extend(tree.findall("start") + (tree.findall("protocol") if len(members) == 1 else [])) - self.wvm.interfaceDefineXML(ElementTree.tostring(member, encoding="unicode"), 0) + if len(members) > 1 and tree.findall("protocol"): + raise util.OperationError(f"Bridge {self.iface.name()} has addresses and several members: " + "give the addresses to one of them on the host first") + with changes(self.wvm): + self.iface.undefine() + for member in members: + member.extend(tree.findall("start") + tree.findall("protocol")) + self.wvm.interfaceDefineXML(ElementTree.tostring(member, encoding="unicode"), 0) diff --git a/vrtManager/tests/test_host_reads.py b/vrtManager/tests/test_host_reads.py index 8efd1c7d..edb32ebe 100644 --- a/vrtManager/tests/test_host_reads.py +++ b/vrtManager/tests/test_host_reads.py @@ -21,6 +21,7 @@ from libvirt import ( VIR_ERR_NO_STORAGE_VOL, VIR_ERR_NO_SUPPORT, VIR_ERR_OPERATION_DENIED, + VIR_ERR_SYSTEM_ERROR, VIR_ERR_XML_ERROR, libvirtError, ) @@ -136,13 +137,16 @@ class ChangeInterfacesTestCase(unittest.TestCase): def test_a_backend_without_define_cannot_change_interfaces(self): conn = connection() - # any other refusal (access denied, ...) leaves the page as it was - for code, changeable in ((VIR_ERR_NO_SUPPORT, False), (VIR_ERR_XML_ERROR, True), (VIR_ERR_OPERATION_DENIED, True)): + for code, changeable in ((VIR_ERR_NO_SUPPORT, False), (VIR_ERR_OPERATION_DENIED, False), (VIR_ERR_XML_ERROR, True)): with self.subTest(code=code): conn.wvm.interfaceDefineXML = MagicMock(side_effect=self.error(code)) self.assertIs(conn.can_change_interfaces(), changeable) # an invalid definition: nothing can be defined by asking conn.wvm.interfaceDefineXML.assert_called_once_with("", 0) + # any other refusal (the connection, the login) is no answer + conn.wvm.interfaceDefineXML = MagicMock(side_effect=self.error(VIR_ERR_SYSTEM_ERROR)) + with self.assertRaises(libvirtError): + conn.can_change_interfaces() class CapacityTestCase(unittest.TestCase):