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

Add initial support for configuring IPv6 with AWS #11442

Merged
merged 3 commits into from
May 19, 2021

Conversation

hakman
Copy link
Member

@hakman hakman commented May 9, 2021

Inspired by #11163.

At the moment, the IPv6 feature configures the following AWS components:

  • VPC:
    • always requests the IPv6 CIDR from AWS
    • cannot be disabled
  • Routes:
    • always creates the default IPv6 route (for public topologies)
  • Security Group:
    • always allows IPv6 egress
    • adds / removes entries based on spec.kubernetesApiAccess and spec.sshAccess
  • Subnets:
    • users can assign / remove an IPV6 CIDR (but cannot change it)
  • Launch Templates:
    • IPv6 addresses are assigned to instances based on subnet setting, which allows marking instances as needing update

TODO:

Ref: #8432

/cc @rifelpet @johngmyers @olemarkus

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/api area/provider/aws Issues or PRs related to aws provider labels May 9, 2021
@hakman hakman force-pushed the ipv6 branch 3 times, most recently from e943bc0 to ddfabbb Compare May 9, 2021 19:33
@hakman
Copy link
Member Author

hakman commented May 9, 2021

/retest

Copy link
Member

@johngmyers johngmyers left a comment

Choose a reason for hiding this comment

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

Things are changing out from under my review, so submitting this early. Will review more later.

cloudmock/aws/mockec2/vpcs.go Outdated Show resolved Hide resolved
pkg/apis/kops/cluster.go Outdated Show resolved Hide resolved
pkg/apis/kops/cluster.go Outdated Show resolved Hide resolved
pkg/apis/kops/cluster.go Outdated Show resolved Hide resolved
pkg/apis/kops/validation/validation.go Outdated Show resolved Hide resolved
upup/pkg/fi/cloudup/awstasks/route.go Show resolved Hide resolved
upup/pkg/fi/cloudup/awstasks/subnet.go Outdated Show resolved Hide resolved
upup/pkg/fi/cloudup/awstasks/subnet.go Show resolved Hide resolved
pkg/apis/kops/validation/validation.go Show resolved Hide resolved
upup/pkg/fi/cloudup/awstasks/vpc.go Show resolved Hide resolved
@hakman hakman force-pushed the ipv6 branch 2 times, most recently from d9ea7f3 to fdfc533 Compare May 11, 2021 07:02
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 11, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 13, 2021
Name: s("subnet1"),
VPC: vpc1,
CIDR: s("172.20.1.0/24"),
IPv6CIDR: s("cidrsubnet-8-1"),
Copy link
Member Author

@hakman hakman May 13, 2021

Choose a reason for hiding this comment

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

@johngmyers @justinsb This is my proposed way of describing a subnet CIDR, using the network CIDR as a base. In this case, it is the equivalent of "2001:db8:0:1::/64.

Copy link
Member Author

@hakman hakman May 13, 2021

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I also don't mind to split this in a separate PR and continue the discussion there.

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer bikeshedding a syntax that looks more like a function call. Perhaps something like cidrsubnet(8,1).

I think a separate PR would help as the rest is a bit more baked.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, separate PR it is. :)

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 14, 2021
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 14, 2021
@hakman hakman force-pushed the ipv6 branch 2 times, most recently from 2733c93 to d768102 Compare May 17, 2021 06:06
@hakman hakman changed the title [WIP] Add initial support for configuring IPv6 with AWS Add initial support for configuring IPv6 with AWS May 17, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 17, 2021
upup/pkg/fi/utils/cidr.go Show resolved Hide resolved
pkg/apis/kops/validation/validation.go Outdated Show resolved Hide resolved
pkg/model/awsmodel/api_loadbalancer.go Show resolved Hide resolved
pkg/model/awsmodel/api_loadbalancer.go Show resolved Hide resolved
pkg/model/awsmodel/autoscalinggroup.go Outdated Show resolved Hide resolved
pkg/model/awsmodel/bastion.go Show resolved Hide resolved
@hakman
Copy link
Member Author

hakman commented May 18, 2021

I will squash / re-stage some of the commits after addressing all review comments and getting the OK.
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 18, 2021
pkg/apis/kops/validation/validation_test.go Show resolved Hide resolved
pkg/model/awsmodel/api_loadbalancer.go Outdated Show resolved Hide resolved
pkg/model/awsmodel/api_loadbalancer.go Outdated Show resolved Hide resolved
upup/pkg/fi/cloudup/awstasks/subnet.go Show resolved Hide resolved
upup/pkg/fi/cloudup/awstasks/vpc.go Show resolved Hide resolved
upup/pkg/fi/cloudup/awstasks/vpc.go Show resolved Hide resolved
upup/pkg/fi/cloudup/awstasks/vpcamazonipv6cidrblock.go Outdated Show resolved Hide resolved
@johngmyers
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 19, 2021
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 19, 2021
@hakman
Copy link
Member Author

hakman commented May 19, 2021

Thanks for the review @johngmyers. I restaged the commits to include the review feedback.
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 19, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 19, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johngmyers

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 19, 2021
@k8s-ci-robot k8s-ci-robot merged commit fe7d6e5 into kubernetes:master May 19, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone May 19, 2021
@hakman hakman deleted the ipv6 branch September 7, 2021 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/api area/provider/aws Issues or PRs related to aws provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants