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

policy: T4151: Add policy ipv6-local-route #1144

Merged
merged 1 commit into from
Jan 20, 2022

Conversation

hensur
Copy link
Contributor

@hensur hensur commented Jan 8, 2022

Change Summary

Adds support for ip -6 rule policy based routing.
Also, extends the existing ipv4 implemenation with a destination key, which is translated as ip rule add to x.x.x.x/x rules.

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)

Component(s) name

policy

Proposed changes

vyos has had support for IPv4 policy based routing for a while. I'm missing the same functionality for IPv6. This change implements the same things for IPv6 as well.
It also adds the option to define the destination in policy routes.

How to test

I wrote several smoketests which hopefully cover all use-cases.
I also tested in a VM using these commands:

set policy ipv6-local-route rule 105 set table 100
set policy ipv6-local-route rule 105 source 2001:db8:26::/48
set policy ipv6-local-route rule 105 source 2001:db8:28::/48
set policy ipv6-local-route rule 105 destination 2001:db8:124::/48
set policy ipv6-local-route rule 105 destination 2001:db8:126::/48
set policy ipv6-local-route rule 105 fwmark 12
ip -6 rule show
105:	from 2001:db8:26::/48 to 2001:db8:124::/48 fwmark 0xc lookup 100
105:	from 2001:db8:26::/48 to 2001:db8:126::/48 fwmark 0xc lookup 100
105:	from 2001:db8:28::/48 to 2001:db8:124::/48 fwmark 0xc lookup 100
105:	from 2001:db8:28::/48 to 2001:db8:126::/48 fwmark 0xc lookup 100

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

@hensur hensur changed the title T4151: Add policy ipv6-local-route policy: T4151: Add policy ipv6-local-route Jan 9, 2022
@c-po
Copy link
Member

c-po commented Jan 9, 2022

Thanks for this PR I have two questions:

  • Why is it required to have local-route and ipv6-local-route?. Why is local-route insufficient? We could take a damn good guess if its either IPv4 or IPv6 - so one node should be sufficient
  • Your implementation extends the current one but I ask myself if we really required calls to node_changed(). Why don't we simply drop the entire iproute2 table and recreate it from scratch? This would make the entire code easier

@hensur
Copy link
Contributor Author

hensur commented Jan 9, 2022

  • You are right, I took some inspiration from other options under the policy tree, like route and ipv6-route. Therefore I decided it might be fitting to replicate this for local-route as well. I guess route and ipv6-route route might actually differ in the provided options though. I will merge both cases into local-route.
  • I thought about doing something like this as well. But I didn't want to impact existing or future implementations that also might make use of ip rule. There seems to be a call in https://github.com/vyos/vyos-1x/blob/current/src/conf_mode/policy-route.py#L111. I'm not sure if we break working behaviour by dropping the entire table. Also, this might create a short timeframe where no rules are applied, while incrementally removing them would make sure untouched rules will always be active.

@sever-sever
Copy link
Member

sever-sever commented Jan 9, 2022

  • Why is it required to have local-route and ipv6-local-route?. Why is local-route insufficient?

@c-po
I think the main reason because it is handled by ip rule and ip -6 rule
so there are 2 option:

  1. Clear all "ip" + "ip -6" rules if one of the rule is changed
  2. Add new syntax something like set policy local-route ipv4|ipv6

The rest of the policies includes firewall/frr configuration

@c-po
Copy link
Member

c-po commented Jan 9, 2022

  • Why is it required to have local-route and ipv6-local-route?. Why is local-route insufficient?

@c-po I think the main reason because it is handled by ip rule and ip -6 rule so there are 2 option:

  1. Clear all "ip" + "ip -6" rules if one of the rule is changed
  2. Add new syntax something like set policy local-route ipv4|ipv6

The rest of the policies includes firewall/frr configuration

I don't understand, if the local-route options contain IPv6 addresses we will sue ip -6 rule - if it does not contain IPv6 addresses we do not specify -6. A verify() stage must be added to not mix IPv4 and IPv6 in one local rule - et voila ;)

@hensur
Copy link
Contributor Author

hensur commented Jan 10, 2022

I started to merge both trees, but I'm not sure if this is possible anymore.
On further thought, this breaks fwmark rules without source or destination address.
How would one know whether those are intended to be set in the ipv4 or ipv6 table?

We could possibly introduce a fwmark and fwmarkv6 option, but I think if something like this has to be done, there is no reason to not just stick with separate configuration trees.

@c-po
Copy link
Member

c-po commented Jan 10, 2022

On further thought, this breaks fwmark rules without source or destination address.
How would one know whether those are intended to be set in the ipv4 or ipv6 table

This is a good one indeed ... maybe this is the corner case which justifies it's own tree? I am no fwmark expert.

@hensur
Copy link
Contributor Author

hensur commented Jan 10, 2022

Well, me neither. But I think having separate config trees will also provide more flexibility to implement other selectors that aren't related to IPv4/IPv6 addresses.
According to the man page, it should be possible to match on things like incoming/outgoing interface, source/destination port or protocol.

@c-po
Copy link
Member

c-po commented Jan 11, 2022

Lets proceed with two trees then. Can you please rename ipv6-local-route to local-route6 so it machtes static and static6 routes?

@hensur
Copy link
Contributor Author

hensur commented Jan 11, 2022

@c-po I've renamed the command.

src/conf_mode/policy-local-route.py Outdated Show resolved Hide resolved
src/conf_mode/policy-local-route.py Outdated Show resolved Hide resolved
src/conf_mode/policy-local-route.py Outdated Show resolved Hide resolved
src/conf_mode/policy-local-route.py Outdated Show resolved Hide resolved
@c-po
Copy link
Member

c-po commented Jan 13, 2022

Other then the requested changes we can merge it.

Adds support for `ip -6 rule` policy based routing.
Also, extends the existing ipv4 implemenation with a
`destination` key, which is translated as
`ip rule add to x.x.x.x/x` rules.

https://phabricator.vyos.net/T4151
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants