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

feat: Add IPAM IPv4 support #716

Merged
merged 12 commits into from
Sep 25, 2022

Conversation

drewmullen
Copy link
Contributor

@drewmullen drewmullen commented Dec 2, 2021

Description

Motivation and Context

AWS launched VPC IPAM service

Breaking Changes

none

How Has This Been Tested?

  • I have tested and validated these changes using one or more of the provided examples/* projects
terraform init
terraform apply

I also attempted to implement terraform test but ran into a breaking bug

@drewmullen
Copy link
Contributor Author

can you please help a guy out with the correct terraform-docs command to run? pre-commit is showing skipped in my terminal:
Terraform docs.......................................(no files to check)Skipped

@bryantbiggs
Copy link
Member

@drewmullen pre-commit run -a

@drewmullen
Copy link
Contributor Author

thank you, @bryantbiggs . not sure why it was skipping b4 but 🤷

fix is pushed.

main.tf Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
@drewmullen drewmullen mentioned this pull request Dec 8, 2021
1 task
@drewmullen drewmullen changed the title add ipam ipv4 support [HOLD] add ipam ipv4 support Dec 20, 2021
@drewmullen
Copy link
Contributor Author

requesting to hold while this is investigated: #715 (comment)

@drewmullen
Copy link
Contributor Author

hoping this data source will allow us to use this module and maintain subnet creation: hashicorp/terraform-provider-aws#22643

commenting for staleness

@drewmullen
Copy link
Contributor Author

The last commit used precommit -no-verify because the example is currently broken and should not be used. I pushed it in case anyone else has recommendations.

Context: VPC module allows users to specify the subnets they want to create inside the VPC, this requires setting the cidr range for each subnet. Using AWS IPAM you may get your VPC CIDR derived from IPAM which means you wont know which subnet cidrs to use. The included examples/ipam-vpc/main.tf attempts to calculate the values by previewing the next cidr that will be used when a VPC is requested:

data "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(data.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              = data.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
  ]
}

ERRORs:

│ Error: Error in function call
│ 
│   on ../../main.tf line 424, in resource "aws_subnet" "private":
│  424:         element(var.azs, count.index),
│     ├────────────────
│     │ count.index is 0
│     │ var.azs is empty list of string

Youll find many similar errors when you run terraform apply. I believe the root cause may be use of count instead of for_each, but I'm honestly not sure.

Any input?

@github-actions github-actions bot added the stale label Feb 26, 2022
@drewmullen
Copy link
Contributor Author

I had an epiphany! I have solved this issue. Ill clean up the PR and add some documentation to make usage more clear. Hopefully can push Monday!

@drewmullen drewmullen changed the title [HOLD] add ipam ipv4 support feat: Add IPAM IPv4 Support Feb 26, 2022
@drewmullen
Copy link
Contributor Author

drewmullen commented Feb 26, 2022

Ready for a re-review. Appreciate the patience while I figure this one out!

Tested:

cd examples/ipam-vpc
terraform apply -auto-approve && terraform destroy -auto-approve

@drewmullen
Copy link
Contributor Author

Any input on what is causing the failure in vpc-flow-logs example? Shouldnt be any effects due to my change (1 new argument / variable)

@bryantbiggs bryantbiggs changed the title feat: Add IPAM IPv4 Support feat: Add IPAM IPv4 support Sep 9, 2022
}

locals {
name = "complete-example"
name = "ex-${replace(basename(path.cwd), "_", "-")}"
Copy link
Member

Choose a reason for hiding this comment

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

All of the provider/locals/tags updates were my fault - just aligning the examples to be consistent


```bash
$ terraform destroy -target=module.vpc # destroy VPC that uses the IPAM pool CIDR first
$ terraform destroy
Copy link
Member

Choose a reason for hiding this comment

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

not sure why, but deleting the aws_vpc_ipam_pool_cidr.this takes forever 🤷🏽‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

It would be great to combine these two terraform destroy into one, as we have in all other modules, but I could not find a way to do this now. Let's leave it as it is now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure why, but deleting the aws_vpc_ipam_pool_cidr.this takes forever 🤷🏽‍♂️

The service holds the allocation record for up to 30m after a vpc releases it. It's possible to ignore the allocation and delete faster but conflicts with terraforms paradigm

Copy link
Member

Choose a reason for hiding this comment

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

Oh, good to know!

@bryantbiggs
Copy link
Member

apologies for the delay, I think when I first followed these PRs I was under the impression that they were breaking changes. @antonbabenko what do you think?

Copy link
Member

@antonbabenko antonbabenko left a comment

Choose a reason for hiding this comment

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

Minor comments, but LGTM.

README.md Outdated Show resolved Hide resolved

```bash
$ terraform destroy -target=module.vpc # destroy VPC that uses the IPAM pool CIDR first
$ terraform destroy
Copy link
Member

Choose a reason for hiding this comment

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

It would be great to combine these two terraform destroy into one, as we have in all other modules, but I could not find a way to do this now. Let's leave it as it is now.

examples/ipam-vpc/versions.tf Outdated Show resolved Hide resolved
examples/ipam-vpc/main.tf Outdated Show resolved Hide resolved
Co-authored-by: drewmullen <mullen.drew@gmail.com>
@bryantbiggs bryantbiggs merged commit 6eddcad into terraform-aws-modules:master Sep 25, 2022
antonbabenko pushed a commit that referenced this pull request Sep 25, 2022
## [3.15.0](v3.14.4...v3.15.0) (2022-09-25)

### Features

* Add IPAM IPv4 support ([#716](#716)) ([6eddcad](6eddcad))
@antonbabenko
Copy link
Member

This PR is included in version 3.15.0 🎉

@drewmullen drewmullen deleted the ipam-ipv4 branch September 25, 2022 18:11
@antonbabenko
Copy link
Member

The PR has been finally merged - it's time for the party at AWS re:invent 2022 🎉🥳 Agree?

@drewmullen
Copy link
Contributor Author

@antonbabenko 100%! I'll be there! We should do a informal tf community meet-up or something

@antonbabenko
Copy link
Member

antonbabenko commented Sep 25, 2022

@drewmullen I agree we can make a more planned Terraform community meetup. I need to know what is the program for and by AWS Heroes, first. Stay in touch!

@bryantbiggs
Copy link
Member

We'll need a 🍻 budget

@github-actions
Copy link

I'm going to lock this pull request 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 related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for AWS IPAM VPCs - IPv4
3 participants