From 36d329f538e4d68a85e2439252df3a0be2405c7f 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 86be8c69..4cc19af2 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 bcaa8706..7bc912d7 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 7ebd05c7..1c19691c 100644 --- a/vmware_nsxlib/v3/policy/core_resources.py +++ b/vmware_nsxlib/v3/policy/core_resources.py @@ -3559,14 +3559,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, @@ -3625,7 +3628,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: