Skip to content

Commit b65fe7c

Browse files
bhouse-nexthopbhouse-nexthop
committed
Fix TypeSet hash to prevent spurious diffs on unchanged rules
The rule TypeSet was using the default hash function which includes ALL fields, including the computed 'uuid' field. This caused Terraform to show spurious diffs where unchanged rules appeared as being removed and re-added when other rules in the same ruleset were modified. Root Cause: - When rule A is updated (e.g., action changes), its UUID may change - The TypeSet hash included the UUID, so the set's internal representation changed even for unchanged rules - Terraform interpreted this as rules being removed and re-added, even though only their position in the set changed Solution: - Added custom hash function resourceCloudStackNetworkACLRulesetRuleHash - Hash is based only on user-configurable fields that define rule identity: rule_number, action, protocol, traffic_type, cidr_list, port, icmp_type, icmp_code, description - Explicitly excludes the computed 'uuid' field - This ensures rules are identified by their configuration, not by their API-assigned UUID Test: Added TestAccCloudStackNetworkACLRuleset_unchanged_rule_stability which verifies that when one rule is modified, other unchanged rules maintain stable UUIDs and don't show spurious diffs. All 12 acceptance tests passing (153.2 seconds total)
1 parent 06543e2 commit b65fe7c

2 files changed

Lines changed: 181 additions & 0 deletions

File tree

‎cloudstack/resource_cloudstack_network_acl_ruleset.go‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@ func resourceCloudStackNetworkACLRuleset() *schema.Resource {
6464
"rule": {
6565
Type: schema.TypeSet,
6666
Required: true,
67+
Set: resourceCloudStackNetworkACLRulesetRuleHash,
6768
Elem: &schema.Resource{
6869
Schema: map[string]*schema.Schema{
6970
"rule_number": {
@@ -128,6 +129,49 @@ func resourceCloudStackNetworkACLRuleset() *schema.Resource {
128129
}
129130
}
130131

132+
// resourceCloudStackNetworkACLRulesetRuleHash computes a hash for a rule based on
133+
// its identifying attributes, excluding the computed uuid field. This ensures that
134+
// rules are identified by their configuration, not by their API-assigned UUID.
135+
func resourceCloudStackNetworkACLRulesetRuleHash(v interface{}) int {
136+
var buf strings.Builder
137+
m := v.(map[string]interface{})
138+
139+
// Include all fields that define the rule's identity from the user's perspective
140+
// Explicitly exclude "uuid" which is computed and should not affect the hash
141+
142+
buf.WriteString(fmt.Sprintf("%d-", m["rule_number"].(int)))
143+
buf.WriteString(fmt.Sprintf("%s-", m["action"].(string)))
144+
buf.WriteString(fmt.Sprintf("%s-", m["protocol"].(string)))
145+
buf.WriteString(fmt.Sprintf("%s-", m["traffic_type"].(string)))
146+
147+
// Hash the cidr_list set
148+
if v, ok := m["cidr_list"]; ok {
149+
cidrSet := v.(*schema.Set)
150+
for _, cidr := range cidrSet.List() {
151+
buf.WriteString(fmt.Sprintf("%s-", cidr.(string)))
152+
}
153+
}
154+
155+
// Include optional fields
156+
if v, ok := m["port"]; ok && v.(string) != "" {
157+
buf.WriteString(fmt.Sprintf("port:%s-", v.(string)))
158+
}
159+
160+
if v, ok := m["icmp_type"]; ok && v.(int) != 0 {
161+
buf.WriteString(fmt.Sprintf("icmp_type:%d-", v.(int)))
162+
}
163+
164+
if v, ok := m["icmp_code"]; ok && v.(int) != 0 {
165+
buf.WriteString(fmt.Sprintf("icmp_code:%d-", v.(int)))
166+
}
167+
168+
if v, ok := m["description"]; ok && v.(string) != "" {
169+
buf.WriteString(fmt.Sprintf("desc:%s-", v.(string)))
170+
}
171+
172+
return schema.HashString(buf.String())
173+
}
174+
131175
func resourceCloudStackNetworkACLRulesetCreate(d *schema.ResourceData, meta interface{}) error {
132176
// We need to set this upfront in order to be able to save a partial state
133177
d.SetId(d.Get("acl_id").(string))

‎cloudstack/resource_cloudstack_network_acl_ruleset_test.go‎

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1560,6 +1560,143 @@ resource "cloudstack_network_acl_ruleset" "numeric_test" {
15601560
}
15611561
`
15621562

1563+
func TestAccCloudStackNetworkACLRuleset_unchanged_rule_stability(t *testing.T) {
1564+
var rule42002UUID string
1565+
1566+
resource.Test(t, resource.TestCase{
1567+
PreCheck: func() { testAccPreCheck(t) },
1568+
Providers: testAccProviders,
1569+
CheckDestroy: testAccCheckCloudStackNetworkACLRulesetDestroy,
1570+
Steps: []resource.TestStep{
1571+
{
1572+
Config: testAccCloudStackNetworkACLRuleset_unchanged_rule_stability_step1,
1573+
Check: resource.ComposeTestCheckFunc(
1574+
testAccCheckCloudStackNetworkACLRulesetExists("cloudstack_network_acl_ruleset.stability_test"),
1575+
resource.TestCheckResourceAttr("cloudstack_network_acl_ruleset.stability_test", "rule.#", "2"),
1576+
// Capture the UUID of rule 42002 (the unchanged rule)
1577+
func(s *terraform.State) error {
1578+
rs := s.RootModule().Resources["cloudstack_network_acl_ruleset.stability_test"]
1579+
for k, v := range rs.Primary.Attributes {
1580+
// Find rule 42002's UUID
1581+
if strings.Contains(k, "rule.") && strings.Contains(k, ".rule_number") && v == "42002" {
1582+
// Extract the index and get the UUID
1583+
parts := strings.Split(k, ".")
1584+
if len(parts) >= 2 {
1585+
uuidKey := fmt.Sprintf("%s.%s.uuid", parts[0], parts[1])
1586+
rule42002UUID = rs.Primary.Attributes[uuidKey]
1587+
t.Logf("Rule 42002 UUID in step 1: %s", rule42002UUID)
1588+
}
1589+
}
1590+
}
1591+
return nil
1592+
},
1593+
),
1594+
},
1595+
{
1596+
Config: testAccCloudStackNetworkACLRuleset_unchanged_rule_stability_step2,
1597+
Check: resource.ComposeTestCheckFunc(
1598+
testAccCheckCloudStackNetworkACLRulesetExists("cloudstack_network_acl_ruleset.stability_test"),
1599+
resource.TestCheckResourceAttr("cloudstack_network_acl_ruleset.stability_test", "rule.#", "2"),
1600+
// Verify that rule 42002's UUID hasn't changed (it shouldn't since the rule is unchanged)
1601+
func(s *terraform.State) error {
1602+
rs := s.RootModule().Resources["cloudstack_network_acl_ruleset.stability_test"]
1603+
var currentUUID string
1604+
for k, v := range rs.Primary.Attributes {
1605+
if strings.Contains(k, "rule.") && strings.Contains(k, ".rule_number") && v == "42002" {
1606+
parts := strings.Split(k, ".")
1607+
if len(parts) >= 2 {
1608+
uuidKey := fmt.Sprintf("%s.%s.uuid", parts[0], parts[1])
1609+
currentUUID = rs.Primary.Attributes[uuidKey]
1610+
t.Logf("Rule 42002 UUID in step 2: %s", currentUUID)
1611+
}
1612+
}
1613+
}
1614+
if currentUUID != rule42002UUID {
1615+
return fmt.Errorf("Rule 42002 UUID changed from %s to %s, but the rule was unchanged!", rule42002UUID, currentUUID)
1616+
}
1617+
t.Logf("✓ Rule 42002 UUID remained stable: %s", currentUUID)
1618+
return nil
1619+
},
1620+
),
1621+
},
1622+
},
1623+
})
1624+
}
1625+
1626+
const testAccCloudStackNetworkACLRuleset_unchanged_rule_stability_step1 = `
1627+
resource "cloudstack_vpc" "stability_test" {
1628+
name = "terraform-vpc-stability-test"
1629+
cidr = "10.0.0.0/8"
1630+
vpc_offering = "Default VPC offering"
1631+
zone = "Sandbox-simulator"
1632+
}
1633+
1634+
resource "cloudstack_network_acl" "stability_test" {
1635+
name = "terraform-acl-stability-test"
1636+
description = "terraform-acl-stability-test-text"
1637+
vpc_id = cloudstack_vpc.stability_test.id
1638+
}
1639+
1640+
resource "cloudstack_network_acl_ruleset" "stability_test" {
1641+
acl_id = cloudstack_network_acl.stability_test.id
1642+
1643+
rule {
1644+
rule_number = 42001
1645+
action = "allow"
1646+
cidr_list = ["0.0.0.0/0"]
1647+
protocol = "all"
1648+
traffic_type = "egress"
1649+
description = "to any vpc: allow egress"
1650+
}
1651+
1652+
rule {
1653+
rule_number = 42002
1654+
action = "allow"
1655+
cidr_list = ["0.0.0.0/0"]
1656+
protocol = "all"
1657+
traffic_type = "ingress"
1658+
description = "from anywhere: allow ingress"
1659+
}
1660+
}
1661+
`
1662+
1663+
const testAccCloudStackNetworkACLRuleset_unchanged_rule_stability_step2 = `
1664+
resource "cloudstack_vpc" "stability_test" {
1665+
name = "terraform-vpc-stability-test"
1666+
cidr = "10.0.0.0/8"
1667+
vpc_offering = "Default VPC offering"
1668+
zone = "Sandbox-simulator"
1669+
}
1670+
1671+
resource "cloudstack_network_acl" "stability_test" {
1672+
name = "terraform-acl-stability-test"
1673+
description = "terraform-acl-stability-test-text"
1674+
vpc_id = cloudstack_vpc.stability_test.id
1675+
}
1676+
1677+
resource "cloudstack_network_acl_ruleset" "stability_test" {
1678+
acl_id = cloudstack_network_acl.stability_test.id
1679+
1680+
rule {
1681+
rule_number = 42001
1682+
action = "deny"
1683+
cidr_list = ["0.0.0.0/0"]
1684+
protocol = "all"
1685+
traffic_type = "egress"
1686+
description = "to any vpc: deny egress"
1687+
}
1688+
1689+
rule {
1690+
rule_number = 42002
1691+
action = "allow"
1692+
cidr_list = ["0.0.0.0/0"]
1693+
protocol = "all"
1694+
traffic_type = "ingress"
1695+
description = "from anywhere: allow ingress"
1696+
}
1697+
}
1698+
`
1699+
15631700
func TestAccCloudStackNetworkACLRuleset_remove(t *testing.T) {
15641701
resource.Test(t, resource.TestCase{
15651702
PreCheck: func() { testAccPreCheck(t) },

0 commit comments

Comments
 (0)