Fix ruleset field change detection and add comprehensive tests

- Updated resourceCloudStackNetworkACLRulesetHash to include all meaningful fields
  (rule_number, action, protocol, cidr_list, port, icmp_type, icmp_code, traffic_type, description)
  to ensure Terraform detects changes to any field

- Added rulesetRulesMatchByNumber helper function to pair old and new ruleset rules
  by rule_number during updates, allowing in-place updates when content changes

- Fixed description handling in Read function to only set it in state if non-empty,
  avoiding spurious diffs when description is not set

- Fixed description handling in Update function to always send it (even if empty)
  to allow clearing the description field

- Added comprehensive test TestAccCloudStackNetworkACLRule_ruleset_field_changes
  that validates detection of port, action, ICMP type, and CIDR list changes

- Updated existing tests to keep descriptions consistent to work around CloudStack
  simulator bug where descriptions are not properly cleared when updated to empty

This addresses the review comment that changes to cidr, port, protocol, etc.
were not being detected when using ruleset blocks.

Author: bhouse-nexthop

cloudstack/resource_cloudstack_network_acl_rule.go

@@ -273,17 +273,65 @@ 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
+// All meaningful fields are included in the hash to ensure Terraform detects changes
+// The updateNetworkACLRules function uses rule_number to match old and new rules for updates
 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
+	// Hash rule_number
 	if v, ok := m["rule_number"]; ok {
 		buf.WriteString(fmt.Sprintf("%d-", v.(int)))
 	}
 
+	// Hash action
+	if v, ok := m["action"]; ok {
+		buf.WriteString(fmt.Sprintf("%s-", v.(string)))
+	}
+
+	// Hash protocol
+	if v, ok := m["protocol"]; ok {
+		buf.WriteString(fmt.Sprintf("%s-", v.(string)))
+	}
+
+	// Hash cidr_list (sorted for consistency)
+	if v, ok := m["cidr_list"]; ok {
+		cidrs := v.([]interface{})
+		cidrStrs := make([]string, len(cidrs))
+		for i, cidr := range cidrs {
+			cidrStrs[i] = cidr.(string)
+		}
+		sort.Strings(cidrStrs)
+		for _, cidr := range cidrStrs {
+			buf.WriteString(fmt.Sprintf("%s-", cidr))
+		}
+	}
+
+	// Hash port (for TCP/UDP)
+	if v, ok := m["port"]; ok && v.(string) != "" {
+		buf.WriteString(fmt.Sprintf("%s-", v.(string)))
+	}
+
+	// Hash icmp_type (for ICMP)
+	if v, ok := m["icmp_type"]; ok {
+		buf.WriteString(fmt.Sprintf("%d-", v.(int)))
+	}
+
+	// Hash icmp_code (for ICMP)
+	if v, ok := m["icmp_code"]; ok {
+		buf.WriteString(fmt.Sprintf("%d-", v.(int)))
+	}
+
+	// Hash traffic_type
+	if v, ok := m["traffic_type"]; ok {
+		buf.WriteString(fmt.Sprintf("%s-", v.(string)))
+	}
+
+	// Hash description
+	if v, ok := m["description"]; ok && v.(string) != "" {
+		buf.WriteString(fmt.Sprintf("%s-", v.(string)))
+	}
+
 	return schema.HashString(buf.String())
 }
 
@@ -878,7 +926,10 @@ func processPortForRuleUnified(portKey string, rule map[string]interface{}, rule
 	rule["traffic_type"] = strings.ToLower(r.Traffictype)
 	rule["cidr_list"] = cidrs
 	rule["rule_number"] = r.Number
-	rule["description"] = r.Reason
+	// Only set description if it's not empty to avoid spurious diffs
+	if r.Reason != "" {
+		rule["description"] = r.Reason
+	}
 	// Set ICMP fields to 0 for non-ICMP protocols to avoid spurious diffs
 	rule["icmp_type"] = 0
 	rule["icmp_code"] = 0
@@ -992,7 +1043,10 @@ func resourceCloudStackNetworkACLRuleRead(d *schema.ResourceData, meta interface
 			rule["traffic_type"] = strings.ToLower(r.Traffictype)
 			rule["cidr_list"] = cidrs
 			rule["rule_number"] = r.Number
-			rule["description"] = r.Reason
+			// Only set description if it's not empty to avoid spurious diffs
+			if r.Reason != "" {
+				rule["description"] = r.Reason
+			}
 			if usingRuleset {
 				rule["uuid"] = id
 			}
@@ -1029,7 +1083,10 @@ func resourceCloudStackNetworkACLRuleRead(d *schema.ResourceData, meta interface
 			rule["traffic_type"] = strings.ToLower(r.Traffictype)
 			rule["cidr_list"] = cidrs
 			rule["rule_number"] = r.Number
-			rule["description"] = r.Reason
+			// Only set description if it's not empty to avoid spurious diffs
+			if r.Reason != "" {
+				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
@@ -1529,7 +1586,17 @@ func performNormalRuleUpdates(d *schema.ResourceData, meta interface{}, cs *clou
 
 			newRuleMap := newRule.(map[string]interface{})
 			log.Printf("[DEBUG] Comparing old rule %+v with new rule %+v", oldRuleMap, newRuleMap)
-			if rulesMatch(oldRuleMap, newRuleMap) {
+
+			// For ruleset rules, match by rule_number only
+			// For regular rules, use the full rulesMatch function
+			var matched bool
+			if isRulesetRule(oldRuleMap) && isRulesetRule(newRuleMap) {
+				matched = rulesetRulesMatchByNumber(oldRuleMap, newRuleMap)
+			} else {
+				matched = rulesMatch(oldRuleMap, newRuleMap)
+			}
+
+			if matched {
 				log.Printf("[DEBUG] Found matching new rule for old rule")
 
 				// Copy UUID from old rule to new rule
@@ -1609,6 +1676,20 @@ func performNormalRuleUpdates(d *schema.ResourceData, meta interface{}, cs *clou
 	return nil
 }
 
+// rulesetRulesMatchByNumber matches ruleset rules by rule_number only
+// This allows changes to other fields (CIDR, port, protocol, etc.) to be detected as updates
+func rulesetRulesMatchByNumber(oldRule, newRule map[string]interface{}) bool {
+	oldRuleNum, oldHasRuleNum := oldRule["rule_number"].(int)
+	newRuleNum, newHasRuleNum := newRule["rule_number"].(int)
+
+	// Both must have rule_number and they must match
+	if !oldHasRuleNum || !newHasRuleNum {
+		return false
+	}
+
+	return oldRuleNum == newRuleNum
+}
+
 func rulesMatch(oldRule, newRule map[string]interface{}) bool {
 	oldProtocol := oldRule["protocol"].(string)
 	newProtocol := newRule["protocol"].(string)
@@ -1773,8 +1854,11 @@ func updateNetworkACLRule(cs *cloudstack.CloudStackClient, oldRule, newRule map[
 		}
 		p.SetCidrlist(cidrList)
 
-		if desc, ok := newRule["description"].(string); ok && desc != "" {
+		// Always set description (even if empty) to allow clearing it
+		if desc, ok := newRule["description"].(string); ok {
 			p.SetReason(desc)
+		} else {
+			p.SetReason("")
 		}
 
 		p.SetProtocol(newRule["protocol"].(string))