From 138d9774000335d66c335e036ea96dac7b4a4799 Mon Sep 17 00:00:00 2001 From: Salvatore Orlando Date: Mon, 9 Jun 2014 09:53:47 -0700 Subject: [PATCH] NSX: Optionally not enforce nat rule match length check This patch adds the 'raise_on_len_mismatch' parameter to the 'delete_nat_rules_by_match' function. The plugin then leverages this parameter for ensuring NAT rules deletion operations are completed successfully even when duplicate rules are found or no corresponding rules are found at all. With this change, the 'remove_router_interface' operation will correctly complete even in cases when NAT rules in Neutron and the NSX backend are out of sync. This patch also changes a check in delete_nat_rules_by_match in order to make it less expensive. Closes-Bug: 1328181 Change-Id: I856d67ef5ff6264374cb8f2569668da4c205ad9f --- neutron/plugins/vmware/nsxlib/router.py | 23 +++++++++--- neutron/plugins/vmware/plugins/base.py | 3 ++ .../tests/unit/vmware/nsxlib/test_router.py | 26 +++++++++++++ neutron/tests/unit/vmware/test_nsx_plugin.py | 37 +++++++++++++++++-- 4 files changed, 81 insertions(+), 8 deletions(-) diff --git a/neutron/plugins/vmware/nsxlib/router.py b/neutron/plugins/vmware/nsxlib/router.py index 52d34299f..254b4b6a5 100644 --- a/neutron/plugins/vmware/nsxlib/router.py +++ b/neutron/plugins/vmware/nsxlib/router.py @@ -551,6 +551,7 @@ def create_lrouter_dnat_rule_v3(cluster, router_id, dst_ip, to_dst_port=None, def delete_nat_rules_by_match(cluster, router_id, rule_type, max_num_expected, min_num_expected=0, + raise_on_len_mismatch=True, **kwargs): # remove nat rules nat_rules = query_nat_rules(cluster, router_id) @@ -564,14 +565,26 @@ def delete_nat_rules_by_match(cluster, router_id, rule_type, break else: to_delete_ids.append(r['uuid']) - if not (len(to_delete_ids) in - range(min_num_expected, max_num_expected + 1)): - raise nsx_exc.NatRuleMismatch(actual_rules=len(to_delete_ids), - min_rules=min_num_expected, - max_rules=max_num_expected) + num_rules_to_delete = len(to_delete_ids) + if (num_rules_to_delete < min_num_expected or + num_rules_to_delete > max_num_expected): + if raise_on_len_mismatch: + raise nsx_exc.NatRuleMismatch(actual_rules=num_rules_to_delete, + min_rules=min_num_expected, + max_rules=max_num_expected) + else: + LOG.warn(_("Found %(actual_rule_num)d matching NAT rules, which " + "is not in the expected range (%(min_exp_rule_num)d," + "%(max_exp_rule_num)d)"), + {'actual_rule_num': num_rules_to_delete, + 'min_exp_rule_num': min_num_expected, + 'max_exp_rule_num': max_num_expected}) for rule_id in to_delete_ids: delete_router_nat_rule(cluster, router_id, rule_id) + # Return number of deleted rules - useful at least for + # testing purposes + return num_rules_to_delete def delete_router_nat_rule(cluster, router_id, rule_id): diff --git a/neutron/plugins/vmware/plugins/base.py b/neutron/plugins/vmware/plugins/base.py index 072fd20eb..3895a042b 100644 --- a/neutron/plugins/vmware/plugins/base.py +++ b/neutron/plugins/vmware/plugins/base.py @@ -298,6 +298,7 @@ class NsxPluginV2(addr_pair_db.AllowedAddressPairsMixin, routerlib.delete_nat_rules_by_match( self.cluster, nsx_router_id, "SourceNatRule", max_num_expected=1, min_num_expected=0, + raise_on_len_mismatch=False, source_ip_addresses=cidr) if add_snat_rules: ip_addresses = self._build_ip_address_list( @@ -1703,6 +1704,7 @@ class NsxPluginV2(addr_pair_db.AllowedAddressPairsMixin, routerlib.delete_nat_rules_by_match( self.cluster, nsx_router_id, "SourceNatRule", max_num_expected=1, min_num_expected=1, + raise_on_len_mismatch=False, source_ip_addresses=subnet['cidr']) def add_router_interface(self, context, router_id, interface_info): @@ -1806,6 +1808,7 @@ class NsxPluginV2(addr_pair_db.AllowedAddressPairsMixin, routerlib.delete_nat_rules_by_match( self.cluster, nsx_router_id, "NoSourceNatRule", max_num_expected=1, min_num_expected=0, + raise_on_len_mismatch=False, destination_ip_addresses=subnet['cidr']) except n_exc.NotFound: LOG.error(_("Logical router resource %s not found " diff --git a/neutron/tests/unit/vmware/nsxlib/test_router.py b/neutron/tests/unit/vmware/nsxlib/test_router.py index 6d4063fe5..39aa2e4a2 100644 --- a/neutron/tests/unit/vmware/nsxlib/test_router.py +++ b/neutron/tests/unit/vmware/nsxlib/test_router.py @@ -920,3 +920,29 @@ class TestLogicalRouters(base.NsxlibTestCase): routerlib.delete_nat_rules_by_match, self.fake_cluster, lrouter['uuid'], 'SomeWeirdType', 1, 1) + + def test_delete_nat_rules_by_match_len_mismatch_does_not_raise(self): + lrouter = self._prepare_nat_rules_for_delete_tests() + rules = routerlib.query_nat_rules(self.fake_cluster, lrouter['uuid']) + self.assertEqual(len(rules), 3) + deleted_rules = routerlib.delete_nat_rules_by_match( + self.fake_cluster, lrouter['uuid'], + 'DestinationNatRule', + max_num_expected=1, min_num_expected=1, + raise_on_len_mismatch=False, + destination_ip_addresses='99.99.99.99') + self.assertEqual(0, deleted_rules) + # add an extra rule to emulate a duplicate one + with mock.patch.object(self.fake_cluster.api_client, + 'get_version', + new=lambda: version_module.Version('2.0')): + routerlib.create_lrouter_snat_rule( + self.fake_cluster, lrouter['uuid'], + '10.0.0.2', '10.0.0.2', order=220, + match_criteria={'source_ip_addresses': '192.168.0.0/24'}) + deleted_rules_2 = routerlib.delete_nat_rules_by_match( + self.fake_cluster, lrouter['uuid'], 'SourceNatRule', + min_num_expected=1, max_num_expected=1, + raise_on_len_mismatch=False, + source_ip_addresses='192.168.0.0/24') + self.assertEqual(2, deleted_rules_2) diff --git a/neutron/tests/unit/vmware/test_nsx_plugin.py b/neutron/tests/unit/vmware/test_nsx_plugin.py index 21b28513a..d898159bb 100644 --- a/neutron/tests/unit/vmware/test_nsx_plugin.py +++ b/neutron/tests/unit/vmware/test_nsx_plugin.py @@ -14,6 +14,7 @@ # limitations under the License. import contextlib +import uuid import mock import netaddr @@ -1165,12 +1166,21 @@ class NeutronNsxOutOfSync(NsxPluginV2TestCase, res = req.get_response(self.ext_api) self.assertEqual(res.status_int, 200) - def test_remove_router_interface_not_in_nsx(self): + def _test_remove_router_interface_nsx_out_of_sync(self, unsync_action): + # Create external network and subnet + ext_net_id = self._create_network_and_subnet('1.1.1.0/24', True)[0] # Create internal network and subnet int_sub_id = self._create_network_and_subnet('10.0.0.0/24')[1] res = self._create_router('json', 'tenant') router = self.deserialize('json', res) - # Add interface to router (needed to generate NAT rule) + # Set gateway and add interface to router (needed to generate NAT rule) + req = self.new_update_request( + 'routers', + {'router': {'external_gateway_info': + {'network_id': ext_net_id}}}, + router['router']['id']) + res = req.get_response(self.ext_api) + self.assertEqual(res.status_int, 200) req = self.new_action_request( 'routers', {'subnet_id': int_sub_id}, @@ -1178,7 +1188,7 @@ class NeutronNsxOutOfSync(NsxPluginV2TestCase, "add_router_interface") res = req.get_response(self.ext_api) self.assertEqual(res.status_int, 200) - self.fc._fake_lrouter_dict.clear() + unsync_action() req = self.new_action_request( 'routers', {'subnet_id': int_sub_id}, @@ -1187,6 +1197,27 @@ class NeutronNsxOutOfSync(NsxPluginV2TestCase, res = req.get_response(self.ext_api) self.assertEqual(res.status_int, 200) + def test_remove_router_interface_not_in_nsx(self): + + def unsync_action(): + self.fc._fake_lrouter_dict.clear() + self.fc._fake_lrouter_nat_dict.clear() + + self._test_remove_router_interface_nsx_out_of_sync(unsync_action) + + def test_remove_router_interface_nat_rule_not_in_nsx(self): + self._test_remove_router_interface_nsx_out_of_sync( + self.fc._fake_lrouter_nat_dict.clear) + + def test_remove_router_interface_duplicate_nat_rules_in_nsx(self): + + def unsync_action(): + # duplicate every entry in the nat rule dict + for (_rule_id, rule) in self.fc._fake_lrouter_nat_dict.items(): + self.fc._fake_lrouter_nat_dict[uuid.uuid4()] = rule + + self._test_remove_router_interface_nsx_out_of_sync(unsync_action) + def test_update_router_not_in_nsx(self): res = self._create_router('json', 'tenant') router = self.deserialize('json', res) -- 2.45.2