Merge lp:~jtv/maas/dns-disable-ipv4 into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

Allow a node to have IPv4 and IPv6 addresses in DNS at the same time; suppress the IPv4 address if node has disable_ipv4 set.

Most of the work for this went into preparatory branches, so the branch you see here is simple. It allows a node to have one static IPv4 address and one static IPv6 address (even on different MACs, although I doubt that matters) and map them both in DNS.

The diff in src/maasserver/models/staticipaddress.py comes out in two chunks for me, but it may help to know that it's all in the same method: get_hostname_ip_mapping. All the relevant code was in that one SQL query there, and as luck would have it, both changes were minor. I hope the query is still clear enough to your fresh eyes. If not, let me know what you'd like me to clarify in comments.

Jeroen

[Julian Edwards (julian-edwards)]

Looks good, but you need more tests for the SQL changes, particularly all the scenarios around mixing v4/v6 and same/different MACs. I trust you will get those right since you're probably the best test writer on the team, which pains me to write this review :)

review: Approve

=== modified file 'src/maasserver/dns/tests/test_config.py'
--- src/maasserver/dns/tests/test_config.py	2014-08-15 09:44:42 +0000
+++ src/maasserver/dns/tests/test_config.py	2014-08-18 06:29:42 +0000
@@ -77,10 +77,6 @@
             ['%0.10d' % i for i in range(initial + 1, initial + 11)],
             [next_zone_serial() for i in range(initial, initial + 10)])
 
-    def patch_DEFAULT_MAAS_URL(self, hostname=None):
-        self.patch(
-            settings, 'DEFAULT_MAAS_URL', factory.make_url(netloc=hostname))
-
 
 class TestDNSServer(MAASServerTestCase):
     """A base class to perform real-world DNS-related tests.
@@ -124,8 +120,7 @@
         if nodegroup is None:
             nodegroup = self.create_managed_nodegroup()
         [interface] = nodegroup.get_managed_interfaces()
-        node = factory.make_node(
-            nodegroup=nodegroup)
+        node = factory.make_node(nodegroup=nodegroup, disable_ipv4=False)
         mac = factory.make_mac_address(node=node, cluster_interface=interface)
         ips = IPRange(
             interface.static_ip_range_low, interface.static_ip_range_high)
@@ -347,7 +342,8 @@
         nodegroup = self.create_managed_nodegroup(network=network)
         [interface] = nodegroup.get_managed_interfaces()
         node = factory.make_node(
-            nodegroup=nodegroup, status=NODE_STATUS.ALLOCATED)
+            nodegroup=nodegroup, status=NODE_STATUS.ALLOCATED,
+            disable_ipv4=False)
         mac = factory.make_mac_address(node=node, cluster_interface=interface)
         # Get an IP in the dynamic range.
         ip_range = IPRange(

=== modified file 'src/maasserver/dns/tests/test_zonegenerator.py'
--- src/maasserver/dns/tests/test_zonegenerator.py	2014-08-15 11:49:09 +0000
+++ src/maasserver/dns/tests/test_zonegenerator.py	2014-08-18 06:29:42 +0000
@@ -210,6 +210,56 @@
             {node.hostname: [staticip.ip]},
             get_hostname_ip_mapping(nodegroup))
 
+    def test_get_hostname_ip_mapping_includes_IPv4_and_IPv6_by_default(self):
+        # A node can have static IP addresses for IPv4 and IPv6 both.
+        # (It can't have DHCP leases for IPv6, since we don't parse or use
+        # those.)
+        node = factory.make_node_with_mac_attached_to_nodegroupinterface(
+            status=NODE_STATUS.ALLOCATED, disable_ipv4=False)
+        mac = node.get_primary_mac()
+        ipv4 = factory.getRandomIPAddress()
+        ipv6 = factory.make_ipv6_address()
+        factory.make_staticipaddress(mac=mac, ip=ipv4)
+        factory.make_staticipaddress(mac=mac, ip=ipv6)
+
+        self.assertItemsEqual(
+            [ipv4, ipv6],
+            get_hostname_ip_mapping(node.nodegroup)[node.hostname])
+
+    def test_get_hostname_ip_mapping_excludes_IPv4_if_disabled_on_node(self):
+        node = factory.make_node_with_mac_attached_to_nodegroupinterface(
+            status=NODE_STATUS.ALLOCATED, disable_ipv4=True)
+        mac = node.get_primary_mac()
+        ipv4 = factory.getRandomIPAddress()
+        ipv6 = factory.make_ipv6_address()
+        factory.make_staticipaddress(mac=mac, ip=ipv4)
+        factory.make_staticipaddress(mac=mac, ip=ipv6)
+
+        self.assertItemsEqual(
+            [ipv6],
+            get_hostname_ip_mapping(node.nodegroup)[node.hostname])
+
+    def test_get_hostname_ip_mapping_disables_IPv4_per_individual_node(self):
+        nodegroup = factory.make_node_group()
+        node_with_ipv4 = (
+            factory.make_node_with_mac_attached_to_nodegroupinterface(
+                nodegroup=nodegroup, status=NODE_STATUS.ALLOCATED,
+                disable_ipv4=False))
+        node_without_ipv4 = (
+            factory.make_node_with_mac_attached_to_nodegroupinterface(
+                nodegroup=nodegroup, status=NODE_STATUS.ALLOCATED,
+                disable_ipv4=True))
+        ipv4 = factory.getRandomIPAddress()
+        factory.make_staticipaddress(
+            mac=node_with_ipv4.get_primary_mac(), ip=ipv4)
+        factory.make_staticipaddress(
+            mac=node_without_ipv4.get_primary_mac())
+
+        mappings = get_hostname_ip_mapping(nodegroup)
+        # The node with IPv4 enabled gets its IPv4 address mapped.
+        # The node with IPv4 disabled gets no mapping.
+        self.assertEqual({node_with_ipv4.hostname: [ipv4]}, mappings)
+
 
 def forward_zone(domain):
     """Create a matcher for a :class:`DNSForwardZoneConfig`.

=== modified file 'src/maasserver/models/staticipaddress.py'
--- src/maasserver/models/staticipaddress.py	2014-08-16 08:53:23 +0000
+++ src/maasserver/models/staticipaddress.py	2014-08-18 06:29:42 +0000
@@ -23,6 +23,8 @@
     'StaticIPAddress',
     ]
 
+from collections import defaultdict
+
 from django.contrib.auth.models import User
 from django.db import (
     connection,
@@ -190,8 +192,13 @@
         return self._deallocate(qs)
 
     def get_hostname_ip_mapping(self, nodegroup):
-        """Return a mapping {hostnames -> ips} for current `StaticIPAddress`es
-        for the nodes in `nodegroup`.
+        """Return hostname mappings for `StaticIPAddress` entries.
+
+        Returns a mapping `{hostnames -> [ips]}` corresponding to current
+        `StaticIPAddress` objects for the nodes in `nodegroup`.
+
+        At most one IPv4 address and one IPv6 address will be returned per
+        node, each the one for whichever `MACAddress` was created first.
 
         Any domain will be stripped from the hostnames.
         """
@@ -200,8 +207,10 @@
         # DISTINCT ON returns the first matching row for any given
         # hostname, using the query's ordering.  Here, we're trying to
         # return the IP for the oldest MAC address.
+        #
+        # For nodes that have disable_ipv4 set, leave out any IPv4 address.
         cursor.execute("""
-            SELECT DISTINCT ON (node.hostname)
+            SELECT DISTINCT ON (node.hostname, family(staticip.ip))
                 node.hostname, staticip.ip
             FROM maasserver_macaddress AS mac
             JOIN maasserver_node AS node ON
@@ -210,13 +219,19 @@
                 link.mac_address_id = mac.id
             JOIN maasserver_staticipaddress AS staticip ON
                 staticip.id = link.ip_address_id
-            WHERE node.nodegroup_id = %s
-            ORDER BY node.hostname, mac.id
+            WHERE
+                node.nodegroup_id = %s AND
+                (
+                    node.disable_ipv4 IS FALSE OR
+                    family(staticip.ip) <> 4
+                )
+            ORDER BY node.hostname, family(staticip.ip), mac.id
             """, (nodegroup.id,))
-        return {
-            strip_domain(hostname): [ip]
-            for hostname, ip in cursor.fetchall()
-            }
+        mapping = defaultdict(list)
+        for hostname, ip in cursor.fetchall():
+            hostname = strip_domain(hostname)
+            mapping[hostname].append(ip)
+        return mapping
 
 
 class StaticIPAddress(CleanSave, TimestampedModel):

=== modified file 'src/maasserver/models/tests/test_staticipaddress.py'
--- src/maasserver/models/tests/test_staticipaddress.py	2014-08-16 12:08:22 +0000
+++ src/maasserver/models/tests/test_staticipaddress.py	2014-08-18 06:29:42 +0000
@@ -283,6 +283,50 @@
             node.nodegroup)
         self.assertEqual({node.hostname: [ip_for_older_mac.ip]}, mapping)
 
+    def test_get_hostname_ip_mapping_combines_IPv4_and_IPv6_addresses(self):
+        node = factory.make_node(mac=True, disable_ipv4=False)
+        mac = node.get_primary_mac()
+        ipv4_address = factory.make_staticipaddress(
+            mac=mac,
+            ip=factory.pick_ip_in_network(factory.getRandomNetwork()))
+        ipv6_address = factory.make_staticipaddress(
+            mac=mac,
+            ip=factory.pick_ip_in_network(factory.make_ipv6_network()))
+        mapping = StaticIPAddress.objects.get_hostname_ip_mapping(
+            node.nodegroup)
+        self.assertItemsEqual(
+            [ipv4_address.ip, ipv6_address.ip],
+            mapping[node.hostname])
+
+    def test_get_hostname_ip_mapping_combines_MACs_for_same_node(self):
+        # A node's preferred static IPv4 and IPv6 addresses may be on
+        # different MACs.
+        node = factory.make_node(disable_ipv4=False)
+        ipv4_address = factory.make_staticipaddress(
+            mac=factory.make_mac_address(node=node),
+            ip=factory.pick_ip_in_network(factory.getRandomNetwork()))
+        ipv6_address = factory.make_staticipaddress(
+            mac=factory.make_mac_address(node=node),
+            ip=factory.pick_ip_in_network(factory.make_ipv6_network()))
+        mapping = StaticIPAddress.objects.get_hostname_ip_mapping(
+            node.nodegroup)
+        self.assertItemsEqual(
+            [ipv4_address.ip, ipv6_address.ip],
+            mapping[node.hostname])
+
+    def test_get_hostname_ip_mapping_skips_ipv4_if_disable_ipv4_set(self):
+        node = factory.make_node(mac=True, disable_ipv4=True)
+        mac = node.get_primary_mac()
+        factory.make_staticipaddress(
+            mac=mac,
+            ip=factory.pick_ip_in_network(factory.getRandomNetwork()))
+        ipv6_address = factory.make_staticipaddress(
+            mac=mac,
+            ip=factory.pick_ip_in_network(factory.make_ipv6_network()))
+        mapping = StaticIPAddress.objects.get_hostname_ip_mapping(
+            node.nodegroup)
+        self.assertEqual({node.hostname: [ipv6_address.ip]}, mapping)
+
 
 class StaticIPAddressTest(MAASServerTestCase):