openstack/neutron
Code Issues Proposed changes

Merge "[OVS][FW] Remote SG IDs left behind when a SG is removed"

Browse Source
This commit is contained in:
Zuul 2020-07-31 06:22:12 +00:00 committed by Gerrit Code Review
commit fe79ef22b8
2 changed files with 169 additions and 45 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

100
neutron/agent/linux/openvswitch_firewall/firewall.py
View File

@ -268,6 +268,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
@ -298,13 +302,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
@ -346,12 +359,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:
@ -360,7 +383,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.
"""
@ -373,7 +396,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,
@ -394,32 +417,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):
@ -500,7 +534,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)
@ -1433,20 +1468,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)

114
neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py
View File

@ -283,23 +283,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):
@ -364,7 +391,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,
@ -377,11 +404,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):
@ -403,7 +445,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 = [
@ -960,6 +1001,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 flow_state.keys() - 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):