| Message ID | f8f3be89a8a63c728eefc7988a5fbc861ad7d32a.1666730286.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] actions: introduce next_table option for CT_COMMIT_V2 | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On Tue, Oct 25, 2022 at 4:39 PM Lorenzo Bianconi <lorenzo.bianconi@redhat.com> 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 next_table option in the ct_commit {} action > allowing the ct packet to proceed in the pipeline. > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2103086 > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> Hi Lorenzo, Thanks for the patch. documentation is missing in ovn-northd.8.xml for the updated logical flows. Please see below for few comments. > --- > include/ovn/actions.h | 1 + > lib/actions.c | 50 ++++++++++++++++++----- > northd/northd.c | 8 ++-- > ovn-sb.xml | 4 +- > tests/ovn-northd.at | 94 +++++++++++++++---------------------------- > tests/ovn.at | 4 ++ > 6 files changed, 86 insertions(+), 75 deletions(-) > > diff --git a/include/ovn/actions.h b/include/ovn/actions.h > index d7ee84dac..d96d34841 100644 > --- a/include/ovn/actions.h > +++ b/include/ovn/actions.h > @@ -315,6 +315,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/lib/actions.c b/lib/actions.c > index adbb42db4..88d7eb571 100644 > --- a/lib/actions.c > +++ b/lib/actions.c > @@ -207,6 +207,10 @@ struct action_context { > > static void parse_actions(struct action_context *, enum lex_type sentinel); > > +static void __parse_nested_action(struct action_context *ctx, > + enum ovnact_type type, > + const char *prereq, > + enum expr_write_scope scope); > static void parse_nested_action(struct action_context *ctx, > enum ovnact_type type, > const char *prereq, > @@ -764,8 +768,23 @@ static void > 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); > + int table = 0; > + lexer_force_match(ctx->lexer, LEX_T_LCURLY); /* Skip '{'. */ > + if (lexer_match_id(ctx->lexer, "next_table")) { > + lexer_match(ctx->lexer, LEX_T_SEMICOLON); > + table = ctx->pp->cur_ltable + 1; > + if (table >= ctx->pp->n_tables) { > + table = 0; > + } > + } Generally actions inside {} are nested actions. But this patch adds a custom flag just before the start of the nested actions. Also this would break the upgrades if ovn-northd is updated first. Because the old ovn-controller will fail parsing this action. @Mark Michelson @Dumitru Ceara @Han Zhou Is it ok for us to not support this upgrade scenario ? i.e ovn-northd and DBs upgraded first before the ovn-controllers ? > + __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 = table; > } else if (ctx->lexer->token.type == LEX_T_LPAREN) { > parse_CT_COMMIT_V1(ctx); > } else { > @@ -775,6 +794,7 @@ parse_CT_COMMIT(struct action_context *ctx) > OVNACT_ALIGN(sizeof *on)); > on->nested_len = 0; > on->nested = NULL; > + on->ltable = 0; > } > } > > @@ -872,12 +892,16 @@ 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, > + const struct ovnact_encode_params *ep, > struct ofpbuf *ofpacts) > { > struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); > ct->flags = NX_CT_F_COMMIT; > - ct->recirc_table = NX_CT_RECIRC_NONE; > + if (on->ltable > 0) { > + ct->recirc_table = first_ptable(ep, ep->pipeline) + on->ltable; > + } else { > + ct->recirc_table = NX_CT_RECIRC_NONE; > + } > ct->zone_src.field = ep->is_switch > ? mf_from_id(MFF_LOG_CT_ZONE) > : mf_from_id(MFF_LOG_DNAT_ZONE); > @@ -1586,13 +1610,9 @@ encode_CT_CLEAR(const struct ovnact_null *null OVS_UNUSED, > /* Implements the "arp", "nd_na", and "clone" actions, which execute nested > * actions on a packet derived from the one being processed. */ > static void > -parse_nested_action(struct action_context *ctx, enum ovnact_type type, > - const char *prereq, enum expr_write_scope scope) > +__parse_nested_action(struct action_context *ctx, enum ovnact_type type, > + const char *prereq, enum expr_write_scope scope) > { > - if (!lexer_force_match(ctx->lexer, LEX_T_LCURLY)) { > - return; > - } > - > if (ctx->depth + 1 == MAX_NESTED_ACTION_DEPTH) { > lexer_error(ctx->lexer, "maximum depth of nested actions reached"); > return; > @@ -1635,6 +1655,16 @@ parse_nested_action(struct action_context *ctx, enum ovnact_type type, > on->nested = ofpbuf_steal_data(&nested); > } > > +static void > +parse_nested_action(struct action_context *ctx, enum ovnact_type type, > + const char *prereq, enum expr_write_scope scope) > +{ > + if (!lexer_force_match(ctx->lexer, LEX_T_LCURLY)) { > + return; > + } > + __parse_nested_action(ctx, type, prereq, scope); > +} > + > static void > parse_ARP(struct action_context *ctx) > { > diff --git a/northd/northd.c b/northd/northd.c > index 6771ccce5..7b31466ac 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -7048,8 +7048,9 @@ 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;", > + ds_put_format(&actions, > + "ct_commit { next_table; %s = 0; " > + "ct_label.label = " REG_LABEL "; };", > ct_block_action); > ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 100, > REGBIT_CONNTRACK_COMMIT" == 1 && " > @@ -7065,7 +7066,8 @@ 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); > + ds_put_format(&actions, "ct_commit { next_table; %s = 0; };", > + ct_block_action); > ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 100, > REGBIT_CONNTRACK_COMMIT" == 1 && " > REGBIT_ACL_LABEL" == 0", > diff --git a/ovn-sb.xml b/ovn-sb.xml > index 315d60853..a4d3302f6 100644 > --- a/ovn-sb.xml > +++ b/ovn-sb.xml > @@ -1373,7 +1373,9 @@ > which will commit connection tracking state, and then drop the > packet. This could be useful for setting <code>ct_mark</code> > on a connection tracking entry before dropping a packet, > - for example. > + for example. In order to allow the packet committed to the ct table > + to continue the processing in the next pipeline stage > + <code>next_table</code> option can be used as first nested actions. > </p> > </dd> > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 7d879b642..c8bd9c20f 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -2961,21 +2961,13 @@ lsp1_inport=$(fetch_column Port_Binding tunnel_key logical_port=lsp1) > # TCP packets should go to conntrack. > flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_tcp}" > AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new --minimal ls "${flow}"], [0], [dnl > -ct_next(ct_state=new|trk) { > - ct_next(ct_state=new|trk) { > - output("lsp2"); > - }; > -}; > +ct_next(ct_state=new|trk); Looks to me you need to enhance ovn-trace to handle this new flag ? With this patch, there is no output to lsp2 now, which seems odd. > ]) > > # UDP packets should go to conntrack. > flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_udp}" > AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new --minimal ls "${flow}"], [0], [dnl > -ct_next(ct_state=new|trk) { > - ct_next(ct_state=new|trk) { > - output("lsp2"); > - }; > -}; > +ct_next(ct_state=new|trk); > ]) > > # Allow stateless for TCP. > @@ -2993,11 +2985,7 @@ output("lsp2"); > # UDP packets still go to conntrack. > flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_udp}" > AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new --minimal ls "${flow}"], [0], [dnl > -ct_next(ct_state=new|trk) { > - ct_next(ct_state=new|trk) { > - output("lsp2"); > - }; > -}; > +ct_next(ct_state=new|trk); > ]) > > # Add a load balancer. > @@ -3105,21 +3093,13 @@ flow_udp='udp && udp.dst == 80' > # TCP packets should go to conntrack. > flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_tcp}" > AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new --minimal ls "${flow}"], [0], [dnl > -ct_next(ct_state=new|trk) { > - ct_next(ct_state=new|trk) { > - output("lsp2"); > - }; > -}; > +ct_next(ct_state=new|trk); > ]) > > # UDP packets should go to conntrack. > flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_udp}" > AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new --minimal ls "${flow}"], [0], [dnl > -ct_next(ct_state=new|trk) { > - ct_next(ct_state=new|trk) { > - output("lsp2"); > - }; > -}; > +ct_next(ct_state=new|trk); > ]) > > # Allow stateless for TCP. > @@ -3137,11 +3117,7 @@ output("lsp2"); > # UDP packets still go to conntrack. > flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_udp}" > AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new --minimal ls "${flow}"], [0], [dnl > -ct_next(ct_state=new|trk) { > - ct_next(ct_state=new|trk) { > - output("lsp2"); > - }; > -}; > +ct_next(ct_state=new|trk); > ]) > > # Add a load balancer. > @@ -3245,11 +3221,7 @@ lsp1_inport=$(fetch_column Port_Binding tunnel_key logical_port=lsp1) > # TCP packets should go to conntrack. > flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_tcp}" > AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new --minimal ls "${flow}"], [0], [dnl > -ct_next(ct_state=new|trk) { > - ct_next(ct_state=new|trk) { > - output("lsp2"); > - }; > -}; > +ct_next(ct_state=new|trk); > ]) > > # Allow stateless with *lower* priority. It always beats stateful rules. > @@ -4027,8 +3999,8 @@ check_stateful_flows() { > > AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed 's/table=../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;) > + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { next_table; ct_mark.blocked = 0; };) > + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0; ct_label.label = reg3; };) > ]) > > AT_CHECK([grep "ls_out_pre_lb" sw0flows | sort], [0], [dnl > @@ -4050,8 +4022,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 && 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;) > + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { next_table; ct_mark.blocked = 0; };) > + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0; ct_label.label = reg3; };) > ]) > } > > @@ -4091,8 +4063,8 @@ AT_CHECK([grep "ls_in_lb" sw0flows | sort | sed 's/table=../table=??/'], [0], [d > > AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed 's/table=../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;) > + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { next_table; ct_mark.blocked = 0; };) > + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0; ct_label.label = reg3; };) > ]) > > AT_CHECK([grep "ls_out_pre_lb" sw0flows | sort], [0], [dnl > @@ -4111,8 +4083,8 @@ 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 && 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;) > + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { next_table; ct_mark.blocked = 0; };) > + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0; ct_label.label = reg3; };) > ]) > > AT_CLEANUP > @@ -4137,8 +4109,8 @@ AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 2002 | sort | sed 's/table=./table > ]) > AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed 's/table=../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;) > + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { next_table; ct_mark.blocked = 0; };) > + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0; ct_label.label = reg3; };) > ]) > > AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 2002 | sort], [0], [dnl > @@ -4147,8 +4119,8 @@ AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 2002 | 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 && 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;) > + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { next_table; ct_mark.blocked = 0; };) > + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0; ct_label.label = reg3; };) > ]) > > # Add new ACL without label > @@ -4166,8 +4138,8 @@ AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 2002 | sort | sed 's/table=./table > ]) > AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed 's/table=../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;) > + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { next_table; ct_mark.blocked = 0; };) > + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0; ct_label.label = reg3; };) > ]) > > AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 2002 | sort], [0], [dnl > @@ -4178,8 +4150,8 @@ AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 2002 | 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 && 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;) > + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { next_table; ct_mark.blocked = 0; };) > + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0; ct_label.label = reg3; };) > ]) > > # Delete new ACL with label > @@ -4195,8 +4167,8 @@ AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 2002 | sort | sed 's/table=./table > ]) > AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed 's/table=../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;) > + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { next_table; ct_mark.blocked = 0; };) > + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0; ct_label.label = reg3; };) > ]) > > AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 2002 | sort], [0], [dnl > @@ -4205,8 +4177,8 @@ AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 2002 | 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 && 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;) > + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { next_table; ct_mark.blocked = 0; };) > + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0; ct_label.label = reg3; };) > ]) > AT_CLEANUP > ]) > @@ -6544,8 +6516,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 { next_table; ct_mark.blocked = 0; };) > + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0; ct_label.label = reg3; };) > ]) > > AS_BOX([Remove and add the ACLs back with the apply-after-lb option]) > @@ -6597,8 +6569,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 { next_table; ct_mark.blocked = 0; };) > + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { next_table; 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]) > @@ -6650,8 +6622,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 { next_table; ct_mark.blocked = 0; };) > + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0; ct_label.label = reg3; };) > ]) > > AT_CLEANUP > diff --git a/tests/ovn.at b/tests/ovn.at > index f8b8db4df..b35f8981b 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 { next_table; ct_mark=1; }; > + formats as ct_commit { ct_mark = 1; }; IMO the formatting of ct_commit{next_table, ...} should not lose the "next_table" flag. I think you should enhance format_CT_COMMIT_V2() to include this flag if set. IMO it's better to add a new action to support this. Which means ovn-northd should check if all the ovn-controllers support this before using this new action. I know there are some concerns about this from Han. So I'd wait for his and other thoughts too before deciding on using a new action - ct_commit_next{} or enhancing the existing ct_commit (which this patch does). Thanks Numan > + 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)) > -- > 2.37.3 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/include/ovn/actions.h b/include/ovn/actions.h index d7ee84dac..d96d34841 100644 --- a/include/ovn/actions.h +++ b/include/ovn/actions.h @@ -315,6 +315,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/lib/actions.c b/lib/actions.c index adbb42db4..88d7eb571 100644 --- a/lib/actions.c +++ b/lib/actions.c @@ -207,6 +207,10 @@ struct action_context { static void parse_actions(struct action_context *, enum lex_type sentinel); +static void __parse_nested_action(struct action_context *ctx, + enum ovnact_type type, + const char *prereq, + enum expr_write_scope scope); static void parse_nested_action(struct action_context *ctx, enum ovnact_type type, const char *prereq, @@ -764,8 +768,23 @@ static void 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); + int table = 0; + lexer_force_match(ctx->lexer, LEX_T_LCURLY); /* Skip '{'. */ + if (lexer_match_id(ctx->lexer, "next_table")) { + lexer_match(ctx->lexer, LEX_T_SEMICOLON); + table = ctx->pp->cur_ltable + 1; + if (table >= ctx->pp->n_tables) { + table = 0; + } + } + __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 = table; } else if (ctx->lexer->token.type == LEX_T_LPAREN) { parse_CT_COMMIT_V1(ctx); } else { @@ -775,6 +794,7 @@ parse_CT_COMMIT(struct action_context *ctx) OVNACT_ALIGN(sizeof *on)); on->nested_len = 0; on->nested = NULL; + on->ltable = 0; } } @@ -872,12 +892,16 @@ 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, + const struct ovnact_encode_params *ep, struct ofpbuf *ofpacts) { struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); ct->flags = NX_CT_F_COMMIT; - ct->recirc_table = NX_CT_RECIRC_NONE; + if (on->ltable > 0) { + ct->recirc_table = first_ptable(ep, ep->pipeline) + on->ltable; + } else { + ct->recirc_table = NX_CT_RECIRC_NONE; + } ct->zone_src.field = ep->is_switch ? mf_from_id(MFF_LOG_CT_ZONE) : mf_from_id(MFF_LOG_DNAT_ZONE); @@ -1586,13 +1610,9 @@ encode_CT_CLEAR(const struct ovnact_null *null OVS_UNUSED, /* Implements the "arp", "nd_na", and "clone" actions, which execute nested * actions on a packet derived from the one being processed. */ static void -parse_nested_action(struct action_context *ctx, enum ovnact_type type, - const char *prereq, enum expr_write_scope scope) +__parse_nested_action(struct action_context *ctx, enum ovnact_type type, + const char *prereq, enum expr_write_scope scope) { - if (!lexer_force_match(ctx->lexer, LEX_T_LCURLY)) { - return; - } - if (ctx->depth + 1 == MAX_NESTED_ACTION_DEPTH) { lexer_error(ctx->lexer, "maximum depth of nested actions reached"); return; @@ -1635,6 +1655,16 @@ parse_nested_action(struct action_context *ctx, enum ovnact_type type, on->nested = ofpbuf_steal_data(&nested); } +static void +parse_nested_action(struct action_context *ctx, enum ovnact_type type, + const char *prereq, enum expr_write_scope scope) +{ + if (!lexer_force_match(ctx->lexer, LEX_T_LCURLY)) { + return; + } + __parse_nested_action(ctx, type, prereq, scope); +} + static void parse_ARP(struct action_context *ctx) { diff --git a/northd/northd.c b/northd/northd.c index 6771ccce5..7b31466ac 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -7048,8 +7048,9 @@ 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;", + ds_put_format(&actions, + "ct_commit { next_table; %s = 0; " + "ct_label.label = " REG_LABEL "; };", ct_block_action); ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 100, REGBIT_CONNTRACK_COMMIT" == 1 && " @@ -7065,7 +7066,8 @@ 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); + ds_put_format(&actions, "ct_commit { next_table; %s = 0; };", + ct_block_action); ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 100, REGBIT_CONNTRACK_COMMIT" == 1 && " REGBIT_ACL_LABEL" == 0", diff --git a/ovn-sb.xml b/ovn-sb.xml index 315d60853..a4d3302f6 100644 --- a/ovn-sb.xml +++ b/ovn-sb.xml @@ -1373,7 +1373,9 @@ which will commit connection tracking state, and then drop the packet. This could be useful for setting <code>ct_mark</code> on a connection tracking entry before dropping a packet, - for example. + for example. In order to allow the packet committed to the ct table + to continue the processing in the next pipeline stage + <code>next_table</code> option can be used as first nested actions. </p> </dd> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 7d879b642..c8bd9c20f 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -2961,21 +2961,13 @@ lsp1_inport=$(fetch_column Port_Binding tunnel_key logical_port=lsp1) # TCP packets should go to conntrack. flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_tcp}" AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new --minimal ls "${flow}"], [0], [dnl -ct_next(ct_state=new|trk) { - ct_next(ct_state=new|trk) { - output("lsp2"); - }; -}; +ct_next(ct_state=new|trk); ]) # UDP packets should go to conntrack. flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_udp}" AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new --minimal ls "${flow}"], [0], [dnl -ct_next(ct_state=new|trk) { - ct_next(ct_state=new|trk) { - output("lsp2"); - }; -}; +ct_next(ct_state=new|trk); ]) # Allow stateless for TCP. @@ -2993,11 +2985,7 @@ output("lsp2"); # UDP packets still go to conntrack. flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_udp}" AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new --minimal ls "${flow}"], [0], [dnl -ct_next(ct_state=new|trk) { - ct_next(ct_state=new|trk) { - output("lsp2"); - }; -}; +ct_next(ct_state=new|trk); ]) # Add a load balancer. @@ -3105,21 +3093,13 @@ flow_udp='udp && udp.dst == 80' # TCP packets should go to conntrack. flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_tcp}" AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new --minimal ls "${flow}"], [0], [dnl -ct_next(ct_state=new|trk) { - ct_next(ct_state=new|trk) { - output("lsp2"); - }; -}; +ct_next(ct_state=new|trk); ]) # UDP packets should go to conntrack. flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_udp}" AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new --minimal ls "${flow}"], [0], [dnl -ct_next(ct_state=new|trk) { - ct_next(ct_state=new|trk) { - output("lsp2"); - }; -}; +ct_next(ct_state=new|trk); ]) # Allow stateless for TCP. @@ -3137,11 +3117,7 @@ output("lsp2"); # UDP packets still go to conntrack. flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_udp}" AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new --minimal ls "${flow}"], [0], [dnl -ct_next(ct_state=new|trk) { - ct_next(ct_state=new|trk) { - output("lsp2"); - }; -}; +ct_next(ct_state=new|trk); ]) # Add a load balancer. @@ -3245,11 +3221,7 @@ lsp1_inport=$(fetch_column Port_Binding tunnel_key logical_port=lsp1) # TCP packets should go to conntrack. flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_tcp}" AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new --minimal ls "${flow}"], [0], [dnl -ct_next(ct_state=new|trk) { - ct_next(ct_state=new|trk) { - output("lsp2"); - }; -}; +ct_next(ct_state=new|trk); ]) # Allow stateless with *lower* priority. It always beats stateful rules. @@ -4027,8 +3999,8 @@ check_stateful_flows() { AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed 's/table=../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;) + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { next_table; ct_mark.blocked = 0; };) + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0; ct_label.label = reg3; };) ]) AT_CHECK([grep "ls_out_pre_lb" sw0flows | sort], [0], [dnl @@ -4050,8 +4022,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 && 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;) + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { next_table; ct_mark.blocked = 0; };) + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0; ct_label.label = reg3; };) ]) } @@ -4091,8 +4063,8 @@ AT_CHECK([grep "ls_in_lb" sw0flows | sort | sed 's/table=../table=??/'], [0], [d AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed 's/table=../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;) + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { next_table; ct_mark.blocked = 0; };) + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0; ct_label.label = reg3; };) ]) AT_CHECK([grep "ls_out_pre_lb" sw0flows | sort], [0], [dnl @@ -4111,8 +4083,8 @@ 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 && 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;) + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { next_table; ct_mark.blocked = 0; };) + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0; ct_label.label = reg3; };) ]) AT_CLEANUP @@ -4137,8 +4109,8 @@ AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 2002 | sort | sed 's/table=./table ]) AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed 's/table=../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;) + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { next_table; ct_mark.blocked = 0; };) + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0; ct_label.label = reg3; };) ]) AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 2002 | sort], [0], [dnl @@ -4147,8 +4119,8 @@ AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 2002 | 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 && 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;) + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { next_table; ct_mark.blocked = 0; };) + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0; ct_label.label = reg3; };) ]) # Add new ACL without label @@ -4166,8 +4138,8 @@ AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 2002 | sort | sed 's/table=./table ]) AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed 's/table=../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;) + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { next_table; ct_mark.blocked = 0; };) + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0; ct_label.label = reg3; };) ]) AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 2002 | sort], [0], [dnl @@ -4178,8 +4150,8 @@ AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 2002 | 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 && 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;) + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { next_table; ct_mark.blocked = 0; };) + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0; ct_label.label = reg3; };) ]) # Delete new ACL with label @@ -4195,8 +4167,8 @@ AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 2002 | sort | sed 's/table=./table ]) AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed 's/table=../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;) + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { next_table; ct_mark.blocked = 0; };) + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0; ct_label.label = reg3; };) ]) AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 2002 | sort], [0], [dnl @@ -4205,8 +4177,8 @@ AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 2002 | 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 && 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;) + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { next_table; ct_mark.blocked = 0; };) + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0; ct_label.label = reg3; };) ]) AT_CLEANUP ]) @@ -6544,8 +6516,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 { next_table; ct_mark.blocked = 0; };) + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0; ct_label.label = reg3; };) ]) AS_BOX([Remove and add the ACLs back with the apply-after-lb option]) @@ -6597,8 +6569,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 { next_table; ct_mark.blocked = 0; };) + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { next_table; 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]) @@ -6650,8 +6622,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 { next_table; ct_mark.blocked = 0; };) + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0; ct_label.label = reg3; };) ]) AT_CLEANUP diff --git a/tests/ovn.at b/tests/ovn.at index f8b8db4df..b35f8981b 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 { next_table; ct_mark=1; }; + formats as ct_commit { 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))
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 next_table option in the ct_commit {} action allowing the ct packet to proceed in the pipeline. Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2103086 Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> --- include/ovn/actions.h | 1 + lib/actions.c | 50 ++++++++++++++++++----- northd/northd.c | 8 ++-- ovn-sb.xml | 4 +- tests/ovn-northd.at | 94 +++++++++++++++---------------------------- tests/ovn.at | 4 ++ 6 files changed, 86 insertions(+), 75 deletions(-)