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

Ensure IPv6 CIDR block is assigned to default VPC #4382

Closed
wants to merge 3 commits into from
Closed

Ensure IPv6 CIDR block is assigned to default VPC #4382

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
798a84f
Ensure IPv6 CIDR block is assigned to default VPC.
ewbankkit Apr 22, 2018
18f6680
Acceptance test changes after review.
ewbankkit Apr 30, 2018
4e8ed5b
Add a CustomizeDiff for aws_default_vpc resource.
ewbankkit May 4, 2018
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
74 changes: 57 additions & 17 deletions aws/resource_aws_default_vpc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ func resourceAwsDefaultVpc() *schema.Resource {
dvpc.Create = resourceAwsDefaultVpcCreate
dvpc.Delete = resourceAwsDefaultVpcDelete

// Can't "terraform import" a Default VPC; Use "terraform apply"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #4462.
Do we want to block terraform import of default VPC?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we need to block import? It should be a valid form of managing an existing default VPC as with any other Terraform resource - although we just won't be performing any validation that it actually is or is not the default VPC on import currently. I think we can (and should) add that validation in the aws_default_vpc read function. Without this extra logic though, I think adding a documentation note that the import validation is not currently present is fine. Since the functionality is already present, some people might be depending on it already in their environment.

Honestly, the "magic" we have some places in the provider to cross-reference schema attributes, functions, etc. often makes everything more complicated in the end as seen here. I would almost prefer we just fully redefine the aws_default_vpc functions and schema to appropriately match the physical resource even if most of the logic is identical to aws_vpc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.
I think if possible it would be great to somehow combine the aws_vpc and aws_default_vpc resources (and maybe deprecate aws_default_vpc) especially given that default VPCs can now be re-created if they are deleted- #1400?

On reflection this PR seems to be changing a lot of code for what on the face of it should be a relatively small (though important) change.
I'm happy to close this PR and spend some time thinking about a more maintainable path forward - All ideas welcome.

dvpc.Importer = nil

dvpc.CustomizeDiff = resourceAwsDefaultVpcCustomizeDiff

// cidr_block is a computed value for Default VPCs
dvpc.Schema["cidr_block"] = &schema.Schema{
Type: schema.TypeString,
Expand All @@ -28,38 +33,73 @@ func resourceAwsDefaultVpc() *schema.Resource {
// assign_generated_ipv6_cidr_block is a computed value for Default VPCs
dvpc.Schema["assign_generated_ipv6_cidr_block"] = &schema.Schema{
Type: schema.TypeBool,
Optional: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For backwards compatibility I don't set the Default: false flag here as is done for the equivalent attribute in the aws_vpc resource.

Computed: true,
}

return dvpc
}

func resourceAwsDefaultVpcCreate(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*AWSClient).ec2conn
req := &ec2.DescribeVpcsInput{
Filters: []*ec2.Filter{
{
Name: aws.String("isDefault"),
Values: aws.StringSlice([]string{"true"}),
},
},
}

resp, err := conn.DescribeVpcs(req)
vpc, err := resourceAwsDefaultVpcFindVpc(meta)
if err != nil {
return err
}

if resp.Vpcs == nil || len(resp.Vpcs) == 0 {
return fmt.Errorf("No default VPC found in this region.")
}

d.SetId(aws.StringValue(resp.Vpcs[0].VpcId))

d.SetId(aws.StringValue(vpc.VpcId))
return resourceAwsVpcUpdate(d, meta)
}

func resourceAwsDefaultVpcDelete(d *schema.ResourceData, meta interface{}) error {
log.Printf("[WARN] Cannot destroy Default VPC. Terraform will remove this resource from the state file, however resources may remain.")
return nil
}

func resourceAwsDefaultVpcCustomizeDiff(diff *schema.ResourceDiff, meta interface{}) error {
if diff.Id() == "" {
// New resource.
v, ok := diff.GetOkExists("assign_generated_ipv6_cidr_block")
if ok {
// assign_generated_ipv6_cidr_block specified.
newIpv6Flag := v.(bool)

// See if the Default VPC already has an IPv6 CIDR block assigned.
vpc, err := resourceAwsDefaultVpcFindVpc(meta)
if err != nil {
return err
}

oldIpv6Flag := resourceAwsVpcFindIpv6CidrBlockAssociation(vpc) != nil
log.Printf("[DEBUG] Default VPC IPv6 %v -> %v", oldIpv6Flag, newIpv6Flag)
if newIpv6Flag == oldIpv6Flag {
diff.Clear("assign_generated_ipv6_cidr_block")
} else {
diff.SetNew("assign_generated_ipv6_cidr_block", newIpv6Flag)
}
}
}

return nil
}

func resourceAwsDefaultVpcFindVpc(meta interface{}) (*ec2.Vpc, error) {
conn := meta.(*AWSClient).ec2conn

req := &ec2.DescribeVpcsInput{}
req.Filters = buildEC2AttributeFilterList(
map[string]string{
"isDefault": "true",
},
)

log.Printf("[DEBUG] Reading Default VPC: %#v", req)
resp, err := conn.DescribeVpcs(req)
if err != nil {
return nil, err
}
if resp.Vpcs == nil || len(resp.Vpcs) == 0 {
return nil, fmt.Errorf("No default VPC found in this region.")
}

return resp.Vpcs[0], nil
}
132 changes: 120 additions & 12 deletions aws/resource_aws_default_vpc_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,77 @@ func TestAccAWSDefaultVpc_basic(t *testing.T) {
resource.TestCheckResourceAttr(
"aws_default_vpc.foo", "tags.%", "1"),
resource.TestCheckResourceAttr(
"aws_default_vpc.foo", "tags.Name", "Default VPC"),
resource.TestCheckNoResourceAttr(
"aws_default_vpc.foo", "assign_generated_ipv6_cidr_block"),
resource.TestCheckNoResourceAttr(
"aws_default_vpc.foo", "ipv6_association_id"),
resource.TestCheckNoResourceAttr(
"aws_default_vpc.foo", "ipv6_cidr_block"),
"aws_default_vpc.foo", "tags.Name", "terraform-testacc-default-vpc"),
),
},
},
})
}

func TestAccAWSDefaultVpc_enableIpv6(t *testing.T) {
var vpc ec2.Vpc

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSDefaultVpcDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSDefaultVpcConfigIpv6Enabled,
Check: resource.ComposeTestCheckFunc(
testAccCheckVpcExists("aws_default_vpc.foo", &vpc),
testAccCheckVpcCidr(&vpc, "172.31.0.0/16"),
resource.TestCheckResourceAttr(
"aws_default_vpc.foo", "cidr_block", "172.31.0.0/16"),
resource.TestCheckResourceAttr(
"aws_default_vpc.foo", "tags.%", "1"),
resource.TestCheckResourceAttr(
"aws_default_vpc.foo", "tags.Name", "terraform-testacc-default-vpc-ipv6"),
testAccCheckVpcIpv6(&vpc, true),
),
},
// Ensure that we don't try an associate another Amazon-provided IPv6 CIDR.
{
Config: testAccAWSDefaultVpcAltConfigIpv6Enabled,
Check: resource.ComposeTestCheckFunc(
testAccCheckVpcExists("aws_default_vpc.bar", &vpc),
testAccCheckVpcCidr(&vpc, "172.31.0.0/16"),
resource.TestCheckResourceAttr(
"aws_default_vpc.bar", "cidr_block", "172.31.0.0/16"),
resource.TestCheckResourceAttr(
"aws_default_vpc.bar", "tags.%", "1"),
resource.TestCheckResourceAttr(
"aws_default_vpc.bar", "tags.Name", "terraform-testacc-default-vpc-ipv6"),
testAccCheckVpcIpv6(&vpc, true),
),
},
{
Config: testAccAWSDefaultVpcConfigIpv6Disabled,
Check: resource.ComposeTestCheckFunc(
testAccCheckVpcExists("aws_default_vpc.foo", &vpc),
testAccCheckVpcCidr(&vpc, "172.31.0.0/16"),
resource.TestCheckResourceAttr(
"aws_default_vpc.foo", "assign_generated_ipv6_cidr_block", "false"),
resource.TestCheckResourceAttr(
"aws_default_vpc.foo", "ipv6_association_id", ""),
resource.TestCheckResourceAttr(
"aws_default_vpc.foo", "ipv6_cidr_block", ""),
testAccCheckVpcIpv6(&vpc, false),
),
},
// Ensure that we don't try an disassociate the Amazon-provided IPv6 CIDR again.
{
Config: testAccAWSDefaultVpcAltConfigIpv6Disabled,
Check: resource.ComposeTestCheckFunc(
testAccCheckVpcExists("aws_default_vpc.baz", &vpc),
testAccCheckVpcCidr(&vpc, "172.31.0.0/16"),
resource.TestCheckResourceAttr(
"aws_default_vpc.baz", "assign_generated_ipv6_cidr_block", "false"),
resource.TestCheckResourceAttr(
"aws_default_vpc.baz", "ipv6_association_id", ""),
resource.TestCheckResourceAttr(
"aws_default_vpc.baz", "ipv6_cidr_block", ""),
testAccCheckVpcIpv6(&vpc, false),
),
},
},
Expand All @@ -45,13 +109,57 @@ func testAccCheckAWSDefaultVpcDestroy(s *terraform.State) error {
}

const testAccAWSDefaultVpcConfigBasic = `
provider "aws" {
region = "us-west-2"
resource "aws_default_vpc" "foo" {
tags {
Name = "terraform-testacc-default-vpc"
}
}
`

const testAccAWSDefaultVpcConfigIpv6Enabled = `
resource "aws_default_vpc" "foo" {
assign_generated_ipv6_cidr_block = true
tags {
Name = "terraform-testacc-default-vpc-ipv6"
}
}
`

const testAccAWSDefaultVpcConfigIpv6Disabled = `
resource "aws_default_vpc" "foo" {
assign_generated_ipv6_cidr_block = false
tags {
Name = "terraform-testacc-default-vpc-ipv6"
}
}
`

const testAccAWSDefaultVpcAltConfigIpv6Enabled = `
resource "aws_default_vpc" "foo" {
tags {
Name = "terraform-testacc-default-vpc-ipv6"
}
}

resource "aws_default_vpc" "bar" {
assign_generated_ipv6_cidr_block = true
tags {
Name = "terraform-testacc-default-vpc-ipv6"
}
}
`

const testAccAWSDefaultVpcAltConfigIpv6Disabled = `
resource "aws_default_vpc" "foo" {
tags {
Name = "Default VPC"
}
tags {
Name = "terraform-testacc-default-vpc-ipv6"
}
}

resource "aws_default_vpc" "baz" {
assign_generated_ipv6_cidr_block = false
tags {
Name = "terraform-testacc-default-vpc-ipv6"
}
}
`
51 changes: 30 additions & 21 deletions aws/resource_aws_vpc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,16 +112,16 @@ func resourceAwsVpc() *schema.Resource {

func resourceAwsVpcCreate(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*AWSClient).ec2conn
instance_tenancy := "default"

instance_tenancy := ec2.VpcTenancyDefault
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor to use constants from the AWS SDK.

if v, ok := d.GetOk("instance_tenancy"); ok {
instance_tenancy = v.(string)
}

// Create the VPC
createOpts := &ec2.CreateVpcInput{
CidrBlock: aws.String(d.Get("cidr_block").(string)),
InstanceTenancy: aws.String(instance_tenancy),
AmazonProvidedIpv6CidrBlock: aws.Bool(d.Get("assign_generated_ipv6_cidr_block").(bool)),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the aws_default_vpc resource calls resourceAwsVpcUpdate() we don't associate an IPv6 CIDR block when creating the VPC but do it later when resourceAwsVpcCreate() calls resourceAwsVpcUpdate().
See removal of !d.IsNewResource() below.

CidrBlock: aws.String(d.Get("cidr_block").(string)),
InstanceTenancy: aws.String(instance_tenancy),
}

log.Printf("[DEBUG] VPC create config: %#v", *createOpts)
Expand All @@ -144,8 +144,8 @@ func resourceAwsVpcCreate(d *schema.ResourceData, meta interface{}) error {
"[DEBUG] Waiting for VPC (%s) to become available",
d.Id())
stateConf := &resource.StateChangeConf{
Pending: []string{"pending"},
Target: []string{"available"},
Pending: []string{ec2.VpcStatePending},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor to use constants from the AWS SDK.

Target: []string{ec2.VpcStateAvailable},
Refresh: VPCStateRefreshFunc(conn, d.Id()),
Timeout: 10 * time.Minute,
}
Expand Down Expand Up @@ -182,16 +182,14 @@ func resourceAwsVpcRead(d *schema.ResourceData, meta interface{}) error {
// Tags
d.Set("tags", tagsToMap(vpc.Tags))

for _, a := range vpc.Ipv6CidrBlockAssociationSet {
if *a.Ipv6CidrBlockState.State == "associated" { //we can only ever have 1 IPv6 block associated at once
d.Set("assign_generated_ipv6_cidr_block", true)
d.Set("ipv6_association_id", a.AssociationId)
d.Set("ipv6_cidr_block", a.Ipv6CidrBlock)
} else {
d.Set("assign_generated_ipv6_cidr_block", false)
d.Set("ipv6_association_id", "") // we blank these out to remove old entries
d.Set("ipv6_cidr_block", "")
}
if a := resourceAwsVpcFindIpv6CidrBlockAssociation(vpc); a != nil {
d.Set("assign_generated_ipv6_cidr_block", true)
d.Set("ipv6_association_id", a.AssociationId)
d.Set("ipv6_cidr_block", a.Ipv6CidrBlock)
} else {
d.Set("assign_generated_ipv6_cidr_block", false)
d.Set("ipv6_association_id", "") // we blank these out to remove old entries
d.Set("ipv6_cidr_block", "")
}

resp, err := awsVpcDescribeVpcAttribute("enableDnsSupport", vpcid, conn)
Expand Down Expand Up @@ -390,7 +388,7 @@ func resourceAwsVpcUpdate(d *schema.ResourceData, meta interface{}) error {
d.SetPartial("enable_classiclink_dns_support")
}

if d.HasChange("assign_generated_ipv6_cidr_block") && !d.IsNewResource() {
if d.HasChange("assign_generated_ipv6_cidr_block") {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments above explaining that any IPv6 CIDR block is not associated during VPC creation but is associated here.

toAssign := d.Get("assign_generated_ipv6_cidr_block").(bool)

log.Printf("[INFO] Modifying assign_generated_ipv6_cidr_block to %#v", toAssign)
Expand All @@ -412,8 +410,8 @@ func resourceAwsVpcUpdate(d *schema.ResourceData, meta interface{}) error {
"[DEBUG] Waiting for IPv6 CIDR (%s) to become associated",
d.Id())
stateConf := &resource.StateChangeConf{
Pending: []string{"associating", "disassociated"},
Target: []string{"associated"},
Pending: []string{ec2.VpcCidrBlockStateCodeAssociating, ec2.VpcCidrBlockStateCodeDisassociated},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor to use constants from the AWS SDK.

Target: []string{ec2.VpcCidrBlockStateCodeAssociated},
Refresh: Ipv6CidrStateRefreshFunc(conn, d.Id(), *resp.Ipv6CidrBlockAssociation.AssociationId),
Timeout: 1 * time.Minute,
}
Expand All @@ -437,8 +435,8 @@ func resourceAwsVpcUpdate(d *schema.ResourceData, meta interface{}) error {
"[DEBUG] Waiting for IPv6 CIDR (%s) to become disassociated",
d.Id())
stateConf := &resource.StateChangeConf{
Pending: []string{"disassociating", "associated"},
Target: []string{"disassociated"},
Pending: []string{ec2.VpcCidrBlockStateCodeDisassociating, ec2.VpcCidrBlockStateCodeAssociated},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor to use constants from the AWS SDK.

Target: []string{ec2.VpcCidrBlockStateCodeDisassociated},
Refresh: Ipv6CidrStateRefreshFunc(conn, d.Id(), d.Get("ipv6_association_id").(string)),
Timeout: 1 * time.Minute,
}
Expand Down Expand Up @@ -650,3 +648,14 @@ func awsVpcDescribeVpcAttribute(attribute string, vpcId string, conn *ec2.EC2) (

return resp, nil
}

func resourceAwsVpcFindIpv6CidrBlockAssociation(vpc *ec2.Vpc) *ec2.VpcIpv6CidrBlockAssociation {
for _, a := range vpc.Ipv6CidrBlockAssociationSet {
if aws.StringValue(a.Ipv6CidrBlockState.State) == ec2.VpcCidrBlockStateCodeAssociated {
// We can only ever have 1 IPv6 block associated at once.
return a
}
}

return nil
}
Loading