Improve validation of PodCIDR and ServiceClusterIPRange

[johngmyers]

Fixes #15034

Removes the requirement (on non-GCE) that the ServiceClusterIPRange be within NonMasqueradeCIDR, as kube-proxy reroutes the ServiceClusterIPRange before masquerading.

Prohibits overlap between the PodCIDR and the ServiceClusterIPRange

Prohibits overlap between either the PodCIDR and the ServiceClusterIPRange and any subnet CIDR or IPv6CIDR. (except IPv6 doesn't have a PodCIDR, so can't overlap).

Adds validation of any podCIDR and requires it be within any NonMasqueradeCIDR.

[johngmyers]

/retest

[johngmyers]

/cc @justinsb

[hakman]

Shouldn't PodCIDR and ServiceClusterIPRange be both part of NonMasqueradeCIDR?

[johngmyers]

ServiceClusterIPRange doesn't have to be part of NonMasqueradeCIDR. That is because only things within the cluster network can talk to ServiceClusterIPRange and kube-proxy reroutes ServiceClusterIPRange before masquerading. GCE currently doesn't require ServiceClusterIPRange to be in NonMasqueradeCIDR; this PR drops that validation for all of the other cloud providers as well.

[hakman]

In this case, does is the NonMasqueradeCIDR still used for anything? It was dropped by kubelet when Docker shim was removed.

[johngmyers]

It's used by ContainerdBuilder and AWSCloudControllerManagerOptionsBuilder. It is also used as an exclusion for the egress proxy.

[johngmyers]

The use by AWSCloudControllerManagerOptionsBuilder looks to me like it's a bug. It should be using the PodCIDR instead.

[hakman]

Yup, same thought. Not sure about the other places, but I suspect we may get rid of it also.
The PR looks ok as is. We can merge it and see about NonMasqueradeCIDR later. Your choice.

[johngmyers]

We can handle AWSCloudControllerManagerOptionsBuilder as a separate PR. That one we might want to backport.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hakman

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 [hakman]

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

[hakman]

/retest

[minkimipt]

we are running a number of production clusters that were setup with kops 1.18 or even earlier on AWS. We’ve been upgrading kops and k8s over the last years and we are currently on kops 1.27 and k8s 1.25. We are expanding our infra into other regions and in aim to keep our older clusters upgraded too. We’ve recently kicked off creation of new clusters, that we do directly with kops 1.28. We are following the same subnetting scheme, that we used to apply for all of our clusters, but we’ve hit some problems in kops 1.28, which leaves us in doubt how to continue further. Here’s the particular problem that we are facing when trying to create a new cluster:

Error: completed cluster failed validation: spec.networking.serviceClusterIPRange: Forbidden: serviceClusterIPRange "10.22.128.0/17" must not overlap podCIDR "10.22.128.0/17"

Those subnets indeed overlap, but that hasn’t been an issue until kops 1.28 and we’ve been able to track that change down to this PR, which introduced the serviceClusterIPRange check, i.e.:

			if subnet.Overlap(podCIDR, serviceClusterIPRange) {
				allErrs = append(allErrs, field.Forbidden(fldPath.Child("serviceClusterIPRange"), fmt.Sprintf("serviceClusterIPRange %q must not overlap podCIDR %q", serviceClusterIPRange, podCIDR)))
			}

Since our clusters used to work well while those subnets were overlapping, could we make this check optional? Is there any risk adding a command line parameters that disables this check? Will we hit any issues in kops 1.28 and later and k8s 1.28 if this check is not done?

@johngmyers may I ask you to elaborate about the reason of this change?

[justinsb]

@minkimipt thanks for reporting and sorry about the issue. IIRC when we merged this we believed this was not supported / caused other problems. However, as it's a regression and existings clusters at least appear to be working fine, I think we should reduce this to a warning / disable it entirely while we track down exactly what goes wrong when these CIDRs overlap, and if it is indeed a problem, figure out how to move the CIDRs to be non-overlapping. I propose we track as #16340

[ebdekock]

@minkimipt thanks for reporting and sorry about the issue. IIRC when we merged this we believed this was not supported / caused other problems. However, as it's a regression and existings clusters at least appear to be working fine, I think we should reduce this to a warning / disable it entirely while we track down exactly what goes wrong when these CIDRs overlap, and if it is indeed a problem, figure out how to move the CIDRs to be non-overlapping. I propose we track as #16340

@justinsb @johngmyers we are running into the same issue, except for the subnet range. We are trying to update a cluster where the subnet and service IP range overlap. We are hitting this validation, preventing the update:

https://github.com/kubernetes/kops/pull/15623/files#diff-ae412ac68b83570fd50e9d5d63873f060eb8f503f5e44be78d710076294c3285R623-R625

I have the same questions here:

Since our clusters used to work well while those subnets were overlapping, could we make this check optional? Is there any risk adding a command line parameters that disables this check? Will we hit any issues in kops 1.28 and later and k8s 1.28 if this check is not done?

I see we've reverted the pod overlap validation: a1bba9d
Are the subnets different or would the same be possible?

[minkimipt]

@ebdekock we were able to upgrade kops to 1.28.5 where that validation was disabled in a cluster where we had those networks overlapping. Just to confirm that it's not a problem anymore.

[ebdekock]

@ebdekock we were able to upgrade kops to 1.28.5 where that validation was disabled in a cluster where we had those networks overlapping. Just to confirm that it's not a problem anymore.

I was waiting for the 1.28.5 release, as I thought it would also fix my issue, but our overlap is with the subnet CIDR and not the PodCIDR

[johngmyers]

The issue is that if the podCDR and serviceClusterIPRange overlap, then a pod could be assigned the same IP as a service. Then traffic intended for the pod would be routed to the wrong place.

Similarly, if the subnet range overlaps with the serviceClusterIPRange, then a node could be assigned the same IP as a service, causing routing problems for host-network pods.

I would say the "appear to be working fine" is merely that problems are rarely detected, despite being there. I would not be in favor of weakening the validations and would suggest the clusters' networking configurations be fixed.

[ebdekock]

I would say the "appear to be working fine" is merely that problems are rarely detected, despite being there. I would not be in favor of weakening the validations and would suggest the clusters' networking configurations be fixed.

Yup, that makes total sense and we agree that fixing the underlying issue is the best way forward. We would like to get the cluster updated and need to prioritize getting the networking fixed. Would it be possible to get this validation behind a temporary flag (that's off by default) to give us time to correct the issue?

[ebdekock]

I would say the "appear to be working fine" is merely that problems are rarely detected, despite being there. I would not be in favor of weakening the validations and would suggest the clusters' networking configurations be fixed.

We have fixed the networking like you suggested and no longer need this, thanks!