Merge lp:~trapnine/maas/fix-1564971 into lp:~maas-committers/maas/trunk

[Jeffrey C Jones]

When creating a new IPRange, make sure no other conflicting ranges or IP addresses are in use.

When creating a new IPRange, make sure no other conflicting ranges or IP addresses are in use.

[Mike Pontillo (mpontillo)]

This looks good in general to me. A few comments below.

I just have one question: I see that you are doing the validation in a signal handler. Normally we would put it in the model or in the form. Was there any particular reason it was done this way? (Is there already a precedent in MAAS code somewhere? I have to admit I haven't done much with signals.)

Did you do any manual testing with the API for this change?

review: Needs Information

[Jeffrey C Jones (trapnine)]

> This looks good in general to me. A few comments below.
>
> I just have one question: I see that you are doing the validation in a signal
> handler. Normally we would put it in the model or in the form. Was there any
> particular reason it was done this way? (Is there already a precedent in MAAS
> code somewhere? I have to admit I haven't done much with signals.)
>
> Did you do any manual testing with the API for this change?

I've actually be directed towards signals and away from complex clean() override processing in the past, most notably for the BMC validation work. There are lots of examples in the signals package this new signal handler lives in. This is better than a form clean() because it will be run for any IPRange model save(), not just a form save().

In this case, it was 'almost' necessary because I actually delete the to-be-saved instance, then do some testing, then recreate it. I was not sure I could get away with that in a model's actual save() override (maybe it'd be OK). Also, signals make it easy to store pre-modification state in the post-init signal, then read them in the post-save signal to see what was changed. Basically, after all the other normal form and model checking has been done, this will test for range conflicts.

I did not manually test the API for the change, just the unit tests. This was written as a 'make model save() self-protect against dupes and overlapped ranges' perspective, not a 'fix the API or UI' perspective. Is there a gotcha I'm missing in the API (like maybe it doesn't error to the API cleanly when an Exception is thrown from the model)?

[Mike Pontillo (mpontillo)]

Sounds good.

Yeah, I asked if you tested the API because I was curious if the errors propagate correctly.

Given your explanation, I'm good with this branch (assuming you address the comments I mentioned).

review: Approve

[Jeffrey C Jones (trapnine)]

Per sprint discussion, will move login to model clean().

[Mike Pontillo (mpontillo)]

Looks good; I see some minor issues that I won't block you on, mostly regarding docstrings and comments. (see comments below.) Also, the major function in this branch is a little long and could stand to be refactored to reduce its complexity, IMO.

review: Approve

=== modified file 'src/maasserver/api/tests/test_ipranges.py'
--- src/maasserver/api/tests/test_ipranges.py	2016-02-26 18:39:26 +0000
+++ src/maasserver/api/tests/test_ipranges.py	2016-04-11 09:56:14 +0000
@@ -40,7 +40,7 @@
 
     def test_read(self):
         subnet = factory.make_Subnet(cidr="10.0.0.0/24")
-        factory.make_IPRange(subnet, '10.0.0.1', '10.0.0.10')
+        factory.make_IPRange(subnet, '10.0.0.2', '10.0.0.10')
         factory.make_IPRange(subnet, '10.0.0.11', '10.0.0.20')
         factory.make_IPRange(subnet, '10.0.0.21', '10.0.0.30')
         uri = get_ipranges_uri()
@@ -97,14 +97,14 @@
 
     def test_handler_path(self):
         subnet = factory.make_Subnet(cidr="10.0.0.0/24")
-        iprange = factory.make_IPRange(subnet, '10.0.0.1', '10.0.0.10')
+        iprange = factory.make_IPRange(subnet, '10.0.0.2', '10.0.0.10')
         self.assertEqual(
             '/api/2.0/ipranges/%s/' % iprange.id,
             get_iprange_uri(iprange))
 
     def test_read(self):
         subnet = factory.make_Subnet(cidr="10.0.0.0/24")
-        iprange = factory.make_IPRange(subnet, '10.0.0.1', '10.0.0.10')
+        iprange = factory.make_IPRange(subnet, '10.0.0.2', '10.0.0.10')
         factory.make_IPRange(subnet, '10.0.0.11', '10.0.0.20')
         factory.make_IPRange(subnet, '10.0.0.21', '10.0.0.30')
         uri = get_iprange_uri(iprange)
@@ -131,7 +131,7 @@
 
     def test_update(self):
         subnet = factory.make_Subnet(cidr="10.0.0.0/24")
-        iprange = factory.make_IPRange(subnet, '10.0.0.1', '10.0.0.10')
+        iprange = factory.make_IPRange(subnet, '10.0.0.2', '10.0.0.10')
         uri = get_iprange_uri(iprange)
         comment = factory.make_name("comment")
         response = self.client.put(uri, {
@@ -147,7 +147,7 @@
 
     def test_delete_deletes_iprange(self):
         subnet = factory.make_Subnet(cidr="10.0.0.0/24")
-        iprange = factory.make_IPRange(subnet, '10.0.0.1', '10.0.0.10')
+        iprange = factory.make_IPRange(subnet, '10.0.0.2', '10.0.0.10')
         uri = get_iprange_uri(iprange)
         response = self.client.delete(uri)
         self.assertEqual(
 
=== modified file 'src/maasserver/api/tests/test_subnets.py'
--- src/maasserver/api/tests/test_subnets.py	2016-03-28 13:54:47 +0000
+++ src/maasserver/api/tests/test_subnets.py	2016-04-11 09:56:14 +0000
@@ -385,14 +385,11 @@
                 "num_addresses": expected_addresses,
             }]))
 
-    def test__returns_empty_list_for_full_subnet(self):
-        subnet = factory.make_Subnet()
-        network = subnet.get_ipnetwork()
-        first_address = inet_ntop(network.first + 1)
-        if network.version == 6:
-            last_address = inet_ntop(network.last)
-        else:
-            last_address = inet_ntop(network.last - 1)
+    def _unreserved_ip_ranges_empty(self, subnet, first_address, last_address):
+        """ Used by the succeeding three tests to prevent duplicating the
+        boilerplate that creates the requested range, then makes sure the
+        unreserved_ip_ranges API call successfully returns an empty list.
+        """
         factory.make_IPRange(
             subnet, first_address, last_address,
             type=IPRANGE_TYPE.DYNAMIC)
@@ -406,6 +403,33 @@
         self.assertThat(
             result, Equals([]), str(subnet.get_ipranges_in_use()))
 
+    def test__returns_empty_list_for_full_ipv4_subnet(self):
+        network = factory.make_ipv4_network()
+        subnet = factory.make_Subnet(cidr=str(network.cidr), dns_servers=[])
+        network = subnet.get_ipnetwork()
+        first_address = inet_ntop(network.first + 2)  # Skip gateway, network.
+        last_address = inet_ntop(network.last - 1)  # Skip broadcast.
+        self._unreserved_ip_ranges_empty(subnet, first_address, last_address)
+
+    def test__returns_empty_list_for_full_ipv6_subnet(self):
+        network = factory.make_ipv6_network()
+        subnet = factory.make_Subnet(cidr=str(network.cidr), dns_servers=[])
+        network = subnet.get_ipnetwork()
+        first_address = inet_ntop(network.first + 2)  # Skip gateway, network.
+        last_address = inet_ntop(network.last)
+        self._unreserved_ip_ranges_empty(subnet, first_address, last_address)
+
+    # Slash-64 ipv6 subnets get a special range put in them - test separately.
+    def test__returns_empty_list_for_full_ipv6_slash_64_subnet(self):
+        network = factory.make_ipv6_network(slash=64)
+        subnet = factory.make_Subnet(cidr=str(network.cidr), dns_servers=[])
+        network = subnet.get_ipnetwork()
+        strip64 = (network.first >> 8) << 8
+        # Skip the automatically reserved range on /64's.
+        first_address = inet_ntop(strip64 + 0x100000000)
+        last_address = inet_ntop(network.last)
+        self._unreserved_ip_ranges_empty(subnet, first_address, last_address)
+
     def test__accounts_for_reserved_ip_address(self):
         subnet = factory.make_ipv4_Subnet_with_IPRanges(
             with_dynamic_range=False, dns_servers=[], with_router=False)
 
=== modified file 'src/maasserver/models/iprange.py'
--- src/maasserver/models/iprange.py	2016-03-28 13:54:47 +0000
+++ src/maasserver/models/iprange.py	2016-04-11 09:56:14 +0000
@@ -19,11 +19,17 @@
     PROTECT,
     QuerySet,
 )
-from maasserver.enum import IPRANGE_TYPE_CHOICES
+from maasserver.enum import (
+    IPRANGE_TYPE,
+    IPRANGE_TYPE_CHOICES,
+)
 from maasserver.fields import MAASIPAddressField
 from maasserver.models.cleansave import CleanSave
 from maasserver.models.timestampedmodel import TimestampedModel
-from maasserver.utils.orm import MAASQueriesMixin
+from maasserver.utils.orm import (
+    MAASQueriesMixin,
+    transactional,
+)
 import netaddr
 from netaddr import (
     AddrFormatError,
@@ -155,6 +161,7 @@
             if end_ip not in cidr:
                 raise ValidationError(
                     "End IP address must be within subnet: %s." % cidr)
+        self.clean_prevent_dupes_and_overlaps()
 
     @property
     def netaddr_iprange(self):
@@ -166,3 +173,65 @@
         # APIs in previous MAAS releases used '-' in range types.
         purpose = purpose.replace('_', '-')
         return make_iprange(self.start_ip, self.end_ip, purpose=purpose)
+
+    @transactional
+    def clean_prevent_dupes_and_overlaps(self):
+
+        # A range overlap/conflict could be due to any of these fields.
+        def fail(message, fields=['start_ip', 'end_ip', 'type']):
+            for field in fields:
+                validation_errors[field] = [message]
+            raise ValidationError(validation_errors)
+
+        """Make sure the new or updated range isn't going to cause a conflict.
+        If it will, raise ValidationError.
+        """
+        # If model is incomplete, save() will fail, so don't bother checking.
+        if self.subnet_id is None or self.start_ip is None or (
+                self.end_ip is None) or self.type is None:
+            return
+
+        # The _state.adding flag is False if this instance exists in the DB.
+        # See https://docs.djangoproject.com/en/1.9/ref/models/instances/.
+        if not self._state.adding:
+            orig = IPRange.objects.get(pk=self.pk)
+            if orig.type == self.type and (
+                    orig.start_ip == self.start_ip) and (
+                    orig.end_ip == self.end_ip):
+                # Range not materially modified, no range dupe check required.
+                return
+            # Pre-existing range moved: remove existing, check, then re-create.
+            if IPRange.objects.filter(pk=self.id).exists():
+                self_id = self.id
+                # Delete will be rolled back if imminent range checks raise.
+                self.delete()
+                # Simulate update by setting the ID back to what it was.
+                self.id = self_id
+
+        # Reserved ranges can overlap allocated IPs but not other ranges.
+        # Dynamic ranges cannot overlap anything (no ranges or IPs).
+        if self.type == IPRANGE_TYPE.RESERVED:
+            unused = self.subnet.get_ipranges_available_for_reserved_range()
+        else:
+            unused = self.subnet.get_ipranges_available_for_dynamic_range()
+
+        validation_errors = {}
+        if len(unused) == 0:
+            fail("There is no room for any %s ranges on this subnet." % (
+                self.type))
+
+        # Find unused range for start_ip
+        for range in unused:
+            if IPAddress(self.start_ip) in range:
+                if IPAddress(self.end_ip) in range:
+                    # Success, start and end IP are in an unused range.
+                    return
+                else:
+                    message = ("Requested %s range conflicts with "
+                               "an existing ") % (self.type)
+                    if self.type == IPRANGE_TYPE.RESERVED:
+                        fail(message + "range.")
+                    else:
+                        fail(message + "IP address or range.")
+        fail("No %s range can be created at requested start IP." % self.type,
+             ['start_ip', 'type'])
 
=== modified file 'src/maasserver/models/subnet.py'
--- src/maasserver/models/subnet.py	2016-04-08 12:03:57 +0000
+++ src/maasserver/models/subnet.py	2016-04-11 09:56:14 +0000
@@ -8,7 +8,6 @@
     'Subnet',
 ]
 
-
 from django.contrib.postgres.fields import ArrayField
 from django.core.exceptions import (
     PermissionDenied,
@@ -407,7 +406,7 @@
             for ip in self.staticipaddress_set.all()
             if ip.ip)
 
-    def get_ipranges_in_use(self, exclude_addresses=[]):
+    def get_ipranges_in_use(self, exclude_addresses=[], ranges_only=False):
         """Returns a `MAASIPSet` of `MAASIPRange` objects which are currently
         in use on this `Subnet`.
 
@@ -429,37 +428,46 @@
             second = str(IPAddress(network.first + 0xFFFFFFFF))
             ranges |= {make_iprange(first, second, purpose="reserved")}
         ipnetwork = self.get_ipnetwork()
-        assigned_ip_addresses = self.get_staticipaddresses_in_use()
-        if (self.gateway_ip is not None and self.gateway_ip != '' and
-                self.gateway_ip in ipnetwork):
-            ranges |= {make_iprange(self.gateway_ip, purpose="gateway-ip")}
-        if self.dns_servers is not None:
-            ranges |= set(
-                make_iprange(server, purpose="dns-server")
-                for server in self.dns_servers
-                if server in ipnetwork
-            )
-        ranges |= set(
-            make_iprange(ip, purpose="assigned-ip")
-            for ip in assigned_ip_addresses
-            if ip in ipnetwork
-        )
-        ranges |= set(
-            make_iprange(address, purpose="excluded")
-            for address in exclude_addresses
-            if address in network
-        )
+        if not ranges_only:
+            assigned_ip_addresses = self.get_staticipaddresses_in_use()
+            if (self.gateway_ip is not None and self.gateway_ip != '' and
+                    self.gateway_ip in ipnetwork):
+                ranges |= {make_iprange(self.gateway_ip, purpose="gateway-ip")}
+            if self.dns_servers is not None:
+                ranges |= set(
+                    make_iprange(server, purpose="dns-server")
+                    for server in self.dns_servers
+                    if server in ipnetwork
+                )
+            ranges |= set(
+                make_iprange(ip, purpose="assigned-ip")
+                for ip in assigned_ip_addresses
+                if ip in ipnetwork
+            )
+            ranges |= set(
+                make_iprange(address, purpose="excluded")
+                for address in exclude_addresses
+                if address in network
+            )
         ranges |= self.get_reserved_maasipset()
         ranges |= self.get_dynamic_maasipset()
         return MAASIPSet(ranges)
 
-    def get_ipranges_not_in_use(self, exclude_addresses=[]):
+    def get_ipranges_available_for_reserved_range(self):
+        return self.get_ipranges_not_in_use(ranges_only=True)
+
+    def get_ipranges_available_for_dynamic_range(self):
+        return self.get_ipranges_not_in_use()
+
+    def get_ipranges_not_in_use(self, exclude_addresses=[], ranges_only=False):
         """Returns a `MAASIPSet` of ranges which are currently free on this
         `Subnet`.
 
         :param exclude_addresses: An iterable of addresses not to use.
         """
-        ranges = self.get_ipranges_in_use(exclude_addresses=exclude_addresses)
+        ranges = self.get_ipranges_in_use(
+            exclude_addresses=exclude_addresses,
+            ranges_only=ranges_only)
         unused = ranges.get_unused_ranges(self.get_ipnetwork())
         return unused
 
 
=== modified file 'src/maasserver/models/tests/test_iprange.py'
--- src/maasserver/models/tests/test_iprange.py	2016-03-28 13:54:47 +0000
+++ src/maasserver/models/tests/test_iprange.py	2016-04-11 09:56:14 +0000
@@ -6,26 +6,37 @@
 __all__ = []
 
 from django.core.exceptions import ValidationError
-from maasserver.enum import IPRANGE_TYPE
+from maasserver.enum import (
+    IPADDRESS_TYPE,
+    IPRANGE_TYPE,
+)
 from maasserver.models import IPRange
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
+from maasserver.utils.orm import reload_object
 from testtools import ExpectedException
 
 
+def make_plain_subnet():
+    return factory.make_Subnet(
+        cidr='192.168.0.0/24',
+        gateway_ip='192.168.0.1',
+        dns_servers=[])
+
+
 class IPRangeTest(MAASServerTestCase):
 
     def test__create(self):
-        subnet = factory.make_Subnet(cidr='192.168.0.0/24')
+        subnet = make_plain_subnet()
         iprange = IPRange(
-            start_ip='192.168.0.1', end_ip='192.168.0.254',
+            start_ip='192.168.0.2', end_ip='192.168.0.254',
             type=IPRANGE_TYPE.RESERVED, user=factory.make_User(),
             comment="The quick brown fox jumps over the lazy dog.",
             subnet=subnet)
         iprange.save()
 
     def test__requires_valid_ip_addresses(self):
-        subnet = factory.make_Subnet(cidr='192.168.0.0/24')
+        subnet = make_plain_subnet()
         iprange = IPRange(
             start_ip='x192.x168.x0.x1', end_ip='y192.y168.y0.y254',
             type=IPRANGE_TYPE.RESERVED, user=factory.make_User(),
@@ -35,7 +46,7 @@
             iprange.save()
 
     def test__requires_start_ip_address(self):
-        subnet = factory.make_Subnet(cidr='192.168.0.0/24')
+        subnet = make_plain_subnet()
         iprange = IPRange(
             start_ip='192.168.0.1', type=IPRANGE_TYPE.RESERVED,
             user=factory.make_User(), subnet=subnet,
@@ -44,7 +55,7 @@
             iprange.save()
 
     def test__requires_end_ip_address(self):
-        subnet = factory.make_Subnet(cidr='192.168.0.0/24')
+        subnet = make_plain_subnet()
         iprange = IPRange(
             end_ip='192.168.0.1', type=IPRANGE_TYPE.RESERVED,
             user=factory.make_User(), subnet=subnet,
@@ -53,7 +64,7 @@
             iprange.save()
 
     def test__requires_matching_address_family(self):
-        subnet = factory.make_Subnet(cidr='192.168.0.0/24')
+        subnet = make_plain_subnet()
         iprange = IPRange(
             start_ip='192.168.0.1', end_ip='2001:db8::1',
             type=IPRANGE_TYPE.RESERVED,
@@ -71,7 +82,7 @@
             iprange.save()
 
     def test__requires_start_ip_and_end_ip(self):
-        subnet = factory.make_Subnet(cidr='192.168.0.0/24')
+        subnet = make_plain_subnet()
         iprange = IPRange(
             subnet=subnet, type=IPRANGE_TYPE.RESERVED,
             user=factory.make_User(),
@@ -80,7 +91,7 @@
             iprange.save()
 
     def test__requires_start_ip_and_end_ip_to_be_within_subnet(self):
-        subnet = factory.make_Subnet(cidr='192.168.0.0/24')
+        subnet = make_plain_subnet()
         iprange = IPRange(
             start_ip='192.168.1.1', end_ip='192.168.1.254', subnet=subnet,
             type=IPRANGE_TYPE.RESERVED, user=factory.make_User(),
@@ -90,7 +101,7 @@
             iprange.save()
 
     def test__requires_start_ip_to_be_within_subnet(self):
-        subnet = factory.make_Subnet(cidr='192.168.0.0/24')
+        subnet = make_plain_subnet()
         iprange = IPRange(
             start_ip='19.168.0.1', end_ip='192.168.0.254', subnet=subnet,
             type=IPRANGE_TYPE.DYNAMIC, user=factory.make_User(),
@@ -100,7 +111,7 @@
             iprange.save()
 
     def test__requires_end_ip_to_be_within_subnet(self):
-        subnet = factory.make_Subnet(cidr='192.168.0.0/24')
+        subnet = make_plain_subnet()
         iprange = IPRange(
             start_ip='192.168.0.1', end_ip='193.168.0.254',
             subnet=subnet, type=IPRANGE_TYPE.DYNAMIC,
@@ -111,7 +122,7 @@
             iprange.save()
 
     def test__requires_end_ip_to_be_greater_or_equal_to_start_ip(self):
-        subnet = factory.make_Subnet(cidr='192.168.0.0/24')
+        subnet = make_plain_subnet()
         iprange = IPRange(
             start_ip='192.168.0.2', end_ip='192.168.0.1',
             user=factory.make_User(), subnet=subnet,
@@ -122,7 +133,7 @@
             iprange.save()
 
     def test__requires_type(self):
-        subnet = factory.make_Subnet(cidr='192.168.0.0/24')
+        subnet = make_plain_subnet()
         iprange = IPRange(
             start_ip='192.168.0.1', end_ip='192.168.0.254',
             user=factory.make_User(), subnet=subnet,
@@ -131,16 +142,377 @@
             iprange.save()
 
     def test__user_optional(self):
-        subnet = factory.make_Subnet(cidr='192.168.0.0/24')
+        subnet = make_plain_subnet()
         iprange = IPRange(
-            start_ip='192.168.0.1', end_ip='192.168.0.254',
+            start_ip='192.168.0.2', end_ip='192.168.0.254',
             type=IPRANGE_TYPE.DYNAMIC, subnet=subnet,
             comment="The quick brown owl jumps over the lazy alligator.")
         iprange.save()
 
     def test__comment_optional(self):
-        subnet = factory.make_Subnet(cidr='192.168.0.0/24')
+        subnet = make_plain_subnet()
         iprange = IPRange(
-            start_ip='192.168.0.1', end_ip='192.168.0.254', subnet=subnet,
+            start_ip='192.168.0.2', end_ip='192.168.0.254', subnet=subnet,
             type=IPRANGE_TYPE.RESERVED, user=factory.make_User())
         iprange.save()
+
+
+class TestIPRangeSavePreventsOverlapping(MAASServerTestCase):
+
+    no_fit = ".*No %s range can be created at requested start IP."
+    dynamic_no_fit = no_fit % IPRANGE_TYPE.DYNAMIC
+    reserved_no_fit = no_fit % IPRANGE_TYPE.RESERVED
+
+    overlaps = ".*Requested %s range conflicts with an existing %srange.*"
+    dynamic_overlaps = overlaps % (IPRANGE_TYPE.DYNAMIC, "IP address or ")
+    reserved_overlaps = overlaps % (IPRANGE_TYPE.RESERVED, "")
+
+    no_room = ".*There is no room for any %s ranges on this subnet.*"
+    dynamic_no_room = no_room % IPRANGE_TYPE.DYNAMIC
+    reserved_no_room = no_room % IPRANGE_TYPE.RESERVED
+
+    def test__no_save_duplicate_ipranges(self):
+        subnet = make_plain_subnet()
+        IPRange(
+            subnet=subnet,
+            type=IPRANGE_TYPE.DYNAMIC,
+            start_ip="192.168.0.100",
+            end_ip="192.168.0.150",
+        ).save()
+        # Make the same range again, should fail to save.
+        iprange = IPRange(
+            subnet=subnet,
+            type=IPRANGE_TYPE.DYNAMIC,
+            start_ip="192.168.0.100",
+            end_ip="192.168.0.150",
+        )
+        with ExpectedException(ValidationError, self.dynamic_no_fit):
+            iprange.save()
+
+    def test__no_save_range_overlap_begin(self):
+        subnet = make_plain_subnet()
+        IPRange(
+            subnet=subnet,
+            type=IPRANGE_TYPE.DYNAMIC,
+            start_ip="192.168.0.100",
+            end_ip="192.168.0.150",
+        ).save()
+        # Make an overlapping range across start_ip, should fail to save.
+        iprange = IPRange(
+            subnet=subnet,
+            type=IPRANGE_TYPE.DYNAMIC,
+            start_ip="192.168.0.90",
+            end_ip="192.168.0.100",
+        )
+        with ExpectedException(ValidationError, self.dynamic_overlaps):
+            iprange.save()
+        # Try as reserved range.
+        iprange.type = IPRANGE_TYPE.RESERVED
+        with ExpectedException(ValidationError, self.reserved_overlaps):
+            iprange.save()
+
+    def test__no_save_range_overlap_end(self):
+        subnet = make_plain_subnet()
+        IPRange(
+            subnet=subnet,
+            type=IPRANGE_TYPE.DYNAMIC,
+            start_ip="192.168.0.100",
+            end_ip="192.168.0.150",
+        ).save()
+        # Make an overlapping range across end_ip, should fail to save.
+        iprange = IPRange(
+            subnet=subnet,
+            type=IPRANGE_TYPE.DYNAMIC,
+            start_ip="192.168.0.140",
+            end_ip="192.168.0.160",
+        )
+        with ExpectedException(ValidationError, self.dynamic_no_fit):
+            iprange.save()
+
+    def test__no_save_range_within_ranges(self):
+        subnet = make_plain_subnet()
+        IPRange(
+            subnet=subnet,
+            type=IPRANGE_TYPE.DYNAMIC,
+            start_ip="192.168.0.100",
+            end_ip="192.168.0.150",
+        ).save()
+        # Make a contained range, should not save.
+        iprange = IPRange(
+            subnet=subnet,
+            type=IPRANGE_TYPE.DYNAMIC,
+            start_ip="192.168.0.110",
+            end_ip="192.168.0.140",
+        )
+        with ExpectedException(ValidationError, self.dynamic_no_fit):
+            iprange.save()
+
+    def test__no_save_range_spanning_existing_range(self):
+        subnet = make_plain_subnet()
+        IPRange(
+            subnet=subnet,
+            type=IPRANGE_TYPE.DYNAMIC,
+            start_ip="192.168.0.100",
+            end_ip="192.168.0.150",
+        ).save()
+        # Make a contained range, should not save.
+        iprange = IPRange(
+            subnet=subnet,
+            type=IPRANGE_TYPE.DYNAMIC,
+            start_ip="192.168.0.10",
+            end_ip="192.168.0.240",
+        )
+        with ExpectedException(ValidationError, self.dynamic_overlaps):
+            iprange.save()
+
+    def test__no_save_range_within_existing_range(self):
+        subnet = make_plain_subnet()
+        IPRange(
+            subnet=subnet,
+            type=IPRANGE_TYPE.DYNAMIC,
+            start_ip="192.168.0.100",
+            end_ip="192.168.0.150",
+        ).save()
+        # Make a contained range, should not save.
+        iprange = IPRange(
+            subnet=subnet,
+            type=IPRANGE_TYPE.DYNAMIC,
+            start_ip="192.168.0.110",
+            end_ip="192.168.0.140",
+        )
+        with ExpectedException(ValidationError, self.dynamic_no_fit):
+            iprange.save()
+
+    def test__no_save_range_within_existing_reserved_range(self):
+        subnet = make_plain_subnet()
+        IPRange(
+            subnet=subnet,
+            type=IPRANGE_TYPE.RESERVED,
+            start_ip="192.168.0.100",
+            end_ip="192.168.0.150",
+        ).save()
+        # Make a contained range, should not save.
+        iprange = IPRange(
+            subnet=subnet,
+            type=IPRANGE_TYPE.DYNAMIC,
+            start_ip="192.168.0.110",
+            end_ip="192.168.0.140",
+        )
+        with ExpectedException(ValidationError, self.dynamic_no_fit):
+            iprange.save()
+
+    def test__no_save_when_no_ranges_available(self):
+        subnet = make_plain_subnet()
+        # Reserve the whole subnet, except gateway.
+        IPRange(
+            subnet=subnet,
+            type=IPRANGE_TYPE.RESERVED,
+            start_ip="192.168.0.2",
+            end_ip="192.168.0.254",
+        ).save()
+        # Try to make dynamic range at gateway (anywhere, actually) = no room!
+        iprange = IPRange(
+            subnet=subnet,
+            type=IPRANGE_TYPE.DYNAMIC,
+            start_ip="192.168.0.1",
+            end_ip="192.168.0.1",
+        )
+        with ExpectedException(ValidationError, self.dynamic_no_room):
+            iprange.save()
+        # We CAN reserve the gateway addr.
+        IPRange(
+            subnet=subnet,
+            type=IPRANGE_TYPE.RESERVED,
+            start_ip="192.168.0.1",
+            end_ip="192.168.0.1",
+        ).save()
+        # But now it's full - trying to save any reserved = no room!
+        iprange = IPRange(
+            subnet=subnet,
+            type=IPRANGE_TYPE.RESERVED,
+            start_ip="192.168.0.25",
+            end_ip="192.168.0.35",
+        )
+        with ExpectedException(ValidationError, self.reserved_no_room):
+            iprange.save()
+
+    def test__modify_existing_performs_validation(self):
+        subnet = make_plain_subnet()
+        IPRange(
+            subnet=subnet,
+            type=IPRANGE_TYPE.DYNAMIC,
+            start_ip="192.168.0.100",
+            end_ip="192.168.0.150",
+        ).save()
+        iprange = IPRange(
+            subnet=subnet,
+            type=IPRANGE_TYPE.DYNAMIC,
+            start_ip="192.168.0.151",
+            end_ip="192.168.0.200",
+        )
+        iprange.save()
+        # Make sure safe modification works.
+        iprange.start_ip = "192.168.0.210"
+        iprange.end_ip = "192.168.0.250"
+        iprange.save()
+        # Modify again, but conflict with first range this time.
+        instance_id = iprange.id
+        iprange.start_ip = "192.168.0.110"
+        iprange.end_ip = "192.168.0.140"
+        with ExpectedException(ValidationError, self.dynamic_no_fit):
+            iprange.save()
+        # Make sure original range isn't deleted after failure to modify.
+        iprange = reload_object(iprange)
+        self.assertEqual(iprange.id, instance_id)
+
+    def test__dynamic_range_cant_overlap_gateway_ip(self):
+        subnet = make_plain_subnet()
+        iprange = IPRange(
+            subnet=subnet,
+            type=IPRANGE_TYPE.DYNAMIC,
+            start_ip="192.168.0.2",
+            end_ip="192.168.0.5",
+        )
+        iprange.save()
+        # A DYNAMIC range cannot overlap the gateway IP.
+        iprange.start_ip = "192.168.0.1"
+        with ExpectedException(ValidationError, self.dynamic_no_fit):
+            iprange.save()
+
+    def test__reserved_range_can_overlap_gateway_ip(self):
+        subnet = make_plain_subnet()
+        iprange = IPRange(
+            subnet=subnet,
+            type=IPRANGE_TYPE.RESERVED,
+            start_ip="192.168.0.2",
+            end_ip="192.168.0.5",
+        )
+        iprange.save()
+        # A RESERVED range can overlap the gateway IP.
+        iprange.start_ip = "192.168.0.1"
+        iprange.save()
+
+    def test__reserved_range_cannot_overlap_dynamic_ranges(self):
+        subnet = factory.make_Subnet(
+            cidr='192.168.0.0/24',
+            gateway_ip='192.168.0.1',
+            dns_servers=['192.168.0.50', '192.168.0.200'])
+        IPRange(
+            subnet=subnet,
+            type=IPRANGE_TYPE.DYNAMIC,
+            start_ip="192.168.0.2",
+            end_ip="192.168.0.49",
+        ).save()
+        iprange = IPRange(
+            subnet=subnet,
+            type=IPRANGE_TYPE.RESERVED,
+            start_ip="192.168.0.25",
+            end_ip="192.168.0.30",
+        )
+        with ExpectedException(ValidationError, self.reserved_no_fit):
+            iprange.save()
+
+    def test__reserved_range_cannot_overlap_reserved_ranges(self):
+        subnet = factory.make_Subnet(
+            cidr='192.168.0.0/24',
+            gateway_ip='192.168.0.1',
+            dns_servers=['192.168.0.50', '192.168.0.200'])
+        IPRange(
+            subnet=subnet,
+            type=IPRANGE_TYPE.RESERVED,
+            start_ip="192.168.0.1",
+            end_ip="192.168.0.250",
+        ).save()
+        iprange = IPRange(
+            subnet=subnet,
+            type=IPRANGE_TYPE.RESERVED,
+            start_ip="192.168.0.250",
+            end_ip="192.168.0.254",
+        )
+        with ExpectedException(ValidationError, self.reserved_no_fit):
+            iprange.save()
+
+    def test__dynamic_range_cannot_overlap_static_ip(self):
+        subnet = make_plain_subnet()
+        factory.make_StaticIPAddress(
+            alloc_type=IPADDRESS_TYPE.USER_RESERVED, subnet=subnet)
+        iprange = IPRange(
+            subnet=subnet,
+            type=IPRANGE_TYPE.DYNAMIC,
+            start_ip="192.168.0.2",
+            end_ip="192.168.0.254",
+        )
+        with ExpectedException(ValidationError, self.dynamic_overlaps):
+            iprange.save()
+
+    def test__reserved_range_can_overlap_static_ip(self):
+        subnet = make_plain_subnet()
+        factory.make_StaticIPAddress(
+            alloc_type=IPADDRESS_TYPE.USER_RESERVED, subnet=subnet)
+        iprange = IPRange(
+            subnet=subnet,
+            type=IPRANGE_TYPE.RESERVED,
+            start_ip="192.168.0.1",
+            end_ip="192.168.0.254",
+        )
+        iprange.save()
+
+    def test__dynamic_range_cannot_overlap_dns_servers(self):
+        subnet = factory.make_Subnet(
+            cidr='192.168.0.0/24',
+            gateway_ip='192.168.0.1',
+            dns_servers=['192.168.0.50', '192.168.0.200'])
+        iprange = IPRange(
+            subnet=subnet,
+            type=IPRANGE_TYPE.DYNAMIC,
+            start_ip="192.168.0.1",
+            end_ip="192.168.0.254",
+        )
+        with ExpectedException(ValidationError, self.dynamic_no_fit):
+            iprange.save()
+
+    def test__reserved_range_can_overlap_dns_servers(self):
+        subnet = factory.make_Subnet(
+            cidr='192.168.0.0/24',
+            gateway_ip='192.168.0.1',
+            dns_servers=['192.168.0.50', '192.168.0.200'])
+        iprange = IPRange(
+            subnet=subnet,
+            type=IPRANGE_TYPE.RESERVED,
+            start_ip="192.168.0.1",
+            end_ip="192.168.0.254",
+        )
+        iprange.save()
+
+    def test__change_reserved_to_dynamic(self):
+        subnet = make_plain_subnet()
+        iprange = IPRange(
+            subnet=subnet,
+            type=IPRANGE_TYPE.RESERVED,
+            start_ip="192.168.0.1",
+            end_ip="192.168.0.5",
+        )
+        # Reserved should save OK overlapping gateway IP.
+        iprange.save()
+
+        # Dynamic should not save overlapping gateway IP.
+        iprange.type = IPRANGE_TYPE.DYNAMIC
+        with ExpectedException(ValidationError, self.dynamic_no_fit):
+            iprange.save()
+        # Fix start_ip and now it should save.
+        iprange.start_ip = "192.168.0.2"
+        iprange.save()
+
+    def test__change_dynamic_to_reserved(self):
+        subnet = make_plain_subnet()
+        iprange = IPRange(
+            subnet=subnet,
+            type=IPRANGE_TYPE.DYNAMIC,
+            start_ip="192.168.0.2",
+            end_ip="192.168.0.5",
+        )
+        iprange.save()
+        # Reserved should save OK overlapping gateway IP.
+        iprange.type = IPRANGE_TYPE.RESERVED
+        iprange.start_ip = "192.168.0.1"
+        iprange.save()
 
=== modified file 'src/maasserver/models/tests/test_staticipaddress.py'
--- src/maasserver/models/tests/test_staticipaddress.py	2016-03-28 13:54:47 +0000
+++ src/maasserver/models/tests/test_staticipaddress.py	2016-04-11 09:56:14 +0000
@@ -322,7 +322,7 @@
         with transaction.atomic():
             subnet = factory.make_Subnet(cidr=network)
             factory.make_IPRange(
-                subnet, '192.168.230.0', '192.168.230.255',
+                subnet, '192.168.230.1', '192.168.230.254',
                 type=IPRANGE_TYPE.RESERVED)
         with transaction.atomic():
             e = self.assertRaises(
 
=== modified file 'src/maasserver/testing/factory.py'
--- src/maasserver/testing/factory.py	2016-04-07 18:58:38 +0000
+++ src/maasserver/testing/factory.py	2016-04-11 09:56:14 +0000
@@ -118,6 +118,7 @@
     IPNetwork,
 )
 from provisioningserver.utils.enum import map_enum
+from provisioningserver.utils.network import inet_ntop
 
 # We have a limited number of public keys:
 # src/maasserver/tests/data/test_rsa{0, 1, 2, 3, 4}.pub
@@ -750,11 +751,13 @@
             vlan = factory.make_VLAN(fabric=fabric, vid=vid, dhcp_on=dhcp_on)
         if space is None:
             space = factory.make_Space()
+        network = None
         if cidr is None:
             network = factory.make_ip4_or_6_network(host_bits=host_bits)
             cidr = str(network.cidr)
         if gateway_ip is None:
-            gateway_ip = factory.pick_ip_in_network(IPNetwork(cidr))
+            network = IPNetwork(cidr) if network is None else network
+            gateway_ip = inet_ntop(network.first + 1)
         if dns_servers is None:
             dns_servers = [
                 self.make_ip_address() for _ in range(random.randint(1, 3))]
 
=== modified file 'src/maasserver/tests/test_dhcp.py'
--- src/maasserver/tests/test_dhcp.py	2016-04-01 21:14:24 +0000
+++ src/maasserver/tests/test_dhcp.py	2016-04-11 09:56:14 +0000
@@ -1220,8 +1220,8 @@
         ha_subnet = factory.make_Subnet(
             vlan=ha_vlan, cidr="fd38:c341:27da:c831::/64")
         factory.make_IPRange(
-            ha_subnet, "fd38:c341:27da:c831::0001:0000",
-            "fd38:c341:27da:c831::FFFF:0000")
+            ha_subnet, "fd38:c341:27da:c831:0:1::",
+            "fd38:c341:27da:c831:0:1:ffff:0")
         factory.make_Interface(
             INTERFACE_TYPE.PHYSICAL, node=primary_rack, vlan=ha_vlan)
         secondary_interface = factory.make_Interface(
@@ -1251,8 +1251,8 @@
             vlan=ha_vlan, cidr="fd38:c341:27da:c831::/64")
         ha_network = ha_subnet.get_ipnetwork()
         factory.make_IPRange(
-            ha_subnet, "fd38:c341:27da:c831::0001:0000",
-            "fd38:c341:27da:c831::FFFF:0000")
+            ha_subnet, "fd38:c341:27da:c831:0:1::",
+            "fd38:c341:27da:c831:0:1:ffff:0")
         ha_dhcp_snippets = [
             factory.make_DHCPSnippet(subnet=ha_subnet, enabled=True)
             for _ in range(3)]
@@ -1375,10 +1375,12 @@
         subnet_v4 = factory.make_ipv4_Subnet_with_IPRanges(
             vlan=vlan, unmanaged=(not dhcp_on))
         subnet_v6 = factory.make_Subnet(
-            vlan=vlan, cidr="fd38:c341:27da:c831::/64")
+            vlan=vlan, cidr="fd38:c341:27da:c831::/64",
+            gateway_ip="fd38:c341:27da:c831::1",
+            dns_servers=[])
         factory.make_IPRange(
-            subnet_v6, "fd38:c341:27da:c831::0001:0000",
-            "fd38:c341:27da:c831::FFFF:0000")
+            subnet_v6, "fd38:c341:27da:c831:0:1::",
+            "fd38:c341:27da:c831:0:1:ffff:0")
 
         factory.make_StaticIPAddress(
             alloc_type=IPADDRESS_TYPE.AUTO, subnet=subnet_v4,
@@ -1596,8 +1598,8 @@
         subnet_v6 = factory.make_Subnet(
             vlan=vlan, cidr="fd38:c341:27da:c831::/64")
         factory.make_IPRange(
-            subnet_v6, "fd38:c341:27da:c831::0001:0000",
-            "fd38:c341:27da:c831::FFFF:0000")
+            subnet_v6, "fd38:c341:27da:c831:0:1::",
+            "fd38:c341:27da:c831:0:1:ffff:0")
 
         factory.make_StaticIPAddress(
             alloc_type=IPADDRESS_TYPE.AUTO, subnet=subnet_v4,