Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

service/ec2: Handle additional EIP association issues and remove EIP hardcoded us-east-1 environment variable handling in tests #16032

Merged
merged 1 commit into from
Nov 12, 2020
Merged

service/ec2: Handle additional EIP association issues and remove EIP hardcoded us-east-1 environment variable handling in tests #16032

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 63 additions & 0 deletions aws/resource_aws_eip.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,16 @@ import (
"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/aws/awserr"
"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"
"github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags"
"github.com/terraform-providers/terraform-provider-aws/aws/internal/tfresource"
)

const (
// Maximum amount of time to wait for EIP association with EC2-Classic instances
ec2AddressAssociationClassicTimeout = 2 * time.Minute
)

func resourceAwsEip() *schema.Resource {
Expand Down Expand Up @@ -376,6 +383,12 @@ func resourceAwsEipUpdate(d *schema.ResourceData, meta interface{}) error {
d.Set("network_interface", "")
return fmt.Errorf("Failure associating EIP: %s", err)
}

if assocOpts.AllocationId == nil {
if err := waitForEc2AddressAssociationClassic(ec2conn, aws.StringValue(assocOpts.PublicIp), aws.StringValue(assocOpts.InstanceId)); err != nil {
return fmt.Errorf("error waiting for EC2 Address (%s) to associate with EC2-Classic Instance (%s): %w", aws.StringValue(assocOpts.PublicIp), aws.StringValue(assocOpts.InstanceId), err)
}
}
}

if d.HasChange("tags") && !d.IsNewResource() {
Expand Down Expand Up @@ -489,3 +502,53 @@ func disassociateEip(d *schema.ResourceData, meta interface{}) error {
}
return err
}

// waitForEc2AddressAssociationClassic ensures the correct Instance is associated with an Address
//
// This can take a few seconds to appear correctly for EC2-Classic addresses.
func waitForEc2AddressAssociationClassic(conn *ec2.EC2, publicIP string, instanceID string) error {
input := &ec2.DescribeAddressesInput{
Filters: []*ec2.Filter{
{
Name: aws.String("public-ip"),
Values: []*string{aws.String(publicIP)},
},
{
Name: aws.String("domain"),
Values: []*string{aws.String(ec2.DomainTypeStandard)},
},
},
}

err := resource.Retry(ec2AddressAssociationClassicTimeout, func() *resource.RetryError {
output, err := conn.DescribeAddresses(input)

if tfawserr.ErrCodeEquals(err, "InvalidAddress.NotFound") {
return resource.RetryableError(err)
}

if err != nil {
return resource.NonRetryableError(err)
}

if len(output.Addresses) == 0 || output.Addresses[0] == nil {
return resource.RetryableError(fmt.Errorf("not found"))
}

if aws.StringValue(output.Addresses[0].InstanceId) != instanceID {
return resource.RetryableError(fmt.Errorf("not associated"))
}

return nil
})

if tfresource.TimedOut(err) {
_, err = conn.DescribeAddresses(input)
}

if err != nil {
return fmt.Errorf("error describing EC2 Address (%s) association: %w", publicIP, err)
}

return nil
}
32 changes: 25 additions & 7 deletions aws/resource_aws_eip_association.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ 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"
)
Expand Down Expand Up @@ -96,12 +97,25 @@ func resourceAwsEipAssociationCreate(d *schema.ResourceData, meta interface{}) e
err := resource.Retry(2*time.Minute, func() *resource.RetryError {
var err error
resp, err = conn.AssociateAddress(request)

// EC2-VPC error for new addresses
if tfawserr.ErrCodeEquals(err, "InvalidAllocationID.NotFound") {
return resource.RetryableError(err)
}

// EC2-Classic error for new addresses
if tfawserr.ErrMessageContains(err, "AuthFailure", "does not belong to you") {
return resource.RetryableError(err)
}

if tfawserr.ErrMessageContains(err, "InvalidInstanceID", "pending instance") {
return resource.RetryableError(err)
}

if err != nil {
if isAWSErr(err, "InvalidInstanceID", "pending instance") {
return resource.RetryableError(err)
}
return resource.NonRetryableError(err)
}

return nil
})
if isResourceTimeoutError(err) {
Expand All @@ -121,11 +135,15 @@ func resourceAwsEipAssociationCreate(d *schema.ResourceData, meta interface{}) e
supportedPlatforms, resp)
}

if resp.AssociationId == nil {
// This is required field for EC2 Classic per docs
d.SetId(*request.PublicIp)
if resp.AssociationId != nil {
d.SetId(aws.StringValue(resp.AssociationId))
} else {
d.SetId(*resp.AssociationId)
// EC2-Classic
d.SetId(aws.StringValue(request.PublicIp))

if err := waitForEc2AddressAssociationClassic(conn, aws.StringValue(request.PublicIp), aws.StringValue(request.InstanceId)); err != nil {
return fmt.Errorf("error waiting for EC2 Address (%s) to associate with EC2-Classic Instance (%s): %w", aws.StringValue(request.PublicIp), aws.StringValue(request.InstanceId), err)
}
}

return resourceAwsEipAssociationRead(d, meta)
Expand Down
132 changes: 68 additions & 64 deletions aws/resource_aws_eip_association_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package aws

import (
"fmt"
"os"
"testing"

"github.com/aws/aws-sdk-go/aws"
Expand All @@ -17,9 +16,9 @@ func TestAccAWSEIPAssociation_instance(t *testing.T) {
var a ec2.Address

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSEIPAssociationDestroy,
PreCheck: func() { testAccPreCheck(t) },
ProviderFactories: testAccProviderFactories,
CheckDestroy: testAccCheckAWSEIPAssociationDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSEIPAssociationConfig_instance(),
Expand All @@ -42,9 +41,9 @@ func TestAccAWSEIPAssociation_networkInterface(t *testing.T) {
var a ec2.Address

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSEIPAssociationDestroy,
PreCheck: func() { testAccPreCheck(t) },
ProviderFactories: testAccProviderFactories,
CheckDestroy: testAccCheckAWSEIPAssociationDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSEIPAssociationConfig_networkInterface,
Expand All @@ -67,9 +66,9 @@ func TestAccAWSEIPAssociation_basic(t *testing.T) {
resourceName := "aws_eip_association.by_allocation_id"

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t); testAccEC2VPCOnlyPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSEIPAssociationDestroy,
PreCheck: func() { testAccPreCheck(t); testAccEC2VPCOnlyPreCheck(t) },
ProviderFactories: testAccProviderFactories,
CheckDestroy: testAccCheckAWSEIPAssociationDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSEIPAssociationConfig(),
Expand All @@ -95,21 +94,16 @@ func TestAccAWSEIPAssociation_ec2Classic(t *testing.T) {
var a ec2.Address
resourceName := "aws_eip_association.test"

oldvar := os.Getenv("AWS_DEFAULT_REGION")
os.Setenv("AWS_DEFAULT_REGION", "us-east-1")
defer os.Setenv("AWS_DEFAULT_REGION", oldvar)

// This test cannot run in parallel with the other EIP Association tests
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t); testAccEC2ClassicPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSEIPAssociationDestroy,
resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t); testAccEC2ClassicPreCheck(t) },
ProviderFactories: testAccProviderFactories,
CheckDestroy: testAccCheckAWSEIPAssociationDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSEIPAssociationConfig_ec2Classic,
Config: testAccAWSEIPAssociationConfig_ec2Classic(),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSEIPExists("aws_eip.test", &a),
testAccCheckAWSEIPAssociationExists(resourceName, &a),
testAccCheckAWSEIPEc2ClassicExists("aws_eip.test", &a),
testAccCheckAWSEIPAssociationEc2ClassicExists(resourceName, &a),
resource.TestCheckResourceAttrSet(resourceName, "public_ip"),
resource.TestCheckResourceAttr(resourceName, "allocation_id", ""),
testAccCheckAWSEIPAssociationHasIpBasedId(resourceName),
Expand All @@ -130,9 +124,9 @@ func TestAccAWSEIPAssociation_spotInstance(t *testing.T) {
resourceName := "aws_eip_association.test"

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSEIPAssociationDestroy,
PreCheck: func() { testAccPreCheck(t) },
ProviderFactories: testAccProviderFactories,
CheckDestroy: testAccCheckAWSEIPAssociationDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSEIPAssociationConfig_spotInstance(rInt),
Expand All @@ -158,9 +152,9 @@ func TestAccAWSEIPAssociation_disappears(t *testing.T) {
resourceName := "aws_eip_association.test"

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSEIPAssociationDestroy,
PreCheck: func() { testAccPreCheck(t) },
ProviderFactories: testAccProviderFactories,
CheckDestroy: testAccCheckAWSEIPAssociationDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSEIPAssociationConfigDisappears(),
Expand Down Expand Up @@ -208,6 +202,42 @@ func testAccCheckAWSEIPAssociationExists(name string, res *ec2.Address) resource
}
}

func testAccCheckAWSEIPAssociationEc2ClassicExists(name string, res *ec2.Address) resource.TestCheckFunc {
return func(s *terraform.State) error {
rs, ok := s.RootModule().Resources[name]
if !ok {
return fmt.Errorf("Not found: %s", name)
}

if rs.Primary.ID == "" {
return fmt.Errorf("No EIP Association ID is set")
}

conn := testAccProviderEc2Classic.Meta().(*AWSClient).ec2conn
platforms := testAccProviderEc2Classic.Meta().(*AWSClient).supportedplatforms

request, err := describeAddressesById(rs.Primary.ID, platforms)

if err != nil {
return err
}

describe, err := conn.DescribeAddresses(request)

if err != nil {
return fmt.Errorf("error describing EC2 Address (%s): %w", rs.Primary.ID, err)
}

if len(describe.Addresses) != 1 || aws.StringValue(describe.Addresses[0].PublicIp) != rs.Primary.ID {
return fmt.Errorf("EC2 Address (%s) not found", rs.Primary.ID)
}

*res = *describe.Addresses[0]

return nil
}
}

func testAccCheckAWSEIPAssociationHasIpBasedId(name string) resource.TestCheckFunc {
return func(s *terraform.State) error {
rs, ok := s.RootModule().Resources[name]
Expand Down Expand Up @@ -385,51 +415,25 @@ resource "aws_eip_association" "test" {
`)
}

const testAccAWSEIPAssociationConfig_ec2Classic = `
provider "aws" {
region = "us-east-1"
}

func testAccAWSEIPAssociationConfig_ec2Classic() string {
return composeConfig(
testAccEc2ClassicRegionProviderConfig(),
testAccLatestAmazonLinuxPvEbsAmiConfig(),
testAccAvailableEc2InstanceTypeForRegion("t1.micro", "m3.medium", "m3.large", "c3.large", "r3.large"),
`
resource "aws_eip" "test" {}

data "aws_availability_zones" "available" {
state = "available"

filter {
name = "opt-in-status"
values = ["opt-in-not-required"]
}
}

data "aws_ami" "ubuntu" {
most_recent = true

filter {
name = "name"
values = ["ubuntu/images/ebs/ubuntu-trusty-14.04-i386-server-*"]
}

filter {
name = "virtualization-type"
values = ["paravirtual"]
}

owners = ["099720109477"] # Canonical
}

resource "aws_instance" "test" {
ami = data.aws_ami.ubuntu.id
availability_zone = data.aws_availability_zones.available.names[0]

# tflint-ignore: aws_instance_previous_type
instance_type = "t1.micro"
ami = data.aws_ami.amzn-ami-minimal-pv-ebs.id
instance_type = data.aws_ec2_instance_type_offering.available.instance_type
}

resource "aws_eip_association" "test" {
public_ip = aws_eip.test.public_ip
instance_id = aws_instance.test.id
}
`
`)
}

func testAccAWSEIPAssociationConfig_spotInstance(rInt int) string {
return composeConfig(
Expand Down
Loading