On Mon, Feb 13, 2023 at 11:36 AM Abhiram Sangana
<sangana.abhi...@nutanix.com> wrote:
>
> This patch adds support to commit connections dropped/rejected by
> ACLs to the connection tracking table. Dropped connections are
> committed to conntrack only if NB_Global options:ct_commit_acl_drop
> is set to true (false by default) and ACL dropping/rejecting the
> connection has label configured. The dropped connections are
> committed in a separate conntrack zone so that they can be managed
> independently and do not interact with the connection tracking state
> of allowed connections.
>
> This provides a new approach to identify connections dropped by ACLs
> besides the existing ACL logging and drop sampling approaches.
>
> Each logical switch is assigned a new conntrack zone for committing
> dropped flows. The zone is loaded into register MFF_LOG_ACL_DROP_ZONE.
> A new lflow action "ct_commit_drop" is introduced that commits flows
> to connection tracking table in a zone identified by
> MFF_LOG_ACL_DROP_ZONE register. An ACL with "drop" or "reject" action
> and non-empty label translates to include "ct_commit_drop" in its
> actions instead of simply dropping/rejecting the packet.
>
> Signed-off-by: Abhiram Sangana <sangana.abhi...@nutanix.com>

Hi Abhiram,

Sorry for the delays in the reviews.

Overall the patch looks good to me.

I do have a few comments and suggestions.

Instead of adding a global option, how about an option per ACL (in the
ACL options column) ?
Users can enable or disable conntrack commit per ACL.

Are you going to use this feature all the time in your deployment ?
Or enable it only when debugging an issue ?
If you want to enable it during debugging,  is it a bit of a hassle to
enable the ct_commit option for the interested ACL ?


I'm a little bit concerned with allocating a zone id for each logical
switch even if there are no ACLs configured with this option.
But I think it's not a big deal.

Thanks
Numan

> ---
>  controller/ovn-controller.c  |  14 +++-
>  controller/physical.c        |  32 ++++++++-
>  include/ovn/actions.h        |   1 +
>  include/ovn/logical-fields.h |   1 +
>  lib/actions.c                |  65 +++++++++++++++++
>  lib/ovn-util.c               |   4 +-
>  lib/ovn-util.h               |   2 +-
>  northd/northd.c              |  25 ++++++-
>  northd/ovn-northd.8.xml      |  30 +++++++-
>  ovn-nb.xml                   |  17 +++--
>  ovn-sb.xml                   |  22 ++++++
>  tests/ovn-nbctl.at           |  10 ++-
>  tests/ovn-northd.at          | 133 ++++++++++++++++++++++++-----------
>  tests/ovn.at                 |  90 +++++++++++++++++++++++-
>  utilities/ovn-nbctl.c        |   7 --
>  utilities/ovn-trace.c        |   2 +
>  16 files changed, 383 insertions(+), 72 deletions(-)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 265740cab..e8f0b7242 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -734,8 +734,8 @@ update_ct_zones(const struct shash *binding_lports,
>      HMAP_FOR_EACH (ld, hmap_node, local_datapaths) {
>          /* XXX Add method to limit zone assignment to logical router
>           * datapaths with NAT */
> -        char *dnat = alloc_nat_zone_key(&ld->datapath->header_.uuid, "dnat");
> -        char *snat = alloc_nat_zone_key(&ld->datapath->header_.uuid, "snat");
> +        char *dnat = alloc_ct_zone_key(&ld->datapath->header_.uuid, "dnat");
> +        char *snat = alloc_ct_zone_key(&ld->datapath->header_.uuid, "snat");
>          sset_add(&all_users, dnat);
>          sset_add(&all_users, snat);
>
> @@ -745,6 +745,14 @@ update_ct_zones(const struct shash *binding_lports,
>          }
>          free(dnat);
>          free(snat);
> +
> +        /* Zone for committing connections dropped by ACLs with labels. */
> +        if (ld->is_switch) {
> +            char *drop = alloc_ct_zone_key(
> +                &ld->datapath->header_.uuid, "drop");
> +            sset_add(&all_users, drop);
> +            free(drop);
> +        }
>      }
>
>      /* Delete zones that do not exist in above sset. */
> @@ -2420,7 +2428,7 @@ ct_zones_datapath_binding_handler(struct engine_node 
> *node, void *data)
>          /* Check if the requested snat zone has changed for the datapath
>           * or not.  If so, then fall back to full recompute of
>           * ct_zone engine. */
> -        char *snat_dp_zone_key = alloc_nat_zone_key(&dp->header_.uuid, 
> "snat");
> +        char *snat_dp_zone_key = alloc_ct_zone_key(&dp->header_.uuid, 
> "snat");
>          struct simap_node *simap_node = simap_find(&ct_zones_data->current,
>                                                     snat_dp_zone_key);
>          free(snat_dp_zone_key);
> diff --git a/controller/physical.c b/controller/physical.c
> index 4dcf44e01..3c573c492 100644
> --- a/controller/physical.c
> +++ b/controller/physical.c
> @@ -60,6 +60,7 @@ struct zone_ids {
>      int ct;                     /* MFF_LOG_CT_ZONE. */
>      int dnat;                   /* MFF_LOG_DNAT_ZONE. */
>      int snat;                   /* MFF_LOG_SNAT_ZONE. */
> +    int drop;                   /* MFF_LOG_ACL_DROP_ZONE. */
>  };
>
>  struct tunnel {
> @@ -204,14 +205,18 @@ get_zone_ids(const struct sbrec_port_binding *binding,
>
>      const struct uuid *key = &binding->datapath->header_.uuid;
>
> -    char *dnat = alloc_nat_zone_key(key, "dnat");
> +    char *dnat = alloc_ct_zone_key(key, "dnat");
>      zone_ids.dnat = simap_get(ct_zones, dnat);
>      free(dnat);
>
> -    char *snat = alloc_nat_zone_key(key, "snat");
> +    char *snat = alloc_ct_zone_key(key, "snat");
>      zone_ids.snat = simap_get(ct_zones, snat);
>      free(snat);
>
> +    char *drop = alloc_ct_zone_key(key, "drop");
> +    zone_ids.drop = simap_get(ct_zones, drop);
> +    free(drop);
> +
>      return zone_ids;
>  }
>
> @@ -830,6 +835,9 @@ put_zones_ofpacts(const struct zone_ids *zone_ids, struct 
> ofpbuf *ofpacts_p)
>          if (zone_ids->snat) {
>              put_load(zone_ids->snat, MFF_LOG_SNAT_ZONE, 0, 32, ofpacts_p);
>          }
> +        if (zone_ids->drop) {
> +            put_load(zone_ids->drop, MFF_LOG_ACL_DROP_ZONE, 0, 32, 
> ofpacts_p);
> +        }
>      }
>  }
>
> @@ -896,6 +904,26 @@ put_local_common_flows(uint32_t dp_key,
>                      pb->header_.uuid.parts[0], &match, ofpacts_p,
>                      &pb->header_.uuid);
>
> +    if (zone_ids->drop) {
> +        /* Table 39, Priority 1.
> +         * =======================
> +         *
> +         * Clear the logical registers (for consistent behavior with packets
> +         * that get tunneled) except MFF_LOG_ACL_DROP_ZONE. */
> +        match_init_catchall(&match);
> +        ofpbuf_clear(ofpacts_p);
> +        match_set_metadata(&match, htonll(dp_key));
> +        for (int i = 0; i < MFF_N_LOG_REGS; i++) {
> +            if ((MFF_REG0 + i) != MFF_LOG_ACL_DROP_ZONE) {
> +                put_load(0, MFF_REG0 + i, 0, 32, ofpacts_p);
> +            }
> +        }
> +        put_resubmit(OFTABLE_LOG_EGRESS_PIPELINE, ofpacts_p);
> +        ofctrl_add_flow(flow_table, OFTABLE_CHECK_LOOPBACK, 1,
> +                        pb->datapath->header_.uuid.parts[0], &match,
> +                        ofpacts_p, &pb->datapath->header_.uuid);
> +    }
> +
>      /* Table 39, Priority 100.
>       * =======================
>       *
> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> index 6ca08b3c6..c7a6785d3 100644
> --- a/include/ovn/actions.h
> +++ b/include/ovn/actions.h
> @@ -125,6 +125,7 @@ struct ovn_extend_table;
>      OVNACT(COMMIT_LB_AFF,     ovnact_commit_lb_aff)   \
>      OVNACT(CHK_LB_AFF,        ovnact_result)          \
>      OVNACT(SAMPLE,            ovnact_sample)          \
> +    OVNACT(CT_COMMIT_DROP,    ovnact_nest)            \
>
>  /* enum ovnact_type, with a member OVNACT_<ENUM> for each action. */
>  enum OVS_PACKED_ENUM ovnact_type {
> diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h
> index a7b64ef67..c7ea9e0ce 100644
> --- a/include/ovn/logical-fields.h
> +++ b/include/ovn/logical-fields.h
> @@ -47,6 +47,7 @@ enum ovn_controller_event {
>  #define MFF_LOG_REG0             MFF_REG0
>  #define MFF_LOG_LB_ORIG_DIP_IPV4 MFF_REG1
>  #define MFF_LOG_LB_ORIG_TP_DPORT MFF_REG2
> +#define MFF_LOG_ACL_DROP_ZONE    MFF_REG9
>
>  #define MFF_LOG_XXREG0           MFF_XXREG0
>  #define MFF_LOG_LB_ORIG_DIP_IPV6 MFF_XXREG1
> diff --git a/lib/actions.c b/lib/actions.c
> index 2da5a696b..d86b1efbc 100644
> --- a/lib/actions.c
> +++ b/lib/actions.c
> @@ -5210,6 +5210,69 @@ encode_CHK_LB_AFF(const struct ovnact_result *res,
>                             MLF_USE_LB_AFF_SESSION_BIT, ofpacts);
>  }
>
> +static void
> +parse_ct_commit_drop(struct action_context *ctx)
> +{
> +    if (ctx->lexer->token.type == LEX_T_LCURLY) {
> +        parse_nested_action(ctx, OVNACT_CT_COMMIT_DROP, "ip",
> +                            WR_CT_COMMIT);
> +    } else {
> +        /* Add an empty nested action to allow for "ct_commit_drop;" syntax 
> */
> +        add_prerequisite(ctx, "ip");
> +        struct ovnact_nest *on = ovnact_put(ctx->ovnacts,
> +                                            OVNACT_CT_COMMIT_DROP,
> +                                            OVNACT_ALIGN(sizeof *on));
> +        on->nested_len = 0;
> +        on->nested = NULL;
> +    }
> +}
> +
> +static void
> +format_CT_COMMIT_DROP(const struct ovnact_nest *on, struct ds *s)
> +{
> +    if (on->nested_len) {
> +        format_nested_action(on, "ct_commit_drop", s);
> +    } else {
> +        ds_put_cstr(s, "ct_commit_drop;");
> +    }
> +}
> +
> +static void
> +encode_CT_COMMIT_DROP(const struct ovnact_nest *on,
> +                      const struct ovnact_encode_params *ep OVS_UNUSED,
> +                      struct ofpbuf *ofpacts)
> +{
> +    struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
> +    ct->flags = NX_CT_F_COMMIT;
> +    ct->recirc_table = NX_CT_RECIRC_NONE;
> +    ct->zone_src.field = mf_from_id(MFF_LOG_ACL_DROP_ZONE);
> +    ct->zone_src.ofs = 0;
> +    ct->zone_src.n_bits = 16;
> +
> +    /* If the datapath supports all-zero SNAT then use it to avoid tuple
> +     * collisions at commit time between NATed and firewalled-only sessions.
> +     */
> +    if (ovs_feature_is_supported(OVS_CT_ZERO_SNAT_SUPPORT)) {
> +        size_t nat_offset = ofpacts->size;
> +        ofpbuf_pull(ofpacts, nat_offset);
> +
> +        struct ofpact_nat *nat = ofpact_put_NAT(ofpacts);
> +        nat->flags = 0;
> +        nat->range_af = AF_UNSPEC;
> +        nat->flags |= NX_NAT_F_SRC;
> +        ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset);
> +        ct = ofpacts->header;
> +    }
> +
> +    size_t set_field_offset = ofpacts->size;
> +    ofpbuf_pull(ofpacts, set_field_offset);
> +
> +    ovnacts_encode(on->nested, on->nested_len, ep, ofpacts);
> +    ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset);
> +    ct = ofpacts->header;
> +    ofpact_finish(ofpacts, &ct->ofpact);
> +}
> +
>  /* Parses an assignment or exchange or put_dhcp_opts action. */
>  static void
>  parse_set_action(struct action_context *ctx)
> @@ -5415,6 +5478,8 @@ parse_action(struct action_context *ctx)
>          parse_commit_lb_aff(ctx, ovnact_put_COMMIT_LB_AFF(ctx->ovnacts));
>      } else if (lexer_match_id(ctx->lexer, "sample")) {
>          parse_sample(ctx);
> +    } else if (lexer_match_id(ctx->lexer, "ct_commit_drop")) {
> +        parse_ct_commit_drop(ctx);
>      } else {
>          lexer_syntax_error(ctx->lexer, "expecting action");
>      }
> diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> index f3665b89f..646aff6ef 100644
> --- a/lib/ovn-util.c
> +++ b/lib/ovn-util.c
> @@ -443,12 +443,12 @@ split_addresses(const char *addresses, struct svec 
> *ipv4_addrs,
>      destroy_lport_addresses(&laddrs);
>  }
>
> -/* Allocates a key for NAT conntrack zone allocation for a provided
> +/* Allocates a key for conntrack zone allocation for a provided
>   * 'key' record and a 'type'.
>   *
>   * It is the caller's responsibility to free the allocated memory. */
>  char *
> -alloc_nat_zone_key(const struct uuid *key, const char *type)
> +alloc_ct_zone_key(const struct uuid *key, const char *type)
>  {
>      return xasprintf(UUID_FMT"_%s", UUID_ARGS(key), type);
>  }
> diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> index cd19919cb..170685872 100644
> --- a/lib/ovn-util.h
> +++ b/lib/ovn-util.h
> @@ -92,7 +92,7 @@ const char *find_lport_address(const struct lport_addresses 
> *laddrs,
>  void split_addresses(const char *addresses, struct svec *ipv4_addrs,
>                       struct svec *ipv6_addrs);
>
> -char *alloc_nat_zone_key(const struct uuid *key, const char *type);
> +char *alloc_ct_zone_key(const struct uuid *key, const char *type);
>
>  const char *default_nb_db(void);
>  const char *default_sb_db(void);
> diff --git a/northd/northd.c b/northd/northd.c
> index 77e105b86..bc39e9bc6 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -84,6 +84,10 @@ static bool use_ct_inv_match = true;
>   */
>  static bool default_acl_drop;
>
> +/* If this option is 'true', drop/reject ACLs with label are translated to
> + * commit flows to conntrack besides dropping/rejecting them. */
> +static bool ct_commit_acl_drop;
> +
>  #define MAX_OVN_TAGS 4096
>
>  /* Pipeline stages. */
> @@ -311,7 +315,7 @@ enum ovn_stage {
>   * 
> +----+----------------------------------------------+---+-----------------------------------+
>   * | R8 |              LB_AFF_MATCH_PORT               |
>   * +----+----------------------------------------------+
> - * | R9 |                   UNUSED                     |
> + * | R9 |              ACL DROP CT ZONE                |
>   * +----+----------------------------------------------+
>   *
>   * Logical Router pipeline:
> @@ -6493,6 +6497,14 @@ consider_acl(struct hmap *lflows, struct ovn_datapath 
> *od,
>              ds_clear(match);
>              ds_clear(actions);
>              ds_put_cstr(match, REGBIT_ACL_HINT_DROP " == 1");
> +            /* If the ACL has a label, we commit the packet in
> +             * a separate zone for debugging purposes before
> +             * rejecting/dropping it. */
> +            if (ct_commit_acl_drop && acl->label) {
> +                ds_put_format(actions, "ct_commit_drop { "
> +                              "ct_label.label = %"PRId64"; }; ",
> +                              acl->label);
> +            }
>              if (!strcmp(acl->action, "reject")) {
>                  build_reject_acl_rules(od, lflows, stage, acl, match,
>                                         actions, &acl->header_, meter_groups);
> @@ -6519,8 +6531,15 @@ consider_acl(struct hmap *lflows, struct ovn_datapath 
> *od,
>              ds_clear(match);
>              ds_clear(actions);
>              ds_put_cstr(match, REGBIT_ACL_HINT_BLOCK " == 1");
> -            ds_put_format(actions, "ct_commit { %s = 1; }; ",
> +            ds_put_format(actions, "ct_commit { %s = 1; ",
>                            ct_blocked_match);
> +            /* Update ct_label.label to reflect the new policy matching
> +             * the connection. */
> +            if (acl->label) {
> +                ds_put_format(actions, "ct_label.label = %"PRId64"; ",
> +                              acl->label);
> +            }
> +            ds_put_cstr(actions, "}; ");
>              if (!strcmp(acl->action, "reject")) {
>                  build_reject_acl_rules(od, lflows, stage, acl, match,
>                                         actions, &acl->header_, meter_groups);
> @@ -16236,6 +16255,8 @@ ovnnb_db_run(struct northd_input *input_data,
>      check_lsp_is_up = !smap_get_bool(&nb->options,
>                                       "ignore_lsp_down", true);
>      default_acl_drop = smap_get_bool(&nb->options, "default_acl_drop", 
> false);
> +    ct_commit_acl_drop = smap_get_bool(&nb->options,
> +                                       "ct_commit_acl_drop", false);
>
>      install_ls_lb_from_router = smap_get_bool(&nb->options,
>                                                "install_ls_lb_from_router",
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index 3d7a92ea8..74f34cfe8 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -727,13 +727,26 @@
>          action for TCP connections,<code>icmp4/icmp6</code> action
>          for UDP connections, and <code>sctp_abort {output &lt;-%gt; inport;
>          next(pipeline=egress,table=5);}</code> action for SCTP associations.
> +        Additionally, if the ACL has a <code>label</code> and
> +        <ref column="options:ct_commit_acl_drop" table="NB_Global"
> +        db="OVN_Northbound"/> column of <ref table="NB_Global"
> +        db="OVN_Northbound"/> table is <code>true</code>, the translated
> +        action includes <code>ct_commit_drop(ct_label.label=label)</code>.
>        </li>
>        <li>
>          Other ACLs translate to <code>drop;</code> for new or untracked
>          connections and <code>ct_commit(ct_label=1/1);</code> for known
>          connections.  Setting <code>ct_label</code> marks a connection
>          as one that was previously allowed, but should no longer be
> -        allowed due to a policy change.
> +        allowed due to a policy change. If the ACLs have <code>label</code>,
> +        then they instead translate to
> +        <code>ct_commit_drop(ct_label.label=label)</code> for
> +        new and untracked connections if
> +        <ref column="options:ct_commit_acl_drop" table="NB_Global"
> +        db="OVN_Northbound"/> column of <ref table="NB_Global"
> +        db="OVN_Northbound"/> table is also set to <code>true</code> and
> +        <code>ct_commit(ct_label.blocked=1; ct_label.label=label);</code>
> +        for known connections.
>        </li>
>      </ul>
>
> @@ -1165,13 +1178,26 @@
>          action for TCP connections,<code>icmp4/icmp6</code> action
>          for UDP connections, and <code>sctp_abort {output &lt;-%gt; inport;
>          next(pipeline=egress,table=5);}</code> action for SCTP associations.
> +        Additionally, if the ACL has a <code>label</code> and
> +        <ref column="options:ct_commit_acl_drop" table="NB_Global"
> +        db="OVN_Northbound"/> column of <ref table="NB_Global"
> +        db="OVN_Northbound"/> table is <code>true</code>, the translated
> +        action includes <code>ct_commit_drop(ct_label.label=label)</code>.
>        </li>
>        <li>
>          Other apply-after-lb ACLs translate to <code>drop;</code> for new
>          or untracked connections and <code>ct_commit(ct_label=1/1);</code> 
> for
>          known connections.  Setting <code>ct_label</code> marks a connection
>          as one that was previously allowed, but should no longer be
> -        allowed due to a policy change.
> +        allowed due to a policy change. If the ACLs have <code>label</code>,
> +        then they instead translate to
> +        <code>ct_commit_drop(ct_label.label=label)</code> for
> +        new and untracked connections if
> +        <ref column="options:ct_commit_acl_drop" table="NB_Global"
> +        db="OVN_Northbound"/> column of <ref table="NB_Global"
> +        db="OVN_Northbound"/> table is also set to <code>true</code> and
> +        <code>ct_commit(ct_label.blocked=1; ct_label.label=label);</code>
> +        for known connections.
>        </li>
>      </ul>
>
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index 4b52b9953..d28c2fea8 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -301,6 +301,15 @@
>          </p>
>        </column>
>
> +      <column name="options" key="ct_commit_acl_drop">
> +        <p>
> +          If set to <code>true</code>, connections dropped/rejected by ACLs
> +          with label are committed to DROP conntrack zone of
> +          <code>logical_switch</code>. By default this option is set to
> +          <code>false</code>.
> +        </p>
> +      </column>
> +
>        <group title="Options for configuring interconnection route 
> advertisement">
>          <p>
>            These options control how routes are advertised between OVN
> @@ -2106,12 +2115,10 @@ or
>        <p>
>          Associates an identifier with the ACL.
>          The same value will be written to corresponding connection
> -        tracker entry. The value should be a valid 32-bit unsigned integer.
> -        This value can help in debugging from connection tracker side.
> +        tracking entry. The value should be a valid 32-bit unsigned integer.
> +        This value can help in debugging from connection tracking 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.
> +        which is causing a "leaked" connection.
>        </p>
>      </column>
>      <column name="priority">
> diff --git a/ovn-sb.xml b/ovn-sb.xml
> index a77f8f4ef..f01f9a927 100644
> --- a/ovn-sb.xml
> +++ b/ovn-sb.xml
> @@ -1408,6 +1408,28 @@
>            </p>
>          </dd>
>
> +        <dt><code>ct_commit_drop { };</code></dt>
> +        <dt><code>ct_commit_drop { ct_mark=<var>value[/mask]</var>; 
> };</code></dt>
> +        <dt><code>ct_commit_drop { ct_label=<var>value[/mask]</var>; 
> };</code></dt>
> +        <dt><code>ct_commit_drop { ct_mark=<var>value[/mask]</var>; 
> ct_label=<var>value[/mask]</var>; };</code></dt>
> +        <dd>
> +          <p>
> +            Commit the flow to a connection tracking entry in the
> +            logical switch DROP zone. This action is identical to
> +            <code>ct_commit</code> besides the connection tracking entry
> +            the flow is committed to.
> +          </p>
> +
> +          <p>
> +            This action is useful to commit connection tracking state for
> +            packets before they are dropped for debugging purposes.
> +            Committing the packets in a separate zone ensures that connection
> +            tracking state of dropped connections doesn't interact with the
> +            state of allowed connections and allows the DROP zone to be
> +            managed separately.
> +          </p>
> +        </dd>
> +
>          <dt><code>ct_dnat;</code></dt>
>          <dt><code>ct_dnat(<var>IP</var>);</code></dt>
>          <dd>
> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> index 2fffe1850..cfbb5e45a 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -222,7 +222,8 @@ ovn_nbctl_test_acl() {
>     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=1234 acl-add $1 from-lport 70 icmp 
> allow-related])
> -   AT_CHECK([ovn-nbctl $2 --label=1235 acl-add $1 to-lport 70 icmp 
> allow-related])
> +   AT_CHECK([ovn-nbctl $2 --label=1235 acl-add $1 to-lport 70 icmp reject])
> +   AT_CHECK([ovn-nbctl $2 --label=1234 acl-add $1 to-lport 50 ip drop])
>     AT_CHECK([ovn-nbctl $2 --apply-after-lb acl-add $1 from-lport 500 tcp 
> allow])
>     AT_CHECK([ovn-nbctl $2 --apply-after-lb acl-add $1 from-lport 300 tcp 
> drop])
>     AT_CHECK([ovn-nbctl $2 --apply-after-lb acl-add $1 from-lport 300 udp 
> allow])
> @@ -232,10 +233,6 @@ ovn_nbctl_test_acl() {
>     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 'can only be set with actions' stderr], [0], [ignore])
> -
>     AT_CHECK([ovn-nbctl $2 --label=abcd acl-add $1 to-lport 50 ip 
> allow-related], [1], [], [stderr])
>     AT_CHECK([grep 'label must in range 0...4294967295' stderr], [0], 
> [ignore])
>
> @@ -256,7 +253,8 @@ from-lport   300 (udp) allow [[after-lb]]
>    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=1235
> +  to-lport    70 (icmp) reject label=1235
> +  to-lport    50 (ip) drop label=1234
>  ])
>
>     dnl Delete in one direction.
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 3fa02d2b3..9fbb82e37 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -4241,88 +4241,141 @@ ovn_start
>  check ovn-nbctl ls-add sw0
>  check ovn-nbctl lsp-add sw0 sw0p1
>
> +AS_BOX([Create ACLs with labels])
>  check ovn-nbctl --wait=sb --label=1234 acl-add sw0 to-lport 1002 tcp 
> allow-related
> +check ovn-nbctl --wait=sb --label=2345 acl-add sw0 to-lport 1001 ip drop
>  check ovn-nbctl --wait=sb --label=1234 acl-add sw0 from-lport 1002 tcp 
> allow-related
> +check ovn-nbctl --wait=sb --label=2345 acl-add sw0 from-lport 1001 ip reject
>
>  ovn-sbctl dump-flows sw0 > sw0flows
>  AT_CAPTURE_FILE([sw0flows])
>
> -AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 2002 | sort | sed 
> 's/table=./table=?/'], [0], [dnl
> -  table=? (ls_in_acl          ), priority=2002 , match=(reg0[[7]] == 1 && 
> (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;)
> -  table=? (ls_in_acl          ), priority=2002 , match=(reg0[[8]] == 1 && 
> (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;)
> +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep -e 2002 -e 2001 | sort | sed 
> 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl
> +  table=??(ls_in_acl          ), priority=2001 , match=((reg0[[10]] == 1) && 
> ip), action=(ct_commit { ct_mark.blocked = 1; ct_label.label = 2345; };  reg0 
> = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ 
> outport <-> inport; next(pipeline=egress,table=5); };)
> +  table=??(ls_in_acl          ), priority=2001 , match=((reg0[[9]] == 1) && 
> ip), action=(reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is 
> implicit. */ outport <-> inport; next(pipeline=egress,table=5); };)
> +  table=??(ls_in_acl          ), priority=2002 , match=(reg0[[7]] == 1 && 
> (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;)
> +  table=??(ls_in_acl          ), priority=2002 , match=(reg0[[8]] == 1 && 
> (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;)
>  ])
> -AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed 
> 's/table=../table=??/'], [0], [dnl
> +AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed 
> 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl
>    table=??(ls_in_stateful     ), priority=0    , match=(1), action=(next;)
>    table=??(ls_in_stateful     ), priority=100  , match=(reg0[[1]] == 1 && 
> reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;)
>    table=??(ls_in_stateful     ), priority=100  , match=(reg0[[1]] == 1 && 
> reg0[[13]] == 1), action=(ct_commit { ct_mark.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 = 1234; next;)
> -  table=4 (ls_out_acl         ), priority=2002 , match=(reg0[[8]] == 1 && 
> (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;)
> +AT_CHECK([grep -w "ls_out_acl" sw0flows | grep -e 2002 -e 2001 | sort | sed 
> 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl
> +  table=??(ls_out_acl         ), priority=2001 , match=(reg0[[10]] == 1 && 
> (ip)), action=(ct_commit { ct_mark.blocked = 1; ct_label.label = 2345; }; /* 
> drop */)
> +  table=??(ls_out_acl         ), priority=2001 , match=(reg0[[9]] == 1 && 
> (ip)), action=(/* drop */)
> +  table=??(ls_out_acl         ), priority=2002 , match=(reg0[[7]] == 1 && 
> (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;)
> +  table=??(ls_out_acl         ), priority=2002 , match=(reg0[[8]] == 1 && 
> (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; 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_mark.blocked = 0; }; next;)
> -  table=7 (ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1 && 
> reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = 
> reg3; }; next;)
> +AT_CHECK([grep "ls_out_stateful" sw0flows | sort | sed 
> 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl
> +  table=??(ls_out_stateful    ), priority=0    , match=(1), action=(next;)
> +  table=??(ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1 && 
> reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;)
> +  table=??(ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1 && 
> reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = 
> reg3; }; next;)
> +])
> +
> +AS_BOX([Set ct_commit_acl_drop option])
> +check ovn-nbctl --wait=sb set NB_Global . options:ct_commit_acl_drop=true
> +
> +ovn-sbctl dump-flows sw0 > sw0flows
> +AT_CAPTURE_FILE([sw0flows])
> +
> +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep -e 2002 -e 2001 | sort | sed 
> 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl
> +  table=??(ls_in_acl          ), priority=2001 , match=((reg0[[10]] == 1) && 
> ip), action=(ct_commit { ct_mark.blocked = 1; ct_label.label = 2345; };  reg0 
> = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ 
> outport <-> inport; next(pipeline=egress,table=5); };)
> +  table=??(ls_in_acl          ), priority=2001 , match=((reg0[[9]] == 1) && 
> ip), action=(ct_commit_drop { ct_label.label = 2345; };  reg0 = 0; reject { 
> /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ outport <-> 
> inport; next(pipeline=egress,table=5); };)
> +  table=??(ls_in_acl          ), priority=2002 , match=(reg0[[7]] == 1 && 
> (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;)
> +  table=??(ls_in_acl          ), priority=2002 , match=(reg0[[8]] == 1 && 
> (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;)
> +])
> +AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed 
> 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl
> +  table=??(ls_in_stateful     ), priority=0    , match=(1), action=(next;)
> +  table=??(ls_in_stateful     ), priority=100  , match=(reg0[[1]] == 1 && 
> reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;)
> +  table=??(ls_in_stateful     ), priority=100  , match=(reg0[[1]] == 1 && 
> reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = 
> reg3; }; next;)
>  ])
>
> -# Add new ACL without label
> +AT_CHECK([grep -w "ls_out_acl" sw0flows | grep -e 2002 -e 2001 | sort | sed 
> 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl
> +  table=??(ls_out_acl         ), priority=2001 , match=(reg0[[10]] == 1 && 
> (ip)), action=(ct_commit { ct_mark.blocked = 1; ct_label.label = 2345; }; /* 
> drop */)
> +  table=??(ls_out_acl         ), priority=2001 , match=(reg0[[9]] == 1 && 
> (ip)), action=(ct_commit_drop { ct_label.label = 2345; }; /* drop */)
> +  table=??(ls_out_acl         ), priority=2002 , match=(reg0[[7]] == 1 && 
> (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;)
> +  table=??(ls_out_acl         ), priority=2002 , match=(reg0[[8]] == 1 && 
> (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;)
> +])
> +AT_CHECK([grep "ls_out_stateful" sw0flows | sort | sed 
> 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl
> +  table=??(ls_out_stateful    ), priority=0    , match=(1), action=(next;)
> +  table=??(ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1 && 
> reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;)
> +  table=??(ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1 && 
> reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = 
> reg3; }; next;)
> +])
> +
> +AS_BOX([Add ACLs without labels])
>  check ovn-nbctl --wait=sb acl-add sw0 to-lport 1002 udp allow-related
> +check ovn-nbctl --wait=sb acl-add sw0 to-lport 1001 ip6 drop
>  check ovn-nbctl --wait=sb acl-add sw0 from-lport 1002 udp allow-related
> +check ovn-nbctl --wait=sb acl-add sw0 from-lport 1001 ip6 reject
>
>  ovn-sbctl dump-flows sw0 > sw0flows
>  AT_CAPTURE_FILE([sw0flows])
>
> -AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 2002 | sort | sed 
> 's/table=./table=?/'], [0], [dnl
> -  table=? (ls_in_acl          ), priority=2002 , match=(reg0[[7]] == 1 && 
> (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;)
> -  table=? (ls_in_acl          ), priority=2002 , match=(reg0[[7]] == 1 && 
> (udp)), action=(reg0[[1]] = 1; next;)
> -  table=? (ls_in_acl          ), priority=2002 , match=(reg0[[8]] == 1 && 
> (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;)
> -  table=? (ls_in_acl          ), priority=2002 , match=(reg0[[8]] == 1 && 
> (udp)), action=(next;)
> -])
> -AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed 
> 's/table=../table=??/'], [0], [dnl
> +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep -e 2002 -e 2001 | sort | sed 
> 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl
> +  table=??(ls_in_acl          ), priority=2001 , match=((reg0[[10]] == 1) && 
> ip), action=(ct_commit { ct_mark.blocked = 1; ct_label.label = 2345; };  reg0 
> = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ 
> outport <-> inport; next(pipeline=egress,table=5); };)
> +  table=??(ls_in_acl          ), priority=2001 , match=((reg0[[10]] == 1) && 
> ip6), action=(ct_commit { ct_mark.blocked = 1; };  reg0 = 0; reject { /* 
> eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ outport <-> inport; 
> next(pipeline=egress,table=5); };)
> +  table=??(ls_in_acl          ), priority=2001 , match=((reg0[[9]] == 1) && 
> ip), action=(ct_commit_drop { ct_label.label = 2345; };  reg0 = 0; reject { 
> /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ outport <-> 
> inport; next(pipeline=egress,table=5); };)
> +  table=??(ls_in_acl          ), priority=2001 , match=((reg0[[9]] == 1) && 
> ip6), action=(reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; 
> is implicit. */ outport <-> inport; next(pipeline=egress,table=5); };)
> +  table=??(ls_in_acl          ), priority=2002 , match=(reg0[[7]] == 1 && 
> (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;)
> +  table=??(ls_in_acl          ), priority=2002 , match=(reg0[[7]] == 1 && 
> (udp)), action=(reg0[[1]] = 1; next;)
> +  table=??(ls_in_acl          ), priority=2002 , match=(reg0[[8]] == 1 && 
> (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;)
> +  table=??(ls_in_acl          ), priority=2002 , match=(reg0[[8]] == 1 && 
> (udp)), action=(next;)
> +])
> +AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed 
> 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl
>    table=??(ls_in_stateful     ), priority=0    , match=(1), action=(next;)
>    table=??(ls_in_stateful     ), priority=100  , match=(reg0[[1]] == 1 && 
> reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;)
>    table=??(ls_in_stateful     ), priority=100  , match=(reg0[[1]] == 1 && 
> reg0[[13]] == 1), action=(ct_commit { ct_mark.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 = 1234; 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 = 1234; next;)
> -  table=4 (ls_out_acl         ), priority=2002 , match=(reg0[[8]] == 1 && 
> (udp)), action=(next;)
> +AT_CHECK([grep -w "ls_out_acl" sw0flows | grep -e 2002 -e 2001 | sort | sed 
> 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl
> +  table=??(ls_out_acl         ), priority=2001 , match=(reg0[[10]] == 1 && 
> (ip)), action=(ct_commit { ct_mark.blocked = 1; ct_label.label = 2345; }; /* 
> drop */)
> +  table=??(ls_out_acl         ), priority=2001 , match=(reg0[[10]] == 1 && 
> (ip6)), action=(ct_commit { ct_mark.blocked = 1; }; /* drop */)
> +  table=??(ls_out_acl         ), priority=2001 , match=(reg0[[9]] == 1 && 
> (ip)), action=(ct_commit_drop { ct_label.label = 2345; }; /* drop */)
> +  table=??(ls_out_acl         ), priority=2001 , match=(reg0[[9]] == 1 && 
> (ip6)), action=(/* drop */)
> +  table=??(ls_out_acl         ), priority=2002 , match=(reg0[[7]] == 1 && 
> (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;)
> +  table=??(ls_out_acl         ), priority=2002 , match=(reg0[[7]] == 1 && 
> (udp)), action=(reg0[[1]] = 1; next;)
> +  table=??(ls_out_acl         ), priority=2002 , match=(reg0[[8]] == 1 && 
> (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;)
> +  table=??(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_mark.blocked = 0; }; next;)
> -  table=7 (ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1 && 
> reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = 
> reg3; }; next;)
> +AT_CHECK([grep "ls_out_stateful" sw0flows | sort | sed 
> 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl
> +  table=??(ls_out_stateful    ), priority=0    , match=(1), action=(next;)
> +  table=??(ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1 && 
> reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;)
> +  table=??(ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1 && 
> reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = 
> reg3; }; next;)
>  ])
>
> -# Delete new ACL with label
> +AS_BOX([Delete ACLs with labels])
>  check ovn-nbctl --wait=sb acl-del sw0 to-lport 1002 tcp
> +check ovn-nbctl --wait=sb acl-del sw0 to-lport 1001 ip
>  check ovn-nbctl --wait=sb acl-del sw0 from-lport 1002 tcp
> +check ovn-nbctl --wait=sb acl-del sw0 from-lport 1001 ip
>
>  ovn-sbctl dump-flows sw0 > sw0flows
>  AT_CAPTURE_FILE([sw0flows])
>
> -AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 2002 | sort | sed 
> 's/table=./table=?/'], [0], [dnl
> -  table=? (ls_in_acl          ), priority=2002 , match=(reg0[[7]] == 1 && 
> (udp)), action=(reg0[[1]] = 1; next;)
> -  table=? (ls_in_acl          ), priority=2002 , match=(reg0[[8]] == 1 && 
> (udp)), action=(next;)
> +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep -e 2002 -e 2001 | sort | sed 
> 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl
> +  table=??(ls_in_acl          ), priority=2001 , match=((reg0[[10]] == 1) && 
> ip6), action=(ct_commit { ct_mark.blocked = 1; };  reg0 = 0; reject { /* 
> eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ outport <-> inport; 
> next(pipeline=egress,table=5); };)
> +  table=??(ls_in_acl          ), priority=2001 , match=((reg0[[9]] == 1) && 
> ip6), action=(reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; 
> is implicit. */ outport <-> inport; next(pipeline=egress,table=5); };)
> +  table=??(ls_in_acl          ), priority=2002 , match=(reg0[[7]] == 1 && 
> (udp)), action=(reg0[[1]] = 1; next;)
> +  table=??(ls_in_acl          ), priority=2002 , match=(reg0[[8]] == 1 && 
> (udp)), action=(next;)
>  ])
> -AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed 
> 's/table=../table=??/'], [0], [dnl
> +AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed 
> 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl
>    table=??(ls_in_stateful     ), priority=0    , match=(1), action=(next;)
>    table=??(ls_in_stateful     ), priority=100  , match=(reg0[[1]] == 1 && 
> reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;)
>    table=??(ls_in_stateful     ), priority=100  , match=(reg0[[1]] == 1 && 
> reg0[[13]] == 1), action=(ct_commit { ct_mark.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 -w "ls_out_acl" sw0flows | grep -e 2002 -e 2001 | sort | sed 
> 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl
> +  table=??(ls_out_acl         ), priority=2001 , match=(reg0[[10]] == 1 && 
> (ip6)), action=(ct_commit { ct_mark.blocked = 1; }; /* drop */)
> +  table=??(ls_out_acl         ), priority=2001 , match=(reg0[[9]] == 1 && 
> (ip6)), action=(/* drop */)
> +  table=??(ls_out_acl         ), priority=2002 , match=(reg0[[7]] == 1 && 
> (udp)), action=(reg0[[1]] = 1; next;)
> +  table=??(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_mark.blocked = 0; }; next;)
> -  table=7 (ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1 && 
> reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = 
> reg3; }; next;)
> +AT_CHECK([grep "ls_out_stateful" sw0flows | sort | sed 
> 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl
> +  table=??(ls_out_stateful    ), priority=0    , match=(1), action=(next;)
> +  table=??(ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1 && 
> reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;)
> +  table=??(ls_out_stateful    ), priority=100  , match=(reg0[[1]] == 1 && 
> reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = 
> reg3; }; next;)
>  ])
>  AT_CLEANUP
>  ])
> diff --git a/tests/ovn.at b/tests/ovn.at
> index e9b8bc677..fc7abfdad 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -1323,6 +1323,58 @@ ct_label = 0xcafe
>  ct_label.blocked = 1/1
>      Field ct_label.blocked is not modifiable.
>
> +# ct_commit_drop
> +ct_commit_drop;
> +    encodes as ct(commit,zone=NXM_NX_REG9[0..15])
> +    has prereqs ip
> +ct_commit_drop { };
> +    formats as ct_commit_drop;
> +    encodes as ct(commit,zone=NXM_NX_REG9[0..15])
> +    has prereqs ip
> +ct_commit_drop { ct_mark=1; };
> +    formats as ct_commit_drop { ct_mark = 1; };
> +    encodes as 
> ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0x1->ct_mark))
> +    has prereqs ip
> +ct_commit_drop { ct_mark=1/1; };
> +    formats as ct_commit_drop { ct_mark = 1/1; };
> +    encodes as 
> ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0x1/0x1->ct_mark))
> +    has prereqs ip
> +ct_commit_drop { ct_label=1; };
> +    formats as ct_commit_drop { ct_label = 1; };
> +    encodes as 
> ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0x1->ct_label))
> +    has prereqs ip
> +ct_commit_drop { ct_label=1/1; };
> +    formats as ct_commit_drop { ct_label = 1/1; };
> +    encodes as 
> ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0x1/0x1->ct_label))
> +    has prereqs ip
> +ct_commit_drop { ct_mark=1; ct_label=2; };
> +    formats as ct_commit_drop { ct_mark = 1; ct_label = 2; };
> +    encodes as 
> ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0x1->ct_mark,set_field:0x2->ct_label))
> +    has prereqs ip
> +
> +ct_commit_drop { ct_label=0x01020304050607080910111213141516; };
> +    formats as ct_commit_drop { ct_label = 
> 0x1020304050607080910111213141516; };
> +    encodes as 
> ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0x1020304050607080910111213141516->ct_label))
> +    has prereqs ip
> +ct_commit_drop { 
> ct_label=0x1000000000000000000000000000000/0x1000000000000000000000000000000; 
> };
> +    formats as ct_commit_drop { ct_label = 
> 0x1000000000000000000000000000000/0x1000000000000000000000000000000; };
> +    encodes as 
> ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0x1000000000000000000000000000000/0x1000000000000000000000000000000->ct_label))
> +    has prereqs ip
> +ct_commit_drop { ct_label=18446744073709551615; };
> +    formats as ct_commit_drop { ct_label = 18446744073709551615; };
> +    encodes as 
> ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0xffffffffffffffff->ct_label))
> +    has prereqs ip
> +ct_commit_drop { ct_label[0..47] = 0x00000f040201; ct_label[48..63] = 
> 0x0002; };
> +    formats as ct_commit_drop { ct_label[0..47] = 0xf040201; 
> ct_label[48..63] = 0x2; };
> +    encodes as 
> ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0xf040201/0xffffffffffff->ct_label,set_field:0x2000000000000/0xffff000000000000->ct_label))
> +    has prereqs ip
> +ct_commit_drop { ct_label=18446744073709551616; };
> +    Decimal constants must be less than 2**64.
> +ct_commit_drop { ct_label=0x181716151413121110090807060504030201; };
> +    141-bit constant is not compatible with 128-bit field ct_label.
> +ct_commit_drop { ip4.dst = 192.168.0.1; };
> +    Field ip4.dst is not modifiable.
> +
>  # ct_dnat
>  ct_dnat;
>      encodes as ct(table=19,zone=NXM_NX_REG11[0..15],nat)
> @@ -33637,7 +33689,7 @@ ovn-nbctl lsp-add sw0 sw0-port1 -- lsp-set-addresses 
> sw0-port1 "50:54:00:00:00:0
>  ovs-vsctl set interface p0 external-ids:iface-id=sw0-port0
>  ovs-vsctl set interface p1 external-ids:iface-id=sw0-port1
>  check ovn-nbctl --wait=hv sync
> -as hv1 ovs-ofctl dump-flows br-int  | sed 's/cookie=0x.*, duration=.*, 
> table/cookie=??, duration=??, table/' | sed 
> 's/load:0x.->NXM_NX_REG1/load:0x?->NXM_NX_REG1/g' | sed 's/idle_age=[[0-9]], 
> //g' | sort > offlows1
> +as hv1 ovs-ofctl dump-flows br-int  | sed 's/cookie=0x.*, duration=.*, 
> table/cookie=??, duration=??, table/' | sed 
> 's/load:0x.->NXM_NX_REG\([[19]]\)/load:0x?->NXM_NX_REG\1/g' | sed 
> 's/idle_age=[[0-9]], //g' | sort > offlows1
>
>  ovs-vsctl set interface p0 external-ids:iface-id=foo0
>  ovs-vsctl set interface p1 external-ids:iface-id=foo1
> @@ -33664,7 +33716,7 @@ sleep 0.5
>  # Restart SB
>  AT_CHECK([kill -CONT $(cat ovn-sb/ovsdb-server.pid)])
>  check ovn-nbctl --wait=hv sync
> -as hv1 ovs-ofctl dump-flows br-int | sed 's/cookie=0x.*, duration=.*, 
> table/cookie=??, duration=??, table/' | sed 
> 's/load:0x.->NXM_NX_REG1/load:0x?->NXM_NX_REG1/g' | sed 's/idle_age=[[0-9]], 
> //g' | sort > offlows2
> +as hv1 ovs-ofctl dump-flows br-int | sed 's/cookie=0x.*, duration=.*, 
> table/cookie=??, duration=??, table/' | sed 
> 's/load:0x.->NXM_NX_REG\([[19]]\)/load:0x?->NXM_NX_REG\1/g' | sed 
> 's/idle_age=[[0-9]], //g' | sort > offlows2
>  AT_CHECK([diff  offlows1 offlows2])
>
>  ovn-nbctl ls-del sw0
> @@ -34689,3 +34741,37 @@ check_packets
>  OVN_CLEANUP([hv1])
>  AT_CLEANUP
>  ])
> +
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([ovn-controller - check logical_switch drop ct zone])
> +ovn_start
> +net_add n1
> +sim_add hv1
> +as hv1
> +ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.1
> +
> +check ovn-nbctl ls-add ls0
> +check ovn-nbctl lsp-add ls0 lsp0
> +check ovn-nbctl lsp-set-addresses lsp0 "00:00:00:00:00:02 10.0.0.2"
> +
> +ovs-vsctl -- add-port br-int vif0 -- set interface vif0 
> external-ids:iface-id=lsp0
> +
> +# Wait for ovn-northd and ovn-controller to catch up.
> +wait_for_ports_up
> +check ovn-nbctl --wait=hv sync
> +
> +# logical_switch should have a CT zone for connections dropped by labeled 
> ACLs
> +ls_uuid=$(fetch_column Datapath_Binding _uuid external_ids:name=ls0)
> +AT_CHECK([as hv1 ovn-appctl -t ovn-controller ct-zone-list | \
> +grep ${ls_uuid}_drop -c], [0], [1
> +])
> +
> +# Check that register storing the drop ct zone is not cleared.
> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=39 | grep -w 'priority=1' 
> | ofctl_strip_all], [0], [dnl
> + table=39, priority=1,metadata=0x1 
> actions=load:0->NXM_NX_REG0[[]],load:0->NXM_NX_REG1[[]],load:0->NXM_NX_REG2[[]],load:0->NXM_NX_REG3[[]],load:0->NXM_NX_REG4[[]],load:0->NXM_NX_REG5[[]],load:0->NXM_NX_REG6[[]],load:0->NXM_NX_REG7[[]],load:0->NXM_NX_REG8[[]],resubmit(,40)
> +])
> +
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP
> +])
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index ae4d6c403..74fa04539 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -2367,13 +2367,6 @@ nbctl_acl_add(struct ctl_context *ctx)
>      /* 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;
> -      }
> -
>        int64_t label_value = 0;
>        error = parse_acl_label(label, &label_value);
>        if (error) {
> diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
> index e5766ed67..5cede3527 100644
> --- a/utilities/ovn-trace.c
> +++ b/utilities/ovn-trace.c
> @@ -3356,6 +3356,8 @@ trace_actions(const struct ovnact *ovnacts, size_t 
> ovnacts_len,
>              break;
>          case OVNACT_SAMPLE:
>              break;
> +        case OVNACT_CT_COMMIT_DROP:
> +            break;
>          }
>      }
>      ofpbuf_uninit(&stack);
> --
> 2.22.3
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to