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

@@ -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
+        if sr_currently_exists:
-        real_org_enable_snat = org_enable_snat and orgaddr
+            # currently there is a service router on the backend
-
+            actions['add_service_router'] = False
-        actions['add_service_router'] = ((real_new_enable_snat and
+            # Should remove the service router if the GW was removed,
-                                          not real_org_enable_snat) or
+            # or no service needs it: SNAT, LBaaS or FWaaS
-                                         (real_new_enable_snat and not
+            actions['remove_service_router'] = (
-                                         orgaddr and newaddr)
+                not has_gw or not (fw_exist or lb_exist or new_with_snat))
-                                         ) and not (fw_exist or lb_exist)
+            if actions['remove_service_router']:
-        actions['remove_service_router'] = ((not real_new_enable_snat and
+                LOG.info("Removing service router [has GW: %s, FW %s, LB %s, "
-                                             real_org_enable_snat) or (
+                         "SNAT %s]",
-                orgaddr and not newaddr)) and not (fw_exist or lb_exist)
+                         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
 

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,
+            new_tier0_uuid, newaddr, new_enable_snat,
-            lb_exist=False)
+            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:

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']:

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):

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)
+                    self.core_plugin.create_service_router(
-                    exists_on_backend = True
+                        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(

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:

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

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'])

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)