diff --git a/neutron/common/ovn/acl.py b/neutron/common/ovn/acl.py index 0553301a596..8d2477f796a 100644 --- a/neutron/common/ovn/acl.py +++ b/neutron/common/ovn/acl.py @@ -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): @@ -297,7 +310,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, @@ -322,7 +335,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( diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py index de35f907fcd..275f30b8c38 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py @@ -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, diff --git a/neutron/tests/unit/common/ovn/test_acl.py b/neutron/tests/unit/common/ovn/test_acl.py index 8e0eb105f75..0cbaa2cd16b 100644 --- a/neutron/tests/unit/common/ovn/test_acl.py +++ b/neutron/tests/unit/common/ovn/test_acl.py @@ -303,6 +303,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', diff --git a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py index d7d9bd1d020..da98110f943 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py +++ b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py @@ -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,