diff mbox series

[ovs-dev,v2] actions: introduce ct_commit_continue action

Message ID 804194d4ee223edb50f094ddb8793a07035d44d9.1670370365.git.lorenzo.bianconi@redhat.com
State Changes Requested
Headers show
Series [ovs-dev,v2] actions: introduce ct_commit_continue action | expand

Checks

Context Check Description
ovsrobot/apply-robot warning apply and check: warning
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/github-robot-_ovn-kubernetes success github build: passed

Commit Message

Lorenzo Bianconi Dec. 6, 2022, 11:53 p.m. UTC
In the current codebase ct_commit {} action clears ct_state metadata of
the incoming packet. This behaviour introduces an issue if we need to
check the connection tracking state in the subsequent pipeline stages,
e.g. for hairpin traffic:

table=14(ls_in_pre_hairpin  ), priority=100  , match=(ip && ct.trk), action=(reg0[6] = chk_lb_hairpin(); reg0[12] = chk_lb_hairpin_reply(); next;)

Fix the issue introducing ct_commit_continue action used to allow the ct
packet to proceed in the pipeline instead of the original one.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2103086
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
Changes since v1:
- introduce new nested action ct_commit_continue instead of modifying
  ct_commit_v2
---
 controller/chassis.c    |  7 +++++
 include/ovn/actions.h   |  2 ++
 include/ovn/features.h  |  1 +
 lib/actions.c           | 61 ++++++++++++++++++++++++++++++++++++++---
 northd/northd.c         | 40 +++++++++++++++++++++++----
 northd/northd.h         |  2 ++
 northd/ovn-northd.8.xml |  7 +++++
 ovn-sb.xml              | 15 ++++++++++
 tests/ovn-controller.at | 42 ++++++++++++++++++++++++++++
 tests/ovn-northd.at     |  8 +++---
 tests/ovn.at            |  4 +++
 utilities/ovn-trace.c   |  2 ++
 12 files changed, 177 insertions(+), 14 deletions(-)

Comments

Mark Michelson Dec. 7, 2022, 8:17 p.m. UTC | #1
Hi Lorenzo,

I can't see anything wrong with the patch. However, I think that since 
the linked bugzilla issue mentions that hairpinning is broken, you 
should add a system test that ensures that hairpinning works as 
expected. This can ensure that we have a test on-hand and can detect if 
that scenario get broken again. You might even be able to add onto an 
existing system test instead of having to write one from scratch.

On 12/6/22 18:53, Lorenzo Bianconi wrote:
> In the current codebase ct_commit {} action clears ct_state metadata of
> the incoming packet. This behaviour introduces an issue if we need to
> check the connection tracking state in the subsequent pipeline stages,
> e.g. for hairpin traffic:
> 
> table=14(ls_in_pre_hairpin  ), priority=100  , match=(ip && ct.trk), action=(reg0[6] = chk_lb_hairpin(); reg0[12] = chk_lb_hairpin_reply(); next;)
> 
> Fix the issue introducing ct_commit_continue action used to allow the ct
> packet to proceed in the pipeline instead of the original one.
> 
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2103086
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---
> Changes since v1:
> - introduce new nested action ct_commit_continue instead of modifying
>    ct_commit_v2
> ---
>   controller/chassis.c    |  7 +++++
>   include/ovn/actions.h   |  2 ++
>   include/ovn/features.h  |  1 +
>   lib/actions.c           | 61 ++++++++++++++++++++++++++++++++++++++---
>   northd/northd.c         | 40 +++++++++++++++++++++++----
>   northd/northd.h         |  2 ++
>   northd/ovn-northd.8.xml |  7 +++++
>   ovn-sb.xml              | 15 ++++++++++
>   tests/ovn-controller.at | 42 ++++++++++++++++++++++++++++
>   tests/ovn-northd.at     |  8 +++---
>   tests/ovn.at            |  4 +++
>   utilities/ovn-trace.c   |  2 ++
>   12 files changed, 177 insertions(+), 14 deletions(-)
> 
> diff --git a/controller/chassis.c b/controller/chassis.c
> index 685d9b2ae..8dc7ecc07 100644
> --- a/controller/chassis.c
> +++ b/controller/chassis.c
> @@ -352,6 +352,7 @@ chassis_build_other_config(const struct ovs_chassis_cfg *ovs_cfg,
>       smap_replace(config, OVN_FEATURE_PORT_UP_NOTIF, "true");
>       smap_replace(config, OVN_FEATURE_CT_NO_MASKED_LABEL, "true");
>       smap_replace(config, OVN_FEATURE_MAC_BINDING_TIMESTAMP, "true");
> +    smap_replace(config, OVN_FEATURE_CT_COMMIT_CONTINUE, "true");
>   }
>   
>   /*
> @@ -469,6 +470,12 @@ chassis_other_config_changed(const struct ovs_chassis_cfg *ovs_cfg,
>           return true;
>       }
>   
> +    if (!smap_get_bool(&chassis_rec->other_config,
> +                       OVN_FEATURE_CT_COMMIT_CONTINUE,
> +                       false)) {
> +        return true;
> +    }
> +
>       return false;
>   }
>   
> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> index a56351081..927818976 100644
> --- a/include/ovn/actions.h
> +++ b/include/ovn/actions.h
> @@ -66,6 +66,7 @@ struct ovn_extend_table;
>       OVNACT(CT_NEXT,           ovnact_ct_next)         \
>       OVNACT(CT_COMMIT_V1,      ovnact_ct_commit_v1)    \
>       OVNACT(CT_COMMIT_V2,      ovnact_nest)            \
> +    OVNACT(CT_COMMIT_CONTINUE, ovnact_nest)           \
>       OVNACT(CT_DNAT,           ovnact_ct_nat)          \
>       OVNACT(CT_SNAT,           ovnact_ct_nat)          \
>       OVNACT(CT_DNAT_IN_CZONE,  ovnact_ct_nat)          \
> @@ -321,6 +322,7 @@ struct ovnact_nest {
>       struct ovnact ovnact;
>       struct ovnact *nested;
>       size_t nested_len;
> +    uint8_t ltable;             /* Logical table ID of next table. */
>   };
>   
>   /* OVNACT_GET_ARP, OVNACT_GET_ND. */
> diff --git a/include/ovn/features.h b/include/ovn/features.h
> index 679f67457..0ad8a27b9 100644
> --- a/include/ovn/features.h
> +++ b/include/ovn/features.h
> @@ -24,6 +24,7 @@
>   #define OVN_FEATURE_PORT_UP_NOTIF      "port-up-notif"
>   #define OVN_FEATURE_CT_NO_MASKED_LABEL "ct-no-masked-label"
>   #define OVN_FEATURE_MAC_BINDING_TIMESTAMP "mac-binding-timestamp"
> +#define OVN_FEATURE_CT_COMMIT_CONTINUE "ct-commit-continue"
>   
>   /* OVS datapath supported features.  Based on availability OVN might generate
>    * different types of openflows.
> diff --git a/lib/actions.c b/lib/actions.c
> index 47ec654e1..807b84127 100644
> --- a/lib/actions.c
> +++ b/lib/actions.c
> @@ -766,6 +766,13 @@ parse_CT_COMMIT(struct action_context *ctx)
>       if (ctx->lexer->token.type == LEX_T_LCURLY) {
>           parse_nested_action(ctx, OVNACT_CT_COMMIT_V2, "ip",
>                               WR_CT_COMMIT);
> +
> +        if (ctx->lexer->error) {
> +            return;
> +        }
> +
> +        struct ovnact_nest *on = ctx->ovnacts->header;
> +        on->ltable = 0;
>       } else if (ctx->lexer->token.type == LEX_T_LPAREN) {
>           parse_CT_COMMIT_V1(ctx);
>       } else {
> @@ -775,6 +782,7 @@ parse_CT_COMMIT(struct action_context *ctx)
>                                               OVNACT_ALIGN(sizeof *on));
>           on->nested_len = 0;
>           on->nested = NULL;
> +        on->ltable = 0;
>       }
>   }
>   
> @@ -871,13 +879,13 @@ format_CT_COMMIT_V2(const struct ovnact_nest *on, struct ds *s)
>   }
>   
>   static void
> -encode_CT_COMMIT_V2(const struct ovnact_nest *on,
> -                    const struct ovnact_encode_params *ep OVS_UNUSED,
> -                    struct ofpbuf *ofpacts)
> +encode_ct_commit_nested(const struct ovnact_nest *on,
> +                        const struct ovnact_encode_params *ep,
> +                        uint8_t recirc_table, 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->recirc_table = recirc_table;
>       ct->zone_src.field = ep->is_switch
>           ? mf_from_id(MFF_LOG_CT_ZONE)
>           : mf_from_id(MFF_LOG_DNAT_ZONE);
> @@ -907,6 +915,49 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on,
>       ct = ofpacts->header;
>       ofpact_finish(ofpacts, &ct->ofpact);
>   }
> +
> +static void
> +encode_CT_COMMIT_V2(const struct ovnact_nest *on,
> +                    const struct ovnact_encode_params *ep,
> +                    struct ofpbuf *ofpacts)
> +{
> +    encode_ct_commit_nested(on, ep, NX_CT_RECIRC_NONE, ofpacts);
> +}
> +
> +static void
> +parse_CT_COMMIT_CONTINUE(struct action_context *ctx)
> +{
> +    int table = ctx->pp->cur_ltable + 1;
> +    if (table >= ctx->pp->n_tables) {
> +        table = 0;
> +    }
> +    parse_nested_action(ctx, OVNACT_CT_COMMIT_CONTINUE, "ip",
> +                        WR_CT_COMMIT);
> +
> +    struct ovnact_nest *on = ctx->ovnacts->header;
> +    on->ltable = table;
> +}
> +
> +static void
> +format_CT_COMMIT_CONTINUE(const struct ovnact_nest *on, struct ds *s)
> +{
> +    if (on->nested_len) {
> +        format_nested_action(on, "ct_commit_continue", s);
> +    } else {
> +        ds_put_cstr(s, "ct_commit_continue;");
> +    }
> +}
> +
> +static void
> +encode_CT_COMMIT_CONTINUE(const struct ovnact_nest *on,
> +                          const struct ovnact_encode_params *ep,
> +                          struct ofpbuf *ofpacts)
> +{
> +    uint8_t recirc_table = first_ptable(ep, ep->pipeline) + on->ltable;
> +
> +    encode_ct_commit_nested(on, ep, recirc_table, ofpacts);
> +}
> +
>   
>   static void
>   parse_ct_nat(struct action_context *ctx, const char *name,
> @@ -5288,6 +5339,8 @@ parse_action(struct action_context *ctx)
>           parse_DEC_TTL(ctx);
>       } else if (lexer_match_id(ctx->lexer, "ct_next")) {
>           parse_CT_NEXT(ctx);
> +    } else if (lexer_match_id(ctx->lexer, "ct_commit_continue")) {
> +        parse_CT_COMMIT_CONTINUE(ctx);
>       } else if (lexer_match_id(ctx->lexer, "ct_commit")) {
>           parse_CT_COMMIT(ctx);
>       } else if (lexer_match_id(ctx->lexer, "ct_dnat")) {
> diff --git a/northd/northd.c b/northd/northd.c
> index 74facce7a..5170e20e2 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -446,6 +446,14 @@ build_chassis_features(const struct northd_input *input_data,
>               chassis_features->mac_binding_timestamp) {
>               chassis_features->mac_binding_timestamp = false;
>           }
> +
> +        bool ct_commit_continue =
> +            smap_get_bool(&chassis->other_config,
> +                          OVN_FEATURE_CT_COMMIT_CONTINUE,
> +                          false);
> +        if (!ct_commit_continue && chassis_features->ct_commit_continue) {
> +            chassis_features->ct_commit_continue = false;
> +        }
>       }
>   }
>   
> @@ -5494,6 +5502,7 @@ ls_get_acl_flags(struct ovn_datapath *od)
>   {
>       od->has_acls = false;
>       od->has_stateful_acl = false;
> +    od->has_apply_after_lb_acls = false;
>   
>       if (od->nbs->n_acls) {
>           od->has_acls = true;
> @@ -5502,7 +5511,9 @@ ls_get_acl_flags(struct ovn_datapath *od)
>               struct nbrec_acl *acl = od->nbs->acls[i];
>               if (!strcmp(acl->action, "allow-related")) {
>                   od->has_stateful_acl = true;
> -                return;
> +            }
> +            if (smap_get_bool(&acl->options, "apply-after-lb", false)) {
> +                od->has_apply_after_lb_acls = true;
>               }
>           }
>       }
> @@ -5516,7 +5527,9 @@ ls_get_acl_flags(struct ovn_datapath *od)
>                   struct nbrec_acl *acl = ls_pg->nb_pg->acls[i];
>                   if (!strcmp(acl->action, "allow-related")) {
>                       od->has_stateful_acl = true;
> -                    return;
> +                }
> +                if (smap_get_bool(&acl->options, "apply-after-lb", false)) {
> +                    od->has_apply_after_lb_acls = true;
>                   }
>               }
>           }
> @@ -7447,9 +7460,17 @@ build_stateful(struct ovn_datapath *od,
>        * 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. */
> -    ds_put_format(&actions, "ct_commit { %s = 0; "
> -                            "ct_label.label = " REG_LABEL "; }; next;",
> -                  ct_block_action);
> +    if (features->ct_commit_continue && od->has_apply_after_lb_acls) {
> +        ds_put_format(&actions,
> +                      "ct_commit_continue { %s = 0; "
> +                      "ct_label.label = " REG_LABEL "; };",
> +                      ct_block_action);
> +    } else {
> +        ds_put_format(&actions,
> +                      "ct_commit { %s = 0; "
> +                      "ct_label.label = " REG_LABEL "; }; next;",
> +                      ct_block_action);
> +    }
>       ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 100,
>                     REGBIT_CONNTRACK_COMMIT" == 1 && "
>                     REGBIT_ACL_LABEL" == 1",
> @@ -7464,7 +7485,13 @@ build_stateful(struct ovn_datapath *od,
>        * any packet that makes it this far is part of a connection we
>        * want to allow to continue. */
>       ds_clear(&actions);
> -    ds_put_format(&actions, "ct_commit { %s = 0; }; next;", ct_block_action);
> +    if (features->ct_commit_continue && od->has_apply_after_lb_acls) {
> +        ds_put_format(&actions, "ct_commit_continue { %s = 0; };",
> +                      ct_block_action);
> +    } else {
> +        ds_put_format(&actions, "ct_commit { %s = 0; }; next;",
> +                      ct_block_action);
> +    }
>       ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 100,
>                     REGBIT_CONNTRACK_COMMIT" == 1 && "
>                     REGBIT_ACL_LABEL" == 0",
> @@ -15875,6 +15902,7 @@ northd_init(struct northd_data *data)
>       data->features = (struct chassis_features) {
>           .ct_no_masked_label = true,
>           .mac_binding_timestamp = true,
> +        .ct_commit_continue = true,
>       };
>       data->ovn_internal_version_changed = false;
>   }
> diff --git a/northd/northd.h b/northd/northd.h
> index 7942c0a34..fee68d1e7 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -69,6 +69,7 @@ struct northd_input {
>   struct chassis_features {
>       bool ct_no_masked_label;
>       bool mac_binding_timestamp;
> +    bool ct_commit_continue;
>   };
>   
>   struct northd_data {
> @@ -211,6 +212,7 @@ struct ovn_datapath {
>       bool has_unknown;
>       bool has_acls;
>       bool has_vtep_lports;
> +    bool has_apply_after_lb_acls;
>   
>       /* IPAM data. */
>       struct ipam_info ipam_info;
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index dffbba96d..6a6425dd4 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -1108,6 +1108,13 @@
>           action based on a hint provided by the previous tables (with a match
>           for <code>reg0[1] == 1 &amp;&amp; reg0[13] == 0</code>).
>         </li>
> +
> +      <li>
> +        If the ACL is configured with <code>apply-after-lb</code> option,
> +        <code>ct_commit_continue</code> action will be used instead of
> +        <code>ct_commit</code> in order to preserve ct_state metadata.
> +      </li>
> +
>         <li>
>           A priority-0 flow that simply moves traffic to the next table.
>         </li>
> diff --git a/ovn-sb.xml b/ovn-sb.xml
> index 4f485b860..6f759b428 100644
> --- a/ovn-sb.xml
> +++ b/ovn-sb.xml
> @@ -1408,6 +1408,21 @@
>             </p>
>           </dd>
>   
> +        <dt><code>ct_commit_continue { };</code></dt>
> +        <dt><code>ct_commit_continue { ct_mark=<var>value[/mask]</var>; };</code></dt>
> +        <dt><code>ct_commit_continue { ct_label=<var>value[/mask]</var>; };</code></dt>
> +        <dt><code>ct_commit_continue { ct_mark=<var>value[/mask]</var>; ct_label=<var>value[/mask]</var>; };</code></dt>
> +
> +        <dd>
> +          <p>
> +            <code>ct_commit_continue</code> action exports the same features
> +            supported by <code>ct_commit</code> but allow the packet committed
> +            to the ct table to continue the processing in the next pipeline
> +            stage. This is useful to maintain ct metadata of the processed
> +            packet.
> +          </p>
> +        </dd>
> +
>           <dt><code>ct_dnat;</code></dt>
>           <dt><code>ct_dnat(<var>IP</var>);</code></dt>
>           <dd>
> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> index 6bc9ba75d..67c74f9cd 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -2499,3 +2499,45 @@ AT_CHECK([GET_LOCAL_TEMPLATE_VARS], [1], [])
>   
>   AT_CLEANUP
>   ])
> +
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([ovn-controller - ct_commit_continue])
> +AT_KEYWORDS([ct_commit_continue])
> +
> +ovn_start
> +
> +net_add n1
> +sim_add hv1
> +ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.1
> +
> +check ovn-nbctl ls-add sw0 \
> +    -- lsp-add sw0 sw0-p0  \
> +    -- lsp-set-addresses sw0-p0 "00:00:00:00:00:01 192.168.1.1"
> +
> +as hv1
> +ovs-vsctl \
> +    -- add-port br-int vif0 \
> +    -- set Interface vif0 external_ids:iface-id=sw0-p0
> +
> +check ovn-nbctl pg-add pg0 sw0-p0
> +check ovn-nbctl --apply-after-lb acl-add pg0 from-lport 1004 "ip4 && ip4.dst == 192.168.1.2" drop
> +check ovn-nbctl --apply-after-lb acl-add pg0 from-lport 1002 "ip4 && tcp" allow-related
> +check ovn-nbctl --apply-after-lb acl-add pg0 from-lport 1003 "ip4 && icmp" allow-related
> +check ovn-nbctl --apply-after-lb acl-add pg0 from-lport 1001 "ip4" drop
> +
> +check ovn-nbctl lb-add lb0 192.168.1.10 192.168.1.2
> +check ovn-nbctl ls-lb-add sw0 lb0
> +
> +check ovn-nbctl --wait=hv sync
> +wait_for_ports_up
> +
> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=23 | grep table=24 | sed -e 's/cookie=0x[[a-z,0-9]]*/cookie=0x0/; s/duration=[[0-9]]*.[[0-9]]*s/duration=<cleared>/' |sort], [0], [dnl
> + cookie=0x0 duration=<cleared>, table=23, n_packets=0, n_bytes=0, idle_age=0, priority=100,ip,reg0=0x2/0x2002,metadata=0x1 actions=ct(commit,table=24,zone=NXM_NX_REG13[[0..15]],nat(src),exec(load:0->NXM_NX_CT_MARK[[0]]))
> + cookie=0x0 duration=<cleared>, table=23, n_packets=0, n_bytes=0, idle_age=0, priority=100,ip,reg0=0x2002/0x2002,metadata=0x1 actions=ct(commit,table=24,zone=NXM_NX_REG13[[0..15]],nat(src),exec(load:0->NXM_NX_CT_MARK[[0]],move:NXM_NX_XXREG0[[0..31]]->NXM_NX_CT_LABEL[[96..127]]))
> + cookie=0x0 duration=<cleared>, table=23, n_packets=0, n_bytes=0, idle_age=0, priority=100,ipv6,reg0=0x2/0x2002,metadata=0x1 actions=ct(commit,table=24,zone=NXM_NX_REG13[[0..15]],nat(src),exec(load:0->NXM_NX_CT_MARK[[0]]))
> + cookie=0x0 duration=<cleared>, table=23, n_packets=0, n_bytes=0, idle_age=0, priority=100,ipv6,reg0=0x2002/0x2002,metadata=0x1 actions=ct(commit,table=24,zone=NXM_NX_REG13[[0..15]],nat(src),exec(load:0->NXM_NX_CT_MARK[[0]],move:NXM_NX_XXREG0[[0..31]]->NXM_NX_CT_LABEL[[96..127]]))
> +])
> +
> +AT_CLEANUP
> +])
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 9a76ca340..7eb965ce8 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -6623,8 +6623,8 @@ AT_CHECK([grep -e "ls_in_lb " lsflows | sed 's/table=../table=??/' | sort], [0],
>   
>   AT_CHECK([grep -e "ls_in_stateful" lsflows | sed 's/table=../table=??/' | sort], [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;)
> +  table=??(ls_in_stateful     ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit_continue { ct_mark.blocked = 0; };)
> +  table=??(ls_in_stateful     ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit_continue { ct_mark.blocked = 0; ct_label.label = reg3; };)
>   ])
>   
>   AS_BOX([Remove and add the ACLs back with a few ACLs with apply-after-lb option])
> @@ -6676,8 +6676,8 @@ AT_CHECK([grep -e "ls_in_lb " lsflows | sed 's/table=../table=??/' | sort], [0],
>   
>   AT_CHECK([grep -e "ls_in_stateful" lsflows | sed 's/table=../table=??/' | sort], [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;)
> +  table=??(ls_in_stateful     ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit_continue { ct_mark.blocked = 0; };)
> +  table=??(ls_in_stateful     ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit_continue { ct_mark.blocked = 0; ct_label.label = reg3; };)
>   ])
>   
>   AT_CLEANUP
> diff --git a/tests/ovn.at b/tests/ovn.at
> index f3bd53242..ed4a2f50d 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -1187,6 +1187,10 @@ ct_commit { ct_mark=1; };
>       formats as ct_commit { ct_mark = 1; };
>       encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1->ct_mark))
>       has prereqs ip
> +ct_commit_continue { ct_mark=1; };
> +    formats as ct_commit_continue { ct_mark = 1; };
> +    encodes as ct(commit,table=19,zone=NXM_NX_REG13[0..15],exec(set_field:0x1->ct_mark))
> +    has prereqs ip
>   ct_commit { ct_mark=1/1; };
>       formats as ct_commit { ct_mark = 1/1; };
>       encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1/0x1->ct_mark))
> diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
> index 79ed5a9af..e1def9eea 100644
> --- a/utilities/ovn-trace.c
> +++ b/utilities/ovn-trace.c
> @@ -3098,6 +3098,8 @@ trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len,
>           case OVNACT_CT_COMMIT_V2:
>               /* Nothing to do. */
>               break;
> +        case OVNACT_CT_COMMIT_CONTINUE:
> +            break;
>   
>           case OVNACT_CT_DNAT:
>               execute_ct_nat(ovnact_get_CT_DNAT(a), dp, uflow, pipeline, super);
diff mbox series

Patch

diff --git a/controller/chassis.c b/controller/chassis.c
index 685d9b2ae..8dc7ecc07 100644
--- a/controller/chassis.c
+++ b/controller/chassis.c
@@ -352,6 +352,7 @@  chassis_build_other_config(const struct ovs_chassis_cfg *ovs_cfg,
     smap_replace(config, OVN_FEATURE_PORT_UP_NOTIF, "true");
     smap_replace(config, OVN_FEATURE_CT_NO_MASKED_LABEL, "true");
     smap_replace(config, OVN_FEATURE_MAC_BINDING_TIMESTAMP, "true");
+    smap_replace(config, OVN_FEATURE_CT_COMMIT_CONTINUE, "true");
 }
 
 /*
@@ -469,6 +470,12 @@  chassis_other_config_changed(const struct ovs_chassis_cfg *ovs_cfg,
         return true;
     }
 
+    if (!smap_get_bool(&chassis_rec->other_config,
+                       OVN_FEATURE_CT_COMMIT_CONTINUE,
+                       false)) {
+        return true;
+    }
+
     return false;
 }
 
diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index a56351081..927818976 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -66,6 +66,7 @@  struct ovn_extend_table;
     OVNACT(CT_NEXT,           ovnact_ct_next)         \
     OVNACT(CT_COMMIT_V1,      ovnact_ct_commit_v1)    \
     OVNACT(CT_COMMIT_V2,      ovnact_nest)            \
+    OVNACT(CT_COMMIT_CONTINUE, ovnact_nest)           \
     OVNACT(CT_DNAT,           ovnact_ct_nat)          \
     OVNACT(CT_SNAT,           ovnact_ct_nat)          \
     OVNACT(CT_DNAT_IN_CZONE,  ovnact_ct_nat)          \
@@ -321,6 +322,7 @@  struct ovnact_nest {
     struct ovnact ovnact;
     struct ovnact *nested;
     size_t nested_len;
+    uint8_t ltable;             /* Logical table ID of next table. */
 };
 
 /* OVNACT_GET_ARP, OVNACT_GET_ND. */
diff --git a/include/ovn/features.h b/include/ovn/features.h
index 679f67457..0ad8a27b9 100644
--- a/include/ovn/features.h
+++ b/include/ovn/features.h
@@ -24,6 +24,7 @@ 
 #define OVN_FEATURE_PORT_UP_NOTIF      "port-up-notif"
 #define OVN_FEATURE_CT_NO_MASKED_LABEL "ct-no-masked-label"
 #define OVN_FEATURE_MAC_BINDING_TIMESTAMP "mac-binding-timestamp"
+#define OVN_FEATURE_CT_COMMIT_CONTINUE "ct-commit-continue"
 
 /* OVS datapath supported features.  Based on availability OVN might generate
  * different types of openflows.
diff --git a/lib/actions.c b/lib/actions.c
index 47ec654e1..807b84127 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -766,6 +766,13 @@  parse_CT_COMMIT(struct action_context *ctx)
     if (ctx->lexer->token.type == LEX_T_LCURLY) {
         parse_nested_action(ctx, OVNACT_CT_COMMIT_V2, "ip",
                             WR_CT_COMMIT);
+
+        if (ctx->lexer->error) {
+            return;
+        }
+
+        struct ovnact_nest *on = ctx->ovnacts->header;
+        on->ltable = 0;
     } else if (ctx->lexer->token.type == LEX_T_LPAREN) {
         parse_CT_COMMIT_V1(ctx);
     } else {
@@ -775,6 +782,7 @@  parse_CT_COMMIT(struct action_context *ctx)
                                             OVNACT_ALIGN(sizeof *on));
         on->nested_len = 0;
         on->nested = NULL;
+        on->ltable = 0;
     }
 }
 
@@ -871,13 +879,13 @@  format_CT_COMMIT_V2(const struct ovnact_nest *on, struct ds *s)
 }
 
 static void
-encode_CT_COMMIT_V2(const struct ovnact_nest *on,
-                    const struct ovnact_encode_params *ep OVS_UNUSED,
-                    struct ofpbuf *ofpacts)
+encode_ct_commit_nested(const struct ovnact_nest *on,
+                        const struct ovnact_encode_params *ep,
+                        uint8_t recirc_table, 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->recirc_table = recirc_table;
     ct->zone_src.field = ep->is_switch
         ? mf_from_id(MFF_LOG_CT_ZONE)
         : mf_from_id(MFF_LOG_DNAT_ZONE);
@@ -907,6 +915,49 @@  encode_CT_COMMIT_V2(const struct ovnact_nest *on,
     ct = ofpacts->header;
     ofpact_finish(ofpacts, &ct->ofpact);
 }
+
+static void
+encode_CT_COMMIT_V2(const struct ovnact_nest *on,
+                    const struct ovnact_encode_params *ep,
+                    struct ofpbuf *ofpacts)
+{
+    encode_ct_commit_nested(on, ep, NX_CT_RECIRC_NONE, ofpacts);
+}
+
+static void
+parse_CT_COMMIT_CONTINUE(struct action_context *ctx)
+{
+    int table = ctx->pp->cur_ltable + 1;
+    if (table >= ctx->pp->n_tables) {
+        table = 0;
+    }
+    parse_nested_action(ctx, OVNACT_CT_COMMIT_CONTINUE, "ip",
+                        WR_CT_COMMIT);
+
+    struct ovnact_nest *on = ctx->ovnacts->header;
+    on->ltable = table;
+}
+
+static void
+format_CT_COMMIT_CONTINUE(const struct ovnact_nest *on, struct ds *s)
+{
+    if (on->nested_len) {
+        format_nested_action(on, "ct_commit_continue", s);
+    } else {
+        ds_put_cstr(s, "ct_commit_continue;");
+    }
+}
+
+static void
+encode_CT_COMMIT_CONTINUE(const struct ovnact_nest *on,
+                          const struct ovnact_encode_params *ep,
+                          struct ofpbuf *ofpacts)
+{
+    uint8_t recirc_table = first_ptable(ep, ep->pipeline) + on->ltable;
+
+    encode_ct_commit_nested(on, ep, recirc_table, ofpacts);
+}
+
 
 static void
 parse_ct_nat(struct action_context *ctx, const char *name,
@@ -5288,6 +5339,8 @@  parse_action(struct action_context *ctx)
         parse_DEC_TTL(ctx);
     } else if (lexer_match_id(ctx->lexer, "ct_next")) {
         parse_CT_NEXT(ctx);
+    } else if (lexer_match_id(ctx->lexer, "ct_commit_continue")) {
+        parse_CT_COMMIT_CONTINUE(ctx);
     } else if (lexer_match_id(ctx->lexer, "ct_commit")) {
         parse_CT_COMMIT(ctx);
     } else if (lexer_match_id(ctx->lexer, "ct_dnat")) {
diff --git a/northd/northd.c b/northd/northd.c
index 74facce7a..5170e20e2 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -446,6 +446,14 @@  build_chassis_features(const struct northd_input *input_data,
             chassis_features->mac_binding_timestamp) {
             chassis_features->mac_binding_timestamp = false;
         }
+
+        bool ct_commit_continue =
+            smap_get_bool(&chassis->other_config,
+                          OVN_FEATURE_CT_COMMIT_CONTINUE,
+                          false);
+        if (!ct_commit_continue && chassis_features->ct_commit_continue) {
+            chassis_features->ct_commit_continue = false;
+        }
     }
 }
 
@@ -5494,6 +5502,7 @@  ls_get_acl_flags(struct ovn_datapath *od)
 {
     od->has_acls = false;
     od->has_stateful_acl = false;
+    od->has_apply_after_lb_acls = false;
 
     if (od->nbs->n_acls) {
         od->has_acls = true;
@@ -5502,7 +5511,9 @@  ls_get_acl_flags(struct ovn_datapath *od)
             struct nbrec_acl *acl = od->nbs->acls[i];
             if (!strcmp(acl->action, "allow-related")) {
                 od->has_stateful_acl = true;
-                return;
+            }
+            if (smap_get_bool(&acl->options, "apply-after-lb", false)) {
+                od->has_apply_after_lb_acls = true;
             }
         }
     }
@@ -5516,7 +5527,9 @@  ls_get_acl_flags(struct ovn_datapath *od)
                 struct nbrec_acl *acl = ls_pg->nb_pg->acls[i];
                 if (!strcmp(acl->action, "allow-related")) {
                     od->has_stateful_acl = true;
-                    return;
+                }
+                if (smap_get_bool(&acl->options, "apply-after-lb", false)) {
+                    od->has_apply_after_lb_acls = true;
                 }
             }
         }
@@ -7447,9 +7460,17 @@  build_stateful(struct ovn_datapath *od,
      * 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. */
-    ds_put_format(&actions, "ct_commit { %s = 0; "
-                            "ct_label.label = " REG_LABEL "; }; next;",
-                  ct_block_action);
+    if (features->ct_commit_continue && od->has_apply_after_lb_acls) {
+        ds_put_format(&actions,
+                      "ct_commit_continue { %s = 0; "
+                      "ct_label.label = " REG_LABEL "; };",
+                      ct_block_action);
+    } else {
+        ds_put_format(&actions,
+                      "ct_commit { %s = 0; "
+                      "ct_label.label = " REG_LABEL "; }; next;",
+                      ct_block_action);
+    }
     ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 100,
                   REGBIT_CONNTRACK_COMMIT" == 1 && "
                   REGBIT_ACL_LABEL" == 1",
@@ -7464,7 +7485,13 @@  build_stateful(struct ovn_datapath *od,
      * any packet that makes it this far is part of a connection we
      * want to allow to continue. */
     ds_clear(&actions);
-    ds_put_format(&actions, "ct_commit { %s = 0; }; next;", ct_block_action);
+    if (features->ct_commit_continue && od->has_apply_after_lb_acls) {
+        ds_put_format(&actions, "ct_commit_continue { %s = 0; };",
+                      ct_block_action);
+    } else {
+        ds_put_format(&actions, "ct_commit { %s = 0; }; next;",
+                      ct_block_action);
+    }
     ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 100,
                   REGBIT_CONNTRACK_COMMIT" == 1 && "
                   REGBIT_ACL_LABEL" == 0",
@@ -15875,6 +15902,7 @@  northd_init(struct northd_data *data)
     data->features = (struct chassis_features) {
         .ct_no_masked_label = true,
         .mac_binding_timestamp = true,
+        .ct_commit_continue = true,
     };
     data->ovn_internal_version_changed = false;
 }
diff --git a/northd/northd.h b/northd/northd.h
index 7942c0a34..fee68d1e7 100644
--- a/northd/northd.h
+++ b/northd/northd.h
@@ -69,6 +69,7 @@  struct northd_input {
 struct chassis_features {
     bool ct_no_masked_label;
     bool mac_binding_timestamp;
+    bool ct_commit_continue;
 };
 
 struct northd_data {
@@ -211,6 +212,7 @@  struct ovn_datapath {
     bool has_unknown;
     bool has_acls;
     bool has_vtep_lports;
+    bool has_apply_after_lb_acls;
 
     /* IPAM data. */
     struct ipam_info ipam_info;
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index dffbba96d..6a6425dd4 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -1108,6 +1108,13 @@ 
         action based on a hint provided by the previous tables (with a match
         for <code>reg0[1] == 1 &amp;&amp; reg0[13] == 0</code>).
       </li>
+
+      <li>
+        If the ACL is configured with <code>apply-after-lb</code> option,
+        <code>ct_commit_continue</code> action will be used instead of
+        <code>ct_commit</code> in order to preserve ct_state metadata.
+      </li>
+
       <li>
         A priority-0 flow that simply moves traffic to the next table.
       </li>
diff --git a/ovn-sb.xml b/ovn-sb.xml
index 4f485b860..6f759b428 100644
--- a/ovn-sb.xml
+++ b/ovn-sb.xml
@@ -1408,6 +1408,21 @@ 
           </p>
         </dd>
 
+        <dt><code>ct_commit_continue { };</code></dt>
+        <dt><code>ct_commit_continue { ct_mark=<var>value[/mask]</var>; };</code></dt>
+        <dt><code>ct_commit_continue { ct_label=<var>value[/mask]</var>; };</code></dt>
+        <dt><code>ct_commit_continue { ct_mark=<var>value[/mask]</var>; ct_label=<var>value[/mask]</var>; };</code></dt>
+
+        <dd>
+          <p>
+            <code>ct_commit_continue</code> action exports the same features
+            supported by <code>ct_commit</code> but allow the packet committed
+            to the ct table to continue the processing in the next pipeline
+            stage. This is useful to maintain ct metadata of the processed
+            packet.
+          </p>
+        </dd>
+
         <dt><code>ct_dnat;</code></dt>
         <dt><code>ct_dnat(<var>IP</var>);</code></dt>
         <dd>
diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
index 6bc9ba75d..67c74f9cd 100644
--- a/tests/ovn-controller.at
+++ b/tests/ovn-controller.at
@@ -2499,3 +2499,45 @@  AT_CHECK([GET_LOCAL_TEMPLATE_VARS], [1], [])
 
 AT_CLEANUP
 ])
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([ovn-controller - ct_commit_continue])
+AT_KEYWORDS([ct_commit_continue])
+
+ovn_start
+
+net_add n1
+sim_add hv1
+ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.1
+
+check ovn-nbctl ls-add sw0 \
+    -- lsp-add sw0 sw0-p0  \
+    -- lsp-set-addresses sw0-p0 "00:00:00:00:00:01 192.168.1.1"
+
+as hv1
+ovs-vsctl \
+    -- add-port br-int vif0 \
+    -- set Interface vif0 external_ids:iface-id=sw0-p0
+
+check ovn-nbctl pg-add pg0 sw0-p0
+check ovn-nbctl --apply-after-lb acl-add pg0 from-lport 1004 "ip4 && ip4.dst == 192.168.1.2" drop
+check ovn-nbctl --apply-after-lb acl-add pg0 from-lport 1002 "ip4 && tcp" allow-related
+check ovn-nbctl --apply-after-lb acl-add pg0 from-lport 1003 "ip4 && icmp" allow-related
+check ovn-nbctl --apply-after-lb acl-add pg0 from-lport 1001 "ip4" drop
+
+check ovn-nbctl lb-add lb0 192.168.1.10 192.168.1.2
+check ovn-nbctl ls-lb-add sw0 lb0
+
+check ovn-nbctl --wait=hv sync
+wait_for_ports_up
+
+AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=23 | grep table=24 | sed -e 's/cookie=0x[[a-z,0-9]]*/cookie=0x0/; s/duration=[[0-9]]*.[[0-9]]*s/duration=<cleared>/' |sort], [0], [dnl
+ cookie=0x0 duration=<cleared>, table=23, n_packets=0, n_bytes=0, idle_age=0, priority=100,ip,reg0=0x2/0x2002,metadata=0x1 actions=ct(commit,table=24,zone=NXM_NX_REG13[[0..15]],nat(src),exec(load:0->NXM_NX_CT_MARK[[0]]))
+ cookie=0x0 duration=<cleared>, table=23, n_packets=0, n_bytes=0, idle_age=0, priority=100,ip,reg0=0x2002/0x2002,metadata=0x1 actions=ct(commit,table=24,zone=NXM_NX_REG13[[0..15]],nat(src),exec(load:0->NXM_NX_CT_MARK[[0]],move:NXM_NX_XXREG0[[0..31]]->NXM_NX_CT_LABEL[[96..127]]))
+ cookie=0x0 duration=<cleared>, table=23, n_packets=0, n_bytes=0, idle_age=0, priority=100,ipv6,reg0=0x2/0x2002,metadata=0x1 actions=ct(commit,table=24,zone=NXM_NX_REG13[[0..15]],nat(src),exec(load:0->NXM_NX_CT_MARK[[0]]))
+ cookie=0x0 duration=<cleared>, table=23, n_packets=0, n_bytes=0, idle_age=0, priority=100,ipv6,reg0=0x2002/0x2002,metadata=0x1 actions=ct(commit,table=24,zone=NXM_NX_REG13[[0..15]],nat(src),exec(load:0->NXM_NX_CT_MARK[[0]],move:NXM_NX_XXREG0[[0..31]]->NXM_NX_CT_LABEL[[96..127]]))
+])
+
+AT_CLEANUP
+])
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 9a76ca340..7eb965ce8 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -6623,8 +6623,8 @@  AT_CHECK([grep -e "ls_in_lb " lsflows | sed 's/table=../table=??/' | sort], [0],
 
 AT_CHECK([grep -e "ls_in_stateful" lsflows | sed 's/table=../table=??/' | sort], [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;)
+  table=??(ls_in_stateful     ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit_continue { ct_mark.blocked = 0; };)
+  table=??(ls_in_stateful     ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit_continue { ct_mark.blocked = 0; ct_label.label = reg3; };)
 ])
 
 AS_BOX([Remove and add the ACLs back with a few ACLs with apply-after-lb option])
@@ -6676,8 +6676,8 @@  AT_CHECK([grep -e "ls_in_lb " lsflows | sed 's/table=../table=??/' | sort], [0],
 
 AT_CHECK([grep -e "ls_in_stateful" lsflows | sed 's/table=../table=??/' | sort], [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;)
+  table=??(ls_in_stateful     ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit_continue { ct_mark.blocked = 0; };)
+  table=??(ls_in_stateful     ), priority=100  , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit_continue { ct_mark.blocked = 0; ct_label.label = reg3; };)
 ])
 
 AT_CLEANUP
diff --git a/tests/ovn.at b/tests/ovn.at
index f3bd53242..ed4a2f50d 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -1187,6 +1187,10 @@  ct_commit { ct_mark=1; };
     formats as ct_commit { ct_mark = 1; };
     encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1->ct_mark))
     has prereqs ip
+ct_commit_continue { ct_mark=1; };
+    formats as ct_commit_continue { ct_mark = 1; };
+    encodes as ct(commit,table=19,zone=NXM_NX_REG13[0..15],exec(set_field:0x1->ct_mark))
+    has prereqs ip
 ct_commit { ct_mark=1/1; };
     formats as ct_commit { ct_mark = 1/1; };
     encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1/0x1->ct_mark))
diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
index 79ed5a9af..e1def9eea 100644
--- a/utilities/ovn-trace.c
+++ b/utilities/ovn-trace.c
@@ -3098,6 +3098,8 @@  trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len,
         case OVNACT_CT_COMMIT_V2:
             /* Nothing to do. */
             break;
+        case OVNACT_CT_COMMIT_CONTINUE:
+            break;
 
         case OVNACT_CT_DNAT:
             execute_ct_nat(ovnact_get_CT_DNAT(a), dp, uflow, pipeline, super);