Add --ipv6 experimental cli flag #11629

hakman · 2021-05-30T08:53:05Z

/hold for #11523

upup/pkg/fi/cloudup/new_cluster.go

johngmyers · 2021-05-30T18:13:18Z

cmd/kops/create_cluster.go

@@ -260,6 +260,10 @@ func NewCmdCreateCluster(f *util.Factory, out io.Writer) *cobra.Command {
 	// TODO: Can we deprecate this flag - it is awkward?
 	cmd.Flags().BoolVar(&associatePublicIP, "associate-public-ip", false, "Specify --associate-public-ip=[true|false] to enable/disable association of public IP for master ASG and nodes. Default is 'true'.")

+	if featureflag.AWSIPv6.Enabled() {
+		cmd.Flags().BoolVar(&options.IPv6, "ipv6", false, "Add IPv6 CIDRs to AWS clusters with public topology")


I don't see any restriction to public topology.

Suggested change

cmd.Flags().BoolVar(&options.IPv6, "ipv6", false, "Add IPv6 CIDRs to AWS clusters with public topology")

cmd.Flags().BoolVar(&options.IPv6, "ipv6", false, "Allocate IPv6 CIDRs to subnets")

There is no restriction, but there is no allocation of subnets either.
At the moment, there is egress only internet gateway created so, private instances will have some trouble with internet access.

I'm failing to see why this shouldn't allocate IPv6 CIDRs to subnets in Private topology.

There is a separate task to provide IPv6 egress for the private route table. But even so, they should be able to use IPv6 to communicate within the VPC or out the Transit Gateway.

The node could not be bootstrapped because of the missing egress gateway.
I am trying to get this working with public topologies and then enable other use cases.
Other than that, this is a helper flag, for convenience. Once an always experiment by modifying the cluster config, where there isn't any limitation.

hakman · 2021-06-02T04:19:51Z

/retest

johngmyers · 2021-06-07T14:56:48Z

upup/pkg/fi/cloudup/new_cluster.go

+			if api.CloudProviderID(cluster.Spec.CloudProvider) == api.CloudProviderAWS {
+				klog.Warningf("IPv6 support is EXPERIMENTAL and can be changed or removed at any time in the future!!!")
+				for i := range cluster.Spec.Subnets {
+					cluster.Spec.Subnets[i].IPv6CIDR = fmt.Sprintf("/64#%x", i)


We should reserve /64#0 for the ClusterCIDR.

Sounds good to me, will change.

For future reference, read NonMasqueradeCIDR instead of ClusterCIDR. The latter is carved out of the former.

Yes, got it, though, we should discuss (again) about it. NonMasqueradeCIDR may go away soon as it's part of the dockershim implementation IIRC. Seems used just by the kubenet family.

The main use that is relevant here is that it's what populateClusterSpec.assignSubnets() carves the default ClusterCIDR and ServiceClusterIPRange out from. We could invent a new, better named field and have NonMasqueradeCIDR default to that.

johngmyers

/hold cancel

hakman · 2021-06-13T11:17:49Z

@johngmyers This should be good to go now. Please take another look.

johngmyers · 2021-06-13T15:46:48Z

cmd/kops/create_cluster.go

@@ -500,6 +504,10 @@ func RunCreateCluster(ctx context.Context, f *util.Factory, out io.Writer, c *Cr
 		cluster.Spec.NetworkCIDR = c.NetworkCIDR
 	}

+	if c.IPv6 {
+		cluster.Spec.NonMasqueradeCIDR = "fd00:10:96::/56"


Why this address? The 10:96 is not random as the spec requires.

If we are going to automatically assign the NonMasquereadeCIDR it should come from the VPC's IPv6 allocation. We should not automatically assign a ULA, especially if it isn't U.

For IPv4 we were using a fixed IP, but don't mind making it random.
What would be the benefit of using a block from the VPC IPv6 allocation?
Should I just remove this and discuss separately?
Is this a good case for making NonMasquereadeCIDR user configurable on create cluster?

Please remove this and discuss separately.

A benefit of using a block from the VPC IPv6 allocation is that one could then choose to make pods directly accessible from outside the cluster. Also, one wouldn't have to NAT the IPv6 outbound from pods, so the IPv6 destinations would be logging the IPv6 addresses of the pods.

I believe NonMasqueradeCIDR could be set using SpecOverride. Possible future work would be to default it to "/64#0" (and make that work).

FYI, making "/64#0" work for NonMasqueradeCIDR depends on work in my #9229 track. In that track I plan to refactor the writing of the completed cluster spec into a ManagedFile task. Once that is done, PopulateClusterSpec would need to be turned into a Task and made dependent on VPCAmazonIPv6CIDRBlock for managed VPCs.

k8s-ci-robot · 2021-06-13T20:27:15Z

[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:

~~OWNERS~~ [johngmyers]

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 requested review from johngmyers and justinsb May 30, 2021 08:53

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 30, 2021

johngmyers requested changes May 30, 2021

View reviewed changes

k8s-ci-robot assigned johngmyers May 30, 2021

hakman force-pushed the ipv6_experimental_flag branch from 014ec7c to 05415bb Compare June 2, 2021 04:04

johngmyers reviewed Jun 7, 2021

View reviewed changes

hakman mentioned this pull request Jun 9, 2021

Cannot know ipv6CIDR in advance, need to edit and resume install after IPv6 VPC created. #11720

Closed

hakman force-pushed the ipv6_experimental_flag branch from 05415bb to 754da7b Compare June 9, 2021 05:25

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/api and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 9, 2021

johngmyers approved these changes Jun 11, 2021

View reviewed changes

Pre-add integration test for creating an IPv6 cluster

fcfba36

hakman force-pushed the ipv6_experimental_flag branch from 754da7b to 9bf62f4 Compare June 13, 2021 11:14

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 13, 2021

johngmyers reviewed Jun 13, 2021

View reviewed changes

Add --ipv6 experimental cli flag

2a11fa7

hakman force-pushed the ipv6_experimental_flag branch from 9bf62f4 to 2a11fa7 Compare June 13, 2021 19:48

johngmyers approved these changes Jun 13, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 13, 2021

k8s-ci-robot merged commit b29c612 into kubernetes:master Jun 13, 2021

k8s-ci-robot added this to the v1.22 milestone Jun 13, 2021

hakman deleted the ipv6_experimental_flag branch June 14, 2021 03:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --ipv6 experimental cli flag #11629

Add --ipv6 experimental cli flag #11629

hakman commented May 30, 2021

johngmyers May 30, 2021

hakman Jun 2, 2021

johngmyers Jun 2, 2021 •

edited

Loading

hakman Jun 2, 2021

hakman commented Jun 2, 2021

johngmyers Jun 7, 2021

hakman Jun 8, 2021

johngmyers Jun 8, 2021

hakman Jun 8, 2021

johngmyers Jun 8, 2021 •

edited

Loading

johngmyers left a comment

hakman commented Jun 13, 2021

johngmyers Jun 13, 2021

johngmyers Jun 13, 2021

hakman Jun 13, 2021

johngmyers Jun 13, 2021

johngmyers Jun 13, 2021

k8s-ci-robot commented Jun 13, 2021

	cmd.Flags().BoolVar(&options.IPv6, "ipv6", false, "Add IPv6 CIDRs to AWS clusters with public topology")
	cmd.Flags().BoolVar(&options.IPv6, "ipv6", false, "Allocate IPv6 CIDRs to subnets")

Add --ipv6 experimental cli flag #11629

Add --ipv6 experimental cli flag #11629

Conversation

hakman commented May 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johngmyers Jun 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hakman commented Jun 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johngmyers Jun 8, 2021 • edited Loading

Choose a reason for hiding this comment

johngmyers left a comment

Choose a reason for hiding this comment

hakman commented Jun 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 13, 2021

johngmyers Jun 2, 2021 •

edited

Loading

johngmyers Jun 8, 2021 •

edited

Loading