search

kubernetes-sigs / cluster-api

Home for Cluster API, a subproject of sig-cluster-lifecycle
https://cluster-api.sigs.k8s.io
Apache License 2.0
3.5k stars 1.3k forks source link

Cluster condition, consumable by Infrastructure Providers for machine IP / pod & service CIDR overlap #3296

Closed randomvariable closed 3 years ago

randomvariable commented 4 years ago

User Story

As a operator, I deploy a cluster in my infrastructure provider. The infrastructure has a CIDR range of 192.168.0.0/24, and I set a pod and service CIDR range of 192.168.0.0/16.

I see intermittent communication failures, but Cluster API hasn't told me what is wrong.

Detailed Description

Cluster API should inform a user that there is overlap between their infrastructure networking and that for the pods and service CIDRs, such that it could cause communication issues. This ideally would be a condition on the Cluster resource, that infrastructure providers could inform.

Anything else you would like to add:

[Miscellaneous information that will assist in solving the issue.]

/kind feature

vincepri commented 4 years ago

/milestone v0.3.x /help

k8s-ci-robot commented 4 years ago

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

Please ensure the request meets the requirements listed here.

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/3296): >/milestone v0.3.x >/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.
vincepri commented 4 years ago

/milestone v0.4.0

vincepri commented 4 years ago

/help /priority important-longterm

iamemilio commented 3 years ago

/assign I would like to take a look at this

fabriziopandini commented 3 years ago

What about having a validation webhook that prevents misconfiguration instead of reporting errors after the cluster is created?

randomvariable commented 3 years ago

@fabriziopandini would need validation webhooks performed by each infraprovider, since the cluster object has no idea of the infrastructure networks being used.

randomvariable commented 3 years ago

Additionally in DHCP environments, even the infra provider doesn't know what the networks are.

iamemilio commented 3 years ago

Excuse my Newbieness, but do we need to dynamically check the network CIDR range? For this validation check, why should we not just assume that the CIDR ranges the users entered here are valid, and just check if they overlap?

If we want to validate the CIDR ranges themselves, I think that should be a separate step, and should be handled by the infra providers

randomvariable commented 3 years ago

We don't know the network range of the infrastructure from that information, which is the main problem that pops up. That struct only tells us the CNI networking.

Should probably be along the lines of comparing Machine.Status.Addresses vs. the CIDRs in the struct above, and add a condition representing a clash.

iamemilio commented 3 years ago

Thanks, that makes more sense. My next question would be why a validation webhook? Do we do a simlar things for other validation steps? Do we have a process for validating configs in general?

iamemilio commented 3 years ago

@randomvariable for clarification, by infraprovider do you mean the underlying infra, or cluster-api-provider-foo?

iamemilio commented 3 years ago

Can we define an interface version of our crd validation webhooks then have the cluster-api-providers implement it downstream?

randomvariable commented 3 years ago

@iamemilio There's no guarantee an infra provider knows what the infrastructure network will be ahead of time, so it can't happen via a webhook.

If we take vSphere or bare metal as an example, you may very well be putting a machine in an L2 network with some external DHCP. There's nothing on the machine specification that mentions an IP address at that time.

iamemilio commented 3 years ago

Given those restrictions, the only way this can really work on all platforms the same way is to check the ip assigned to each machine against the service and pods network CIDRS and throw and error or warning in the machine-controller logs if they overlap. Is that an acceptable solution?

Another option to consider is how things are handled in OpenShift. We require admins to do a little legwork upfront and tell us the CIDR of the network our nodes will get their IPs from. This makes the validation easy and upfront. In the CAPI case, this would mean that we would add another optional param to the ClusterNetwork object: Machines *NetworkRanges. If this is provided, we can quickly check for overlap with a webhook validation on the cluster object. The big question is if this is worth adding a parameter to the cluster API.

ncdc commented 3 years ago

and throw and error or warning in the machine-controller logs

Logs are not user-facing. The only available user-facing status indicator is the conditions array in status. The original description above suggests making this a condition in Cluster status.

I'll defer to @detiber @CecileRobertMichon @randomvariable etc. for thoughts on the suggested new API field.

detiber commented 3 years ago

I'm not necessarily sure I'm a fan of adding a field for this unless we have a comprehensive proposal around it, how it would impact providers, and how it would impact users.

I want to make sure we aren't creating a situation where it's really easy for a user to shoot themselves in the foot, and that we aren't necessarily imposing requirements on providers that are unreasonable.

For example, with AWS, I could easily see us being able to report the network range in use by the VPC (either cluster-api managed or user-provided) in a way that the Cluster resource could verify things (likely not at admission time, but could still short circuit the feedback loop on conditions before things get too far along). I'm not sure how feasible that approach would be for vSphere, metal3, packet, or other types of bare metal'ish providers.

Also depending on the use case we are talking about (such as a managed service provider model), it may not be reasonable to expect the end user to know what network address block will be used in advance.

randomvariable commented 3 years ago

Agree with @detiber .

Just to clarify the use case for having a condition on the cluster object:

Having a tool like the at-the-glance status in https://github.com/kubernetes-sigs/cluster-api/issues/3802 could show the problem across infrastructure providers.

Concretely, just thinking of a defined ConditionType constant for the condition and leaving the rest to infra providers.

fejta-bot commented 3 years ago

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta. /lifecycle stale

fabriziopandini commented 3 years ago

/remove-lifecycle stale

fejta-bot commented 3 years ago

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale

fejta-bot commented 3 years ago

Stale issues rot after 30d of inactivity. Mark the issue as fresh with /remove-lifecycle rotten. Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten

fejta-bot commented 3 years ago

Rotten issues close after 30d of inactivity. Reopen the issue with /reopen. Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community. /close

k8s-ci-robot commented 3 years ago

@fejta-bot: Closing this issue.

In response to [this](https://github.com/kubernetes-sigs/cluster-api/issues/3296#issuecomment-860282698): >Rotten issues close after 30d of inactivity. >Reopen the issue with `/reopen`. >Mark the issue as fresh with `/remove-lifecycle rotten`. > >Send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community). >/close 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.