[ovs-dev] [PATCH ovn] northd: Add ACL label

Priyankar Jain priyankar.jain at nutanix.com
Tue Jul 20 16:16:46 UTC 2021


ping for review.

On 12/07/21 1:04 pm, Priyankar Jain wrote:
> Allow adding label to an ACL to identify which ACL allowed a particular
> flow in the connection tracking table.
> 
> The ACL label covers 32 bits at the end of ct_label. Since only allowed
> connections are committed, only "allow" and "allow-related" ACLs can
> have the label.
> 
> If the ACL allowing the connection changes, the label associated with the
> new ACL gets updated in the ct_label field. This is achieved by committing
> every packet that hits the ACL with the label to the connection tracking
> table.
> In case the new ACL doesn't have a label, the ct_label field is not
> cleared. This is done to prevent any performance change with ACLs that
> don't have label set.
> For the packets which hits an ACL without label, the behaviour remains the
> same as before with respect to the conntrack commit.
> 
> Performance:
> We used ftrace to measure the time taken by an extra conntrack commit for
> the packets hitting the ACL with label. We measured the time taken to
> execute the ovs_ct_execute call inside the sock_sendmsg call.
> 
> ACL used :-
>   from-lport  2000 (tcp && ip4.src == 10.0.0.11 && ip4.dst == 10.0.0.12) allow-related --label=0x1234
> 
> It was observed that the extra ovs_ct_execute call accounted for 1-2%
> of the round trip time (sock_sendmsg duration). The actual percentage
> is expected to be lesser since it doesn't take into account the tracing
> overhead which is substantial for smaller functions.
> 
> Signed-off-by: Priyankar Jain <priyankar.jain at nutanix.com>
> ---
>   lib/logical-fields.c      |   2 +
>   northd/ovn-northd.c       |  53 +++++++++++++++----
>   northd/ovn_northd.dl      |  75 +++++++++++++++++++-------
>   ovn-nb.ovsschema          |   3 +-
>   ovn-nb.xml                |  12 +++++
>   tests/ovn-nbctl.at        |  22 ++++++++
>   tests/ovn-northd.at       | 107 ++++++++++++++++++++++++++++++++++++--
>   tests/ovn.at              |   1 +
>   utilities/ovn-nbctl.8.xml |   2 +-
>   utilities/ovn-nbctl.c     |  32 +++++++++++-
>   10 files changed, 273 insertions(+), 36 deletions(-)
> 
> diff --git a/lib/logical-fields.c b/lib/logical-fields.c
> index 72853013e..7b3d431e0 100644
> --- a/lib/logical-fields.c
> +++ b/lib/logical-fields.c
> @@ -146,6 +146,8 @@ ovn_init_symtab(struct shash *symtab)
>                                       "ct_label[32..79]", WR_CT_COMMIT);
>       expr_symtab_add_subfield_scoped(symtab, "ct_label.ecmp_reply_port", NULL,
>                                       "ct_label[80..95]", WR_CT_COMMIT);
> +    expr_symtab_add_subfield_scoped(symtab, "ct_label.label", NULL,
> +                                    "ct_label[96..127]", WR_CT_COMMIT);
>   
>       expr_symtab_add_field(symtab, "ct_state", MFF_CT_STATE, NULL, false);
>   
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 562dc62b2..92590a49d 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -238,6 +238,7 @@ enum ovn_stage {
>   #define REGBIT_ACL_HINT_BLOCK     "reg0[10]"
>   #define REGBIT_LKUP_FDB           "reg0[11]"
>   #define REGBIT_HAIRPIN_REPLY      "reg0[12]"
> +#define REGBIT_ACL_LABEL          "reg0[13]"
>   
>   #define REG_ORIG_DIP_IPV4         "reg1"
>   #define REG_ORIG_DIP_IPV6         "xxreg1"
> @@ -270,6 +271,9 @@ enum ovn_stage {
>   #define REG_SRC_IPV4 "reg1"
>   #define REG_SRC_IPV6 "xxreg1"
>   
> +/* Register used for setting a label for ACLs in a Logical Switch. */
> +#define REG_LABEL "reg3"
> +
>   #define FLAGBIT_NOT_VXLAN "flags[1] == 0"
>   
>   /*
> @@ -278,14 +282,15 @@ enum ovn_stage {
>    * Logical Switch pipeline:
>    * +----+----------------------------------------------+---+------------------+
>    * | R0 |     REGBIT_{CONNTRACK/DHCP/DNS}              |   |                  |
> - * |    |     REGBIT_{HAIRPIN/HAIRPIN_REPLY}           | X |                  |
> - * |    | REGBIT_ACL_HINT_{ALLOW_NEW/ALLOW/DROP/BLOCK} | X |                  |
> + * |    |     REGBIT_{HAIRPIN/HAIRPIN_REPLY}           |   |                  |
> + * |    | REGBIT_ACL_HINT_{ALLOW_NEW/ALLOW/DROP/BLOCK} |   |                  |
> + * |    |     REGBIT_ACL_LABEL                         | X |                  |
>    * +----+----------------------------------------------+ X |                  |
>    * | R1 |         ORIG_DIP_IPV4 (>= IN_STATEFUL)       | R |                  |
>    * +----+----------------------------------------------+ E |                  |
>    * | R2 |         ORIG_TP_DPORT (>= IN_STATEFUL)       | G |                  |
>    * +----+----------------------------------------------+ 0 |                  |
> - * | R3 |                   UNUSED                     |   |                  |
> + * | R3 |                  ACL LABEL                   |   |                  |
>    * +----+----------------------------------------------+---+------------------+
>    * | R4 |                   UNUSED                     |   |                  |
>    * +----+----------------------------------------------+ X |   ORIG_DIP_IPV6  |
> @@ -5732,7 +5737,12 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od,
>               ds_clear(actions);
>               ds_put_format(match, REGBIT_ACL_HINT_ALLOW_NEW " == 1 && (%s)",
>                             acl->match);
> +
>               ds_put_cstr(actions, REGBIT_CONNTRACK_COMMIT" = 1; ");
> +            if (acl->label) {
> +                ds_put_format(actions, REGBIT_ACL_LABEL" = 1; "
> +                              REG_LABEL" = %s; ", acl->label);
> +            }
>               build_acl_log(actions, acl, meter_groups);
>               ds_put_cstr(actions, "next;");
>               ovn_lflow_add_with_hint(lflows, od, stage,
> @@ -5743,15 +5753,21 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od,
>   
>               /* Match on traffic in the request direction for an established
>                * connection tracking entry that has not been marked for
> -             * deletion.  There is no need to commit here, so we can just
> -             * proceed to the next table. We use this to ensure that this
> +             * deletion. We use this to ensure that this
>                * connection is still allowed by the currently defined
> -             * policy. Match untracked packets too. */
> +             * policy. Match untracked packets too.
> +             * Commit the connection only if the ACL has a label. This is done
> +             * to update the connection tracking entry label in case the ACL
> +             * allowing the connection changes. */
>               ds_clear(match);
>               ds_clear(actions);
>               ds_put_format(match, REGBIT_ACL_HINT_ALLOW " == 1 && (%s)",
>                             acl->match);
> -
> +            if (acl->label) {
> +                ds_put_cstr(actions, REGBIT_CONNTRACK_COMMIT" = 1; ");
> +                ds_put_format(actions, REGBIT_ACL_LABEL" = 1; "
> +                              REG_LABEL" = %s; ", acl->label);
> +            }
>               build_acl_log(actions, acl, meter_groups);
>               ds_put_cstr(actions, "next;");
>               ovn_lflow_add_with_hint(lflows, od, stage,
> @@ -6240,15 +6256,34 @@ 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, 100,
> +                  REGBIT_CONNTRACK_COMMIT" == 1 && "
> +                  REGBIT_ACL_LABEL" == 1",
> +                  "ct_commit { ct_label.blocked = 0; "
> +                  "ct_label.label = " REG_LABEL "; }; next;");
> +    ovn_lflow_add(lflows, od, S_SWITCH_OUT_STATEFUL, 100,
> +                  REGBIT_CONNTRACK_COMMIT" == 1 && "
> +                  REGBIT_ACL_LABEL" == 1",
> +                  "ct_commit { ct_label.blocked = 0; "
> +                  "ct_label.label = " REG_LABEL "; }; next;");
> +
>       /* If REGBIT_CONNTRACK_COMMIT is set as 1, then the packets should be
>        * committed to conntrack. We always set ct_label.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, 100,
> -                  REGBIT_CONNTRACK_COMMIT" == 1",
> +                  REGBIT_CONNTRACK_COMMIT" == 1 && "
> +                  REGBIT_ACL_LABEL" == 0",
>                     "ct_commit { ct_label.blocked = 0; }; next;");
>       ovn_lflow_add(lflows, od, S_SWITCH_OUT_STATEFUL, 100,
> -                  REGBIT_CONNTRACK_COMMIT" == 1",
> +                  REGBIT_CONNTRACK_COMMIT" == 1 && "
> +                  REGBIT_ACL_LABEL" == 0",
>                     "ct_commit { ct_label.blocked = 0; }; next;");
>   }
>   
> diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
> index 499130d7e..175642dad 100644
> --- a/northd/ovn_northd.dl
> +++ b/northd/ovn_northd.dl
> @@ -1502,13 +1502,14 @@ function s_ROUTER_OUT_DELIVERY():       Stage { Stage{ Egress,  4, "lr_out_deliv
>    * Logical Switch pipeline:
>    * +----+----------------------------------------------+---+------------------+
>    * | R0 |     REGBIT_{CONNTRACK/DHCP/DNS}              |   |                  |
> - * |    |     REGBIT_{HAIRPIN/HAIRPIN_REPLY}           | X |                  |
> + * |    |     REGBIT_{HAIRPIN/HAIRPIN_REPLY}           |   |                  |
> + * |    |     REGBIT_ACL_LABEL                         | X |                  |
>    * +----+----------------------------------------------+ X |                  |
>    * | R1 |         ORIG_DIP_IPV4 (>= IN_STATEFUL)       | R |                  |
>    * +----+----------------------------------------------+ E |                  |
>    * | R2 |         ORIG_TP_DPORT (>= IN_STATEFUL)       | G |                  |
>    * +----+----------------------------------------------+ 0 |                  |
> - * | R3 |                   UNUSED                     |   |                  |
> + * | R3 |                   ACL_LABEL                  |   |                  |
>    * +----+----------------------------------------------+---+------------------+
>    * | R4 |                   UNUSED                     |   |                  |
>    * +----+----------------------------------------------+ X |   ORIG_DIP_IPV6  |
> @@ -1581,6 +1582,7 @@ function rEGBIT_ACL_HINT_DROP()    : string = "reg0[9]"
>   function rEGBIT_ACL_HINT_BLOCK()   : string = "reg0[10]"
>   function rEGBIT_LKUP_FDB()         : string = "reg0[11]"
>   function rEGBIT_HAIRPIN_REPLY()    : string = "reg0[12]"
> +function rEGBIT_ACL_LABEL()        : string = "reg0[13]"
>   
>   function rEG_ORIG_DIP_IPV4()       : string = "reg1"
>   function rEG_ORIG_DIP_IPV6()       : string = "xxreg1"
> @@ -1607,6 +1609,9 @@ function rEG_INPORT_ETH_ADDR() : string = "xreg0[0..47]"
>   function rEG_ECMP_GROUP_ID()  : string = "reg8[0..15]"
>   function rEG_ECMP_MEMBER_ID() : string = "reg8[16..31]"
>   
> +/* Register used for setting a label for ACLs in a Logical Switch. */
> +function rEG_LABEL() : string = "reg3"
> +
>   function fLAGBIT_NOT_VXLAN() : string = "flags[1] == 0"
>   
>   function mFF_N_LOG_REGS()          : bit<32> = 10
> @@ -2639,26 +2644,40 @@ for (&SwitchACL(.sw = sw, .acl = acl, .has_fair_meter = fair_meter)) {
>                * that case here and un-set ct_label.blocked (which will be done
>                * by ct_commit in the "stateful" stage) to indicate that the
>                * connection should be allowed to resume.
> +             * If the ACL has a label, then load REG_LABEL with the label and
> +             * set the REGBIT_ACL_LABEL field.
>                */
> -            Flow(.logical_datapath = sw._uuid,
> -                 .stage            = stage,
> -                 .priority         = acl.priority + oVN_ACL_PRI_OFFSET(),
> -                 .__match          = "${rEGBIT_ACL_HINT_ALLOW_NEW()} == 1 && (${acl.__match})",
> -                 .actions          = "${rEGBIT_CONNTRACK_COMMIT()} = 1; ${acl_log}next;",
> -                 .external_ids     = stage_hint);
> +            var __action = match (acl.label) {
> +                None        -> "${rEGBIT_CONNTRACK_COMMIT()} = 1; ${acl_log}next;",
> +                Some{label} -> "${rEGBIT_CONNTRACK_COMMIT()} = 1; ${rEGBIT_ACL_LABEL()} = 1;\
> +                                \ ${rEG_LABEL()} = ${label}; ${acl_log}next;",
> +            } in Flow(.logical_datapath = sw._uuid,
> +                      .stage            = stage,
> +                      .priority         = acl.priority + oVN_ACL_PRI_OFFSET(),
> +                      .__match          = "${rEGBIT_ACL_HINT_ALLOW_NEW()} == 1 && (${acl.__match})",
> +                      .actions          = __action,
> +                      .external_ids     = stage_hint);
>   
>               /* Match on traffic in the request direction for an established
>                * connection tracking entry that has not been marked for
> -             * deletion.  There is no need to commit here, so we can just
> -             * proceed to the next table. We use this to ensure that this
> +             * deletion. We use this to ensure that this
>                * connection is still allowed by the currently defined
> -             * policy. Match untracked packets too. */
> -            Flow(.logical_datapath = sw._uuid,
> -                 .stage            = stage,
> -                 .priority         = acl.priority + oVN_ACL_PRI_OFFSET(),
> -                 .__match          = "${rEGBIT_ACL_HINT_ALLOW()} == 1 && (${acl.__match})",
> -                 .actions          = "${acl_log}next;",
> -                 .external_ids     = stage_hint)
> +             * policy. Match untracked packets too.
> +             * Commit the connection only if the ACL has a label. This is done to
> +             * update the connection tracking entry label in case the ACL
> +             * allowing the connection changes.
> +             */
> +            var __action = match (acl.label) {
> +                None        -> "${acl_log}next;",
> +                Some{label} -> "${rEGBIT_CONNTRACK_COMMIT()} = 1; ${rEGBIT_ACL_LABEL()} = 1;\
> +                                \ ${rEG_LABEL()} = ${label}; ${acl_log}next;",
> +
> +            } in Flow(.logical_datapath = sw._uuid,
> +                      .stage            = stage,
> +                      .priority         = acl.priority + oVN_ACL_PRI_OFFSET(),
> +                      .__match          = "${rEGBIT_ACL_HINT_ALLOW()} == 1 && (${acl.__match})",
> +                      .actions          = __action,
> +                      .external_ids     = stage_hint)
>           }
>       } else if (acl.action == "allow-stateless") {
>           Flow(.logical_datapath = sw._uuid,
> @@ -2870,6 +2889,24 @@ for (&Switch(._uuid = ls_uuid)) {
>            .actions          = "next;",
>            .external_ids     = map_empty());
>   
> +    /* 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_label.blocked to 0 here as
> +     * any packet that makes it this far is part of a connection we
> +     * want to allow to continue. */
> +    Flow(.logical_datapath = ls_uuid,
> +         .stage            = s_SWITCH_IN_STATEFUL(),
> +         .priority         = 100,
> +         .__match          = "${rEGBIT_CONNTRACK_COMMIT()} == 1 && ${rEGBIT_ACL_LABEL()} == 1",
> +         .actions          = "ct_commit { ct_label.blocked = 0; ct_label.label = ${rEG_LABEL()}; }; next;",
> +         .external_ids     = map_empty());
> +    Flow(.logical_datapath = ls_uuid,
> +         .stage            = s_SWITCH_OUT_STATEFUL(),
> +         .priority         = 100,
> +         .__match          = "${rEGBIT_CONNTRACK_COMMIT()} == 1 && ${rEGBIT_ACL_LABEL()} == 1",
> +         .actions          = "ct_commit { ct_label.blocked = 0; ct_label.label = ${rEG_LABEL()}; }; next;",
> +         .external_ids     = map_empty());
> +
>       /* If REGBIT_CONNTRACK_COMMIT is set as 1, then the packets should be
>        * committed to conntrack. We always set ct_label.blocked to 0 here as
>        * any packet that makes it this far is part of a connection we
> @@ -2877,13 +2914,13 @@ for (&Switch(._uuid = ls_uuid)) {
>       Flow(.logical_datapath = ls_uuid,
>            .stage            = s_SWITCH_IN_STATEFUL(),
>            .priority         = 100,
> -         .__match          = "${rEGBIT_CONNTRACK_COMMIT()} == 1",
> +         .__match          = "${rEGBIT_CONNTRACK_COMMIT()} == 1 && ${rEGBIT_ACL_LABEL()} == 0",
>            .actions          = "ct_commit { ct_label.blocked = 0; }; next;",
>            .external_ids     = map_empty());
>       Flow(.logical_datapath = ls_uuid,
>            .stage            = s_SWITCH_OUT_STATEFUL(),
>            .priority         = 100,
> -         .__match          = "${rEGBIT_CONNTRACK_COMMIT()} == 1",
> +         .__match          = "${rEGBIT_CONNTRACK_COMMIT()} == 1 && ${rEGBIT_ACL_LABEL()} == 0",
>            .actions          = "ct_commit { ct_label.blocked = 0; }; next;",
>            .external_ids     = map_empty())
>   }
> diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
> index faf619a1c..ddd4637cd 100644
> --- a/ovn-nb.ovsschema
> +++ b/ovn-nb.ovsschema
> @@ -1,7 +1,7 @@
>   {
>       "name": "OVN_Northbound",
>       "version": "5.32.0",
> -    "cksum": "204590300 28863",
> +    "cksum": "4049685538 28937",
>       "tables": {
>           "NB_Global": {
>               "columns": {
> @@ -233,6 +233,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-nb.xml b/ovn-nb.xml
> index b6a0d1f43..cc79b30ac 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -1779,6 +1779,18 @@
>         and <code>deny</code> as <ref column="action"/>.)
>       </p>
>   
> +    <column name="label">
> +      <p>
> +        Associates an identifier with the ACL.
> +        The same value will be written to corresponding connection
> +        tracker entry. The 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. Connection tracker entries are
> +        created only for allowed connections so the label is valid only
> +        for allow and allow-related actions.
> +      </p>
> +    </column>
>       <column name="priority">
>         <p>
>           The ACL rule's priority.  Rules with numerically higher priority
> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> index 828777b82..53502196d 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -211,18 +211,39 @@ ovn_nbctl_test_acl() {
>      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 from-lport 70 icmp allow-related])
> +   AT_CHECK([ovn-nbctl $2 --label=0x1235 acl-add $1 to-lport 70 icmp allow-related])
> +
>      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=0x1234 acl-add $1 to-lport 50 ip drop], [1], [], [stderr])
> +   AT_CHECK([grep 'can only be set with actions' stderr], [0], [ignore])
> +
> +   AT_CHECK([ovn-nbctl $2 --label=1234 acl-add $1 to-lport 50 ip allow-related], [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 allow-related], [1], [], [stderr])
> +   AT_CHECK([grep 'should be in hex format' stderr], [0], [ignore])
> +
> +   AT_CHECK([ovn-nbctl $2 --label=0x acl-add $1 to-lport 50 ip allow-related], [1], [], [stderr])
> +   AT_CHECK([grep 'should be between 0x0 and 0xffffffff' stderr], [0], [ignore])
> +
> +   AT_CHECK([ovn-nbctl $2 --label=0xfffffffff acl-add $1 to-lport 50 ip allow-related], [1], [], [stderr])
> +   AT_CHECK([grep 'label should be between 0x0 and 0xffffffff' 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
> +from-lport    70 (icmp) allow-related label=0x1234
>     to-lport   500 (udp) drop log(name=test,severity=info)
>     to-lport   300 (tcp) drop
>     to-lport   100 (ip) drop
> +  to-lport    70 (icmp) allow-related label=0x1235
>   ])
>   
>      dnl Delete in one direction.
> @@ -231,6 +252,7 @@ from-lport   200 (ip) drop
>   from-lport   600 (udp) drop log()
>   from-lport   400 (tcp) drop
>   from-lport   200 (ip) drop
> +from-lport    70 (icmp) allow-related label=0x1234
>   ])
>   
>      dnl Delete all ACLs.
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 11461d3f4..123d7362a 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -3515,7 +3515,8 @@ check_stateful_flows() {
>   
>       AT_CHECK([grep "ls_in_stateful" sw0flows | sort], [0], [dnl
>     table=12(ls_in_stateful     ), priority=0    , match=(1), action=(next;)
> -  table=12(ls_in_stateful     ), priority=100  , match=(reg0[[1]] == 1), action=(ct_commit { ct_label.blocked = 0; }; next;)
> +  table=12(ls_in_stateful     ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;)
> +  table=12(ls_in_stateful     ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;)
>     table=12(ls_in_stateful     ), priority=120  , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg1 = 10.0.0.10; reg2[[0..15]] = 80; ct_lb(backends=10.0.0.4:8080);)
>   ])
>   
> @@ -3537,7 +3538,8 @@ check_stateful_flows() {
>   
>       AT_CHECK([grep "ls_out_stateful" sw0flows | sort], [0], [dnl
>     table=7 (ls_out_stateful    ), priority=0    , match=(1), action=(next;)
> -  table=7 (ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1), action=(ct_commit { ct_label.blocked = 0; }; next;)
> +  table=7 (ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;)
> +  table=7 (ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;)
>   ])
>   }
>   
> @@ -3576,7 +3578,8 @@ AT_CHECK([grep "ls_in_pre_stateful" sw0flows | sort], [0], [dnl
>   
>   AT_CHECK([grep "ls_in_stateful" sw0flows | sort], [0], [dnl
>     table=12(ls_in_stateful     ), priority=0    , match=(1), action=(next;)
> -  table=12(ls_in_stateful     ), priority=100  , match=(reg0[[1]] == 1), action=(ct_commit { ct_label.blocked = 0; }; next;)
> +  table=12(ls_in_stateful     ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;)
> +  table=12(ls_in_stateful     ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;)
>   ])
>   
>   AT_CHECK([grep "ls_out_pre_lb" sw0flows | sort], [0], [dnl
> @@ -3594,14 +3597,108 @@ AT_CHECK([grep "ls_out_pre_stateful" sw0flows | sort], [0], [dnl
>   
>   AT_CHECK([grep "ls_out_stateful" sw0flows | sort], [0], [dnl
>     table=7 (ls_out_stateful    ), priority=0    , match=(1), action=(next;)
> -  table=7 (ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1), action=(ct_commit { ct_label.blocked = 0; }; next;)
> +  table=7 (ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;)
> +  table=7 (ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;)
>   ])
>   
>   AT_CLEANUP
>   ])
>   
>   OVN_FOR_EACH_NORTHD([
> -AT_SETUP([ct.inv usage])
> +AT_SETUP([ovn -- ACL label usage])
> +ovn_start
> +
> +check ovn-nbctl ls-add sw0
> +check ovn-nbctl lsp-add sw0 sw0p1
> +
> +check ovn-nbctl --wait=sb --label=0x1234 acl-add sw0 to-lport 1002 tcp allow-related
> +check ovn-nbctl --wait=sb --label=0x1234 acl-add sw0 from-lport 1002 tcp allow-related
> +
> +ovn-sbctl dump-flows sw0 > sw0flows
> +AT_CAPTURE_FILE([sw0flows])
> +
> +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 2002 | sort], [0], [dnl
> +  table=9 (ls_in_acl          ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 0x1234; next;)
> +  table=9 (ls_in_acl          ), priority=2002 , match=(reg0[[8]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 0x1234; next;)
> +])
> +AT_CHECK([grep "ls_in_stateful" sw0flows | sort], [0], [dnl
> +  table=12(ls_in_stateful     ), priority=0    , match=(1), action=(next;)
> +  table=12(ls_in_stateful     ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;)
> +  table=12(ls_in_stateful     ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;)
> +])
> +
> +AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 2002 | sort], [0], [dnl
> +  table=4 (ls_out_acl         ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 0x1234; next;)
> +  table=4 (ls_out_acl         ), priority=2002 , match=(reg0[[8]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 0x1234; next;)
> +])
> +AT_CHECK([grep "ls_out_stateful" sw0flows | sort], [0], [dnl
> +  table=7 (ls_out_stateful    ), priority=0    , match=(1), action=(next;)
> +  table=7 (ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;)
> +  table=7 (ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;)
> +])
> +
> +# Add new ACL without label
> +check ovn-nbctl --wait=sb acl-add sw0 to-lport 1002 udp allow-related
> +check ovn-nbctl --wait=sb acl-add sw0 from-lport 1002 udp allow-related
> +
> +ovn-sbctl dump-flows sw0 > sw0flows
> +AT_CAPTURE_FILE([sw0flows])
> +
> +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 2002 | sort], [0], [dnl
> +  table=9 (ls_in_acl          ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 0x1234; next;)
> +  table=9 (ls_in_acl          ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;)
> +  table=9 (ls_in_acl          ), priority=2002 , match=(reg0[[8]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 0x1234; next;)
> +  table=9 (ls_in_acl          ), priority=2002 , match=(reg0[[8]] == 1 && (udp)), action=(next;)
> +])
> +AT_CHECK([grep "ls_in_stateful" sw0flows | sort], [0], [dnl
> +  table=12(ls_in_stateful     ), priority=0    , match=(1), action=(next;)
> +  table=12(ls_in_stateful     ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;)
> +  table=12(ls_in_stateful     ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;)
> +])
> +
> +AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 2002 | sort], [0], [dnl
> +  table=4 (ls_out_acl         ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 0x1234; next;)
> +  table=4 (ls_out_acl         ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;)
> +  table=4 (ls_out_acl         ), priority=2002 , match=(reg0[[8]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 0x1234; next;)
> +  table=4 (ls_out_acl         ), priority=2002 , match=(reg0[[8]] == 1 && (udp)), action=(next;)
> +])
> +AT_CHECK([grep "ls_out_stateful" sw0flows | sort], [0], [dnl
> +  table=7 (ls_out_stateful    ), priority=0    , match=(1), action=(next;)
> +  table=7 (ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;)
> +  table=7 (ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;)
> +])
> +
> +# Delete new ACL with label
> +check ovn-nbctl --wait=sb acl-del sw0 to-lport 1002 tcp
> +check ovn-nbctl --wait=sb acl-del sw0 from-lport 1002 tcp
> +
> +ovn-sbctl dump-flows sw0 > sw0flows
> +AT_CAPTURE_FILE([sw0flows])
> +
> +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 2002 | sort], [0], [dnl
> +  table=9 (ls_in_acl          ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;)
> +  table=9 (ls_in_acl          ), priority=2002 , match=(reg0[[8]] == 1 && (udp)), action=(next;)
> +])
> +AT_CHECK([grep "ls_in_stateful" sw0flows | sort], [0], [dnl
> +  table=12(ls_in_stateful     ), priority=0    , match=(1), action=(next;)
> +  table=12(ls_in_stateful     ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;)
> +  table=12(ls_in_stateful     ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;)
> +])
> +
> +AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 2002 | sort], [0], [dnl
> +  table=4 (ls_out_acl         ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;)
> +  table=4 (ls_out_acl         ), priority=2002 , match=(reg0[[8]] == 1 && (udp)), action=(next;)
> +])
> +AT_CHECK([grep "ls_out_stateful" sw0flows | sort], [0], [dnl
> +  table=7 (ls_out_stateful    ), priority=0    , match=(1), action=(next;)
> +  table=7 (ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;)
> +  table=7 (ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;)
> +])
> +AT_CLEANUP
> +])
> +
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([ovn -- ct.inv usage])
>   ovn_start
>   
>   check ovn-nbctl ls-add sw0
> diff --git a/tests/ovn.at b/tests/ovn.at
> index e5d8869a8..29d0f7f1b 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -197,6 +197,7 @@ ct_label = NXM_NX_CT_LABEL
>   ct_label.blocked = ct_label[0]
>   ct_label.ecmp_reply_eth = ct_label[32..79]
>   ct_label.ecmp_reply_port = ct_label[80..95]
> +ct_label.label = ct_label[96..127]
>   ct_label.natted = ct_label[1]
>   ct_mark = NXM_NX_CT_MARK
>   ct_state = NXM_NX_CT_STATE
> diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml
> index c3ff87e74..d1d8c24d6 100644
> --- a/utilities/ovn-nbctl.8.xml
> +++ b/utilities/ovn-nbctl.8.xml
> @@ -399,7 +399,7 @@
>         must be either <code>switch</code> or <code>port-group</code>.
>       </p>
>       <dl>
> -      <dt>[<code>--type=</code>{<code>switch</code> | <code>port-group</code>}] [<code>--log</code>] [<code>--meter=</code><var>meter</var>] [<code>--severity=</code><var>severity</var>] [<code>--name=</code><var>name</var>] [<code>--may-exist</code>] <code>acl-add</code> <var>entity</var> <var>direction</var> <var>priority</var> <var>match</var> <var>verdict</var></dt>
> +      <dt>[<code>--type=</code>{<code>switch</code> | <code>port-group</code>}] [<code>--log</code>] [<code>--meter=</code><var>meter</var>] [<code>--severity=</code><var>severity</var>] [<code>--name=</code><var>name</var>] [<code>--label=</code><var>label</var>] [<code>--may-exist</code>] <code>acl-add</code> <var>entity</var> <var>direction</var> <var>priority</var> <var>match</var> <var>verdict</var></dt>
>         <dd>
>           <p>
>             Adds the specified ACL to <var>entity</var>.  <var>direction</var>
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index 3144fba49..a51a6ea8e 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -1988,6 +1988,9 @@ 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");
>       }
>   
> @@ -2070,6 +2073,7 @@ nbctl_pre_acl_list(struct ctl_context *ctx)
>       ovsdb_idl_add_column(ctx->idl, &nbrec_acl_col_name);
>       ovsdb_idl_add_column(ctx->idl, &nbrec_acl_col_severity);
>       ovsdb_idl_add_column(ctx->idl, &nbrec_acl_col_meter);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_acl_col_label);
>   }
>   
>   static void
> @@ -2137,6 +2141,32 @@ nbctl_acl_add(struct ctl_context *ctx)
>           nbrec_acl_set_meter(acl, meter);
>       }
>   
> +    /* Set the ACL label */
> +    const char *label = shash_find_data(&ctx->options, "--label");
> +    if (label) {
> +      /* Ensure that the action is either allow or allow-related */
> +      if (strcmp(action, "allow") && strcmp(action, "allow-related")) {
> +        ctl_error(ctx, "label can only be set with actions \"allow\" or "
> +                  "\"allow-related\"");
> +        return;
> +      }
> +      /* Validate that label is in the hex format (for example: 0x1234) */
> +      size_t label_len = strlen(label);
> +      if (strncmp(label, "0x", 2)) {
> +        ctl_error(ctx, "label: %s, should start with \"0x\"", label);
> +        return;
> +      }
> +      if (label_len < 3 || label_len > 10) {
> +        ctl_error(ctx, "label should be between 0x0 and 0xffffffff");
> +        return;
> +      }
> +      if (strspn(label + 2, "0123456789abcdefABCDEF") + 2 != label_len) {
> +        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;
> @@ -6593,7 +6623,7 @@ static const struct ctl_command_syntax nbctl_commands[] = {
>       /* acl commands. */
>       { "acl-add", 5, 6, "{SWITCH | PORTGROUP} DIRECTION PRIORITY MATCH ACTION",
>         nbctl_pre_acl, 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]]",
>         nbctl_pre_acl, nbctl_acl_del, NULL, "--type=", RW },
>       { "acl-list", 1, 1, "{SWITCH | PORTGROUP}",
> 


More information about the dev mailing list