events.action: opened
events.author: randomvariable
events.text: <!-- NOTE: ⚠️ For larger proposals, we follow the CAEP process as outlined in https://sigs.k8s.io/cluster-api/CONTRIBUTING.md. -->
  
  **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
events.title: Cluster condition, consumable by Infrastructure Providers for machine IP / pod & service CIDR overlap
events.type: issue
events.action: created
events.author: vincepri
events.text: /milestone v0.3.x
  /help
events.type: comment
events.action: created
events.author: vincepri
events.text: /milestone v0.4.0
events.type: comment
events.action: created
events.author: vincepri
events.text: /help
  /priority important-longterm
events.type: comment
events.action: created
events.author: iamemilio
events.text: /assign
  I would like to take a look at this
events.type: comment
events.action: created
events.author: fabriziopandini
events.text: What about having a validation webhook that prevents misconfiguration instead of reporting errors after the cluster is created?
events.type: comment
events.action: created
events.author: randomvariable
events.text: @fabriziopandini Could do, but would have to be infrastructure specific then, which is fine as well.
events.type: comment
events.action: created
events.author: randomvariable
events.text: @fabriziopandini would need validation webhooks performed by each infraprovider, since the cluster object has no idea of the infrastructure networks being used.
events.type: comment
events.action: created
events.author: randomvariable
events.text: Additionally in DHCP environments, even the infra provider doesn't know what the networks are.
events.type: comment
events.action: created
events.author: iamemilio
events.text: 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](https://github.com/kubernetes-sigs/cluster-api/blob/7fdecfe013260f6ca4dd5afa1ae72a37766f7506/api/v1alpha3/cluster_types.go#L67) 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
events.type: comment
events.action: created
events.author: randomvariable
events.text: 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.
events.type: comment
events.action: created
events.author: iamemilio
events.text: 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?
events.type: comment
events.action: created
events.author: iamemilio
events.text: @randomvariable for clarification, by infraprovider do you mean the underlying infra, or cluster-api-provider-foo?
events.type: comment
events.action: created
events.author: iamemilio
events.text: Can we define an interface version of our [crd validation webhooks](https://github.com/kubernetes-sigs/cluster-api/blob/9cbf8be8d643203d99aca6714825abdabcbca0b1/api/v1alpha3/cluster_webhook.go#L64) then have the cluster-api-providers implement it downstream?
events.type: comment
events.action: created
events.author: randomvariable
events.text: @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.
events.type: comment
events.action: created
events.author: iamemilio
events.text: 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. The big question is if this is worth adding a parameter to the cluster API.
events.type: comment
events.action: created
events.author: ncdc
events.text: 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.
events.type: comment
events.action: created
events.author: detiber
events.text: 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.
events.type: comment
events.action: created
events.author: randomvariable
events.text: 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.
events.type: comment
events.action: created
events.author: fabriziopandini
events.text: /remove-lifecycle stale
events.type: comment
data_with_bots.bot: 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](https://github.com/kubernetes/community).
  /lifecycle stale
data_with_bots.meta: This message directly preceded comment from bot *fejta-bot* by author *fabriziopandini*:
data_with_bots.text: /remove-lifecycle stale
data_without_bots.meta: This message directly preceded comment from bot *fejta-bot* by author *fabriziopandini*:
data_without_bots.text: /remove-lifecycle stale