Browse Source

[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]118930f03d/neutron/agent/linux/openvswitch_firewall/firewall.py (L731)
[2]118930f03d/neutron/agent/linux/openvswitch_firewall/firewall.py (L399)

Conflicts:
    neutron/agent/linux/openvswitch_firewall/firewall.py
    neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py

Change-Id: I63e446a30cf10e7bcd34a6f0d6ba1711301efcbe
Related-Bug: #1881157
(cherry picked from commit 0eebd002cc)
(cherry picked from commit ed22f7a2ff)
(cherry picked from commit 6615f248e2)
changes/81/747381/1
Rodolfo Alonso Hernandez 3 months ago
committed by Slawek Kaplonski
parent
commit
d0b696f076
2 changed files with 170 additions and 46 deletions
  1. +73
    -27
      neutron/agent/linux/openvswitch_firewall/firewall.py
  2. +97
    -19
      neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py

+ 73
- 27
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.
"""
@@ -372,7 +395,7 @@ class ConjIPFlowManager(object):
ethertype, sg_conj_id_map)
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,
@@ -393,32 +416,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):
@@ -499,7 +533,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)
@@ -1416,20 +1451,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)

+ 97
- 19
neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py View File

@@ -282,23 +282,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,
firewall.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', firewall.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):
@@ -363,7 +390,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,
@@ -376,11 +403,26 @@ class TestConjIPFlowManager(base.BaseTestCase):
firewall.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'}, firewall.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):
@@ -401,7 +443,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 = [
@@ -925,6 +966,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):


Loading…
Cancel
Save