ClusterLabs / resource-agents

Combined repository of OCF agents from the RHCS and Linux-HA projects
GNU General Public License v2.0
489 stars 577 forks source link

findif.sh: dont use table parameter as it returns no netmask (tested with main/local/custom tables) #1891

Closed oalbrigt closed 9 months ago

lge commented 5 months ago

I'm late to the party and this has been merged and released already. But I think this is dropping good functionality based on some misunderstanding. See below.

@oalbrigt do you remember a specific use case that broke?

I try to demo something here. Say I have 3 demo in /etc/iproute2/rt_tables.

# for br in demo{0,1,2} ; do brctl addbr $br; ip link set up $br ; done
# ip route flush table demo
# ip route list table demo
# ip route add table demo 192.168.122.192/28 dev demo0
# ip route add table demo 192.168.122.200/32 dev demo1
# ip route add table demo 192.168.122.64/26 dev demo1
# ip route add table demo 192.168.122.128/25 dev demo2

# ip route list table demo
192.168.122.64/26 dev demo1 scope link linkdown 
192.168.122.128/25 dev demo2 scope link linkdown 
192.168.122.192/28 dev demo0 scope link linkdown 
192.168.122.200 dev demo1 scope link linkdown 

# for ip in 192.168.122.{1,70,130,199,200}; do echo == $ip; ip -f inet -o route list match $ip table demo ; done
== 192.168.122.1
== 192.168.122.70
192.168.122.64/26 dev demo1 scope link linkdown 
== 192.168.122.130
192.168.122.128/25 dev demo2 scope link linkdown 
== 192.168.122.199
192.168.122.128/25 dev demo2 scope link linkdown 
192.168.122.192/28 dev demo0 scope link linkdown 
== 192.168.122.200
192.168.122.128/25 dev demo2 scope link linkdown 
192.168.122.192/28 dev demo0 scope link linkdown 
192.168.122.200 dev demo1 scope link linkdown 

Now, the .1 has no output (no matching route in that table for that prefix), .70 and .130 have one matching route, and .199 and .200 have two respectively three matches.

Now, the awk best route magic was probably missing the "no netmask is /32" part, so would not handle .200 correctly (and probably not find any route at all if we only have an exact route and no less specific route as well.

But rather fix that than drop the table parameter completely.

I see several options: first try an ip route list exact, and if that is empty, try the ip route list match | awk as we did before. Or detect host routes and treat them as /32 in awk. Or maybe do something like this:

ip -f inet -o route list match 192.168.122.200 table demo |
    sed -e 's,^\([0-9.]\+\) ,\1/32 ,' |
    sort -t/ -k2,2nr |
    head -n1
lge commented 5 months ago

| sed -e 's,^\([0-9.]\+\) ,\1/32 ,;s,^\([0-9a-f:]\+\) ,\1/128 ,' | sort -t/ -k2,2nr (and | head -n1) to also catch inet6, if there is a use case for host routes in ipv6 ;-)

oalbrigt commented 5 months ago

@lge Thanks. I'll look into it.

lge commented 5 months ago

Considering that the only matching route may be a (not "the"; with routing rules and tables, we may have setups with multiple default routes) default route, depending on what we want to do with this output, default as $1 may need special treatment right there, or in the case block below. Or we agree to NOT handle it but require explicit nic and netmask if the only matching route is a default route.

oalbrigt commented 3 months ago

I've tested your patch, and it seem to work well for our scenarioes as well. Will be looking into more advanced scenarioes and the IPv6 side of things after releasing 4.14.0.

oalbrigt commented 1 month ago

@lge I've added IPv6 support to IPsrcaddr, and detection of more than 1 route in findif.sh in https://github.com/ClusterLabs/resource-agents/pull/1947.