Merge lp:~jtv/maas/clean-up-networking_preseed into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

Simplify much-used data structure in networking_preseed: instead of defaultdict-of-sets-of-IPAddress, just work with dict-of-sets-of-unicode, with the unicode strings containing normalised IP addresses.

This will simplify the job of generating a node's networking configuration data server-side. These data structures need to be sent over RPC, so simple is good.

Jeroen

[Gavin Panella (allenap)]

Looks good. One little mini-rant about setdefault, but that's all.

review: Approve

[Jeroen T. Vermeulen (jtv)]

Thanks. I do agree in the general case. Here, it was the intermediate JSON step for RPC that made me do it. I wanted to avoid as much as I could any subtle differences between the same data structure on the region side and the cluster side. (And yes I do feel slightly awkward about using set() for that reason, but that seemed more effort to remove.)

=== modified file 'src/maasserver/networking_preseed.py'
--- src/maasserver/networking_preseed.py	2014-09-30 04:57:45 +0000
+++ src/maasserver/networking_preseed.py	2014-10-01 06:35:17 +0000
@@ -28,8 +28,6 @@
     'generate_networking_config',
     ]
 
-from collections import defaultdict
-
 from lxml import etree
 from maasserver.dns.zonegenerator import get_dns_server_address
 from maasserver.exceptions import UnresolvableHost
@@ -38,6 +36,7 @@
 from netaddr import (
     IPAddress,
     IPNetwork,
+    valid_ipv4,
     )
 
 
@@ -68,6 +67,14 @@
     return mac.strip().lower()
 
 
+def normalise_ip(ip):
+    """Return an IP address' normalised representation.
+
+    IPv6 addresses in particular can have many different spellings.
+    """
+    return unicode(IPAddress(ip.strip()))
+
+
 def extract_network_interfaces(node):
     """Extract network interfaces from node's `lshw` output.
 
@@ -152,7 +159,7 @@
     if cluster_interface.router_ip in ('', None):
         # No routes available.
         return []
-    elif IPAddress(cluster_interface.ip).version == 4:
+    elif valid_ipv4(cluster_interface.ip):
         return [
             {
                 'network': '0.0.0.0',
@@ -252,28 +259,25 @@
 def add_ip_to_mapping(mapping, macaddress, ip):
     """Add IP address to a `defaultdict` keyed by MAC string.
 
-    :param mapping: A `defaultdict` (defaulting to the empty set) mapping
-        normalised MAC address strings to sets of `IPAddress`.
+    :param mapping: A `dict` mapping normalised MAC address strings to sets of
+        normalised IP address strings.
     :param macaddress: A `MACAddress`.
     :param ip: An IP address string.  If it is empty or `None`, it will not
         be added to the mapping.
     """
-    if ip in (None, ''):
-        return
-    assert isinstance(ip, (unicode, bytes, IPAddress))
-    mac = extract_mac_string(macaddress)
-    ip = IPAddress(ip)
-    mapping[mac].add(ip)
+    if ip not in (None, ''):
+        mac = extract_mac_string(macaddress)
+        mapping.setdefault(mac, set()).add(normalise_ip(ip))
 
 
 def map_static_ips(node):
     """Return a `defaultdict` mapping node's MAC addresses to their static IPs.
 
     :param node: A `Node`.
-    :return: A `defaultdict` (defaulting to the empty set) mapping normalised
-        MAC address strings to sets of `IPAddress`.
+    :return: A dict mapping normalised MAC address strings to sets of
+        normalised IP address strings.
     """
-    mapping = defaultdict(set)
+    mapping = {}
     for sip in StaticIPAddress.objects.filter(macaddress__node=node):
         for mac in sip.macaddress_set.all():
             add_ip_to_mapping(mapping, mac, sip.ip)
@@ -284,10 +288,10 @@
     """Return a `defaultdict` mapping node's MAC addresses to their gateways.
 
     :param node: A `Node`.
-    :return: A `defaultdict` (defaulting to the empty set) mapping normalised
-        MAC address strings to sets of `IPAddress`.
+    :return: A dict mapping normalised MAC address strings to sets of
+        normalised IP address strings.
     """
-    mapping = defaultdict(set)
+    mapping = {}
     for mac in node.macaddress_set.all():
         for cluster_interface in mac.get_cluster_interfaces():
             if cluster_interface.manages_static_range():

=== modified file 'src/maasserver/tests/test_networking_preseed.py'
--- src/maasserver/tests/test_networking_preseed.py	2014-09-30 04:57:45 +0000
+++ src/maasserver/tests/test_networking_preseed.py	2014-10-01 06:35:17 +0000
@@ -15,7 +15,6 @@
 __all__ = [
     ]
 
-from collections import defaultdict
 from random import randint
 
 from maasserver import networking_preseed
@@ -37,12 +36,12 @@
     list_dns_servers,
     map_gateways,
     map_static_ips,
+    normalise_ip,
     normalise_mac,
     )
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
 from maastesting.matchers import MockCalledOnceWith
-from netaddr import IPAddress
 from testtools.matchers import HasLength
 
 
@@ -222,6 +221,30 @@
             normalise_mac(normalise_mac(mac)))
 
 
+class TestNormaliseIP(MAASServerTestCase):
+
+    def test__normalises_case(self):
+        ip = factory.make_ipv6_address()
+        self.assertEqual(
+            normalise_ip(ip.upper()),
+            normalise_ip(ip.lower()))
+
+    def test__strips_whitespace(self):
+        ip = factory.make_ipv4_address()
+        self.assertEqual(ip, normalise_ip(' %s ' % ip))
+
+    def test__normalises_zeroes(self):
+        self.assertEqual('::1', normalise_ip('0000:000:00:0::1'))
+
+    def test__accepts_bytes(self):
+        ip = factory.make_ipv6_address()
+        self.assertEqual(normalise_ip(ip), normalise_ip(ip.encode('ascii')))
+
+    def test__is_idempotent(self):
+        ip = factory.make_ipv6_address()
+        self.assertEqual(normalise_ip(ip), normalise_ip(normalise_ip(ip)))
+
+
 class TestGenerateEthernetLinkEntry(MAASServerTestCase):
 
     def test__generates_dict(self):
@@ -517,38 +540,31 @@
 
 class TestAddIPToMapping(MAASServerTestCase):
 
-    def make_mapping(self):
-        return defaultdict(set)
-
     def test__adds_to_empty_entry(self):
-        mapping = self.make_mapping()
         mac = factory.make_MACAddress()
         ip = factory.make_ipv4_address()
+        mapping = {}
         add_ip_to_mapping(mapping, mac, ip)
-        self.assertEqual(
-            {mac.mac_address: {IPAddress(ip)}},
-            mapping)
+        self.assertEqual({mac.mac_address: {ip}}, mapping)
 
     def test__adds_to_nonempty_entry(self):
-        mapping = self.make_mapping()
+        mapping = {}
         mac = factory.make_MACAddress()
         ip1 = factory.make_ipv4_address()
         add_ip_to_mapping(mapping, mac, ip1)
         ip2 = factory.make_ipv4_address()
         add_ip_to_mapping(mapping, mac, ip2)
-        self.assertEqual(
-            {mac.mac_address: {IPAddress(ip1), IPAddress(ip2)}},
-            mapping)
+        self.assertEqual({mac.mac_address: {ip1, ip2}}, mapping)
 
     def test__does_not_add_None(self):
-        mapping = self.make_mapping()
         mac = factory.make_MACAddress()
+        mapping = {}
         add_ip_to_mapping(mapping, mac, None)
         self.assertEqual({}, mapping)
 
     def test__does_not_add_empty_string(self):
-        mapping = self.make_mapping()
         mac = factory.make_MACAddress()
+        mapping = {}
         add_ip_to_mapping(mapping, mac, '')
         self.assertEqual({}, mapping)
 
@@ -564,7 +580,7 @@
         ip = factory.make_ipv4_address()
         factory.make_StaticIPAddress(ip=ip, mac=mac)
         self.assertEqual(
-            {mac.mac_address: {IPAddress(ip)}},
+            {mac.mac_address: {ip}},
             map_static_ips(node))
 
     def test__finds_IPv6_address(self):
@@ -573,7 +589,7 @@
         ip = factory.make_ipv6_address()
         factory.make_StaticIPAddress(ip=ip, mac=mac)
         self.assertEqual(
-            {mac.mac_address: {IPAddress(ip)}},
+            {mac.mac_address: {ip}},
             map_static_ips(node))
 
     def test__finds_addresses_on_multiple_MACs(self):
@@ -586,8 +602,8 @@
         factory.make_StaticIPAddress(ip=ip2, mac=mac2)
         self.assertEqual(
             {
-                mac1.mac_address: {IPAddress(ip1)},
-                mac2.mac_address: {IPAddress(ip2)},
+                mac1.mac_address: {ip1},
+                mac2.mac_address: {ip2},
             },
             map_static_ips(node))
 
@@ -599,7 +615,7 @@
         factory.make_StaticIPAddress(ip=ipv4, mac=mac)
         factory.make_StaticIPAddress(ip=ipv6, mac=mac)
         self.assertEqual(
-            {mac.mac_address: {IPAddress(ipv4), IPAddress(ipv6)}},
+            {mac.mac_address: {ipv4, ipv6}},
             map_static_ips(node))
 
 
@@ -620,7 +636,7 @@
             node=node, cluster_interface=cluster_interface)
 
         self.assertEqual(
-            {mac.mac_address: {IPAddress(gateway)}},
+            {mac.mac_address: {gateway}},
             map_gateways(node))
 
     def test__finds_IPv6_gateway(self):
@@ -640,7 +656,7 @@
             node=node, cluster_interface=ipv4_interface)
 
         self.assertEqual(
-            {mac.mac_address: {IPAddress(gateway)}},
+            {mac.mac_address: {gateway}},
             map_gateways(node))
 
     def test__finds_gateways_on_multiple_MACs(self):
@@ -663,8 +679,8 @@
 
         self.assertEqual(
             {
-                mac1.mac_address: {IPAddress(gateway1)},
-                mac2.mac_address: {IPAddress(gateway2)},
+                mac1.mac_address: {gateway1},
+                mac2.mac_address: {gateway2},
             },
             map_gateways(node))
 
@@ -690,8 +706,8 @@
         self.assertEqual(
             {
                 mac.mac_address: {
-                    IPAddress(ipv4_gateway),
-                    IPAddress(ipv6_gateway),
+                    ipv4_gateway,
+                    ipv6_gateway,
                     },
             },
             map_gateways(node))