resource/aws_subnet: Apply attribute waiter logic to map_public_ip_on…

…_launch attribute and tidy up attribute testing Reference: #16696 This resource attribute has long been the cause of flakey acceptance testing across the codebase, such as: ``` === CONT TestAccAWSLB_applicationLoadBalancer_updateHttp2 TestAccAWSLB_applicationLoadBalancer_updateHttp2: resource_aws_lb_test.go:522: Step 1/3 error: After applying this test step, the plan was not empty. stdout: An execution plan has been generated and is shown below. Resource actions are indicated with the following symbols: ~ update in-place Terraform will perform the following actions: # aws_subnet.alb_test[0] will be updated in-place ~ resource "aws_subnet" "alb_test" { id = "subnet-088715d2b9827af18" ~ map_public_ip_on_launch = false -> true tags = { "Name" = "tf-acc-lb-basic-0" } # (7 unchanged attributes hidden) } Plan: 0 to add, 1 to change, 0 to destroy. ``` Adding logic, similar to `SubnetMapCustomerOwnedIpOnLaunchUpdated` in #16676 can be used to ensure the attribute value has flipped correctly after calling the `ModifySubnetAttribute` API. This attribute waiter setup will be added to a forthcoming Retries and Waiters section in the Contribution Guide. Output from acceptance testing in AWS Commercial: ``` --- PASS: TestAccAWSLB_applicationLoadBalancer_updateHttp2 (302.23s) --- PASS: TestAccAWSSubnet_availabilityZoneId (37.76s) --- PASS: TestAccAWSSubnet_basic (35.50s) --- PASS: TestAccAWSSubnet_disappears (27.55s) --- PASS: TestAccAWSSubnet_enableIpv6 (88.46s) --- PASS: TestAccAWSSubnet_ignoreTags (57.96s) --- PASS: TestAccAWSSubnet_ipv6 (99.54s) --- PASS: TestAccAWSSubnet_MapPublicIpOnLaunch (99.92s) --- PASS: TestAccAWSSubnet_tags (84.05s) --- SKIP: TestAccAWSSubnet_CustomerOwnedIpv4Pool (7.36s) --- SKIP: TestAccAWSSubnet_MapCustomerOwnedIpOnLaunch (1.82s) --- SKIP: TestAccAWSSubnet_outpost (1.62s) ``` Output from acceptance testing in AWS GovCloud (US): ``` --- PASS: TestAccAWSLB_applicationLoadBalancer_updateHttp2 (310.96s) --- PASS: TestAccAWSSubnet_availabilityZoneId (42.49s) --- PASS: TestAccAWSSubnet_basic (39.27s) --- PASS: TestAccAWSSubnet_disappears (31.78s) --- PASS: TestAccAWSSubnet_enableIpv6 (100.77s) --- PASS: TestAccAWSSubnet_ignoreTags (67.49s) --- PASS: TestAccAWSSubnet_ipv6 (108.38s) --- PASS: TestAccAWSSubnet_MapPublicIpOnLaunch (109.87s) --- PASS: TestAccAWSSubnet_tags (95.24s) --- SKIP: TestAccAWSSubnet_CustomerOwnedIpv4Pool (7.11s) --- SKIP: TestAccAWSSubnet_MapCustomerOwnedIpOnLaunch (2.17s) --- SKIP: TestAccAWSSubnet_outpost (10.06s) ```
hashicorp · Feb 2, 2021 · cce0c0b · cce0c0b
1 parent 075f130
commit cce0c0b
Show file tree

Hide file tree

Showing 4 changed files with 117 additions and 28 deletions.
diff --git a/aws/internal/service/ec2/waiter/status.go b/aws/internal/service/ec2/waiter/status.go
@@ -259,6 +259,27 @@ func SubnetMapCustomerOwnedIpOnLaunch(conn *ec2.EC2, id string) resource.StateRe
 	}
 }
 
+// SubnetMapPublicIpOnLaunch fetches the Subnet and its MapPublicIpOnLaunch
+func SubnetMapPublicIpOnLaunch(conn *ec2.EC2, id string) resource.StateRefreshFunc {
+	return func() (interface{}, string, error) {
+		subnet, err := finder.SubnetByID(conn, id)
+
+		if tfawserr.ErrCodeEquals(err, tfec2.ErrCodeInvalidSubnetIDNotFound) {
+			return nil, "false", nil
+		}
+
+		if err != nil {
+			return nil, "false", err
+		}
+
+		if subnet == nil {
+			return nil, "false", nil
+		}
+
+		return subnet, strconv.FormatBool(aws.BoolValue(subnet.MapPublicIpOnLaunch)), nil
+	}
+}
+
 const (
 	vpcPeeringConnectionStatusNotFound = "NotFound"
 	vpcPeeringConnectionStatusUnknown  = "Unknown"

diff --git a/aws/internal/service/ec2/waiter/waiter.go b/aws/internal/service/ec2/waiter/waiter.go
@@ -272,6 +272,24 @@ func SubnetMapCustomerOwnedIpOnLaunchUpdated(conn *ec2.EC2, subnetID string, exp
 	return nil, err
 }
 
+func SubnetMapPublicIpOnLaunchUpdated(conn *ec2.EC2, subnetID string, expectedValue bool) (*ec2.Subnet, error) {
+	stateConf := &resource.StateChangeConf{
+		Target:     []string{strconv.FormatBool(expectedValue)},
+		Refresh:    SubnetMapPublicIpOnLaunch(conn, subnetID),
+		Timeout:    SubnetAttributePropagationTimeout,
+		Delay:      10 * time.Second,
+		MinTimeout: 3 * time.Second,
+	}
+
+	outputRaw, err := stateConf.WaitForState()
+
+	if output, ok := outputRaw.(*ec2.Subnet); ok {
+		return output, err
+	}
+
+	return nil, err
+}
+
 const (
 	VpnGatewayVpcAttachmentAttachedTimeout = 15 * time.Minute
 

diff --git a/aws/resource_aws_subnet.go b/aws/resource_aws_subnet.go
@@ -212,6 +212,10 @@ func resourceAwsSubnetCreate(d *schema.ResourceData, meta interface{}) error {
 		if _, err := conn.ModifySubnetAttribute(input); err != nil {
 			return fmt.Errorf("error enabling EC2 Subnet (%s) map public IP on launch: %w", d.Id(), err)
 		}
+
+		if _, err := waiter.SubnetMapPublicIpOnLaunchUpdated(conn, d.Id(), d.Get("map_public_ip_on_launch").(bool)); err != nil {
+			return fmt.Errorf("error waiting for EC2 Subnet (%s) map public IP on launch update: %w", d.Id(), err)
+		}
 	}
 
 	return resourceAwsSubnetRead(d, meta)
@@ -316,12 +320,14 @@ func resourceAwsSubnetUpdate(d *schema.ResourceData, meta interface{}) error {
 			},
 		}
 
-		log.Printf("[DEBUG] Subnet modify attributes: %#v", modifyOpts)
-
 		_, err := conn.ModifySubnetAttribute(modifyOpts)
 
 		if err != nil {
-			return err
+			return fmt.Errorf("error updating EC2 Subnet (%s) map public IP on launch: %w", d.Id(), err)
+		}
+
+		if _, err := waiter.SubnetMapPublicIpOnLaunchUpdated(conn, d.Id(), d.Get("map_public_ip_on_launch").(bool)); err != nil {
+			return fmt.Errorf("error waiting for EC2 Subnet (%s) map public IP on launch update: %w", d.Id(), err)
 		}
 	}
 

diff --git a/aws/resource_aws_subnet_test.go b/aws/resource_aws_subnet_test.go
@@ -127,18 +127,6 @@ func TestAccAWSSubnet_basic(t *testing.T) {
 	var v ec2.Subnet
 	resourceName := "aws_subnet.test"
 
-	testCheck := func(*terraform.State) error {
-		if aws.StringValue(v.CidrBlock) != "10.1.1.0/24" {
-			return fmt.Errorf("bad cidr: %s", aws.StringValue(v.CidrBlock))
-		}
-
-		if !aws.BoolValue(v.MapPublicIpOnLaunch) {
-			return fmt.Errorf("bad MapPublicIpOnLaunch: %t", aws.BoolValue(v.MapPublicIpOnLaunch))
-		}
-
-		return nil
-	}
-
 	resource.ParallelTest(t, resource.TestCase{
 		PreCheck:      func() { testAccPreCheck(t) },
 		IDRefreshName: resourceName,
@@ -149,16 +137,16 @@ func TestAccAWSSubnet_basic(t *testing.T) {
 				Config: testAccSubnetConfig,
 				Check: resource.ComposeTestCheckFunc(
 					testAccCheckSubnetExists(resourceName, &v),
-					testCheck,
-					// ipv6 should be empty if disabled so we can still use the property in conditionals
-					resource.TestCheckResourceAttr(resourceName, "ipv6_cidr_block", ""),
 					testAccMatchResourceAttrRegionalARN(resourceName, "arn", "ec2", regexp.MustCompile(`subnet/subnet-.+`)),
-					testAccCheckResourceAttrAccountID(resourceName, "owner_id"),
 					resource.TestCheckResourceAttrSet(resourceName, "availability_zone"),
 					resource.TestCheckResourceAttrSet(resourceName, "availability_zone_id"),
+					resource.TestCheckResourceAttr(resourceName, "cidr_block", "10.1.1.0/24"),
 					resource.TestCheckResourceAttr(resourceName, "customer_owned_ipv4_pool", ""),
+					resource.TestCheckResourceAttr(resourceName, "ipv6_cidr_block", ""),
 					resource.TestCheckResourceAttr(resourceName, "map_customer_owned_ip_on_launch", "false"),
+					resource.TestCheckResourceAttr(resourceName, "map_public_ip_on_launch", "false"),
 					resource.TestCheckResourceAttr(resourceName, "outpost_arn", ""),
+					testAccCheckResourceAttrAccountID(resourceName, "owner_id"),
 					resource.TestCheckResourceAttr(resourceName, "tags.%", "0"),
 				),
 			},
@@ -427,6 +415,45 @@ func TestAccAWSSubnet_MapCustomerOwnedIpOnLaunch(t *testing.T) {
 	})
 }
 
+func TestAccAWSSubnet_MapPublicIpOnLaunch(t *testing.T) {
+	var subnet ec2.Subnet
+	resourceName := "aws_subnet.test"
+
+	resource.ParallelTest(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckSubnetDestroy,
+		Steps: []resource.TestStep{
+			{
+				Config: testAccSubnetConfigMapPublicIpOnLaunch(true),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckSubnetExists(resourceName, &subnet),
+					resource.TestCheckResourceAttr(resourceName, "map_public_ip_on_launch", "true"),
+				),
+			},
+			{
+				ResourceName:      resourceName,
+				ImportState:       true,
+				ImportStateVerify: true,
+			},
+			{
+				Config: testAccSubnetConfigMapPublicIpOnLaunch(false),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckSubnetExists(resourceName, &subnet),
+					resource.TestCheckResourceAttr(resourceName, "map_public_ip_on_launch", "false"),
+				),
+			},
+			{
+				Config: testAccSubnetConfigMapPublicIpOnLaunch(true),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckSubnetExists(resourceName, &subnet),
+					resource.TestCheckResourceAttr(resourceName, "map_public_ip_on_launch", "true"),
+				),
+			},
+		},
+	})
+}
+
 func TestAccAWSSubnet_outpost(t *testing.T) {
 	var v ec2.Subnet
 	outpostDataSourceName := "data.aws_outposts_outpost.test"
@@ -567,9 +594,8 @@ resource "aws_vpc" "test" {
 }
 
 resource "aws_subnet" "test" {
-  cidr_block              = "10.1.1.0/24"
-  vpc_id                  = aws_vpc.test.id
-  map_public_ip_on_launch = true
+  cidr_block = "10.1.1.0/24"
+  vpc_id     = aws_vpc.test.id
 }
 `
 
@@ -627,9 +653,8 @@ resource "aws_vpc" "test" {
 }
 
 resource "aws_subnet" "test" {
-  cidr_block              = "10.10.1.0/24"
-  vpc_id                  = aws_vpc.test.id
-  map_public_ip_on_launch = true
+  cidr_block = "10.10.1.0/24"
+  vpc_id     = aws_vpc.test.id
 
   tags = {
     Name = "tf-acc-subnet-ipv6"
@@ -651,7 +676,6 @@ resource "aws_subnet" "test" {
   cidr_block                      = "10.10.1.0/24"
   vpc_id                          = aws_vpc.test.id
   ipv6_cidr_block                 = cidrsubnet(aws_vpc.test.ipv6_cidr_block, 8, 1)
-  map_public_ip_on_launch         = true
   assign_ipv6_address_on_creation = true
 
   tags = {
@@ -674,7 +698,6 @@ resource "aws_subnet" "test" {
   cidr_block                      = "10.10.1.0/24"
   vpc_id                          = aws_vpc.test.id
   ipv6_cidr_block                 = cidrsubnet(aws_vpc.test.ipv6_cidr_block, 8, 1)
-  map_public_ip_on_launch         = true
   assign_ipv6_address_on_creation = false
 
   tags = {
@@ -697,7 +720,6 @@ resource "aws_subnet" "test" {
   cidr_block                      = "10.10.1.0/24"
   vpc_id                          = aws_vpc.test.id
   ipv6_cidr_block                 = cidrsubnet(aws_vpc.test.ipv6_cidr_block, 8, 3)
-  map_public_ip_on_launch         = true
   assign_ipv6_address_on_creation = false
 
   tags = {
@@ -834,6 +856,28 @@ resource "aws_subnet" "test" {
 `, mapCustomerOwnedIpOnLaunch)
 }
 
+func testAccSubnetConfigMapPublicIpOnLaunch(mapPublicIpOnLaunch bool) string {
+	return fmt.Sprintf(`
+resource "aws_vpc" "test" {
+  cidr_block = "10.0.0.0/16"
+
+  tags = {
+    Name = "tf-acc-test-subnet-map-public-ip-on-launch"
+  }
+}
+
+resource "aws_subnet" "test" {
+  cidr_block              = cidrsubnet(aws_vpc.test.cidr_block, 8, 0)
+  map_public_ip_on_launch = %[1]t
+  vpc_id                  = aws_vpc.test.id
+
+  tags = {
+    Name = "tf-acc-test-subnet-map-public-ip-on-launch"
+  }
+}
+`, mapPublicIpOnLaunch)
+}
+
 func testAccSubnetConfigOutpost() string {
 	return `
 data "aws_outposts_outposts" "test" {}