On 4/9/24 14:38, Martin Kalcok wrote: > LGTM, thanks for helping with this clean-up. > > On Wed, Apr 3, 2024 at 7:43 AM Ales Musil <amu...@redhat.com> wrote: > >> >> >> On Tue, Apr 2, 2024 at 5:31 PM Dumitru Ceara <dce...@redhat.com> wrote: >> >>> It's not used by any logical flow since v20.12.0. It was kept for >>> backwards compatibility reasons in 4799f73fd6ed ("Fix OVN update >>> issue when ovn-controller is updated first from 20.06 to 20.09.") but >>> now there's no supported release that uses it. >>> >>> Therefore the action is safe to be removed. Update the tests to reflect >>> the fact that the action is not supported anymore. >>> >>> Signed-off-by: Dumitru Ceara <dce...@redhat.com> >>> --- >>> include/ovn/actions.h | 9 +-- >>> lib/actions.c | 141 ------------------------------------------ >>> tests/ovn.at | 41 ++++-------- >>> utilities/ovn-trace.c | 1 - >>> 4 files changed, 12 insertions(+), 180 deletions(-) >>> >>> diff --git a/include/ovn/actions.h b/include/ovn/actions.h >>> index dcacbb1fff..8e794450cf 100644 >>> --- a/include/ovn/actions.h >>> +++ b/include/ovn/actions.h >>> @@ -66,7 +66,7 @@ struct collector_set_ids; >>> OVNACT(EXCHANGE, ovnact_move) \ >>> OVNACT(DEC_TTL, ovnact_null) \ >>> OVNACT(CT_NEXT, ovnact_ct_next) \ >>> - OVNACT(CT_COMMIT_V1, ovnact_ct_commit_v1) \ >>> + /* CT_COMMIT_V1 is not supported anymore. */ \ >>> OVNACT(CT_COMMIT_V2, ovnact_nest) \ >>> OVNACT(CT_DNAT, ovnact_ct_nat) \ >>> OVNACT(CT_SNAT, ovnact_ct_nat) \ >>> @@ -260,13 +260,6 @@ struct ovnact_ct_next { >>> uint8_t ltable; /* Logical table ID of next table. */ >>> }; >>> >>> -/* OVNACT_CT_COMMIT_V1. */ >>> -struct ovnact_ct_commit_v1 { >>> - struct ovnact ovnact; >>> - uint32_t ct_mark, ct_mark_mask; >>> - ovs_be128 ct_label, ct_label_mask; >>> -}; >>> - >>> /* Type of NAT used for the particular action. >>> * UNSPEC translates to applying NAT that works for both directions. */ >>> enum ovnact_ct_nat_type { >>> diff --git a/lib/actions.c b/lib/actions.c >>> index 71615fc53c..7c20535717 100644 >>> --- a/lib/actions.c >>> +++ b/lib/actions.c >>> @@ -729,71 +729,12 @@ ovnact_ct_next_free(struct ovnact_ct_next *a >>> OVS_UNUSED) >>> { >>> } >>> >>> -static void >>> -parse_ct_commit_v1_arg(struct action_context *ctx, >>> - struct ovnact_ct_commit_v1 *cc) >>> -{ >>> - if (lexer_match_id(ctx->lexer, "ct_mark")) { >>> - if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) { >>> - return; >>> - } >>> - if (ctx->lexer->token.type == LEX_T_INTEGER) { >>> - cc->ct_mark = ntohll(ctx->lexer->token.value.integer); >>> - cc->ct_mark_mask = UINT32_MAX; >>> - } else if (ctx->lexer->token.type == LEX_T_MASKED_INTEGER) { >>> - cc->ct_mark = ntohll(ctx->lexer->token.value.integer); >>> - cc->ct_mark_mask = ntohll(ctx->lexer->token.mask.integer); >>> - } else { >>> - lexer_syntax_error(ctx->lexer, "expecting integer"); >>> - return; >>> - } >>> - lexer_get(ctx->lexer); >>> - } else if (lexer_match_id(ctx->lexer, "ct_label")) { >>> - if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) { >>> - return; >>> - } >>> - if (ctx->lexer->token.type == LEX_T_INTEGER) { >>> - cc->ct_label = ctx->lexer->token.value.be128_int; >>> - cc->ct_label_mask = OVS_BE128_MAX; >>> - } else if (ctx->lexer->token.type == LEX_T_MASKED_INTEGER) { >>> - cc->ct_label = ctx->lexer->token.value.be128_int; >>> - cc->ct_label_mask = ctx->lexer->token.mask.be128_int; >>> - } else { >>> - lexer_syntax_error(ctx->lexer, "expecting integer"); >>> - return; >>> - } >>> - lexer_get(ctx->lexer); >>> - } else { >>> - lexer_syntax_error(ctx->lexer, NULL); >>> - } >>> -} >>> - >>> -static void >>> -parse_CT_COMMIT_V1(struct action_context *ctx) >>> -{ >>> - add_prerequisite(ctx, "ip"); >>> - >>> - struct ovnact_ct_commit_v1 *ct_commit = >>> - ovnact_put_CT_COMMIT_V1(ctx->ovnacts); >>> - if (lexer_match(ctx->lexer, LEX_T_LPAREN)) { >>> - while (!lexer_match(ctx->lexer, LEX_T_RPAREN)) { >>> - parse_ct_commit_v1_arg(ctx, ct_commit); >>> - if (ctx->lexer->error) { >>> - return; >>> - } >>> - lexer_match(ctx->lexer, LEX_T_COMMA); >>> - } >>> - } >>> -} >>> - >>> 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); >>> - } else if (ctx->lexer->token.type == LEX_T_LPAREN) { >>> - parse_CT_COMMIT_V1(ctx); >>> } else { >>> /* Add an empty nested action to allow for "ct_commit;" syntax */ >>> add_prerequisite(ctx, "ip"); >>> @@ -804,88 +745,6 @@ parse_CT_COMMIT(struct action_context *ctx) >>> } >>> } >>> >>> -static void >>> -format_CT_COMMIT_V1(const struct ovnact_ct_commit_v1 *cc, struct ds *s) >>> -{ >>> - ds_put_cstr(s, "ct_commit("); >>> - if (cc->ct_mark_mask) { >>> - ds_put_format(s, "ct_mark=%#"PRIx32, cc->ct_mark); >>> - if (cc->ct_mark_mask != UINT32_MAX) { >>> - ds_put_format(s, "/%#"PRIx32, cc->ct_mark_mask); >>> - } >>> - } >>> - if (!ovs_be128_is_zero(cc->ct_label_mask)) { >>> - if (ds_last(s) != '(') { >>> - ds_put_cstr(s, ", "); >>> - } >>> - >>> - ds_put_format(s, "ct_label="); >>> - ds_put_hex(s, &cc->ct_label, sizeof cc->ct_label); >>> - if (!ovs_be128_equals(cc->ct_label_mask, OVS_BE128_MAX)) { >>> - ds_put_char(s, '/'); >>> - ds_put_hex(s, &cc->ct_label_mask, sizeof cc->ct_label_mask); >>> - } >>> - } >>> - if (!ds_chomp(s, '(')) { >>> - ds_put_char(s, ')'); >>> - } >>> - ds_put_char(s, ';'); >>> -} >>> - >>> -static void >>> -encode_CT_COMMIT_V1(const struct ovnact_ct_commit_v1 *cc, >>> - 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_CT_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); >>> - >>> - if (cc->ct_mark_mask) { >>> - const ovs_be32 value = htonl(cc->ct_mark); >>> - const ovs_be32 mask = htonl(cc->ct_mark_mask); >>> - ofpact_put_set_field(ofpacts, mf_from_id(MFF_CT_MARK), &value, >>> &mask); >>> - } >>> - >>> - if (!ovs_be128_is_zero(cc->ct_label_mask)) { >>> - ofpact_put_set_field(ofpacts, mf_from_id(MFF_CT_LABEL), >>> &cc->ct_label, >>> - &cc->ct_label_mask); >>> - } >>> - >>> - ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset); >>> - ct = ofpacts->header; >>> - ofpact_finish(ofpacts, &ct->ofpact); >>> -} >>> - >>> -static void >>> -ovnact_ct_commit_v1_free(struct ovnact_ct_commit_v1 *cc OVS_UNUSED) >>> -{ >>> -} >>> - >>> - >>> - >>> static void >>> format_CT_COMMIT_V2(const struct ovnact_nest *on, struct ds *s) >>> { >>> diff --git a/tests/ovn.at b/tests/ovn.at >>> index dd2ebce98e..f05437b9cd 100644 >>> --- a/tests/ovn.at >>> +++ b/tests/ovn.at >>> @@ -1318,47 +1318,28 @@ ct_commit { ip4.dst = 192.168.0.1; }; >>> >>> # Legact ct_commit_v1 action. >>> ct_commit(); >>> - formats as ct_commit; >>> - encodes as ct(commit,zone=NXM_NX_REG13[[0..15]]) >>> - has prereqs ip >>> + Syntax error at `(' expecting `;'. >>> ct_commit(ct_mark=1); >>> - formats as ct_commit(ct_mark=0x1); >>> - encodes as >>> ct(commit,zone=NXM_NX_REG13[[0..15]],exec(set_field:0x1->ct_mark)) >>> - has prereqs ip >>> + Syntax error at `(' expecting `;'. >>> ct_commit(ct_mark=1/1); >>> - formats as ct_commit(ct_mark=0x1/0x1); >>> - encodes as >>> ct(commit,zone=NXM_NX_REG13[[0..15]],exec(set_field:0x1/0x1->ct_mark)) >>> - has prereqs ip >>> + Syntax error at `(' expecting `;'. >>> ct_commit(ct_label=1); >>> - formats as ct_commit(ct_label=0x1); >>> - encodes as >>> ct(commit,zone=NXM_NX_REG13[[0..15]],exec(set_field:0x1->ct_label)) >>> - has prereqs ip >>> + Syntax error at `(' expecting `;'. >>> ct_commit(ct_label=1/1); >>> - formats as ct_commit(ct_label=0x1/0x1); >>> - encodes as >>> ct(commit,zone=NXM_NX_REG13[[0..15]],exec(set_field:0x1/0x1->ct_label)) >>> - has prereqs ip >>> + Syntax error at `(' expecting `;'. >>> ct_commit(ct_mark=1, ct_label=2); >>> - formats as ct_commit(ct_mark=0x1, ct_label=0x2); >>> - encodes as >>> ct(commit,zone=NXM_NX_REG13[[0..15]],exec(set_field:0x1->ct_mark,set_field:0x2->ct_label)) >>> - has prereqs ip >>> + Syntax error at `(' expecting `;'. >>> >>> ct_commit(ct_label=0x01020304050607080910111213141516); >>> - formats as ct_commit(ct_label=0x1020304050607080910111213141516); >>> - encodes as >>> ct(commit,zone=NXM_NX_REG13[[0..15]],exec(set_field:0x1020304050607080910111213141516->ct_label)) >>> - has prereqs ip >>> + Syntax error at `(' expecting `;'. >>> ct_commit(ct_label=0x181716151413121110090807060504030201); >>> - formats as ct_commit(ct_label=0x16151413121110090807060504030201); >>> - encodes as >>> ct(commit,zone=NXM_NX_REG13[[0..15]],exec(set_field:0x16151413121110090807060504030201->ct_label)) >>> - has prereqs ip >>> + Syntax error at `(' expecting `;'. >>> >>> >>> ct_commit(ct_label=0x1000000000000000000000000000000/0x1000000000000000000000000000000); >>> - encodes as >>> ct(commit,zone=NXM_NX_REG13[[0..15]],exec(set_field:0x1000000000000000000000000000000/0x1000000000000000000000000000000->ct_label)) >>> - has prereqs ip >>> + Syntax error at `(' expecting `;'. >>> ct_commit(ct_label=18446744073709551615); >>> - formats as ct_commit(ct_label=0xffffffffffffffff); >>> - encodes as >>> ct(commit,zone=NXM_NX_REG13[[0..15]],exec(set_field:0xffffffffffffffff->ct_label)) >>> - has prereqs ip >>> + Syntax error at `(' expecting `;'. >>> ct_commit(ct_label=18446744073709551616); >>> - Decimal constants must be less than 2**64. >>> + Syntax error at `(' expecting `;'. >>> >>> ct_mark = 12345 >>> Field ct_mark is not modifiable. >>> diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c >>> index e0f1c3ec9a..5e55fbbcc0 100644 >>> --- a/utilities/ovn-trace.c >>> +++ b/utilities/ovn-trace.c >>> @@ -3107,7 +3107,6 @@ trace_actions(const struct ovnact *ovnacts, size_t >>> ovnacts_len, >>> execute_ct_next(ovnact_get_CT_NEXT(a), dp, uflow, pipeline, >>> super); >>> break; >>> >>> - case OVNACT_CT_COMMIT_V1: >>> case OVNACT_CT_COMMIT_V2: >>> /* Nothing to do. */ >>> break; >>> -- >>> 2.44.0 >>> >>> >> Makes sense, thanks! >> >> Acked-by: Ales Musil <amu...@redhat.com> >>
Thanks, Ales and Martin! I applied this to main with the following minor change (documentation parts that I had forgotten to change initially): diff --git a/northd/northd.c b/northd/northd.c index 02cf5b2344..51181e08aa 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -6568,7 +6568,7 @@ consider_acl(struct lflow_table *lflows, const struct ovn_datapath *od, * that this connection is set for deletion. By not * specifying "next;", we implicitly drop the packet after * updating conntrack state. We would normally defer - * ct_commit() to the "stateful" stage, but since we're + * ct_commit to the "stateful" stage, but since we're * rejecting/dropping the packet, we go ahead and do it here. */ ds_truncate(match, match_tier_len); @@ -6883,7 +6883,7 @@ build_acls(const struct ls_stateful_record *ls_stateful_rec, * set on them. That's a connection that was disallowed, but is * now allowed by policy again since it hit this default-allow flow. * We need to set ct_mark.blocked=0 to let the connection continue, - * which will be done by ct_commit() in the "stateful" stage. + * which will be done by ct_commit in the "stateful" stage. * Subsequent packets will hit the flow at priority 0 that just * uses "next;". */ ds_clear(&match); diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml index 90ce0de3f9..b14a302852 100644 --- a/northd/ovn-northd.8.xml +++ b/northd/ovn-northd.8.xml @@ -724,7 +724,7 @@ </li> <li> <code>allow-related</code> ACLs translate into logical flows that set - the allow bit and additionally have <code>ct_commit(ct_label=0/1); + the allow bit and additionally have <code>ct_commit { ct_label=0/1; }; next;</code> actions for new connections and <code>reg0[1] = 1; next;</code> for existing connections. In case the <code>ACL</code> has a label then <code>reg3</code> is loaded with the label value and @@ -746,9 +746,9 @@ <li> Other ACLs set the drop bit and advance to the next table for new or untracked connections. For known connections, they set the drop bit, - as well as running the <code>ct_commit(ct_label=1/1);</code> action. - Setting <code>ct_label</code> marks a connection as one that was - previously allowed, but should no longer be allowed due to a policy + as well as running the <code>ct_commit { ct_label=1/1; };</code> + action. 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. </li> </ul> @@ -1218,10 +1218,11 @@ </li> <li> <code>allow-related</code> apply-after-lb ACLs translate into logical - flows that set the allow bit and run the <code>ct_commit(ct_label=0/1); - next;</code> actions for new connections and <code>reg0[1] = 1; - next;</code> for existing connections. In case the <code>ACL</code> - has a label then <code>reg3</code> is loaded with the label value and + flows that set the allow bit and run the + <code>ct_commit {ct_label=0/1; }; next;</code> actions for new + connections and <code>reg0[1] = 1; next;</code> for existing + connections. In case the <code>ACL</code> has a label then + <code>reg3</code> is loaded with the label value and <code>reg0[13]</code> bit is set to 1 (which acts as a hint for the next tables to commit the label to conntrack). </li> @@ -1240,7 +1241,7 @@ </li> <li> Other apply-after-lb ACLs set the drop bit for new or untracked - connections and <code>ct_commit(ct_label=1/1);</code> for known + 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. --- Regards, Dumitru _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev