diff mbox series

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

Message ID 1547165793-14659-4-git-send-email-ankur.sharma@nutanix.com
State Changes Requested
Headers show
Series Associate identifier with OVN ACL connection tracking entry | expand

Commit Message

Ankur Sharma Jan. 11, 2019, 12:16 a.m. UTC
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@nutanix.com>
---
 ovn/northd/ovn-northd.c   | 37 ++++++++++++++++++++++++------
 ovn/ovn-nb.ovsschema      |  5 +++--
 ovn/ovn-nb.xml            |  9 ++++++++
 ovn/utilities/ovn-nbctl.c | 24 +++++++++++++++++++-
 tests/ovn-nbctl.at        | 12 ++++++++--
 tests/ovn.at              | 57 +++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 132 insertions(+), 12 deletions(-)

Comments

Ben Pfaff Feb. 5, 2019, 9:40 p.m. UTC | #1
On Fri, Jan 11, 2019 at 12:16:39AM +0000, Ankur Sharma wrote:
> 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@nutanix.com>

Thanks for the patch!

Please capitalize the "S" in "Signed-off-by".

This adds a column in ovn-sb.ovsschema, so it should increment the minor
version (the y in x.y.z).

The documentation for the new column explains what it does, but it does
not explain the purpose.  Why would a user set this column?  What are
its effects?

The column is a string, but its value is an integer.  Maybe that is
because OVSDB integer columns are limited to 64 bits, but this value can
be 128 bits.  That is a very large space.  What is the reason that so
much space should be dedicated to this identifier?  Even 64 bits is more
identifiers than any deployment will ever use, so there must be some
other reason.

Please do not use // comments.

Please document the new option in the ovn-sbctl manpage.

Please add a NEWS item for the new feature.
Ankur Sharma Feb. 6, 2019, 10:06 p.m. UTC | #2
Hi Ben,

Thanks a lot for review.
Sure, V2 will have all the comments addressed.

Reason for using 128 bits:
a. Connection tracker has only 2 fields for metadata, ct.mark(32 bits) and ct.label(128 bits).
b. Purpose of this series is to ensure that we use smaller field, i.e  ct.mark for flags and use the bigger field, i.e ct.label for associating metadata with the ct entry.
c. An example of metadata could be a value which maps ct entry to corresponding OVN ACL or Security Group or both.
d. Yes, I agree that 128 could more than sufficient for most of the cases, but unless we see a use case of dividing ct.label in subfields, i thought we can leverage on full 128 bits.
This keeps implementation simple and  also keeps the interpretation of a connection tracking entry simple.

Please let me know, if it sounds reasonable.

Thanks

Regards,
Ankur

-----Original Message-----
From: Ben Pfaff <blp@ovn.org> 
Sent: Tuesday, February 5, 2019 1:40 PM
To: Ankur Sharma <ankur.sharma@nutanix.com>
Cc: ovs-dev@openvswitch.org
Subject: Re: [ovs-dev] [RFC PATCH v1 3/3] OVN ACL: Allow a user to input ct.label value for an acl

On Fri, Jan 11, 2019 at 12:16:39AM +0000, Ankur Sharma wrote:
> 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@nutanix.com>

Thanks for the patch!

Please capitalize the "S" in "Signed-off-by".

This adds a column in ovn-sb.ovsschema, so it should increment the minor version (the y in x.y.z).

The documentation for the new column explains what it does, but it does not explain the purpose.  Why would a user set this column?  What are its effects?

The column is a string, but its value is an integer.  Maybe that is because OVSDB integer columns are limited to 64 bits, but this value can be 128 bits.  That is a very large space.  What is the reason that so much space should be dedicated to this identifier?  Even 64 bits is more identifiers than any deployment will ever use, so there must be some other reason.

Please do not use // comments.

Please document the new option in the ovn-sbctl manpage.

Please add a NEWS item for the new feature.
Ben Pfaff Feb. 7, 2019, 2:10 a.m. UTC | #3
I'd like to hear some kind of overall use case here.  Sure, you can use
it to identify an OVN ACL, or a security group, or anything else.  How
does that contribute to a larger system?  There should be a hint to the
reader about how and why to use it.

On Wed, Feb 06, 2019 at 10:06:46PM +0000, Ankur Sharma wrote:
> Reason for using 128 bits:
> a. Connection tracker has only 2 fields for metadata, ct.mark(32 bits) and ct.label(128 bits).
> b. Purpose of this series is to ensure that we use smaller field, i.e  ct.mark for flags and use the bigger field, i.e ct.label for associating metadata with the ct entry.
> c. An example of metadata could be a value which maps ct entry to corresponding OVN ACL or Security Group or both.
> d. Yes, I agree that 128 could more than sufficient for most of the cases, but unless we see a use case of dividing ct.label in subfields, i thought we can leverage on full 128 bits.
> This keeps implementation simple and  also keeps the interpretation of a connection tracking entry simple.
> 
> Please let me know, if it sounds reasonable.
> 
> Thanks
> 
> Regards,
> Ankur
> 
> -----Original Message-----
> From: Ben Pfaff <blp@ovn.org> 
> Sent: Tuesday, February 5, 2019 1:40 PM
> To: Ankur Sharma <ankur.sharma@nutanix.com>
> Cc: ovs-dev@openvswitch.org
> Subject: Re: [ovs-dev] [RFC PATCH v1 3/3] OVN ACL: Allow a user to input ct.label value for an acl
> 
> On Fri, Jan 11, 2019 at 12:16:39AM +0000, Ankur Sharma wrote:
> > 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@nutanix.com>
> 
> Thanks for the patch!
> 
> Please capitalize the "S" in "Signed-off-by".
> 
> This adds a column in ovn-sb.ovsschema, so it should increment the minor version (the y in x.y.z).
> 
> The documentation for the new column explains what it does, but it does not explain the purpose.  Why would a user set this column?  What are its effects?
> 
> The column is a string, but its value is an integer.  Maybe that is because OVSDB integer columns are limited to 64 bits, but this value can be 128 bits.  That is a very large space.  What is the reason that so much space should be dedicated to this identifier?  Even 64 bits is more identifiers than any deployment will ever use, so there must be some other reason.
> 
> Please do not use // comments.
> 
> Please document the new option in the ovn-sbctl manpage.
> 
> Please add a NEWS item for the new feature.
Ankur Sharma Feb. 7, 2019, 9:32 p.m. UTC | #4
Hi Ben,

My bad, should have explained the use case as well.

Following is the use case I started with:
a. By associating identifier with connection tracker we can facilitate debugging from connection tracker side.
b. For example, if a system has multiple OVN ACLs (Hundreds or Thousands), and we see an unexpected FLOW getting established.
     We can debug it from connection tracker side to see which "Allow" ACL, let the packet passthrough.


I will update the documentation with this use case as well.
Please let me know your thoughts.

Regards,
Ankur

-----Original Message-----
From: Ben Pfaff <blp@ovn.org> 
Sent: Wednesday, February 6, 2019 6:11 PM
To: Ankur Sharma <ankur.sharma@nutanix.com>
Cc: ovs-dev@openvswitch.org
Subject: Re: [ovs-dev] [RFC PATCH v1 3/3] OVN ACL: Allow a user to input ct.label value for an acl

I'd like to hear some kind of overall use case here.  Sure, you can use it to identify an OVN ACL, or a security group, or anything else.  How does that contribute to a larger system?  There should be a hint to the reader about how and why to use it.

On Wed, Feb 06, 2019 at 10:06:46PM +0000, Ankur Sharma wrote:
> Reason for using 128 bits:
> a. Connection tracker has only 2 fields for metadata, ct.mark(32 bits) and ct.label(128 bits).
> b. Purpose of this series is to ensure that we use smaller field, i.e  ct.mark for flags and use the bigger field, i.e ct.label for associating metadata with the ct entry.
> c. An example of metadata could be a value which maps ct entry to corresponding OVN ACL or Security Group or both.
> d. Yes, I agree that 128 could more than sufficient for most of the cases, but unless we see a use case of dividing ct.label in subfields, i thought we can leverage on full 128 bits.
> This keeps implementation simple and  also keeps the interpretation of a connection tracking entry simple.
> 
> Please let me know, if it sounds reasonable.
> 
> Thanks
> 
> Regards,
> Ankur
> 
> -----Original Message-----
> From: Ben Pfaff <blp@ovn.org>
> Sent: Tuesday, February 5, 2019 1:40 PM
> To: Ankur Sharma <ankur.sharma@nutanix.com>
> Cc: ovs-dev@openvswitch.org
> Subject: Re: [ovs-dev] [RFC PATCH v1 3/3] OVN ACL: Allow a user to 
> input ct.label value for an acl
> 
> On Fri, Jan 11, 2019 at 12:16:39AM +0000, Ankur Sharma wrote:
> > 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@nutanix.com>
> 
> Thanks for the patch!
> 
> Please capitalize the "S" in "Signed-off-by".
> 
> This adds a column in ovn-sb.ovsschema, so it should increment the minor version (the y in x.y.z).
> 
> The documentation for the new column explains what it does, but it does not explain the purpose.  Why would a user set this column?  What are its effects?
> 
> The column is a string, but its value is an integer.  Maybe that is because OVSDB integer columns are limited to 64 bits, but this value can be 128 bits.  That is a very large space.  What is the reason that so much space should be dedicated to this identifier?  Even 64 bits is more identifiers than any deployment will ever use, so there must be some other reason.
> 
> Please do not use // comments.
> 
> Please document the new option in the ovn-sbctl manpage.
> 
> Please add a NEWS item for the new feature.
Ben Pfaff Feb. 8, 2019, 2:51 a.m. UTC | #5
That's helpful.

On Thu, Feb 07, 2019 at 09:32:33PM +0000, Ankur Sharma wrote:
> Hi Ben,
> 
> My bad, should have explained the use case as well.
> 
> Following is the use case I started with:
> a. By associating identifier with connection tracker we can facilitate debugging from connection tracker side.
> b. For example, if a system has multiple OVN ACLs (Hundreds or Thousands), and we see an unexpected FLOW getting established.
>      We can debug it from connection tracker side to see which "Allow" ACL, let the packet passthrough.
> 
> 
> I will update the documentation with this use case as well.
> Please let me know your thoughts.
> 
> Regards,
> Ankur
> 
> -----Original Message-----
> From: Ben Pfaff <blp@ovn.org> 
> Sent: Wednesday, February 6, 2019 6:11 PM
> To: Ankur Sharma <ankur.sharma@nutanix.com>
> Cc: ovs-dev@openvswitch.org
> Subject: Re: [ovs-dev] [RFC PATCH v1 3/3] OVN ACL: Allow a user to input ct.label value for an acl
> 
> I'd like to hear some kind of overall use case here.  Sure, you can use it to identify an OVN ACL, or a security group, or anything else.  How does that contribute to a larger system?  There should be a hint to the reader about how and why to use it.
> 
> On Wed, Feb 06, 2019 at 10:06:46PM +0000, Ankur Sharma wrote:
> > Reason for using 128 bits:
> > a. Connection tracker has only 2 fields for metadata, ct.mark(32 bits) and ct.label(128 bits).
> > b. Purpose of this series is to ensure that we use smaller field, i.e  ct.mark for flags and use the bigger field, i.e ct.label for associating metadata with the ct entry.
> > c. An example of metadata could be a value which maps ct entry to corresponding OVN ACL or Security Group or both.
> > d. Yes, I agree that 128 could more than sufficient for most of the cases, but unless we see a use case of dividing ct.label in subfields, i thought we can leverage on full 128 bits.
> > This keeps implementation simple and  also keeps the interpretation of a connection tracking entry simple.
> > 
> > Please let me know, if it sounds reasonable.
> > 
> > Thanks
> > 
> > Regards,
> > Ankur
> > 
> > -----Original Message-----
> > From: Ben Pfaff <blp@ovn.org>
> > Sent: Tuesday, February 5, 2019 1:40 PM
> > To: Ankur Sharma <ankur.sharma@nutanix.com>
> > Cc: ovs-dev@openvswitch.org
> > Subject: Re: [ovs-dev] [RFC PATCH v1 3/3] OVN ACL: Allow a user to 
> > input ct.label value for an acl
> > 
> > On Fri, Jan 11, 2019 at 12:16:39AM +0000, Ankur Sharma wrote:
> > > 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@nutanix.com>
> > 
> > Thanks for the patch!
> > 
> > Please capitalize the "S" in "Signed-off-by".
> > 
> > This adds a column in ovn-sb.ovsschema, so it should increment the minor version (the y in x.y.z).
> > 
> > The documentation for the new column explains what it does, but it does not explain the purpose.  Why would a user set this column?  What are its effects?
> > 
> > The column is a string, but its value is an integer.  Maybe that is because OVSDB integer columns are limited to 64 bits, but this value can be 128 bits.  That is a very large space.  What is the reason that so much space should be dedicated to this identifier?  Even 64 bits is more identifiers than any deployment will ever use, so there must be some other reason.
> > 
> > Please do not use // comments.
> > 
> > Please document the new option in the ovn-sbctl manpage.
> > 
> > Please add a NEWS item for the new feature.
diff mbox series

Patch

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index a3249c4..2088d51 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -166,12 +166,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]"
@@ -3554,7 +3555,14 @@  consider_acl(struct hmap *lflows, struct ovn_datapath *od,
                                   " || (!ct.new && ct.est && !ct.rpl "
                                        "&& ct_mark.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,
@@ -3988,6 +3996,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_mark.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 f3683df..bd2f1f8 100644
--- a/ovn/ovn-nb.ovsschema
+++ b/ovn/ovn-nb.ovsschema
@@ -1,7 +1,7 @@ 
 {
     "name": "OVN_Northbound",
-    "version": "5.14.0",
-    "cksum": "3600467067 20513",
+    "version": "5.14.1",
+    "cksum": "2042385824 20587",
     "tables": {
         "NB_Global": {
             "columns": {
@@ -171,6 +171,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 6d6fb05..6e624c8 100644
--- a/ovn/ovn-nb.xml
+++ b/ovn/ovn-nb.xml
@@ -1219,6 +1219,15 @@ 
             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.
+        </p>
+      </column>
+
     </group>
 
     <group title="Common Columns">
diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index 2fa0b33..4582fad 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -1997,6 +1997,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");
     }
 
@@ -2118,6 +2123,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 eg: 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;
@@ -5055,7 +5077,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 f55277c..f403606 100644
--- a/tests/ovn-nbctl.at
+++ b/tests/ovn-nbctl.at
@@ -196,19 +196,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 506c979..823de60 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -11282,6 +11282,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])