diff mbox series

[ovs-dev,v2,1/2] ovs-router: Introduce src option in ovs/route/add command.

Message ID 20230207064816.79821-2-nmiki@yahoo-corp.jp
State Superseded
Headers show
Series Add support for preferred src address in ovs-router. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

Nobuhiro MIKI Feb. 7, 2023, 6:48 a.m. UTC 
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.

Signed-off-by: Nobuhiro MIKI <nmiki@yahoo-corp.jp>
---
 NEWS                |  2 ++
 lib/ovs-router.c    | 79 ++++++++++++++++++++++++++++++++++-----------
 tests/ovs-router.at | 46 ++++++++++++++++++++++++++
 3 files changed, 109 insertions(+), 18 deletions(-)

Comments

Eelco Chaudron Feb. 10, 2023, 11:16 a.m. UTC | #1 
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

> Signed-off-by: Nobuhiro MIKI <nmiki@yahoo-corp.jp>
> ---
>  NEWS                |  2 ++
>  lib/ovs-router.c    | 79 ++++++++++++++++++++++++++++++++++-----------
>  tests/ovs-router.at | 46 ++++++++++++++++++++++++++
>  3 files changed, 109 insertions(+), 18 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index fe6055a2700b..929437733e3d 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -4,6 +4,8 @@ Post-v3.1.0
>       * OVS now collects per-interface upcall statistics that can be obtained
>         via 'ovs-appctl dpctl/show -s' or the interface's statistics column
>         in OVSDB.  Available with upstream kernel 6.2+.
> +   - 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.


>
>  v3.1.0 - xx xxx xxxx
> diff --git a/lib/ovs-router.c b/lib/ovs-router.c
> index 5d0fbd503e9e..fc95a91012f9 100644
> --- a/lib/ovs-router.c
> +++ b/lib/ovs-router.c
> @@ -217,12 +217,13 @@ static int
>  ovs_router_insert__(uint32_t mark, uint8_t priority, bool local,
>                      const struct in6_addr *ip6_dst,
>                      uint8_t plen, const char output_bridge[],
> -                    const struct in6_addr *gw)
> +                    const struct in6_addr *gw,
> +                    const struct in6_addr *ip6_src)
>  {
>      const struct cls_rule *cr;
>      struct ovs_router_entry *p;
>      struct match match;
> -    int err;
> +    int err = 0;
>
>      rt_init_match(&match, mark, ip6_dst, plen);
>
> @@ -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.

> +    } else {
> +        err = ovs_router_get_netdev_source_address(ip6_dst, output_bridge,
> +                                                   &p->src_addr);
> +    }
> +
>      if (err && ipv6_addr_is_set(gw)) {
>          err = ovs_router_get_netdev_source_address(gw, output_bridge,
>                                                     &p->src_addr);
> @@ -274,7 +281,8 @@ ovs_router_insert(uint32_t mark, const struct in6_addr *ip_dst, uint8_t plen,
>  {
>      if (use_system_routing_table) {
>          uint8_t priority = local ? plen + 64 : plen;
> -        ovs_router_insert__(mark, priority, local, ip_dst, plen, output_bridge, gw);
> +        ovs_router_insert__(mark, priority, local, ip_dst, plen,
> +                            output_bridge, gw, &in6addr_any);
>      }
>  }
>
> @@ -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.

> @@ -375,14 +423,9 @@ ovs_router_add(struct unixctl_conn *conn, int argc,
>          unixctl_command_reply_error(conn, "Invalid parameters");
>          return;
>      }
> -    if (argc > 4) {
> -        if (!ovs_scan(argv[4], "pkt_mark=%"SCNi32, &mark)) {
> -            unixctl_command_reply_error(conn, "Invalid pkt_mark");
> -            return;
> -        }
> -    }
>
> -    err = ovs_router_insert__(mark, plen + 32, false, &ip6, plen, argv[2], &gw6);
> +    err = ovs_router_insert__(mark, plen + 32, false, &ip6, plen, argv[2],
> +                              &gw6, &src6);
>      if (err) {
>          unixctl_command_reply_error(conn, "Error while inserting route.");
>      } else {
> @@ -533,8 +576,8 @@ ovs_router_init(void)
>          classifier_init(&cls, NULL);
>          unixctl_command_register("ovs/route/add",
>                                   "ip_addr/prefix_len out_br_name [gw] "
> -                                 "[pkt_mark=mark]",
> -                                 2, 4, ovs_router_add, NULL);
> +                                 "[pkt_mark=mark] [src=src_ip_addr]",
> +                                 2, 5, ovs_router_add, NULL);
>          unixctl_command_register("ovs/route/show", "", 0, 0,
>                                   ovs_router_show, NULL);
>          unixctl_command_register("ovs/route/del", "ip_addr/prefix_len "
> 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
])


> +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?

> +AT_CHECK([ovs-appctl ovs/route/show | grep User | grep 2001:db8:beef | sort], [0], [dnl
> +User: 2001:db8:beef::12/128 dev br0 GW 2001:db8:cafe::1 SRC 2001:db8:cafe::3
> +User: 2001:db8:beef::13/128 MARK 13 dev br0 GW 2001:db8:cafe::1 SRC 2001:db8:cafe::3
> +User: 2001:db8:beef::14/128 MARK 14 dev br0 GW 2001:db8:cafe::1 SRC 2001:db8:cafe::2
> +])
> +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])
> -- 
> 2.31.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Nobuhiro MIKI Feb. 13, 2023, 3:05 a.m. UTC | #2 
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.
diff mbox series

Patch

diff --git a/NEWS b/NEWS
index fe6055a2700b..929437733e3d 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,8 @@  Post-v3.1.0
      * OVS now collects per-interface upcall statistics that can be obtained
        via 'ovs-appctl dpctl/show -s' or the interface's statistics column
        in OVSDB.  Available with upstream kernel 6.2+.
+   - ovs-appctl:
+     * "ovs-appctl ovs/route/add" command can now support "src" option.
 
 
 v3.1.0 - xx xxx xxxx
diff --git a/lib/ovs-router.c b/lib/ovs-router.c
index 5d0fbd503e9e..fc95a91012f9 100644
--- a/lib/ovs-router.c
+++ b/lib/ovs-router.c
@@ -217,12 +217,13 @@  static int
 ovs_router_insert__(uint32_t mark, uint8_t priority, bool local,
                     const struct in6_addr *ip6_dst,
                     uint8_t plen, const char output_bridge[],
-                    const struct in6_addr *gw)
+                    const struct in6_addr *gw,
+                    const struct in6_addr *ip6_src)
 {
     const struct cls_rule *cr;
     struct ovs_router_entry *p;
     struct match match;
-    int err;
+    int err = 0;
 
     rt_init_match(&match, mark, ip6_dst, plen);
 
@@ -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;
+    } else {
+        err = ovs_router_get_netdev_source_address(ip6_dst, output_bridge,
+                                                   &p->src_addr);
+    }
+
     if (err && ipv6_addr_is_set(gw)) {
         err = ovs_router_get_netdev_source_address(gw, output_bridge,
                                                    &p->src_addr);
@@ -274,7 +281,8 @@  ovs_router_insert(uint32_t mark, const struct in6_addr *ip_dst, uint8_t plen,
 {
     if (use_system_routing_table) {
         uint8_t priority = local ? plen + 64 : plen;
-        ovs_router_insert__(mark, priority, local, ip_dst, plen, output_bridge, gw);
+        ovs_router_insert__(mark, priority, local, ip_dst, plen,
+                            output_bridge, gw, &in6addr_any);
     }
 }
 
@@ -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;
             }
         }
@@ -375,14 +423,9 @@  ovs_router_add(struct unixctl_conn *conn, int argc,
         unixctl_command_reply_error(conn, "Invalid parameters");
         return;
     }
-    if (argc > 4) {
-        if (!ovs_scan(argv[4], "pkt_mark=%"SCNi32, &mark)) {
-            unixctl_command_reply_error(conn, "Invalid pkt_mark");
-            return;
-        }
-    }
 
-    err = ovs_router_insert__(mark, plen + 32, false, &ip6, plen, argv[2], &gw6);
+    err = ovs_router_insert__(mark, plen + 32, false, &ip6, plen, argv[2],
+                              &gw6, &src6);
     if (err) {
         unixctl_command_reply_error(conn, "Error while inserting route.");
     } else {
@@ -533,8 +576,8 @@  ovs_router_init(void)
         classifier_init(&cls, NULL);
         unixctl_command_register("ovs/route/add",
                                  "ip_addr/prefix_len out_br_name [gw] "
-                                 "[pkt_mark=mark]",
-                                 2, 4, ovs_router_add, NULL);
+                                 "[pkt_mark=mark] [src=src_ip_addr]",
+                                 2, 5, ovs_router_add, NULL);
         unixctl_command_register("ovs/route/show", "", 0, 0,
                                  ovs_router_show, NULL);
         unixctl_command_register("ovs/route/del", "ip_addr/prefix_len "
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
+])
+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
+])
+AT_CHECK([ovs-appctl ovs/route/show | grep User | grep 2001:db8:beef | sort], [0], [dnl
+User: 2001:db8:beef::12/128 dev br0 GW 2001:db8:cafe::1 SRC 2001:db8:cafe::3
+User: 2001:db8:beef::13/128 MARK 13 dev br0 GW 2001:db8:cafe::1 SRC 2001:db8:cafe::3
+User: 2001:db8:beef::14/128 MARK 14 dev br0 GW 2001:db8:cafe::1 SRC 2001:db8:cafe::2
+])
+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])