Merge lp:~blake-rouse/maas/force-untagged-api into lp:~maas-committers/maas/trunk

[Blake Rouse]

Prevent allowing tagged VLAN from being attached to a physical or bond interface. Prevent untagged VLAN from being attached to a VLAN interface. This causes the API to match what the UI does.

Not having the API not match the UI is wrong and invalid. Its either we support one way or the other way, we should not support 50% one way and 50% another way. That makes for a confusing and incomplete product.

[Mike Pontillo (mpontillo)]

Please see my comments here:

https://code.launchpad.net/~blake-rouse/maas/rack-nic-fabric-change/+merge/287852

Most of these validations are okay, but the ones that force the model to require physical interfaces to be on the default ("untagged") VLAN are deal-breakers for me.

I understand that we are making MAAS work out-of-the-box with switch fabrics where all ports always have the same default VLAN. But this change goes a step further and forces that to be true. The underlying model was designed to handle both scenarios, so forcing the issue in this way prevents a large percentage of customers from using MAAS, with no possible workaround. (I would argue that it is also a regression, since MAAS 1.8 supports this. MAAS 1.9 and 1.10 support this as well, though it is not the default behavior.)

review: Disapprove

[Blake Rouse (blake-rouse)]

My comments are documented in the bug. https://bugs.launchpad.net/maas/+bug/1552923

[Mike Pontillo (mpontillo)]

I still strongly disagree with this additional validation. However, I am changing my vote to "Approve" based on discussions with our management team; the preferred approach is to add this validation despite the noted risks.

review: Approve

=== modified file 'src/maasserver/api/tests/test_interfaces.py'
--- src/maasserver/api/tests/test_interfaces.py	2016-02-26 18:39:26 +0000
+++ src/maasserver/api/tests/test_interfaces.py	2016-03-03 21:30:31 +0000
@@ -48,14 +48,14 @@
 def make_complex_interface(node, name=None):
     """Makes interface with parents and children."""
     fabric = factory.make_Fabric()
-    vlan_5 = factory.make_VLAN(vid=5, fabric=fabric)
+    untagged = fabric.get_default_vlan()
     nic_0 = factory.make_Interface(
-        INTERFACE_TYPE.PHYSICAL, vlan=vlan_5, node=node)
+        INTERFACE_TYPE.PHYSICAL, vlan=untagged, node=node)
     nic_1 = factory.make_Interface(
-        INTERFACE_TYPE.PHYSICAL, vlan=vlan_5, node=node)
+        INTERFACE_TYPE.PHYSICAL, vlan=untagged, node=node)
     parents = [nic_0, nic_1]
     bond_interface = factory.make_Interface(
-        INTERFACE_TYPE.BOND, mac_address=nic_0.mac_address, vlan=vlan_5,
+        INTERFACE_TYPE.BOND, mac_address=nic_0.mac_address, vlan=untagged,
         parents=parents, name=name)
     vlan_10 = factory.make_VLAN(vid=10, fabric=fabric)
     vlan_nic_10 = factory.make_Interface(
@@ -112,7 +112,8 @@
             node = factory.make_Node(status=status)
             mac = factory.make_mac_address()
             name = factory.make_name("eth")
-            vlan = factory.make_VLAN()
+            fabric = factory.make_Fabric()
+            vlan = fabric.get_default_vlan()
             tags = [
                 factory.make_name("tag")
                 for _ in range(3)
@@ -145,7 +146,8 @@
             owner=self.logged_in_user, parent=parent)
         mac = factory.make_mac_address()
         name = factory.make_name("eth")
-        vlan = factory.make_VLAN()
+        fabric = factory.make_Fabric()
+        vlan = fabric.get_default_vlan()
         tags = [
             factory.make_name("tag")
             for _ in range(3)
@@ -178,7 +180,8 @@
             node = factory.make_Node(status=status)
             mac = factory.make_mac_address()
             name = factory.make_name("eth")
-            vlan = factory.make_VLAN()
+            fabric = factory.make_Fabric()
+            vlan = fabric.get_default_vlan()
             tags = [
                 factory.make_name("tag")
                 for _ in range(3)
@@ -274,7 +277,8 @@
         interface_on_other_node = factory.make_Interface(
             INTERFACE_TYPE.PHYSICAL)
         name = factory.make_name("eth")
-        vlan = factory.make_VLAN()
+        fabric = factory.make_Fabric()
+        vlan = fabric.get_default_vlan()
         uri = get_interfaces_uri(node)
         response = self.client.post(uri, {
             "op": "create_physical",
@@ -294,7 +298,8 @@
         self.become_admin()
         for status in (NODE_STATUS.READY, NODE_STATUS.BROKEN):
             node = factory.make_Node(status=status)
-            vlan = factory.make_VLAN()
+            fabric = factory.make_Fabric()
+            vlan = fabric.get_default_vlan()
             parent_1_iface = factory.make_Interface(
                 INTERFACE_TYPE.PHYSICAL, vlan=vlan, node=node)
             parent_2_iface = factory.make_Interface(
@@ -398,7 +403,7 @@
             self.assertEqual(
                 http.client.CONFLICT, response.status_code, response.content)
 
-    def test_create_bond_requires_name_vlan_and_parents(self):
+    def test_create_bond_requires_name_and_parents(self):
         self.become_admin()
         node = factory.make_Node(status=NODE_STATUS.READY)
         uri = get_interfaces_uri(node)
@@ -411,7 +416,6 @@
         self.assertEqual({
             "mac_address": ["This field cannot be blank."],
             "name": ["This field is required."],
-            "vlan": ["This field is required."],
             "parents": ["A Bond interface must have one or more parents."],
             }, json_load_bytes(response.content))
 
@@ -635,7 +639,8 @@
             interface = factory.make_Interface(
                 INTERFACE_TYPE.PHYSICAL, node=node)
             new_name = factory.make_name("name")
-            new_vlan = factory.make_VLAN()
+            new_fabric = factory.make_Fabric()
+            new_vlan = new_fabric.get_default_vlan()
             uri = get_interface_uri(interface)
             response = self.client.put(uri, {
                 "name": new_name,
@@ -654,7 +659,8 @@
         interface = factory.make_Interface(
             INTERFACE_TYPE.PHYSICAL, node=device)
         new_name = factory.make_name("name")
-        new_vlan = factory.make_VLAN()
+        new_fabric = factory.make_Fabric()
+        new_vlan = new_fabric.get_default_vlan()
         uri = get_interface_uri(interface)
         response = self.client.put(uri, {
             "name": new_name,
@@ -689,9 +695,11 @@
                 node)
             physical_interface = factory.make_Interface(
                 INTERFACE_TYPE.PHYSICAL, node=node)
+            new_vlan = factory.make_VLAN(fabric=physical_interface.vlan.fabric)
             uri = get_interface_uri(vlan_10)
             response = self.client.put(uri, {
                 "parent": physical_interface.id,
+                "vlan": new_vlan.id,
                 })
             self.assertEqual(
                 http.client.OK, response.status_code, response.content)
 
=== modified file 'src/maasserver/forms_interface.py'
--- src/maasserver/forms_interface.py	2015-12-01 18:12:59 +0000
+++ src/maasserver/forms_interface.py	2016-03-03 21:30:31 +0000
@@ -186,6 +186,13 @@
         if len(parents) > 0:
             raise ValidationError("A physical interface cannot have parents.")
 
+    def clean_vlan(self):
+        new_vlan = self.cleaned_data.get('vlan')
+        if new_vlan and new_vlan.fabric.get_default_vlan() != new_vlan:
+            raise ValidationError(
+                "A physical interface can only belong to an untagged VLAN.")
+        return new_vlan
+
     def clean(self):
         cleaned_data = super(PhysicalInterfaceForm, self).clean()
         new_name = cleaned_data.get('name')
@@ -223,11 +230,25 @@
                 "in a bond.")
         return parents
 
+    def clean_vlan(self):
+        new_vlan = self.cleaned_data.get('vlan')
+        if new_vlan and new_vlan.fabric.get_default_vlan() == new_vlan:
+            raise ValidationError(
+                "A VLAN interface can only belong to a tagged VLAN.")
+        return new_vlan
+
     def clean(self):
         cleaned_data = super(VLANInterfaceForm, self).clean()
         if self.fields_ok(['vlan', 'parents']):
             new_vlan = self.cleaned_data.get('vlan')
             if new_vlan:
+                # VLAN needs to be the in the same fabric as the parent.
+                parent = self.cleaned_data.get('parents')[0]
+                if parent.vlan.fabric_id != new_vlan.fabric_id:
+                    set_form_error(
+                        self, "vlan",
+                        "A VLAN interface can only belong to a tagged VLAN on "
+                        "the same fabric as its parent interface.")
                 name = build_vlan_interface_name(
                     self.cleaned_data.get('parents').first(), new_vlan)
                 self.clean_interface_name_uniqueness(name)
@@ -271,6 +292,15 @@
             'name',
         )
 
+    def __init__(self, *args, **kwargs):
+        super(BondInterfaceForm, self).__init__(*args, **kwargs)
+        # Allow VLAN to be blank when creating.
+        instance = kwargs.get("instance", None)
+        if instance is not None and instance.id is not None:
+            self.fields['vlan'].required = True
+        else:
+            self.fields['vlan'].required = False
+
     def clean_parents(self):
         parents = self.get_clean_parents()
         if parents is None:
@@ -280,9 +310,16 @@
                 "A Bond interface must have one or more parents.")
         return parents
 
+    def clean_vlan(self):
+        new_vlan = self.cleaned_data.get('vlan')
+        if new_vlan and new_vlan.fabric.get_default_vlan() != new_vlan:
+            raise ValidationError(
+                "A bond interface can only belong to an untagged VLAN.")
+        return new_vlan
+
     def clean(self):
         cleaned_data = super(BondInterfaceForm, self).clean()
-        if self.fields_ok(['parents']):
+        if self.fields_ok(['vlan', 'parents']):
             parents = self.cleaned_data.get('parents')
             # Set the mac_address if its missing and the interface is being
             # created.
@@ -323,6 +360,23 @@
                         self, 'parents',
                         "%s is already in-use by another interface." % (
                             ', '.join(sorted(parents_with_other_children))))
+
+                # When creating the bond set VLAN to the same as the parents
+                # and check that the parents all belong to the same VLAN.
+                if self.instance.id is None:
+                    vlan = self.cleaned_data.get('vlan')
+                    if vlan is None:
+                        vlan = parents[0].vlan
+                        self.cleaned_data['vlan'] = vlan
+                    parent_vlans = {
+                        parent.vlan
+                        for parent in parents
+                    }
+                    if parent_vlans != set([vlan]):
+                        set_form_error(
+                            self, 'parents',
+                            "All parents must belong to the same VLAN.")
+
         return cleaned_data
 
     def set_extra_parameters(self, interface, created):
@@ -346,17 +400,6 @@
             elif created:
                 interface.params[bond_field] = self.fields[bond_field].initial
 
-    def save(self, *args, **kwargs):
-        """Persist the interface into the database."""
-        created = self.instance.id is None
-        interface = super(BondInterfaceForm, self).save()
-        if created:
-            # Bond was created we remove all the links on the parent interfaces
-            # and ensure that the bond has atleast a LINK_UP.
-            for parent in interface.parents.all():
-                parent.clear_all_links(clearing_config=True)
-        return interface
-
 
 INTERFACE_FORM_MAPPING = {
     INTERFACE_TYPE.PHYSICAL: PhysicalInterfaceForm,
 
=== modified file 'src/maasserver/tests/test_forms_interface.py'
--- src/maasserver/tests/test_forms_interface.py	2015-12-01 18:12:59 +0000
+++ src/maasserver/tests/test_forms_interface.py	2016-03-03 21:30:31 +0000
@@ -25,6 +25,7 @@
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
 from maasserver.utils.forms import compose_invalid_choice_text
+from maasserver.utils.orm import reload_object
 from testtools import ExpectedException
 from testtools.matchers import MatchesStructure
 
@@ -58,7 +59,8 @@
         node = factory.make_Node()
         mac_address = factory.make_mac_address()
         interface_name = 'eth0'
-        vlan = factory.make_VLAN()
+        fabric = factory.make_Fabric()
+        vlan = fabric.get_default_vlan()
         tags = [
             factory.make_name("tag")
             for _ in range(3)
@@ -84,7 +86,8 @@
         node = factory.make_Node()
         mac_address = factory.make_mac_address()
         interface_name = 'eth0'
-        vlan = factory.make_VLAN()
+        fabric = factory.make_Fabric()
+        vlan = fabric.get_default_vlan()
         tags = [
             factory.make_name("tag")
             for _ in range(3)
@@ -104,7 +107,8 @@
 
     def test__requires_mac_address(self):
         interface_name = 'eth0'
-        vlan = factory.make_VLAN()
+        fabric = factory.make_Fabric()
+        vlan = fabric.get_default_vlan()
         form = PhysicalInterfaceForm(
             node=factory.make_Node(),
             data={
@@ -119,7 +123,9 @@
             form.errors['mac_address'][0])
 
     def test_rejects_interface_with_duplicate_name(self):
-        interface = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
+        fabric = factory.make_Fabric()
+        vlan = fabric.get_default_vlan()
+        interface = factory.make_Interface(INTERFACE_TYPE.PHYSICAL, vlan=vlan)
         mac_address = factory.make_mac_address()
         form = PhysicalInterfaceForm(
             node=interface.node,
@@ -135,9 +141,30 @@
             "already has an interface named '%s'." % interface.name,
             form.errors['name'][0])
 
+    def test_rejects_interface_on_tagged_vlan(self):
+        fabric = factory.make_Fabric()
+        interface = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, vlan=fabric.get_default_vlan())
+        vlan = factory.make_VLAN(fabric=fabric)
+        mac_address = factory.make_mac_address()
+        form = PhysicalInterfaceForm(
+            node=interface.node,
+            data={
+                'name': factory.make_name("eth"),
+                'mac_address': mac_address,
+                'vlan': vlan.id,
+            })
+        self.assertFalse(form.is_valid(), form.errors)
+        self.assertItemsEqual(
+            ['vlan'], form.errors.keys(), form.errors)
+        self.assertIn(
+            "A physical interface can only belong to an untagged VLAN.",
+            form.errors['vlan'][0])
+
     def test__rejects_parents(self):
         parent = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
-        vlan = factory.make_VLAN()
+        fabric = factory.make_Fabric()
+        vlan = fabric.get_default_vlan()
         form = PhysicalInterfaceForm(
             node=parent.node,
             data={
@@ -157,7 +184,8 @@
         interface = factory.make_Interface(
             INTERFACE_TYPE.PHYSICAL, name='eth0')
         new_name = 'eth1'
-        new_vlan = factory.make_VLAN(vid=33)
+        new_fabric = factory.make_Fabric()
+        new_vlan = new_fabric.get_default_vlan()
         form = PhysicalInterfaceForm(
             instance=interface,
             data={
@@ -178,7 +206,8 @@
         node = factory.make_Node()
         mac_address = factory.make_mac_address()
         interface_name = 'eth0'
-        vlan = factory.make_VLAN()
+        fabric = factory.make_Fabric()
+        vlan = fabric.get_default_vlan()
         tags = [
             factory.make_name("tag")
             for _ in range(3)
@@ -217,7 +246,8 @@
             "autoconf": autoconf,
         }
         new_name = 'eth1'
-        new_vlan = factory.make_VLAN(vid=33)
+        new_fabric = factory.make_Fabric()
+        new_vlan = new_fabric.get_default_vlan()
         form = PhysicalInterfaceForm(
             instance=interface,
             data={
@@ -290,7 +320,7 @@
 
     def test__creates_vlan_interface(self):
         parent = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
-        vlan = factory.make_VLAN(vid=10)
+        vlan = factory.make_VLAN(fabric=parent.vlan.fabric, vid=10)
         form = VLANInterfaceForm(
             node=parent.node,
             data={
@@ -308,7 +338,7 @@
 
     def test__create_ensures_link_up(self):
         parent = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
-        vlan = factory.make_VLAN(vid=10)
+        vlan = factory.make_VLAN(fabric=parent.vlan.fabric, vid=10)
         form = VLANInterfaceForm(
             node=parent.node,
             data={
@@ -321,13 +351,10 @@
             interface.ip_addresses.filter(alloc_type=IPADDRESS_TYPE.STICKY))
 
     def test_rejects_interface_with_duplicate_name(self):
-        vlan = factory.make_VLAN(vid=10)
-        parent = factory.make_Interface(
-            INTERFACE_TYPE.PHYSICAL,
-            vlan=vlan)
+        parent = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
+        vlan = factory.make_VLAN(fabric=parent.vlan.fabric, vid=10)
         interface = factory.make_Interface(
-            INTERFACE_TYPE.VLAN,
-            vlan=vlan, parents=[parent])
+            INTERFACE_TYPE.VLAN, vlan=vlan, parents=[parent])
         form = VLANInterfaceForm(
             node=parent.node,
             data={
@@ -341,6 +368,22 @@
             "already has an interface named '%s'." % interface.name,
             form.errors['name'][0])
 
+    def test_rejects_interface_on_default_fabric(self):
+        parent = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
+        vlan = parent.vlan.fabric.get_default_vlan()
+        form = VLANInterfaceForm(
+            node=parent.node,
+            data={
+                'vlan': vlan.id,
+                'parents': [parent.id],
+            })
+        self.assertFalse(form.is_valid(), form.errors)
+        self.assertItemsEqual(
+            ['vlan'], form.errors.keys(), form.errors)
+        self.assertIn(
+            "A VLAN interface can only belong to a tagged VLAN.",
+            form.errors['vlan'][0])
+
     def test__rejects_no_parents(self):
         vlan = factory.make_VLAN(vid=10)
         form = VLANInterfaceForm(
@@ -356,13 +399,14 @@
 
     def test__rejects_vlan_parent(self):
         parent = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
+        vlan = factory.make_VLAN(fabric=parent.vlan.fabric, vid=10)
         vlan_parent = factory.make_Interface(
-            INTERFACE_TYPE.VLAN, parents=[parent])
-        vlan = factory.make_VLAN(vid=10)
+            INTERFACE_TYPE.VLAN, vlan=vlan, parents=[parent])
+        other_vlan = factory.make_VLAN(fabric=parent.vlan.fabric, vid=11)
         form = VLANInterfaceForm(
             node=parent.node,
             data={
-                'vlan': vlan.id,
+                'vlan': other_vlan.id,
                 'parents': [vlan_parent.id],
             })
         self.assertFalse(form.is_valid(), form.errors)
@@ -371,10 +415,27 @@
             "VLAN interface can't have another VLAN interface as parent.",
             form.errors['parents'][0])
 
+    def test__rejects_vlan_not_on_same_fabric(self):
+        parent = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
+        factory.make_VLAN(fabric=parent.vlan.fabric, vid=10)
+        other_vlan = factory.make_VLAN()
+        form = VLANInterfaceForm(
+            node=parent.node,
+            data={
+                'vlan': other_vlan.id,
+                'parents': [parent.id],
+            })
+        self.assertFalse(form.is_valid(), form.errors)
+        self.assertItemsEqual(['vlan'], form.errors.keys())
+        self.assertIn(
+            "A VLAN interface can only belong to a tagged VLAN on "
+            "the same fabric as its parent interface.",
+            form.errors['vlan'][0])
+
     def test__rejects_parent_on_bond(self):
         parent = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
-        factory.make_Interface(INTERFACE_TYPE.BOND, parents=[parent])
-        vlan = factory.make_VLAN(vid=10)
+        bond = factory.make_Interface(INTERFACE_TYPE.BOND, parents=[parent])
+        vlan = factory.make_VLAN(fabric=bond.vlan.fabric, vid=10)
         form = VLANInterfaceForm(
             node=parent.node,
             data={
@@ -408,7 +469,7 @@
         parent = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
         interface = factory.make_Interface(
             INTERFACE_TYPE.VLAN, parents=[parent])
-        new_vlan = factory.make_VLAN(vid=33)
+        new_vlan = factory.make_VLAN(fabric=interface.vlan.fabric, vid=33)
         form = VLANInterfaceForm(
             instance=interface,
             data={
@@ -429,15 +490,13 @@
     def test__error_with_invalid_bond_mode(self):
         parent1 = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
         parent2 = factory.make_Interface(
-            INTERFACE_TYPE.PHYSICAL, node=parent1.node)
+            INTERFACE_TYPE.PHYSICAL, node=parent1.node, vlan=parent1.vlan)
         interface_name = factory.make_name()
-        vlan = factory.make_VLAN(vid=10)
         bond_mode = factory.make_name("bond_mode")
         form = BondInterfaceForm(
             node=parent1.node,
             data={
                 'name': interface_name,
-                'vlan': vlan.id,
                 'parents': [parent1.id, parent2.id],
                 'bond_mode': bond_mode,
             })
@@ -451,14 +510,12 @@
     def test__creates_bond_interface(self):
         parent1 = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
         parent2 = factory.make_Interface(
-            INTERFACE_TYPE.PHYSICAL, node=parent1.node)
+            INTERFACE_TYPE.PHYSICAL, node=parent1.node, vlan=parent1.vlan)
         interface_name = factory.make_name()
-        vlan = factory.make_VLAN(vid=10)
         form = BondInterfaceForm(
             node=parent1.node,
             data={
                 'name': interface_name,
-                'vlan': vlan.id,
                 'parents': [parent1.id, parent2.id],
             })
         self.assertTrue(form.is_valid(), form.errors)
@@ -475,15 +532,13 @@
         parent1 = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
         parent1.ensure_link_up()
         parent2 = factory.make_Interface(
-            INTERFACE_TYPE.PHYSICAL, node=parent1.node)
+            INTERFACE_TYPE.PHYSICAL, node=parent1.node, vlan=parent1.vlan)
         parent2.ensure_link_up()
         interface_name = factory.make_name()
-        vlan = factory.make_VLAN(vid=10)
         form = BondInterfaceForm(
             node=parent1.node,
             data={
                 'name': interface_name,
-                'vlan': vlan.id,
                 'parents': [parent1.id, parent2.id],
             })
         self.assertTrue(form.is_valid(), form.errors)
@@ -502,14 +557,12 @@
     def test__creates_bond_interface_with_parent_mac_address(self):
         parent1 = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
         parent2 = factory.make_Interface(
-            INTERFACE_TYPE.PHYSICAL, node=parent1.node)
+            INTERFACE_TYPE.PHYSICAL, node=parent1.node, vlan=parent1.vlan)
         interface_name = factory.make_name()
-        vlan = factory.make_VLAN(vid=10)
         form = BondInterfaceForm(
             node=parent1.node,
             data={
                 'name': interface_name,
-                'vlan': vlan.id,
                 'parents': [parent1.id, parent2.id],
                 'mac_address': parent1.mac_address,
             })
@@ -525,14 +578,12 @@
     def test__creates_bond_interface_with_default_bond_params(self):
         parent1 = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
         parent2 = factory.make_Interface(
-            INTERFACE_TYPE.PHYSICAL, node=parent1.node)
+            INTERFACE_TYPE.PHYSICAL, node=parent1.node, vlan=parent1.vlan)
         interface_name = factory.make_name()
-        vlan = factory.make_VLAN(vid=10)
         form = BondInterfaceForm(
             node=parent1.node,
             data={
                 'name': interface_name,
-                'vlan': vlan.id,
                 'parents': [parent1.id, parent2.id],
             })
         self.assertTrue(form.is_valid(), form.errors)
@@ -549,9 +600,8 @@
     def test__creates_bond_interface_with_bond_params(self):
         parent1 = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
         parent2 = factory.make_Interface(
-            INTERFACE_TYPE.PHYSICAL, node=parent1.node)
+            INTERFACE_TYPE.PHYSICAL, node=parent1.node, vlan=parent1.vlan)
         interface_name = factory.make_name()
-        vlan = factory.make_VLAN(vid=10)
         bond_mode = factory.pick_choice(BOND_MODE_CHOICES)
         bond_miimon = random.randint(0, 1000)
         bond_downdelay = random.randint(0, 1000)
@@ -563,7 +613,6 @@
             node=parent1.node,
             data={
                 'name': interface_name,
-                'vlan': vlan.id,
                 'parents': [parent1.id, parent2.id],
                 'bond_mode': bond_mode,
                 'bond_miimon': bond_miimon,
@@ -584,13 +633,11 @@
             }, interface.params)
 
     def test__rejects_no_parents(self):
-        vlan = factory.make_VLAN(vid=10)
         interface_name = factory.make_name()
         form = BondInterfaceForm(
             node=factory.make_Node(),
             data={
                 'name': interface_name,
-                'vlan': vlan.id,
             })
         self.assertFalse(form.is_valid(), form.errors)
         self.assertItemsEqual(['parents', 'mac_address'], form.errors.keys())
@@ -598,21 +645,37 @@
             "A Bond interface must have one or more parents.",
             form.errors['parents'][0])
 
+    def test__rejects_when_vlan_not_untagged(self):
+        interface_name = factory.make_name()
+        parent = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
+        vlan = factory.make_VLAN(fabric=parent.vlan.fabric)
+        form = BondInterfaceForm(
+            node=parent.node,
+            data={
+                'name': interface_name,
+                'parents': [parent.id],
+                'mac_address': parent.mac_address,
+                'vlan': vlan.id,
+            })
+        self.assertFalse(form.is_valid(), form.errors)
+        self.assertItemsEqual(['vlan'], form.errors.keys())
+        self.assertIn(
+            "A bond interface can only belong to an untagged VLAN.",
+            form.errors['vlan'][0])
+
     def test__rejects_when_parents_already_have_children(self):
         node = factory.make_Node()
         parent1 = factory.make_Interface(
             INTERFACE_TYPE.PHYSICAL, node=node, name="eth0")
         factory.make_Interface(INTERFACE_TYPE.VLAN, parents=[parent1])
         parent2 = factory.make_Interface(
-            INTERFACE_TYPE.PHYSICAL, node=node, name="eth1")
+            INTERFACE_TYPE.PHYSICAL, node=node, name="eth1", vlan=parent1.vlan)
         factory.make_Interface(INTERFACE_TYPE.VLAN, parents=[parent2])
-        vlan = factory.make_VLAN(vid=10)
         interface_name = factory.make_name()
         form = BondInterfaceForm(
             node=node,
             data={
                 'name': interface_name,
-                'vlan': vlan.id,
                 'parents': [parent1.id, parent2.id]
             })
         self.assertFalse(form.is_valid(), form.errors)
@@ -620,17 +683,36 @@
             "eth0, eth1 is already in-use by another interface.",
             form.errors['parents'][0])
 
+    def test__rejects_when_parents_not_in_same_vlan(self):
+        node = factory.make_Node()
+        parent1 = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, node=node, name="eth0")
+        parent2 = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, node=node, name="eth1")
+        interface_name = factory.make_name()
+        form = BondInterfaceForm(
+            node=node,
+            data={
+                'name': interface_name,
+                'parents': [parent1.id, parent2.id]
+            })
+        self.assertFalse(form.is_valid(), form.errors)
+        self.assertEquals(
+            "All parents must belong to the same VLAN.",
+            form.errors['parents'][0])
+
     def test__edits_interface(self):
         parent1 = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
         parent2 = factory.make_Interface(
-            INTERFACE_TYPE.PHYSICAL, node=parent1.node)
+            INTERFACE_TYPE.PHYSICAL, node=parent1.node, vlan=parent1.vlan)
         interface = factory.make_Interface(
             INTERFACE_TYPE.BOND,
             parents=[parent1, parent2])
-        new_vlan = factory.make_VLAN(vid=33)
+        new_fabric = factory.make_Fabric()
+        new_vlan = new_fabric.get_default_vlan()
         new_name = factory.make_name()
         new_parent = factory.make_Interface(
-            INTERFACE_TYPE.PHYSICAL, node=parent1.node)
+            INTERFACE_TYPE.PHYSICAL, node=parent1.node, vlan=parent1.vlan)
         form = BondInterfaceForm(
             instance=interface,
             data={
@@ -647,6 +729,10 @@
                 vlan=new_vlan, type=INTERFACE_TYPE.BOND))
         self.assertItemsEqual(
             [parent1, parent2, new_parent], interface.parents.all())
+        self.assertItemsEqual([new_vlan], set(
+            reload_object(parent).vlan
+            for parent in [parent1, parent2, new_parent]
+        ))
 
     def test__edits_interface_removes_parents(self):
         parent1 = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
@@ -704,7 +790,7 @@
     def test__edit_doesnt_overwrite_params(self):
         parent1 = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
         parent2 = factory.make_Interface(
-            INTERFACE_TYPE.PHYSICAL, node=parent1.node)
+            INTERFACE_TYPE.PHYSICAL, node=parent1.node, vlan=parent1.vlan)
         interface = factory.make_Interface(
             INTERFACE_TYPE.BOND,
             parents=[parent1, parent2])
@@ -724,12 +810,10 @@
             "bond_xmit_hash_policy": bond_xmit_hash_policy,
         }
         interface.save()
-        new_vlan = factory.make_VLAN(vid=33)
         new_name = factory.make_name()
         form = BondInterfaceForm(
             instance=interface,
             data={
-                'vlan': new_vlan.id,
                 'name': new_name,
             })
         self.assertTrue(form.is_valid(), form.errors)
@@ -746,7 +830,7 @@
     def test__edit_does_overwrite_params(self):
         parent1 = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
         parent2 = factory.make_Interface(
-            INTERFACE_TYPE.PHYSICAL, node=parent1.node)
+            INTERFACE_TYPE.PHYSICAL, node=parent1.node, vlan=parent1.vlan)
         interface = factory.make_Interface(
             INTERFACE_TYPE.BOND,
             parents=[parent1, parent2])
@@ -766,7 +850,6 @@
             "bond_xmit_hash_policy": bond_xmit_hash_policy,
         }
         interface.save()
-        new_vlan = factory.make_VLAN(vid=33)
         new_name = factory.make_name()
         new_bond_mode = factory.pick_choice(BOND_MODE_CHOICES)
         new_bond_miimon = random.randint(0, 1000)
@@ -778,7 +861,6 @@
         form = BondInterfaceForm(
             instance=interface,
             data={
-                'vlan': new_vlan.id,
                 'name': new_name,
                 'bond_mode': new_bond_mode,
                 'bond_miimon': new_bond_miimon,
@@ -801,7 +883,7 @@
     def test__edit_allows_zero_params(self):
         parent1 = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
         parent2 = factory.make_Interface(
-            INTERFACE_TYPE.PHYSICAL, node=parent1.node)
+            INTERFACE_TYPE.PHYSICAL, node=parent1.node, vlan=parent1.vlan)
         interface = factory.make_Interface(
             INTERFACE_TYPE.BOND,
             parents=[parent1, parent2])
@@ -821,7 +903,6 @@
             "bond_xmit_hash_policy": bond_xmit_hash_policy,
         }
         interface.save()
-        new_vlan = factory.make_VLAN(vid=33)
         new_name = factory.make_name()
         new_bond_mode = factory.pick_choice(BOND_MODE_CHOICES)
         new_bond_miimon = 0
@@ -833,7 +914,6 @@
         form = BondInterfaceForm(
             instance=interface,
             data={
-                'vlan': new_vlan.id,
                 'name': new_name,
                 'bond_mode': new_bond_mode,
                 'bond_miimon': new_bond_miimon,
 
=== modified file 'src/maasserver/websockets/handlers/tests/test_machine.py'
--- src/maasserver/websockets/handlers/tests/test_machine.py	2016-03-02 10:23:02 +0000
+++ src/maasserver/websockets/handlers/tests/test_machine.py	2016-03-03 21:30:31 +0000
@@ -1843,7 +1843,8 @@
         handler = MachineHandler(user, {})
         name = factory.make_name("eth")
         mac_address = factory.make_mac_address()
-        vlan = factory.make_VLAN()
+        fabric = factory.make_Fabric()
+        vlan = fabric.get_default_vlan()
         handler.create_physical({
             "system_id": node.system_id,
             "name": name,
@@ -1860,7 +1861,8 @@
         handler = MachineHandler(user, {})
         name = factory.make_name("eth")
         mac_address = factory.make_mac_address()
-        vlan = factory.make_VLAN()
+        fabric = factory.make_Fabric()
+        vlan = fabric.get_default_vlan()
         subnet = factory.make_Subnet(vlan=vlan)
         handler.create_physical({
             "system_id": node.system_id,
@@ -1882,7 +1884,8 @@
         handler = MachineHandler(user, {})
         name = factory.make_name("eth")
         mac_address = factory.make_mac_address()
-        vlan = factory.make_VLAN()
+        fabric = factory.make_Fabric()
+        vlan = fabric.get_default_vlan()
         handler.create_physical({
             "system_id": node.system_id,
             "name": name,
@@ -1902,7 +1905,8 @@
         handler = MachineHandler(user, {})
         name = factory.make_name("eth")
         mac_address = factory.make_mac_address()
-        vlan = factory.make_VLAN()
+        fabric = factory.make_Fabric()
+        vlan = fabric.get_default_vlan()
         subnet = factory.make_Subnet(vlan=vlan)
         handler.create_physical({
             "system_id": node.system_id,
@@ -1923,7 +1927,7 @@
         node = factory.make_Node()
         handler = MachineHandler(user, {})
         interface = factory.make_Interface(INTERFACE_TYPE.PHYSICAL, node=node)
-        new_vlan = factory.make_VLAN()
+        new_vlan = factory.make_VLAN(fabric=interface.vlan.fabric)
         handler.create_vlan({
             "system_id": node.system_id,
             "parent": interface.id,
@@ -1939,7 +1943,7 @@
         node = factory.make_Node()
         handler = MachineHandler(user, {})
         interface = factory.make_Interface(INTERFACE_TYPE.PHYSICAL, node=node)
-        new_vlan = factory.make_VLAN()
+        new_vlan = factory.make_VLAN(fabric=interface.vlan.fabric)
         new_subnet = factory.make_Subnet(vlan=new_vlan)
         handler.create_vlan({
             "system_id": node.system_id,
@@ -1961,7 +1965,7 @@
         node = factory.make_Node()
         handler = MachineHandler(user, {})
         interface = factory.make_Interface(INTERFACE_TYPE.PHYSICAL, node=node)
-        new_vlan = factory.make_VLAN()
+        new_vlan = factory.make_VLAN(fabric=interface.vlan.fabric)
         handler.create_vlan({
             "system_id": node.system_id,
             "parent": interface.id,
@@ -1981,7 +1985,7 @@
         node = factory.make_Node()
         handler = MachineHandler(user, {})
         interface = factory.make_Interface(INTERFACE_TYPE.PHYSICAL, node=node)
-        new_vlan = factory.make_VLAN()
+        new_vlan = factory.make_VLAN(fabric=interface.vlan.fabric)
         new_subnet = factory.make_Subnet(vlan=new_vlan)
         handler.create_vlan({
             "system_id": node.system_id,
@@ -2041,7 +2045,8 @@
         handler = MachineHandler(user, {})
         interface = factory.make_Interface(INTERFACE_TYPE.PHYSICAL, node=node)
         new_name = factory.make_name("name")
-        new_vlan = factory.make_VLAN()
+        new_fabric = factory.make_Fabric()
+        new_vlan = new_fabric.get_default_vlan()
         handler.update_interface({
             "system_id": node.system_id,
             "interface_id": interface.id,