On 5/14/24 00:42, Ilya Maximets wrote: > On 5/14/24 00:11, William Tu wrote: >> Add route-table support for ipv4 dst via ipv6. One use case is BGP >> unnumbered, a mechanism that establishes peering sessions without the >> need to explicitly configure IPv4 addresses on the interfaces involved >> in the peering. Without using IPv4 address assignments, it uses >> link-local IPv6 addresses of the directly connected neighbors for >> peering purposes. For example, BGP might install the following route: >> $ ip route get 100.87.18.3 >> 100.87.18.3 via inet6 fe80::920a:84ff:fe9e:9570 \ >> dev br-phy src 100.87.18.6 >> >> Note that the v6 addr fe80::920a:84ff:fe9e:9570 is not being used in >> the packet header, but only used for lookup the out dev br-phy. >> Currently OVS can only support either all-ipv4 or all-ipv6, the patch >> adds support for such use case. >> >> Reported-at: >> https://mail.openvswitch.org/pipermail/ovs-discuss/2024-January/052908.html >> Acked-by: Simon Horman <[email protected]> >> Signed-off-by: William Tu <[email protected]> >> --- >> v3: add vxlan test, remove rfc >> v2: fix CI error >> --- >> lib/ovs-router.c | 34 ++++++++++++++-------------- >> lib/route-table.c | 21 ++++++++++++++++++ >> tests/ovs-router.at | 33 +++++++++++++++++++++++++++ >> tests/system-route.at | 24 ++++++++++++++++++++ >> tests/tunnel-push-pop.at | 48 ++++++++++++++++++++++++++++++++++++++++ >> 5 files changed, 142 insertions(+), 18 deletions(-)
I didn't look at the code changes too closely, but in case you wan't to re-spin the patch, I added a few comments for the test cases below. Best regards, Ilya Maximets. <snip> >> diff --git a/tests/ovs-router.at b/tests/ovs-router.at >> index b3314b3dff0d..f34dd3c99f5d 100644 >> --- a/tests/ovs-router.at >> +++ b/tests/ovs-router.at >> @@ -109,6 +109,39 @@ User: 2001:db8:beef::14/128 MARK 14 dev br0 GW >> 2001:db8:cafe::1 SRC 2001:db8:caf >> OVS_VSWITCHD_STOP >> AT_CLEANUP >> >> +AT_SETUP([appctl - route/add with ipv4 via ipv6]) We should probably also add a few cases for the lookup and del, not just the add. >> +AT_KEYWORDS([ovs_router]) >> +OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=dummy]) >> +AT_CHECK([ovs-appctl netdev-dummy/ip4addr br0 192.168.9.2/24], [0], [OK >> +]) >> +AT_CHECK([ovs-appctl netdev-dummy/ip4addr br0 192.168.9.3/24], [0], [OK >> +]) >> + >> +# defualt pick the first IP, 192.168.9.2, as src >> +AT_CHECK([ovs-appctl ovs/route/add 192.168.9.10/32 br0 >> fe80::920a:84ff:beef:9510], [0], [OK >> +]) >> +AT_CHECK([ovs-appctl ovs/route/add 192.168.9.11/32 br0 >> fe80::920a:84ff:beef:9511 src=192.168.9.2], [0], [OK >> +]) >> +AT_CHECK([ovs-appctl ovs/route/add 192.168.9.12/32 br0 >> fe80::920a:84ff:beef:9512 src=192.168.9.3], [0], [OK >> +]) >> +AT_CHECK([ovs-appctl ovs/route/add 192.168.9.13/32 br0 >> fe80::920a:84ff:beef:9513 pkt_mark=13 src=192.168.9.3], [0], [OK >> +]) Might be better to wrap some of the longer lines, e.g. after the bridge name. >> + >> +AT_CHECK([ovs-appctl ovs/route/show | grep User | grep beef | sort], [0], >> [dnl >> +User: 192.168.9.10/32 dev br0 GW fe80::920a:84ff:beef:9510 SRC 192.168.9.2 >> +User: 192.168.9.11/32 dev br0 GW fe80::920a:84ff:beef:9511 SRC 192.168.9.2 >> +User: 192.168.9.12/32 dev br0 GW fe80::920a:84ff:beef:9512 SRC 192.168.9.3 >> +User: 192.168.9.13/32 MARK 13 dev br0 GW fe80::920a:84ff:beef:9513 SRC >> 192.168.9.3 >> +]) >> + >> +# DST and SRC must be in the same subnet domain >> +AT_CHECK([ovs-appctl ovs/route/add 192.168.10.12/32 br0 >> fe80::920a:84ff:beef:9512 src=192.168.9.3], [2], [], [dnl >> +Error while inserting route. >> +ovs-appctl: ovs-vswitchd: server returned an error >> +]) >> +OVS_VSWITCHD_STOP >> +AT_CLEANUP >> + >> AT_SETUP([appctl - route/lookup]) >> AT_KEYWORDS([ovs_router]) >> OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=dummy]) >> diff --git a/tests/system-route.at b/tests/system-route.at >> index c0ecad6cfb49..fd698321b401 100644 >> --- a/tests/system-route.at >> +++ b/tests/system-route.at >> @@ -65,6 +65,30 @@ Cached: fc00:db8:beef::13/128 dev br0 GW fc00:db8:cafe::1 >> SRC fc00:db8:cafe::2]) >> OVS_TRAFFIC_VSWITCHD_STOP >> AT_CLEANUP >> >> +AT_SETUP([ovs-route - add system route ipv4 via ipv6]) >> +AT_KEYWORDS([route]) >> +OVS_TRAFFIC_VSWITCHD_START() >> +AT_CHECK([ip link set br0 up]) >> + >> +AT_CHECK([ip addr add 192.168.9.2/24 dev br0], [0], [stdout]) >> +AT_CHECK([ip addr add 192.168.9.3/24 dev br0], [0], [stdout]) >> + >> +AT_CHECK([ip -6 addr add fc00:db8:cafe::2/64 dev br0], [0], [stdout]) >> +AT_CHECK([ip -6 addr add fc00:db8:cafe::3/64 dev br0], [0], [stdout]) >> + >> +AT_CHECK([ip route add 192.168.9.12/32 dev br0 via inet6 >> fe80::920a:84ff:fe9e:9512 src 192.168.9.2], [0], [stdout]) >> +AT_CHECK([ip route add 192.168.9.13/32 dev br0 via inet6 >> fe80::920a:84ff:fe9e:9513 src 192.168.9.3], [0], [stdout]) maybe wrap these lines. >> + >> +OVS_WAIT_UNTIL_EQUAL([ovs-appctl ovs/route/show | grep -E >> '192.168.9.1[[23]]/32' | sort], [dnl >> +Cached: 192.168.9.12/32 dev br0 GW fe80::920a:84ff:fe9e:9512 SRC 192.168.9.2 >> +Cached: 192.168.9.13/32 dev br0 GW fe80::920a:84ff:fe9e:9513 SRC >> 192.168.9.3]) >> + >> +AT_CHECK([ovs-appctl ovs/route/add 192.168.9.14/32 br0 >> fe80::920a:84ff:fe9e:9514], [0], [OK >> +]) Not sure why this add is here as we're not checking the result. >> + >> +OVS_TRAFFIC_VSWITCHD_STOP >> +AT_CLEANUP >> + >> dnl Checks that OVS doesn't use routes from non-standard tables. >> AT_SETUP([ovs-route - route tables]) >> AT_KEYWORDS([route]) >> diff --git a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at >> index 508737c53ec6..0c8f3862cce9 100644 >> --- a/tests/tunnel-push-pop.at >> +++ b/tests/tunnel-push-pop.at >> @@ -196,6 +196,54 @@ OVS_WAIT_UNTIL([test `ovs-pcap p0.pcap | grep >> 100022eb0000000120000237 | wc -l` >> OVS_VSWITCHD_STOP >> AT_CLEANUP >> >> +AT_SETUP([tunnel_push_pop - v4 via v6 route]) Overall, I'd suggest to format this test in a way some of the newer tests are written. See 'recirculation after encapsulation' or 'local_ip configuration' tests for example. >> + >> +OVS_VSWITCHD_START([add-port br0 p0 -- set Interface p0 type=dummy >> ofport_request=1 other-config:hwaddr=aa:55:aa:55:00:00]) >> +AT_CHECK([ovs-vsctl add-br int-br -- set bridge int-br >> datapath_type=dummy], [0]) >> +AT_CHECK([ovs-vsctl add-port int-br t1 -- set Interface t1 type=vxlan \ >> + options:remote_ip=1.1.2.92 options:key=123 >> ofport_request=1\ >> + ], [0]) >> + >> +AT_CHECK([ovs-appctl dpif/show], [0], [dnl >> +dummy@ovs-dummy: hit:0 missed:0 >> + br0: >> + br0 65534/100: (dummy-internal) >> + p0 1/1: (dummy) >> + int-br: >> + int-br 65534/2: (dummy-internal) >> + t1 1/4789: (vxlan: key=123, remote_ip=1.1.2.92) >> +]) >> + >> +AT_CHECK([ovs-ofctl add-flow br0 action=normal]) >> + >> +dnl Setup dummy interface IP addresses. >> +AT_CHECK([ovs-appctl netdev-dummy/ip4addr br0 1.1.2.88/24], [0], [OK > > Hmm. I'm a little confused by this. The whole point of v4 over v6 > routing is that the network we're routing the packets over doesn't > have IPv4 routing, i.e. br0 bridge that is containing an external port > should not be part of IPv4 network, i.e. the address should not be > routable. Otherwise, why do we even need to use v4 over v6, if there > is a native v4? > > Or am I missing something? > > In the reporter's case there was an IPv4 address on br-phy, but it > was a /32 address and there was no actual route between tunnel > endpoints, except via v6 gateway. > > Best regards, Ilya Maixmets. > >> +]) >> +AT_CHECK([ovs-appctl netdev-dummy/ip6addr br0 2001:cafe::88/64], [0], [OK >> +]) >> +dnl Add a static v4 via v6 route >> +AT_CHECK([ovs-appctl ovs/route/add 1.1.2.92/32 br0 2001:cafe::10], [0], [OK >> +]) >> + >> +AT_CHECK([ovs-appctl ovs/route/show | grep br0 | sort], [0], [dnl >> +Cached: 1.1.2.0/24 dev br0 SRC 1.1.2.88 local >> +Cached: 2001:cafe::/64 dev br0 SRC 2001:cafe::88 local >> +User: 1.1.2.92/32 dev br0 GW 2001:cafe::10 SRC 1.1.2.88 >> +]) >> + >> +AT_CHECK([ovs-appctl tnl/neigh/set br0 1.1.2.92 f8:bc:12:44:34:c8], [0], [OK >> +]) Beside the other comments, we should not set neighbor cache manually here. We should generate Neighbor Advertisement packet from p0 port so the bridge learns where the 2001:cafe::10 is. Tunnel should be able to work with only this information. >> + >> +dnl Check VXLAN tunnel push >> +AT_CHECK([ovs-ofctl add-flow int-br action=1]) >> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy >> 'in_port(2),eth(src=f8:bc:12:44:34:b6,dst=aa:55:aa:55:00:00),eth_type(0x0800),ipv4(src=1.1.3.88,dst=1.1.3.112,proto=47,tos=0,ttl=64,frag=no)'], >> [0], [stdout]) >> +AT_CHECK([tail -1 stdout], [0], >> + [Datapath actions: >> tnl_push(tnl_port(4789),header(size=50,type=4,eth(dst=f8:bc:12:44:34:c8,src=aa:55:aa:55:00:00,dl_type=0x0800),ipv4(src=1.1.2.88,dst=1.1.2.92,proto=17,tos=0,ttl=64,frag=0x4000),udp(src=0,dst=4789,csum=0x0),vxlan(flags=0x8000000,vni=0x7b)),out_port(100)),1 >> +]) We should preserve more information on the trace here including the routing decisions and addresses chosen. See the previously mentioned tests for example. >> + >> +OVS_VSWITCHD_STOP >> +AT_CLEANUP >> + >> AT_SETUP([tunnel_push_pop - action]) >> >> OVS_VSWITCHD_START([add-port br0 p0 -- set Interface p0 type=dummy >> ofport_request=1 other-config:hwaddr=aa:55:aa:55:00:00]) > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
