Copilot commented on code in PR #281:
URL:
https://github.com/apache/cloudstack-terraform-provider/pull/281#discussion_r2910363620
##########
cloudstack/resource_cloudstack_network_acl_rule.go:
##########
@@ -596,87 +843,108 @@ func resourceCloudStackNetworkACLRuleRead(d
*schema.ResourceData, meta interface
// Create an empty rule list to hold all rules
var rules []interface{}
- // Read all rules that are configured
- if rs := d.Get("rule").([]interface{}); len(rs) > 0 {
- for _, rule := range rs {
- rule := rule.(map[string]interface{})
- uuids := rule["uuids"].(map[string]interface{})
- log.Printf("[DEBUG] Processing rule with protocol=%s,
uuids=%+v", rule["protocol"].(string), uuids)
-
- if rule["protocol"].(string) == "icmp" {
- id, ok := uuids["icmp"]
- if !ok {
- log.Printf("[DEBUG] No ICMP UUID found,
skipping rule")
- continue
- }
+ // Determine which field is being used and get the rules list
+ var configuredRules []interface{}
+ usingRuleset := false
+ if rs := d.Get("ruleset").(*schema.Set); rs != nil && rs.Len() > 0 {
+ usingRuleset = true
+ configuredRules = rs.List()
+ } else if rs := d.Get("rule").([]interface{}); len(rs) > 0 {
+ configuredRules = rs
+ }
Review Comment:
The new `usingRuleset` path doesn’t account for `managed=true` unknown-rule
handling later in the Read: the dummy-rule code still constructs legacy-shaped
rules (with `uuids` and no required `rule_number`). When `usingRuleset` is
true, those dummy rules will be fed into `d.Set("ruleset", ...)`, which can
fail or drop unknown rules, breaking managed mode. Adjust the managed
dummy-rule representation for ruleset (use `uuid` and a placeholder
`rule_number`/other required fields) or explicitly disallow `managed=true` with
`ruleset`.
##########
cloudstack/resource_cloudstack_network_acl_rule.go:
##########
@@ -187,6 +188,75 @@ func resourceCloudStackNetworkACLRule() *schema.Resource {
},
},
+ "ruleset": {
+ Type: schema.TypeSet,
+ Optional: true,
+ ConflictsWith: []string{"rule"},
+ Set:
resourceCloudStackNetworkACLRulesetHash,
+ Elem: &schema.Resource{
+ Schema: map[string]*schema.Schema{
+ "rule_number": {
+ Type:
schema.TypeInt,
+ Required: true,
+ },
Review Comment:
`ruleset` elements are hashed only by `rule_number`, but there’s no
validation preventing duplicate `rule_number` values. With a TypeSet,
duplicates will be de-duplicated/overwritten in planning, which can silently
drop rules and make applies unpredictable. Add validation (e.g., CustomizeDiff)
that enforces unique `rule_number` across all `ruleset` blocks and returns a
clear error when duplicates exist.
##########
website/docs/r/network_acl_rule.html.markdown:
##########
@@ -127,6 +127,61 @@ resource "cloudstack_network_acl_rule" "web_server" {
description = "Allow all outbound TCP"
}
}
+```
+
+### Using `ruleset` for Better Change Management
+
+The `ruleset` field is recommended when you need to insert or remove rules
without
+triggering unnecessary updates to other rules. Unlike `rule` (which uses a
list),
+`ruleset` uses a set that identifies rules by their content rather than
position.
+
Review Comment:
Docs state that `ruleset` “identifies rules by their content rather than
position,” but the implementation hashes only `rule_number`
(`resourceCloudStackNetworkACLRulesetHash`). This is a different contract:
identity is by `rule_number`, not full content. Update the wording to avoid
misleading users (and consider explicitly stating `rule_number` must be unique).
##########
cloudstack/resource_cloudstack_network_acl_rule.go:
##########
@@ -202,6 +272,159 @@ func resourceCloudStackNetworkACLRule() *schema.Resource {
}
}
+// resourceCloudStackNetworkACLRulesetHash computes the hash for a ruleset
element
+// Only the rule_number is used for hashing since it uniquely identifies a rule
+// This prevents spurious diffs when fields with defaults are populated in
state but not in config
+func resourceCloudStackNetworkACLRulesetHash(v interface{}) int {
+ var buf bytes.Buffer
+ m := v.(map[string]interface{})
+
+ // Only hash on rule_number since it's the unique identifier
+ if v, ok := m["rule_number"]; ok {
+ buf.WriteString(fmt.Sprintf("%d-", v.(int)))
+ }
+
+ return schema.HashString(buf.String())
+}
+
+// Helper functions for UUID handling to abstract differences between
+// 'rule' (uses uuids map) and 'ruleset' (uses uuid string)
+
+// getRuleUUID gets the UUID for a rule, handling both formats
+// For ruleset: returns the uuid string
+// For rule with key: returns the UUID from uuids map for the given key
+// For rule without key: returns the first UUID from uuids map
+func getRuleUUID(rule map[string]interface{}, key string) (string, bool) {
+ // Try uuid string first (ruleset format)
+ if uuidVal, ok := rule["uuid"]; ok && uuidVal != nil {
+ if uuid, ok := uuidVal.(string); ok && uuid != "" {
+ return uuid, true
+ }
+ }
+
+ // Try uuids map (rule format)
+ if uuidsVal, ok := rule["uuids"]; ok && uuidsVal != nil {
+ if uuids, ok := uuidsVal.(map[string]interface{}); ok {
+ if key != "" {
+ // Get specific key
+ if idVal, ok := uuids[key]; ok && idVal != nil {
+ if id, ok := idVal.(string); ok {
+ return id, true
+ }
+ }
+ } else {
+ // Get first non-nil UUID
+ for _, idVal := range uuids {
+ if idVal != nil {
+ if id, ok := idVal.(string); ok
{
+ return id, true
+ }
+ }
+ }
+ }
+ }
+ }
+
+ return "", false
+}
+
+// setRuleUUID sets the UUID for a rule, handling both formats
+// For ruleset: sets the uuid string
+// For rule: sets the UUID in uuids map with the given key
+func setRuleUUID(rule map[string]interface{}, key string, uuid string) {
+ // Check if this is a ruleset (has uuid field) or rule (has uuids field)
+ if _, hasUUID := rule["uuid"]; hasUUID {
+ // Ruleset format
+ rule["uuid"] = uuid
+ } else {
+ // Rule format - ensure uuids map exists
+ var uuids map[string]interface{}
+ if uuidsVal, ok := rule["uuids"]; ok && uuidsVal != nil {
+ uuids = uuidsVal.(map[string]interface{})
+ } else {
+ uuids = make(map[string]interface{})
+ rule["uuids"] = uuids
+ }
+ uuids[key] = uuid
+ }
+}
+
+// hasRuleUUID checks if a rule has any UUID set
+func hasRuleUUID(rule map[string]interface{}) bool {
+ // Check uuid string (ruleset format)
+ if uuidVal, ok := rule["uuid"]; ok && uuidVal != nil {
+ if uuid, ok := uuidVal.(string); ok && uuid != "" {
+ return true
+ }
+ }
+
+ // Check uuids map (rule format)
+ if uuidsVal, ok := rule["uuids"]; ok && uuidsVal != nil {
+ if uuids, ok := uuidsVal.(map[string]interface{}); ok &&
len(uuids) > 0 {
+ return true
+ }
+ }
+
+ return false
+}
+
+// copyRuleUUIDs copies all UUIDs from source rule to destination rule
+func copyRuleUUIDs(dst, src map[string]interface{}) {
+ // Check if source has uuid string (ruleset format)
+ if uuidVal, ok := src["uuid"]; ok && uuidVal != nil {
+ if uuid, ok := uuidVal.(string); ok && uuid != "" {
+ dst["uuid"] = uuid
+ return
+ }
+ }
+
+ // Check if source has uuids map (rule format)
+ if uuidsVal, ok := src["uuids"]; ok && uuidsVal != nil {
+ if uuids, ok := uuidsVal.(map[string]interface{}); ok {
+ dst["uuids"] = uuids
+ return
+ }
+ }
+}
+
+// isRulesetRule checks if a rule is from a ruleset (has uuid field) vs rule
(has uuids field)
+func isRulesetRule(rule map[string]interface{}) bool {
+ _, hasUUID := rule["uuid"]
+ return hasUUID
+}
+
+// assignRuleNumbers assigns rule numbers to rules that don't have them
+// Rules are numbered sequentially starting from 1
+// If a rule has an explicit rule_number, numbering continues from that value
+func assignRuleNumbers(rules []interface{}) []interface{} {
+ result := make([]interface{}, len(rules))
+ nextNumber := 1
+
+ for i, rule := range rules {
+ ruleMap := make(map[string]interface{})
+ // Copy the rule
+ for k, v := range rule.(map[string]interface{}) {
+ ruleMap[k] = v
+ }
+
+ // Check if rule_number is set
+ if ruleNum, ok := ruleMap["rule_number"].(int); ok && ruleNum >
0 {
+ // Rule has explicit number, use it and continue from
there
+ nextNumber = ruleNum + 1
+ log.Printf("[DEBUG] Rule at index %d has explicit
rule_number=%d", i, ruleNum)
+ } else {
+ // Auto-assign sequential number
+ ruleMap["rule_number"] = nextNumber
+ log.Printf("[DEBUG] Auto-assigned rule_number=%d to
rule at index %d", nextNumber, i)
+ nextNumber++
+ }
Review Comment:
`assignRuleNumbers` can generate duplicate or decreasing rule numbers if a
later rule specifies an explicit `rule_number` that’s <= the current
`nextNumber` (e.g., first rule auto-assigns 1, second explicitly sets 1, third
becomes 2). This can cause CloudStack API errors or unintended priority
changes. Consider updating the logic so `nextNumber` never moves backwards
(e.g., `nextNumber = max(nextNumber, ruleNum+1)`), and optionally detect/raise
on duplicates within the list.
##########
cloudstack/resource_cloudstack_network_acl_rule_test.go:
##########
@@ -251,6 +252,369 @@ func testAccCheckCloudStackNetworkACLRuleDestroy(s
*terraform.State) error {
return nil
}
+func TestAccCloudStackNetworkACLRule_ruleset_basic(t *testing.T) {
+ resource.Test(t, resource.TestCase{
+ PreCheck: func() { testAccPreCheck(t) },
+ Providers: testAccProviders,
+ CheckDestroy: testAccCheckCloudStackNetworkACLRuleDestroy,
+ Steps: []resource.TestStep{
+ {
+ Config:
testAccCloudStackNetworkACLRule_ruleset_basic,
+ Check: resource.ComposeTestCheckFunc(
+
testAccCheckCloudStackNetworkACLRulesExist("cloudstack_network_acl.bar"),
+ resource.TestCheckResourceAttr(
+
"cloudstack_network_acl_rule.bar", "ruleset.#", "4"),
+ // Check for the expected rules using
TypeSet elem matching
+
resource.TestCheckTypeSetElemNestedAttrs(
+
"cloudstack_network_acl_rule.bar", "ruleset.*", map[string]string{
+ "rule_number": "10",
+ "action": "allow",
+ "protocol": "all",
+ "traffic_type":
"ingress",
+ "description": "Allow
all traffic",
+ }),
+
resource.TestCheckTypeSetElemNestedAttrs(
+
"cloudstack_network_acl_rule.bar", "ruleset.*", map[string]string{
+ "rule_number": "20",
+ "action": "allow",
+ "protocol": "icmp",
+ "icmp_type": "-1",
+ "icmp_code": "-1",
+ "traffic_type":
"ingress",
+ "description": "Allow
ICMP traffic",
+ }),
+
resource.TestCheckTypeSetElemNestedAttrs(
+
"cloudstack_network_acl_rule.bar", "ruleset.*", map[string]string{
+ "rule_number": "30",
+ "action": "allow",
+ "protocol": "tcp",
+ "port": "80",
+ "traffic_type":
"ingress",
+ "description": "Allow
HTTP",
+ }),
+
resource.TestCheckTypeSetElemNestedAttrs(
+
"cloudstack_network_acl_rule.bar", "ruleset.*", map[string]string{
+ "rule_number": "40",
+ "action": "allow",
+ "protocol": "tcp",
+ "port": "443",
+ "traffic_type":
"ingress",
+ "description": "Allow
HTTPS",
+ }),
+ ),
+ },
+ },
+ })
+}
+
+func TestAccCloudStackNetworkACLRule_ruleset_update(t *testing.T) {
+ resource.Test(t, resource.TestCase{
+ PreCheck: func() { testAccPreCheck(t) },
+ Providers: testAccProviders,
+ CheckDestroy: testAccCheckCloudStackNetworkACLRuleDestroy,
+ Steps: []resource.TestStep{
+ {
+ Config:
testAccCloudStackNetworkACLRule_ruleset_basic,
+ Check: resource.ComposeTestCheckFunc(
+
testAccCheckCloudStackNetworkACLRulesExist("cloudstack_network_acl.bar"),
+ resource.TestCheckResourceAttr(
+
"cloudstack_network_acl_rule.bar", "ruleset.#", "4"),
+
resource.TestCheckTypeSetElemNestedAttrs(
+
"cloudstack_network_acl_rule.bar", "ruleset.*", map[string]string{
+ "rule_number": "10",
+ "action": "allow",
+ "protocol": "all",
+ "traffic_type":
"ingress",
+ "description": "Allow
all traffic",
+ }),
+
resource.TestCheckTypeSetElemNestedAttrs(
+
"cloudstack_network_acl_rule.bar", "ruleset.*", map[string]string{
+ "rule_number": "20",
+ "action": "allow",
+ "protocol": "icmp",
+ "icmp_type": "-1",
+ "icmp_code": "-1",
+ "traffic_type":
"ingress",
+ "description": "Allow
ICMP traffic",
+ }),
+
resource.TestCheckTypeSetElemNestedAttrs(
+
"cloudstack_network_acl_rule.bar", "ruleset.*", map[string]string{
+ "rule_number": "30",
+ "action": "allow",
+ "protocol": "tcp",
+ "port": "80",
+ "traffic_type":
"ingress",
+ "description": "Allow
HTTP",
+ }),
+
resource.TestCheckTypeSetElemNestedAttrs(
+
"cloudstack_network_acl_rule.bar", "ruleset.*", map[string]string{
+ "rule_number": "40",
+ "action": "allow",
+ "protocol": "tcp",
+ "port": "443",
+ "traffic_type":
"ingress",
+ "description": "Allow
HTTPS",
+ }),
+ ),
+ },
+
+ {
+ Config:
testAccCloudStackNetworkACLRule_ruleset_update,
+ Check: resource.ComposeTestCheckFunc(
+
testAccCheckCloudStackNetworkACLRulesExist("cloudstack_network_acl.bar"),
+ resource.TestCheckResourceAttr(
+
"cloudstack_network_acl_rule.bar", "ruleset.#", "6"),
+ // Check for the expected rules using
TypeSet elem matching
+
resource.TestCheckTypeSetElemNestedAttrs(
+
"cloudstack_network_acl_rule.bar", "ruleset.*", map[string]string{
+ "rule_number": "10",
+ "action": "deny",
+ "protocol": "all",
+ "traffic_type":
"ingress",
+ }),
+
resource.TestCheckTypeSetElemNestedAttrs(
+
"cloudstack_network_acl_rule.bar", "ruleset.*", map[string]string{
+ "rule_number": "20",
+ "action": "deny",
+ "protocol": "icmp",
+ "icmp_type": "-1",
+ "icmp_code": "-1",
+ "traffic_type":
"ingress",
+ "description": "Deny
ICMP traffic",
+ }),
+
resource.TestCheckTypeSetElemNestedAttrs(
+
"cloudstack_network_acl_rule.bar", "ruleset.*", map[string]string{
+ "rule_number": "30",
+ "action": "allow",
+ "protocol": "tcp",
+ "port": "80",
+ "traffic_type":
"ingress",
+ }),
+
resource.TestCheckTypeSetElemNestedAttrs(
+
"cloudstack_network_acl_rule.bar", "ruleset.*", map[string]string{
+ "rule_number": "40",
+ "action": "allow",
+ "protocol": "tcp",
+ "port": "443",
+ "traffic_type":
"ingress",
+ }),
+
resource.TestCheckTypeSetElemNestedAttrs(
+
"cloudstack_network_acl_rule.bar", "ruleset.*", map[string]string{
+ "rule_number": "50",
+ "action": "deny",
+ "protocol": "tcp",
+ "port": "80",
+ "traffic_type":
"egress",
+ "description": "Deny
specific TCP ports",
+ }),
+
resource.TestCheckTypeSetElemNestedAttrs(
+
"cloudstack_network_acl_rule.bar", "ruleset.*", map[string]string{
+ "rule_number": "60",
+ "action": "deny",
+ "protocol": "tcp",
+ "port":
"1000-2000",
+ "traffic_type":
"egress",
+ "description": "Deny
specific TCP ports",
+ }),
+ ),
+ },
+ },
+ })
+}
+
+func TestAccCloudStackNetworkACLRule_ruleset_insert(t *testing.T) {
+ resource.Test(t, resource.TestCase{
+ PreCheck: func() { testAccPreCheck(t) },
+ Providers: testAccProviders,
+ CheckDestroy: testAccCheckCloudStackNetworkACLRuleDestroy,
+ Steps: []resource.TestStep{
+ {
+ Config:
testAccCloudStackNetworkACLRule_ruleset_insert_initial,
+ Check: resource.ComposeTestCheckFunc(
+
testAccCheckCloudStackNetworkACLRulesExist("cloudstack_network_acl.baz"),
+ resource.TestCheckResourceAttr(
+
"cloudstack_network_acl_rule.baz", "ruleset.#", "3"),
+ // Initial rules: 10, 30, 50
+
resource.TestCheckTypeSetElemNestedAttrs(
+
"cloudstack_network_acl_rule.baz", "ruleset.*", map[string]string{
+ "rule_number": "10",
+ "action": "allow",
+ "protocol": "tcp",
+ "port": "22",
+ "traffic_type":
"ingress",
+ "description": "Allow
SSH",
+ }),
+
resource.TestCheckTypeSetElemNestedAttrs(
+
"cloudstack_network_acl_rule.baz", "ruleset.*", map[string]string{
+ "rule_number": "30",
+ "action": "allow",
+ "protocol": "tcp",
+ "port": "443",
+ "traffic_type":
"ingress",
+ "description": "Allow
HTTPS",
+ }),
+
resource.TestCheckTypeSetElemNestedAttrs(
+
"cloudstack_network_acl_rule.baz", "ruleset.*", map[string]string{
+ "rule_number": "50",
+ "action": "allow",
+ "protocol": "tcp",
+ "port": "3306",
+ "traffic_type":
"ingress",
+ "description": "Allow
MySQL",
+ }),
+ ),
+ },
+
+ {
+ Config:
testAccCloudStackNetworkACLRule_ruleset_insert_middle,
+ Check: resource.ComposeTestCheckFunc(
+
testAccCheckCloudStackNetworkACLRulesExist("cloudstack_network_acl.baz"),
+ resource.TestCheckResourceAttr(
+
"cloudstack_network_acl_rule.baz", "ruleset.#", "4"),
+ // After inserting rule 20 in the
middle, all original rules should still exist
+
resource.TestCheckTypeSetElemNestedAttrs(
+
"cloudstack_network_acl_rule.baz", "ruleset.*", map[string]string{
+ "rule_number": "10",
+ "action": "allow",
+ "protocol": "tcp",
+ "port": "22",
+ "traffic_type":
"ingress",
+ "description": "Allow
SSH",
+ }),
+ // NEW RULE inserted in the middle
+
resource.TestCheckTypeSetElemNestedAttrs(
+
"cloudstack_network_acl_rule.baz", "ruleset.*", map[string]string{
+ "rule_number": "20",
+ "action": "allow",
+ "protocol": "tcp",
+ "port": "80",
+ "traffic_type":
"ingress",
+ "description": "Allow
HTTP",
+ }),
+
resource.TestCheckTypeSetElemNestedAttrs(
+
"cloudstack_network_acl_rule.baz", "ruleset.*", map[string]string{
+ "rule_number": "30",
+ "action": "allow",
+ "protocol": "tcp",
+ "port": "443",
+ "traffic_type":
"ingress",
+ "description": "Allow
HTTPS",
+ }),
+
resource.TestCheckTypeSetElemNestedAttrs(
+
"cloudstack_network_acl_rule.baz", "ruleset.*", map[string]string{
+ "rule_number": "50",
+ "action": "allow",
+ "protocol": "tcp",
+ "port": "3306",
+ "traffic_type":
"ingress",
+ "description": "Allow
MySQL",
+ }),
+ ),
+ },
+ },
+ })
+}
+
+func TestAccCloudStackNetworkACLRule_ruleset_insert_plan_check(t *testing.T) {
+ resource.Test(t, resource.TestCase{
+ PreCheck: func() { testAccPreCheck(t) },
+ Providers: testAccProviders,
+ CheckDestroy: testAccCheckCloudStackNetworkACLRuleDestroy,
+ Steps: []resource.TestStep{
+ {
+ Config:
testAccCloudStackNetworkACLRule_ruleset_plan_check_initial,
+ Check: resource.ComposeTestCheckFunc(
+
testAccCheckCloudStackNetworkACLRulesExist("cloudstack_network_acl.plan_check"),
+ resource.TestCheckResourceAttr(
+
"cloudstack_network_acl_rule.plan_check", "ruleset.#", "3"),
+ // Initial rules: 10, 30, 50
+
resource.TestCheckTypeSetElemNestedAttrs(
+
"cloudstack_network_acl_rule.plan_check", "ruleset.*", map[string]string{
+ "rule_number": "10",
+ "action": "allow",
+ "protocol": "tcp",
+ "port": "22",
+ "traffic_type":
"ingress",
+ "description": "Allow
SSH",
+ }),
+
resource.TestCheckTypeSetElemNestedAttrs(
+
"cloudstack_network_acl_rule.plan_check", "ruleset.*", map[string]string{
+ "rule_number": "30",
+ "action": "allow",
+ "protocol": "tcp",
+ "port": "443",
+ "traffic_type":
"ingress",
+ "description": "Allow
HTTPS",
+ }),
+
resource.TestCheckTypeSetElemNestedAttrs(
+
"cloudstack_network_acl_rule.plan_check", "ruleset.*", map[string]string{
+ "rule_number": "50",
+ "action": "allow",
+ "protocol": "tcp",
+ "port": "3306",
+ "traffic_type":
"ingress",
+ "description": "Allow
MySQL",
+ }),
+ ),
+ },
+
+ {
+ Config:
testAccCloudStackNetworkACLRule_ruleset_plan_check_insert,
+ ConfigPlanChecks: resource.ConfigPlanChecks{
+ PreApply: []plancheck.PlanCheck{
+ // Verify that only 1 rule is
being added (the new rule 20)
+ // and the existing rules (10,
30, 50) are not being modified
+
plancheck.ExpectResourceAction("cloudstack_network_acl_rule.plan_check",
plancheck.ResourceActionUpdate),
+ },
Review Comment:
The test comment says it’s verifying that only one rule is added and
existing rules aren’t modified, but the only plan check is
`ExpectResourceAction(...Update)`, which doesn’t assert the absence of spurious
changes within the `ruleset`. Strengthen this by asserting the planned changes
are exactly one nested block add (and no deletes/updates) using more specific
planchecks (e.g., checks on planned `ruleset.#` delta and/or attribute-level
change expectations).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
