Merge lp:~rbanffy/maas/1508059-node-if-config--on-ready-ui into lp:~maas-committers/maas/trunk

[Ricardo Bánffy]

Disables the UI for changing networking info when the node is on any state other than "Ready" or "Broken". Changes the API to also allow changes when the node is "Broken".

[LaMont Jones (lamont)]

Other htan the comment update, looks good.

review: Approve

[Andres Rodriguez (andreserl)]

So, I think here's what we need:

1. Users can't update network config when machine is 'Deploying'
2. Users can't update network config when machine is 'Commissioning'
3. Users can't update network config when machine is 'Deployed'.

Other than that, users can edit network config.

review: Needs Fixing

[Blake Rouse (blake-rouse)]

No it should only be when ready. Please read the attached bug for more information on why. I can explain in more detail later. Dealing with some other things at the moment.

I have not looked at this branch yet. But i will test it out soon.

[Blake Rouse (blake-rouse)]

Below is a breakdown on all the states an why only allowing Ready is the winner.

* NEW
   - Has not been commissioned so networking should not be changed.
* COMMISSIONING
   - Networking is going to be reset once finished.
* DEPLOYED
   - Already deployed changing anything would have no affect.
* BROKEN
   - Not even a usable node so manipulating the networking has no point. Must
     be placed it Ready state before it can be used.
* DEPLOYING
   - Already deploying, it is too late.
* ALLOCATED
   - Now this one is debatable, but I believe it needs to be consistent with
     storage. To make it consistent it should be disallowed.
        Reasoning: Storage can be modified in this state but only fstype and
                   mount_point. All other operations are disabled. This is
                   because once the node is released they will return to the
                   previous value before the acquire action. Networking should
                   disallow this state because if you make a change while it is
                   acquired they will be perminent, and will not reset upon
                   release. I think it would be confusing to the user to
                   provide two different outcomes from the allocated state.
* RELEASING
   - Well it is releasing so it will go to ready no action should be allowed.
* DISK_ERASING
   - Similar to releasing same reasoning applies.

* FAILED_COMMISSIONING
* FAILED_DEPLOYMENT
* FAILED_RELEASING
* FAILED_DISK_ERASING
   - All failed states require action from the user so they should be disabled
     as well.

* READY
   - Winner: Should be allowed!

[Blake Rouse (blake-rouse)]

Now I have actually looked at the code. I think it still needs some improvements and there are no unit tests for this change that you have already implemented.

Take a look at: http://paste.ubuntu.com/12898049/

I have made the changes to disallow editing even for non-admins and it shows a nice information message to an administrator when the node is not ready.

I would use that to improve this branch, but you also need to add unit tests for all the methods that I have added in the controller in that paste.

review: Needs Fixing

[Christian Reis (kiko)]

What about assigned/allocated?

[Blake Rouse (blake-rouse)]

Uhm... I described why...

[Ricardo Bánffy (rbanffy)]

Thanks Andres, Blake and Kiko for your comments. I refrained from adding any new UI elements to the initial implementation (which actually made things vanish from the interface, which is something I sometimes consider confusing and, therefore, I went with just disabling the affected elements) without having someone from design chiming in, but the message is coherent with the rest of the UI.

[Andres Rodriguez (andreserl)]

Hi Blake,

> * NEW
> - Has not been commissioned so networking should not be changed.
>

agree

> * COMMISSIONING
> - Networking is going to be reset once finished.
>

agree

* DEPLOYED
> - Already deployed changing anything would have no affect.
>
agree

* BROKEN
> - Not even a usable node so manipulating the networking has no point.
> Must
> be placed it Ready state before it can be used.
>

Disagree. The user should be able to change his network config. In fact, he
might have marked it broken to change things in MAAS, like adding more NICs
or god knows, so he should be able to make changes here. Once he marks the
machine fixed, it will be ready an usable.

> * DEPLOYING
> - Already deploying, it is too late.
>

Agreed.

> * ALLOCATED
> - Now this one is debatable, but I believe it needs to be consistent
> with
> storage. To make it consistent it should be disallowed.
> Reasoning: Storage can be modified in this state but only fstype
> and
> mount_point. All other operations are disabled. This is
> because once the node is released they will return to
> the
> previous value before the acquire action. Networking
> should
> disallow this state because if you make a change while
> it is
> acquired they will be perminent, and will not reset upon
> release. I think it would be confusing to the user to
> provide two different outcomes from the allocated state.
>

Disagree. Landscape guys allocate machines to them for them to do
operations in the machine. It is a way for them to reserve machines so
nobody else uses them. The networking is not like storage. The way
networking can change is more flexible than storage.

> * RELEASING
> - Well it is releasing so it will go to ready no action should be
> allowed.
>

Agree.

> * DISK_ERASING
> - Similar to releasing same reasoning applies.
>

Agree.

>
> * FAILED_COMMISSIONING
> * FAILED_DEPLOYMENT
> * FAILED_RELEASING
> * FAILED_DISK_ERASING
> - All failed states require action from the user so they should be
> disabled
> as well.
>

Agree.

>
> * READY
> - Winner: Should be allowed!
>

Agree.

So the states where we can change networking are: READY, BROKEN, ALLOCATED.

@Ricardo, please address that!

--
Andres Rodriguez (RoAkSoAx)
Ubuntu Server Developer
MSc. Telecom & Networking
Systems Engineer

[Ricardo Bánffy (rbanffy)]

BTW, this is what I get when I run a subset of the tests and push it to review before the tests finish running... It was a very educational experience.

[Blake Rouse (blake-rouse)]

> Hi Blake,
>
>
>
> > * NEW
> > - Has not been commissioned so networking should not be changed.
> >
>
> agree
>
> > * COMMISSIONING
> > - Networking is going to be reset once finished.
> >
>
> agree
>
> * DEPLOYED
> > - Already deployed changing anything would have no affect.
> >
> agree
>
> * BROKEN
> > - Not even a usable node so manipulating the networking has no point.
> > Must
> > be placed it Ready state before it can be used.
> >
>
> Disagree. The user should be able to change his network config. In fact, he
> might have marked it broken to change things in MAAS, like adding more NICs
> or god knows, so he should be able to make changes here. Once he marks the
> machine fixed, it will be ready an usable.

I think it is weird to allow them to change the networking configuration in this state, but don't really care if its allowed.

>
>
> > * DEPLOYING
> > - Already deploying, it is too late.
> >
>
> Agreed.
>
>
> > * ALLOCATED
> > - Now this one is debatable, but I believe it needs to be consistent
> > with
> > storage. To make it consistent it should be disallowed.
> > Reasoning: Storage can be modified in this state but only fstype
> > and
> > mount_point. All other operations are disabled. This is
> > because once the node is released they will return to
> > the
> > previous value before the acquire action. Networking
> > should
> > disallow this state because if you make a change while
> > it is
> > acquired they will be perminent, and will not reset upon
> > release. I think it would be confusing to the user to
> > provide two different outcomes from the allocated state.
> >
>
> Disagree. Landscape guys allocate machines to them for them to do
> operations in the machine. It is a way for them to reserve machines so
> nobody else uses them. The networking is not like storage. The way
> networking can change is more flexible than storage.

Yeah I said this was debatable. My vision was just to not confuse the user as storage configuration will be reset, unlike networking.

>
>
> > * RELEASING
> > - Well it is releasing so it will go to ready no action should be
> > allowed.
> >
>
> Agree.
>
>
> > * DISK_ERASING
> > - Similar to releasing same reasoning applies.
> >
>
> Agree.
>
> >
> > * FAILED_COMMISSIONING
> > * FAILED_DEPLOYMENT
> > * FAILED_RELEASING
> > * FAILED_DISK_ERASING
> > - All failed states require action from the user so they should be
> > disabled
> > as well.
> >
>
> Agree.
>
> >
> > * READY
> > - Winner: Should be allowed!
> >
>
> Agree.
>
> So the states where we can change networking are: READY, BROKEN, ALLOCATED.
>
> @Ricardo, please address that!

I am good with that.

>
>
>
> --
> Andres Rodriguez (RoAkSoAx)
> Ubuntu Server Developer
> MSc. Telecom & Networking
> Systems Engineer

[Ricardo Bánffy (rbanffy)]

One JS style question below

[Blake Rouse (blake-rouse)]

Looks good. Just got one issue with the jQuery in the JS. Please use my suggestion which is just plain JS.

review: Approve

=== modified file 'src/maasserver/api/interfaces.py'
--- src/maasserver/api/interfaces.py	2015-10-21 21:32:35 +0000
+++ src/maasserver/api/interfaces.py	2015-10-26 18:43:18 +0000
@@ -68,7 +68,7 @@
 
 def raise_error_for_invalid_state_on_allocated_operations(
         node, user, operation):
-    if node.status != NODE_STATUS.READY:
+    if node.status not in (NODE_STATUS.READY, NODE_STATUS.BROKEN):
         raise NodeStateViolation(
             "Cannot %s interface because the node is not Ready." % operation)
 

=== modified file 'src/maasserver/api/tests/test_interfaces.py'
--- src/maasserver/api/tests/test_interfaces.py	2015-10-21 21:32:35 +0000
+++ src/maasserver/api/tests/test_interfaces.py	2015-10-26 18:43:18 +0000
@@ -98,66 +98,70 @@
 
     def test_create_physical(self):
         self.become_admin()
-        node = factory.make_Node(status=NODE_STATUS.READY)
-        mac = factory.make_mac_address()
-        name = factory.make_name("eth")
-        vlan = factory.make_VLAN()
-        tags = [
-            factory.make_name("tag")
-            for _ in range(3)
-        ]
-        uri = get_node_interfaces_uri(node)
-        response = self.client.post(uri, {
-            "op": "create_physical",
-            "mac_address": mac,
-            "name": name,
-            "vlan": vlan.id,
-            "tags": ",".join(tags),
-            })
+        for status in (NODE_STATUS.READY, NODE_STATUS.BROKEN):
+            node = factory.make_Node(status=status)
+            mac = factory.make_mac_address()
+            name = factory.make_name("eth")
+            vlan = factory.make_VLAN()
+            tags = [
+                factory.make_name("tag")
+                for _ in range(3)
+            ]
+            uri = get_node_interfaces_uri(node)
+            response = self.client.post(uri, {
+                "op": "create_physical",
+                "mac_address": mac,
+                "name": name,
+                "vlan": vlan.id,
+                "tags": ",".join(tags),
+                })
 
-        self.assertEqual(httplib.OK, response.status_code, response.content)
-        self.assertThat(json.loads(response.content), ContainsDict({
-            "mac_address": Equals(mac),
-            "name": Equals(name),
-            "vlan": ContainsDict({
-                "id": Equals(vlan.id),
-                }),
-            "type": Equals("physical"),
-            "tags": Equals(tags),
-            "enabled": Equals(True),
-            }))
+            self.assertEqual(
+                httplib.OK, response.status_code, response.content)
+            self.assertThat(json.loads(response.content), ContainsDict({
+                "mac_address": Equals(mac),
+                "name": Equals(name),
+                "vlan": ContainsDict({
+                    "id": Equals(vlan.id),
+                    }),
+                "type": Equals("physical"),
+                "tags": Equals(tags),
+                "enabled": Equals(True),
+                }))
 
     def test_create_physical_disabled(self):
         self.become_admin()
-        node = factory.make_Node(status=NODE_STATUS.READY)
-        mac = factory.make_mac_address()
-        name = factory.make_name("eth")
-        vlan = factory.make_VLAN()
-        tags = [
-            factory.make_name("tag")
-            for _ in range(3)
-        ]
-        uri = get_node_interfaces_uri(node)
-        response = self.client.post(uri, {
-            "op": "create_physical",
-            "mac_address": mac,
-            "name": name,
-            "vlan": vlan.id,
-            "tags": ",".join(tags),
-            "enabled": False,
-            })
+        for status in (NODE_STATUS.READY, NODE_STATUS.BROKEN):
+            node = factory.make_Node(status=status)
+            mac = factory.make_mac_address()
+            name = factory.make_name("eth")
+            vlan = factory.make_VLAN()
+            tags = [
+                factory.make_name("tag")
+                for _ in range(3)
+            ]
+            uri = get_node_interfaces_uri(node)
+            response = self.client.post(uri, {
+                "op": "create_physical",
+                "mac_address": mac,
+                "name": name,
+                "vlan": vlan.id,
+                "tags": ",".join(tags),
+                "enabled": False,
+                })
 
-        self.assertEqual(httplib.OK, response.status_code, response.content)
-        self.assertThat(json.loads(response.content), ContainsDict({
-            "mac_address": Equals(mac),
-            "name": Equals(name),
-            "vlan": ContainsDict({
-                "id": Equals(vlan.id),
-                }),
-            "type": Equals("physical"),
-            "tags": Equals(tags),
-            "enabled": Equals(False),
-            }))
+            self.assertEqual(
+                httplib.OK, response.status_code, response.content)
+            self.assertThat(json.loads(response.content), ContainsDict({
+                "mac_address": Equals(mac),
+                "name": Equals(name),
+                "vlan": ContainsDict({
+                    "id": Equals(vlan.id),
+                    }),
+                "type": Equals("physical"),
+                "tags": Equals(tags),
+                "enabled": Equals(False),
+                }))
 
     def test_create_physical_requires_admin(self):
         node = factory.make_Node()
@@ -174,7 +178,7 @@
         self.assertEqual(
             httplib.FORBIDDEN, response.status_code, response.content)
 
-    def test_create_physical_409_when_not_ready(self):
+    def test_create_physical_409_when_not_ready_or_broken(self):
         self.become_admin()
         for status in (
                 NODE_STATUS.NEW,
@@ -186,7 +190,6 @@
                 NODE_STATUS.DEPLOYING,
                 NODE_STATUS.DEPLOYED,
                 NODE_STATUS.RETIRED,
-                NODE_STATUS.BROKEN,
                 NODE_STATUS.FAILED_DEPLOYMENT,
                 NODE_STATUS.RELEASING,
                 NODE_STATUS.FAILED_RELEASING,
@@ -246,42 +249,44 @@
 
     def test_create_bond(self):
         self.become_admin()
-        node = factory.make_Node(status=NODE_STATUS.READY)
-        vlan = factory.make_VLAN()
-        parent_1_iface = factory.make_Interface(
-            INTERFACE_TYPE.PHYSICAL, vlan=vlan, node=node)
-        parent_2_iface = factory.make_Interface(
-            INTERFACE_TYPE.PHYSICAL, vlan=vlan, node=node)
-        name = factory.make_name("bond")
-        tags = [
-            factory.make_name("tag")
-            for _ in range(3)
-        ]
-        uri = get_node_interfaces_uri(node)
-        response = self.client.post(uri, {
-            "op": "create_bond",
-            "mac_address": "%s" % parent_1_iface.mac_address,
-            "name": name,
-            "vlan": vlan.id,
-            "parents": [parent_1_iface.id, parent_2_iface.id],
-            "tags": ",".join(tags),
-            })
+        for status in (NODE_STATUS.READY, NODE_STATUS.BROKEN):
+            node = factory.make_Node(status=status)
+            vlan = factory.make_VLAN()
+            parent_1_iface = factory.make_Interface(
+                INTERFACE_TYPE.PHYSICAL, vlan=vlan, node=node)
+            parent_2_iface = factory.make_Interface(
+                INTERFACE_TYPE.PHYSICAL, vlan=vlan, node=node)
+            name = factory.make_name("bond")
+            tags = [
+                factory.make_name("tag")
+                for _ in range(3)
+            ]
+            uri = get_node_interfaces_uri(node)
+            response = self.client.post(uri, {
+                "op": "create_bond",
+                "mac_address": "%s" % parent_1_iface.mac_address,
+                "name": name,
+                "vlan": vlan.id,
+                "parents": [parent_1_iface.id, parent_2_iface.id],
+                "tags": ",".join(tags),
+                })
 
-        self.assertEqual(httplib.OK, response.status_code, response.content)
-        parsed_interface = json.loads(response.content)
-        self.assertThat(parsed_interface, ContainsDict({
-            "mac_address": Equals("%s" % parent_1_iface.mac_address),
-            "name": Equals(name),
-            "vlan": ContainsDict({
-                "id": Equals(vlan.id),
-                }),
-            "type": Equals("bond"),
-            "tags": Equals(tags),
-            }))
-        self.assertItemsEqual([
-            parent_1_iface.name,
-            parent_2_iface.name,
-            ], parsed_interface['parents'])
+            self.assertEqual(
+                httplib.OK, response.status_code, response.content)
+            parsed_interface = json.loads(response.content)
+            self.assertThat(parsed_interface, ContainsDict({
+                "mac_address": Equals("%s" % parent_1_iface.mac_address),
+                "name": Equals(name),
+                "vlan": ContainsDict({
+                    "id": Equals(vlan.id),
+                    }),
+                "type": Equals("bond"),
+                "tags": Equals(tags),
+                }))
+            self.assertItemsEqual([
+                parent_1_iface.name,
+                parent_2_iface.name,
+                ], parsed_interface['parents'])
 
     def test_create_bond_requires_admin(self):
         node = factory.make_Node()
@@ -302,7 +307,7 @@
         self.assertEqual(
             httplib.FORBIDDEN, response.status_code, response.content)
 
-    def test_create_bond_409_when_not_ready(self):
+    def test_create_bond_409_when_not_ready_or_broken(self):
         self.become_admin()
         for status in (
                 NODE_STATUS.NEW,
@@ -314,7 +319,6 @@
                 NODE_STATUS.DEPLOYING,
                 NODE_STATUS.DEPLOYED,
                 NODE_STATUS.RETIRED,
-                NODE_STATUS.BROKEN,
                 NODE_STATUS.FAILED_DEPLOYMENT,
                 NODE_STATUS.RELEASING,
                 NODE_STATUS.FAILED_RELEASING,
@@ -524,46 +528,55 @@
 
     def test_update_physical_interface(self):
         self.become_admin()
-        node = factory.make_Node(status=NODE_STATUS.READY)
-        interface = factory.make_Interface(INTERFACE_TYPE.PHYSICAL, node=node)
-        new_name = factory.make_name("name")
-        new_vlan = factory.make_VLAN()
-        uri = get_node_interface_uri(interface)
-        response = self.client.put(uri, {
-            "name": new_name,
-            "vlan": new_vlan.id,
-            })
-        self.assertEqual(httplib.OK, response.status_code, response.content)
-        parsed_interface = json.loads(response.content)
-        self.assertEquals(new_name, parsed_interface["name"])
-        self.assertEquals(new_vlan.id, parsed_interface["vlan"]["id"])
+        for status in (NODE_STATUS.READY, NODE_STATUS.BROKEN):
+            node = factory.make_Node(status=status)
+            interface = factory.make_Interface(
+                INTERFACE_TYPE.PHYSICAL, node=node)
+            new_name = factory.make_name("name")
+            new_vlan = factory.make_VLAN()
+            uri = get_node_interface_uri(interface)
+            response = self.client.put(uri, {
+                "name": new_name,
+                "vlan": new_vlan.id,
+                })
+            self.assertEqual(
+                httplib.OK, response.status_code, response.content)
+            parsed_interface = json.loads(response.content)
+            self.assertEquals(new_name, parsed_interface["name"])
+            self.assertEquals(new_vlan.id, parsed_interface["vlan"]["id"])
 
     def test_update_bond_interface(self):
         self.become_admin()
-        node = factory.make_Node(status=NODE_STATUS.READY)
-        bond, [nic_0, nic_1], [vlan_10, vlan_11] = make_complex_interface(node)
-        uri = get_node_interface_uri(bond)
-        response = self.client.put(uri, {
-            "parents": [nic_0.id],
-            })
-        self.assertEqual(httplib.OK, response.status_code, response.content)
-        parsed_interface = json.loads(response.content)
-        self.assertEquals([nic_0.name], parsed_interface["parents"])
+        for status in (NODE_STATUS.READY, NODE_STATUS.BROKEN):
+            node = factory.make_Node(status=status)
+            bond, [nic_0, nic_1], [vlan_10, vlan_11] = make_complex_interface(
+                node)
+            uri = get_node_interface_uri(bond)
+            response = self.client.put(uri, {
+                "parents": [nic_0.id],
+                })
+            self.assertEqual(
+                httplib.OK, response.status_code, response.content)
+            parsed_interface = json.loads(response.content)
+            self.assertEquals([nic_0.name], parsed_interface["parents"])
 
     def test_update_vlan_interface(self):
         self.become_admin()
-        node = factory.make_Node(status=NODE_STATUS.READY)
-        bond, [nic_0, nic_1], [vlan_10, vlan_11] = make_complex_interface(node)
-        physical_interface = factory.make_Interface(
-            INTERFACE_TYPE.PHYSICAL, node=node)
-        uri = get_node_interface_uri(vlan_10)
-        response = self.client.put(uri, {
-            "parent": physical_interface.id,
-            })
-        self.assertEqual(httplib.OK, response.status_code, response.content)
-        parsed_interface = json.loads(response.content)
-        self.assertEquals(
-            [physical_interface.name], parsed_interface["parents"])
+        for status in (NODE_STATUS.READY, NODE_STATUS.BROKEN):
+            node = factory.make_Node(status=status)
+            bond, [nic_0, nic_1], [vlan_10, vlan_11] = make_complex_interface(
+                node)
+            physical_interface = factory.make_Interface(
+                INTERFACE_TYPE.PHYSICAL, node=node)
+            uri = get_node_interface_uri(vlan_10)
+            response = self.client.put(uri, {
+                "parent": physical_interface.id,
+                })
+            self.assertEqual(
+                httplib.OK, response.status_code, response.content)
+            parsed_interface = json.loads(response.content)
+            self.assertEquals(
+                [physical_interface.name], parsed_interface["parents"])
 
     def test_update_requires_admin(self):
         node = factory.make_Node()
@@ -576,7 +589,7 @@
         self.assertEqual(
             httplib.FORBIDDEN, response.status_code, response.content)
 
-    def test_read_409_when_not_ready(self):
+    def test_read_409_when_not_ready_or_broken(self):
         self.become_admin()
         for status in (
                 NODE_STATUS.NEW,
@@ -588,7 +601,6 @@
                 NODE_STATUS.DEPLOYING,
                 NODE_STATUS.DEPLOYED,
                 NODE_STATUS.RETIRED,
-                NODE_STATUS.BROKEN,
                 NODE_STATUS.FAILED_DEPLOYMENT,
                 NODE_STATUS.RELEASING,
                 NODE_STATUS.FAILED_RELEASING,
@@ -608,13 +620,14 @@
 
     def test_delete_deletes_interface(self):
         self.become_admin()
-        node = factory.make_Node(interface=True, status=NODE_STATUS.READY)
-        interface = node.get_boot_interface()
-        uri = get_node_interface_uri(interface)
-        response = self.client.delete(uri)
-        self.assertEqual(
-            httplib.NO_CONTENT, response.status_code, response.content)
-        self.assertIsNone(reload_object(interface))
+        for status in (NODE_STATUS.READY, NODE_STATUS.BROKEN):
+            node = factory.make_Node(interface=True, status=status)
+            interface = node.get_boot_interface()
+            uri = get_node_interface_uri(interface)
+            response = self.client.delete(uri)
+            self.assertEqual(
+                httplib.NO_CONTENT, response.status_code, response.content)
+            self.assertIsNone(reload_object(interface))
 
     def test_delete_403_when_not_admin(self):
         node = factory.make_Node(interface=True)
@@ -634,7 +647,7 @@
         self.assertEqual(
             httplib.NOT_FOUND, response.status_code, response.content)
 
-    def test_delete_409_when_not_ready(self):
+    def test_delete_409_when_not_ready_or_broken(self):
         self.become_admin()
         for status in (
                 NODE_STATUS.NEW,
@@ -646,7 +659,6 @@
                 NODE_STATUS.DEPLOYING,
                 NODE_STATUS.DEPLOYED,
                 NODE_STATUS.RETIRED,
-                NODE_STATUS.BROKEN,
                 NODE_STATUS.FAILED_DEPLOYMENT,
                 NODE_STATUS.RELEASING,
                 NODE_STATUS.FAILED_RELEASING,
@@ -665,33 +677,36 @@
         # This just tests that the form is saved and the updated interface
         # is returned.
         self.become_admin()
-        node = factory.make_Node(interface=True, status=NODE_STATUS.READY)
-        interface = node.get_boot_interface()
-        uri = get_node_interface_uri(interface)
-        response = self.client.post(uri, {
-            "op": "link_subnet",
-            "mode": INTERFACE_LINK_TYPE.DHCP,
-            })
-        self.assertEqual(httplib.OK, response.status_code, response.content)
-        parsed_response = json.loads(response.content)
-        self.assertThat(
-            parsed_response["links"][0], ContainsDict({
-                "mode": Equals(INTERFACE_LINK_TYPE.DHCP),
-                }))
+        for status in (NODE_STATUS.READY, NODE_STATUS.BROKEN):
+            node = factory.make_Node(interface=True, status=status)
+            interface = node.get_boot_interface()
+            uri = get_node_interface_uri(interface)
+            response = self.client.post(uri, {
+                "op": "link_subnet",
+                "mode": INTERFACE_LINK_TYPE.DHCP,
+                })
+            self.assertEqual(
+                httplib.OK, response.status_code, response.content)
+            parsed_response = json.loads(response.content)
+            self.assertThat(
+                parsed_response["links"][0], ContainsDict({
+                    "mode": Equals(INTERFACE_LINK_TYPE.DHCP),
+                    }))
 
     def test_link_subnet_raises_error(self):
         self.become_admin()
-        node = factory.make_Node(interface=True, status=NODE_STATUS.READY)
-        interface = node.get_boot_interface()
-        uri = get_node_interface_uri(interface)
-        response = self.client.post(uri, {
-            "op": "link_subnet",
-            })
-        self.assertEqual(
-            httplib.BAD_REQUEST, response.status_code, response.content)
-        self.assertEquals({
-            "mode": ["This field is required."]
-            }, json.loads(response.content))
+        for status in (NODE_STATUS.READY, NODE_STATUS.BROKEN):
+            node = factory.make_Node(interface=True, status=status)
+            interface = node.get_boot_interface()
+            uri = get_node_interface_uri(interface)
+            response = self.client.post(uri, {
+                "op": "link_subnet",
+                })
+            self.assertEqual(
+                httplib.BAD_REQUEST, response.status_code, response.content)
+            self.assertEquals({
+                "mode": ["This field is required."]
+                }, json.loads(response.content))
 
     def test_link_subnet_requries_admin(self):
         node = factory.make_Node(interface=True)
@@ -703,7 +718,7 @@
         self.assertEqual(
             httplib.FORBIDDEN, response.status_code, response.content)
 
-    def test_link_subnet_409_when_not_ready(self):
+    def test_link_subnet_409_when_not_ready_or_broken(self):
         self.become_admin()
         for status in (
                 NODE_STATUS.NEW,
@@ -715,7 +730,6 @@
                 NODE_STATUS.DEPLOYING,
                 NODE_STATUS.DEPLOYED,
                 NODE_STATUS.RETIRED,
-                NODE_STATUS.BROKEN,
                 NODE_STATUS.FAILED_DEPLOYMENT,
                 NODE_STATUS.RELEASING,
                 NODE_STATUS.FAILED_RELEASING,
@@ -737,33 +751,36 @@
         # This just tests that the form is saved and the updated interface
         # is returned.
         self.become_admin()
-        node = factory.make_Node(interface=True, status=NODE_STATUS.READY)
-        interface = node.get_boot_interface()
-        subnet = factory.make_Subnet()
-        dhcp_ip = factory.make_StaticIPAddress(
-            alloc_type=IPADDRESS_TYPE.DHCP, ip="",
-            subnet=subnet, interface=interface)
-        uri = get_node_interface_uri(interface)
-        response = self.client.post(uri, {
-            "op": "unlink_subnet",
-            "id": dhcp_ip.id,
-            })
-        self.assertEqual(httplib.OK, response.status_code, response.content)
-        self.assertIsNone(reload_object(dhcp_ip))
+        for status in (NODE_STATUS.READY, NODE_STATUS.BROKEN):
+            node = factory.make_Node(interface=True, status=status)
+            interface = node.get_boot_interface()
+            subnet = factory.make_Subnet()
+            dhcp_ip = factory.make_StaticIPAddress(
+                alloc_type=IPADDRESS_TYPE.DHCP, ip="",
+                subnet=subnet, interface=interface)
+            uri = get_node_interface_uri(interface)
+            response = self.client.post(uri, {
+                "op": "unlink_subnet",
+                "id": dhcp_ip.id,
+                })
+            self.assertEqual(
+                httplib.OK, response.status_code, response.content)
+            self.assertIsNone(reload_object(dhcp_ip))
 
     def test_unlink_subnet_raises_error(self):
         self.become_admin()
-        node = factory.make_Node(interface=True, status=NODE_STATUS.READY)
-        interface = node.get_boot_interface()
-        uri = get_node_interface_uri(interface)
-        response = self.client.post(uri, {
-            "op": "unlink_subnet",
-            })
-        self.assertEqual(
-            httplib.BAD_REQUEST, response.status_code, response.content)
-        self.assertEquals({
-            "id": ["This field is required."]
-            }, json.loads(response.content))
+        for status in (NODE_STATUS.READY, NODE_STATUS.BROKEN):
+            node = factory.make_Node(interface=True, status=status)
+            interface = node.get_boot_interface()
+            uri = get_node_interface_uri(interface)
+            response = self.client.post(uri, {
+                "op": "unlink_subnet",
+                })
+            self.assertEqual(
+                httplib.BAD_REQUEST, response.status_code, response.content)
+            self.assertEquals({
+                "id": ["This field is required."]
+                }, json.loads(response.content))
 
     def test_unlink_subnet_requries_admin(self):
         node = factory.make_Node(interface=True)
@@ -775,7 +792,7 @@
         self.assertEqual(
             httplib.FORBIDDEN, response.status_code, response.content)
 
-    def test_unlink_subnet_409_when_not_ready(self):
+    def test_unlink_subnet_409_when_not_ready_or_broken(self):
         self.become_admin()
         for status in (
                 NODE_STATUS.NEW,
@@ -787,7 +804,6 @@
                 NODE_STATUS.DEPLOYING,
                 NODE_STATUS.DEPLOYED,
                 NODE_STATUS.RETIRED,
-                NODE_STATUS.BROKEN,
                 NODE_STATUS.FAILED_DEPLOYMENT,
                 NODE_STATUS.RELEASING,
                 NODE_STATUS.FAILED_RELEASING,
@@ -807,55 +823,60 @@
         # The form that is used is fully tested in test_forms_interface_link.
         # This just tests that the form is saved and the node link is created.
         self.become_admin()
-        node = factory.make_Node(interface=True, status=NODE_STATUS.READY)
-        interface = node.get_boot_interface()
-        network = factory.make_ipv4_network()
-        subnet = factory.make_Subnet(
-            cidr=unicode(network.cidr), vlan=interface.vlan)
-        link_ip = factory.make_StaticIPAddress(
-            alloc_type=IPADDRESS_TYPE.AUTO, ip="",
-            subnet=subnet, interface=interface)
-        uri = get_node_interface_uri(interface)
-        response = self.client.post(uri, {
-            "op": "set_default_gateway",
-            "link_id": link_ip.id
-            })
-        self.assertEqual(httplib.OK, response.status_code, response.content)
-        self.assertEqual(link_ip, reload_object(node).gateway_link_ipv4)
+        for status in (NODE_STATUS.READY, NODE_STATUS.BROKEN):
+            node = factory.make_Node(interface=True, status=status)
+            interface = node.get_boot_interface()
+            network = factory.make_ipv4_network()
+            subnet = factory.make_Subnet(
+                cidr=unicode(network.cidr), vlan=interface.vlan)
+            link_ip = factory.make_StaticIPAddress(
+                alloc_type=IPADDRESS_TYPE.AUTO, ip="",
+                subnet=subnet, interface=interface)
+            uri = get_node_interface_uri(interface)
+            response = self.client.post(uri, {
+                "op": "set_default_gateway",
+                "link_id": link_ip.id
+                })
+            self.assertEqual(
+                httplib.OK, response.status_code, response.content)
+            self.assertEqual(link_ip, reload_object(node).gateway_link_ipv4)
 
     def test_set_default_gateway_sets_gateway_link_ipv6_on_node(self):
         # The form that is used is fully tested in test_forms_interface_link.
         # This just tests that the form is saved and the node link is created.
         self.become_admin()
-        node = factory.make_Node(interface=True, status=NODE_STATUS.READY)
-        interface = node.get_boot_interface()
-        network = factory.make_ipv6_network()
-        subnet = factory.make_Subnet(
-            cidr=unicode(network.cidr), vlan=interface.vlan)
-        link_ip = factory.make_StaticIPAddress(
-            alloc_type=IPADDRESS_TYPE.AUTO, ip="",
-            subnet=subnet, interface=interface)
-        uri = get_node_interface_uri(interface)
-        response = self.client.post(uri, {
-            "op": "set_default_gateway",
-            "link_id": link_ip.id
-            })
-        self.assertEqual(httplib.OK, response.status_code, response.content)
-        self.assertEqual(link_ip, reload_object(node).gateway_link_ipv6)
+        for status in (NODE_STATUS.READY, NODE_STATUS.BROKEN):
+            node = factory.make_Node(interface=True, status=status)
+            interface = node.get_boot_interface()
+            network = factory.make_ipv6_network()
+            subnet = factory.make_Subnet(
+                cidr=unicode(network.cidr), vlan=interface.vlan)
+            link_ip = factory.make_StaticIPAddress(
+                alloc_type=IPADDRESS_TYPE.AUTO, ip="",
+                subnet=subnet, interface=interface)
+            uri = get_node_interface_uri(interface)
+            response = self.client.post(uri, {
+                "op": "set_default_gateway",
+                "link_id": link_ip.id
+                })
+            self.assertEqual(
+                httplib.OK, response.status_code, response.content)
+            self.assertEqual(link_ip, reload_object(node).gateway_link_ipv6)
 
     def test_set_default_gateway_raises_error(self):
         self.become_admin()
-        node = factory.make_Node(interface=True, status=NODE_STATUS.READY)
-        interface = node.get_boot_interface()
-        uri = get_node_interface_uri(interface)
-        response = self.client.post(uri, {
-            "op": "set_default_gateway",
-            })
-        self.assertEqual(
-            httplib.BAD_REQUEST, response.status_code, response.content)
-        self.assertEquals({
-            "__all__": ["This interface has no usable gateways."]
-            }, json.loads(response.content))
+        for status in (NODE_STATUS.READY, NODE_STATUS.BROKEN):
+            node = factory.make_Node(interface=True, status=status)
+            interface = node.get_boot_interface()
+            uri = get_node_interface_uri(interface)
+            response = self.client.post(uri, {
+                "op": "set_default_gateway",
+                })
+            self.assertEqual(
+                httplib.BAD_REQUEST, response.status_code, response.content)
+            self.assertEquals({
+                "__all__": ["This interface has no usable gateways."]
+                }, json.loads(response.content))
 
     def test_set_default_gateway_requries_admin(self):
         node = factory.make_Node(interface=True)
@@ -867,7 +888,7 @@
         self.assertEqual(
             httplib.FORBIDDEN, response.status_code, response.content)
 
-    def test_set_default_gateway_409_when_not_ready(self):
+    def test_set_default_gateway_409_when_not_ready_or_broken(self):
         self.become_admin()
         for status in (
                 NODE_STATUS.NEW,
@@ -879,7 +900,6 @@
                 NODE_STATUS.DEPLOYING,
                 NODE_STATUS.DEPLOYED,
                 NODE_STATUS.RETIRED,
-                NODE_STATUS.BROKEN,
                 NODE_STATUS.FAILED_DEPLOYMENT,
                 NODE_STATUS.RELEASING,
                 NODE_STATUS.FAILED_RELEASING,

=== modified file 'src/maasserver/static/js/angular/controllers/node_details_networking.js'
--- src/maasserver/static/js/angular/controllers/node_details_networking.js	2015-10-15 19:36:22 +0000
+++ src/maasserver/static/js/angular/controllers/node_details_networking.js	2015-10-26 18:43:18 +0000
@@ -83,11 +83,11 @@
 
 angular.module('MAAS').controller('NodeNetworkingController', [
     '$scope', '$filter', 'FabricsManager', 'VLANsManager', 'SubnetsManager',
-    'NodesManager', 'GeneralManager', 'ManagerHelperService',
+    'NodesManager', 'GeneralManager', 'UsersManager', 'ManagerHelperService',
     'ValidationService',
     function(
         $scope, $filter, FabricsManager, VLANsManager, SubnetsManager,
-        NodesManager, GeneralManager, ManagerHelperService,
+        NodesManager, GeneralManager, UsersManager, ManagerHelperService,
         ValidationService) {
 
         // Different interface types.
@@ -122,7 +122,7 @@
         var SELECTION_MODE = {
             NONE: null,
             SINGLE: "single",
-            MUTLI: "multi",
+            MULTI: "multi",
             DELETE: "delete",
             ADD: "add",
             CREATE_BOND: "create-bond"
@@ -408,6 +408,33 @@
             updateLoaded();
         };
 
+        // Return true if user is a super user/
+        $scope.isSuperUser = function() {
+            var authUser = UsersManager.getAuthUser();
+            if(!angular.isObject(authUser)) {
+                return false;
+            }
+            return authUser.is_superuser;
+        };
+
+        // Return true if the networking information cannot be edited.
+        // (it can't be changed when the node is in any state other
+        // than Ready or Broken and the user is not a superuser)
+        $scope.isAllNetworkingDisabled = function() {
+            if (!$scope.isSuperUser()) {
+                // If the user is not a superuser, disable the networking panel.
+                return true;
+            } else if (angular.isObject($scope.node) &&
+                ["Ready", "Broken"].indexOf($scope.node.status) === -1) {
+                // If the node is not ready or broken, disable networking panel.
+                return true;
+            } else {
+                // User must be a superuser and the node must be
+                // either ready or broken. Enable it.
+                return false;
+            }
+        };
+
         // Get the text for the type of the interface.
         $scope.getInterfaceTypeText = function(nic) {
             var text = INTERFACE_TYPE_TEXTS[nic.type];
@@ -682,7 +709,7 @@
 
             if($scope.selectedInterfaces.length > 1) {
                 if($scope.selectedMode !== SELECTION_MODE.BOND) {
-                    $scope.selectedMode = SELECTION_MODE.MUTLI;
+                    $scope.selectedMode = SELECTION_MODE.MULTI;
                 }
             } else if($scope.selectedInterfaces.length === 1) {
                 $scope.selectedMode = SELECTION_MODE.SINGLE;
@@ -786,7 +813,7 @@
             $scope.newInterface = {};
             $scope.newBondInterface = {};
             if($scope.selectedMode === SELECTION_MODE.CREATE_BOND) {
-                $scope.selectedMode = SELECTION_MODE.MUTLI;
+                $scope.selectedMode = SELECTION_MODE.MULTI;
             } else {
                 $scope.selectedMode = SELECTION_MODE.SINGLE;
             }
@@ -935,20 +962,25 @@
             }
         };
 
-        // Return true if this interface should be disabled in the list. Only
+        // Return true if the networking information cannot be edited
+        // or if this interface should be disabled in the list. Only
         // returns true when in create bond mode.
         $scope.isDisabled = function() {
-            return (
-                $scope.selectedMode !== SELECTION_MODE.NONE &&
-                $scope.selectedMode !== SELECTION_MODE.SINGLE &&
-                $scope.selectedMode !== SELECTION_MODE.MUTLI);
+            if ($scope.isAllNetworkingDisabled()) {
+                return true;
+            } else {
+                return (
+                    $scope.selectedMode !== SELECTION_MODE.NONE &&
+                    $scope.selectedMode !== SELECTION_MODE.SINGLE &&
+                    $scope.selectedMode !== SELECTION_MODE.MULTI);
+            }
         };
 
         // Return true when a bond can be created based on the current
         // selection. Only can be done if no aliases are selected and all
         // selected interfaces are on the same VLAN.
         $scope.canCreateBond = function() {
-            if($scope.selectedMode !== SELECTION_MODE.MUTLI) {
+            if($scope.selectedMode !== SELECTION_MODE.MULTI) {
                 return false;
             }
             var interfaces = getSelectedInterfaces();
@@ -974,7 +1006,7 @@
 
         // Show the create bond view.
         $scope.showCreateBond = function() {
-            if($scope.selectedMode === SELECTION_MODE.MUTLI &&
+            if($scope.selectedMode === SELECTION_MODE.MULTI &&
                 $scope.canCreateBond()) {
                 $scope.selectedMode = SELECTION_MODE.CREATE_BOND;
 
@@ -1069,7 +1101,8 @@
         ManagerHelperService.loadManagers([
             FabricsManager,
             VLANsManager,
-            SubnetsManager
+            SubnetsManager,
+            UsersManager
         ]).then(function() {
             $scope.managersHaveLoaded = true;
             updateLoaded();

=== modified file 'src/maasserver/static/js/angular/controllers/tests/test_node_details_networking.js'
--- src/maasserver/static/js/angular/controllers/tests/test_node_details_networking.js	2015-10-23 01:40:32 +0000
+++ src/maasserver/static/js/angular/controllers/tests/test_node_details_networking.js	2015-10-26 18:43:18 +0000
@@ -182,18 +182,18 @@
     }));
 
     // Load the required dependencies for the NodeNetworkingController.
-    var FabricsManager, VLANsManager, SubnetsManager, NodesManager;
-    var GeneralManager, ManagerHelperService;
+    var FabricsManager, VLANsManager, SubnetsManager, UsersManager;
+    var NodesManager, GeneralManager, ManagerHelperService;
     beforeEach(inject(function($injector) {
         FabricsManager = $injector.get("FabricsManager");
         VLANsManager = $injector.get("VLANsManager");
         SubnetsManager = $injector.get("SubnetsManager");
         NodesManager = $injector.get("NodesManager");
         GeneralManager = $injector.get("GeneralManager");
+        UsersManager = $injector.get("UsersManager");
         ManagerHelperService = $injector.get("ManagerHelperService");
     }));
 
-    // Create the node on the parent.
     var node;
     beforeEach(function() {
         node = {
@@ -2626,6 +2626,9 @@
 
         it("returns false when in none, single, or multi mode", function() {
             var controller = makeController();
+            $scope.isSuperUser = function() { return true; };
+            // Node needs to be Ready or Broken for the mode to be considered.
+            $scope.node = {status: "Ready"};
             $scope.selectedMode = null;
             expect($scope.isDisabled()).toBe(false);
             $scope.selectedMode = "single";
@@ -2636,6 +2639,9 @@
 
         it("returns true when in delete, add, or create modes", function() {
             var controller = makeController();
+            $scope.isSuperUser = function() { return true; };
+            // Node needs to be Ready or Broken for the mode to be considered.
+            $scope.node = {status: "Ready"};
             $scope.selectedMode = "create-bond";
             expect($scope.isDisabled()).toBe(true);
             $scope.selectedMode = "add";
@@ -2643,6 +2649,96 @@
             $scope.selectedMode = "delete";
             expect($scope.isDisabled()).toBe(true);
         });
+
+        it("returns true when the node state is not 'Ready' or 'Broken'",
+            function() {
+            var controller = makeController();
+            $scope.isSuperUser = function() { return true; };
+            $scope.node = {status: "Ready"};
+            expect($scope.isDisabled()).toBe(false);
+            $scope.node = {status: "Broken"};
+            expect($scope.isDisabled()).toBe(false);
+            ["New",
+             "Commissioning",
+             "Failed commissioning",
+             "Missing",
+             "Reserved",
+             "Allocated",
+             "Deploying",
+             "Deployed",
+             "Retired",
+             "Failed deployment",
+             "Releasing",
+             "Releasing failed",
+             "Disk erasing",
+             "Failed disk erasing"].forEach(function (s) {
+                 $scope.node = {state: s};
+                 expect($scope.isDisabled()).toBe(true);
+             });
+        });
+
+        it("returns true if the user is not a superuser", function() {
+            var controller = makeController();
+            $scope.isSuperUser = function() { return false; };
+            $scope.node = {status: "Ready"};
+            expect($scope.isDisabled()).toBe(true);
+            $scope.node = {status: "Broken"};
+            expect($scope.isDisabled()).toBe(true);
+        });
+    });
+
+    describe("isSuperUser", function() {
+        it("returns true if the user is a superuser", function() {
+            var controller = makeController();
+            spyOn(UsersManager, "getAuthUser").and.returnValue(
+                { is_superuser: true });
+            expect($scope.isSuperUser()).toBe(true);
+        });
+
+        it("returns false if the user is not a superuser", function() {
+            var controller = makeController();
+            spyOn(UsersManager, "getAuthUser").and.returnValue(
+                { is_superuser: false });
+            expect($scope.isSuperUser()).toBe(false);
+        });
+    });
+
+    describe("isAllNetworkingDisabled", function() {
+        it("returns true if the user is not a superuser and the node is ready",
+            function() {
+            var controller = makeController();
+            spyOn(UsersManager, "getAuthUser").and.returnValue(
+                { is_superuser: false });
+            expect($scope.isSuperUser()).toBe(false);
+            expect($scope.isAllNetworkingDisabled()).toBe(true);
+        });
+
+        it("return false if the node is Ready and we are a superuser",
+            function() {
+            var controller = makeController();
+            spyOn(UsersManager, "getAuthUser").and.returnValue(
+                { is_superuser: true });
+            $scope.node.status = "Ready";
+            expect($scope.isAllNetworkingDisabled()).toBe(false);
+        });
+
+        it("return false if the node is broken and we are a superuser",
+            function() {
+            var controller = makeController();
+            spyOn(UsersManager, "getAuthUser").and.returnValue(
+                { is_superuser: true });
+            $scope.node.status = "Broken";
+            expect($scope.isAllNetworkingDisabled()).toBe(false);
+        });
+
+        it("return true if the node is deploying and we are a superuser",
+            function() {
+            var controller = makeController();
+            spyOn(UsersManager, "getAuthUser").and.returnValue(
+                { is_superuser: true });
+            $scope.node.status = "Deploying";
+            expect($scope.isAllNetworkingDisabled()).toBe(true);
+        });
     });
 
     describe("canCreateBond", function() {

=== modified file 'src/maasserver/static/partials/node-details.html'
--- src/maasserver/static/partials/node-details.html	2015-10-26 09:48:55 +0000
+++ src/maasserver/static/partials/node-details.html	2015-10-26 18:43:18 +0000
@@ -316,6 +316,13 @@
             <form class="ng-hide" data-ng-show="loaded">
                 <div class="twelve-col">
                     <h2 class="title">Network</h2>
+                    <div class="twelve-col error ng-hide" data-ng-show="isAllNetworkingDisabled() && isSuperUser()">
+                        <ul class="flash-messages">
+                            <li class="flash-messages__item info">
+                                Network configuration cannot be modified unless the node is Ready or Broken.
+                            </li>
+                        </ul>
+                    </div>
                     <div class="table margin-bottom">
                         <header class="table__head">
                             <div class="table__row">
@@ -344,11 +351,13 @@
                                 data-ng-class="{ active: isInterfaceSelected(interface), disabled: isDisabled(), selected: !isOnlyInterfaceSelected(interface) }"
                                 data-ng-repeat="interface in interfaces | removeBondParents:newBondInterface">
                                 <div class="table__data table__column--3">
-                                    <input class="checkbox" type="checkbox" id="{$ getUniqueKey(interface) $}"
-                                        data-ng-checked="isInterfaceSelected(interface)"
-                                        data-ng-click="toggleInterfaceSelect(interface)"
-                                        data-ng-disabled="isDisabled()" />
-                                    <label class="checkbox-label" for="{$ getUniqueKey(interface) $}"></label>
+                                    <span data-ng-hide="isAllNetworkingDisabled()">
+                                        <input class="checkbox" type="checkbox" id="{$ getUniqueKey(interface) $}"
+                                            data-ng-checked="isInterfaceSelected(interface)"
+                                            data-ng-click="toggleInterfaceSelect(interface)"
+                                            data-ng-disabled="isDisabled()">
+                                        <label class="checkbox-label" for="{$ getUniqueKey(interface) $}"></label>
+                                    </span>
                                 </div>
                                 <div class="table__data table__column--15" data-ng-show="column == 'name'">
                                     <span class="ng-hide" data-ng-show="interface.type == 'alias' || interface.type == 'vlan'">{$ interface.name $}</span>
@@ -595,7 +604,7 @@
                     <a class="link-cta-ubuntu secondary margin-right"
                         data-ng-class="{ disabled: !canCreateBond() }"
                         data-ng-click="showCreateBond()"
-                        data-ng-hide="isShowingCreateBond()">Create Bond</a>
+                        data-ng-hide="isAllNetworkingDisabled() || isShowingCreateBond()">Create Bond</a>
                 </div>
             </form>
         </div>