From ce3b6efb07a3978d298d441258cfdfac86e16687 Mon Sep 17 00:00:00 2001 From: Anna Khmelnitsky Date: Fri, 8 Sep 2023 16:59:35 +0000 Subject: [PATCH] Fix rule order bug NSX can not guarantee rule order unless sequence_number is provided, or `revise` API is called. This change auto-assigns sequence number to all rules in list, for which it was not provided by the user. It takes care of provider upgrade by making sure rules without assigned sequence number are updated even if terraform does not detect a change in them. Signed-off-by: Anna Khmelnitsky --- nsxt/policy_common.go | 11 ++++++---- nsxt/resource_nsxt_policy_gateway_policy.go | 13 +++++++----- nsxt/resource_nsxt_policy_security_policy.go | 14 +++++++++---- ...source_nsxt_policy_security_policy_test.go | 20 ++++++++++--------- 4 files changed, 36 insertions(+), 22 deletions(-) diff --git a/nsxt/policy_common.go b/nsxt/policy_common.go index cae89697b..94eed9e11 100644 --- a/nsxt/policy_common.go +++ b/nsxt/policy_common.go @@ -440,6 +440,7 @@ func validatePolicyRuleSequence(d *schema.ResourceData) error { func getPolicyRulesFromSchema(d *schema.ResourceData) []model.Rule { rules := d.Get("rule").([]interface{}) var ruleList []model.Rule + lastSequence := int64(0) for _, rule := range rules { data := rule.(map[string]interface{}) displayName := data["display_name"].(string) @@ -469,6 +470,11 @@ func getPolicyRulesFromSchema(d *schema.ResourceData) []model.Rule { } resourceType := "Rule" + if sequenceNumber == 0 { + sequenceNumber = lastSequence + 1 + } + lastSequence = sequenceNumber + elem := model.Rule{ ResourceType: &resourceType, Id: &id, @@ -489,10 +495,7 @@ func getPolicyRulesFromSchema(d *schema.ResourceData) []model.Rule { Services: getPathListFromMap(data, "services"), Scope: getPathListFromMap(data, "scope"), Profiles: getPathListFromMap(data, "profiles"), - } - - if sequenceNumber > 0 { - elem.SequenceNumber = &sequenceNumber + SequenceNumber: &sequenceNumber, } ruleList = append(ruleList, elem) diff --git a/nsxt/resource_nsxt_policy_gateway_policy.go b/nsxt/resource_nsxt_policy_gateway_policy.go index 9b9adfee2..454f09480 100644 --- a/nsxt/resource_nsxt_policy_gateway_policy.go +++ b/nsxt/resource_nsxt_policy_gateway_policy.go @@ -64,16 +64,19 @@ func getUpdatedRuleChildren(d *schema.ResourceData) ([]*data.StructValue, error) } oldRules, newRules := d.GetChange("rule") - err1 := validatePolicyRuleSequence(d) - if err1 != nil { - return policyChildren, err1 - } rules := getPolicyRulesFromSchema(d) newRulesCount := len(newRules.([]interface{})) oldRulesCount := len(oldRules.([]interface{})) for ruleNo := 0; ruleNo < newRulesCount; ruleNo++ { ruleIndicator := fmt.Sprintf("rule.%d", ruleNo) - if d.HasChange(ruleIndicator) { + autoAssignedSequence := false + originalSequence := d.Get(fmt.Sprintf("%s.sequence_number", ruleIndicator)).(int) + if originalSequence == 0 { + autoAssignedSequence = true + } + if d.HasChange(ruleIndicator) || autoAssignedSequence { + // If the provider assigned sequence number to this rule, we need to update it even + // though terraform sees no diff rule := rules[ruleNo] // New or updated rule ruleID := newUUID() diff --git a/nsxt/resource_nsxt_policy_security_policy.go b/nsxt/resource_nsxt_policy_security_policy.go index bb969f3fc..b88a145e2 100644 --- a/nsxt/resource_nsxt_policy_security_policy.go +++ b/nsxt/resource_nsxt_policy_security_policy.go @@ -55,7 +55,7 @@ func resourceNsxtPolicySecurityPolicyExistsPartial(domainName string) func(sessi } } -func policySecurityPolicyBuildAndPatch(d *schema.ResourceData, m interface{}, connector client.Connector, isGlobalManager bool, id string) error { +func policySecurityPolicyBuildAndPatch(d *schema.ResourceData, m interface{}, connector client.Connector, isGlobalManager bool, id string, createFlow bool) error { domain := d.Get("domain").(string) displayName := d.Get("display_name").(string) @@ -87,11 +87,16 @@ func policySecurityPolicyBuildAndPatch(d *schema.ResourceData, m interface{}, co } log.Printf("[INFO] Creating Security Policy with ID %s", id) - if len(d.Id()) > 0 { + if !createFlow { // This is update flow obj.Revision = &revision } + err := validatePolicyRuleSequence(d) + if err != nil { + return err + } + policyChildren, err := getUpdatedRuleChildren(d) if err != nil { return err @@ -100,6 +105,7 @@ func policySecurityPolicyBuildAndPatch(d *schema.ResourceData, m interface{}, co obj.Children = policyChildren } + log.Printf("[INFO] Using selective H-API for policy with ID %s", id) return securityPolicyInfraPatch(getSessionContext(d, m), obj, domain, m) } @@ -112,7 +118,7 @@ func resourceNsxtPolicySecurityPolicyCreate(d *schema.ResourceData, m interface{ return err } - err = policySecurityPolicyBuildAndPatch(d, m, connector, isPolicyGlobalManager(m), id) + err = policySecurityPolicyBuildAndPatch(d, m, connector, isPolicyGlobalManager(m), id, true) if err != nil { return handleCreateError("Security Policy", id, err) @@ -164,7 +170,7 @@ func resourceNsxtPolicySecurityPolicyUpdate(d *schema.ResourceData, m interface{ if id == "" { return fmt.Errorf("Error obtaining Security Policy id") } - err := policySecurityPolicyBuildAndPatch(d, m, connector, isPolicyGlobalManager(m), id) + err := policySecurityPolicyBuildAndPatch(d, m, connector, isPolicyGlobalManager(m), id, false) if err != nil { return handleUpdateError("Security Policy", id, err) } diff --git a/nsxt/resource_nsxt_policy_security_policy_test.go b/nsxt/resource_nsxt_policy_security_policy_test.go index 445ac0f10..f756cff22 100644 --- a/nsxt/resource_nsxt_policy_security_policy_test.go +++ b/nsxt/resource_nsxt_policy_security_policy_test.go @@ -952,38 +952,40 @@ func testAccNsxtPolicySecurityPolicyWithIPCidrRange(name string, destIP string, destination_groups = ["%s"] services = [nsxt_policy_service.icmp.path] action = "ALLOW" - } + } - rule { + rule { display_name = "rule3" source_groups = [nsxt_policy_group.group1.path] destination_groups = ["%s"] services = [nsxt_policy_service.icmp.path] action = "ALLOW" - } - rule { + sequence_number = 50 + } + rule { display_name = "rule4" source_groups = ["%s"] destination_groups = [nsxt_policy_group.group2.path] services = [nsxt_policy_service.icmp.path] action = "ALLOW" - } + } - rule { + rule { display_name = "rule5" source_groups = ["%s"] destination_groups = [nsxt_policy_group.group2.path] services = [nsxt_policy_service.icmp.path] action = "ALLOW" - } + sequence_number = 105 + } - rule { + rule { display_name = "rule6" source_groups = ["%s"] destination_groups = [nsxt_policy_group.group2.path] services = [nsxt_policy_service.icmp.path] action = "ALLOW" - } + } }`, name, destIP, destCidr, destIPRange, sourceIP, sourceCidr, sourceIPRange) }