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 <nm...@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
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
