Merge ~blake-rouse/maas:fix-1836276 into maas:master

[Blake Rouse]

Fixes LP: #1836276 - Select IP address from interface for shared network defined from the region controller, allowing relayed VLAN's to point to the correct IP address on the rack controller.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b fix-1836276 lp:~blake-rouse/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 6bd4762051b3ec6b8cbb526178a09a5ae41b94a8

review: Approve

21213e9... by Blake Rouse on 2019-08-16

Extract if statement.

[Blake Rouse (blake-rouse)]

Thanks for the review. I extracted the if statement into a helper function, much cleaner, thanks for the suggestion.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b fix-1836276 lp:~blake-rouse/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 21213e9f59ad439d7548d1289d44e8b438631fe3

review: Approve

[Newell Jensen (newell-jensen)]

LGTM

review: Approve

diff --git a/src/maasserver/dhcp.py b/src/maasserver/dhcp.py
index 21faa35..ab47384 100644
--- a/src/maasserver/dhcp.py
+++ b/src/maasserver/dhcp.py
@@ -151,6 +151,17 @@ def _key_interface_subnet_dynamic_range_count(interface):
     return count
 
 
+def _ip_version_on_vlan(ip_address, ip_version, vlan):
+    """Return True when the `ip_address` is the same `ip_version` and is on
+    the same `vlan` or relay VLAN's for the `vlan."""
+    return (
+        ip_is_version(ip_address, ip_version) and
+        ip_address.subnet is not None and
+        ip_address.subnet.vlan is not None and (
+            ip_address.subnet.vlan == vlan or
+            vlan in list(ip_address.subnet.vlan.relay_vlans.all())))
+
+
 def get_interfaces_with_ip_on_vlan(rack_controller, vlan, ip_version):
     """Return a list of interfaces that have an assigned IP address on `vlan`.
 
@@ -164,20 +175,16 @@ def get_interfaces_with_ip_on_vlan(rack_controller, vlan, ip_version):
     interfaces_with_static = []
     interfaces_with_discovered = []
     for interface in rack_controller.interface_set.all().prefetch_related(
-            "ip_addresses__subnet__vlan",
+            "ip_addresses__subnet__vlan__relay_vlans",
             "ip_addresses__subnet__iprange_set"):
         for ip_address in interface.ip_addresses.all():
             if ip_address.alloc_type in [
                     IPADDRESS_TYPE.AUTO, IPADDRESS_TYPE.STICKY]:
-                if (ip_is_version(ip_address, ip_version) and
-                        ip_address.subnet is not None and
-                        ip_address.subnet.vlan == vlan):
+                if _ip_version_on_vlan(ip_address, ip_version, vlan):
                     interfaces_with_static.append(interface)
                     break
             elif ip_address.alloc_type == IPADDRESS_TYPE.DISCOVERED:
-                if (ip_is_version(ip_address, ip_version) and
-                        ip_address.subnet is not None and
-                        ip_address.subnet.vlan == vlan):
+                if _ip_version_on_vlan(ip_address, ip_version, vlan):
                     interfaces_with_discovered.append(interface)
                     break
     if len(interfaces_with_static) == 1:
@@ -689,14 +696,16 @@ def get_dhcp_configuration(rack_controller, test_dhcp_snippet=None):
             failover_peer, subnets, hosts, interface = config
             if failover_peer is not None:
                 failover_peers_v4.append(failover_peer)
-            shared_networks_v4.append({
+            shared_network = {
                 "name": "vlan-%d" % vlan.id,
                 "mtu": vlan.mtu,
                 "subnets": subnets,
-            })
+            }
+            shared_networks_v4.append(shared_network)
             hosts_v4.extend(hosts)
             if interface is not None:
                 interfaces_v4.add(interface)
+                shared_network["interface"] = interface
         # IPv6
         if len(subnets_v6) > 0:
             config = get_dhcp_configure_for(
@@ -706,14 +715,16 @@ def get_dhcp_configuration(rack_controller, test_dhcp_snippet=None):
             failover_peer, subnets, hosts, interface = config
             if failover_peer is not None:
                 failover_peers_v6.append(failover_peer)
-            shared_networks_v6.append({
+            shared_network = {
                 "name": "vlan-%d" % vlan.id,
                 "mtu": vlan.mtu,
                 "subnets": subnets,
-            })
+            }
+            shared_networks_v6.append(shared_network)
             hosts_v6.extend(hosts)
             if interface is not None:
                 interfaces_v6.add(interface)
+                shared_network["interface"] = interface
     # When no interfaces exist for each IP version clear the shared networks
     # as DHCP server cannot be started and needs to be stopped.
     if len(interfaces_v4) == 0:
diff --git a/src/maasserver/tests/test_dhcp.py b/src/maasserver/tests/test_dhcp.py
index 8601d84..2c25bcc 100644
--- a/src/maasserver/tests/test_dhcp.py
+++ b/src/maasserver/tests/test_dhcp.py
@@ -266,7 +266,7 @@ class TestGetInterfacesWithIPOnVLAN(MAASServerTestCase):
         # affects the number of queries performed when performing this
         # operation. It is important to keep this number as low as possible.
         self.assertEqual(
-            query_10_count, 5,
+            query_10_count, 6,
             "Number of queries has changed; make sure this is expected.")
         self.assertEqual(
             query_10_count, query_20_count,
@@ -426,6 +426,42 @@ class TestGetInterfacesWithIPOnVLAN(MAASServerTestCase):
             dhcp.get_interfaces_with_ip_on_vlan(
                 rack_controller, vlan, subnet.get_ipnetwork().version))
 
+    def test__returns_interface_with_static_ip_on_vlan_from_relay(self):
+        rack_controller = factory.make_RackController()
+        vlan = factory.make_VLAN()
+        relayed_to_another = factory.make_VLAN(relay_vlan=vlan)
+        subnet = factory.make_Subnet(vlan=vlan)
+        interface = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, node=rack_controller, vlan=vlan)
+        factory.make_StaticIPAddress(
+            alloc_type=IPADDRESS_TYPE.AUTO, subnet=subnet, interface=interface)
+        self.assertEquals(
+            [interface],
+            dhcp.get_interfaces_with_ip_on_vlan(
+                rack_controller, relayed_to_another,
+                subnet.get_ipnetwork().version))
+
+    def test__returns_interfaces_with_discovered_ips_on_vlan_from_relay(self):
+        rack_controller = factory.make_RackController()
+        vlan = factory.make_VLAN()
+        relayed_to_another = factory.make_VLAN(relay_vlan=vlan)
+        subnet = factory.make_Subnet(vlan=vlan)
+        interface_one = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, node=rack_controller, vlan=vlan)
+        factory.make_StaticIPAddress(
+            alloc_type=IPADDRESS_TYPE.DISCOVERED,
+            subnet=subnet, interface=interface_one)
+        interface_two = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, node=rack_controller, vlan=vlan)
+        factory.make_StaticIPAddress(
+            alloc_type=IPADDRESS_TYPE.DISCOVERED,
+            subnet=subnet, interface=interface_two)
+        self.assertItemsEqual(
+            [interface_one, interface_two],
+            dhcp.get_interfaces_with_ip_on_vlan(
+                rack_controller, relayed_to_another,
+                subnet.get_ipnetwork().version))
+
 
 class TestGenManagedVLANsFor(MAASServerTestCase):
     """Tests for `gen_managed_vlans_for`."""
diff --git a/src/provisioningserver/dhcp/config.py b/src/provisioningserver/dhcp/config.py
index 7199e83..63bd67f 100644
--- a/src/provisioningserver/dhcp/config.py
+++ b/src/provisioningserver/dhcp/config.py
@@ -350,6 +350,32 @@ def normalise_any_iterable_to_quoted_comma_list(iterable):
         return ", ".join(map(quote, iterable))
 
 
+def get_rack_ip_for_subnet(version, cidr, interface):
+    """
+    Return the IP address on this rack controller.
+
+    First the code will try to find an IP address that is in the cidr, but if
+    not it will use the first IP address on the interface (to support
+    VLAN relay).
+    """
+    cidr = IPNetwork(cidr)
+    if interface:
+        ip_addresses = [
+            IPAddress(addr)
+            for addr in net_utils.get_all_addresses_for_interface(interface)]
+    else:
+        ip_addresses = [
+            IPAddress(addr)
+            for addr in net_utils.get_all_interface_addresses()]
+    for ip_addr in ip_addresses:
+        if ip_addr in cidr:
+            return ip_addr
+    for ip_addr in ip_addresses:
+        if ip_addr.version == version:
+            return ip_addr
+    return None
+
+
 @typed
 def get_config_v4(
         template_name: str, global_dhcp_snippets: Sequence[dict],
@@ -373,22 +399,15 @@ def get_config_v4(
         "running_in_snap": snappy.running_in_snap(),
     }
 
-    rack_addrs = [
-        IPAddress(addr)
-        for addr in net_utils.get_all_interface_addresses()]
-
     for shared_network in shared_networks:
+        interface = shared_network.get("interface", None)
         for subnet in shared_network["subnets"]:
-            cidr = IPNetwork(subnet['subnet_cidr'])
-            rack_ips = [
-                str(rack_addr)
-                for rack_addr in rack_addrs
-                if rack_addr in cidr
-            ]
-            if len(rack_ips) > 0:
-                subnet["next_server"] = rack_ips[0]
+            rack_ip = get_rack_ip_for_subnet(
+                4, subnet['subnet_cidr'], interface)
+            if rack_ip is not None:
+                subnet["next_server"] = rack_ip
                 subnet["bootloader"] = compose_conditional_bootloader(
-                    False, rack_ips[0])
+                    False, rack_ip)
             ntp_servers = subnet["ntp_servers"]  # Is a list.
             ntp_servers_ipv4, ntp_servers_ipv6 = _get_addresses(*ntp_servers)
             subnet["ntp_servers_ipv4"] = ", ".join(ntp_servers_ipv4)
@@ -428,12 +447,8 @@ def get_config_v6(
         "running_in_snap": snappy.running_in_snap(),
     }
 
-    rack_addrs = [
-        IPAddress(addr)
-        for addr in net_utils.get_all_interface_addresses()]
-
     shared_networks = _process_network_parameters_v6(
-        rack_addrs, failover_peers, shared_networks)
+        failover_peers, shared_networks)
 
     try:
         return template.substitute(
@@ -446,15 +461,12 @@ def get_config_v6(
             "Failed to render DHCP configuration.") from error
 
 
-def _process_network_parameters_v6(
-        rack_addrs, failover_peers, shared_networks):
+def _process_network_parameters_v6(failover_peers, shared_networks):
     """Preprocess shared_networks prior to rendering the template.
 
     This is a separate function, partly for readability, and partly for ease
     of testing.
 
-    :param rack_addrs: a list of IPAddress values for the interfaces on this
-        rack controller.
     :param failover_peers: failover_peers from get_config_v6.
     :param shared_networks: shared_networks from get_config_v6.
     :return: an updated shared_networks, suitable for rendering the template.
@@ -462,15 +474,11 @@ def _process_network_parameters_v6(
     peers = {x["name"]: x for x in failover_peers}
 
     for shared_network in shared_networks:
+        interface = shared_network.get("interface", None)
         for subnet in shared_network["subnets"]:
-            cidr = IPNetwork(subnet['subnet_cidr'])
-            rack_ip_found = False
-            for rack_addr in rack_addrs:
-                if rack_addr in cidr:
-                    rack_ip = str(rack_addr)
-                    rack_ip_found = True
-                    break
-            if rack_ip_found:
+            rack_ip = get_rack_ip_for_subnet(
+                6, subnet['subnet_cidr'], interface)
+            if rack_ip is not None:
                 subnet["bootloader"] = compose_conditional_bootloader(
                     True, rack_ip)
             ntp_servers = subnet["ntp_servers"]  # Is a list.
diff --git a/src/provisioningserver/dhcp/testing/config.py b/src/provisioningserver/dhcp/testing/config.py
index c2640d4..068f82f 100644
--- a/src/provisioningserver/dhcp/testing/config.py
+++ b/src/provisioningserver/dhcp/testing/config.py
@@ -133,7 +133,8 @@ def make_subnet_config(network=None, pools=None, ipv6=False,
     }
 
 
-def make_shared_network(name=None, subnets=None, ipv6=False):
+def make_shared_network(
+        name=None, subnets=None, ipv6=False, with_interface=False):
     """Return complete DHCP configuration dict for a shared network."""
     if name is None:
         name = factory.make_name("vlan")
@@ -142,11 +143,14 @@ def make_shared_network(name=None, subnets=None, ipv6=False):
             make_subnet_config(ipv6=ipv6)
             for _ in range(3)
         ]
-    return {
+    data = {
         "name": name,
         "mtu": 1500,
         "subnets": subnets,
     }
+    if with_interface:
+        data["interface"] = factory.make_name("eth")
+    return data
 
 
 def make_shared_network_v1(name=None, subnets=None, ipv6=False):
diff --git a/src/provisioningserver/dhcp/tests/test_config.py b/src/provisioningserver/dhcp/tests/test_config.py
index 13d6a18..69deb0e 100644
--- a/src/provisioningserver/dhcp/tests/test_config.py
+++ b/src/provisioningserver/dhcp/tests/test_config.py
@@ -61,7 +61,7 @@ def is_ip_address(string):
         return True
 
 
-def make_sample_params_only(ipv6=False):
+def make_sample_params_only(ipv6=False, with_interface=False):
     """Return a dict of arbitrary DHCP configuration parameters.
 
     :param ipv6: When true, prepare configuration for a DHCPv6 server,
@@ -73,7 +73,7 @@ def make_sample_params_only(ipv6=False):
         for _ in range(3)
     ]
     shared_networks = [
-        make_shared_network(ipv6=ipv6)
+        make_shared_network(ipv6=ipv6, with_interface=with_interface)
         for _ in range(3)
     ]
 
@@ -114,7 +114,7 @@ def read_aliases_from_etc_hosts():
 aliases_from_etc_hosts = tuple(read_aliases_from_etc_hosts())
 
 
-def make_sample_params(test, ipv6=False):
+def make_sample_params(test, ipv6=False, with_interface=False):
     """Return a dict of arbitrary DHCP configuration parameters.
 
     This differs from `make_sample_params_only` in that it arranges it such
@@ -126,7 +126,8 @@ def make_sample_params(test, ipv6=False):
         otherwise prepare configuration for a DHCPv4 server.
     :return: A dictionary of sample configuration.
     """
-    sample_params = make_sample_params_only(ipv6=ipv6)
+    sample_params = make_sample_params_only(
+        ipv6=ipv6, with_interface=with_interface)
 
     # So that get_config can resolve the configuration, collect hostnames from
     # the sample params that need to resolve then replace them with names that
@@ -433,7 +434,7 @@ class TestGetConfig(MAASTestCase):
 class TestGetConfigIPv4(MAASTestCase):
     """Tests for `get_config`."""
 
-    def test__includes_next_server_in_config(self):
+    def test__includes_next_server_in_config_from_all_addresses(self):
         params = make_sample_params(self, ipv6=False)
         subnet = params['shared_networks'][0]['subnets'][0]
         next_server_ip = factory.pick_ip_in_network(
@@ -446,6 +447,20 @@ class TestGetConfigIPv4(MAASTestCase):
         self.assertThat(
             config_output, Contains('next-server %s;' % next_server_ip))
 
+    def test__includes_next_server_in_config_from_interface_addresses(self):
+        params = make_sample_params(self, ipv6=False, with_interface=True)
+        subnet = params['shared_networks'][0]['subnets'][0]
+        next_server_ip = factory.pick_ip_in_network(
+            netaddr.IPNetwork(subnet['subnet_cidr']))
+        self.patch(
+            net_utils, 'get_all_addresses_for_interface').return_value = [
+                next_server_ip
+            ]
+        config_output = config.get_config('dhcpd.conf.template', **params)
+        validate_dhcpd_configuration(self, config_output, False)
+        self.assertThat(
+            config_output, Contains('next-server %s;' % next_server_ip))
+
 
 class Test_process_shared_network_v6(MAASTestCase):
     """Tests for `_process_network_parameters_v6`."""
@@ -456,14 +471,12 @@ class Test_process_shared_network_v6(MAASTestCase):
                 'ip_range_high': '2001:db8:3:1::ffff',
                 'ip_range_low': '2001:db8:3:1::',
                 'failover_peer': None},
-            rack_addrs=[netaddr.IPAddress('2001:db8:3:280::2')],
             failover_peers=[])),
         ('primary', dict(
             expected={
                 'ip_range_high': '2001:db8:3:1::7fff',
                 'ip_range_low': '2001:db8:3:1::',
                 'failover_peer': 'failover-vlan-5020'},
-            rack_addrs=[netaddr.IPAddress('2001:db8:3:280::2')],
             failover_peers=[{
                 'mode': 'primary',
                 'peer_address': '2001:db8:3:0:1::',
@@ -474,7 +487,6 @@ class Test_process_shared_network_v6(MAASTestCase):
                 'ip_range_high': '2001:db8:3:1::ffff',
                 'ip_range_low': '2001:db8:3:1::8000',
                 'failover_peer': 'failover-vlan-5020'},
-            rack_addrs=[netaddr.IPAddress('2001:db8:3:0:1::')],
             failover_peers=[{
                 'mode': 'secondary',
                 'address': '2001:db8:3:0:1::',
@@ -485,6 +497,7 @@ class Test_process_shared_network_v6(MAASTestCase):
         shared_networks = [
             {
                 'name': 'vlan-5020',
+                'interface': 'eth0',
                 'subnets': [
                     {
                         'domain_name': 'maas.example.com',
@@ -503,12 +516,14 @@ class Test_process_shared_network_v6(MAASTestCase):
                     }],
             }]
         self.patch(
+            config, 'get_rack_ip_for_subnet').return_value = None
+        self.patch(
             config, 'compose_conditional_bootloader').return_value = ""
         self.patch(
             config, '_get_addresses').return_value = (
                 [''], ['2001:db8:280::2'])
         actual = config._process_network_parameters_v6(
-            self.rack_addrs, self.failover_peers, shared_networks)
+            self.failover_peers, shared_networks)
         self.assertDictEqual(
             actual[0]['subnets'][0]['pools'][0], self.expected)
 
diff --git a/src/provisioningserver/rpc/cluster.py b/src/provisioningserver/rpc/cluster.py
index 5469cf1..40988e2 100644
--- a/src/provisioningserver/rpc/cluster.py
+++ b/src/provisioningserver/rpc/cluster.py
@@ -416,6 +416,7 @@ class _ConfigureDHCP_V2(amp.Command):
                     ], optional=True)),
                 ])),
             (b"mtu", amp.Integer(optional=True)),
+            (b"interface", amp.Unicode(optional=True)),
         ])),
         (b"hosts", CompressedAmpList([
             (b"host", amp.Unicode()),
diff --git a/src/provisioningserver/rpc/dhcp.py b/src/provisioningserver/rpc/dhcp.py
index dc42518..babf9ed 100644
--- a/src/provisioningserver/rpc/dhcp.py
+++ b/src/provisioningserver/rpc/dhcp.py
@@ -529,6 +529,7 @@ def downgrade_shared_networks(shared_networks):
     Mutates `shared_networks` in place.
     """
     for shared_network in shared_networks:
+        shared_network.pop("interface", None)
         for subnet in shared_network["subnets"]:
             dns_servers = subnet["dns_servers"]
             if not isinstance(dns_servers, str):
diff --git a/src/provisioningserver/rpc/tests/test_clusterservice.py b/src/provisioningserver/rpc/tests/test_clusterservice.py
index 07e1645..0b19738 100644
--- a/src/provisioningserver/rpc/tests/test_clusterservice.py
+++ b/src/provisioningserver/rpc/tests/test_clusterservice.py
@@ -2323,6 +2323,7 @@ class TestClusterProtocol_ConfigureDHCP(MAASTestCase):
             "command": cluster.ConfigureDHCPv4,
             "make_network": factory.make_ipv4_network,
             "make_shared_network": make_shared_network_v1,
+            "make_shared_network_kwargs": {},
             "concurrency_lock": concurrency.dhcpv4,
         }),
         ("DHCPv4,V2", {
@@ -2330,6 +2331,7 @@ class TestClusterProtocol_ConfigureDHCP(MAASTestCase):
             "command": cluster.ConfigureDHCPv4_V2,
             "make_network": factory.make_ipv4_network,
             "make_shared_network": make_shared_network,
+            "make_shared_network_kwargs": {"with_interface": True},
             "concurrency_lock": concurrency.dhcpv4,
         }),
         ("DHCPv6", {
@@ -2337,6 +2339,7 @@ class TestClusterProtocol_ConfigureDHCP(MAASTestCase):
             "command": cluster.ConfigureDHCPv6,
             "make_network": factory.make_ipv6_network,
             "make_shared_network": make_shared_network_v1,
+            "make_shared_network_kwargs": {},
             "concurrency_lock": concurrency.dhcpv6,
         }),
         ("DHCPv6,V2", {
@@ -2344,6 +2347,7 @@ class TestClusterProtocol_ConfigureDHCP(MAASTestCase):
             "command": cluster.ConfigureDHCPv6_V2,
             "make_network": factory.make_ipv6_network,
             "make_shared_network": make_shared_network,
+            "make_shared_network_kwargs": {"with_interface": True},
             "concurrency_lock": concurrency.dhcpv6,
         }),
     )
@@ -2361,7 +2365,8 @@ class TestClusterProtocol_ConfigureDHCP(MAASTestCase):
 
         omapi_key = factory.make_name('key')
         failover_peers = [make_failover_peer_config()]
-        shared_networks = [self.make_shared_network()]
+        shared_networks = [self.make_shared_network(
+            **self.make_shared_network_kwargs)]
         shared_networks = fix_shared_networks_failover(
             shared_networks, failover_peers)
         hosts = [make_host()]