Browse Source

Check for sg_rules correctness

This patch is updating networking-ovn to check for correctness when
creating or deleting security group rules.

This patch also allows to insert duplicate entries in ACL table in
case that two indentical rules belong to different SGs. Each acl will
reference to its own SG rule in the external_ids column so that we
can ensure consistency across Neutron and OVN objects. The main
drawback is that duplicated acls will make ovn-northd insert duplicate
lflows in SB database which, in turn, makes ovn-controller drop the
flows when it's processing the logical flows and log INFO messages. To
overcome this, I have sent a patch [0] to core OVN so that
ovn-controller logs those traces as DBG instead and reduce noise.
Please see the references in the commit message at [0] and the
discussion around this.

Partial-Bug: #1605089

[0] 5905b28f1a

Change-Id: Ie2659ecb84193d58d35ced6b8fb0b89fc03cf6e7
Signed-off-by: Daniel Alvarez <dalvarez@redhat.com>
changes/33/531033/12
Daniel Alvarez 4 years ago
parent
commit
c2e6038fa1
  1. 35
      networking_ovn/common/acl.py
  2. 2
      networking_ovn/common/constants.py
  3. 24
      networking_ovn/common/maintenance.py
  4. 2
      networking_ovn/common/ovn_client.py
  5. 4
      networking_ovn/common/utils.py
  6. 10
      networking_ovn/ml2/mech_driver.py
  7. 10
      networking_ovn/ovsdb/commands.py
  8. 6
      networking_ovn/ovsdb/impl_idl_ovn.py
  9. 9
      networking_ovn/ovsdb/ovn_api.py
  10. 21
      networking_ovn/tests/unit/common/test_acl.py
  11. 109
      networking_ovn/tests/unit/ml2/test_mech_driver.py
  12. 6
      networking_ovn/tests/unit/ovsdb/test_commands.py

35
networking_ovn/common/acl.py

@ -168,7 +168,8 @@ def add_sg_rule_acl_for_port(port, r, match):
"severity": [],
"direction": dir_map[r['direction']],
"match": match,
"external_ids": {'neutron:lport': port['id']}}
"external_ids": {'neutron:lport': port['id'],
ovn_const.OVN_SG_RULE_EXT_ID_KEY: r['id']}}
return acl
@ -279,22 +280,6 @@ def _acl_columns_name_severity_supported(nb_idl):
return ('name' in columns) and ('severity' in columns)
def _filter_security_groups_by_rule(plugin, admin_context,
security_group_rule, security_groups):
if not security_groups:
return set()
# Need accurate match including value None
filters = {key: security_group_rule.get(key)
for key in ('direction', 'protocol', 'ethertype',
'port_range_min', 'port_range_max',
'remote_ip_prefix', 'remote_group_id')}
filters['security_group_id'] = set(security_groups)
rules = plugin.get_security_group_rules(admin_context,
filters=filters)
return set([r['security_group_id'] for r in rules])
def update_acls_for_security_group(plugin,
admin_context,
ovn,
@ -321,14 +306,6 @@ def update_acls_for_security_group(plugin,
if not port_list:
return
# Port can have multiple security groups, we will check whether other
# related security groups contain duplicate rules.
# If true, this rule will not be added to or removed from OVN.
related_sgs = [sg for p in port_list for sg in p['security_groups']
if sg != security_group_id]
duplicate_sgs = _filter_security_groups_by_rule(
plugin, admin_context, security_group_rule, related_sgs)
acl_new_values_dict = {}
update_port_list = []
@ -341,11 +318,6 @@ def update_acls_for_security_group(plugin,
if utils.is_lsp_trusted(port):
continue
# Check whether other security groups contain duplicate rules.
# If true, this rule will not be added or removed.
if set(port['security_groups']) & duplicate_sgs:
continue
update_port_list.append(port)
acl = _add_sg_rule_acl_for_port(port, security_group_rule)
# Remove lport and lswitch since we don't need them
@ -405,8 +377,7 @@ def add_acls(plugin, admin_context, port, sg_cache, subnet_cache, ovn):
sg_id)
for r in sg['security_group_rules']:
acl = _add_sg_rule_acl_for_port(port, r)
if acl not in acl_list:
acl_list.append(acl)
acl_list.append(acl)
# Remove ACL log name and severity if not supported,
if not _acl_columns_name_severity_supported(ovn):

2
networking_ovn/common/constants.py

@ -17,6 +17,7 @@ import six
# TODO(lucasagomes): Remove OVN_SG_NAME_EXT_ID_KEY in the Rocky release
OVN_SG_NAME_EXT_ID_KEY = 'neutron:security_group_name'
OVN_SG_EXT_ID_KEY = 'neutron:security_group_id'
OVN_SG_RULE_EXT_ID_KEY = 'neutron:security_group_rule_id'
OVN_ML2_MECH_DRIVER_NAME = 'ovn'
OVN_NETWORK_NAME_EXT_ID_KEY = 'neutron:network_name'
OVN_PORT_NAME_EXT_ID_KEY = 'neutron:port_name'
@ -93,3 +94,4 @@ INITIAL_REV_NUM = -1
# Resource types
TYPE_NETWORKS = 'networks'
TYPE_PORTS = 'ports'
TYPE_SECURITY_GROUP_RULES = 'security_group_rules'

24
networking_ovn/common/maintenance.py

@ -160,6 +160,26 @@ class DBInconsistenciesPeriodics(object):
else:
self._ovn_client.delete_port(row.resource_uuid)
def _fix_delete_sg_rule(self, row):
acl = self._nb_idl.get_acl_by_id(row.resource_uuid)
if not acl:
db_rev.delete_revision(row.resource_uuid)
else:
self._ovn_client.delete_security_group_rule(
row.resource_uuid)
def _fix_create_sg_rule(self, row):
# Get the latest version of the sg rule in Neutron DB
admin_context = n_context.get_admin_context()
sgr_db_obj = self._ovn_client._plugin.get_security_group_rule(
admin_context, row.resource_uuid)
if row.revision_number == ovn_const.INITIAL_REV_NUM:
self._ovn_client.create_security_group_rule(sgr_db_obj)
else:
LOG.error("SG rule %s found with a revision number while this "
"resource doesn't support updates.", row.resource_uuid)
@periodics.periodic(spacing=DB_CONSISTENCY_CHECK_INTERVAL,
run_immediately=True)
def check_for_inconsistencies(self):
@ -181,6 +201,8 @@ class DBInconsistenciesPeriodics(object):
self._fix_create_update_network(row)
elif row.resource_type == ovn_const.TYPE_PORTS:
self._fix_create_update_port(row)
elif row.resource_type == ovn_const.TYPE_SECURITY_GROUP_RULES:
self._fix_create_sg_rule(row)
except Exception:
LOG.exception('Failed to fix resource %(res_uuid)s '
'(type: %(res_type)s)',
@ -194,6 +216,8 @@ class DBInconsistenciesPeriodics(object):
self._fix_delete_network(row)
elif row.resource_type == ovn_const.TYPE_PORTS:
self._fix_delete_port(row)
elif row.resource_type == ovn_const.TYPE_SECURITY_GROUP_RULES:
self._fix_delete_sg_rule(row)
except Exception:
LOG.exception('Failed to fix deleted resource %(res_uuid)s '
'(type: %(res_type)s)',

2
networking_ovn/common/ovn_client.py

@ -1298,9 +1298,11 @@ class OVNClient(object):
def create_security_group_rule(self, rule):
self._process_security_group_rule(rule)
db_rev.bump_revision(rule, ovn_const.TYPE_SECURITY_GROUP_RULES)
def delete_security_group_rule(self, rule):
self._process_security_group_rule(rule, is_add_acl=False)
db_rev.delete_revision(rule['id'])
def _find_metadata_port(self, context, network_id):
if not config.is_ovn_metadata_enabled():

4
networking_ovn/common/utils.py

@ -202,7 +202,9 @@ def get_ovn_ipv6_address_mode(address_mode):
def get_revision_number(resource, resource_type):
"""Get the resource's revision number based on its type."""
if resource_type in (constants.TYPE_NETWORKS, constants.TYPE_PORTS):
if resource_type in (constants.TYPE_NETWORKS,
constants.TYPE_PORTS,
constants.TYPE_SECURITY_GROUP_RULES):
return resource['revision_number']
else:
raise ovn_exc.UnknownResourceType(resource_type=resource_type)

10
networking_ovn/ml2/mech_driver.py

@ -153,6 +153,9 @@ class OVNMechanismDriver(api.MechanismDriver):
registry.subscribe(self._delete_security_group,
resources.SECURITY_GROUP,
events.AFTER_DELETE)
registry.subscribe(self._create_sg_rule_precommit,
resources.SECURITY_GROUP_RULE,
events.PRECOMMIT_CREATE)
registry.subscribe(self._process_sg_rule_notification,
resources.SECURITY_GROUP_RULE,
events.AFTER_CREATE)
@ -203,6 +206,13 @@ class OVNMechanismDriver(api.MechanismDriver):
security_group_id, **kwargs):
self._ovn_client.delete_security_group(security_group_id)
def _create_sg_rule_precommit(self, resource, event, trigger, **kwargs):
sg_rule = kwargs.get('security_group_rule')
context = kwargs.get('context')
db_rev.create_initial_revision(sg_rule['id'],
ovn_const.TYPE_SECURITY_GROUP_RULES,
context.session)
def _process_sg_rule_notification(
self, resource, event, trigger, **kwargs):
if event == events.AFTER_CREATE:

10
networking_ovn/ovsdb/commands.py

@ -431,7 +431,6 @@ class AddACLCommand(command.BaseCommand):
row = txn.insert(self.api._tables['ACL'])
for col, val in self.columns.items():
setattr(row, col, val)
row.external_ids = {'neutron:lport': self.lport}
_addvalue_to_list(lswitch, 'acls', row.uuid)
@ -561,15 +560,18 @@ class UpdateACLsCommand(command.BaseCommand):
else:
acl_add_values_dict = {}
acl_del_objs_dict = {}
del_acl_matches = []
del_acl_extids = []
for acl_dict in self.acl_new_values_dict.values():
del_acl_matches.append(acl_dict['match'])
del_acl_extids.append({acl_dict['match']:
acl_dict['external_ids']})
for switch_name, lswitch in lswitch_ovsdb_dict.items():
if switch_name not in acl_del_objs_dict:
acl_del_objs_dict[switch_name] = []
acls = getattr(lswitch, 'acls', [])
for acl in acls:
if getattr(acl, 'match') in del_acl_matches:
match = getattr(acl, 'match')
acl_extids = {match: getattr(acl, 'external_ids')}
if acl_extids in del_acl_extids:
acl_del_objs_dict[switch_name].append(acl)
return lswitch_ovsdb_dict, acl_del_objs_dict, acl_add_values_dict

6
networking_ovn/ovsdb/impl_idl_ovn.py

@ -226,6 +226,12 @@ class OvsdbNbOvnIdl(nb_impl_idl.OvnNbApiIdlImpl, Backend):
'dnat_and_snats': dnat_and_snats})
return result
def get_acl_by_id(self, acl_id):
try:
return self.lookup('ACL', uuid.UUID(acl_id))
except idlutils.RowNotFound:
return
def get_acls_for_lswitches(self, lswitch_names):
"""Get the existing set of acls that belong to the logical switches

9
networking_ovn/ovsdb/ovn_api.py

@ -227,6 +227,15 @@ class API(api.API):
:type is_add_acl: bool
"""
@abc.abstractmethod
def get_acl_by_id(self, acl_id):
"""Get an ACL by its ID.
:param acl_id: ID of the ACL to lookup
:type acl_id: string
:returns The ACL row or None:
"""
@abc.abstractmethod
def add_static_route(self, lrouter, **columns):
"""Add static route to logical router.

21
networking_ovn/tests/unit/common/test_acl.py

@ -124,11 +124,14 @@ class TestACLs(base.TestCase):
'log': False, 'name': [], 'severity': [],
'direction': direction,
'match': match,
'external_ids': {'neutron:lport': 'port-id'}},
'external_ids': {
'neutron:lport': 'port-id',
'neutron:security_group_rule_id': 'sgr_id'}},
acl)
def test_add_sg_rule_acl_for_port_remote_ip_prefix(self):
sg_rule = {'direction': 'ingress',
sg_rule = {'id': 'sgr_id',
'direction': 'ingress',
'ethertype': 'IPv4',
'remote_group_id': None,
'remote_ip_prefix': '1.1.1.0/24',
@ -144,7 +147,8 @@ class TestACLs(base.TestCase):
match)
def test_add_sg_rule_acl_for_port_remote_group(self):
sg_rule = {'direction': 'ingress',
sg_rule = {'id': 'sgr_id',
'direction': 'ingress',
'ethertype': 'IPv4',
'remote_group_id': 'sg1',
'remote_ip_prefix': None,
@ -296,10 +300,10 @@ class TestACLs(base.TestCase):
ports = [port1, port2]
aclport1_new = {'priority': 1002, 'direction': 'to-lport',
'match': 'outport == %s && ip4 && icmp4' %
(port1['id'])}
(port1['id']), 'external_ids': {}}
aclport2_new = {'priority': 1002, 'direction': 'to-lport',
'match': 'outport == %s && ip4 && icmp4' %
(port2['id'])}
(port2['id']), 'external_ids': {}}
acls_new_dict = {'%s' % (port1['id']): aclport1_new,
'%s' % (port2['id']): aclport2_new}
@ -319,11 +323,12 @@ class TestACLs(base.TestCase):
# test for deleting existing acls
acl1 = mock.Mock(
match='outport == port-id1 && ip4 && icmp4')
match='outport == port-id1 && ip4 && icmp4', external_ids={})
acl2 = mock.Mock(
match='outport == port-id2 && ip4 && icmp4')
match='outport == port-id2 && ip4 && icmp4', external_ids={})
acl3 = mock.Mock(
match='outport == port-id1 && ip4 && (ip4.src == fake_ip)')
match='outport == port-id1 && ip4 && (ip4.src == fake_ip)',
external_ids={})
lswitch_obj = mock.Mock(
name='neutron-lswitch-1', acls=[acl1, acl2, acl3])
with mock.patch('ovsdbapp.backend.ovs_idl.idlutils.row_by_value',

109
networking_ovn/tests/unit/ml2/test_mech_driver.py

@ -43,6 +43,7 @@ from networking_ovn.common import config as ovn_config
from networking_ovn.common import constants as ovn_const
from networking_ovn.common import ovn_client
from networking_ovn.common import utils as ovn_utils
from networking_ovn.db import revision as db_rev
from networking_ovn.ml2 import mech_driver
from networking_ovn.tests.unit import fakes
@ -116,7 +117,8 @@ class TestOVNMechanismDriver(test_plugin.Ml2PluginV2TestCase):
self.nb_ovn.delete_address_set.assert_has_calls(
delete_address_set_calls, any_order=True)
def test__process_sg_rule_notifications_sgr_create(self):
@mock.patch.object(db_rev, 'bump_revision')
def test__process_sg_rule_notifications_sgr_create(self, mock_bump):
with mock.patch(
'networking_ovn.common.acl.update_acls_for_security_group'
) as ovn_acl_up:
@ -127,9 +129,12 @@ class TestOVNMechanismDriver(test_plugin.Ml2PluginV2TestCase):
ovn_acl_up.assert_called_once_with(
mock.ANY, mock.ANY, mock.ANY,
'sg_id', rule, is_add_acl=True)
mock_bump.assert_called_once_with(
rule, ovn_const.TYPE_SECURITY_GROUP_RULES)
def test_process_sg_rule_notifications_sgr_delete(self):
rule = {'security_group_id': 'sg_id'}
@mock.patch.object(db_rev, 'delete_revision')
def test_process_sg_rule_notifications_sgr_delete(self, mock_delrev):
rule = {'id': 'sgr_id', 'security_group_id': 'sg_id'}
with mock.patch(
'networking_ovn.common.acl.update_acls_for_security_group'
) as ovn_acl_up:
@ -144,6 +149,7 @@ class TestOVNMechanismDriver(test_plugin.Ml2PluginV2TestCase):
ovn_acl_up.assert_called_once_with(
mock.ANY, mock.ANY, mock.ANY,
'sg_id', rule, is_add_acl=False)
mock_delrev.assert_called_once_with(rule['id'])
def test_add_acls_no_sec_group(self):
acls = ovn_acl.add_acls(self.mech_driver._plugin,
@ -1914,73 +1920,6 @@ class TestOVNMechanismDriverSecurityGroup(
def _delete_sg_rule(self, rule_id):
self._delete('security-group-rules', rule_id)
def test__filter_security_groups_by_rule(self):
sg1 = self._create_empty_sg('sg1')
sg2 = self._create_empty_sg('sg2')
sg3 = self._create_empty_sg('sg3')
sg4 = self._create_empty_sg('sg4')
r1 = self._create_sg_rule(sg1['id'], 'ingress', const.PROTO_NAME_TCP)
r2 = self._create_sg_rule(sg1['id'], 'ingress', const.PROTO_NAME_TCP,
port_range_min=22, port_range_max=22,
remote_ip_prefix='10.0.0.0/24')
r3 = self._create_sg_rule(sg1['id'], 'ingress', const.PROTO_NAME_UDP,
remote_group_id=sg4['id'],
ethertype=const.IPv6)
# Rules in sg2 are not duplicate with any rules in sg1
self._create_sg_rule(sg2['id'], 'egress', const.PROTO_NAME_TCP)
self._create_sg_rule(sg2['id'], 'ingress', const.PROTO_NAME_UDP)
self._create_sg_rule(sg2['id'], 'ingress', const.PROTO_NAME_TCP,
port_range_min=22, port_range_max=23,
remote_ip_prefix='10.0.0.0/24')
self._create_sg_rule(sg2['id'], 'ingress', const.PROTO_NAME_TCP,
port_range_min=22, port_range_max=22,
remote_ip_prefix='10.0.0.0/25')
self._create_sg_rule(sg2['id'], 'egress', const.PROTO_NAME_UDP)
self._create_sg_rule(sg2['id'], 'ingress', const.PROTO_NAME_UDP,
remote_group_id=sg3['id'],
ethertype=const.IPv6)
self._create_sg_rule(sg2['id'], 'ingress', const.PROTO_NAME_UDP,
remote_group_id=sg4['id'],
ethertype=const.IPv4)
# Rules in sg3 are duplicate with r1 and r2 in sg1
self._create_sg_rule(sg3['id'], 'ingress', const.PROTO_NAME_TCP)
self._create_sg_rule(sg3['id'], 'ingress', const.PROTO_NAME_TCP,
port_range_min=22, port_range_max=22,
remote_ip_prefix='10.0.0.0/24')
# Rules in sg4 are duplicate with r1 and r3 in sg1
self._create_sg_rule(sg4['id'], 'ingress', const.PROTO_NAME_TCP)
self._create_sg_rule(sg4['id'], 'ingress', const.PROTO_NAME_UDP,
remote_group_id=sg4['id'],
ethertype=const.IPv6)
# Check r1
all_sgs = [sg1['id'], sg2['id'], sg3['id'], sg4['id']]
sgs = ovn_acl._filter_security_groups_by_rule(self.plugin, self.ctx,
r1, all_sgs)
self.assertItemsEqual(list(sgs), [sg1['id'], sg3['id'], sg4['id']])
sgs = ovn_acl._filter_security_groups_by_rule(self.plugin, self.ctx,
r1, [])
self.assertItemsEqual(list(sgs), [])
# Check r2
sgs = ovn_acl._filter_security_groups_by_rule(self.plugin, self.ctx,
r2, all_sgs)
self.assertItemsEqual(list(sgs), [sg1['id'], sg3['id']])
sgs = ovn_acl._filter_security_groups_by_rule(
self.plugin, self.ctx, r2, [sg2['id'], sg4['id']])
self.assertItemsEqual(list(sgs), [])
# Check r3
sgs = ovn_acl._filter_security_groups_by_rule(self.plugin, self.ctx,
r3, all_sgs)
self.assertItemsEqual(list(sgs), [sg1['id'], sg4['id']])
def test_create_port_with_sg_default_rules(self):
with self.network() as n, self.subnet(n):
sg = self._create_sg('sg')
@ -2030,9 +1969,9 @@ class TestOVNMechanismDriverSecurityGroup(
self._make_port(self.fmt, n['network']['id'],
security_groups=[sg1['id'], sg2['id']])
# One DHCP rule, one TCP rule and two default dropping rules.
# One DHCP rule, two TCP rule and two default dropping rules.
self.assertEqual(
4, self.mech_driver._nb_ovn.add_acl.call_count)
5, self.mech_driver._nb_ovn.add_acl.call_count)
def test_update_port_with_sgs(self):
with self.network() as n, self.subnet(n):
@ -2108,15 +2047,16 @@ class TestOVNMechanismDriverSecurityGroup(
self.assertEqual(
4, self.mech_driver._nb_ovn.add_acl.call_count)
# Add a new duplicate rule to sg2
# Add a new duplicate rule to sg2. It's expected to be added.
sg2_r = self._create_sg_rule(sg2['id'], 'ingress',
const.PROTO_NAME_UDP,
port_range_min=22, port_range_max=23)
self.mech_driver._nb_ovn.update_acls.assert_not_called()
self.mech_driver._nb_ovn.update_acls.assert_called_once()
# Delete the duplicate rule
# Delete the duplicate rule. It's expected to be deleted.
self._delete_sg_rule(sg2_r['id'])
self.mech_driver._nb_ovn.update_acls.assert_not_called()
self.assertEqual(
2, self.mech_driver._nb_ovn.update_acls.call_count)
def test_update_sg_duplicate_rule_multi_ports(self):
with self.network() as n, self.subnet(n):
@ -2139,28 +2079,29 @@ class TestOVNMechanismDriverSecurityGroup(
self.assertEqual(
14, self.mech_driver._nb_ovn.add_acl.call_count)
# Add a rule to sg1 duplicate with sg2
# Add a rule to sg1 duplicate with sg2. It's expected to be added.
sg1_r = self._create_sg_rule(sg1['id'], 'egress',
const.PROTO_NAME_TCP,
port_range_min=60, port_range_max=70)
self.mech_driver._nb_ovn.update_acls.assert_not_called()
self.mech_driver._nb_ovn.update_acls.assert_called_once()
# Add a rule to sg2 duplicate with sg1 but not duplicate with sg3
# Add a rule to sg2 duplicate with sg1 but not duplicate with sg3.
# It's expected to be added as well.
sg2_r = self._create_sg_rule(sg2['id'], 'ingress',
const.PROTO_NAME_UDP,
remote_group_id=sg3['id'])
self.assertEqual(
1, self.mech_driver._nb_ovn.update_acls.call_count)
2, self.mech_driver._nb_ovn.update_acls.call_count)
# Delete the duplicate rule in sg1
# Delete the duplicate rule in sg1. It's expected to be deleted.
self._delete_sg_rule(sg1_r['id'])
self.assertEqual(
1, self.mech_driver._nb_ovn.update_acls.call_count)
3, self.mech_driver._nb_ovn.update_acls.call_count)
# Delete the duplicate rule in sg2
# Delete the duplicate rule in sg2. It's expected to be deleted.
self._delete_sg_rule(sg2_r['id'])
self.assertEqual(
2, self.mech_driver._nb_ovn.update_acls.call_count)
4, self.mech_driver._nb_ovn.update_acls.call_count)
class TestOVNMechanismDriverMetadataPort(test_plugin.Ml2PluginV2TestCase):

6
networking_ovn/tests/unit/ovsdb/test_commands.py

@ -734,8 +734,6 @@ class TestAddACLCommand(TestBaseCommand):
self.ovn_api._tables['ACL'])
fake_lswitch.addvalue.assert_called_once_with(
'acls', fake_acl.uuid)
self.assertEqual({'neutron:lport': 'fake-lsp'},
fake_acl.external_ids)
self.assertEqual('*', fake_acl.match)
@ -865,7 +863,9 @@ class TestUpdateACLsCommand(TestBaseCommand):
fakes.FakeSecurityGroupRule.create_one_security_group_rule().info()
fake_port = fakes.FakePort.create_one_port().info()
fake_acl = fakes.FakeOvsdbRow.create_one_ovsdb_row(
attrs={'match': '*'})
attrs={'match': '*', 'external_ids':
{'neutron:lport': fake_port['id'],
'neutron:security_group_rule_id': fake_sg_rule['id']}})
fake_lswitch = fakes.FakeOvsdbRow.create_one_ovsdb_row(
attrs={'name': ovn_utils.ovn_name(fake_port['network_id']),
'acls': [fake_acl]})

Loading…
Cancel
Save