From ecb41d478aff5687b796c2b2c05cc542b39b7ed8 Mon Sep 17 00:00:00 2001 From: Sander van Harmelen Date: Wed, 18 May 2016 12:22:32 +0200 Subject: [PATCH] Make replacing the ACL of a network update the network in place This prevents having to recreate the whole network and fixes #6630 --- .../cloudstack/resource_cloudstack_network.go | 21 ++++- .../resource_cloudstack_network_acl.go | 4 +- .../resource_cloudstack_network_test.go | 83 +++++++++++++++++++ .../cloudstack/r/network.html.markdown | 3 +- 4 files changed, 106 insertions(+), 5 deletions(-) diff --git a/builtin/providers/cloudstack/resource_cloudstack_network.go b/builtin/providers/cloudstack/resource_cloudstack_network.go index 69dc27091..458a768fe 100644 --- a/builtin/providers/cloudstack/resource_cloudstack_network.go +++ b/builtin/providers/cloudstack/resource_cloudstack_network.go @@ -86,14 +86,12 @@ func resourceCloudStackNetwork() *schema.Resource { Type: schema.TypeString, Optional: true, Computed: true, - ForceNew: true, ConflictsWith: []string{"aclid"}, }, "aclid": &schema.Schema{ Type: schema.TypeString, Optional: true, - ForceNew: true, Deprecated: "Please use the `acl_id` field instead", }, @@ -298,6 +296,25 @@ func resourceCloudStackNetworkUpdate(d *schema.ResourceData, meta interface{}) e "Error updating network %s: %s", name, err) } + // Replace the ACL if the ID has changed + if d.HasChange("acl_id") || d.HasChange("acl") { + aclid, ok := d.GetOk("acl_id") + if !ok { + aclid, ok = d.GetOk("acl") + } + if !ok { + return fmt.Errorf("Replacing the ACL requires a valid ACL ID") + } + + p := cs.NetworkACL.NewReplaceNetworkACLListParams(aclid.(string)) + p.SetNetworkid(d.Id()) + + _, err := cs.NetworkACL.ReplaceNetworkACLList(p) + if err != nil { + return fmt.Errorf("Error replacing ACL: %s", err) + } + } + // Update tags if they have changed if d.HasChange("tags") { err = setTags(cs, d, "network") diff --git a/builtin/providers/cloudstack/resource_cloudstack_network_acl.go b/builtin/providers/cloudstack/resource_cloudstack_network_acl.go index c39c695d9..188c5f46b 100644 --- a/builtin/providers/cloudstack/resource_cloudstack_network_acl.go +++ b/builtin/providers/cloudstack/resource_cloudstack_network_acl.go @@ -117,7 +117,9 @@ func resourceCloudStackNetworkACLDelete(d *schema.ResourceData, meta interface{} p := cs.NetworkACL.NewDeleteNetworkACLListParams(d.Id()) // Delete the network ACL list - _, err := cs.NetworkACL.DeleteNetworkACLList(p) + _, err := Retry(3, func() (interface{}, error) { + return cs.NetworkACL.DeleteNetworkACLList(p) + }) if err != nil { // This is a very poor way to be told the ID does no longer exist :( if strings.Contains(err.Error(), fmt.Sprintf( diff --git a/builtin/providers/cloudstack/resource_cloudstack_network_test.go b/builtin/providers/cloudstack/resource_cloudstack_network_test.go index 49400dad7..0333f6c25 100644 --- a/builtin/providers/cloudstack/resource_cloudstack_network_test.go +++ b/builtin/providers/cloudstack/resource_cloudstack_network_test.go @@ -50,6 +50,35 @@ func TestAccCloudStackNetwork_vpc(t *testing.T) { }) } +func TestAccCloudStackNetwork_updateACL(t *testing.T) { + var network cloudstack.Network + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckCloudStackNetworkDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: testAccCloudStackNetwork_acl, + Check: resource.ComposeTestCheckFunc( + testAccCheckCloudStackNetworkExists( + "cloudstack_network.foo", &network), + testAccCheckCloudStackNetworkVPCAttributes(&network), + ), + }, + + resource.TestStep{ + Config: testAccCloudStackNetwork_updateACL, + Check: resource.ComposeTestCheckFunc( + testAccCheckCloudStackNetworkExists( + "cloudstack_network.foo", &network), + testAccCheckCloudStackNetworkVPCAttributes(&network), + ), + }, + }, + }) +} + func testAccCheckCloudStackNetworkExists( n string, network *cloudstack.Network) resource.TestCheckFunc { return func(s *terraform.State) error { @@ -193,3 +222,57 @@ resource "cloudstack_network" "foo" { CLOUDSTACK_ZONE, CLOUDSTACK_VPC_NETWORK_CIDR, CLOUDSTACK_VPC_NETWORK_OFFERING) + +var testAccCloudStackNetwork_acl = fmt.Sprintf(` +resource "cloudstack_vpc" "foobar" { + name = "terraform-vpc" + cidr = "%s" + vpc_offering = "%s" + zone = "%s" +} + +resource "cloudstack_network_acl" "foo" { + name = "foo" + vpc_id = "${cloudstack_vpc.foobar.id}" +} + +resource "cloudstack_network" "foo" { + name = "terraform-network" + cidr = "%s" + network_offering = "%s" + vpc_id = "${cloudstack_vpc.foobar.id}" + acl_id = "${cloudstack_network_acl.foo.id}" + zone = "${cloudstack_vpc.foobar.zone}" +}`, + CLOUDSTACK_VPC_CIDR_1, + CLOUDSTACK_VPC_OFFERING, + CLOUDSTACK_ZONE, + CLOUDSTACK_VPC_NETWORK_CIDR, + CLOUDSTACK_VPC_NETWORK_OFFERING) + +var testAccCloudStackNetwork_updateACL = fmt.Sprintf(` +resource "cloudstack_vpc" "foobar" { + name = "terraform-vpc" + cidr = "%s" + vpc_offering = "%s" + zone = "%s" +} + +resource "cloudstack_network_acl" "bar" { + name = "bar" + vpc_id = "${cloudstack_vpc.foobar.id}" +} + +resource "cloudstack_network" "foo" { + name = "terraform-network" + cidr = "%s" + network_offering = "%s" + vpc_id = "${cloudstack_vpc.foobar.id}" + acl_id = "${cloudstack_network_acl.bar.id}" + zone = "${cloudstack_vpc.foobar.zone}" +}`, + CLOUDSTACK_VPC_CIDR_1, + CLOUDSTACK_VPC_OFFERING, + CLOUDSTACK_ZONE, + CLOUDSTACK_VPC_NETWORK_CIDR, + CLOUDSTACK_VPC_NETWORK_OFFERING) diff --git a/website/source/docs/providers/cloudstack/r/network.html.markdown b/website/source/docs/providers/cloudstack/r/network.html.markdown index 3f00f2ee5..5d40a43cf 100644 --- a/website/source/docs/providers/cloudstack/r/network.html.markdown +++ b/website/source/docs/providers/cloudstack/r/network.html.markdown @@ -57,10 +57,9 @@ The following arguments are supported: for. Changing this forces a new resource to be created. * `acl_id` - (Optional) The network ACL ID that should be attached to the network. - Changing this forces a new resource to be created. * `aclid` - (Optional, Deprecated) The ID of a network ACL that should be attached - to the network. Changing this forces a new resource to be created. + to the network. * `project` - (Optional) The name or ID of the project to deploy this instance to. Changing this forces a new resource to be created.