From 17f2ba3afbbbb929155c2ac3fa396784badd981b 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 --- 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 5378c477bc1..3364ad79342 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 d4dcd2d5df3..884f225e92d 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 b2b03a58c42..cf06bbec4db 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,