fix call which is only specific to enhanced_rpc

security_group_updated method is not implemented in the default
FirewallDriver class. But SecurityGroupAgentRpc class assumes the method
is implemented by the underlying Firewall driver.

This patch adds a check to ensure that only the drivers which leverages
enhanced RPC will make the call.

Related-Bug: #1493104
Change-Id: I0ee44a5ce69368c7731adfe9953adc0b2c5ff2d2

diff --git a/neutron/agent/firewall.py b/neutron/agent/firewall.py
index 04a327b21df66f01bd77898c2d6275592634d4ab..18623ee3a250ab0cff9005cdb2b3ac157e062813 100644 (file)
--- a/neutron/agent/firewall.py
+++ b/neutron/agent/firewall.py
@@ -119,7 +119,11 @@ class FirewallDriver(object):
 
     def security_group_updated(self, action_type, sec_group_ids,
                                device_id=None):
-        """Called when a security group is updated."""
+        """Called when a security group is updated.
+
+        Note: This method needs to be implemented by the firewall drivers
+        which use enhanced RPC for security_groups.
+        """
         raise NotImplementedError()
 
 

diff --git a/neutron/agent/securitygroups_rpc.py b/neutron/agent/securitygroups_rpc.py
index 5efc4cfe3e9645dcd0bde3c7dba815a37132fb33..a08736773bbb5d22a679eca28c40007a06c907c2 100644 (file)
--- a/neutron/agent/securitygroups_rpc.py
+++ b/neutron/agent/securitygroups_rpc.py
@@ -200,7 +200,8 @@ class SecurityGroupAgentRpc(object):
             if sec_grp_set & set(device.get(attribute, [])):
                 devices.append(device['device'])
         if devices:
-            self.firewall.security_group_updated(action_type, sec_grp_set)
+            if self.use_enhanced_rpc:
+                self.firewall.security_group_updated(action_type, sec_grp_set)
             if self.defer_refresh_firewall:
                 LOG.debug("Adding %s devices to the list of devices "
                           "for which firewall needs to be refreshed",
@@ -293,8 +294,9 @@ class SecurityGroupAgentRpc(object):
             LOG.debug("Refreshing firewall for all filtered devices")
             self.refresh_firewall()
         else:
-            self.firewall.security_group_updated('sg_member', [],
-                                                 updated_devices)
+            if self.use_enhanced_rpc:
+                self.firewall.security_group_updated('sg_member', [],
+                                                     updated_devices)
             # If a device is both in new and updated devices
             # avoid reprocessing it
             updated_devices = ((updated_devices | devices_to_refilter) -

diff --git a/neutron/tests/unit/agent/test_securitygroups_rpc.py b/neutron/tests/unit/agent/test_securitygroups_rpc.py
index 7f63c31c8c7c186c5fd97182e010f97fa45852c2..3e755d34d85f3858344ff03eb3ba9eaf129c4f1c 100644 (file)
--- a/neutron/tests/unit/agent/test_securitygroups_rpc.py
+++ b/neutron/tests/unit/agent/test_securitygroups_rpc.py
@@ -1129,6 +1129,7 @@ class BaseSecurityGroupAgentRpcTestCase(base.BaseTestCase):
                                                       'remote_group_id':
                                                       'fake_sgid2'}]}
         self.firewall.ports = {'fake_device': self.fake_device}
+        self.firewall.security_group_updated = mock.Mock()
 
 
 class SecurityGroupAgentRpcTestCase(BaseSecurityGroupAgentRpcTestCase):
@@ -1179,12 +1180,14 @@ class SecurityGroupAgentRpcTestCase(BaseSecurityGroupAgentRpcTestCase):
         self.agent.security_groups_rule_updated(['fake_sgid1', 'fake_sgid3'])
         self.agent.refresh_firewall.assert_has_calls(
             [mock.call.refresh_firewall([self.fake_device['device']])])
+        self.assertFalse(self.firewall.security_group_updated.called)
 
     def test_security_groups_rule_not_updated(self):
         self.agent.refresh_firewall = mock.Mock()
         self.agent.prepare_devices_filter(['fake_port_id'])
         self.agent.security_groups_rule_updated(['fake_sgid3', 'fake_sgid4'])
         self.assertFalse(self.agent.refresh_firewall.called)
+        self.assertFalse(self.firewall.security_group_updated.called)
 
     def test_security_groups_member_updated(self):
         self.agent.refresh_firewall = mock.Mock()
@@ -1192,12 +1195,14 @@ class SecurityGroupAgentRpcTestCase(BaseSecurityGroupAgentRpcTestCase):
         self.agent.security_groups_member_updated(['fake_sgid2', 'fake_sgid3'])
         self.agent.refresh_firewall.assert_has_calls(
             [mock.call.refresh_firewall([self.fake_device['device']])])
+        self.assertFalse(self.firewall.security_group_updated.called)
 
     def test_security_groups_member_not_updated(self):
         self.agent.refresh_firewall = mock.Mock()
         self.agent.prepare_devices_filter(['fake_port_id'])
         self.agent.security_groups_member_updated(['fake_sgid3', 'fake_sgid4'])
         self.assertFalse(self.agent.refresh_firewall.called)
+        self.assertFalse(self.firewall.security_group_updated.called)
 
     def test_security_groups_provider_updated(self):
         self.agent.refresh_firewall = mock.Mock()
@@ -1289,26 +1294,31 @@ class SecurityGroupAgentEnhancedRpcTestCase(
                                         ])
 
     def test_security_groups_rule_updated_enhanced_rpc(self):
+        sg_list = ['fake_sgid1', 'fake_sgid3']
         self.agent.refresh_firewall = mock.Mock()
         self.agent.prepare_devices_filter(['fake_port_id'])
-        self.agent.security_groups_rule_updated(['fake_sgid1', 'fake_sgid3'])
+        self.agent.security_groups_rule_updated(sg_list)
         self.agent.refresh_firewall.assert_called_once_with(
             [self.fake_device['device']])
+        self.firewall.security_group_updated.assert_called_once_with(
+            'sg_rule', set(sg_list))
 
     def test_security_groups_rule_not_updated_enhanced_rpc(self):
         self.agent.refresh_firewall = mock.Mock()
         self.agent.prepare_devices_filter(['fake_port_id'])
         self.agent.security_groups_rule_updated(['fake_sgid3', 'fake_sgid4'])
         self.assertFalse(self.agent.refresh_firewall.called)
+        self.assertFalse(self.firewall.security_group_updated.called)
 
     def test_security_groups_member_updated_enhanced_rpc(self):
+        sg_list = ['fake_sgid2', 'fake_sgid3']
         self.agent.refresh_firewall = mock.Mock()
         self.agent.prepare_devices_filter(['fake_port_id'])
-        self.agent.security_groups_member_updated(
-            ['fake_sgid2', 'fake_sgid3'])
-
+        self.agent.security_groups_member_updated(sg_list)
         self.agent.refresh_firewall.assert_called_once_with(
             [self.fake_device['device']])
+        self.firewall.security_group_updated.assert_called_once_with(
+            'sg_member', set(sg_list))
 
     def test_security_groups_member_not_updated_enhanced_rpc(self):
         self.agent.refresh_firewall = mock.Mock()
@@ -1316,6 +1326,7 @@ class SecurityGroupAgentEnhancedRpcTestCase(
         self.agent.security_groups_member_updated(
             ['fake_sgid3', 'fake_sgid4'])
         self.assertFalse(self.agent.refresh_firewall.called)
+        self.assertFalse(self.firewall.security_group_updated.called)
 
     def test_security_groups_provider_updated_enhanced_rpc(self):
         self.agent.refresh_firewall = mock.Mock()
@@ -1391,6 +1402,7 @@ class SecurityGroupAgentRpcWithDeferredRefreshTestCase(
     def test_security_groups_rule_updated(self):
         self.agent.security_groups_rule_updated(['fake_sgid1', 'fake_sgid3'])
         self.assertIn('fake_device', self.agent.devices_to_refilter)
+        self.assertFalse(self.firewall.security_group_updated.called)
 
     def test_multiple_security_groups_rule_updated_same_port(self):
         with self.add_fake_device(device='fake_device_2',
@@ -1400,6 +1412,7 @@ class SecurityGroupAgentRpcWithDeferredRefreshTestCase(
             self.agent.security_groups_rule_updated(['fake_sgid2'])
             self.assertIn('fake_device', self.agent.devices_to_refilter)
             self.assertNotIn('fake_device_2', self.agent.devices_to_refilter)
+            self.assertFalse(self.firewall.security_group_updated.called)
 
     def test_security_groups_rule_updated_multiple_ports(self):
         with self.add_fake_device(device='fake_device_2',
@@ -1409,6 +1422,7 @@ class SecurityGroupAgentRpcWithDeferredRefreshTestCase(
                                                      'fake_sgid2'])
             self.assertIn('fake_device', self.agent.devices_to_refilter)
             self.assertIn('fake_device_2', self.agent.devices_to_refilter)
+            self.assertFalse(self.firewall.security_group_updated.called)
 
     def test_multiple_security_groups_rule_updated_multiple_ports(self):
         with self.add_fake_device(device='fake_device_2',
@@ -1418,10 +1432,12 @@ class SecurityGroupAgentRpcWithDeferredRefreshTestCase(
             self.agent.security_groups_rule_updated(['fake_sgid2'])
             self.assertIn('fake_device', self.agent.devices_to_refilter)
             self.assertIn('fake_device_2', self.agent.devices_to_refilter)
+            self.assertFalse(self.firewall.security_group_updated.called)
 
     def test_security_groups_member_updated(self):
         self.agent.security_groups_member_updated(['fake_sgid2', 'fake_sgid3'])
         self.assertIn('fake_device', self.agent.devices_to_refilter)
+        self.assertFalse(self.firewall.security_group_updated.called)
 
     def test_multiple_security_groups_member_updated_same_port(self):
         with self.add_fake_device(device='fake_device_2',
@@ -1434,6 +1450,7 @@ class SecurityGroupAgentRpcWithDeferredRefreshTestCase(
                                                        'fake_sgid3'])
             self.assertIn('fake_device', self.agent.devices_to_refilter)
             self.assertNotIn('fake_device_2', self.agent.devices_to_refilter)
+            self.assertFalse(self.firewall.security_group_updated.called)
 
     def test_security_groups_member_updated_multiple_ports(self):
         with self.add_fake_device(device='fake_device_2',
@@ -1442,6 +1459,7 @@ class SecurityGroupAgentRpcWithDeferredRefreshTestCase(
             self.agent.security_groups_member_updated(['fake_sgid2'])
             self.assertIn('fake_device', self.agent.devices_to_refilter)
             self.assertIn('fake_device_2', self.agent.devices_to_refilter)
+            self.assertFalse(self.firewall.security_group_updated.called)
 
     def test_multiple_security_groups_member_updated_multiple_ports(self):
         with self.add_fake_device(device='fake_device_2',
@@ -1451,6 +1469,7 @@ class SecurityGroupAgentRpcWithDeferredRefreshTestCase(
             self.agent.security_groups_member_updated(['fake_sgid2'])
             self.assertIn('fake_device', self.agent.devices_to_refilter)
             self.assertIn('fake_device_2', self.agent.devices_to_refilter)
+            self.assertFalse(self.firewall.security_group_updated.called)
 
     def test_security_groups_provider_updated(self):
         self.agent.security_groups_provider_updated(None)
@@ -1474,6 +1493,7 @@ class SecurityGroupAgentRpcWithDeferredRefreshTestCase(
         self.agent.prepare_devices_filter.assert_called_once_with(
             set(['fake_new_device']))
         self.assertFalse(self.agent.refresh_firewall.called)
+        self.assertFalse(self.firewall.security_group_updated.called)
 
     def test_setup_port_filters_updated_ports_only(self):
         self.agent.prepare_devices_filter = mock.Mock()
@@ -1486,6 +1506,7 @@ class SecurityGroupAgentRpcWithDeferredRefreshTestCase(
         self.agent.refresh_firewall.assert_called_once_with(
             set(['fake_updated_device']))
         self.assertFalse(self.agent.prepare_devices_filter.called)
+        self.assertFalse(self.firewall.security_group_updated.called)
 
     def test_setup_port_filter_new_and_updated_ports(self):
         self.agent.prepare_devices_filter = mock.Mock()
@@ -1500,6 +1521,7 @@ class SecurityGroupAgentRpcWithDeferredRefreshTestCase(
             set(['fake_new_device']))
         self.agent.refresh_firewall.assert_called_once_with(
             set(['fake_updated_device']))
+        self.assertFalse(self.firewall.security_group_updated.called)
 
     def test_setup_port_filters_sg_updates_only(self):
         self.agent.prepare_devices_filter = mock.Mock()
@@ -1512,6 +1534,7 @@ class SecurityGroupAgentRpcWithDeferredRefreshTestCase(
         self.agent.refresh_firewall.assert_called_once_with(
             set(['fake_device']))
         self.assertFalse(self.agent.prepare_devices_filter.called)
+        self.assertFalse(self.firewall.security_group_updated.called)
 
     def test_setup_port_filters_sg_updates_and_new_ports(self):
         self.agent.prepare_devices_filter = mock.Mock()
@@ -1525,6 +1548,7 @@ class SecurityGroupAgentRpcWithDeferredRefreshTestCase(
             set(['fake_new_device']))
         self.agent.refresh_firewall.assert_called_once_with(
             set(['fake_device']))
+        self.assertFalse(self.firewall.security_group_updated.called)
 
     def _test_prepare_devices_filter(self, devices):
         # simulate an RPC arriving and calling _security_group_updated()
@@ -1544,6 +1568,7 @@ class SecurityGroupAgentRpcWithDeferredRefreshTestCase(
         self.assertFalse(self.agent.global_refresh_firewall)
         self.agent.refresh_firewall.assert_called_once_with(
             set(['fake_device']))
+        self.assertFalse(self.firewall.security_group_updated.called)
 
     def test_setup_port_filters_sg_updates_and_updated_ports(self):
         self.agent.prepare_devices_filter = mock.Mock()
@@ -1557,6 +1582,7 @@ class SecurityGroupAgentRpcWithDeferredRefreshTestCase(
         self.agent.refresh_firewall.assert_called_once_with(
             set(['fake_device', 'fake_device_2', 'fake_updated_device']))
         self.assertFalse(self.agent.prepare_devices_filter.called)
+        self.assertFalse(self.firewall.security_group_updated.called)
 
     def test_setup_port_filters_all_updates(self):
         self.agent.prepare_devices_filter = mock.Mock()
@@ -1572,6 +1598,7 @@ class SecurityGroupAgentRpcWithDeferredRefreshTestCase(
             set(['fake_new_device']))
         self.agent.refresh_firewall.assert_called_once_with(
             set(['fake_device', 'fake_device_2', 'fake_updated_device']))
+        self.assertFalse(self.firewall.security_group_updated.called)
 
     def test_setup_port_filters_no_update(self):
         self.agent.prepare_devices_filter = mock.Mock()
@@ -1583,6 +1610,7 @@ class SecurityGroupAgentRpcWithDeferredRefreshTestCase(
         self.assertFalse(self.agent.global_refresh_firewall)
         self.assertFalse(self.agent.refresh_firewall.called)
         self.assertFalse(self.agent.prepare_devices_filter.called)
+        self.assertFalse(self.firewall.security_group_updated.called)
 
     def test_setup_port_filters_with_global_refresh(self):
         self.agent.prepare_devices_filter = mock.Mock()
@@ -1594,6 +1622,7 @@ class SecurityGroupAgentRpcWithDeferredRefreshTestCase(
         self.assertFalse(self.agent.global_refresh_firewall)
         self.agent.refresh_firewall.assert_called_once_with()
         self.assertFalse(self.agent.prepare_devices_filter.called)
+        self.assertFalse(self.firewall.security_group_updated.called)
 
 
 class FakeSGNotifierAPI(sg_rpc.SecurityGroupAgentRpcApiMixin):
@@ -2631,6 +2660,7 @@ class TestSecurityGroupAgentWithIptables(base.BaseTestCase):
                                        '12:34:56:78:9a:bd',
                                        rule5))
         ])
+        self.agent.firewall.security_group_updated = mock.Mock()
 
     @staticmethod
     def _enforce_order_in_firewall(firewall):
@@ -2677,7 +2707,7 @@ class TestSecurityGroupAgentWithIptables(base.BaseTestCase):
         self.expected_calls.append(mock.call(*args, **kwargs))
         self.expected_call_count += 1
 
-    def _verify_mock_calls(self):
+    def _verify_mock_calls(self, exp_fw_sg_updated_call=False):
         self.assertEqual(self.expected_call_count,
                          self.iptables_execute.call_count)
         self.iptables_execute.assert_has_calls(self.expected_calls)
@@ -2698,6 +2728,8 @@ class TestSecurityGroupAgentWithIptables(base.BaseTestCase):
         for e in expected:
             self.utils_exec.assert_any_call(['sysctl', '-w', e],
                                             run_as_root=True)
+        self.assertEqual(exp_fw_sg_updated_call,
+                         self.agent.firewall.security_group_updated.called)
 
     def _replay_iptables(self, v4_filter, v6_filter, raw):
         self._register_mock_call(
@@ -2870,7 +2902,9 @@ class TestSecurityGroupAgentEnhancedRpcWithIptables(
         self.agent.remove_devices_filter(['tap_port2'])
         self.agent.remove_devices_filter(['tap_port1'])
 
-        self._verify_mock_calls()
+        self._verify_mock_calls(True)
+        self.assertEqual(
+            2, self.agent.firewall.security_group_updated.call_count)
 
     def test_security_group_rule_updated(self):
         self.sg_info.return_value = self.devices_info2
@@ -2883,7 +2917,9 @@ class TestSecurityGroupAgentEnhancedRpcWithIptables(
         self.sg_info.return_value = self.devices_info3
         self.agent.security_groups_rule_updated(['security_group1'])
 
-        self._verify_mock_calls()
+        self._verify_mock_calls(True)
+        self.agent.firewall.security_group_updated.assert_called_with(
+            'sg_rule', set(['security_group1']))
 
 
 class TestSecurityGroupAgentEnhancedIpsetWithIptables(
@@ -2932,7 +2968,9 @@ class TestSecurityGroupAgentEnhancedIpsetWithIptables(
         self.agent.remove_devices_filter(['tap_port2'])
         self.agent.remove_devices_filter(['tap_port1'])
 
-        self._verify_mock_calls()
+        self._verify_mock_calls(True)
+        self.assertEqual(
+            2, self.agent.firewall.security_group_updated.call_count)
 
     def test_security_group_rule_updated(self):
         self.sg_info.return_value = self.devices_info2
@@ -2945,7 +2983,9 @@ class TestSecurityGroupAgentEnhancedIpsetWithIptables(
         self.sg_info.return_value = self.devices_info3
         self.agent.security_groups_rule_updated(['security_group1'])
 
-        self._verify_mock_calls()
+        self._verify_mock_calls(True)
+        self.agent.firewall.security_group_updated.assert_called_with(
+            'sg_rule', set(['security_group1']))
 
 
 class SGNotificationTestMixin(object):