On 2023/02/10 20:16, Eelco Chaudron wrote:
> On 7 Feb 2023, at 7:48, Nobuhiro MIKI wrote:
>
>> When adding a route with ovs/route/add command, the source address
>> in "ovs_router_entry" structure is always the FIRST address that the
>> interface has. See "ovs_router_get_netdev_source_address"
>> function for more information.
>>
>> If an interface has multiple ipv4 and/or ipv6 addresses, there are use
>> cases where the user wants to control the source address. This patch
>> therefore addresses this issue by adding a src parameter.
>>
>> Note that same constraints also exist when caching routes from
>> Kernel FIB with Netlink, but are not dealt with in this patch.
>
> Thanks for this patch! Can you also update the man pages for routes, i.e.
> ofproto-tnl-unixctl.man?
>
> See some more comments inline below.
>
> Cheers,
>
> Eelco
Hi,
Thanks for your review.
Sure. I'll update the ofproto-tnl-unixctl.man in the next patch.
Best Regards,
Nobuhiro MIKI
>> + - ovs-appctl:
>> + * "ovs-appctl ovs/route/add" command can now support "src" option.
>
>
> * Add support for selecting the source address with the “ovs-appctl
> ovs/route/add" command.
I'll fix it.
>> @@ -236,8 +237,14 @@ ovs_router_insert__(uint32_t mark, uint8_t priority,
>> bool local,
>> p->plen = plen;
>> p->local = local;
>> p->priority = priority;
>> - err = ovs_router_get_netdev_source_address(ip6_dst, output_bridge,
>> - &p->src_addr);
>> +
>> + if (ipv6_addr_is_set(ip6_src)) {
>> + p->src_addr = *ip6_src;
>
> We need some verification, as any IP is accepted now. See the test cases
> below.
I think src should be chosen from the IP addresses that the interface has.
I'll add the validation.
>> @@ -342,6 +350,7 @@ ovs_router_add(struct unixctl_conn *conn, int argc,
>> const char *argv[], void *aux OVS_UNUSED)
>> {
>> struct in6_addr gw6 = in6addr_any;
>> + struct in6_addr src6 = in6addr_any;
>> struct in6_addr ip6;
>> uint32_t mark = 0;
>> unsigned int plen;
>> @@ -350,11 +359,27 @@ ovs_router_add(struct unixctl_conn *conn, int argc,
>>
>> if (scan_ipv4_route(argv[1], &ip, &plen)) {
>> ovs_be32 gw = 0;
>> + ovs_be32 src = 0;
>>
>> if (argc > 3) {
>> - if (!ovs_scan(argv[3], "pkt_mark=%"SCNi32, &mark) &&
>> + if (!ovs_scan(argv[3], "src="IP_SCAN_FMT, IP_SCAN_ARGS(&src)) &&
>> + !ovs_scan(argv[3], "pkt_mark=%"SCNi32, &mark) &&
>> !ip_parse(argv[3], &gw)) {
>> - unixctl_command_reply_error(conn, "Invalid pkt_mark or
>> gateway");
>> + unixctl_command_reply_error(
>> + conn, "Invalid src, pkt_mark or gateway");
>> + return;
>> + }
>> + }
>> + if (argc > 4) {
>> + if (!ovs_scan(argv[4], "src="IP_SCAN_FMT, IP_SCAN_ARGS(&src)) &&
>> + !ovs_scan(argv[4], "pkt_mark=%"SCNi32, &mark)) {
>> + unixctl_command_reply_error(conn, "Invalid src or
>> pkt_mark");
>> + return;
>> + }
>> + }
>> + if (argc > 5) {
>> + if (!ovs_scan(argv[5], "src="IP_SCAN_FMT, IP_SCAN_ARGS(&src))) {
>> + unixctl_command_reply_error(conn, "Invalid src");
>> return;
>> }
>> }
>> @@ -362,12 +387,35 @@ ovs_router_add(struct unixctl_conn *conn, int argc,
>> if (gw) {
>> in6_addr_set_mapped_ipv4(&gw6, gw);
>> }
>> + if (src) {
>> + in6_addr_set_mapped_ipv4(&src6, src);
>> + }
>> plen += 96;
>> } else if (scan_ipv6_route(argv[1], &ip6, &plen)) {
>> + char src6_s[IPV6_SCAN_LEN + 1];
>> +
>> if (argc > 3) {
>> - if (!ovs_scan(argv[3], "pkt_mark=%"SCNi32, &mark) &&
>> + if (!(ovs_scan(argv[3], "src="IPV6_SCAN_FMT, src6_s) &&
>> + ipv6_parse(src6_s, &src6)) &&
>> + !ovs_scan(argv[3], "pkt_mark=%"SCNi32, &mark) &&
>> !ipv6_parse(argv[3], &gw6)) {
>> - unixctl_command_reply_error(conn, "Invalid pkt_mark or IPv6
>> gateway");
>> + unixctl_command_reply_error(
>> + conn, "Invalid src, pkt_mark or IPv6 gateway");
>> + return;
>> + }
>> + }
>> + if (argc > 4) {
>> + if (!(ovs_scan(argv[4], "src="IPV6_SCAN_FMT, src6_s) &&
>> + ipv6_parse(src6_s, &src6)) &&
>> + !ovs_scan(argv[4], "pkt_mark=%"SCNi32, &mark)) {
>> + unixctl_command_reply_error(conn, "Invalid src or
>> pkt_mark");
>> + return;
>> + }
>> + }
>> + if (argc > 5) {
>> + if (!(ovs_scan(argv[5], "src="IPV6_SCAN_FMT, src6_s) &&
>> + ipv6_parse(src6_s, &src6))) {
>> + unixctl_command_reply_error(conn, "Invalid src");
>> return;
>> }
>> }
>
> I think this function needs some proper refactoring, as it has become very
> messy.
> Guess we should first parse all arguments, and then do the verification.
OK. I will refactor the code.
>> diff --git a/tests/ovs-router.at b/tests/ovs-router.at
>> index 6dacc2954bc6..d4e39b4dd995 100644
>> --- a/tests/ovs-router.at
>> +++ b/tests/ovs-router.at
>> @@ -12,6 +12,52 @@ AT_CHECK([ovs-appctl ovs/route/add 1.1.1.0/24 br0
>> 2.2.2.10], [0], [OK
>> OVS_VSWITCHD_STOP
>> AT_CLEANUP
>>
>> +AT_SETUP([appctl - route/add with src - ipv4])
>> +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
>> +])
>> +AT_CHECK([ovs-appctl ovs/route/add 192.168.10.12/32 br0 192.168.9.1
>> src=192.168.9.3], [0], [OK
>> +])
>> +AT_CHECK([ovs-appctl ovs/route/add 192.168.10.13/32 br0 192.168.9.1
>> pkt_mark=13 src=192.168.9.3], [0], [OK
>> +])
>> +AT_CHECK([ovs-appctl ovs/route/add 192.168.10.14/32 br0 192.168.9.1
>> pkt_mark=14 src=192.168.9.2], [0], [OK
>> +])
>
> I’ve added the below, and the test case is also passing (for this AT_CHECK).
> We might need to add a check, see the earlier comment.
>
>
> AT_CHECK([ovs-appctl ovs/route/add 192.168.10.14/32 br0 192.168.9.1
> pkt_mark=14 src=192.168.9.20], [0], [OK
> ])
>
I think the test case should fail because the src option is an IP address
that is not assigned to br0. Thank you. I will add a validation.
>> +AT_CHECK([ovs-appctl ovs/route/add 192.168.10.15/32 br0 192.168.9.1
>> src=foo.bar.9.200], [2], [], [dnl
>> +Invalid src or pkt_mark
>> +ovs-appctl: ovs-vswitchd: server returned an error
>> +])
>> +AT_CHECK([ovs-appctl ovs/route/show | grep User | grep 192.168.10 | sort],
>> [0], [dnl
>> +User: 192.168.10.12/32 dev br0 GW 192.168.9.1 SRC 192.168.9.3
>> +User: 192.168.10.13/32 MARK 13 dev br0 GW 192.168.9.1 SRC 192.168.9.3
>> +User: 192.168.10.14/32 MARK 14 dev br0 GW 192.168.9.1 SRC 192.168.9.2
>> +])
>> +OVS_VSWITCHD_STOP
>> +AT_CLEANUP
>> +
>> +AT_SETUP([appctl - route/add with src - ipv6])
>> +AT_KEYWORDS([ovs_router])
>> +OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=dummy])
>> +AT_CHECK([ovs-appctl netdev-dummy/ip6addr br0 2001:db8:cafe::2/64], [0], [OK
>> +])
>> +AT_CHECK([ovs-appctl netdev-dummy/ip6addr br0 2001:db8:cafe::3/64], [0], [OK
>> +])
>> +AT_CHECK([ovs-appctl ovs/route/add 2001:db8:beef::12/128 br0
>> 2001:db8:cafe::1 src=2001:db8:cafe::3], [0], [OK
>> +])
>> +AT_CHECK([ovs-appctl ovs/route/add 2001:db8:beef::13/128 br0
>> 2001:db8:cafe::1 pkt_mark=13 src=2001:db8:cafe::3], [0], [OK
>> +])
>> +AT_CHECK([ovs-appctl ovs/route/add 2001:db8:beef::14/128 br0
>> 2001:db8:cafe::1 pkt_mark=14 src=2001:db8:cafe::2], [0], [OK
>> +])
>
> Do we need an invalid and unknown source IP added to this test case also?
>
Yes, that test case is missing. Thank you.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
