From 9e699c3743857009f7e33bf0a2d780a6cfbeafb2 Mon Sep 17 00:00:00 2001 From: Slawek Kaplonski Date: Wed, 17 Jun 2020 23:28:16 +0200 Subject: [PATCH] [ovn] Use normalized remote prefix IPs in OVN driver 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 (cherry picked from commit 17f2ba3afbbbb929155c2ac3fa396784badd981b) --- neutron/common/ovn/acl.py | 21 ++++- .../drivers/ovn/mech_driver/mech_driver.py | 31 +++++++ neutron/tests/unit/common/ovn/test_acl.py | 13 +++ .../ovn/mech_driver/test_mech_driver.py | 81 ++++++++++++++++++- 4 files changed, 140 insertions(+), 6 deletions(-) diff --git a/neutron/common/ovn/acl.py b/neutron/common/ovn/acl.py index a0a3e47fac1..c568c23fe32 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): @@ -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( 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 54268ef6025..55b74d85989 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 @@ -314,10 +315,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 43bfe1325f2..da4decceaa1 100644 --- a/neutron/tests/unit/common/ovn/test_acl.py +++ b/neutron/tests/unit/common/ovn/test_acl.py @@ -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', 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 b10558420a2..668eec9d1a7 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 @@ -139,12 +139,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) @@ -168,6 +192,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,