From 025fec42cebe9efc7c6a2d94816b173748e7e4f6 Mon Sep 17 00:00:00 2001

From: Numan Siddique <numans@ovn.org>

Date: Tue, 23 Jun 2020 17:07:04 +0530

Subject: [PATCH 04/22] ovn-nbctl: Enhance lr-policy-add to set the options.

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.")

Acked-by: Mark Michelson <mmichels@redhat.com>

Signed-off-by: Numan Siddique <numans@ovn.org>

(cherry-picked from upstream master commit 742474bad730fbdc9705b4c2784a2b4acca327cf)

Change-Id: I64c786ff4c5244b643a57bff270a14d85d5204f1

---

 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 dc9d9d76a..6d6608729 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 8ce45823f..b84cf75fd 100644

--- a/tests/ovn.at

+++ b/tests/ovn.at

@@ -20286,20 +20286,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 @@

       
lr-policy-add router priority

-          match action [nexthop]

+          match action [nexthop]

+          [options key=value]] 

           Add Policy to router which provides a way to configure

@@ -732,6 +733,8 @@

           only when action is reroute. A policy is

           uniquely identified by priority and match.

           Multiple policies can have the same priority.

+          options sets the router policy options as key-value pair.

+          The supported option is : pkt_mark.

@@ -743,6 +746,12 @@

           lr-policy-add lr1 100 ip4.src == 192.168.100.0/24 drop.

+          

+          

+            lr-policy-add lr1 100 ip4.src == 192.168.100.0/24 allow

+            pkt_mark=100

+          .

+          

       
lr-policy-del router [{priority | uuid}

diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c

index 159a44960..7578b9928 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\

@@ -3609,16 +3610,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.

@@ -3629,12 +3633,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;

         }

     }

@@ -3643,9 +3649,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));

@@ -3773,6 +3798,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');

 }

@@ -3788,7 +3823,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;

@@ -6362,8 +6397,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,

-- 

2.26.2