From c1ae9a5145eaaba8d6a0ab1272dc4ddd61043ac4 Mon Sep 17 00:00:00 2001 From: Adit Sarfaty Date: Thu, 14 Feb 2019 15:19:30 +0200 Subject: [PATCH] NSX|V3: FWaaS v2 without GW When adding FWaaS v2 policy to a router interface, but the router has no GW (and no service router), the rules should not be created on the backend. Only when adding a GW, will all the rules be applied. Since the FWaaS if for N-S traffic only, it shouldn't be applied without a GW anyway. This change required a little change in the service-rotuer creation logic for NSX-V3 & NSX-P. Since the logic got too complicated (FW rule can exist withour SR), the new code will also check the current status on the backend. Change-Id: I2a5d69e9443e8a468ce0d934ff1c846dc837bc89 --- vmware_nsx/plugins/common_v3/plugin.py | 42 ++++++++++++------- vmware_nsx/plugins/nsx_p/plugin.py | 23 +++++++--- vmware_nsx/plugins/nsx_v3/plugin.py | 40 ++++++++++++++---- .../fwaas/nsx_v3/edge_fwaas_driver_base.py | 8 ++-- .../fwaas/nsx_v3/fwaas_callbacks_v2.py | 4 +- vmware_nsx/tests/unit/nsx_p/test_plugin.py | 3 ++ .../tests/unit/nsx_v3/test_fwaas_v2_driver.py | 2 +- vmware_nsx/tests/unit/nsx_v3/test_plugin.py | 12 ++++-- .../unit/services/vpnaas/test_nsxv3_vpnaas.py | 12 +++--- 9 files changed, 103 insertions(+), 43 deletions(-) diff --git a/vmware_nsx/plugins/common_v3/plugin.py b/vmware_nsx/plugins/common_v3/plugin.py index b6a5132385..1ad58fe941 100644 --- a/vmware_nsx/plugins/common_v3/plugin.py +++ b/vmware_nsx/plugins/common_v3/plugin.py @@ -1067,7 +1067,8 @@ class NsxPluginV3Base(agentschedulers_db.AZDhcpAgentSchedulerDbMixin, def _get_update_router_gw_actions( self, org_tier0_uuid, orgaddr, org_enable_snat, - new_tier0_uuid, newaddr, new_enable_snat, lb_exist, fw_exist): + new_tier0_uuid, newaddr, new_enable_snat, + lb_exist, fw_exist, sr_currently_exists): """Return a dictionary of flags indicating which actions should be performed on this router GW update. """ @@ -1126,21 +1127,34 @@ class NsxPluginV3Base(agentschedulers_db.AZDhcpAgentSchedulerDbMixin, actions['advertise_route_connected_flag'] = ( True if not new_enable_snat else False) - # the purpose of the two vars is to be able to differ between + # the purpose of this var is to be able to differ between # adding a gateway w/o snat and adding snat (when adding/removing gw - # the snat option is on by default. + # the snat option is on by default). + new_with_snat = True if (new_enable_snat and newaddr) else False + has_gw = True if newaddr else False - real_new_enable_snat = new_enable_snat and newaddr - real_org_enable_snat = org_enable_snat and orgaddr - - actions['add_service_router'] = ((real_new_enable_snat and - not real_org_enable_snat) or - (real_new_enable_snat and not - orgaddr and newaddr) - ) and not (fw_exist or lb_exist) - actions['remove_service_router'] = ((not real_new_enable_snat and - real_org_enable_snat) or ( - orgaddr and not newaddr)) and not (fw_exist or lb_exist) + if sr_currently_exists: + # currently there is a service router on the backend + actions['add_service_router'] = False + # Should remove the service router if the GW was removed, + # or no service needs it: SNAT, LBaaS or FWaaS + actions['remove_service_router'] = ( + not has_gw or not (fw_exist or lb_exist or new_with_snat)) + if actions['remove_service_router']: + LOG.info("Removing service router [has GW: %s, FW %s, LB %s, " + "SNAT %s]", + has_gw, fw_exist, lb_exist, new_with_snat) + else: + # currently there is no service router on the backend + actions['remove_service_router'] = False + # Should add service router if there is a GW + # and there is a service that needs it: SNAT, LB or FWaaS + actions['add_service_router'] = ( + has_gw is not None and (new_with_snat or fw_exist or lb_exist)) + if actions['add_service_router']: + LOG.info("Adding service router [has GW: %s, FW %s, LB %s, " + "SNAT %s]", + has_gw, fw_exist, lb_exist, new_with_snat) return actions diff --git a/vmware_nsx/plugins/nsx_p/plugin.py b/vmware_nsx/plugins/nsx_p/plugin.py index 17567c03ee..6362b1acf4 100644 --- a/vmware_nsx/plugins/nsx_p/plugin.py +++ b/vmware_nsx/plugins/nsx_p/plugin.py @@ -1134,10 +1134,20 @@ class NsxPolicyPlugin(nsx_plugin_common.NsxPluginV3Base): return self.nsxpolicy.tier0.get_edge_cluster_path( tier0_uuid) - def create_service_router(self, context, router_id): + def verify_sr_at_backend(self, router_id): + """Check if the backend Tier1 has a service router or not""" + if self.nsxpolicy.tier1.get_edge_cluster_path(router_id): + return True + + def create_service_router(self, context, router_id, router=None): """Create a service router and enable standby relocation""" - router = self._get_router(context, router_id) + if not router: + router = self._get_router(context, router_id) tier0_uuid = self._get_tier0_uuid_by_router(context, router) + if not tier0_uuid: + err_msg = (_("Cannot create service router for %s without a " + "gateway") % router_id) + raise n_exc.InvalidInput(error_message=err_msg) edge_cluster_path = self._get_edge_cluster_path( tier0_uuid, router) if edge_cluster_path: @@ -1198,13 +1208,16 @@ class NsxPolicyPlugin(nsx_plugin_common.NsxPluginV3Base): router['id']) router_subnets = self._find_router_subnets( context.elevated(), router_id) + sr_currently_exists = self.verify_sr_at_backend(router_id) + lb_exist = False + fw_exist = False actions = self._get_update_router_gw_actions( org_tier0_uuid, orgaddr, org_enable_snat, - new_tier0_uuid, newaddr, new_enable_snat, fw_exist=False, - lb_exist=False) + new_tier0_uuid, newaddr, new_enable_snat, + lb_exist, fw_exist, sr_currently_exists) if actions['add_service_router']: - self.create_service_router(context, router_id) + self.create_service_router(context, router_id, router=router) if actions['remove_snat_rules']: for subnet in router_subnets: diff --git a/vmware_nsx/plugins/nsx_v3/plugin.py b/vmware_nsx/plugins/nsx_v3/plugin.py index 2dcb5fe225..fb552b1b7d 100644 --- a/vmware_nsx/plugins/nsx_v3/plugin.py +++ b/vmware_nsx/plugins/nsx_v3/plugin.py @@ -2101,9 +2101,13 @@ class NsxV3Plugin(nsx_plugin_common.NsxPluginV3Base, 'net': sub['network_id']}) raise n_exc.InvalidInput(error_message=msg) - def state_firewall_rules(self, context, router_id): + def _router_has_edge_fw_rules(self, context, router): + if not router.gw_port_id: + # No GW -> No rule on the edge firewall + return False + if self.fwaas_callbacks and self.fwaas_callbacks.fwaas_enabled: - ports = self._get_router_interfaces(context, router_id) + ports = self._get_router_interfaces(context, router.id) return self.fwaas_callbacks.router_with_fwg(context, ports) def verify_sr_at_backend(self, context, router_id): @@ -2118,15 +2122,22 @@ class NsxV3Plugin(nsx_plugin_common.NsxPluginV3Base, snat_exist = router.enable_snat lb_exist = nsx_db.has_nsx_lbaas_loadbalancer_binding_by_router( context.session, nsx_router_id) - fw_exist = self.state_firewall_rules(context, router_id) + fw_exist = self._router_has_edge_fw_rules(context, router) if snat_exist or lb_exist or fw_exist: return True return snat_exist or lb_exist or fw_exist - def create_service_router(self, context, router_id): + def create_service_router(self, context, router_id, router=None, + update_firewall=True): """Create a service router and enable standby relocation""" - router = self._get_router(context, router_id) + if not router: + router = self._get_router(context, router_id) tier0_uuid = self._get_tier0_uuid_by_router(context, router) + if not tier0_uuid: + err_msg = (_("Cannot create service router for %s without a " + "gateway") % router_id) + raise n_exc.InvalidInput(error_message=err_msg) + edge_cluster_uuid = self._get_edge_cluster(tier0_uuid, router) nsx_router_id = nsx_db.get_nsx_router_id(context.session, router_id) @@ -2140,6 +2151,14 @@ class NsxV3Plugin(nsx_plugin_common.NsxPluginV3Base, edge_cluster_id=edge_cluster_uuid, enable_standby_relocation=enable_standby_relocation) + LOG.info("Created service router for %s (NSX logical router %s)", + router_id, nsx_router_id) + + # update firewall rules (there might be FW group waiting for a + # service router) + if update_firewall: + self.update_router_firewall(context, router_id) + def delete_service_router(self, context, router_id): nsx_router_id = nsx_db.get_nsx_router_id(context.session, router_id) @@ -2149,6 +2168,8 @@ class NsxV3Plugin(nsx_plugin_common.NsxPluginV3Base, nsx_router_id, edge_cluster_id=None, enable_standby_relocation=False) + LOG.info("Deleted service router for %s (NSX logical router %s)", + router_id, nsx_router_id) def _update_router_gw_info(self, context, router_id, info): router = self._get_router(context, router_id) @@ -2178,17 +2199,18 @@ class NsxV3Plugin(nsx_plugin_common.NsxPluginV3Base, lb_exist = nsx_db.has_nsx_lbaas_loadbalancer_binding_by_router( context.session, nsx_router_id) - fw_exist = self.state_firewall_rules(context, router_id) + fw_exist = self._router_has_edge_fw_rules(context, router) + sr_currently_exists = self.verify_sr_at_backend(context, router_id) actions = self._get_update_router_gw_actions( org_tier0_uuid, orgaddr, org_enable_snat, - new_tier0_uuid, newaddr, new_enable_snat, lb_exist, fw_exist) + new_tier0_uuid, newaddr, new_enable_snat, + lb_exist, fw_exist, sr_currently_exists) if actions['add_service_router']: - self.create_service_router(context, router_id) + self.create_service_router(context, router_id, router=router) if actions['revocate_bgp_announce']: - # TODO(berlin): revocate bgp announce on org tier0 router pass if actions['remove_snat_rules']: diff --git a/vmware_nsx/services/fwaas/nsx_v3/edge_fwaas_driver_base.py b/vmware_nsx/services/fwaas/nsx_v3/edge_fwaas_driver_base.py index bd48421cad..7d8fea07c7 100644 --- a/vmware_nsx/services/fwaas/nsx_v3/edge_fwaas_driver_base.py +++ b/vmware_nsx/services/fwaas/nsx_v3/edge_fwaas_driver_base.py @@ -83,10 +83,12 @@ class CommonEdgeFwaasV3Driver(fwaas_driver_base.EdgeFwaasDriverBaseV2): self.backend_support = False def should_apply_firewall_to_router(self, router_data): - """Return True if the firewall rules should be added the router + """Return True if the firewall rules should be added the router""" + if not router_data.get('external_gateway_info'): + LOG.info("Cannot apply firewall to router %s with no gateway", + router_data['id']) + return False - Right now the driver supports for all routers. - """ return True def _translate_action(self, fwaas_action, fwaas_rule_id): diff --git a/vmware_nsx/services/fwaas/nsx_v3/fwaas_callbacks_v2.py b/vmware_nsx/services/fwaas/nsx_v3/fwaas_callbacks_v2.py index 566eab38b6..3508303643 100644 --- a/vmware_nsx/services/fwaas/nsx_v3/fwaas_callbacks_v2.py +++ b/vmware_nsx/services/fwaas/nsx_v3/fwaas_callbacks_v2.py @@ -118,8 +118,8 @@ class Nsxv3FwaasCallbacksV2(com_callbacks.NsxFwaasCallbacksV2): if with_fw: # firewall exists in Neutron and not on backend - create if not exists_on_backend: - self.core_plugin.create_service_router(context, router_id) - exists_on_backend = True + self.core_plugin.create_service_router( + context, router_id, update_firewall=False) else: # First, check if other services exist and use the sr sr_exists = self.core_plugin.service_router_has_services( diff --git a/vmware_nsx/tests/unit/nsx_p/test_plugin.py b/vmware_nsx/tests/unit/nsx_p/test_plugin.py index 8b52b0b14d..6163768f3d 100644 --- a/vmware_nsx/tests/unit/nsx_p/test_plugin.py +++ b/vmware_nsx/tests/unit/nsx_p/test_plugin.py @@ -1810,6 +1810,9 @@ class NsxPTestL3NatTestCase(NsxPTestL3NatTest, self._create_l3_ext_network() as ext_net,\ self.subnet(network=ext_net, cidr='10.0.1.0/24', enable_dhcp=False) as s,\ + mock.patch("vmware_nsxlib.v3.policy.core_resources." + "NsxPolicyTier1Api.get_edge_cluster_path", + return_value=False),\ mock.patch("vmware_nsxlib.v3.policy.core_resources." "NsxPolicyTier1Api.set_edge_cluster_path" ) as add_srv_router: diff --git a/vmware_nsx/tests/unit/nsx_v3/test_fwaas_v2_driver.py b/vmware_nsx/tests/unit/nsx_v3/test_fwaas_v2_driver.py index 117f075ce0..27c688f3af 100644 --- a/vmware_nsx/tests/unit/nsx_v3/test_fwaas_v2_driver.py +++ b/vmware_nsx/tests/unit/nsx_v3/test_fwaas_v2_driver.py @@ -204,7 +204,7 @@ class Nsxv3FwaasTestCase(test_v3_plugin.NsxV3PluginTestCaseMixin): rule_list, is_ingress=is_ingress, admin_state_up=False) def _fake_apply_list(self): - router_inst = {'id': FAKE_ROUTER_ID} + router_inst = {'id': FAKE_ROUTER_ID, 'external_gateway_info': 'dummy'} router_info_inst = mock.Mock() router_info_inst.router = router_inst router_info_inst.router_id = FAKE_ROUTER_ID diff --git a/vmware_nsx/tests/unit/nsx_v3/test_plugin.py b/vmware_nsx/tests/unit/nsx_v3/test_plugin.py index 59fe34158e..7d00072054 100644 --- a/vmware_nsx/tests/unit/nsx_v3/test_plugin.py +++ b/vmware_nsx/tests/unit/nsx_v3/test_plugin.py @@ -2663,7 +2663,9 @@ class TestL3NatTestCase(L3NatTest, # create a router with this gateway with self.router() as r, \ - self._mock_add_remove_service_router() as change_sr: + mock.patch("vmware_nsxlib.v3.router.RouterLib." + "has_service_router", return_value=False),\ + self._mock_add_remove_service_router() as change_sr: router_id = r['router']['id'] self._add_external_gateway_to_router( router_id, ext_subnet['network_id']) @@ -2684,7 +2686,9 @@ class TestL3NatTestCase(L3NatTest, self._add_external_gateway_to_router( r['router']['id'], ext_subnet['network_id']) - with self._mock_add_remove_service_router() as change_sr: + with mock.patch("vmware_nsxlib.v3.router.RouterLib." + "has_service_router", return_value=True),\ + self._mock_add_remove_service_router() as change_sr: self._update_router_enable_snat( r['router']['id'], ext_subnet['network_id'], @@ -3025,7 +3029,9 @@ class TestL3NatTestCase(L3NatTest, # create a router with this gateway with self.router() as r, \ - self._mock_add_remove_service_router() as change_sr: + mock.patch("vmware_nsxlib.v3.router.RouterLib." + "has_service_router", return_value=False),\ + self._mock_add_remove_service_router() as change_sr: router_id = r['router']['id'] self._add_external_gateway_to_router( router_id, ext_subnet['network_id']) diff --git a/vmware_nsx/tests/unit/services/vpnaas/test_nsxv3_vpnaas.py b/vmware_nsx/tests/unit/services/vpnaas/test_nsxv3_vpnaas.py index ebc7bc7211..d4436ea612 100644 --- a/vmware_nsx/tests/unit/services/vpnaas/test_nsxv3_vpnaas.py +++ b/vmware_nsx/tests/unit/services/vpnaas/test_nsxv3_vpnaas.py @@ -604,10 +604,13 @@ class TestVpnaasDriver(test_plugin.NsxV3PluginTestCaseMixin): providernet_args = {extnet_apidef.EXTERNAL: True} router_db = namedtuple("Router", FAKE_ROUTER.keys())( *FAKE_ROUTER.values()) + tier0_uuid = 'tier-0' with self.network(name='ext-net', providernet_args=providernet_args, arg_list=(extnet_apidef.EXTERNAL, )) as ext_net,\ self.subnet(ext_net),\ + mock.patch.object(self.plugin, '_get_tier0_uuid_by_router', + return_value=tier0_uuid),\ self.router(external_gateway_info={'network_id': ext_net['network']['id']}) as router,\ self.subnet() as sub: @@ -620,7 +623,6 @@ class TestVpnaasDriver(test_plugin.NsxV3PluginTestCaseMixin): dummy_port = {'id': 'dummy_port', 'fixed_ips': [{'ip_address': '1.1.1.1'}]} tier0_rtr = {'high_availability_mode': 'ACTIVE_STANDBY'} - tier0_uuid = 'tier-0' with mock.patch.object(self.service_plugin, '_get_vpnservice', return_value=FAKE_VPNSERVICE),\ mock.patch.object(self.nsxlib_vpn.service, @@ -631,8 +633,6 @@ class TestVpnaasDriver(test_plugin.NsxV3PluginTestCaseMixin): return_value=FAKE_ROUTER),\ mock.patch.object(self.plugin, 'get_ports', return_value=[dummy_port]),\ - mock.patch.object(self.plugin, '_get_tier0_uuid_by_router', - return_value=tier0_uuid),\ mock.patch.object(self.plugin.nsxlib.logical_router, 'get', return_value=tier0_rtr): self.driver.create_vpnservice(self.context, FAKE_VPNSERVICE) @@ -656,10 +656,13 @@ class TestVpnaasDriver(test_plugin.NsxV3PluginTestCaseMixin): providernet_args = {extnet_apidef.EXTERNAL: True} router_db = namedtuple("Router", FAKE_ROUTER.keys())( *FAKE_ROUTER.values()) + tier0_rtr_id = _uuid() with self.network(name='ext-net', providernet_args=providernet_args, arg_list=(extnet_apidef.EXTERNAL, )) as ext_net,\ self.subnet(ext_net),\ + mock.patch.object(self.plugin, '_get_tier0_uuid_by_router', + return_value=tier0_rtr_id),\ self.router(external_gateway_info={'network_id': ext_net['network']['id']}) as router,\ self.subnet() as sub: @@ -671,7 +674,6 @@ class TestVpnaasDriver(test_plugin.NsxV3PluginTestCaseMixin): # create the service dummy_port = {'id': 'dummy_port', 'fixed_ips': [{'ip_address': '1.1.1.1'}]} - tier0_rtr_id = _uuid() tier0_rtr = {'id': tier0_rtr_id, 'high_availability_mode': 'ACTIVE_STANDBY'} nsx_srv = {'logical_router_id': tier0_rtr_id, @@ -690,8 +692,6 @@ class TestVpnaasDriver(test_plugin.NsxV3PluginTestCaseMixin): return_value=FAKE_ROUTER),\ mock.patch.object(self.plugin, 'get_ports', return_value=[dummy_port]),\ - mock.patch.object(self.plugin, '_get_tier0_uuid_by_router', - return_value=tier0_rtr_id),\ mock.patch.object(self.plugin.nsxlib.logical_router, 'get', return_value=tier0_rtr): self.driver.create_vpnservice(self.context, FAKE_VPNSERVICE)