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

Proposed by Mike Pontillo on 2018-12-11
Status: Merged
Approved by: Mike Pontillo on 2018-12-11
Approved revision: cb149591de8a698dd578e041b25a1a7bbbd7b13d
Merged at revision: fab931122fd159f31eba70ea52618dfee715c6d9
Proposed branch: ~mpontillo/maas:commissioning-prevent-ip-creation-on-same-subnet--bug-1803188--2.4
Merge into: maas:2.4
Diff against target: 94 lines (+51/-8)
2 files modified
src/maasserver/models/interface.py (+33/-8)
src/maasserver/models/tests/test_interface.py (+18/-0)
Reviewer Review Type Date Requested Status
Mike Pontillo (community) Approve on 2018-12-11
MAAS Lander unittests Pending
Review via email: mp+360729@code.launchpad.net

Commit message

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.

To post a comment you must log in.
Mike Pontillo (mpontillo) wrote :

Self-approve backport.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/src/maasserver/models/interface.py b/src/maasserver/models/interface.py
2index 5839e73..5e13965 100644
3--- a/src/maasserver/models/interface.py
4+++ b/src/maasserver/models/interface.py
5@@ -724,6 +724,10 @@ class Interface(CleanSave, TimestampedModel):
6 StaticIPAddress.objects.filter(
7 interface=self, alloc_type=IPADDRESS_TYPE.DISCOVERED).delete()
8
9+ # Keep track of which subnets were found, in order to avoid linking
10+ # duplicates.
11+ created_on_subnets = set()
12+
13 # Sort the cidr list by prefixlen.
14 for ip in sorted(cidr_list, key=lambda x: int(x.split('/')[1])):
15 network = IPNetwork(ip)
16@@ -822,20 +826,41 @@ class Interface(CleanSave, TimestampedModel):
17 " on " + node.fqdn if node is not None else '')
18 prev_address.delete()
19
20- # XXX lamont 2016-11-03 Bug#1639090
21 # At the moment, IPv6 autoconf (SLAAC) is required so that we get
22 # the correct subnet block created above. However, if we add SLAAC
23 # addresses to the DB, then we wind up creating 2 autoassigned
24 # addresses on the interface. We need to discuss how to model them
25 # and incorporate the change for 2.2. For now, just drop them with
26 # prejudice. (Bug#1639288)
27- if address != str(self._eui64_address(subnet.cidr)):
28- # Create the newly discovered IP address.
29- new_address = StaticIPAddress(
30- alloc_type=IPADDRESS_TYPE.DISCOVERED, ip=address,
31- subnet=subnet)
32- new_address.save()
33- self.ip_addresses.add(new_address)
34+ if address == str(self._eui64_address(subnet.cidr)):
35+ maaslog.warning(
36+ "IP address (%s)%s was skipped because "
37+ "it is an EUI-64 (SLAAC) address.",
38+ address,
39+ " on " + self.node.fqdn if self.node is not None else '')
40+ continue
41+
42+ # Remember which subnets we created addresses on; we don't want to
43+ # link more than one address per subnet in case of a duplicate.
44+ # Duplicates could happen, in theory, if the IP address configured
45+ # in the preboot environment differs from the IP address acquired
46+ # by the DHCP client. See bug #1803188.
47+ if subnet in created_on_subnets:
48+ maaslog.warning(
49+ "IP address (%s)%s was skipped because it was found on "
50+ "the same subnet as a previous address: %s.",
51+ address,
52+ " on " + self.node.fqdn if self.node is not None else '',
53+ network)
54+ continue
55+
56+ # Create the newly discovered IP address.
57+ new_address = StaticIPAddress(
58+ alloc_type=IPADDRESS_TYPE.DISCOVERED, ip=address,
59+ subnet=subnet)
60+ new_address.save()
61+ self.ip_addresses.add(new_address)
62+ created_on_subnets.add(subnet)
63
64 def _eui64_address(self, net_cidr):
65 """Return the SLAAC address for this interface."""
66diff --git a/src/maasserver/models/tests/test_interface.py b/src/maasserver/models/tests/test_interface.py
67index afb0d38..c7262ad 100644
68--- a/src/maasserver/models/tests/test_interface.py
69+++ b/src/maasserver/models/tests/test_interface.py
70@@ -1912,6 +1912,24 @@ class UpdateIpAddressesTest(MAASServerTestCase):
71 self.assertEqual(0, iface.ip_addresses.count())
72 self.assertEqual(1, Subnet.objects.filter(cidr=network.cidr).count())
73
74+ def test__does_not_add_addresses_from_duplicate_subnet(self):
75+ # See also LP#1803188.
76+ mac_address = factory.make_MAC()
77+ vlan = factory.make_VLAN()
78+ factory.make_Subnet(cidr="10.0.0.0/8", vlan=vlan)
79+ factory.make_Subnet(cidr="2001::/64", vlan=vlan)
80+ node = factory.make_Node()
81+ iface = factory.make_Interface(
82+ INTERFACE_TYPE.PHYSICAL, mac_address=mac_address, vlan=vlan,
83+ node=node)
84+ iface.update_ip_addresses([
85+ "10.0.0.1/8",
86+ "10.0.0.2/8",
87+ "2001::1/64",
88+ "2001::2/64",
89+ ])
90+ self.assertEqual(2, iface.ip_addresses.count())
91+
92 def test__finds_ipv6_subnet_regardless_of_order(self):
93 iface = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
94 network = factory.make_ipv6_network()

Subscribers

People subscribed via source and target branches