Copilot commented on code in PR #281:
URL:
https://github.com/apache/cloudstack-terraform-provider/pull/281#discussion_r2916230554
##########
cloudstack/resource_cloudstack_network_acl_rule_test.go:
##########
@@ -251,6 +254,375 @@ 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(
Review Comment:
These new `ruleset` acceptance tests call
`testAccCheckCloudStackNetworkACLRulesExist("cloudstack_network_acl.bar")`, but
that helper only inspects attributes containing `.uuids.` (legacy `rule`
format). The `cloudstack_network_acl` resource has no `uuids`, and `ruleset`
uses `uuid`, so this check becomes a no-op and won't catch create/read
failures. Consider updating the test to check the
`cloudstack_network_acl_rule.bar` resource and to validate `ruleset.*.uuid`
(and similarly update destroy checks) so ruleset coverage actually verifies
rule existence.
##########
website/docs/r/network_acl_rule.html.markdown:
##########
@@ -140,17 +196,30 @@ The following arguments are supported:
all firewall rules that are not in your config! (defaults false)
* `rule` - (Optional) Can be specified multiple times. Each rule block supports
- fields documented below. If `managed = false` at least one rule is
required!
+ fields documented below. If `managed = false` at least one rule or ruleset
is required!
+ **Cannot be used together with `ruleset`.**
+
+* `ruleset` - (Optional) Can be specified multiple times. Similar to `rule`
but uses
+ a set instead of a list, which prevents spurious updates when inserting
rules.
+ Each ruleset block supports the same fields as `rule` (documented below),
with these differences:
+ - `rule_number` is **required** (no auto-numbering)
+ - `ports` field is not supported (use `port` instead)
+ **Cannot be used together with `rule`.**
* `project` - (Optional) The name or ID of the project to deploy this
instance to. Changing this forces a new resource to be created.
* `parallelism` (Optional) Specifies how much rules will be created or deleted
concurrently. (defaults 2)
-The `rule` block supports:
+The `rule` and `ruleset` blocks support:
-* `rule_number` - (Optional) The number of the ACL item used to order the ACL
rules. The ACL rule with the lowest number has the highest priority. If not
specified, the ACL item will be created with a number one greater than the
highest numbered rule.
+* `rule_number` - (Optional for `rule`, **Required** for `ruleset`) The number
of the ACL
+ item used to order the ACL rules. The ACL rule with the lowest number has
the highest
+ priority.
+ - For `rule`: If not specified, the ACL item will be created with a number
one greater
+ than the highest numbered rule or the previous rule in the list.
Review Comment:
Docs for `rule.rule_number` say CloudStack will auto-assign "one greater
than the highest numbered rule", but the implementation now explicitly
enumerates missing `rule_number` values sequentially (starting at 1) via
`assignRuleNumbers`. Update this wording to match the provider behavior (or
adjust the implementation to actually base numbering on the highest existing
rule in the ACL).
```suggestion
- For `rule`: If not specified, the provider will auto-assign rule
numbers starting at 1,
increasing sequentially in the order the rules are defined and filling
any gaps, rather
than basing the number on the highest existing rule in the ACL.
```
##########
cloudstack/resource_cloudstack_network_acl_rule.go:
##########
@@ -202,6 +272,162 @@ 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, nextNumber advances to ensure no
duplicates
+func assignRuleNumbers(rules []interface{}) []interface{} {
+ result := make([]interface{}, len(rules))
+ nextNumber := 1
+
Review Comment:
`assignRuleNumbers` always starts auto-assigned numbering at 1. If a user
creates `cloudstack_network_acl_rule` against an ACL that already has existing
rules (common with `managed=true` or when adopting/importing), this can collide
with existing CloudStack rule numbers and either fail API calls or unexpectedly
change rule priority. Consider initializing `nextNumber` based on the current
max rule number in CloudStack for this ACL (e.g., list existing ACL rules and
start at max+1), or otherwise preserve the prior CloudStack auto-numbering
behavior for rules without an explicit `rule_number`.
--
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]
