wlb: T7977: Fix weight calculation for multiple interfaces

[l0crian1]

Change summary

When more than 2 interfaces were configured in a WLB rule, nftables would fail to load the config since there were duplicate keys in the vmap. This corrects that issue.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Code style update (formatting, renaming)
• Refactoring (no functional changes)
• Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
• Other (please describe):

Related Task(s)

https://vyos.dev/T7977

Related PR(s)

How to test / Smoketest result

Configure 3 or more interfaces in a WLB rule:

set load-balancing wan rule 10 interface eth0.20
set load-balancing wan rule 10 interface eth0.101
set load-balancing wan rule 10 interface eth0.102

Before:

Check for presence of nft chain:

sudo nft list table vyos_wanloadbalance
Error: No such file or directory
list table vyos_wanloadbalance
           ^^^^^^^^^^^^^^^^^^^

Errors can be viewed in the log. You'll see errors like this:

/run/nftables_wlb.conf:15:139-139: Error: Could not process rule: File exists
        iifname "eth1" ct state new counter numgen random mod 3 vmap { 0 : jump wlb_mangle_isp_eth0.20, 1 : jump wlb_mangle_isp_eth0.101, 1 : jump wlb_mangle_isp_eth0.102 }
                                                                                                                                          ^

After:

Check the nft chain. vmap keys are now correct:

sudo nft list table vyos_wanloadbalance
table ip vyos_wanloadbalance {
        chain wlb_nat_postrouting {
                type nat hook postrouting priority srcnat - 1; policy accept;
                ct mark 0x000000c9 oifname "eth0.20" counter packets 0 bytes 0 snat to 192.168.2.186
                ct mark 0x000000ca oifname "eth0.101" counter packets 0 bytes 0 snat to 10.0.101.207
                ct mark 0x000000cb oifname "eth2" counter packets 0 bytes 0 snat to 10.0.101.208
        }

        chain wlb_mangle_prerouting {
                type filter hook prerouting priority mangle; policy accept;
                iifname "eth1" ct state new counter packets 0 bytes 0 numgen random mod 12 vmap { 0 : jump wlb_mangle_isp_eth0.20, 1 : jump wlb_mangle_isp_eth2, 2-11 : jump wlb_mangle_isp_eth0.101 }
                iifname "eth1" counter packets 0 bytes 0 meta mark set ct mark
        }

        chain wlb_mangle_output {
                type filter hook output priority mangle; policy accept;
                meta mark != 0x00000000 counter packets 0 bytes 0 return
                meta l4proto icmp counter packets 198 bytes 16632 return
                ip saddr 127.0.0.0/8 ip daddr 127.0.0.0/8 counter packets 24 bytes 1440 return
                oifname != "eth1" ct state new counter packets 0 bytes 0 numgen random mod 12 vmap { 0 : jump wlb_mangle_isp_eth0.20, 1 : jump wlb_mangle_isp_eth2, 2-11 : jump wlb_mangle_isp_eth0.101 }
                oifname != "eth1" counter packets 0 bytes 0 meta mark set ct mark
        }

        chain wlb_mangle_isp_eth0.20 {
                meta mark set 0x000000c9 ct mark set 0x000000c9 counter packets 0 bytes 0 accept
        }

        chain wlb_mangle_isp_eth0.101 {
                meta mark set 0x000000ca ct mark set 0x000000ca counter packets 0 bytes 0 accept
        }

        chain wlb_mangle_isp_eth2 {
                meta mark set 0x000000cb ct mark set 0x000000cb counter packets 0 bytes 0 accept
        }
}

Smoketest results:

test_3_or_more_interfaces_in_rule (__main__.TestLoadBalancingWan.test_3_or_more_interfaces_in_rule) ... ok
test_check_chains (__main__.TestLoadBalancingWan.test_check_chains) ... ok
test_criteria_failover_hook (__main__.TestLoadBalancingWan.test_criteria_failover_hook) ... ok
test_firewall_groups (__main__.TestLoadBalancingWan.test_firewall_groups) ... ok
test_table_routes (__main__.TestLoadBalancingWan.test_table_routes) ... ok

----------------------------------------------------------------------
Ran 5 tests in 142.330s

OK

Checklist:

• I have read the CONTRIBUTING document
• I have linked this PR to one or more Phabricator Task(s)
• I have run the components SMOKETESTS if applicable
• My commit headlines contain a valid Task id
• My change requires a change to the documentation
• I have updated the documentation accordingly

[github-actions]

👍
No issues in PR Title / Commit Title

[sever-sever]

Provided example confusing

set load-balancing wan rule 10 interface eth0.20
set load-balancing wan rule 10 interface eth0.101
set load-balancing wan rule 10 interface eth0.102

What does eth2 do here?

table ip vyos_wanloadbalance {
        chain wlb_nat_postrouting {
                type nat hook postrouting priority srcnat - 1; policy accept;
                ct mark 0x000000c9 oifname "eth0.20" counter packets 0 bytes 0 snat to 192.168.2.186
                ct mark 0x000000ca oifname "eth0.101" counter packets 0 bytes 0 snat to 10.0.101.207
                ct mark 0x000000cb oifname "eth2" counter packets 0 bytes 0 snat to 10.0.101.208
        }

[l0crian1]

@sever-sever the output was just what I had in my nftables at the time since I needed to make sure both equal weight and unequal weight configs work.

What was in the output wasn’t really important, just that it existed after the change when 3 or more interfaces were configured.

[c-po]

Fix issue in non auto-incrementing weight for multiple interfaces.

Could you extend the smoketests to validate this szenario?

[l0crian1]

Could you extend the smoketests to validate this szenario?

I considered doing that, but I worried it could be fragile. For instance, when you first configure WLB, it may not fail since the interfaces are not in an active state (the vmap would be empty at that point). So it may pass no matter what. It's only when the health-checks pass and the interfaces go into an active state would cause illegal nftables config.

[sever-sever]

Smoke tests passed health checks, as I remember. We use vethpairs

vyos-1x/smoketest/scripts/cli/test_load-balancing_wan.py

Line 170 in 89e4e09

iifname "veth3" ip saddr 198.51.100.0/24 ct state new counter numgen random mod 11 vmap { 0 : jump wlb_mangle_isp_veth1, 1-10 : jump wlb_mangle_isp_veth2 }

[l0crian1]

Yeah, but it's being done with a bunch of static sleep timers to overcome that fragility. I may have a better way for that, so it's not adding a bunch of time to the testing.

[l0crian1]

@c-po @sever-sever I added a smoketest along with a helper function that waits for a condition to be met or reach a timeout.

[sarthurdev]

Good catch and thanks for the fix. Tested locally.

Do you think it'd be beneficial to also verify the verdict map in a smoketest?

[l0crian1]

Do you think it'd be beneficial to also verify the verdict map in a smoketest?

The accuracy of the vmaps appears to be checked in test_check_chains, so I think that would already be covered.

[sarthurdev]

Do you think it'd be beneficial to also verify the verdict map in a smoketest?

The accuracy of the vmaps appears to be checked in test_check_chains, so I think that would already be covered.

I was thinking more to extend it so it also covers instances of more than 2 interfaces.

[l0crian1]

There's probably some value to that. A lot of users with multiple interfaces may just do this when they have 2 interfaces:

set load-balancing wan rule 10 interface eth0
set load-balancing wan rule 10 interface eth1 weight 2

But with 3 or more interfaces, they may have weights across multiple interfaces (or all of them), which makes the logic a little more rigid. It seems to be handled fine right now, but it could be good regression protection to have it covered in a smoketest.

[l0crian1]

@sarthurdev Do you want me to extend the smoketest I added to validate the vmaps have the correct number of weight buckets? Or would it be added in a different PR?

[c-po]

@l0crian1 please extend the smoketest and then we can merge it.

[l0crian1]

@c-po Done!

[github-actions]

CI integration 👍 passed!

Details

CI logs

• CLI Smoketests (no interfaces) 👍 passed
• CLI Smoketests VPP ❌ failed
• CLI Smoketests (interfaces only) 👍 passed
• Config tests 👍 passed
• Config tests VPP 👍 passed
• RAID1 tests 👍 passed
• TPM tests 👍 passed