From 910744781c46e6c200298922cc9c251725ce19fc Mon Sep 17 00:00:00 2001 From: John Schwarz Date: Wed, 23 Mar 2016 14:05:37 +0200 Subject: [PATCH] Fix reference to uninitialized iptables manager DvrEdgeRouter.process_address_scope() currently assumes that snat_iptables_manager was initialized, however this is only done when an external gateway is added. In case a new DVR+HA router was created without an external gateway, the l3 agent will raise an exception and will not create the router correctly. This patch adds a simple check to make sure that it is defined before it's actually used. Closes-Bug: #1560945 (cherry picked from commit a8b60671150ac383c6ed24c26e773a97a476f7d2) Change-Id: I677e0837956a6d008a3935d961f078987a07d0c4 --- neutron/agent/l3/dvr_edge_router.py | 8 ++- neutron/tests/common/l3_test_common.py | 16 +++-- .../functional/agent/l3/test_dvr_router.py | 71 ++++++++++++------- neutron/tests/unit/agent/l3/test_agent.py | 25 +++++++ 4 files changed, 85 insertions(+), 35 deletions(-) diff --git a/neutron/agent/l3/dvr_edge_router.py b/neutron/agent/l3/dvr_edge_router.py index 48c75213677..0719b9fabfd 100644 --- a/neutron/agent/l3/dvr_edge_router.py +++ b/neutron/agent/l3/dvr_edge_router.py @@ -216,8 +216,10 @@ class DvrEdgeRouter(dvr_local_router.DvrLocalRouter): if not self._is_this_snat_host(): return + if not self.snat_iptables_manager: + LOG.debug("DVR router: no snat rules to be handled") + return - snat_iptables_manager = self.snat_iptables_manager # Prepare address scope iptables rule for dvr snat interfaces internal_ports = self.get_snat_interfaces() ports_scopemark = self._get_port_devicename_scopemark( @@ -232,6 +234,6 @@ class DvrEdgeRouter(dvr_local_router.DvrLocalRouter): ports_scopemark[ip_version].update( external_port_scopemark[ip_version]) - with snat_iptables_manager.defer_apply(): + with self.snat_iptables_manager.defer_apply(): self._add_address_scope_mark( - snat_iptables_manager, ports_scopemark) + self.snat_iptables_manager, ports_scopemark) diff --git a/neutron/tests/common/l3_test_common.py b/neutron/tests/common/l3_test_common.py index 257c7f12c88..f0509625f78 100644 --- a/neutron/tests/common/l3_test_common.py +++ b/neutron/tests/common/l3_test_common.py @@ -53,7 +53,7 @@ def get_ha_interface(ip='169.254.192.1', mac='12:34:56:78:2b:5d'): def prepare_router_data(ip_version=4, enable_snat=None, num_internal_ports=1, enable_floating_ip=False, enable_ha=False, extra_routes=False, dual_stack=False, - v6_ext_gw_with_sub=True, **kwargs): + enable_gw=True, v6_ext_gw_with_sub=True, **kwargs): fixed_ips = [] subnets = [] gateway_mac = kwargs.get('gateway_mac', 'ca:fe:de:ad:be:ee') @@ -86,12 +86,14 @@ def prepare_router_data(ip_version=4, enable_snat=None, num_internal_ports=1, raise ValueError("Invalid ip_version: %s" % ip_version) router_id = _uuid() - ex_gw_port = {'id': _uuid(), - 'mac_address': gateway_mac, - 'network_id': _uuid(), - 'fixed_ips': fixed_ips, - 'subnets': subnets, - 'extra_subnets': extra_subnets} + ex_gw_port = {} + if enable_gw: + ex_gw_port = {'id': _uuid(), + 'mac_address': gateway_mac, + 'network_id': _uuid(), + 'fixed_ips': fixed_ips, + 'subnets': subnets, + 'extra_subnets': extra_subnets} routes = [] if extra_routes: diff --git a/neutron/tests/functional/agent/l3/test_dvr_router.py b/neutron/tests/functional/agent/l3/test_dvr_router.py index 2e9eef9f793..0d2ac27f9ef 100644 --- a/neutron/tests/functional/agent/l3/test_dvr_router.py +++ b/neutron/tests/functional/agent/l3/test_dvr_router.py @@ -42,9 +42,10 @@ class TestDvrRouter(framework.L3AgentTestFramework): agent.fipnamespace_delete_on_ext_net(None, ext_net_id) except RuntimeError: pass - self.addCleanup( - _safe_fipnamespace_delete_on_ext_net, - router['gw_port']['network_id']) + if router['gw_port']: + self.addCleanup( + _safe_fipnamespace_delete_on_ext_net, + router['gw_port']['network_id']) return super(TestDvrRouter, self).manage_router(agent, router) @@ -207,6 +208,7 @@ class TestDvrRouter(framework.L3AgentTestFramework): def generate_dvr_router_info(self, enable_ha=False, enable_snat=False, + enable_gw=True, agent=None, extra_routes=False, **kwargs): @@ -218,24 +220,28 @@ class TestDvrRouter(framework.L3AgentTestFramework): enable_ha=enable_ha, extra_routes=extra_routes, num_internal_ports=2, + enable_gw=enable_gw, **kwargs) internal_ports = router.get(l3_constants.INTERFACE_KEY, []) router['distributed'] = True router['gw_port_host'] = agent.conf.host - router['gw_port'][portbindings.HOST_ID] = agent.conf.host - floating_ip = router['_floatingips'][0] - floating_ip['floating_network_id'] = router['gw_port']['network_id'] - floating_ip['host'] = agent.conf.host - floating_ip['port_id'] = internal_ports[0]['id'] - floating_ip['status'] = 'ACTIVE' - self._add_snat_port_info_to_router(router, internal_ports) - # FIP has a dependency on external gateway. So we need to create - # the snat_port info and fip_agent_gw_port_info irrespective of - # the agent type the dvr supports. The namespace creation is - # dependent on the agent_type. - external_gw_port = router['gw_port'] - self._add_fip_agent_gw_port_info_to_router(router, external_gw_port) + floating_ip = router['_floatingips'][0] + floating_ip['host'] = agent.conf.host + if enable_gw: + external_gw_port = router['gw_port'] + router['gw_port'][portbindings.HOST_ID] = agent.conf.host + floating_ip['floating_network_id'] = external_gw_port['network_id'] + floating_ip['port_id'] = internal_ports[0]['id'] + floating_ip['status'] = 'ACTIVE' + + self._add_snat_port_info_to_router(router, internal_ports) + # FIP has a dependency on external gateway. So we need to create + # the snat_port info and fip_agent_gw_port_info irrespective of + # the agent type the dvr supports. The namespace creation is + # dependent on the agent_type. + self._add_fip_agent_gw_port_info_to_router(router, + external_gw_port) return router def _add_fip_agent_gw_port_info_to_router(self, router, external_gw_port): @@ -599,10 +605,11 @@ class TestDvrRouter(framework.L3AgentTestFramework): self.assertFalse(sg_device) self.assertTrue(qg_device) - def _mocked_dvr_ha_router(self, agent): + def _mocked_dvr_ha_router(self, agent, enable_gw=True): r_info = self.generate_dvr_router_info(enable_ha=True, enable_snat=True, - agent=agent) + agent=agent, + enable_gw=enable_gw) r_snat_ns_name = namespaces.build_ns_name(dvr_snat_ns.SNAT_NS_PREFIX, r_info['id']) @@ -631,14 +638,14 @@ class TestDvrRouter(framework.L3AgentTestFramework): br_int_1.add_port(veth1.name) br_int_2.add_port(veth2.name) - def _create_dvr_ha_router(self, agent): + def _create_dvr_ha_router(self, agent, enable_gw=True): get_ns_name = mock.patch.object(namespaces.RouterNamespace, '_get_ns_name').start() get_snat_ns_name = mock.patch.object(dvr_snat_ns.SnatNamespace, 'get_snat_ns_name').start() (r_info, mocked_r_ns_name, - mocked_r_snat_ns_name) = self._mocked_dvr_ha_router(agent) + mocked_r_snat_ns_name) = self._mocked_dvr_ha_router(agent, enable_gw) get_ns_name.return_value = mocked_r_ns_name get_snat_ns_name.return_value = mocked_r_snat_ns_name router = self.manage_router(agent, r_info) @@ -647,7 +654,11 @@ class TestDvrRouter(framework.L3AgentTestFramework): def _assert_ip_addresses_in_dvr_ha_snat_namespace(self, router): namespace = router.ha_namespace ex_gw_port = router.get_ex_gw_port() - snat_port = router.get_snat_interfaces()[0] + snat_ports = router.get_snat_interfaces() + if not snat_ports: + return + + snat_port = snat_ports[0] ex_gw_port_name = router.get_external_device_name( ex_gw_port['id']) snat_port_name = router._get_snat_int_device_name( @@ -670,7 +681,11 @@ class TestDvrRouter(framework.L3AgentTestFramework): def _assert_no_ip_addresses_in_dvr_ha_snat_namespace(self, router): namespace = router.ha_namespace ex_gw_port = router.get_ex_gw_port() - snat_port = router.get_snat_interfaces()[0] + snat_ports = router.get_snat_interfaces() + if not snat_ports: + return + + snat_port = snat_ports[0] ex_gw_port_name = router.get_external_device_name( ex_gw_port['id']) snat_port_name = router._get_snat_int_device_name( @@ -681,12 +696,12 @@ class TestDvrRouter(framework.L3AgentTestFramework): self._assert_no_ip_addresses_on_interface(namespace, ex_gw_port_name) - def test_dvr_ha_router_failover(self): + def _test_dvr_ha_router_failover(self, enable_gw): self._setup_dvr_ha_agents() self._setup_dvr_ha_bridges() - router1 = self._create_dvr_ha_router(self.agent) - router2 = self._create_dvr_ha_router(self.failover_agent) + router1 = self._create_dvr_ha_router(self.agent, enable_gw=enable_gw) + router2 = self._create_dvr_ha_router(self.failover_agent, enable_gw) utils.wait_until_true(lambda: router1.ha_state == 'master') utils.wait_until_true(lambda: router2.ha_state == 'backup') @@ -702,6 +717,12 @@ class TestDvrRouter(framework.L3AgentTestFramework): self._assert_ip_addresses_in_dvr_ha_snat_namespace(router2) self._assert_no_ip_addresses_in_dvr_ha_snat_namespace(router1) + def test_dvr_ha_router_failover_with_gw(self): + self._test_dvr_ha_router_failover(enable_gw=True) + + def test_dvr_ha_router_failover_without_gw(self): + self._test_dvr_ha_router_failover(enable_gw=False) + def test_dvr_router_static_routes(self): """Test to validate the extra routes on dvr routers.""" self.agent.conf.agent_mode = 'dvr_snat' diff --git a/neutron/tests/unit/agent/l3/test_agent.py b/neutron/tests/unit/agent/l3/test_agent.py index 3021d17f4dc..1a1bbc77a1b 100644 --- a/neutron/tests/unit/agent/l3/test_agent.py +++ b/neutron/tests/unit/agent/l3/test_agent.py @@ -41,6 +41,7 @@ from neutron.agent.l3 import router_processing_queue from neutron.agent.linux import dibbler from neutron.agent.linux import external_process from neutron.agent.linux import interface +from neutron.agent.linux import iptables_manager from neutron.agent.linux import pd from neutron.agent.linux import ra from neutron.agent.metadata import driver as metadata_driver @@ -2203,6 +2204,30 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): self.assertEqual(3, self.mock_driver.plug.call_count) self.assertEqual(3, self.mock_driver.init_router_port.call_count) + def test_process_address_scope(self): + router = l3_test_common.prepare_router_data() + router['distributed'] = True + router['gw_port_host'] = HOSTNAME + + agent = l3_agent.L3NATAgent(HOSTNAME, self.conf) + ri = dvr_router.DvrEdgeRouter(agent, + HOSTNAME, + router['id'], + router, + **self.ri_kwargs) + ri.get_ex_gw_port = mock.Mock(return_value=None) + + # Make sure the code doesn't crash if ri.snat_iptables_manager is None. + ri.process_address_scope() + + with mock.patch.object(ri, '_add_address_scope_mark') as mocked_func: + ri.snat_iptables_manager = iptables_manager.IptablesManager( + namespace=mock.ANY, use_ipv6=False) + ri.snat_iptables_manager.defer_apply_off = mock.Mock() + + ri.process_address_scope() + self.assertEqual(2, mocked_func.call_count) + def test_get_service_plugin_list(self): service_plugins = [p_const.L3_ROUTER_NAT] self.plugin_api.get_service_plugin_list.return_value = service_plugins