⚠️ Add strict validation for CIDR ranges specified in Clusters #7420

killianmuldoon · 2022-10-17T18:45:14Z

Signed-off-by: killianmuldoon [email protected]

Add a check in the Cluster webhook to ensure each CIDR block only contains valid CIDR blocks.

/kind bug

Fixes: #7358

internal/webhooks/cluster.go

killianmuldoon · 2022-10-17T18:53:19Z

@marvinWolff - it would be great to get your input on this.

killianmuldoon · 2022-10-17T18:55:50Z

flake?

/retest

fabriziopandini · 2022-10-17T19:03:36Z

@jayunit100 PTAL if you have some time. We are discussing validation rules for CIDR in ClusterNetwork (ServiceCIDRs and Node/PodsCIDR)

killianmuldoon · 2022-10-18T10:38:00Z

I've updated the implementation to match that done by kubeadm:

The following is validated in the webhook:

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 is done using the Cluster's GetIPFamily method. This is currently used in CAPI in the topology controller and in CAPD.

This change could be breaking for some providers if they were using these values for something other than passing them directly to Kubernetes e.g. informing an IP pool for some IPAM CNI, so I think we might need to be careful with including this change. That may be the case in #7358

The base problem is that validation on this field is done differently depending on whether or not users are using ClusterClass. If it's better to leave webhook validation as is, then validation should no longer be done when computing variables on the ClusterClass to make it consistent.

killianmuldoon · 2022-10-18T10:39:00Z

/hold

For more input from providers on the blast radius of this change

vincepri

Minor comments, rest lgtm

api/v1beta1/cluster_types.go

vincepri · 2022-10-19T17:22:56Z

We've discussed during the Cluster API office hours meeting on Oct 19th and decided by consensus that we should upgrade this PR to a ⚠️ (breaking change) and go ahead and merge the stricter validations, given that's what ultimately Kubernetes expects.

@killianmuldoon will send an email to the mailing list.

marvinWolff · 2022-10-20T06:45:21Z

Hello @killianmuldoon,

sorry for the late reply.

This looks good to me. Validating what kubernetes expects is a good idea to prevent creating clusters with a basically invalid configuration like we did.

We also solved our issue in the meantime by specifiying the whole ip-range and using calico ip-pools.

killianmuldoon · 2022-10-20T08:43:36Z

Thanks @marvinWolff can you share which providers you were using with the original config? Just wondering how the original version you had was working initially.

killianmuldoon · 2022-10-25T15:39:50Z

I did some digging into how Kubernetes validates these IP ranges (and which components use which) across the core components.

The conclusion is that these ranges are assumed to be either a single entry or dualstack if there are two entries. Supplying more than two CIDR ranges is not supported. With this in mind I think the validation approach in this PR is correct.

Details:

Kube Proxy

ClusterCIDR from flag cluster-cidr. CIDR range of pods in cluster.
Validated so there can not be more than two entries. If there are two entries they must be dualstack.
Ref: https://github.com/kubernetes/kubernetes/blob/7df6c02288833c7be66f425e03c1e858d2343f1f/pkg/proxy/apis/config/validation/validation.go#L73

Kube APIServer

ServiceClusterIPRanges from flag service-cluster-ip-range. CIDR range of services in the cluster.
Validated so there can not be more than two entries. If there are two entries they must be dualstack.
Ref: https://github.com/kubernetes/kubernetes/blob/6dc81c3280996a9a4267bf8ef30d4bda36f8a293/cmd/kube-apiserver/app/options/validation.go#L45

Kube Controller Manager

ClusterCIDR from flag cluster-cidr. CIDR range of pods in cluster. Validates so there can not be more than two entries. If there are two entries they must be dualstack.
Ref: https://github.com/kubernetes/kubernetes/blob/08749750a96fe7f236befa40076a3117e7b36733/staging/src/k8s.io/cloud-provider/app/core.go#L102

ServiceCIDR from flag service-cluster-ip-range. CIDR range for Services in Cluster. Takes the first two values given, but doesn't validate if more than two are given, instead just ignoring additional values. If there are two entries they must be dualstack.

Ref: https://github.com/kubernetes/kubernetes/blob/7b478d15d5838e14b8286c9b541c8cbd626cbe3c/cmd/kube-controller-manager/app/options/nodeipamcontroller.go#L51

Kubelet

PodCIDR from flag pod-cidr string. Only used in standalone mode, not relevant here.

Kube Scheduler

No CIDR related flags.

killianmuldoon · 2022-11-09T15:43:16Z

/hold

Want to bring this up one more time at office hours today - we can merge after that.

elmiko · 2022-11-09T18:37:38Z

as discussed in the community meeting on 9 November, we'd like to get more comments on this before merging. please review the defaults as described in #7521 (comment) , these will be encoded by this PR.

if there are no additional comments or objections by 11 November, we will merge this.

sbueringer · 2022-11-10T13:37:53Z

@killianmuldoon Can you rebase when you have some time?

sbueringer · 2022-11-11T06:10:45Z

Thank you very much!!

/lgtm

sbueringer · 2022-11-11T06:10:51Z

/assign @fabriziopandini

fabriziopandini

I like the fact that we are not making IpFamily a first-class concern given that it is CAPD specific.
I have one last question on validation to make sure we are not blocking use cases allowed upstream (like the one in the attached issue).

fabriziopandini · 2022-11-11T16:43:28Z

internal/webhooks/cluster.go

@@ -443,3 +470,40 @@ func machineDeploymentClassOfName(clusterClass *clusterv1.ClusterClass, name str
 	}
 	return nil
 }
+
+// validateCIDRBlocks validates that:
+// 1) No more than two CIDR blocks are specified.


Is this a Kubernetes limitation that applies both to service and pod cidr?
same for 2

If not we are not sure I will simply validate all the CIDR are valid, without making assumptions on the combination of them in this first iteration

Both

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 #7420 (comment))

killianmuldoon · 2022-11-14T10:18:24Z

/remove-hold

fabriziopandini · 2022-11-14T13:57:56Z

/lgtm
Thanks for opening the follow-up issue, appreciated

sbueringer

one nit, otherwise lgtm

internal/webhooks/cluster.go

Signed-off-by: killianmuldoon <[email protected]>

sbueringer · 2022-11-14T15:19:03Z

Thx!

/lgtm
/approve

k8s-ci-robot · 2022-11-14T15:19:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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~~ [sbueringer]

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 added kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 17, 2022

k8s-ci-robot requested review from CecileRobertMichon and fabriziopandini October 17, 2022 18:45

killianmuldoon commented Oct 17, 2022

View reviewed changes

internal/webhooks/cluster.go Outdated Show resolved Hide resolved

CecileRobertMichon mentioned this pull request Oct 17, 2022

Use Helm to install Calico CNI in e2e tests instead of ClusterResourceSets kubernetes-sigs/cluster-api-provider-azure#2495

Merged

3 tasks

killianmuldoon changed the title ~~🐛 Add validation for number of CIDR ranges specified in Clusters~~ 🐛 [WIP] Add validation for number of CIDR ranges specified in Clusters Oct 17, 2022

killianmuldoon force-pushed the webhook/add-cidr-validation branch 3 times, most recently from 19fa536 to 03f6846 Compare October 18, 2022 10:22

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 18, 2022

killianmuldoon force-pushed the webhook/add-cidr-validation branch from 03f6846 to cfe6ea5 Compare October 18, 2022 10:29

killianmuldoon changed the title ~~🐛 [WIP] Add validation for number of CIDR ranges specified in Clusters~~ 🐛 Add validation for number of CIDR ranges specified in Clusters Oct 18, 2022

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 18, 2022

killianmuldoon mentioned this pull request Oct 18, 2022

Can't delete an invalid cluster #7423

Closed

vincepri reviewed Oct 18, 2022

View reviewed changes

api/v1beta1/cluster_types.go Outdated Show resolved Hide resolved

api/v1beta1/cluster_types.go Outdated Show resolved Hide resolved

killianmuldoon force-pushed the webhook/add-cidr-validation branch 2 times, most recently from b05d675 to 6849902 Compare October 18, 2022 14:59

killianmuldoon changed the title ~~🐛 Add validation for number of CIDR ranges specified in Clusters~~ ⚠️ Add validation for number of CIDR ranges specified in Clusters Oct 19, 2022

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 10, 2022

killianmuldoon force-pushed the webhook/add-cidr-validation branch from 8808d54 to a1e0ee5 Compare November 10, 2022 19:04

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 10, 2022

killianmuldoon force-pushed the webhook/add-cidr-validation branch from a1e0ee5 to ed74c64 Compare November 10, 2022 19:08

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 10, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 11, 2022

fabriziopandini reviewed Nov 11, 2022

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 14, 2022

killianmuldoon force-pushed the webhook/add-cidr-validation branch from ed74c64 to 84ee36d Compare November 14, 2022 12:58

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 14, 2022

killianmuldoon force-pushed the webhook/add-cidr-validation branch from 84ee36d to 0d1bed7 Compare November 14, 2022 13:04

killianmuldoon mentioned this pull request Nov 14, 2022

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

Open

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 14, 2022

sbueringer reviewed Nov 14, 2022

View reviewed changes

internal/webhooks/cluster.go Outdated Show resolved Hide resolved

Add validation for number of CIDR ranges specified in Clusters

bef7d72

Signed-off-by: killianmuldoon <[email protected]>

killianmuldoon force-pushed the webhook/add-cidr-validation branch from 0d1bed7 to bef7d72 Compare November 14, 2022 15:05

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 14, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 14, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 14, 2022

k8s-ci-robot merged commit f9fcf3f into kubernetes-sigs:main Nov 14, 2022

k8s-ci-robot added this to the v1.3 milestone Nov 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠️ Add strict validation for CIDR ranges specified in Clusters #7420

⚠️ Add strict validation for CIDR ranges specified in Clusters #7420

killianmuldoon commented Oct 17, 2022 •

edited

Loading

killianmuldoon commented Oct 17, 2022

killianmuldoon commented Oct 17, 2022

fabriziopandini commented Oct 17, 2022

killianmuldoon commented Oct 18, 2022

killianmuldoon commented Oct 18, 2022

vincepri left a comment

vincepri commented Oct 19, 2022 •

edited

Loading

marvinWolff commented Oct 20, 2022 •

edited

Loading

killianmuldoon commented Oct 20, 2022

killianmuldoon commented Oct 25, 2022

killianmuldoon commented Nov 9, 2022

elmiko commented Nov 9, 2022 •

edited

Loading

sbueringer commented Nov 10, 2022

sbueringer commented Nov 11, 2022

sbueringer commented Nov 11, 2022

fabriziopandini left a comment

fabriziopandini Nov 11, 2022 •

edited

Loading

fabriziopandini Nov 11, 2022 •

edited

Loading

sbueringer Nov 11, 2022

killianmuldoon commented Nov 14, 2022

fabriziopandini commented Nov 14, 2022

sbueringer left a comment

sbueringer commented Nov 14, 2022

k8s-ci-robot commented Nov 14, 2022

⚠️ Add strict validation for CIDR ranges specified in Clusters #7420

⚠️ Add strict validation for CIDR ranges specified in Clusters #7420

Conversation

killianmuldoon commented Oct 17, 2022 • edited Loading

killianmuldoon commented Oct 17, 2022

killianmuldoon commented Oct 17, 2022

fabriziopandini commented Oct 17, 2022

killianmuldoon commented Oct 18, 2022

killianmuldoon commented Oct 18, 2022

vincepri left a comment

Choose a reason for hiding this comment

vincepri commented Oct 19, 2022 • edited Loading

marvinWolff commented Oct 20, 2022 • edited Loading

killianmuldoon commented Oct 20, 2022

killianmuldoon commented Oct 25, 2022

Kube Proxy

Kube APIServer

Kube Controller Manager

Kubelet

Kube Scheduler

killianmuldoon commented Nov 9, 2022

elmiko commented Nov 9, 2022 • edited Loading

sbueringer commented Nov 10, 2022

sbueringer commented Nov 11, 2022

sbueringer commented Nov 11, 2022

fabriziopandini left a comment

Choose a reason for hiding this comment

fabriziopandini Nov 11, 2022 • edited Loading

Choose a reason for hiding this comment

fabriziopandini Nov 11, 2022 • edited Loading

Choose a reason for hiding this comment

sbueringer Nov 11, 2022

Choose a reason for hiding this comment

killianmuldoon commented Nov 14, 2022

fabriziopandini commented Nov 14, 2022

sbueringer left a comment

Choose a reason for hiding this comment

sbueringer commented Nov 14, 2022

k8s-ci-robot commented Nov 14, 2022

killianmuldoon commented Oct 17, 2022 •

edited

Loading

vincepri commented Oct 19, 2022 •

edited

Loading

marvinWolff commented Oct 20, 2022 •

edited

Loading

elmiko commented Nov 9, 2022 •

edited

Loading

fabriziopandini Nov 11, 2022 •

edited

Loading

fabriziopandini Nov 11, 2022 •

edited

Loading