On 7/24/20 4:56 AM, Numan Siddique wrote:
On Fri, Jul 24, 2020 at 1:07 AM Mark Michelson <[email protected]
<mailto:[email protected]>> wrote:
ct_commit allow for ct_label and ct_mark to be set within. However,
there are some restrictions with the current implementation:
* It is not possible to address the indiviual bits within the ct_mark or
ct_label.
* It is not possible to set these to the value of a register. Only
explicit integer setting can be used.
With this change, ct_commit now can have arbitrary nested actions
inside. This makes it similar to how the "exec" option works in OVS's
ct() action.
In this commit, the only noticeable effect is that it allows for
slightly more expressive setting of ct_label.blocked. A future commit
will take further advantage of this.
Signed-off-by: Mark Michelson <[email protected]
<mailto:[email protected]>>
Hi Mark,
I have a few comments. Please see below.
---
include/ovn/actions.h | 9 +---
lib/actions.c | 109 +++++++-----------------------------------
northd/ovn-northd.c | 8 ++--
tests/ovn.at <http://ovn.at> | 50 +++++++++----------
4 files changed, 47 insertions(+), 129 deletions(-)
diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index 0f7f4cdb8..12b7ab0df 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -57,7 +57,7 @@ struct ovn_extend_table;
OVNACT(EXCHANGE, ovnact_move) \
OVNACT(DEC_TTL, ovnact_null) \
OVNACT(CT_NEXT, ovnact_ct_next) \
- OVNACT(CT_COMMIT, ovnact_ct_commit) \
+ OVNACT(CT_COMMIT, ovnact_nest) \
OVNACT(CT_DNAT, ovnact_ct_nat) \
OVNACT(CT_SNAT, ovnact_ct_nat) \
OVNACT(CT_LB, ovnact_ct_lb) \
@@ -220,13 +220,6 @@ struct ovnact_ct_next {
uint8_t ltable; /* Logical table ID of next
table. */
};
-/* OVNACT_CT_COMMIT. */
-struct ovnact_ct_commit {
- struct ovnact ovnact;
- uint32_t ct_mark, ct_mark_mask;
- ovs_be128 ct_label, ct_label_mask;
-};
-
/* OVNACT_CT_DNAT, OVNACT_CT_SNAT. */
struct ovnact_ct_nat {
struct ovnact ovnact;
diff --git a/lib/actions.c b/lib/actions.c
index e14907e3d..330fd616b 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -199,6 +199,14 @@ 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);
+
+static void format_nested_action(const struct ovnact_nest *on,
+ const char *name,
+ struct ds *s);
+
static bool
action_parse_field(struct action_context *ctx,
int n_bits, bool rw, struct expr_field *f)
@@ -617,125 +625,40 @@ ovnact_ct_next_free(struct ovnact_ct_next *a
OVS_UNUSED)
{
}
-static void
-parse_ct_commit_arg(struct action_context *ctx,
- struct ovnact_ct_commit *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(struct action_context *ctx)
{
- add_prerequisite(ctx, "ip");
-
- struct ovnact_ct_commit *ct_commit =
ovnact_put_CT_COMMIT(ctx->ovnacts);
- if (lexer_match(ctx->lexer, LEX_T_LPAREN)) {
- while (!lexer_match(ctx->lexer, LEX_T_RPAREN)) {
- parse_ct_commit_arg(ctx, ct_commit);
- if (ctx->lexer->error) {
- return;
- }
- lexer_match(ctx->lexer, LEX_T_COMMA);
- }
- }
+ parse_nested_action(ctx, OVNACT_CT_COMMIT, "ip");
I like the idea of allowing ct_commit to have nested actions.
There is one issue here. The OVS ct action when used with commit,
allows exec
to set only 2 fields - ct_mark and ct_label.
But with this patch, ovn-northd can add a logical flow like ct_commit {
ip4.src = 10.0.0.4}
and the OVN parse will not complain about it.
I think we can have something like below to restrict the exec to only
ct_mark and ct_label.
*********
struct shash ct_symtab;
shash_init(&ct_symtab);
expr_symtab_add_field(&ct_symtab, "ct_mark", MFF_CT_MARK, NULL, false);
expr_symtab_add_field(&ct_symtab, "ct_label", MFF_CT_LABEL, NULL, false);
expr_symtab_add_subfield(&ct_symtab, "ct_label.blocked", NULL,
"ct_label[0]");
struct shash *orig_symtab = ctx->pp->symtab;
ctx->pp->symtab = &ct_symtab;
parse_nested_action(ctx, OVNACT_CT_COMMIT, "ip");
ctx->pp->symtab = orig_symtab;
*********
This ct_symtab can be initialized in logical-fields.c too.
This is the approach I thought of. There can be a better approach too.
Please feel free to propose a better one.
But I think we should restrict the actions inside ct_commit{}.
Thanks
Numan
Yes, thanks for the explanation. I had misread the OVS ct() action
documentation. I thought that exec() allowed for any arbitrary action to
be used. But I also thought that setting ct_mark and ct_label were
restricted to within ct(exec()) command. I see now how that is incorrect
and I'll alter the OVN action parser to restrict what actions can be
performed within ct_commit.
}
static void
-format_CT_COMMIT(const struct ovnact_ct_commit *cc, struct ds *s)
+format_CT_COMMIT(const struct ovnact_nest *on, 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, ';');
+ format_nested_action(on, "ct_commit", s);
}
static void
-encode_CT_COMMIT(const struct ovnact_ct_commit *cc,
+encode_CT_COMMIT(const struct ovnact_nest *on,
const struct ovnact_encode_params *ep OVS_UNUSED,
struct ofpbuf *ofpacts)
{
struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
ct->flags = NX_CT_F_COMMIT;
ct->recirc_table = NX_CT_RECIRC_NONE;
- ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE);
+ ct->zone_src.field = ep->is_switch
+ ? mf_from_id(MFF_LOG_CT_ZONE)
+ : mf_from_id(MFF_LOG_DNAT_ZONE);
ct->zone_src.ofs = 0;
ct->zone_src.n_bits = 16;
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);
- }
-
+ ovnacts_encode(on->nested, on->nested_len, ep, ofpacts);
ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset);
ct = ofpacts->header;
ofpact_finish(ofpacts, &ct->ofpact);
}
-
-static void
-ovnact_ct_commit_free(struct ovnact_ct_commit *cc OVS_UNUSED)
-{
-}
static void
parse_ct_nat(struct action_context *ctx, const char *name,
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 192198272..d10e5ee5d 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -5357,7 +5357,7 @@ consider_acl(struct hmap *lflows, struct
ovn_datapath *od,
ds_clear(&match);
ds_clear(&actions);
ds_put_cstr(&match, "ct.est && ct_label.blocked == 0");
- ds_put_cstr(&actions, "ct_commit(ct_label=1/1); ");
+ ds_put_cstr(&actions, "ct_commit { ct_label.blocked =
1; }; ");
if (!strcmp(acl->action, "reject")) {
build_reject_acl_rules(od, lflows, stage, acl, &match,
&actions, &acl->header_);
@@ -5881,9 +5881,11 @@ build_stateful(struct ovn_datapath *od,
struct hmap *lflows, struct hmap *lbs)
* any packet that makes it this far is part of a connection we
* want to allow to continue. */
ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 100,
- REGBIT_CONNTRACK_COMMIT" == 1",
"ct_commit(ct_label=0/1); next;");
+ REGBIT_CONNTRACK_COMMIT" == 1",
+ "ct_commit { ct_label.blocked = 0; }; next;");
ovn_lflow_add(lflows, od, S_SWITCH_OUT_STATEFUL, 100,
- REGBIT_CONNTRACK_COMMIT" == 1",
"ct_commit(ct_label=0/1); next;");
+ REGBIT_CONNTRACK_COMMIT" == 1",
+ "ct_commit { ct_label.blocked = 0; }; next;");
/* If REGBIT_CONNTRACK_NAT is set as 1, then packets should
just be sent
* through nat (without committing).
diff --git a/tests/ovn.at <http://ovn.at> b/tests/ovn.at <http://ovn.at>
index e19efafbe..3afdcca1e 100644
--- a/tests/ovn.at <http://ovn.at>
+++ b/tests/ovn.at <http://ovn.at>
@@ -1048,51 +1048,51 @@ ct_next;
has prereqs ip
# ct_commit
-ct_commit;
+ct_commit { };
+ formats as ct_commit { drop; };
encodes as ct(commit,zone=NXM_NX_REG13[0..15])
has prereqs ip
-ct_commit();
- formats as ct_commit;
- encodes as ct(commit,zone=NXM_NX_REG13[0..15])
- has prereqs ip
-ct_commit(ct_mark=1);
- formats as ct_commit(ct_mark=0x1);
+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(ct_mark=1/1);
- formats as ct_commit(ct_mark=0x1/0x1);
+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))
has prereqs ip
-ct_commit(ct_label=1);
- formats as ct_commit(ct_label=0x1);
+ct_commit { ct_label=1; };
+ formats as ct_commit { ct_label = 1; };
encodes as
ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1->ct_label))
has prereqs ip
-ct_commit(ct_label=1/1);
- formats as ct_commit(ct_label=0x1/0x1);
+ct_commit { ct_label=1/1; };
+ formats as ct_commit { ct_label = 1/1; };
encodes as
ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1/0x1->ct_label))
has prereqs ip
-ct_commit(ct_mark=1, ct_label=2);
- formats as ct_commit(ct_mark=0x1, ct_label=0x2);
+ct_commit { ct_mark=1; ct_label=2; };
+ formats as ct_commit { ct_mark = 1; ct_label = 2; };
encodes as
ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1->ct_mark,set_field:0x2->ct_label))
has prereqs ip
-ct_commit(ct_label=0x01020304050607080910111213141516);
- formats as ct_commit(ct_label=0x1020304050607080910111213141516);
+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
-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
-ct_commit(ct_label=0x1000000000000000000000000000000/0x1000000000000000000000000000000);
+ct_commit {
ct_label=0x1000000000000000000000000000000/0x1000000000000000000000000000000;
};
+ formats as 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
-ct_commit(ct_label=18446744073709551615);
- formats as ct_commit(ct_label=0xffffffffffffffff);
+ct_commit { ct_label=18446744073709551615; };
+ formats as ct_commit { ct_label = 18446744073709551615; };
encodes as
ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0xffffffffffffffff->ct_label))
has prereqs ip
-ct_commit(ct_label=18446744073709551616);
+ct_commit { ct_label[0..47] = 0x00000f040201; ct_label[48..63] =
0x0002; };
+ formats as ct_commit { ct_label[0..47] = 0xf040201;
ct_label[48..63] = 0x2; };
+ encodes as
ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0xf040201/0xffffffffffff->ct_label,set_field:0x2000000000000/0xffff000000000000->ct_label))
+ has prereqs ip
+ct_commit { ct_label=18446744073709551616; };
Decimal constants must be less than 2**64.
+ct_commit { ct_label=0x181716151413121110090807060504030201; };
+ 141-bit constant is not compatible with 128-bit field ct_label.
# ct_dnat
ct_dnat;
--
2.25.4
_______________________________________________
dev mailing list
[email protected] <mailto:[email protected]>
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
