Skip to content

Commit e16d5a2

Browse files
sidshas03sidshas03
committed
egress: robust TCP/UDP all-ports read (0/0, -1/-1, 1/65535) + CIDR-set match; UDP acc test; docs (Fixes #202)
1 parent afa7a18 commit e16d5a2

File tree

3 files changed

+398
-4
lines changed

3 files changed

+398
-4
lines changed

‎cloudstack/resource_cloudstack_egress_firewall.go‎

Lines changed: 153 additions & 3 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,
@@ -250,6 +326,15 @@ func createEgressFirewallRule(d *schema.ResourceData, meta interface{}, rule map
250326
uuids[port.(string)] = r.Id
251327
rule["uuids"] = uuids
252328
}
329+
} else {
330+
// No ports specified - create a rule that encompasses all ports
331+
// by not setting startport and endport parameters
332+
r, err := cs.Firewall.CreateEgressFirewallRule(p)
333+
if err != nil {
334+
return err
335+
}
336+
uuids["all"] = r.Id
337+
rule["uuids"] = uuids
253338
}
254339
}
255340

@@ -368,8 +453,74 @@ func resourceCloudStackEgressFirewallRead(d *schema.ResourceData, meta interface
368453
rule["ports"] = ports
369454
rules.Add(rule)
370455
}
456+
} else {
457+
// No ports specified - check if we have an "all" rule
458+
id, ok := uuids["all"]
459+
if !ok {
460+
continue
461+
}
462+
463+
// Get the rule
464+
r, ok := ruleMap[id.(string)]
465+
if !ok {
466+
delete(uuids, "all")
467+
continue
468+
}
469+
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+
477+
// Delete the known rule so only unknown rules remain in the ruleMap
478+
delete(ruleMap, id.(string))
479+
480+
// Create a set with all CIDR's
481+
cidrs := &schema.Set{F: schema.HashString}
482+
for _, cidr := range strings.Split(r.Cidrlist, ",") {
483+
cidrs.Add(cidr)
484+
}
485+
486+
// Update the values
487+
rule["protocol"] = r.Protocol
488+
rule["cidr_list"] = cidrs
489+
rules.Add(rule)
490+
}
491+
}
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+
}
371521
}
372522
}
523+
373524
if strings.ToLower(rule["protocol"].(string)) == "all" {
374525
id, ok := uuids["all"]
375526
if !ok {
@@ -606,10 +757,9 @@ func verifyEgressFirewallRuleParams(d *schema.ResourceData, rule map[string]inte
606757
"%q is not a valid port value. Valid options are '80' or '80-90'", port.(string))
607758
}
608759
}
609-
} else {
610-
return fmt.Errorf(
611-
"Parameter ports is a required parameter when *not* using protocol 'icmp'")
612760
}
761+
// Note: ports parameter is optional for TCP/UDP protocols
762+
// When omitted, the rule will encompass all ports
613763
} else if strings.ToLower(protocol) == "all" {
614764
if ports, _ := rule["ports"].(*schema.Set); ports.Len() > 0 {
615765
return fmt.Errorf(

‎cloudstack/resource_cloudstack_egress_firewall_test.go‎

Lines changed: 194 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -202,3 +202,197 @@ resource "cloudstack_egress_firewall" "foo" {
202202
ports = ["80", "1000-2000"]
203203
}
204204
}`
205+
206+
func TestAccCloudStackEgressFirewall_allPortsTCP(t *testing.T) {
207+
resource.Test(t, resource.TestCase{
208+
PreCheck: func() { testAccPreCheck(t) },
209+
Providers: testAccProviders,
210+
CheckDestroy: testAccCheckCloudStackEgressFirewallDestroy,
211+
Steps: []resource.TestStep{
212+
{
213+
Config: testAccCloudStackEgressFirewall_allPorts,
214+
Check: resource.ComposeTestCheckFunc(
215+
testAccCheckCloudStackEgressFirewallRulesExist("cloudstack_egress_firewall.foo"),
216+
resource.TestCheckResourceAttr(
217+
"cloudstack_egress_firewall.foo", "rule.#", "1"),
218+
resource.TestCheckResourceAttr(
219+
"cloudstack_egress_firewall.foo", "rule.0.cidr_list.0", "10.1.1.10/32"),
220+
resource.TestCheckResourceAttr(
221+
"cloudstack_egress_firewall.foo", "rule.0.protocol", "tcp"),
222+
// No ports should be set when omitting the ports parameter
223+
resource.TestCheckNoResourceAttr(
224+
"cloudstack_egress_firewall.foo", "rule.0.ports"),
225+
),
226+
},
227+
},
228+
})
229+
}
230+
231+
const testAccCloudStackEgressFirewall_allPorts = `
232+
resource "cloudstack_network" "foo" {
233+
name = "terraform-network-tcp"
234+
display_text = "terraform-network-tcp"
235+
cidr = "10.1.1.0/24"
236+
network_offering = "DefaultIsolatedNetworkOfferingWithSourceNatService"
237+
zone = "Sandbox-simulator"
238+
}
239+
240+
resource "cloudstack_egress_firewall" "foo" {
241+
network_id = cloudstack_network.foo.id
242+
243+
rule {
244+
cidr_list = ["10.1.1.10/32"]
245+
protocol = "tcp"
246+
# No ports specified - should create a rule for all ports
247+
}
248+
}`
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+
`

0 commit comments

Comments
 (0)