Merge ~bjornt/maas:bug-1988229-dhcp-snippet-relay into maas:master

[Björn Tillenius]

LP #1988229: dhcp snippet create fails when dhcp subnet is relayed regression

Extract logic to make it easier to test, and make use of get_dhcp_vlan, which
knows about dhcp relaying.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b bug-1988229-dhcp-snippet-relay lp:~bjornt/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 1b2b5b2b54024701278262d834b6d2eb42e74ae8

review: Approve

[Alberto Donato (ack)]

+1

review: Approve

ae560ac... by Björn Tillenius on 2022-11-15

Rename parameter.

diff --git a/src/maasserver/dhcp.py b/src/maasserver/dhcp.py
index ccbe8f3..32ad98a 100644
--- a/src/maasserver/dhcp.py
+++ b/src/maasserver/dhcp.py
@@ -977,6 +977,13 @@ def get_racks_by_subnet(subnet):
     return racks
 
 
+def _get_dhcp_rackcontrollers(dhcp_snippet):
+    if dhcp_snippet.subnet is not None:
+        return get_racks_by_subnet(dhcp_snippet.subnet)
+    elif dhcp_snippet.node is not None:
+        return dhcp_snippet.node.get_boot_rack_controllers()
+
+
 def validate_dhcp_config(test_dhcp_snippet=None):
     """Validate a DHCPD config with uncommitted values.
 
@@ -987,9 +994,6 @@ def validate_dhcp_config(test_dhcp_snippet=None):
     :param test_dhcp_snippet: A DHCPSnippet which has not yet been committed to
         the database and needs to be validated.
     """
-    # XXX ltrager 2016-03-28 - This only tests the existing config with new
-    # DHCPSnippets but could be expanded to test changes to the config(e.g
-    # subnets, omapi_key, interfaces, etc) before they are commited.
 
     def find_connected_rack(racks):
         connected_racks = [client.ident for client in getAllClients()]
@@ -1006,17 +1010,15 @@ def validate_dhcp_config(test_dhcp_snippet=None):
             "no available rack controller connected."
         )
 
-    rack_controller = None
+    # XXX ltrager 2016-03-28 - This only tests the existing config with new
+    # DHCPSnippets but could be expanded to test changes to the config(e.g
+    # subnets, omapi_key, interfaces, etc) before they are commited.
     # Test on the rack controller where the DHCPSnippet will be used
-    if test_dhcp_snippet is not None:
-        if test_dhcp_snippet.subnet is not None:
-            rack_controller = find_connected_rack(
-                get_racks_by_subnet(test_dhcp_snippet.subnet)
-            )
-        elif test_dhcp_snippet.node is not None:
-            rack_controller = find_connected_rack(
-                test_dhcp_snippet.node.get_boot_rack_controllers()
-            )
+    rack_controller = None
+    if test_dhcp_snippet is not None and not test_dhcp_snippet.is_global:
+        rack_controller = find_connected_rack(
+            _get_dhcp_rackcontrollers(test_dhcp_snippet)
+        )
     # If no rack controller is linked to the DHCPSnippet its a global DHCP
     # snippet which we can test anywhere.
     if rack_controller is None:
diff --git a/src/maasserver/models/dhcpsnippet.py b/src/maasserver/models/dhcpsnippet.py
index 7a401d7..054ccce 100644
--- a/src/maasserver/models/dhcpsnippet.py
+++ b/src/maasserver/models/dhcpsnippet.py
@@ -93,6 +93,10 @@ class DHCPSnippet(CleanSave, TimestampedModel):
     def __str__(self):
         return self.name
 
+    @property
+    def is_global(self):
+        return self.node_id is None and self.subnet_id is None
+
     def clean(self, *args, **kwargs):
         super().clean(*args, **kwargs)
         if self.node is not None and self.subnet is not None:
diff --git a/src/maasserver/models/node.py b/src/maasserver/models/node.py
index 06e80f8..ba2f710 100644
--- a/src/maasserver/models/node.py
+++ b/src/maasserver/models/node.py
@@ -130,7 +130,7 @@ from maasserver.models.physicalblockdevice import PhysicalBlockDevice
 from maasserver.models.resourcepool import ResourcePool
 from maasserver.models.service import Service
 from maasserver.models.staticipaddress import StaticIPAddress
-from maasserver.models.subnet import Subnet
+from maasserver.models.subnet import get_dhcp_vlan, Subnet
 from maasserver.models.tag import Tag
 from maasserver.models.timestampedmodel import now, TimestampedModel
 from maasserver.models.vlan import VLAN
@@ -5360,18 +5360,16 @@ class Node(CleanSave, TimestampedModel):
     def get_boot_rack_controllers(self):
         """Return the `RackController` that this node will boot from."""
         boot_interface = self.get_boot_interface()
-        if (
-            boot_interface is None
-            or boot_interface.vlan is None
-            or not boot_interface.vlan.dhcp_on
-        ):
+        if boot_interface is None:
             return []
-        else:
-            racks = [
-                boot_interface.vlan.primary_rack,
-                boot_interface.vlan.secondary_rack,
-            ]
-            return [rack for rack in racks if rack is not None]
+
+        boot_vlan = get_dhcp_vlan(boot_interface.vlan)
+        if boot_vlan is None:
+            return []
+        racks = [boot_vlan.primary_rack]
+        if boot_vlan.secondary_rack:
+            racks.append(boot_vlan.secondary_rack)
+        return racks
 
     def get_extra_macs(self):
         """Get the MACs other that the one the node booted from."""
diff --git a/src/maasserver/models/subnet.py b/src/maasserver/models/subnet.py
index 924aefa..870d4eb 100644
--- a/src/maasserver/models/subnet.py
+++ b/src/maasserver/models/subnet.py
@@ -1065,14 +1065,10 @@ def get_allocated_ips(subnets):
         yield subnet, mapping[subnet.id]
 
 
-def get_dhcp_vlan(subnet):
-    if subnet is None:
+def get_dhcp_vlan(vlan):
+    if vlan is None:
         return None
-    dhcp_vlan = (
-        subnet.vlan
-        if subnet.vlan.relay_vlan_id is None
-        else subnet.vlan.relay_vlan
-    )
+    dhcp_vlan = vlan if vlan.relay_vlan_id is None else vlan.relay_vlan
     if not dhcp_vlan.dhcp_on or dhcp_vlan.primary_rack is None:
         return None
     return dhcp_vlan
@@ -1104,7 +1100,9 @@ def get_boot_rackcontroller_ips(subnet):
 
     from maasserver.models.staticipaddress import StaticIPAddress
 
-    dhcp_vlan = get_dhcp_vlan(subnet)
+    dhcp_vlan = None
+    if subnet is not None:
+        dhcp_vlan = get_dhcp_vlan(subnet.vlan)
     if dhcp_vlan is None:
         return []
 
diff --git a/src/maasserver/models/tests/test_dhcpsnippet.py b/src/maasserver/models/tests/test_dhcpsnippet.py
index acefc80..465f445 100644
--- a/src/maasserver/models/tests/test_dhcpsnippet.py
+++ b/src/maasserver/models/tests/test_dhcpsnippet.py
@@ -138,3 +138,17 @@ class TestDHCPSnippet(MAASServerTestCase):
                 VersionedTextFile.objects.get,
                 id=i,
             )
+
+    def test_is_global(self):
+        snippet = factory.make_DHCPSnippet(subnet=None, node=None)
+        self.assertTrue(snippet.is_global)
+
+    def test_is_global_node(self):
+        node = factory.make_Machine()
+        snippet = factory.make_DHCPSnippet(subnet=None, node=node)
+        self.assertFalse(snippet.is_global)
+
+    def test_is_global_subnet(self):
+        subnet = factory.make_Subnet()
+        snippet = factory.make_DHCPSnippet(subnet=subnet, node=None)
+        self.assertFalse(snippet.is_global)
diff --git a/src/maasserver/tests/test_dhcp.py b/src/maasserver/tests/test_dhcp.py
index 58bee72..e9d0cfa 100644
--- a/src/maasserver/tests/test_dhcp.py
+++ b/src/maasserver/tests/test_dhcp.py
@@ -25,7 +25,7 @@ from twisted.internet.threads import deferToThread
 
 from maasserver import dhcp
 from maasserver import server_address as server_address_module
-from maasserver.dhcp import get_default_dns_servers
+from maasserver.dhcp import _get_dhcp_rackcontrollers, get_default_dns_servers
 from maasserver.enum import INTERFACE_TYPE, IPADDRESS_TYPE, SERVICE_STATUS
 from maasserver.models import (
     Config,
@@ -2923,6 +2923,88 @@ class TestConfigureDHCP(MAASTransactionServerTestCase):
         yield deferToDatabase(service_status_updated)
 
 
+class TestGetDHCPRackcontroller(MAASTransactionServerTestCase):
+    def create_dhcp_vlan(self, primary_address, secondary_adress=None):
+        dhcp_vlan = factory.make_VLAN()
+        dhcp_subnet = factory.make_Subnet(
+            vlan=dhcp_vlan, cidr=IPNetwork(primary_address).cidr
+        )
+        primary_rack = factory.make_rack_with_interfaces(
+            eth0=[primary_address]
+        )
+        secondary_rack = None
+        if secondary_adress is not None:
+            secondary_rack = factory.make_rack_with_interfaces(
+                eth0=[secondary_adress]
+            )
+        dhcp_vlan.dhcp_on = True
+        dhcp_vlan.primary_rack = primary_rack
+        dhcp_vlan.secondary_rack = secondary_rack
+        dhcp_vlan.save()
+        return dhcp_subnet
+
+    def test_subnet(self):
+        subnet = self.create_dhcp_vlan("10.10.10.2/24")
+        self.create_dhcp_vlan("10.10.20.2/24")
+        snippet = factory.make_DHCPSnippet(subnet=subnet, enabled=True)
+        self.assertCountEqual(
+            [subnet.vlan.primary_rack], _get_dhcp_rackcontrollers(snippet)
+        )
+
+    def test_subnet_includes_secondary(self):
+        subnet = self.create_dhcp_vlan("10.10.10.2/24", "10.10.10.3/24")
+        self.create_dhcp_vlan("10.10.20.0/24")
+        snippet = factory.make_DHCPSnippet(subnet=subnet, enabled=True)
+        self.assertCountEqual(
+            [subnet.vlan.primary_rack, subnet.vlan.secondary_rack],
+            _get_dhcp_rackcontrollers(snippet),
+        )
+
+    def test_subnet_relay(self):
+        dhcp_subnet = self.create_dhcp_vlan("10.10.10.2/24")
+        self.create_dhcp_vlan("10.10.20.2/24")
+        relay_subnet = factory.make_Subnet(cidr="10.10.30.0/24")
+        relay_subnet.vlan.relay_vlan = dhcp_subnet.vlan
+        relay_subnet.vlan.save()
+        snippet = factory.make_DHCPSnippet(subnet=relay_subnet, enabled=True)
+        self.assertCountEqual(
+            [dhcp_subnet.vlan.primary_rack], _get_dhcp_rackcontrollers(snippet)
+        )
+
+    def test_node(self):
+        subnet = self.create_dhcp_vlan("10.10.10.2/24")
+        self.create_dhcp_vlan("10.10.20.0/24")
+        node = factory.make_Machine_with_Interface_on_Subnet(subnet=subnet)
+        snippet = factory.make_DHCPSnippet(node=node, enabled=True)
+        self.assertCountEqual(
+            [subnet.vlan.primary_rack], _get_dhcp_rackcontrollers(snippet)
+        )
+
+    def test_node_includes_secondary(self):
+        subnet = self.create_dhcp_vlan("10.10.10.2/24", "10.10.10.3/24")
+        self.create_dhcp_vlan("10.10.20.0/24")
+        node = factory.make_Machine_with_Interface_on_Subnet(subnet=subnet)
+        snippet = factory.make_DHCPSnippet(node=node, enabled=True)
+        self.assertCountEqual(
+            [subnet.vlan.primary_rack, subnet.vlan.secondary_rack],
+            _get_dhcp_rackcontrollers(snippet),
+        )
+
+    def test_node_relay(self):
+        dhcp_subnet = self.create_dhcp_vlan("10.10.10.2/24")
+        self.create_dhcp_vlan("10.10.20.2/24")
+        relay_subnet = factory.make_Subnet(cidr="10.10.30.0/24")
+        relay_subnet.vlan.relay_vlan = dhcp_subnet.vlan
+        relay_subnet.vlan.save()
+        node = factory.make_Machine_with_Interface_on_Subnet(
+            subnet=relay_subnet
+        )
+        snippet = factory.make_DHCPSnippet(node=node, enabled=True)
+        self.assertCountEqual(
+            [dhcp_subnet.vlan.primary_rack], _get_dhcp_rackcontrollers(snippet)
+        )
+
+
 class TestValidateDHCPConfig(MAASTransactionServerTestCase):
     """Tests for `validate_dhcp_config`."""