Merge ~r00ta/maas:lp-2027621 into maas:master

[Jacopo Rota]

fix dhcp make_failover_peer_config mixing ipv4 and ipv6 addresses

This MP aims to fix https://bugs.launchpad.net/maas/+bug/2027621.

The function make_failover_peer_config is mixing ipv4 and ipv6 addresses when the configuration is rendered.

The fix is
1) pass the `ip_version` to this function
2) select the interfaces of the rack controller that are on the vlan and that match the `ip_version`
3) select the best interface according to a specific logic already implemented
4) get an ip from that interface that is on the vlan and that matches the `ip_version`

A test to verify this scenario has been added.

[Anton Troyanov (troyanov)]

LGTM!

review: Approve

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp-2027621 lp:~r00ta/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas-tester/3016/console
COMMIT: 04242f723144301e93011ac95a79a2f947b33fca

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp-2027621 lp:~r00ta/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 983c55ba1a9b58c2f7a5cb6b0838eb0db27eddd8

review: Approve

[MAAS Lander (maas-lander)]

LANDING
-b lp-2027621 lp:~r00ta/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci.internal:8080/job/maas-tester/3024/console

diff --git a/src/maasserver/dhcp.py b/src/maasserver/dhcp.py
index 32ad98a..8f51cbf 100644
--- a/src/maasserver/dhcp.py
+++ b/src/maasserver/dhcp.py
@@ -227,15 +227,17 @@ def ip_is_on_vlan(ip_address, vlan):
     )
 
 
-def get_ip_address_for_interface(interface, vlan):
+def get_ip_address_for_interface(interface, vlan, ip_version: int):
     """Return the IP address for `interface` on `vlan`."""
     for ip_address in interface.ip_addresses.all():
-        if ip_is_on_vlan(ip_address, vlan):
+        if ip_is_version(ip_address, ip_version) and ip_is_on_vlan(
+            ip_address, vlan
+        ):
             return ip_address
     return None
 
 
-def get_ip_address_for_rack_controller(rack_controller, vlan):
+def get_ip_address_for_rack_controller(rack_controller, vlan, ip_version: int):
     """Return the IP address for `rack_controller` on `vlan`."""
     # First we build a list of all interfaces that have an IP address
     # on that vlan. Then we pick the best interface for that vlan
@@ -248,10 +250,12 @@ def get_ip_address_for_rack_controller(rack_controller, vlan):
     matching_interfaces = set()
     for interface in interfaces:
         for ip_address in interface.ip_addresses.all():
-            if ip_is_on_vlan(ip_address, vlan):
+            if ip_is_version(ip_address, ip_version) and ip_is_on_vlan(
+                ip_address, vlan
+            ):
                 matching_interfaces.add(interface)
     interface = get_best_interface(matching_interfaces)
-    return get_ip_address_for_interface(interface, vlan)
+    return get_ip_address_for_interface(interface, vlan, ip_version)
 
 
 def get_ntp_server_addresses_for_rack(rack: RackController) -> dict:
@@ -498,17 +502,19 @@ def make_subnet_config(
     return subnet_config
 
 
-def make_failover_peer_config(vlan, rack_controller):
+def make_failover_peer_config(vlan, rack_controller, ip_version: int):
     """Return DHCP failover peer configuration dict for a rack controller."""
     is_primary = vlan.primary_rack_id == rack_controller.id
     interface_ip_address = get_ip_address_for_rack_controller(
-        rack_controller, vlan
+        rack_controller, vlan, ip_version
     )
     if is_primary:
         peer_rack = vlan.secondary_rack
     else:
         peer_rack = vlan.primary_rack
-    peer_address = get_ip_address_for_rack_controller(peer_rack, vlan)
+    peer_address = get_ip_address_for_rack_controller(
+        peer_rack, vlan, ip_version
+    )
     name = "failover-vlan-%d" % vlan.id
     return (
         name,
@@ -631,7 +637,7 @@ def get_dhcp_configure_for(
     if has_secondary:
         # Generate the failover peer for this VLAN.
         peer_name, peer_config, peer_rack = make_failover_peer_config(
-            vlan, rack_controller
+            vlan, rack_controller, ip_version
         )
     else:
         peer_name, peer_config, peer_rack = None, None, None
diff --git a/src/maasserver/tests/test_dhcp.py b/src/maasserver/tests/test_dhcp.py
index e9d0cfa..04d15dc 100644
--- a/src/maasserver/tests/test_dhcp.py
+++ b/src/maasserver/tests/test_dhcp.py
@@ -815,10 +815,16 @@ class TestGetIPAddressForInterface(MAASServerTestCase):
         interface = factory.make_Interface(INTERFACE_TYPE.PHYSICAL, vlan=vlan)
         subnet = factory.make_Subnet(vlan=vlan)
         ip_address = factory.make_StaticIPAddress(
-            alloc_type=IPADDRESS_TYPE.AUTO, subnet=subnet, interface=interface
+            ip=factory.pick_ip_in_Subnet(subnet),
+            alloc_type=IPADDRESS_TYPE.AUTO,
+            subnet=subnet,
+            interface=interface,
         )
         self.assertEqual(
-            ip_address, dhcp.get_ip_address_for_interface(interface, vlan)
+            ip_address,
+            dhcp.get_ip_address_for_interface(
+                interface, vlan, subnet.get_ip_version()
+            ),
         )
 
     def test_returns_None(self):
@@ -826,10 +832,15 @@ class TestGetIPAddressForInterface(MAASServerTestCase):
         interface = factory.make_Interface(INTERFACE_TYPE.PHYSICAL, vlan=vlan)
         subnet = factory.make_Subnet(vlan=vlan)
         factory.make_StaticIPAddress(
-            alloc_type=IPADDRESS_TYPE.AUTO, subnet=subnet, interface=interface
+            ip=factory.pick_ip_in_Subnet(subnet),
+            alloc_type=IPADDRESS_TYPE.AUTO,
+            subnet=subnet,
+            interface=interface,
         )
         self.assertIsNone(
-            dhcp.get_ip_address_for_interface(interface, factory.make_VLAN())
+            dhcp.get_ip_address_for_interface(
+                interface, factory.make_VLAN(), subnet.get_ip_version()
+            )
         )
 
 
@@ -844,11 +855,16 @@ class TestGetIPAddressForRackController(MAASServerTestCase):
         )
         subnet = factory.make_Subnet(vlan=vlan)
         ip_address = factory.make_StaticIPAddress(
-            alloc_type=IPADDRESS_TYPE.AUTO, subnet=subnet, interface=interface
+            ip=factory.pick_ip_in_Subnet(subnet),
+            alloc_type=IPADDRESS_TYPE.AUTO,
+            subnet=subnet,
+            interface=interface,
         )
         self.assertEqual(
             ip_address,
-            dhcp.get_ip_address_for_rack_controller(rack_controller, vlan),
+            dhcp.get_ip_address_for_rack_controller(
+                rack_controller, vlan, subnet.get_ip_version()
+            ),
         )
 
     def test_returns_ip_address_from_best_interface_on_rack_controller(self):
@@ -868,7 +884,10 @@ class TestGetIPAddressForRackController(MAASServerTestCase):
         )
         subnet = factory.make_Subnet(vlan=vlan)
         factory.make_StaticIPAddress(
-            alloc_type=IPADDRESS_TYPE.AUTO, subnet=subnet, interface=interface
+            ip=factory.pick_ip_in_Subnet(subnet),
+            alloc_type=IPADDRESS_TYPE.AUTO,
+            subnet=subnet,
+            interface=interface,
         )
         bond_ip_address = factory.make_StaticIPAddress(
             alloc_type=IPADDRESS_TYPE.AUTO,
@@ -877,7 +896,9 @@ class TestGetIPAddressForRackController(MAASServerTestCase):
         )
         self.assertEqual(
             bond_ip_address,
-            dhcp.get_ip_address_for_rack_controller(rack_controller, vlan),
+            dhcp.get_ip_address_for_rack_controller(
+                rack_controller, vlan, subnet.get_ip_version()
+            ),
         )
 
 
@@ -2121,11 +2142,12 @@ class TestMakeFailoverPeerConfig(MAASServerTestCase):
             primary_rack=primary_rack,
             secondary_rack=secondary_rack,
         )
-        subnet = factory.make_Subnet(vlan=vlan)
+        subnet = factory.make_Subnet(vlan=vlan, version=4)
         primary_interface = factory.make_Interface(
             INTERFACE_TYPE.PHYSICAL, node=primary_rack, vlan=vlan
         )
         primary_ip = factory.make_StaticIPAddress(
+            ip=factory.pick_ip_in_Subnet(subnet),
             alloc_type=IPADDRESS_TYPE.AUTO,
             subnet=subnet,
             interface=primary_interface,
@@ -2134,6 +2156,7 @@ class TestMakeFailoverPeerConfig(MAASServerTestCase):
             INTERFACE_TYPE.PHYSICAL, node=secondary_rack, vlan=vlan
         )
         secondary_ip = factory.make_StaticIPAddress(
+            ip=factory.pick_ip_in_Subnet(subnet),
             alloc_type=IPADDRESS_TYPE.AUTO,
             subnet=subnet,
             interface=secondary_interface,
@@ -2150,7 +2173,7 @@ class TestMakeFailoverPeerConfig(MAASServerTestCase):
                 },
                 secondary_rack,
             ),
-            dhcp.make_failover_peer_config(vlan, primary_rack),
+            dhcp.make_failover_peer_config(vlan, primary_rack, 4),
         )
 
     def test_renders_config_for_secondary(self):
@@ -2161,11 +2184,12 @@ class TestMakeFailoverPeerConfig(MAASServerTestCase):
             primary_rack=primary_rack,
             secondary_rack=secondary_rack,
         )
-        subnet = factory.make_Subnet(vlan=vlan)
+        subnet = factory.make_Subnet(vlan=vlan, version=4)
         primary_interface = factory.make_Interface(
             INTERFACE_TYPE.PHYSICAL, node=primary_rack, vlan=vlan
         )
         primary_ip = factory.make_StaticIPAddress(
+            ip=factory.pick_ip_in_Subnet(subnet),
             alloc_type=IPADDRESS_TYPE.AUTO,
             subnet=subnet,
             interface=primary_interface,
@@ -2174,6 +2198,7 @@ class TestMakeFailoverPeerConfig(MAASServerTestCase):
             INTERFACE_TYPE.PHYSICAL, node=secondary_rack, vlan=vlan
         )
         secondary_ip = factory.make_StaticIPAddress(
+            ip=factory.pick_ip_in_Subnet(subnet),
             alloc_type=IPADDRESS_TYPE.AUTO,
             subnet=subnet,
             interface=secondary_interface,
@@ -2190,7 +2215,78 @@ class TestMakeFailoverPeerConfig(MAASServerTestCase):
                 },
                 primary_rack,
             ),
-            dhcp.make_failover_peer_config(vlan, secondary_rack),
+            dhcp.make_failover_peer_config(vlan, secondary_rack, 4),
+        )
+
+    # See https://bugs.launchpad.net/maas/+bug/2027621
+    def test_renders_config_for_secondary_should_not_mix_v4_and_v6_addresses(
+        self,
+    ):
+        primary_rack = factory.make_RackController()
+        secondary_rack = factory.make_RackController()
+        vlan = factory.make_VLAN(
+            dhcp_on=True,
+            primary_rack=primary_rack,
+            secondary_rack=secondary_rack,
+        )
+        subnet_v4 = factory.make_Subnet(vlan=vlan, version=4)
+        subnet_v6 = factory.make_Subnet(vlan=vlan, version=6)
+        primary_interface = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, node=primary_rack, vlan=vlan
+        )
+        primary_ip_v4 = factory.make_StaticIPAddress(
+            ip=factory.pick_ip_in_Subnet(subnet_v4),
+            alloc_type=IPADDRESS_TYPE.AUTO,
+            subnet=subnet_v4,
+            interface=primary_interface,
+        )
+        primary_ip_v6 = factory.make_StaticIPAddress(
+            ip=factory.pick_ip_in_Subnet(subnet_v6),
+            alloc_type=IPADDRESS_TYPE.AUTO,
+            subnet=subnet_v6,
+            interface=primary_interface,
+        )
+        secondary_interface = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, node=secondary_rack, vlan=vlan
+        )
+        secondary_ip_v4 = factory.make_StaticIPAddress(
+            ip=factory.pick_ip_in_Subnet(subnet_v4),
+            alloc_type=IPADDRESS_TYPE.AUTO,
+            subnet=subnet_v4,
+            interface=secondary_interface,
+        )
+        secondary_ip_v6 = factory.make_StaticIPAddress(
+            ip=factory.pick_ip_in_Subnet(subnet_v6),
+            alloc_type=IPADDRESS_TYPE.AUTO,
+            subnet=subnet_v6,
+            interface=secondary_interface,
+        )
+        failover_peer_name = "failover-vlan-%d" % vlan.id
+        self.assertEqual(
+            (
+                failover_peer_name,
+                {
+                    "name": failover_peer_name,
+                    "mode": "secondary",
+                    "address": str(secondary_ip_v4.ip),
+                    "peer_address": str(primary_ip_v4.ip),
+                },
+                primary_rack,
+            ),
+            dhcp.make_failover_peer_config(vlan, secondary_rack, 4),
+        )
+        self.assertEqual(
+            (
+                failover_peer_name,
+                {
+                    "name": failover_peer_name,
+                    "mode": "secondary",
+                    "address": str(secondary_ip_v6.ip),
+                    "peer_address": str(primary_ip_v6.ip),
+                },
+                primary_rack,
+            ),
+            dhcp.make_failover_peer_config(vlan, secondary_rack, 6),
         )