wlb: T9472: keep marks of interfaces when others change

The routing table and mark of a WAN interface followed its position in the
sorted interface list. Adding an interface which sorts before the others
renumbered all of them. Existing connections still carried their old mark
in conntrack and were routed into the wrong table - they broke.

Every interface now keeps its table as long as it stays configured, new
interfaces get the lowest free table. The assignment is dropped once WAN
load-balancing is removed.

4 files changed, 96 insertions, 11 deletions

diff --git a/python/vyos/wanloadbalance.py b/python/vyos/wanloadbalance.py
index faea469c2..8acaa361e 100644
--- a/python/vyos/wanloadbalance.py
+++ b/python/vyos/wanloadbalance.py
@@ -21,6 +21,8 @@ from vyos.utils.process import rc_cmd
 from vyos.utils.process import run
 
 dhclient_lease = 'dhclient_{0}.lease'
+# routing table - and mark - of every WAN interface, see vyos-load-balancer.py
+wlb_tables_file = '/run/wlb_tables.json'
 
 def nft_rule(rule_conf, rule_id, local=False, exclude=False, limit=False, weight=None, health_state=None, action=None, restore_mark=False):
     output = []
diff --git a/smoketest/scripts/cli/test_load-balancing_wan.py b/smoketest/scripts/cli/test_load-balancing_wan.py
index 5c67778e3..c2162396a 100755
--- a/smoketest/scripts/cli/test_load-balancing_wan.py
+++ b/smoketest/scripts/cli/test_load-balancing_wan.py
@@ -16,6 +16,7 @@
 
 import json
 import os
+import select
 import subprocess
 import tempfile
 import unittest
@@ -881,6 +882,61 @@ printf 'WLB_INTERFACE_NAME=%s WLB_SCRIPT_IFACE=%s\\n' \\
 
         found, last = self.wait_for_result(reported, {'eth231 ACTIVE', 'eth232 ACTIVE'}, timeout=20)
         self.assertTrue(found, msg=f'hook reported {last}')
+    def test_mark_stable_when_adding_interface(self):
+        # marks and routing tables followed the sorted interface list, adding
+        # an interface renumbered the others and broke their connections
+        isps = ['eth231', 'eth232', 'eth229']
+        self.lab_setup('eth230', isps)
+        self.lab_health('eth231', 1)
+        self.lab_health('eth232', 2)
+        for ifname in ['eth231', 'eth232']:
+            self.cli_set(base_path + ['wan', 'rule', '10', 'interface', ifname])
+        self.cli_set(base_path + ['wan', 'rule', '10', 'inbound-interface', 'eth230'])
+        self.cli_commit()
+        self.lab_wait_status({'eth231': 'ACTIVE', 'eth232': 'ACTIVE'})
+
+        def isp_marks():
+            marks = {}
+            chain = None
+            for line in rc_cmd('sudo nft list table ip vyos_wanloadbalance')[1].splitlines():
+                if 'chain wlb_mangle_isp_' in line:
+                    chain = line.split('wlb_mangle_isp_')[1].split()[0]
+                elif chain and 'meta mark set' in line:
+                    marks[chain] = line.split('meta mark set ')[1].split()[0]
+            return marks
+
+        marks = isp_marks()
+
+        # a long-lived connection through one of the ISPs
+        client = subprocess.Popen(['sudo', 'ip', 'netns', 'exec', 'wlb-lan', 'socat', '-',
+                                   f'TCP:{lab_target}:90'], stdin=subprocess.PIPE,
+                                  stdout=subprocess.PIPE, text=True)
+        self.addCleanup(client.kill)
+
+        def echo(text, timeout=5):
+            client.stdin.write(f'{text}\n')
+            client.stdin.flush()
+            if select.select([client.stdout], [], [], timeout)[0]:
+                return client.stdout.readline().strip()
+            return None
+
+        self.assertEqual(echo('first'), 'first')
+
+        # eth229 sorts before the interfaces already in use
+        self.cli_set(base_path + ['wan', 'interface-health', 'eth229', 'nexthop', '10.250.3.2'])
+        self.cli_set(base_path + ['wan', 'interface-health', 'eth229', 'test', '10', 'target', '10.250.3.2'])
+        self.cli_commit()
+        self.lab_wait_status({'eth229': 'ACTIVE', 'eth231': 'ACTIVE', 'eth232': 'ACTIVE'})
+
+        new_marks = isp_marks()
+        for ifname in ['eth231', 'eth232']:
+            self.assertEqual(new_marks.get(ifname), marks[ifname], msg=f'mark of {ifname} changed')
+        self.assertEqual(echo('second'), 'second')
+
+        # a removed WAN load-balancing starts over with the first table
+        self.cli_delete(base_path)
+        self.cli_commit()
+        self.assertFalse(os.path.exists('/run/wlb_tables.json'))
 
 if __name__ == '__main__':
     unittest.main(verbosity=2, failfast=VyOSUnitTestSHIM.TestCase.debug_on())
diff --git a/src/conf_mode/load-balancing_wan.py b/src/conf_mode/load-balancing_wan.py
index 63943510f..fa95bf794 100755
--- a/src/conf_mode/load-balancing_wan.py
+++ b/src/conf_mode/load-balancing_wan.py
@@ -14,12 +14,15 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+import os
+
 from sys import exit
 
 from vyos.config import Config
 from vyos.configdep import set_dependents, call_dependents
 from vyos.utils.dict import dict_search_args
 from vyos.utils.process import cmdl
+from vyos.wanloadbalance import wlb_tables_file
 from vyos import ConfigError
 from vyos import airbag
 airbag.enable()
@@ -162,6 +165,9 @@ def generate(lb):
 def apply(lb):
     if not lb:
         cmdl(['systemctl', 'stop', service])
+        # without WAN load-balancing the marks of existing connections are moot
+        if os.path.exists(wlb_tables_file):
+            os.unlink(wlb_tables_file)
     else:
         cmdl(['systemctl', 'restart', service])
 
diff --git a/src/helpers/vyos-load-balancer.py b/src/helpers/vyos-load-balancer.py
index 38ba8d87b..d4db328c7 100755
--- a/src/helpers/vyos-load-balancer.py
+++ b/src/helpers/vyos-load-balancer.py
@@ -33,6 +33,7 @@ from vyos.wanloadbalance import health_ping_host
 from vyos.wanloadbalance import health_ping_host_ttl
 from vyos.wanloadbalance import parse_dhcp_nexthop
 from vyos.wanloadbalance import parse_ppp_nexthop
+from vyos.wanloadbalance import wlb_tables_file
 
 nftables_wlb_conf = '/run/nftables_wlb.conf'
 wlb_status_file = '/run/wlb_status.json'
@@ -207,12 +208,35 @@ def nftables_update(lb):
 
     return True
 
+def assign_tables(interfaces, mark_offset):
+    """ Return the routing table - and mark - of every interface. Connections
+    keep their mark in conntrack, an interface keeps its table as long as it
+    stays configured. New interfaces get the lowest free table. """
+    try:
+        with open(wlb_tables_file) as f:
+            previous = json.load(f)
+    except (OSError, ValueError):
+        previous = {}
+
+    tables = {ifname: previous[ifname] for ifname in interfaces if ifname in previous}
+    table_num = mark_offset
+    for ifname in interfaces:
+        if ifname in tables:
+            continue
+        table_num += 1
+        while table_num in tables.values():
+            table_num += 1
+        tables[ifname] = table_num
+
+    with open(wlb_tables_file, 'w') as f:
+        json.dump(tables, f)
+    return tables
+
 def cleanup(lb):
-    if 'interface_health' in lb:
-        index = 1
-        for ifname, health_conf in lb['interface_health'].items():
-            table_num = lb['mark_offset'] + index
-            suppress_prio = lb['mark_offset'] + index
+    if 'health_state' in lb:
+        for ifname, state in lb['health_state'].items():
+            table_num = state['table_number']
+            suppress_prio = table_num
             table_prio = suppress_prio + 100
             run(f'ip route del table {table_num} default')
             run(
@@ -224,7 +248,6 @@ def cleanup(lb):
                 f'priority {table_prio}'
             )
             run(f'ip rule del fwmark {hex(table_num)} table {table_num}')
-            index += 1
 
     run(f'nft delete table ip vyos_wanloadbalance')
 
@@ -261,9 +284,9 @@ if __name__ == '__main__':
 
     # Create state dicts, interface address and nexthop, install routes and ip rules
     if 'interface_health' in lb:
-        index = 1
+        tables = assign_tables(lb['interface_health'], lb['mark_offset'])
         for ifname, health_conf in lb['interface_health'].items():
-            table_num = lb['mark_offset'] + index
+            table_num = tables[ifname]
             addr = get_ipv4_address(ifname)
             lb['health_state'][ifname] = {
                 'if_addr': addr,
@@ -284,7 +307,7 @@ if __name__ == '__main__':
             else:
                 run(f'ip route replace table {table_num} default dev {ifname} via {health_conf["nexthop"]}')
 
-            suppress_prio = lb['mark_offset'] + index
+            suppress_prio = table_num
             table_prio = suppress_prio + 100
             if 'only_default_route' in lb:
                 run(
@@ -298,8 +321,6 @@ if __name__ == '__main__':
             else:
                 run(f'ip rule add fwmark {hex(table_num)} table {table_num}')
 
-            index += 1
-
         nftables_update(lb)
 
         run('ip route flush cache')