diff -Nru neutron-14.3.1/debian/changelog neutron-14.3.1/debian/changelog --- neutron-14.3.1/debian/changelog 2020-09-21 15:13:41.000000000 +0100 +++ neutron-14.3.1/debian/changelog 2020-10-24 16:13:14.000000000 +0100 @@ -1,3 +1,10 @@ +neutron (2:14.3.1-0ubuntu1~cloud0+test.0) bionic; urgency=medium + + * Backport fix for sg removal cleanup (LP: #1881157) + * d/p/OVS-FW-Remote-SG-IDs-left-behind-when-a-SG-is-remove.patch + + -- Edward Hope-Morley Sat, 24 Oct 2020 16:13:14 +0100 + neutron (2:14.3.1-0ubuntu1~cloud0) bionic-stein; urgency=medium * New stable point release for OpenStack Stein (LP: #1896478). diff -Nru neutron-14.3.1/debian/patches/OVS-FW-Remote-SG-IDs-left-behind-when-a-SG-is-remove.patch neutron-14.3.1/debian/patches/OVS-FW-Remote-SG-IDs-left-behind-when-a-SG-is-remove.patch --- neutron-14.3.1/debian/patches/OVS-FW-Remote-SG-IDs-left-behind-when-a-SG-is-remove.patch 1970-01-01 01:00:00.000000000 +0100 +++ neutron-14.3.1/debian/patches/OVS-FW-Remote-SG-IDs-left-behind-when-a-SG-is-remove.patch 2020-10-24 16:13:10.000000000 +0100 @@ -0,0 +1,402 @@ +From aaf87aeb55949a32412ffdc46089e67c1ca24d54 Mon Sep 17 00:00:00 2001 +From: Rodolfo Alonso Hernandez +Date: Tue, 2 Jun 2020 17:09:07 +0000 +Subject: [PATCH] [OVS][FW] Remote SG IDs left behind when a SG is removed + +When any port in the OVS agent is using a security groups (SG) and +this SG is removed, is marked to be deleted. This deletion process +is done in [1]. + +The SG deletion process consists on removing any reference of this SG +from the firewall and the SG port map. The firewall removes this SG in +[2]. + +The information of a SG is stored in: +* ConjIPFlowManager.conj_id_map = ConjIdMap(). This class stores the + conjunction IDS (conj_ids) in a dictionary using the following keys: + + ConjIdMap.id_map[(sg_id, remote_sg_id, direction, ethertype, + conj_ids)] = conj_id_XXX + +* ConjIPFlowManager.conj_ids is a nested dictionary, built in the + following way: + + self.conj_ids[vlan_tag][(direction, ethertype)][remote_sg_id] = \ + set([conj_id_1, conj_id_2, ...]) + +This patch stores all conjuntion IDs generated and assigned to the +tuple (sg_id, remote_sg_id, direction, ethertype). When a SG is +removed, the deletion method will look for this SG in the new storage +variable created, ConjIdMap.id_map_group, and will mark all the +conjuntion IDs related to be removed. That will cleanup those rules +left in the OVS matching: + action=conjunction(conj_id, 1/2) + +[1]https://github.com/openstack/neutron/blob/118930f03d31f157f8c7a9e6c57122ecea8982b9/neutron/agent/linux/openvswitch_firewall/firewall.py#L731 +[2]https://github.com/openstack/neutron/blob/118930f03d31f157f8c7a9e6c57122ecea8982b9/neutron/agent/linux/openvswitch_firewall/firewall.py#L399 + +Conflicts: + neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py + +Change-Id: I63e446a30cf10e7bcd34a6f0d6ba1711301efcbe +Related-Bug: #1881157 +(cherry picked from commit 0eebd002ccda66dc6d9f9e5a254815109225e299) +(cherry picked from commit ed22f7a2ff19a874bc8521f84cb4fd1c7483a23f) +(cherry picked from commit 6615f248e25a361d988db1795a3486a58f4768cb) +--- + .../linux/openvswitch_firewall/firewall.py | 100 +++++++++++---- + .../openvswitch_firewall/test_firewall.py | 116 +++++++++++++++--- + 2 files changed, 170 insertions(+), 46 deletions(-) + +diff --git a/neutron/agent/linux/openvswitch_firewall/firewall.py b/neutron/agent/linux/openvswitch_firewall/firewall.py +index 2e424120f9..d589257db3 100644 +--- a/neutron/agent/linux/openvswitch_firewall/firewall.py ++++ b/neutron/agent/linux/openvswitch_firewall/firewall.py +@@ -269,6 +269,10 @@ class ConjIdMap(object): + + def __init__(self): + self.id_map = collections.defaultdict(self._conj_id_factory) ++ # Stores the set of conjuntion IDs used for each unique tuple ++ # (sg_id, remote_sg_id, direction, ethertype). Each tuple can have up ++ # to 8 conjuntion IDs (see ConjIPFlowManager.add()). ++ self.id_map_group = collections.defaultdict(set) + self.id_free = collections.deque() + self.max_id = 0 + +@@ -299,13 +303,22 @@ class ConjIdMap(object): + return a list of (remote_sg_id, conj_id), which are no longer + in use. + """ +- result = [] ++ result = set([]) + for k in list(self.id_map.keys()): + if sg_id in k[0:2]: + conj_id = self.id_map.pop(k) +- result.append((k[1], conj_id)) ++ result.add((k[1], conj_id)) + self.id_free.append(conj_id) + ++ # If the remote_sg_id is removed, the tuple (sg_id, remote_sg_id, ++ # direction, ethertype) no longer exists; the conjunction IDs assigned ++ # to this tuple should be removed too. ++ for k in list(self.id_map_group.keys()): ++ if sg_id in k[0:2]: ++ conj_id_groups = self.id_map_group.pop(k, []) ++ for conj_id in conj_id_groups: ++ result.add((k[1], conj_id)) ++ + return result + + +@@ -347,12 +360,22 @@ class ConjIPFlowManager(object): + return addr_to_conj + + def _update_flows_for_vlan_subr(self, direction, ethertype, vlan_tag, +- flow_state, addr_to_conj): ++ flow_state, addr_to_conj, ++ conj_id_to_remove): + """Do the actual flow updates for given direction and ethertype.""" +- current_ips = set(flow_state.keys()) +- self.driver.delete_flows_for_ip_addresses( +- current_ips - set(addr_to_conj.keys()), +- direction, ethertype, vlan_tag) ++ conj_id_to_remove = conj_id_to_remove or [] ++ # Delete any current flow related to any deleted IP address, before ++ # creating the flows for the current IPs. ++ self.driver.delete_flows_for_flow_state( ++ flow_state, addr_to_conj, direction, ethertype, vlan_tag) ++ for conj_id_set in conj_id_to_remove: ++ # Remove any remaining flow with remote SG ID conj_id_to_remove ++ for current_ip, conj_ids in flow_state.items(): ++ conj_ids_to_remove = conj_id_set & set(conj_ids) ++ self.driver.delete_flow_for_ip( ++ current_ip, direction, ethertype, vlan_tag, ++ conj_ids_to_remove) ++ + for addr, conj_ids in addr_to_conj.items(): + conj_ids.sort() + if flow_state.get(addr) == conj_ids: +@@ -361,7 +384,7 @@ class ConjIPFlowManager(object): + addr, direction, ethertype, vlan_tag, conj_ids): + self.driver._add_flow(**flow) + +- def update_flows_for_vlan(self, vlan_tag): ++ def update_flows_for_vlan(self, vlan_tag, conj_id_to_remove=None): + """Install action=conjunction(conj_id, 1/2) flows, + which depend on IP addresses of remote_group_id. + """ +@@ -374,7 +397,7 @@ class ConjIPFlowManager(object): + self._update_flows_for_vlan_subr( + direction, ethertype, vlan_tag, + self.flow_state[vlan_tag][(direction, ethertype)], +- addr_to_conj) ++ addr_to_conj, conj_id_to_remove) + self.flow_state[vlan_tag][(direction, ethertype)] = addr_to_conj + + def add(self, vlan_tag, sg_id, remote_sg_id, direction, ethertype, +@@ -395,32 +418,43 @@ class ConjIPFlowManager(object): + collections.defaultdict(set)) + self.conj_ids[vlan_tag][(direction, ethertype)][remote_sg_id].add( + conj_id) ++ ++ conj_id_tuple = (sg_id, remote_sg_id, direction, ethertype) ++ self.conj_id_map.id_map_group[conj_id_tuple].add(conj_id) + return conj_id + + def sg_removed(self, sg_id): + """Handle SG removal events. + +- Free all conj_ids associated with the sg_id and clean up ++ Free all conj_ids associated with the sg_id removed and clean up + obsolete entries from the self.conj_ids map. Unlike the add + method, it also updates flows. ++ If a SG is removed, both sg_id and remote_sg_id should be removed from ++ the "vlan_conj_id_map". + """ +- id_list = self.conj_id_map.delete_sg(sg_id) ++ id_set = self.conj_id_map.delete_sg(sg_id) + unused_dict = collections.defaultdict(set) +- for remote_sg_id, conj_id in id_list: ++ for remote_sg_id, conj_id in id_set: + unused_dict[remote_sg_id].add(conj_id) + + for vlan_tag, vlan_conj_id_map in self.conj_ids.items(): + update = False ++ conj_id_to_remove = [] + for sg_conj_id_map in vlan_conj_id_map.values(): + for remote_sg_id, unused in unused_dict.items(): + if (remote_sg_id in sg_conj_id_map and + sg_conj_id_map[remote_sg_id] & unused): ++ if remote_sg_id == sg_id: ++ conj_id_to_remove.append( ++ sg_conj_id_map[remote_sg_id] & unused) + sg_conj_id_map[remote_sg_id] -= unused + if not sg_conj_id_map[remote_sg_id]: + del sg_conj_id_map[remote_sg_id] + update = True ++ + if update: +- self.update_flows_for_vlan(vlan_tag) ++ self.update_flows_for_vlan(vlan_tag, ++ conj_id_to_remove=conj_id_to_remove) + + + class OVSFirewallDriver(firewall.FirewallDriver): +@@ -501,7 +535,8 @@ class OVSFirewallDriver(firewall.FirewallDriver): + + def _delete_flows(self, **kwargs): + create_reg_numbers(kwargs) +- if self._deferred: ++ deferred = kwargs.pop('deferred', self._deferred) ++ if deferred: + self.int_br.delete_flows(**kwargs) + else: + self.int_br.br.delete_flows(**kwargs) +@@ -1418,20 +1453,31 @@ class OVSFirewallDriver(firewall.FirewallDriver): + in_port=port.ofport) + self._delete_flows(reg_port=port.ofport) + +- def delete_flows_for_ip_addresses( +- self, ip_addresses, direction, ethertype, vlan_tag): ++ def delete_flows_for_flow_state( ++ self, flow_state, addr_to_conj, direction, ethertype, vlan_tag): ++ # Remove rules for deleted IPs and action=conjunction(conj_id, 1/2) ++ removed_ips = set(flow_state.keys()) - set(addr_to_conj.keys()) ++ for removed_ip in removed_ips: ++ conj_ids = flow_state[removed_ip] ++ self.delete_flow_for_ip(removed_ip, direction, ethertype, vlan_tag, ++ conj_ids) ++ + if not cfg.CONF.AGENT.explicitly_egress_direct: + return + +- for ip_addr in ip_addresses: ++ for ip_addr in removed_ips: + # Generate deletion template with bogus conj_id. +- flows = rules.create_flows_for_ip_address( +- ip_addr, direction, ethertype, vlan_tag, [0]) +- for f in flows: +- # The following del statements are partly for +- # complying the OpenFlow spec. It forbids the use of +- # these field in non-strict delete flow messages, and +- # the actions field is bogus anyway. +- del f['actions'] +- del f['priority'] +- self._delete_flows(**f) ++ self.delete_flow_for_ip(ip_addr, direction, ethertype, vlan_tag, ++ [0]) ++ ++ def delete_flow_for_ip(self, ip_address, direction, ethertype, ++ vlan_tag, conj_ids): ++ for flow in rules.create_flows_for_ip_address( ++ ip_address, direction, ethertype, vlan_tag, conj_ids): ++ # The following del statements are partly for ++ # complying the OpenFlow spec. It forbids the use of ++ # these field in non-strict delete flow messages, and ++ # the actions field is bogus anyway. ++ del flow['actions'] ++ del flow['priority'] ++ self._delete_flows(deferred=False, **flow) +diff --git a/neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py b/neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py +index 4c2dcfa81e..3436b68f9a 100644 +--- a/neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py ++++ b/neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py +@@ -281,23 +281,50 @@ class TestConjIdMap(base.BaseTestCase): + constants.IPv6) + + def test_delete_sg(self): +- test_data = [('sg1', 'sg1'), ('sg1', 'sg2')] ++ test_data = [ ++ # conj_id: 8 ++ ('sg1', 'sg1', constants.INGRESS_DIRECTION, constants.IPv6, 0), ++ # conj_id: 10 ++ ('sg1', 'sg1', constants.INGRESS_DIRECTION, constants.IPv6, 1), ++ # conj_id: 12 ++ ('sg1', 'sg1', constants.INGRESS_DIRECTION, constants.IPv6, 2), ++ # conj_id: 16 ++ ('sg2', 'sg1', constants.EGRESS_DIRECTION, constants.IPv6, 0), ++ # conj_id: 24 ++ ('sg1', 'sg3', constants.INGRESS_DIRECTION, constants.IPv6, 0), ++ # conj_id: 36 (and 32 without priority offset, stored in id_map) ++ ('sg3', 'sg4', constants.INGRESS_DIRECTION, constants.IPv4, 2), ++ # conj_id: 40 ++ ('sg5', 'sg4', constants.EGRESS_DIRECTION, constants.IPv4, 0), ++ ] + + ids = [] +- for sg_id, remote_sg_id in test_data: +- ids.append(self.conj_id_map.get_conj_id( +- sg_id, remote_sg_id, +- constants.INGRESS_DIRECTION, constants.IPv6)) ++ conj_id_segment = set([]) # see ConjIPFlowManager.get_conj_id ++ # This is similar to ConjIPFlowManager.add method ++ for sg_id, rsg_id, direction, ip_version, prio_offset in test_data: ++ conj_id_tuple = (sg_id, rsg_id, direction, ip_version) ++ conj_id = self.conj_id_map.get_conj_id(*conj_id_tuple) ++ conj_id_segment.add(conj_id) ++ conj_id_plus_prio = conj_id + prio_offset * 2 ++ self.conj_id_map.id_map_group[conj_id_tuple].add(conj_id_plus_prio) ++ ids.append(conj_id_plus_prio) + + result = self.conj_id_map.delete_sg('sg1') +- self.assertIn(('sg1', ids[0]), result) +- self.assertIn(('sg2', ids[1]), result) +- self.assertFalse(self.conj_id_map.id_map) +- +- reallocated = self.conj_id_map.get_conj_id( +- 'sg-foo', 'sg-foo', constants.INGRESS_DIRECTION, +- constants.IPv6) +- self.assertIn(reallocated, ids) ++ self.assertEqual( ++ {('sg3', 24), ('sg1', 12), ('sg1', 16), ('sg1', 8), ('sg1', 10)}, ++ result) ++ result = self.conj_id_map.delete_sg('sg3') ++ self.assertEqual({('sg4', 32), ('sg4', 36)}, result) ++ result = self.conj_id_map.delete_sg('sg4') ++ self.assertEqual({('sg4', 40)}, result) ++ self.assertEqual({}, self.conj_id_map.id_map) ++ self.assertEqual({}, self.conj_id_map.id_map_group) ++ ++ reallocated = set([]) ++ for sg_id, rsg_id, direction, ip_version, _ in test_data: ++ conj_id_tuple = (sg_id, rsg_id, direction, ip_version) ++ reallocated.add(self.conj_id_map.get_conj_id(*conj_id_tuple)) ++ self.assertEqual(reallocated, conj_id_segment) + + + class TestConjIPFlowManager(base.BaseTestCase): +@@ -362,7 +389,7 @@ class TestConjIPFlowManager(base.BaseTestCase): + dl_type=2048, nw_src='10.22.3.4/32', priority=73, + reg_net=self.vlan_tag, table=82)]) + +- def test_sg_removed(self): ++ def _sg_removed(self, sg_name): + with mock.patch.object(self.manager.conj_id_map, + 'get_conj_id') as get_id_mock, \ + mock.patch.object(self.manager.conj_id_map, +@@ -375,11 +402,26 @@ class TestConjIPFlowManager(base.BaseTestCase): + constants.INGRESS_DIRECTION, constants.IPv4)] = { + '10.22.3.4': [self.conj_id]} + +- self.manager.sg_removed('sg') ++ self.manager.sg_removed(sg_name) ++ ++ def test_sg_removed(self): ++ self._sg_removed('sg') ++ self.driver._add_flow.assert_not_called() ++ self.driver.delete_flows_for_flow_state.assert_called_once_with( ++ {'10.22.3.4': [self.conj_id]}, {}, ++ constants.INGRESS_DIRECTION, constants.IPv4, self.vlan_tag) ++ self.driver.delete_flow_for_ip.assert_not_called() ++ ++ def test_remote_sg_removed(self): ++ self._sg_removed('remote_id') + self.driver._add_flow.assert_not_called() +- self.driver.delete_flows_for_ip_addresses.assert_called_once_with( +- {'10.22.3.4'}, constants.INGRESS_DIRECTION, constants.IPv4, +- self.vlan_tag) ++ self.driver.delete_flows_for_flow_state.assert_called_once_with( ++ {'10.22.3.4': [self.conj_id]}, {}, ++ constants.INGRESS_DIRECTION, constants.IPv4, self.vlan_tag) ++ # "conj_id_to_remove" is populated with the remote_sg conj_id assigned, ++ # "_update_flows_for_vlan_subr" will call "delete_flow_for_ip". ++ self.driver.delete_flow_for_ip.assert_called_once_with( ++ '10.22.3.4', 'ingress', 'IPv4', 100, {self.conj_id}) + + + class FakeOVSPort(object): +@@ -400,7 +442,6 @@ class TestOVSFirewallDriver(base.BaseTestCase): + self.fake_ovs_port = FakeOVSPort('port', 1, '00:00:00:00:00:00') + self.mock_bridge.br.get_vif_port_by_id.return_value = \ + self.fake_ovs_port +- cfg.CONF.set_override('explicitly_egress_direct', True, 'AGENT') + + def _prepare_security_group(self): + security_group_rules = [ +@@ -924,6 +965,43 @@ class TestOVSFirewallDriver(base.BaseTestCase): + """Check that exception is not propagated outside.""" + self.firewall.remove_trusted_ports(['port_id']) + ++ def _test_delete_flows_for_flow_state(self, addr_to_conj, ++ explicitly_egress_direct=True): ++ direction = 'one_direction' ++ ethertype = 'ethertype' ++ vlan_tag = 'taaag' ++ with mock.patch.object(self.firewall, 'delete_flow_for_ip') as \ ++ mock_delete_flow_for_ip: ++ flow_state = {'addr1': {8, 16, 24}, 'addr2': {32, 40}} ++ cfg.CONF.set_override('explicitly_egress_direct', ++ explicitly_egress_direct, 'AGENT') ++ self.firewall.delete_flows_for_flow_state( ++ flow_state, addr_to_conj, direction, ethertype, vlan_tag) ++ calls = [] ++ for removed_ip in set(flow_state.keys()) - set(addr_to_conj.keys()): ++ calls.append(mock.call(removed_ip, direction, ethertype, vlan_tag, ++ flow_state[removed_ip])) ++ if explicitly_egress_direct: ++ calls.append(mock.call(removed_ip, direction, ethertype, ++ vlan_tag, [0])) ++ mock_delete_flow_for_ip.assert_has_calls(calls) ++ ++ def test_delete_flows_for_flow_state_no_removed_ips_exp_egress(self): ++ addr_to_conj = {'addr1': {8, 16, 24}, 'addr2': {32, 40}} ++ self._test_delete_flows_for_flow_state(addr_to_conj) ++ ++ def test_delete_flows_for_flow_state_no_removed_ips_no_exp_egress(self): ++ addr_to_conj = {'addr1': {8, 16, 24}, 'addr2': {32, 40}} ++ self._test_delete_flows_for_flow_state(addr_to_conj, False) ++ ++ def test_delete_flows_for_flow_state_removed_ips_exp_egress(self): ++ addr_to_conj = {'addr2': {32, 40}} ++ self._test_delete_flows_for_flow_state(addr_to_conj) ++ ++ def test_delete_flows_for_flow_state_removed_ips_no_exp_egress(self): ++ addr_to_conj = {'addr1': {8, 16, 24}} ++ self._test_delete_flows_for_flow_state(addr_to_conj, False) ++ + + class TestCookieContext(base.BaseTestCase): + def setUp(self): +-- +2.17.1 + diff -Nru neutron-14.3.1/debian/patches/series neutron-14.3.1/debian/patches/series --- neutron-14.3.1/debian/patches/series 2020-09-21 15:13:41.000000000 +0100 +++ neutron-14.3.1/debian/patches/series 2020-10-24 16:13:10.000000000 +0100 @@ -1,3 +1,4 @@ skip-iptest.patch flake8-legacy.patch Ensure-fip-ip-rules-deleted-when-fip-removed.patch +OVS-FW-Remote-SG-IDs-left-behind-when-a-SG-is-remove.patch