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.99k stars 4.44k forks source link

feat: Support pre-IPAM BYOIPv6 #897

Closed nshenry03 closed 1 year ago

nshenry03 commented 1 year ago

Description

Before IPAM, users were still able to BYOIPv6. This change allows these users to use this module.

Motivation and Context

We used AWS BYOIPv6 before the AWS IPAM feature so we have to pass ipv6_ipam_pool_id AND pass null to assign_generated_ipv6_cidr_block. Currently the code for assign_generated_ipv6_cidr_block is:

  assign_generated_ipv6_cidr_block = var.enable_ipv6 && !var.use_ipam_pool ? true : null

so there is no way for us to pass assign_generated_ipv6_cidr_block = null without setting use_ipam_pool which has it's own implications since:

  cidr_block          = var.use_ipam_pool ? null : var.cidr

Breaking Changes

No, I double checked that this shouldn't break anything or change existing behavior.

How Has This Been Tested?

I have updated at least one of the examples/* to demonstrate and validate my change(s)

I haven't done this because the developer (or CI/CD) needs to have BYOIPv6 setup.

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

I haven't done this because the developer (or CI/CD) needs to have BYOIPv6 setup.

Instead, I have done some manual testing...

Existing code

assign_generated_ipv6_cidr_block = var.enable_ipv6 && !var.use_ipam_pool ? true : null

Tests

Existing functionality

The input, assign_generated_ipv6_cidr_block, is only true when enable_ipv6 is true and use_ipam_pool is false; otherwise it's null. This is expected since we only want AWS to generate an IPv6 CIDR when IPv6 is enabled and we aren't using an IPAM pool.

enable_ipv6 = true && use_ipam_pool = true
> true && !true ? true : null
tobool(null)
enable_ipv6 = true && use_ipam_pool = false
> true && !false ? true : null
true
enable_ipv6 = false && use_ipam_pool = true
> false && !true ? true : null
tobool(null)
enable_ipv6 = false && use_ipam_pool = false
> false && !false ? true : null
tobool(null)
Ensure we don't break existing functionality

Functionality is the same. If the user doesn't pass assign_generated_ipv6_cidr_block, then the only time we ask AWS to generate an IPv6 CIDR is when IPv6 is enabled and we aren't using an IPAM pool.

enable_ipv6 = true && use_ipam_pool = true && assign_generated_ipv6_cidr_block = null
> true && coalesce(null, true) && !true ? true : null
tobool(null)
enable_ipv6 = true && use_ipam_pool = false && assign_generated_ipv6_cidr_block = null
> true && coalesce(null, true) && !false ? true : null
true
enable_ipv6 = false && use_ipam_pool = true && assign_generated_ipv6_cidr_block = null
> false && coalesce(null, true) && !true ? true : null
tobool(null)
enable_ipv6 = false && use_ipam_pool = false && assign_generated_ipv6_cidr_block = null
> false && coalesce(null, true) && !false ? true : null
tobool(null)
Ensure that if assign_generated_ipv6_cidr_block = false, we never ask AWS to generate an IPv6 CIDR block

If assign_generated_ipv6_cidr_block = false, we never ask AWS to generate an IPv6 CIDR block.

enable_ipv6 = true && use_ipam_pool = true && assign_generated_ipv6_cidr_block = false
> true && coalesce(false, true) && !true ? true : null
tobool(null)
enable_ipv6 = true && use_ipam_pool = false && assign_generated_ipv6_cidr_block = false
> true && coalesce(false, true) && !false ? true : null
tobool(null)
enable_ipv6 = false && use_ipam_pool = true && assign_generated_ipv6_cidr_block = false
> false && coalesce(false, true) && !true ? true : null
tobool(null)
enable_ipv6 = false && use_ipam_pool = false && assign_generated_ipv6_cidr_block = false
> false && coalesce(false, true) && !false ? true : null
tobool(null)
Ensure that if assign_generated_ipv6_cidr_block = true, things are handled as they are today

We can be certain that since things were the same when assign_generated_ipv6_cidr_block was null (and the default value was true, that when assign_generated_ipv6_cidr_block = true, things are handled as they are today; we would only ask AWS to generate an IPv6 CIDR if IPv6 is enabled, and we aren't using an IPAM pool.

What about when ipv6_ipam_pool_id is not null?

The only time there is a conflict is when ipv6_ipam_pool_id is not null (and assign_generated_ipv6_cidr_block evaluates to true).

If a user passes ipv6_ipam_pool_id, and leaves assign_generated_ipv6_cidr_block as null, then the only time assign_generated_ipv6_cidr_block is not null is when enable_ipv6 is true and use_ipam_pool is false. In this case, it works the same as today and the user should set use_ipam_pool to true since they are passing a value for assign_generated_ipv6_cidr_block.

If a user passes ipv6_ipam_pool_id, and sets assign_generated_ipv6_cidr_block to true, then the only time assign_generated_ipv6_cidr_block is not null is when enable_ipv6 is true and use_ipam_pool is false. In this case, the user messed up 2x, they should NOT have set assign_generated_ipv6_cidr_block to true AND they SHOULD have set use_ipam_pool to true; fixing either mistake will resolve the conflict.

I have executed pre-commit run -a on my pull request

nicholas7DYYW:terraform-aws-vpc hnicholas$ git status
On branch support-legacy-ipam
Your branch is up to date with 'origin/support-legacy-ipam'.

nothing to commit, working tree clean
hnicholas7DYYW:terraform-aws-vpc hnicholas$ pre-commit run -a
Terraform fmt............................................................Passed
Terraform validate.......................................................Passed
Terraform docs...........................................................Passed
Terraform validate with tflint...........................................Passed
check for merge conflicts................................................Passed
fix end of files.........................................................Passed
github-actions[bot] commented 1 year ago

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

britthouser commented 1 year ago

@nshenry03 - Does the underlying VPC resource itself support this? The resource has ipv6_ipam_pool_id input option, which implies requiring IPAM. Have you successfully passed in a non-IPAM ipv6 pool to this option and it accepted it?

britthouser commented 1 year ago

@nshenry03 - Does the underlying VPC resource itself support this? The resource has ipv6_ipam_pool_id input option, which implies requiring IPAM. Have you successfully passed in a non-IPAM ipv6 pool to this option and it accepted it?

nshenry03 commented 1 year ago

@nshenry03 - Does the underlying VPC resource itself support this? The resource has ipv6_ipam_pool_id input option, which implies requiring IPAM. Have you successfully passed in a non-IPAM ipv6 pool to this option and it accepted it?

Hey @britthouser , yes, we currently are using this in our environment and it works

github-actions[bot] commented 1 year ago

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

github-actions[bot] commented 1 year ago

This PR was automatically closed because of stale in 10 days

github-actions[bot] commented 1 year ago

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.