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

diff --git a/neutron/agent/linux/iptables_firewall.py b/neutron/agent/linux/iptables_firewall.py
index 9684e331390bdd8ece67dbca9262a7befa113238..a080e6ef20e9992807a9d145be78cc3a9ddd4a27 100644 (file)
--- 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 8c9b9e2a4bd3a38e1ba7992d34f0210112af9db3..3f878ecb14dd71fde56d921b8f3c318ea2d07392 100644 (file)
--- 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)