Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds basic support for advertising both v4 and v6 VIPs in a dual-stack cluster #1120

Conversation

sanjmonkey
Copy link

@sanjmonkey sanjmonkey commented Jul 2, 2021

A service can have multiple address from both v4 and v6 in a dual-stack cluster. Prior to this PR, only single-stack was really considered, and further, all VIP prefixes were masked with /32 which is not the intention for single-stack v6 clusters.

This PR lets you to advertise dual-stack service VIPs to upstream neighbours, and correctly masks the v6 VIPs in a single-stack v6 cluster.

In this implementation, for a node to be considered dual-stack it should have addresses in both address families in either NodeInternalIP or NodeExternalIP. The first of each of these addresses are chosen as the NH in the update to the upstream peer. You can do this by starting kubelet with --node-ip=<node_v4_address>,<node_v6_address>

Upstream neighbours dual-stack sessions can be initiated by annotating the node like this:

'kube-router.io/peer.ips=<neigh_v4_address>,<neigh_v6_address>'
'kube-router.io/bgp-local-addresses=<node_v4_address>,<node_v6_address>'
'kube-router.io/peer.asns=<upstream_asn>,<upstream_asn>'

@sanjmonkey
Copy link
Author

@aauren / @murali-reddy - Is this simple PR still useful to anyone, if so I'll rebase, if not feel free to close. I know its not much of a contribution in the feat that remains to support dual-stack everywhere else in KR, but likewise its not breaking and may help others get off the ground.

Note; We use this branch in production where KR does v4/v6 VIP advertisement only, and cilium for CNI / everything else.

@aauren
Copy link
Collaborator

aauren commented Mar 12, 2022

@sanjmonkey Thanks for submitting this and sorry for the late response! I think that you're right, that this doesn't hurt anything and it makes the IPv6 support a little more robust along the way.

Can you let me know how much testing have you done with these changes so far? Preferably, this patch would be tested on more than just a dev cluster and shown that it can handle the mileage.

Even if with just a dev cluster I think that we could accept this PR if you're willing to rebase and add a few things:

  • Add unit testing around the utility methods you added
  • Add unit testing around the v6 additions to the VIP and policy changes (we have fairly robust unit tests for this stuff, and it would be good to get this stuff tested so that as we make more v6 additions we can ensure we don't regress functionality)
  • Add documentation to the ipv6 documentation stating how to use this new functionality (basically the annotation bit you have there) as well as any limitations with this functionality in regards to the network_routes_controller.

I know that's a lot, so let me know if you're not up for it. However, I'm trying to keep active here again, so I should be able to give you a more timely review if you add those items.


// MatchAddressFamily compares 2 addresses families and returns true if they're the same, else false
func MatchAddressFamily(x net.IP, y net.IP) bool {
return x.To4() != nil && y.To4() != nil || x.To16() != nil && x.To4() == nil && y.To16() != nil && y.To4() == nil
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a neat 1-liner, but its really terse to read. It would probably be better broken out into individual if statements so it was more readable.


addresses := node.Status.Addresses
for i := range addresses {
if addresses[i].Type == apiv1.NodeInternalIP || addresses[i].Type == apiv1.NodeExternalIP {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpic, but usually for stuff like this where there isn't an else, I like to invert the condition and have it continue so that the rest of the logic can be outdented and IMO it makes it a bit more readable.

Something like:

if addresses[i].Type != apiv1.NodeInternalIP && addresses[i].Type != apiv1.NodeExternalIP {
  continue
}

@aauren
Copy link
Collaborator

aauren commented Oct 31, 2022

Replaced by #1386

@aauren aauren closed this Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants