On Tue, Feb 14, 2023 at 12:39:05PM +0900, 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.
>
> Signed-off-by: Nobuhiro MIKI <nm...@yahoo-corp.jp>
aside: this patch was base64 encoded, which is slightly inconvenient
on my side (which is not important in itself). Curiously
the other patches in this series are not.
> ---
> NEWS | 3 +
> lib/ovs-router.c | 134 ++++++++++++++++++++++++--------
> ofproto/ofproto-tnl-unixctl.man | 9 ++-
> tests/ovs-router.at | 78 +++++++++++++++++++
> 4 files changed, 188 insertions(+), 36 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index fe6055a2700b..9d98e1573e3b 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -4,6 +4,9 @@ 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:
> + * 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..8f2587444034 100644
> --- a/lib/ovs-router.c
> +++ b/lib/ovs-router.c
> @@ -164,6 +164,47 @@ static void rt_init_match(struct match *match, uint32_t
> mark,
> match->flow.pkt_mark = mark;
> }
>
> +static int
Maybe bool is a more appropriate return type for this function?
Likewise for ovs_router_get_netdev_source_address().
But perhaps that is a cleanup for another time.
Also, there is a lot of comonality between verify_prefsrc()
and ovs_router_get_netdev_source_address(). I am curious to know
if you considered combining them, or otherwise sharing code between them?
> +verify_prefsrc(const struct in6_addr *ip6_dst,
> + const char output_bridge[],
> + struct in6_addr *prefsrc)
> +{
> + struct in6_addr *mask, *addr6;
> + int err, n_in6, i;
> + struct netdev *dev;
> +
> + err = netdev_open(output_bridge, NULL, &dev);
> + if (err) {
> + return err;
> + }
> +
> + err = netdev_get_addr_list(dev, &addr6, &mask, &n_in6);
> + if (err) {
> + goto out;
> + }
> +
> + err = ENOENT;
nit: If you move this to below the for loop...
> + for (i = 0; i < n_in6; i++) {
> + struct in6_addr a1, a2;
> + a1 = ipv6_addr_bitand(ip6_dst, &mask[i]);
> + a2 = ipv6_addr_bitand(prefsrc, &mask[i]);
> +
> + /* Check that the intarface has "prefsrc" and
> + * it is same broadcast domain with "ip6_dst". */
> + if (IN6_ARE_ADDR_EQUAL(prefsrc, &addr6[i]) &&
> + IN6_ARE_ADDR_EQUAL(&a1, &a2)) {
> + err = 0;
... then I don't think you need to set err here, as it will already
be set to zero from the call to netdev_get_addr_list.
> + goto out;
> + }
> + }
> +
> +out:
> + free(addr6);
> + free(mask);
> + netdev_close(dev);
> + return err;
> +}
> +
> int
> ovs_router_get_netdev_source_address(const struct in6_addr *ip6_dst,
> const char output_bridge[],
> @@ -217,7 +258,8 @@ 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;
> @@ -236,11 +278,21 @@ 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 (err && ipv6_addr_is_set(gw)) {
> - err = ovs_router_get_netdev_source_address(gw, output_bridge,
> +
> + if (ipv6_addr_is_set(ip6_src)) {
> + p->src_addr = *ip6_src;
> +
> + err = verify_prefsrc(ip6_dst, output_bridge, &p->src_addr);
> + if (err && ipv6_addr_is_set(gw)) {
> + err = verify_prefsrc(gw, output_bridge, &p->src_addr);
> + }
> + } 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);
> + }
> }
This seems very repetitive. Combining verify_prefsrc() and
ovs_router_get_netdev_source_address() might simplify things.
Another idea I had was to use a function pointer.
*compile tested only*
int (*f)(const struct in6_addr *ip6_dst,
const char output_bridge[],
struct in6_addr *prefsrc);
...
if (ipv6_addr_is_set(ip6_src)) {
p->src_addr = *ip6_src;
f = verify_prefsrc;
} else {
f = ovs_router_get_netdev_source_address;
}
err = f(ip6_dst, output_bridge, &p->src_addr);
if (err && ipv6_addr_is_set(gw)) {
err = f(gw, output_bridge, &p->src_addr);
}
> if (err) {
> struct ds ds = DS_EMPTY_INITIALIZER;
> @@ -274,7 +326,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,47 +395,64 @@ 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;
> + ovs_be32 gw = 0;
> + ovs_be32 src = 0;
> ovs_be32 ip;
> int err;
> + bool is_ipv6 = false;
> + char src6_s[IPV6_SCAN_LEN + 1];
> + int i;
In general I think it would be best if code moved towards
reverse-xmas tree sorting of local variables - longest line to shortest.
This seems to go in the opposite direction.
>
> if (scan_ipv4_route(argv[1], &ip, &plen)) {
> - ovs_be32 gw = 0;
> -
> - if (argc > 3) {
> - if (!ovs_scan(argv[3], "pkt_mark=%"SCNi32, &mark) &&
> - !ip_parse(argv[3], &gw)) {
> - unixctl_command_reply_error(conn, "Invalid pkt_mark or
> gateway");
> - return;
> - }
> - }
> in6_addr_set_mapped_ipv4(&ip6, ip);
> - if (gw) {
> - in6_addr_set_mapped_ipv4(&gw6, gw);
> - }
> plen += 96;
I think it would be cleaner to set is_ipv6 = false here
and not set it when it is declared.
> } else if (scan_ipv6_route(argv[1], &ip6, &plen)) {
> - if (argc > 3) {
> - if (!ovs_scan(argv[3], "pkt_mark=%"SCNi32, &mark) &&
> - !ipv6_parse(argv[3], &gw6)) {
> - unixctl_command_reply_error(conn, "Invalid pkt_mark or IPv6
> gateway");
> - return;
> - }
> - }
> + is_ipv6 = true;
> } else {
> 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;
> +
> + /* Parse optional parameters. */
> + for (i = 3; i < argc; i++) {
> + if (ovs_scan(argv[i], "pkt_mark=%"SCNi32, &mark)) {
> + continue;
> }
> +
> + if (is_ipv6) {
> + if (ovs_scan(argv[i], "src="IPV6_SCAN_FMT, src6_s) &&
> + ipv6_parse(src6_s, &src6)) {
> + continue;
> + }
> + if (ipv6_parse(argv[i], &gw6)) {
> + continue;
> + }
> + } else {
> + if (ovs_scan(argv[i], "src="IP_SCAN_FMT, IP_SCAN_ARGS(&src))) {
> + continue;
> + }
> + if (ip_parse(argv[i], &gw)) {
> + continue;
> + }
> + }
If I read this correctly then, before this patch gw (if present)
must come before pkt_mark (if present). A maximum of two arguments
are parsed for gw and pkt_mark. And pkt_mark, but not gw,
may occur twice (in which case gw is not parsed).
But after this patch pkt_mark, gw and src may come in any order.
And each may occur any number of times.
Perhaps my analysis is wrong. But my question is, are there
any backwards compatibility issues here?
Also, perhaps it would be best to refactor the parser, as a cleanup
patch, then add support for 'src'.
> +
> + unixctl_command_reply_error(conn, "Invalid parameters");
> + return;
> }
>
> - err = ovs_router_insert__(mark, plen + 32, false, &ip6, plen, argv[2],
> &gw6);
> + if (gw) {
> + in6_addr_set_mapped_ipv4(&gw6, gw);
> + }
> + if (src) {
> + in6_addr_set_mapped_ipv4(&src6, src);
> + }
> +
> + 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 +603,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/ofproto/ofproto-tnl-unixctl.man b/ofproto/ofproto-tnl-unixctl.man
> index 13a465119a90..3d8f8261f3c4 100644
> --- a/ofproto/ofproto-tnl-unixctl.man
> +++ b/ofproto/ofproto-tnl-unixctl.man
> @@ -1,8 +1,9 @@
> .SS "OPENVSWITCH TUNNELING COMMANDS"
> These commands query and modify OVS tunnel components.
> .
> -.IP "\fBovs/route/add ipv4_address/plen output_bridge [GW]\fR"
> -Adds ipv4_address/plen route to vswitchd routing table. output_bridge
> +.IP "\fBovs/route/add \fIip\fB/\fIplen\fB \fIoutput_bridge\fB \
> +[\fIgateway\fB] [pkt_mark=\fImark\fB] [src=\fIsrc_ip\fB]\fR"
> +Adds \fIip\fR/\fIplen\fR route to vswitchd routing table. \fIoutput_bridge\fR
> needs to be OVS bridge name. This command is useful if OVS cached
> routes does not look right.
> .
> @@ -10,8 +11,8 @@ routes does not look right.
> Print all routes in OVS routing table, This includes routes cached
> from system routing table and user configured routes.
> .
> -.IP "\fBovs/route/del ipv4_address/plen\fR"
> -Delete ipv4_address/plen route from OVS routing table.
> +.IP "\fBovs/route/del \fIip\fB/\fIplen\fR"
> +Delete \fIip\fR/\fIplen\fR route from OVS routing table.
> .
> .IP "\fBtnl/neigh/show\fR"
> .IP "\fBtnl/arp/show\fR"
Is the reason for the s/ipv4_address/ip/ portion of the man page changes
because both IPv4 and IPv6 are supported?
If so is that also the case before this patch?
If so, perhaps that change should be in a separate patch.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
