IP addresses are not removed from the loopback adapter

[an0nz]

When existing is an empty set as --label=something has not been defined, IP's are not removed from the loopback adapter and the for loop does not trigger as toremove becomes an empty set

exabgp/src/exabgp/application/healthcheck.py

Line 263 in 925f493

toremove = set([ip_network(ip) for net in ips for ip in net]) & existing

I have added some debugging and changed the code as per below to demonstrate what is going on
Debug Changes / Fix

def remove_ips(ips, label, sudo=False):
    """Remove added IP on loopback interface"""
    existing = set(loopback_ips(label, True))
    logger.info("ips= %s", ips)
    logger.info("existing= %s", existing)
    # Get intersection of IPs (ips setup, and IPs configured by ExaBGP)
    toremove = set([ip_network(ip) for net in ips for ip in net]) & existing
    logger.info("toremove1= %s", toremove)
    toremove = set([ip_network(ip) for net in ips for ip in net]) | existing
    logger.info("toremove2= %s", toremove)

Resulting Log Entries

healthcheck[3678046]: ips= [IPv4Network('172.16.0.50/32')]
healthcheck[3678046]: existing= set()
healthcheck[3678046]: toremove1= set()
healthcheck[3678046]: toremove2= {IPv4Network('172.16.0.50/32')}

healthcheck[3678046]: Remove loopback IP address 114.23.24.79/32

Fix
Either use | existing instead of & existing for joining the sets, or if only the IP's matching the label should be removed then handle when label=None

Workaround
Use --label=ip1 or similar

[thomas-mangin]

@vincentbernat I assume you are busy, but I would appreciate it if you could give this issue a look when time allows. The comment above the code indicates that the intersection is what was expected:

    # Get intersection of IPs (ips setup, and IPs configured by ExaBGP)
    toremove = set([ip_network(ip) for net in ips for ip in net]) & existing

Thank you.

[vincentbernat]

This is not the first time we have a modification on this code. For me, this is &. We want to remove the IPs that are currently configured on the loopback and should be setup on the loopback. If there is something to fix, this is the construction of the existing set. Otherwise, we would remove IP addresses configured.

[an0nz]

Thanks @vincentbernat for the explanation, it makes sense to use intersection so commands aren't run to remove IP's that do not exist on the adapter.

When I submitted I hadn't yet worked out what & did when joining sets in python as my python knowledge is poor just trying to help after running into the issue with no labels set.

The current implementation is more accurate since it checks ips exist before trying to remove them, the problem in that case looks to be as existing enforces labels when set using existing = set(loopback_ips(label, True)) then loopback_ips needs to handle returning only ips where they have no label set if label=None

Maybe adding an elif (label is none): to the below if statement or similar to handle that validation is needed?

exabgp/src/exabgp/application/healthcheck.py

Line 216 in 925f493

if label:

[vincentbernat]

Yes, this seems sensible. I think we should have enforced labels from the beginning (but I think they are not possible with IPv6, so it may be the reason this is not the case). The complexity around all the possible paths is likely to trigger more bugs.

[thomas-mangin]

@an0nz I am currently busy trying to get work stuff closed before going on holiday, but if you want to propose a PR in line with what you proposed, I will happily review it in a few days when I can look into it.

[thomas-mangin]

@an0nz what is your conclusion after this discussion? Should I look into potential change or ar you happy as it is?

[an0nz]

@thomas-mangin if you have time to look into a potential change as per the discussion it would prevent others from running into the same issue when not using labels.
I have not had time to look at creating a pull request yet thanks to a recent run in with covid.

[v-kamerdinerov]

@thomas-mangin @vincentbernat Hello guys. Please check my little PR for resolution of this problem

#1202