Fix egress firewall rules to support all-ports without explicit ports

cloudstack/resource_cloudstack_egress_firewall.go

@@ -21,6 +21,7 @@ package cloudstack
 
 import (
 	"fmt"
+	"sort"
 	"strconv"
 	"strings"
 	"sync"
@@ -31,6 +32,81 @@ import (
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 )
 
+// treats 'all ports' for tcp/udp across CS versions returning 0/0, -1/-1, or 1/65535
+func isAllPortsTCPUDP(protocol string, start, end int) bool {
+	p := strings.ToLower(protocol)
+	if p != "tcp" && p != "udp" {
+		return false
+	}
+	// handle various representations of all ports across CloudStack versions
+	if (start == 0 && end == 0) ||
+		(start == -1 && end == -1) ||
+		(start == 1 && end == 65535) {
+		return true
+	}
+	return false
+}
+
+// normalizeRemoteCIDRs normalizes a comma-separated CIDR string from CloudStack API
+func normalizeRemoteCIDRs(cidrList string) []string {
+	if cidrList == "" {
+		return []string{}
+	}
+
+	cidrs := strings.Split(cidrList, ",")
+	normalized := make([]string, 0, len(cidrs))
+
+	for _, cidr := range cidrs {
+		trimmed := strings.TrimSpace(cidr)
+		if trimmed != "" {
+			normalized = append(normalized, trimmed)
+		}
+	}
+
+	sort.Strings(normalized)
+	return normalized
+}
+
+// normalizeLocalCIDRs normalizes a Terraform schema.Set of CIDRs
+func normalizeLocalCIDRs(cidrSet *schema.Set) []string {
+	if cidrSet == nil {
+		return []string{}
+	}
+
+	normalized := make([]string, 0, cidrSet.Len())
+	for _, cidr := range cidrSet.List() {
+		if cidrStr, ok := cidr.(string); ok {
+			trimmed := strings.TrimSpace(cidrStr)
+			if trimmed != "" {
+				normalized = append(normalized, trimmed)
+			}
+		}
+	}
+
+	sort.Strings(normalized)
+	return normalized
+}
+
+// cidrSetsEqual compares normalized CIDR sets for equality (order/whitespace agnostic)
+func cidrSetsEqual(remoteCidrList string, localCidrSet *schema.Set) bool {
+	remoteCidrs := normalizeRemoteCIDRs(remoteCidrList)
+	localCidrs := normalizeLocalCIDRs(localCidrSet)
+
+	// Compare lengths first
+	if len(remoteCidrs) != len(localCidrs) {
+		return false
+	}
+
+	// Compare each element (both are already sorted)
+	for i, remoteCidr := range remoteCidrs {
+		if remoteCidr != localCidrs[i] {
+			return false
+		}
+	}
+
+	return true
+}
+
 func resourceCloudStackEgressFirewall() *schema.Resource {
 	return &schema.Resource{
 		Create: resourceCloudStackEgressFirewallCreate,
@@ -250,6 +326,15 @@ func createEgressFirewallRule(d *schema.ResourceData, meta interface{}, rule map
 				uuids[port.(string)] = r.Id
 				rule["uuids"] = uuids
 			}
+		} else {
+			// No ports specified - create a rule that encompasses all ports
+			// by not setting startport and endport parameters
+			r, err := cs.Firewall.CreateEgressFirewallRule(p)
+			if err != nil {
+				return err
+			}
+			uuids["all"] = r.Id
+			rule["uuids"] = uuids
 		}
 	}
 
@@ -368,8 +453,74 @@ func resourceCloudStackEgressFirewallRead(d *schema.ResourceData, meta interface
 						rule["ports"] = ports
 						rules.Add(rule)
 					}
+				} else {
+					// No ports specified - check if we have an "all" rule
+					id, ok := uuids["all"]
+					if !ok {
+						continue
+					}
+
+					// Get the rule
+					r, ok := ruleMap[id.(string)]
+					if !ok {
+						delete(uuids, "all")
+						continue
+					}
+
+					// Verify this is actually an all-ports rule using our helper
+					if !isAllPortsTCPUDP(r.Protocol, r.Startport, r.Endport) {
+						// This rule has specific ports, but we expected all-ports
+						// This might happen if CloudStack behavior changed
+						continue
+					}
+
+					// Delete the known rule so only unknown rules remain in the ruleMap
+					delete(ruleMap, id.(string))
+
+					// Create a set with all CIDR's
+					cidrs := &schema.Set{F: schema.HashString}
+					for _, cidr := range strings.Split(r.Cidrlist, ",") {
+						cidrs.Add(cidr)
+					}
+
+					// Update the values
+					rule["protocol"] = r.Protocol
+					rule["cidr_list"] = cidrs
+					rules.Add(rule)
+				}
+			}
+
+			// Fallback: Check if any remaining rules in ruleMap match our expected all-ports pattern
+			// This handles cases where CloudStack might return all-ports rules in unexpected formats
+			if rule["protocol"].(string) != "icmp" && strings.ToLower(rule["protocol"].(string)) != "all" {
+				// Look for any remaining rules that might be our all-ports rule
+				for ruleID, r := range ruleMap {
+					// Get local CIDR set for comparison
+					localCidrSet, ok := rule["cidr_list"].(*schema.Set)
+					if !ok {
+						continue
+					}
+
+					if isAllPortsTCPUDP(r.Protocol, r.Startport, r.Endport) &&
+						strings.EqualFold(r.Protocol, rule["protocol"].(string)) &&
+						cidrSetsEqual(r.Cidrlist, localCidrSet) {
+						// This looks like our all-ports rule, add it to state
+						cidrs := &schema.Set{F: schema.HashString}
+						for _, cidr := range strings.Split(r.Cidrlist, ",") {
+							cidrs.Add(cidr)
+						}
+
+						rule["protocol"] = r.Protocol
+						rule["cidr_list"] = cidrs
+						rules.Add(rule)
+
+						// Remove from ruleMap so it's not processed again
+						delete(ruleMap, ruleID)
+						break
+					}
 				}
 			}
+
 			if strings.ToLower(rule["protocol"].(string)) == "all" {
 				id, ok := uuids["all"]
 				if !ok {
@@ -606,10 +757,9 @@ func verifyEgressFirewallRuleParams(d *schema.ResourceData, rule map[string]inte
 						"%q is not a valid port value. Valid options are '80' or '80-90'", port.(string))
 				}
 			}
-		} else {
-			return fmt.Errorf(
-				"Parameter ports is a required parameter when *not* using protocol 'icmp'")
 		}
+		// Note: ports parameter is optional for TCP/UDP protocols
+		// When omitted, the rule will encompass all ports
 	} else if strings.ToLower(protocol) == "all" {
 		if ports, _ := rule["ports"].(*schema.Set); ports.Len() > 0 {
 			return fmt.Errorf(

cloudstack/resource_cloudstack_egress_firewall_test.go

@@ -202,3 +202,197 @@ resource "cloudstack_egress_firewall" "foo" {
     ports = ["80", "1000-2000"]
   }
 }`
+
+func TestAccCloudStackEgressFirewall_allPortsTCP(t *testing.T) {
+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckCloudStackEgressFirewallDestroy,
+		Steps: []resource.TestStep{
+			{
+				Config: testAccCloudStackEgressFirewall_allPorts,
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckCloudStackEgressFirewallRulesExist("cloudstack_egress_firewall.foo"),
+					resource.TestCheckResourceAttr(
+						"cloudstack_egress_firewall.foo", "rule.#", "1"),
+					resource.TestCheckResourceAttr(
+						"cloudstack_egress_firewall.foo", "rule.0.cidr_list.0", "10.1.1.10/32"),
+					resource.TestCheckResourceAttr(
+						"cloudstack_egress_firewall.foo", "rule.0.protocol", "tcp"),
+					// No ports should be set when omitting the ports parameter
+					resource.TestCheckNoResourceAttr(
+						"cloudstack_egress_firewall.foo", "rule.0.ports"),
+				),
+			},
+		},
+	})
+}
+
+const testAccCloudStackEgressFirewall_allPorts = `
+resource "cloudstack_network" "foo" {
+  name = "terraform-network-tcp"
+  display_text = "terraform-network-tcp"
+  cidr = "10.1.1.0/24"
+  network_offering = "DefaultIsolatedNetworkOfferingWithSourceNatService"
+  zone = "Sandbox-simulator"
+}
+
+resource "cloudstack_egress_firewall" "foo" {
+  network_id = cloudstack_network.foo.id
+
+  rule {
+    cidr_list = ["10.1.1.10/32"]
+    protocol  = "tcp"
+    # No ports specified - should create a rule for all ports
+  }
+}`
+
+func TestAccCloudStackEgressFirewall_allPortsUDP(t *testing.T) {
+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckCloudStackEgressFirewallDestroy,
+		Steps: []resource.TestStep{
+			{
+				Config: testAccCloudStackEgressFirewall_allPortsUDP,
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckCloudStackEgressFirewallRulesExist("cloudstack_egress_firewall.foo"),
+					resource.TestCheckResourceAttr("cloudstack_egress_firewall.foo", "rule.#", "1"),
+					resource.TestCheckResourceAttr("cloudstack_egress_firewall.foo", "rule.0.protocol", "udp"),
+					resource.TestCheckNoResourceAttr("cloudstack_egress_firewall.foo", "rule.0.ports"),
+				),
+			},
+		},
+	})
+}
+
+const testAccCloudStackEgressFirewall_allPortsUDP = `
+resource "cloudstack_network" "foo" {
+  name             = "tf-egress-udp-all"
+  display_text     = "tf-egress-udp-all"
+  cidr             = "10.8.0.0/24"
+  network_offering = "DefaultIsolatedNetworkOfferingWithSourceNatService"
+  zone             = "Sandbox-simulator"
+}
+
+resource "cloudstack_egress_firewall" "foo" {
+  network_id = cloudstack_network.foo.id
+
+  rule {
+    cidr_list = ["10.8.0.10/32"]
+    protocol  = "udp"
+    # no ports => all ports
+  }
+}`
+
+func TestAccCloudStackEgressFirewall_allPortsCombined(t *testing.T) {
+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckCloudStackEgressFirewallDestroy,
+		Steps: []resource.TestStep{
+			{
+				Config: testAccCloudStackEgressFirewall_allPortsCombined,
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckCloudStackEgressFirewallRulesExist("cloudstack_egress_firewall.mixed"),
+					resource.TestCheckResourceAttr("cloudstack_egress_firewall.mixed", "rule.#", "2"),
+					resource.TestCheckResourceAttr("cloudstack_egress_firewall.mixed", "rule.0.protocol", "tcp"),
+					resource.TestCheckResourceAttr("cloudstack_egress_firewall.mixed", "rule.0.ports.#", "2"),
+					resource.TestCheckResourceAttr("cloudstack_egress_firewall.mixed", "rule.1.protocol", "udp"),
+					resource.TestCheckNoResourceAttr("cloudstack_egress_firewall.mixed", "rule.1.ports"),
+				),
+			},
+		},
+	})
+}
+
+const testAccCloudStackEgressFirewall_allPortsCombined = `
+resource "cloudstack_network" "foo" {
+  name             = "terraform-network-mixed"
+  display_text     = "terraform-network-mixed"
+  cidr             = "10.1.3.0/24"
+  network_offering = "DefaultIsolatedNetworkOfferingWithSourceNatService"
+  zone             = "Sandbox-simulator"
+}
+
+resource "cloudstack_egress_firewall" "mixed" {
+  network_id = cloudstack_network.foo.id
+
+  rule {
+    cidr_list = ["10.0.0.0/8"]
+    protocol  = "tcp"
+    ports     = ["80", "443"]
+  }
+
+  rule {
+    cidr_list = ["10.1.0.0/16"]
+    protocol  = "udp"
+    # no ports => all ports
+  }
+}`
+
+func TestAccCloudStackEgressFirewall_portsToAllPorts(t *testing.T) {
+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckCloudStackEgressFirewallDestroy,
+		Steps: []resource.TestStep{
+			{
+				Config: testAccCloudStackEgressFirewall_specificPorts,
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckCloudStackEgressFirewallRulesExist("cloudstack_egress_firewall.foo"),
+					resource.TestCheckResourceAttr("cloudstack_egress_firewall.foo", "rule.#", "1"),
+					resource.TestCheckResourceAttr("cloudstack_egress_firewall.foo", "rule.0.ports.#", "2"),
+				),
+			},
+			{
+				Config: testAccCloudStackEgressFirewall_allPortsTransition,
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckCloudStackEgressFirewallRulesExist("cloudstack_egress_firewall.foo"),
+					resource.TestCheckResourceAttr("cloudstack_egress_firewall.foo", "rule.#", "1"),
+					resource.TestCheckNoResourceAttr("cloudstack_egress_firewall.foo", "rule.0.ports"),
+				),
+			},
+		},
+	})
+}
+
+const testAccCloudStackEgressFirewall_specificPorts = `
+resource "cloudstack_network" "foo" {
+  name             = "terraform-network-transition"
+  display_text     = "terraform-network-transition"
+  cidr             = "10.1.4.0/24"
+  network_offering = "DefaultIsolatedNetworkOfferingWithSourceNatService"
+  zone             = "Sandbox-simulator"
+}
+
+resource "cloudstack_egress_firewall" "foo" {
+  network_id = cloudstack_network.foo.id
+
+  rule {
+    cidr_list = ["10.1.1.10/32"]
+    protocol  = "tcp"
+    ports     = ["80", "1000-2000"]
+  }
+}
+`
+
+const testAccCloudStackEgressFirewall_allPortsTransition = `
+resource "cloudstack_network" "foo" {
+  name             = "terraform-network-transition"
+  display_text     = "terraform-network-transition"
+  cidr             = "10.1.4.0/24"
+  network_offering = "DefaultIsolatedNetworkOfferingWithSourceNatService"
+  zone             = "Sandbox-simulator"
+}
+
+resource "cloudstack_egress_firewall" "foo" {
+  network_id = cloudstack_network.foo.id
+
+  rule {
+    cidr_list = ["10.1.1.10/32"]
+    protocol  = "tcp"
+    # no ports => all ports
+  }
+}
+`