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 <[email protected]>
vmware · Sep 13, 2023 · ce3b6ef · ce3b6ef
1 parent 0555997
commit ce3b6ef
Show file tree

Hide file tree

Showing 4 changed files with 36 additions and 22 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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)
 }