Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

provider/aws: Support multiple subnets in Network ACL #1931

Merged
merged 5 commits into from
May 15, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 104 additions & 12 deletions builtin/providers/aws/resource_aws_network_acl.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"bytes"
"fmt"
"log"
"sort"
"strconv"
"time"

Expand All @@ -30,10 +31,19 @@ func resourceAwsNetworkAcl() *schema.Resource {
Computed: false,
},
"subnet_id": &schema.Schema{
Type: schema.TypeString,
Optional: true,
ForceNew: true,
Computed: false,
Type: schema.TypeString,
Optional: true,
ForceNew: true,
Computed: false,
ConflictsWith: []string{"subnet_ids"},
Deprecated: "Attribute subnet_id is deprecated on network_acl resources. Use subnet_ids instead",
},
"subnet_ids": &schema.Schema{
Type: schema.TypeSet,
Optional: true,
ConflictsWith: []string{"subnet_id"},
Elem: &schema.Schema{Type: schema.TypeString},
Set: schema.HashString,
},
"ingress": &schema.Schema{
Type: schema.TypeSet,
Expand Down Expand Up @@ -168,6 +178,15 @@ func resourceAwsNetworkAclRead(d *schema.ResourceData, meta interface{}) error {
d.Set("vpc_id", networkAcl.VPCID)
d.Set("tags", tagsToMapSDK(networkAcl.Tags))

var s []string
for _, a := range networkAcl.Associations {
s = append(s, *a.SubnetID)
}
sort.Strings(s)
if err := d.Set("subnet_ids", s); err != nil {
return err
}

if err := d.Set("ingress", networkAclEntriesToMapList(ingressEntries)); err != nil {
return err
}
Expand Down Expand Up @@ -213,6 +232,61 @@ func resourceAwsNetworkAclUpdate(d *schema.ResourceData, meta interface{}) error
}
}

if d.HasChange("subnet_ids") {
o, n := d.GetChange("subnet_ids")
if o == nil {
o = new(schema.Set)
}
if n == nil {
n = new(schema.Set)
}

os := o.(*schema.Set)
ns := n.(*schema.Set)

remove := os.Difference(ns).List()
add := ns.Difference(os).List()

if len(remove) > 0 {
// A Network ACL is required for each subnet. In order to disassociate a
// subnet from this ACL, we must associate it with the default ACL.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh the dances we do for Amazon. 😁

defaultAcl, err := getDefaultNetworkAcl(d.Get("vpc_id").(string), conn)
if err != nil {
return fmt.Errorf("Failed to find Default ACL for VPC %s", d.Get("vpc_id").(string))
}
for _, r := range remove {
association, err := findNetworkAclAssociation(r.(string), conn)
if err != nil {
return fmt.Errorf("Failed to find acl association: acl %s with subnet %s: %s", d.Id(), r, err)
}
_, err = conn.ReplaceNetworkACLAssociation(&ec2.ReplaceNetworkACLAssociationInput{
AssociationID: association.NetworkACLAssociationID,
NetworkACLID: defaultAcl.NetworkACLID,
})
if err != nil {
return err
}
}
}

if len(add) > 0 {
for _, a := range add {
association, err := findNetworkAclAssociation(a.(string), conn)
if err != nil {
return fmt.Errorf("Failed to find acl association: acl %s with subnet %s: %s", d.Id(), a, err)
}
_, err = conn.ReplaceNetworkACLAssociation(&ec2.ReplaceNetworkACLAssociationInput{
AssociationID: association.NetworkACLAssociationID,
NetworkACLID: aws.String(d.Id()),
})
if err != nil {
return err
}
}
}

}

if err := setTagsSDK(conn, d); err != nil {
return err
} else {
Expand Down Expand Up @@ -326,18 +400,36 @@ func resourceAwsNetworkAclDelete(d *schema.ResourceData, meta interface{}) error
case "DependencyViolation":
// In case of dependency violation, we remove the association between subnet and network acl.
// This means the subnet is attached to default acl of vpc.
association, err := findNetworkAclAssociation(d.Get("subnet_id").(string), conn)
if err != nil {
return resource.RetryError{Err: fmt.Errorf("Dependency violation: Cannot delete acl %s: %s", d.Id(), err)}
var associations []*ec2.NetworkACLAssociation
if v, ok := d.GetOk("subnet_id"); ok {
a, err := findNetworkAclAssociation(v.(string), conn)
if err != nil {
return resource.RetryError{Err: fmt.Errorf("Dependency violation: Cannot delete acl %s: %s", d.Id(), err)}
}
associations = append(associations, a)
}

if v, ok := d.GetOk("subnet_ids"); ok {
ids := v.(*schema.Set).List()
for _, i := range ids {
a, err := findNetworkAclAssociation(i.(string), conn)
if err != nil {
return resource.RetryError{Err: fmt.Errorf("Dependency violation: Cannot delete acl %s: %s", d.Id(), err)}
}
associations = append(associations, a)
}
}
defaultAcl, err := getDefaultNetworkAcl(d.Get("vpc_id").(string), conn)
if err != nil {
return resource.RetryError{Err: fmt.Errorf("Dependency violation: Cannot delete acl %s: %s", d.Id(), err)}
}
_, err = conn.ReplaceNetworkACLAssociation(&ec2.ReplaceNetworkACLAssociationInput{
AssociationID: association.NetworkACLAssociationID,
NetworkACLID: defaultAcl.NetworkACLID,
})

for _, a := range associations {
_, err = conn.ReplaceNetworkACLAssociation(&ec2.ReplaceNetworkACLAssociationInput{
AssociationID: a.NetworkACLAssociationID,
NetworkACLID: defaultAcl.NetworkACLID,
})
}
return resource.RetryError{Err: err}
default:
// Any other error, we want to quit the retry loop immediately
Expand Down Expand Up @@ -417,7 +509,7 @@ func findNetworkAclAssociation(subnetId string, conn *ec2.EC2) (networkAclAssoci
}
}
}
return nil, fmt.Errorf("could not find association for subnet %s ", subnetId)
return nil, fmt.Errorf("could not find association for subnet: %s ", subnetId)
}

// networkAclEntriesToMapList turns ingress/egress rules read from AWS into a list
Expand Down
102 changes: 98 additions & 4 deletions builtin/providers/aws/resource_aws_network_acl_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,49 @@ func TestAccAWSNetworkAcl_SubnetChange(t *testing.T) {

}

func TestAccAWSNetworkAcl_Subnets(t *testing.T) {
var networkAcl ec2.NetworkACL

checkACLSubnets := func(acl *ec2.NetworkACL, count int) resource.TestCheckFunc {
return func(*terraform.State) (err error) {
if count != len(acl.Associations) {
return fmt.Errorf("ACL association count does not match, expected %d, got %d", count, len(acl.Associations))
}

return nil
}
}

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSNetworkAclDestroy,
Steps: []resource.TestStep{
resource.TestStep{
Config: testAccAWSNetworkAclSubnet_SubnetIds,
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSNetworkAclExists("aws_network_acl.bar", &networkAcl),
testAccCheckSubnetIsAssociatedWithAcl("aws_network_acl.bar", "aws_subnet.one"),
testAccCheckSubnetIsAssociatedWithAcl("aws_network_acl.bar", "aws_subnet.two"),
checkACLSubnets(&networkAcl, 2),
),
},

resource.TestStep{
Config: testAccAWSNetworkAclSubnet_SubnetIdsUpdate,
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSNetworkAclExists("aws_network_acl.bar", &networkAcl),
testAccCheckSubnetIsAssociatedWithAcl("aws_network_acl.bar", "aws_subnet.one"),
testAccCheckSubnetIsAssociatedWithAcl("aws_network_acl.bar", "aws_subnet.three"),
testAccCheckSubnetIsAssociatedWithAcl("aws_network_acl.bar", "aws_subnet.four"),
checkACLSubnets(&networkAcl, 3),
),
},
},
})

}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add another step to this test to exercise the update functionality?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can, but the test doesn't pass ATM b/c plan isn't empty (sorting issue mentioned above)


func testAccCheckAWSNetworkAclDestroy(s *terraform.State) error {
conn := testAccProvider.Meta().(*AWSClient).ec2conn

Expand Down Expand Up @@ -281,10 +324,6 @@ func testAccCheckSubnetIsAssociatedWithAcl(acl string, sub string) resource.Test
return nil
}

// r, _ := conn.NetworkACLs([]string{}, ec2.NewFilter())
// fmt.Printf("\n\nall acls\n %#v\n\n", r.NetworkAcls)
// conn.NetworkAcls([]string{}, filter)

return fmt.Errorf("Network Acl %s is not associated with subnet %s", acl, sub)
}
}
Expand Down Expand Up @@ -494,3 +533,58 @@ resource "aws_network_acl" "bar" {
subnet_id = "${aws_subnet.new.id}"
}
`

const testAccAWSNetworkAclSubnet_SubnetIds = `
resource "aws_vpc" "foo" {
cidr_block = "10.1.0.0/16"
tags {
Name = "acl-subnets-test"
}
}
resource "aws_subnet" "one" {
cidr_block = "10.1.111.0/24"
vpc_id = "${aws_vpc.foo.id}"
}
resource "aws_subnet" "two" {
cidr_block = "10.1.1.0/24"
vpc_id = "${aws_vpc.foo.id}"
}
resource "aws_network_acl" "bar" {
vpc_id = "${aws_vpc.foo.id}"
subnet_ids = ["${aws_subnet.one.id}", "${aws_subnet.two.id}"]
}
`

const testAccAWSNetworkAclSubnet_SubnetIdsUpdate = `
resource "aws_vpc" "foo" {
cidr_block = "10.1.0.0/16"
tags {
Name = "acl-subnets-test"
}
}
resource "aws_subnet" "one" {
cidr_block = "10.1.111.0/24"
vpc_id = "${aws_vpc.foo.id}"
}
resource "aws_subnet" "two" {
cidr_block = "10.1.1.0/24"
vpc_id = "${aws_vpc.foo.id}"
}

resource "aws_subnet" "three" {
cidr_block = "10.1.222.0/24"
vpc_id = "${aws_vpc.foo.id}"
}
resource "aws_subnet" "four" {
cidr_block = "10.1.4.0/24"
vpc_id = "${aws_vpc.foo.id}"
}
resource "aws_network_acl" "bar" {
vpc_id = "${aws_vpc.foo.id}"
subnet_ids = [
"${aws_subnet.one.id}",
"${aws_subnet.three.id}",
"${aws_subnet.four.id}",
]
}
`
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,9 @@ resource "aws_network_acl" "main" {
The following arguments are supported:

* `vpc_id` - (Required) The ID of the associated VPC.
* `subnet_id` - (Optional) The ID of the associated subnet.
* `subnet_ids` - (Optional) A list of Subnet IDs to apply the ACL to
* `subnet_id` - (Optional, Deprecated) The ID of the associated Subnet. This
attribute is deprecated, please use the `subnet_ids` attribute instead
* `ingress` - (Optional) Specifies an ingress rule. Parameters defined below.
* `egress` - (Optional) Specifies an egress rule. Parameters defined below.
* `tags` - (Optional) A mapping of tags to assign to the resource.
Expand Down