openstack/neutron
Code Issues Proposed changes

[ovn] Use normalized remote prefix IPs in OVN driver

Browse Source 
OVN firewall driver can't silently normalize CIRDs given in
the security group rule's "remote_ip_prefix".
Because of that if user created rule with not normalized CIDR, it
wasn't applied by the OVN driver.
Now OVN driver will normalize such rules before applying them.

The OVN driver will now also check if SG rules with same normalized
and same direction, port range, protocol and ethertype already exists in
the SG. If so, it will not add or remove rule in the OVN.
Rule will be added or removed only if there is no other same rules in
the SG.

Change-Id: I0d9295545384844e81b0ffe3aa7483324f9a9ae5
Related-Bug: #1869129
This commit is contained in:
Slawek Kaplonski 2020-06-17 23:28:16 +02:00
parent a3c7aad790
commit 17f2ba3afb
4 changed files with 140 additions and 6 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

21
neutron/common/ovn/acl.py
View File

@ -12,14 +12,19 @@
# under the License.
#
import netaddr
from neutron_lib import constants as const
from neutron_lib import exceptions as n_exceptions
from oslo_config import cfg
from oslo_log import log as logging
from neutron._i18n import _
from neutron.common.ovn import constants as ovn_const
from neutron.common.ovn import utils
LOG = logging.getLogger(__name__)
# Convert the protocol number from integer to strings because that's
# how Neutron will pass it to us
PROTOCOL_NAME_TO_NUM_MAP = {k: str(v) for k, v in
@ -84,9 +89,17 @@ def acl_ethertype(r):
def acl_remote_ip_prefix(r, ip_version):
if not r['remote_ip_prefix']:
return ''
cidr = netaddr.IPNetwork(r['remote_ip_prefix'])
normalized_ip_prefix = "%s/%s" % (cidr.network, cidr.prefixlen)
if r['remote_ip_prefix'] != normalized_ip_prefix:
LOG.info("remote_ip_prefix %(remote_ip_prefix)s configured in "
"rule %(rule_id)s is not normalized. Normalized CIDR "
"%(normalized_ip_prefix)s will be used instead.",
{'remote_ip_prefix': r['remote_ip_prefix'],
'rule_id': r['id'],
'normalized_ip_prefix': normalized_ip_prefix})
src_or_dst = 'src' if r['direction'] == const.INGRESS_DIRECTION else 'dst'
return ' && %s.%s == %s' % (ip_version, src_or_dst,
r['remote_ip_prefix'])
return ' && %s.%s == %s' % (ip_version, src_or_dst, normalized_ip_prefix)
def _get_protocol_number(protocol):
@ -314,7 +327,7 @@ def add_acls_for_sg_port_group(ovn, security_group, txn):
for r in security_group['security_group_rules']:
acl = _add_sg_rule_acl_for_port_group(
utils.ovn_port_group_name(security_group['id']), r)
txn.add(ovn.pg_acl_add(**acl))
txn.add(ovn.pg_acl_add(**acl, may_exist=True))
def update_acls_for_security_group(plugin,
@ -339,7 +352,7 @@ def update_acls_for_security_group(plugin,
if not keep_name_severity:
acl.pop('name')
acl.pop('severity')
ovn.pg_acl_add(**acl).execute(check_error=True)
ovn.pg_acl_add(**acl, may_exist=True).execute(check_error=True)
else:
ovn.pg_acl_del(acl['port_group'], acl['direction'],
acl['priority'], acl['match']).execute(

31
neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py
View File

@ -21,6 +21,7 @@ import signal
import threading
import types
import netaddr
from neutron_lib.api.definitions import portbindings
from neutron_lib.api.definitions import segment as segment_def
from neutron_lib.callbacks import events
@ -360,10 +361,40 @@ class OVNMechanismDriver(api.MechanismDriver):
elif event == events.BEFORE_DELETE:
sg_rule = self._plugin.get_security_group_rule(
kwargs['context'], kwargs.get('security_group_rule_id'))
if sg_rule.get('remote_ip_prefix') is not None:
if self._sg_has_rules_with_same_normalized_cidr(sg_rule):
return
self._ovn_client.delete_security_group_rule(
kwargs['context'],
sg_rule)
def _sg_has_rules_with_same_normalized_cidr(self, sg_rule):
compare_keys = [
'ethertype', 'direction', 'protocol',
'port_range_min', 'port_range_max']
sg_rules = self._plugin.get_security_group_rules(
n_context.get_admin_context(),
{'security_group_id': [sg_rule['security_group_id']]})
cidr_sg_rule = netaddr.IPNetwork(sg_rule['remote_ip_prefix'])
normalized_sg_rule_prefix = "%s/%s" % (cidr_sg_rule.network,
cidr_sg_rule.prefixlen)
def _rules_equal(rule1, rule2):
return not any(
rule1.get(key) != rule2.get(key) for key in compare_keys)
for rule in sg_rules:
if not rule.get('remote_ip_prefix') or rule['id'] == sg_rule['id']:
continue
cidr_rule = netaddr.IPNetwork(rule['remote_ip_prefix'])
normalized_rule_prefix = "%s/%s" % (cidr_rule.network,
cidr_rule.prefixlen)
if normalized_sg_rule_prefix != normalized_rule_prefix:
continue
if _rules_equal(sg_rule, rule):
return True
return False
def _is_network_type_supported(self, network_type):
return (network_type in [const.TYPE_LOCAL,
const.TYPE_FLAT,

13
neutron/tests/unit/common/ovn/test_acl.py
View File

@ -539,6 +539,19 @@ class TestACLs(base.BaseTestCase):
expected_match = ' && %s.dst == %s' % (ip_version, remote_ip_prefix)
self.assertEqual(expected_match, match)
def test_acl_remote_ip_prefix_not_normalized(self):
sg_rule = fakes.FakeSecurityGroupRule.create_one_security_group_rule({
'direction': 'ingress',
'remote_ip_prefix': '10.10.10.175/26'
}).info()
normalized_ip_prefix = '10.10.10.128/26'
ip_version = 'ip4'
match = ovn_acl.acl_remote_ip_prefix(sg_rule, ip_version)
expected_match = ' && %s.src == %s' % (ip_version,
normalized_ip_prefix)
self.assertEqual(expected_match, match)
def test_acl_remote_group_id(self):
sg_rule = fakes.FakeSecurityGroupRule.create_one_security_group_rule({
'direction': 'ingress',

81
neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py
View File

@ -183,12 +183,36 @@ class TestOVNMechanismDriver(test_plugin.Ml2PluginV2TestCase):
@mock.patch.object(ovn_revision_numbers_db, 'bump_revision')
def test__process_sg_rule_notifications_sgr_create(self, mock_bump):
with mock.patch.object(ovn_acl, 'update_acls_for_security_group') \
as ovn_acl_up:
with mock.patch.object(
self.mech_driver,
'_sg_has_rules_with_same_normalized_cidr') as has_same_rules, \
mock.patch.object(
ovn_acl, 'update_acls_for_security_group') as ovn_acl_up:
rule = {'security_group_id': 'sg_id'}
self.mech_driver._process_sg_rule_notification(
resources.SECURITY_GROUP_RULE, events.AFTER_CREATE, {},
security_group_rule=rule, context=self.context)
has_same_rules.assert_not_called()
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(
mock.ANY, rule, ovn_const.TYPE_SECURITY_GROUP_RULES)
@mock.patch.object(ovn_revision_numbers_db, 'bump_revision')
def test__process_sg_rule_notifications_sgr_create_with_remote_ip_prefix(
self, mock_bump):
with mock.patch.object(
self.mech_driver,
'_sg_has_rules_with_same_normalized_cidr') as has_same_rules, \
mock.patch.object(
ovn_acl, 'update_acls_for_security_group') as ovn_acl_up:
rule = {'security_group_id': 'sg_id',
'remote_ip_prefix': '1.0.0.0/24'}
self.mech_driver._process_sg_rule_notification(
resources.SECURITY_GROUP_RULE, events.AFTER_CREATE, {},
security_group_rule=rule, context=self.context)
has_same_rules.assert_not_called()
ovn_acl_up.assert_called_once_with(
mock.ANY, mock.ANY, mock.ANY,
'sg_id', rule, is_add_acl=True)
@ -212,6 +236,59 @@ class TestOVNMechanismDriver(test_plugin.Ml2PluginV2TestCase):
mock_delrev.assert_called_once_with(
mock.ANY, rule['id'], ovn_const.TYPE_SECURITY_GROUP_RULES)
def test__sg_has_rules_with_same_normalized_cidr(self):
scenarios = [
({'id': 'rule-id', 'security_group_id': 'sec-group-uuid',
'remote_ip_prefix': '10.10.10.175/26',
'protocol': 'tcp'}, False),
({'id': 'rule-id', 'security_group_id': 'sec-group-uuid',
'remote_ip_prefix': '10.10.10.175/26',
'protocol': 'udp'}, False),
({'id': 'rule-id', 'security_group_id': 'sec-group-uuid',
'remote_ip_prefix': '10.10.10.175/26',
'protocol': 'tcp'}, False),
({'id': 'rule-id', 'security_group_id': 'sec-group-uuid',
'remote_ip_prefix': '10.10.10.175/26',
'protocol': 'tcp',
'port_range_min': '2000', 'port_range_max': '2100'}, False),
({'id': 'rule-id', 'security_group_id': 'sec-group-uuid',
'remote_ip_prefix': '192.168.0.0/24',
'protocol': 'tcp',
'port_range_min': '2000', 'port_range_max': '3000',
'direction': 'ingress'}, False),
({'id': 'rule-id', 'security_group_id': 'sec-group-uuid',
'remote_ip_prefix': '10.10.10.175/26',
'protocol': 'tcp',
'port_range_min': '2000', 'port_range_max': '3000',
'direction': 'egress'}, False),
({'id': 'rule-id', 'security_group_id': 'sec-group-uuid',
'remote_ip_prefix': '10.10.10.175/26',
'protocol': 'tcp',
'port_range_min': '2000', 'port_range_max': '3000',
'direction': 'ingress'}, True)]
rules = [
{
'id': 'rule-1-id',
'protocol': 'udp',
}, {
'id': 'rule-2-id',
'remote_ip_prefix': '10.10.10.128/26',
'protocol': 'tcp',
'port_range_min': '2000',
'port_range_max': '3000',
'direction': 'ingress'
}]
with mock.patch.object(securitygroups_db.SecurityGroupDbMixin,
'get_security_group_rules',
return_value=rules):
for rule, expected_result in scenarios:
self.assertEqual(
expected_result,
self.mech_driver._sg_has_rules_with_same_normalized_cidr(
rule))
def test_port_invalid_binding_profile(self):
invalid_binding_profiles = [
{'tag': 0,