From 653fd35be2ebc7bdfd1eb417d086cf0884be29cd Mon Sep 17 00:00:00 2001 From: shihanzhang Date: Tue, 9 Jun 2015 17:47:39 +0800 Subject: [PATCH] Destroy ipset when the corresponding rule is removed if a security group has a rule which allow a remote group access, but this remote group has no IPv4 and IPv6 members, L2 agent should not clear the remote group in internal cache of sg_members, because when above rule is deleted, L2 agent can get the remote group id from the diff of pre_sg_members-sg_members, then destroy the ipset set for remote group. Change-Id: I801b14c9f506c5a07f8875b8f9be1b05d181b842 Closes-bug: #1463331 --- neutron/agent/linux/iptables_firewall.py | 21 ++++------ .../agent/linux/test_iptables_firewall.py | 39 +++++++++++-------- 2 files changed, 29 insertions(+), 31 deletions(-) diff --git a/neutron/agent/linux/iptables_firewall.py b/neutron/agent/linux/iptables_firewall.py index 9684e331390..a080e6ef20e 100644 --- a/neutron/agent/linux/iptables_firewall.py +++ b/neutron/agent/linux/iptables_firewall.py @@ -638,11 +638,10 @@ class IptablesFirewallDriver(firewall.FirewallDriver): filtered_ports) for ip_version, remote_sg_ids in six.iteritems(remote_sgs_to_remove): - self._clear_sg_members(ip_version, remote_sg_ids) if self.enable_ipset: self._remove_ipsets_for_remote_sgs(ip_version, remote_sg_ids) - self._remove_unused_sg_members() + self._remove_sg_members(remote_sgs_to_remove) # Remove unused security group rules for remove_group_id in self._determine_sg_rules_to_remove( @@ -690,23 +689,17 @@ class IptablesFirewallDriver(firewall.FirewallDriver): port_group_ids.update(port.get('security_groups', [])) return port_group_ids - def _clear_sg_members(self, ip_version, remote_sg_ids): - """Clear our internal cache of sg members matching the parameters.""" - for remote_sg_id in remote_sg_ids: - if self.sg_members[remote_sg_id][ip_version]: - self.sg_members[remote_sg_id][ip_version] = [] - def _remove_ipsets_for_remote_sgs(self, ip_version, remote_sg_ids): """Remove system ipsets matching the provided parameters.""" for remote_sg_id in remote_sg_ids: self.ipset.destroy(remote_sg_id, ip_version) - def _remove_unused_sg_members(self): - """Remove sg_member entries where no IPv4 or IPv6 is associated.""" - for sg_id in list(self.sg_members.keys()): - sg_has_members = (self.sg_members[sg_id][constants.IPv4] or - self.sg_members[sg_id][constants.IPv6]) - if not sg_has_members: + def _remove_sg_members(self, remote_sgs_to_remove): + """Remove sg_member entries.""" + ipv4_sec_group_set = remote_sgs_to_remove.get(constants.IPv4) + ipv6_sec_group_set = remote_sgs_to_remove.get(constants.IPv6) + for sg_id in (ipv4_sec_group_set & ipv6_sec_group_set): + if sg_id in self.sg_members: del self.sg_members[sg_id] def _find_deleted_sg_rules(self, sg_id): diff --git a/neutron/tests/unit/agent/linux/test_iptables_firewall.py b/neutron/tests/unit/agent/linux/test_iptables_firewall.py index 8c9b9e2a4bd..3f878ecb14d 100644 --- a/neutron/tests/unit/agent/linux/test_iptables_firewall.py +++ b/neutron/tests/unit/agent/linux/test_iptables_firewall.py @@ -1620,20 +1620,12 @@ class IptablesFirewallEnhancedIpsetTestCase(BaseIptablesFirewallTestCase): self.assertEqual(sg_ids, self.firewall._get_sg_ids_set_for_ports(ports)) - def test_clear_sg_members(self): - self.firewall.sg_members = self._fake_sg_members( - sg_ids=[FAKE_SGID, OTHER_SGID]) - self.firewall._clear_sg_members(_IPv4, [OTHER_SGID]) - - self.assertEqual(0, len(self.firewall.sg_members[OTHER_SGID][_IPv4])) - - def test_remove_unused_sg_members(self): + def test_remove_sg_members(self): self.firewall.sg_members = self._fake_sg_members([FAKE_SGID, OTHER_SGID]) - self.firewall.sg_members[FAKE_SGID][_IPv4] = [] - self.firewall.sg_members[FAKE_SGID][_IPv6] = [] - self.firewall.sg_members[OTHER_SGID][_IPv6] = [] - self.firewall._remove_unused_sg_members() + remote_sgs_to_remove = {_IPv4: set([FAKE_SGID]), + _IPv6: set([FAKE_SGID, OTHER_SGID])} + self.firewall._remove_sg_members(remote_sgs_to_remove) self.assertIn(OTHER_SGID, self.firewall.sg_members) self.assertNotIn(FAKE_SGID, self.firewall.sg_members) @@ -1652,13 +1644,26 @@ class IptablesFirewallEnhancedIpsetTestCase(BaseIptablesFirewallTestCase): self.assertNotIn(OTHER_SGID, self.firewall.sg_rules) def test_remove_unused_security_group_info(self): - self._setup_fake_firewall_members_and_rules(self.firewall) - # no filtered ports in 'fake_sgid', so all rules and members - # are not needed and we expect them to be cleaned up - self.firewall.prepare_port_filter(self._fake_port(OTHER_SGID)) + self.firewall.sg_members = {OTHER_SGID: {_IPv4: [], _IPv6: []}} + self.firewall.pre_sg_members = self.firewall.sg_members + self.firewall.sg_rules = self._fake_sg_rules( + remote_groups={_IPv4: [FAKE_SGID], _IPv6: [FAKE_SGID]}) + self.firewall.pre_sg_rules = self.firewall.sg_rules + port = self._fake_port() + self.firewall.filtered_ports['tapfake_dev'] = port self.firewall._remove_unused_security_group_info() + self.assertNotIn(OTHER_SGID, self.firewall.sg_members) - self.assertNotIn(FAKE_SGID, self.firewall.sg_members) + def test_not_remove_used_security_group_info(self): + self.firewall.sg_members = {OTHER_SGID: {_IPv4: [], _IPv6: []}} + self.firewall.pre_sg_members = self.firewall.sg_members + self.firewall.sg_rules = self._fake_sg_rules( + remote_groups={_IPv4: [OTHER_SGID], _IPv6: [OTHER_SGID]}) + self.firewall.pre_sg_rules = self.firewall.sg_rules + port = self._fake_port() + self.firewall.filtered_ports['tapfake_dev'] = port + self.firewall._remove_unused_security_group_info() + self.assertIn(OTHER_SGID, self.firewall.sg_members) def test_remove_all_unused_info(self): self._setup_fake_firewall_members_and_rules(self.firewall)