Merge lp:~mpontillo/maas/dns-dhcp-fixes--bug-1686234 into lp:~maas-committers/maas/trunk

[Mike Pontillo]

Fix automatic addition and removal of hostnames provided by DHCP.

 * Hostnames belonging to a Node can no longer be eclipsed by a dynamic hostname.
 * DNS resources already containing a static IP address can no longer be modified by DHCP lease events.
 * DNS resources are now properly removed when the last dynamic IP address associated with them is deleted.
 * Logging has been added when adding or removing a dynamic hostname.
 * Dynamic IP addresses are no longer deleted and recreated when not necessary.
 * Move the coerce_to_valid_hostname() function to the provisioningserver.utils.network module.

[Andres Rodriguez (andreserl)]

overall it looks good, I just have a few questions inline.

review: Needs Information

[Mike Pontillo (mpontillo)]

Thanks for taking a look. See replies below.

[Blake Rouse (blake-rouse)]

Looks good. Just fix the typo.

review: Approve

=== modified file 'src/maasserver/models/dnsresource.py'
--- src/maasserver/models/dnsresource.py	2016-07-07 22:42:56 +0000
+++ src/maasserver/models/dnsresource.py	2017-05-05 13:38:34 +0000
@@ -28,12 +28,18 @@
 )
 from django.db.models.query import QuerySet
 from maasserver import DefaultMeta
+from maasserver.enum import IPADDRESS_TYPE
 from maasserver.models import domain
 from maasserver.models.cleansave import CleanSave
 from maasserver.models.domain import Domain
 from maasserver.models.node import Node
 from maasserver.models.timestampedmodel import TimestampedModel
 from maasserver.utils.orm import MAASQueriesMixin
+from provisioningserver.logger import LegacyLogger
+from provisioningserver.utils.network import coerce_to_valid_hostname
+
+
+log = LegacyLogger()
 
 
 LABEL = r'[a-zA-Z0-9]([-a-zA-Z0-9]{0,62}[a-zA-Z0-9]){0,1}'
@@ -154,6 +160,65 @@
         else:
             raise PermissionDenied()
 
+    def update_dynamic_hostname(self, sip, hostname):
+        """Creates or updates the DHCP hostname for a StaticIPAddress.
+
+        The hostname will be coerced into a valid hostname before being saved,
+        since DHCP clients may report hostnames with embedded spaces, etc.
+
+        :param sip: a StaticIPAddress of alloc_type DISCOVERED
+        :param hostname: the hostname provided by the DHCP client.
+        """
+        assert sip.ip is not None
+        assert sip.alloc_type == IPADDRESS_TYPE.DISCOVERED
+        hostname = coerce_to_valid_hostname(hostname)
+        self.release_dynamic_hostname(sip, but_not_for=hostname)
+        dnsrr, created = self.get_or_create(name=hostname)
+        if created:
+            dnsrr.ip_addresses.add(sip)
+            log.msg("Added dynamic hostname '%s' for IP address '%s'." % (
+                dnsrr.fqdn, sip.ip))
+        else:
+            if dnsrr.has_static_ip():
+                log.msg(
+                    "Skipped adding dynamic hostname '%s' for IP address "
+                    "'%s': already exists in DNS with a static IP." % (
+                        dnsrr.fqdn, sip.ip))
+            else:
+                if dnsrr.ip_addresses.count() == 1:
+                    # Don't bother updating if it's already the same address.
+                    if set(dnsrr.ip_addresses.all()) == set([sip]):
+                        return
+                # One or more dynamic IPs for this hostname. Replace them.
+                dnsrr.ip_addresses.clear()
+                dnsrr.ip_addresses.add(sip)
+                log.msg(
+                    "Updated dynamic hostname '%s' for IP address '%s'." % (
+                        dnsrr.fqdn, sip.ip))
+
+    def release_dynamic_hostname(self, sip, but_not_for=None):
+        """
+        Releases the DHCP hostname for the specified StaticIPAddress.
+
+        :param sip: a StaticIPAddress of alloc_type DISCOVERED.
+        """
+        assert sip.ip is not None
+        assert sip.alloc_type == IPADDRESS_TYPE.DISCOVERED
+        resources = self.filter(
+            domain=get_default_domain(), ip_addresses__in=[sip])
+        if but_not_for is not None:
+            resources = resources.exclude(name=but_not_for)
+        for dnsrr in resources:
+            dynamic_ip = dnsrr.ip_addresses.filter(
+                alloc_type=IPADDRESS_TYPE.DISCOVERED, ip=sip.ip).first()
+            if dynamic_ip is not None:
+                dnsrr.ip_addresses.remove(dynamic_ip)
+            if dnsrr.ip_addresses.count() == 0:
+                dnsrr.delete()
+                log.msg(
+                    "Deleted dynamic hostname '%s' for IP address '%s'." % (
+                        dnsrr.fqdn, sip.ip))
+
 
 class DNSResource(CleanSave, TimestampedModel):
     """A `DNSResource`.
@@ -217,6 +282,10 @@
         else:
             return "%s.%s" % (self.name, self.domain.name)
 
+    def has_static_ip(self):
+        return self.ip_addresses.exclude(
+            alloc_type=IPADDRESS_TYPE.DISCOVERED).exists()
+
     def get_addresses(self):
         """Return all addresses associated with this FQDN."""
         # Since Node.hostname is unique, this will be at most 1 node.

=== modified file 'src/maasserver/models/signals/staticipaddress.py'
--- src/maasserver/models/signals/staticipaddress.py	2017-04-07 21:52:35 +0000
+++ src/maasserver/models/signals/staticipaddress.py	2017-05-05 13:38:34 +0000
@@ -16,12 +16,14 @@
 )
 from maasserver.models import StaticIPAddress
 from maasserver.utils.signals import SignalsManager
-
-
+from provisioningserver.logger import LegacyLogger
+
+
+log = LegacyLogger()
 signals = SignalsManager()
 
 
-def pre_delete_record_bmcs_on_delete(sender, instance, **kwargs):
+def pre_delete_record_relations_on_delete(sender, instance, **kwargs):
     """Store the instance's bmc_set for use in post_delete.
 
     It is coerced to a set to force it to be evaluated here in the pre_delete
@@ -33,9 +35,10 @@
     can be called on in post_delete to make their own StaticIPAddresses.
     """
     instance.__previous_bmcs = set(instance.bmc_set.all())
+    instance.__previous_dnsresources = set(instance.dnsresource_set.all())
 
 signals.watch(
-    pre_delete, pre_delete_record_bmcs_on_delete, sender=StaticIPAddress)
+    pre_delete, pre_delete_record_relations_on_delete, sender=StaticIPAddress)
 
 
 def post_delete_remake_sip_for_bmc(sender, instance, **kwargs):
@@ -59,6 +62,21 @@
     post_delete, post_delete_remake_sip_for_bmc, sender=StaticIPAddress)
 
 
+def post_delete_clean_up_dns(sender, instance, **kwargs):
+    """Now that the StaticIPAddress instance is gone, check if any related
+    DNS records should be deleted.
+    """
+    for dnsrr in instance.__previous_dnsresources:
+        if dnsrr.ip_addresses.count() == 0:
+            dnsrr.delete()
+            log.msg(
+                "Removed orphan DNS record '%s' for deleted IP address '%s'."
+                % (dnsrr.fqdn, instance.ip))
+
+signals.watch(
+    post_delete, post_delete_clean_up_dns, sender=StaticIPAddress)
+
+
 def post_init_store_previous_ip(sender, instance, **kwargs):
     """Store the pre_save IP address of the instance.
 

=== modified file 'src/maasserver/models/signals/tests/test_staticipaddress.py'
--- src/maasserver/models/signals/tests/test_staticipaddress.py	2016-03-10 17:54:26 +0000
+++ src/maasserver/models/signals/tests/test_staticipaddress.py	2017-05-05 13:38:34 +0000
@@ -4,8 +4,11 @@
 """
 Test the behaviour of staticipaddress signals.
 
-These signals are tested in src/maasserver/models/tests/test_bmc.py. They
-handle IP sharing issues between BMCs and Machines.
+Tests for sharing IP addresses between BMCs and machines are in:
+    src/maasserver/models/tests/test_bmc.py
+
+Tests related to DNS resources are in:
+    src/maasserver/models/tests/test_dnsresource.py
 """
 
 __all__ = []

=== modified file 'src/maasserver/models/tests/test_dnsresource.py'
--- src/maasserver/models/tests/test_dnsresource.py	2016-07-07 22:42:56 +0000
+++ src/maasserver/models/tests/test_dnsresource.py	2017-05-05 13:38:34 +0000
@@ -5,22 +5,36 @@
 
 __all__ = []
 
-
+from datetime import (
+    datetime,
+    timedelta,
+)
 import re
 
 from django.core.exceptions import (
     PermissionDenied,
     ValidationError,
 )
-from maasserver.enum import NODE_PERMISSION
+from maasserver.enum import (
+    IPADDRESS_TYPE,
+    NODE_PERMISSION,
+)
+from maasserver.models import StaticIPAddress
 from maasserver.models.dnsresource import (
     DNSResource,
     separate_fqdn,
 )
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
+from maasserver.utils.orm import reload_object
 from testtools import ExpectedException
-from testtools.matchers import MatchesStructure
+from testtools.matchers import (
+    Contains,
+    Equals,
+    Is,
+    MatchesStructure,
+    Not,
+)
 
 
 class TestDNSResourceManagerGetDNSResourceOr404(MAASServerTestCase):
@@ -241,3 +255,134 @@
         self.assertItemsEqual(
             (sip1.get_ip(), sip2.get_ip()),
             dnsresource.get_addresses())
+
+
+class TestUpdateDynamicHostname(MAASServerTestCase):
+
+    def test__adds_new_hostname(self):
+        sip = factory.make_StaticIPAddress(
+            ip='10.0.0.1',
+            alloc_type=IPADDRESS_TYPE.DISCOVERED)
+        hostname = factory.make_name().lower()
+        DNSResource.objects.update_dynamic_hostname(sip, hostname)
+        dnsrr = DNSResource.objects.get(name=hostname)
+        self.assertThat(dnsrr.ip_addresses.all(), Contains(sip))
+
+    def test__coerces_to_valid_hostname(self):
+        sip = factory.make_StaticIPAddress(
+            ip='10.0.0.1',
+            alloc_type=IPADDRESS_TYPE.DISCOVERED)
+        hostname = "no tea"
+        DNSResource.objects.update_dynamic_hostname(sip, hostname)
+        dnsrr = DNSResource.objects.get(name="no-tea")
+        self.assertThat(dnsrr.ip_addresses.all(), Contains(sip))
+
+    def test__does_not_modify_existing_non_dynamic_records(self):
+        sip_reserved = factory.make_StaticIPAddress(
+            ip='10.0.0.1',
+            alloc_type=IPADDRESS_TYPE.USER_RESERVED)
+        hostname = factory.make_name().lower()
+        factory.make_DNSResource(name=hostname, ip_addresses=[sip_reserved])
+        sip_dynamic = factory.make_StaticIPAddress(
+            ip='10.0.0.2',
+            alloc_type=IPADDRESS_TYPE.DISCOVERED)
+        DNSResource.objects.update_dynamic_hostname(sip_dynamic, hostname)
+        dnsrr = DNSResource.objects.get(name=hostname)
+        self.assertThat(dnsrr.ip_addresses.all(), Contains(sip_reserved))
+        self.assertThat(dnsrr.ip_addresses.all(), Not(Contains(sip_dynamic)))
+
+    def test__updates_existing_dynamic_record(self):
+        sip_before = factory.make_StaticIPAddress(
+            ip='10.0.0.1',
+            alloc_type=IPADDRESS_TYPE.DISCOVERED)
+        hostname = factory.make_name().lower()
+        DNSResource.objects.update_dynamic_hostname(sip_before, hostname)
+        sip_after = factory.make_StaticIPAddress(
+            ip='10.0.0.2',
+            alloc_type=IPADDRESS_TYPE.DISCOVERED)
+        DNSResource.objects.update_dynamic_hostname(sip_after, hostname)
+        dnsrr = DNSResource.objects.get(name=hostname)
+        self.assertThat(dnsrr.ip_addresses.all(), Contains(sip_after))
+        self.assertThat(dnsrr.ip_addresses.all(), Not(Contains(sip_before)))
+
+    def test__skips_updating_identical_ip(self):
+        sip = factory.make_StaticIPAddress(
+            ip='10.0.0.1',
+            alloc_type=IPADDRESS_TYPE.DISCOVERED)
+        hostname = factory.make_name().lower()
+        DNSResource.objects.update_dynamic_hostname(sip, hostname)
+        dnsrr = DNSResource.objects.get(name=hostname)
+        # Create a date object for one week ago.
+        before = datetime.fromtimestamp(
+            datetime.now().timestamp() - timedelta(days=7).total_seconds())
+        dnsrr.save(_created=before, _updated=before)
+        dnsrr = DNSResource.objects.get(name=hostname)
+        self.assertThat(dnsrr.updated, Equals(before))
+        self.assertThat(dnsrr.created, Equals(before))
+        # Test that the timestamps weren't updated after updating again.
+        DNSResource.objects.update_dynamic_hostname(sip, hostname)
+        dnsrr = DNSResource.objects.get(name=hostname)
+        self.assertThat(dnsrr.updated, Equals(before))
+        self.assertThat(dnsrr.created, Equals(before))
+
+    def test__update_releases_obsolete_hostnames(self):
+        sip = factory.make_StaticIPAddress(
+            ip='10.0.0.1',
+            alloc_type=IPADDRESS_TYPE.DISCOVERED)
+        hostname_old = factory.make_name().lower()
+        DNSResource.objects.update_dynamic_hostname(sip, hostname_old)
+        hostname_new = factory.make_name().lower()
+        DNSResource.objects.update_dynamic_hostname(sip, hostname_new)
+        dnsrr = DNSResource.objects.get(name=hostname_new)
+        self.assertThat(dnsrr.ip_addresses.all(), Contains(sip))
+        self.assertThat(
+            DNSResource.objects.filter(name=hostname_old).first(),
+            Equals(None))
+
+
+class TestReleaseDynamicHostname(MAASServerTestCase):
+
+    def test__releases_dynamic_hostname(self):
+        sip = factory.make_StaticIPAddress(
+            ip='10.0.0.1',
+            alloc_type=IPADDRESS_TYPE.DISCOVERED)
+        hostname = factory.make_name().lower()
+        DNSResource.objects.update_dynamic_hostname(sip, hostname)
+        DNSResource.objects.release_dynamic_hostname(sip)
+        self.assertThat(
+            DNSResource.objects.filter(name=hostname).first(),
+            Equals(None))
+
+    def test__leaves_static_hostnames_untouched(self):
+        sip = factory.make_StaticIPAddress(
+            ip='10.0.0.1',
+            alloc_type=IPADDRESS_TYPE.DISCOVERED)
+        hostname = factory.make_name().lower()
+        DNSResource.objects.update_dynamic_hostname(sip, hostname)
+        sip_reserved = factory.make_StaticIPAddress(
+            ip='10.0.0.2',
+            alloc_type=IPADDRESS_TYPE.USER_RESERVED)
+        dnsrr = DNSResource.objects.get(name=hostname)
+        dnsrr.ip_addresses.add(sip_reserved)
+        DNSResource.objects.release_dynamic_hostname(sip)
+        dnsrr = reload_object(dnsrr)
+        self.assertThat(dnsrr.ip_addresses.all(), Contains(sip_reserved))
+        self.assertThat(dnsrr.ip_addresses.all(), Not(Contains(sip)))
+
+
+class TestStaticIPAddressSignals(MAASServerTestCase):
+    """Tests the signals signals/staticipaddress.py."""
+
+    def test__deletes_orphaned_record(self):
+        dnsrr = factory.make_DNSResource()
+        StaticIPAddress.objects.all().delete()
+        dnsrr = reload_object(dnsrr)
+        self.assertThat(dnsrr, Is(None))
+
+    def test__non_orphaned_record_not_deleted(self):
+        dnsrr = factory.make_DNSResource(ip_addresses=['8.8.8.8', '8.8.4.4'])
+        sip = StaticIPAddress.objects.get(ip='8.8.4.4')
+        sip.delete()
+        dnsrr = reload_object(dnsrr)
+        sip = StaticIPAddress.objects.get(ip='8.8.8.8')
+        self.assertThat(dnsrr.ip_addresses.all(), Contains(sip))

=== modified file 'src/maasserver/rpc/leases.py'
--- src/maasserver/rpc/leases.py	2016-03-28 13:54:47 +0000
+++ src/maasserver/rpc/leases.py	2017-05-05 13:38:34 +0000
@@ -13,15 +13,17 @@
     IPADDRESS_FAMILY,
     IPADDRESS_TYPE,
 )
-from maasserver.models.dnsresource import DNSResource
-from maasserver.models.interface import (
+from maasserver.models import (
+    DNSResource,
     Interface,
+    Node,
+    StaticIPAddress,
+    Subnet,
     UnknownInterface,
 )
-from maasserver.models.staticipaddress import StaticIPAddress
-from maasserver.models.subnet import Subnet
 from maasserver.utils.orm import transactional
 from netaddr import IPAddress
+from provisioningserver.utils.network import coerce_to_valid_hostname
 from provisioningserver.utils.twisted import synchronous
 
 
@@ -102,6 +104,7 @@
         # No interfaces and not commit action so nothing needs to be done.
         return {}
 
+    sip = None
     # Delete all discovered IP addresses attached to all interfaces of the same
     # IP address family.
     old_family_addresses = StaticIPAddress.objects.filter_by_ip_family(
@@ -109,7 +112,15 @@
     old_family_addresses = old_family_addresses.filter(
         alloc_type=IPADDRESS_TYPE.DISCOVERED,
         interface__in=interfaces)
-    old_family_addresses.delete()
+    for address in old_family_addresses:
+        # Release old DHCP hostnames, but only for obsolete dynamic addresses.
+        if address.ip != ip:
+            if address.ip is not None:
+                DNSResource.objects.release_dynamic_hostname(address)
+            address.delete()
+        else:
+            # Avoid recreating a new StaticIPAddress later.
+            sip = address
 
     # Create the new StaticIPAddress object based on the action.
     if action == "commit":
@@ -129,21 +140,35 @@
         # object. That will make sure that the lease_time is correct from
         # the created time.
         created = datetime.fromtimestamp(timestamp)
-        sip = StaticIPAddress.objects.create(
-            alloc_type=IPADDRESS_TYPE.DISCOVERED, ip=ip, subnet=subnet,
-            lease_time=lease_time, created=created, updated=created)
+        sip, _ = StaticIPAddress.objects.update_or_create(
+            defaults=dict(
+                subnet=subnet, lease_time=lease_time,
+                created=created, updated=created),
+            alloc_type=IPADDRESS_TYPE.DISCOVERED, ip=ip)
         for interface in interfaces:
             interface.ip_addresses.add(sip)
         if sip_hostname is not None:
-            dnsrr, created = DNSResource.objects.get_or_create(
-                name=sip_hostname)
-            dnsrr.ip_addresses.add(sip)
+            # MAAS automatically manages DNS for node hostnames, so we cannot
+            # allow a DHCP client to override that.
+            hostname_belongs_to_a_node = (
+                Node.objects.filter(
+                    hostname=coerce_to_valid_hostname(sip_hostname)).exists()
+            )
+            if hostname_belongs_to_a_node:
+                # Ensure we don't allow a DHCP hostname to override a node
+                # hostname.
+                DNSResource.objects.release_dynamic_hostname(sip)
+            else:
+                DNSResource.objects.update_dynamic_hostname(sip, sip_hostname)
     elif action == "expiry" or action == "release":
         # Interfaces no longer holds an active lease. Create the new object
         # to show that it used to be connected to this subnet.
-        sip = StaticIPAddress.objects.create(
-            alloc_type=IPADDRESS_TYPE.DISCOVERED, ip=None, subnet=subnet)
+        if sip is None:
+            sip = StaticIPAddress.objects.create(
+                alloc_type=IPADDRESS_TYPE.DISCOVERED, ip=None, subnet=subnet)
+        else:
+            sip.ip = None
+            sip.save()
         for interface in interfaces:
             interface.ip_addresses.add(sip)
-
     return {}

=== modified file 'src/maasserver/rpc/tests/test_leases.py'
--- src/maasserver/rpc/tests/test_leases.py	2016-03-28 13:54:47 +0000
+++ src/maasserver/rpc/tests/test_leases.py	2017-05-05 13:38:34 +0000
@@ -14,6 +14,7 @@
     IPADDRESS_FAMILY,
     IPADDRESS_TYPE,
 )
+from maasserver.models import DNSResource
 from maasserver.models.interface import UnknownInterface
 from maasserver.models.staticipaddress import StaticIPAddress
 from maasserver.rpc.leases import (
@@ -22,9 +23,17 @@
 )
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
-from maasserver.utils.orm import reload_object
+from maasserver.utils.orm import (
+    get_one,
+    reload_object,
+)
 from netaddr import IPAddress
-from testtools.matchers import MatchesStructure
+from testtools.matchers import (
+    Contains,
+    Equals,
+    MatchesStructure,
+    Not,
+)
 
 
 class TestUpdateLease(MAASServerTestCase):
@@ -140,13 +149,14 @@
             updated=datetime.fromtimestamp(kwargs["timestamp"]),
         ))
 
-    def test_creates_ignores_none_hostname(self):
+    def test_create_ignores_none_hostname(self):
         subnet = factory.make_ipv4_Subnet_with_IPRanges(
             with_static_range=False, dhcp_on=True)
         dynamic_range = subnet.get_dynamic_ranges()[0]
         ip = factory.pick_ip_in_IPRange(dynamic_range)
+        hostname = "(none)"
         kwargs = self.make_kwargs(
-            action="commit", ip=ip, hostname="(none)")
+            action="commit", ip=ip, hostname=hostname)
         update_lease(**kwargs)
         unknown_interface = UnknownInterface.objects.filter(
             mac_address=kwargs["mac"]).first()
@@ -162,6 +172,76 @@
             created=datetime.fromtimestamp(kwargs["timestamp"]),
             updated=datetime.fromtimestamp(kwargs["timestamp"]),
         ))
+        # No DNS record should have been crated.
+        self.assertThat(DNSResource.objects.count(), Equals(0))
+
+    def test_creates_dns_record_for_hostname(self):
+        subnet = factory.make_ipv4_Subnet_with_IPRanges(
+            with_static_range=False, dhcp_on=True)
+        dynamic_range = subnet.get_dynamic_ranges()[0]
+        ip = factory.pick_ip_in_IPRange(dynamic_range)
+        hostname = factory.make_name().lower()
+        kwargs = self.make_kwargs(
+            action="commit", ip=ip, hostname=hostname)
+        update_lease(**kwargs)
+        unknown_interface = UnknownInterface.objects.filter(
+            mac_address=kwargs["mac"]).first()
+        self.assertIsNotNone(unknown_interface)
+        self.assertEquals(subnet.vlan, unknown_interface.vlan)
+        sip = unknown_interface.ip_addresses.first()
+        self.assertIsNotNone(sip)
+        dnsrr = get_one(DNSResource.objects.filter(name=hostname))
+        self.assertThat(sip.dnsresource_set.all(), Contains(dnsrr))
+
+    def test_mutiple_calls_reuse_existing_staticipaddress_records(self):
+        subnet = factory.make_ipv4_Subnet_with_IPRanges(
+            with_static_range=False, dhcp_on=True)
+        dynamic_range = subnet.get_dynamic_ranges()[0]
+        ip = factory.pick_ip_in_IPRange(dynamic_range)
+        hostname = factory.make_name().lower()
+        kwargs = self.make_kwargs(
+            action="commit", ip=ip, hostname=hostname)
+        update_lease(**kwargs)
+        sip1 = StaticIPAddress.objects.get(ip=ip)
+        update_lease(**kwargs)
+        sip2 = StaticIPAddress.objects.get(ip=ip)
+        self.assertThat(sip1.id, Equals(sip2.id))
+
+    def test_skips_dns_record_for_hostname_from_existing_node(self):
+        subnet = factory.make_ipv4_Subnet_with_IPRanges(
+            with_static_range=False, dhcp_on=True)
+        dynamic_range = subnet.get_dynamic_ranges()[0]
+        ip = factory.pick_ip_in_IPRange(dynamic_range)
+        hostname = factory.make_name().lower()
+        factory.make_Node(hostname=hostname)
+        kwargs = self.make_kwargs(
+            action="commit", ip=ip, hostname=hostname)
+        update_lease(**kwargs)
+        unknown_interface = UnknownInterface.objects.filter(
+            mac_address=kwargs["mac"]).first()
+        self.assertIsNotNone(unknown_interface)
+        self.assertEquals(subnet.vlan, unknown_interface.vlan)
+        sip = unknown_interface.ip_addresses.first()
+        self.assertIsNotNone(sip)
+        self.assertThat(sip.dnsresource_set.all(), Not(Contains(sip)))
+
+    def test_skips_dns_record_for_coerced_hostname_from_existing_node(self):
+        subnet = factory.make_ipv4_Subnet_with_IPRanges(
+            with_static_range=False, dhcp_on=True)
+        dynamic_range = subnet.get_dynamic_ranges()[0]
+        ip = factory.pick_ip_in_IPRange(dynamic_range)
+        hostname = "gaming device"
+        factory.make_Node(hostname="gaming-device")
+        kwargs = self.make_kwargs(
+            action="commit", ip=ip, hostname=hostname)
+        update_lease(**kwargs)
+        unknown_interface = UnknownInterface.objects.filter(
+            mac_address=kwargs["mac"]).first()
+        self.assertIsNotNone(unknown_interface)
+        self.assertEquals(subnet.vlan, unknown_interface.vlan)
+        sip = unknown_interface.ip_addresses.first()
+        self.assertIsNotNone(sip)
+        self.assertThat(sip.dnsresource_set.all(), Not(Contains(sip)))
 
     def test_creates_lease_for_physical_interface(self):
         subnet = factory.make_ipv4_Subnet_with_IPRanges(

=== modified file 'src/maasserver/testing/factory.py'
--- src/maasserver/testing/factory.py	2017-04-11 16:49:44 +0000
+++ src/maasserver/testing/factory.py	2017-05-05 13:38:34 +0000
@@ -647,6 +647,14 @@
             name = self.make_name('label')
         if ip_addresses is None and not no_ip_addresses:
             ip_addresses = [self.make_StaticIPAddress(**kwargs)]
+        elif isinstance(ip_addresses, list):
+            ip_addresses = [
+                self.make_StaticIPAddress(
+                    ip=ip, alloc_type=IPADDRESS_TYPE.USER_RESERVED)
+                if not isinstance(ip, StaticIPAddress)
+                else ip
+                for ip in ip_addresses
+            ]
         dnsrr = DNSResource(
             name=name, address_ttl=address_ttl,
             domain=domain)
@@ -857,7 +865,8 @@
             # that in ages past (?) MAAS did not require a network to be
             # modelled in order to add a USER_RESERVED address, and there are
             # most likely test cases that depend on this behavior.
-            if subnet is None and alloc_type != IPADDRESS_TYPE.USER_RESERVED:
+            if (ip is undefined and subnet is None and
+                    alloc_type != IPADDRESS_TYPE.USER_RESERVED):
                 subnet = self.make_Subnet(vlan=vlan)
         else:
             if vlan is None:
@@ -872,7 +881,7 @@
             # hints as to why we think this behaviour exists.
             if not subnet and alloc_type == IPADDRESS_TYPE.USER_RESERVED:
                 ip = self.make_ip_address()
-            else:
+            elif subnet is not None:
                 ip = self.pick_ip_in_network(
                     IPNetwork(subnet.cidr),
                     but_not=self._get_exclude_list(subnet))

=== modified file 'src/provisioningserver/rpc/tests/test_utils.py'
--- src/provisioningserver/rpc/tests/test_utils.py	2016-10-18 11:55:44 +0000
+++ src/provisioningserver/rpc/tests/test_utils.py	2017-05-05 13:38:34 +0000
@@ -24,7 +24,6 @@
 from provisioningserver.rpc.testing import MockLiveClusterToRegionRPCFixture
 import provisioningserver.rpc.utils
 from provisioningserver.rpc.utils import (
-    coerce_to_valid_hostname,
     commission_node,
     create_node,
 )
@@ -32,29 +31,6 @@
 from twisted.internet import defer
 
 
-class TestCoerceHostname(MAASTestCase):
-
-    def test_replaces_international_characters(self):
-        self.assertEqual("abc-123", coerce_to_valid_hostname("abc青い空123"))
-
-    def test_removes_illegal_dashes(self):
-        self.assertEqual("abc123", coerce_to_valid_hostname("-abc123-"))
-
-    def test_replaces_whitespace_and_special_characters(self):
-        self.assertEqual(
-            "abc123-ubuntu", coerce_to_valid_hostname("abc123 (ubuntu)"))
-
-    def test_makes_hostname_lowercase(self):
-        self.assertEqual(
-            "ubunturocks", coerce_to_valid_hostname("UbuntuRocks"))
-
-    def test_returns_none_if_result_empty(self):
-        self.assertIsNone(coerce_to_valid_hostname("-人間性-"))
-
-    def test_returns_none_if_result_too_large(self):
-        self.assertIsNone(coerce_to_valid_hostname('a' * 65))
-
-
 class TestCreateNode(MAASTestCase):
 
     run_tests_with = MAASTwistedRunTest.make_factory(timeout=5)

=== modified file 'src/provisioningserver/rpc/utils.py'
--- src/provisioningserver/rpc/utils.py	2016-10-25 13:57:02 +0000
+++ src/provisioningserver/rpc/utils.py	2017-05-05 13:38:34 +0000
@@ -9,7 +9,6 @@
 ]
 
 import json
-import re
 
 from provisioningserver.logger import get_maas_logger
 from provisioningserver.rpc import getRegionClient
@@ -19,6 +18,7 @@
     NodeAlreadyExists,
 )
 from provisioningserver.rpc.region import CreateNode
+from provisioningserver.utils.network import coerce_to_valid_hostname
 from provisioningserver.utils.twisted import (
     asynchronous,
     pause,
@@ -38,21 +38,6 @@
 maaslog = get_maas_logger("region")
 
 
-def coerce_to_valid_hostname(hostname):
-    """Given a server name that may contain spaces and special characters,
-    attempts to derive a valid hostname.
-
-    :param hostname: the specified (possibly invalid) hostname
-    :return: the resulting string, or None if the hostname could not be coerced
-    """
-    hostname = hostname.lower()
-    hostname = re.sub(r'[^a-z0-9-]+', '-', hostname)
-    hostname = hostname.strip('-')
-    if hostname == '' or len(hostname) > 64:
-        return None
-    return hostname
-
-
 @asynchronous
 @inlineCallbacks
 def create_node(

=== modified file 'src/provisioningserver/utils/network.py'
--- src/provisioningserver/utils/network.py	2017-04-17 23:17:25 +0000
+++ src/provisioningserver/utils/network.py	2017-05-05 13:38:34 +0000
@@ -22,6 +22,7 @@
 import codecs
 from collections import namedtuple
 from operator import attrgetter
+import re
 import socket
 from socket import (
     AF_INET,
@@ -1305,3 +1306,18 @@
     else:
         return results
     return None
+
+
+def coerce_to_valid_hostname(hostname):
+    """Given a server name that may contain spaces and special characters,
+    attempts to derive a valid hostname.
+
+    :param hostname: the specified (possibly invalid) hostname
+    :return: the resulting string, or None if the hostname could not be coerced
+    """
+    hostname = hostname.lower()
+    hostname = re.sub(r'[^a-z0-9-]+', '-', hostname)
+    hostname = hostname.strip('-')
+    if hostname == '' or len(hostname) > 64:
+        return None
+    return hostname

=== modified file 'src/provisioningserver/utils/tests/test_network.py'
--- src/provisioningserver/utils/tests/test_network.py	2017-04-17 23:17:25 +0000
+++ src/provisioningserver/utils/tests/test_network.py	2017-05-05 13:38:34 +0000
@@ -45,6 +45,7 @@
     bytes_to_hex,
     bytes_to_int,
     clean_up_netifaces_address,
+    coerce_to_valid_hostname,
     find_ip_via_arp,
     find_mac_via_arp,
     format_eui,
@@ -2095,3 +2096,26 @@
         self.lookupPointer.side_effect = TypeError
         with ExpectedException(TypeError):
             yield reverseResolve(factory.make_ip_address())
+
+
+class TestCoerceHostname(MAASTestCase):
+
+    def test_replaces_international_characters(self):
+        self.assertEqual("abc-123", coerce_to_valid_hostname("abc青い空123"))
+
+    def test_removes_illegal_dashes(self):
+        self.assertEqual("abc123", coerce_to_valid_hostname("-abc123-"))
+
+    def test_replaces_whitespace_and_special_characters(self):
+        self.assertEqual(
+            "abc123-ubuntu", coerce_to_valid_hostname("abc123 (ubuntu)"))
+
+    def test_makes_hostname_lowercase(self):
+        self.assertEqual(
+            "ubunturocks", coerce_to_valid_hostname("UbuntuRocks"))
+
+    def test_returns_none_if_result_empty(self):
+        self.assertIsNone(coerce_to_valid_hostname("-人間性-"))
+
+    def test_returns_none_if_result_too_large(self):
+        self.assertIsNone(coerce_to_valid_hostname('a' * 65))