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

[Julian Edwards]

Allow API users to optionally request specific IP addresses when requesting USER_RESERVED static IP addresses. Previously a random one was always allocated.

[Jeroen T. Vermeulen (jtv)]

I can see why you might return an HTTP status code of Forbidden for an address that wasn't in the right range, but Not Found for an address that's already taken seems strange. Isn't this a classic case for the Conflict error?

review: Approve

[Julian Edwards (julian-edwards)]

On Thursday 11 Sep 2014 05:42:49 you wrote:
> Review: Approve
>
> I can see why you might return an HTTP status code of Forbidden for an
> address that wasn't in the right range, but Not Found for an address that's
> already taken seems strange. Isn't this a classic case for the Conflict
> error?

I am following what I did in the similar change in the claim_sticky_ip stuff.
There is a good reason, conflict is wrong IMO. If I can find the original MP
I'll show you later.

Thanks for the review!

=== modified file 'src/maasserver/api/ip_addresses.py'
--- src/maasserver/api/ip_addresses.py	2014-08-19 02:14:31 +0000
+++ src/maasserver/api/ip_addresses.py	2014-09-11 03:26:49 +0000
@@ -21,7 +21,10 @@
     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 (
     IPADDRESS_TYPE,
     NODEGROUP_STATUS,
@@ -32,6 +35,10 @@
     StaticIPAddress,
     )
 import netaddr
+from provisioningserver.logger import get_maas_logger
+
+
+maaslog = get_maas_logger("ip_addresses")
 
 
 class IPAddressesHandler(OperationsHandler):
@@ -46,17 +53,20 @@
     def resource_uri(cls, *args, **kwargs):
         return ('ipaddresses_handler', [])
 
-    def claim_ip(self, user, interface):
+    def claim_ip(self, user, interface, requested_address):
         """Attempt to get a USER_RESERVED StaticIPAddress for `user` on
         `interface`.
 
         :raises StaticIPAddressExhaustion: If no IPs available.
         """
-        return StaticIPAddress.objects.allocate_new(
+        sip = StaticIPAddress.objects.allocate_new(
             range_low=interface.static_ip_range_low,
             range_high=interface.static_ip_range_high,
             alloc_type=IPADDRESS_TYPE.USER_RESERVED,
+            requested_address=requested_address,
             user=user)
+        maaslog.info("User %s was allocated IP %s", user.username, sip.ip)
+        return sip
 
     @operation(idempotent=False)
     def reserve(self, request):
@@ -71,6 +81,8 @@
         :type network: unicode
         """
         network = get_mandatory_param(request.POST, "network")
+        requested_address = get_optional_param(
+            request.POST, "requested_address")
         # Validate the passed network.
         try:
             valid_network = netaddr.IPNetwork(network)
@@ -87,7 +99,8 @@
         for interface in interfaces:
             if valid_network == interface.network:
                 # Winner winner chicken dinner.
-                return self.claim_ip(request.user, interface)
+                return self.claim_ip(
+                    request.user, interface, requested_address)
         raise MAASAPIBadRequest("No network found matching %s" % network)
 
     @operation(idempotent=False)
@@ -101,6 +114,7 @@
         staticaddress = get_object_or_404(
             StaticIPAddress, ip=ip, user=request.user)
         staticaddress.deallocate()
+        maaslog.info("User %s released IP %s", request.user.username, ip)
 
     def read(self, request):
         """List IPAddresses.

=== modified file 'src/maasserver/api/tests/test_ipaddresses.py'
--- src/maasserver/api/tests/test_ipaddresses.py	2014-09-05 16:38:32 +0000
+++ src/maasserver/api/tests/test_ipaddresses.py	2014-09-11 03:26:49 +0000
@@ -26,6 +26,8 @@
 from maasserver.testing.api import APITestCase
 from maasserver.testing.factory import factory
 from maasserver.testing.orm import reload_object
+from netaddr import IPAddress
+from testtools.matchers import Equals
 
 
 class TestNetworksAPI(APITestCase):
@@ -34,11 +36,13 @@
         cluster = factory.make_NodeGroup(status=status, **kwargs)
         return factory.make_NodeGroupInterface(cluster)
 
-    def post_reservation_request(self, net):
+    def post_reservation_request(self, net, requested_address=None):
         params = {
             'op': 'reserve',
             'network': unicode(net),
         }
+        if requested_address is not None:
+            params["requested_address"] = requested_address
         return self.client.post(reverse('ipaddresses_handler'), params)
 
     def post_release_request(self, ip):
@@ -103,6 +107,40 @@
             "Invalid network parameter %s" % net,
             response.content)
 
+    def test_POST_reserve_creates_requested_address(self):
+        interface = self.make_interface()
+        net = interface.network
+        ip_in_network = interface.static_ip_range_low
+        response = self.post_reservation_request(net, ip_in_network)
+        self.assertEqual(httplib.OK, response.status_code, response.content)
+        returned_address = json.loads(response.content)
+        [staticipaddress] = StaticIPAddress.objects.all()
+        self.expectThat(
+            returned_address["alloc_type"],
+            Equals(IPADDRESS_TYPE.USER_RESERVED))
+        self.expectThat(returned_address["ip"], Equals(ip_in_network))
+        self.expectThat(staticipaddress.ip, Equals(ip_in_network))
+
+    def test_POST_reserve_requested_address_detects_in_use_address(self):
+        interface = self.make_interface()
+        net = interface.network
+        ip_in_network = interface.static_ip_range_low
+        response = self.post_reservation_request(net, ip_in_network)
+        self.assertEqual(httplib.OK, response.status_code, response.content)
+        # Do same request again and check it is rejected.
+        response = self.post_reservation_request(net, ip_in_network)
+        self.assertEqual(
+            httplib.NOT_FOUND, response.status_code, response.content)
+
+    def test_POST_reserve_requested_address_detects_out_of_range_addr(self):
+        interface = self.make_interface()
+        net = interface.network
+        ip_not_in_network = unicode(
+            IPAddress(interface.static_ip_range_low) - 1)
+        response = self.post_reservation_request(net, ip_not_in_network)
+        self.assertEqual(
+            httplib.FORBIDDEN, response.status_code, response.content)
+
     def test_GET_returns_ipaddresses(self):
         original_ipaddress = factory.make_StaticIPAddress(
             user=self.logged_in_user)