From d938e19af75507592ba011a5a0766d858bc67735 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 b243719e..2ab957df 100644 --- a/vmware_nsxlib/tests/unit/v3/policy/test_resources.py +++ b/vmware_nsxlib/tests/unit/v3/policy/test_resources.py @@ -1955,6 +1955,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 091ca9b1..e22f2c2d 100644 --- a/vmware_nsxlib/tests/unit/v3/policy/test_transaction.py +++ b/vmware_nsxlib/tests/unit/v3/policy/test_transaction.py @@ -391,7 +391,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', @@ -449,15 +450,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 @@ -471,6 +477,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 37d222e2..65e12b8f 100644 --- a/vmware_nsxlib/v3/policy/core_resources.py +++ b/vmware_nsxlib/v3/policy/core_resources.py @@ -3469,14 +3469,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, @@ -3535,7 +3538,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: