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

Jump to bottom

WIP: Add ipv4_ipam_pool_id & ipv4_netmask_length #102

Merged
merged 3 commits into from
Apr 25, 2022
Merged

WIP: Add ipv4_ipam_pool_id & ipv4_netmask_length #102

merged 3 commits into from
Apr 25, 2022

Conversation

jbouse
Copy link
Contributor

@jbouse jbouse commented Jan 10, 2022 •
edited
Loading

what

  • Add ipv4_ipam_pool_id & ipv4_netmask_length variables
  • Update cidr_block with default value of null
  • Variables cidr_block is mutually exclusive to using ipv4_ipam_pool_id and ipv4_netmask_length

why

  • In order to support assigning the cidr_block auto-assignment for a VPC provided from an AWS VPC IPAM pool

@jbouse
Add ipv4_ipam_pool_id & ipv4_netmask_length
25d5967 
Signed-off-by: Jeremy T. Bouse <Jeremy.Bouse@UnderGrid.net>
@jbouse jbouse requested review from a team as code owners January 10, 2022 19:59
@jbouse jbouse requested review from srhopkins and nitrocode and removed request for a team January 10, 2022 19:59
@jbouse jbouse changed the title Add ipv4_ipam_pool_id & ipv4_netmask_length WIP: Add ipv4_ipam_pool_id & ipv4_netmask_length Jan 10, 2022
@jbouse
Copy link
Contributor Author

jbouse commented Jan 11, 2022

@Nuru this is a modification I was working on locally and mentioned in last weeks office hours. Was suggested I run past you to get your thoughts.

@korenyoni korenyoni added the enhancement New feature or request label Feb 10, 2022
@korenyoni
Copy link
Member

/test all

@nitrocode nitrocode removed their request for review February 10, 2022 18:02
@korenyoni
Copy link
Member

@jbouse sorry for the late review.

Tests are failing because of conflicting arguments at the provider level:

TestExamplesComplete 2022-02-10T17:42:02Z logger.go:66: 
TestExamplesComplete 2022-02-10T17:42:02Z logger.go:66: Error: Conflicting configuration arguments
TestExamplesComplete 2022-02-10T17:42:02Z logger.go:66: 
TestExamplesComplete 2022-02-10T17:42:02Z logger.go:66:   on ../../main.tf line 17, in resource "aws_vpc" "default":
TestExamplesComplete 2022-02-10T17:42:02Z logger.go:66:   17:   cidr_block                       = var.cidr_block
TestExamplesComplete 2022-02-10T17:42:02Z logger.go:66: 
TestExamplesComplete 2022-02-10T17:42:02Z logger.go:66: "cidr_block": conflicts with ipv4_netmask_length
TestExamplesComplete 2022-02-10T17:42:02Z logger.go:66: 
TestExamplesComplete 2022-02-10T17:42:02Z logger.go:66: 
TestExamplesComplete 2022-02-10T17:42:02Z logger.go:66: Error: Missing required argument
TestExamplesComplete 2022-02-10T17:42:02Z logger.go:66: 
TestExamplesComplete 2022-02-10T17:42:02Z logger.go:66:   on ../../main.tf line 19, in resource "aws_vpc" "default":
TestExamplesComplete 2022-02-10T17:42:02Z logger.go:66:   19:   ipv4_netmask_length              = var.ipv4_netmask_length
TestExamplesComplete 2022-02-10T17:42:02Z logger.go:66: 
TestExamplesComplete 2022-02-10T17:42:02Z logger.go:66: "ipv4_netmask_length": all of `ipv4_ipam_pool_id,ipv4_netmask_length` must be
TestExamplesComplete 2022-02-10T17:42:02Z logger.go:66: specified

korenyoni
korenyoni previously requested changes Feb 10, 2022
View reviewed changes
Copy link
Member

@korenyoni korenyoni left a comment

Choose a reason for hiding this comment

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

See comment

main.tf Outdated
Comment on lines 18 to 19
ipv4_ipam_pool_id = var.ipv4_ipam_pool_id
ipv4_netmask_length = var.ipv4_netmask_length
Copy link
Member

Choose a reason for hiding this comment

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

Seems to conflict with cidr_block

https://github.com/cloudposse/actions/actions/runs/1825260398

TestExamplesComplete 2022-02-10T17:42:02Z logger.go:66: 
TestExamplesComplete 2022-02-10T17:42:02Z logger.go:66: Error: Conflicting configuration arguments
TestExamplesComplete 2022-02-10T17:42:02Z logger.go:66: 
TestExamplesComplete 2022-02-10T17:42:02Z logger.go:66:   on ../../main.tf line 17, in resource "aws_vpc" "default":
TestExamplesComplete 2022-02-10T17:42:02Z logger.go:66:   17:   cidr_block                       = var.cidr_block
TestExamplesComplete 2022-02-10T17:42:02Z logger.go:66: 
TestExamplesComplete 2022-02-10T17:42:02Z logger.go:66: "cidr_block": conflicts with ipv4_netmask_length
TestExamplesComplete 2022-02-10T17:42:02Z logger.go:66: 
TestExamplesComplete 2022-02-10T17:42:02Z logger.go:66: 
TestExamplesComplete 2022-02-10T17:42:02Z logger.go:66: Error: Missing required argument
TestExamplesComplete 2022-02-10T17:42:02Z logger.go:66: 
TestExamplesComplete 2022-02-10T17:42:02Z logger.go:66:   on ../../main.tf line 19, in resource "aws_vpc" "default":
TestExamplesComplete 2022-02-10T17:42:02Z logger.go:66:   19:   ipv4_netmask_length              = var.ipv4_netmask_length
TestExamplesComplete 2022-02-10T17:42:02Z logger.go:66: 
TestExamplesComplete 2022-02-10T17:42:02Z logger.go:66: "ipv4_netmask_length": all of `ipv4_ipam_pool_id,ipv4_netmask_length` must be
TestExamplesComplete 2022-02-10T17:42:02Z logger.go:66: specified

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe I've updated to execute examples/complete without regression... I still need to put together an example to validate the use of IPAM instead of using cidr_block as examples/complete does. I'll remove WIP: from the PR title when I believe it's fully ready with testing completed.

@jbouse
Copy link
Contributor Author

jbouse commented Feb 11, 2022

@korenyoni I'll go back and take another look at this... The use case I'm working on is being able to use AWS IPAM when deploying out the VPC so it would either use cidr_block or ipv4_ipam_pool_id & ipv4_netmask_length. The two should be mutually exclusive. You couldn't set all 3 variables obviously as they would conflict.

@jbouse
Improve conditional variable usage
d3843e3 
As `cidr_block` and `ipv4_ipam_pool_id` are mutually exclusive, ensure
that `ipv4_netmask_length` is null if `ipv4_ipam_pool_id` is null.

Signed-off-by: Jeremy T. Bouse <Jeremy.Bouse@UnderGrid.net>
@mergify mergify bot dismissed korenyoni’s stale review February 13, 2022 20:20

This Pull Request has been updated, so we're dismissing all reviews.

@Nuru
Copy link
Contributor

Nuru commented Apr 25, 2022

/test all

@Nuru Nuru added the no-release Do not create a new release (wait for additional code changes) label Apr 25, 2022
@Nuru Nuru self-requested a review April 25, 2022 05:10
Nuru
Nuru approved these changes Apr 25, 2022
View reviewed changes
Copy link
Contributor

@Nuru Nuru left a comment

Choose a reason for hiding this comment

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

@jbouse Thank you for this work and sorry it took so long to review it. I am merging this PR but not releasing it so that I can make some changes rather than ask you to make them.

Supersedes and closes #103

@Nuru Nuru merged commit 5bca106 into cloudposse:master Apr 25, 2022
@Nuru
Copy link
Contributor

Nuru commented Apr 28, 2022

@jbouse IPv6 and IPAM support released in v1.1.0. Please try it out and report any issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Nuru Nuru Nuru approved these changes

@korenyoni korenyoni korenyoni left review comments

@srhopkins srhopkins Awaiting requested review from srhopkins srhopkins was automatically assigned from cloudposse/contributors

Assignees
No one assigned
Labels
enhancement New feature or request no-release Do not create a new release (wait for additional code changes)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jbouse @korenyoni @Nuru @cloudpossebot