> On Dec 28, 2016, at 1:44 AM, Pravin B Shelar <pshe...@ovn.org> wrote:
>
> OVS router is basically partial copy of linux kernel FIB.
> kernel routing table use skb-mark along with usual routing
“use”->”uses”
Could you elaborate a bit here, is the kernel doing a simple exact match of the
whole mark? Is the value 0 special in any way?
> parameters. Following patch brings in support for skb-mark
> to ovs-router so that we can lookup route according to
> flow skb-mark.
>
So by default we use the flow’s skb_mark, but if the ‘egress_pkt_mark’ is set,
then we use that?
> Signed-off-by: Pravin B Shelar <pshe...@ovn.org>
> ---
> lib/netdev-vport.c | 4 +-
> lib/ovs-router.c | 192 ++++++++++++++++++++++++++++++++++---------
> lib/ovs-router.h | 6 +-
> lib/route-table.c | 2 +-
> ofproto/ofproto-dpif-sflow.c | 2 +-
> ofproto/ofproto-dpif-xlate.c | 2 +-
> tests/ovs-router.at | 22 +++++
> tests/tunnel-push-pop.at | 3 +
> 8 files changed, 188 insertions(+), 45 deletions(-)
>
> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
> index 25dd0e8..54db559 100644
> --- a/lib/netdev-vport.c
> +++ b/lib/netdev-vport.c
> @@ -259,10 +259,12 @@ tunnel_check_status_change__(struct netdev_vport
> *netdev)
> bool status = false;
> struct in6_addr *route;
> struct in6_addr gw;
> + uint32_t mark;
>
> iface[0] = '\0';
> route = &netdev->tnl_cfg.ipv6_dst;
> - if (ovs_router_lookup(route, iface, NULL, &gw)) {
> + mark = netdev->tnl_cfg.egress_pkt_mark;
> + if (ovs_router_lookup(mark, route, iface, NULL, &gw)) {
> struct netdev *egress_netdev;
>
> if (!netdev_open(iface, NULL, &egress_netdev)) {
> diff --git a/lib/ovs-router.c b/lib/ovs-router.c
> index 935b60a..7300b36 100644
> --- a/lib/ovs-router.c
> +++ b/lib/ovs-router.c
> @@ -45,6 +45,11 @@
> #include "unaligned.h"
> #include "unixctl.h"
> #include "util.h"
> +#include "openvswitch/vlog.h"
> +
> +VLOG_DEFINE_THIS_MODULE(ovs_route);
> +
This should be ‘ovs_router’ instead.
> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
>
> static struct ovs_mutex mutex = OVS_MUTEX_INITIALIZER;
> static struct classifier cls;
> @@ -57,6 +62,7 @@ struct ovs_router_entry {
> struct in6_addr src_addr;
> uint8_t plen;
> uint8_t priority;
> + uint32_t mark;
> };
>
> static struct ovs_router_entry *
> @@ -88,11 +94,12 @@ ovs_router_lookup_fallback(const struct in6_addr
> *ip6_dst, char output_bridge[],
> }
>
> bool
> -ovs_router_lookup(const struct in6_addr *ip6_dst, char output_bridge[],
> +ovs_router_lookup(uint32_t mark, const struct in6_addr *ip6_dst,
> + char output_bridge[],
> struct in6_addr *src, struct in6_addr *gw)
> {
> const struct cls_rule *cr;
> - struct flow flow = {.ipv6_dst = *ip6_dst};
> + struct flow flow = {.ipv6_dst = *ip6_dst, .pkt_mark = mark};
>
> cr = classifier_lookup(&cls, OVS_VERSION_MAX, &flow, NULL);
> if (cr) {
> @@ -115,7 +122,8 @@ rt_entry_free(struct ovs_router_entry *p)
> free(p);
> }
>
> -static void rt_init_match(struct match *match, const struct in6_addr
> *ip6_dst,
> +static void rt_init_match(struct match *match, uint32_t mark,
> + const struct in6_addr *ip6_dst,
> uint8_t plen)
> {
> struct in6_addr dst;
> @@ -127,6 +135,8 @@ static void rt_init_match(struct match *match, const
> struct in6_addr *ip6_dst,
> memset(match, 0, sizeof *match);
> match->flow.ipv6_dst = dst;
> match->wc.masks.ipv6_dst = mask;
> + match->wc.masks.pkt_mark = UINT32_MAX;
> + match->flow.pkt_mark = mark;
So by default, when the route entry does not specify a mark, an exact match on
zero is made?
> }
>
> static int
> @@ -178,7 +188,8 @@ out:
> }
>
> static int
> -ovs_router_insert__(uint8_t priority, const struct in6_addr *ip6_dst,
> +ovs_router_insert__(uint32_t mark, uint8_t priority,
> + const struct in6_addr *ip6_dst,
> uint8_t plen, const char output_bridge[],
> const struct in6_addr *gw)
> {
> @@ -187,13 +198,14 @@ ovs_router_insert__(uint8_t priority, const struct
> in6_addr *ip6_dst,
> struct match match;
> int err;
>
> - rt_init_match(&match, ip6_dst, plen);
> + rt_init_match(&match, mark, ip6_dst, plen);
>
> p = xzalloc(sizeof *p);
> ovs_strlcpy(p->output_bridge, output_bridge, sizeof p->output_bridge);
> if (ipv6_addr_is_set(gw)) {
> p->gw = *gw;
> }
> + p->mark = mark;
> p->nw_addr = match.flow.ipv6_dst;
> p->plen = plen;
> p->priority = priority;
> @@ -202,7 +214,12 @@ ovs_router_insert__(uint8_t priority, const struct
> in6_addr *ip6_dst,
> err = get_src_addr(gw, output_bridge, &p->src_addr);
> }
> if (err) {
> + struct ds ds = DS_EMPTY_INITIALIZER;
> +
> + ipv6_format_mapped(ip6_dst, &ds);
> + VLOG_DBG_RL(&rl, "src addr not available for route %s",
> ds_cstr(&ds));
> free(p);
> + ds_destroy(&ds);
> return err;
> }
> /* Longest prefix matches first. */
> @@ -222,13 +239,12 @@ ovs_router_insert__(uint8_t priority, const struct
> in6_addr *ip6_dst,
> }
>
> void
> -ovs_router_insert(const struct in6_addr *ip_dst, uint8_t plen,
> +ovs_router_insert(uint32_t mark, const struct in6_addr *ip_dst, uint8_t plen,
> const char output_bridge[], const struct in6_addr *gw)
> {
> - ovs_router_insert__(plen, ip_dst, plen, output_bridge, gw);
> + ovs_router_insert__(mark, plen, ip_dst, plen, output_bridge, gw);
> }
>
> -
> static bool
> __rt_entry_delete(const struct cls_rule *cr)
> {
> @@ -245,14 +261,15 @@ __rt_entry_delete(const struct cls_rule *cr)
> }
>
> static bool
> -rt_entry_delete(uint8_t priority, const struct in6_addr *ip6_dst, uint8_t
> plen)
> +rt_entry_delete(uint32_t mark, uint8_t priority,
> + const struct in6_addr *ip6_dst, uint8_t plen)
> {
> const struct cls_rule *cr;
> struct cls_rule rule;
> struct match match;
> bool res = false;
>
> - rt_init_match(&match, ip6_dst, plen);
> + rt_init_match(&match, mark, ip6_dst, plen);
>
> cls_rule_init(&rule, &match, priority);
>
> @@ -288,36 +305,118 @@ scan_ipv4_route(const char *s, ovs_be32 *addr,
> unsigned int *plen)
> return true;
> }
>
> +static bool
> +parse_ip_addr(const char *arg, struct in6_addr *ip6, unsigned int *plen)
> +{
> + ovs_be32 ip;
> +
> + if (scan_ipv4_route(arg, &ip, plen)) {
> + in6_addr_set_mapped_ipv4(ip6, ip);
> + *plen += 96;
> + return true;
> + } else if (scan_ipv6_route(arg, ip6, plen)) {
> + return true;
> + }
> + return false;
> +}
> +
> +static void
> +ovs_router2_add(struct unixctl_conn *conn, int argc,
> + const char *argv[], void *aux OVS_UNUSED)
> +{
> + struct in6_addr gw6 = in6addr_any;
> + uint32_t plen = 0, mark = 0;
> + const char *dev = NULL;
> + bool req_arg = false;
> + struct in6_addr ip6;
> + int err;
> + int i;
> +
> + for (i = 1; i < argc; i++) {
> + const char *arg = argv[i];
> + unsigned int n;
> +
> + if (ovs_scan(arg, "subnet=%n", &n)) {
> + if (req_arg) {
> + unixctl_command_reply_error(conn, "Multiple subnet not
> supported");
> + return;
> + }
> + if (parse_ip_addr(&arg[n], &ip6, &plen)) {
> + req_arg = true;
> + } else {
> + unixctl_command_reply_error(conn, "Invalid subnet");
> + return;
> + }
> + } else if (ovs_scan(arg, "dev=%n", &n)) {
> + dev = &arg[n];
> + } else if (ovs_scan(arg, "gw=%n", &n)) {
> + unsigned int plen_gw;
> +
> + if (memcmp(&gw6, &in6addr_any, sizeof (gw6))) {
> + unixctl_command_reply_error(conn, "Multiple Gateway not
> supported");
> + return;
> + }
> + if (parse_ip_addr(&arg[n], &gw6, &plen_gw)) {
> + req_arg = true;
> + if (plen_gw != 128) {
> + unixctl_command_reply_error(conn, "gw mask is invalid");
> + }
> + } else {
> + unixctl_command_reply_error(conn, "Invalid gateway");
> + return;
> + }
> + } else if (ovs_scan(arg, "pkt_mark=%n", &n)) {
> + if (mark) {
> + unixctl_command_reply_error(conn, "multiple mark not
> supported");
> + return;
> + }
> + ovs_scan(&arg[n], "%"SCNi32, &mark);
> + } else {
> + unixctl_command_reply_error(conn, "Invalid parameter");
> + return;
> + }
> + }
> +
> + if (!req_arg) {
> + unixctl_command_reply_error(conn, "subnet is required parameter.");
> + return;
> + }
> + if (!dev) {
> + unixctl_command_reply_error(conn, "egress dev is required
> parameter.");
> + return;
> + }
> +
> + err = ovs_router_insert__(mark, plen + 32, &ip6, plen, dev, &gw6);
> + if (err) {
> + unixctl_command_reply_error(conn, "Error while inserting route.");
> + } else {
> + unixctl_command_reply(conn, "OK");
> + }
> + return;
> +}
> +
> static void
> ovs_router_add(struct unixctl_conn *conn, int argc,
> - const char *argv[], void *aux OVS_UNUSED)
> + const char *argv[], void *aux OVS_UNUSED)
> {
> - ovs_be32 ip;
> - unsigned int plen;
> struct in6_addr ip6;
> - struct in6_addr gw6;
> + struct in6_addr gw6 = in6addr_any;
> + uint32_t plen;
parse_ip_addr() expects unsigned int pointer for ‘plen’, so why change it’s
type?
> int err;
>
> - if (scan_ipv4_route(argv[1], &ip, &plen)) {
> - ovs_be32 gw = 0;
> - if (argc > 3 && !ip_parse(argv[3], &gw)) {
> + if (parse_ip_addr(argv[1], &ip6, &plen)) {
> + uint32_t plen_gw;
same
> +
> + if (argc > 3 && !parse_ip_addr(argv[3], &gw6, &plen_gw)) {
> unixctl_command_reply_error(conn, "Invalid gateway");
> return;
> }
What if the subnet is IPv6 and gw is IPv4 or vice versa? Will the refactored
code catch that error case?
> - in6_addr_set_mapped_ipv4(&ip6, ip);
> - in6_addr_set_mapped_ipv4(&gw6, gw);
> - plen += 96;
> - } else if (scan_ipv6_route(argv[1], &ip6, &plen)) {
> - gw6 = in6addr_any;
> - if (argc > 3 && !ipv6_parse(argv[3], &gw6)) {
> - unixctl_command_reply_error(conn, "Invalid IPv6 gateway");
> - return;
> - }
> } else {
> - unixctl_command_reply_error(conn, "Invalid parameters");
> + unixctl_command_reply_error(conn, "Error while parsing subnet");
> return;
> }
> - err = ovs_router_insert__(plen + 32, &ip6, plen, argv[2], &gw6);
> +
> + err = ovs_router_insert__(0, plen + 32, &ip6, plen, argv[2], &gw6);
> if (err) {
> unixctl_command_reply_error(conn, "Error while inserting route.");
> } else {
> @@ -330,7 +429,7 @@ ovs_router_del(struct unixctl_conn *conn, int argc
> OVS_UNUSED,
> const char *argv[], void *aux OVS_UNUSED)
> {
> ovs_be32 ip;
> - unsigned int plen;
> + uint32_t plen, mark = 0;
Again, I’d rather keep ‘plen’ as ‘unsigned int’.
> struct in6_addr ip6;
>
> if (scan_ipv4_route(argv[1], &ip, &plen)) {
> @@ -340,7 +439,11 @@ ovs_router_del(struct unixctl_conn *conn, int argc
> OVS_UNUSED,
> unixctl_command_reply_error(conn, "Invalid parameters");
> return;
> }
> - if (rt_entry_delete(plen + 32, &ip6, plen)) {
> + if (argc > 2) {
> + ovs_scan(argv[2], "%"SCNi32, &mark);
> + }
> +
> + if (rt_entry_delete(mark, plen + 32, &ip6, plen)) {
> unixctl_command_reply(conn, "OK");
> seq_change(tnl_conf_seq);
> } else {
> @@ -368,7 +471,12 @@ ovs_router_show(struct unixctl_conn *conn, int argc
> OVS_UNUSED,
> if (IN6_IS_ADDR_V4MAPPED(&rt->nw_addr)) {
> plen -= 96;
> }
> - ds_put_format(&ds, "/%"PRIu16" dev %s", plen, rt->output_bridge);
> + ds_put_format(&ds, "/%"PRIu16, plen);
> + if (rt->mark) {
> + ds_put_format(&ds, " MARK %"PRIu32, rt->mark);
> + }
Again, I think we should document somewhere that non-shown mark means matching
on mark value 0.
> +
> + ds_put_format(&ds, " dev %s", rt->output_bridge);
> if (ipv6_addr_is_set(&rt->gw)) {
> ds_put_format(&ds, " GW ");
> ipv6_format_mapped(&rt->gw, &ds);
> @@ -382,12 +490,12 @@ ovs_router_show(struct unixctl_conn *conn, int argc
> OVS_UNUSED,
> }
>
> static void
> -ovs_router_lookup_cmd(struct unixctl_conn *conn, int argc OVS_UNUSED,
> +ovs_router_lookup_cmd(struct unixctl_conn *conn, int argc,
> const char *argv[], void *aux OVS_UNUSED)
> {
> ovs_be32 ip;
> struct in6_addr ip6;
> - unsigned int plen;
> + uint32_t plen, mark = 0;
ditto.
> char iface[IFNAMSIZ];
> struct in6_addr gw, src;
>
> @@ -397,9 +505,12 @@ ovs_router_lookup_cmd(struct unixctl_conn *conn, int
> argc OVS_UNUSED,
> unixctl_command_reply_error(conn, "Invalid parameters");
> return;
> }
> -
> - if (ovs_router_lookup(&ip6, iface, &src, &gw)) {
> + if (argc > 2) {
> + ovs_scan(argv[2], "%"SCNi32, &mark);
> + }
> + if (ovs_router_lookup(mark, &ip6, iface, &src, &gw)) {
> struct ds ds = DS_EMPTY_INITIALIZER;
> +
> ds_put_format(&ds, "src ");
> ipv6_format_mapped(&src, &ds);
> ds_put_format(&ds, "\ngateway ");
> @@ -434,11 +545,14 @@ void
> ovs_router_init(void)
> {
> classifier_init(&cls, NULL);
> - unixctl_command_register("ovs/route/add", "ip_addr/prefix_len
> out_br_name gw", 2, 3,
> + unixctl_command_register("ovs/route/add", "ip_addr/prefix_len
> out_br_name gw mark", 2, 3,
There is one more optional parameter, so should the “3” be changed to “4”?
There was no test case for the optional mark here, so this was not caught.
> ovs_router_add, NULL);
> + unixctl_command_register("ovs/route2/add",
> + "subnet=ip_addr/prefix_len dev=out_br_name
> gw=ipaddr pkt_mark=mark",
> + 2, 4, ovs_router2_add, NULL);
I’d rather not add a new command, but extend the syntax of the old one, I.e.,
In addition to old syntax, allow naming any of the parameters, and maybe
support mark without a gateway also in the unnamed syntax (a mark should not
parse as an IP address, so it is possible to figure out a mark was given
without a gw). If the parameter includes ‘=‘ parse accordingly, otherwise parse
positionally as before?
> unixctl_command_register("ovs/route/show", "", 0, 0, ovs_router_show,
> NULL);
> - unixctl_command_register("ovs/route/del", "ip_addr/prefix_len", 1, 1,
> ovs_router_del,
> - NULL);
> - unixctl_command_register("ovs/route/lookup", "ip_addr", 1, 1,
> + unixctl_command_register("ovs/route/del", "ip_addr/prefix_len mark", 1,
> 2,
> + ovs_router_del, NULL);
> + unixctl_command_register("ovs/route/lookup", "ip_addr mark", 1, 2,
> ovs_router_lookup_cmd, NULL);
IMO the mark parameter should be handled consistently across all the commands,
i.e., allow either “mark=<number>”, or “<number>” in all commands, or require
one or the other in all of them.
> }
> diff --git a/lib/ovs-router.h b/lib/ovs-router.h
> index 9cb7509..29c7c5f 100644
> --- a/lib/ovs-router.h
> +++ b/lib/ovs-router.h
> @@ -25,10 +25,12 @@
> extern "C" {
> #endif
>
> -bool ovs_router_lookup(const struct in6_addr *ip_dst, char out_dev[],
> +bool ovs_router_lookup(uint32_t mark, const struct in6_addr *ip_dst,
> + char out_dev[],
> struct in6_addr *src, struct in6_addr *gw);
> void ovs_router_init(void);
> -void ovs_router_insert(const struct in6_addr *ip_dst, uint8_t plen,
> +void ovs_router_insert(uint32_t mark, const struct in6_addr *ip_dst,
> + uint8_t plen,
> const char output_bridge[], const struct in6_addr *gw);
> void ovs_router_flush(void);
> #ifdef __cplusplus
> diff --git a/lib/route-table.c b/lib/route-table.c
> index 61c8cd8..ae8af43 100644
> --- a/lib/route-table.c
> +++ b/lib/route-table.c
> @@ -298,7 +298,7 @@ route_table_handle_msg(const struct route_table_msg
> *change)
> if (change->relevant && change->nlmsg_type == RTM_NEWROUTE) {
> const struct route_data *rd = &change->rd;
>
> - ovs_router_insert(&rd->rta_dst, rd->rtm_dst_len,
> + ovs_router_insert(rd->mark, &rd->rta_dst, rd->rtm_dst_len,
> rd->ifname, &rd->rta_gw);
> }
> }
> diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c
> index 37992b4..d0ec1cd 100644
> --- a/ofproto/ofproto-dpif-sflow.c
> +++ b/ofproto/ofproto-dpif-sflow.c
> @@ -449,7 +449,7 @@ sflow_choose_agent_address(const char *agent_device,
> struct in6_addr addr6, src, gw;
>
> in6_addr_set_mapped_ipv4(&addr6, sa.sin.sin_addr.s_addr);
> - if (ovs_router_lookup(&addr6, name, &src, &gw)) {
> + if (ovs_router_lookup(0, &addr6, name, &src, &gw)) {
>
Is it possible that the route we need here actually has a non-zero mark? Maybe
add a comment either way?
> in4.s_addr = in6_addr_get_mapped_ipv4(&src);
> goto success;
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 2977be5..ad779c0 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -2715,7 +2715,7 @@ tnl_route_lookup_flow(const struct flow *oflow,
> struct in6_addr dst;
>
> dst = flow_tnl_dst(&oflow->tunnel);
> - if (!ovs_router_lookup(&dst, out_dev, src, &gw)) {
> + if (!ovs_router_lookup(oflow->pkt_mark, &dst, out_dev, src, &gw)) {
> return -ENOENT;
> }
>
> diff --git a/tests/ovs-router.at b/tests/ovs-router.at
> index 93a730a..9019e26 100644
> --- a/tests/ovs-router.at
> +++ b/tests/ovs-router.at
> @@ -20,10 +20,32 @@ AT_CHECK([ovs-appctl netdev-dummy/ip4addr br0
> 192.0.2.1/24], [0], [OK
> ])
> AT_CHECK([ovs-appctl ovs/route/add 198.51.100.0/24 br0 192.0.2.254], [0], [OK
> ])
> +AT_CHECK([ovs-appctl ovs/route2/add subnet=192.0.2.1/24 dev=br0
> pkt_mark=123], [0], [OK
> +])
> +
> +AT_CHECK([ovs-appctl ovs/route2/add subnet=198.51.100.200/24 dev=br0
> gw=192.0.2.250 pkt_mark=1234], [0], [OK
> +])
> +
> +AT_CHECK([ovs-appctl ovs/route/show | grep User | sort], [0], [User:
> 192.0.2.0/24 MARK 123 dev br0 SRC 192.0.2.1
> +User: 198.51.100.0/24 MARK 1234 dev br0 GW 192.0.2.250 SRC 192.0.2.1
> +User: 198.51.100.0/24 dev br0 GW 192.0.2.254 SRC 192.0.2.1
> +])
> +
> AT_CHECK([ovs-appctl ovs/route/lookup 198.51.100.1], [0], [src 192.0.2.1
> gateway 192.0.2.254
> dev br0
> ])
> +
> +AT_CHECK([ovs-appctl ovs/route/lookup 198.51.100.1 1234], [0], [src 192.0.2.1
> +gateway 192.0.2.250
> +dev br0
> +])
> +AT_CHECK([ovs-appctl ovs/route/del 198.51.100.0/24 1234], [0], [OK
> +])
> +AT_CHECK([ovs-appctl ovs/route/show | grep User | sort], [0], [User:
> 192.0.2.0/24 MARK 123 dev br0 SRC 192.0.2.1
> +User: 198.51.100.0/24 dev br0 GW 192.0.2.254 SRC 192.0.2.1
> +])
> +
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> diff --git a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at
> index 4aaa669..60d5224 100644
> --- a/tests/tunnel-push-pop.at
> +++ b/tests/tunnel-push-pop.at
> @@ -40,6 +40,9 @@ AT_CHECK([ovs-appctl netdev-dummy/ip6addr br0
> 2001:cafe::88/24], [0], [OK
> AT_CHECK([ovs-appctl ovs/route/add 1.1.2.92/24 br0], [0], [OK
> ])
>
> +AT_CHECK([ovs-appctl ovs/route2/add subnet=1.1.2.92/24 pkt_mark=1234
> dev=br0], [0], [OK
> +])
> +
> AT_CHECK([ovs-ofctl add-flow br0 action=normal])
>
> dnl Check ARP request
> --
> 2.9.3
>
> _______________________________________________
> 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
