[ovs-dev] [PATCH v4 3/3] OVN ACL: Allow a user to input ct.label value for an acl

Ankur Sharma ankur.sharma at nutanix.com
Wed Apr 17 23:02:18 UTC 2019


This patch allows user to associate a value with acl,
which will be assigned to ct.label of the corresponding
connection tracking entry.

This value can be used to map a ct entry with corresponding
OVN ACL or higher level constructs like security group.

Signed-off-by: Ankur Sharma <ankur.sharma at nutanix.com>
---
 ovn/northd/ovn-northd.c   | 37 ++++++++++++++++++++++++------
 ovn/ovn-nb.ovsschema      |  5 +++--
 ovn/ovn-nb.xml            | 12 ++++++++++
 ovn/utilities/ovn-nbctl.c | 24 +++++++++++++++++++-
 tests/ovn-nbctl.at        | 12 ++++++++--
 tests/ovn.at              | 57 +++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 135 insertions(+), 12 deletions(-)

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 3254a61..3c73f36 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -168,12 +168,13 @@ enum ovn_stage {
 #define OVN_ACL_PRI_OFFSET 1000
 
 /* Register definitions specific to switches. */
-#define REGBIT_CONNTRACK_DEFRAG  "reg0[0]"
-#define REGBIT_CONNTRACK_COMMIT  "reg0[1]"
-#define REGBIT_CONNTRACK_NAT     "reg0[2]"
-#define REGBIT_DHCP_OPTS_RESULT  "reg0[3]"
-#define REGBIT_DNS_LOOKUP_RESULT "reg0[4]"
-#define REGBIT_ND_RA_OPTS_RESULT "reg0[5]"
+#define REGBIT_CONNTRACK_DEFRAG     "reg0[0]"
+#define REGBIT_CONNTRACK_COMMIT     "reg0[1]"
+#define REGBIT_CONNTRACK_NAT        "reg0[2]"
+#define REGBIT_CONNTRACK_SET_LABEL  "reg0[3]"
+#define REGBIT_DHCP_OPTS_RESULT     "reg0[4]"
+#define REGBIT_DNS_LOOKUP_RESULT    "reg0[5]"
+#define REGBIT_ND_RA_OPTS_RESULT    "reg0[6]"
 
 /* Register definitions for switches and routers. */
 #define REGBIT_NAT_REDIRECT     "reg9[0]"
@@ -3715,7 +3716,14 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od,
                                   " || (!ct.new && ct.est && !ct.rpl "
                                        "&& ct.blocked == 1)) "
                                   "&& (%s)", acl->match);
-            ds_put_cstr(&actions, REGBIT_CONNTRACK_COMMIT" = 1; ");
+            if (acl->label) {
+               ds_put_format(&actions, REGBIT_CONNTRACK_COMMIT" = 1; "
+                             ""REGBIT_CONNTRACK_SET_LABEL" = 1; "
+                             "xxreg1 = %s; ", acl->label);
+            } else {
+               ds_put_cstr(&actions, REGBIT_CONNTRACK_COMMIT" = 1; ");
+            }
+
             build_acl_log(&actions, acl);
             ds_put_cstr(&actions, "next;");
             ovn_lflow_add_with_hint(lflows, od, stage,
@@ -4153,6 +4161,21 @@ build_stateful(struct ovn_datapath *od, struct hmap *lflows)
     ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 0, "1", "next;");
     ovn_lflow_add(lflows, od, S_SWITCH_OUT_STATEFUL, 0, "1", "next;");
 
+    /* If REGBIT_CONNTRACK_COMMIT is set as 1 and
+     * REGBIT_CONNTRACK_SET_LABEL is set to 1, then the packets should be
+     * committed to conntrack.
+     * We always set ct_mark.blocked to 0 here as
+     * any packet that makes it this far is part of a connection we
+     * want to allow to continue. */
+    ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 150,
+                  REGBIT_CONNTRACK_COMMIT" == 1 && "
+                  ""REGBIT_CONNTRACK_SET_LABEL" == 1",
+                  "ct_commit(ct_mark=0/1, ct_label=xxreg1); next;");
+    ovn_lflow_add(lflows, od, S_SWITCH_OUT_STATEFUL, 150,
+                  REGBIT_CONNTRACK_COMMIT" == 1 && "
+                  ""REGBIT_CONNTRACK_SET_LABEL" == 1",
+                  "ct_commit(ct_mark=0/1, ct_label=xxreg1); next;");
+
     /* If REGBIT_CONNTRACK_COMMIT is set as 1, then the packets should be
      * committed to conntrack. We always set ct.blocked to 0 here as
      * any packet that makes it this far is part of a connection we
diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
index 2c87cbb..75f34c8 100644
--- a/ovn/ovn-nb.ovsschema
+++ b/ovn/ovn-nb.ovsschema
@@ -1,7 +1,7 @@
 {
     "name": "OVN_Northbound",
-    "version": "5.16.0",
-    "cksum": "923459061 23095",
+    "version": "5.17.0",
+    "cksum": "1043090930 23169",
     "tables": {
         "NB_Global": {
             "columns": {
@@ -177,6 +177,7 @@
                                                         "debug"]]},
                                       "min": 0, "max": 1}},
                 "meter": {"type": {"key": "string", "min": 0, "max": 1}},
+                "label": {"type": {"key": "string", "min": 0, "max": 1}},
                 "external_ids": {
                     "type": {"key": "string", "value": "string",
                              "min": 0, "max": "unlimited"}}},
diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
index cbaa949..7c10278 100644
--- a/ovn/ovn-nb.xml
+++ b/ovn/ovn-nb.xml
@@ -1299,6 +1299,18 @@
             default, log messages are not rate-limited.
         </p>
       </column>
+
+      <column name="label">
+        <p>
+            Associates an identifier with the ACL.
+            Same value will be written to corresponding connection
+            tracker entry. Value should be in hex, for example: 0x1234.
+            This value can help in debugging from connection tracker side,
+            for example, through this "label" we can backtrack to the ACL rule
+            which is causing a "leaked" connection.
+        </p>
+      </column>
+
     </group>
 
     <group title="Common Columns">
diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index e86ab7f..f52377d 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -2065,6 +2065,11 @@ nbctl_acl_list(struct ctl_context *ctx)
             ds_chomp(&ctx->output, ',');
             ds_put_cstr(&ctx->output, ")");
         }
+
+        if (acl->label) {
+           ds_put_format(&ctx->output, " label=%s", acl->label);
+        }
+
         ds_put_cstr(&ctx->output, "\n");
     }
 
@@ -2186,6 +2191,23 @@ nbctl_acl_add(struct ctl_context *ctx)
         nbrec_acl_set_meter(acl, meter);
     }
 
+    /* Label */
+    const char *label = shash_find_data(&ctx->options, "--label");
+    if (label) {
+       /* Validate that label is in the hex format (for example: 0x1234) */
+       if (strncmp(label, "0x", 2)) {
+          ctl_error(ctx, "Label: %s, should start with \"0x\"", label);
+          return;
+       }
+
+       if (label[strspn(label + 2, "0123456789abcdefABCDEF") + 2]) {
+          ctl_error(ctx, "Label: %s, should be in hex format", label);
+          return;
+       }
+
+       nbrec_acl_set_label(acl, label);
+    }
+
     /* Check if same acl already exists for the ls/portgroup */
     size_t n_acls = pg ? pg->n_acls : ls->n_acls;
     struct nbrec_acl **acls = pg ? pg->acls : ls->acls;
@@ -5512,7 +5534,7 @@ static const struct ctl_command_syntax nbctl_commands[] = {
     /* acl commands. */
     { "acl-add", 5, 6, "{SWITCH | PORTGROUP} DIRECTION PRIORITY MATCH ACTION",
       NULL, nbctl_acl_add, NULL,
-      "--log,--may-exist,--type=,--name=,--severity=,--meter=", RW },
+      "--log,--may-exist,--type=,--name=,--severity=,--meter=,--label=", RW },
     { "acl-del", 1, 4, "{SWITCH | PORTGROUP} [DIRECTION [PRIORITY MATCH]]",
       NULL, nbctl_acl_del, NULL, "--type=", RW },
     { "acl-list", 1, 1, "{SWITCH | PORTGROUP}",
diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
index 18c5c1d..0c5b681 100644
--- a/tests/ovn-nbctl.at
+++ b/tests/ovn-nbctl.at
@@ -210,19 +210,27 @@ ovn_nbctl_test_acl() {
    AT_CHECK([ovn-nbctl $2 acl-add $1 from-lport 400 tcp drop])
    AT_CHECK([ovn-nbctl $2 acl-add $1 to-lport 300 tcp drop])
    AT_CHECK([ovn-nbctl $2 acl-add $1 from-lport 200 ip drop])
-   AT_CHECK([ovn-nbctl $2 acl-add $1 to-lport 100 ip drop])
+   AT_CHECK([ovn-nbctl $2 --label=0x1234 acl-add $1 to-lport 100 ip drop])
+
    dnl Add duplicated ACL
    AT_CHECK([ovn-nbctl $2 acl-add $1 to-lport 100 ip drop], [1], [], [stderr])
    AT_CHECK([grep 'already existed' stderr], [0], [ignore])
    AT_CHECK([ovn-nbctl $2 --may-exist acl-add $1 to-lport 100 ip drop])
 
+   dnl Add invalid ACL label
+   AT_CHECK([ovn-nbctl $2 --label=1234 acl-add $1 to-lport 50 ip drop], [1], [], [stderr])
+   AT_CHECK([grep 'should start with "0x"' stderr], [0], [ignore])
+
+   AT_CHECK([ovn-nbctl $2 --label=0xagh acl-add $1 to-lport 50 ip drop], [1], [], [stderr])
+   AT_CHECK([grep 'should be in hex format' stderr], [0], [ignore])
+
    AT_CHECK([ovn-nbctl $2 acl-list $1], [0], [dnl
 from-lport   600 (udp) drop log()
 from-lport   400 (tcp) drop
 from-lport   200 (ip) drop
   to-lport   500 (udp) drop log(name=test,severity=info)
   to-lport   300 (tcp) drop
-  to-lport   100 (ip) drop
+  to-lport   100 (ip) drop label=0x1234
 ])
 
    dnl Delete in one direction.
diff --git a/tests/ovn.at b/tests/ovn.at
index f4e3650..547da49 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -12177,6 +12177,63 @@ AT_CHECK([cat 2.packets], [0], [])
 
 AT_CLEANUP
 
+AT_SETUP([ovn -- ACL label])
+AT_KEYWORDS([ovn])
+ovn_start
+
+net_add n1
+
+sim_add hv
+as hv
+ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.1
+for i in lp1 lp2; do
+    ovs-vsctl -- add-port br-int $i -- \
+        set interface $i external-ids:iface-id=$i \
+        options:tx_pcap=hv/$i-tx.pcap \
+        options:rxq_pcap=hv/$i-rx.pcap
+done
+
+lp1_mac="f0:00:00:00:00:01"
+lp1_ip="192.168.1.2"
+
+lp2_mac="f0:00:00:00:00:02"
+lp2_ip="192.168.1.3"
+
+ovn-nbctl ls-add lsw0
+ovn-nbctl --wait=sb lsp-add lsw0 lp1
+ovn-nbctl --wait=sb lsp-add lsw0 lp2
+ovn-nbctl lsp-set-addresses lp1 $lp1_mac
+ovn-nbctl lsp-set-addresses lp2 $lp2_mac
+ovn-nbctl --wait=sb sync
+
+ovn-nbctl --label=0x1234 acl-add lsw0 to-lport 1000 'tcp.dst==82' allow-related
+
+ovn-sbctl dump-flows
+
+# Check logical flow
+AT_CHECK([ovn-sbctl dump-flows | grep ls_out_acl | grep "xxreg1 = 0x1234;" | wc -l], [0], [dnl
+1
+])
+
+AT_CHECK([ovn-sbctl dump-flows | grep ls_out_stateful | grep "ct_label=xxreg1" | wc -l], [0], [dnl
+1
+])
+
+# Send packet.
+packet="inport==\"lp1\" && eth.src==$lp1_mac && eth.dst==$lp2_mac &&
+        ip4 && ip.ttl==64 && ip4.src==$lp1_ip && ip4.dst==$lp2_ip &&
+        tcp && tcp.flags==2 && tcp.src==4362 && tcp.dst==82"
+as hv ovs-appctl -t ovn-controller inject-pkt "$packet"
+
+# Check connection tracker state
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep labels=0x1234 | wc -l], [0], [dnl
+1
+])
+
+OVN_CLEANUP([hv])
+AT_CLEANUP
+
 AT_SETUP([ovn -- TTL exceeded])
 AT_KEYWORDS([ttl-exceeded])
 AT_SKIP_IF([test $HAVE_PYTHON = no])
-- 
1.8.3.1



More information about the dev mailing list