Merge lp:~mpontillo/maas/api-fix--link-up-with-subnet--bug-1659607--trunk into lp:~maas-committers/maas/trunk

[Mike Pontillo]

API cleanup for `interface link-subnet`.

 * Fix bug that disallowed changing a LINK_UP link to a LINK_UP link with a different subnet on the same VLAN (or no subnet).
 * Allow a subnet that seems invalid or a LINK_UP mode to be forcibly set via the API. (new force=True parameter)
 * Allow an interface to be explicitly disconnected. (Removes all IP addresses, and unsets its VLAN.)

See examples of new allowed usage here:

    https://paste.ubuntu.com/23870718/

[Andres Rodriguez (andreserl)]

lgtm!

review: Approve

[MAAS Lander (maas-lander)]

The attempt to merge lp:~mpontillo/maas/api-fix--link-up-with-subnet--bug-1659607--trunk into lp:maas failed. Below is the output from the failed tests.

Get:1 http://security.ubuntu.com/ubuntu xenial-security InRelease [102 kB]
Hit:2 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial InRelease
Get:3 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates InRelease [102 kB]
Hit:4 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-backports InRelease
Get:5 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates/universe Sources [121 kB]
Get:6 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates/main amd64 Packages [459 kB]
Get:7 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates/universe amd64 Packages [387 kB]
Get:8 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates/universe Translation-en [143 kB]
Get:9 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates/multiverse amd64 Packages [8,080 B]
Fetched 1,322 kB in 0s (2,707 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
    --no-install-recommends install apache2 archdetect-deb authbind avahi-utils bash bind9 bind9utils build-essential bzr bzr-builddeb chromium-browser chromium-chromedriver curl daemontools debhelper dh-apport dh-systemd distro-info dnsutils firefox freeipmi-tools git gjs ipython isc-dhcp-common isc-dhcp-server libjs-angularjs libjs-jquery libjs-jquery-hotkeys libjs-yui3-full libjs-yui3-min libnss-wrapper libpq-dev make nodejs-legacy npm postgresql pxelinux python3-all python3-apt python3-attr python3-bson python3-convoy python3-crochet python3-cssselect python3-curtin python3-dev python3-distro-info python3-django python3-django-nose python3-django-piston3 python3-dnspython python3-docutils python3-formencode python3-hivex python3-httplib2 python3-jinja2 python3-jsonschema python3-lxml python3-netaddr python3-netifaces python3-novaclient python3-oauth python3-oauthlib python3-openssl python3-paramiko python3-petname python3-pexpect python3-psycopg2 python3-pyinotify python3-pyparsing python3-pyvmomi python3-requests python3-seamicroclient python3-setuptools python3-simplestreams python3-sphinx python3-tempita python3-twisted python3-txtftp python3-tz python3-yaml python3-zope.interface python-bson python-crochet python-django python-django-piston python-djorm-ext-pgarray python-formencode python-lxml python-netaddr python-netifaces python-pocket-lint python-psycopg2 python-simplejson python-tempita python-twisted python-yaml socat syslinux-common tgt ubuntu-cloudimage-keyring wget xvfb
Reading package lists...
Building dependency tree...
Reading state information...
authbind is already the newest version (2.1.1+nmu1).
avahi-utils is already the newest version (0.6.32~rc+dfsg-1ubuntu2).
build-essential is already the newest version (12.1ubuntu2).
debhelper is already the newest version (9.20160115ubuntu3).
distro-info is already the newest version (0.14build1).
git is already the newest version (1:2.7.4-0ubuntu1).
libjs-angularjs is already the newest version (1.2.28-1ubuntu2).
libjs-jquery is already the newes...

=== modified file 'src/maasserver/api/interfaces.py'
--- src/maasserver/api/interfaces.py	2016-12-09 15:16:44 +0000
+++ src/maasserver/api/interfaces.py	2017-01-27 19:44:54 +0000
@@ -4,11 +4,15 @@
 """API handlers: `Interface`."""
 
 from django.forms.utils import ErrorList
+from formencode.validators import StringBool
 from maasserver.api.support import (
     operation,
     OperationsHandler,
 )
-from maasserver.api.utils import get_mandatory_param
+from maasserver.api.utils import (
+    get_mandatory_param,
+    get_optional_param,
+)
 from maasserver.enum import (
     INTERFACE_LINK_TYPE,
     INTERFACE_TYPE,
@@ -556,6 +560,11 @@
         :param ip_address: IP address for the interface in subnet. Only used
             when mode is STATIC. If not provided an IP address from subnet
             will be auto selected.
+        :param force: If True, allows LINK_UP to be set on the interface
+            even if other links already exist. Also allows the selection of any
+            VLAN, even a VLAN MAAS does not believe the interface to currently
+            be on. Using this option will cause all other links on the
+            interface to be deleted. (Defaults to False.)
         :param default_gateway: True sets the gateway IP address for the subnet
             as the default gateway for the node this interface belongs to.
             Option can only be used with the AUTO and STATIC modes.
@@ -578,6 +587,8 @@
 
         Returns 404 if the node or interface is not found.
         """
+        force = get_optional_param(
+            request.POST, 'force', default=False, validator=StringBool)
         interface = Interface.objects.get_interface_or_404(
             system_id, id, request.user, NODE_PERMISSION.EDIT)
         node = interface.get_node()
@@ -597,13 +608,37 @@
             # Devices can only be set in static IP mode.
             allowed_modes = [INTERFACE_LINK_TYPE.STATIC]
         form = InterfaceLinkForm(
-            instance=interface, data=request.data, allowed_modes=allowed_modes)
+            instance=interface, data=request.data, allowed_modes=allowed_modes,
+            force=force)
         if form.is_valid():
             return form.save()
         else:
             raise MAASAPIValidationError(form.errors)
 
     @operation(idempotent=False)
+    def disconnect(self, request, system_id, id):
+        """Disconnect an interface.
+
+        Deletes any linked subnets and IP addresses, and disconnects the
+        interface from any associated VLAN.
+
+        Returns 404 if the node or interface is not found.
+        """
+        interface = Interface.objects.get_interface_or_404(
+            system_id, id, request.user, NODE_PERMISSION.EDIT)
+        node = interface.get_node()
+        raise_error_if_controller(node, "disconnect")
+        if node.node_type == NODE_TYPE.MACHINE:
+            # This node needs to be in the correct state to modify
+            # the interface.
+            raise_error_for_invalid_state_on_allocated_operations(
+                node, request.user, "disconnect")
+        interface.ip_addresses.all().delete()
+        interface.vlan = None
+        interface.save()
+        return interface
+
+    @operation(idempotent=False)
     def unlink_subnet(self, request, system_id, id):
         """Unlink interface to a subnet.
 

=== modified file 'src/maasserver/api/tests/test_interfaces.py'
--- src/maasserver/api/tests/test_interfaces.py	2016-12-12 14:17:23 +0000
+++ src/maasserver/api/tests/test_interfaces.py	2017-01-27 19:44:54 +0000
@@ -31,6 +31,7 @@
     MatchesDict,
     MatchesListwise,
     MatchesSetwise,
+    Not,
 )
 
 
@@ -1093,6 +1094,186 @@
                 "mode": Equals(INTERFACE_LINK_TYPE.STATIC),
                 }))
 
+    def test_link_subnet_allows_subnet_with_link_up(self):
+        self.become_admin()
+        node = factory.make_Node(owner=self.user, status=NODE_STATUS.READY)
+        interface = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, node=node)
+        subnet = factory.make_Subnet(vlan=interface.vlan)
+        uri = get_interface_uri(interface)
+        response = self.client.post(uri, {
+            "op": "link_subnet",
+            "mode": INTERFACE_LINK_TYPE.LINK_UP,
+            "subnet": subnet.id,
+            })
+        self.assertEqual(
+            http.client.OK, response.status_code, response.content)
+        parsed_response = json_load_bytes(response.content)
+        self.assertThat(
+            parsed_response["links"][0], ContainsDict({
+                "mode": Equals(INTERFACE_LINK_TYPE.LINK_UP),
+                }))
+        self.assertThat(
+            parsed_response["links"][0]['subnet']['id'], Equals(subnet.id))
+
+    def test_link_subnet_allows_link_up_subnet_to_be_cleared(self):
+        self.become_admin()
+        node = factory.make_Node(owner=self.user, status=NODE_STATUS.READY)
+        interface = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, node=node)
+        subnet = factory.make_Subnet(vlan=interface.vlan)
+        uri = get_interface_uri(interface)
+        self.client.post(uri, {
+            "op": "link_subnet",
+            "mode": INTERFACE_LINK_TYPE.LINK_UP,
+            "subnet": subnet.id,
+            })
+        response = self.client.post(uri, {
+            "op": "link_subnet",
+            "mode": INTERFACE_LINK_TYPE.LINK_UP,
+        })
+        self.assertEqual(
+            http.client.OK, response.status_code, response.content)
+        parsed_response = json_load_bytes(response.content)
+        self.assertThat(
+            parsed_response["links"][0], ContainsDict({
+                "mode": Equals(INTERFACE_LINK_TYPE.LINK_UP),
+                }))
+        self.assertThat(parsed_response["links"][0], Not(Contains("subnet")))
+
+    def test_link_subnet_allows_link_up_subnet_to_be_changed(self):
+        self.become_admin()
+        node = factory.make_Node(owner=self.user, status=NODE_STATUS.READY)
+        interface = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, node=node)
+        subnet = factory.make_Subnet(vlan=interface.vlan)
+        subnet2 = factory.make_Subnet(vlan=interface.vlan)
+        uri = get_interface_uri(interface)
+        self.client.post(uri, {
+            "op": "link_subnet",
+            "mode": INTERFACE_LINK_TYPE.LINK_UP,
+            "subnet": subnet.id,
+            })
+        response = self.client.post(uri, {
+            "op": "link_subnet",
+            "mode": INTERFACE_LINK_TYPE.LINK_UP,
+            "subnet": subnet2.id,
+        })
+        self.assertEqual(
+            http.client.OK, response.status_code, response.content)
+        parsed_response = json_load_bytes(response.content)
+        self.assertThat(
+            parsed_response["links"][0], ContainsDict({
+                "mode": Equals(INTERFACE_LINK_TYPE.LINK_UP),
+                }))
+        self.assertThat(
+            parsed_response["links"][0]['subnet']['id'], Equals(subnet2.id))
+
+    def test_link_subnet_disallows_subnets_on_another_vlan(self):
+        self.become_admin()
+        node = factory.make_Node(owner=self.user, status=NODE_STATUS.READY)
+        interface = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, node=node)
+        vlan2 = factory.make_VLAN()
+        subnet = factory.make_Subnet(vlan=interface.vlan)
+        subnet2 = factory.make_Subnet(vlan=vlan2)
+        uri = get_interface_uri(interface)
+        self.client.post(uri, {
+            "op": "link_subnet",
+            "mode": INTERFACE_LINK_TYPE.LINK_UP,
+            "subnet": subnet.id,
+            })
+        response = self.client.post(uri, {
+            "op": "link_subnet",
+            "mode": INTERFACE_LINK_TYPE.LINK_UP,
+            "subnet": subnet2.id,
+        })
+        self.assertEqual(
+            http.client.BAD_REQUEST, response.status_code, response.content)
+
+    def test_link_subnet_allows_link_up_subnet_to_be_forcibly_changed(self):
+        self.become_admin()
+        node = factory.make_Node(owner=self.user, status=NODE_STATUS.READY)
+        interface = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, node=node)
+        vlan2 = factory.make_VLAN()
+        subnet = factory.make_Subnet(vlan=interface.vlan)
+        subnet2 = factory.make_Subnet(vlan=vlan2)
+        uri = get_interface_uri(interface)
+        self.client.post(uri, {
+            "op": "link_subnet",
+            "mode": INTERFACE_LINK_TYPE.LINK_UP,
+            "subnet": subnet.id,
+            })
+        response = self.client.post(uri, {
+            "op": "link_subnet",
+            "mode": INTERFACE_LINK_TYPE.LINK_UP,
+            "force": "True",
+            "subnet": subnet2.id,
+        })
+        self.assertEqual(
+            http.client.OK, response.status_code, response.content)
+        parsed_response = json_load_bytes(response.content)
+        self.assertThat(
+            parsed_response["links"][0], ContainsDict({
+                "mode": Equals(INTERFACE_LINK_TYPE.LINK_UP),
+                }))
+        self.assertThat(
+            parsed_response["links"][0]['subnet']['id'], Equals(subnet2.id))
+
+    def test_link_subnet_force_link_up_deletes_existing_links(self):
+        self.become_admin()
+        node = factory.make_Node(owner=self.user, status=NODE_STATUS.READY)
+        interface = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, node=node)
+        vlan2 = factory.make_VLAN()
+        subnet = factory.make_Subnet(vlan=interface.vlan)
+        subnet2 = factory.make_Subnet(vlan=vlan2)
+        uri = get_interface_uri(interface)
+        self.client.post(uri, {
+            "op": "link_subnet",
+            "mode": INTERFACE_LINK_TYPE.DHCP,
+            "subnet": subnet.id,
+            })
+        response = self.client.post(uri, {
+            "op": "link_subnet",
+            "mode": INTERFACE_LINK_TYPE.LINK_UP,
+            "force": "True",
+            "subnet": subnet2.id,
+        })
+        self.assertEqual(
+            http.client.OK, response.status_code, response.content)
+        parsed_response = json_load_bytes(response.content)
+        self.assertThat(
+            parsed_response["links"][0], ContainsDict({
+                "mode": Equals(INTERFACE_LINK_TYPE.LINK_UP),
+                }))
+        self.assertThat(
+            parsed_response["links"][0]['subnet']['id'], Equals(subnet2.id))
+
+    def test_link_subnet_without_force_link_up_returns_bad_request(self):
+        self.become_admin()
+        node = factory.make_Node(owner=self.user, status=NODE_STATUS.READY)
+        interface = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, node=node)
+        vlan2 = factory.make_VLAN()
+        subnet = factory.make_Subnet(vlan=interface.vlan)
+        subnet2 = factory.make_Subnet(vlan=vlan2)
+        uri = get_interface_uri(interface)
+        self.client.post(uri, {
+            "op": "link_subnet",
+            "mode": INTERFACE_LINK_TYPE.DHCP,
+            "subnet": subnet.id,
+            })
+        response = self.client.post(uri, {
+            "op": "link_subnet",
+            "mode": INTERFACE_LINK_TYPE.LINK_UP,
+            "force": "False",
+            "subnet": subnet2.id,
+        })
+        self.assertEqual(
+            http.client.BAD_REQUEST, response.status_code, response.content)
+
     def test_link_subnet_on_device_only_allows_static(self):
         parent = factory.make_Node()
         device = factory.make_Device(
@@ -1229,6 +1410,49 @@
             self.assertEqual(
                 http.client.CONFLICT, response.status_code, response.content)
 
+    def test_disconnect_deletes_links_and_clears_vlan(self):
+        # The form that is used is fully tested in test_forms_interface_link.
+        # This just tests that the form is saved and the updated interface
+        # is returned.
+        self.become_admin()
+        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_interface_uri(interface)
+            response = self.client.post(uri, {
+                "op": "disconnect",
+                })
+            self.assertEqual(
+                http.client.OK, response.status_code, response.content)
+            self.assertIsNone(reload_object(dhcp_ip))
+            self.assertIsNone(reload_object(interface).vlan)
+
+    def test_disconnect_requries_admin(self):
+        node = factory.make_Node(interface=True)
+        interface = node.get_boot_interface()
+        uri = get_interface_uri(interface)
+        response = self.client.post(uri, {
+            "op": "disconnect",
+        })
+        self.assertEqual(
+            http.client.FORBIDDEN, response.status_code, response.content)
+
+    def test_disconnect_409_when_not_ready_or_broken(self):
+        self.become_admin()
+        for status in STATUSES:
+            node = factory.make_Node(interface=True, status=status)
+            interface = node.get_boot_interface()
+            uri = get_interface_uri(interface)
+            response = self.client.post(uri, {
+                "op": "disconnect",
+            })
+            self.assertEqual(
+                http.client.CONFLICT, response.status_code, response.content)
+
     def test_set_default_gateway_sets_gateway_link_ipv4_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.

=== modified file 'src/maasserver/fields.py'
--- src/maasserver/fields.py	2016-10-12 15:26:17 +0000
+++ src/maasserver/fields.py	2017-01-27 19:44:54 +0000
@@ -27,7 +27,10 @@
 import re
 
 from django import forms
-from django.core.exceptions import ValidationError
+from django.core.exceptions import (
+    ObjectDoesNotExist,
+    ValidationError,
+)
 from django.core.validators import (
     RegexValidator,
     URLValidator,
@@ -662,7 +665,13 @@
                     if obj is not None:
                         return obj
                 else:
-                    return self.queryset.get(id=object_id)
+                    try:
+                        return self.queryset.get(id=object_id)
+                    except ObjectDoesNotExist:
+                        # Re-raising this as a ValidationError prevents the API
+                        # from returning an internal server error rather than
+                        # a bad request.
+                        raise ValidationError("None found with id=%s." % value)
             raise e
 
 

=== modified file 'src/maasserver/forms_interface_link.py'
--- src/maasserver/forms_interface_link.py	2016-12-07 12:46:14 +0000
+++ src/maasserver/forms_interface_link.py	2017-01-27 19:44:54 +0000
@@ -58,6 +58,7 @@
             INTERFACE_LINK_TYPE.STATIC,
             INTERFACE_LINK_TYPE.LINK_UP,
         ])
+        self.force = kwargs.pop("force", False)
         mode_choices = [
             (key, value)
             for key, value in INTERFACE_LINK_TYPE_CHOICES
@@ -74,7 +75,7 @@
                 'invalid_choice': compose_invalid_choice_text(
                     'mode', mode_choices),
             })
-        if self.instance.vlan is None:
+        if self.instance.vlan is None or self.force is True:
             self.fields['subnet'].queryset = Subnet.objects.all()
         else:
             self.fields['subnet'].queryset = (
@@ -142,12 +143,21 @@
 
     def _clean_mode_link_up(self):
         # Cannot set LINK_UP unless no other IP address are attached to
-        # this interface.
-        if self.instance.ip_addresses.count() > 0:
+        # this interface. But exclude STICKY addresses where the IP address is
+        # null, because the user could be trying to change the subnet for a
+        # LINK_UP address. And exclude DISCOVERED because what MAAS discovered
+        # doesn't matter with regard to the user's intention.
+        exclude_types = (
+            IPADDRESS_TYPE.STICKY, IPADDRESS_TYPE.DISCOVERED
+        )
+        has_active_links = self.instance.ip_addresses.exclude(
+            alloc_type__in=exclude_types, ip__isnull=True).count() > 0
+        if has_active_links and self.force is not True:
             set_form_error(
                 self, "mode",
                 "Cannot configure interface to link up (with no IP address) "
-                "while other links are already configured.")
+                "while other links are already configured. Specify force=True "
+                "to override this behavior and delete all links.")
 
     def _clean_default_gateway(self, cleaned_data):
         mode = cleaned_data.get("mode", None)
@@ -174,6 +184,21 @@
         subnet = self.cleaned_data.get("subnet", None)
         ip_address = self.cleaned_data.get("ip_address", None)
         default_gateway = self.cleaned_data.get("default_gateway", False)
+        # If force=True, allow the user to select any subnet (this will
+        # implicitly change the VLAN).
+        if mode == INTERFACE_LINK_TYPE.LINK_UP or self.force is True:
+            # We're either setting the LINK_UP to a new subnet, or we're
+            # forcing the issue.
+            self.instance.clear_all_links(clearing_config=True)
+        # If the user wants to force a particular subnet to be linked, clear
+        # out the VLAN so that link_subnet() will reset the interface's subnet
+        # to be correct.
+        should_clear_vlan = (
+            self.force is True and subnet is not None and
+            self.instance.vlan is not None
+        )
+        if should_clear_vlan:
+            self.instance.vlan = None
         if not ip_address:
             ip_address = None
         link_ip = self.instance.link_subnet(

=== modified file 'src/maasserver/tests/test_forms_interface_link.py'
--- src/maasserver/tests/test_forms_interface_link.py	2016-12-12 14:17:23 +0000
+++ src/maasserver/tests/test_forms_interface_link.py	2017-01-27 19:44:54 +0000
@@ -382,7 +382,8 @@
         self.assertEqual({
             "mode": [
                 "Cannot configure interface to link up (with no IP address) "
-                "while other links are already configured."]
+                "while other links are already configured. Specify force=True "
+                "to override this behavior and delete all links."]
             }, form.errors)
 
     def test__LINK_UP_creates_link_STICKY_with_subnet(self):