Merge lp:~allenap/maas/ntp-vendor-data-split-servers-and-pools into lp:~maas-committers/maas/trunk

[Gavin Panella]

Publish both NTP servers and pools in vendor-data.

[Blake Rouse (blake-rouse)]

Looks good.

review: Approve

=== modified file 'src/metadataserver/tests/test_vendor_data.py'
--- src/metadataserver/tests/test_vendor_data.py	2016-10-05 06:13:22 +0000
+++ src/metadataserver/tests/test_vendor_data.py	2016-10-05 06:13:22 +0000
@@ -14,6 +14,7 @@
     generate_system_info,
     get_vendor_data,
 )
+from netaddr import IPAddress
 from testtools.matchers import (
     Contains,
     ContainsDict,
@@ -55,7 +56,8 @@
         vendor_data = get_vendor_data(node)
         self.assertThat(vendor_data, ContainsDict({
             "ntp": Equals({
-                "servers": ["bar", "foo"],
+                "servers": [],
+                "pools": ["bar", "foo"],
             }),
         }))
 
@@ -104,12 +106,15 @@
 
     def test_external_only_yields_all_ntp_servers_when_defined(self):
         Config.objects.set_config("ntp_external_only", True)
-        ntp_servers = factory.make_hostname(), factory.make_hostname()
+        ntp_hosts = factory.make_hostname(), factory.make_hostname()
+        ntp_addrs = factory.make_ipv4_address(), factory.make_ipv6_address()
+        ntp_servers = ntp_hosts + ntp_addrs
         Config.objects.set_config("ntp_servers", " ".join(ntp_servers))
         configuration = generate_ntp_configuration(node=None)
         self.assertThat(dict(configuration), Equals({
             "ntp": {
-                "servers": sorted(ntp_servers),
+                "servers": sorted(ntp_addrs, key=IPAddress),
+                "pools": sorted(ntp_hosts),
             },
         }))
 
@@ -148,9 +153,9 @@
         configuration = generate_ntp_configuration(machine)
         self.assertThat(dict(configuration), Equals({
             "ntp": {
-                "servers": sorted((
-                    rack_primary_address.ip,
-                    rack_secondary_address.ip,
-                )),
+                "servers": sorted(
+                    (rack_primary_address.ip, rack_secondary_address.ip),
+                    key=IPAddress),
+                "pools": [],
             },
         }))
 
=== modified file 'src/metadataserver/vendor_data.py'
--- src/metadataserver/vendor_data.py	2016-10-05 06:13:22 +0000
+++ src/metadataserver/vendor_data.py	2016-10-05 06:13:22 +0000
@@ -10,6 +10,8 @@
 from itertools import chain
 
 from maasserver import ntp
+from netaddr import IPAddress
+from provisioningserver.ntp.config import normalise_address
 from provisioningserver.utils.text import make_gecos_field
 
 
@@ -47,9 +49,15 @@
           - 102.10.10.10
           - ntp.ubuntu.com
 
-    but MAAS does not yet distinguish between pool and non-pool servers, and
-    so this returns a single set of time references.
+    MAAS assumes that IP addresses are "servers" and hostnames/FQDNs "pools".
     """
     ntp_servers = ntp.get_servers_for(node)
     if len(ntp_servers) >= 1:
-        yield "ntp", {"servers": sorted(ntp_servers)}
+        # Separate out IP addresses from the rest.
+        addrs, other = set(), set()
+        for ntp_server in map(normalise_address, ntp_servers):
+            bucket = addrs if isinstance(ntp_server, IPAddress) else other
+            bucket.add(ntp_server)
+        servers = [addr.format() for addr in sorted(addrs)]
+        pools = sorted(other)  # Hostnames and FQDNs only.
+        yield "ntp", {"servers": servers, "pools": pools}
 
=== modified file 'src/provisioningserver/ntp/config.py'
--- src/provisioningserver/ntp/config.py	2016-09-29 14:28:06 +0000
+++ src/provisioningserver/ntp/config.py	2016-10-05 06:13:22 +0000
@@ -6,6 +6,7 @@
 __all__ = [
     "configure_rack",
     "configure_region",
+    "normalise_address",
 ]
 
 from functools import partial
@@ -58,6 +59,23 @@
 configure_rack = partial(configure, offset=1)
 
 
+def normalise_address(address):
+    """Normalise an IP address into a form suitable for the `ntp` daemon.
+
+    It seems to prefer non-mapped IPv4 addresses, for example. Hostnames are
+    passed through.
+    """
+    try:
+        address = IPAddress(address)
+    except AddrFormatError:
+        return address  # Hostname.
+    else:
+        if address.is_ipv4_mapped():
+            return address.ipv4()
+        else:
+            return address
+
+
 def _render_ntp_conf(includefile):
     """Render ``ntp.conf`` based on the existing configuration.
 
@@ -93,12 +111,12 @@
         orphan mode (see http://support.ntp.org/bin/view/Support/OrphanMode).
     """
     lines = ["# MAAS NTP configuration."]
-    servers = map(_normalise_address, servers)
+    servers = map(normalise_address, servers)
     lines.extend(
         "%s %s iburst" % (
             ("server" if isinstance(server, IPAddress) else "pool"), server)
         for server in servers)
-    peers = map(_normalise_address, peers)
+    peers = map(normalise_address, peers)
     lines.extend("peer %s" % peer for peer in peers)
     lines.append("tos orphan {:d}".format(offset + 8))
     lines.append("")  # Add newline at end.
@@ -168,20 +186,3 @@
         if not blank:
             yield from lines
             yield "\n"
-
-
-def _normalise_address(address):
-    """Normalise an IP address into a form suitable for the `ntp` daemon.
-
-    It seems to prefer non-mapped IPv4 addresses, for example. Hostnames are
-    passed through.
-    """
-    try:
-        address = IPAddress(address)
-    except AddrFormatError:
-        return address  # Hostname.
-    else:
-        if address.is_ipv4_mapped():
-            return address.ipv4()
-        else:
-            return address
 
=== modified file 'src/provisioningserver/ntp/tests/test_config.py'
--- src/provisioningserver/ntp/tests/test_config.py	2016-09-29 14:28:06 +0000
+++ src/provisioningserver/ntp/tests/test_config.py	2016-10-05 06:13:22 +0000
@@ -114,6 +114,35 @@
                 keywords=Equals({"offset": 1})))
 
 
+class TestNormaliseAddress(MAASTestCase):
+    """Tests for `p.ntp.config.normalise_address`."""
+
+    def test_returns_hostnames_unchanged(self):
+        hostname = factory.make_hostname()
+        self.assertThat(config.normalise_address(hostname), Equals(hostname))
+
+    def test_returns_ipv4_addresses_as_IPAddress(self):
+        address = factory.make_ipv4_address()
+        normalised = config.normalise_address(address)
+        self.assertThat(normalised, IsInstance(IPAddress))
+        self.assertThat(normalised.version, Equals(4))
+        self.assertThat(str(normalised), Equals(address))
+
+    def test_returns_ipv6_addresses_as_IPAddress(self):
+        address = factory.make_ipv6_address()
+        normalised = config.normalise_address(address)
+        self.assertThat(normalised, IsInstance(IPAddress))
+        self.assertThat(normalised.version, Equals(6))
+        self.assertThat(str(normalised), Equals(address))
+
+    def test_renders_ipv6_mapped_ipv4_addresses_as_plain_ipv4(self):
+        address_as_ipv4 = factory.make_ipv4_address()
+        address_as_ipv6 = str(IPAddress(address_as_ipv4).ipv6())
+        normalised = config.normalise_address(address_as_ipv6)
+        self.assertThat(normalised, IsInstance(IPAddress))
+        self.assertThat(str(normalised), Equals(address_as_ipv4))
+
+
 def extract_tos_options(configuration):
     commands = re.findall(r"^ *tos +([^\r\n]+)$", configuration, re.MULTILINE)
     return list(chain.from_iterable(map(str.split, commands)))