diff --git a/neutron/agent/linux/openvswitch_firewall/firewall.py b/neutron/agent/linux/openvswitch_firewall/firewall.py index 2e424120f95..d589257db36 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 4c2dcfa81e5..3436b68f9a7 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) + 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 = self.conj_id_map.get_conj_id( - 'sg-foo', 'sg-foo', constants.INGRESS_DIRECTION, - constants.IPv6) - self.assertIn(reallocated, ids) + 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_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) + 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_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):