Merge lp:~gmb/maas/add-api-reserve-sip-for-mac-bug-1387239 into lp:~maas-committers/maas/trunk

[Graham Binns]

Add an optional "mac" parameter to the static IP address reservation API. Specifying a MAC address here will link the new static IP to that MAC address. A DHCP host map will be created for the MAC address. No other IPs may be reserved for that MAC address until the current one is released.

Based loosely on the old branch, but significantly updated to make use of MACAddress.node being null-able now.

We don't do the work of linking MACAddress and StaticIPAddress in the StaticIPAddressManager class because MACAddress.claim_static_ips() gives it us for free! It also gives us protection against trying to create > 1 Static IP for a MAC on a given cluster interface, though I've added an exception to explain that to the user (because otherwise, this would happen):

>>> claim_ip(requested_ip="123.123.0.1", mac="aa:bb:cc:dd:ee:ff")
123.123.0.1
>>> claim_ip(requested_ip="234.234.0.1", mac="aa:bb:cc:dd:ee:ff")
123.123.0.1

Which is confusing.

[Gavin Panella (allenap)]

LGTM.

review: Approve

[Graham Binns (gmb)]

On 26 November 2014 at 11:09, Gavin Panella <email address hidden>
wrote:

> Review: Approve
>
> LGTM.
>
> Diff comments:
>
> > === modified file 'src/maasserver/api/ip_addresses.py'
> > --- src/maasserver/api/ip_addresses.py 2014-11-24 17:55:55 +0000
> > +++ src/maasserver/api/ip_addresses.py 2014-11-25 14:41:46 +0000
> > @@ -16,6 +16,7 @@
> > 'IPAddressesHandler',
> > ]
> >
> > +from django.db import transaction
> > from django.shortcuts import get_object_or_404
> > from maasserver.api.support import (
> > operation,
> > @@ -25,15 +26,23 @@
> > get_mandatory_param,
> > get_optional_param,
> > )
> > +from maasserver.clusterrpc.dhcp import (
> > + remove_host_maps,
> > + update_host_maps,
> > + )
> > from maasserver.enum import (
> > IPADDRESS_TYPE,
> > NODEGROUP_STATUS,
> > )
> > -from maasserver.exceptions import MAASAPIBadRequest
> > +from maasserver.exceptions import (
> > + MAASAPIBadRequest,
> > + StaticIPAlreadyExistsForMACAddress,
> > + )
> > from maasserver.models import (
> > NodeGroupInterface,
> > StaticIPAddress,
> > )
> > +from maasserver.models.macaddress import MACAddress
> > import netaddr
> > from provisioningserver.logger import get_maas_logger
> >
> > @@ -53,19 +62,53 @@
> > def resource_uri(cls, *args, **kwargs):
> > return ('ipaddresses_handler', [])
> >
> > - def claim_ip(self, user, interface, requested_address):
> > + @transaction.atomic
> > + def claim_ip(self, user, interface, requested_address, mac=None):
> > """Attempt to get a USER_RESERVED StaticIPAddress for `user` on
> > `interface`.
> >
> > :raises StaticIPAddressExhaustion: If no IPs available.
> > """
> > - 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)
> > + if mac is not None:
>
> Might be worth switching around this conditional to remove the negative.
>

Yep.

>
> > + mac_address, _ = MACAddress.objects.get_or_create(
> > + mac_address=mac, cluster_interface=interface)
> > + ips_on_interface = (
> > + addr.ip for addr in mac_address.ip_addresses.all()
> > + if netaddr.IPAddress(addr.ip) in interface.network)
> > + if any(ips_on_interface):
> > + # If this MAC already has static IPs on the interface in
> > + # question we raise an error, since we can't sanely
> > + # allocate more addresses for the MAC here.
> > + raise StaticIPAlreadyExistsForMACAddress(
> > + "MAC address %s already has the IP address(es) %s."
> %
> > + (mac, ips_on_interface))
> > +
> > + [sip] = mac_address.claim_static_ips(
> > + alloc_type=IPADDRESS_TYPE.USER_RESERVED, user=user...

=== modified file 'src/maasserver/api/ip_addresses.py'
--- src/maasserver/api/ip_addresses.py	2014-11-24 17:55:55 +0000
+++ src/maasserver/api/ip_addresses.py	2014-11-26 12:14:08 +0000
@@ -16,6 +16,7 @@
     'IPAddressesHandler',
     ]
 
+from django.db import transaction
 from django.shortcuts import get_object_or_404
 from maasserver.api.support import (
     operation,
@@ -25,15 +26,24 @@
     get_mandatory_param,
     get_optional_param,
     )
+from maasserver.clusterrpc.dhcp import (
+    remove_host_maps,
+    update_host_maps,
+    )
 from maasserver.enum import (
     IPADDRESS_TYPE,
     NODEGROUP_STATUS,
     )
-from maasserver.exceptions import MAASAPIBadRequest
+from maasserver.exceptions import (
+    MAASAPIBadRequest,
+    StaticIPAlreadyExistsForMACAddress,
+    )
 from maasserver.models import (
     NodeGroupInterface,
     StaticIPAddress,
     )
+from maasserver.models.macaddress import MACAddress
+from maasserver.utils.orm import commit_within_atomic_block
 import netaddr
 from provisioningserver.logger import get_maas_logger
 
@@ -53,19 +63,65 @@
     def resource_uri(cls, *args, **kwargs):
         return ('ipaddresses_handler', [])
 
-    def claim_ip(self, user, interface, requested_address):
+    @transaction.atomic
+    def claim_ip(self, user, interface, requested_address, mac=None):
         """Attempt to get a USER_RESERVED StaticIPAddress for `user` on
         `interface`.
 
         :raises StaticIPAddressExhaustion: If no IPs available.
         """
-        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)
+        if mac is None:
+            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)
+            commit_within_atomic_block()
+            maaslog.info("User %s was allocated IP %s", user.username, sip.ip)
+        else:
+            # The user has requested a static IP linked to a MAC
+            # address, so we set that up via the MACAddress model.
+            mac_address, _ = MACAddress.objects.get_or_create(
+                mac_address=mac, cluster_interface=interface)
+            ips_on_interface = (
+                addr.ip for addr in mac_address.ip_addresses.all()
+                if netaddr.IPAddress(addr.ip) in interface.network)
+            if any(ips_on_interface):
+                # If this MAC already has static IPs on the interface in
+                # question we raise an error, since we can't sanely
+                # allocate more addresses for the MAC here.
+                raise StaticIPAlreadyExistsForMACAddress(
+                    "MAC address %s already has the IP address(es) %s." %
+                    (mac, ips_on_interface))
+
+            [sip] = mac_address.claim_static_ips(
+                alloc_type=IPADDRESS_TYPE.USER_RESERVED, user=user,
+                requested_address=requested_address)
+            # Update the DHCP host maps for the cluster so that this MAC
+            # gets an entry with this static IP.
+            host_map_updates = {
+                interface.nodegroup: {
+                    sip.ip: mac_address.mac_address,
+                }
+            }
+            # Commit the DB changes before we do RPC calls.
+            commit_within_atomic_block()
+            update_host_maps_failures = list(
+                update_host_maps(host_map_updates))
+            if len(update_host_maps_failures) > 0:
+                # Deallocate the static IPs and delete the MAC address
+                # if it doesn't have a Node attached.
+                if mac_address.node is None:
+                    mac_address.delete()
+                sip.deallocate()
+                commit_within_atomic_block()
+
+                # There will only ever be one error, so raise that.
+                raise update_host_maps_failures[0].value
+            maaslog.info(
+                "User %s was allocated IP %s for MAC address %s",
+                user.username, sip.ip, mac_address.mac_address)
         return sip
 
     @operation(idempotent=False)
@@ -86,6 +142,7 @@
         network = get_mandatory_param(request.POST, "network")
         requested_address = get_optional_param(
             request.POST, "requested_address")
+        mac_address = get_optional_param(request.POST, "mac")
         # Validate the passed network.
         try:
             valid_network = netaddr.IPNetwork(network)
@@ -103,7 +160,7 @@
             if valid_network == interface.network:
                 # Winner winner chicken dinner.
                 return self.claim_ip(
-                    request.user, interface, requested_address)
+                    request.user, interface, requested_address, mac_address)
         raise MAASAPIBadRequest(
             "No network found matching %s; you may be requesting an IP "
             "on a network with no static IP range defined." % network)
@@ -120,7 +177,30 @@
         ip = get_mandatory_param(request.POST, "ip")
         staticaddress = get_object_or_404(
             StaticIPAddress, ip=ip, user=request.user)
+
+        linked_mac_addresses = staticaddress.macaddress_set
+        linked_mac_address_interfaces = set(
+            mac_address.cluster_interface
+            for mac_address in linked_mac_addresses.all())
+
+        # Remove any hostmaps for this IP.
+        host_maps_to_remove = {
+            interface.nodegroup: [staticaddress.ip]
+            for interface in linked_mac_address_interfaces
+            }
+        remove_host_maps_failures = list(
+            remove_host_maps(host_maps_to_remove))
+        if len(remove_host_maps_failures) > 0:
+            # There's only going to be one failure, so raise that.
+            raise remove_host_maps_failures[0].value
+
+        # Delete any MACAddress entries that are attached to this static
+        # IP but that *aren't* attached to a Node. With the DB isolation
+        # at SERIALIZABLE there will be no race here, and it's better to
+        # keep cruft out of the DB.
+        linked_mac_addresses.filter(node=None).delete()
         staticaddress.deallocate()
+
         maaslog.info("User %s released IP %s", request.user.username, ip)
 
     def read(self, request):

=== modified file 'src/maasserver/api/tests/test_ipaddresses.py'
--- src/maasserver/api/tests/test_ipaddresses.py	2014-10-22 10:35:09 +0000
+++ src/maasserver/api/tests/test_ipaddresses.py	2014-11-26 12:14:08 +0000
@@ -18,16 +18,26 @@
 import json
 
 from django.core.urlresolvers import reverse
+from maasserver.api import ip_addresses as ip_addresses_module
 from maasserver.enum import (
     IPADDRESS_TYPE,
     NODEGROUP_STATUS,
     )
 from maasserver.models import StaticIPAddress
+from maasserver.models.macaddress import MACAddress
 from maasserver.testing.api import APITestCase
 from maasserver.testing.factory import factory
 from maasserver.testing.orm import reload_object
+from maastesting.matchers import MockCalledOnceWith
 from netaddr import IPAddress
-from testtools.matchers import Equals
+from provisioningserver.rpc.exceptions import NoConnectionsAvailable
+from testtools.matchers import (
+    Equals,
+    HasLength,
+    Is,
+    Not,
+    )
+from twisted.python.failure import Failure
 
 
 class TestNetworksAPI(APITestCase):
@@ -36,19 +46,22 @@
         cluster = factory.make_NodeGroup(status=status, **kwargs)
         return factory.make_NodeGroupInterface(cluster)
 
-    def post_reservation_request(self, net, requested_address=None):
+    def post_reservation_request(self, net, requested_address=None, mac=None):
         params = {
             'op': 'reserve',
             'network': unicode(net),
         }
         if requested_address is not None:
             params["requested_address"] = requested_address
+        if mac is not None:
+            params["mac"] = mac
         return self.client.post(reverse('ipaddresses_handler'), params)
 
-    def post_release_request(self, ip):
+    def post_release_request(self, ip, mac=None):
         params = {
             'op': 'release',
             'ip': ip,
+            'mac': mac,
         }
         return self.client.post(reverse('ipaddresses_handler'), params)
 
@@ -84,6 +97,77 @@
             IPADDRESS_TYPE.USER_RESERVED, staticipaddress.alloc_type)
         self.assertEqual(self.logged_in_user, staticipaddress.user)
 
+    def test_POST_reserve_with_MAC_links_MAC_to_ip_address(self):
+        update_host_maps = self.patch(ip_addresses_module, 'update_host_maps')
+        interface = self.make_interface()
+        net = interface.network
+        mac = factory.make_mac_address()
+
+        response = self.post_reservation_request(net, mac=mac)
+        self.assertEqual(httplib.OK, response.status_code)
+        returned_address = json.loads(response.content)
+        [staticipaddress] = StaticIPAddress.objects.all()
+        self.expectThat(
+            staticipaddress.macaddress_set.first().mac_address,
+            Equals(mac))
+
+        # DHCP Host maps have been updated.
+        self.expectThat(
+            update_host_maps,
+            MockCalledOnceWith(
+                {interface.nodegroup: {returned_address['ip']: mac}}))
+
+    def test_POST_reserve_with_MAC_returns_503_if_hostmap_update_fails(self):
+        update_host_maps = self.patch(ip_addresses_module, 'update_host_maps')
+        # We a specific exception here because update_host_maps() will
+        # fail with RPC-specific errors.
+        update_host_maps.return_value = [
+            Failure(
+                NoConnectionsAvailable(
+                    "Are you sure you're not Elvis?"))
+            ]
+        interface = self.make_interface()
+        net = interface.network
+        mac = factory.make_mac_address()
+
+        response = self.post_reservation_request(net, mac=mac)
+        self.expectThat(
+            response.status_code, Equals(httplib.SERVICE_UNAVAILABLE))
+        # No static IP has been created.
+        self.expectThat(
+            StaticIPAddress.objects.all(), HasLength(0))
+        # No MAC address has been created, either.
+        self.expectThat(
+            MACAddress.objects.all(), HasLength(0))
+
+    def test_POST_returns_CONFLICT_when_static_ip_for_MAC_already_exists(self):
+        interface = self.make_interface()
+        mac = factory.make_MACAddress(cluster_interface=interface)
+        mac.claim_static_ips()
+        net = interface.network
+
+        response = self.post_reservation_request(net, mac=mac.mac_address)
+        self.expectThat(
+            response.status_code, Equals(httplib.CONFLICT))
+        # No new static IP has been created.
+        self.expectThat(
+            StaticIPAddress.objects.all().count(),
+            Equals(1))
+
+    def test_POST_allows_claiming_of_new_static_ips_for_existing_MAC(self):
+        self.patch(ip_addresses_module, 'update_host_maps')
+
+        interface = self.make_interface()
+        net = interface.network
+        mac = factory.make_MACAddress(cluster_interface=interface)
+
+        response = self.post_reservation_request(net, mac=mac.mac_address)
+        self.expectThat(response.status_code, Equals(httplib.OK))
+        [staticipaddress] = StaticIPAddress.objects.all()
+        self.assertEqual(
+            staticipaddress.macaddress_set.first().mac_address,
+            mac.mac_address)
+
     def test_POST_reserve_errors_for_no_matching_interface(self):
         interface = self.make_interface()
         net = factory.make_ipv4_network(but_not=[interface.network])
@@ -202,6 +286,70 @@
         self.assertEqual(httplib.OK, response.status_code, response.content)
         self.assertIsNone(reload_object(ipaddress))
 
+    def test_POST_release_deletes_floating_MAC_address(self):
+        self.patch(ip_addresses_module, 'remove_host_maps')
+
+        interface = self.make_interface()
+        floating_mac = factory.make_MACAddress(cluster_interface=interface)
+        [ipaddress] = floating_mac.claim_static_ips(
+            alloc_type=IPADDRESS_TYPE.USER_RESERVED, user=self.logged_in_user)
+
+        self.post_release_request(ipaddress.ip)
+        self.assertIsNone(reload_object(floating_mac))
+
+    def test_POST_release_does_not_delete_MACs_linked_to_nodes(self):
+        self.patch(ip_addresses_module, 'remove_host_maps')
+
+        interface = self.make_interface()
+        node = factory.make_Node(nodegroup=interface.nodegroup)
+        attached_mac = factory.make_MACAddress(
+            node=node, cluster_interface=interface)
+        [ipaddress] = attached_mac.claim_static_ips(
+            alloc_type=IPADDRESS_TYPE.USER_RESERVED, user=self.logged_in_user)
+
+        self.post_release_request(ipaddress.ip)
+        self.assertEqual(attached_mac, reload_object(attached_mac))
+
+    def test_POST_release_updates_DNS_and_DHCP(self):
+        remove_host_maps = self.patch(ip_addresses_module, 'remove_host_maps')
+
+        interface = self.make_interface()
+        floating_mac = factory.make_MACAddress(cluster_interface=interface)
+        [ipaddress] = floating_mac.claim_static_ips(
+            alloc_type=IPADDRESS_TYPE.USER_RESERVED, user=self.logged_in_user)
+
+        self.post_release_request(ipaddress.ip)
+        self.expectThat(
+            remove_host_maps, MockCalledOnceWith(
+                {interface.nodegroup: [ipaddress.ip]}))
+
+    def test_POST_release_raises_503_if_removing_host_maps_errors(self):
+        remove_host_maps = self.patch(ip_addresses_module, 'remove_host_maps')
+        # Failures in remove_host_maps() will be RPC-related exceptions,
+        # so we use one of those explicitly.
+        remove_host_maps.return_value = [
+            Failure(
+                NoConnectionsAvailable(
+                    "The wizard's staff has a knob on the end."))
+            ]
+
+        interface = self.make_interface()
+        floating_mac = factory.make_MACAddress(cluster_interface=interface)
+        [ipaddress] = floating_mac.claim_static_ips(
+            alloc_type=IPADDRESS_TYPE.USER_RESERVED, user=self.logged_in_user)
+
+        response = self.post_release_request(ipaddress.ip)
+        self.expectThat(
+            response.status_code, Equals(httplib.SERVICE_UNAVAILABLE))
+
+        # The static IP hasn't been deleted.
+        self.expectThat(
+            reload_object(ipaddress), Not(Is(None)))
+
+        # Neither has the DHCPHost.
+        self.expectThat(
+            reload_object(floating_mac), Not(Is(None)))
+
     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)

=== modified file 'src/maasserver/exceptions.py'
--- src/maasserver/exceptions.py	2014-10-08 09:43:51 +0000
+++ src/maasserver/exceptions.py	2014-11-26 12:14:08 +0000
@@ -140,6 +140,12 @@
     api_error = httplib.CONFLICT
 
 
+class StaticIPAlreadyExistsForMACAddress(MAASAPIException):
+    """Raised when trying to allocate a static IP for a non-node MAC
+    where a node with that MAC already exists."""
+    api_error = httplib.CONFLICT
+
+
 class NodeActionError(MAASException):
     """Raised when there is an error performing a NodeAction."""
 

=== modified file 'src/maasserver/models/macaddress.py'
--- src/maasserver/models/macaddress.py	2014-11-21 20:25:54 +0000
+++ src/maasserver/models/macaddress.py	2014-11-26 12:14:08 +0000
@@ -204,7 +204,7 @@
         return allocations
 
     def _allocate_static_address(self, cluster_interface, alloc_type,
-                                 requested_address=None):
+                                 requested_address=None, user=None):
         """Allocate a `StaticIPAddress` for this MAC."""
         # Avoid circular imports.
         from maasserver.models import (
@@ -215,12 +215,13 @@
         new_sip = StaticIPAddress.objects.allocate_new(
             cluster_interface.static_ip_range_low,
             cluster_interface.static_ip_range_high,
-            alloc_type, requested_address=requested_address)
+            alloc_type, requested_address=requested_address,
+            user=user)
         MACStaticIPAddressLink(mac_address=self, ip_address=new_sip).save()
         return new_sip
 
     def claim_static_ips(self, alloc_type=IPADDRESS_TYPE.AUTO,
-                         requested_address=None):
+                         requested_address=None, user=None):
         """Assign static IP addresses to this MAC.
 
         Allocates one address per managed cluster interface connected to this
@@ -235,6 +236,8 @@
             the range defined on some cluster interface to which this
             MACAddress is related.  If given, no allocations will be made on
             any other cluster interfaces the MAC may be connected to.
+        :param user: Optional User who will be given ownership of any
+            `StaticIPAddress`es claimed.
         :return: A list of :class:`StaticIPAddress`.  Returns empty if
             the cluster_interface is not yet known, or the
             static_ip_range_low/high values values are not set on the
@@ -257,9 +260,13 @@
         # different representations for "none" values in IP addresses.
         if self.cluster_interface is None:
             # No known cluster interface.  Nothing we can do.
+            if self.node is not None:
+                hostname_string = "%s: " % self.node.hostname
+            else:
+                hostname_string = ""
             maaslog.error(
-                "%s: Tried to allocate an IP to MAC %s but its cluster "
-                "interface is not known", self.node.hostname, self)
+                "%sTried to allocate an IP to MAC %s but its cluster "
+                "interface is not known", hostname_string, self)
             return []
         cluster_interfaces = [
             interface
@@ -301,7 +308,7 @@
             if allocations[interface] is None:
                 # No IP address yet on this cluster interface.  Get one.
                 allocations[interface] = self._allocate_static_address(
-                    interface, alloc_type, requested_address)
+                    interface, alloc_type, requested_address, user=user)
 
         # We now have a static IP allocated to each of our cluster interfaces.
         # Ignore the clashes.  Return the ones that have the right type: those

=== modified file 'src/maasserver/models/tests/test_macaddress.py'
--- src/maasserver/models/tests/test_macaddress.py	2014-11-25 14:23:55 +0000
+++ src/maasserver/models/tests/test_macaddress.py	2014-11-26 12:14:08 +0000
@@ -635,6 +635,16 @@
         [sip] = allocation
         self.assertEqual(IPAddress(requested_ip), IPAddress(sip.ip))
 
+    def test__links_static_ip_to_user_if_passed(self):
+        cluster = factory.make_NodeGroup()
+        cluster_interface = factory.make_NodeGroupInterface(cluster)
+        mac_address = factory.make_MACAddress(
+            cluster_interface=cluster_interface)
+        user = factory.make_User()
+        [sip] = mac_address.claim_static_ips(
+            user=user, alloc_type=IPADDRESS_TYPE.USER_RESERVED)
+        self.assertEqual(sip.user, user)
+
 
 class TestGetClusterInterfaces(MAASServerTestCase):
     """Tests for `MACAddress.get_cluster_interfaces`."""

=== modified file 'src/maasserver/testing/factory.py'
--- src/maasserver/testing/factory.py	2014-11-25 14:23:55 +0000
+++ src/maasserver/testing/factory.py	2014-11-26 12:14:08 +0000
@@ -461,8 +461,6 @@
     def make_MACAddress(self, address=None, node=None, networks=None,
                         **kwargs):
         """Create a `MACAddress` model object."""
-        if node is None:
-            node = self.make_Node()
         if address is None:
             address = self.make_mac_address()
         mac = MACAddress(mac_address=MAC(address), node=node, **kwargs)