| Message ID | 20200623142901.1919822-1-numans@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,ovn,v2] ovn-nbctl: Enhance lr-policy-add to set the options. | expand |
Acked-by: Mark Michelson <mmichels@redhat.com> On 6/23/20 10:29 AM, numans@ovn.org wrote: > From: Numan Siddique <numans@ovn.org> > > The commit [1] added a new column - 'options' to Logical_Router_Policy NB DB > table. This patch enhances the lr-policy-add command to set the options > as key=value pairs. > > For nbctl_lr_policy_add(), this patch now returns after ctl_error() as there is no > point continuing further and the comments in the ctl_error() implementation says so. > > [1] - a123ef0fb8fd("Support packet metadata marking for logical router policies.") > > Signed-off-by: Numan Siddique <numans@ovn.org> > --- > tests/ovn-nbctl.at | 15 +++++++++--- > tests/ovn.at | 8 ++----- > utilities/ovn-nbctl.8.xml | 11 ++++++++- > utilities/ovn-nbctl.c | 48 ++++++++++++++++++++++++++++++++++----- > 4 files changed, 66 insertions(+), 16 deletions(-) > > diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at > index 14de1a8bf..3f874721c 100644 > --- a/tests/ovn-nbctl.at > +++ b/tests/ovn-nbctl.at > @@ -1590,11 +1590,20 @@ AT_CHECK([ovn-nbctl lr-add lr0]) > > dnl Add policies with allow and drop actions > AT_CHECK([ovn-nbctl lr-policy-add lr0 100 "ip4.src == 1.1.1.0/24" drop]) > -AT_CHECK([ovn-nbctl lr-policy-add lr0 100 "ip4.src == 1.1.2.0/24" allow]) > +AT_CHECK([ovn-nbctl lr-policy-add lr0 100 "ip4.src == 1.1.2.0/24" allow pkt_mark=100,foo=bar]) > AT_CHECK([ovn-nbctl lr-policy-add lr0 101 "ip4.src == 2.1.1.0/24" allow]) > AT_CHECK([ovn-nbctl lr-policy-add lr0 101 "ip4.src == 2.1.2.0/24" drop]) > AT_CHECK([ovn-nbctl lr-policy-add lr0 101 "ip6.src == 2002::/64" drop]) > > +dnl Incomplete option set. > +AT_CHECK([ovn-nbctl lr-policy-add lr0 200 "ip4.src == 1.1.4.0/24" reroute 192.168.0.10 foo], [1], [], > + [ovn-nbctl: No value specified for the option : foo > +]) > + > +AT_CHECK([ovn-nbctl lr-policy-add lr0 200 "ip4.src == 1.1.4.0/24" allow bar=], [1], [], > + [ovn-nbctl: No value specified for the option : bar > +]) > + > dnl Add duplicated policy > AT_CHECK([ovn-nbctl lr-policy-add lr0 100 "ip4.src == 1.1.1.0/24" drop], [1], [], > [ovn-nbctl: Same routing policy already existed on the logical router lr0. > @@ -1612,14 +1621,14 @@ Routing Policies > 101 ip4.src == 2.1.1.0/24 allow > 101 ip4.src == 2.1.2.0/24 drop > 101 ip6.src == 2002::/64 drop > - 100 ip4.src == 1.1.2.0/24 allow > + 100 ip4.src == 1.1.2.0/24 allow pkt_mark=100,foo=bar > ]) > > dnl Delete all policies for given priority > AT_CHECK([ovn-nbctl lr-policy-del lr0 101]) > AT_CHECK([ovn-nbctl lr-policy-list lr0], [0], [dnl > Routing Policies > - 100 ip4.src == 1.1.2.0/24 allow > + 100 ip4.src == 1.1.2.0/24 allow pkt_mark=100,foo=bar > ]) > > > diff --git a/tests/ovn.at b/tests/ovn.at > index 1ff795294..00ed875ff 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -19993,20 +19993,16 @@ static_routes @lrt > ovn-nbctl --wait=hv sync > > # Add logical router policy and set pkt_mark on it. > -ovn-nbctl lr-policy-add lr0 2000 "ip4.src == 10.0.0.3" allow > +ovn-nbctl lr-policy-add lr0 2000 "ip4.src == 10.0.0.3" allow pkt_mark=100 > ovn-nbctl lr-policy-add lr0 1000 "ip4.src == 10.0.0.4" allow > -ovn-nbctl lr-policy-add lr0 900 "ip4.src == 10.0.0.5" reroute 172.168.0.200 > +ovn-nbctl lr-policy-add lr0 900 "ip4.src == 10.0.0.5" reroute 172.168.0.200 pkt_mark=3 > ovn-nbctl lr-policy-add lr0 2001 "ip6.dst == bef0::5" reroute bef0::6 > ovn-nbctl lr-policy-add lr0 1001 "ip6" allow > > - > pol1=$(ovn-nbctl --bare --columns _uuid find logical_router_policy priority=2000) > -pol3=$(ovn-nbctl --bare --columns _uuid find logical_router_policy priority=900) > pol4=$(ovn-nbctl --bare --columns _uuid find logical_router_policy priority=2001) > pol5=$(ovn-nbctl --bare --columns _uuid find logical_router_policy priority=1001) > > -ovn-nbctl set logical_router_policy $pol1 options:pkt_mark=100 > -ovn-nbctl set logical_router_policy $pol3 options:pkt_mark=3 > ovn-nbctl set logical_router_policy $pol4 options:pkt_mark=4 > ovn-nbctl set logical_router_policy $pol5 options:pkt_mark=5 > ovn-nbctl --wait=hv sync > diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml > index d265c7fcc..de86b70e6 100644 > --- a/utilities/ovn-nbctl.8.xml > +++ b/utilities/ovn-nbctl.8.xml > @@ -721,7 +721,8 @@ > > <dl> > <dt><code>lr-policy-add</code> <var>router</var> <var>priority</var> > - <var>match</var> <var>action</var> [<var>nexthop</var>]</dt> > + <var>match</var> <var>action</var> [<var>nexthop</var>] > + [<var>options key=value]</var>] </dt> > <dd> > <p> > Add Policy to <var>router</var> which provides a way to configure > @@ -732,6 +733,8 @@ > only when <var>action</var> is <var>reroute</var>. A policy is > uniquely identified by <var>priority</var> and <var>match</var>. > Multiple policies can have the same <var>priority</var>. > + <var>options</var> sets the router policy options as key-value pair. > + The supported option is : <code>pkt_mark</code>. > </p> > > <p> > @@ -743,6 +746,12 @@ > <code>lr-policy-add lr1 100 ip4.src == 192.168.100.0/24 drop</code>. > </p> > > + <p> > + <code> > + lr-policy-add lr1 100 ip4.src == 192.168.100.0/24 allow > + pkt_mark=100 > + </code>. > + </p> > </dd> > > <dt><code>lr-policy-del</code> <var>router</var> [<var>{priority | uuid} > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c > index 6ccc7025d..ee13423ce 100644 > --- a/utilities/ovn-nbctl.c > +++ b/utilities/ovn-nbctl.c > @@ -694,7 +694,8 @@ Route commands:\n\ > lr-route-list ROUTER print routes for ROUTER\n\ > \n\ > Policy commands:\n\ > - lr-policy-add ROUTER PRIORITY MATCH ACTION [NEXTHOP]\n\ > + lr-policy-add ROUTER PRIORITY MATCH ACTION [NEXTHOP] \ > +[OPTIONS KEY=VALUE ...] \n\ > add a policy to router\n\ > lr-policy-del ROUTER [{PRIORITY | UUID} [MATCH]]\n\ > remove policies from ROUTER\n\ > @@ -3552,16 +3553,19 @@ nbctl_lr_policy_add(struct ctl_context *ctx) > const char *action = ctx->argv[4]; > char *next_hop = NULL; > > + bool reroute = false; > /* Validate action. */ > if (strcmp(action, "allow") && strcmp(action, "drop") > && strcmp(action, "reroute")) { > ctl_error(ctx, "%s: action must be one of \"allow\", \"drop\", " > "and \"reroute\"", action); > + return; > } > if (!strcmp(action, "reroute")) { > if (ctx->argc < 6) { > ctl_error(ctx, "Nexthop is required when action is reroute."); > } > + reroute = true; > } > > /* Check if same routing policy already exists. > @@ -3572,12 +3576,14 @@ nbctl_lr_policy_add(struct ctl_context *ctx) > !strcmp(policy->match, ctx->argv[3])) { > ctl_error(ctx, "Same routing policy already existed on the " > "logical router %s.", ctx->argv[1]); > + return; > } > } > - if (ctx->argc == 6) { > + if (reroute) { > next_hop = normalize_prefix_str(ctx->argv[5]); > if (!next_hop) { > ctl_error(ctx, "bad next hop argument: %s", ctx->argv[5]); > + return; > } > } > > @@ -3586,9 +3592,28 @@ nbctl_lr_policy_add(struct ctl_context *ctx) > nbrec_logical_router_policy_set_priority(policy, priority); > nbrec_logical_router_policy_set_match(policy, ctx->argv[3]); > nbrec_logical_router_policy_set_action(policy, action); > - if (ctx->argc == 6) { > + if (reroute) { > nbrec_logical_router_policy_set_nexthop(policy, next_hop); > } > + > + /* Parse the options. */ > + struct smap options = SMAP_INITIALIZER(&options); > + for (size_t i = reroute ? 6 : 5; i < ctx->argc; i++) { > + char *key, *value; > + value = xstrdup(ctx->argv[i]); > + key = strsep(&value, "="); > + if (value && value[0]) { > + smap_add(&options, key, value); > + } else { > + ctl_error(ctx, "No value specified for the option : %s", key); > + free(key); > + return; > + } > + free(key); > + } > + nbrec_logical_router_policy_set_options(policy, &options); > + smap_destroy(&options); > + > nbrec_logical_router_verify_policies(lr); > struct nbrec_logical_router_policy **new_policies > = xmalloc(sizeof *new_policies * (lr->n_policies + 1)); > @@ -3716,6 +3741,16 @@ print_routing_policy(const struct nbrec_logical_router_policy *policy, > ds_put_format(s, "%10"PRId64" %50s %15s", policy->priority, > policy->match, policy->action); > } > + > + if (!smap_is_empty(&policy->options)) { > + ds_put_format(s, "%15s", ""); > + struct smap_node *node; > + SMAP_FOR_EACH (node, &policy->options) { > + ds_put_format(s, "%s=%s,", node->key, node->value); > + } > + ds_chomp(s, ','); > + } > + > ds_put_char(s, '\n'); > } > > @@ -3731,7 +3766,7 @@ nbctl_lr_policy_list(struct ctl_context *ctx) > return; > } > policies = xmalloc(sizeof *policies * lr->n_policies); > - for (int i = 0; i < lr->n_policies; i++) { > + for (int i = 0; i < lr->n_policies; i++) { > const struct nbrec_logical_router_policy *policy > = lr->policies[i]; > policies[n_policies].priority = policy->priority; > @@ -6273,8 +6308,9 @@ static const struct ctl_command_syntax nbctl_commands[] = { > "", RO }, > > /* Policy commands */ > - { "lr-policy-add", 4, 5, "ROUTER PRIORITY MATCH ACTION [NEXTHOP]", NULL, > - nbctl_lr_policy_add, NULL, "", RW }, > + { "lr-policy-add", 4, INT_MAX, > + "ROUTER PRIORITY MATCH ACTION [NEXTHOP] [OPTIONS - KEY=VALUE ...]", > + NULL, nbctl_lr_policy_add, NULL, "", RW }, > { "lr-policy-del", 1, 3, "ROUTER [{PRIORITY | UUID} [MATCH]]", NULL, > nbctl_lr_policy_del, NULL, "", RW }, > { "lr-policy-list", 1, 1, "ROUTER", NULL, nbctl_lr_policy_list, NULL, >
On Tue, Jun 23, 2020 at 8:07 PM Mark Michelson <mmichels@redhat.com> wrote: > Acked-by: Mark Michelson <mmichels@redhat.com> > Thanks for the review. I applied this patch to master. Thanks Numan > > On 6/23/20 10:29 AM, numans@ovn.org wrote: > > From: Numan Siddique <numans@ovn.org> > > > > The commit [1] added a new column - 'options' to Logical_Router_Policy > NB DB > > table. This patch enhances the lr-policy-add command to set the options > > as key=value pairs. > > > > For nbctl_lr_policy_add(), this patch now returns after ctl_error() as > there is no > > point continuing further and the comments in the ctl_error() > implementation says so. > > > > [1] - a123ef0fb8fd("Support packet metadata marking for logical router > policies.") > > > > Signed-off-by: Numan Siddique <numans@ovn.org> > > --- > > tests/ovn-nbctl.at | 15 +++++++++--- > > tests/ovn.at | 8 ++----- > > utilities/ovn-nbctl.8.xml | 11 ++++++++- > > utilities/ovn-nbctl.c | 48 ++++++++++++++++++++++++++++++++++----- > > 4 files changed, 66 insertions(+), 16 deletions(-) > > > > diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at > > index 14de1a8bf..3f874721c 100644 > > --- a/tests/ovn-nbctl.at > > +++ b/tests/ovn-nbctl.at > > @@ -1590,11 +1590,20 @@ AT_CHECK([ovn-nbctl lr-add lr0]) > > > > dnl Add policies with allow and drop actions > > AT_CHECK([ovn-nbctl lr-policy-add lr0 100 "ip4.src == 1.1.1.0/24" > drop]) > > -AT_CHECK([ovn-nbctl lr-policy-add lr0 100 "ip4.src == 1.1.2.0/24" > allow]) > > +AT_CHECK([ovn-nbctl lr-policy-add lr0 100 "ip4.src == 1.1.2.0/24" > allow pkt_mark=100,foo=bar]) > > AT_CHECK([ovn-nbctl lr-policy-add lr0 101 "ip4.src == 2.1.1.0/24" > allow]) > > AT_CHECK([ovn-nbctl lr-policy-add lr0 101 "ip4.src == 2.1.2.0/24" > drop]) > > AT_CHECK([ovn-nbctl lr-policy-add lr0 101 "ip6.src == 2002::/64" drop]) > > > > +dnl Incomplete option set. > > +AT_CHECK([ovn-nbctl lr-policy-add lr0 200 "ip4.src == 1.1.4.0/24" > reroute 192.168.0.10 foo], [1], [], > > + [ovn-nbctl: No value specified for the option : foo > > +]) > > + > > +AT_CHECK([ovn-nbctl lr-policy-add lr0 200 "ip4.src == 1.1.4.0/24" > allow bar=], [1], [], > > + [ovn-nbctl: No value specified for the option : bar > > +]) > > + > > dnl Add duplicated policy > > AT_CHECK([ovn-nbctl lr-policy-add lr0 100 "ip4.src == 1.1.1.0/24" > drop], [1], [], > > [ovn-nbctl: Same routing policy already existed on the logical > router lr0. > > @@ -1612,14 +1621,14 @@ Routing Policies > > 101 ip4.src == 2.1.1.0/24 > allow > > 101 ip4.src == 2.1.2.0/24 > drop > > 101 ip6.src == 2002::/64 > drop > > - 100 ip4.src == 1.1.2.0/24 > allow > > + 100 ip4.src == 1.1.2.0/24 > allow pkt_mark=100,foo=bar > > ]) > > > > dnl Delete all policies for given priority > > AT_CHECK([ovn-nbctl lr-policy-del lr0 101]) > > AT_CHECK([ovn-nbctl lr-policy-list lr0], [0], [dnl > > Routing Policies > > - 100 ip4.src == 1.1.2.0/24 > allow > > + 100 ip4.src == 1.1.2.0/24 > allow pkt_mark=100,foo=bar > > ]) > > > > > > diff --git a/tests/ovn.at b/tests/ovn.at > > index 1ff795294..00ed875ff 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -19993,20 +19993,16 @@ static_routes @lrt > > ovn-nbctl --wait=hv sync > > > > # Add logical router policy and set pkt_mark on it. > > -ovn-nbctl lr-policy-add lr0 2000 "ip4.src == 10.0.0.3" allow > > +ovn-nbctl lr-policy-add lr0 2000 "ip4.src == 10.0.0.3" allow > pkt_mark=100 > > ovn-nbctl lr-policy-add lr0 1000 "ip4.src == 10.0.0.4" allow > > -ovn-nbctl lr-policy-add lr0 900 "ip4.src == 10.0.0.5" reroute > 172.168.0.200 > > +ovn-nbctl lr-policy-add lr0 900 "ip4.src == 10.0.0.5" reroute > 172.168.0.200 pkt_mark=3 > > ovn-nbctl lr-policy-add lr0 2001 "ip6.dst == bef0::5" reroute bef0::6 > > ovn-nbctl lr-policy-add lr0 1001 "ip6" allow > > > > - > > pol1=$(ovn-nbctl --bare --columns _uuid find logical_router_policy > priority=2000) > > -pol3=$(ovn-nbctl --bare --columns _uuid find logical_router_policy > priority=900) > > pol4=$(ovn-nbctl --bare --columns _uuid find logical_router_policy > priority=2001) > > pol5=$(ovn-nbctl --bare --columns _uuid find logical_router_policy > priority=1001) > > > > -ovn-nbctl set logical_router_policy $pol1 options:pkt_mark=100 > > -ovn-nbctl set logical_router_policy $pol3 options:pkt_mark=3 > > ovn-nbctl set logical_router_policy $pol4 options:pkt_mark=4 > > ovn-nbctl set logical_router_policy $pol5 options:pkt_mark=5 > > ovn-nbctl --wait=hv sync > > diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml > > index d265c7fcc..de86b70e6 100644 > > --- a/utilities/ovn-nbctl.8.xml > > +++ b/utilities/ovn-nbctl.8.xml > > @@ -721,7 +721,8 @@ > > > > <dl> > > <dt><code>lr-policy-add</code> <var>router</var> > <var>priority</var> > > - <var>match</var> <var>action</var> [<var>nexthop</var>]</dt> > > + <var>match</var> <var>action</var> [<var>nexthop</var>] > > + [<var>options key=value]</var>] </dt> > > <dd> > > <p> > > Add Policy to <var>router</var> which provides a way to > configure > > @@ -732,6 +733,8 @@ > > only when <var>action</var> is <var>reroute</var>. A policy > is > > uniquely identified by <var>priority</var> and > <var>match</var>. > > Multiple policies can have the same <var>priority</var>. > > + <var>options</var> sets the router policy options as > key-value pair. > > + The supported option is : <code>pkt_mark</code>. > > </p> > > > > <p> > > @@ -743,6 +746,12 @@ > > <code>lr-policy-add lr1 100 ip4.src == 192.168.100.0/24 > drop</code>. > > </p> > > > > + <p> > > + <code> > > + lr-policy-add lr1 100 ip4.src == 192.168.100.0/24 allow > > + pkt_mark=100 > > + </code>. > > + </p> > > </dd> > > > > <dt><code>lr-policy-del</code> <var>router</var> [<var>{priority > | uuid} > > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c > > index 6ccc7025d..ee13423ce 100644 > > --- a/utilities/ovn-nbctl.c > > +++ b/utilities/ovn-nbctl.c > > @@ -694,7 +694,8 @@ Route commands:\n\ > > lr-route-list ROUTER print routes for ROUTER\n\ > > \n\ > > Policy commands:\n\ > > - lr-policy-add ROUTER PRIORITY MATCH ACTION [NEXTHOP]\n\ > > + lr-policy-add ROUTER PRIORITY MATCH ACTION [NEXTHOP] \ > > +[OPTIONS KEY=VALUE ...] \n\ > > add a policy to router\n\ > > lr-policy-del ROUTER [{PRIORITY | UUID} [MATCH]]\n\ > > remove policies from ROUTER\n\ > > @@ -3552,16 +3553,19 @@ nbctl_lr_policy_add(struct ctl_context *ctx) > > const char *action = ctx->argv[4]; > > char *next_hop = NULL; > > > > + bool reroute = false; > > /* Validate action. */ > > if (strcmp(action, "allow") && strcmp(action, "drop") > > && strcmp(action, "reroute")) { > > ctl_error(ctx, "%s: action must be one of \"allow\", \"drop\", > " > > "and \"reroute\"", action); > > + return; > > } > > if (!strcmp(action, "reroute")) { > > if (ctx->argc < 6) { > > ctl_error(ctx, "Nexthop is required when action is > reroute."); > > } > > + reroute = true; > > } > > > > /* Check if same routing policy already exists. > > @@ -3572,12 +3576,14 @@ nbctl_lr_policy_add(struct ctl_context *ctx) > > !strcmp(policy->match, ctx->argv[3])) { > > ctl_error(ctx, "Same routing policy already existed on the > " > > "logical router %s.", ctx->argv[1]); > > + return; > > } > > } > > - if (ctx->argc == 6) { > > + if (reroute) { > > next_hop = normalize_prefix_str(ctx->argv[5]); > > if (!next_hop) { > > ctl_error(ctx, "bad next hop argument: %s", ctx->argv[5]); > > + return; > > } > > } > > > > @@ -3586,9 +3592,28 @@ nbctl_lr_policy_add(struct ctl_context *ctx) > > nbrec_logical_router_policy_set_priority(policy, priority); > > nbrec_logical_router_policy_set_match(policy, ctx->argv[3]); > > nbrec_logical_router_policy_set_action(policy, action); > > - if (ctx->argc == 6) { > > + if (reroute) { > > nbrec_logical_router_policy_set_nexthop(policy, next_hop); > > } > > + > > + /* Parse the options. */ > > + struct smap options = SMAP_INITIALIZER(&options); > > + for (size_t i = reroute ? 6 : 5; i < ctx->argc; i++) { > > + char *key, *value; > > + value = xstrdup(ctx->argv[i]); > > + key = strsep(&value, "="); > > + if (value && value[0]) { > > + smap_add(&options, key, value); > > + } else { > > + ctl_error(ctx, "No value specified for the option : %s", > key); > > + free(key); > > + return; > > + } > > + free(key); > > + } > > + nbrec_logical_router_policy_set_options(policy, &options); > > + smap_destroy(&options); > > + > > nbrec_logical_router_verify_policies(lr); > > struct nbrec_logical_router_policy **new_policies > > = xmalloc(sizeof *new_policies * (lr->n_policies + 1)); > > @@ -3716,6 +3741,16 @@ print_routing_policy(const struct > nbrec_logical_router_policy *policy, > > ds_put_format(s, "%10"PRId64" %50s %15s", policy->priority, > > policy->match, policy->action); > > } > > + > > + if (!smap_is_empty(&policy->options)) { > > + ds_put_format(s, "%15s", ""); > > + struct smap_node *node; > > + SMAP_FOR_EACH (node, &policy->options) { > > + ds_put_format(s, "%s=%s,", node->key, node->value); > > + } > > + ds_chomp(s, ','); > > + } > > + > > ds_put_char(s, '\n'); > > } > > > > @@ -3731,7 +3766,7 @@ nbctl_lr_policy_list(struct ctl_context *ctx) > > return; > > } > > policies = xmalloc(sizeof *policies * lr->n_policies); > > - for (int i = 0; i < lr->n_policies; i++) { > > + for (int i = 0; i < lr->n_policies; i++) { > > const struct nbrec_logical_router_policy *policy > > = lr->policies[i]; > > policies[n_policies].priority = policy->priority; > > @@ -6273,8 +6308,9 @@ static const struct ctl_command_syntax > nbctl_commands[] = { > > "", RO }, > > > > /* Policy commands */ > > - { "lr-policy-add", 4, 5, "ROUTER PRIORITY MATCH ACTION [NEXTHOP]", > NULL, > > - nbctl_lr_policy_add, NULL, "", RW }, > > + { "lr-policy-add", 4, INT_MAX, > > + "ROUTER PRIORITY MATCH ACTION [NEXTHOP] [OPTIONS - KEY=VALUE ...]", > > + NULL, nbctl_lr_policy_add, NULL, "", RW }, > > { "lr-policy-del", 1, 3, "ROUTER [{PRIORITY | UUID} [MATCH]]", > NULL, > > nbctl_lr_policy_del, NULL, "", RW }, > > { "lr-policy-list", 1, 1, "ROUTER", NULL, nbctl_lr_policy_list, > NULL, > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at index 14de1a8bf..3f874721c 100644 --- a/tests/ovn-nbctl.at +++ b/tests/ovn-nbctl.at @@ -1590,11 +1590,20 @@ AT_CHECK([ovn-nbctl lr-add lr0]) dnl Add policies with allow and drop actions AT_CHECK([ovn-nbctl lr-policy-add lr0 100 "ip4.src == 1.1.1.0/24" drop]) -AT_CHECK([ovn-nbctl lr-policy-add lr0 100 "ip4.src == 1.1.2.0/24" allow]) +AT_CHECK([ovn-nbctl lr-policy-add lr0 100 "ip4.src == 1.1.2.0/24" allow pkt_mark=100,foo=bar]) AT_CHECK([ovn-nbctl lr-policy-add lr0 101 "ip4.src == 2.1.1.0/24" allow]) AT_CHECK([ovn-nbctl lr-policy-add lr0 101 "ip4.src == 2.1.2.0/24" drop]) AT_CHECK([ovn-nbctl lr-policy-add lr0 101 "ip6.src == 2002::/64" drop]) +dnl Incomplete option set. +AT_CHECK([ovn-nbctl lr-policy-add lr0 200 "ip4.src == 1.1.4.0/24" reroute 192.168.0.10 foo], [1], [], + [ovn-nbctl: No value specified for the option : foo +]) + +AT_CHECK([ovn-nbctl lr-policy-add lr0 200 "ip4.src == 1.1.4.0/24" allow bar=], [1], [], + [ovn-nbctl: No value specified for the option : bar +]) + dnl Add duplicated policy AT_CHECK([ovn-nbctl lr-policy-add lr0 100 "ip4.src == 1.1.1.0/24" drop], [1], [], [ovn-nbctl: Same routing policy already existed on the logical router lr0. @@ -1612,14 +1621,14 @@ Routing Policies 101 ip4.src == 2.1.1.0/24 allow 101 ip4.src == 2.1.2.0/24 drop 101 ip6.src == 2002::/64 drop - 100 ip4.src == 1.1.2.0/24 allow + 100 ip4.src == 1.1.2.0/24 allow pkt_mark=100,foo=bar ]) dnl Delete all policies for given priority AT_CHECK([ovn-nbctl lr-policy-del lr0 101]) AT_CHECK([ovn-nbctl lr-policy-list lr0], [0], [dnl Routing Policies - 100 ip4.src == 1.1.2.0/24 allow + 100 ip4.src == 1.1.2.0/24 allow pkt_mark=100,foo=bar ]) diff --git a/tests/ovn.at b/tests/ovn.at index 1ff795294..00ed875ff 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -19993,20 +19993,16 @@ static_routes @lrt ovn-nbctl --wait=hv sync # Add logical router policy and set pkt_mark on it. -ovn-nbctl lr-policy-add lr0 2000 "ip4.src == 10.0.0.3" allow +ovn-nbctl lr-policy-add lr0 2000 "ip4.src == 10.0.0.3" allow pkt_mark=100 ovn-nbctl lr-policy-add lr0 1000 "ip4.src == 10.0.0.4" allow -ovn-nbctl lr-policy-add lr0 900 "ip4.src == 10.0.0.5" reroute 172.168.0.200 +ovn-nbctl lr-policy-add lr0 900 "ip4.src == 10.0.0.5" reroute 172.168.0.200 pkt_mark=3 ovn-nbctl lr-policy-add lr0 2001 "ip6.dst == bef0::5" reroute bef0::6 ovn-nbctl lr-policy-add lr0 1001 "ip6" allow - pol1=$(ovn-nbctl --bare --columns _uuid find logical_router_policy priority=2000) -pol3=$(ovn-nbctl --bare --columns _uuid find logical_router_policy priority=900) pol4=$(ovn-nbctl --bare --columns _uuid find logical_router_policy priority=2001) pol5=$(ovn-nbctl --bare --columns _uuid find logical_router_policy priority=1001) -ovn-nbctl set logical_router_policy $pol1 options:pkt_mark=100 -ovn-nbctl set logical_router_policy $pol3 options:pkt_mark=3 ovn-nbctl set logical_router_policy $pol4 options:pkt_mark=4 ovn-nbctl set logical_router_policy $pol5 options:pkt_mark=5 ovn-nbctl --wait=hv sync diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml index d265c7fcc..de86b70e6 100644 --- a/utilities/ovn-nbctl.8.xml +++ b/utilities/ovn-nbctl.8.xml @@ -721,7 +721,8 @@ <dl> <dt><code>lr-policy-add</code> <var>router</var> <var>priority</var> - <var>match</var> <var>action</var> [<var>nexthop</var>]</dt> + <var>match</var> <var>action</var> [<var>nexthop</var>] + [<var>options key=value]</var>] </dt> <dd> <p> Add Policy to <var>router</var> which provides a way to configure @@ -732,6 +733,8 @@ only when <var>action</var> is <var>reroute</var>. A policy is uniquely identified by <var>priority</var> and <var>match</var>. Multiple policies can have the same <var>priority</var>. + <var>options</var> sets the router policy options as key-value pair. + The supported option is : <code>pkt_mark</code>. </p> <p> @@ -743,6 +746,12 @@ <code>lr-policy-add lr1 100 ip4.src == 192.168.100.0/24 drop</code>. </p> + <p> + <code> + lr-policy-add lr1 100 ip4.src == 192.168.100.0/24 allow + pkt_mark=100 + </code>. + </p> </dd> <dt><code>lr-policy-del</code> <var>router</var> [<var>{priority | uuid} diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c index 6ccc7025d..ee13423ce 100644 --- a/utilities/ovn-nbctl.c +++ b/utilities/ovn-nbctl.c @@ -694,7 +694,8 @@ Route commands:\n\ lr-route-list ROUTER print routes for ROUTER\n\ \n\ Policy commands:\n\ - lr-policy-add ROUTER PRIORITY MATCH ACTION [NEXTHOP]\n\ + lr-policy-add ROUTER PRIORITY MATCH ACTION [NEXTHOP] \ +[OPTIONS KEY=VALUE ...] \n\ add a policy to router\n\ lr-policy-del ROUTER [{PRIORITY | UUID} [MATCH]]\n\ remove policies from ROUTER\n\ @@ -3552,16 +3553,19 @@ nbctl_lr_policy_add(struct ctl_context *ctx) const char *action = ctx->argv[4]; char *next_hop = NULL; + bool reroute = false; /* Validate action. */ if (strcmp(action, "allow") && strcmp(action, "drop") && strcmp(action, "reroute")) { ctl_error(ctx, "%s: action must be one of \"allow\", \"drop\", " "and \"reroute\"", action); + return; } if (!strcmp(action, "reroute")) { if (ctx->argc < 6) { ctl_error(ctx, "Nexthop is required when action is reroute."); } + reroute = true; } /* Check if same routing policy already exists. @@ -3572,12 +3576,14 @@ nbctl_lr_policy_add(struct ctl_context *ctx) !strcmp(policy->match, ctx->argv[3])) { ctl_error(ctx, "Same routing policy already existed on the " "logical router %s.", ctx->argv[1]); + return; } } - if (ctx->argc == 6) { + if (reroute) { next_hop = normalize_prefix_str(ctx->argv[5]); if (!next_hop) { ctl_error(ctx, "bad next hop argument: %s", ctx->argv[5]); + return; } } @@ -3586,9 +3592,28 @@ nbctl_lr_policy_add(struct ctl_context *ctx) nbrec_logical_router_policy_set_priority(policy, priority); nbrec_logical_router_policy_set_match(policy, ctx->argv[3]); nbrec_logical_router_policy_set_action(policy, action); - if (ctx->argc == 6) { + if (reroute) { nbrec_logical_router_policy_set_nexthop(policy, next_hop); } + + /* Parse the options. */ + struct smap options = SMAP_INITIALIZER(&options); + for (size_t i = reroute ? 6 : 5; i < ctx->argc; i++) { + char *key, *value; + value = xstrdup(ctx->argv[i]); + key = strsep(&value, "="); + if (value && value[0]) { + smap_add(&options, key, value); + } else { + ctl_error(ctx, "No value specified for the option : %s", key); + free(key); + return; + } + free(key); + } + nbrec_logical_router_policy_set_options(policy, &options); + smap_destroy(&options); + nbrec_logical_router_verify_policies(lr); struct nbrec_logical_router_policy **new_policies = xmalloc(sizeof *new_policies * (lr->n_policies + 1)); @@ -3716,6 +3741,16 @@ print_routing_policy(const struct nbrec_logical_router_policy *policy, ds_put_format(s, "%10"PRId64" %50s %15s", policy->priority, policy->match, policy->action); } + + if (!smap_is_empty(&policy->options)) { + ds_put_format(s, "%15s", ""); + struct smap_node *node; + SMAP_FOR_EACH (node, &policy->options) { + ds_put_format(s, "%s=%s,", node->key, node->value); + } + ds_chomp(s, ','); + } + ds_put_char(s, '\n'); } @@ -3731,7 +3766,7 @@ nbctl_lr_policy_list(struct ctl_context *ctx) return; } policies = xmalloc(sizeof *policies * lr->n_policies); - for (int i = 0; i < lr->n_policies; i++) { + for (int i = 0; i < lr->n_policies; i++) { const struct nbrec_logical_router_policy *policy = lr->policies[i]; policies[n_policies].priority = policy->priority; @@ -6273,8 +6308,9 @@ static const struct ctl_command_syntax nbctl_commands[] = { "", RO }, /* Policy commands */ - { "lr-policy-add", 4, 5, "ROUTER PRIORITY MATCH ACTION [NEXTHOP]", NULL, - nbctl_lr_policy_add, NULL, "", RW }, + { "lr-policy-add", 4, INT_MAX, + "ROUTER PRIORITY MATCH ACTION [NEXTHOP] [OPTIONS - KEY=VALUE ...]", + NULL, nbctl_lr_policy_add, NULL, "", RW }, { "lr-policy-del", 1, 3, "ROUTER [{PRIORITY | UUID} [MATCH]]", NULL, nbctl_lr_policy_del, NULL, "", RW }, { "lr-policy-list", 1, 1, "ROUTER", NULL, nbctl_lr_policy_list, NULL,