add loadbalancer address allocator #1501
Conversation
|
This seems really interesting. I'm hoping to get some time soon to take a look through it. |
|
Hey @whooo can you please rebase on prep-v2.0 again? prep-v2.0 was recently rebased on master to catch up on some bug fixes and dependency updates. I know its a bit annoying, but I try to keep prep-v2.0 merge-able with master so that they don't drift too far apart. |
Done |
There was a problem hiding this comment.
This was quite a bit of work, thanks for thinking of us and submitting it upstream @whooo! I know there is a lot of feedback on this, but at least half of it is stylistic and nitpics so hopefully it will be easy to move through.
I wanted to mention a lot of the things that I really liked about it as well:
- I liked your function granularity, it helped make the review easier as a lot of it was essentially self-documented via function name
- I liked how you made it idempotent and stateless via how you handle looking across ranges. This made the logic a bit more complex, but it means that you don't have to catch up on state when new leaders take over
After these changes are made, I'll do a functional review as a last pass where I exercise the controller a bit, but I don't expect many additional gotchas there. However, there may be 1 or 2 more asks on this PR. After that, we'll merge it.
Again, thanks for contributing!
whooo
force-pushed
the
lballoc
branch
2 times, most recently
from
9d72bbf
to
bc62958
Compare
There was a problem hiding this comment.
Alright, I got a chance to test it out. By in large, everything worked really great!
Having some experience with other LB allocation implementations, I noticed that this one wasn't able to handle self-assigned LB addresses like another implementation I've used. When I tried to debug this down, I found that apparently the status doesn't come through upon add (i.e. Kubernetes isn't passing it to the informer for a newly added service). Not sure how those other implementations achieved that, but people can always use externalIP for that use-case. So I think that we're good there unless you want to mention something in your documentation.
I added a note about it in the documentation, it would be possible to add some kind of support via annotations in the future |
|
So it looks like some of the unit tests are failing now. And I found one other small thing that we need to do. In the network_policy_controller we need to add some handling for this new range to ensure that it gets allow listed when the service resolves to the host. To make this a bit easier for you, I've created a list of code references below that show the handling of external IPs, you would basically just need to do the same with the load-balancer CIDRs:
|
This adds a simple controller that will watch for services of type LoadBalancer and try to allocated addresses from the specified IPv4 and/or IPv6 ranges. It's assumed that kube-router (or another network controller) will announce the addresses. As the controller uses leases for leader election and updates the service status new RBAC permissions are required.
I don't know why it failed, the checks passed on my computer, but looks good now
Done, more or less just did a copy and paste from the external IP parts |
|
LGTM! Thanks for all of your work on this @whooo! I'm sure that lots of people will be happy for the addition. |
This adds a simple controller that will watch for services of type LoadBalancer and try to allocated addresses from the specified IPv4 and/or IPv6 ranges. It's assumed that kube-router (or another network controller) will announce the addresses.
As the controller uses leases for leader election and updates the service status new RBAC permissions are required.
Tested on a small IPv6-only cluster, so IPv4/dual stack is not tested outside the unit tests.
Fixes #242