Add support for creating IPv6 VPC and cluster #4317

nikimanoledaki · 2021-10-06T16:06:21Z

Description

This PR introduces the option to create a cluster with an IPv6 enabled VPC.

When VPC.IPFamily is set to IPv6 we create a VPC with IPv6 enabled. To checkout all the resource we create, see pkg/cfn/builder/vpc_ipv6.go. These are: VPC, IPv4 CIDR, IPv6 CIDR, EOIGW, NAT GW, IGW, route tables, routes, private and public subnets, and route table associations.

CreateAddonTasks waits for addons to be healthy only if there are nodegroups. (already merged to main #4332)

AssignIpv6AddressOnCreation is set to true on public subnets as a post-creation task due to a CF bug.

Checklist

Added tests that cover your change (if possible)
Added/modified documentation as required (such as the README.md, or the userdocs directory)
Manually tested
Made sure the title of the PR is a good description that can go into the release notes
(Core team) Added labels for change area (e.g. area/nodegroup) and kind (e.g. kind/improvement)

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

Backfilled missing tests for code in same general area 🎉
Refactored something and made the world a better place 🌟

go.mod

Skarlso

First run of the PR with initial comments and suggestions.

Well done you two, this is excellent work. :)

integration/tests/ipv6/ipv6_test.go

pkg/cfn/builder/cluster.go

integration/tests/ipv6/ipv6_test.go

pkg/cfn/builder/cluster.go

pkg/cfn/builder/vpc.go

pkg/cfn/manager/create_tasks.go

pkg/cfn/manager/tasks.go

pkg/cfn/manager/tasks_test.go

pkg/ctl/cmdutils/configfile.go

go.mod

pkg/cfn/builder/cluster.go

integration/tests/ipv6/ipv6_test.go

cPu1 · 2021-10-21T13:05:32Z

pkg/cfn/builder/vpc.go

@@ -1,655 +1,69 @@
 package builder


This file has almost all code moved to vpc_ipv4.go and that's shown as a lot of additions and deletions by Git. This pollutes the Git history and can be misleading when tracking down the changes later. Can we do a git mv pkg/cfn/builder/vpc.go pkg/cfn/builder/vpc_ipv4.go first, make modifications, and then create a new file vpc.go with the changes common to IPv4 and IPv6? It'll also make the changelist small and the PR easier to review.

I just ticked [x] Viewed while viewing the diff, and that help me a lot to reduce the clutter change set.

good idea to use git mv to preserve the git history, will do that! :)

@cPu1 how would you suggest doing this retroactively? I'm not sure how to do this with a rebase 🤔

integration/tests/ipv6/ipv6_test.go

pkg/ctl/cmdutils/configfile.go

Skarlso

Some minor comments still, but otherwise I THINK THIS IS OKAY!!! :) 🥳 🎈 🎉

Himangini

LGTM. Remember to Squash and Merge when you merge this PR.

nikimanoledaki · 2021-10-25T15:29:41Z

@Himangini if we squash and merge we might lose some of the useful context from the commits and their messages, especially since it's such a large PR! maybe we could clean up the git history and squash some commits but not all of them? wdyt? :)

Himangini · 2021-10-25T16:48:12Z

@Himangini if we squash and merge we might lose some of the useful context from the commits and their messages, especially since it's such a large PR! maybe we could clean up the git history and squash some commits but not all of them? wdyt? :)

Isn't this PR enough for context? it'll be cleaner to have single commits in the feature branch 🤷🏻‍♀️ easier to revert too

aclevername · 2021-10-25T17:12:50Z

Isn't this PR enough for context? it'll be cleaner to have single commits in the feature branch

I think this is the single biggest thing that will go into the branch though, its worth having this extra historical commit history. This PR will also be behind two-PRs, since we will then merge the ipv6 branch into main, so its not that clear really.

easier to revert too

The PR has a revert button for after its merged encase we had to revert it. This PR also contains a commit that would ideally be reverted in the future as well, which is another reason to keep it c489d36

Himangini · 2021-10-26T11:08:50Z

Isn't this PR enough for context? it'll be cleaner to have single commits in the feature branch

I think this is the single biggest thing that will go into the branch though, its worth having this extra historical commit history. This PR will also be behind two-PRs, since we will then merge the ipv6 branch into main, so its not that clear really.

easier to revert too

The PR has a revert button for after its merged encase we had to revert it. This PR also contains a commit that would ideally be reverted in the future as well, which is another reason to keep it c489d36

You can't guarantee this is the single biggest thing that will go into the branch. I know the PR has a revert button, I was mentioning this in the context of a feature branch.

This PR also contains a commit that would ideally be reverted in the future as well, which is another reason to keep it c489d36

Useful info, it wasn't obvious to me, maybe I've missed it in the description. I guess it's fine to merge them as it is.

- Add integration test for ipv6 - Renamed VPC files to IPv4 file, and added IPv6 VPC files. - Created new resource set for IPv6 - Added all of the resources to create IPv6 VPC to CF template - Resources being created: VPC, IPv4 CIDR, IPv6 CIDR, EOIGW, NAT GW, IGW, route tables, routes, private and public subnets, route table associations - Added outputs to CF template for VPC and public/private subnets - Added integration and unit tests Co-authored-by: Jake Klein <aclevername@users.noreply.github.com> Refactoring VPC template creation - Making addResources and addOutputs private - Creating new CreateTemplate function for both IPv4 and IPv6

* Adding integration test set AssignIpv6AddressOnCreation to true for public subnets after the rest of the template.

- Revert me!

… CF bug - AssignIpv6AddressOnCreation also needs to be set on public subnets, but due to a current bug in CF, this cannot be set alongside MapPublicIpOnLaunch at create time. This means we need to add it "manually" by hitting the VPC API to update each public subnet after launch. - Added extra validation that NAT is nil

This reverts commit dc2c50a.

- Also pass unit tests and remove commented code

Update integration/tests/ipv6/ipv6_test.go Co-authored-by: Chetan Patwal <cPu1@users.noreply.github.com> set example ipv6 region to us-west-2

nikimanoledaki · 2021-10-26T14:40:52Z

I squashed some commits so that we have 5 fewer of them while still maintaining some extra history/context and the commits that need to be reverted in the future - hope this works for everyone

nikimanoledaki added kind/feature New feature or request area/ipv6 labels Oct 6, 2021

nikimanoledaki force-pushed the v6-vpc-creation branch 4 times, most recently from e660058 to 31e61b4 Compare October 12, 2021 13:22

aclevername reviewed Oct 14, 2021

View reviewed changes

go.mod Outdated Show resolved Hide resolved

nikimanoledaki force-pushed the v6-vpc-creation branch 5 times, most recently from 11b756e to 2dc5c45 Compare October 15, 2021 15:25

nikimanoledaki marked this pull request as ready for review October 18, 2021 09:14

nikimanoledaki requested review from Himangini, Skarlso and cPu1 October 18, 2021 09:14

Skarlso suggested changes Oct 20, 2021

View reviewed changes

cPu1 reviewed Oct 20, 2021

View reviewed changes

go.mod Outdated Show resolved Hide resolved

cPu1 reviewed Oct 20, 2021

View reviewed changes

pkg/cfn/builder/cluster.go Outdated Show resolved Hide resolved

nikimanoledaki force-pushed the v6-vpc-creation branch from 28e08a1 to f67da11 Compare October 20, 2021 15:14

aclevername force-pushed the v6-vpc-creation branch from f67da11 to 3b48a63 Compare October 20, 2021 15:56

aclevername requested review from Skarlso and cPu1 October 20, 2021 15:56

cPu1 reviewed Oct 21, 2021

View reviewed changes

integration/tests/ipv6/ipv6_test.go Outdated Show resolved Hide resolved

cPu1 reviewed Oct 21, 2021

View reviewed changes