Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix rule order bug #967

Merged
merged 1 commit into from
Sep 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions nsxt/policy_common.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -469,6 +470,11 @@ func getPolicyRulesFromSchema(d *schema.ResourceData) []model.Rule {
}

resourceType := "Rule"
if sequenceNumber == 0 {
sequenceNumber = lastSequence + 1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to set sequence numbers with some gap between them? e.g 10, 20... etc so later a rule can be inserted between without recreating the whole list?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From terraform perspective the list is ordered, which means if rule 3.5 is inserted between rule 3 and rule 4, this will result in diff for rule 4, and a new rule 5. As a result, we will update all rules starting from the inserted rule, hence we don't need to bother about gaps in sequence numbers.

}
lastSequence = sequenceNumber

elem := model.Rule{
ResourceType: &resourceType,
Id: &id,
Expand All @@ -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)
Expand Down
13 changes: 8 additions & 5 deletions nsxt/resource_nsxt_policy_gateway_policy.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
14 changes: 10 additions & 4 deletions nsxt/resource_nsxt_policy_security_policy.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand All @@ -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)
}

Expand All @@ -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)
Expand Down Expand Up @@ -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)
}
Expand Down
20 changes: 11 additions & 9 deletions nsxt/resource_nsxt_policy_security_policy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down