Fixed update of ingress/egress rules

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")) {
+	if d.HasChange("subnet_id") {
-		association, err := findNetworkAclAssociation(d.Get("subnet_id").(string), ec2conn)
+
-		if(err != nil){
+		//associate new subnet with the acl.
-			return fmt.Errorf("Depedency voilation: Could find association: %s", d.Id(), err)
+		_, 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,7 +238,6 @@ 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
@@ -250,20 +245,21 @@ func resourceAwsNetworkAclDelete(d *schema.ResourceData, meta interface{}) error
 				// 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){
+				if err != nil {
-						return fmt.Errorf("Depedency voilation: Could find association: %s", d.Id(), err)
+					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){
+				if err != nil {
-						return fmt.Errorf("Depedency voilation: Could not dissociate subnet from %s acl: %s", d.Id(), err)
+					return fmt.Errorf("Depedency voilation: Can not delete acl %s", d.Id(), err)
 				}
 				_, err = ec2conn.ReplaceNetworkAclAssociation(association.NetworkAclAssociationId, defaultAcl.NetworkAclId)
-					return err
+				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("default", "true")
-	filter.Add("vpc-id", vpc_id )
+	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)
 }

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,7 +293,47 @@ 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]
+		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]
@@ -173,35 +346,14 @@ func testAccCheckSubnetAssociation(acl string, subnet string) resource.TestCheck
 		if err != nil {
 			return err
 		}
-		if len(resp.NetworkAcls) > 0 && resp.NetworkAcls[0].NetworkAclId == networkAcl.Primary.ID {
+		if len(resp.NetworkAcls) > 0 {
-			return nil
+			return fmt.Errorf("Network Acl %s is still associated with subnet %s", acl, subnet)
 		}
-		return fmt.Errorf("Network Acl %s is not 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}"
+}
+`