From 237ec30ca94322716a1af5e59c0960f0eef24194 Mon Sep 17 00:00:00 2001 From: IWAMOTO Toshihiro Date: Wed, 2 Aug 2017 17:12:56 +0900 Subject: [PATCH] ovsfw: Merge multiple conjunction flows The ovsfw code generated multiple flows with the same or overlapping match fields and different actions=conjunction(nnn,2/2) flows. Merge such flows and generate only one flow with actions=conjunction(mmm,2/2),conjunction(nnn,2/2) so that filtering are correctly performed. Change-Id: I0cd325b02f35e103606595b8b124010fff8dc397 Partial-bug: #1708092 --- .../internals/openvswitch_firewall.rst | 6 + .../linux/openvswitch_firewall/firewall.py | 36 ++++-- .../agent/linux/openvswitch_firewall/rules.py | 106 ++++++++++++++++++ neutron/tests/fullstack/test_securitygroup.py | 51 ++++++--- .../linux/openvswitch_firewall/test_rules.py | 93 +++++++++++++++ 5 files changed, 272 insertions(+), 20 deletions(-) diff --git a/doc/source/contributor/internals/openvswitch_firewall.rst b/doc/source/contributor/internals/openvswitch_firewall.rst index 6d8fc91bade..f128f618a92 100644 --- a/doc/source/contributor/internals/openvswitch_firewall.rst +++ b/doc/source/contributor/internals/openvswitch_firewall.rst @@ -114,6 +114,12 @@ the dimension 2 of 2 are generated by the method ``substitute_conjunction_actions``, which represents the portion of the rule other than its remote group ID. +Those dimension 2 of 2 flows are per port and contain no remote group +information. When there are multiple security group rules for a port, +those flows can overlap. To avoid such a situation, flows are sorted +and fed to ``merge_port_ranges`` or ``merge_common_rules`` methods to +rearrange them. + Rules example with explanation: ------------------------------- diff --git a/neutron/agent/linux/openvswitch_firewall/firewall.py b/neutron/agent/linux/openvswitch_firewall/firewall.py index 7c743191168..e0af325a54b 100644 --- a/neutron/agent/linux/openvswitch_firewall/firewall.py +++ b/neutron/agent/linux/openvswitch_firewall/firewall.py @@ -984,24 +984,46 @@ class OVSFirewallDriver(firewall.FirewallDriver): The remaining part is done by ConjIPFlowManager. """ + port_rules = collections.defaultdict(list) for sec_group_id, rule in ( self._create_remote_rules_generator_for_port(port)): direction = rule['direction'] ethertype = rule['ethertype'] + protocol = rule.get('protocol') conj_id = self.conj_ip_manager.add(port.vlan_tag, sec_group_id, rule['remote_group_id'], direction, ethertype) - flows = rules.create_flows_from_rule_and_port(rule, port) - for flow in rules.substitute_conjunction_actions( - flows, 2, [conj_id]): - self._add_flow(**flow) + rule1 = rule.copy() + del rule1['remote_group_id'] + port_rules_key = (direction, ethertype, protocol) + port_rules[port_rules_key].append((rule1, conj_id)) + + for (direction, ethertype, protocol), rule_conj_list in ( + port_rules.items()): + all_conj_ids = set() + for rule, conj_id in rule_conj_list: + all_conj_ids.add(conj_id) + + if protocol in [lib_const.PROTO_NUM_SCTP, + lib_const.PROTO_NUM_TCP, + lib_const.PROTO_NUM_UDP]: + rule_conj_list = rules.merge_port_ranges(rule_conj_list) + else: + rule_conj_list = rules.merge_common_rules(rule_conj_list) + + for rule, conj_ids in rule_conj_list: + flows = rules.create_flows_from_rule_and_port(rule, port) + for flow in rules.substitute_conjunction_actions( + flows, 2, conj_ids): + self._add_flow(**flow) # Install actions=accept flows. - for flow in rules.create_conj_flows( - port, conj_id, direction, ethertype): - self._add_flow(**flow) + for conj_id in all_conj_ids: + for flow in rules.create_conj_flows( + port, conj_id, direction, ethertype): + self._add_flow(**flow) def add_flows_from_rules(self, port): self._initialize_tracked_ingress(port) diff --git a/neutron/agent/linux/openvswitch_firewall/rules.py b/neutron/agent/linux/openvswitch_firewall/rules.py index 930984f7681..c82c98dfcb0 100644 --- a/neutron/agent/linux/openvswitch_firewall/rules.py +++ b/neutron/agent/linux/openvswitch_firewall/rules.py @@ -13,6 +13,8 @@ # License for the specific language governing permissions and limitations # under the License. +import collections + import netaddr from neutron_lib import constants as n_consts from oslo_log import log as logging @@ -46,6 +48,110 @@ def is_valid_prefix(ip_prefix): str(netaddr.IPNetwork(ip_prefix)) not in FORBIDDEN_PREFIXES) +def _assert_mergeable_rules(rule_conj_list): + """Assert a given (rule, conj_ids) list has mergeable rules. + + The given rules must be the same except for port_range_{min,max} + differences. + """ + rule_tmpl = rule_conj_list[0][0].copy() + rule_tmpl.pop('port_range_min', None) + rule_tmpl.pop('port_range_max', None) + for rule, conj_id in rule_conj_list[1:]: + rule1 = rule.copy() + rule1.pop('port_range_min', None) + rule1.pop('port_range_max', None) + if rule_tmpl != rule1: + raise RuntimeError( + "Incompatible SG rules detected: %(rule1)s and %(rule2)s. " + "They cannot be merged. This should not happen." % + {'rule1': rule_tmpl, 'rule2': rule}) + + +def merge_common_rules(rule_conj_list): + """Take a list of (rule, conj_id) and merge elements with the same rules. + Return a list of (rule, conj_id_list). + """ + if len(rule_conj_list) == 1: + rule, conj_id = rule_conj_list[0] + return [(rule, [conj_id])] + + _assert_mergeable_rules(rule_conj_list) + rule_conj_map = collections.defaultdict(list) + for rule, conj_id in rule_conj_list: + rule_conj_map[(rule.get('port_range_min'), + rule.get('port_range_max'))].append(conj_id) + + result = [] + rule_tmpl = rule_conj_list[0][0] + rule_tmpl.pop('port_range_min', None) + rule_tmpl.pop('port_range_max', None) + for (port_min, port_max), conj_ids in rule_conj_map.items(): + rule = rule_tmpl.copy() + if port_min is not None: + rule['port_range_min'] = port_min + if port_max is not None: + rule['port_range_max'] = port_max + result.append((rule, conj_ids)) + return result + + +def _merge_port_ranges_helper(port_range_item): + # Sort with 'port' but 'min' things must come first. + port, m, dummy = port_range_item + return port * 2 + (0 if m == 'min' else 1) + + +def merge_port_ranges(rule_conj_list): + """Take a list of (rule, conj_id) and transform into a list + whose rules don't overlap. Return a list of (rule, conj_id_list). + """ + if len(rule_conj_list) == 1: + rule, conj_id = rule_conj_list[0] + return [(rule, [conj_id])] + + _assert_mergeable_rules(rule_conj_list) + port_ranges = [] + for rule, conj_id in rule_conj_list: + port_ranges.append((rule.get('port_range_min', 1), 'min', conj_id)) + port_ranges.append((rule.get('port_range_max', 65535), 'max', conj_id)) + + port_ranges.sort(key=_merge_port_ranges_helper) + + # The idea here is to scan the port_ranges list in an ascending order, + # keeping active conjunction IDs and range in cur_conj and cur_range_min. + # A 'min' port_ranges item means an addition to cur_conj, while a 'max' + # item means a removal. + result = [] + rule_tmpl = rule_conj_list[0][0] + cur_conj = set() + cur_range_min = None + for port, m, conj_id in port_ranges: + if m == 'min': + if cur_conj and cur_range_min != port: + rule = rule_tmpl.copy() + rule['port_range_min'] = cur_range_min + rule['port_range_max'] = port - 1 + result.append((rule, list(cur_conj))) + cur_range_min = port + cur_conj.add(conj_id) + else: + if cur_range_min <= port: + rule = rule_tmpl.copy() + rule['port_range_min'] = cur_range_min + rule['port_range_max'] = port + result.append((rule, list(cur_conj))) + # The next port range without 'port' starts from (port + 1) + cur_range_min = port + 1 + cur_conj.remove(conj_id) + + if (len(result) == 1 and result[0][0]['port_range_min'] == 1 and + result[0][0]['port_range_max'] == 65535): + del result[0][0]['port_range_min'] + del result[0][0]['port_range_max'] + return result + + def create_flows_from_rule_and_port(rule, port): ethertype = rule['ethertype'] direction = rule['direction'] diff --git a/neutron/tests/fullstack/test_securitygroup.py b/neutron/tests/fullstack/test_securitygroup.py index c367904b9b7..c4a3ad8ddae 100644 --- a/neutron/tests/fullstack/test_securitygroup.py +++ b/neutron/tests/fullstack/test_securitygroup.py @@ -106,16 +106,17 @@ class TestSecurityGroupsSameNetwork(BaseSecurityGroupsSameNetworkTest): 4. a security group update takes effect, 5. a remote security group member addition works, and 6. an established connection stops by deleting a SG rule. - 7. test other protocol functionality by using SCTP protocol - 8. test two vms with same mac on the same host in different + 7. multiple overlapping remote rules work, + 8. test other protocol functionality by using SCTP protocol + 9. test two vms with same mac on the same host in different networks """ - index_to_sg = [0, 0, 1] + index_to_sg = [0, 0, 1, 2] if self.firewall_driver == 'iptables_hybrid': # The iptables_hybrid driver lacks isolation between agents - index_to_host = [0] * 3 + index_to_host = [0] * 4 else: - index_to_host = [0, 1, 1] + index_to_host = [0, 1, 1, 0] tenant_uuid = uuidutils.generate_uuid() @@ -124,7 +125,7 @@ class TestSecurityGroupsSameNetwork(BaseSecurityGroupsSameNetworkTest): tenant_uuid, network['id'], '20.0.0.0/24') sgs = [self.safe_client.create_security_group(tenant_uuid) - for i in range(2)] + for i in range(3)] ports = [ self.safe_client.create_port(tenant_uuid, network['id'], self.environment.hosts[host].hostname, @@ -228,22 +229,23 @@ class TestSecurityGroupsSameNetwork(BaseSecurityGroupsSameNetworkTest): ports.append( self.safe_client.create_port(tenant_uuid, network['id'], self.environment.hosts[ - index_to_host[3]].hostname, + index_to_host[-1]].hostname, security_groups=[sgs[1]['id']])) vms.append( self.useFixture( machine.FakeFullstackMachine( - self.environment.hosts[index_to_host[3]], + self.environment.hosts[index_to_host[-1]], network['id'], tenant_uuid, self.safe_client, - neutron_port=ports[3], + neutron_port=ports[-1], use_dhcp=True))) + self.assertEqual(5, len(vms)) - vms[3].block_until_boot() + vms[4].block_until_boot() - netcat = net_helpers.NetcatTester(vms[3].namespace, + netcat = net_helpers.NetcatTester(vms[4].namespace, vms[0].namespace, vms[0].ip, 3355, net_helpers.NetcatTester.TCP) @@ -255,7 +257,30 @@ class TestSecurityGroupsSameNetwork(BaseSecurityGroupsSameNetworkTest): sleep=8) netcat.stop_processes() - # 7. check SCTP is supported by security group + # 7. check if multiple overlapping remote rules work + self.safe_client.create_security_group_rule( + tenant_uuid, sgs[0]['id'], + remote_group_id=sgs[1]['id'], direction='ingress', + ethertype=constants.IPv4, + protocol=constants.PROTO_NAME_TCP, + port_range_min=3333, port_range_max=3333) + self.safe_client.create_security_group_rule( + tenant_uuid, sgs[0]['id'], + remote_group_id=sgs[2]['id'], direction='ingress', + ethertype=constants.IPv4) + + for i in range(2): + self.assert_connection( + vms[0].namespace, vms[1].namespace, vms[1].ip, 3333, + net_helpers.NetcatTester.TCP) + self.assert_connection( + vms[2].namespace, vms[1].namespace, vms[1].ip, 3333, + net_helpers.NetcatTester.TCP) + self.assert_connection( + vms[3].namespace, vms[0].namespace, vms[0].ip, 8080, + net_helpers.NetcatTester.TCP) + + # 8. check SCTP is supported by security group self.assert_no_connection( vms[1].namespace, vms[0].namespace, vms[0].ip, 3366, net_helpers.NetcatTester.SCTP) @@ -271,7 +296,7 @@ class TestSecurityGroupsSameNetwork(BaseSecurityGroupsSameNetworkTest): vms[1].namespace, vms[0].namespace, vms[0].ip, 3366, net_helpers.NetcatTester.SCTP) - # 8. test two vms with same mac on the same host in different networks + # 9. test two vms with same mac on the same host in different networks self._test_overlapping_mac_addresses() def _create_vm_on_host( diff --git a/neutron/tests/unit/agent/linux/openvswitch_firewall/test_rules.py b/neutron/tests/unit/agent/linux/openvswitch_firewall/test_rules.py index 9c145bdb5b7..e4375336083 100644 --- a/neutron/tests/unit/agent/linux/openvswitch_firewall/test_rules.py +++ b/neutron/tests/unit/agent/linux/openvswitch_firewall/test_rules.py @@ -404,3 +404,96 @@ class TestCreateConjFlows(base.BaseTestCase): del f['ct_state'] self.assertEqual(expected_template, f) expected_template['conj_id'] += 1 + + +class TestMergeRules(base.BaseTestCase): + def setUp(self): + super(TestMergeRules, self).setUp() + self.rule_tmpl = [('direction', 'ingress'), ('ethertype', 'IPv4'), + ('protocol', 6)] + + def _test_merge_port_ranges_helper(self, expected, result): + """Take a list of (port_range_min, port_range_max, conj_ids) + and an output from rules.merge_port_ranges and check if they + are identical, ignoring the other rule fields. + """ + self.assertEqual(len(expected), len(result)) + for (range_min, range_max, conj_ids), result1 in zip( + expected, result): + self.assertEqual(range_min, result1[0]['port_range_min']) + self.assertEqual(range_max, result1[0]['port_range_max']) + self.assertEqual(conj_ids, set(result1[1])) + + def test__assert_mergeable_rules(self): + self.assertRaises(RuntimeError, + rules._assert_mergeable_rules, + [({'direction': 'ingress', 'ethertype': 'IPv4', + 'protocol': 1}, 8), + ({'direction': 'ingress', 'ethertype': 'IPv6'}, + 16)]) + + def test_merge_common_rules_single(self): + rule_conj_tuple = ({'direction': 'egress', 'ethertype': 'IPv4', + 'protocol': 1}, 8) + result = rules.merge_common_rules([rule_conj_tuple]) + self.assertEqual([(rule_conj_tuple[0], [rule_conj_tuple[1]])], + result) + + def test_merge_common_rules(self): + rule_conj_list = [({'direction': 'ingress', 'ethertype': 'IPv4', + 'protocol': 1}, 8), + ({'direction': 'ingress', 'ethertype': 'IPv4', + 'protocol': 1, 'port_range_min': 3}, 16), + ({'direction': 'ingress', 'ethertype': 'IPv4', + 'protocol': 1, 'port_range_min': 3, + 'port_range_max': 0}, 40), + ({'direction': 'ingress', 'ethertype': 'IPv4', + 'protocol': 1}, 24)] + result = rules.merge_common_rules(rule_conj_list) + self.assertItemsEqual( + [({'direction': 'ingress', 'ethertype': 'IPv4', + 'protocol': 1}, [8, 24]), + ({'direction': 'ingress', 'ethertype': 'IPv4', + 'protocol': 1, 'port_range_min': 3}, [16]), + ({'direction': 'ingress', 'ethertype': 'IPv4', + 'protocol': 1, 'port_range_min': 3, 'port_range_max': 0}, + [40])], + result) + + def test_merge_port_ranges_overlapping(self): + result = rules.merge_port_ranges( + [(dict([('port_range_min', 20), ('port_range_max', 30)] + + self.rule_tmpl), 6), + (dict([('port_range_min', 30), ('port_range_max', 40)] + + self.rule_tmpl), 14), + (dict([('port_range_min', 35), ('port_range_max', 40)] + + self.rule_tmpl), 22), + (dict([('port_range_min', 20), ('port_range_max', 20)] + + self.rule_tmpl), 30)]) + self._test_merge_port_ranges_helper([ + # port_range_min, port_range_max, conj_ids + (20, 20, {6, 30}), + (21, 29, {6}), + (30, 30, {6, 14}), + (31, 34, {14}), + (35, 40, {14, 22})], result) + + def test_merge_port_ranges_no_port_ranges(self): + result = rules.merge_port_ranges( + [(dict(self.rule_tmpl), 10), + (dict(self.rule_tmpl), 12), + (dict([('port_range_min', 30), ('port_range_max', 40)] + + self.rule_tmpl), 4)]) + self._test_merge_port_ranges_helper([ + (1, 29, {10, 12}), + (30, 40, {10, 12, 4}), + (41, 65535, {10, 12})], result) + + def test_merge_port_ranges_nonoverlapping(self): + result = rules.merge_port_ranges( + [(dict([('port_range_min', 30), ('port_range_max', 40)] + + self.rule_tmpl), 32), + (dict([('port_range_min', 100), ('port_range_max', 140)] + + self.rule_tmpl), 40)]) + self._test_merge_port_ranges_helper( + [(30, 40, {32}), (100, 140, {40})], result)