From 18c578aa10c19a6befdf1f1510645200f173eb44 Mon Sep 17 00:00:00 2001 From: Brian Haley Date: Thu, 28 Feb 2019 22:19:16 -0500 Subject: [PATCH] Fix KeyError in OVS firewall When merging port ranges, the code never assumed the conjunction ID might not be present in the set due to already being removed. In this case there were two security groups, both using the same remote security group, but the first security group does not define a port range and the second one does. Or more generally, the first SG port range is a subset of the second, as no port-range means the full range. Change-Id: I17ab643abbd2ec21eda4ae1dfb9abf2d4b0657f2 Closes-bug: #1813007 --- .../agent/linux/openvswitch_firewall/rules.py | 16 +++++++++++----- .../linux/openvswitch_firewall/test_rules.py | 13 +++++++++++-- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/neutron/agent/linux/openvswitch_firewall/rules.py b/neutron/agent/linux/openvswitch_firewall/rules.py index c23c9ea0c80..86f2ad70d61 100644 --- a/neutron/agent/linux/openvswitch_firewall/rules.py +++ b/neutron/agent/linux/openvswitch_firewall/rules.py @@ -121,26 +121,32 @@ def merge_port_ranges(rule_conj_list): # item means a removal. result = [] rule_tmpl = rule_conj_list[0][0] - cur_conj = set() + cur_conj = {} cur_range_min = None for port, m, conj_id in port_ranges: if m == 'min': + if conj_id in cur_conj: + cur_conj[conj_id] += 1 + continue 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))) + result.append((rule, list(cur_conj.keys()))) cur_range_min = port - cur_conj.add(conj_id) + cur_conj[conj_id] = 1 else: + if cur_conj[conj_id] > 1: + cur_conj[conj_id] -= 1 + continue 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))) + result.append((rule, list(cur_conj.keys()))) # The next port range without 'port' starts from (port + 1) cur_range_min = port + 1 - cur_conj.remove(conj_id) + del cur_conj[conj_id] if (len(result) == 1 and result[0][0]['port_range_min'] == 1 and result[0][0]['port_range_max'] == 65535): 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 2a720504f39..5f0dec8a6d6 100644 --- a/neutron/tests/unit/agent/linux/openvswitch_firewall/test_rules.py +++ b/neutron/tests/unit/agent/linux/openvswitch_firewall/test_rules.py @@ -420,8 +420,8 @@ class TestMergeRules(base.BaseTestCase): 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(range_min, result1[0].get('port_range_min')) + self.assertEqual(range_max, result1[0].get('port_range_max')) self.assertEqual(conj_ids, set(result1[1])) def test__assert_mergeable_rules(self): @@ -489,6 +489,15 @@ class TestMergeRules(base.BaseTestCase): (30, 40, {10, 12, 4}), (41, 65535, {10, 12})], result) + def test_merge_port_ranges_no_port_ranges_same_conj_id(self): + result = rules.merge_port_ranges( + [(dict(self.rule_tmpl), 10), + (dict(self.rule_tmpl), 12), + (dict([('port_range_min', 30), ('port_range_max', 30)] + + self.rule_tmpl), 10)]) + self._test_merge_port_ranges_helper([ + (None, None, {10, 12})], result) + def test_merge_port_ranges_nonoverlapping(self): result = rules.merge_port_ranges( [(dict([('port_range_min', 30), ('port_range_max', 40)] +