Support Multiple Cluster CIDRs

[rahulkjoshi]

Issue: #2593

[k8s-ci-robot]

Welcome @rahulkjoshi!

It looks like this is your first PR to kubernetes/enhancements 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/enhancements has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @rahulkjoshi. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[aojea]

/cc

[aramase]

/cc

[rahulkjoshi]

/cc @sdmodi

[k8s-ci-robot]

@rahulkjoshi: GitHub didn't allow me to request PR reviews from the following users: sdmodi.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @sdmodi

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[danwinship]

Tying into what was discussed in the SIG meeting, it would be worth considering the possibility that instead of adding a new config object for extending the apiserver's --allocate-node-cidrs functionality, that instead we should deprecate --allocate-node-cidrs and add a new config object so that the network plugin can explain to other components how it is allocating pod IPs itself.

[sdmodi]

Right now the way this is scoped, we do not require existing Node IPAM allocators to populate what ranges have been allocated. If we did this, then it would force existing implementations to create a new object when a new range is allocated.

I view this as a good second step if there is consensus that this is a good idea. We can start with just requiring the object for input purposes and then enhance it to be consumable by other parts of the system

[danwinship]

oh, I just meant having the network plugin create an object/objects saying "The following CIDRs are in use by the pod network: ...". Not necessarily saying which specific subnets of it have been allocated where.

[danwinship]

I think a good user story is "I want to add more pods to a node after filling up its CIDR, but I don't want to have to kill and recreate the node's existing pods". This is something Calico (IIRC) supports, by letting you add additional CIDRs to a node rather than making you drop its existing CIDR before assigning it a new one.

(And yes, it would be complicated to add this to kcm, and it would require changing the semantics of the podCIDRs field on the node, etc. My point was more that this is something people are doing already, without having changed kcm, and maybe that's a better model.)

[thockin]

But this is NOT the story being told here. Unless I gravely misunderstand, this proposes multiple CIDRs for the cluster, but still one per node. (we probably want that TOO but it's not this KEP)

[sdmodi]

Yes. We are still saying only one per node.

We do need to decide whether it is a better model to let CNI implementations do this. The fundamental question is whether we want Kubernetes to do the IPAM or not. Today Kubernetes does do IPAM out of the box and it is lacking a lot of functionality. We are proposing we enhance these capabilities.

[thockin]

That is the extreme other end of the spectrum: "We never should have done this, we're sorry to have teased you, please do it yourself."

We'd still need a resource (or 2) and admission controller(s). Fundamentally the model would be the same, just not built-in.

I'm not outright against adding this as another built-in, as long as we can do it in reasonable complexity. It definitely pushed the bounds and we need to think hard about it.

[aojea]

just to confirm, migrating a node from single-stack to dual-stack is not possible, you have single-stack nodes, you have to create new nodes, right?

[rahulkjoshi]

That's correct. What I'm unsure of is whether we want to support migrating the cluster from single-stack to dual-stack while nodes are running.

For example, the model proposed here says that if any Node is in single-stack mode, the controller will only allocate single-stack CIDRs to new nodes even if there are IPv4 and IPv6 ClusterCIDRConfigs provided. The only way to migrate your cluster is to add both IPv4 and IPv6 ClusterCIDRConfigs and then delete all nodes. Then once you start adding nodes again, the controller will allocate them in dual-stack mode. Is this an acceptable workflow for us -- I have seen concern about mixing single-stack and dual-stack nodes hence this setup.

[aojea]

I will go for implementing something simple, and let the users do the heavylifting with the APIs we provide, they can delete nodes and reasign and deal with the pod migration, blah,blaa

[rahulkjoshi]

Agreed, I've reworked the section to that effect. Let me know if it sounds good.

[aojea]

gateway friends did this dance recently kubernetes-sigs/gateway-api#707 , we should ask them about the experience
cc: @robscott

[aojea]

we can omit this comment if we don't start with CRDs/custom controller #2594 (comment)

[rahulkjoshi]

Sounds like we can dodge this work 😄

[rahulkjoshi]

@thockin @aojea @mskrocki Let's get this finalized and merged in. The KEP deadline is fast approaching and we still need PRR sign-off. I think they stop accepting new requests soon.

[mskrocki]

LGTM

[thockin]

Thanks!

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rahulkjoshi, sdmodi, thockin

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:

• keps/sig-network/OWNERS [thockin]

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