fix(interfaces): a new interface never replaces one the host defines

- The create form refuses a name the host already defines. Defining it
  again replaced that interface's configuration, for example the host's
  own uplink when a bridge was named after it.
- An interface that does not start is undefined again, so the name is
  free for another try; the name check above makes this safe.

interfaces/forms.py

@@ -32,11 +32,13 @@ 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=(), **kwargs):
+    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.
+        # ethernet interface configures, must be one of them. The interfaces
+        # the host defines: defining one again would replace its configuration.
         super().__init__(*args, **kwargs)
         self.netdevs = set(netdevs)
+        self.defined = set(defined)
 
     def clean_ipv4_addr(self):
         ipv4_addr = self.cleaned_data["ipv4_addr"]
@@ -90,6 +92,8 @@ class AddInterface(forms.Form):
             raise forms.ValidationError(
                 _("The interface name may have up to 15 letters, digits and . _ -")
             )
+        if name in self.defined:
+            raise forms.ValidationError(_("The host already defines interface %(name)s") % {"name": name})
         return name
 
     def clean_netdev(self):

interfaces/tests/test_forms.py

@@ -28,6 +28,16 @@ def form(**values):
     return AddInterface(data={**BASE, **values}, netdevs=NETDEVS)
 
 
+class DefinedNameTestCase(SimpleTestCase):
+    def test_an_interface_the_host_defines_is_not_replaced(self):
+        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)
+                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())
+
+
 class Ipv6FormTestCase(SimpleTestCase):
     def test_valid_addresses_are_normalized(self):
         f = form(ipv6_addr="2001:DB8::10/64", ipv6_gw="2001:DB8::1")
@@ -196,11 +206,8 @@ class BridgeMemberXmlTestCase(SimpleTestCase):
     def member(self, host_xml):
         conn = wvmInterfaces.__new__(wvmInterfaces)
         conn.define_iface = MagicMock()
-        created = MagicMock()
 
         def get_iface(name):
-            if name == "br1":
-                return created
             if host_xml is None:
                 raise no_interface(name)
             iface = MagicMock()
@@ -209,7 +216,7 @@ class BridgeMemberXmlTestCase(SimpleTestCase):
 
         conn.get_iface = MagicMock(side_effect=get_iface)
         conn.create_iface("br1", "bridge", "onboot", "bond0", "none", "", "", "none", "", "", "on", 0)
-        self.assertTrue(created.create.called)
+        self.assertTrue(conn.define_iface.return_value.create.called)
         members = etree.fromstring(conn.define_iface.call_args[0][0]).findall("bridge/interface")
         self.assertEqual(len(members), 1)
         return members[0]
@@ -254,6 +261,15 @@ 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):
+        conn = wvmInterfaces.__new__(wvmInterfaces)
+        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()
+
 
 class BridgeSettingsTestCase(SimpleTestCase):
     def test_a_bridge_needs_stp_and_a_delay(self):

interfaces/views.py

@@ -55,7 +55,7 @@ def interfaces(request, compute_id):
             # 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)
+                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"):

vrtManager/interface.py

@@ -8,7 +8,7 @@ from vrtManager.connection import wvmConnect
 
 class wvmInterfaces(wvmConnect):
     def define_iface(self, xml, flag=0):
-        self.wvm.interfaceDefineXML(xml, flag)
+        return self.wvm.interfaceDefineXML(xml, flag)
 
     def bridge_member_xml(self, netdev):
         """netdev as a bridge member: as the host defines it (a bond keeps its
@@ -69,9 +69,13 @@ class wvmInterfaces(wvmConnect):
                         {self.bridge_member_xml(netdev)}
                       </bridge>"""
         xml += """</interface>"""
-        self.define_iface(xml)
-        iface = self.get_iface(name)
-        iface.create()
+        # 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
 
 
 class wvmInterface(wvmConnect):