Skip to content

Commit ec063c3

Browse files
bhouse-nexthopbhouse-nexthop
committed
Eliminate spurious diffs in ACL ruleset when modifying individual rules
When modifying a single rule in a ruleset, Terraform's TypeSet would show all rules as being replaced due to hash changes, even though only one rule actually changed. This created confusing diffs that made it hard to see what was actually being modified. This commit implements a CustomizeDiff function that: - Compares old and new rule sets by matching on rule_number (the natural key) - Performs field-by-field comparison of each rule - Suppresses the diff if rules are functionally identical (same rule_number with same values) - Only shows diffs for rules that actually changed Key changes: - Added CustomizeDiff to resource schema - Changed 'rule' attribute from Required to Optional+Computed (required for SetNew to work) - Added resourceCloudStackNetworkACLRulesetCustomizeDiff function - Added compareACLRules helper for deep field comparison - Added TestAccCloudStackNetworkACLRuleset_no_spurious_diff to verify the fix All existing tests continue to pass.
1 parent cad7264 commit ec063c3

2 files changed

Lines changed: 241 additions & 1 deletion

File tree

‎cloudstack/resource_cloudstack_network_acl_ruleset.go‎

Lines changed: 100 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
package cloudstack
2121

2222
import (
23+
"context"
2324
"fmt"
2425
"log"
2526
"strconv"
@@ -41,6 +42,7 @@ func resourceCloudStackNetworkACLRuleset() *schema.Resource {
4142
Importer: &schema.ResourceImporter{
4243
State: resourceCloudStackNetworkACLRulesetImport,
4344
},
45+
CustomizeDiff: resourceCloudStackNetworkACLRulesetCustomizeDiff,
4446

4547
Schema: map[string]*schema.Schema{
4648
"acl_id": {
@@ -63,7 +65,8 @@ func resourceCloudStackNetworkACLRuleset() *schema.Resource {
6365

6466
"rule": {
6567
Type: schema.TypeSet,
66-
Required: true,
68+
Optional: true,
69+
Computed: true,
6770
Elem: &schema.Resource{
6871
Schema: map[string]*schema.Schema{
6972
"rule_number": {
@@ -128,6 +131,102 @@ func resourceCloudStackNetworkACLRuleset() *schema.Resource {
128131
}
129132
}
130133

134+
func resourceCloudStackNetworkACLRulesetCustomizeDiff(ctx context.Context, d *schema.ResourceDiff, meta interface{}) error {
135+
// Only apply this logic during updates, not creates
136+
if d.Id() == "" {
137+
return nil
138+
}
139+
140+
old, new := d.GetChange("rule")
141+
oldSet := old.(*schema.Set)
142+
newSet := new.(*schema.Set)
143+
144+
// If the sets are empty or have different lengths, there's a real change
145+
if oldSet.Len() == 0 || newSet.Len() == 0 || oldSet.Len() != newSet.Len() {
146+
return nil
147+
}
148+
149+
// Create maps indexed by rule_number (the natural key)
150+
oldMap := make(map[int]map[string]interface{})
151+
for _, v := range oldSet.List() {
152+
m := v.(map[string]interface{})
153+
ruleNum := m["rule_number"].(int)
154+
oldMap[ruleNum] = m
155+
}
156+
157+
newMap := make(map[int]map[string]interface{})
158+
for _, v := range newSet.List() {
159+
m := v.(map[string]interface{})
160+
ruleNum := m["rule_number"].(int)
161+
newMap[ruleNum] = m
162+
}
163+
164+
// Check if the rules are functionally identical when matched by rule_number
165+
functionallyIdentical := true
166+
for ruleNum, newRule := range newMap {
167+
oldRule, exists := oldMap[ruleNum]
168+
if !exists {
169+
// New rule added
170+
functionallyIdentical = false
171+
break
172+
}
173+
174+
// Compare all fields except uuid
175+
if !compareACLRules(oldRule, newRule) {
176+
functionallyIdentical = false
177+
break
178+
}
179+
}
180+
181+
// Also check if any rules were removed
182+
for ruleNum := range oldMap {
183+
if _, exists := newMap[ruleNum]; !exists {
184+
functionallyIdentical = false
185+
break
186+
}
187+
}
188+
189+
// If the rules are functionally identical (same rule_numbers with same values),
190+
// suppress the diff by keeping the old state
191+
if functionallyIdentical {
192+
return d.SetNew("rule", old)
193+
}
194+
195+
return nil
196+
}
197+
198+
func compareACLRules(old, new map[string]interface{}) bool {
199+
// Compare all fields except uuid (which is computed and may differ)
200+
fields := []string{"rule_number", "action", "protocol", "icmp_type", "icmp_code", "port", "traffic_type", "description"}
201+
202+
for _, field := range fields {
203+
oldVal := old[field]
204+
newVal := new[field]
205+
206+
// Handle nil/empty string equivalence
207+
if oldVal == nil && newVal == "" {
208+
continue
209+
}
210+
if oldVal == "" && newVal == nil {
211+
continue
212+
}
213+
214+
if oldVal != newVal {
215+
return false
216+
}
217+
}
218+
219+
// Compare cidr_list (TypeSet)
220+
oldCIDR := old["cidr_list"].(*schema.Set)
221+
newCIDR := new["cidr_list"].(*schema.Set)
222+
223+
if !oldCIDR.Equal(newCIDR) {
224+
return false
225+
}
226+
227+
return true
228+
}
229+
131230
func resourceCloudStackNetworkACLRulesetCreate(d *schema.ResourceData, meta interface{}) error {
132231
// We need to set this upfront in order to be able to save a partial state
133232
d.SetId(d.Get("acl_id").(string))

‎cloudstack/resource_cloudstack_network_acl_ruleset_test.go‎

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1520,6 +1520,147 @@ resource "cloudstack_network_acl_ruleset" "protocol_test" {
15201520
}
15211521
`
15221522

1523+
func TestAccCloudStackNetworkACLRuleset_no_spurious_diff(t *testing.T) {
1524+
resource.Test(t, resource.TestCase{
1525+
PreCheck: func() { testAccPreCheck(t) },
1526+
Providers: testAccProviders,
1527+
CheckDestroy: testAccCheckCloudStackNetworkACLRulesetDestroy,
1528+
Steps: []resource.TestStep{
1529+
{
1530+
Config: testAccCloudStackNetworkACLRuleset_no_spurious_diff_initial,
1531+
Check: resource.ComposeTestCheckFunc(
1532+
testAccCheckCloudStackNetworkACLRulesetExists("cloudstack_network_acl_ruleset.no_spurious_diff"),
1533+
resource.TestCheckResourceAttr(
1534+
"cloudstack_network_acl_ruleset.no_spurious_diff", "rule.#", "3"),
1535+
),
1536+
},
1537+
{
1538+
Config: testAccCloudStackNetworkACLRuleset_no_spurious_diff_change_one_rule,
1539+
Check: resource.ComposeTestCheckFunc(
1540+
testAccCheckCloudStackNetworkACLRulesetExists("cloudstack_network_acl_ruleset.no_spurious_diff"),
1541+
resource.TestCheckResourceAttr(
1542+
"cloudstack_network_acl_ruleset.no_spurious_diff", "rule.#", "3"),
1543+
// Verify rule 20 was updated
1544+
resource.TestCheckTypeSetElemNestedAttrs(
1545+
"cloudstack_network_acl_ruleset.no_spurious_diff", "rule.*", map[string]string{
1546+
"rule_number": "20",
1547+
"port": "8080", // Changed from 80
1548+
}),
1549+
// Verify rules 10 and 30 still exist with their original values
1550+
resource.TestCheckTypeSetElemNestedAttrs(
1551+
"cloudstack_network_acl_ruleset.no_spurious_diff", "rule.*", map[string]string{
1552+
"rule_number": "10",
1553+
"port": "22",
1554+
}),
1555+
resource.TestCheckTypeSetElemNestedAttrs(
1556+
"cloudstack_network_acl_ruleset.no_spurious_diff", "rule.*", map[string]string{
1557+
"rule_number": "30",
1558+
"port": "443",
1559+
}),
1560+
),
1561+
},
1562+
},
1563+
})
1564+
}
1565+
1566+
const testAccCloudStackNetworkACLRuleset_no_spurious_diff_initial = `
1567+
resource "cloudstack_vpc" "no_spurious_diff" {
1568+
name = "terraform-vpc-no-spurious-diff"
1569+
cidr = "10.0.0.0/8"
1570+
vpc_offering = "Default VPC offering"
1571+
zone = "Sandbox-simulator"
1572+
}
1573+
1574+
resource "cloudstack_network_acl" "no_spurious_diff" {
1575+
name = "terraform-acl-no-spurious-diff"
1576+
description = "terraform-acl-no-spurious-diff-text"
1577+
vpc_id = cloudstack_vpc.no_spurious_diff.id
1578+
}
1579+
1580+
resource "cloudstack_network_acl_ruleset" "no_spurious_diff" {
1581+
acl_id = cloudstack_network_acl.no_spurious_diff.id
1582+
1583+
rule {
1584+
rule_number = 10
1585+
action = "allow"
1586+
cidr_list = ["0.0.0.0/0"]
1587+
protocol = "tcp"
1588+
port = "22"
1589+
traffic_type = "ingress"
1590+
description = "SSH"
1591+
}
1592+
1593+
rule {
1594+
rule_number = 20
1595+
action = "allow"
1596+
cidr_list = ["0.0.0.0/0"]
1597+
protocol = "tcp"
1598+
port = "80"
1599+
traffic_type = "ingress"
1600+
description = "HTTP"
1601+
}
1602+
1603+
rule {
1604+
rule_number = 30
1605+
action = "allow"
1606+
cidr_list = ["0.0.0.0/0"]
1607+
protocol = "tcp"
1608+
port = "443"
1609+
traffic_type = "ingress"
1610+
description = "HTTPS"
1611+
}
1612+
}
1613+
`
1614+
1615+
const testAccCloudStackNetworkACLRuleset_no_spurious_diff_change_one_rule = `
1616+
resource "cloudstack_vpc" "no_spurious_diff" {
1617+
name = "terraform-vpc-no-spurious-diff"
1618+
cidr = "10.0.0.0/8"
1619+
vpc_offering = "Default VPC offering"
1620+
zone = "Sandbox-simulator"
1621+
}
1622+
1623+
resource "cloudstack_network_acl" "no_spurious_diff" {
1624+
name = "terraform-acl-no-spurious-diff"
1625+
description = "terraform-acl-no-spurious-diff-text"
1626+
vpc_id = cloudstack_vpc.no_spurious_diff.id
1627+
}
1628+
1629+
resource "cloudstack_network_acl_ruleset" "no_spurious_diff" {
1630+
acl_id = cloudstack_network_acl.no_spurious_diff.id
1631+
1632+
rule {
1633+
rule_number = 10
1634+
action = "allow"
1635+
cidr_list = ["0.0.0.0/0"]
1636+
protocol = "tcp"
1637+
port = "22"
1638+
traffic_type = "ingress"
1639+
description = "SSH"
1640+
}
1641+
1642+
rule {
1643+
rule_number = 20
1644+
action = "allow"
1645+
cidr_list = ["0.0.0.0/0"]
1646+
protocol = "tcp"
1647+
port = "8080" # Changed from 80
1648+
traffic_type = "ingress"
1649+
description = "HTTP"
1650+
}
1651+
1652+
rule {
1653+
rule_number = 30
1654+
action = "allow"
1655+
cidr_list = ["0.0.0.0/0"]
1656+
protocol = "tcp"
1657+
port = "443"
1658+
traffic_type = "ingress"
1659+
description = "HTTPS"
1660+
}
1661+
}
1662+
`
1663+
15231664
func TestAccCloudStackNetworkACLRuleset_import(t *testing.T) {
15241665
resource.Test(t, resource.TestCase{
15251666
PreCheck: func() { testAccPreCheck(t) },

0 commit comments

Comments
 (0)