cloudnativelabs / kube-router

Kube-router, a turnkey solution for Kubernetes networking.
https://kube-router.io
Apache License 2.0
2.33k stars 471 forks source link

fix: update peers on node update #1723

Open sebltm opened 3 months ago

sebltm commented 3 months ago

Fixes https://github.com/cloudnativelabs/kube-router/issues/676 by reloading the peer configuration when nodes are updated

aauren commented 3 months ago

Thanks @sebltm!

I won’t be able to test this out in my setup for a week or so do to IRL complications, but I’ll give it a go as soon as I’m able to.

FWIW on the surface this seems super simple and straight forward, if it’s really this simple I’ll be face-palming that we didn’t do this earlier 🙂

aauren commented 2 months ago

After reviewing your change and putting it through its paces a bit, I think that this is definitely a good start, but that there is more to the story here.

From what I can tell, putting in the nrc.OnNodeUpdate(newObj) will update remote nodes, but it would not work cohesively as that won't cause the node to reload its own annotations and settings. There are a bunch of settings for the node itself that are only considered either when the Routes controller is being created (https://github.com/cloudnativelabs/kube-router/blob/master/pkg/controllers/routing/network_routes_controller.go#L1424-L1682) or when it is being started (https://github.com/cloudnativelabs/kube-router/blob/master/pkg/controllers/routing/network_routes_controller.go#L1074-L1371)

Even in the functions that nrc.OnNodeUpdate() calls, there are a few bits of logic that specifically exclude the node that it is running on from being considered: https://github.com/cloudnativelabs/kube-router/blob/master/pkg/controllers/routing/bgp_peers.go#L56

I think that we're going to need a more holistic overhaul of the controller before #676 is resolved.

github-actions[bot] commented 5 days ago

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 10 days.