Merge lp:~allenap/maas/dhcp-ntp-publish-external into lp:~maas-committers/maas/trunk

[Gavin Panella]

Configure NTP servers per subnet when generating DHCP configurations.

[Blake Rouse (blake-rouse)]

Overall looks good. Just a couple of comments. Be sure the CI the whole set of branches before landing.

review: Approve

[Gavin Panella (allenap)]

Thanks for the review. I have a question for you in the diff.

=== modified file 'src/maasserver/dhcp.py'
--- src/maasserver/dhcp.py	2016-09-23 07:59:24 +0000
+++ src/maasserver/dhcp.py	2016-09-23 07:59:24 +0000
@@ -12,8 +12,12 @@
     defaultdict,
     namedtuple,
 )
+from itertools import groupby
 from operator import itemgetter
-from typing import Iterable
+from typing import (
+    Iterable,
+    Union,
+)
 
 from django.conf import settings
 from django.core.exceptions import ValidationError
@@ -43,7 +47,10 @@
 )
 from maasserver.utils.orm import transactional
 from maasserver.utils.threads import deferToDatabase
-from netaddr import IPAddress
+from netaddr import (
+    IPAddress,
+    IPNetwork,
+)
 from provisioningserver.dhcp.omshell import generate_omapi_key
 from provisioningserver.rpc.cluster import (
     ConfigureDHCPv4,
@@ -229,6 +236,37 @@
     return get_ip_address_for_interface(interface, vlan)
 
 
+@typed
+def get_ntp_server_addresses_for_rack(rack: RackController) -> dict:
+    """Return a map of rack IP addresses suitable for NTP.
+
+    These are keyed by `(subnet-space-id, subnet-ip-address-family)`, e.g.::
+
+      {(73, 4): "192.168.1.1"}
+
+    Only a single routable address for the rack will be returned in each
+    space+family group even if there are multiple.
+    """
+    rack_addresses = StaticIPAddress.objects.filter(
+        interface__enabled=True, interface__node=rack, alloc_type__in={
+            IPADDRESS_TYPE.STICKY, IPADDRESS_TYPE.USER_RESERVED})
+    rack_addresses = rack_addresses.order_by(
+        "subnet__space_id", "subnet__cidr", "ip")
+    rack_addresses = rack_addresses.values_list(
+        "subnet__space_id", "subnet__cidr", "ip")
+
+    def get_space_id_and_family(record):
+        space_id, cidr, ip = record
+        return space_id, IPNetwork(cidr).version
+
+    return {
+        space_id_and_family: min(
+            (ip for _, _, ip in group), key=IPAddress)
+        for space_id_and_family, group in groupby(
+            rack_addresses, get_space_id_and_family)
+    }
+
+
 def make_interface_hostname(interface):
     """Return the host decleration name for DHCPD for this `interface`."""
     interface_name = interface.name.replace(".", "-")
@@ -332,9 +370,15 @@
 
 @typed
 def make_subnet_config(
-        rack_controller, subnet, maas_dns_server, ntp_servers: list,
-        default_domain, failover_peer=None, subnets_dhcp_snippets: list=None):
-    """Return DHCP subnet configuration dict for a rack interface."""
+        rack_controller, subnet, maas_dns_server,
+        ntp_servers: Union[list, dict], default_domain,
+        failover_peer=None, subnets_dhcp_snippets: list=None):
+    """Return DHCP subnet configuration dict for a rack interface.
+
+    :param ntp_servers: Either a list of NTP server addresses or hostnames to
+        include in DHCP responses, or a dict; if the latter, it ought to match
+        the output from `get_ntp_server_addresses_for_rack`.
+    """
     ip_network = subnet.get_ipnetwork()
     if subnet.dns_servers is not None and len(subnet.dns_servers) > 0:
         # Replace MAAS DNS with the servers defined on the subnet.
@@ -345,6 +389,15 @@
         dns_servers = []
     if subnets_dhcp_snippets is None:
         subnets_dhcp_snippets = []
+
+    if isinstance(ntp_servers, dict):
+        try:
+            ntp_server = ntp_servers[subnet.space_id, subnet.get_ip_version()]
+        except KeyError:
+            ntp_servers = []
+        else:
+            ntp_servers = [ntp_server]
+
     return {
         'subnet': str(ip_network.network),
         'subnet_mask': str(ip_network.netmask),
@@ -388,7 +441,7 @@
 @typed
 def get_dhcp_configure_for(
         ip_version: int, rack_controller, vlan, subnets: list,
-        ntp_servers: list, domain, dhcp_snippets: Iterable=None):
+        ntp_servers: Union[list, dict], domain, dhcp_snippets: Iterable=None):
     """Get the DHCP configuration for `ip_version`."""
     # Circular imports.
     from maasserver.dns.zonegenerator import get_dns_server_address
@@ -493,8 +546,15 @@
     shared_networks_v6 = []
     hosts_v6 = []
     interfaces_v6 = set()
-    ntp_servers = Config.objects.get_config("ntp_servers")
-    ntp_servers = list(split_string_list(ntp_servers))
+
+    # NTP configuration can get tricky...
+    ntp_external_only = Config.objects.get_config("ntp_external_only")
+    if ntp_external_only:
+        ntp_servers = Config.objects.get_config("ntp_servers")
+        ntp_servers = list(split_string_list(ntp_servers))
+    else:
+        ntp_servers = get_ntp_server_addresses_for_rack(rack_controller)
+
     default_domain = Domain.objects.get_default_domain()
     for vlan, (subnets_v4, subnets_v6) in vlan_subnets.items():
         # IPv4

=== modified file 'src/maasserver/tests/test_dhcp.py'
--- src/maasserver/tests/test_dhcp.py	2016-09-23 07:59:24 +0000
+++ src/maasserver/tests/test_dhcp.py	2016-09-23 07:59:24 +0000
@@ -65,10 +65,15 @@
 from provisioningserver.rpc.exceptions import CannotConfigureDHCP
 from provisioningserver.utils.twisted import synchronous
 from testtools.matchers import (
+    AllMatch,
     ContainsAll,
+    ContainsDict,
     Equals,
+    HasLength,
     IsInstance,
+    MatchesAll,
     MatchesStructure,
+    Not,
 )
 from twisted.internet import defer
 from twisted.internet.defer import inlineCallbacks
@@ -604,6 +609,93 @@
             dhcp.get_ip_address_for_rack_controller(rack_controller, vlan))
 
 
+class TestGetNTPServerAddressesForRack(MAASServerTestCase):
+    """Tests for `get_ntp_server_addresses_for_rack`."""
+
+    def test__returns_empty_dict_for_unconnected_rack(self):
+        rack = factory.make_RackController()
+        self.assertThat(
+            dhcp.get_ntp_server_addresses_for_rack(rack),
+            Equals({}))
+
+    def test__returns_dict_with_rack_addresses(self):
+        rack = factory.make_RackController()
+        space = factory.make_Space()
+        subnet = factory.make_Subnet(space=space)
+        interface = factory.make_Interface(node=rack)
+        address = factory.make_StaticIPAddress(
+            interface=interface, subnet=subnet,
+            alloc_type=IPADDRESS_TYPE.STICKY)
+
+        self.assertThat(
+            dhcp.get_ntp_server_addresses_for_rack(rack), Equals({
+                (space.id, subnet.get_ipnetwork().version): address.ip,
+            }))
+
+    def test__returns_dict_grouped_by_space_and_address_family(self):
+        rack = factory.make_RackController()
+        space1 = factory.make_Space()
+        space2 = factory.make_Space()
+        subnet1 = factory.make_Subnet(space=space1)
+        subnet2 = factory.make_Subnet(space=space2)
+        interface = factory.make_Interface(node=rack)
+        address1 = factory.make_StaticIPAddress(
+            interface=interface, subnet=subnet1,
+            alloc_type=IPADDRESS_TYPE.STICKY)
+        address2 = factory.make_StaticIPAddress(
+            interface=interface, subnet=subnet2,
+            alloc_type=IPADDRESS_TYPE.STICKY)
+
+        self.assertThat(
+            dhcp.get_ntp_server_addresses_for_rack(rack), Equals({
+                (space1.id, subnet1.get_ipnetwork().version): address1.ip,
+                (space2.id, subnet2.get_ipnetwork().version): address2.ip,
+            }))
+
+    def test__returned_dict_chooses_minimum_address(self):
+        rack = factory.make_RackController()
+        space = factory.make_Space()
+        cidr = factory.make_ip4_or_6_network(host_bits=16)
+        subnet = factory.make_Subnet(space=space, cidr=cidr)
+        interface = factory.make_Interface(node=rack)
+        addresses = {
+            factory.make_StaticIPAddress(
+                interface=interface, subnet=subnet,
+                alloc_type=IPADDRESS_TYPE.STICKY)
+            for _ in range(10)
+        }
+
+        self.assertThat(
+            dhcp.get_ntp_server_addresses_for_rack(rack), Equals({
+                (space.id, subnet.get_ipnetwork().version): min(
+                    (address.ip for address in addresses), key=IPAddress),
+            }))
+
+    def test__constant_query_count(self):
+        rack = factory.make_RackController()
+        interface = factory.make_Interface(node=rack)
+
+        count, result = count_queries(
+            dhcp.get_ntp_server_addresses_for_rack, rack)
+        self.assertThat(count, Equals(1))
+        self.assertThat(result, Equals({}))
+
+        for _ in (1, 2):
+            space = factory.make_Space()
+            for family in (4, 6):
+                cidr = factory.make_ip4_or_6_network(family, host_bits=8)
+                subnet = factory.make_Subnet(space=space, cidr=cidr)
+                for _ in (1, 2):
+                    factory.make_StaticIPAddress(
+                        interface=interface, subnet=subnet,
+                        alloc_type=IPADDRESS_TYPE.STICKY)
+
+        count, result = count_queries(
+            dhcp.get_ntp_server_addresses_for_rack, rack)
+        self.assertThat(count, Equals(1))
+        self.assertThat(result, Not(Equals({})))
+
+
 class TestMakeSubnetConfig(MAASServerTestCase):
     """Tests for `make_subnet_config`."""
 
@@ -659,7 +751,7 @@
             rack_controller, subnet, maas_dns, ntp_servers, default_domain)
         self.assertThat(config['dns_servers'], Equals([IPAddress(maas_dns)]))
 
-    def test__sets_ntp_from_arguments(self):
+    def test__sets_ntp_from_list_argument(self):
         rack_controller = factory.make_RackController(interface=False)
         vlan = factory.make_VLAN()
         subnet = factory.make_Subnet(vlan=vlan, dns_servers=[])
@@ -671,6 +763,34 @@
             rack_controller, subnet, "", ntp_servers, default_domain)
         self.expectThat(config['ntp_servers'], Equals(ntp_servers))
 
+    def test__sets_ntp_from_empty_dict_argument(self):
+        rack_controller = factory.make_RackController(interface=False)
+        vlan = factory.make_VLAN()
+        subnet = factory.make_Subnet(vlan=vlan, dns_servers=[])
+        factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, vlan=vlan, node=rack_controller)
+        default_domain = Domain.objects.get_default_domain()
+        config = dhcp.make_subnet_config(
+            rack_controller, subnet, "", {}, default_domain)
+        self.expectThat(config['ntp_servers'], Equals([]))
+
+    def test__sets_ntp_from_dict_argument(self):
+        rack_controller = factory.make_RackController(interface=False)
+        vlan = factory.make_VLAN()
+        subnet = factory.make_Subnet(vlan=vlan, dns_servers=[])
+        interface = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, vlan=vlan, node=rack_controller)
+        address = factory.make_StaticIPAddress(
+            interface=interface, subnet=subnet,
+            alloc_type=IPADDRESS_TYPE.STICKY)
+        ntp_servers = {
+            (subnet.space_id, subnet.get_ipnetwork().version): address.ip,
+        }
+        default_domain = Domain.objects.get_default_domain()
+        config = dhcp.make_subnet_config(
+            rack_controller, subnet, "", ntp_servers, default_domain)
+        self.expectThat(config['ntp_servers'], Equals([address.ip]))
+
     def test__overrides_ipv4_dns_from_subnet(self):
         rack_controller = factory.make_RackController(interface=False)
         vlan = factory.make_VLAN()
@@ -1402,6 +1522,70 @@
         self.assertEqual(primary_interface.name, observed_interface)
 
 
+class TestGetDHCPConfiguration(MAASServerTestCase):
+    """Tests for `get_dhcp_configuration`."""
+
+    def make_RackController_ready_for_DHCP(self):
+        rack = factory.make_RackController()
+        vlan = factory.make_VLAN(dhcp_on=True, primary_rack=rack)
+        subnet4 = factory.make_Subnet(
+            vlan=vlan, cidr="10.20.30.0/24")
+        subnet6 = factory.make_Subnet(
+            vlan=vlan, cidr="fd38:c341:27da:c831::/64")
+        interface = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, node=rack, vlan=vlan)
+        address4 = factory.make_StaticIPAddress(
+            alloc_type=IPADDRESS_TYPE.STICKY, subnet=subnet4,
+            interface=interface)
+        address6 = factory.make_StaticIPAddress(
+            alloc_type=IPADDRESS_TYPE.STICKY, subnet=subnet6,
+            interface=interface)
+        return rack, (address4, address6)
+
+    def assertHasConfigurationForNTP(
+            self, shared_network, subnet, ntp_servers):
+        self.assertThat(
+            shared_network, MatchesAll(
+                # Quick-n-dirty: match only one shared network.
+                HasLength(1), AllMatch(ContainsDict({
+                    "subnets": MatchesAll(
+                        # Quick-n-dirty: match only one subnet.
+                        HasLength(1), AllMatch(ContainsDict({
+                            "subnet_cidr": Equals(subnet.cidr),
+                            "ntp_servers": Equals(ntp_servers),
+                        })),
+                    ),
+                })),
+            )
+        )
+
+    def test__uses_global_ntp_servers_when_ntp_external_only_is_set(self):
+        ntp_servers = [factory.make_hostname(), factory.make_ip_address()]
+        Config.objects.set_config("ntp_servers", ", ".join(ntp_servers))
+        Config.objects.set_config("ntp_external_only", True)
+
+        rack, (addr4, addr6) = self.make_RackController_ready_for_DHCP()
+        config = dhcp.get_dhcp_configuration(rack)
+
+        self.assertHasConfigurationForNTP(
+            config.shared_networks_v4, addr4.subnet, ntp_servers)
+        self.assertHasConfigurationForNTP(
+            config.shared_networks_v6, addr6.subnet, ntp_servers)
+
+    def test__finds_per_subnet_addresses_when_ntp_external_only_not_set(self):
+        ntp_servers = [factory.make_hostname(), factory.make_ip_address()]
+        Config.objects.set_config("ntp_servers", ", ".join(ntp_servers))
+        Config.objects.set_config("ntp_external_only", False)
+
+        rack, (addr4, addr6) = self.make_RackController_ready_for_DHCP()
+        config = dhcp.get_dhcp_configuration(rack)
+
+        self.assertHasConfigurationForNTP(
+            config.shared_networks_v4, addr4.subnet, [addr4.ip])
+        self.assertHasConfigurationForNTP(
+            config.shared_networks_v6, addr6.subnet, [addr6.ip])
+
+
 class TestConfigureDHCP(MAASTransactionServerTestCase):
     """Tests for `configure_dhcp`."""