Advertise Load Balancer IPs like External IPs

[salanki]

MetalLB can currently not be used with Calico as it sets a LoadBalancer Ingress IP instead of an External IP. By advertising Ingress IPs as well as External IPs MetalLB can work with Calico without additional BGP peers.

The linked issue suggests enabling this functionality with a config flag. I'm happy to implement that with some pointers on how to add one.

Tested in production. A calico-node image based on 3.16 can be pulled at: salanki/calico:node-advertiselb

Fixes: https://github.com/projectcalico/confd/issues/301
Fixes: metallb/metallb#114

[CLAassistant]

All committers have signed the CLA.

[caseydavenport]

@salanki thanks for the PR!

I think we'll want to plumb through a new API field to enable / disable advertisement of these addresses.

To do that, we'll want to add a new field to the BGPConfiguration API - likely called serviceLoadbalancerIPs - similar to this: projectcalico/libcalico-go#1123

Once that is in, we can update this PR to import the new libcalico and only advertise loadbalancer IPs if they are within an allowed CIDR.

What do you think? The main thing we want to do is enable users to control whether or not this feature is enabled, and if it is enabled which IP ranges are allowed to be advertised into the network by Calico.

[salanki]

@caseydavenport: Sounds fair. I'll take a stab at it.

[gclawes]

It looks like this is using isAllowedExternalIP. There was discussion in #301 about adding a new serviceLoadBalancerIPs field to the BGPConfiguration resource to whitelist LB IPs separately from ExternalIP whitelisting. Should this PR include that extension?

[gclawes]

There's also the fact that svc.Status.LoadBalancer.Ingress entries are not necessarily IP addresses. This is usually in the case of cloud providers, but that's not a guarantee.

AWS:

status:
  loadBalancer:
    ingress:
    - hostname: <LB UUID>.elb.us-east-1.amazonaws.com

MetalLB:

$ kubectl -n kube-system get svc/ingress-nginx-controller -o yaml | tail -4
status:
  loadBalancer:
    ingress:
    - ip: 10.102.0.0

[mtryfoss]

Interesting.
I tried putting the address (single /32) assigned to MetalLB in as serviceExternalIPs and got it advertised to our routers.
Unexpected behaviour?

[gclawes]

I think it will "work" as it is right now, but I don't think it's a complete implementation.

ExternalIP is a different concept in a k8s Service object than the IPs in .status.loadBalancer.ingress, I think these should be separate concepts within the BGPConfiguration API object.

https://kubernetes.io/docs/concepts/services-networking/service/#loadbalancer
https://kubernetes.io/docs/concepts/services-networking/service/#external-ips

[caseydavenport]

@salanki have you had a chance to look at adding an API for this?

If not, I may have some time to help with that part very soon.

[caseydavenport]

I've got a PR for the API bits here: projectcalico/libcalico-go#1349

[salanki]

Thank you @caseydavenport. I have been running the confd change in production (1,000 nodes, 100
LB services) since this PR and it’s been rock solid. Regretfully the Calico rollout needed a lot of work in other areas that took my time from doing the API bit.

[gclawes]

This PR should probably be modified to match the new LB API.

[salanki]

@gclawes link to what you are referring to?

[gclawes]

@caseydavenport's change in projectcalico/libcalico-go#1349 adds serviceLoadBalancerIPs to the BGPConfiguration object.

This PR uses isAllowedExternalIP, which checks the serviceExternalIPs fields in BGPConfiguration. In order to properly use serviceLoadBalancerIPs there needs to be a method to check that, something like isAllowedLoadBalancerIP, and probably further changes that I'm not aware of to support that.

[salanki]

Oh of course. I thought you meant something completely different (a new Kubernetes LB API) for some reason.

[caseydavenport]

@salanki FYI my PR in libcalico has been merged and the pin updated here in confd, so you should be able to pull it in to this PR now with a rebase.

[caseydavenport]

@salanki hope you don't mind, but I had some time to work on this and I'd really like to see this in v3.18, so I've built on-top of your commit here: #422

I'm going to close this PR for now and continue working there.

[salanki]

Thank you for bringing this home @caseydavenport!