Remove chain for correct router during update_routers()

The existing code incorrectly used the stale value from a previous list
comprehension - and deleted the chains for the wrong router :(
(Found via pylint)

Also: change to using a set() rather than a list(), since it is used for
repeated membership tests.

Also: refactor test cases to remove test case duplication.

Closes-Bug: #1362466
Change-Id: I4df400d57bab5427362db47a715576faa6340173

diff --git a/neutron/services/metering/drivers/iptables/iptables_driver.py b/neutron/services/metering/drivers/iptables/iptables_driver.py
index 6c3290502cf04152bc68baed2fe78c69f7998334..12dd1bd26f5cbbc058e284c3fcaf8d812210fb57 100644 (file)
--- a/neutron/services/metering/drivers/iptables/iptables_driver.py
+++ b/neutron/services/metering/drivers/iptables/iptables_driver.py
@@ -15,6 +15,7 @@
 # under the License.
 
 from oslo.config import cfg
+import six
 
 from neutron.agent.common import config
 from neutron.agent.linux import interface
@@ -104,10 +105,10 @@ class IptablesMeteringDriver(abstract_driver.MeteringAbstractDriver):
     @log.log
     def update_routers(self, context, routers):
         # disassociate removed routers
-        router_ids = [router['id'] for router in routers]
-        for router_id in self.routers:
+        router_ids = set(router['id'] for router in routers)
+        for router_id, rm in six.iteritems(self.routers):
             if router_id not in router_ids:
-                self._process_disassociate_metering_label(router)
+                self._process_disassociate_metering_label(rm.router)
 
         for router in routers:
             old_gw_port_id = None

diff --git a/neutron/tests/unit/services/metering/drivers/test_iptables_driver.py b/neutron/tests/unit/services/metering/drivers/test_iptables_driver.py
index 8f18343a0c20f924f227c8082f54f364e58115be..160a01bf613730ad575268866015e1e76739149e 100644 (file)
--- a/neutron/tests/unit/services/metering/drivers/test_iptables_driver.py
+++ b/neutron/tests/unit/services/metering/drivers/test_iptables_driver.py
@@ -26,6 +26,38 @@ from neutron.tests.unit import test_api_v2
 _uuid = test_api_v2._uuid
 
 
+TEST_ROUTERS = [
+    {'_metering_labels': [
+        {'id': 'c5df2fe5-c600-4a2a-b2f4-c0fb6df73c83',
+         'rules': [{
+             'direction': 'ingress',
+             'excluded': False,
+             'id': '7f1a261f-2489-4ed1-870c-a62754501379',
+             'metering_label_id': 'c5df2fe5-c600-4a2a-b2f4-c0fb6df73c83',
+             'remote_ip_prefix': '10.0.0.0/24'}]}],
+     'admin_state_up': True,
+     'gw_port_id': '6d411f48-ecc7-45e0-9ece-3b5bdb54fcee',
+     'id': '473ec392-1711-44e3-b008-3251ccfc5099',
+     'name': 'router1',
+     'status': 'ACTIVE',
+     'tenant_id': '6c5f5d2a1fa2441e88e35422926f48e8'},
+    {'_metering_labels': [
+        {'id': 'eeef45da-c600-4a2a-b2f4-c0fb6df73c83',
+         'rules': [{
+             'direction': 'egress',
+             'excluded': False,
+             'id': 'fa2441e8-2489-4ed1-870c-a62754501379',
+             'metering_label_id': 'eeef45da-c600-4a2a-b2f4-c0fb6df73c83',
+             'remote_ip_prefix': '20.0.0.0/24'}]}],
+     'admin_state_up': True,
+     'gw_port_id': '7d411f48-ecc7-45e0-9ece-3b5bdb54fcee',
+     'id': '373ec392-1711-44e3-b008-3251ccfc5099',
+     'name': 'router2',
+     'status': 'ACTIVE',
+     'tenant_id': '6c5f5d2a1fa2441e88e35422926f48e8'},
+]
+
+
 class IptablesDriverTestCase(base.BaseTestCase):
     def setUp(self):
         super(IptablesDriverTestCase, self).setUp()
@@ -52,16 +84,7 @@ class IptablesDriverTestCase(base.BaseTestCase):
                                                                cfg.CONF)
 
     def test_root_helper(self):
-        routers = [{'_metering_labels': [
-            {'id': 'c5df2fe5-c600-4a2a-b2f4-c0fb6df73c83',
-             'rules': []}],
-            'admin_state_up': True,
-            'gw_port_id': '7d411f48-ecc7-45e0-9ece-3b5bdb54fcee',
-            'id': '473ec392-1711-44e3-b008-3251ccfc5099',
-            'name': 'router1',
-            'status': 'ACTIVE',
-            'tenant_id': '6c5f5d2a1fa2441e88e35422926f48e8'}]
-        self.metering.add_metering_label(None, routers)
+        self.metering.add_metering_label(None, TEST_ROUTERS)
 
         self.iptables_cls.assert_called_with(root_helper='fake_sudo',
                                              namespace=mock.ANY,
@@ -69,15 +92,7 @@ class IptablesDriverTestCase(base.BaseTestCase):
                                              use_ipv6=mock.ANY)
 
     def test_add_metering_label(self):
-        routers = [{'_metering_labels': [
-            {'id': 'c5df2fe5-c600-4a2a-b2f4-c0fb6df73c83',
-             'rules': []}],
-            'admin_state_up': True,
-            'gw_port_id': '7d411f48-ecc7-45e0-9ece-3b5bdb54fcee',
-            'id': '473ec392-1711-44e3-b008-3251ccfc5099',
-            'name': 'router1',
-            'status': 'ACTIVE',
-            'tenant_id': '6c5f5d2a1fa2441e88e35422926f48e8'}]
+        routers = TEST_ROUTERS[:1]
 
         self.metering.add_metering_label(None, routers)
         calls = [mock.call.add_chain('neutron-meter-l-c5df2fe5-c60',
@@ -94,35 +109,7 @@ class IptablesDriverTestCase(base.BaseTestCase):
         self.v4filter_inst.assert_has_calls(calls)
 
     def test_process_metering_label_rules(self):
-        routers = [{'_metering_labels': [
-            {'id': 'c5df2fe5-c600-4a2a-b2f4-c0fb6df73c83',
-             'rules': [{
-                 'direction': 'ingress',
-                 'excluded': False,
-                 'id': '7f1a261f-2489-4ed1-870c-a62754501379',
-                 'metering_label_id': 'c5df2fe5-c600-4a2a-b2f4-c0fb6df73c83',
-                 'remote_ip_prefix': '10.0.0.0/24'}]}],
-            'admin_state_up': True,
-            'gw_port_id': '6d411f48-ecc7-45e0-9ece-3b5bdb54fcee',
-            'id': '473ec392-1711-44e3-b008-3251ccfc5099',
-            'name': 'router1',
-            'status': 'ACTIVE',
-            'tenant_id': '6c5f5d2a1fa2441e88e35422926f48e8'},
-            {'_metering_labels': [
-             {'id': 'eeef45da-c600-4a2a-b2f4-c0fb6df73c83',
-              'rules': [{
-                  'direction': 'egress',
-                  'excluded': False,
-                  'id': 'fa2441e8-2489-4ed1-870c-a62754501379',
-                  'metering_label_id': 'eeef45da-c600-4a2a-b2f4-c0fb6df73c83',
-                  'remote_ip_prefix': '20.0.0.0/24'}]}],
-             'admin_state_up': True,
-             'gw_port_id': '7d411f48-ecc7-45e0-9ece-3b5bdb54fcee',
-             'id': '373ec392-1711-44e3-b008-3251ccfc5099',
-             'name': 'router2',
-             'status': 'ACTIVE',
-             'tenant_id': '6c5f5d2a1fa2441e88e35422926f48e8'}]
-        self.metering.add_metering_label(None, routers)
+        self.metering.add_metering_label(None, TEST_ROUTERS)
 
         calls = [mock.call.add_chain('neutron-meter-l-c5df2fe5-c60',
                                      wrap=False),
@@ -156,34 +143,11 @@ class IptablesDriverTestCase(base.BaseTestCase):
         self.v4filter_inst.assert_has_calls(calls)
 
     def test_add_metering_label_with_rules(self):
-        routers = [{'_metering_labels': [
-            {'id': 'c5df2fe5-c600-4a2a-b2f4-c0fb6df73c83',
-             'rules': [{
-                 'direction': 'ingress',
-                 'excluded': False,
-                 'id': '7f1a261f-2489-4ed1-870c-a62754501379',
-                 'metering_label_id': 'c5df2fe5-c600-4a2a-b2f4-c0fb6df73c83',
-                 'remote_ip_prefix': '10.0.0.0/24'}]}],
-            'admin_state_up': True,
-            'gw_port_id': '6d411f48-ecc7-45e0-9ece-3b5bdb54fcee',
-            'id': '473ec392-1711-44e3-b008-3251ccfc5099',
-            'name': 'router1',
-            'status': 'ACTIVE',
-            'tenant_id': '6c5f5d2a1fa2441e88e35422926f48e8'},
-            {'_metering_labels': [
-             {'id': 'eeef45da-c600-4a2a-b2f4-c0fb6df73c83',
-              'rules': [{
-                  'direction': 'ingress',
-                  'excluded': True,
-                  'id': 'fa2441e8-2489-4ed1-870c-a62754501379',
-                  'metering_label_id': 'eeef45da-c600-4a2a-b2f4-c0fb6df73c83',
-                  'remote_ip_prefix': '20.0.0.0/24'}]}],
-             'admin_state_up': True,
-             'gw_port_id': '7d411f48-ecc7-45e0-9ece-3b5bdb54fcee',
-             'id': '373ec392-1711-44e3-b008-3251ccfc5099',
-             'name': 'router2',
-             'status': 'ACTIVE',
-             'tenant_id': '6c5f5d2a1fa2441e88e35422926f48e8'}]
+        routers = copy.deepcopy(TEST_ROUTERS)
+        routers[1]['_metering_labels'][0]['rules'][0].update({
+            'direction': 'ingress',
+            'excluded': True,
+        })
 
         self.metering.add_metering_label(None, routers)
         calls = [mock.call.add_chain('neutron-meter-l-c5df2fe5-c60',
@@ -218,20 +182,7 @@ class IptablesDriverTestCase(base.BaseTestCase):
         self.v4filter_inst.assert_has_calls(calls)
 
     def test_update_metering_label_rules(self):
-        routers = [{'_metering_labels': [
-            {'id': 'c5df2fe5-c600-4a2a-b2f4-c0fb6df73c83',
-             'rules': [{
-                 'direction': 'ingress',
-                 'excluded': False,
-                 'id': '7f1a261f-2489-4ed1-870c-a62754501379',
-                 'metering_label_id': 'c5df2fe5-c600-4a2a-b2f4-c0fb6df73c83',
-                 'remote_ip_prefix': '10.0.0.0/24'}]}],
-            'admin_state_up': True,
-            'gw_port_id': '6d411f48-ecc7-45e0-9ece-3b5bdb54fcee',
-            'id': '473ec392-1711-44e3-b008-3251ccfc5099',
-            'name': 'router1',
-            'status': 'ACTIVE',
-            'tenant_id': '6c5f5d2a1fa2441e88e35422926f48e8'}]
+        routers = TEST_ROUTERS[:1]
 
         self.metering.add_metering_label(None, routers)
 
@@ -278,44 +229,18 @@ class IptablesDriverTestCase(base.BaseTestCase):
         self.v4filter_inst.assert_has_calls(calls)
 
     def test_remove_metering_label_rule(self):
-        routers = [{'_metering_labels': [
-            {'id': 'c5df2fe5-c600-4a2a-b2f4-c0fb6df73c83',
-             'rules': [{
-                 'direction': 'ingress',
-                 'excluded': False,
-                 'id': '7f1a261f-2489-4ed1-870c-a62754501379',
-                 'metering_label_id': 'c5df2fe5-c600-4a2a-b2f4-c0fb6df73c83',
-                 'remote_ip_prefix': '10.0.0.0/24'},
-                 {'direction': 'ingress',
-                  'excluded': False,
-                  'id': 'aaaa261f-2489-4ed1-870c-a62754501379',
-                  'metering_label_id': 'c5df2fe5-c600-4a2a-b2f4-c0fb6df73c83',
-                  'remote_ip_prefix': '20.0.0.0/24'}]
-             }],
-            'admin_state_up': True,
-            'gw_port_id': '7d411f48-ecc7-45e0-9ece-3b5bdb54fcee',
-            'id': '473ec392-1711-44e3-b008-3251ccfc5099',
-            'name': 'router1',
-            'status': 'ACTIVE',
-            'tenant_id': '6c5f5d2a1fa2441e88e35422926f48e8'}]
+        routers = copy.deepcopy(TEST_ROUTERS[:1])
+        routers[0]['_metering_labels'][0]['rules'].append({
+            'direction': 'ingress',
+            'excluded': False,
+            'id': 'aaaa261f-2489-4ed1-870c-a62754501379',
+            'metering_label_id': 'c5df2fe5-c600-4a2a-b2f4-c0fb6df73c83',
+            'remote_ip_prefix': '20.0.0.0/24',
+        })
 
         self.metering.add_metering_label(None, routers)
 
-        routers = [{'_metering_labels': [
-            {'id': 'c5df2fe5-c600-4a2a-b2f4-c0fb6df73c83',
-             'rules': [{
-                 'direction': 'ingress',
-                 'excluded': False,
-                 'id': '7f1a261f-2489-4ed1-870c-a62754501379',
-                 'metering_label_id': 'c5df2fe5-c600-4a2a-b2f4-c0fb6df73c83',
-                 'remote_ip_prefix': '10.0.0.0/24'}]
-             }],
-            'admin_state_up': True,
-            'gw_port_id': '7d411f48-ecc7-45e0-9ece-3b5bdb54fcee',
-            'id': '473ec392-1711-44e3-b008-3251ccfc5099',
-            'name': 'router1',
-            'status': 'ACTIVE',
-            'tenant_id': '6c5f5d2a1fa2441e88e35422926f48e8'}]
+        del routers[0]['_metering_labels'][0]['rules'][1]
 
         self.metering.update_metering_label_rules(None, routers)
         calls = [mock.call.add_chain('neutron-meter-l-c5df2fe5-c60',
@@ -329,38 +254,24 @@ class IptablesDriverTestCase(base.BaseTestCase):
                                     '',
                                     wrap=False),
                  mock.call.add_rule('neutron-meter-r-c5df2fe5-c60',
-                                    '-i qg-7d411f48-ec -d 10.0.0.0/24'
+                                    '-i qg-6d411f48-ec -d 10.0.0.0/24'
                                     ' -j neutron-meter-l-c5df2fe5-c60',
                                     wrap=False, top=False),
                  mock.call.add_rule('neutron-meter-r-c5df2fe5-c60',
-                                    '-i qg-7d411f48-ec -d 20.0.0.0/24'
+                                    '-i qg-6d411f48-ec -d 20.0.0.0/24'
                                     ' -j neutron-meter-l-c5df2fe5-c60',
                                     wrap=False, top=False),
                  mock.call.empty_chain('neutron-meter-r-c5df2fe5-c60',
                                        wrap=False),
                  mock.call.add_rule('neutron-meter-r-c5df2fe5-c60',
-                                    '-i qg-7d411f48-ec -d 10.0.0.0/24'
+                                    '-i qg-6d411f48-ec -d 10.0.0.0/24'
                                     ' -j neutron-meter-l-c5df2fe5-c60',
                                     wrap=False, top=False)]
 
         self.v4filter_inst.assert_has_calls(calls)
 
     def test_remove_metering_label(self):
-        routers = [{'_metering_labels': [
-            {'id': 'c5df2fe5-c600-4a2a-b2f4-c0fb6df73c83',
-             'rules': [{
-                 'direction': 'ingress',
-                 'excluded': False,
-                 'id': '7f1a261f-2489-4ed1-870c-a62754501379',
-                 'metering_label_id': 'c5df2fe5-c600-4a2a-b2f4-c0fb6df73c83',
-                 'remote_ip_prefix': '10.0.0.0/24'}]
-             }],
-            'admin_state_up': True,
-            'gw_port_id': '7d411f48-ecc7-45e0-9ece-3b5bdb54fcee',
-            'id': '473ec392-1711-44e3-b008-3251ccfc5099',
-            'name': 'router1',
-            'status': 'ACTIVE',
-            'tenant_id': '6c5f5d2a1fa2441e88e35422926f48e8'}]
+        routers = TEST_ROUTERS[:1]
 
         self.metering.add_metering_label(None, routers)
         self.metering.remove_metering_label(None, routers)
@@ -375,7 +286,7 @@ class IptablesDriverTestCase(base.BaseTestCase):
                                     '',
                                     wrap=False),
                  mock.call.add_rule('neutron-meter-r-c5df2fe5-c60',
-                                    '-i qg-7d411f48-ec -d 10.0.0.0/24'
+                                    '-i qg-6d411f48-ec -d 10.0.0.0/24'
                                     ' -j neutron-meter-l-c5df2fe5-c60',
                                     wrap=False, top=False),
                  mock.call.remove_chain('neutron-meter-l-c5df2fe5-c60',
@@ -386,34 +297,11 @@ class IptablesDriverTestCase(base.BaseTestCase):
         self.v4filter_inst.assert_has_calls(calls)
 
     def test_update_routers(self):
-        routers = [{'_metering_labels': [
-            {'id': 'c5df2fe5-c600-4a2a-b2f4-c0fb6df73c83',
-             'rules': [{
-                 'direction': 'ingress',
-                 'excluded': False,
-                 'id': '7f1a261f-2489-4ed1-870c-a62754501379',
-                 'metering_label_id': 'c5df2fe5-c600-4a2a-b2f4-c0fb6df73c83',
-                 'remote_ip_prefix': '10.0.0.0/24'}]}],
-            'admin_state_up': True,
-            'gw_port_id': '6d411f48-ecc7-45e0-9ece-3b5bdb54fcee',
-            'id': '473ec392-1711-44e3-b008-3251ccfc5099',
-            'name': 'router1',
-            'status': 'ACTIVE',
-            'tenant_id': '6c5f5d2a1fa2441e88e35422926f48e8'},
-            {'_metering_labels': [
-             {'id': 'eeef45da-c600-4a2a-b2f4-c0fb6df73c83',
-              'rules': [{
-                  'direction': 'ingress',
-                  'excluded': True,
-                  'id': 'fa2441e8-2489-4ed1-870c-a62754501379',
-                  'metering_label_id': 'eeef45da-c600-4a2a-b2f4-c0fb6df73c83',
-                  'remote_ip_prefix': '20.0.0.0/24'}]}],
-             'admin_state_up': True,
-             'gw_port_id': '7d411f48-ecc7-45e0-9ece-3b5bdb54fcee',
-             'id': '373ec392-1711-44e3-b008-3251ccfc5099',
-             'name': 'router2',
-             'status': 'ACTIVE',
-             'tenant_id': '6c5f5d2a1fa2441e88e35422926f48e8'}]
+        routers = copy.deepcopy(TEST_ROUTERS)
+        routers[1]['_metering_labels'][0]['rules'][0].update({
+            'direction': 'ingress',
+            'excluded': True,
+        })
 
         self.metering.add_metering_label(None, routers)
 
@@ -469,3 +357,19 @@ class IptablesDriverTestCase(base.BaseTestCase):
                                     wrap=False, top=False)]
 
         self.v4filter_inst.assert_has_calls(calls)
+
+    def test_update_routers_removal(self):
+        routers = TEST_ROUTERS
+
+        self.metering.add_metering_label(None, routers)
+
+        # Remove router id '373ec392-1711-44e3-b008-3251ccfc5099'
+        updates = TEST_ROUTERS[:1]
+
+        self.metering.update_routers(None, updates)
+        calls = [mock.call.remove_chain('neutron-meter-l-eeef45da-c60',
+                                        wrap=False),
+                 mock.call.remove_chain('neutron-meter-r-eeef45da-c60',
+                                        wrap=False)]
+
+        self.v4filter_inst.assert_has_calls(calls)