vyos-1x icon indicating copy to clipboard operation
vyos-1x copied to clipboard

backport: T4515: T4219: policy local-route6 and inbound-interface support

Open hensur opened this issue 3 years ago • 8 comments
trafficstars

Sorry for the inconvenience, I hadn't have much time the past few days. This should finally fix the issues from my backport attempt. I think I messed up a rebase or merge that caused this additional check to appear again.

https://github.com/vyos/vyos-1x/pull/1235

Change Summary

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://phabricator.vyos.net/Txxxx

Component(s) name

Proposed changes

How to test

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 avatar Mar 31 '22 11:03 hensur

vyos@vyos:~$ /usr/libexec/vyos/tests/smoke/cli/test_policy.py
test_access_list (__main__.TestPolicy) ... ok
test_access_list6 (__main__.TestPolicy) ... ok
test_as_path_list (__main__.TestPolicy) ... ok
test_community_list (__main__.TestPolicy) ... ok
test_delete_ipv4_ipv6_table_id (__main__.TestPolicy) ... ok
test_destination_ipv6_table_id (__main__.TestPolicy) ... ok
test_destination_table_id (__main__.TestPolicy) ... ok
test_extended_community_list (__main__.TestPolicy) ... ok
test_fwmark_ipv6_table_id (__main__.TestPolicy) ... ok
test_fwmark_sources_destination_ipv6_table_id (__main__.TestPolicy) ... ok
test_fwmark_sources_destination_table_id (__main__.TestPolicy) ... ok
test_fwmark_sources_ipv6_table_id (__main__.TestPolicy) ... ok
test_fwmark_sources_table_id (__main__.TestPolicy) ... ok
test_fwmark_table_id (__main__.TestPolicy) ... ok
test_iif_sources_ipv6_table_id (__main__.TestPolicy) ... ok
test_iif_sources_table_id (__main__.TestPolicy) ... ok
test_ipv6_table_id (__main__.TestPolicy) ... ok
test_large_community_list (__main__.TestPolicy) ... ok
test_multiple_commit_ipv4_table_id (__main__.TestPolicy) ... ok
test_prefix_list (__main__.TestPolicy) ... ok
test_prefix_list6 (__main__.TestPolicy) ... ok
test_table_id (__main__.TestPolicy) ... ok

----------------------------------------------------------------------
Ran 22 tests in 51.417s

OK

hensur avatar Mar 31 '22 13:03 hensur

Doesn't work properly Initial configuration:

set policy local-route rule 110 destination '203.0.113.0/24'
set policy local-route rule 110 fwmark '123'
set policy local-route rule 110 set table '170'
set policy local-route rule 110 source '192.0.5.22'

IP rule:

vyos@testrouter# sudo ip rule show
0:	from all lookup local
110:	from 192.0.5.22 to 203.0.113.0/24 fwmark 0x7b lookup 170
32766:	from all lookup main
32767:	from all lookup default

Delete source and we still see source address 192.0.5.22:

vyos@testrouter# delete policy local-route rule 110 source 
[edit]
vyos@testrouter# commit
[edit]
vyos@testrouter# sudo ip rule show
0:	from all lookup local
110:	from 192.0.5.22 to 203.0.113.0/24 fwmark 0x7b lookup 170
32766:	from all lookup main
32767:	from all lookup default
[edit]
vyos@testrouter# 

What we expect after deleting the source:

vyos@testrouter# sudo ip rule show
0:	from all lookup local
110:	from all to 203.0.113.0/24 fwmark 0x7b lookup 170
32766:	from all lookup main
32767:	from all lookup default

sever-sever avatar Apr 05 '22 18:04 sever-sever

@sever-sever Sorry about that. Good catch :) On removal of other options, fwmark should be set to the current value, however, this was a string type, while a list is returned if fwmark itself gets deleted. This happens for inbound-interface as well. Untyped python is still confusing me sometimes.

Both are fixed by 45734d25f6b4f930fbc572be7ab247a377e179bf I also added a smoketest and fixed two tests, which were inserted wrongly after my rebase (I think).

hensur avatar Apr 06 '22 11:04 hensur

@hensur it still incorrect behaviour

set policy local-route rule 110 destination '203.0.113.0/24'
set policy local-route rule 110 set table '100'
commit
set policy local-route rule 110 set table '101'
commit

Expected table 101 only:

110:	from all to 203.0.113.0/24 lookup 100
110:	from all to 203.0.113.0/24 lookup 101

Could you do more test? Thanks

sever-sever avatar Apr 08 '22 22:04 sever-sever

Oh, well. There never was a check for differences in the table during remove, so I didn't think to add one as well.

I just implemented one, along with a matching smoketest.

hensur avatar Apr 09 '22 11:04 hensur

@dmbaturin @zdc can you check as well?

andamasov avatar May 21 '22 10:05 andamasov

Tried to use this patch, running into issues with invalid commit application ordering between inbound-interface and the creation of the interface. To expand further I have a bonding interface with a VLAN on it similar to this:

interfaces {
    bonding bond0 {
        ...
        vif 10 {
            ...
        }
    }
}

When a policy local-route or local-route6 uses the bonding VLAN interface (bond0.10 in the example) as an inbound-interface like so:

policy {
    local-route {
        rule 1 {
            ...
            inbound-interface bond0.10
        }
    }
}

Configuration fails at boot time claiming the interface does not exist. Trying to repair the configuration requires creating the interface, committing, then adding the policy back before the config applies.

Edit: Upon further testing the issue occurs both on a pure (non-VLAN) bonding interface as well as an ethernet VLAN interface, suggesting that the incompatibility exists whenever the interface is created at 'runtime'. The same issue is present in 1.4 as well, with the bug report here.

initramfs avatar Sep 09 '22 13:09 initramfs

@initramfs I've backported the changes from https://github.com/vyos/vyos-1x/pull/1532/files as well. I guess that should fix the issue for 1.3 as well. Thank you!

hensur avatar Sep 20 '22 08:09 hensur