terraform-aws-modules / terraform-aws-vpc

Terraform module to create AWS VPC resources πŸ‡ΊπŸ‡¦
https://registry.terraform.io/modules/terraform-aws-modules/vpc/aws
Apache License 2.0
2.96k stars 4.42k forks source link

Add support for AWS IPAM VPCs - IPv4 #715

Closed drewmullen closed 2 years ago

drewmullen commented 2 years ago

https://aws.amazon.com/about-aws/whats-new/2021/12/amazon-virtual-private-cloud-vpc-announces-ip-address-manager-ipam/

will be creating a 2nd issue/ PR for ipv6 support. theres some potentially breaking changes for the current release, will want to discuss with yall

drewmullen commented 2 years ago

I am currently working on this feature

yahyabmx commented 2 years ago

Hi, maybe with creating subnets dynamically, I'm trying to do it with cidrsubnets() but stuck with dependencies with the vpc cidr (as there is no attribute cidr_block for aws_vpc resource, I have to do it with datasource hence my dependency problem :/

drewmullen commented 2 years ago

@yahyabmx will this feature solve your issue? Here is the PR, just waiting for it to be approved and merged: https://github.com/terraform-aws-modules/terraform-aws-vpc/pull/716

yahyabmx commented 2 years ago

@drewmullen unfortunatly it does not resolve my issue. as my understanding you still have to give the subnets cidrs as input ? even though how can you predict what IPAM will allocate as cidr for the vpc ?

drewmullen commented 2 years ago

I believe I understand your concern, @yahyabmx. Let me try and elaborate and see if we agree on the problem:

IPAM allows you to derive CIDR ranges for VPCs in 3 different ways:

  1. explicitly requesting a CIDR range from IPAM (you must know that this range is not already used somewhere else)
  2. requesting a range by explicitly setting a default netmask on the vpc resource
  3. allowing the IPAM pool to derive by setting a default at the pool level using the attribute allocation_default_netmask_length

Your concern is, within this module, creating subnets from the VPC using private_subnets & public_subnets? That is something I had not tested. This is an interesting problem. Good catch!

there is no attribute cidr_block for aws_vpc resource

cidr_block is outputted as a read attribute so we can use that to compute cidr ranges in a local. The issue is how to implement without breaking.

My first thought is that we could create separate subnet resource definitions that build with the condition var.ipv4_ipam_pool_id != null. At that point instead of passing explicit cidrs, perhaps you just pass a list of netmasks you want, say [24,24,28] and we compute 3 subnets off the IPAM derived CIDR

thoughts?

yahyabmx commented 2 years ago

@drewmullen Thank you for summing it up. We agree on the 3 different ways (didn't know about the 3rd one with allocation_default_netmask_length) In my case it's the 2nd case we are talking about yes.

Your concern is, within this module, creating subnets from the VPC using private_subnets & public_subnets? Correct

Sorry I dont understand what a read attribute is ? you mean once the resource is created ? if it's the case, we agree on that (I can get it from aws_vpc datasource for example. I've looked in aws_vpc that there is no cidr_block attribute.

Yes we can generate subnets by passing a list of masks (using cidrsubnets() function)

And we compute 3 subnets off the IPAM derived CIDR

You mean we can get the CIDR beforehand ? if that is possible, I think I got my solution.

drewmullen commented 2 years ago

Here is an example, @yahyabmx

setup resources for example

data "aws_region" "current" {}

resource "aws_vpc_ipam" "example" {
  operating_regions {
    region_name = data.aws_region.current.name
  }
}

resource "aws_vpc_ipam_pool" "ipv4_example" {
  address_family                    = "ipv4"
  ipam_scope_id                     = aws_vpc_ipam.example.private_default_scope_id
  locale                            = data.aws_region.current.name
  allocation_default_netmask_length = 28
}

resource "aws_vpc_ipam_pool_cidr" "ipv4_example" {
  ipam_pool_id = aws_vpc_ipam_pool.ipv4_example.id
  cidr         = "172.2.0.0/16"
}

create VPC and output cidr

module "ipv4_ipam_explicit_netmask_vpc" {
  source              = "../.."
  name                = "ipv4-explicit-netmask-${local.name}"
  ipv4_ipam_pool_id   = aws_vpc_ipam_pool.ipv4_example.id
  ipv4_netmask_length = 28
  depends_on = [
    aws_vpc_ipam_pool_cidr.ipv4_example
  ]
}

output "dervied_cidr" {
  value = module.ipv4_ipam_explicit_netmask_vpc.vpc_cidr_block
}

$ tf apply
...
Apply complete! Resources: 0 added, 0 changed, 0 destroyed.

Outputs:

dervied_cidr = "172.2.0.0/28"

I'm glad that your brought this up though because I do think we need to account for creating subnets within the module itself. I wont have time to update this till ... probably january... but my thought is (from above)

that we could create separate subnet resource definitions that build with the condition var.ipv4_ipam_pool_id != null. At that point instead of passing explicit cidrs, perhaps you just pass a list of netmasks you want, say [24,24,28] and we compute 3 subnets off the IPAM derived CIDR

jbouse commented 2 years ago

I've been delving into this myself within my multi-account setup where my management account has the IPAM shared as RAM resources. Retrieving the appropriate regional IPAM pool ID isn't a problem, just in using the VPC module. I could create the VPC as documented above passing the ipv4_ipam_pool_id I retrieved using a data source and ipv4_netmask_length desired for the VPC CIDR block. Again the issue is defining the various subnet blocks outside the VPC module. Currently without this can use the module like this:

data "aws_region" "current" {}

data "aws_vpc_ipam_pool" "this" {
  locale = data.aws_region.current.name
}

resource "aws_vpc_ipam_pool_cidr_allocation" "this" {
  netmask_length = 28
  ipam_pool_id   = data.aws_vpc_ipam_pool.this.id
}

I can get the CIDR block as aws_vpc_ipam_pool_cidr_allocation.this.cidr to pass in to the module as the base_cidr and using either the hashicorp/subnets/cidr module or cidrsubnets() function generate the various subnet values to pass in as well. The problem I had was that I had to disable my organization SCPs that were attempting to enforce VPC CIDRs be associated with the IPAM pool or be denied since the ec2:CreateVpc and ec2:AssociateVpcCidrBlock calls did not include the ec2:Ipv4IpamPoolId key for the conditional.

The big challenge I see in attempting to get this included in the module itself is that the aws_subnet doesn't take anything but a CIDR which if you just pass the ipv4_ipam_pool_id to the aws_vpc resource you won't know the block yet so the module would have to be extremely opinionated or have some way to designate how to break down the VPC CIDR into the various subnets, possibly similar to how IPv6 prefixes are handled?

drewmullen commented 2 years ago

@jbouse just one to clarify 1 thing. aws_vpc_ipam_pool_cidr_allocation is only used to reserve space so its not used up by IPAM. The only way to allocate a cidr for usage is by using aws_vpc or the aws_vpc_ipv4_cidr_block_association resource. To request a CIDR from a ram shared pool, just use the pool id on aws_vpc.

ipam DOES have a notion of previewing the next available cidr. i considered making a data source for that for situations like youre describing however, i was concerned of scenarios where the cidr you preview is used by the time you request it. at the end of the day you can just request the CIDR from IPAM when your VPC is created and reference the derived CIDR range as an attribute of the resource inside cidrsubnets(). aka: aws_vpc.this.cidr_block

Although, regarding this module, i am starting to reconsider that decision. perhaps i should make a data source to preview the next cidr when you could then subnet and pass the values to the module as is.

drewmullen commented 2 years ago

now that .72 is merged we have access to the new resource to preview cidrs. However, im struggling to figure this out because the subnet variables are used in count which cannot be determined during plan. see below:

resource "aws_vpc_ipam_preview_next_cidr" "previewed_cidr" {
  ipam_pool_id   = aws_vpc_ipam_pool.ipv4_example.id
  netmask_length = 24

  depends_on = [
    aws_vpc_ipam_pool_cidr.ipv4_example
  ]
}

locals {
  partition = cidrsubnets(aws_vpc_ipam_preview_next_cidr.previewed_cidr.cidr, 2, 2)
  private_subnets = cidrsubnets(local.partition[0], 2, 2)
  public_subnets = cidrsubnets(local.partition[1], 2, 2)
}

module "ipv4_ipam_calculate_subnets" {
  source            = "../.."
  name              = "ipv4-calculated-subnets-${local.name}"
  ipv4_ipam_pool_id = aws_vpc_ipam_pool.ipv4_example.id
  cidr              = aws_vpc_ipam_preview_next_cidr.previewed_cidr.cidr
  private_subnets   = local.private_subnets
  public_subnets    = local.public_subnets
  depends_on = [
    aws_vpc_ipam_pool_cidr.ipv4_example
  ]
}

Sample of error (there are several):

β•·
β”‚ Error: Invalid count argument
β”‚ 
β”‚   on ../../main.tf line 409, in resource "aws_subnet" "private":
β”‚  409:   count = var.create_vpc && length(var.private_subnets) > 0 ? length(var.private_subnets) : 0
β”‚ 
β”‚ The "count" value depends on resource attributes that cannot be determined until apply, so Terraform cannot predict how many instances will be created. To work around this, use the -target argument to
β”‚ first apply only the resources that the count depends on.

I believe the issue is because aws_vpc_ipam_preview_next_cidr was made a resource and not a data source. We choose to make it a resource instead of a data source because after a cidr is allocated the preview would find a new value, ergo you'd need to always set ignore_changes to it. Thats makes the derived cidr like an id so it made sense at the time. The resource does work in many situations but when required for a module which loops over its output, im at a loss for how to get it to work. Any ideas?

Otherwise, we may just need to create a data source as well 😭

jbouse commented 2 years ago

@drewmullen yeah that's been the issue I'm running up against. I think this is a side-effect of expecting the VPC module to also be the SUBNET module. As a result, I'm moving away from this module and using smaller focused unit modules. If my VPC module is more focused on just defining the VPC and not including the subnets, I can pass ipv4_ipam_pool_id and ipv4_netmask_length rather than the cidr_block. Then I can have the VPC module return the ipv4_cidr_block which can be passed to the module to set up the subnets within the VPC.

I guess this could be done within the existing framework if the subnets were a separate internal module that could be called after the VPC resource had been created and cidr_block known and could be passed on. Though this would introduce a potentially backward-incompatible change for resources created with an earlier version. An added positive side-effect of this would be making the aws_subnet resource creation DRY as you could reuse the same module for all subnet groups.

drewmullen commented 2 years ago

Release .73 included the new data source but im still running into compile-time computation issues. looking for input: https://github.com/terraform-aws-modules/terraform-aws-vpc/pull/716

github-actions[bot] commented 2 years ago

This issue has been automatically marked as stale because it has been open 30 days with no activity. Remove stale label or comment or this issue will be closed in 10 days

danielvrog commented 2 years ago

I am working on IPAM adoption for my organization and I faced a issue with not being able to add secondary_cidr_blocks based on aws_vpc_ipam_pool. Since VPC's CIDR is limited to /16 by default, we use secondary_cidr_blocks to reach /12 range that we need. Assuming that this will require list of ipv4_netmask_lengths to be allocated from the same pool as the main CIDR.

Also, I think that only one of ipv4_ipam_pool_id and cidr variables can be defined while ipv4_ipam_pool_id should be come with ipv4_netmask_length

drewmullen commented 2 years ago

@danielvrog It definitely makes sense to add ipam arguments to https://github.com/terraform-aws-modules/terraform-aws-vpc/blob/e02118633f268ff1f86021a8fa9f3afcd1c37d85/main.tf#L38

Also, I think that only one of ipv4_ipam_pool_id and cidr variables can be defined while ipv4_ipam_pool_id should be come with ipv4_netmask_length

generally speaking its perfectly valid to provide any combination:

when it comes to this module though, you technically could set ipv4_netmask_length but then you cannot know which SUBNET cidrs to provision. You can example of how to use ipam with this module in the PR readme: https://github.com/terraform-aws-modules/terraform-aws-vpc/pull/716/files

danielvrog commented 2 years ago

I like what you did there. Now, since aws_vpc_ipv4_cidr_block_association relies on secondary_cidr_blocks, following your approach in #716 , we need to use aws_vpc_ipam_preview_next_cidr with count for list of ipv4_netmask_lengths that may be provided in order to calculate and associate extra ranges. The challenge is in aws_vpc_ipam_preview_next_cidr that does not see count as number of the next CIDR blocks of netmask_length size to return and returns same block for each count. Still cannot wrap my mind around what to do with subnets in such case...

drewmullen commented 2 years ago

If we add the arguments to that PR, you could at least assign the 2nd cidr block with IPAM and you would just have to do the subnets some other way :/ @danielvrog

drewmullen commented 2 years ago

@danielvrog feel free to reach out to me privately through the AWS network slack: https://join.slack.com/t/cloud-network-as-code/shared_invite/zt-143uj48qa-L8VhLZMg7oAO1hLHE0o5QA

github-actions[bot] commented 2 years ago

This issue has been automatically marked as stale because it has been open 30 days with no activity. Remove stale label or comment or this issue will be closed in 10 days

github-actions[bot] commented 2 years ago

This issue was automatically closed because of stale in 10 days

antonbabenko commented 2 years ago

This issue has been resolved in version 3.15.0 :tada:

github-actions[bot] commented 1 year ago

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.