cloud-init

Merge ~smoser/cloud-init:bug/1669860-no-rename-bonds into cloud-init:master

  1. Git
  2. lp:~smoser/cloud-init
  3. bug/1669860-no-rename-bonds
  4. Merge into master
Proposed by Scott Moser on 2017-03-31
Status: Merged
Merged at revision: bf7723e8092bb1f8a442aa2399dd870e130a27d9
Proposed branch: ~smoser/cloud-init:bug/1669860-no-rename-bonds
Merge into: cloud-init:master
Diff against target: 212 lines (+135/-19)
2 files modified
cloudinit/net/__init__.py (+59/-19)
tests/unittests/test_net.py (+76/-0)
Reviewer Review Type Date Requested Status
Server Team CI bot continuous-integration Approve on 2017-03-31
Ryan Harper 2017-03-31 Approve on 2017-03-31
Review via email: mp+321578@code.launchpad.net

Commit Message

Fix bug that resulted in an attempt to rename bonds or vlans.

When cloud-init ran in the init stage (after networking had come up).
A bug could occur where cloud-init would attempt and fail to rename
network devices that had "inherited" mac addresses.

The intent of apply_network_config_names was always to rename only
the devices that were "physical" per the network config. (This would
include veth devices in a container). The bug was in creating
the dictionary of interfaces by mac address. If there were multiple
interfaces with the same mac address then renames could fail.
This situation was guaranteed to occur with bonds or vlans or other
devices that inherit their mac.

The solution is to change get_interfaces_by_mac to skip interfaces
that have an inherited mac.

LP: #1669860

To post a comment you must log in.
Ryan Harper (raharper) :
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Scott Moser (smoser) wrote :

Generally speaking, get_interfaces_by_mac was simply broken for
any occurance of duplicate mac addresses. From that perspective, this
is simply a fix. The reality was that the "last" nic (per os.listdir())
would be the one chosen, which is not even sorted() (unless /sys was returning
a directory list that way).

I went ahead and added a RuntimeError on duplicate macs found. That
makes this function safer.

As to whether or not all the callers are OK with this change,
the callers are:
 $ git grep -l get_interfaces_by_mac
 cloudinit/net/__init__.py
 cloudinit/sources/DataSourceOpenNebula.py
 cloudinit/sources/helpers/digitalocean.py
 cloudinit/sources/helpers/openstack.py
 tests/unittests/test_datasource/test_configdrive.py

cloudinit/net/__init__.py:
  this is just the definition and then the call that i just added.

cloudinit/sources/DataSourceOpenNebula.py:
 called from get_physical_nics_by_mac, which is pretty self
 explanatory, not expected to return bond or vlan. it then filters
 the returned list by 'net.is_physical()' so that would filter out
 the bond or vlan anyway.

cloudinit/sources/helpers/digitalocean.py:
 called from convert_network_configuration which says:
   Convert the DigitalOcean Network description into Cloud-init's netconfig
   format.

 This method is just looking for macs in the network data from
 digital ocean to convert that to names. Any vlan or bond here are
 going to break things.

cloudinit/sources/helpers/openstack.py
 called from 'convert_net_json' which is very similar to the digital ocean
 caller.

Server Team CI bot (server-team-bot) wrote :
review: Needs Fixing (continuous-integration)
Server Team CI bot (server-team-bot) wrote :
review: Needs Fixing (continuous-integration)
Ryan Harper (raharper) wrote :

Approved with the addition of a unittest, ideally a check that we don't attempt to rename a bond/bridge device.

review: Approve
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Scott Moser (smoser) wrote :

> Approved with the addition of a unittest, ideally a check that we don't
> attempt to rename a bond/bridge device.

I'll add a few unit tests for get_interfaces_by_mac that test that it does not show bonds or bridges, and raises exception on a duplicate mac.

Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/cloudinit/net/__init__.py b/cloudinit/net/__init__.py
2old mode 100755
3new mode 100644
4index 1cf98ef..346be5d
5--- a/cloudinit/net/__init__.py
6+++ b/cloudinit/net/__init__.py
7@@ -82,6 +82,10 @@ def is_wireless(devname):
8 return os.path.exists(sys_dev_path(devname, "wireless"))
9
10
11+def is_bridge(devname):
12+ return os.path.exists(sys_dev_path(devname, "bridge"))
13+
14+
15 def is_connected(devname):
16 # is_connected isn't really as simple as that. 2 is
17 # 'physically connected'. 3 is 'not connected'. but a wlan interface will
18@@ -132,7 +136,7 @@ def generate_fallback_config():
19 for interface in potential_interfaces:
20 if interface.startswith("veth"):
21 continue
22- if os.path.exists(sys_dev_path(interface, "bridge")):
23+ if is_bridge(interface):
24 # skip any bridges
25 continue
26 carrier = read_sys_net_int(interface, 'carrier')
27@@ -187,7 +191,11 @@ def apply_network_config_names(netcfg, strict_present=True, strict_busy=True):
28 """read the network config and rename devices accordingly.
29 if strict_present is false, then do not raise exception if no devices
30 match. if strict_busy is false, then do not raise exception if the
31- device cannot be renamed because it is currently configured."""
32+ device cannot be renamed because it is currently configured.
33+
34+ renames are only attempted for interfaces of type 'physical'. It is
35+ expected that the network system will create other devices with the
36+ correct name in place."""
37 renames = []
38 for ent in netcfg.get('config', {}):
39 if ent.get('type') != 'physical':
40@@ -201,13 +209,35 @@ def apply_network_config_names(netcfg, strict_present=True, strict_busy=True):
41 return _rename_interfaces(renames)
42
43
44+def interface_has_own_mac(ifname, strict=False):
45+ """return True if the provided interface has its own address.
46+
47+ Based on addr_assign_type in /sys. Return true for any interface
48+ that does not have a 'stolen' address. Examples of such devices
49+ are bonds or vlans that inherit their mac from another device.
50+ Possible values are:
51+ 0: permanent address 2: stolen from another device
52+ 1: randomly generated 3: set using dev_set_mac_address"""
53+
54+ assign_type = read_sys_net_int(ifname, "addr_assign_type")
55+ if strict and assign_type is None:
56+ raise ValueError("%s had no addr_assign_type.")
57+ return assign_type in (0, 1, 3)
58+
59+
60 def _get_current_rename_info(check_downable=True):
61- """Collect information necessary for rename_interfaces."""
62- names = get_devicelist()
63+ """Collect information necessary for rename_interfaces.
64+
65+ returns a dictionary by mac address like:
66+ {mac:
67+ {'name': name
68+ 'up': boolean: is_up(name),
69+ 'downable': None or boolean indicating that the
70+ device has only automatically assigned ip addrs.}}
71+ """
72 bymac = {}
73- for n in names:
74- bymac[get_interface_mac(n)] = {
75- 'name': n, 'up': is_up(n), 'downable': None}
76+ for mac, name in get_interfaces_by_mac().items():
77+ bymac[mac] = {'name': name, 'up': is_up(name), 'downable': None}
78
79 if check_downable:
80 nmatch = re.compile(r"[0-9]+:\s+(\w+)[@:]")
81@@ -346,22 +376,32 @@ def get_interface_mac(ifname):
82 return read_sys_net_safe(ifname, path)
83
84
85-def get_interfaces_by_mac(devs=None):
86- """Build a dictionary of tuples {mac: name}"""
87- if devs is None:
88- try:
89- devs = get_devicelist()
90- except OSError as e:
91- if e.errno == errno.ENOENT:
92- devs = []
93- else:
94- raise
95+def get_interfaces_by_mac():
96+ """Build a dictionary of tuples {mac: name}.
97+
98+ Bridges and any devices that have a 'stolen' mac are excluded."""
99+ try:
100+ devs = get_devicelist()
101+ except OSError as e:
102+ if e.errno == errno.ENOENT:
103+ devs = []
104+ else:
105+ raise
106 ret = {}
107 for name in devs:
108+ if not interface_has_own_mac(name):
109+ continue
110+ if is_bridge(name):
111+ continue
112 mac = get_interface_mac(name)
113 # some devices may not have a mac (tun0)
114- if mac:
115- ret[mac] = name
116+ if not mac:
117+ continue
118+ if mac in ret:
119+ raise RuntimeError(
120+ "duplicate mac found! both '%s' and '%s' have mac '%s'" %
121+ (name, ret[mac], mac))
122+ ret[mac] = name
123 return ret
124
125
126diff --git a/tests/unittests/test_net.py b/tests/unittests/test_net.py
127index bfd04ba..9cc5e4a 100644
128--- a/tests/unittests/test_net.py
129+++ b/tests/unittests/test_net.py
130@@ -1461,6 +1461,82 @@ class TestNetRenderers(CiTestCase):
131 priority=['sysconfig', 'eni'])
132
133
134+class TestGetInterfacesByMac(CiTestCase):
135+ _data = {'devices': ['enp0s1', 'enp0s2', 'bond1', 'bridge1',
136+ 'bridge1-nic', 'tun0'],
137+ 'bonds': ['bond1'],
138+ 'bridges': ['bridge1'],
139+ 'own_macs': ['enp0s1', 'enp0s2', 'bridge1-nic', 'bridge1'],
140+ 'macs': {'enp0s1': 'aa:aa:aa:aa:aa:01',
141+ 'enp0s2': 'aa:aa:aa:aa:aa:02',
142+ 'bond1': 'aa:aa:aa:aa:aa:01',
143+ 'bridge1': 'aa:aa:aa:aa:aa:03',
144+ 'bridge1-nic': 'aa:aa:aa:aa:aa:03',
145+ 'tun0': None}}
146+ data = {}
147+
148+ def _se_get_devicelist(self):
149+ return self.data['devices']
150+
151+ def _se_get_interface_mac(self, name):
152+ return self.data['macs'][name]
153+
154+ def _se_is_bridge(self, name):
155+ return name in self.data['bridges']
156+
157+ def _se_interface_has_own_mac(self, name):
158+ return name in self.data['own_macs']
159+
160+ def _mock_setup(self):
161+ self.data = copy.deepcopy(self._data)
162+ mocks = ('get_devicelist', 'get_interface_mac', 'is_bridge',
163+ 'interface_has_own_mac')
164+ self.mocks = {}
165+ for n in mocks:
166+ m = mock.patch('cloudinit.net.' + n,
167+ side_effect=getattr(self, '_se_' + n))
168+ self.addCleanup(m.stop)
169+ self.mocks[n] = m.start()
170+
171+ def test_raise_exception_on_duplicate_macs(self):
172+ self._mock_setup()
173+ self.data['macs']['bridge1-nic'] = self.data['macs']['enp0s1']
174+ self.assertRaises(RuntimeError, net.get_interfaces_by_mac)
175+
176+ def test_excludes_any_without_mac_address(self):
177+ self._mock_setup()
178+ ret = net.get_interfaces_by_mac()
179+ self.assertIn('tun0', self._se_get_devicelist())
180+ self.assertNotIn('tun0', ret.values())
181+
182+ def test_excludes_stolen_macs(self):
183+ self._mock_setup()
184+ ret = net.get_interfaces_by_mac()
185+ self.mocks['interface_has_own_mac'].assert_has_calls(
186+ [mock.call('enp0s1'), mock.call('bond1')], any_order=True)
187+ self.assertEqual(
188+ {'aa:aa:aa:aa:aa:01': 'enp0s1', 'aa:aa:aa:aa:aa:02': 'enp0s2',
189+ 'aa:aa:aa:aa:aa:03': 'bridge1-nic'},
190+ ret)
191+
192+ def test_excludes_bridges(self):
193+ self._mock_setup()
194+ # add a device 'b1', make all return they have their "own mac",
195+ # set everything other than 'b1' to be a bridge.
196+ # then expect b1 is the only thing left.
197+ self.data['macs']['b1'] = 'aa:aa:aa:aa:aa:b1'
198+ self.data['devices'].append('b1')
199+ self.data['bonds'] = []
200+ self.data['own_macs'] = self.data['devices']
201+ self.data['bridges'] = [f for f in self.data['devices'] if f != "b1"]
202+ ret = net.get_interfaces_by_mac()
203+ self.assertEqual({'aa:aa:aa:aa:aa:b1': 'b1'}, ret)
204+ self.mocks['is_bridge'].assert_has_calls(
205+ [mock.call('bridge1'), mock.call('enp0s1'), mock.call('bond1'),
206+ mock.call('b1')],
207+ any_order=True)
208+
209+
210 def _gzip_data(data):
211 with io.BytesIO() as iobuf:
212 gzfp = gzip.GzipFile(mode="wb", fileobj=iobuf)

Subscribers

People subscribed via source and target branches

to all changes: