bradh352 commented on code in PR #281:
URL:
https://github.com/apache/cloudstack-terraform-provider/pull/281#discussion_r2936957384
##########
cloudstack/resource_cloudstack_network_acl_rule.go:
##########
@@ -596,118 +845,196 @@ 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
+ }
- // Get the rule
- r, ok := ruleMap[id.(string)]
- if !ok {
- log.Printf("[DEBUG] ICMP rule with ID
%s not found, removing UUID", id.(string))
- delete(uuids, "icmp")
- continue
- }
+ // Process all configured rules (works for both 'rule' and 'ruleset')
+ for _, rule := range configuredRules {
+ rule := rule.(map[string]interface{})
- // Delete the known rule so only unknown rules
remain in the ruleMap
- delete(ruleMap, id.(string))
+ fieldName := "rule"
+ if usingRuleset {
+ fieldName = "ruleset"
+ }
+ log.Printf("[DEBUG] Processing %s with protocol=%s", fieldName,
rule["protocol"].(string))
- // Create a list with all CIDR's
- var cidrs []interface{}
- for _, cidr := range strings.Split(r.Cidrlist,
",") {
- cidrs = append(cidrs, cidr)
- }
+ if rule["protocol"].(string) == "icmp" {
+ id, ok := getRuleUUID(rule, "icmp")
+ if !ok {
+ log.Printf("[DEBUG] No ICMP UUID found,
skipping rule")
+ continue
+ }
- // Update the values
- rule["action"] = strings.ToLower(r.Action)
- rule["protocol"] = r.Protocol
- rule["icmp_type"] = r.Icmptype
- rule["icmp_code"] = r.Icmpcode
- rule["traffic_type"] =
strings.ToLower(r.Traffictype)
- rule["cidr_list"] = cidrs
- rule["rule_number"] = r.Number
- rules = append(rules, rule)
- log.Printf("[DEBUG] Added ICMP rule to state:
%+v", rule)
+ // Get the rule
+ r, ok := ruleMap[id]
+ if !ok {
+ log.Printf("[DEBUG] ICMP rule with ID %s not
found", id)
+ continue
}
- if rule["protocol"].(string) == "all" {
- id, ok := uuids["all"]
- if !ok {
- log.Printf("[DEBUG] No ALL UUID found,
skipping rule")
- continue
- }
+ // Delete the known rule so only unknown rules remain
in the ruleMap
+ delete(ruleMap, id)
- // Get the rule
- r, ok := ruleMap[id.(string)]
- if !ok {
- log.Printf("[DEBUG] ALL rule with ID %s
not found, removing UUID", id.(string))
- delete(uuids, "all")
- continue
- }
+ // Create a list with all CIDR's
+ var cidrs []interface{}
+ for _, cidr := range strings.Split(r.Cidrlist, ",") {
+ cidrs = append(cidrs, cidr)
+ }
- // Delete the known rule so only unknown rules
remain in the ruleMap
- delete(ruleMap, id.(string))
+ // Update the values
+ rule["action"] = strings.ToLower(r.Action)
+ rule["protocol"] = r.Protocol
+ rule["icmp_type"] = r.Icmptype
+ rule["icmp_code"] = r.Icmpcode
+ rule["traffic_type"] = strings.ToLower(r.Traffictype)
+ rule["cidr_list"] = cidrs
+ rule["rule_number"] = r.Number
+ rule["description"] = r.Reason
+ if usingRuleset {
+ rule["uuid"] = id
+ }
+ rules = append(rules, rule)
+ log.Printf("[DEBUG] Added ICMP rule to state: %+v",
rule)
+ }
- // Create a list with all CIDR's
- var cidrs []interface{}
- for _, cidr := range strings.Split(r.Cidrlist,
",") {
- cidrs = append(cidrs, cidr)
- }
+ if rule["protocol"].(string) == "all" {
+ id, ok := getRuleUUID(rule, "all")
+ if !ok {
+ log.Printf("[DEBUG] No ALL UUID found, skipping
rule")
+ continue
+ }
+
+ // Get the rule
+ r, ok := ruleMap[id]
+ if !ok {
+ log.Printf("[DEBUG] ALL rule with ID %s not
found", id)
+ continue
+ }
+
+ // Delete the known rule so only unknown rules remain
in the ruleMap
+ delete(ruleMap, id)
- // Update the values
- rule["action"] = strings.ToLower(r.Action)
- rule["protocol"] = r.Protocol
- rule["traffic_type"] =
strings.ToLower(r.Traffictype)
- rule["cidr_list"] = cidrs
- rule["rule_number"] = r.Number
- rules = append(rules, rule)
- log.Printf("[DEBUG] Added ALL rule to state:
%+v", rule)
+ // Create a list with all CIDR's
+ var cidrs []interface{}
+ for _, cidr := range strings.Split(r.Cidrlist, ",") {
+ cidrs = append(cidrs, cidr)
}
- if rule["protocol"].(string) == "tcp" ||
rule["protocol"].(string) == "udp" {
- uuids := rule["uuids"].(map[string]interface{})
- processTCPUDPRule(rule, ruleMap, uuids, &rules)
+ // Update the values
+ rule["action"] = strings.ToLower(r.Action)
+ rule["protocol"] = r.Protocol
+ rule["traffic_type"] = strings.ToLower(r.Traffictype)
+ rule["cidr_list"] = cidrs
+ rule["rule_number"] = r.Number
+ rule["description"] = r.Reason
+ delete(rule, "port") // Remove port field for "all"
protocol
+ // Set ICMP fields to 0 for non-ICMP protocols to avoid
spurious diffs
+ rule["icmp_type"] = 0
+ rule["icmp_code"] = 0
+ if usingRuleset {
+ rule["uuid"] = id
}
+ rules = append(rules, rule)
+ log.Printf("[DEBUG] Added ALL rule to state: %+v", rule)
+ }
+
+ if rule["protocol"].(string) == "tcp" ||
rule["protocol"].(string) == "udp" {
+ processTCPUDPRule(rule, ruleMap, &rules)
}
}
- // If this is a managed firewall, add all unknown rules into dummy rules
+ // If this is a managed ACL, add all unknown rules as out-of-band rule
placeholders
managed := d.Get("managed").(bool)
if managed && len(ruleMap) > 0 {
+ // Find the highest rule_number to avoid conflicts when
creating out-of-band rule placeholders
+ maxRuleNumber := 0
+ for _, rule := range rules {
+ if ruleMap, ok := rule.(map[string]interface{}); ok {
+ if ruleNum, ok := ruleMap["rule_number"].(int);
ok && ruleNum > maxRuleNumber {
+ maxRuleNumber = ruleNum
+ }
+ }
+ }
+
+ // Start assigning out-of-band rule numbers after the highest
existing rule_number
+ outOfBandRuleNumber := maxRuleNumber + 1
+
for uuid := range ruleMap {
- // We need to create and add a dummy value to a list as
the
+ // We need to create and add a placeholder value to a
list as the
// cidr_list is a required field and thus needs a value
cidrs := []interface{}{uuid}
- // Make a dummy rule to hold the unknown UUID
- rule := map[string]interface{}{
- "cidr_list": cidrs,
- "protocol": uuid,
- "uuids": map[string]interface{}{uuid: uuid},
+ // Make a placeholder rule to hold the unknown UUID
+ // Format differs between 'rule' and 'ruleset'
+ var rule map[string]interface{}
+ if usingRuleset {
+ // For ruleset: use 'uuid' string and include
rule_number
+ // Include all fields with defaults to avoid
spurious diffs
+ rule = map[string]interface{}{
+ "cidr_list": cidrs,
+ "protocol": uuid,
+ "uuid": uuid,
+ "rule_number": outOfBandRuleNumber,
+ "action": "allow", // default
value
+ "traffic_type": "ingress", // default
value
+ "icmp_type": 0, // default
value
+ "icmp_code": 0, // default
value
+ "description": "", // empty
string for optional field
+ "port": "", // empty
string for optional field
+ }
+ outOfBandRuleNumber++
+ } else {
+ // For rule: use 'uuids' map
+ rule = map[string]interface{}{
+ "cidr_list": cidrs,
+ "protocol": uuid,
+ "uuids":
map[string]interface{}{uuid: uuid},
+ }
}
- // Add the dummy rule to the rules list
+ // Add the out-of-band rule placeholder to the rules
list
rules = append(rules, rule)
- log.Printf("[DEBUG] Added managed dummy rule for UUID
%s", uuid)
+ log.Printf("[DEBUG] Added out-of-band rule placeholder
for UUID %s (usingRuleset=%t)", uuid, usingRuleset)
}
}
- if len(rules) > 0 {
- log.Printf("[DEBUG] Setting %d rules in state", len(rules))
+ // Always set the rules in state, even if empty (for managed=true case)
+
+ if usingRuleset {
+ // For TypeSet, we need to be very careful about state updates
+ // The SDK has issues with properly clearing removed elements
from TypeSet
+ // So we explicitly set to empty first, then set the new value
+ rulesetResource :=
resourceCloudStackNetworkACLRule().Schema["ruleset"].Elem.(*schema.Resource)
+ hashFunc := schema.HashResource(rulesetResource)
+
+ // First, clear the ruleset completely
+ emptySet := schema.NewSet(hashFunc, []interface{}{})
+ if err := d.Set("ruleset", emptySet); err != nil {
+ log.Printf("[ERROR] Failed to clear ruleset attribute:
%v", err)
+ return err
+ }
+
+ // Now set the new rules
+ newSet := schema.NewSet(hashFunc, rules)
+ if err := d.Set("ruleset", newSet); err != nil {
Review Comment:
fixed
##########
website/docs/r/network_acl_rule.html.markdown:
##########
@@ -166,15 +236,15 @@ The `rule` block supports:
* `icmp_code` - (Optional) The ICMP code to allow, or `-1` to allow `any`. This
can only be specified if the protocol is ICMP. (defaults 0)
-* `port` - (Optional) Port or port range to allow. This can only be specified
if
+* `port` - (Optional) Port or port range to allow. This can only be specified
if
the protocol is TCP, UDP, ALL or a valid protocol number. Valid formats
are:
- Single port: `"80"`
- Port range: `"8000-8010"`
- If not specified for TCP/UDP, allows all ports for that protocol
-* `ports` - (Optional) **DEPRECATED**: Use `port` instead. List of ports
and/or
- port ranges to allow. This field is deprecated and will be removed in a
future
- version. For backward compatibility only.
+* `ports` - (Optional) **DEPRECATED**: Use `port` instead. List of ports and/or
+ port ranges to allow. This field is deprecated and will be removed in a
future
+ version. For backward compatibility only. **Not available in `ruleset`.**
Review Comment:
fixed
--
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]
