From e04cef8c17887b3614701a3bbb16fc14db4bd22d Mon Sep 17 00:00:00 2001 From: Adit Sarfaty Date: Wed, 19 Apr 2017 15:00:20 +0300 Subject: [PATCH] NSX-V| Fix FWaaS rules order Reorder the FW rules on the edge, so that internal & MD proxy traffic will always be allowed, but other traffic will go through the FWaaS rules. In additon support the case of firewall policy with no rules, and do not add the firewall rules if the router has no external gateway. Change-Id: Ia4afad53a4b68f87947eec9d0d25007128b174e9 --- .../nsx_v/drivers/shared_router_driver.py | 2 + vmware_nsx/plugins/nsx_v/plugin.py | 43 ++++++---- .../nsx_v/vshield/edge_firewall_driver.py | 13 +-- .../services/fwaas/nsx_v/edge_fwaas_driver.py | 83 ++++++++++++++----- .../services/fwaas/nsx_v/fwaas_callbacks.py | 37 +++++---- .../tests/unit/nsx_v/test_fwaas_driver.py | 30 +++++++ vmware_nsx/tests/unit/nsx_v/test_plugin.py | 9 ++ 7 files changed, 162 insertions(+), 55 deletions(-) diff --git a/vmware_nsx/plugins/nsx_v/drivers/shared_router_driver.py b/vmware_nsx/plugins/nsx_v/drivers/shared_router_driver.py index 23c4398215..e5ad059ee4 100644 --- a/vmware_nsx/plugins/nsx_v/drivers/shared_router_driver.py +++ b/vmware_nsx/plugins/nsx_v/drivers/shared_router_driver.py @@ -297,6 +297,7 @@ class RouterSharedDriver(router_driver.RouterBaseDriver): # Fake fw rule to open subnets firewall flows and static routes # relative flows fake_subnet_fw_rule = { + 'name': 'Subnet Rule', 'action': 'allow', 'enabled': True, 'source_ip_address': subnet_cidrs, @@ -307,6 +308,7 @@ class RouterSharedDriver(router_driver.RouterBaseDriver): if dnat_cidrs: # Fake fw rule to open dnat firewall flows fake_dnat_fw_rule = { + 'name': 'DNAT Rule', 'action': 'allow', 'enabled': True, 'destination_ip_address': dnat_cidrs} diff --git a/vmware_nsx/plugins/nsx_v/plugin.py b/vmware_nsx/plugins/nsx_v/plugin.py index 99bcff62d6..17fe7be75f 100644 --- a/vmware_nsx/plugins/nsx_v/plugin.py +++ b/vmware_nsx/plugins/nsx_v/plugin.py @@ -3318,6 +3318,7 @@ class NsxVPluginV2(addr_pair_db.AllowedAddressPairsMixin, router['id']) if subnet_cidrs: no_snat_fw_rules.append({ + 'name': 'No SNAT Rule', 'action': 'allow', 'enabled': True, 'source_vnic_groups': ["external"], @@ -3496,41 +3497,57 @@ class NsxVPluginV2(addr_pair_db.AllowedAddressPairsMixin, self._update_edge_router(context, router_id) def _update_subnets_and_dnat_firewall(self, context, router, - router_id=None, allow_external=True): + router_id=None): fw_rules = [] if not router_id: router_id = router['id'] + + # Add FW rule to open subnets firewall flows and static routes + # relative flows subnet_cidrs = self._find_router_subnets_cidrs(context, router['id']) routes = self._get_extra_routes_by_router_id(context, router_id) subnet_cidrs.extend([route['destination'] for route in routes]) if subnet_cidrs: - # Fake fw rule to open subnets firewall flows and static routes - # relative flows - fake_subnet_fw_rule = { + subnet_fw_rule = { + 'name': 'Subnet Rule', 'action': 'allow', 'enabled': True, 'source_ip_address': subnet_cidrs, 'destination_ip_address': subnet_cidrs} - fw_rules.append(fake_subnet_fw_rule) - _, dnat_rules = self._get_nat_rules(context, router) + fw_rules.append(subnet_fw_rule) # If metadata service is enabled, block access to inter-edge network if self.metadata_proxy_handler: fw_rules += nsx_v_md_proxy.get_router_fw_rules() + # Add fw rules if FWaaS is enabled + allow_external = True + if (self.fwaas_callbacks.should_apply_firewall_to_router( + context, router, router_id)): + fw_rules.extend(self.fwaas_callbacks.get_fwaas_rules_for_router( + context, router['id'])) + # If we have a firewall we shouldn't add the default + # allow-external rule + allow_external = False + + # Add FW rule to open dnat firewall flows + _, dnat_rules = self._get_nat_rules(context, router) dnat_cidrs = [rule['dst'] for rule in dnat_rules] if dnat_cidrs: - # Fake fw rule to open dnat firewall flows - fake_dnat_fw_rule = { + dnat_fw_rule = { + 'name': 'DNAT Rule', 'action': 'allow', 'enabled': True, 'destination_ip_address': dnat_cidrs} - fw_rules.append(fake_dnat_fw_rule) + fw_rules.append(dnat_fw_rule) + + # Add no-snat rules nosnat_fw_rules = self._get_nosnat_subnets_fw_rules( context, router) fw_rules.extend(nosnat_fw_rules) # Get the load balancer rules in case they are refreshed + # (relevant only for older LB that are still on the router edge) edge_id = self._get_edge_id_by_rtr_id(context, router_id) lb_rules = nsxv_db.get_nsxv_lbaas_loadbalancer_binding_by_edge( context.session, edge_id) @@ -3546,13 +3563,9 @@ class NsxVPluginV2(addr_pair_db.AllowedAddressPairsMixin, } fw_rules.append(lb_fw_rule) - # Add fw rules if FWaaS is enabled - fw_rules.extend(self.fwaas_callbacks.get_fwaas_rules_for_router( - context, router, router_id)) - - fake_fw = {'firewall_rule_list': fw_rules} + fw = {'firewall_rule_list': fw_rules} try: - edge_utils.update_firewall(self.nsx_v, context, router_id, fake_fw, + edge_utils.update_firewall(self.nsx_v, context, router_id, fw, allow_external=allow_external) except vsh_exc.ResourceNotFound: LOG.error("Failed to update firewall for router %s", diff --git a/vmware_nsx/plugins/nsx_v/vshield/edge_firewall_driver.py b/vmware_nsx/plugins/nsx_v/vshield/edge_firewall_driver.py index d77cf4330d..ca0dc3df1d 100644 --- a/vmware_nsx/plugins/nsx_v/vshield/edge_firewall_driver.py +++ b/vmware_nsx/plugins/nsx_v/vshield/edge_firewall_driver.py @@ -30,6 +30,7 @@ VSE_FWAAS_REJECT = "reject" FWAAS_ALLOW = "allow" FWAAS_DENY = "deny" FWAAS_REJECT = "reject" +FWAAS_ALLOW_EXT_RULE_NAME = 'Allow To External' class EdgeFirewallDriver(object): @@ -191,11 +192,12 @@ class EdgeFirewallDriver(object): ruleTag = 1 vcns_rules = [] if allow_external: - vcns_rules.append( - {'action': "accept", - 'enabled': True, - 'destination': {'vnicGroupId': ["external"]}, - 'ruleTag': ruleTag}) + vcns_rules.append({'name': FWAAS_ALLOW_EXT_RULE_NAME, + 'action': "accept", + 'enabled': True, + 'destination': {'vnicGroupId': ["external"]}, + 'ruleTag': ruleTag}) + ruleTag += 1 for rule in firewall['firewall_rule_list']: tag = rule.get('ruleTag', ruleTag) vcns_rule = self._convert_firewall_rule(rule, tag) @@ -387,7 +389,6 @@ class EdgeFirewallDriver(object): LOG.exception("Failed to update firewall " "with edge_id: %s", edge_id) vcns_fw_config = self._get_firewall(edge_id) - nsxv_db.cleanup_nsxv_edge_firewallrule_binding( context.session, edge_id) diff --git a/vmware_nsx/services/fwaas/nsx_v/edge_fwaas_driver.py b/vmware_nsx/services/fwaas/nsx_v/edge_fwaas_driver.py index b925aaafec..1c4c256a66 100644 --- a/vmware_nsx/services/fwaas/nsx_v/edge_fwaas_driver.py +++ b/vmware_nsx/services/fwaas/nsx_v/edge_fwaas_driver.py @@ -24,6 +24,7 @@ from neutron_fwaas.services.firewall.drivers import fwaas_base from vmware_nsx.common import locking from vmware_nsx.plugins.nsx_v.vshield.common import ( exceptions as vcns_exc) +from vmware_nsx.plugins.nsx_v.vshield import edge_firewall_driver from vmware_nsx.plugins.nsx_v.vshield import edge_utils from vmware_nsx.plugins.nsx_v.vshield import vcns_driver @@ -44,12 +45,35 @@ class EdgeFwaasDriver(fwaas_base.FwaasDriverBase): super(EdgeFwaasDriver, self).__init__() self._nsxv = vcns_driver.VcnsDriver(None) + def should_apply_firewall_to_router(self, router_data): + """Return True if the firewall rules should be added the router + + Return False in those cases: + - shared router (not supported) + - router without an external gateway + """ + if not router_data.get('external_gateway_info'): + LOG.info("Cannot apply firewall to router %s with no gateway", + router_data['id']) + return False + if (not router_data.get('distributed') and + router_data.get('router_type') == 'shared'): + LOG.info("Cannot apply firewall to shared router %s", + router_data['id']) + return False + return True + def _get_routers_edges(self, context, apply_list): # Get edges for all the routers in the apply list. # note that shared routers are currently not supported edge_manager = self.edge_manager edges = [] for router_info in apply_list: + + # No FWaaS rules needed if there is no external gateway + if not self.should_apply_firewall_to_router(router_info.router): + continue + lookup_id = None router_id = router_info.router_id if router_info.router.get('distributed'): @@ -57,10 +81,6 @@ class EdgeFwaasDriver(fwaas_base.FwaasDriverBase): # we need the plr edge id lookup_id = edge_manager.get_plr_by_tlr_id( context, router_id) - elif router_info.router.get('router_type') == 'shared': - # Shared router (currently not supported) - LOG.info("Cannot apply firewall to shared router %s", - router_id) else: # Exclusive router lookup_id = router_id @@ -94,6 +114,18 @@ class EdgeFwaasDriver(fwaas_base.FwaasDriverBase): return translated_rules + def _is_allow_external_rule(self, rule): + rule_name = rule.get('name', '') + if rule_name == edge_firewall_driver.FWAAS_ALLOW_EXT_RULE_NAME: + return True + # For older routers, the allow-external rule didn't have a name + # TODO(asarfaty): delete this in the future + if (not rule_name and + rule.get('action') == edge_firewall_driver.FWAAS_ALLOW and + rule.get('destination_vnic_groups', []) == ['external']): + return True + return False + def _get_other_backend_rules(self, context, edge_id): """Get a list of current backend rules from other applications @@ -107,36 +139,42 @@ class EdgeFwaasDriver(fwaas_base.FwaasDriverBase): # Need to create a new one backend_rules = [] - # remove old FWaaS rules from the rules list - relevant_rules = [] + # remove old FWaaS rules from the rules list. + # also delete the allow-external rule, if it is there. + # If necessary - we will add it again later + other_rules = [] for rule_item in backend_rules: rule = rule_item['firewall_rule'] - if not rule.get('name', '').startswith(RULE_NAME_PREFIX): - relevant_rules.append(rule) + rule_name = rule.get('name', '') + if (not rule_name.startswith(RULE_NAME_PREFIX) and + not self._is_allow_external_rule(rule)): + other_rules.append(rule) - return relevant_rules + return other_rules - def _set_rules_on_edge(self, context, edge_id, fw_id, translated_rules): + def _set_rules_on_edge(self, context, edge_id, fw_id, translated_rules, + allow_external=False): """delete old FWaaS rules from the Edge, and add new ones Note that the edge might have other FW rules like NAT or LBaas that should remain there. + + allow_external is usually False because it shouldn't exist with a + firewall. It should only be True when the firewall is being deleted. """ # Get the existing backend rules which do not belong to FWaaS backend_rules = self._get_other_backend_rules(context, edge_id) # add new FWaaS rules at the end by their original order backend_rules.extend(translated_rules) - # update the backend - # allow_external is False because it was already added try: with locking.LockManager.get_lock(str(edge_id)): self._nsxv.update_firewall( edge_id, {'firewall_rule_list': backend_rules}, context, - allow_external=False) + allow_external=allow_external) except Exception as e: # catch known library exceptions and raise Fwaas generic exception LOG.error("Failed to update backend firewall %(fw)s: " @@ -174,29 +212,34 @@ class EdgeFwaasDriver(fwaas_base.FwaasDriverBase): """Remove previous policy and apply the new policy.""" self._create_or_update_firewall(agent_mode, apply_list, firewall) - def _delete_firewall_or_set_default_policy(self, apply_list, firewall): + def _delete_firewall_or_set_default_policy(self, apply_list, firewall, + allow_external): context = n_context.get_admin_context() router_edges = self._get_routers_edges(context, apply_list) if router_edges: for edge_id in router_edges: - self._set_rules_on_edge(context, edge_id, firewall['id'], []) + self._set_rules_on_edge(context, edge_id, firewall['id'], [], + allow_external=allow_external) @log_helpers.log_method_call def delete_firewall(self, agent_mode, apply_list, firewall): """Delete firewall. - Removes rules created by this instance from the backend firewall. + Removes rules created by this instance from the backend firewall + And add the default allow-external rule. """ - self._delete_firewall_or_set_default_policy(apply_list, firewall) + self._delete_firewall_or_set_default_policy(apply_list, firewall, + allow_external=True) @log_helpers.log_method_call def apply_default_policy(self, agent_mode, apply_list, firewall): """Apply the default policy (deny all). - The backend firewall always has this policy as default, so we only - need to delete the current rules. + The backend firewall always has this policy (=deny all) as default, + so we only need to delete the current rules. """ - self._delete_firewall_or_set_default_policy(apply_list, firewall) + self._delete_firewall_or_set_default_policy(apply_list, firewall, + allow_external=False) def get_firewall_translated_rules(self, firewall): if firewall['admin_state_up']: diff --git a/vmware_nsx/services/fwaas/nsx_v/fwaas_callbacks.py b/vmware_nsx/services/fwaas/nsx_v/fwaas_callbacks.py index 408c591990..53e90fadd5 100644 --- a/vmware_nsx/services/fwaas/nsx_v/fwaas_callbacks.py +++ b/vmware_nsx/services/fwaas/nsx_v/fwaas_callbacks.py @@ -14,6 +14,7 @@ # under the License. from oslo_config import cfg +from oslo_log import log as logging from neutron.agent.l3 import router_info from neutron.common import config as neutron_config # noqa @@ -25,6 +26,8 @@ from neutron_fwaas.services.firewall.agents.l3reference \ from neutron_lib import context as n_context from neutron_lib.plugins import directory +LOG = logging.getLogger(__name__) + class NsxvFwaasCallbacks(firewall_l3_agent.L3WithFWaaS): """NSX-V RPC callbacks for Firewall As A Service - V1.""" @@ -70,31 +73,37 @@ class NsxvFwaasCallbacks(firewall_l3_agent.L3WithFWaaS): return [self._router_dict_to_obj(ri) for ri in tenant_routers if ri['id'] in router_ids] - def get_fwaas_rules_for_router(self, context, router, router_id): - """Return the list of (translated) fwaas rules for this router.""" + def should_apply_firewall_to_router(self, context, router, router_id): + """Return True if the FWaaS rules should be added to this router.""" if not self.fwaas_enabled: - return [] - - ctx_elevated = context.elevated() + return False # get all the relevant router info # ("router" does not have all the fields) + ctx_elevated = context.elevated() router_data = self.core_plugin.get_router(ctx_elevated, router['id']) if not router_data: - return [] + LOG.error("Couldn't read router %s data", router['id']) + return False + + # Check if the FWaaS driver supports this router + if not self.fwaas_driver.should_apply_firewall_to_router(router_data): + return False + if router_data.get('distributed'): - # in case of distributed router router['id'] is the id of the - # neutron router + # in case of a distributed-router: + # router['id'] is the id of the neutron router (=tlr) # and router_id is the plr/tlr (the one that is being updated) if router_id == router['id']: # Do not add firewall rules on the tlr router. - return [] - if router_data.get('router_type') == 'shared': - # Currently there is no FWaaS support for shared routers - return [] + return False - # Exclusive router or PLR - fw_id = self._get_router_firewall_id(ctx_elevated, router['id']) + return True + + def get_fwaas_rules_for_router(self, context, router_id): + """Return the list of (translated) FWaaS rules for this router.""" + ctx_elevated = context.elevated() + fw_id = self._get_router_firewall_id(ctx_elevated, router_id) if fw_id: return self._get_fw_applicable_rules(ctx_elevated, fw_id) return [] diff --git a/vmware_nsx/tests/unit/nsx_v/test_fwaas_driver.py b/vmware_nsx/tests/unit/nsx_v/test_fwaas_driver.py index f9ce643029..0b1b24dc59 100644 --- a/vmware_nsx/tests/unit/nsx_v/test_fwaas_driver.py +++ b/vmware_nsx/tests/unit/nsx_v/test_fwaas_driver.py @@ -99,6 +99,8 @@ class NsxvFwaasTestCase(test_v_plugin.NsxVPluginV2TestCase): self.assertEqual(router_count, update_fw.call_count) bakend_rules = update_fw.call_args[0][1]['firewall_rule_list'] self.assertEqual(len(rule_list), len(bakend_rules)) + allow_ext = update_fw.call_args[1]['allow_external'] + self.assertEqual(False, allow_ext) def test_create_firewall_no_rules(self): apply_list = self._fake_apply_list() @@ -109,6 +111,8 @@ class NsxvFwaasTestCase(test_v_plugin.NsxVPluginV2TestCase): self.assertEqual(1, update_fw.call_count) bakend_rules = update_fw.call_args[0][1]['firewall_rule_list'] self.assertEqual(0, len(bakend_rules)) + allow_ext = update_fw.call_args[1]['allow_external'] + self.assertEqual(False, allow_ext) def test_create_firewall_with_rules(self): self._setup_firewall_with_rules(self.firewall.create_firewall) @@ -129,6 +133,8 @@ class NsxvFwaasTestCase(test_v_plugin.NsxVPluginV2TestCase): self.assertEqual(1, update_fw.call_count) bakend_rules = update_fw.call_args[0][1]['firewall_rule_list'] self.assertEqual(0, len(bakend_rules)) + allow_ext = update_fw.call_args[1]['allow_external'] + self.assertEqual(True, allow_ext) def test_create_firewall_with_admin_down(self): apply_list = self._fake_apply_list() @@ -140,3 +146,27 @@ class NsxvFwaasTestCase(test_v_plugin.NsxVPluginV2TestCase): self.assertEqual(1, update_fw.call_count) bakend_rules = update_fw.call_args[0][1]['firewall_rule_list'] self.assertEqual(0, len(bakend_rules)) + allow_ext = update_fw.call_args[1]['allow_external'] + self.assertEqual(False, allow_ext) + + def test_should_apply_firewall_to_router(self): + router = {'id': 'fake_id', + 'external_gateway_info': 'fake_data', + 'router_type': 'exclusive', + 'distributed': False} + self.assertTrue(self.firewall.should_apply_firewall_to_router(router)) + + # no external gateway: + router['external_gateway_info'] = None + self.assertFalse(self.firewall.should_apply_firewall_to_router(router)) + router['external_gateway_info'] = 'Dummy' + + # not for shared router: + router['router_type'] = 'shared' + router['distributed'] = False + self.assertFalse(self.firewall.should_apply_firewall_to_router(router)) + + # should work for distributed router + router['router_type'] = 'exclusive' + router['distributed'] = True + self.assertTrue(self.firewall.should_apply_firewall_to_router(router)) diff --git a/vmware_nsx/tests/unit/nsx_v/test_plugin.py b/vmware_nsx/tests/unit/nsx_v/test_plugin.py index 7b118bdb73..23dfbc1553 100644 --- a/vmware_nsx/tests/unit/nsx_v/test_plugin.py +++ b/vmware_nsx/tests/unit/nsx_v/test_plugin.py @@ -2294,10 +2294,12 @@ class L3NatTestCaseBase(test_l3_plugin.L3NatTestCaseMixin): def test_router_set_gateway_with_nosnat(self, mock): expected_fw = [{'action': 'allow', 'enabled': True, + 'name': 'Subnet Rule', 'source_ip_address': [], 'destination_ip_address': []}] nosnat_fw = [{'action': 'allow', 'enabled': True, + 'name': 'No SNAT Rule', 'source_vnic_groups': ["external"], 'destination_ip_address': []}] @@ -2995,6 +2997,7 @@ class TestExclusiveRouterTestCase(L3NatTest, L3NatTestCaseBase, expected_cidrs = [s1_cidr, s2_cidr] expected_fw = [{'action': 'allow', 'enabled': True, + 'name': 'Subnet Rule', 'source_ip_address': expected_cidrs, 'destination_ip_address': expected_cidrs}] fw_rules = mock.call_args[0][3]['firewall_rule_list'] @@ -3030,6 +3033,7 @@ class TestExclusiveRouterTestCase(L3NatTest, L3NatTestCaseBase, expected_cidrs = [s1_cidr, s2_cidr] fw_rule = {'action': 'allow', 'enabled': True, + 'name': 'Subnet Rule', 'source_ip_address': expected_cidrs, 'destination_ip_address': expected_cidrs} vse_rule = {'action': 'allow', @@ -3150,6 +3154,7 @@ class TestExclusiveRouterTestCase(L3NatTest, L3NatTestCaseBase, expected_cidrs = [s1_cidr, s2_cidr] expected_fw = [{'action': 'allow', 'enabled': True, + 'name': 'Subnet Rule', 'source_ip_address': expected_cidrs, 'destination_ip_address': expected_cidrs}] fw_rules = mock.call_args[0][3]['firewall_rule_list'] @@ -4495,18 +4500,22 @@ class TestSharedRouterTestCase(L3NatTest, L3NatTestCaseBase, def test_routers_set_gateway_with_nosnat(self, mock): expected_fw1 = [{'action': 'allow', 'enabled': True, + 'name': 'Subnet Rule', 'source_ip_address': [], 'destination_ip_address': []}] expected_fw2 = [{'action': 'allow', 'enabled': True, + 'name': 'Subnet Rule', 'source_ip_address': [], 'destination_ip_address': []}] nosnat_fw1 = [{'action': 'allow', 'enabled': True, + 'name': 'No SNAT Rule', 'source_vnic_groups': ["external"], 'destination_ip_address': []}] nosnat_fw2 = [{'action': 'allow', 'enabled': True, + 'name': 'No SNAT Rule', 'source_vnic_groups': ["external"], 'destination_ip_address': []}] with self.router() as r1, self.router() as r2,\