From 24069cc9778f6b5fbcecc43d6793aa88000d2a9b Mon Sep 17 00:00:00 2001 From: asarfaty Date: Thu, 16 Jul 2020 08:58:26 +0200 Subject: [PATCH] Improve security policy update rules with transactions Use the policy 'rules' attribute instead of adding child rules. This is expected to have better performance on the NSX side. This patch re-itroduce the fix from: I213616a8b47f11adb1a897568746885f3e77078c but this time with a flag to not break stuff Change-Id: Ib6361575642fa96a93dd49107ece1f120a6e61b2 --- .../tests/unit/v3/policy/test_resources.py | 1 + .../tests/unit/v3/policy/test_transaction.py | 24 ++++++++++++++----- vmware_nsxlib/v3/policy/core_resources.py | 14 ++++++++++- 3 files changed, 32 insertions(+), 7 deletions(-) diff --git a/vmware_nsxlib/tests/unit/v3/policy/test_resources.py b/vmware_nsxlib/tests/unit/v3/policy/test_resources.py index abb90639..35dd1c06 100644 --- a/vmware_nsxlib/tests/unit/v3/policy/test_resources.py +++ b/vmware_nsxlib/tests/unit/v3/policy/test_resources.py @@ -2022,6 +2022,7 @@ class TestPolicyCommunicationMap(NsxPolicyLibTestCase): update_call.assert_called_once_with( domain_id, map_id, entries, category=constants.CATEGORY_APPLICATION, + use_child_rules=True, tenant=TEST_TENANT) def test_update_with_entries(self): diff --git a/vmware_nsxlib/tests/unit/v3/policy/test_transaction.py b/vmware_nsxlib/tests/unit/v3/policy/test_transaction.py index 8cd29a42..259549c2 100644 --- a/vmware_nsxlib/tests/unit/v3/policy/test_transaction.py +++ b/vmware_nsxlib/tests/unit/v3/policy/test_transaction.py @@ -393,7 +393,8 @@ class TestPolicyTransaction(policy_testcase.TestPolicyApi): self.assert_infra_patch_call(expected_body) @mock.patch('vmware_nsxlib.v3.policy.core_defs.NsxPolicyApi.get') - def test_updating_security_policy_and_dfw_rules(self, mock_get_api): + def _test_updating_security_policy_and_dfw_rules( + self, use_child_rules, mock_get_api): dfw_rule1 = {'id': 'rule_id1', 'action': 'ALLOW', 'display_name': 'rule1', 'description': None, 'direction': 'IN_OUT', 'ip_protocol': 'IPV4_IPV6', @@ -451,15 +452,20 @@ class TestPolicyTransaction(policy_testcase.TestPolicyApi): name=security_policy['display_name'], domain_id=domain_id, map_id=map_id, - entries=dfw_rule_entries + entries=dfw_rule_entries, + use_child_rules=use_child_rules ) dfw_rule1['display_name'] = new_rule_name dfw_rule1['direction'] = new_direction - child_rules = [{'resource_type': 'ChildRule', 'Rule': dfw_rule1}, - {'resource_type': 'ChildRule', 'Rule': dfw_rule2, - 'marked_for_delete': True}] - security_policy.update({'children': child_rules}) + if use_child_rules: + child_rules = [{'resource_type': 'ChildRule', 'Rule': dfw_rule1}, + {'resource_type': 'ChildRule', 'Rule': dfw_rule2, + 'marked_for_delete': True}] + security_policy.update({'children': child_rules}) + else: + security_policy['rules'] = copy.deepcopy([dfw_rule1, dfw_rule2]) + child_security_policies = [{ 'resource_type': 'ChildSecurityPolicy', 'SecurityPolicy': security_policy @@ -473,6 +479,12 @@ class TestPolicyTransaction(policy_testcase.TestPolicyApi): 'children': child_domains} self.assert_infra_patch_call(expected_body) + def test_updating_security_policy_and_dfw_rules(self): + return self._test_updating_security_policy_and_dfw_rules(True) + + def test_updating_security_policy_and_dfw_rules_no_child_rules(self): + return self._test_updating_security_policy_and_dfw_rules(False) + @mock.patch('vmware_nsxlib.v3.policy.core_defs.NsxPolicyApi.get') def test_updating_security_policy_with_no_entries_set(self, mock_get_api): dfw_rule1 = {'id': 'rule_id1', 'action': 'ALLOW', diff --git a/vmware_nsxlib/v3/policy/core_resources.py b/vmware_nsxlib/v3/policy/core_resources.py index d66ef82f..fd80d96e 100644 --- a/vmware_nsxlib/v3/policy/core_resources.py +++ b/vmware_nsxlib/v3/policy/core_resources.py @@ -3560,14 +3560,17 @@ class NsxPolicySecurityPolicyBaseApi(NsxPolicyResourceBase): def update_entries(self, domain_id, map_id, entries, category=constants.CATEGORY_APPLICATION, + use_child_rules=True, tenant=constants.POLICY_INFRA_TENANT): self.update_with_entries(domain_id, map_id, entries, category=category, + use_child_rules=use_child_rules, tenant=tenant) def update_with_entries(self, domain_id, map_id, entries=IGNORE, name=IGNORE, description=IGNORE, category=constants.CATEGORY_APPLICATION, tags=IGNORE, map_sequence_number=IGNORE, + use_child_rules=True, tenant=constants.POLICY_INFRA_TENANT): map_def = self._init_parent_def( domain_id=domain_id, map_id=map_id, @@ -3626,7 +3629,16 @@ class NsxPolicySecurityPolicyBaseApi(NsxPolicyResourceBase): map_def.set_obj_dict(comm_map) # Update the entire map at the NSX if transaction: - self._create_or_store(map_def, replaced_entries) + if use_child_rules: + self._create_or_store(map_def, replaced_entries) + else: + if not ignore_entries: + # Add the rules under the map and not as ChildRules for + # improved performance on the NSX side + comm_map['rules'] = [rule.get_obj_dict() for rule in + replaced_entries] + map_def.set_obj_dict(comm_map) + self._create_or_store(map_def) else: body = map_def.get_obj_dict() if not ignore_entries: