Re: [PATCH net-next] ipv6: do not merge differe type and protocol routes

[Hangbin Liu <liuhangbin@gmail.com>]

Hi Nicolas,
On Thu, Aug 31, 2023 at 10:17:19AM +0200, Nicolas Dichtel wrote:
> >>> So let's skip counting the different type and protocol routes as siblings.
> >>> After update, the different type/protocol routes will not be merged.
> >>>
> >>> + ip -6 route show table 100
> >>> local 2001:db8:103::/64 via 2001:db8:101::10 dev dummy1 metric 1024 pref medium
> >>> 2001:db8:103::/64 via 2001:db8:101::10 dev dummy2 metric 1024 pref medium
> >>>
> >>> + ip -6 route show table 200
> >>> 2001:db8:104::/64 via 2001:db8:101::10 dev dummy1 proto kernel metric 1024 pref medium
> >>> 2001:db8:104::/64 via 2001:db8:101::10 dev dummy2 proto bgp metric 1024 pref medium
> >>
> >> This seems wrong. The goal of 'ip route append' is to add a next hop, not to
> >> create a new route. Ok, it adds a new route if no route exists, but it seems
> >> wrong to me to use it by default, instead of 'add', to make things work magically.
> > 
> > Legacy API; nothing can be done about that (ie., that append makes a new
> > route when none exists).
> > 
> >>
> >> It seems more correct to return an error in these cases, but this will change
> >> the uapi and it may break existing setups.
> >>
> >> Before this patch, both next hops could be used by the kernel. After it, one
> >> route will be ignored (the former or the last one?). This is confusing and also
> >> seems wrong.
> > 
> > Append should match all details of a route to add to an existing entry
> > and make it multipath. If there is a difference (especially the type -
> > protocol difference is arguable) in attributes, then they are different
> > routes.
> > 
> 
> As you said, the protocol difference is arguable. It's not a property of the
> route, just a hint.
> I think the 'append' should match a route whatever the protocol is.
> 'ip route change' for example does not use the protocol to find the existing
> route, it will update it:
> 
> $ ip -6 route add 2003:1:2:3::/64 via 2001::2 dev eth1
> $ ip -6 route
> 2003:1:2:3::/64 via 2001::2 dev eth1 metric 1024 pref medium
> $ ip -6 route change 2003:1:2:3::/64 via 2001::2 dev eth1 protocol bgp
> $ ip -6 route
> 2003:1:2:3::/64 via 2001::2 dev eth1 proto bgp metric 1024 pref medium
> $ ip -6 route change 2003:1:2:3::/64 via 2001::2 dev eth1 protocol kernel
> $ ip -6 route
> 2003:1:2:3::/64 via 2001::2 dev eth1 proto kernel metric 1024 pref medium

Not sure if I understand correctly, `ip route replace` should able to
replace all other field other than dest and dev. It's for changing the route,
not only nexthop.
> 
> Why would 'append' selects route differently?

The append should also works for a single route, not only for append nexthop, no?

> 
> This patch breaks the legacy API.

As the patch's description. Who would expect different type/protocol route
should be merged as multipath route? I don't think the old API is correct.

Thanks
Hangbin