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

pbr: T6430: Allow forwarding into VRFs by name as well as route table IDs #3740

Merged
merged 2 commits into from
Jul 30, 2024

Conversation

talmakion
Copy link
Contributor

@talmakion talmakion commented Jun 29, 2024

Change Summary

My previous PR for simply extending the table ID range was rejected, so I've prepared another PR closer to my own use case for this - "policy based route leaking" between VRFs. It may or may not line up with Bernhard's intention from the original ticket.

This PR extends PBR syntax to allow "set vrf" as well as "set table", resolving table ID from an active VRF and then treating it the same as a "set table".

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)

Related PR(s)

Component(s) name

  • pbr

Proposed changes

  • PBR can only target table IDs up to 200 and the previous PR to extend the range was rejected
  • PBR with this PR can now also target VRFs directly by name, working around targeting problems for VRF table IDs outside the overlapping 100-200 range
  • Validation ensures rules can't target both a table ID and a VRF name (internally they are handled the same)
  • Added a simple accessor (get_vrf_table_id) for runtime mapping a VRF name to table ID, based on vyos.ifconfig.interface._set_vrf_ct_zone(). It does not replace that usage, as it deliberately does not handle non-VRF interface lookups (would fail with a KeyError).
  • Added smoketest to validate PBR 'set vrf' syntax

The use cases of this feature are limited and you will need to know exactly what you are doing to have it work properly, but for example, when I was experimenting with cross-VRF DNS lookup solutions, this would've been better than manipulating ip rules and nftables manually and externally.

How to test

Rules apply OK:

set name MGT table '666'
set policy route TEST rule 100 destination address '8.8.8.8'
set policy route TEST rule 100 destination port '53'
set policy route TEST rule 100 protocol 'tcp_udp'
set policy route TEST rule 100 set vrf 'MGT'
set policy route TEST2 rule 100 destination address '10.11.12.77'
set policy route TEST2 rule 100 protocol 'tcp_udp'
set policy route TEST2 rule 100 set vrf 'default'
set policy route TEST2 rule 100 source port '53'

Looking at the results:

# sudo ip rule show
220:    from all lookup 220
254:    from all fwmark 0x7fffff01 lookup main
666:    from all fwmark 0x7ffffd65 lookup MGT 
1000:   from all lookup [l3mdev-table]
2000:   from all lookup [l3mdev-table] unreachable
32765:  from all lookup local
32766:  from all lookup main
32767:  from all lookup default
# sudo nft list ruleset
[...]
table ip vyos_mangle {
        chain VYOS_PBR_PREROUTING {
                type filter hook prerouting priority mangle; policy accept;
        }

        chain VYOS_PBR_POSTROUTING {
                type filter hook postrouting priority mangle; policy accept;
        }

        chain VYOS_PBR_UD_TEST {
                meta l4proto { tcp, udp } ip daddr 8.8.8.8 th dport 53 counter packets 0 bytes 0 meta mark set 0x7ffffd65 return comment "ipv4-route-TEST-100"
        }

        chain VYOS_PBR_UD_TEST2 {
                meta l4proto { tcp, udp } ip daddr 10.11.12.77 th sport 53 counter packets 0 bytes 0 meta mark set 0x7fffff01 return comment "ipv4-route-TEST2-100"
        }
}
[...]

Attaching the PBRs to interfaces and watching traffic flow with tcpdump works as expected, the same as set table.

Smoketest result

$ python3 /usr/libexec/vyos/tests/smoke/cli/test_policy_route.py 
test_pbr_group (__main__.TestPolicyRoute.test_pbr_group) ... ok
test_pbr_mark (__main__.TestPolicyRoute.test_pbr_mark) ... ok
test_pbr_mark_connection (__main__.TestPolicyRoute.test_pbr_mark_connection) ... ok
test_pbr_matching_criteria (__main__.TestPolicyRoute.test_pbr_matching_criteria) ... ok
test_pbr_table (__main__.TestPolicyRoute.test_pbr_table) ... ok
test_pbr_vrf (__main__.TestPolicyRoute.test_pbr_vrf) ... ok

----------------------------------------------------------------------
Ran 6 tests in 32.673s

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

Copy link

❌ VyOS CLI smoketests failed!

Copy link

github-actions bot commented Jul 2, 2024

👍
No issues in PR Title / Commit Title

@talmakion
Copy link
Contributor Author

I want to update this to also implement this for policy local-route, so withdrawing to draft.

@talmakion talmakion marked this pull request as draft July 2, 2024 14:41
@talmakion talmakion marked this pull request as draft July 2, 2024 14:41
@sever-sever
Copy link
Member

I guess we should not change two components even if they are similar in one PR
There should be another PR for the policy local-route as it does not use nftables at all.

@talmakion talmakion marked this pull request as ready for review July 2, 2024 15:58
@talmakion
Copy link
Contributor Author

Fair call - I'll put together another PR for local-route tomorrow. This one should be already good to go if it passes review.

@talmakion talmakion force-pushed the feature/T6430-vrf-direct branch from af86aa5 to 14c5754 Compare July 5, 2024 10:58
@talmakion
Copy link
Contributor Author

Updated with smoke tests as requested by @nicolas-fort

@talmakion talmakion force-pushed the feature/T6430-vrf-direct branch from 14c5754 to 089e61b Compare July 7, 2024 07:34
Copy link

github-actions bot commented Jul 7, 2024


warning: Unused import pprint in src/op_mode/ipsec.py:16.

@talmakion
Copy link
Contributor Author

Noticed a minor issue with completion ordering & missing extras while working on the local-route version of this, fixed.

src/conf_mode/policy_route.py Outdated Show resolved Hide resolved
@talmakion talmakion force-pushed the feature/T6430-vrf-direct branch from 089e61b to 98c0ed7 Compare July 29, 2024 12:25
python/vyos/firewall.py Outdated Show resolved Hide resolved
… IDs

* PBR can only target table IDs up to 200 and the previous PR to extend the
  range was rejected
* PBR with this PR can now also target VRFs directly by name, working around
  targeting problems for VRF table IDs outside the overlapping 100-200 range
* Validation ensures rules can't target both a table ID and a VRF name
  (internally they are handled the same)
* Added a simple accessor (get_vrf_table_id) for runtime mapping a VRF name
  to table ID, based on vyos.ifconfig.interface._set_vrf_ct_zone().
  It does not replace that usage, as it deliberately does not handle non-VRF
  interface lookups (would fail with a KeyError).
* Added route table ID lookup dict, global route table and VRF table defs
  to vyos.defaults. Table ID references have been updated in code touched
  by this PR.
* Added a simple smoketest to validate 'set vrf' usage in PBR rules
@talmakion talmakion force-pushed the feature/T6430-vrf-direct branch from 98c0ed7 to adeac78 Compare July 30, 2024 03:48
@talmakion talmakion requested a review from c-po July 30, 2024 03:50
@talmakion
Copy link
Contributor Author

Updated as requested with central default table IDs, manual tests and smoketests all OK.

Additionally, in the spots where the table IDs were being defined as strings, they're now integers. Casts tweaked to match. They were always cast to ints anyway for the fwmark calculation. Multiple and overlapping table rules still work in tests.

Commit 452068c ("interfaces: T6592: moving an interface between VRF instances
failed") added a similar but more detailed implementation of get_vrf_table_id()
that was added in commit adeac78 of this PR. Move to the common available
implementation.
@c-po
Copy link
Member

c-po commented Jul 30, 2024

I did a last minor change in 9b99a01 which dropped your implementation of get_vrf_table_id, as we have this as a common utility function as of last week.

@c-po c-po merged commit 8b0f36e into vyos:current Jul 30, 2024
8 of 9 checks passed
Copy link

CI integration 👍 passed!

Details

CI logs

  • CLI Smoketests 👍 passed
  • Config tests 👍 passed
  • RAID1 tests 👍 passed

@talmakion talmakion deleted the feature/T6430-vrf-direct branch July 30, 2024 16:35
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