Add strict validation for CIDR ranges in the Cluster webhook

killianmuldoon commented 1 year ago

Add a check in the Cluster webhook to ensure each CIDR block only contains valid CIDR blocks with the following rules:

No more than two CIDR blocks are specified under Pods or Services
If two are specified the blocks need to be from different IP families i.e. one IPv4 and one IPv6
The IPFamily for pods and services must be compatible
The CIDR ranges are valid CIDR ranges

This change ensures Clusters can not be created or updated with invalid CIDR blocks. This is the value that the Kubernetes control plane components take - e.g. the kube-apiserver flag --service-cluster-ip-range is documented:

A CIDR notation IP range from which to assign service cluster IPs. This must not overlap with any IP ranges assigned to nodes or pods. Max of two dual-stack CIDRs is allowed.

/kind feature /area api /kind api-change

fabriziopandini commented 1 year ago

/triage accepted just as a reference the upstream code where Kubernetes components validate CIDR

Services: https://github.com/kubernetes/kubernetes/blob/6dc81c3280996a9a4267bf8ef30d4bda36f8a293/cmd/kube-apiserver/app/options/validation.go#L47-L69

Pods: https://github.com/kubernetes/kubernetes/blob/08749750a96fe7f236befa40076a3117e7b36733/staging/src/k8s.io/cloud-provider/app/core.go#L127-L133

(see https://github.com/kubernetes-sigs/cluster-api/pull/7420#issuecomment-1290764699)

fabriziopandini commented 1 year ago

note: this is initially considered an API change because currently we are not prescriptive on what Cluster.Network CIDRs are for, we saw some borderline usage of that info, and we want to move to a place where it is clear that those values are meant for defining values to be passed to Kubernetes components only, so it makes sense to apply the same validations rules /help

k8s-ci-robot commented 1 year ago

@fabriziopandini: This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

Why are we solving this issue?
To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
Does this issue have zero to low barrier of entry?
How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed by commenting with the /remove-help command.

In response to [this](https://github.com/kubernetes-sigs/cluster-api/issues/7538): >note: this is initially considered an API change because currently we are not prescriptive on what Cluster.Network CIDRs are for, we saw some borderline usage of that info, and we want to move to a place where it is clear that those values are meant for defining values to be passed to Kubernetes components only, so it makes sense to apply the same validations rules >/help Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

viveksyngh commented 1 year ago

/assign

viveksyngh commented 1 year ago

I would like to help with this.

killianmuldoon commented 1 year ago

@viveksyngh Thanks for the interest! But this issue isn't ready to be worked on yet - we've added the label api-change which means we can't add this until we have a new API version, which I don't think there's been any planning for as of yet. That means this work is at least months away.

/help remove

(to avoid confusion)

viveksyngh commented 1 year ago

Thanks for the info @killianmuldoon

sbueringer commented 1 year ago

Just rememberd something from a different life :).

The "normal" use case is that the Pod CIDRs are passed through to the kube-controller-manager. The kube-controller-manager takes this CIDR and splits it up to assign Node CIDRs to Nodes. Then the "host-local" IPAM plugin on the nodes assigns IPs from the Node CIDR to individual Pods.

I don't remember how this works e.g. when Calico takes over the IPAM part:

does the kube-controller-manager still implement the Node CIDR IPAM part (and calico only per-Pod IPAM)
is the CIDR configured in Calico, how does this relate to the one in kube-controller-manager

Just wondering if there are scenarios where the baked in kube-controller-manager validation doesn't matter because the whole Node/Pod IPAM stuff is outsourced to e.g. something like Calico.

k8s-triage-robot commented 8 months ago

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

Confirm that this issue is still relevant with /triage accepted (org members only)
Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

fabriziopandini commented 5 months ago

/priority important-longterm

fabriziopandini commented 4 months ago

In pipeline for the next API version

/triage accepted /remove-priority important-longterm /priority backlog

kubernetes-sigs / cluster-api

Add strict validation for CIDR ranges in the Cluster webhook #7538

Guidelines