Skip to content

Commit 4e43e06

Browse files
Support deprecated 'ports' field for backward compatibility
The 'ports' field in the legacy 'rule' block was documented as being kept 'for backward compatibility only', but the provider was rejecting it during rule creation/verification. This commit implements full support for the deprecated field to match the documentation. Changes: 1. Rule Expansion: - When a rule uses 'ports' with multiple values (e.g., ['80', '443']), the provider now creates separate CloudStack ACL rules for each port - Each expanded rule gets a unique, sequential rule number - Port ranges (e.g., '8000-8100') are supported and treated as single entries 2. Auto-Numbering Enhancement: - Updated assignRuleNumbers() to detect 'ports' usage and reserve enough sequential numbers for all expanded rules - Ensures subsequent rule blocks don't collide with expanded rules - Example: A rule with ports=['80','443'] reserves numbers N and N+1 3. Validation: - Added validation to prevent explicit 'rule_number' when 'ports' contains multiple values, since the provider must manage numbering for expansion - Moved validation to occur before auto-numbering so it can distinguish between user-provided and auto-assigned numbers - Clear error message guides users to either: * Use a single port in 'ports' * Omit 'rule_number' (let it auto-assign) * Migrate to the new 'port' field with explicit rule_number 4. Comprehensive Testing: - TestAccCloudStackNetworkACLRule_deprecated_ports: Basic functionality test verifying multiple ports and port ranges are expanded correctly - TestAccCloudStackNetworkACLRule_deprecated_ports_managed: Verifies that managed=true behavior works correctly with expanded rules, including deletion of out-of-band rules - TestAccCloudStackNetworkACLRule_deprecated_ports_not_managed: Verifies that managed=false preserves out-of-band rules when using deprecated ports field All 15 NetworkACLRule acceptance tests pass, ensuring existing configurations using the deprecated 'ports' field continue to work as documented.
1 parent b2843da commit 4e43e06

File tree

2 files changed

+354
-19
lines changed

2 files changed

+354
-19
lines changed

cloudstack/resource_cloudstack_network_acl_rule.go

Lines changed: 133 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -396,6 +396,7 @@ func isRulesetRule(rule map[string]interface{}) bool {
396396
// assignRuleNumbers assigns rule numbers to rules that don't have them
397397
// Rules are numbered sequentially starting from 1
398398
// If a rule has an explicit rule_number, nextNumber advances to ensure no duplicates
399+
// For rules using the deprecated 'ports' field with multiple ports, reserves enough numbers
399400
func assignRuleNumbers(rules []interface{}) []interface{} {
400401
result := make([]interface{}, len(rules))
401402
nextNumber := 1
@@ -419,7 +420,17 @@ func assignRuleNumbers(rules []interface{}) []interface{} {
419420
// Auto-assign sequential number
420421
ruleMap["rule_number"] = nextNumber
421422
log.Printf("[DEBUG] Auto-assigned rule_number=%d to rule at index %d", nextNumber, i)
422-
nextNumber++
423+
424+
// Check if this rule uses the deprecated 'ports' field with multiple ports
425+
// If so, we need to reserve additional rule numbers for the expanded rules
426+
if portsSet, ok := ruleMap["ports"].(*schema.Set); ok && portsSet.Len() > 1 {
427+
// Reserve portsSet.Len() numbers (one for each port)
428+
// The first port gets nextNumber, subsequent ports get nextNumber+1, nextNumber+2, etc.
429+
nextNumber += portsSet.Len()
430+
log.Printf("[DEBUG] Rule uses deprecated ports field with %d ports, reserved numbers up to %d", portsSet.Len(), nextNumber-1)
431+
} else {
432+
nextNumber++
433+
}
423434
}
424435

425436
result[i] = ruleMap
@@ -444,6 +455,14 @@ func resourceCloudStackNetworkACLRuleCreate(d *schema.ResourceData, meta interfa
444455

445456
log.Printf("[DEBUG] Processing %d rules from 'rule' field", len(nrs))
446457

458+
// Validate rules BEFORE assigning numbers, so we can detect user-provided rule_number
459+
for _, rule := range nrs {
460+
if err := verifyNetworkACLRuleParams(d, rule.(map[string]interface{})); err != nil {
461+
log.Printf("[ERROR] Failed to verify rule parameters: %v", err)
462+
return err
463+
}
464+
}
465+
447466
// Assign rule numbers to rules that don't have them
448467
rulesWithNumbers := assignRuleNumbers(nrs)
449468

@@ -564,11 +583,7 @@ func createNetworkACLRule(d *schema.ResourceData, meta interface{}, rule map[str
564583

565584
log.Printf("[DEBUG] Creating network ACL rule with protocol=%s, action=%s, traffic_type=%s", protocol, action, trafficType)
566585

567-
// Make sure all required parameters are there
568-
if err := verifyNetworkACLRuleParams(d, rule); err != nil {
569-
log.Printf("[ERROR] Failed to verify rule parameters: %v", err)
570-
return err
571-
}
586+
// Note: Parameter verification is done before assignRuleNumbers in resourceCloudStackNetworkACLRuleCreate
572587

573588
// Create a new parameter struct
574589
p := cs.NetworkACL.NewCreateNetworkACLParams(protocol)
@@ -637,15 +652,85 @@ func createNetworkACLRule(d *schema.ResourceData, meta interface{}, rule map[str
637652

638653
// If protocol is TCP or UDP, create the rule (with or without port)
639654
if rule["protocol"].(string) == "tcp" || rule["protocol"].(string) == "udp" {
640-
// Check if deprecated ports field is used and reject it
641-
if portsSet, hasPortsSet := rule["ports"].(*schema.Set); hasPortsSet && portsSet.Len() > 0 {
642-
log.Printf("[ERROR] Attempt to create rule with deprecated ports field")
643-
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.")
644-
}
645-
655+
// Check if deprecated ports field is used (for backward compatibility)
656+
portsSet, hasPortsSet := rule["ports"].(*schema.Set)
646657
portStr, hasPort := rule["port"].(string)
647658

648-
if hasPort && portStr != "" {
659+
if hasPortsSet && portsSet.Len() > 0 {
660+
// Handle deprecated ports field for backward compatibility
661+
// Create a separate rule for each port in the set, each with a unique rule number
662+
log.Printf("[DEBUG] Using deprecated ports field for backward compatibility, creating %d rules", portsSet.Len())
663+
664+
// Get the base rule number - this should always be set by assignRuleNumbers
665+
baseRuleNum := 0
666+
if ruleNum, ok := rule["rule_number"].(int); ok && ruleNum > 0 {
667+
baseRuleNum = ruleNum
668+
}
669+
670+
portIndex := 0
671+
for _, port := range portsSet.List() {
672+
portValue := port.(string)
673+
674+
// Check if this port already has a UUID
675+
if _, hasUUID := getRuleUUID(rule, portValue); !hasUUID {
676+
m := splitPorts.FindStringSubmatch(portValue)
677+
if m == nil {
678+
log.Printf("[ERROR] Invalid port format: %s", portValue)
679+
return fmt.Errorf("%q is not a valid port value. Valid options are '80' or '80-90'", portValue)
680+
}
681+
682+
startPort, err := strconv.Atoi(m[1])
683+
if err != nil {
684+
log.Printf("[ERROR] Failed to parse start port %s: %v", m[1], err)
685+
return err
686+
}
687+
688+
endPort := startPort
689+
if m[2] != "" {
690+
endPort, err = strconv.Atoi(m[2])
691+
if err != nil {
692+
log.Printf("[ERROR] Failed to parse end port %s: %v", m[2], err)
693+
return err
694+
}
695+
}
696+
697+
// Create a new parameter object for this specific port with a unique rule number
698+
portP := cs.NetworkACL.NewCreateNetworkACLParams(protocol)
699+
portP.SetAclid(aclID)
700+
portP.SetAction(action)
701+
portP.SetCidrlist(cidrList)
702+
portP.SetTraffictype(trafficType)
703+
if desc, ok := rule["description"].(string); ok && desc != "" {
704+
portP.SetReason(desc)
705+
}
706+
707+
// Set a unique rule number for each port by adding the port index
708+
// This ensures each expanded rule gets a unique number
709+
uniqueRuleNum := baseRuleNum + portIndex
710+
portP.SetNumber(uniqueRuleNum)
711+
log.Printf("[DEBUG] Set unique rule_number=%d for port %s (base=%d, index=%d)", uniqueRuleNum, portValue, baseRuleNum, portIndex)
712+
713+
portP.SetStartport(startPort)
714+
portP.SetEndport(endPort)
715+
log.Printf("[DEBUG] Set port start=%d, end=%d for deprecated ports field", startPort, endPort)
716+
717+
r, err := Retry(4, retryableACLCreationFunc(cs, portP))
718+
if err != nil {
719+
log.Printf("[ERROR] Failed to create TCP/UDP rule for port %s: %v", portValue, err)
720+
return err
721+
}
722+
723+
ruleID := r.(*cloudstack.CreateNetworkACLResponse).Id
724+
setRuleUUID(rule, portValue, ruleID)
725+
log.Printf("[DEBUG] Created TCP/UDP rule for port %s with ID=%s (deprecated ports field)", portValue, ruleID)
726+
727+
portIndex++
728+
} else {
729+
log.Printf("[DEBUG] Port %s already has UUID, skipping", portValue)
730+
portIndex++
731+
}
732+
}
733+
} else if hasPort && portStr != "" {
649734
// Handle single port
650735
log.Printf("[DEBUG] Processing single port for TCP/UDP rule: %s", portStr)
651736

@@ -1226,14 +1311,40 @@ func verifyNetworkACLRuleParams(d *schema.ResourceData, rule map[string]interfac
12261311
// No additional test are needed
12271312
log.Printf("[DEBUG] Protocol 'all' validated")
12281313
case "tcp", "udp":
1229-
// The deprecated 'ports' field is no longer supported in any scenario
1314+
// The deprecated 'ports' field is allowed for backward compatibility
1315+
// but users should migrate to the 'port' field
12301316
portsSet, hasPortsSet := rule["ports"].(*schema.Set)
12311317
portStr, hasPort := rule["port"].(string)
12321318

1233-
// Block deprecated ports field completely
1319+
// Allow deprecated ports field for backward compatibility
1320+
// The schema already marks it as deprecated with a warning
12341321
if hasPortsSet && portsSet.Len() > 0 {
1235-
log.Printf("[ERROR] Attempt to use deprecated ports field")
1236-
return fmt.Errorf("The 'ports' field is no longer supported. Please use the 'port' field instead.")
1322+
log.Printf("[DEBUG] Using deprecated ports field for backward compatibility")
1323+
1324+
// When using deprecated ports field with multiple values, rule_number cannot be specified
1325+
// because we auto-generate sequential rule numbers for each port
1326+
if portsSet.Len() > 1 {
1327+
if ruleNum, ok := rule["rule_number"]; ok && ruleNum != nil {
1328+
if number, ok := ruleNum.(int); ok && number > 0 {
1329+
log.Printf("[ERROR] Cannot specify rule_number when using deprecated ports field with multiple values")
1330+
return fmt.Errorf(
1331+
"Cannot specify 'rule_number' when using deprecated 'ports' field with multiple values. " +
1332+
"Rule numbers are auto-generated for each port (starting from the auto-assigned base number). " +
1333+
"Either use a single port in 'ports', or omit 'rule_number', or migrate to the 'port' field.")
1334+
}
1335+
}
1336+
}
1337+
1338+
// Validate each port in the set
1339+
for _, p := range portsSet.List() {
1340+
portValue := p.(string)
1341+
m := splitPorts.FindStringSubmatch(portValue)
1342+
if m == nil {
1343+
log.Printf("[ERROR] Invalid port format in ports field: %s", portValue)
1344+
return fmt.Errorf(
1345+
"%q is not a valid port value. Valid options are '80' or '80-90'", portValue)
1346+
}
1347+
}
12371348
}
12381349

12391350
// Validate the new port field if used
@@ -1245,8 +1356,11 @@ func verifyNetworkACLRuleParams(d *schema.ResourceData, rule map[string]interfac
12451356
return fmt.Errorf(
12461357
"%q is not a valid port value. Valid options are '80' or '80-90'", portStr)
12471358
}
1248-
} else {
1249-
log.Printf("[DEBUG] No port specified for TCP/UDP, allowing empty port")
1359+
}
1360+
1361+
// If neither port nor ports is specified, that's also valid (allows all ports)
1362+
if (!hasPort || portStr == "") && (!hasPortsSet || portsSet.Len() == 0) {
1363+
log.Printf("[DEBUG] No port specified for TCP/UDP, allowing all ports")
12501364
}
12511365
default:
12521366
_, err := strconv.ParseInt(protocol, 0, 0)

0 commit comments

Comments
 (0)