From d4ce2b87fb18816061812cc55d2181f5f4abc19d Mon Sep 17 00:00:00 2001 From: Sander van Harmelen Date: Fri, 27 Nov 2015 14:49:28 +0100 Subject: [PATCH 1/3] Modified executable bit --- builtin/providers/cloudstack/resource_cloudstack_template_test.go | 0 1 file changed, 0 insertions(+), 0 deletions(-) mode change 100755 => 100644 builtin/providers/cloudstack/resource_cloudstack_template_test.go diff --git a/builtin/providers/cloudstack/resource_cloudstack_template_test.go b/builtin/providers/cloudstack/resource_cloudstack_template_test.go old mode 100755 new mode 100644 From 84645bd8b5a71d060563f16855044248b1ea673d Mon Sep 17 00:00:00 2001 From: Sander van Harmelen Date: Tue, 1 Dec 2015 00:36:33 +0100 Subject: [PATCH 2/3] More tweaks to improve performance --- builtin/providers/cloudstack/provider.go | 2 +- .../resource_cloudstack_egress_firewall.go | 20 +- .../resource_cloudstack_firewall.go | 20 +- .../resource_cloudstack_network_acl_rule.go | 195 +++++++++++++----- .../resource_cloudstack_port_forward.go | 70 +++---- builtin/providers/cloudstack/resources.go | 6 +- 6 files changed, 196 insertions(+), 117 deletions(-) diff --git a/builtin/providers/cloudstack/provider.go b/builtin/providers/cloudstack/provider.go index c7ce67ff0..ac2f0f521 100644 --- a/builtin/providers/cloudstack/provider.go +++ b/builtin/providers/cloudstack/provider.go @@ -36,7 +36,7 @@ func Provider() terraform.ResourceProvider { "timeout": &schema.Schema{ Type: schema.TypeInt, Required: true, - DefaultFunc: schema.EnvDefaultFunc("CLOUDSTACK_TIMEOUT", 300), + DefaultFunc: schema.EnvDefaultFunc("CLOUDSTACK_TIMEOUT", 900), }, }, diff --git a/builtin/providers/cloudstack/resource_cloudstack_egress_firewall.go b/builtin/providers/cloudstack/resource_cloudstack_egress_firewall.go index 37979e13f..a1d73b167 100644 --- a/builtin/providers/cloudstack/resource_cloudstack_egress_firewall.go +++ b/builtin/providers/cloudstack/resource_cloudstack_egress_firewall.go @@ -301,21 +301,17 @@ func resourceCloudStackEgressFirewallRead(d *schema.ResourceData, meta interface // If this is a managed firewall, add all unknown rules into a single dummy rule managed := d.Get("managed").(bool) if managed && len(ruleMap) > 0 { - // Add all UUIDs to a uuids map - uuids := make(map[string]interface{}, len(ruleMap)) for uuid := range ruleMap { - uuids[uuid] = uuid - } + // Make a dummy rule to hold the unknown UUID + rule := map[string]interface{}{ + "source_cidr": uuid, + "protocol": uuid, + "uuids": map[string]interface{}{uuid: uuid}, + } - // Make a dummy rule to hold all unknown UUIDs - rule := map[string]interface{}{ - "source_cidr": "N/A", - "protocol": "N/A", - "uuids": ruleMap, + // Add the dummy rule to the rules set + rules.Add(rule) } - - // Add the dummy rule to the rules set - rules.Add(rule) } if rules.Len() > 0 { diff --git a/builtin/providers/cloudstack/resource_cloudstack_firewall.go b/builtin/providers/cloudstack/resource_cloudstack_firewall.go index 3bcced02e..c5a8f8763 100644 --- a/builtin/providers/cloudstack/resource_cloudstack_firewall.go +++ b/builtin/providers/cloudstack/resource_cloudstack_firewall.go @@ -301,21 +301,17 @@ func resourceCloudStackFirewallRead(d *schema.ResourceData, meta interface{}) er // If this is a managed firewall, add all unknown rules into a single dummy rule managed := d.Get("managed").(bool) if managed && len(ruleMap) > 0 { - // Add all UUIDs to a uuids map - uuids := make(map[string]interface{}, len(ruleMap)) for uuid := range ruleMap { - uuids[uuid] = uuid - } + // Make a dummy rule to hold the unknown UUID + rule := map[string]interface{}{ + "source_cidr": uuid, + "protocol": uuid, + "uuids": map[string]interface{}{uuid: uuid}, + } - // Make a dummy rule to hold all unknown UUIDs - rule := map[string]interface{}{ - "source_cidr": "N/A", - "protocol": "N/A", - "uuids": uuids, + // Add the dummy rule to the rules set + rules.Add(rule) } - - // Add the dummy rule to the rules set - rules.Add(rule) } if rules.Len() > 0 { diff --git a/builtin/providers/cloudstack/resource_cloudstack_network_acl_rule.go b/builtin/providers/cloudstack/resource_cloudstack_network_acl_rule.go index 18446738a..10c91de69 100644 --- a/builtin/providers/cloudstack/resource_cloudstack_network_acl_rule.go +++ b/builtin/providers/cloudstack/resource_cloudstack_network_acl_rule.go @@ -7,7 +7,10 @@ import ( "sort" "strconv" "strings" + "sync" + "time" + "github.com/hashicorp/go-multierror" "github.com/hashicorp/terraform/helper/hashcode" "github.com/hashicorp/terraform/helper/schema" "github.com/xanzy/go-cloudstack/cloudstack" @@ -103,32 +106,72 @@ func resourceCloudStackNetworkACLRuleCreate(d *schema.ResourceData, meta interfa d.SetId(d.Get("aclid").(string)) // Create all rules that are configured - if rs := d.Get("rule").(*schema.Set); rs.Len() > 0 { - - // Create an empty schema.Set to hold all rules + if nrs := d.Get("rule").(*schema.Set); nrs.Len() > 0 { + // Create an empty rule set to hold all newly created rules rules := &schema.Set{ F: resourceCloudStackNetworkACLRuleHash, } - for _, rule := range rs.List() { - // Create a single rule - err := resourceCloudStackNetworkACLRuleCreateRule(d, meta, rule.(map[string]interface{})) + err := resourceCloudStackNetworkACLRuleCreateRules(d, meta, rules, nrs) - // We need to update this first to preserve the correct state - rules.Add(rule) - d.Set("rule", rules) + // We need to update this first to preserve the correct state + d.Set("rule", rules) - if err != nil { - return err - } + if err != nil { + return err } } return resourceCloudStackNetworkACLRuleRead(d, meta) } +func resourceCloudStackNetworkACLRuleCreateRules( + d *schema.ResourceData, + meta interface{}, + rules *schema.Set, + nrs *schema.Set) error { + var errs *multierror.Error + + var wg sync.WaitGroup + wg.Add(nrs.Len()) + + sem := make(chan struct{}, 10) + for _, rule := range nrs.List() { + // Put in a tiny sleep here to avoid DoS'ing the API + time.Sleep(500 * time.Millisecond) + + go func(rule map[string]interface{}) { + defer wg.Done() + sem <- struct{}{} + + // Create a single rule + err := resourceCloudStackNetworkACLRuleCreateRule(d, meta, rule) + + // If we have at least one UUID, we need to save the rule + if len(rule["uuids"].(map[string]interface{})) > 0 { + rules.Add(rule) + } + + if err != nil { + errs = multierror.Append(errs, err) + } + + <-sem + }(rule.(map[string]interface{})) + } + + wg.Wait() + + // We need to update this first to preserve the correct state + d.Set("rule", rules) + + return errs.ErrorOrNil() +} + func resourceCloudStackNetworkACLRuleCreateRule( - d *schema.ResourceData, meta interface{}, rule map[string]interface{}) error { + d *schema.ResourceData, + meta interface{}, + rule map[string]interface{}) error { cs := meta.(*cloudstack.CloudStackClient) uuids := rule["uuids"].(map[string]interface{}) @@ -188,8 +231,16 @@ func resourceCloudStackNetworkACLRuleCreateRule( }, } + // Define a regexp for parsing the port + re := regexp.MustCompile(`^(\d+)(?:-(\d+))?$`) + for _, port := range ps.List() { - re := regexp.MustCompile(`^(\d+)(?:-(\d+))?$`) + if _, ok := uuids[port.(string)]; ok { + ports.Add(port) + rule["ports"] = ports + continue + } + m := re.FindStringSubmatch(port.(string)) startPort, err := strconv.Atoi(m[1]) @@ -354,20 +405,17 @@ func resourceCloudStackNetworkACLRuleRead(d *schema.ResourceData, meta interface // If this is a managed firewall, add all unknown rules into a single dummy rule managed := d.Get("managed").(bool) if managed && len(ruleMap) > 0 { - // Add all UUIDs to a uuids map - uuids := make(map[string]interface{}, len(ruleMap)) for uuid := range ruleMap { - uuids[uuid] = uuid - } + // Make a dummy rule to hold the unknown UUID + rule := map[string]interface{}{ + "source_cidr": uuid, + "protocol": uuid, + "uuids": map[string]interface{}{uuid: uuid}, + } - rule := map[string]interface{}{ - "source_cidr": "N/A", - "protocol": "N/A", - "uuids": uuids, + // Add the dummy rule to the rules set + rules.Add(rule) } - - // Add the dummy rule to the rules set - rules.Add(rule) } if rules.Len() > 0 { @@ -391,26 +439,29 @@ func resourceCloudStackNetworkACLRuleUpdate(d *schema.ResourceData, meta interfa ors := o.(*schema.Set).Difference(n.(*schema.Set)) nrs := n.(*schema.Set).Difference(o.(*schema.Set)) - // Now first loop through all the old rules and delete any obsolete ones - for _, rule := range ors.List() { - // Delete the rule as it no longer exists in the config - err := resourceCloudStackNetworkACLRuleDeleteRule(d, meta, rule.(map[string]interface{})) + // We need to start with a rule set containing all the rules we + // already have and want to keep. Any rules that are not deleted + // correctly and any newly created rules, will be added to this + // set to make sure we end up in a consistent state + rules := o.(*schema.Set).Intersection(n.(*schema.Set)) + + // Now first loop through all the old rules and delete them + if ors.Len() > 0 { + err := resourceCloudStackNetworkACLRuleDeleteRules(d, meta, rules, ors) + + // We need to update this first to preserve the correct state + d.Set("rule", rules) + if err != nil { return err } } - // Make sure we save the state of the currently configured rules - rules := o.(*schema.Set).Intersection(n.(*schema.Set)) - d.Set("rule", rules) - - // Then loop through all the currently configured rules and create the new ones - for _, rule := range nrs.List() { - // When successfully deleted, re-create it again if it still exists - err := resourceCloudStackNetworkACLRuleCreateRule(d, meta, rule.(map[string]interface{})) + // Then loop through all the new rules and create them + if nrs.Len() > 0 { + err := resourceCloudStackNetworkACLRuleCreateRules(d, meta, rules, nrs) // We need to update this first to preserve the correct state - rules.Add(rule) d.Set("rule", rules) if err != nil { @@ -423,26 +474,71 @@ func resourceCloudStackNetworkACLRuleUpdate(d *schema.ResourceData, meta interfa } func resourceCloudStackNetworkACLRuleDelete(d *schema.ResourceData, meta interface{}) error { + // Create an empty rule set to hold all rules that where + // not deleted correctly + rules := &schema.Set{ + F: resourceCloudStackNetworkACLRuleHash, + } + // Delete all rules if rs := d.Get("rule").(*schema.Set); rs.Len() > 0 { - for _, rule := range rs.List() { - // Delete a single rule - err := resourceCloudStackNetworkACLRuleDeleteRule(d, meta, rule.(map[string]interface{})) + err := resourceCloudStackNetworkACLRuleDeleteRules(d, meta, rules, rs) - // We need to update this first to preserve the correct state - d.Set("rule", rs) + // We need to update this first to preserve the correct state + d.Set("rule", rules) - if err != nil { - return err - } + if err != nil { + return err } } return nil } +func resourceCloudStackNetworkACLRuleDeleteRules( + d *schema.ResourceData, + meta interface{}, + rules *schema.Set, + ors *schema.Set) error { + var errs *multierror.Error + + var wg sync.WaitGroup + wg.Add(ors.Len()) + + sem := make(chan struct{}, 10) + for _, rule := range ors.List() { + // Put a sleep here to avoid DoS'ing the API + time.Sleep(500 * time.Millisecond) + + go func(rule map[string]interface{}) { + defer wg.Done() + sem <- struct{}{} + + // Delete a single rule + err := resourceCloudStackNetworkACLRuleDeleteRule(d, meta, rule) + + // If we have at least one UUID, we need to save the rule + if len(rule["uuids"].(map[string]interface{})) > 0 { + rules.Add(rule) + } + + if err != nil { + errs = multierror.Append(errs, err) + } + + <-sem + }(rule.(map[string]interface{})) + } + + wg.Wait() + + return errs.ErrorOrNil() +} + func resourceCloudStackNetworkACLRuleDeleteRule( - d *schema.ResourceData, meta interface{}, rule map[string]interface{}) error { + d *schema.ResourceData, + meta interface{}, + rule map[string]interface{}) error { cs := meta.(*cloudstack.CloudStackClient) uuids := rule["uuids"].(map[string]interface{}) @@ -463,6 +559,7 @@ func resourceCloudStackNetworkACLRuleDeleteRule( "Invalid parameter id value=%s due to incorrect long value format, "+ "or entity does not exist", id.(string))) { delete(uuids, k) + rule["uuids"] = uuids continue } @@ -471,11 +568,9 @@ func resourceCloudStackNetworkACLRuleDeleteRule( // Delete the UUID of this rule delete(uuids, k) + rule["uuids"] = uuids } - // Update the UUIDs - rule["uuids"] = uuids - return nil } diff --git a/builtin/providers/cloudstack/resource_cloudstack_port_forward.go b/builtin/providers/cloudstack/resource_cloudstack_port_forward.go index 0bec41af5..e1f8c99fc 100644 --- a/builtin/providers/cloudstack/resource_cloudstack_port_forward.go +++ b/builtin/providers/cloudstack/resource_cloudstack_port_forward.go @@ -150,6 +150,22 @@ func resourceCloudStackPortForwardCreateForward( func resourceCloudStackPortForwardRead(d *schema.ResourceData, meta interface{}) error { cs := meta.(*cloudstack.CloudStackClient) + // Get all the forwards from the running environment + p := cs.Firewall.NewListPortForwardingRulesParams() + p.SetIpaddressid(d.Id()) + p.SetListall(true) + + l, err := cs.Firewall.ListPortForwardingRules(p) + if err != nil { + return err + } + + // Make a map of all the forwards so we can easily find a forward + forwardMap := make(map[string]*cloudstack.PortForwardingRule, l.Count) + for _, f := range l.PortForwardingRules { + forwardMap[f.Id] = f + } + // Create an empty schema.Set to hold all forwards forwards := &schema.Set{ F: resourceCloudStackPortForwardHash, @@ -166,36 +182,34 @@ func resourceCloudStackPortForwardRead(d *schema.ResourceData, meta interface{}) } // Get the forward - r, count, err := cs.Firewall.GetPortForwardingRuleByID(id.(string)) - // If the count == 0, there is no object found for this ID - if err != nil { - if count == 0 { - forward["uuid"] = "" - continue - } - - return err + f, ok := forwardMap[id.(string)] + if !ok { + forward["uuid"] = "" + continue } - privPort, err := strconv.Atoi(r.Privateport) + // Delete the known rule so only unknown rules remain in the ruleMap + delete(forwardMap, id.(string)) + + privPort, err := strconv.Atoi(f.Privateport) if err != nil { return err } - pubPort, err := strconv.Atoi(r.Publicport) + pubPort, err := strconv.Atoi(f.Publicport) if err != nil { return err } // Update the values - forward["protocol"] = r.Protocol + forward["protocol"] = f.Protocol forward["private_port"] = privPort forward["public_port"] = pubPort if isID(forward["virtual_machine"].(string)) { - forward["virtual_machine"] = r.Virtualmachineid + forward["virtual_machine"] = f.Virtualmachineid } else { - forward["virtual_machine"] = r.Virtualmachinename + forward["virtual_machine"] = f.Virtualmachinename } forwards.Add(forward) @@ -204,33 +218,11 @@ func resourceCloudStackPortForwardRead(d *schema.ResourceData, meta interface{}) // If this is a managed resource, add all unknown forwards to dummy forwards managed := d.Get("managed").(bool) - if managed { - // Get all the forwards from the running environment - p := cs.Firewall.NewListPortForwardingRulesParams() - p.SetIpaddressid(d.Id()) - p.SetListall(true) - - r, err := cs.Firewall.ListPortForwardingRules(p) - if err != nil { - return err - } - - // Add all UUIDs to the uuids map - uuids := make(map[string]interface{}, len(r.PortForwardingRules)) - for _, r := range r.PortForwardingRules { - uuids[r.Id] = r.Id - } - - // Delete all expected UUIDs from the uuids map - for _, forward := range forwards.List() { - forward := forward.(map[string]interface{}) - delete(uuids, forward["uuid"].(string)) - } - - for uuid := range uuids { + if managed && len(forwardMap) > 0 { + for uuid := range forwardMap { // Make a dummy forward to hold the unknown UUID forward := map[string]interface{}{ - "protocol": "N/A", + "protocol": uuid, "private_port": 0, "public_port": 0, "virtual_machine": uuid, diff --git a/builtin/providers/cloudstack/resources.go b/builtin/providers/cloudstack/resources.go index f7115e793..2fe67c6e3 100644 --- a/builtin/providers/cloudstack/resources.go +++ b/builtin/providers/cloudstack/resources.go @@ -10,7 +10,7 @@ import ( "github.com/xanzy/go-cloudstack/cloudstack" ) -// CloudStack uses a "special" ID of -1 to define an unlimited resource +// UnlimitedResourceID is a "special" ID to define an unlimited resource const UnlimitedResourceID = "-1" type retrieveError struct { @@ -135,8 +135,8 @@ func Retry(n int, f RetryFunc) (interface{}, error) { for i := 0; i < n; i++ { r, err := f() - if err == nil { - return r, nil + if err == nil || err == cloudstack.AsyncTimeoutErr { + return r, err } lastErr = err From b8f3417e79c216e6cc5ff5baa50c1c6047f3f2f0 Mon Sep 17 00:00:00 2001 From: Sander van Harmelen Date: Thu, 3 Dec 2015 11:10:42 +0100 Subject: [PATCH 3/3] Change all firewall related resources to take a cidr_list Also some additional tweaks to improve performance and add in a little concurrency to speed things up a little. --- builtin/providers/cloudstack/provider_test.go | 12 +- .../resource_cloudstack_egress_firewall.go | 275 ++++++++++------- ...esource_cloudstack_egress_firewall_test.go | 118 ++++---- .../resource_cloudstack_firewall.go | 276 +++++++++++------- .../resource_cloudstack_firewall_test.go | 70 +++-- .../resource_cloudstack_instance_test.go | 6 +- .../resource_cloudstack_network_acl_rule.go | 161 ++++------ ...source_cloudstack_network_acl_rule_test.go | 98 +++++-- .../resource_cloudstack_port_forward.go | 183 ++++++++---- .../resource_cloudstack_port_forward_test.go | 32 +- ...rce_cloudstack_secondary_ipaddress_test.go | 6 +- builtin/providers/cloudstack/resources.go | 34 +++ .../r/egress_firewall.html.markdown | 7 +- .../cloudstack/r/firewall.html.markdown | 7 +- .../r/network_acl_rule.html.markdown | 7 +- 15 files changed, 783 insertions(+), 509 deletions(-) diff --git a/builtin/providers/cloudstack/provider_test.go b/builtin/providers/cloudstack/provider_test.go index b1b8442a5..2585c5fa0 100644 --- a/builtin/providers/cloudstack/provider_test.go +++ b/builtin/providers/cloudstack/provider_test.go @@ -84,8 +84,11 @@ func testAccPreCheck(t *testing.T) { if v := os.Getenv("CLOUDSTACK_NETWORK_1"); v == "" { t.Fatal("CLOUDSTACK_NETWORK_1 must be set for acceptance tests") } - if v := os.Getenv("CLOUDSTACK_NETWORK_1_IPADDRESS"); v == "" { - t.Fatal("CLOUDSTACK_NETWORK_1_IPADDRESS must be set for acceptance tests") + if v := os.Getenv("CLOUDSTACK_NETWORK_1_IPADDRESS1"); v == "" { + t.Fatal("CLOUDSTACK_NETWORK_1_IPADDRESS1 must be set for acceptance tests") + } + if v := os.Getenv("CLOUDSTACK_NETWORK_1_IPADDRESS2"); v == "" { + t.Fatal("CLOUDSTACK_NETWORK_1_IPADDRESS2 must be set for acceptance tests") } if v := os.Getenv("CLOUDSTACK_NETWORK_2"); v == "" { t.Fatal("CLOUDSTACK_NETWORK_2 must be set for acceptance tests") @@ -159,7 +162,10 @@ var CLOUDSTACK_SERVICE_OFFERING_2 = os.Getenv("CLOUDSTACK_SERVICE_OFFERING_2") var CLOUDSTACK_NETWORK_1 = os.Getenv("CLOUDSTACK_NETWORK_1") // A valid IP address in CLOUDSTACK_NETWORK_1 -var CLOUDSTACK_NETWORK_1_IPADDRESS = os.Getenv("CLOUDSTACK_NETWORK_1_IPADDRESS") +var CLOUDSTACK_NETWORK_1_IPADDRESS1 = os.Getenv("CLOUDSTACK_NETWORK_1_IPADDRESS1") + +// A valid IP address in CLOUDSTACK_NETWORK_1 +var CLOUDSTACK_NETWORK_1_IPADDRESS2 = os.Getenv("CLOUDSTACK_NETWORK_1_IPADDRESS2") // Name for a network that will be created var CLOUDSTACK_NETWORK_2 = os.Getenv("CLOUDSTACK_NETWORK_2") diff --git a/builtin/providers/cloudstack/resource_cloudstack_egress_firewall.go b/builtin/providers/cloudstack/resource_cloudstack_egress_firewall.go index a1d73b167..41909f90e 100644 --- a/builtin/providers/cloudstack/resource_cloudstack_egress_firewall.go +++ b/builtin/providers/cloudstack/resource_cloudstack_egress_firewall.go @@ -1,14 +1,14 @@ package cloudstack import ( - "bytes" "fmt" "regexp" - "sort" "strconv" "strings" + "sync" + "time" - "github.com/hashicorp/terraform/helper/hashcode" + "github.com/hashicorp/go-multierror" "github.com/hashicorp/terraform/helper/schema" "github.com/xanzy/go-cloudstack/cloudstack" ) @@ -38,9 +38,17 @@ func resourceCloudStackEgressFirewall() *schema.Resource { Optional: true, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ + "cidr_list": &schema.Schema{ + Type: schema.TypeSet, + Optional: true, + Elem: &schema.Schema{Type: schema.TypeString}, + Set: schema.HashString, + }, + "source_cidr": &schema.Schema{ - Type: schema.TypeString, - Required: true, + Type: schema.TypeString, + Optional: true, + Deprecated: "Please use the `cidr_list` field instead", }, "protocol": &schema.Schema{ @@ -64,9 +72,7 @@ func resourceCloudStackEgressFirewall() *schema.Resource { Type: schema.TypeSet, Optional: true, Elem: &schema.Schema{Type: schema.TypeString}, - Set: func(v interface{}) int { - return hashcode.String(v.(string)) - }, + Set: schema.HashString, }, "uuids": &schema.Schema{ @@ -75,7 +81,6 @@ func resourceCloudStackEgressFirewall() *schema.Resource { }, }, }, - Set: resourceCloudStackEgressFirewallRuleHash, }, }, } @@ -99,32 +104,66 @@ func resourceCloudStackEgressFirewallCreate(d *schema.ResourceData, meta interfa d.SetId(networkid) // Create all rules that are configured - if rs := d.Get("rule").(*schema.Set); rs.Len() > 0 { - + if nrs := d.Get("rule").(*schema.Set); nrs.Len() > 0 { // Create an empty schema.Set to hold all rules - rules := &schema.Set{ - F: resourceCloudStackEgressFirewallRuleHash, - } + rules := resourceCloudStackEgressFirewall().Schema["rule"].ZeroValue().(*schema.Set) - for _, rule := range rs.List() { - // Create a single rule - err := resourceCloudStackEgressFirewallCreateRule(d, meta, rule.(map[string]interface{})) + err := createEgressFirewallRules(d, meta, rules, nrs) - // We need to update this first to preserve the correct state - rules.Add(rule) - d.Set("rule", rules) + // We need to update this first to preserve the correct state + d.Set("rule", rules) - if err != nil { - return err - } + if err != nil { + return err } } return resourceCloudStackEgressFirewallRead(d, meta) } -func resourceCloudStackEgressFirewallCreateRule( - d *schema.ResourceData, meta interface{}, rule map[string]interface{}) error { +func createEgressFirewallRules( + d *schema.ResourceData, + meta interface{}, + rules *schema.Set, + nrs *schema.Set) error { + var errs *multierror.Error + + var wg sync.WaitGroup + wg.Add(nrs.Len()) + + sem := make(chan struct{}, 10) + for _, rule := range nrs.List() { + // Put in a tiny sleep here to avoid DoS'ing the API + time.Sleep(500 * time.Millisecond) + + go func(rule map[string]interface{}) { + defer wg.Done() + sem <- struct{}{} + + // Create a single rule + err := createEgressFirewallRule(d, meta, rule) + + // If we have at least one UUID, we need to save the rule + if len(rule["uuids"].(map[string]interface{})) > 0 { + rules.Add(rule) + } + + if err != nil { + errs = multierror.Append(errs, err) + } + + <-sem + }(rule.(map[string]interface{})) + } + + wg.Wait() + + return errs.ErrorOrNil() +} +func createEgressFirewallRule( + d *schema.ResourceData, + meta interface{}, + rule map[string]interface{}) error { cs := meta.(*cloudstack.CloudStackClient) uuids := rule["uuids"].(map[string]interface{}) @@ -137,7 +176,7 @@ func resourceCloudStackEgressFirewallCreateRule( p := cs.Firewall.NewCreateEgressFirewallRuleParams(d.Id(), rule["protocol"].(string)) // Set the CIDR list - p.SetCidrlist([]string{rule["source_cidr"].(string)}) + p.SetCidrlist(retrieveCidrList(rule)) // If the protocol is ICMP set the needed ICMP parameters if rule["protocol"].(string) == "icmp" { @@ -157,14 +196,18 @@ func resourceCloudStackEgressFirewallCreateRule( if ps := rule["ports"].(*schema.Set); ps.Len() > 0 { // Create an empty schema.Set to hold all processed ports - ports := &schema.Set{ - F: func(v interface{}) int { - return hashcode.String(v.(string)) - }, - } + ports := &schema.Set{F: schema.HashString} + + // Define a regexp for parsing the port + re := regexp.MustCompile(`^(\d+)(?:-(\d+))?$`) for _, port := range ps.List() { - re := regexp.MustCompile(`^(\d+)(?:-(\d+))?$`) + if _, ok := uuids[port.(string)]; ok { + ports.Add(port) + rule["ports"] = ports + continue + } + m := re.FindStringSubmatch(port.(string)) startPort, err := strconv.Atoi(m[1]) @@ -220,9 +263,7 @@ func resourceCloudStackEgressFirewallRead(d *schema.ResourceData, meta interface } // Create an empty schema.Set to hold all rules - rules := &schema.Set{ - F: resourceCloudStackEgressFirewallRuleHash, - } + rules := resourceCloudStackEgressFirewall().Schema["rule"].ZeroValue().(*schema.Set) // Read all rules that are configured if rs := d.Get("rule").(*schema.Set); rs.Len() > 0 { @@ -247,10 +288,10 @@ func resourceCloudStackEgressFirewallRead(d *schema.ResourceData, meta interface delete(ruleMap, id.(string)) // Update the values - rule["source_cidr"] = r.Cidrlist rule["protocol"] = r.Protocol rule["icmp_type"] = r.Icmptype rule["icmp_code"] = r.Icmpcode + setCidrList(rule, r.Cidrlist) rules.Add(rule) } @@ -259,11 +300,7 @@ func resourceCloudStackEgressFirewallRead(d *schema.ResourceData, meta interface if ps := rule["ports"].(*schema.Set); ps.Len() > 0 { // Create an empty schema.Set to hold all ports - ports := &schema.Set{ - F: func(v interface{}) int { - return hashcode.String(v.(string)) - }, - } + ports := &schema.Set{F: schema.HashString} // Loop through all ports and retrieve their info for _, port := range ps.List() { @@ -283,8 +320,8 @@ func resourceCloudStackEgressFirewallRead(d *schema.ResourceData, meta interface delete(ruleMap, id.(string)) // Update the values - rule["source_cidr"] = r.Cidrlist rule["protocol"] = r.Protocol + setCidrList(rule, r.Cidrlist) ports.Add(port) } @@ -302,11 +339,16 @@ func resourceCloudStackEgressFirewallRead(d *schema.ResourceData, meta interface managed := d.Get("managed").(bool) if managed && len(ruleMap) > 0 { for uuid := range ruleMap { + // We need to create and add a dummy value to a schema.Set as the + // cidr_list is a required field and thus needs a value + cidrs := &schema.Set{F: schema.HashString} + cidrs.Add(uuid) + // Make a dummy rule to hold the unknown UUID rule := map[string]interface{}{ - "source_cidr": uuid, - "protocol": uuid, - "uuids": map[string]interface{}{uuid: uuid}, + "cidr_list": uuid, + "protocol": uuid, + "uuids": map[string]interface{}{uuid: uuid}, } // Add the dummy rule to the rules set @@ -335,27 +377,29 @@ func resourceCloudStackEgressFirewallUpdate(d *schema.ResourceData, meta interfa ors := o.(*schema.Set).Difference(n.(*schema.Set)) nrs := n.(*schema.Set).Difference(o.(*schema.Set)) - // Now first loop through all the old rules and delete any obsolete ones - for _, rule := range ors.List() { - // Delete the rule as it no longer exists in the config - err := resourceCloudStackEgressFirewallDeleteRule(d, meta, rule.(map[string]interface{})) + // We need to start with a rule set containing all the rules we + // already have and want to keep. Any rules that are not deleted + // correctly and any newly created rules, will be added to this + // set to make sure we end up in a consistent state + rules := o.(*schema.Set).Intersection(n.(*schema.Set)) + + // First loop through all the old rules and delete them + if ors.Len() > 0 { + err := deleteEgressFirewallRules(d, meta, rules, ors) + + // We need to update this first to preserve the correct state + d.Set("rule", rules) + if err != nil { return err } } - // Make sure we save the state of the currently configured rules - rules := o.(*schema.Set).Intersection(n.(*schema.Set)) - d.Set("rule", rules) - - // Then loop through all the currently configured rules and create the new ones - for _, rule := range nrs.List() { - // When successfully deleted, re-create it again if it still exists - err := resourceCloudStackEgressFirewallCreateRule( - d, meta, rule.(map[string]interface{})) + // Then loop through all the new rules and create them + if nrs.Len() > 0 { + err := createEgressFirewallRules(d, meta, rules, nrs) // We need to update this first to preserve the correct state - rules.Add(rule) d.Set("rule", rules) if err != nil { @@ -368,26 +412,69 @@ func resourceCloudStackEgressFirewallUpdate(d *schema.ResourceData, meta interfa } func resourceCloudStackEgressFirewallDelete(d *schema.ResourceData, meta interface{}) error { + // Create an empty rule set to hold all rules that where + // not deleted correctly + rules := resourceCloudStackEgressFirewall().Schema["rule"].ZeroValue().(*schema.Set) + // Delete all rules - if rs := d.Get("rule").(*schema.Set); rs.Len() > 0 { - for _, rule := range rs.List() { - // Delete a single rule - err := resourceCloudStackEgressFirewallDeleteRule(d, meta, rule.(map[string]interface{})) + if ors := d.Get("rule").(*schema.Set); ors.Len() > 0 { + err := deleteEgressFirewallRules(d, meta, rules, ors) - // We need to update this first to preserve the correct state - d.Set("rule", rs) + // We need to update this first to preserve the correct state + d.Set("rule", rules) - if err != nil { - return err - } + if err != nil { + return err } } return nil } -func resourceCloudStackEgressFirewallDeleteRule( - d *schema.ResourceData, meta interface{}, rule map[string]interface{}) error { +func deleteEgressFirewallRules( + d *schema.ResourceData, + meta interface{}, + rules *schema.Set, + ors *schema.Set) error { + var errs *multierror.Error + + var wg sync.WaitGroup + wg.Add(ors.Len()) + + sem := make(chan struct{}, 10) + for _, rule := range ors.List() { + // Put a sleep here to avoid DoS'ing the API + time.Sleep(500 * time.Millisecond) + + go func(rule map[string]interface{}) { + defer wg.Done() + sem <- struct{}{} + + // Delete a single rule + err := deleteEgressFirewallRule(d, meta, rule) + + // If we have at least one UUID, we need to save the rule + if len(rule["uuids"].(map[string]interface{})) > 0 { + rules.Add(rule) + } + + if err != nil { + errs = multierror.Append(errs, err) + } + + <-sem + }(rule.(map[string]interface{})) + } + + wg.Wait() + + return errs.ErrorOrNil() +} + +func deleteEgressFirewallRule( + d *schema.ResourceData, + meta interface{}, + rule map[string]interface{}) error { cs := meta.(*cloudstack.CloudStackClient) uuids := rule["uuids"].(map[string]interface{}) @@ -416,47 +503,12 @@ func resourceCloudStackEgressFirewallDeleteRule( // Delete the UUID of this rule delete(uuids, k) + rule["uuids"] = uuids } - // Update the UUIDs - rule["uuids"] = uuids - return nil } -func resourceCloudStackEgressFirewallRuleHash(v interface{}) int { - var buf bytes.Buffer - m := v.(map[string]interface{}) - buf.WriteString(fmt.Sprintf( - "%s-%s-", m["source_cidr"].(string), m["protocol"].(string))) - - if v, ok := m["icmp_type"]; ok { - buf.WriteString(fmt.Sprintf("%d-", v.(int))) - } - - if v, ok := m["icmp_code"]; ok { - buf.WriteString(fmt.Sprintf("%d-", v.(int))) - } - - // We need to make sure to sort the strings below so that we always - // generate the same hash code no matter what is in the set. - if v, ok := m["ports"]; ok { - vs := v.(*schema.Set).List() - s := make([]string, len(vs)) - - for i, raw := range vs { - s[i] = raw.(string) - } - sort.Strings(s) - - for _, v := range s { - buf.WriteString(fmt.Sprintf("%s-", v)) - } - } - - return hashcode.String(buf.String()) -} - func verifyEgressFirewallParams(d *schema.ResourceData) error { managed := d.Get("managed").(bool) _, rules := d.GetOk("rule") @@ -470,6 +522,17 @@ func verifyEgressFirewallParams(d *schema.ResourceData) error { } func verifyEgressFirewallRuleParams(d *schema.ResourceData, rule map[string]interface{}) error { + cidrList := rule["cidr_list"].(*schema.Set) + sourceCidr := rule["source_cidr"].(string) + if cidrList.Len() == 0 && sourceCidr == "" { + return fmt.Errorf( + "Parameter cidr_list is a required parameter") + } + if cidrList.Len() > 0 && sourceCidr != "" { + return fmt.Errorf( + "Parameter source_cidr is deprecated and cannot be used together with cidr_list") + } + protocol := rule["protocol"].(string) if protocol != "tcp" && protocol != "udp" && protocol != "icmp" { return fmt.Errorf( diff --git a/builtin/providers/cloudstack/resource_cloudstack_egress_firewall_test.go b/builtin/providers/cloudstack/resource_cloudstack_egress_firewall_test.go index dbca8c32b..07f4e0d8a 100644 --- a/builtin/providers/cloudstack/resource_cloudstack_egress_firewall_test.go +++ b/builtin/providers/cloudstack/resource_cloudstack_egress_firewall_test.go @@ -2,19 +2,15 @@ package cloudstack import ( "fmt" - "strconv" "strings" "testing" "github.com/hashicorp/terraform/helper/resource" - "github.com/hashicorp/terraform/helper/schema" "github.com/hashicorp/terraform/terraform" "github.com/xanzy/go-cloudstack/cloudstack" ) func TestAccCloudStackEgressFirewall_basic(t *testing.T) { - hash := makeTestCloudStackEgressFirewallRuleHash([]interface{}{"1000-2000", "80"}) - resource.Test(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, Providers: testAccProviders, @@ -26,18 +22,26 @@ func TestAccCloudStackEgressFirewall_basic(t *testing.T) { testAccCheckCloudStackEgressFirewallRulesExist("cloudstack_egress_firewall.foo"), resource.TestCheckResourceAttr( "cloudstack_egress_firewall.foo", "network", CLOUDSTACK_NETWORK_1), + resource.TestCheckResourceAttr( + "cloudstack_egress_firewall.foo", "rule.#", "2"), resource.TestCheckResourceAttr( "cloudstack_egress_firewall.foo", - "rule."+hash+".source_cidr", - CLOUDSTACK_NETWORK_1_IPADDRESS+"/32"), + "rule.1081385056.cidr_list.3378711023", + CLOUDSTACK_NETWORK_1_IPADDRESS1+"/32"), resource.TestCheckResourceAttr( - "cloudstack_egress_firewall.foo", "rule."+hash+".protocol", "tcp"), + "cloudstack_egress_firewall.foo", "rule.1081385056.protocol", "tcp"), resource.TestCheckResourceAttr( - "cloudstack_egress_firewall.foo", "rule."+hash+".ports.#", "2"), + "cloudstack_egress_firewall.foo", "rule.1081385056.ports.32925333", "8080"), resource.TestCheckResourceAttr( - "cloudstack_egress_firewall.foo", "rule."+hash+".ports.1209010669", "1000-2000"), + "cloudstack_egress_firewall.foo", + "rule.1129999216.source_cidr", + CLOUDSTACK_NETWORK_1_IPADDRESS1+"/32"), resource.TestCheckResourceAttr( - "cloudstack_egress_firewall.foo", "rule."+hash+".ports.1889509032", "80"), + "cloudstack_egress_firewall.foo", "rule.1129999216.protocol", "tcp"), + resource.TestCheckResourceAttr( + "cloudstack_egress_firewall.foo", "rule.1129999216.ports.1209010669", "1000-2000"), + resource.TestCheckResourceAttr( + "cloudstack_egress_firewall.foo", "rule.1129999216.ports.1889509032", "80"), ), }, }, @@ -45,9 +49,6 @@ func TestAccCloudStackEgressFirewall_basic(t *testing.T) { } func TestAccCloudStackEgressFirewall_update(t *testing.T) { - hash1 := makeTestCloudStackEgressFirewallRuleHash([]interface{}{"1000-2000", "80"}) - hash2 := makeTestCloudStackEgressFirewallRuleHash([]interface{}{"443"}) - resource.Test(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, Providers: testAccProviders, @@ -60,19 +61,25 @@ func TestAccCloudStackEgressFirewall_update(t *testing.T) { resource.TestCheckResourceAttr( "cloudstack_egress_firewall.foo", "network", CLOUDSTACK_NETWORK_1), resource.TestCheckResourceAttr( - "cloudstack_egress_firewall.foo", "rule.#", "1"), + "cloudstack_egress_firewall.foo", "rule.#", "2"), resource.TestCheckResourceAttr( "cloudstack_egress_firewall.foo", - "rule."+hash1+".source_cidr", - CLOUDSTACK_NETWORK_1_IPADDRESS+"/32"), + "rule.1081385056.cidr_list.3378711023", + CLOUDSTACK_NETWORK_1_IPADDRESS1+"/32"), resource.TestCheckResourceAttr( - "cloudstack_egress_firewall.foo", "rule."+hash1+".protocol", "tcp"), + "cloudstack_egress_firewall.foo", "rule.1081385056.protocol", "tcp"), resource.TestCheckResourceAttr( - "cloudstack_egress_firewall.foo", "rule."+hash1+".ports.#", "2"), + "cloudstack_egress_firewall.foo", "rule.1081385056.ports.32925333", "8080"), resource.TestCheckResourceAttr( - "cloudstack_egress_firewall.foo", "rule."+hash1+".ports.1209010669", "1000-2000"), + "cloudstack_egress_firewall.foo", + "rule.1129999216.source_cidr", + CLOUDSTACK_NETWORK_1_IPADDRESS1+"/32"), resource.TestCheckResourceAttr( - "cloudstack_egress_firewall.foo", "rule."+hash1+".ports.1889509032", "80"), + "cloudstack_egress_firewall.foo", "rule.1129999216.protocol", "tcp"), + resource.TestCheckResourceAttr( + "cloudstack_egress_firewall.foo", "rule.1129999216.ports.1209010669", "1000-2000"), + resource.TestCheckResourceAttr( + "cloudstack_egress_firewall.foo", "rule.1129999216.ports.1889509032", "80"), ), }, @@ -83,29 +90,37 @@ func TestAccCloudStackEgressFirewall_update(t *testing.T) { resource.TestCheckResourceAttr( "cloudstack_egress_firewall.foo", "network", CLOUDSTACK_NETWORK_1), resource.TestCheckResourceAttr( - "cloudstack_egress_firewall.foo", "rule.#", "2"), + "cloudstack_egress_firewall.foo", "rule.#", "3"), resource.TestCheckResourceAttr( "cloudstack_egress_firewall.foo", - "rule."+hash1+".source_cidr", - CLOUDSTACK_NETWORK_1_IPADDRESS+"/32"), - resource.TestCheckResourceAttr( - "cloudstack_egress_firewall.foo", "rule."+hash1+".protocol", "tcp"), - resource.TestCheckResourceAttr( - "cloudstack_egress_firewall.foo", "rule."+hash1+".ports.#", "2"), - resource.TestCheckResourceAttr( - "cloudstack_egress_firewall.foo", "rule."+hash1+".ports.1209010669", "1000-2000"), - resource.TestCheckResourceAttr( - "cloudstack_egress_firewall.foo", "rule."+hash1+".ports.1889509032", "80"), + "rule.59731059.cidr_list.1910468234", + CLOUDSTACK_NETWORK_1_IPADDRESS2+"/32"), resource.TestCheckResourceAttr( "cloudstack_egress_firewall.foo", - "rule."+hash2+".source_cidr", - CLOUDSTACK_NETWORK_1_IPADDRESS+"/32"), + "rule.59731059.cidr_list.3378711023", + CLOUDSTACK_NETWORK_1_IPADDRESS1+"/32"), resource.TestCheckResourceAttr( - "cloudstack_egress_firewall.foo", "rule."+hash2+".protocol", "tcp"), + "cloudstack_egress_firewall.foo", "rule.59731059.protocol", "tcp"), resource.TestCheckResourceAttr( - "cloudstack_egress_firewall.foo", "rule."+hash2+".ports.#", "1"), + "cloudstack_egress_firewall.foo", "rule.59731059.ports.32925333", "8080"), resource.TestCheckResourceAttr( - "cloudstack_egress_firewall.foo", "rule."+hash2+".ports.3638101695", "443"), + "cloudstack_egress_firewall.foo", + "rule.1052669680.source_cidr", + CLOUDSTACK_NETWORK_1_IPADDRESS1+"/32"), + resource.TestCheckResourceAttr( + "cloudstack_egress_firewall.foo", "rule.1052669680.protocol", "tcp"), + resource.TestCheckResourceAttr( + "cloudstack_egress_firewall.foo", "rule.1052669680.ports.3638101695", "443"), + resource.TestCheckResourceAttr( + "cloudstack_egress_firewall.foo", + "rule.1129999216.source_cidr", + CLOUDSTACK_NETWORK_1_IPADDRESS1+"/32"), + resource.TestCheckResourceAttr( + "cloudstack_egress_firewall.foo", "rule.1129999216.protocol", "tcp"), + resource.TestCheckResourceAttr( + "cloudstack_egress_firewall.foo", "rule.1129999216.ports.1209010669", "1000-2000"), + resource.TestCheckResourceAttr( + "cloudstack_egress_firewall.foo", "rule.1129999216.ports.1889509032", "80"), ), }, }, @@ -171,20 +186,16 @@ func testAccCheckCloudStackEgressFirewallDestroy(s *terraform.State) error { return nil } -func makeTestCloudStackEgressFirewallRuleHash(ports []interface{}) string { - return strconv.Itoa(resourceCloudStackEgressFirewallRuleHash(map[string]interface{}{ - "source_cidr": CLOUDSTACK_NETWORK_1_IPADDRESS + "/32", - "protocol": "tcp", - "ports": schema.NewSet(schema.HashString, ports), - "icmp_type": 0, - "icmp_code": 0, - })) -} - var testAccCloudStackEgressFirewall_basic = fmt.Sprintf(` resource "cloudstack_egress_firewall" "foo" { network = "%s" + rule { + cidr_list = ["%s/32"] + protocol = "tcp" + ports = ["8080"] + } + rule { source_cidr = "%s/32" protocol = "tcp" @@ -192,12 +203,19 @@ resource "cloudstack_egress_firewall" "foo" { } }`, CLOUDSTACK_NETWORK_1, - CLOUDSTACK_NETWORK_1_IPADDRESS) + CLOUDSTACK_NETWORK_1_IPADDRESS1, + CLOUDSTACK_NETWORK_1_IPADDRESS1) var testAccCloudStackEgressFirewall_update = fmt.Sprintf(` resource "cloudstack_egress_firewall" "foo" { network = "%s" + rule { + cidr_list = ["%s/32", "%s/32"] + protocol = "tcp" + ports = ["8080"] + } + rule { source_cidr = "%s/32" protocol = "tcp" @@ -211,5 +229,7 @@ resource "cloudstack_egress_firewall" "foo" { } }`, CLOUDSTACK_NETWORK_1, - CLOUDSTACK_NETWORK_1_IPADDRESS, - CLOUDSTACK_NETWORK_1_IPADDRESS) + CLOUDSTACK_NETWORK_1_IPADDRESS1, + CLOUDSTACK_NETWORK_1_IPADDRESS2, + CLOUDSTACK_NETWORK_1_IPADDRESS1, + CLOUDSTACK_NETWORK_1_IPADDRESS1) diff --git a/builtin/providers/cloudstack/resource_cloudstack_firewall.go b/builtin/providers/cloudstack/resource_cloudstack_firewall.go index c5a8f8763..b8f92b553 100644 --- a/builtin/providers/cloudstack/resource_cloudstack_firewall.go +++ b/builtin/providers/cloudstack/resource_cloudstack_firewall.go @@ -1,14 +1,14 @@ package cloudstack import ( - "bytes" "fmt" "regexp" - "sort" "strconv" "strings" + "sync" + "time" - "github.com/hashicorp/terraform/helper/hashcode" + "github.com/hashicorp/go-multierror" "github.com/hashicorp/terraform/helper/schema" "github.com/xanzy/go-cloudstack/cloudstack" ) @@ -38,9 +38,17 @@ func resourceCloudStackFirewall() *schema.Resource { Optional: true, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ + "cidr_list": &schema.Schema{ + Type: schema.TypeSet, + Optional: true, + Elem: &schema.Schema{Type: schema.TypeString}, + Set: schema.HashString, + }, + "source_cidr": &schema.Schema{ - Type: schema.TypeString, - Required: true, + Type: schema.TypeString, + Optional: true, + Deprecated: "Please use the `cidr_list` field instead", }, "protocol": &schema.Schema{ @@ -64,9 +72,7 @@ func resourceCloudStackFirewall() *schema.Resource { Type: schema.TypeSet, Optional: true, Elem: &schema.Schema{Type: schema.TypeString}, - Set: func(v interface{}) int { - return hashcode.String(v.(string)) - }, + Set: schema.HashString, }, "uuids": &schema.Schema{ @@ -75,7 +81,6 @@ func resourceCloudStackFirewall() *schema.Resource { }, }, }, - Set: resourceCloudStackFirewallRuleHash, }, }, } @@ -99,32 +104,66 @@ func resourceCloudStackFirewallCreate(d *schema.ResourceData, meta interface{}) d.SetId(ipaddressid) // Create all rules that are configured - if rs := d.Get("rule").(*schema.Set); rs.Len() > 0 { - + if nrs := d.Get("rule").(*schema.Set); nrs.Len() > 0 { // Create an empty schema.Set to hold all rules - rules := &schema.Set{ - F: resourceCloudStackFirewallRuleHash, - } + rules := resourceCloudStackFirewall().Schema["rule"].ZeroValue().(*schema.Set) - for _, rule := range rs.List() { - // Create a single rule - err := resourceCloudStackFirewallCreateRule(d, meta, rule.(map[string]interface{})) + err := createFirewallRules(d, meta, rules, nrs) - // We need to update this first to preserve the correct state - rules.Add(rule) - d.Set("rule", rules) + // We need to update this first to preserve the correct state + d.Set("rule", rules) - if err != nil { - return err - } + if err != nil { + return err } } return resourceCloudStackFirewallRead(d, meta) } +func createFirewallRules( + d *schema.ResourceData, + meta interface{}, + rules *schema.Set, + nrs *schema.Set) error { + var errs *multierror.Error -func resourceCloudStackFirewallCreateRule( - d *schema.ResourceData, meta interface{}, rule map[string]interface{}) error { + var wg sync.WaitGroup + wg.Add(nrs.Len()) + + sem := make(chan struct{}, 10) + for _, rule := range nrs.List() { + // Put in a tiny sleep here to avoid DoS'ing the API + time.Sleep(500 * time.Millisecond) + + go func(rule map[string]interface{}) { + defer wg.Done() + sem <- struct{}{} + + // Create a single rule + err := createFirewallRule(d, meta, rule) + + // If we have at least one UUID, we need to save the rule + if len(rule["uuids"].(map[string]interface{})) > 0 { + rules.Add(rule) + } + + if err != nil { + errs = multierror.Append(errs, err) + } + + <-sem + }(rule.(map[string]interface{})) + } + + wg.Wait() + + return errs.ErrorOrNil() +} + +func createFirewallRule( + d *schema.ResourceData, + meta interface{}, + rule map[string]interface{}) error { cs := meta.(*cloudstack.CloudStackClient) uuids := rule["uuids"].(map[string]interface{}) @@ -137,7 +176,7 @@ func resourceCloudStackFirewallCreateRule( p := cs.Firewall.NewCreateFirewallRuleParams(d.Id(), rule["protocol"].(string)) // Set the CIDR list - p.SetCidrlist([]string{rule["source_cidr"].(string)}) + p.SetCidrlist(retrieveCidrList(rule)) // If the protocol is ICMP set the needed ICMP parameters if rule["protocol"].(string) == "icmp" { @@ -148,6 +187,7 @@ func resourceCloudStackFirewallCreateRule( if err != nil { return err } + uuids["icmp"] = r.Id rule["uuids"] = uuids } @@ -157,14 +197,18 @@ func resourceCloudStackFirewallCreateRule( if ps := rule["ports"].(*schema.Set); ps.Len() > 0 { // Create an empty schema.Set to hold all processed ports - ports := &schema.Set{ - F: func(v interface{}) int { - return hashcode.String(v.(string)) - }, - } + ports := &schema.Set{F: schema.HashString} + + // Define a regexp for parsing the port + re := regexp.MustCompile(`^(\d+)(?:-(\d+))?$`) for _, port := range ps.List() { - re := regexp.MustCompile(`^(\d+)(?:-(\d+))?$`) + if _, ok := uuids[port.(string)]; ok { + ports.Add(port) + rule["ports"] = ports + continue + } + m := re.FindStringSubmatch(port.(string)) startPort, err := strconv.Atoi(m[1]) @@ -220,9 +264,7 @@ func resourceCloudStackFirewallRead(d *schema.ResourceData, meta interface{}) er } // Create an empty schema.Set to hold all rules - rules := &schema.Set{ - F: resourceCloudStackFirewallRuleHash, - } + rules := resourceCloudStackFirewall().Schema["rule"].ZeroValue().(*schema.Set) // Read all rules that are configured if rs := d.Get("rule").(*schema.Set); rs.Len() > 0 { @@ -247,10 +289,10 @@ func resourceCloudStackFirewallRead(d *schema.ResourceData, meta interface{}) er delete(ruleMap, id.(string)) // Update the values - rule["source_cidr"] = r.Cidrlist rule["protocol"] = r.Protocol rule["icmp_type"] = r.Icmptype rule["icmp_code"] = r.Icmpcode + setCidrList(rule, r.Cidrlist) rules.Add(rule) } @@ -259,11 +301,7 @@ func resourceCloudStackFirewallRead(d *schema.ResourceData, meta interface{}) er if ps := rule["ports"].(*schema.Set); ps.Len() > 0 { // Create an empty schema.Set to hold all ports - ports := &schema.Set{ - F: func(v interface{}) int { - return hashcode.String(v.(string)) - }, - } + ports := &schema.Set{F: schema.HashString} // Loop through all ports and retrieve their info for _, port := range ps.List() { @@ -283,8 +321,8 @@ func resourceCloudStackFirewallRead(d *schema.ResourceData, meta interface{}) er delete(ruleMap, id.(string)) // Update the values - rule["source_cidr"] = r.Cidrlist rule["protocol"] = r.Protocol + setCidrList(rule, r.Cidrlist) ports.Add(port) } @@ -302,11 +340,16 @@ func resourceCloudStackFirewallRead(d *schema.ResourceData, meta interface{}) er managed := d.Get("managed").(bool) if managed && len(ruleMap) > 0 { for uuid := range ruleMap { + // We need to create and add a dummy value to a schema.Set as the + // cidr_list is a required field and thus needs a value + cidrs := &schema.Set{F: schema.HashString} + cidrs.Add(uuid) + // Make a dummy rule to hold the unknown UUID rule := map[string]interface{}{ - "source_cidr": uuid, - "protocol": uuid, - "uuids": map[string]interface{}{uuid: uuid}, + "cidr_list": cidrs, + "protocol": uuid, + "uuids": map[string]interface{}{uuid: uuid}, } // Add the dummy rule to the rules set @@ -335,27 +378,29 @@ func resourceCloudStackFirewallUpdate(d *schema.ResourceData, meta interface{}) ors := o.(*schema.Set).Difference(n.(*schema.Set)) nrs := n.(*schema.Set).Difference(o.(*schema.Set)) - // Now first loop through all the old rules and delete any obsolete ones - for _, rule := range ors.List() { - // Delete the rule as it no longer exists in the config - err := resourceCloudStackFirewallDeleteRule(d, meta, rule.(map[string]interface{})) + // We need to start with a rule set containing all the rules we + // already have and want to keep. Any rules that are not deleted + // correctly and any newly created rules, will be added to this + // set to make sure we end up in a consistent state + rules := o.(*schema.Set).Intersection(n.(*schema.Set)) + + // First loop through all the old rules and delete them + if ors.Len() > 0 { + err := deleteFirewallRules(d, meta, rules, ors) + + // We need to update this first to preserve the correct state + d.Set("rule", rules) + if err != nil { return err } } - // Make sure we save the state of the currently configured rules - rules := o.(*schema.Set).Intersection(n.(*schema.Set)) - d.Set("rule", rules) - - // Then loop through all the currently configured rules and create the new ones - for _, rule := range nrs.List() { - // When successfully deleted, re-create it again if it still exists - err := resourceCloudStackFirewallCreateRule( - d, meta, rule.(map[string]interface{})) + // Then loop through all the new rules and create them + if nrs.Len() > 0 { + err := createFirewallRules(d, meta, rules, nrs) // We need to update this first to preserve the correct state - rules.Add(rule) d.Set("rule", rules) if err != nil { @@ -368,26 +413,69 @@ func resourceCloudStackFirewallUpdate(d *schema.ResourceData, meta interface{}) } func resourceCloudStackFirewallDelete(d *schema.ResourceData, meta interface{}) error { + // Create an empty rule set to hold all rules that where + // not deleted correctly + rules := resourceCloudStackFirewall().Schema["rule"].ZeroValue().(*schema.Set) + // Delete all rules - if rs := d.Get("rule").(*schema.Set); rs.Len() > 0 { - for _, rule := range rs.List() { - // Delete a single rule - err := resourceCloudStackFirewallDeleteRule(d, meta, rule.(map[string]interface{})) + if ors := d.Get("rule").(*schema.Set); ors.Len() > 0 { + err := deleteFirewallRules(d, meta, rules, ors) - // We need to update this first to preserve the correct state - d.Set("rule", rs) + // We need to update this first to preserve the correct state + d.Set("rule", rules) - if err != nil { - return err - } + if err != nil { + return err } } return nil } -func resourceCloudStackFirewallDeleteRule( - d *schema.ResourceData, meta interface{}, rule map[string]interface{}) error { +func deleteFirewallRules( + d *schema.ResourceData, + meta interface{}, + rules *schema.Set, + ors *schema.Set) error { + var errs *multierror.Error + + var wg sync.WaitGroup + wg.Add(ors.Len()) + + sem := make(chan struct{}, 10) + for _, rule := range ors.List() { + // Put a sleep here to avoid DoS'ing the API + time.Sleep(500 * time.Millisecond) + + go func(rule map[string]interface{}) { + defer wg.Done() + sem <- struct{}{} + + // Delete a single rule + err := deleteFirewallRule(d, meta, rule) + + // If we have at least one UUID, we need to save the rule + if len(rule["uuids"].(map[string]interface{})) > 0 { + rules.Add(rule) + } + + if err != nil { + errs = multierror.Append(errs, err) + } + + <-sem + }(rule.(map[string]interface{})) + } + + wg.Wait() + + return errs.ErrorOrNil() +} + +func deleteFirewallRule( + d *schema.ResourceData, + meta interface{}, + rule map[string]interface{}) error { cs := meta.(*cloudstack.CloudStackClient) uuids := rule["uuids"].(map[string]interface{}) @@ -416,47 +504,12 @@ func resourceCloudStackFirewallDeleteRule( // Delete the UUID of this rule delete(uuids, k) + rule["uuids"] = uuids } - // Update the UUIDs - rule["uuids"] = uuids - return nil } -func resourceCloudStackFirewallRuleHash(v interface{}) int { - var buf bytes.Buffer - m := v.(map[string]interface{}) - buf.WriteString(fmt.Sprintf( - "%s-%s-", m["source_cidr"].(string), m["protocol"].(string))) - - if v, ok := m["icmp_type"]; ok { - buf.WriteString(fmt.Sprintf("%d-", v.(int))) - } - - if v, ok := m["icmp_code"]; ok { - buf.WriteString(fmt.Sprintf("%d-", v.(int))) - } - - // We need to make sure to sort the strings below so that we always - // generate the same hash code no matter what is in the set. - if v, ok := m["ports"]; ok { - vs := v.(*schema.Set).List() - s := make([]string, len(vs)) - - for i, raw := range vs { - s[i] = raw.(string) - } - sort.Strings(s) - - for _, v := range s { - buf.WriteString(fmt.Sprintf("%s-", v)) - } - } - - return hashcode.String(buf.String()) -} - func verifyFirewallParams(d *schema.ResourceData) error { managed := d.Get("managed").(bool) _, rules := d.GetOk("rule") @@ -470,6 +523,17 @@ func verifyFirewallParams(d *schema.ResourceData) error { } func verifyFirewallRuleParams(d *schema.ResourceData, rule map[string]interface{}) error { + cidrList := rule["cidr_list"].(*schema.Set) + sourceCidr := rule["source_cidr"].(string) + if cidrList.Len() == 0 && sourceCidr == "" { + return fmt.Errorf( + "Parameter cidr_list is a required parameter") + } + if cidrList.Len() > 0 && sourceCidr != "" { + return fmt.Errorf( + "Parameter source_cidr is deprecated and cannot be used together with cidr_list") + } + protocol := rule["protocol"].(string) if protocol != "tcp" && protocol != "udp" && protocol != "icmp" { return fmt.Errorf( diff --git a/builtin/providers/cloudstack/resource_cloudstack_firewall_test.go b/builtin/providers/cloudstack/resource_cloudstack_firewall_test.go index a86cdc3b2..d93a2c73e 100644 --- a/builtin/providers/cloudstack/resource_cloudstack_firewall_test.go +++ b/builtin/providers/cloudstack/resource_cloudstack_firewall_test.go @@ -23,15 +23,21 @@ func TestAccCloudStackFirewall_basic(t *testing.T) { resource.TestCheckResourceAttr( "cloudstack_firewall.foo", "ipaddress", CLOUDSTACK_PUBLIC_IPADDRESS), resource.TestCheckResourceAttr( - "cloudstack_firewall.foo", "rule.1702320581.source_cidr", "10.0.0.0/24"), + "cloudstack_firewall.foo", "rule.#", "2"), resource.TestCheckResourceAttr( - "cloudstack_firewall.foo", "rule.1702320581.protocol", "tcp"), + "cloudstack_firewall.foo", "rule.60926170.cidr_list.3482919157", "10.0.0.0/24"), resource.TestCheckResourceAttr( - "cloudstack_firewall.foo", "rule.1702320581.ports.#", "2"), + "cloudstack_firewall.foo", "rule.60926170.protocol", "tcp"), resource.TestCheckResourceAttr( - "cloudstack_firewall.foo", "rule.1702320581.ports.1209010669", "1000-2000"), + "cloudstack_firewall.foo", "rule.60926170.ports.32925333", "8080"), resource.TestCheckResourceAttr( - "cloudstack_firewall.foo", "rule.1702320581.ports.1889509032", "80"), + "cloudstack_firewall.foo", "rule.716592205.source_cidr", "10.0.0.0/24"), + resource.TestCheckResourceAttr( + "cloudstack_firewall.foo", "rule.716592205.protocol", "tcp"), + resource.TestCheckResourceAttr( + "cloudstack_firewall.foo", "rule.716592205.ports.1209010669", "1000-2000"), + resource.TestCheckResourceAttr( + "cloudstack_firewall.foo", "rule.716592205.ports.1889509032", "80"), ), }, }, @@ -51,17 +57,21 @@ func TestAccCloudStackFirewall_update(t *testing.T) { resource.TestCheckResourceAttr( "cloudstack_firewall.foo", "ipaddress", CLOUDSTACK_PUBLIC_IPADDRESS), resource.TestCheckResourceAttr( - "cloudstack_firewall.foo", "rule.#", "1"), + "cloudstack_firewall.foo", "rule.#", "2"), resource.TestCheckResourceAttr( - "cloudstack_firewall.foo", "rule.1702320581.source_cidr", "10.0.0.0/24"), + "cloudstack_firewall.foo", "rule.60926170.cidr_list.3482919157", "10.0.0.0/24"), resource.TestCheckResourceAttr( - "cloudstack_firewall.foo", "rule.1702320581.protocol", "tcp"), + "cloudstack_firewall.foo", "rule.60926170.protocol", "tcp"), resource.TestCheckResourceAttr( - "cloudstack_firewall.foo", "rule.1702320581.ports.#", "2"), + "cloudstack_firewall.foo", "rule.60926170.ports.32925333", "8080"), resource.TestCheckResourceAttr( - "cloudstack_firewall.foo", "rule.1702320581.ports.1209010669", "1000-2000"), + "cloudstack_firewall.foo", "rule.716592205.source_cidr", "10.0.0.0/24"), resource.TestCheckResourceAttr( - "cloudstack_firewall.foo", "rule.1702320581.ports.1889509032", "80"), + "cloudstack_firewall.foo", "rule.716592205.protocol", "tcp"), + resource.TestCheckResourceAttr( + "cloudstack_firewall.foo", "rule.716592205.ports.1209010669", "1000-2000"), + resource.TestCheckResourceAttr( + "cloudstack_firewall.foo", "rule.716592205.ports.1889509032", "80"), ), }, @@ -72,27 +82,31 @@ func TestAccCloudStackFirewall_update(t *testing.T) { resource.TestCheckResourceAttr( "cloudstack_firewall.foo", "ipaddress", CLOUDSTACK_PUBLIC_IPADDRESS), resource.TestCheckResourceAttr( - "cloudstack_firewall.foo", "rule.#", "2"), + "cloudstack_firewall.foo", "rule.#", "3"), resource.TestCheckResourceAttr( - "cloudstack_firewall.foo", "rule.1702320581.source_cidr", "10.0.0.0/24"), + "cloudstack_firewall.foo", "rule.2207610982.cidr_list.80081744", "10.0.1.0/24"), resource.TestCheckResourceAttr( - "cloudstack_firewall.foo", "rule.1702320581.protocol", "tcp"), + "cloudstack_firewall.foo", "rule.2207610982.cidr_list.3482919157", "10.0.0.0/24"), resource.TestCheckResourceAttr( - "cloudstack_firewall.foo", "rule.1702320581.ports.#", "2"), + "cloudstack_firewall.foo", "rule.2207610982.protocol", "tcp"), resource.TestCheckResourceAttr( - "cloudstack_firewall.foo", "rule.1702320581.ports.1209010669", "1000-2000"), + "cloudstack_firewall.foo", "rule.2207610982.ports.32925333", "8080"), resource.TestCheckResourceAttr( - "cloudstack_firewall.foo", "rule.1702320581.ports.1889509032", "80"), + "cloudstack_firewall.foo", "rule.716592205.source_cidr", "10.0.0.0/24"), resource.TestCheckResourceAttr( - "cloudstack_firewall.foo", "rule.3779782959.source_cidr", "172.16.100.0/24"), + "cloudstack_firewall.foo", "rule.716592205.protocol", "tcp"), resource.TestCheckResourceAttr( - "cloudstack_firewall.foo", "rule.3779782959.protocol", "tcp"), + "cloudstack_firewall.foo", "rule.716592205.ports.1209010669", "1000-2000"), resource.TestCheckResourceAttr( - "cloudstack_firewall.foo", "rule.3779782959.ports.#", "2"), + "cloudstack_firewall.foo", "rule.716592205.ports.1889509032", "80"), resource.TestCheckResourceAttr( - "cloudstack_firewall.foo", "rule.3779782959.ports.1889509032", "80"), + "cloudstack_firewall.foo", "rule.4449157.source_cidr", "172.16.100.0/24"), resource.TestCheckResourceAttr( - "cloudstack_firewall.foo", "rule.3779782959.ports.3638101695", "443"), + "cloudstack_firewall.foo", "rule.4449157.protocol", "tcp"), + resource.TestCheckResourceAttr( + "cloudstack_firewall.foo", "rule.4449157.ports.1889509032", "80"), + resource.TestCheckResourceAttr( + "cloudstack_firewall.foo", "rule.4449157.ports.3638101695", "443"), ), }, }, @@ -162,6 +176,12 @@ var testAccCloudStackFirewall_basic = fmt.Sprintf(` resource "cloudstack_firewall" "foo" { ipaddress = "%s" + rule { + cidr_list = ["10.0.0.0/24"] + protocol = "tcp" + ports = ["8080"] + } + rule { source_cidr = "10.0.0.0/24" protocol = "tcp" @@ -173,6 +193,12 @@ var testAccCloudStackFirewall_update = fmt.Sprintf(` resource "cloudstack_firewall" "foo" { ipaddress = "%s" + rule { + cidr_list = ["10.0.0.0/24", "10.0.1.0/24"] + protocol = "tcp" + ports = ["8080"] + } + rule { source_cidr = "10.0.0.0/24" protocol = "tcp" diff --git a/builtin/providers/cloudstack/resource_cloudstack_instance_test.go b/builtin/providers/cloudstack/resource_cloudstack_instance_test.go index b0f241a67..ced4514be 100644 --- a/builtin/providers/cloudstack/resource_cloudstack_instance_test.go +++ b/builtin/providers/cloudstack/resource_cloudstack_instance_test.go @@ -80,7 +80,7 @@ func TestAccCloudStackInstance_fixedIP(t *testing.T) { testAccCheckCloudStackInstanceExists( "cloudstack_instance.foobar", &instance), resource.TestCheckResourceAttr( - "cloudstack_instance.foobar", "ipaddress", CLOUDSTACK_NETWORK_1_IPADDRESS), + "cloudstack_instance.foobar", "ipaddress", CLOUDSTACK_NETWORK_1_IPADDRESS1), ), }, }, @@ -268,7 +268,7 @@ resource "cloudstack_instance" "foobar" { }`, CLOUDSTACK_SERVICE_OFFERING_1, CLOUDSTACK_NETWORK_1, - CLOUDSTACK_NETWORK_1_IPADDRESS, + CLOUDSTACK_NETWORK_1_IPADDRESS1, CLOUDSTACK_TEMPLATE, CLOUDSTACK_ZONE) @@ -290,7 +290,7 @@ resource "cloudstack_instance" "foobar" { }`, CLOUDSTACK_SERVICE_OFFERING_1, CLOUDSTACK_NETWORK_1, - CLOUDSTACK_NETWORK_1_IPADDRESS, + CLOUDSTACK_NETWORK_1_IPADDRESS1, CLOUDSTACK_TEMPLATE, CLOUDSTACK_ZONE) diff --git a/builtin/providers/cloudstack/resource_cloudstack_network_acl_rule.go b/builtin/providers/cloudstack/resource_cloudstack_network_acl_rule.go index 10c91de69..2a5542755 100644 --- a/builtin/providers/cloudstack/resource_cloudstack_network_acl_rule.go +++ b/builtin/providers/cloudstack/resource_cloudstack_network_acl_rule.go @@ -1,17 +1,14 @@ package cloudstack import ( - "bytes" "fmt" "regexp" - "sort" "strconv" "strings" "sync" "time" "github.com/hashicorp/go-multierror" - "github.com/hashicorp/terraform/helper/hashcode" "github.com/hashicorp/terraform/helper/schema" "github.com/xanzy/go-cloudstack/cloudstack" ) @@ -47,9 +44,17 @@ func resourceCloudStackNetworkACLRule() *schema.Resource { Default: "allow", }, + "cidr_list": &schema.Schema{ + Type: schema.TypeSet, + Optional: true, + Elem: &schema.Schema{Type: schema.TypeString}, + Set: schema.HashString, + }, + "source_cidr": &schema.Schema{ - Type: schema.TypeString, - Required: true, + Type: schema.TypeString, + Optional: true, + Deprecated: "Please use the `cidr_list` field instead", }, "protocol": &schema.Schema{ @@ -73,9 +78,7 @@ func resourceCloudStackNetworkACLRule() *schema.Resource { Type: schema.TypeSet, Optional: true, Elem: &schema.Schema{Type: schema.TypeString}, - Set: func(v interface{}) int { - return hashcode.String(v.(string)) - }, + Set: schema.HashString, }, "traffic_type": &schema.Schema{ @@ -90,7 +93,6 @@ func resourceCloudStackNetworkACLRule() *schema.Resource { }, }, }, - Set: resourceCloudStackNetworkACLRuleHash, }, }, } @@ -108,11 +110,9 @@ func resourceCloudStackNetworkACLRuleCreate(d *schema.ResourceData, meta interfa // Create all rules that are configured if nrs := d.Get("rule").(*schema.Set); nrs.Len() > 0 { // Create an empty rule set to hold all newly created rules - rules := &schema.Set{ - F: resourceCloudStackNetworkACLRuleHash, - } + rules := resourceCloudStackNetworkACLRule().Schema["rule"].ZeroValue().(*schema.Set) - err := resourceCloudStackNetworkACLRuleCreateRules(d, meta, rules, nrs) + err := createNetworkACLRules(d, meta, rules, nrs) // We need to update this first to preserve the correct state d.Set("rule", rules) @@ -125,7 +125,7 @@ func resourceCloudStackNetworkACLRuleCreate(d *schema.ResourceData, meta interfa return resourceCloudStackNetworkACLRuleRead(d, meta) } -func resourceCloudStackNetworkACLRuleCreateRules( +func createNetworkACLRules( d *schema.ResourceData, meta interface{}, rules *schema.Set, @@ -145,7 +145,7 @@ func resourceCloudStackNetworkACLRuleCreateRules( sem <- struct{}{} // Create a single rule - err := resourceCloudStackNetworkACLRuleCreateRule(d, meta, rule) + err := createNetworkACLRule(d, meta, rule) // If we have at least one UUID, we need to save the rule if len(rule["uuids"].(map[string]interface{})) > 0 { @@ -162,13 +162,10 @@ func resourceCloudStackNetworkACLRuleCreateRules( wg.Wait() - // We need to update this first to preserve the correct state - d.Set("rule", rules) - return errs.ErrorOrNil() } -func resourceCloudStackNetworkACLRuleCreateRule( +func createNetworkACLRule( d *schema.ResourceData, meta interface{}, rule map[string]interface{}) error { @@ -190,7 +187,7 @@ func resourceCloudStackNetworkACLRuleCreateRule( p.SetAction(rule["action"].(string)) // Set the CIDR list - p.SetCidrlist([]string{rule["source_cidr"].(string)}) + p.SetCidrlist(retrieveCidrList(rule)) // Set the traffic type p.SetTraffictype(rule["traffic_type"].(string)) @@ -225,11 +222,7 @@ func resourceCloudStackNetworkACLRuleCreateRule( if ps := rule["ports"].(*schema.Set); ps.Len() > 0 { // Create an empty schema.Set to hold all processed ports - ports := &schema.Set{ - F: func(v interface{}) int { - return hashcode.String(v.(string)) - }, - } + ports := &schema.Set{F: schema.HashString} // Define a regexp for parsing the port re := regexp.MustCompile(`^(\d+)(?:-(\d+))?$`) @@ -296,9 +289,7 @@ func resourceCloudStackNetworkACLRuleRead(d *schema.ResourceData, meta interface } // Create an empty schema.Set to hold all rules - rules := &schema.Set{ - F: resourceCloudStackNetworkACLRuleHash, - } + rules := resourceCloudStackNetworkACLRule().Schema["rule"].ZeroValue().(*schema.Set) // Read all rules that are configured if rs := d.Get("rule").(*schema.Set); rs.Len() > 0 { @@ -324,11 +315,11 @@ func resourceCloudStackNetworkACLRuleRead(d *schema.ResourceData, meta interface // Update the values rule["action"] = strings.ToLower(r.Action) - rule["source_cidr"] = r.Cidrlist rule["protocol"] = r.Protocol rule["icmp_type"] = r.Icmptype rule["icmp_code"] = r.Icmpcode rule["traffic_type"] = strings.ToLower(r.Traffictype) + setCidrList(rule, r.Cidrlist) rules.Add(rule) } @@ -350,9 +341,9 @@ func resourceCloudStackNetworkACLRuleRead(d *schema.ResourceData, meta interface // Update the values rule["action"] = strings.ToLower(r.Action) - rule["source_cidr"] = r.Cidrlist rule["protocol"] = r.Protocol rule["traffic_type"] = strings.ToLower(r.Traffictype) + setCidrList(rule, r.Cidrlist) rules.Add(rule) } @@ -361,11 +352,7 @@ func resourceCloudStackNetworkACLRuleRead(d *schema.ResourceData, meta interface if ps := rule["ports"].(*schema.Set); ps.Len() > 0 { // Create an empty schema.Set to hold all ports - ports := &schema.Set{ - F: func(v interface{}) int { - return hashcode.String(v.(string)) - }, - } + ports := &schema.Set{F: schema.HashString} // Loop through all ports and retrieve their info for _, port := range ps.List() { @@ -386,9 +373,9 @@ func resourceCloudStackNetworkACLRuleRead(d *schema.ResourceData, meta interface // Update the values rule["action"] = strings.ToLower(r.Action) - rule["source_cidr"] = r.Cidrlist rule["protocol"] = r.Protocol rule["traffic_type"] = strings.ToLower(r.Traffictype) + setCidrList(rule, r.Cidrlist) ports.Add(port) } @@ -402,15 +389,20 @@ func resourceCloudStackNetworkACLRuleRead(d *schema.ResourceData, meta interface } } - // If this is a managed firewall, add all unknown rules into a single dummy rule + // If this is a managed firewall, add all unknown rules into dummy rules managed := d.Get("managed").(bool) if managed && len(ruleMap) > 0 { for uuid := range ruleMap { + // We need to create and add a dummy value to a schema.Set as the + // cidr_list is a required field and thus needs a value + cidrs := &schema.Set{F: schema.HashString} + cidrs.Add(uuid) + // Make a dummy rule to hold the unknown UUID rule := map[string]interface{}{ - "source_cidr": uuid, - "protocol": uuid, - "uuids": map[string]interface{}{uuid: uuid}, + "cidr_list": cidrs, + "protocol": uuid, + "uuids": map[string]interface{}{uuid: uuid}, } // Add the dummy rule to the rules set @@ -445,9 +437,9 @@ func resourceCloudStackNetworkACLRuleUpdate(d *schema.ResourceData, meta interfa // set to make sure we end up in a consistent state rules := o.(*schema.Set).Intersection(n.(*schema.Set)) - // Now first loop through all the old rules and delete them - if ors.Len() > 0 { - err := resourceCloudStackNetworkACLRuleDeleteRules(d, meta, rules, ors) + // First loop through all the new rules and create (before destroy) them + if nrs.Len() > 0 { + err := createNetworkACLRules(d, meta, rules, nrs) // We need to update this first to preserve the correct state d.Set("rule", rules) @@ -457,9 +449,9 @@ func resourceCloudStackNetworkACLRuleUpdate(d *schema.ResourceData, meta interfa } } - // Then loop through all the new rules and create them - if nrs.Len() > 0 { - err := resourceCloudStackNetworkACLRuleCreateRules(d, meta, rules, nrs) + // Then loop through all the old rules and delete them + if ors.Len() > 0 { + err := deleteNetworkACLRules(d, meta, rules, ors) // We need to update this first to preserve the correct state d.Set("rule", rules) @@ -476,13 +468,11 @@ func resourceCloudStackNetworkACLRuleUpdate(d *schema.ResourceData, meta interfa func resourceCloudStackNetworkACLRuleDelete(d *schema.ResourceData, meta interface{}) error { // Create an empty rule set to hold all rules that where // not deleted correctly - rules := &schema.Set{ - F: resourceCloudStackNetworkACLRuleHash, - } + rules := resourceCloudStackNetworkACLRule().Schema["rule"].ZeroValue().(*schema.Set) // Delete all rules - if rs := d.Get("rule").(*schema.Set); rs.Len() > 0 { - err := resourceCloudStackNetworkACLRuleDeleteRules(d, meta, rules, rs) + if ors := d.Get("rule").(*schema.Set); ors.Len() > 0 { + err := deleteNetworkACLRules(d, meta, rules, ors) // We need to update this first to preserve the correct state d.Set("rule", rules) @@ -495,7 +485,7 @@ func resourceCloudStackNetworkACLRuleDelete(d *schema.ResourceData, meta interfa return nil } -func resourceCloudStackNetworkACLRuleDeleteRules( +func deleteNetworkACLRules( d *schema.ResourceData, meta interface{}, rules *schema.Set, @@ -515,7 +505,7 @@ func resourceCloudStackNetworkACLRuleDeleteRules( sem <- struct{}{} // Delete a single rule - err := resourceCloudStackNetworkACLRuleDeleteRule(d, meta, rule) + err := deleteNetworkACLRule(d, meta, rule) // If we have at least one UUID, we need to save the rule if len(rule["uuids"].(map[string]interface{})) > 0 { @@ -535,7 +525,7 @@ func resourceCloudStackNetworkACLRuleDeleteRules( return errs.ErrorOrNil() } -func resourceCloudStackNetworkACLRuleDeleteRule( +func deleteNetworkACLRule( d *schema.ResourceData, meta interface{}, rule map[string]interface{}) error { @@ -574,58 +564,6 @@ func resourceCloudStackNetworkACLRuleDeleteRule( return nil } -func resourceCloudStackNetworkACLRuleHash(v interface{}) int { - var buf bytes.Buffer - m := v.(map[string]interface{}) - - // This is a little ugly, but it's needed because these arguments have - // a default value that needs to be part of the string to hash - var action, trafficType string - if a, ok := m["action"]; ok { - action = a.(string) - } else { - action = "allow" - } - if t, ok := m["traffic_type"]; ok { - trafficType = t.(string) - } else { - trafficType = "ingress" - } - - buf.WriteString(fmt.Sprintf( - "%s-%s-%s-%s-", - action, - m["source_cidr"].(string), - m["protocol"].(string), - trafficType)) - - if v, ok := m["icmp_type"]; ok { - buf.WriteString(fmt.Sprintf("%d-", v.(int))) - } - - if v, ok := m["icmp_code"]; ok { - buf.WriteString(fmt.Sprintf("%d-", v.(int))) - } - - // We need to make sure to sort the strings below so that we always - // generate the same hash code no matter what is in the set. - if v, ok := m["ports"]; ok { - vs := v.(*schema.Set).List() - s := make([]string, len(vs)) - - for i, raw := range vs { - s[i] = raw.(string) - } - sort.Strings(s) - - for _, v := range s { - buf.WriteString(fmt.Sprintf("%s-", v)) - } - } - - return hashcode.String(buf.String()) -} - func verifyNetworkACLParams(d *schema.ResourceData) error { managed := d.Get("managed").(bool) _, rules := d.GetOk("rule") @@ -644,6 +582,17 @@ func verifyNetworkACLRuleParams(d *schema.ResourceData, rule map[string]interfac return fmt.Errorf("Parameter action only accepts 'allow' or 'deny' as values") } + cidrList := rule["cidr_list"].(*schema.Set) + sourceCidr := rule["source_cidr"].(string) + if cidrList.Len() == 0 && sourceCidr == "" { + return fmt.Errorf( + "Parameter cidr_list is a required parameter") + } + if cidrList.Len() > 0 && sourceCidr != "" { + return fmt.Errorf( + "Parameter source_cidr is deprecated and cannot be used together with cidr_list") + } + protocol := rule["protocol"].(string) switch protocol { case "icmp": diff --git a/builtin/providers/cloudstack/resource_cloudstack_network_acl_rule_test.go b/builtin/providers/cloudstack/resource_cloudstack_network_acl_rule_test.go index 6f2370f5b..862418f70 100644 --- a/builtin/providers/cloudstack/resource_cloudstack_network_acl_rule_test.go +++ b/builtin/providers/cloudstack/resource_cloudstack_network_acl_rule_test.go @@ -23,19 +23,31 @@ func TestAccCloudStackNetworkACLRule_basic(t *testing.T) { resource.TestCheckResourceAttr( "cloudstack_network_acl_rule.foo", "rule.#", "3"), resource.TestCheckResourceAttr( - "cloudstack_network_acl_rule.foo", "rule.3247834462.action", "allow"), + "cloudstack_network_acl_rule.foo", "rule.2792403380.action", "allow"), resource.TestCheckResourceAttr( - "cloudstack_network_acl_rule.foo", "rule.3247834462.source_cidr", "172.16.100.0/24"), + "cloudstack_network_acl_rule.foo", "rule.2792403380.source_cidr", "172.16.100.0/24"), resource.TestCheckResourceAttr( - "cloudstack_network_acl_rule.foo", "rule.3247834462.protocol", "tcp"), + "cloudstack_network_acl_rule.foo", "rule.2792403380.protocol", "tcp"), resource.TestCheckResourceAttr( - "cloudstack_network_acl_rule.foo", "rule.3247834462.ports.#", "2"), + "cloudstack_network_acl_rule.foo", "rule.2792403380.ports.#", "2"), resource.TestCheckResourceAttr( - "cloudstack_network_acl_rule.foo", "rule.3247834462.ports.1889509032", "80"), + "cloudstack_network_acl_rule.foo", "rule.2792403380.ports.1889509032", "80"), resource.TestCheckResourceAttr( - "cloudstack_network_acl_rule.foo", "rule.3247834462.ports.3638101695", "443"), + "cloudstack_network_acl_rule.foo", "rule.2792403380.ports.3638101695", "443"), resource.TestCheckResourceAttr( - "cloudstack_network_acl_rule.foo", "rule.3247834462.traffic_type", "ingress"), + "cloudstack_network_acl_rule.foo", "rule.2792403380.traffic_type", "ingress"), + resource.TestCheckResourceAttr( + "cloudstack_network_acl_rule.foo", "rule.4029966697.action", "allow"), + resource.TestCheckResourceAttr( + "cloudstack_network_acl_rule.foo", "rule.4029966697.cidr_list.#", "1"), + resource.TestCheckResourceAttr( + "cloudstack_network_acl_rule.foo", "rule.4029966697.cidr_list.3056857544", "172.18.100.0/24"), + resource.TestCheckResourceAttr( + "cloudstack_network_acl_rule.foo", "rule.4029966697.icmp_code", "-1"), + resource.TestCheckResourceAttr( + "cloudstack_network_acl_rule.foo", "rule.4029966697.icmp_type", "-1"), + resource.TestCheckResourceAttr( + "cloudstack_network_acl_rule.foo", "rule.4029966697.traffic_type", "ingress"), ), }, }, @@ -55,19 +67,31 @@ func TestAccCloudStackNetworkACLRule_update(t *testing.T) { resource.TestCheckResourceAttr( "cloudstack_network_acl_rule.foo", "rule.#", "3"), resource.TestCheckResourceAttr( - "cloudstack_network_acl_rule.foo", "rule.3247834462.action", "allow"), + "cloudstack_network_acl_rule.foo", "rule.2792403380.action", "allow"), resource.TestCheckResourceAttr( - "cloudstack_network_acl_rule.foo", "rule.3247834462.source_cidr", "172.16.100.0/24"), + "cloudstack_network_acl_rule.foo", "rule.2792403380.source_cidr", "172.16.100.0/24"), resource.TestCheckResourceAttr( - "cloudstack_network_acl_rule.foo", "rule.3247834462.protocol", "tcp"), + "cloudstack_network_acl_rule.foo", "rule.2792403380.protocol", "tcp"), resource.TestCheckResourceAttr( - "cloudstack_network_acl_rule.foo", "rule.3247834462.ports.#", "2"), + "cloudstack_network_acl_rule.foo", "rule.2792403380.ports.#", "2"), resource.TestCheckResourceAttr( - "cloudstack_network_acl_rule.foo", "rule.3247834462.ports.1889509032", "80"), + "cloudstack_network_acl_rule.foo", "rule.2792403380.ports.1889509032", "80"), resource.TestCheckResourceAttr( - "cloudstack_network_acl_rule.foo", "rule.3247834462.ports.3638101695", "443"), + "cloudstack_network_acl_rule.foo", "rule.2792403380.ports.3638101695", "443"), resource.TestCheckResourceAttr( - "cloudstack_network_acl_rule.foo", "rule.3247834462.traffic_type", "ingress"), + "cloudstack_network_acl_rule.foo", "rule.2792403380.traffic_type", "ingress"), + resource.TestCheckResourceAttr( + "cloudstack_network_acl_rule.foo", "rule.4029966697.action", "allow"), + resource.TestCheckResourceAttr( + "cloudstack_network_acl_rule.foo", "rule.4029966697.cidr_list.#", "1"), + resource.TestCheckResourceAttr( + "cloudstack_network_acl_rule.foo", "rule.4029966697.cidr_list.3056857544", "172.18.100.0/24"), + resource.TestCheckResourceAttr( + "cloudstack_network_acl_rule.foo", "rule.4029966697.icmp_code", "-1"), + resource.TestCheckResourceAttr( + "cloudstack_network_acl_rule.foo", "rule.4029966697.icmp_type", "-1"), + resource.TestCheckResourceAttr( + "cloudstack_network_acl_rule.foo", "rule.4029966697.traffic_type", "ingress"), ), }, @@ -78,33 +102,47 @@ func TestAccCloudStackNetworkACLRule_update(t *testing.T) { resource.TestCheckResourceAttr( "cloudstack_network_acl_rule.foo", "rule.#", "4"), resource.TestCheckResourceAttr( - "cloudstack_network_acl_rule.foo", "rule.3247834462.action", "allow"), + "cloudstack_network_acl_rule.foo", "rule.2254982534.action", "deny"), resource.TestCheckResourceAttr( - "cloudstack_network_acl_rule.foo", "rule.3247834462.source_cidr", "172.16.100.0/24"), + "cloudstack_network_acl_rule.foo", "rule.2254982534.source_cidr", "10.0.0.0/24"), resource.TestCheckResourceAttr( - "cloudstack_network_acl_rule.foo", "rule.3247834462.protocol", "tcp"), + "cloudstack_network_acl_rule.foo", "rule.2254982534.protocol", "tcp"), resource.TestCheckResourceAttr( - "cloudstack_network_acl_rule.foo", "rule.3247834462.ports.#", "2"), + "cloudstack_network_acl_rule.foo", "rule.2254982534.ports.#", "2"), resource.TestCheckResourceAttr( - "cloudstack_network_acl_rule.foo", "rule.3247834462.ports.1889509032", "80"), + "cloudstack_network_acl_rule.foo", "rule.2254982534.ports.1209010669", "1000-2000"), resource.TestCheckResourceAttr( - "cloudstack_network_acl_rule.foo", "rule.3247834462.ports.3638101695", "443"), + "cloudstack_network_acl_rule.foo", "rule.2254982534.ports.1889509032", "80"), resource.TestCheckResourceAttr( - "cloudstack_network_acl_rule.foo", "rule.3247834462.traffic_type", "ingress"), + "cloudstack_network_acl_rule.foo", "rule.2254982534.traffic_type", "egress"), resource.TestCheckResourceAttr( - "cloudstack_network_acl_rule.foo", "rule.4267872693.action", "deny"), + "cloudstack_network_acl_rule.foo", "rule.2704020556.action", "deny"), resource.TestCheckResourceAttr( - "cloudstack_network_acl_rule.foo", "rule.4267872693.source_cidr", "10.0.0.0/24"), + "cloudstack_network_acl_rule.foo", "rule.2704020556.cidr_list.#", "2"), resource.TestCheckResourceAttr( - "cloudstack_network_acl_rule.foo", "rule.4267872693.protocol", "tcp"), + "cloudstack_network_acl_rule.foo", "rule.2704020556.cidr_list.2104435309", "172.18.101.0/24"), resource.TestCheckResourceAttr( - "cloudstack_network_acl_rule.foo", "rule.4267872693.ports.#", "2"), + "cloudstack_network_acl_rule.foo", "rule.2704020556.cidr_list.3056857544", "172.18.100.0/24"), resource.TestCheckResourceAttr( - "cloudstack_network_acl_rule.foo", "rule.4267872693.ports.1209010669", "1000-2000"), + "cloudstack_network_acl_rule.foo", "rule.2704020556.icmp_code", "-1"), resource.TestCheckResourceAttr( - "cloudstack_network_acl_rule.foo", "rule.4267872693.ports.1889509032", "80"), + "cloudstack_network_acl_rule.foo", "rule.2704020556.icmp_type", "-1"), resource.TestCheckResourceAttr( - "cloudstack_network_acl_rule.foo", "rule.4267872693.traffic_type", "egress"), + "cloudstack_network_acl_rule.foo", "rule.2704020556.traffic_type", "ingress"), + resource.TestCheckResourceAttr( + "cloudstack_network_acl_rule.foo", "rule.2792403380.action", "allow"), + resource.TestCheckResourceAttr( + "cloudstack_network_acl_rule.foo", "rule.2792403380.source_cidr", "172.16.100.0/24"), + resource.TestCheckResourceAttr( + "cloudstack_network_acl_rule.foo", "rule.2792403380.protocol", "tcp"), + resource.TestCheckResourceAttr( + "cloudstack_network_acl_rule.foo", "rule.2792403380.ports.#", "2"), + resource.TestCheckResourceAttr( + "cloudstack_network_acl_rule.foo", "rule.2792403380.ports.1889509032", "80"), + resource.TestCheckResourceAttr( + "cloudstack_network_acl_rule.foo", "rule.2792403380.ports.3638101695", "443"), + resource.TestCheckResourceAttr( + "cloudstack_network_acl_rule.foo", "rule.2792403380.traffic_type", "ingress"), ), }, }, @@ -196,7 +234,7 @@ resource "cloudstack_network_acl_rule" "foo" { rule { action = "allow" - source_cidr = "172.18.100.0/24" + cidr_list = ["172.18.100.0/24"] protocol = "icmp" icmp_type = "-1" icmp_code = "-1" @@ -240,7 +278,7 @@ resource "cloudstack_network_acl_rule" "foo" { rule { action = "deny" - source_cidr = "172.18.100.0/24" + cidr_list = ["172.18.100.0/24", "172.18.101.0/24"] protocol = "icmp" icmp_type = "-1" icmp_code = "-1" diff --git a/builtin/providers/cloudstack/resource_cloudstack_port_forward.go b/builtin/providers/cloudstack/resource_cloudstack_port_forward.go index e1f8c99fc..044482bcb 100644 --- a/builtin/providers/cloudstack/resource_cloudstack_port_forward.go +++ b/builtin/providers/cloudstack/resource_cloudstack_port_forward.go @@ -1,13 +1,14 @@ package cloudstack import ( - "bytes" "fmt" + "sync" + "time" "strconv" "strings" - "github.com/hashicorp/terraform/helper/hashcode" + "github.com/hashicorp/go-multierror" "github.com/hashicorp/terraform/helper/schema" "github.com/xanzy/go-cloudstack/cloudstack" ) @@ -63,7 +64,6 @@ func resourceCloudStackPortForward() *schema.Resource { }, }, }, - Set: resourceCloudStackPortForwardHash, }, }, } @@ -82,32 +82,66 @@ func resourceCloudStackPortForwardCreate(d *schema.ResourceData, meta interface{ d.SetId(ipaddressid) // Create all forwards that are configured - if rs := d.Get("forward").(*schema.Set); rs.Len() > 0 { - + if nrs := d.Get("forward").(*schema.Set); nrs.Len() > 0 { // Create an empty schema.Set to hold all forwards - forwards := &schema.Set{ - F: resourceCloudStackPortForwardHash, - } + forwards := resourceCloudStackPortForward().Schema["forward"].ZeroValue().(*schema.Set) - for _, forward := range rs.List() { - // Create a single forward - err := resourceCloudStackPortForwardCreateForward(d, meta, forward.(map[string]interface{})) + err := createPortForwards(d, meta, forwards, nrs) - // We need to update this first to preserve the correct state - forwards.Add(forward) - d.Set("forward", forwards) + // We need to update this first to preserve the correct state + d.Set("forward", forwards) - if err != nil { - return err - } + if err != nil { + return err } } return resourceCloudStackPortForwardRead(d, meta) } -func resourceCloudStackPortForwardCreateForward( - d *schema.ResourceData, meta interface{}, forward map[string]interface{}) error { +func createPortForwards( + d *schema.ResourceData, + meta interface{}, + forwards *schema.Set, + nrs *schema.Set) error { + var errs *multierror.Error + + var wg sync.WaitGroup + wg.Add(nrs.Len()) + + sem := make(chan struct{}, 10) + for _, forward := range nrs.List() { + // Put in a tiny sleep here to avoid DoS'ing the API + time.Sleep(500 * time.Millisecond) + + go func(forward map[string]interface{}) { + defer wg.Done() + sem <- struct{}{} + + // Create a single forward + err := createPortForward(d, meta, forward) + + // If we have a UUID, we need to save the forward + if forward["uuid"].(string) != "" { + forwards.Add(forward) + } + + if err != nil { + errs = multierror.Append(errs, err) + } + + <-sem + }(forward.(map[string]interface{})) + } + + wg.Wait() + + return errs.ErrorOrNil() +} +func createPortForward( + d *schema.ResourceData, + meta interface{}, + forward map[string]interface{}) error { cs := meta.(*cloudstack.CloudStackClient) // Make sure all required parameters are there @@ -167,9 +201,7 @@ func resourceCloudStackPortForwardRead(d *schema.ResourceData, meta interface{}) } // Create an empty schema.Set to hold all forwards - forwards := &schema.Set{ - F: resourceCloudStackPortForwardHash, - } + forwards := resourceCloudStackPortForward().Schema["forward"].ZeroValue().(*schema.Set) // Read all forwards that are configured if rs := d.Get("forward").(*schema.Set); rs.Len() > 0 { @@ -250,26 +282,29 @@ func resourceCloudStackPortForwardUpdate(d *schema.ResourceData, meta interface{ ors := o.(*schema.Set).Difference(n.(*schema.Set)) nrs := n.(*schema.Set).Difference(o.(*schema.Set)) - // Now first loop through all the old forwards and delete any obsolete ones - for _, forward := range ors.List() { - // Delete the forward as it no longer exists in the config - err := resourceCloudStackPortForwardDeleteForward(d, meta, forward.(map[string]interface{})) + // We need to start with a rule set containing all the rules we + // already have and want to keep. Any rules that are not deleted + // correctly and any newly created rules, will be added to this + // set to make sure we end up in a consistent state + forwards := o.(*schema.Set).Intersection(n.(*schema.Set)) + + // First loop through all the new forwards and create (before destroy) them + if nrs.Len() > 0 { + err := createPortForwards(d, meta, forwards, nrs) + + // We need to update this first to preserve the correct state + d.Set("forward", forwards) + if err != nil { return err } } - // Make sure we save the state of the currently configured forwards - forwards := o.(*schema.Set).Intersection(n.(*schema.Set)) - d.Set("forward", forwards) - - // Then loop through all the currently configured forwards and create the new ones - for _, forward := range nrs.List() { - err := resourceCloudStackPortForwardCreateForward( - d, meta, forward.(map[string]interface{})) + // Then loop through all the old forwards and delete them + if ors.Len() > 0 { + err := deletePortForwards(d, meta, forwards, ors) // We need to update this first to preserve the correct state - forwards.Add(forward) d.Set("forward", forwards) if err != nil { @@ -282,26 +317,69 @@ func resourceCloudStackPortForwardUpdate(d *schema.ResourceData, meta interface{ } func resourceCloudStackPortForwardDelete(d *schema.ResourceData, meta interface{}) error { + // Create an empty rule set to hold all rules that where + // not deleted correctly + forwards := resourceCloudStackPortForward().Schema["forward"].ZeroValue().(*schema.Set) + // Delete all forwards - if rs := d.Get("forward").(*schema.Set); rs.Len() > 0 { - for _, forward := range rs.List() { - // Delete a single forward - err := resourceCloudStackPortForwardDeleteForward(d, meta, forward.(map[string]interface{})) + if ors := d.Get("forward").(*schema.Set); ors.Len() > 0 { + err := deletePortForwards(d, meta, forwards, ors) - // We need to update this first to preserve the correct state - d.Set("forward", rs) + // We need to update this first to preserve the correct state + d.Set("forward", forwards) - if err != nil { - return err - } + if err != nil { + return err } } return nil } -func resourceCloudStackPortForwardDeleteForward( - d *schema.ResourceData, meta interface{}, forward map[string]interface{}) error { +func deletePortForwards( + d *schema.ResourceData, + meta interface{}, + forwards *schema.Set, + ors *schema.Set) error { + var errs *multierror.Error + + var wg sync.WaitGroup + wg.Add(ors.Len()) + + sem := make(chan struct{}, 10) + for _, forward := range ors.List() { + // Put a sleep here to avoid DoS'ing the API + time.Sleep(500 * time.Millisecond) + + go func(forward map[string]interface{}) { + defer wg.Done() + sem <- struct{}{} + + // Delete a single forward + err := deletePortForward(d, meta, forward) + + // If we have a UUID, we need to save the forward + if forward["uuid"].(string) != "" { + forwards.Add(forward) + } + + if err != nil { + errs = multierror.Append(errs, err) + } + + <-sem + }(forward.(map[string]interface{})) + } + + wg.Wait() + + return errs.ErrorOrNil() +} + +func deletePortForward( + d *schema.ResourceData, + meta interface{}, + forward map[string]interface{}) error { cs := meta.(*cloudstack.CloudStackClient) // Create the parameter struct @@ -323,19 +401,6 @@ func resourceCloudStackPortForwardDeleteForward( return nil } -func resourceCloudStackPortForwardHash(v interface{}) int { - var buf bytes.Buffer - m := v.(map[string]interface{}) - buf.WriteString(fmt.Sprintf( - "%s-%d-%d-%s", - m["protocol"].(string), - m["private_port"].(int), - m["public_port"].(int), - m["virtual_machine"].(string))) - - return hashcode.String(buf.String()) -} - func verifyPortForwardParams(d *schema.ResourceData, forward map[string]interface{}) error { protocol := forward["protocol"].(string) if protocol != "tcp" && protocol != "udp" { diff --git a/builtin/providers/cloudstack/resource_cloudstack_port_forward_test.go b/builtin/providers/cloudstack/resource_cloudstack_port_forward_test.go index b0851753f..63dcdb001 100644 --- a/builtin/providers/cloudstack/resource_cloudstack_port_forward_test.go +++ b/builtin/providers/cloudstack/resource_cloudstack_port_forward_test.go @@ -23,13 +23,13 @@ func TestAccCloudStackPortForward_basic(t *testing.T) { resource.TestCheckResourceAttr( "cloudstack_port_forward.foo", "ipaddress", CLOUDSTACK_PUBLIC_IPADDRESS), resource.TestCheckResourceAttr( - "cloudstack_port_forward.foo", "forward.1537694805.protocol", "tcp"), + "cloudstack_port_forward.foo", "forward.952396423.protocol", "tcp"), resource.TestCheckResourceAttr( - "cloudstack_port_forward.foo", "forward.1537694805.private_port", "443"), + "cloudstack_port_forward.foo", "forward.952396423.private_port", "443"), resource.TestCheckResourceAttr( - "cloudstack_port_forward.foo", "forward.1537694805.public_port", "8443"), + "cloudstack_port_forward.foo", "forward.952396423.public_port", "8443"), resource.TestCheckResourceAttr( - "cloudstack_port_forward.foo", "forward.1537694805.virtual_machine", "terraform-test"), + "cloudstack_port_forward.foo", "forward.952396423.virtual_machine", "terraform-test"), ), }, }, @@ -51,13 +51,13 @@ func TestAccCloudStackPortForward_update(t *testing.T) { resource.TestCheckResourceAttr( "cloudstack_port_forward.foo", "forward.#", "1"), resource.TestCheckResourceAttr( - "cloudstack_port_forward.foo", "forward.1537694805.protocol", "tcp"), + "cloudstack_port_forward.foo", "forward.952396423.protocol", "tcp"), resource.TestCheckResourceAttr( - "cloudstack_port_forward.foo", "forward.1537694805.private_port", "443"), + "cloudstack_port_forward.foo", "forward.952396423.private_port", "443"), resource.TestCheckResourceAttr( - "cloudstack_port_forward.foo", "forward.1537694805.public_port", "8443"), + "cloudstack_port_forward.foo", "forward.952396423.public_port", "8443"), resource.TestCheckResourceAttr( - "cloudstack_port_forward.foo", "forward.1537694805.virtual_machine", "terraform-test"), + "cloudstack_port_forward.foo", "forward.952396423.virtual_machine", "terraform-test"), ), }, @@ -70,21 +70,21 @@ func TestAccCloudStackPortForward_update(t *testing.T) { resource.TestCheckResourceAttr( "cloudstack_port_forward.foo", "forward.#", "2"), resource.TestCheckResourceAttr( - "cloudstack_port_forward.foo", "forward.8416686.protocol", "tcp"), + "cloudstack_port_forward.foo", "forward.260687715.protocol", "tcp"), resource.TestCheckResourceAttr( - "cloudstack_port_forward.foo", "forward.8416686.private_port", "80"), + "cloudstack_port_forward.foo", "forward.260687715.private_port", "80"), resource.TestCheckResourceAttr( - "cloudstack_port_forward.foo", "forward.8416686.public_port", "8080"), + "cloudstack_port_forward.foo", "forward.260687715.public_port", "8080"), resource.TestCheckResourceAttr( - "cloudstack_port_forward.foo", "forward.8416686.virtual_machine", "terraform-test"), + "cloudstack_port_forward.foo", "forward.260687715.virtual_machine", "terraform-test"), resource.TestCheckResourceAttr( - "cloudstack_port_forward.foo", "forward.1537694805.protocol", "tcp"), + "cloudstack_port_forward.foo", "forward.952396423.protocol", "tcp"), resource.TestCheckResourceAttr( - "cloudstack_port_forward.foo", "forward.1537694805.private_port", "443"), + "cloudstack_port_forward.foo", "forward.952396423.private_port", "443"), resource.TestCheckResourceAttr( - "cloudstack_port_forward.foo", "forward.1537694805.public_port", "8443"), + "cloudstack_port_forward.foo", "forward.952396423.public_port", "8443"), resource.TestCheckResourceAttr( - "cloudstack_port_forward.foo", "forward.1537694805.virtual_machine", "terraform-test"), + "cloudstack_port_forward.foo", "forward.952396423.virtual_machine", "terraform-test"), ), }, }, diff --git a/builtin/providers/cloudstack/resource_cloudstack_secondary_ipaddress_test.go b/builtin/providers/cloudstack/resource_cloudstack_secondary_ipaddress_test.go index beedcd2cb..dd59ca3f4 100644 --- a/builtin/providers/cloudstack/resource_cloudstack_secondary_ipaddress_test.go +++ b/builtin/providers/cloudstack/resource_cloudstack_secondary_ipaddress_test.go @@ -43,7 +43,7 @@ func TestAccCloudStackSecondaryIPAddress_fixedIP(t *testing.T) { "cloudstack_secondary_ipaddress.foo", &ip), testAccCheckCloudStackSecondaryIPAddressAttributes(&ip), resource.TestCheckResourceAttr( - "cloudstack_secondary_ipaddress.foo", "ipaddress", CLOUDSTACK_NETWORK_1_IPADDRESS), + "cloudstack_secondary_ipaddress.foo", "ipaddress", CLOUDSTACK_NETWORK_1_IPADDRESS1), ), }, }, @@ -117,7 +117,7 @@ func testAccCheckCloudStackSecondaryIPAddressAttributes( ip *cloudstack.AddIpToNicResponse) resource.TestCheckFunc { return func(s *terraform.State) error { - if ip.Ipaddress != CLOUDSTACK_NETWORK_1_IPADDRESS { + if ip.Ipaddress != CLOUDSTACK_NETWORK_1_IPADDRESS1 { return fmt.Errorf("Bad IP address: %s", ip.Ipaddress) } return nil @@ -222,4 +222,4 @@ resource "cloudstack_secondary_ipaddress" "foo" { CLOUDSTACK_NETWORK_1, CLOUDSTACK_TEMPLATE, CLOUDSTACK_ZONE, - CLOUDSTACK_NETWORK_1_IPADDRESS) + CLOUDSTACK_NETWORK_1_IPADDRESS1) diff --git a/builtin/providers/cloudstack/resources.go b/builtin/providers/cloudstack/resources.go index 2fe67c6e3..4182b947f 100644 --- a/builtin/providers/cloudstack/resources.go +++ b/builtin/providers/cloudstack/resources.go @@ -4,6 +4,7 @@ import ( "fmt" "log" "regexp" + "strings" "time" "github.com/hashicorp/terraform/helper/schema" @@ -145,3 +146,36 @@ func Retry(n int, f RetryFunc) (interface{}, error) { return nil, lastErr } + +// This is a temporary helper function to support both the new +// cidr_list and the deprecated source_cidr parameter +func retrieveCidrList(rule map[string]interface{}) []string { + sourceCidr := rule["source_cidr"].(string) + if sourceCidr != "" { + return []string{sourceCidr} + } + + var cidrList []string + for _, cidr := range rule["cidr_list"].(*schema.Set).List() { + cidrList = append(cidrList, cidr.(string)) + } + + return cidrList +} + +// This is a temporary helper function to support both the new +// cidr_list and the deprecated source_cidr parameter +func setCidrList(rule map[string]interface{}, cidrList string) { + sourceCidr := rule["source_cidr"].(string) + if sourceCidr != "" { + rule["source_cidr"] = cidrList + return + } + + cidrs := &schema.Set{F: schema.HashString} + for _, cidr := range strings.Split(cidrList, ",") { + cidrs.Add(cidr) + } + + rule["cidr_list"] = cidrs +} diff --git a/website/source/docs/providers/cloudstack/r/egress_firewall.html.markdown b/website/source/docs/providers/cloudstack/r/egress_firewall.html.markdown index b905bc0e9..dfb42281a 100644 --- a/website/source/docs/providers/cloudstack/r/egress_firewall.html.markdown +++ b/website/source/docs/providers/cloudstack/r/egress_firewall.html.markdown @@ -17,7 +17,7 @@ resource "cloudstack_egress_firewall" "default" { network = "test-network" rule { - source_cidr = "10.0.0.0/8" + cidr_list = ["10.0.0.0/8"] protocol = "tcp" ports = ["80", "1000-2000"] } @@ -40,7 +40,10 @@ The following arguments are supported: The `rule` block supports: -* `source_cidr` - (Required) The source CIDR to allow access to the given ports. +* `cidr_list` - (Required) A CIDR list to allow access to the given ports. + +* `source_cidr` - (Optional, Deprecated) The source CIDR to allow access to the + given ports. This attribute is deprecated, please use `cidr_list` instead. * `protocol` - (Required) The name of the protocol to allow. Valid options are: `tcp`, `udp` and `icmp`. diff --git a/website/source/docs/providers/cloudstack/r/firewall.html.markdown b/website/source/docs/providers/cloudstack/r/firewall.html.markdown index 455449049..70478fa10 100644 --- a/website/source/docs/providers/cloudstack/r/firewall.html.markdown +++ b/website/source/docs/providers/cloudstack/r/firewall.html.markdown @@ -17,7 +17,7 @@ resource "cloudstack_firewall" "default" { ipaddress = "192.168.0.1" rule { - source_cidr = "10.0.0.0/8" + cidr_list = ["10.0.0.0/8"] protocol = "tcp" ports = ["80", "1000-2000"] } @@ -40,7 +40,10 @@ The following arguments are supported: The `rule` block supports: -* `source_cidr` - (Required) The source CIDR to allow access to the given ports. +* `cidr_list` - (Required) A CIDR list to allow access to the given ports. + +* `source_cidr` - (Optional, Deprecated) The source CIDR to allow access to the + given ports. This attribute is deprecated, please use `cidr_list` instead. * `protocol` - (Required) The name of the protocol to allow. Valid options are: `tcp`, `udp` and `icmp`. diff --git a/website/source/docs/providers/cloudstack/r/network_acl_rule.html.markdown b/website/source/docs/providers/cloudstack/r/network_acl_rule.html.markdown index f82b8f446..59b6f1b9a 100644 --- a/website/source/docs/providers/cloudstack/r/network_acl_rule.html.markdown +++ b/website/source/docs/providers/cloudstack/r/network_acl_rule.html.markdown @@ -18,7 +18,7 @@ resource "cloudstack_network_acl_rule" "default" { rule { action = "allow" - source_cidr = "10.0.0.0/8" + cidr_list = ["10.0.0.0/8"] protocol = "tcp" ports = ["80", "1000-2000"] traffic_type = "ingress" @@ -45,7 +45,10 @@ The `rule` block supports: * `action` - (Optional) The action for the rule. Valid options are: `allow` and `deny` (defaults allow). -* `source_cidr` - (Required) The source CIDR to allow access to the given ports. +* `cidr_list` - (Required) A CIDR list to allow access to the given ports. + +* `source_cidr` - (Optional, Deprecated) The source CIDR to allow access to the + given ports. This attribute is deprecated, please use `cidr_list` instead. * `protocol` - (Required) The name of the protocol to allow. Valid options are: `tcp`, `udp`, `icmp`, `all` or a valid protocol number.