From cce0c0b94970a9e20d0500da4d9b26c06ef66502 Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Tue, 2 Feb 2021 11:18:45 -0500 Subject: [PATCH] resource/aws_subnet: Apply attribute waiter logic to map_public_ip_on_launch attribute and tidy up attribute testing Reference: https://github.com/hashicorp/terraform-provider-aws/issues/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 https://github.com/hashicorp/terraform-provider-aws/pull/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) ``` --- aws/internal/service/ec2/waiter/status.go | 21 +++++ aws/internal/service/ec2/waiter/waiter.go | 18 +++++ aws/resource_aws_subnet.go | 12 ++- aws/resource_aws_subnet_test.go | 94 +++++++++++++++++------ 4 files changed, 117 insertions(+), 28 deletions(-) diff --git a/aws/internal/service/ec2/waiter/status.go b/aws/internal/service/ec2/waiter/status.go index 47a78775504..74e0c9308f5 100644 --- 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 index c8c516294ef..0b76dd4f980 100644 --- 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 index d05943849cf..d93e6ee1257 100644 --- 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 index 7da2ee09b6a..fed7b085741 100644 --- 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" {}