From eab9f72fa58f113ffb6c238175c9fd7fee101a28 Mon Sep 17 00:00:00 2001
From: Anna Khmelnitsky <akhmelnitsky@vmware.com>
Date: Mon, 16 Oct 2023 22:35:17 +0000
Subject: [PATCH] Validate rule sequence numbers on create only

Since sequence number is Computed in rule, bad sequence numbers
may be stuck in state as a result of configurations with previous
provider version.
Rather than throwing an error, the provider will auto-correct the
sequence number while also logging this event.
For create flow, we leave sequence number validation since state
for new object is clean.

Signed-off-by: Anna Khmelnitsky <akhmelnitsky@vmware.com>
---
 nsxt/policy_common.go                                | 12 ++++++++++--
 ...resource_nsxt_policy_predefined_gateway_policy.go |  4 ----
 ...esource_nsxt_policy_predefined_security_policy.go |  4 ----
 nsxt/resource_nsxt_policy_security_policy.go         | 11 +++++------
 4 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/nsxt/policy_common.go b/nsxt/policy_common.go
index 94eed9e11..4e22f113b 100644
--- a/nsxt/policy_common.go
+++ b/nsxt/policy_common.go
@@ -2,6 +2,7 @@ package nsxt
 
 import (
 	"fmt"
+	"log"
 	"strings"
 
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
@@ -423,8 +424,9 @@ func validatePolicyRuleSequence(d *schema.ResourceData) error {
 	for _, rule := range rules {
 		data := rule.(map[string]interface{})
 		sequenceNumber := int64(data["sequence_number"].(int))
+		displayName := data["display_name"].(string)
 		if sequenceNumber > 0 && sequenceNumber <= latestNum {
-			return fmt.Errorf("when sequence_number is specified in a rule, it must be consistent with rule order. To avoid confusion, it is recommended to either specify sequence numbers in all rules, or none")
+			return fmt.Errorf("when sequence_number is specified in a rule, it must be consistent with rule order. To avoid confusion, it is recommended to either specify sequence numbers in all rules, or none. Error detected with rule %s: %v <= %v", displayName, sequenceNumber, latestNum)
 		}
 
 		if sequenceNumber == 0 {
@@ -470,7 +472,13 @@ func getPolicyRulesFromSchema(d *schema.ResourceData) []model.Rule {
 		}
 
 		resourceType := "Rule"
-		if sequenceNumber == 0 {
+		if sequenceNumber == 0 || sequenceNumber <= lastSequence {
+			// We overwrite sequence number in case its not specified,
+			// or out of order, which might be due to provider upgrade
+			// or bad user configuration
+			if sequenceNumber <= lastSequence {
+				log.Printf("[WARNING] Sequence_number %v for rule %s is out of order - overriding with sequence number %v", sequenceNumber, displayName, lastSequence+1)
+			}
 			sequenceNumber = lastSequence + 1
 		}
 		lastSequence = sequenceNumber
diff --git a/nsxt/resource_nsxt_policy_predefined_gateway_policy.go b/nsxt/resource_nsxt_policy_predefined_gateway_policy.go
index ed7a035be..717dbcf5c 100644
--- a/nsxt/resource_nsxt_policy_predefined_gateway_policy.go
+++ b/nsxt/resource_nsxt_policy_predefined_gateway_policy.go
@@ -314,10 +314,6 @@ func updatePolicyPredefinedGatewayPolicy(id string, d *schema.ResourceData, m in
 	var childRules []*data.StructValue
 	if d.HasChange("rule") {
 		oldRules, _ := d.GetChange("rule")
-		err1 := validatePolicyRuleSequence(d)
-		if err1 != nil {
-			return err1
-		}
 		rules := getPolicyRulesFromSchema(d)
 
 		existingRules := make(map[string]bool)
diff --git a/nsxt/resource_nsxt_policy_predefined_security_policy.go b/nsxt/resource_nsxt_policy_predefined_security_policy.go
index 530bc4175..42025ada0 100644
--- a/nsxt/resource_nsxt_policy_predefined_security_policy.go
+++ b/nsxt/resource_nsxt_policy_predefined_security_policy.go
@@ -236,10 +236,6 @@ func updatePolicyPredefinedSecurityPolicy(id string, d *schema.ResourceData, m i
 	var childRules []*data.StructValue
 	if d.HasChange("rule") {
 		oldRules, _ := d.GetChange("rule")
-		err1 := validatePolicyRuleSequence(d)
-		if err1 != nil {
-			return err1
-		}
 		rules := getPolicyRulesFromSchema(d)
 
 		existingRules := make(map[string]bool)
diff --git a/nsxt/resource_nsxt_policy_security_policy.go b/nsxt/resource_nsxt_policy_security_policy.go
index b88a145e2..1ca398f6b 100644
--- a/nsxt/resource_nsxt_policy_security_policy.go
+++ b/nsxt/resource_nsxt_policy_security_policy.go
@@ -87,16 +87,15 @@ func policySecurityPolicyBuildAndPatch(d *schema.ResourceData, m interface{}, co
 	}
 	log.Printf("[INFO] Creating Security Policy with ID %s", id)
 
-	if !createFlow {
+	if createFlow {
+		if err := validatePolicyRuleSequence(d); err != nil {
+			return err
+		}
+	} else {
 		// This is update flow
 		obj.Revision = &revision
 	}
 
-	err := validatePolicyRuleSequence(d)
-	if err != nil {
-		return err
-	}
-
 	policyChildren, err := getUpdatedRuleChildren(d)
 	if err != nil {
 		return err