Open an0nz opened 2 years ago
@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.
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.
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?
https://github.com/Exa-Networks/exabgp/blob/925f49307bc06b3bd5313826f927cf48f0984bbe/src/exabgp/application/healthcheck.py#L216
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.
@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.
@an0nz what is your conclusion after this discussion? Should I look into potential change or ar you happy as it is?
@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.
@thomas-mangin @vincentbernat Hello guys. Please check my little PR for resolution of this problem
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 sethttps://github.com/Exa-Networks/exabgp/blob/925f49307bc06b3bd5313826f927cf48f0984bbe/src/exabgp/application/healthcheck.py#L263
I have added some debugging and changed the code as per below to demonstrate what is going on Debug Changes / Fix
Resulting Log Entries
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=NoneWorkaround Use
--label=ip1
or similar