Merge ~ack/maas:1930227-full-netmask-fix into maas:master

[Alberto Donato]

LP:1930227 - fix netmask for /32 and /128 to match actual subnet

[MAAS Lander (maas-lander)]

UNIT TESTS
-b 1930227-full-netmask-fix lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 2b137cd95c26c6001f4a23d447c334aa374292e5

review: Approve

[Björn Tillenius (bjornt)]

+1

review: Approve

diff --git a/src/metadataserver/builtin_scripts/network.py b/src/metadataserver/builtin_scripts/network.py
index 44d8691..ed1e0f1 100644
--- a/src/metadataserver/builtin_scripts/network.py
+++ b/src/metadataserver/builtin_scripts/network.py
@@ -16,6 +16,7 @@ from maasserver.models.vlan import VLAN
 from maasserver.utils.orm import transactional
 from provisioningserver.logger import get_maas_logger
 from provisioningserver.utils import flatten, sorttop
+from provisioningserver.utils.network import fix_link_addresses
 from provisioningserver.utils.twisted import synchronous
 
 maaslog = get_maas_logger("metadataserver.network")
@@ -160,13 +161,17 @@ def update_interface(node, name, data, address_extra, hints=None):
         # same, hard-coded OpenBMC MAC address.
         return None
 
-    links = [
-        address.copy()
-        for address in network["addresses"]
-        if address["scope"] == "global"
-    ]
-    for link in links:
+    links = []
+    for address in network["addresses"]:
+        if address["scope"] != "global":
+            continue
+        link = address.copy()
         link.update(address_extra.get(link["address"], {}))
+        links.append(link)
+    # fix networks that have a /32 or /128 netmask to the closest wider known
+    # subnet
+    fix_link_addresses(links)
+
     if network["vlan"]:
         return update_vlan_interface(node, name, network, links)
     elif network["bond"] or network["bridge"]:
diff --git a/src/metadataserver/builtin_scripts/tests/test_network.py b/src/metadataserver/builtin_scripts/tests/test_network.py
index c8667d5..1162ba4 100644
--- a/src/metadataserver/builtin_scripts/tests/test_network.py
+++ b/src/metadataserver/builtin_scripts/tests/test_network.py
@@ -2419,6 +2419,32 @@ class TestUpdateInterfaces(MAASServerTestCase, UpdateInterfacesMixin):
         self.assertTrue(eth0.enabled)
         self.assertEqual(data.networks["eth0"].hwaddr, eth0.mac_address)
 
+    def test_fixes_link_netmasks(self):
+        controller = self.create_empty_controller(with_empty_script_sets=True)
+        data = FakeCommissioningData()
+        card = data.create_network_card()
+        card.vendor = factory.make_name("vendor")
+        card.product = factory.make_name("product")
+        card.firmware_version = factory.make_name("firmware_version")
+        network = data.create_physical_network("eth0", card=card)
+        network.addresses = [
+            LXDAddress("192.168.0.1", 24),
+            LXDAddress("192.168.0.2", 32),
+            LXDAddress("2001::aaaa:1", 112),
+            LXDAddress("2001::aaaa:2", 128),
+        ]
+        self.update_interfaces(controller, data)
+        eth0 = Interface.objects.get(name="eth0", node=controller)
+        self.assertItemsEqual(
+            eth0.ip_addresses.values_list("ip", "subnet__cidr"),
+            [
+                ("192.168.0.1", "192.168.0.0/24"),
+                ("192.168.0.2", "192.168.0.0/24"),
+                ("2001::aaaa:1", "2001::aaaa:0/112"),
+                ("2001::aaaa:2", "2001::aaaa:0/112"),
+            ],
+        )
+
 
 class TestUpdateInterfacesWithHints(
     MAASTransactionServerTestCase, UpdateInterfacesMixin
diff --git a/src/provisioningserver/utils/network.py b/src/provisioningserver/utils/network.py
index 794521b..d8c781e 100644
--- a/src/provisioningserver/utils/network.py
+++ b/src/provisioningserver/utils/network.py
@@ -939,6 +939,11 @@ def get_mac_organization(mac):
 def fix_link_addresses(links):
     """Fix the addresses defined in `links`.
 
+    Each link entry can contain addresses in the form:
+       {"address": "1.2.3.4/24", ...}
+    or
+       {"address": "1.2.3.4", "netmask": 24, ...}
+
     Some address will have a prefixlen of 32 or 128 depending if IPv4 or IPv6.
     Fix those address to fall within a subnet that is already defined in
     another link. The addresses that get fixed will be placed into the smallest
@@ -952,7 +957,10 @@ def fix_link_addresses(links):
     # Loop through and build a list of subnets where the prefixlen is not
     # 32 or 128 for IPv4 and IPv6 respectively.
     for link in links:
-        ip_addr = IPNetwork(link["address"])
+        if "netmask" in link:
+            ip_addr = IPNetwork(f"{link['address']}/{link['netmask']}")
+        else:
+            ip_addr = IPNetwork(link["address"])
         if ip_addr.version == 4:
             if ip_addr.prefixlen == 32:
                 links_v4.append(link)
@@ -964,26 +972,25 @@ def fix_link_addresses(links):
             else:
                 subnets_v6.append(ip_addr.cidr)
 
-    # Sort the subnets so the smallest prefixlen is first.
-    subnets_v4 = sorted(subnets_v4, key=attrgetter("prefixlen"), reverse=True)
-    subnets_v6 = sorted(subnets_v6, key=attrgetter("prefixlen"), reverse=True)
-
-    # Fix all addresses that have prefixlen of 32 or 128 that fit in inside
-    # one of the already defined subnets.
-    for link in links_v4:
-        ip_addr = IPNetwork(link["address"])
-        for subnet in subnets_v4:
-            if ip_addr.ip in subnet:
-                ip_addr.prefixlen = subnet.prefixlen
-                link["address"] = str(ip_addr)
-                break
-    for link in links_v6:
-        ip_addr = IPNetwork(link["address"])
-        for subnet in subnets_v6:
-            if ip_addr.ip in subnet:
-                ip_addr.prefixlen = subnet.prefixlen
-                link["address"] = str(ip_addr)
-                break
+    for links, subnets in ((links_v4, subnets_v4), (links_v6, subnets_v6)):
+        subnets = sorted(subnets, key=attrgetter("prefixlen"), reverse=True)
+        # Fix all addresses that have prefixlen of 32 or 128 that fit in inside
+        # one of the already defined subnets.
+        for link in links:
+            has_separate_netmask = "netmask" in link
+            if has_separate_netmask:
+                ip_addr = IPNetwork(f"{link['address']}/{link['netmask']}")
+            else:
+                ip_addr = IPNetwork(link["address"])
+
+            for subnet in subnets:
+                if ip_addr.ip in subnet:
+                    if has_separate_netmask:
+                        link["netmask"] = subnet.prefixlen
+                    else:
+                        ip_addr.prefixlen = subnet.prefixlen
+                        link["address"] = str(ip_addr)
+                    break
 
 
 def fix_link_gateways(links, iproute_info):
diff --git a/src/provisioningserver/utils/tests/test_network.py b/src/provisioningserver/utils/tests/test_network.py
index 6a0d438..3b96869 100644
--- a/src/provisioningserver/utils/tests/test_network.py
+++ b/src/provisioningserver/utils/tests/test_network.py
@@ -1,15 +1,12 @@
 # Copyright 2014-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-"""Tests for network helpers."""
-
-
 import itertools
 import json
 import socket
 from socket import EAI_BADFLAGS, EAI_NODATA, EAI_NONAME, gaierror, IPPROTO_TCP
 from typing import List
-from unittest import mock
+from unittest import mock, TestCase
 from unittest.mock import Mock
 
 from netaddr import EUI, IPAddress, IPNetwork, IPRange
@@ -57,6 +54,7 @@ from provisioningserver.utils.network import (
     enumerate_ipv4_addresses,
     find_ip_via_arp,
     find_mac_via_arp,
+    fix_link_addresses,
     format_eui,
     generate_mac_address,
     get_all_addresses_for_interface,
@@ -94,6 +92,44 @@ from provisioningserver.utils.network import (
 from provisioningserver.utils.shell import get_env_with_locale
 
 
+class TestFixLinkAddresses(TestCase):
+    def test_fixes_with_cidr(self):
+        links = [
+            {"address": "1.2.3.1/24", "mode": "static"},
+            {"address": "1.2.3.4/32", "mode": "static"},
+            {"address": "2001::1/96", "mode": "static"},
+            {"address": "2001::123/128", "mode": "static"},
+        ]
+        fix_link_addresses(links)
+        self.assertEqual(
+            links,
+            [
+                {"address": "1.2.3.1/24", "mode": "static"},
+                {"address": "1.2.3.4/24", "mode": "static"},
+                {"address": "2001::1/96", "mode": "static"},
+                {"address": "2001::123/96", "mode": "static"},
+            ],
+        )
+
+    def test_fixes_with_netmask(self):
+        links = [
+            {"address": "1.2.3.1", "netmask": 24, "mode": "static"},
+            {"address": "1.2.3.4", "netmask": 32, "mode": "static"},
+            {"address": "2001::1", "netmask": 96, "mode": "static"},
+            {"address": "2001::123", "netmask": 128, "mode": "static"},
+        ]
+        fix_link_addresses(links)
+        self.assertEqual(
+            links,
+            [
+                {"address": "1.2.3.1", "netmask": 24, "mode": "static"},
+                {"address": "1.2.3.4", "netmask": 24, "mode": "static"},
+                {"address": "2001::1", "netmask": 96, "mode": "static"},
+                {"address": "2001::123", "netmask": 96, "mode": "static"},
+            ],
+        )
+
+
 class TestMakeNetwork(MAASTestCase):
     def test_constructs_IPNetwork(self):
         network = make_network("10.22.82.0", 24)