Re: [ovs-dev] [PATCH ovn v2 2/3] Use nested actions in ct_commit.

Fri, 24 Jul 2020 11:56:21 -0700 

On 7/24/20 9:46 AM, Mark Michelson wrote:

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.


Hi Numan,


The method you suggested (with a few tweaks) worked well up to a point. 
The problem is that limiting the number of symbols to "ct_mark" and 
"ct_label" means you can't do things like:


    ct_commit { ct_label[0..47] = eth.src; };


because "eth.src" is an unrecognized symbol. What I realized is that I 
want access to all the symbols, but I want most of them to be read-only. 
So in my next version of the patch, I have introduced writeability 
scopes. It gets us exactly what we want: ct_commit can only set ct_mark 
and ct_label. It also gets us the bonus that you can no longer set 
ct_mark and ct_label outside of ct_commit. I'll post it once I address 
your comments on patch 3 in the series.









       }

       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






















					Reply via email to