Copilot commented on code in PR #281:
URL:
https://github.com/apache/cloudstack-terraform-provider/pull/281#discussion_r2936975512
##########
cloudstack/resource_cloudstack_network_acl_rule.go:
##########
@@ -232,6 +481,30 @@ func resourceCloudStackNetworkACLRuleCreate(d
*schema.ResourceData, meta interfa
log.Printf("[ERROR] Failed to set rule attribute: %v",
err)
return err
}
+ } else if nrs := d.Get("ruleset").(*schema.Set); nrs.Len() > 0 {
+ // Handle 'ruleset' (TypeSet with mandatory rule_number)
+ rules := make([]interface{}, 0)
+
+ log.Printf("[DEBUG] Processing %d rules from 'ruleset' field",
nrs.Len())
+
+ // Convert Set to list (no auto-numbering needed, rule_number
is required)
+ rulesList := nrs.List()
+
Review Comment:
`ruleset` create path skips `verifyNetworkACLRuleParams` entirely before
calling `createNetworkACLRules`. Because `createNetworkACLRule` performs
unchecked type assertions (e.g., `rule["icmp_type"].(int)`), invalid/missing
fields can cause panics or much less actionable CloudStack API errors. Validate
each `ruleset` element (same as the legacy `rule` path) before creation, and do
the same validation for `newRules`/`newRuleset` in Update before calling
`updateNetworkACLRules`/creating new rules.
##########
cloudstack/resource_cloudstack_network_acl_rule.go:
##########
@@ -369,26 +645,97 @@ func createNetworkACLRule(d *schema.ResourceData, meta
interface{}, rule map[str
log.Printf("[ERROR] Failed to create ALL rule: %v", err)
return err
}
- uuids["all"] = r.(*cloudstack.CreateNetworkACLResponse).Id
- rule["uuids"] = uuids
- log.Printf("[DEBUG] Created ALL rule with ID=%s",
r.(*cloudstack.CreateNetworkACLResponse).Id)
+ ruleID := r.(*cloudstack.CreateNetworkACLResponse).Id
+ setRuleUUID(rule, "all", ruleID)
+ log.Printf("[DEBUG] Created ALL rule with ID=%s", ruleID)
}
// If protocol is TCP or UDP, create the rule (with or without port)
if rule["protocol"].(string) == "tcp" || rule["protocol"].(string) ==
"udp" {
- // Check if deprecated ports field is used and reject it
- if portsSet, hasPortsSet := rule["ports"].(*schema.Set);
hasPortsSet && portsSet.Len() > 0 {
- log.Printf("[ERROR] Attempt to create rule with
deprecated ports field")
- return fmt.Errorf("The 'ports' field is no longer
supported for creating new rules. Please use the 'port' field with separate
rules for each port/range.")
- }
-
+ // Check if deprecated ports field is used (for backward
compatibility)
+ portsSet, hasPortsSet := rule["ports"].(*schema.Set)
portStr, hasPort := rule["port"].(string)
- if hasPort && portStr != "" {
+ if hasPortsSet && portsSet.Len() > 0 {
+ // Handle deprecated ports field for backward
compatibility
+ // Create a separate rule for each port in the set,
each with a unique rule number
+ log.Printf("[DEBUG] Using deprecated ports field for
backward compatibility, creating %d rules", portsSet.Len())
+
+ // Get the base rule number - this should always be set
by assignRuleNumbers
+ baseRuleNum := 0
+ if ruleNum, ok := rule["rule_number"].(int); ok &&
ruleNum > 0 {
+ baseRuleNum = ruleNum
+ }
+
+ portIndex := 0
+ for _, port := range portsSet.List() {
+ portValue := port.(string)
+
Review Comment:
When expanding deprecated `ports` into multiple CloudStack rules, iteration
uses `portsSet.List()` (TypeSet) which is not ordered. Because you derive
`uniqueRuleNum := baseRuleNum + portIndex`, the assigned rule numbers (and
therefore rule priorities) can change between runs even if the configured ports
are the same. Sort the port strings before iterating so numbering is
deterministic and stable.
##########
cloudstack/resource_cloudstack_network_acl_rule.go:
##########
@@ -300,17 +576,17 @@ func createNetworkACLRules(d *schema.ResourceData, meta
interface{}, rules *[]in
func createNetworkACLRule(d *schema.ResourceData, meta interface{}, rule
map[string]interface{}) error {
cs := meta.(*cloudstack.CloudStackClient)
- uuids := rule["uuids"].(map[string]interface{})
- log.Printf("[DEBUG] Creating network ACL rule with protocol=%s",
rule["protocol"].(string))
- // Make sure all required parameters are there
- if err := verifyNetworkACLRuleParams(d, rule); err != nil {
- log.Printf("[ERROR] Failed to verify rule parameters: %v", err)
- return err
- }
+ protocol := rule["protocol"].(string)
+ action := rule["action"].(string)
+ trafficType := rule["traffic_type"].(string)
+
+ log.Printf("[DEBUG] Creating network ACL rule with protocol=%s,
action=%s, traffic_type=%s", protocol, action, trafficType)
+
+ // Note: Parameter verification is done before assignRuleNumbers in
resourceCloudStackNetworkACLRuleCreate
// Create a new parameter struct
Review Comment:
`createNetworkACLRule` assumes “parameter verification is done before
assignRuleNumbers in Create”, but this function is also called from Update (for
new rules) and from the `ruleset` Create path, where that verification is not
guaranteed. Either re-introduce `verifyNetworkACLRuleParams` inside
`createNetworkACLRule` (so all call sites are protected) or ensure every caller
(Create/Update for both `rule` and `ruleset`) validates rules before invoking
it.
--
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]
