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.

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

New Resource: aws_vpc_associate_cidr_block #1568

Closed
wants to merge 1 commit into from

Conversation

stack72
Copy link
Contributor

@stack72 stack72 commented Sep 1, 2017

Closes: #1539
Fixes: #1537

Adds the ability to associate extra IPv4 or IPv6 CIDR Blocks with a VPC.
In order to avoid getting into the same issue as security_group and
security_group_rule, we added a diffSuppressFunc that stops people
enabling ipv6 for an existant AWS VPC. We added a note to the
documentation to talk about this

% make testacc TEST=./aws TESTARGS='-run=TestAccAWSVpc_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSVpc_ -timeout 120m
=== RUN   TestAccAWSVpc_importBasic
--- PASS: TestAccAWSVpc_importBasic (54.16s)
=== RUN   TestAccAWSVpc_basic
--- PASS: TestAccAWSVpc_basic (45.26s)
=== RUN   TestAccAWSVpc_enableIpv6
--- PASS: TestAccAWSVpc_enableIpv6 (87.40s)
=== RUN   TestAccAWSVpc_dedicatedTenancy
--- PASS: TestAccAWSVpc_dedicatedTenancy (45.51s)
=== RUN   TestAccAWSVpc_tags
--- PASS: TestAccAWSVpc_tags (84.80s)
=== RUN   TestAccAWSVpc_update
--- PASS: TestAccAWSVpc_update (97.12s)
=== RUN   TestAccAWSVpc_bothDnsOptionsSet
--- PASS: TestAccAWSVpc_bothDnsOptionsSet (19.82s)
=== RUN   TestAccAWSVpc_DisabledDnsSupport
--- PASS: TestAccAWSVpc_DisabledDnsSupport (44.93s)
=== RUN   TestAccAWSVpc_classiclinkOptionSet
--- PASS: TestAccAWSVpc_classiclinkOptionSet (46.18s)
=== RUN   TestAccAWSVpc_classiclinkDnsSupportOptionSet
--- PASS: TestAccAWSVpc_classiclinkDnsSupportOptionSet (47.51s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	572.738s
% make testacc TEST=./aws TESTARGS='-run=TestAccAWSVpcAssociat'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSVpcAssociat -timeout 120m
=== RUN   TestAccAWSVpcAssociateIpv4CidrBlock
--- PASS: TestAccAWSVpcAssociateIpv4CidrBlock (51.48s)
=== RUN   TestAccAWSVpcAssociateIpv6CidrBlock
--- PASS: TestAccAWSVpcAssociateIpv6CidrBlock (50.16s)
=== RUN   TestAccAWSVpcAssociateIpv4AndIpv6CidrBlock
--- PASS: TestAccAWSVpcAssociateIpv4AndIpv6CidrBlock (2.13s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	103.802s

Adds the ability to associate extra IPv4 or IPv6 CIDR Blocks with a VPC.
In order to avoid getting into the same issue as security_group and
security_group_rule, we added a diffSuppressFunc that stops people
enabling `ipv6` for an existant AWS VPC. We added a note to the
documentation to talk about this

```
% make testacc TEST=./aws TESTARGS='-run=TestAccAWSVpc_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSVpc_ -timeout 120m
=== RUN   TestAccAWSVpc_importBasic
--- PASS: TestAccAWSVpc_importBasic (54.16s)
=== RUN   TestAccAWSVpc_basic
--- PASS: TestAccAWSVpc_basic (45.26s)
=== RUN   TestAccAWSVpc_enableIpv6
--- PASS: TestAccAWSVpc_enableIpv6 (87.40s)
=== RUN   TestAccAWSVpc_dedicatedTenancy
--- PASS: TestAccAWSVpc_dedicatedTenancy (45.51s)
=== RUN   TestAccAWSVpc_tags
--- PASS: TestAccAWSVpc_tags (84.80s)
=== RUN   TestAccAWSVpc_update
--- PASS: TestAccAWSVpc_update (97.12s)
=== RUN   TestAccAWSVpc_bothDnsOptionsSet
--- PASS: TestAccAWSVpc_bothDnsOptionsSet (19.82s)
=== RUN   TestAccAWSVpc_DisabledDnsSupport
--- PASS: TestAccAWSVpc_DisabledDnsSupport (44.93s)
=== RUN   TestAccAWSVpc_classiclinkOptionSet
--- PASS: TestAccAWSVpc_classiclinkOptionSet (46.18s)
=== RUN   TestAccAWSVpc_classiclinkDnsSupportOptionSet
--- PASS: TestAccAWSVpc_classiclinkDnsSupportOptionSet (47.51s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	572.738s
```

```
% make testacc TEST=./aws TESTARGS='-run=TestAccAWSVpcAssociat'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSVpcAssociat -timeout 120m
=== RUN   TestAccAWSVpcAssociateIpv4CidrBlock
--- PASS: TestAccAWSVpcAssociateIpv4CidrBlock (51.48s)
=== RUN   TestAccAWSVpcAssociateIpv6CidrBlock
--- PASS: TestAccAWSVpcAssociateIpv6CidrBlock (50.16s)
=== RUN   TestAccAWSVpcAssociateIpv4AndIpv6CidrBlock
--- PASS: TestAccAWSVpcAssociateIpv4AndIpv6CidrBlock (2.13s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	103.802s
```
@grubernaut grubernaut added enhancement Requests to existing resources that expand the functionality or scope. new-resource Introduces a new resource. labels Sep 1, 2017
@p0wl
Copy link

p0wl commented Oct 16, 2017

any idea when this will be merged?

thanks for your awesome work!

@kwilczynski
Copy link
Contributor

Hi @radeksimko, any idea when this could be merged? It is a helpful feature.

@frightenedmonkey
Copy link

I would love to also get some color on when this might be merged/shipped. I actually could really use the functionality pretty soon.

@stephen-kainos
Copy link

Would also love to see this getting merged. Got something I could use this for now

@AzCii
Copy link

AzCii commented Oct 31, 2017

Any update on when this will be merged?

@aakash-garg-olx
Copy link

When it is going to be merged

@nicwise
Copy link
Contributor

nicwise commented Nov 2, 2017

gonna 👍 +1 this, like everyone else.

Mostly cos I want to use it, like, next week.

@radeksimko radeksimko added the size/XL Managed by automation to categorize the size of a PR. label Nov 15, 2017
@tprobinson
Copy link

I've been using this branch to build out deployments for the past few weeks, it works well but there is one thing that I've had to work around with it. It doesn't seem to create implicit dependencies, so I've had to do things like this (sanitized example):

resource "aws_vpc" "main" {
  cidr_block = "10.0.0.0/24"
}

resource "aws_vpc_associate_cidr_block" "main_cidr_b" {
  vpc_id          = "${aws_vpc.main.id}"
  ipv4_cidr_block = "10.0.1.0/24"
}

resource "aws_vpc_associate_cidr_block" "main_cidr_c" {
  vpc_id          = "${aws_vpc.main.id}"
  ipv4_cidr_block = "10.0.2.0/24"
}

resource "aws_subnet" "main_a" {
  vpc_id            = "${aws_vpc.main.id}"
  cidr_block        = "10.0.0.0/24"
}

resource "aws_subnet" "main_b" {
  vpc_id            = "${aws_vpc.main.id}"
  cidr_block        = "10.0.1.0/24"

  depends_on = ["aws_vpc_associate_cidr_block.main_cidr_b"]
}

resource "aws_subnet" "main_c" {
  vpc_id            = "${aws_vpc.main.id}"
  cidr_block        = "10.0.2.0/24"

  depends_on = ["aws_vpc_associate_cidr_block.main_cidr_c"]
}

resource "aws_vpc_peering_connection" "main2other" {
  peer_vpc_id = "${local.other_vpc_id}"
  vpc_id      = "${aws_vpc.main.id}"

  depends_on = [
    "aws_vpc_associate_cidr_block.main_cidr_b",
    "aws_vpc_associate_cidr_block.main_cidr_c",
  ]
}

After doing this it works, but I'd love if all subnets and peerings would depend on CIDR associations implicitly.

@danabr
Copy link

danabr commented Dec 11, 2017

If an ipv4_cidr_address attribute was added to aws_vpc_associate_cidr_block, you could solve @tprobinson's issue like this:

resource "aws_subnet" "main_b" {
  vpc_id            = "${aws_vpc.main.id}"
  cidr_block        = "${aws_vpc_associate_cidr_block.main_cidr_b.ipv4_cidr_address}"
}

And if you want to divide it into smaller subnets, you could use the cidrsubnetbuiltin function.

@jen20
Copy link
Contributor

jen20 commented Dec 18, 2017

I think @danabr is onto a nice solution here to solve @tprobinson's issue (not least because it allows use of cidrsubnet and friends in order to prevent having to hard-code CIDR blocks). @stack72, any thoughts?

@tprobinson
Copy link

I'm not sure I like that solution, mostly because you're then required to use a particular convention if you want the resources to work correctly.

In the original code that I sanitized that example from, I actually do use cidrsubnet to define my nets elsewhere, then just reference it in both resources like:

resource "aws_vpc_associate_cidr_block" "main_cidr_c" {
  vpc_id          = "${aws_vpc.main.id}"
  ipv4_cidr_block = "${local.main_cidr_c}"
}

resource "aws_subnet" "main_c" {
  vpc_id            = "${aws_vpc.main.id}"
  cidr_block        = "${local.main_cidr_c}"

  depends_on = ["aws_vpc_associate_cidr_block.main_cidr_c"]
}

I guess this resource is sort of an odd duck because it's a "glue" resource, but doesn't have the same unique-id-requirement information flow that other components have, like aws_lb, aws_lb_target_group, and aws_lb_target_group_attachment, which is why the chained attribute solution feels unsatisfying.

But if there's no way to prioritize resources implicitly (like all aws_vpc → all vpc_cidr → all aws_subnet), then @danabr 's solution makes sense.

@jen20
Copy link
Contributor

jen20 commented Dec 20, 2017

Sadly there is no way to do this internally - though is there a reason you couldn't build this into aws_vpc rather than as a separate resource?

@radeksimko radeksimko added service/ec2-classic service/ec2 Issues and PRs that pertain to the ec2 service. and removed service/ec2-classic labels Jan 16, 2018
@robh007
Copy link
Contributor

robh007 commented Jan 22, 2018

The only thing I can see what's actually causing an issue is the peering connection creation. I don't need to use the depends on meta-parameter within the subnet creation. I've not had to hard code anything. The error I'm seeing is as follows and appears to me to be a race condition based on the VPC being in a blocked state whilst the peering connection is created and accepted. I assume this is because you wouldn't want to allow a peering connection with a potential overlapping address range to be created. Whilst trying to add second address range to the VPC in parallel.

aws_vpc_associate_cidr_block.main_cidr_b: InvalidCidrAssociation.PendingVpcPeeringConnection. You cannot add a CIDR block to your VPC due to your pending VPC peering request

I personally don't think adding the secondary CIDR belongs in the initial VPC creation.

How does this get moved along ?

@vpadronblanco
Copy link
Contributor

I would support the solution not being as a separate resource, as specified in issue #3403. Makes the abstraction simpler, and exploits the use of the api. The associateVpcCidrBlock is already being used for ipv6 addresses from what I can see. So it would be just to clone that functionality when updating the resource.

@ewbankkit
Copy link
Contributor

As a VPC can have at most one IPv6 CIDR block and that IPv6 CIDR block can be assigned during VPC creation I would think that the assign_generated_ipv6_cidr_block attribute should continue to belong exclusively to the aws_vpc resource.
Secondary IPv4 CIDR blocks for a VPC should be managed via this new resource, perhaps renamed to something like aws_vpc_secondary_ipv4_cidr_block?

@robh007 There are a whole set of rules for creating a secondary CIDR block. I think a CustomizeDiff function should be able to trap many of these, including the condition you mention.

@ewbankkit
Copy link
Contributor

I have implemented the modifications I described above in #3723.
@stack72 Hope you don't mind.

@ghost
Copy link

ghost commented May 9, 2018

When will this be merged? Really need this feature! Thanks...

@scrothers
Copy link
Contributor

@bflad Is there additional help needed from the community with these Github issues?

Do we need more community involvement to get these committed? Seems like you're swamped with other merges, which is why you haven't gotten to this one.

@bflad
Copy link
Contributor

bflad commented Jun 28, 2018

Having looked at this #1568 and #3723, I personally lean a little more towards the updated implementation of #3723 (new resource naming aside) that does not change the existing behavior of the aws_vpc resource with respect to the self-contained "primary" IPv6 CIDR block handling. The self-contained "primary" IPv4/IPv4 CIDR blocks within aws_vpc I think should continue covering most VPC implementations and provide the least hassle for Terraform configuration implementations.

Given that this pull request is likely superseded by #3723 (and it properly contains the commits of this pull request), I'm going to close this one now.

@bflad bflad closed this Jun 28, 2018
@stack72 stack72 deleted the f-aws-vpc-cidr-association branch June 28, 2018 17:22
@ghost
Copy link

ghost commented Apr 4, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Apr 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Requests to existing resources that expand the functionality or scope. new-resource Introduces a new resource. service/ec2 Issues and PRs that pertain to the ec2 service. size/XL Managed by automation to categorize the size of a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support adding CIDRs to existing VPC