[ovs-dev] [PATCH ovn v2] ovn-nbctl: Enhance lr-policy-add to set the options.
Mark Michelson
mmichels at redhat.com
Tue Jun 23 14:36:26 UTC 2020
Acked-by: Mark Michelson <mmichels at redhat.com>
On 6/23/20 10:29 AM, numans at ovn.org wrote:
> From: Numan Siddique <numans at 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 at 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,
>
More information about the dev
mailing list