From 6c7d06bb29ccc00bd1d67dd1fedc925f6bc8292a Mon Sep 17 00:00:00 2001 From: Adit Sarfaty Date: Thu, 27 Jun 2019 14:48:48 +0300 Subject: [PATCH] NSX|P: Refactor GW FW creation & deletion The FWaaS GW policy will be deleted only when there are no more rules, and created only if relevant. Change-Id: I90118da9d492fecf7dae151f2734a5b71b7109b9 --- .../fwaas/nsx_p/fwaas_callbacks_v2.py | 24 +++++++------- .../tests/unit/nsx_p/test_fwaas_v2_driver.py | 31 +++---------------- 2 files changed, 18 insertions(+), 37 deletions(-) diff --git a/vmware_nsx/services/fwaas/nsx_p/fwaas_callbacks_v2.py b/vmware_nsx/services/fwaas/nsx_p/fwaas_callbacks_v2.py index 7167e323bb..b4a7f9f603 100644 --- a/vmware_nsx/services/fwaas/nsx_p/fwaas_callbacks_v2.py +++ b/vmware_nsx/services/fwaas/nsx_p/fwaas_callbacks_v2.py @@ -57,7 +57,7 @@ class NsxpFwaasCallbacksV2(com_callbacks.NsxCommonv3FwaasCallbacksV2): def _get_default_backend_rule(self, router_id): """Return the default allow-all rule entry - This rule enrty will be added to the end of the rules list + This rule entry will be added to the end of the rules list """ return self.nsxpolicy.gateway_policy.build_entry( DEFAULT_RULE_NAME, @@ -363,17 +363,19 @@ class NsxpFwaasCallbacksV2(com_callbacks.NsxCommonv3FwaasCallbacksV2): sr_exists_on_backend = False if sr_exists_on_backend: - # update the edge firewall - self.create_router_gateway_policy(context, router_id, - router, fw_rules) + if router_with_fw: + self.create_or_update_router_gateway_policy(context, router_id, + router, fw_rules) + else: + # Do all the cleanup once the router has no more FW rules + # create or update the edge firewall + # TODO(asarfaty): Consider keeping the FW with default allow + # rule instead of deletion as it may be created again soon + self.delete_router_gateway_policy(router_id) + self.cleanup_router_fw_resources(router_id) - if not router_with_fw: - # Do all the cleanup once the router has no more FW rules - self.delete_router_gateway_policy(router_id) - self.cleanup_router_fw_resources(router_id) - - def create_router_gateway_policy(self, context, router_id, - router, fw_rules): + def create_or_update_router_gateway_policy(self, context, router_id, + router, fw_rules): """Create/Overwrite gateway policy for a router with firewall rules""" # Check if the gateway policy already exists try: diff --git a/vmware_nsx/tests/unit/nsx_p/test_fwaas_v2_driver.py b/vmware_nsx/tests/unit/nsx_p/test_fwaas_v2_driver.py index bfe874ea92..adcf235016 100644 --- a/vmware_nsx/tests/unit/nsx_p/test_fwaas_v2_driver.py +++ b/vmware_nsx/tests/unit/nsx_p/test_fwaas_v2_driver.py @@ -337,20 +337,10 @@ class NsxpFwaasTestCase(test_p_plugin.NsxPPluginTestCaseMixin): return_value={'project_id': self.project_id}),\ mock.patch.object(self.plugin, 'service_router_has_services', return_value=True), \ - mock.patch(GW_POLICY_PATH + ".update_entries") as update_fw: + mock.patch(GW_POLICY_PATH + ".delete") as delete_fw: self.firewall.delete_firewall_group('nsx', apply_list, firewall) - - # expecting only the default allow-all rule - expected_rules = [self._default_rule(0)] - update_fw.assert_called_once_with( - policy_constants.DEFAULT_DOMAIN, FAKE_ROUTER_ID, mock.ANY, - category=policy_constants.CATEGORY_LOCAL_GW) - # compare rules one by one - actual_rules = update_fw.call_args[0][2] - self.assertEqual(len(expected_rules), len(actual_rules)) - for index in range(len(actual_rules)): - self.assertEqual(expected_rules[index], - actual_rules[index].get_obj_dict()) + delete_fw.assert_called_once_with( + policy_constants.DEFAULT_DOMAIN, map_id=FAKE_ROUTER_ID) def test_create_firewall_with_admin_down(self): apply_list = self._fake_apply_list() @@ -360,17 +350,6 @@ class NsxpFwaasTestCase(test_p_plugin.NsxPPluginTestCaseMixin): return_value=True), \ mock.patch.object(self.plugin, '_get_router', return_value={'project_id': self.project_id}),\ - mock.patch(GW_POLICY_PATH + ".update_entries") as update_fw: + mock.patch(GW_POLICY_PATH + ".create_with_entries") as update_fw: self.firewall.create_firewall_group('nsx', apply_list, firewall) - - # expecting only the default allow-all rule - expected_rules = [self._default_rule(0)] - update_fw.assert_called_once_with( - policy_constants.DEFAULT_DOMAIN, FAKE_ROUTER_ID, mock.ANY, - category=policy_constants.CATEGORY_LOCAL_GW) - # compare rules one by one - actual_rules = update_fw.call_args[0][2] - self.assertEqual(len(expected_rules), len(actual_rules)) - for index in range(len(actual_rules)): - self.assertEqual(expected_rules[index], - actual_rules[index].get_obj_dict()) + update_fw.assert_not_called()