vrf: T4191: bugfix for "ip rule" when VRFs are created

We always mangled and worked on the "ip rule" singleton even when nothing
needed to be changed. This resulted in a VRF hickup when the same VRF was added
and removed multiple times.

set interfaces ethernet eth1 vrf foo
set vrf name foo table '1000'
commit

delete interfaces ethernet eth1 vrf
delete vrf
commit

set interfaces ethernet eth1 vrf foo
set vrf name foo table '1000'
commit

broke reachability on eth1 - a reboot was required.

This change will now only alter the ip rule tables once when VRF instances
are created for the first time and will not touch the Kernel "ip rule"
representation afterwards.

(cherry picked from commit 2cec431e5caf9df85640f707cd6dc3077c17c238)

1 files changed, 53 insertions, 52 deletions

diff --git a/src/conf_mode/vrf.py b/src/conf_mode/vrf.py
index 9d36819be..e64f35ab5 100755
--- a/src/conf_mode/vrf.py
+++ b/src/conf_mode/vrf.py
@@ -1,6 +1,6 @@
 #!/usr/bin/env python3
 #
-# Copyright (C) 2020-2021 VyOS maintainers and contributors
+# Copyright (C) 2020-2022 VyOS maintainers and contributors
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 2 or later as
@@ -26,16 +26,23 @@ from vyos.template import render
 from vyos.util import call
 from vyos.util import cmd
 from vyos.util import dict_search
+from vyos.util import sysctl
 from vyos import ConfigError
 from vyos import airbag
 airbag.enable()
 
 config_file = r'/etc/iproute2/rt_tables.d/vyos-vrf.conf'
 
-def list_rules():
-    command = 'ip -j -4 rule show'
-    answer = loads(cmd(command))
-    return [_ for _ in answer if _]
+def has_rule(af : str, priority : int, table : str):
+    """ Check if a given ip rule exists """
+    if af not in ['-4', '-6']:
+        raise ValueError()
+    command = f'ip -j {af} rule show'
+    for tmp in loads(cmd(command)):
+        if {'priority', 'table'} <= set(tmp):
+            if tmp['priority'] == priority and tmp['table'] == table:
+                return True
+    return False
 
 def vrf_interfaces(c, match):
     matched = []
@@ -64,7 +71,6 @@ def vrf_routing(c, match):
     c.set_level(old_level)
     return matched
 
-
 def get_config(config=None):
     if config:
         conf = config
@@ -138,36 +144,63 @@ def apply(vrf):
     bind_all = '0'
     if 'bind-to-all' in vrf:
         bind_all = '1'
-    call(f'sysctl -wq net.ipv4.tcp_l3mdev_accept={bind_all}')
-    call(f'sysctl -wq net.ipv4.udp_l3mdev_accept={bind_all}')
+    sysctl('net.ipv4.tcp_l3mdev_accept', bind_all)
+    sysctl('net.ipv4.udp_l3mdev_accept', bind_all)
 
     for tmp in (dict_search('vrf_remove', vrf) or []):
         if os.path.isdir(f'/sys/class/net/{tmp}'):
-            call(f'ip -4 route del vrf {tmp} unreachable default metric 4278198272')
-            call(f'ip -6 route del vrf {tmp} unreachable default metric 4278198272')
             call(f'ip link delete dev {tmp}')
 
     if 'name' in vrf:
+        # Linux routing uses rules to find tables - routing targets are then
+        # looked up in those tables. If the lookup got a matching route, the
+        # process ends.
+        #
+        # TL;DR; first table with a matching entry wins!
+        #
+        # You can see your routing table lookup rules using "ip rule", sadly the
+        # local lookup is hit before any VRF lookup. Pinging an addresses from the
+        # VRF will usually find a hit in the local table, and never reach the VRF
+        # routing table - this is usually not what you want. Thus we will
+        # re-arrange the tables and move the local lookup further down once VRFs
+        # are enabled.
+        #
+        # Thanks to https://stbuehler.de/blog/article/2020/02/29/using_vrf__virtual_routing_and_forwarding__on_linux.html
+
+        for afi in ['-4', '-6']:
+            # move lookup local to pref 32765 (from 0)
+            if not has_rule(afi, 32765, 'local'):
+                call(f'ip {afi} rule add pref 32765 table local')
+            if has_rule(afi, 0, 'local'):
+                call(f'ip {afi} rule del pref 0')
+            # make sure that in VRFs after failed lookup in the VRF specific table
+            # nothing else is reached
+            if not has_rule(afi, 1000, 'l3mdev'):
+                # this should be added by the kernel when a VRF is created
+                # add it here for completeness
+                call(f'ip {afi} rule add pref 1000 l3mdev protocol kernel')
+
+            # add another rule with an unreachable target which only triggers in VRF context
+            # if a route could not be reached
+            if not has_rule(afi, 2000, 'l3mdev'):
+                call(f'ip {afi} rule add pref 2000 l3mdev unreachable')
+
         for name, config in vrf['name'].items():
             table = config['table']
-
             if not os.path.isdir(f'/sys/class/net/{name}'):
                 # For each VRF apart from your default context create a VRF
                 # interface with a separate routing table
                 call(f'ip link add {name} type vrf table {table}')
-                # The kernel Documentation/networking/vrf.txt also recommends
-                # adding unreachable routes to the VRF routing tables so that routes
-                # afterwards are taken.
-                call(f'ip -4 route add vrf {name} unreachable default metric 4278198272')
-                call(f'ip -6 route add vrf {name} unreachable default metric 4278198272')
-                # We also should add proper loopback IP addresses to the newly
-                # created VRFs for services bound to the loopback address (SNMP, NTP)
-                call(f'ip -4 addr add 127.0.0.1/8 dev {name}')
-                call(f'ip -6 addr add ::1/128 dev {name}')
 
             # set VRF description for e.g. SNMP monitoring
             vrf_if = Interface(name)
+            # We also should add proper loopback IP addresses to the newly
+            # created VRFs for services bound to the loopback address (SNMP, NTP)
+            vrf_if.add_addr('127.0.0.1/8')
+            vrf_if.add_addr('::1/128')
+            # add VRF description if available
             vrf_if.set_alias(config.get('description', ''))
+
             # Enable/Disable of an interface must always be done at the end of the
             # derived class to make use of the ref-counting set_admin_state()
             # function. We will only enable the interface if 'up' was called as
@@ -178,38 +211,6 @@ def apply(vrf):
             state = 'down' if 'disable' in config else 'up'
             vrf_if.set_admin_state(state)
 
-    # Linux routing uses rules to find tables - routing targets are then
-    # looked up in those tables. If the lookup got a matching route, the
-    # process ends.
-    #
-    # TL;DR; first table with a matching entry wins!
-    #
-    # You can see your routing table lookup rules using "ip rule", sadly the
-    # local lookup is hit before any VRF lookup. Pinging an addresses from the
-    # VRF will usually find a hit in the local table, and never reach the VRF
-    # routing table - this is usually not what you want. Thus we will
-    # re-arrange the tables and move the local lookup furhter down once VRFs
-    # are enabled.
-
-    # get current preference on local table
-    local_pref = [r.get('priority') for r in list_rules() if r.get('table') == 'local'][0]
-
-    # change preference when VRFs are enabled and local lookup table is default
-    if not local_pref and 'name' in vrf:
-        for af in ['-4', '-6']:
-            call(f'ip {af} rule add pref 32765 table local')
-            call(f'ip {af} rule del pref 0')
-
-    # return to default lookup preference when no VRF is configured
-    if 'name' not in vrf:
-        for af in ['-4', '-6']:
-            call(f'ip {af} rule add pref 0 table local')
-            call(f'ip {af} rule del pref 32765')
-
-            # clean out l3mdev-table rule if present
-            if 1000 in [r.get('priority') for r in list_rules() if r.get('priority') == 1000]:
-                call(f'ip {af} rule del pref 1000')
-
     return None
 
 if __name__ == '__main__':