rethinking the ppp defaultroute* options

jkroonza commented 10 months ago

          The relevant documentation of the defaultroute options:

   defaultroute
          Add  a  default  route to the system routing tables, using the peer as the gateway,
          when IPCP negotiation is successfully completed.  This entry is  removed  when  the
          PPP  connection  is broken.  This option is privileged if the nodefaultroute option
          has been specified.

   defaultroute-metric
          Define the metric of the defaultroute and only add it if there is no other  default
          route  with the same metric.  With the default value of -1, the route is only added
          if there is no default route at all.

   replacedefaultroute
          This  option  is a flag to the defaultroute option. If defaultroute is set and this
          flag is also set, pppd replaces an existing default  route  with  the  new  default
          route.  This option is privileged.

(An appenddefaultroute option would also be convenient but let's not over-complicate things).

I think we should ignore the IPv6 situation, defaultroute6 technically already violates the standards as I understand them, but it's been stated that providers are messing around, so let's just leave that as is. Also, this could technically be handled more flexibly from ipv6-up. Same for ip-up actually come to think about it.

So there are a few valid combinations of options:

defaultroute
defaultroute+defaultroute-metric
defaultroute+replacedefaultroute
defaultroute+defaultroute-metric+replacedefaultroute

So I believe for these combinations the behaviour should be as follows (Ignoring 3 which is problematic):

If any default route exist, don't add one, if none exist, add with metric 0.
If a default route with the same metric exist, don't add one, else add with relevant metric.
If a default route with the same metric exist, remove it (restore on ipv4 down) and re-add via ppp.

A further problem with replacedefaultroute is that it's possible in linux to have multiple exact same routes at the same metric, so technically even 4 is a problem above:

plastiekpoot [09:01:41] ~ # ip ro sh
default via 192.168.42.1 dev wlp2s0 proto dhcp src 192.168.42.21 metric 2002 
...
plastiekpoot [09:02:05] ~ # ip route append default via 192.168.42.1 dev wlp2s0 proto static src 192.168.42.21 metric 2002
plastiekpoot [09:02:10] ~ # ip ro sh
default via 192.168.42.1 dev wlp2s0 proto dhcp src 192.168.42.21 metric 2002 
default via 192.168.42.1 dev wlp2s0 proto static src 192.168.42.21 metric 2002 
...

So with replacedefaultroute - should we remove all matching default routes or only the first matching one?

Which leads into my logic issue with 3 above. If there is only one other default route it's all good and well, we remove it and re-add later, but what if there are multiple such routes?

I guess the replacedefaultroute option is problematic regardless. We should only ever add routes and remove our own, the system administrator should manage priority via metrics.

So really the question is if a matching route with the same metric exist, do we append or not? Does yet another separate option for that even make sense or should we simply append our route regardless - as such - nuke the replacedefaultroute option entirely, and append another default route at the specified metric and move on, thus reduce the defaultroute options to "defaultroute" and "defaultroute-metric" where if defaultroute is given we ALWAYS APPEND a default route at the specified defaultroute-metric (which then defaults to 0).

If the system administrator wants more control than that - have them rely on ip-(pre-)up or net-pre-up and let them deal with it there?

Whilst the existing logic is definitely broken, I'm not convinced that the PR brings things closer to the documented behaviour either, nor am I sure the documented behaviour is well defined.

Originally posted by @jkroonza in https://github.com/ppp-project/ppp/pull/472#pullrequestreview-1814815012

In addition, I believe the IPv6 defaultroute6 option should be brought in line with whatever it's decided to do with IPv4.

Neustradamus commented 10 months ago

@sthibaul: What is the solution? ^^

jkroonza commented 10 months ago

My opinion is that we should only ever append default route at given metric, never removing any routes not added by pppd.

I think this would simplify the routing related code in pppd quite a bit (for ipv4) at least. And I think a defaultroute6 option should be added.

This leaves two knobs for each protocol:

defaultroute yes/no. defaultroutemetric value.

Perhaps these can be combined as per some other suggestion on original PR I hijacked.

If the sysadmin wants more control than that, deal with it from the hook scripts or using routing daemon (eg, frr) and don't use defaultroute* options from pppd.

One scenario for example that can't be currently handled is adding multiple default routes at the same metric and only failing over if the current primary fails. Let's say I have two uplinks, same specs but different public IPs, I don't want one to take priority over the other and only want to switch if the current active fails.

ppp1 comes up, adds default metric 100 ppp2 comes up, adds default metric 100

Now we have:

default dev ppp1 metric 100
default dev ppp2 metric 100

ppp2 fails, it merely retracts, no change in routing. ppp2 comes back, and is restored to same state as above.

If however ppp1 fails, default is updated to ppp2 by the kernel. Some NAT/MASQUERADEs break, tough, that's the way of it. If you used MASQUERADE rather than SNAT the kernel will sort that out.

ppp1 restores, and re-appends default dev ppp1 metric 100, routing is now:

default dev ppp2 metric 100
default dev ppp1 metric 100

Routing remains via ppp2, not interrupting any connections, which in my world is preferable.

If I always want ppp1 to take priority, then I would set defaultmetric for ppp1 to 50, and defaultmetric for ppp2 to 100 (for example, any values where defaultmetric for ppp1 is less than defaultmetric for ppp2 would work).

We have cases where we need more control, but they're the exception, not the rule. And we can trivially deal with those cases using frr, or in cases where we can't do that with a little more complexity using -up and -down scripts.