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 <[email protected]> wrote:
>
>>
>>
>> On Tue, Apr 2, 2024 at 5:31 PM Dumitru Ceara <[email protected]> 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 <[email protected]>
>>> ---
>>> 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 <[email protected]>
>>
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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
