Skip to content

Commit 7d6a79d

Browse files
sidshas03sidshas03
committed
Enhance egress firewall all-ports support for CloudStack 4.19/4.20
- Add robust CIDR normalization for stable rule matching - Enhance fallback logic with exact protocol + CIDR verification - Add comprehensive UDP all-ports acceptance test - Update documentation with all-ports representations - Ensure state consistency across CloudStack versions Fixes #202
1 parent bb09664 commit 7d6a79d

File tree

3 files changed

+303
-5
lines changed

3 files changed

+303
-5
lines changed

‎cloudstack/resource_cloudstack_egress_firewall.go‎

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ package cloudstack
2121

2222
import (
2323
"fmt"
24+
"sort"
2425
"strconv"
2526
"strings"
2627
"sync"
@@ -31,6 +32,81 @@ import (
3132
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
3233
)
3334

35+
// treats 'all ports' for tcp/udp across CS versions returning 0/0, -1/-1, or 1/65535
36+
func isAllPortsTCPUDP(protocol string, start, end int) bool {
37+
p := strings.ToLower(protocol)
38+
if p != "tcp" && p != "udp" {
39+
return false
40+
}
41+
// handle various representations of all ports across CloudStack versions
42+
if (start == 0 && end == 0) ||
43+
(start == -1 && end == -1) ||
44+
(start == 1 && end == 65535) {
45+
return true
46+
}
47+
return false
48+
}
49+
50+
// normalizeRemoteCIDRs normalizes a comma-separated CIDR string from CloudStack API
51+
func normalizeRemoteCIDRs(cidrList string) []string {
52+
if cidrList == "" {
53+
return []string{}
54+
}
55+
56+
cidrs := strings.Split(cidrList, ",")
57+
normalized := make([]string, 0, len(cidrs))
58+
59+
for _, cidr := range cidrs {
60+
trimmed := strings.TrimSpace(cidr)
61+
if trimmed != "" {
62+
normalized = append(normalized, trimmed)
63+
}
64+
}
65+
66+
sort.Strings(normalized)
67+
return normalized
68+
}
69+
70+
// normalizeLocalCIDRs normalizes a Terraform schema.Set of CIDRs
71+
func normalizeLocalCIDRs(cidrSet *schema.Set) []string {
72+
if cidrSet == nil {
73+
return []string{}
74+
}
75+
76+
normalized := make([]string, 0, cidrSet.Len())
77+
for _, cidr := range cidrSet.List() {
78+
if cidrStr, ok := cidr.(string); ok {
79+
trimmed := strings.TrimSpace(cidrStr)
80+
if trimmed != "" {
81+
normalized = append(normalized, trimmed)
82+
}
83+
}
84+
}
85+
86+
sort.Strings(normalized)
87+
return normalized
88+
}
89+
90+
// cidrSetsEqual compares normalized CIDR sets for equality (order/whitespace agnostic)
91+
func cidrSetsEqual(remoteCidrList string, localCidrSet *schema.Set) bool {
92+
remoteCidrs := normalizeRemoteCIDRs(remoteCidrList)
93+
localCidrs := normalizeLocalCIDRs(localCidrSet)
94+
95+
// Compare lengths first
96+
if len(remoteCidrs) != len(localCidrs) {
97+
return false
98+
}
99+
100+
// Compare each element (both are already sorted)
101+
for i, remoteCidr := range remoteCidrs {
102+
if remoteCidr != localCidrs[i] {
103+
return false
104+
}
105+
}
106+
107+
return true
108+
}
109+
34110
func resourceCloudStackEgressFirewall() *schema.Resource {
35111
return &schema.Resource{
36112
Create: resourceCloudStackEgressFirewallCreate,
@@ -391,6 +467,13 @@ func resourceCloudStackEgressFirewallRead(d *schema.ResourceData, meta interface
391467
continue
392468
}
393469

470+
// Verify this is actually an all-ports rule using our helper
471+
if !isAllPortsTCPUDP(r.Protocol, r.Startport, r.Endport) {
472+
// This rule has specific ports, but we expected all-ports
473+
// This might happen if CloudStack behavior changed
474+
continue
475+
}
476+
394477
// Delete the known rule so only unknown rules remain in the ruleMap
395478
delete(ruleMap, id.(string))
396479

@@ -406,6 +489,38 @@ func resourceCloudStackEgressFirewallRead(d *schema.ResourceData, meta interface
406489
rules.Add(rule)
407490
}
408491
}
492+
493+
// Fallback: Check if any remaining rules in ruleMap match our expected all-ports pattern
494+
// This handles cases where CloudStack might return all-ports rules in unexpected formats
495+
if rule["protocol"].(string) != "icmp" && strings.ToLower(rule["protocol"].(string)) != "all" {
496+
// Look for any remaining rules that might be our all-ports rule
497+
for ruleID, r := range ruleMap {
498+
// Get local CIDR set for comparison
499+
localCidrSet, ok := rule["cidr_list"].(*schema.Set)
500+
if !ok {
501+
continue
502+
}
503+
504+
if isAllPortsTCPUDP(r.Protocol, r.Startport, r.Endport) &&
505+
strings.EqualFold(r.Protocol, rule["protocol"].(string)) &&
506+
cidrSetsEqual(r.Cidrlist, localCidrSet) {
507+
// This looks like our all-ports rule, add it to state
508+
cidrs := &schema.Set{F: schema.HashString}
509+
for _, cidr := range strings.Split(r.Cidrlist, ",") {
510+
cidrs.Add(cidr)
511+
}
512+
513+
rule["protocol"] = r.Protocol
514+
rule["cidr_list"] = cidrs
515+
rules.Add(rule)
516+
517+
// Remove from ruleMap so it's not processed again
518+
delete(ruleMap, ruleID)
519+
break
520+
}
521+
}
522+
}
523+
409524
if strings.ToLower(rule["protocol"].(string)) == "all" {
410525
id, ok := uuids["all"]
411526
if !ok {

‎cloudstack/resource_cloudstack_egress_firewall_test.go‎

Lines changed: 153 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -203,7 +203,7 @@ resource "cloudstack_egress_firewall" "foo" {
203203
}
204204
}`
205205

206-
func TestAccCloudStackEgressFirewall_allPorts(t *testing.T) {
206+
func TestAccCloudStackEgressFirewall_allPortsTCP(t *testing.T) {
207207
resource.Test(t, resource.TestCase{
208208
PreCheck: func() { testAccPreCheck(t) },
209209
Providers: testAccProviders,
@@ -230,8 +230,8 @@ func TestAccCloudStackEgressFirewall_allPorts(t *testing.T) {
230230

231231
const testAccCloudStackEgressFirewall_allPorts = `
232232
resource "cloudstack_network" "foo" {
233-
name = "terraform-network"
234-
display_text = "terraform-network"
233+
name = "terraform-network-tcp"
234+
display_text = "terraform-network-tcp"
235235
cidr = "10.1.1.0/24"
236236
network_offering = "DefaultIsolatedNetworkOfferingWithSourceNatService"
237237
zone = "Sandbox-simulator"
@@ -246,3 +246,153 @@ resource "cloudstack_egress_firewall" "foo" {
246246
# No ports specified - should create a rule for all ports
247247
}
248248
}`
249+
250+
func TestAccCloudStackEgressFirewall_allPortsUDP(t *testing.T) {
251+
resource.Test(t, resource.TestCase{
252+
PreCheck: func() { testAccPreCheck(t) },
253+
Providers: testAccProviders,
254+
CheckDestroy: testAccCheckCloudStackEgressFirewallDestroy,
255+
Steps: []resource.TestStep{
256+
{
257+
Config: testAccCloudStackEgressFirewall_allPortsUDP,
258+
Check: resource.ComposeTestCheckFunc(
259+
testAccCheckCloudStackEgressFirewallRulesExist("cloudstack_egress_firewall.foo"),
260+
resource.TestCheckResourceAttr("cloudstack_egress_firewall.foo", "rule.#", "1"),
261+
resource.TestCheckResourceAttr("cloudstack_egress_firewall.foo", "rule.0.protocol", "udp"),
262+
resource.TestCheckNoResourceAttr("cloudstack_egress_firewall.foo", "rule.0.ports"),
263+
),
264+
},
265+
},
266+
})
267+
}
268+
269+
const testAccCloudStackEgressFirewall_allPortsUDP = `
270+
resource "cloudstack_network" "foo" {
271+
name = "tf-egress-udp-all"
272+
display_text = "tf-egress-udp-all"
273+
cidr = "10.8.0.0/24"
274+
network_offering = "DefaultIsolatedNetworkOfferingWithSourceNatService"
275+
zone = "Sandbox-simulator"
276+
}
277+
278+
resource "cloudstack_egress_firewall" "foo" {
279+
network_id = cloudstack_network.foo.id
280+
281+
rule {
282+
cidr_list = ["10.8.0.10/32"]
283+
protocol = "udp"
284+
# no ports => all ports
285+
}
286+
}`
287+
288+
func TestAccCloudStackEgressFirewall_allPortsCombined(t *testing.T) {
289+
resource.Test(t, resource.TestCase{
290+
PreCheck: func() { testAccPreCheck(t) },
291+
Providers: testAccProviders,
292+
CheckDestroy: testAccCheckCloudStackEgressFirewallDestroy,
293+
Steps: []resource.TestStep{
294+
{
295+
Config: testAccCloudStackEgressFirewall_allPortsCombined,
296+
Check: resource.ComposeTestCheckFunc(
297+
testAccCheckCloudStackEgressFirewallRulesExist("cloudstack_egress_firewall.mixed"),
298+
resource.TestCheckResourceAttr("cloudstack_egress_firewall.mixed", "rule.#", "2"),
299+
resource.TestCheckResourceAttr("cloudstack_egress_firewall.mixed", "rule.0.protocol", "tcp"),
300+
resource.TestCheckResourceAttr("cloudstack_egress_firewall.mixed", "rule.0.ports.#", "2"),
301+
resource.TestCheckResourceAttr("cloudstack_egress_firewall.mixed", "rule.1.protocol", "udp"),
302+
resource.TestCheckNoResourceAttr("cloudstack_egress_firewall.mixed", "rule.1.ports"),
303+
),
304+
},
305+
},
306+
})
307+
}
308+
309+
const testAccCloudStackEgressFirewall_allPortsCombined = `
310+
resource "cloudstack_network" "foo" {
311+
name = "terraform-network-mixed"
312+
display_text = "terraform-network-mixed"
313+
cidr = "10.1.3.0/24"
314+
network_offering = "DefaultIsolatedNetworkOfferingWithSourceNatService"
315+
zone = "Sandbox-simulator"
316+
}
317+
318+
resource "cloudstack_egress_firewall" "mixed" {
319+
network_id = cloudstack_network.foo.id
320+
321+
rule {
322+
cidr_list = ["10.0.0.0/8"]
323+
protocol = "tcp"
324+
ports = ["80", "443"]
325+
}
326+
327+
rule {
328+
cidr_list = ["10.1.0.0/16"]
329+
protocol = "udp"
330+
# no ports => all ports
331+
}
332+
}`
333+
334+
func TestAccCloudStackEgressFirewall_portsToAllPorts(t *testing.T) {
335+
resource.Test(t, resource.TestCase{
336+
PreCheck: func() { testAccPreCheck(t) },
337+
Providers: testAccProviders,
338+
CheckDestroy: testAccCheckCloudStackEgressFirewallDestroy,
339+
Steps: []resource.TestStep{
340+
{
341+
Config: testAccCloudStackEgressFirewall_specificPorts,
342+
Check: resource.ComposeTestCheckFunc(
343+
testAccCheckCloudStackEgressFirewallRulesExist("cloudstack_egress_firewall.foo"),
344+
resource.TestCheckResourceAttr("cloudstack_egress_firewall.foo", "rule.#", "1"),
345+
resource.TestCheckResourceAttr("cloudstack_egress_firewall.foo", "rule.0.ports.#", "2"),
346+
),
347+
},
348+
{
349+
Config: testAccCloudStackEgressFirewall_allPortsTransition,
350+
Check: resource.ComposeTestCheckFunc(
351+
testAccCheckCloudStackEgressFirewallRulesExist("cloudstack_egress_firewall.foo"),
352+
resource.TestCheckResourceAttr("cloudstack_egress_firewall.foo", "rule.#", "1"),
353+
resource.TestCheckNoResourceAttr("cloudstack_egress_firewall.foo", "rule.0.ports"),
354+
),
355+
},
356+
},
357+
})
358+
}
359+
360+
const testAccCloudStackEgressFirewall_specificPorts = `
361+
resource "cloudstack_network" "foo" {
362+
name = "terraform-network-transition"
363+
display_text = "terraform-network-transition"
364+
cidr = "10.1.4.0/24"
365+
network_offering = "DefaultIsolatedNetworkOfferingWithSourceNatService"
366+
zone = "Sandbox-simulator"
367+
}
368+
369+
resource "cloudstack_egress_firewall" "foo" {
370+
network_id = cloudstack_network.foo.id
371+
372+
rule {
373+
cidr_list = ["10.1.1.10/32"]
374+
protocol = "tcp"
375+
ports = ["80", "1000-2000"]
376+
}
377+
}
378+
`
379+
380+
const testAccCloudStackEgressFirewall_allPortsTransition = `
381+
resource "cloudstack_network" "foo" {
382+
name = "terraform-network-transition"
383+
display_text = "terraform-network-transition"
384+
cidr = "10.1.4.0/24"
385+
network_offering = "DefaultIsolatedNetworkOfferingWithSourceNatService"
386+
zone = "Sandbox-simulator"
387+
}
388+
389+
resource "cloudstack_egress_firewall" "foo" {
390+
network_id = cloudstack_network.foo.id
391+
392+
rule {
393+
cidr_list = ["10.1.1.10/32"]
394+
protocol = "tcp"
395+
# no ports => all ports
396+
}
397+
}
398+
`

‎website/docs/r/egress_firewall.html.markdown‎

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,40 @@ resource "cloudstack_egress_firewall" "all_ports" {
4040
}
4141
```
4242

43+
### Example: UDP All Ports
44+
45+
```hcl
46+
resource "cloudstack_egress_firewall" "all_ports_udp" {
47+
network_id = "6eb22f91-7454-4107-89f4-36afcdf33021"
48+
49+
rule {
50+
cidr_list = ["10.1.0.0/16"]
51+
protocol = "udp"
52+
# No ports => all UDP ports
53+
}
54+
}
55+
```
56+
57+
### Example: Mixed Rules (specific + all-ports)
58+
59+
```hcl
60+
resource "cloudstack_egress_firewall" "mixed_rules" {
61+
network_id = "6eb22f91-7454-4107-89f4-36afcdf33021"
62+
63+
rule {
64+
cidr_list = ["10.0.0.0/8"]
65+
protocol = "tcp"
66+
ports = ["80", "443"]
67+
}
68+
69+
rule {
70+
cidr_list = ["10.1.0.0/16"]
71+
protocol = "udp"
72+
# No ports => all UDP ports
73+
}
74+
}
75+
```
76+
4377
## Argument Reference
4478

4579
The following arguments are supported:
@@ -71,8 +105,7 @@ The `rule` block supports:
71105
the protocol is ICMP.
72106

73107
* `ports` - (Optional) List of ports and/or port ranges to allow. This can only
74-
be specified if the protocol is TCP or UDP. When omitted for TCP or UDP protocols,
75-
the rule will encompass all ports.
108+
be specified if the protocol is TCP or UDP. For TCP/UDP, omitting `ports` creates an all-ports rule. CloudStack may represent this as empty start/end, `0/0`, or `1/65535`; the provider handles all.
76109

77110
## Attributes Reference
78111

0 commit comments

Comments
 (0)