Merge ~chris.macnaughton/ubuntu/+source/neutron:bug/1881157 into ~ubuntu-server-dev/ubuntu/+source/neutron:stable/queens

Proposed by Chris MacNaughton on 2020-09-22
Status: Needs review
Proposed branch: ~chris.macnaughton/ubuntu/+source/neutron:bug/1881157
Merge into: ~ubuntu-server-dev/ubuntu/+source/neutron:stable/queens
Diff against target: 425 lines (+403/-0)
3 files modified
debian/changelog (+7/-0)
debian/patches/lp1881157-remote-sg-is-left-behind.patch (+395/-0)
debian/patches/series (+1/-0)
Reviewer Review Type Date Requested Status
Corey Bryant 2020-09-22 Approve on 2020-09-22
Review via email: mp+391122@code.launchpad.net
To post a comment you must log in.
Corey Bryant (corey.bryant) wrote :

Seems fine on a quick glance. We'll need to get point releases uploaded for > queens before we can upload this one. Let's hold off on merging until then in case something else comes up ahead of this.

Corey Bryant (corey.bryant) wrote :

Diff with stable/queens patch looks good. Looks like there was a quilt refresh to adapt to the current state of the package branch. Approving for now but not merging yet.

review: Approve

Agreed - ideally we'll get that neutron point release out soon which will give us S/T/U more easily!

Unmerged commits

0489eea... by Chris MacNaughton on 2020-09-22

d/p/lp1881157-remote-sg-is-left-behind.patch: Backport fix for security group IDs being left behind (LP: #1881157).

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/debian/changelog b/debian/changelog
2index 75f60a5..70e65db 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,10 @@
6+neutron (2:12.1.1-0ubuntu3) UNRELEASED; urgency=medium
7+
8+ * d/p/lp1881157-remote-sg-is-left-behind.patch: Backport fix for security
9+ group IDs being left behind (LP: #1881157).
10+
11+ -- Chris MacNaughton <chris.macnaughton@canonical.com> Tue, 22 Sep 2020 11:16:27 +0000
12+
13 neutron (2:12.1.1-0ubuntu2) bionic; urgency=medium
14
15 * d/p/Ensure-fip-ip-rules-deleted-when-fip-removed.patch
16diff --git a/debian/patches/lp1881157-remote-sg-is-left-behind.patch b/debian/patches/lp1881157-remote-sg-is-left-behind.patch
17new file mode 100644
18index 0000000..fda14bc
19--- /dev/null
20+++ b/debian/patches/lp1881157-remote-sg-is-left-behind.patch
21@@ -0,0 +1,395 @@
22+From d0b696f076a94830b040ffcb2dec28e2eabe2e04 Mon Sep 17 00:00:00 2001
23+From: Rodolfo Alonso Hernandez <ralonsoh@redhat.com>
24+Date: Tue, 02 Jun 2020 17:09:07 +0000
25+Subject: [PATCH] [OVS][FW] Remote SG IDs left behind when a SG is removed
26+
27+When any port in the OVS agent is using a security groups (SG) and
28+this SG is removed, is marked to be deleted. This deletion process
29+is done in [1].
30+
31+The SG deletion process consists on removing any reference of this SG
32+from the firewall and the SG port map. The firewall removes this SG in
33+[2].
34+
35+The information of a SG is stored in:
36+* ConjIPFlowManager.conj_id_map = ConjIdMap(). This class stores the
37+ conjunction IDS (conj_ids) in a dictionary using the following keys:
38+
39+ ConjIdMap.id_map[(sg_id, remote_sg_id, direction, ethertype,
40+ conj_ids)] = conj_id_XXX
41+
42+* ConjIPFlowManager.conj_ids is a nested dictionary, built in the
43+ following way:
44+
45+ self.conj_ids[vlan_tag][(direction, ethertype)][remote_sg_id] = \
46+ set([conj_id_1, conj_id_2, ...])
47+
48+This patch stores all conjuntion IDs generated and assigned to the
49+tuple (sg_id, remote_sg_id, direction, ethertype). When a SG is
50+removed, the deletion method will look for this SG in the new storage
51+variable created, ConjIdMap.id_map_group, and will mark all the
52+conjuntion IDs related to be removed. That will cleanup those rules
53+left in the OVS matching:
54+ action=conjunction(conj_id, 1/2)
55+
56+[1]https://github.com/openstack/neutron/blob/118930f03d31f157f8c7a9e6c57122ecea8982b9/neutron/agent/linux/openvswitch_firewall/firewall.py#L731
57+[2]https://github.com/openstack/neutron/blob/118930f03d31f157f8c7a9e6c57122ecea8982b9/neutron/agent/linux/openvswitch_firewall/firewall.py#L399
58+
59+Conflicts:
60+ neutron/agent/linux/openvswitch_firewall/firewall.py
61+ neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py
62+
63+Change-Id: I63e446a30cf10e7bcd34a6f0d6ba1711301efcbe
64+Related-Bug: #1881157
65+(cherry picked from commit 0eebd002ccda66dc6d9f9e5a254815109225e299)
66+(cherry picked from commit ed22f7a2ff19a874bc8521f84cb4fd1c7483a23f)
67+(cherry picked from commit 6615f248e25a361d988db1795a3486a58f4768cb)
68+---
69+
70+diff --git a/neutron/agent/linux/openvswitch_firewall/firewall.py b/neutron/agent/linux/openvswitch_firewall/firewall.py
71+index 3570032..b8522b2 100644
72+--- a/neutron/agent/linux/openvswitch_firewall/firewall.py
73++++ b/neutron/agent/linux/openvswitch_firewall/firewall.py
74+@@ -268,6 +268,10 @@
75+
76+ def __init__(self):
77+ self.id_map = collections.defaultdict(self._conj_id_factory)
78++ # Stores the set of conjuntion IDs used for each unique tuple
79++ # (sg_id, remote_sg_id, direction, ethertype). Each tuple can have up
80++ # to 8 conjuntion IDs (see ConjIPFlowManager.add()).
81++ self.id_map_group = collections.defaultdict(set)
82+ self.id_free = collections.deque()
83+ self.max_id = 0
84+
85+@@ -298,12 +302,21 @@
86+ return a list of (remote_sg_id, conj_id), which are no longer
87+ in use.
88+ """
89+- result = []
90++ result = set([])
91+ for k in list(self.id_map.keys()):
92+ if sg_id in k[0:2]:
93+ conj_id = self.id_map.pop(k)
94+- result.append((k[1], conj_id))
95++ result.add((k[1], conj_id))
96+ self.id_free.append(conj_id)
97++
98++ # If the remote_sg_id is removed, the tuple (sg_id, remote_sg_id,
99++ # direction, ethertype) no longer exists; the conjunction IDs assigned
100++ # to this tuple should be removed too.
101++ for k in list(self.id_map_group.keys()):
102++ if sg_id in k[0:2]:
103++ conj_id_groups = self.id_map_group.pop(k, [])
104++ for conj_id in conj_id_groups:
105++ result.add((k[1], conj_id))
106+
107+ return result
108+
109+@@ -346,12 +359,22 @@
110+ return addr_to_conj
111+
112+ def _update_flows_for_vlan_subr(self, direction, ethertype, vlan_tag,
113+- flow_state, addr_to_conj):
114++ flow_state, addr_to_conj,
115++ conj_id_to_remove):
116+ """Do the actual flow updates for given direction and ethertype."""
117+- current_ips = set(flow_state.keys())
118+- self.driver.delete_flows_for_ip_addresses(
119+- current_ips - set(addr_to_conj.keys()),
120+- direction, ethertype, vlan_tag)
121++ conj_id_to_remove = conj_id_to_remove or []
122++ # Delete any current flow related to any deleted IP address, before
123++ # creating the flows for the current IPs.
124++ self.driver.delete_flows_for_flow_state(
125++ flow_state, addr_to_conj, direction, ethertype, vlan_tag)
126++ for conj_id_set in conj_id_to_remove:
127++ # Remove any remaining flow with remote SG ID conj_id_to_remove
128++ for current_ip, conj_ids in flow_state.items():
129++ conj_ids_to_remove = conj_id_set & set(conj_ids)
130++ self.driver.delete_flow_for_ip(
131++ current_ip, direction, ethertype, vlan_tag,
132++ conj_ids_to_remove)
133++
134+ for addr, conj_ids in addr_to_conj.items():
135+ conj_ids.sort()
136+ if flow_state.get(addr) == conj_ids:
137+@@ -360,7 +383,7 @@
138+ addr, direction, ethertype, vlan_tag, conj_ids):
139+ self.driver._add_flow(**flow)
140+
141+- def update_flows_for_vlan(self, vlan_tag):
142++ def update_flows_for_vlan(self, vlan_tag, conj_id_to_remove=None):
143+ """Install action=conjunction(conj_id, 1/2) flows,
144+ which depend on IP addresses of remote_group_id.
145+ """
146+@@ -372,7 +395,7 @@
147+ ethertype, sg_conj_id_map)
148+ self._update_flows_for_vlan_subr(direction, ethertype, vlan_tag,
149+ self.flow_state[vlan_tag][(direction, ethertype)],
150+- addr_to_conj)
151++ addr_to_conj, conj_id_to_remove)
152+ self.flow_state[vlan_tag][(direction, ethertype)] = addr_to_conj
153+
154+ def add(self, vlan_tag, sg_id, remote_sg_id, direction, ethertype,
155+@@ -393,32 +416,43 @@
156+ collections.defaultdict(set))
157+ self.conj_ids[vlan_tag][(direction, ethertype)][remote_sg_id].add(
158+ conj_id)
159++
160++ conj_id_tuple = (sg_id, remote_sg_id, direction, ethertype)
161++ self.conj_id_map.id_map_group[conj_id_tuple].add(conj_id)
162+ return conj_id
163+
164+ def sg_removed(self, sg_id):
165+ """Handle SG removal events.
166+
167+- Free all conj_ids associated with the sg_id and clean up
168++ Free all conj_ids associated with the sg_id removed and clean up
169+ obsolete entries from the self.conj_ids map. Unlike the add
170+ method, it also updates flows.
171++ If a SG is removed, both sg_id and remote_sg_id should be removed from
172++ the "vlan_conj_id_map".
173+ """
174+- id_list = self.conj_id_map.delete_sg(sg_id)
175++ id_set = self.conj_id_map.delete_sg(sg_id)
176+ unused_dict = collections.defaultdict(set)
177+- for remote_sg_id, conj_id in id_list:
178++ for remote_sg_id, conj_id in id_set:
179+ unused_dict[remote_sg_id].add(conj_id)
180+
181+ for vlan_tag, vlan_conj_id_map in self.conj_ids.items():
182+ update = False
183++ conj_id_to_remove = []
184+ for sg_conj_id_map in vlan_conj_id_map.values():
185+ for remote_sg_id, unused in unused_dict.items():
186+ if (remote_sg_id in sg_conj_id_map and
187+ sg_conj_id_map[remote_sg_id] & unused):
188++ if remote_sg_id == sg_id:
189++ conj_id_to_remove.append(
190++ sg_conj_id_map[remote_sg_id] & unused)
191+ sg_conj_id_map[remote_sg_id] -= unused
192+ if not sg_conj_id_map[remote_sg_id]:
193+ del sg_conj_id_map[remote_sg_id]
194+ update = True
195++
196+ if update:
197+- self.update_flows_for_vlan(vlan_tag)
198++ self.update_flows_for_vlan(vlan_tag,
199++ conj_id_to_remove=conj_id_to_remove)
200+
201+
202+ class OVSFirewallDriver(firewall.FirewallDriver):
203+@@ -499,7 +533,8 @@
204+
205+ def _delete_flows(self, **kwargs):
206+ create_reg_numbers(kwargs)
207+- if self._deferred:
208++ deferred = kwargs.pop('deferred', self._deferred)
209++ if deferred:
210+ self.int_br.delete_flows(**kwargs)
211+ else:
212+ self.int_br.br.delete_flows(**kwargs)
213+@@ -1416,20 +1451,31 @@
214+ in_port=port.ofport)
215+ self._delete_flows(reg_port=port.ofport)
216+
217+- def delete_flows_for_ip_addresses(
218+- self, ip_addresses, direction, ethertype, vlan_tag):
219++ def delete_flows_for_flow_state(
220++ self, flow_state, addr_to_conj, direction, ethertype, vlan_tag):
221++ # Remove rules for deleted IPs and action=conjunction(conj_id, 1/2)
222++ removed_ips = set(flow_state.keys()) - set(addr_to_conj.keys())
223++ for removed_ip in removed_ips:
224++ conj_ids = flow_state[removed_ip]
225++ self.delete_flow_for_ip(removed_ip, direction, ethertype, vlan_tag,
226++ conj_ids)
227++
228+ if not cfg.CONF.AGENT.explicitly_egress_direct:
229+ return
230+
231+- for ip_addr in ip_addresses:
232++ for ip_addr in removed_ips:
233+ # Generate deletion template with bogus conj_id.
234+- flows = rules.create_flows_for_ip_address(
235+- ip_addr, direction, ethertype, vlan_tag, [0])
236+- for f in flows:
237+- # The following del statements are partly for
238+- # complying the OpenFlow spec. It forbids the use of
239+- # these field in non-strict delete flow messages, and
240+- # the actions field is bogus anyway.
241+- del f['actions']
242+- del f['priority']
243+- self._delete_flows(**f)
244++ self.delete_flow_for_ip(ip_addr, direction, ethertype, vlan_tag,
245++ [0])
246++
247++ def delete_flow_for_ip(self, ip_address, direction, ethertype,
248++ vlan_tag, conj_ids):
249++ for flow in rules.create_flows_for_ip_address(
250++ ip_address, direction, ethertype, vlan_tag, conj_ids):
251++ # The following del statements are partly for
252++ # complying the OpenFlow spec. It forbids the use of
253++ # these field in non-strict delete flow messages, and
254++ # the actions field is bogus anyway.
255++ del flow['actions']
256++ del flow['priority']
257++ self._delete_flows(deferred=False, **flow)
258+diff --git a/neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py b/neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py
259+index 6aeb7a4..c1aed92 100644
260+--- a/neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py
261++++ b/neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py
262+@@ -282,23 +282,50 @@
263+ constants.IPv6)
264+
265+ def test_delete_sg(self):
266+- test_data = [('sg1', 'sg1'), ('sg1', 'sg2')]
267++ test_data = [
268++ # conj_id: 8
269++ ('sg1', 'sg1', constants.INGRESS_DIRECTION, constants.IPv6, 0),
270++ # conj_id: 10
271++ ('sg1', 'sg1', constants.INGRESS_DIRECTION, constants.IPv6, 1),
272++ # conj_id: 12
273++ ('sg1', 'sg1', constants.INGRESS_DIRECTION, constants.IPv6, 2),
274++ # conj_id: 16
275++ ('sg2', 'sg1', constants.EGRESS_DIRECTION, constants.IPv6, 0),
276++ # conj_id: 24
277++ ('sg1', 'sg3', constants.INGRESS_DIRECTION, constants.IPv6, 0),
278++ # conj_id: 36 (and 32 without priority offset, stored in id_map)
279++ ('sg3', 'sg4', constants.INGRESS_DIRECTION, constants.IPv4, 2),
280++ # conj_id: 40
281++ ('sg5', 'sg4', constants.EGRESS_DIRECTION, constants.IPv4, 0),
282++ ]
283+
284+ ids = []
285+- for sg_id, remote_sg_id in test_data:
286+- ids.append(self.conj_id_map.get_conj_id(
287+- sg_id, remote_sg_id,
288+- firewall.INGRESS_DIRECTION, constants.IPv6))
289++ conj_id_segment = set([]) # see ConjIPFlowManager.get_conj_id
290++ # This is similar to ConjIPFlowManager.add method
291++ for sg_id, rsg_id, direction, ip_version, prio_offset in test_data:
292++ conj_id_tuple = (sg_id, rsg_id, direction, ip_version)
293++ conj_id = self.conj_id_map.get_conj_id(*conj_id_tuple)
294++ conj_id_segment.add(conj_id)
295++ conj_id_plus_prio = conj_id + prio_offset * 2
296++ self.conj_id_map.id_map_group[conj_id_tuple].add(conj_id_plus_prio)
297++ ids.append(conj_id_plus_prio)
298+
299+ result = self.conj_id_map.delete_sg('sg1')
300+- self.assertIn(('sg1', ids[0]), result)
301+- self.assertIn(('sg2', ids[1]), result)
302+- self.assertFalse(self.conj_id_map.id_map)
303++ self.assertEqual(
304++ {('sg3', 24), ('sg1', 12), ('sg1', 16), ('sg1', 8), ('sg1', 10)},
305++ result)
306++ result = self.conj_id_map.delete_sg('sg3')
307++ self.assertEqual({('sg4', 32), ('sg4', 36)}, result)
308++ result = self.conj_id_map.delete_sg('sg4')
309++ self.assertEqual({('sg4', 40)}, result)
310++ self.assertEqual({}, self.conj_id_map.id_map)
311++ self.assertEqual({}, self.conj_id_map.id_map_group)
312+
313+- reallocated = self.conj_id_map.get_conj_id(
314+- 'sg-foo', 'sg-foo', firewall.INGRESS_DIRECTION,
315+- constants.IPv6)
316+- self.assertIn(reallocated, ids)
317++ reallocated = set([])
318++ for sg_id, rsg_id, direction, ip_version, _ in test_data:
319++ conj_id_tuple = (sg_id, rsg_id, direction, ip_version)
320++ reallocated.add(self.conj_id_map.get_conj_id(*conj_id_tuple))
321++ self.assertEqual(reallocated, conj_id_segment)
322+
323+
324+ class TestConjIPFlowManager(base.BaseTestCase):
325+@@ -363,7 +390,7 @@
326+ dl_type=2048, nw_src='10.22.3.4/32', priority=73,
327+ reg_net=self.vlan_tag, table=82)])
328+
329+- def test_sg_removed(self):
330++ def _sg_removed(self, sg_name):
331+ with mock.patch.object(self.manager.conj_id_map,
332+ 'get_conj_id') as get_id_mock, \
333+ mock.patch.object(self.manager.conj_id_map,
334+@@ -376,11 +403,26 @@
335+ firewall.INGRESS_DIRECTION, constants.IPv4)] = {
336+ '10.22.3.4': [self.conj_id]}
337+
338+- self.manager.sg_removed('sg')
339++ self.manager.sg_removed(sg_name)
340++
341++ def test_sg_removed(self):
342++ self._sg_removed('sg')
343+ self.driver._add_flow.assert_not_called()
344+- self.driver.delete_flows_for_ip_addresses.assert_called_once_with(
345+- {'10.22.3.4'}, firewall.INGRESS_DIRECTION, constants.IPv4,
346+- self.vlan_tag)
347++ self.driver.delete_flows_for_flow_state.assert_called_once_with(
348++ {'10.22.3.4': [self.conj_id]}, {},
349++ constants.INGRESS_DIRECTION, constants.IPv4, self.vlan_tag)
350++ self.driver.delete_flow_for_ip.assert_not_called()
351++
352++ def test_remote_sg_removed(self):
353++ self._sg_removed('remote_id')
354++ self.driver._add_flow.assert_not_called()
355++ self.driver.delete_flows_for_flow_state.assert_called_once_with(
356++ {'10.22.3.4': [self.conj_id]}, {},
357++ constants.INGRESS_DIRECTION, constants.IPv4, self.vlan_tag)
358++ # "conj_id_to_remove" is populated with the remote_sg conj_id assigned,
359++ # "_update_flows_for_vlan_subr" will call "delete_flow_for_ip".
360++ self.driver.delete_flow_for_ip.assert_called_once_with(
361++ '10.22.3.4', 'ingress', 'IPv4', 100, {self.conj_id})
362+
363+
364+ class FakeOVSPort(object):
365+@@ -401,7 +443,6 @@
366+ self.fake_ovs_port = FakeOVSPort('port', 1, '00:00:00:00:00:00')
367+ self.mock_bridge.br.get_vif_port_by_id.return_value = \
368+ self.fake_ovs_port
369+- cfg.CONF.set_override('explicitly_egress_direct', True, 'AGENT')
370+
371+ def _prepare_security_group(self):
372+ security_group_rules = [
373+@@ -925,6 +966,43 @@
374+ """Check that exception is not propagated outside."""
375+ self.firewall.remove_trusted_ports(['port_id'])
376+
377++ def _test_delete_flows_for_flow_state(self, addr_to_conj,
378++ explicitly_egress_direct=True):
379++ direction = 'one_direction'
380++ ethertype = 'ethertype'
381++ vlan_tag = 'taaag'
382++ with mock.patch.object(self.firewall, 'delete_flow_for_ip') as \
383++ mock_delete_flow_for_ip:
384++ flow_state = {'addr1': {8, 16, 24}, 'addr2': {32, 40}}
385++ cfg.CONF.set_override('explicitly_egress_direct',
386++ explicitly_egress_direct, 'AGENT')
387++ self.firewall.delete_flows_for_flow_state(
388++ flow_state, addr_to_conj, direction, ethertype, vlan_tag)
389++ calls = []
390++ for removed_ip in set(flow_state.keys()) - set(addr_to_conj.keys()):
391++ calls.append(mock.call(removed_ip, direction, ethertype, vlan_tag,
392++ flow_state[removed_ip]))
393++ if explicitly_egress_direct:
394++ calls.append(mock.call(removed_ip, direction, ethertype,
395++ vlan_tag, [0]))
396++ mock_delete_flow_for_ip.assert_has_calls(calls)
397++
398++ def test_delete_flows_for_flow_state_no_removed_ips_exp_egress(self):
399++ addr_to_conj = {'addr1': {8, 16, 24}, 'addr2': {32, 40}}
400++ self._test_delete_flows_for_flow_state(addr_to_conj)
401++
402++ def test_delete_flows_for_flow_state_no_removed_ips_no_exp_egress(self):
403++ addr_to_conj = {'addr1': {8, 16, 24}, 'addr2': {32, 40}}
404++ self._test_delete_flows_for_flow_state(addr_to_conj, False)
405++
406++ def test_delete_flows_for_flow_state_removed_ips_exp_egress(self):
407++ addr_to_conj = {'addr2': {32, 40}}
408++ self._test_delete_flows_for_flow_state(addr_to_conj)
409++
410++ def test_delete_flows_for_flow_state_removed_ips_no_exp_egress(self):
411++ addr_to_conj = {'addr1': {8, 16, 24}}
412++ self._test_delete_flows_for_flow_state(addr_to_conj, False)
413++
414+
415+ class TestCookieContext(base.BaseTestCase):
416+ def setUp(self):
417diff --git a/debian/patches/series b/debian/patches/series
418index 96882c3..e65079e 100644
419--- a/debian/patches/series
420+++ b/debian/patches/series
421@@ -1,3 +1,4 @@
422 skip-iptest.patch
423 flake8-legacy.patch
424 Ensure-fip-ip-rules-deleted-when-fip-removed.patch
425+lp1881157-remote-sg-is-left-behind.patch

Subscribers

People subscribed via source and target branches