From 067b8feea0a9a84e2b9e394ef40da5a76b3250c9 Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Thu, 1 Apr 2021 12:35:01 -0400 Subject: [PATCH] resource/aws_network_interface_sg_attachment: Handle read-after-create eventual consistency (#18466) * resource/aws_network_interface_sg_attachment: Handle read-after-create eventual consistency Reference: https://github.com/hashicorp/terraform-provider-aws/issues/10363 Reference: https://github.com/hashicorp/terraform-provider-aws/issues/16201 Reference: https://github.com/hashicorp/terraform-provider-aws/issues/16796 Output from acceptance testing in AWS Commercial: ``` --- PASS: TestAccAWSNetworkInterfaceSGAttachment_disappears (59.24s) --- PASS: TestAccAWSNetworkInterfaceSGAttachment_basic (59.57s) --- PASS: TestAccAWSNetworkInterfaceSGAttachment_Multiple (63.42s) --- PASS: TestAccAWSNetworkInterfaceSGAttachment_Instance (93.21s) --- PASS: TestAccAWSNetworkInterfaceSGAttachment_DataSource (105.56s) ``` Output from acceptance testing in AWS GovCloud (US): ``` --- PASS: TestAccAWSNetworkInterfaceSGAttachment_disappears (61.51s) --- PASS: TestAccAWSNetworkInterfaceSGAttachment_basic (62.15s) --- PASS: TestAccAWSNetworkInterfaceSGAttachment_Multiple (65.05s) --- PASS: TestAccAWSNetworkInterfaceSGAttachment_DataSource (93.55s) --- PASS: TestAccAWSNetworkInterfaceSGAttachment_Instance (114.74s) ``` * Update CHANGELOG for #18466 --- .changelog/18466.txt | 3 + aws/internal/service/ec2/errors.go | 4 + aws/internal/service/ec2/finder/finder.go | 55 +++++++++ ...rce_aws_network_interface_sg_attachment.go | 116 ++++++++++-------- ...ws_network_interface_sg_attachment_test.go | 65 +++------- 5 files changed, 148 insertions(+), 95 deletions(-) create mode 100644 .changelog/18466.txt diff --git a/.changelog/18466.txt b/.changelog/18466.txt new file mode 100644 index 00000000000..595fc08ff4a --- /dev/null +++ b/.changelog/18466.txt @@ -0,0 +1,3 @@ +```release-note:bug +resource/aws_network_interface_sg_attachment: Handle read-after-create eventual consistency +``` diff --git a/aws/internal/service/ec2/errors.go b/aws/internal/service/ec2/errors.go index 4414e732b06..9be28b76eb1 100644 --- a/aws/internal/service/ec2/errors.go +++ b/aws/internal/service/ec2/errors.go @@ -17,6 +17,10 @@ const ( ErrCodeInvalidCarrierGatewayIDNotFound = "InvalidCarrierGatewayID.NotFound" ) +const ( + ErrCodeInvalidNetworkInterfaceIDNotFound = "InvalidNetworkInterfaceID.NotFound" +) + const ( ErrCodeInvalidPrefixListIDNotFound = "InvalidPrefixListID.NotFound" ) diff --git a/aws/internal/service/ec2/finder/finder.go b/aws/internal/service/ec2/finder/finder.go index 8d960821c3b..0c79c5d0d3c 100644 --- a/aws/internal/service/ec2/finder/finder.go +++ b/aws/internal/service/ec2/finder/finder.go @@ -180,6 +180,61 @@ func NetworkAclEntry(conn *ec2.EC2, networkAclID string, egress bool, ruleNumber return nil, nil } +// NetworkInterfaceByID looks up a NetworkInterface by ID. When not found, returns nil and potentially an API error. +func NetworkInterfaceByID(conn *ec2.EC2, id string) (*ec2.NetworkInterface, error) { + input := &ec2.DescribeNetworkInterfacesInput{ + NetworkInterfaceIds: aws.StringSlice([]string{id}), + } + + output, err := conn.DescribeNetworkInterfaces(input) + + if err != nil { + return nil, err + } + + if output == nil { + return nil, nil + } + + for _, networkInterface := range output.NetworkInterfaces { + if networkInterface == nil { + continue + } + + if aws.StringValue(networkInterface.NetworkInterfaceId) != id { + continue + } + + return networkInterface, nil + } + + return nil, nil +} + +// NetworkInterfaceSecurityGroup returns the associated GroupIdentifier if found +func NetworkInterfaceSecurityGroup(conn *ec2.EC2, networkInterfaceID string, securityGroupID string) (*ec2.GroupIdentifier, error) { + var result *ec2.GroupIdentifier + + networkInterface, err := NetworkInterfaceByID(conn, networkInterfaceID) + + if err != nil { + return nil, err + } + + if networkInterface == nil { + return nil, nil + } + + for _, groupIdentifier := range networkInterface.Groups { + if aws.StringValue(groupIdentifier.GroupId) == securityGroupID { + result = groupIdentifier + break + } + } + + return result, err +} + // RouteTableByID returns the route table corresponding to the specified identifier. // Returns NotFoundError if no route table is found. func RouteTableByID(conn *ec2.EC2, routeTableID string) (*ec2.RouteTable, error) { diff --git a/aws/resource_aws_network_interface_sg_attachment.go b/aws/resource_aws_network_interface_sg_attachment.go index 54635a72769..1684f41639e 100644 --- a/aws/resource_aws_network_interface_sg_attachment.go +++ b/aws/resource_aws_network_interface_sg_attachment.go @@ -7,7 +7,13 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" + "github.com/hashicorp/aws-sdk-go-base/tfawserr" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" + tfec2 "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/ec2" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/ec2/finder" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/ec2/waiter" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/tfresource" ) func resourceAwsNetworkInterfaceSGAttachment() *schema.Resource { @@ -40,34 +46,38 @@ func resourceAwsNetworkInterfaceSGAttachmentCreate(d *schema.ResourceData, meta conn := meta.(*AWSClient).ec2conn - // Fetch the network interface we will be working with. - iface, err := fetchNetworkInterface(conn, interfaceID) + iface, err := finder.NetworkInterfaceByID(conn, interfaceID) + if err != nil { - return err + return fmt.Errorf("error reading EC2 Network Interface (%s): %w", interfaceID, err) } - // Add the security group to the network interface. - log.Printf("[DEBUG] Attaching security group %s to network interface ID %s", sgID, interfaceID) + groupIDs := []string{sgID} - if sgExistsInENI(sgID, iface) { - return fmt.Errorf("security group %s already attached to interface ID %s", sgID, *iface.NetworkInterfaceId) - } - var groupIDs []string - for _, v := range iface.Groups { - groupIDs = append(groupIDs, *v.GroupId) + for _, group := range iface.Groups { + if group == nil { + continue + } + + if aws.StringValue(group.GroupId) == sgID { + return fmt.Errorf("EC2 Security Group (%s) already attached to EC2 Network Interface ID: %s", sgID, interfaceID) + } + + groupIDs = append(groupIDs, aws.StringValue(group.GroupId)) } - groupIDs = append(groupIDs, sgID) + params := &ec2.ModifyNetworkInterfaceAttributeInput{ NetworkInterfaceId: iface.NetworkInterfaceId, Groups: aws.StringSlice(groupIDs), } _, err = conn.ModifyNetworkInterfaceAttribute(params) + if err != nil { - return err + return fmt.Errorf("error modifying EC2 Network Interface (%s): %w", interfaceID, err) } - log.Printf("[DEBUG] Successful attachment of security group %s to network interface ID %s", sgID, interfaceID) + d.SetId(fmt.Sprintf("%s_%s", sgID, interfaceID)) return resourceAwsNetworkInterfaceSGAttachmentRead(d, meta) } @@ -80,25 +90,57 @@ func resourceAwsNetworkInterfaceSGAttachmentRead(d *schema.ResourceData, meta in conn := meta.(*AWSClient).ec2conn - iface, err := fetchNetworkInterface(conn, interfaceID) + var groupIdentifier *ec2.GroupIdentifier - if isAWSErr(err, "InvalidNetworkInterfaceID.NotFound", "") { - log.Printf("[WARN] EC2 Network Interface (%s) not found, removing from state", interfaceID) + err := resource.Retry(waiter.PropagationTimeout, func() *resource.RetryError { + var err error + + groupIdentifier, err = finder.NetworkInterfaceSecurityGroup(conn, interfaceID, sgID) + + if d.IsNewResource() && tfawserr.ErrCodeEquals(err, tfec2.ErrCodeInvalidNetworkInterfaceIDNotFound) { + return resource.RetryableError(err) + } + + if err != nil { + return resource.NonRetryableError(err) + } + + if d.IsNewResource() && groupIdentifier == nil { + return resource.RetryableError(&resource.NotFoundError{ + LastError: fmt.Errorf("EC2 Network Interface Security Group Attachment (%s) not found", d.Id()), + }) + } + + return nil + }) + + if tfresource.TimedOut(err) { + groupIdentifier, err = finder.NetworkInterfaceSecurityGroup(conn, interfaceID, sgID) + } + + if !d.IsNewResource() && tfawserr.ErrCodeEquals(err, tfec2.ErrCodeInvalidNetworkInterfaceIDNotFound) { + log.Printf("[WARN] EC2 Network Interface Security Group Attachment (%s) not found, removing from state", d.Id()) d.SetId("") return nil } if err != nil { - return err + return fmt.Errorf("error reading EC2 Network Interface Security Group Attachment (%s): %w", d.Id(), err) } - if sgExistsInENI(sgID, iface) { - d.SetId(fmt.Sprintf("%s_%s", sgID, interfaceID)) - } else { - // The association does not exist when it should, taint this resource. - log.Printf("[WARN] Security group %s not associated with network interface ID %s, tainting", sgID, interfaceID) + if groupIdentifier == nil { + if d.IsNewResource() { + return fmt.Errorf("error reading EC2 Network Interface Security Group Attachment (%s): not found after creation", d.Id()) + } + + log.Printf("[WARN] EC2 Network Interface Security Group Attachment (%s) not found, removing from state", d.Id()) d.SetId("") + return nil } + + d.Set("network_interface_id", interfaceID) + d.Set("security_group_id", groupIdentifier.GroupId) + return nil } @@ -114,7 +156,7 @@ func resourceAwsNetworkInterfaceSGAttachmentDelete(d *schema.ResourceData, meta conn := meta.(*AWSClient).ec2conn - iface, err := fetchNetworkInterface(conn, interfaceID) + iface, err := finder.NetworkInterfaceByID(conn, interfaceID) if isAWSErr(err, "InvalidNetworkInterfaceID.NotFound", "") { return nil @@ -127,21 +169,6 @@ func resourceAwsNetworkInterfaceSGAttachmentDelete(d *schema.ResourceData, meta return delSGFromENI(conn, sgID, iface) } -// fetchNetworkInterface is a utility function used by Read and Delete to fetch -// the full ENI details for a specific interface ID. -func fetchNetworkInterface(conn *ec2.EC2, ifaceID string) (*ec2.NetworkInterface, error) { - log.Printf("[DEBUG] Fetching information for interface ID %s", ifaceID) - dniParams := &ec2.DescribeNetworkInterfacesInput{ - NetworkInterfaceIds: aws.StringSlice([]string{ifaceID}), - } - - dniResp, err := conn.DescribeNetworkInterfaces(dniParams) - if err != nil { - return nil, err - } - return dniResp.NetworkInterfaces[0], nil -} - func delSGFromENI(conn *ec2.EC2, sgID string, iface *ec2.NetworkInterface) error { old := iface.Groups var new []*string @@ -169,14 +196,3 @@ func delSGFromENI(conn *ec2.EC2, sgID string, iface *ec2.NetworkInterface) error return err } - -// sgExistsInENI is a utility function that can be used to quickly check to -// see if a security group exists in an *ec2.NetworkInterface. -func sgExistsInENI(sgID string, iface *ec2.NetworkInterface) bool { - for _, v := range iface.Groups { - if *v.GroupId == sgID { - return true - } - } - return false -} diff --git a/aws/resource_aws_network_interface_sg_attachment_test.go b/aws/resource_aws_network_interface_sg_attachment_test.go index 2d03de45136..5437b5c18be 100644 --- a/aws/resource_aws_network_interface_sg_attachment_test.go +++ b/aws/resource_aws_network_interface_sg_attachment_test.go @@ -5,13 +5,15 @@ import ( "testing" "github.com/aws/aws-sdk-go/service/ec2" + "github.com/hashicorp/aws-sdk-go-base/tfawserr" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/acctest" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" + tfec2 "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/ec2" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/ec2/finder" ) func TestAccAWSNetworkInterfaceSGAttachment_basic(t *testing.T) { - var networkInterface ec2.NetworkInterface networkInterfaceResourceName := "aws_network_interface.test" securityGroupResourceName := "aws_security_group.test" resourceName := "aws_network_interface_sg_attachment.test" @@ -26,7 +28,7 @@ func TestAccAWSNetworkInterfaceSGAttachment_basic(t *testing.T) { { Config: testAccAwsNetworkInterfaceSGAttachmentConfig(rName), Check: resource.ComposeTestCheckFunc( - testAccCheckAWSNetworkInterfaceSGAttachmentExists(resourceName, &networkInterface), + testAccCheckAWSNetworkInterfaceSGAttachmentExists(resourceName), resource.TestCheckResourceAttrPair(resourceName, "network_interface_id", networkInterfaceResourceName, "id"), resource.TestCheckResourceAttrPair(resourceName, "security_group_id", securityGroupResourceName, "id"), ), @@ -36,7 +38,6 @@ func TestAccAWSNetworkInterfaceSGAttachment_basic(t *testing.T) { } func TestAccAWSNetworkInterfaceSGAttachment_disappears(t *testing.T) { - var networkInterface ec2.NetworkInterface resourceName := "aws_network_interface_sg_attachment.test" rName := acctest.RandomWithPrefix("tf-acc-test") @@ -49,15 +50,7 @@ func TestAccAWSNetworkInterfaceSGAttachment_disappears(t *testing.T) { { Config: testAccAwsNetworkInterfaceSGAttachmentConfig(rName), Check: resource.ComposeTestCheckFunc( - testAccCheckAWSNetworkInterfaceSGAttachmentExists(resourceName, &networkInterface), - testAccCheckAwsNetworkInterfaceSGAttachmentDisappears(resourceName, &networkInterface), - ), - ExpectNonEmptyPlan: true, - }, - { - Config: testAccAwsNetworkInterfaceSGAttachmentConfig(rName), - Check: resource.ComposeTestCheckFunc( - testAccCheckAWSNetworkInterfaceSGAttachmentExists(resourceName, &networkInterface), + testAccCheckAWSNetworkInterfaceSGAttachmentExists(resourceName), testAccCheckResourceDisappears(testAccProvider, resourceAwsNetworkInterfaceSGAttachment(), resourceName), ), ExpectNonEmptyPlan: true, @@ -67,7 +60,6 @@ func TestAccAWSNetworkInterfaceSGAttachment_disappears(t *testing.T) { } func TestAccAWSNetworkInterfaceSGAttachment_Instance(t *testing.T) { - var networkInterface ec2.NetworkInterface instanceResourceName := "aws_instance.test" securityGroupResourceName := "aws_security_group.test" resourceName := "aws_network_interface_sg_attachment.test" @@ -82,7 +74,7 @@ func TestAccAWSNetworkInterfaceSGAttachment_Instance(t *testing.T) { { Config: testAccAwsNetworkInterfaceSGAttachmentConfigViaInstance(rName), Check: resource.ComposeTestCheckFunc( - testAccCheckAWSNetworkInterfaceSGAttachmentExists(resourceName, &networkInterface), + testAccCheckAWSNetworkInterfaceSGAttachmentExists(resourceName), resource.TestCheckResourceAttrPair(resourceName, "network_interface_id", instanceResourceName, "primary_network_interface_id"), resource.TestCheckResourceAttrPair(resourceName, "security_group_id", securityGroupResourceName, "id"), ), @@ -92,7 +84,6 @@ func TestAccAWSNetworkInterfaceSGAttachment_Instance(t *testing.T) { } func TestAccAWSNetworkInterfaceSGAttachment_DataSource(t *testing.T) { - var networkInterface ec2.NetworkInterface instanceDataSourceName := "data.aws_instance.test" securityGroupResourceName := "aws_security_group.test" resourceName := "aws_network_interface_sg_attachment.test" @@ -107,7 +98,7 @@ func TestAccAWSNetworkInterfaceSGAttachment_DataSource(t *testing.T) { { Config: testAccAwsNetworkInterfaceSGAttachmentConfigViaDataSource(rName), Check: resource.ComposeTestCheckFunc( - testAccCheckAWSNetworkInterfaceSGAttachmentExists(resourceName, &networkInterface), + testAccCheckAWSNetworkInterfaceSGAttachmentExists(resourceName), resource.TestCheckResourceAttrPair(resourceName, "network_interface_id", instanceDataSourceName, "network_interface_id"), resource.TestCheckResourceAttrPair(resourceName, "security_group_id", securityGroupResourceName, "id"), ), @@ -117,7 +108,6 @@ func TestAccAWSNetworkInterfaceSGAttachment_DataSource(t *testing.T) { } func TestAccAWSNetworkInterfaceSGAttachment_Multiple(t *testing.T) { - var networkInterface ec2.NetworkInterface networkInterfaceResourceName := "aws_network_interface.test" securityGroupResourceName1 := "aws_security_group.test.0" securityGroupResourceName2 := "aws_security_group.test.1" @@ -138,16 +128,16 @@ func TestAccAWSNetworkInterfaceSGAttachment_Multiple(t *testing.T) { { Config: testAccAwsNetworkInterfaceSGAttachmentConfigMultiple(rName), Check: resource.ComposeTestCheckFunc( - testAccCheckAWSNetworkInterfaceSGAttachmentExists(resourceName1, &networkInterface), + testAccCheckAWSNetworkInterfaceSGAttachmentExists(resourceName1), resource.TestCheckResourceAttrPair(resourceName1, "network_interface_id", networkInterfaceResourceName, "id"), resource.TestCheckResourceAttrPair(resourceName1, "security_group_id", securityGroupResourceName1, "id"), - testAccCheckAWSNetworkInterfaceSGAttachmentExists(resourceName2, &networkInterface), + testAccCheckAWSNetworkInterfaceSGAttachmentExists(resourceName2), resource.TestCheckResourceAttrPair(resourceName2, "network_interface_id", networkInterfaceResourceName, "id"), resource.TestCheckResourceAttrPair(resourceName2, "security_group_id", securityGroupResourceName2, "id"), - testAccCheckAWSNetworkInterfaceSGAttachmentExists(resourceName3, &networkInterface), + testAccCheckAWSNetworkInterfaceSGAttachmentExists(resourceName3), resource.TestCheckResourceAttrPair(resourceName3, "network_interface_id", networkInterfaceResourceName, "id"), resource.TestCheckResourceAttrPair(resourceName3, "security_group_id", securityGroupResourceName3, "id"), - testAccCheckAWSNetworkInterfaceSGAttachmentExists(resourceName4, &networkInterface), + testAccCheckAWSNetworkInterfaceSGAttachmentExists(resourceName4), resource.TestCheckResourceAttrPair(resourceName4, "network_interface_id", networkInterfaceResourceName, "id"), resource.TestCheckResourceAttrPair(resourceName4, "security_group_id", securityGroupResourceName4, "id"), ), @@ -156,7 +146,7 @@ func TestAccAWSNetworkInterfaceSGAttachment_Multiple(t *testing.T) { }) } -func testAccCheckAWSNetworkInterfaceSGAttachmentExists(resourceName string, networkInterface *ec2.NetworkInterface) resource.TestCheckFunc { +func testAccCheckAWSNetworkInterfaceSGAttachmentExists(resourceName string) resource.TestCheckFunc { return func(s *terraform.State) error { rs, ok := s.RootModule().Resources[resourceName] if !ok { @@ -171,17 +161,16 @@ func testAccCheckAWSNetworkInterfaceSGAttachmentExists(resourceName string, netw networkInterfaceID := rs.Primary.Attributes["network_interface_id"] securityGroupID := rs.Primary.Attributes["security_group_id"] - iface, err := fetchNetworkInterface(conn, networkInterfaceID) + groupIdentifier, err := finder.NetworkInterfaceSecurityGroup(conn, networkInterfaceID, securityGroupID) + if err != nil { - return fmt.Errorf("ENI (%s) not found: %s", networkInterfaceID, err) + return fmt.Errorf("error reading EC2 Network Interface Security Group Attachment (%s): %s", rs.Primary.ID, err) } - if !sgExistsInENI(securityGroupID, iface) { + if groupIdentifier == nil { return fmt.Errorf("Security Group ID (%s) not attached to ENI (%s)", securityGroupID, networkInterfaceID) } - *networkInterface = *iface - return nil } } @@ -197,17 +186,17 @@ func testAccCheckAWSNetworkInterfaceSGAttachmentDestroy(s *terraform.State) erro networkInterfaceID := rs.Primary.Attributes["network_interface_id"] securityGroupID := rs.Primary.Attributes["security_group_id"] - networkInterface, err := fetchNetworkInterface(conn, networkInterfaceID) + groupIdentifier, err := finder.NetworkInterfaceSecurityGroup(conn, networkInterfaceID, securityGroupID) - if isAWSErr(err, "InvalidNetworkInterfaceID.NotFound", "") { + if tfawserr.ErrCodeEquals(err, tfec2.ErrCodeInvalidNetworkInterfaceIDNotFound) { continue } if err != nil { - return fmt.Errorf("ENI (%s) not found: %s", networkInterfaceID, err) + return fmt.Errorf("error reading EC2 Network Interface Security Group Attachment (%s): %s", rs.Primary.ID, err) } - if sgExistsInENI(securityGroupID, networkInterface) { + if groupIdentifier != nil { return fmt.Errorf("Security Group ID (%s) still attached to ENI (%s)", securityGroupID, networkInterfaceID) } } @@ -215,20 +204,6 @@ func testAccCheckAWSNetworkInterfaceSGAttachmentDestroy(s *terraform.State) erro return nil } -func testAccCheckAwsNetworkInterfaceSGAttachmentDisappears(resourceName string, networkInterface *ec2.NetworkInterface) resource.TestCheckFunc { - return func(s *terraform.State) error { - rs, ok := s.RootModule().Resources[resourceName] - if !ok { - return fmt.Errorf("Not found: %s", resourceName) - } - - conn := testAccProvider.Meta().(*AWSClient).ec2conn - securityGroupID := rs.Primary.Attributes["security_group_id"] - - return delSGFromENI(conn, securityGroupID, networkInterface) - } -} - func testAccAwsNetworkInterfaceSGAttachmentConfig(rName string) string { return fmt.Sprintf(` resource "aws_vpc" "test" {