From ca792b0d569ca9d55617d6fddfdb53a743c05661 Mon Sep 17 00:00:00 2001 From: venkata anil Date: Fri, 8 Jul 2016 18:49:45 +0000 Subject: [PATCH] Avoid duplicate ipset processing for security groups While applying firewall rules for ports, existing implementation iterates through each port and applies ipset for its security groups. With this, when ports share the security group, ipset for same security group is called again and again while iterating through ports. From the DB, we already get the list of security groups for which ipset members have to be updated. In the new approach, we apply ipset on these security groups(before firewall rules setup), instead of iterating through all ports(during settig up firewall rules)and parsing them for security groups and then applying ipset. With this we can avoid duplicate ipset processing for same security groups. Closes-bug: #1598734 Partial-Bug: #1499177 Change-Id: I3f16d1a3a847e706ff743a8e1a5e7598f9f4c6dd --- neutron/agent/linux/iptables_firewall.py | 13 +--- .../agent/linux/test_iptables_firewall.py | 60 ++----------------- 2 files changed, 8 insertions(+), 65 deletions(-) diff --git a/neutron/agent/linux/iptables_firewall.py b/neutron/agent/linux/iptables_firewall.py index 84629b02df2..2910a1be7d6 100644 --- a/neutron/agent/linux/iptables_firewall.py +++ b/neutron/agent/linux/iptables_firewall.py @@ -142,6 +142,9 @@ class IptablesFirewallDriver(firewall.FirewallDriver): def update_security_group_members(self, sg_id, sg_members): LOG.debug("Update members of security group (%s)", sg_id) self.sg_members[sg_id] = collections.defaultdict(list, sg_members) + if self.enable_ipset: + for ip_version, current_ips in sg_members.items(): + self.ipset.set_members(sg_id, ip_version, current_ips) def _set_ports(self, port): if not firewall.port_sec_enabled(port): @@ -501,10 +504,6 @@ class IptablesFirewallDriver(firewall.FirewallDriver): # select rules for current port and direction security_group_rules = self._select_sgr_by_direction(port, direction) security_group_rules += self._select_sg_rules_for_port(port, direction) - # make sure ipset members are updated for remote security groups - if self.enable_ipset: - remote_sg_ids = self._get_remote_sg_ids(port, direction) - self._update_ipset_members(remote_sg_ids) # split groups by ip version # for ipv4, iptables command is used # for ipv6, iptables6 command is used @@ -536,12 +535,6 @@ class IptablesFirewallDriver(firewall.FirewallDriver): ipv6_iptables_rules) self._drop_dhcp_rule(ipv4_iptables_rules, ipv6_iptables_rules) - def _update_ipset_members(self, security_group_ids): - for ip_version, sg_ids in security_group_ids.items(): - for sg_id in sg_ids: - current_ips = self.sg_members[sg_id][ip_version] - self.ipset.set_members(sg_id, ip_version, current_ips) - def _generate_ipset_rule_args(self, sg_rule, remote_gid): ethertype = sg_rule.get('ethertype') ipset_name = self.ipset.get_name(remote_gid, ethertype) diff --git a/neutron/tests/unit/agent/linux/test_iptables_firewall.py b/neutron/tests/unit/agent/linux/test_iptables_firewall.py index f203556752d..829fe650bf0 100644 --- a/neutron/tests/unit/agent/linux/test_iptables_firewall.py +++ b/neutron/tests/unit/agent/linux/test_iptables_firewall.py @@ -1631,13 +1631,9 @@ class IptablesFirewallEnhancedIpsetTestCase(BaseIptablesFirewallTestCase): def _fake_sg_members(self, sg_ids=None): return {sg_id: copy.copy(FAKE_IP) for sg_id in (sg_ids or [FAKE_SGID])} - def test_prepare_port_filter_with_new_members(self): - self.firewall.sg_rules = self._fake_sg_rules() - self.firewall.sg_members = {'fake_sgid': { - 'IPv4': ['10.0.0.1', '10.0.0.2'], 'IPv6': ['fe80::1']}} - self.firewall.pre_sg_members = {} - port = self._fake_port() - self.firewall.prepare_port_filter(port) + def test_update_security_group_members(self): + sg_members = {'IPv4': ['10.0.0.1', '10.0.0.2'], 'IPv6': ['fe80::1']} + self.firewall.update_security_group_members('fake_sgid', sg_members) calls = [ mock.call.set_members('fake_sgid', 'IPv4', ['10.0.0.1', '10.0.0.2']), @@ -1774,34 +1770,14 @@ class IptablesFirewallEnhancedIpsetTestCase(BaseIptablesFirewallTestCase): self.assertEqual(1, len(sg_chain_v4_accept)) self.assertEqual(1, len(sg_chain_v6_accept)) - def test_prepare_port_filter_with_deleted_member(self): - self.firewall.sg_rules = self._fake_sg_rules() - self.firewall.pre_sg_rules = self._fake_sg_rules() - self.firewall.sg_members = {'fake_sgid': { - 'IPv4': [ - '10.0.0.1', '10.0.0.3', '10.0.0.4', '10.0.0.5'], - 'IPv6': ['fe80::1']}} - self.firewall.pre_sg_members = {'fake_sgid': { - 'IPv4': ['10.0.0.2'], - 'IPv6': ['fe80::1']}} - self.firewall.prepare_port_filter(self._fake_port()) - calls = [ - mock.call.set_members('fake_sgid', 'IPv4', - ['10.0.0.1', '10.0.0.3', '10.0.0.4', - '10.0.0.5']), - mock.call.set_members('fake_sgid', 'IPv6', ['fe80::1'])] - - self.firewall.ipset.assert_has_calls(calls, True) - def test_remove_port_filter_with_destroy_ipset_chain(self): self.firewall.sg_rules = self._fake_sg_rules() port = self._fake_port() - self.firewall.sg_members = {'fake_sgid': { - 'IPv4': ['10.0.0.1'], - 'IPv6': ['fe80::1']}} self.firewall.pre_sg_members = {'fake_sgid': { 'IPv4': [], 'IPv6': []}} + sg_members = {'IPv4': ['10.0.0.1'], 'IPv6': ['fe80::1']} + self.firewall.update_security_group_members('fake_sgid', sg_members) self.firewall.prepare_port_filter(port) self.firewall.filter_defer_apply_on() self.firewall.sg_members = {'fake_sgid': { @@ -1824,24 +1800,6 @@ class IptablesFirewallEnhancedIpsetTestCase(BaseIptablesFirewallTestCase): self.firewall.ipset.assert_has_calls(calls, any_order=True) - def test_prepare_port_filter_with_sg_no_member(self): - self.firewall.sg_rules = self._fake_sg_rules() - self.firewall.sg_rules[FAKE_SGID].append( - {'direction': 'ingress', 'remote_group_id': 'fake_sgid2', - 'ethertype': 'IPv4'}) - self.firewall.sg_rules.update() - self.firewall.sg_members['fake_sgid'] = { - 'IPv4': ['10.0.0.1', '10.0.0.2'], 'IPv6': ['fe80::1']} - self.firewall.pre_sg_members = {} - port = self._fake_port() - port['security_group_source_groups'].append('fake_sgid2') - self.firewall.prepare_port_filter(port) - calls = [mock.call.set_members('fake_sgid', 'IPv4', - ['10.0.0.1', '10.0.0.2']), - mock.call.set_members('fake_sgid', 'IPv6', ['fe80::1'])] - - self.firewall.ipset.assert_has_calls(calls, any_order=True) - def test_filter_defer_apply_off_with_sg_only_ipv6_rule(self): self.firewall.sg_rules = self._fake_sg_rules() self.firewall.pre_sg_rules = self._fake_sg_rules() @@ -1903,14 +1861,6 @@ class IptablesFirewallEnhancedIpsetTestCase(BaseIptablesFirewallTestCase): mac_ipv4_pairs, mac_ipv6_pairs) self.assertEqual(fake_ipv6_pair, mac_ipv6_pairs) - def test_update_ipset_members(self): - self.firewall.sg_members[FAKE_SGID][_IPv4] = [] - self.firewall.sg_members[FAKE_SGID][_IPv6] = [] - sg_info = {constants.IPv4: [FAKE_SGID]} - self.firewall._update_ipset_members(sg_info) - calls = [mock.call.set_members(FAKE_SGID, constants.IPv4, [])] - self.firewall.ipset.assert_has_calls(calls) - class OVSHybridIptablesFirewallTestCase(BaseIptablesFirewallTestCase):