kube-vip/kube-vip-cloud-provider

First and last IPs from CIDR block is ignored

0ljik opened this issue · 8 comments

0ljik commented

I have block of 4 IPs as cidr-global, but kube-vip-cloud-provider is throwing error after using 2nd and 3rd addresses:
Error syncing load balancer: failed to ensure load balancer: no addresses available in [envoy-gateway-system] cidr [{IP}/30]

lubronzhan commented

It's introduced by this PR https://github.com/kube-vip/kube-vip-cloud-provider/pull/69/files#diff-a166480b55dac4f8c491356fb0bd1930e511f677c59a01453e9dbb6493c8a5e8R26-R43

It assumes the first ip and last ip would be occupied by network ip and broadcast ip regardless how large the network range is. Previously we only skip .0 and .255.

I think we could use at least below 3 options:

  1. Document that this is expected, since the first and last ip could be occupied
  2. reverse to the original option, and if the user wants to ignore some IP, we add skip-ip-range-<namespace> option, if the user want to skip some ip, just add it here, then it could work with all kinds of customization of network if they want to use cidr
  3. Add a flag that skip-cidr-end-ips which set default to false, if we need to skip end ips then we ask the user to enable that. otherwise we won't skip the end ips, unless it's .0 and .255 (original behavior)
lubronzhan commented

On the second thought, it make more sense to exclude the first ip and last ip in cidr. If you just want a specific range to be allocated, you can use ip range instead. Let me update the doc

0ljik commented

On the second thought, it make more sense to exclude the first ip and last ip in cidr. If you just want a specific range to be allocated, you can use ip range instead. Let me update the doc

Can you explain why does it makes sense? I just want to understand. It's not a problem configuring by range, but sometimes you want to write it cleaner.)

Aren't these correct for CIDR Blocks with:

  • up to 24bit: always start with 0 and end with 255 with inbetween 0,255 addresses which also does need skipping.

  • 24bit: starts and ends with 0 and 255

  • 25 to 30: only these can use id or broadcast adresses other than 0 or 255.

I didn't read the code, but in my opinion it is only targeted for 25-30bit adresses and only if they are required to setup broadcast and id adresses

lubronzhan commented

MM, your point also make sense, when we specify cidr for allocating IP, who knows whether that cidr contains broadcast and id ip or not. Maybe we could have a flag to explicitly skip the end ips. And switch back to the original behavior.

Hi @wusendong, since you made this change https://github.com/kube-vip/kube-vip-cloud-provider/pull/69/files#diff-a166480b55dac4f8c491356fb0bd1930e511f677c59a01453e9dbb6493c8a5e8R26-R43, are you ok with above suggestion?

wusendong commented

MM, your point also make sense, when we specify cidr for allocating IP, who knows whether that cidr contains broadcast and id ip or not. Maybe we could have a flag to explicitly skip the end ips. And switch back to the original behavior.

Hi @wusendong, since you made this change https://github.com/kube-vip/kube-vip-cloud-provider/pull/69/files#diff-a166480b55dac4f8c491356fb0bd1930e511f677c59a01453e9dbb6493c8a5e8R26-R43, are you ok with above suggestion?

I had a similar confusion when I wrote that PR, when using range-<ns> it should be up to the user to ensure that it's full of usable IPs, and we shouldn't take it upon ourselves to remove .0 and .255.
So I agree more with Option 2 in your previous comment, let the user specify the list of IPs to skip, but remove the code that automatically skips .0 and .255 so that the semantics are intuitively expected by the normal user. And the automatically skips should only work with cidr-<ns> option which can be recognized with network knowledge.

lubronzhan commented

Thanks for the input, what about option 3, user might not always they have a networkid or broadcast ip when using cidr

wusendong commented

Thanks for the input, what about option 3, user might not always they have a networkid or broadcast ip when using cidr

Although option 3 makes new users confused, I think option 3 is acceptable because it maintains the compatibility of the original options.

lubronzhan commented

In the newest 0.0.10 release I reverted the default behavior to previous one and took option 3 as a workaround for now. Just a head up

  • 👍2