Merge lp:~julian-edwards/maas/api-reserve-user-ip into lp:~maas-committers/maas/trunk

[Julian Edwards]

Add API to enable the reservation of USER_RESERVED static IP addresses.

I decided to make this a collection on a top-level API endpoint, so that in the future we can add more features to enable people to query more information about in-use static IPs. We also probably need an admin-only API function to get a view on all the IPs in use in the system (and web UI) but that can wait until later.

So now you can do:

POST /ipaddresses?op=reserve&net=<foonet/bits>
POST /ipaddresses?op=release&ip=<fooip>
GET /ipaddresses (to show all your own addresses in use)

[Raphaël Badin (rvb)]

Looks good.

An alternate design would have been to stick the new endpoint on the nodegroupinterface but I guess adding a new top level endpoint (/ipaddresses/) is a bit simpler.

As I said on IRC, I'd advice refactoring the reserve() API call to use a form. A form is just the natural abstraction when doing any kind of validation that is longer than two lines. It has several advantages:
- the code will be easier to test (the form it only doing the validation so testing the different error cases will be much simpler)
- the tests which only exercise the form will be much faster (because they don't have to deal with the HTTP layer)
- the form can be reused in the UI

Also, I've got a couple of minor remarks, see the inline comments.

review: Approve

[Julian Edwards (julian-edwards)]

Thanks for the review!

On Tuesday 01 Jul 2014 11:58:12 you wrote:
> Review: Approve
>
> Looks good.
>
> An alternate design would have been to stick the new endpoint on the
> nodegroupinterface but I guess adding a new top level endpoint
> (/ipaddresses/) is a bit simpler.

I explicitly didn't want to do that for several reasons:
 1. addressing the NGI is problematic
 2. it would obviate the desired mechanism of selecting a network - the CIDR
 3. it is not future proof for querying other types of addresses (as I said in
the cover note!)

>
> As I said on IRC, I'd advice refactoring the reserve() API call to use a
> form. A form is just the natural abstraction when doing any kind of
> validation that is longer than two lines. It has several advantages: - the
> code will be easier to test (the form it only doing the validation so
> testing the different error cases will be much simpler) - the tests which
> only exercise the form will be much faster (because they don't have to deal
> with the HTTP layer) - the form can be reused in the UI

I must admit I thought I wouldn't need a form when I started this, and then
after it was done I had some regrets, and also figured you might say that :)

> You'll get a KeyError if it's not there; that's not what I call proper
> testing :).

I think it's fine, the test will stop with an error and it'll be obvious why.

> > + del returned_address['created']
> > + expected = dict(
>
> You could use testtools.matchers.MatchesStructure here.

Ah yes I had forgotten about that, thanks for the reminder!

J

[Raphaël Badin (rvb)]

On 07/01/2014 02:15 PM, Julian Edwards wrote:
> Thanks for the review!
>
> On Tuesday 01 Jul 2014 11:58:12 you wrote:
>> Review: Approve
>>
>> Looks good.
>>
>> An alternate design would have been to stick the new endpoint on the
>> nodegroupinterface but I guess adding a new top level endpoint
>> (/ipaddresses/) is a bit simpler.
>
> I explicitly didn't want to do that for several reasons:
> 1. addressing the NGI is problematic
> 2. it would obviate the desired mechanism of selecting a network - the CIDR
> 3. it is not future proof for querying other types of addresses (as I said in
> the cover note!)

Yeah, I agree with your conclusion here.

[Julian Edwards (julian-edwards)]

On 01/07/14 21:58, Raphaël Badin wrote:
> As I said on IRC, I'd advice refactoring the reserve() API call to use a form. A form is just the natural abstraction when doing any kind of validation that is longer than two lines. It has several advantages:

I'd like to land this now and refactor later because I need some advice
for setting up this form (it's not a Django thing I am all that familiar
with). Also I'd like to get doing some QA on this and start a release
ready for next week!

[MAAS Lander (maas-lander)]

The attempt to merge lp:~julian-edwards/maas/api-reserve-user-ip into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com trusty-security InRelease
Hit http://security.ubuntu.com trusty-security Release.gpg
Hit http://security.ubuntu.com trusty-security Release
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Hit http://nova.clouds.archive.ubuntu.com trusty Release.gpg
Get:1 http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg [933 B]
Hit http://nova.clouds.archive.ubuntu.com trusty Release
Get:2 http://nova.clouds.archive.ubuntu.com trusty-updates Release [58.5 kB]
Hit http://security.ubuntu.com trusty-security/main Sources
Hit http://security.ubuntu.com trusty-security/universe Sources
Hit http://security.ubuntu.com trusty-security/main amd64 Packages
Hit http://security.ubuntu.com trusty-security/universe amd64 Packages
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages
Ign http://security.ubuntu.com trusty-security/main Translation-en_US
Ign http://security.ubuntu.com trusty-security/universe Translation-en_US
Hit http://nova.clouds.archive.ubuntu.com trusty/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Get:3 http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources [78.4 kB]
Get:4 http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources [55.6 kB]
Get:5 http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages [210 kB]
Get:6 http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages [148 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en_US
Fetched 552 kB in 0s (1,421 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 bind9 bind9utils build-essential bzr-builddeb curl daemontools debhelper dh-apport distro-info dnsutils firefox freeipmi-tools ipython isc-dhcp-common libjs-raphael libjs-yui3-full libjs-yui3-min libpq-dev make postgresql python-amqplib python-bzrlib python-celery python-convoy python-crochet python-cssselect python-curtin python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-formencode python-hivex python-httplib2 python-jinja2 python-jsonschema python-lockfile python-lxml python-netaddr...

[Julian Edwards (julian-edwards)]

It looks like getRandomNetwork() has a bug. I can't disable this test as that factory method is used everywhere. Retrying, and filing a bug.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2014-06-27 02:26:51 +0000
+++ src/maasserver/api.py	2014-07-01 06:06:42 +0000
@@ -225,6 +225,7 @@
     NodeGroupInterface,
     SSHKey,
     SSLKey,
+    StaticIPAddress,
     Tag,
     Zone,
     )
@@ -3658,3 +3659,78 @@
     @classmethod
     def resource_uri(cls, *args, **kwargs):
         return ('networks_handler', [])
+
+
+class IPAddressesHandler(OperationsHandler):
+    """Manage IP addresses allocated by MAAS."""
+    api_doc_section_name = "IP Addresses"
+
+    model = StaticIPAddress
+    fields = ('alloc_type', 'created', 'ip')
+    create = update = delete = None
+
+    @classmethod
+    def resource_uri(cls, *args, **kwargs):
+        return ('ipaddresses_handler', [])
+
+    def claim_ip(self, user, interface):
+        """Attempt to get a USER_RESERVED StaticIPAddress for `user` on
+        `interface`.
+
+        :raises StaticIPAddressExhaustion: If no IPs available.
+        """
+        return StaticIPAddress.objects.allocate_new(
+            range_low=interface.static_ip_range_low,
+            range_high=interface.static_ip_range_high,
+            alloc_type=IPADDRESS_TYPE.USER_RESERVED)
+
+    @operation(idempotent=False)
+    def reserve(self, request):
+        """Reserve an IP address for use outside of MAAS.
+
+        Returns an IP adddress for which MAAS will not allow any of its
+        known devices and Nodes to use; it is free for use by the requesting
+        user until released by the user.
+
+        :param network: CIDR representation of the network on which the IP
+            reservation is required. e.g. 10.1.2.0/24
+        :type network: unicode
+        """
+        network = get_mandatory_param(request.POST, "network")
+        # Validate the passed network.
+        try:
+            valid_network = netaddr.IPNetwork(network)
+        except netaddr.core.AddrFormatError:
+            raise MAASAPIBadRequest("Invalid network parameter %s" % network)
+
+        # Match the network to a nodegroupinterface.
+        interfaces = (
+            NodeGroupInterface.objects.filter(
+                nodegroup__status=NODEGROUP_STATUS.ACCEPTED)
+            .exclude(static_ip_range_low__isnull=True)
+            .exclude(static_ip_range_high__isnull=True)
+        )
+        for interface in interfaces:
+            if valid_network == interface.network:
+                # Winner winner chicken dinner.
+                return self.claim_ip(request.user, interface)
+        raise MAASAPIBadRequest("No network found matching %s" % network)
+
+    @operation(idempotent=False)
+    def release(self, request):
+        """Release an IP address that was previously reserved by the user.
+
+        :param ip: The IP address to release.
+        :type ip: unicode
+        """
+        ip = get_mandatory_param(request.POST, "ip")
+        staticaddress = get_object_or_404(
+            StaticIPAddress, ip=ip, user=request.user)
+        staticaddress.deallocate()
+
+    def read(self, request):
+        """List IPAddresses.
+
+        Get a listing of all IPAddresses allocated to the requesting user.
+        """
+        return StaticIPAddress.objects.filter(user=request.user).order_by('id')

=== modified file 'src/maasserver/testing/factory.py'
--- src/maasserver/testing/factory.py	2014-06-27 08:31:11 +0000
+++ src/maasserver/testing/factory.py	2014-07-01 06:06:42 +0000
@@ -436,7 +436,7 @@
         return node
 
     def make_staticipaddress(self, ip=None, alloc_type=IPADDRESS_TYPE.AUTO,
-                             mac=None):
+                             mac=None, user=None):
         """Create and return a StaticIPAddress model object.
 
         If a non-None `mac` is passed, connect this IP address to the
@@ -444,7 +444,7 @@
         """
         if ip is None:
             ip = self.getRandomIPAddress()
-        ipaddress = StaticIPAddress(ip=ip, alloc_type=alloc_type)
+        ipaddress = StaticIPAddress(ip=ip, alloc_type=alloc_type, user=user)
         ipaddress.save()
         if mac is not None:
             MACStaticIPAddressLink(

=== added file 'src/maasserver/tests/test_api_ipaddresses.py'
--- src/maasserver/tests/test_api_ipaddresses.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/tests/test_api_ipaddresses.py	2014-07-01 06:06:42 +0000
@@ -0,0 +1,174 @@
+# Copyright 2014 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for IP addresses API."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+str = None
+
+__metaclass__ = type
+__all__ = []
+
+import httplib
+import json
+
+from django.core.urlresolvers import reverse
+from maasserver.enum import (
+    IPADDRESS_TYPE,
+    NODEGROUP_STATUS,
+    )
+from maasserver.models import StaticIPAddress
+from maasserver.testing import reload_object
+from maasserver.testing.api import APITestCase
+from maasserver.testing.factory import factory
+
+
+class TestNetworksAPI(APITestCase):
+
+    def make_interface(self, status=NODEGROUP_STATUS.ACCEPTED, **kwargs):
+        ng = factory.make_node_group(status=status, **kwargs)
+        return ng.get_managed_interfaces()[0]
+
+    def post_reservation_request(self, net):
+        params = {
+            'op': 'reserve',
+            'network': unicode(net),
+        }
+        return self.client.post(reverse('ipaddresses_handler'), params)
+
+    def post_release_request(self, ip):
+        params = {
+            'op': 'release',
+            'ip': ip,
+        }
+        return self.client.post(reverse('ipaddresses_handler'), params)
+
+    def assertNoMatchingNetworkError(self, response, net):
+        self.assertEqual(
+            httplib.BAD_REQUEST, response.status_code, response.content)
+        self.assertEqual(
+            "No network found matching %s" % unicode(net),
+            response.content)
+
+    def test_handler_path(self):
+        self.assertEqual(
+            '/api/1.0/ipaddresses/', reverse('ipaddresses_handler'))
+
+    def test_POST_reserve_creates_ipaddress(self):
+        interface = self.make_interface()
+        net = interface.network
+        response = self.post_reservation_request(net)
+        self.assertEqual(httplib.OK, response.status_code)
+        returned_address = json.loads(response.content)
+        [staticipaddress] = StaticIPAddress.objects.all()
+        # We don't need to test the value of the 'created' datetime
+        # field. By removing it, we also test for its presence.
+        del returned_address['created']
+        expected = dict(
+            alloc_type=staticipaddress.alloc_type,
+            ip=staticipaddress.ip,
+            resource_uri=reverse('ipaddresses_handler'),
+            )
+        self.assertEqual(expected, returned_address)
+        self.assertEqual(
+            IPADDRESS_TYPE.USER_RESERVED, staticipaddress.alloc_type)
+
+    def test_POST_reserve_errors_for_no_matching_interface(self):
+        interface = self.make_interface()
+        net = factory.getRandomNetwork(but_not=[interface.network])
+        response = self.post_reservation_request(net)
+        self.assertNoMatchingNetworkError(response, net)
+
+    def test_POST_reserve_errors_for_interface_with_no_IP_range(self):
+        interface = self.make_interface()
+        net = interface.network
+        interface.static_ip_range_low = None
+        interface.static_ip_range_high = None
+        interface.save()
+        response = self.post_reservation_request(net)
+        self.assertNoMatchingNetworkError(response, net)
+
+    def test_POST_reserve_errors_for_invalid_network(self):
+        net = factory.getRandomString()
+        response = self.post_reservation_request(net)
+        self.assertEqual(
+            httplib.BAD_REQUEST, response.status_code, response.content)
+        self.assertEqual(
+            "Invalid network parameter %s" % net,
+            response.content)
+
+    def test_GET_returns_ipaddresses(self):
+        original_ipaddress = factory.make_staticipaddress(
+            user=self.logged_in_user)
+        response = self.client.get(reverse('ipaddresses_handler'))
+        self.assertEqual(httplib.OK, response.status_code, response.content)
+
+        parsed_result = json.loads(response.content)
+        self.assertEqual(1, len(parsed_result), response.content)
+        [returned_address] = parsed_result
+        fields = {'alloc_type', 'ip'}
+        self.assertEqual(
+            fields.union({'resource_uri', 'created'}),
+            set(returned_address.keys()))
+        expected_values = {
+            field: getattr(original_ipaddress, field)
+            for field in fields
+            if field not in ('resource_uri', 'created')
+        }
+        # We don't need to test the value of the 'created' datetime
+        # field.
+        del returned_address['created']
+        expected_values['resource_uri'] = reverse('ipaddresses_handler')
+        self.assertEqual(expected_values, returned_address)
+
+    def test_GET_returns_empty_if_no_ipaddresses(self):
+        response = self.client.get(reverse('ipaddresses_handler'))
+        self.assertEqual(httplib.OK, response.status_code, response.content)
+        self.assertEqual([], json.loads(response.content))
+
+    def test_GET_only_returns_request_users_addresses(self):
+        ipaddress = factory.make_staticipaddress(user=self.logged_in_user)
+        factory.make_staticipaddress(user=factory.make_user())
+        response = self.client.get(reverse('ipaddresses_handler'))
+        self.assertEqual(httplib.OK, response.status_code, response.content)
+        parsed_result = json.loads(response.content)
+        [returned_address] = parsed_result
+        self.assertEqual(ipaddress.ip, returned_address['ip'])
+
+    def test_GET_sorts_by_id(self):
+        addrs = []
+        for _ in range(3):
+            addrs.append(
+                factory.make_staticipaddress(user=self.logged_in_user))
+        response = self.client.get(reverse('ipaddresses_handler'))
+        self.assertEqual(httplib.OK, response.status_code, response.content)
+        parsed_result = json.loads(response.content)
+        expected = [
+            addr.ip for addr in
+            sorted(addrs, key=lambda addr: getattr(addr, "id"))]
+        observed = [result['ip'] for result in parsed_result]
+        self.assertEqual(expected, observed)
+
+    def test_POST_release_deallocates_address(self):
+        ipaddress = factory.make_staticipaddress(user=self.logged_in_user)
+        response = self.post_release_request(ipaddress.ip)
+        self.assertEqual(httplib.OK, response.status_code, response.content)
+        self.assertIsNone(reload_object(ipaddress))
+
+    def test_POST_release_does_not_delete_IP_that_I_dont_own(self):
+        ipaddress = factory.make_staticipaddress(user=factory.make_user())
+        response = self.post_release_request(ipaddress.ip)
+        self.assertEqual(
+            httplib.NOT_FOUND, response.status_code, response.content)
+
+    def test_POST_release_does_not_delete_other_IPs_I_own(self):
+        ipaddress = factory.make_staticipaddress(user=self.logged_in_user)
+        other_address = factory.make_staticipaddress(user=self.logged_in_user)
+        response = self.post_release_request(ipaddress.ip)
+        self.assertEqual(httplib.OK, response.status_code, response.content)
+        self.assertIsNotNone(reload_object(other_address))

=== modified file 'src/maasserver/urls_api.py'
--- src/maasserver/urls_api.py	2014-06-09 19:58:06 +0000
+++ src/maasserver/urls_api.py	2014-07-01 06:06:42 +0000
@@ -33,6 +33,7 @@
     describe,
     FileHandler,
     FilesHandler,
+    IPAddressesHandler,
     MaasHandler,
     NetworkHandler,
     NetworksHandler,
@@ -67,6 +68,8 @@
 account_handler = RestrictedResource(AccountHandler, authentication=api_auth)
 files_handler = RestrictedResource(FilesHandler, authentication=api_auth)
 file_handler = RestrictedResource(FileHandler, authentication=api_auth)
+ipaddresses_handler = RestrictedResource(
+    IPAddressesHandler, authentication=api_auth)
 network_handler = RestrictedResource(NetworkHandler, authentication=api_auth)
 networks_handler = RestrictedResource(NetworksHandler, authentication=api_auth)
 node_handler = RestrictedResource(NodeHandler, authentication=api_auth)
@@ -180,6 +183,7 @@
     url(r'^users/(?P<username>[^/]+)/$', user_handler, name='user_handler'),
     url(r'^zones/(?P<name>[^/]+)/$', zone_handler, name='zone_handler'),
     url(r'^zones/$', zones_handler, name='zones_handler'),
+    url(r'^ipaddresses/$', ipaddresses_handler, name='ipaddresses_handler'),
 )