Merge ~mpontillo/maas:commissioning-prevent-ip-creation-on-same-subnet--bug-1803188--2.4 into maas:2.4

[Mike Pontillo]

Backport 594f8f8bb: LP #1803188: Prevent creation of multiple IPs (per subnet, per interface) during commissioning.

In some situations, the IP address from the preboot environment differs
from the IP address acquired by the DHCP client during commissioning. This
results in MAAS linking the interface to the subnet twice, which is
unexpected and can cause problems for future usage of the machine.

[Mike Pontillo (mpontillo)]

Self-approve backport.

review: Approve

diff --git a/src/maasserver/models/interface.py b/src/maasserver/models/interface.py
index 5839e73..5e13965 100644
--- a/src/maasserver/models/interface.py
+++ b/src/maasserver/models/interface.py
@@ -724,6 +724,10 @@ class Interface(CleanSave, TimestampedModel):
         StaticIPAddress.objects.filter(
             interface=self, alloc_type=IPADDRESS_TYPE.DISCOVERED).delete()
 
+        # Keep track of which subnets were found, in order to avoid linking
+        # duplicates.
+        created_on_subnets = set()
+
         # Sort the cidr list by prefixlen.
         for ip in sorted(cidr_list, key=lambda x: int(x.split('/')[1])):
             network = IPNetwork(ip)
@@ -822,20 +826,41 @@ class Interface(CleanSave, TimestampedModel):
                             " on " + node.fqdn if node is not None else '')
                         prev_address.delete()
 
-            # XXX lamont 2016-11-03 Bug#1639090
             # At the moment, IPv6 autoconf (SLAAC) is required so that we get
             # the correct subnet block created above.  However, if we add SLAAC
             # addresses to the DB, then we wind up creating 2 autoassigned
             # addresses on the interface.  We need to discuss how to model them
             # and incorporate the change for 2.2.  For now, just drop them with
             # prejudice. (Bug#1639288)
-            if address != str(self._eui64_address(subnet.cidr)):
-                # Create the newly discovered IP address.
-                new_address = StaticIPAddress(
-                    alloc_type=IPADDRESS_TYPE.DISCOVERED, ip=address,
-                    subnet=subnet)
-                new_address.save()
-                self.ip_addresses.add(new_address)
+            if address == str(self._eui64_address(subnet.cidr)):
+                maaslog.warning(
+                    "IP address (%s)%s was skipped because "
+                    "it is an EUI-64 (SLAAC) address.",
+                    address,
+                    " on " + self.node.fqdn if self.node is not None else '')
+                continue
+
+            # Remember which subnets we created addresses on; we don't want to
+            # link more than one address per subnet in case of a duplicate.
+            # Duplicates could happen, in theory, if the IP address configured
+            # in the preboot environment differs from the IP address acquired
+            # by the DHCP client. See bug #1803188.
+            if subnet in created_on_subnets:
+                maaslog.warning(
+                    "IP address (%s)%s was skipped because it was found on "
+                    "the same subnet as a previous address: %s.",
+                    address,
+                    " on " + self.node.fqdn if self.node is not None else '',
+                    network)
+                continue
+
+            # Create the newly discovered IP address.
+            new_address = StaticIPAddress(
+                alloc_type=IPADDRESS_TYPE.DISCOVERED, ip=address,
+                subnet=subnet)
+            new_address.save()
+            self.ip_addresses.add(new_address)
+            created_on_subnets.add(subnet)
 
     def _eui64_address(self, net_cidr):
         """Return the SLAAC address for this interface."""
diff --git a/src/maasserver/models/tests/test_interface.py b/src/maasserver/models/tests/test_interface.py
index afb0d38..c7262ad 100644
--- a/src/maasserver/models/tests/test_interface.py
+++ b/src/maasserver/models/tests/test_interface.py
@@ -1912,6 +1912,24 @@ class UpdateIpAddressesTest(MAASServerTestCase):
         self.assertEqual(0, iface.ip_addresses.count())
         self.assertEqual(1, Subnet.objects.filter(cidr=network.cidr).count())
 
+    def test__does_not_add_addresses_from_duplicate_subnet(self):
+        # See also LP#1803188.
+        mac_address = factory.make_MAC()
+        vlan = factory.make_VLAN()
+        factory.make_Subnet(cidr="10.0.0.0/8", vlan=vlan)
+        factory.make_Subnet(cidr="2001::/64", vlan=vlan)
+        node = factory.make_Node()
+        iface = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, mac_address=mac_address, vlan=vlan,
+            node=node)
+        iface.update_ip_addresses([
+            "10.0.0.1/8",
+            "10.0.0.2/8",
+            "2001::1/64",
+            "2001::2/64",
+        ])
+        self.assertEqual(2, iface.ip_addresses.count())
+
     def test__finds_ipv6_subnet_regardless_of_order(self):
         iface = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
         network = factory.make_ipv6_network()