From f0cc239a83e18911a899ce3fa7a470443472fa5e Mon Sep 17 00:00:00 2001 From: Yang Ding Date: Thu, 5 Mar 2020 16:48:55 -0800 Subject: [PATCH] Dedup conditions before build union condition NSX will reject expression list of a group if defined like follows: --- "expression": [{"expressions": [ {"member_type": "LogicalPort", "operator": "EQUALS", "value": "tag1|x"}, {"conjunction_operator": "AND", "resource_type": "ConjunctionOperator"}, {member_type": "LogicalPort", "operator": "EQUALS", "value": "tag2|y"} ], "resource_type": "NestedExpression"}, {"conjunction_operator": "OR", "resource_type": "ConjunctionOperator"}, {"expressions": [ {"member_type": "LogicalPort", "operator": "EQUALS", "value": "tag1|x"}, {"conjunction_operator": "AND", "resource_type": "ConjunctionOperator"}, {member_type": "LogicalPort", "operator": "EQUALS", "value": "tag2|y"} ], "resource_type": "NestedExpression"}, "resource_type": "NestedExpression"} ] --- Because the two NestedExpressions are identical. If patch a group with the spec above, 'Duplicate expressions specified' error will be returned. This patch ensures that before a union condition is built, all conditions are first dedupped. Change-Id: I0d2d93f6ade992582ad931b3622354e5b9398a1d Signed-off-by: Yang Ding --- .../tests/unit/v3/policy/test_resources.py | 59 +++++++++++++++++-- vmware_nsxlib/v3/policy/core_defs.py | 48 +++++++++++++++ vmware_nsxlib/v3/policy/core_resources.py | 17 ++++-- 3 files changed, 114 insertions(+), 10 deletions(-) diff --git a/vmware_nsxlib/tests/unit/v3/policy/test_resources.py b/vmware_nsxlib/tests/unit/v3/policy/test_resources.py index 192bf66b..d5d7447b 100644 --- a/vmware_nsxlib/tests/unit/v3/policy/test_resources.py +++ b/vmware_nsxlib/tests/unit/v3/policy/test_resources.py @@ -384,8 +384,16 @@ class TestPolicyGroup(NsxPolicyLibTestCase): cond_op=cond_op, cond_member_type=cond_member_type, cond_key=cond_key) - union_cond = self.resourceApi.build_union_condition( + cond1_dup = self.resourceApi.build_condition( + cond_val=cond_val1, + cond_op=cond_op, + cond_member_type=cond_member_type, + cond_key=cond_key) + + union_cond_no_dup = self.resourceApi.build_union_condition( conditions=[cond1, cond2]) + union_cond_dup = self.resourceApi.build_union_condition( + conditions=[cond1, cond1_dup]) exp_cond1 = core_defs.Condition(value=cond_val1, key=cond_key, @@ -397,8 +405,10 @@ class TestPolicyGroup(NsxPolicyLibTestCase): member_type=cond_member_type) or_cond = core_defs.ConjunctionOperator( operator=constants.CONDITION_OP_OR) - exp_cond = [exp_cond1, or_cond, exp_cond2] - self._test_create_with_condition(union_cond, exp_cond) + exp_cond = list(set([exp_cond1, exp_cond2])) + exp_cond.insert(1, or_cond) + self._test_create_with_condition(union_cond_no_dup, exp_cond) + self._test_create_with_condition(union_cond_dup, [exp_cond1]) def test_create_with_nested_condition(self): cond_val1 = '123' @@ -429,10 +439,49 @@ class TestPolicyGroup(NsxPolicyLibTestCase): operator=cond_op, member_type=cond_member_type) and_cond = core_defs.ConjunctionOperator() - exp_cond = core_defs.NestedExpression( - expressions=[exp_cond1, and_cond, exp_cond2]) + expressions = list(set([exp_cond1, exp_cond2])) + expressions.insert(1, and_cond) + exp_cond = core_defs.NestedExpression(expressions=expressions) self._test_create_with_condition(nested, exp_cond) + def test_create_with_dup_nested_condition(self): + cond_val1 = '123' + cond_val2 = '456' + cond_op = constants.CONDITION_OP_EQUALS + cond_member_type = constants.CONDITION_MEMBER_VM + cond_key = constants.CONDITION_KEY_TAG + + cond1 = self.resourceApi.build_condition( + cond_val=cond_val1, + cond_op=cond_op, + cond_member_type=cond_member_type, + cond_key=cond_key) + cond2 = self.resourceApi.build_condition( + cond_val=cond_val2, + cond_op=cond_op, + cond_member_type=cond_member_type, + cond_key=cond_key) + nested = self.resourceApi.build_nested_condition( + conditions=[cond1, cond2]) + + cond1_dup = self.resourceApi.build_condition( + cond_val=cond_val1, + cond_op=cond_op, + cond_member_type=cond_member_type, + cond_key=cond_key) + cond2_dup = self.resourceApi.build_condition( + cond_val=cond_val2, + cond_op=cond_op, + cond_member_type=cond_member_type, + cond_key=cond_key) + nested_dup = self.resourceApi.build_nested_condition( + conditions=[cond1_dup, cond2_dup]) + + double_nested = self.resourceApi.build_nested_condition( + conditions=[nested, nested_dup]) + exp_cond = core_defs.NestedExpression(expressions=[nested]) + self._test_create_with_condition(double_nested, exp_cond) + def test_create_with_ip_expression(self): domain_id = '111' name = 'g1' diff --git a/vmware_nsxlib/v3/policy/core_defs.py b/vmware_nsxlib/v3/policy/core_defs.py index 68c56f56..7c6f0313 100644 --- a/vmware_nsxlib/v3/policy/core_defs.py +++ b/vmware_nsxlib/v3/policy/core_defs.py @@ -1485,6 +1485,14 @@ class Condition(object): 'value': self.value, 'operator': self.operator} + def __eq__(self, other): + if isinstance(other, Condition): + return self.get_obj_dict() == other.get_obj_dict() + return False + + def __hash__(self): + return hash(tuple(self.get_obj_dict().values())) + class IPAddressExpression(object): def __init__(self, ip_addresses): @@ -1521,6 +1529,46 @@ class NestedExpression(object): return {'resource_type': 'NestedExpression', 'expressions': [ex.get_obj_dict() for ex in self.expressions]} + def _get_unique_expressions(self): + """Only AND operator is supported in a nested expression. + + When comparing two nested expressions, only checking the Conditions + in the expression list will suffice since all ConjunctionOperators + will be the same. + """ + expr_list = self.get_obj_dict()['expressions'] + unique_exprs = [expr for expr in expr_list + if expr.get('resource_type') != 'ConjunctionOperator'] + return unique_exprs + + def __eq__(self, other): + + def expr_identifier(expr): + return expr.get('member_type'), expr.get('value') + + if not isinstance(other, NestedExpression): + return False + self_expr_list = self._get_unique_expressions() + other_expr_list = other._get_unique_expressions() + if len(self_expr_list) != len(other_expr_list): + return False + # Each expression dict in the list should be uniquely identified by + # (expr.member_type, expr.value pair). In case of segment expressions, + # the identifier will be of format ("Segment", "tag_scope|tag_value") + self_sorted = sorted(self_expr_list, key=expr_identifier) + other_sorted = sorted(other_expr_list, key=expr_identifier) + for i in range(len(self_sorted)): + if not all(item in other_sorted[i].items() + for item in self_sorted[i].items()): + return False + return True + + def __hash__(self): + # List is not hashable in python + value_list = [tuple(expr.values()) + for expr in self._get_unique_expressions()] + return hash(tuple(value_list)) + class GroupDef(ResourceDef): diff --git a/vmware_nsxlib/v3/policy/core_resources.py b/vmware_nsxlib/v3/policy/core_resources.py index b7503284..cf67d4f6 100644 --- a/vmware_nsxlib/v3/policy/core_resources.py +++ b/vmware_nsxlib/v3/policy/core_resources.py @@ -498,12 +498,19 @@ class NsxPolicyGroupApi(NsxPolicyResourceBase): def build_union_condition(self, operator=constants.CONDITION_OP_OR, conditions=None): + # NSX don't allow duplicate expressions in expression list + # of a group -> (ERROR: Duplicate expressions specified) + # Members of input conditions is either instance of Condition + # or NestedExpression class. expressions = [] - for cond in conditions: - if len(expressions): - expressions.append(core_defs.ConjunctionOperator( - operator=operator)) - expressions.append(cond) + if conditions: + conditions = list(set(conditions)) + expressions = [] + for cond in conditions: + if len(expressions): + expressions.append(core_defs.ConjunctionOperator( + operator=operator)) + expressions.append(cond) return expressions def build_nested_condition(