Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

T6254: Extend VRF table numbers #3353

Closed
wants to merge 1 commit into from
Closed

Conversation

sever-sever
Copy link
Member

Change Summary

Extend the table number that can be used for VRF to 1-65535

The current limit <200-65535>
Actual kernel limit <1-294967295>

For now, we cannot use actual kernel limits and it should be fixed in another commit
The reason is it somehow related to nft conntrack rules

Traceback (most recent call last):
  File "/usr/libexec/vyos/conf_mode/vrf.py", line 351, in <module>
    apply(c)
  File "/usr/libexec/vyos/conf_mode/vrf.py", line 308, in apply
    cmd(f'nft {nft_add_element}')
  File "/usr/lib/python3/dist-packages/vyos/utils/process.py", line 155, in cmd
    raise OSError(code, feedback)
PermissionError: [Errno 1] failed to run command: nft add element inet vrf_zones ct_iface_map { "blue" : 4294967295 }
returned: 
exit code: 1

noteworthy:
cmd 'nft add element inet vrf_zones ct_iface_map { "blue" : 4294967295 }'
returned (out):

returned (err):
Error: Value 4294967295 exceeds valid range 0-65535
add element inet vrf_zones ct_iface_map { blue : 4294967295 }
     

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): Extend VRF table numbers

Related Task(s)

Related PR(s)

Component(s) name

vrf

Proposed changes

How to test

Add Table 1 for the VRF and check the actual table (expected vrf table 1)

set vrf name red table 1
commit

vyos@r4# sudo ip -d link show type vrf
228: red: <NOARP,MASTER,UP,LOWER_UP> mtu 65575 qdisc noqueue state UP mode DEFAULT group default qlen 1000
    link/ether ea:20:1e:f9:0a:7e brd ff:ff:ff:ff:ff:ff promiscuity 0 allmulti 0 minmtu 1280 maxmtu 65575 
    vrf table 1 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 tso_max_size 65536 tso_max_segs 65535 gro_max_size 65536 gso_ipv4_max_size 65536 gro_ipv4_max_size 65536 
[edit]
vyos@r4#

Smoketest result

vyos@r4:~$ /usr/libexec/vyos/tests/smoke/cli/test_vrf.py
test_vrf_assign_interface (__main__.VRFTest.test_vrf_assign_interface) ... ok
test_vrf_bind_all (__main__.VRFTest.test_vrf_bind_all) ... ok
test_vrf_conntrack (__main__.VRFTest.test_vrf_conntrack) ... ok
test_vrf_disable_forwarding (__main__.VRFTest.test_vrf_disable_forwarding) ... ok
test_vrf_ip_ipv6_nht (__main__.VRFTest.test_vrf_ip_ipv6_nht) ... ok
test_vrf_ip_ipv6_protocol_non_existing_route_map (__main__.VRFTest.test_vrf_ip_ipv6_protocol_non_existing_route_map) ... 
Specified route-map "v4-non-existing" does not exist!


Specified route-map "v6-non-existing" does not exist!

ok
test_vrf_ip_protocol_route_map (__main__.VRFTest.test_vrf_ip_protocol_route_map) ... ok
test_vrf_ipv6_protocol_route_map (__main__.VRFTest.test_vrf_ipv6_protocol_route_map) ... ok
test_vrf_link_local_ip_addresses (__main__.VRFTest.test_vrf_link_local_ip_addresses) ... ok
test_vrf_loopbacks_ips (__main__.VRFTest.test_vrf_loopbacks_ips) ... ok
test_vrf_static_route (__main__.VRFTest.test_vrf_static_route) ... 
VRF "red" table id is mandatory!


VRF "green" table id is mandatory!


VRF "blue" table id is mandatory!


VRF "foo-bar" table id is mandatory!


VRF "baz_foo" table id is mandatory!

ok
test_vrf_table_id_is_unalterable (__main__.VRFTest.test_vrf_table_id_is_unalterable) ... 
VRF "red" table id modification not possible!

ok
test_vrf_vni_add_change_remove (__main__.VRFTest.test_vrf_vni_add_change_remove) ... ok
test_vrf_vni_and_table_id (__main__.VRFTest.test_vrf_vni_and_table_id) ... 
VRF "red" table id is mandatory!


VRF "green" table id is mandatory!


VRF "blue" table id is mandatory!


VRF "foo-bar" table id is mandatory!


VRF "baz_foo" table id is mandatory!

ok
test_vrf_vni_duplicates (__main__.VRFTest.test_vrf_vni_duplicates) ... 
VRF "blue" VNI is not unique!


VRF "blue" VNI is not unique!


VRF "blue" VNI is not unique!


VRF "blue" VNI is not unique!


VRF "blue" VNI is not unique!

ok

----------------------------------------------------------------------
Ran 15 tests in 287.970s

OK
vyos@r4:~$ 

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

Extend the table number that can be used for VRF to 1-65535
@vyosbot vyosbot requested a review from a team April 23, 2024 08:55
@vyosbot vyosbot requested review from dmbaturin, sarthurdev, zdc, jestabro and c-po and removed request for a team April 23, 2024 08:55
@c-po
Copy link
Member

c-po commented Apr 23, 2024

The main reason for selecting this number range was that protocols static table makes use of the same underlaying routing tables.

We can lift this lower limit for sure. We only need to make sure (via verify()) that there is no collision between VRF table ID and static table ID

@sever-sever
Copy link
Member Author

sever-sever commented Apr 23, 2024

The main reason for selecting this number range was that protocols static table makes use of the same underlaying routing tables.

We can lift this lower limit for sure. We only need to make sure (via verify()) that there is no collision between VRF table ID and static table ID

It was the use-case for this. Why do we need to verify it? Table 10 and VRF Table 10 seem the same. It looks OK to combine them for some features.
For example, I have bgp in the default VRF and want to put routes to the VRF red table

set policy route-map RMAP rule 10 action 'permit'
set policy route-map RMAP rule 10 set table '10'

set protocols bgp neighbor 192.168.122.11 address-family ipv4-unicast route-map import 'RMAP'
set protocols bgp neighbor 192.168.122.11 remote-as '65001'
set protocols bgp system-as '65001'

set vrf name red table '10'

Check

vyos@r4# run show ip route 100.64.21.0/24
% Network not in table
[edit]
vyos@r4# 
[edit]
vyos@r4# run show ip route vrf red
Codes: K - kernel route, C - connected, S - static, R - RIP,
       O - OSPF, I - IS-IS, B - BGP, E - EIGRP, N - NHRP,
       T - Table, v - VNC, V - VNC-Direct, A - Babel, F - PBR,
       f - OpenFabric,
       > - selected route, * - FIB route, q - queued, r - rejected, b - backup
       t - trapped, o - offload failure

VRF red:
B>* 100.64.21.0/24 [200/0] via 192.168.122.11, eth0, weight 1, 00:04:52
B>* 100.64.22.0/24 [200/0] via 192.168.122.11, eth0, weight 1, 00:04:52
[edit]
vyos@r4# 

If we don't implement this approach, I can close it.
Without the same table values, it does not make any sense in this feature/change.

@c-po
Copy link
Member

c-po commented Apr 23, 2024

It’s a legit extension indeed which we should not limit artificially. but do those routes actually work?

Usually that’s called VRF leaking and is done using route-target import/export

@c-po c-po removed the on-hold label May 23, 2024
@dmbaturin dmbaturin closed this Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants