From 4b154b8fe76f2fe7149c8d4cd2a8af8e3f2cf194 Mon Sep 17 00:00:00 2001 From: Sneha Somwanshi Date: Wed, 3 Dec 2014 13:23:18 +0530 Subject: [PATCH] Fixed update of ingress/egress rules --- .../providers/aws/resource_aws_network_acl.go | 86 +++--- .../aws/resource_aws_network_acl_test.go | 257 +++++++++++++++--- 2 files changed, 269 insertions(+), 74 deletions(-) diff --git a/builtin/providers/aws/resource_aws_network_acl.go b/builtin/providers/aws/resource_aws_network_acl.go index eb39e2450..c26bf5f19 100644 --- a/builtin/providers/aws/resource_aws_network_acl.go +++ b/builtin/providers/aws/resource_aws_network_acl.go @@ -23,15 +23,15 @@ func resourceAwsNetworkAcl() *schema.Resource { Schema: map[string]*schema.Schema{ "vpc_id": &schema.Schema{ Type: schema.TypeString, - Optional: true, + Required: true, ForceNew: true, - Computed: true, + Computed: false, }, "subnet_id": &schema.Schema{ Type: schema.TypeString, Optional: true, ForceNew: true, - Computed: true, + Computed: false, }, "ingress": &schema.Schema{ Type: schema.TypeSet, @@ -179,17 +179,19 @@ func resourceAwsNetworkAclUpdate(d *schema.ResourceData, meta interface{}) error } } - if(d.HasChange("subnet_id")) { - association, err := findNetworkAclAssociation(d.Get("subnet_id").(string), ec2conn) - if(err != nil){ - return fmt.Errorf("Depedency voilation: Could find association: %s", d.Id(), err) + if d.HasChange("subnet_id") { + + //associate new subnet with the acl. + _, n := d.GetChange("subnet_id") + newSubnet := n.(string) + association, err := findNetworkAclAssociation(newSubnet, ec2conn) + if err != nil { + return fmt.Errorf("Failed to update acl %s with subnet %s: ", d.Id(), newSubnet, err) } - // change acl and subnet association if subnet_id has changed _, err = ec2conn.ReplaceNetworkAclAssociation(association.NetworkAclAssociationId, d.Id()) if err != nil { return err } - } d.Partial(false) @@ -199,8 +201,6 @@ func resourceAwsNetworkAclUpdate(d *schema.ResourceData, meta interface{}) error func updateNetworkAclEntries(d *schema.ResourceData, entryType string, ec2conn *ec2.EC2) error { o, n := d.GetChange(entryType) - fmt.Printf("Old : %s", o) - fmt.Printf("New : %s", n) if o == nil { o = new(schema.Set) @@ -211,10 +211,8 @@ func updateNetworkAclEntries(d *schema.ResourceData, entryType string, ec2conn * os := o.(*schema.Set) ns := n.(*schema.Set) - toBeDeleted := expandNetworkAclEntries(os.Difference(ns).List(), entryType) toBeCreated := expandNetworkAclEntries(ns.Difference(os).List(), entryType) - fmt.Printf("to be created %s", toBeCreated) for _, remove := range toBeDeleted { // Delete old Acl _, err := ec2conn.DeleteNetworkAclEntry(d.Id(), remove.RuleNumber, remove.Egress) @@ -222,12 +220,10 @@ func updateNetworkAclEntries(d *schema.ResourceData, entryType string, ec2conn * return fmt.Errorf("Error deleting %s entry: %s", entryType, err) } } - fmt.Printf("to be deleted %s", toBeDeleted) for _, add := range toBeCreated { // Add new Acl entry _, err := ec2conn.CreateNetworkAclEntry(d.Id(), &add) - fmt.Printf("$$$$#### %s", err) if err != nil { return fmt.Errorf("Error creating %s entry: %s", entryType, err) } @@ -242,28 +238,28 @@ func resourceAwsNetworkAclDelete(d *schema.ResourceData, meta interface{}) error return resource.Retry(5*time.Minute, func() error { if _, err := ec2conn.DeleteNetworkAcl(d.Id()); err != nil { ec2err := err.(*ec2.Error) - fmt.Printf("\n\n error code: %s \n", ec2err.Code) switch ec2err.Code { - case "InvalidNetworkAclID.NotFound": - return nil - 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), ec2conn) - if(err != nil){ - return fmt.Errorf("Depedency voilation: Could find association: %s", d.Id(), err) - } - defaultAcl, err := getDefaultNetworkAcl(d.Get("vpc_id").(string), ec2conn) - if(err != nil){ - return fmt.Errorf("Depedency voilation: Could not dissociate subnet from %s acl: %s", d.Id(), err) - } - _, err = ec2conn.ReplaceNetworkAclAssociation(association.NetworkAclAssociationId, defaultAcl.NetworkAclId) - return err - default: - // Any other error, we want to quit the retry loop immediately - return resource.RetryError{err} + case "InvalidNetworkAclID.NotFound": + return nil + 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), ec2conn) + if err != nil { + return fmt.Errorf("Depedency voilation: Can not delete acl: %s", d.Id(), err) + } + defaultAcl, err := getDefaultNetworkAcl(d.Get("vpc_id").(string), ec2conn) + if err != nil { + return fmt.Errorf("Depedency voilation: Can not delete acl %s", d.Id(), err) + } + _, err = ec2conn.ReplaceNetworkAclAssociation(association.NetworkAclAssociationId, defaultAcl.NetworkAclId) + return resource.RetryError{err} + default: + // Any other error, we want to quit the retry loop immediately + return resource.RetryError{err} } } + log.Printf("[Info] Deleted network ACL %s successfully", d.Id()) return nil }) } @@ -285,11 +281,10 @@ func resourceAwsNetworkAclEntryHash(v interface{}) int { return hashcode.String(buf.String()) } - -func getDefaultNetworkAcl(vpc_id string, ec2conn *ec2.EC2)(defaultAcl *ec2.NetworkAcl, err error){ +func getDefaultNetworkAcl(vpc_id string, ec2conn *ec2.EC2) (defaultAcl *ec2.NetworkAcl, err error) { filter := ec2.NewFilter() - filter.Add("default", "true" ) - filter.Add("vpc-id", vpc_id ) + filter.Add("default", "true") + filter.Add("vpc-id", vpc_id) resp, err := ec2conn.NetworkAcls([]string{}, filter) @@ -297,16 +292,21 @@ func getDefaultNetworkAcl(vpc_id string, ec2conn *ec2.EC2)(defaultAcl *ec2.Netwo return nil, err } return &resp.NetworkAcls[0], nil - } +} -func findNetworkAclAssociation(subnet_id string,ec2conn *ec2.EC2)(networkAclAssociation *ec2.NetworkAclAssociation, err error){ +func findNetworkAclAssociation(subnetId string, ec2conn *ec2.EC2) (networkAclAssociation *ec2.NetworkAclAssociation, err error) { filter := ec2.NewFilter() - filter.Add("association.subnet-id", subnet_id ) - + filter.Add("association.subnet-id", subnetId) + resp, err := ec2conn.NetworkAcls([]string{}, filter) if err != nil { return nil, err } - return &resp.NetworkAcls[0].AssociationSet[0], nil + for _, association := range resp.NetworkAcls[0].AssociationSet { + if association.SubnetId == subnetId { + return &association, nil + } + } + return nil, fmt.Errorf("could not find association for subnet %s ", subnetId) } diff --git a/builtin/providers/aws/resource_aws_network_acl_test.go b/builtin/providers/aws/resource_aws_network_acl_test.go index 8e578fa09..2186aa74e 100644 --- a/builtin/providers/aws/resource_aws_network_acl_test.go +++ b/builtin/providers/aws/resource_aws_network_acl_test.go @@ -55,7 +55,6 @@ func TestAccAWSNetworkAclsWithEgressAndIngressRules(t *testing.T) { func TestAccAWSNetworkAclsOnlyIngressRules(t *testing.T) { var networkAcl ec2.NetworkAcl - // var subnet ec2.Subnet resource.Test(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, @@ -66,7 +65,114 @@ func TestAccAWSNetworkAclsOnlyIngressRules(t *testing.T) { Config: testAccAWSNetworkAclIngressConfig, Check: resource.ComposeTestCheckFunc( testAccCheckAWSNetworkAclExists("aws_network_acl.foos", &networkAcl), - testAccCheckSubnetAssociation("aws_network_acl.foos", "aws_subnet.blob"), + // testAccCheckSubnetAssociation("aws_network_acl.foos", "aws_subnet.blob"), + resource.TestCheckResourceAttr( + "aws_network_acl.foos", "ingress.0.protocol", "tcp"), + resource.TestCheckResourceAttr( + "aws_network_acl.foos", "ingress.0.rule_no", "2"), + resource.TestCheckResourceAttr( + "aws_network_acl.foos", "ingress.0.from_port", "0"), + resource.TestCheckResourceAttr( + "aws_network_acl.foos", "ingress.0.to_port", "22"), + resource.TestCheckResourceAttr( + "aws_network_acl.foos", "ingress.0.action", "deny"), + resource.TestCheckResourceAttr( + "aws_network_acl.foos", "ingress.0.cidr_block", "10.2.2.3/18"), + ), + }, + }, + }) +} + +const testAccAWSNetworkAclIngressConfig = ` +resource "aws_vpc" "foo" { + cidr_block = "10.1.0.0/16" +} +resource "aws_subnet" "blob" { + cidr_block = "10.1.1.0/24" + vpc_id = "${aws_vpc.foo.id}" + map_public_ip_on_launch = true +} +resource "aws_network_acl" "foos" { + vpc_id = "${aws_vpc.foo.id}" + ingress = { + protocol = "tcp" + rule_no = 1 + action = "deny" + cidr_block = "10.2.2.3/18" + from_port = 0 + to_port = 22 + } + ingress = { + protocol = "tcp" + rule_no = 2 + action = "deny" + cidr_block = "10.2.2.3/18" + from_port = 443 + to_port = 443 + } + subnet_id = "${aws_subnet.blob.id}" +} +` +const testAccAWSNetworkAclIngressConfigChange = ` +resource "aws_vpc" "foo" { + cidr_block = "10.1.0.0/16" +} +resource "aws_subnet" "blob" { + cidr_block = "10.1.1.0/24" + vpc_id = "${aws_vpc.foo.id}" + map_public_ip_on_launch = true +} +resource "aws_network_acl" "foos" { + vpc_id = "${aws_vpc.foo.id}" + ingress = { + protocol = "tcp" + rule_no = 1 + action = "deny" + cidr_block = "10.2.2.3/18" + from_port = 0 + to_port = 22 + } + subnet_id = "${aws_subnet.blob.id}" +} +` + +func TestAccAWSNetworkAclsOnlyIngressRulesChange(t *testing.T) { + var networkAcl ec2.NetworkAcl + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckAWSNetworkAclDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: testAccAWSNetworkAclIngressConfig, + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSNetworkAclExists("aws_network_acl.foos", &networkAcl), + testIngressRuleLength(&networkAcl, 2), + resource.TestCheckResourceAttr( + "aws_network_acl.foos", "ingress.0.protocol", "tcp"), + resource.TestCheckResourceAttr( + "aws_network_acl.foos", "ingress.0.rule_no", "1"), + resource.TestCheckResourceAttr( + "aws_network_acl.foos", "ingress.0.from_port", "0"), + resource.TestCheckResourceAttr( + "aws_network_acl.foos", "ingress.0.to_port", "22"), + resource.TestCheckResourceAttr( + "aws_network_acl.foos", "ingress.0.action", "deny"), + resource.TestCheckResourceAttr( + "aws_network_acl.foos", "ingress.0.cidr_block", "10.2.2.3/18"), + resource.TestCheckResourceAttr( + "aws_network_acl.foos", "ingress.1.from_port", "443"), + resource.TestCheckResourceAttr( + "aws_network_acl.foos", "ingress.1.rule_no", "2"), + ), + }, + resource.TestStep{ + Config: testAccAWSNetworkAclIngressConfigChange, + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSNetworkAclExists("aws_network_acl.foos", &networkAcl), + testIngressRuleLength(&networkAcl, 1), resource.TestCheckResourceAttr( "aws_network_acl.foos", "ingress.0.protocol", "tcp"), resource.TestCheckResourceAttr( @@ -103,6 +209,33 @@ func TestAccAWSNetworkAclsOnlyEgressRules(t *testing.T) { }) } + + +func TestAccNetworkAcl_SubnetChange(t *testing.T) { + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckAWSNetworkAclDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: testAccAWSNetworkAclSubnetConfig, + Check: resource.ComposeTestCheckFunc( + testAccCheckSubnetIsAssociatedWithAcl("aws_network_acl.bar", "aws_subnet.old"), + ), + }, + resource.TestStep{ + Config: testAccAWSNetworkAclSubnetConfigChange, + Check: resource.ComposeTestCheckFunc( + testAccCheckSubnetIsNotAssociatedWithAcl("aws_network_acl.bar", "aws_subnet.old"), + testAccCheckSubnetIsAssociatedWithAcl("aws_network_acl.bar", "aws_subnet.new"), + ), + }, + }, + }) + +} + func testAccCheckAWSNetworkAclDestroy(s *terraform.State) error { conn := testAccProvider.Meta().(*AWSClient).ec2conn @@ -160,48 +293,67 @@ func testAccCheckAWSNetworkAclExists(n string, networkAcl *ec2.NetworkAcl) resou } } -func testAccCheckSubnetAssociation(acl string, subnet string) resource.TestCheckFunc { +func testIngressRuleLength(networkAcl *ec2.NetworkAcl, length int) resource.TestCheckFunc { + return func(s *terraform.State) error{ + var ingressEntries []ec2.NetworkAclEntry + for _, e := range networkAcl.EntrySet { + if e.Egress == false { + ingressEntries = append(ingressEntries, e) + } + } + if len(ingressEntries) != length { + return fmt.Errorf("Invalid number of ingress entries found; count = %s", len(ingressEntries)) + } + return nil + } +} + +func testAccCheckSubnetIsAssociatedWithAcl(acl string, sub string) resource.TestCheckFunc { return func(s *terraform.State) error { - networkAcl := s.RootModule().Resources[acl] + networkAcl := s.RootModule().Resources[acl] + subnet := s.RootModule().Resources[sub] + + conn := testAccProvider.Meta().(*AWSClient).ec2conn + filter := ec2.NewFilter() + filter.Add("association.subnet-id", subnet.Primary.ID) + resp, err := conn.NetworkAcls([]string{networkAcl.Primary.ID}, filter) + + if err != nil { + return err + } + if len(resp.NetworkAcls) > 0 { + return nil + } + + r, _ := conn.NetworkAcls([]string{}, ec2.NewFilter()) + fmt.Printf("\n\nall acls\n %s\n\n", r.NetworkAcls) + conn.NetworkAcls([]string{}, filter) + + return fmt.Errorf("Network Acl %s is not associated with subnet %s", acl, sub) + } +} + +func testAccCheckSubnetIsNotAssociatedWithAcl(acl string, subnet string) resource.TestCheckFunc { + return func(s *terraform.State) error { + networkAcl := s.RootModule().Resources[acl] subnet := s.RootModule().Resources[subnet] conn := testAccProvider.Meta().(*AWSClient).ec2conn filter := ec2.NewFilter() filter.Add("association.subnet-id", subnet.Primary.ID) - resp, err := conn.NetworkAcls([]string{networkAcl.Primary.ID}, filter) + resp, err := conn.NetworkAcls([]string{networkAcl.Primary.ID}, filter) if err != nil { return err } - if len(resp.NetworkAcls) > 0 && resp.NetworkAcls[0].NetworkAclId == networkAcl.Primary.ID { - return nil - } - return fmt.Errorf("Network Acl %s is not associated with subnet %s", acl, subnet) + if len(resp.NetworkAcls) > 0 { + return fmt.Errorf("Network Acl %s is still associated with subnet %s", acl, subnet) + } + return nil } } -const testAccAWSNetworkAclIngressConfig = ` -resource "aws_vpc" "foo" { - cidr_block = "10.1.0.0/16" -} -resource "aws_subnet" "blob" { - cidr_block = "10.1.1.0/24" - vpc_id = "${aws_vpc.foo.id}" - map_public_ip_on_launch = true -} -resource "aws_network_acl" "foos" { - vpc_id = "${aws_vpc.foo.id}" - ingress = { - protocol = "tcp" - rule_no = 2 - action = "deny" - cidr_block = "10.2.2.3/18" - from_port = 0 - to_port = 22 - } - subnet_id = "${aws_subnet.blob.id}" -} -` + const testAccAWSNetworkAclEgressConfig = ` resource "aws_vpc" "foo" { @@ -273,3 +425,46 @@ resource "aws_network_acl" "bar" { } } ` +const testAccAWSNetworkAclSubnetConfig = ` +resource "aws_vpc" "foo" { + cidr_block = "10.1.0.0/16" +} +resource "aws_subnet" "old" { + cidr_block = "10.1.111.0/24" + vpc_id = "${aws_vpc.foo.id}" + map_public_ip_on_launch = true +} +resource "aws_subnet" "new" { + cidr_block = "10.1.1.0/24" + vpc_id = "${aws_vpc.foo.id}" + map_public_ip_on_launch = true +} +resource "aws_network_acl" "roll" { + vpc_id = "${aws_vpc.foo.id}" + subnet_id = "${aws_subnet.new.id}" +} +resource "aws_network_acl" "bar" { + vpc_id = "${aws_vpc.foo.id}" + subnet_id = "${aws_subnet.old.id}" +} +` + +const testAccAWSNetworkAclSubnetConfigChange = ` +resource "aws_vpc" "foo" { + cidr_block = "10.1.0.0/16" +} +resource "aws_subnet" "old" { + cidr_block = "10.1.111.0/24" + vpc_id = "${aws_vpc.foo.id}" + map_public_ip_on_launch = true +} +resource "aws_subnet" "new" { + cidr_block = "10.1.1.0/24" + vpc_id = "${aws_vpc.foo.id}" + map_public_ip_on_launch = true +} +resource "aws_network_acl" "bar" { + vpc_id = "${aws_vpc.foo.id}" + subnet_id = "${aws_subnet.new.id}" +} +`