| Message ID | 1575290417-8690-1-git-send-email-dceara@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,ovn] ovn-controller: Add missing port group lflow references. | expand |
Bleep bloop. Greetings Dumitru Ceara, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: ERROR: Remove Gerrit Change-Id's before submitting upstream. 21: Change-Id: Id39695f022f16b598fd8416cd2494e7dab3bf11b Lines checked: 230, Warnings: 0, Errors: 1 Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
Thanks for this patch. This can be a security issue as ACLs applied to a Port Group may not be taking effect. Tested this patch on an OpenStack environment that recreated the issue and I confirm that it fixes the problem. On Mon, Dec 2, 2019 at 1:40 PM Dumitru Ceara <dceara@redhat.com> wrote: > The commit that adds incremental processing for port-group changes > doesn't store logical flow references for port groups. If a port group > is updated (e.g., a port is added) no logical flow recalculation will be > performed. > > To fix this, when parsing the flow expression also store the referenced > port groups and bind them to the logical flows that depend on them. If > the port group is updated then the logical flows referring them will > also be reinstalled. > > Reported-by: Daniel Alvarez <dalvarez@redhat.com> > Reported-at: https://bugzilla.redhat.com/1778164 > CC: Han Zhou <hzhou@ovn.org> > Fixes: 978f5e90af0a ("ovn-controller: Incremental processing for > port-group changes.") > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > Change-Id: Id39695f022f16b598fd8416cd2494e7dab3bf11b > --- > controller/lflow.c | 9 ++++++++- > include/ovn/expr.h | 4 +++- > lib/actions.c | 4 ++-- > lib/expr.c | 24 +++++++++++++++++------- > tests/test-ovn.c | 8 ++++---- > utilities/ovn-trace.c | 2 +- > 6 files changed, 35 insertions(+), 16 deletions(-) > > diff --git a/controller/lflow.c b/controller/lflow.c > index 36150bd..a689320 100644 > --- a/controller/lflow.c > +++ b/controller/lflow.c > @@ -616,14 +616,21 @@ consider_logical_flow( > struct expr *expr; > > struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref); > + struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref); > expr = expr_parse_string(lflow->match, &symtab, addr_sets, > port_groups, > - &addr_sets_ref, &error); > + &addr_sets_ref, &port_groups_ref, &error); > const char *addr_set_name; > SSET_FOR_EACH (addr_set_name, &addr_sets_ref) { > lflow_resource_add(lfrr, REF_TYPE_ADDRSET, addr_set_name, > &lflow->header_.uuid); > } > + const char *port_group_name; > + SSET_FOR_EACH (port_group_name, &port_groups_ref) { > + lflow_resource_add(lfrr, REF_TYPE_PORTGROUP, port_group_name, > + &lflow->header_.uuid); > + } > sset_destroy(&addr_sets_ref); > + sset_destroy(&port_groups_ref); > > if (!error) { > if (prereqs) { > diff --git a/include/ovn/expr.h b/include/ovn/expr.h > index 22f633e..21bf51c 100644 > --- a/include/ovn/expr.h > +++ b/include/ovn/expr.h > @@ -390,11 +390,13 @@ void expr_print(const struct expr *); > struct expr *expr_parse(struct lexer *, const struct shash *symtab, > const struct shash *addr_sets, > const struct shash *port_groups, > - struct sset *addr_sets_ref); > + struct sset *addr_sets_ref, > + struct sset *port_groups_ref); > struct expr *expr_parse_string(const char *, const struct shash *symtab, > const struct shash *addr_sets, > const struct shash *port_groups, > struct sset *addr_sets_ref, > + struct sset *port_groups_ref, > char **errorp); > > struct expr *expr_clone(struct expr *); > diff --git a/lib/actions.c b/lib/actions.c > index 586d7b7..051e6c8 100644 > --- a/lib/actions.c > +++ b/lib/actions.c > @@ -240,8 +240,8 @@ add_prerequisite(struct action_context *ctx, const > char *prerequisite) > struct expr *expr; > char *error; > > - expr = expr_parse_string(prerequisite, ctx->pp->symtab, NULL, NULL, > NULL, > - &error); > + expr = expr_parse_string(prerequisite, ctx->pp->symtab, NULL, NULL, > + NULL, NULL, &error); > ovs_assert(!error); > ctx->prereqs = expr_combine(EXPR_T_AND, ctx->prereqs, expr); > } > diff --git a/lib/expr.c b/lib/expr.c > index 71de615..e5e4d21 100644 > --- a/lib/expr.c > +++ b/lib/expr.c > @@ -480,7 +480,8 @@ struct expr_context { > const struct shash *symtab; /* Symbol table. */ > const struct shash *addr_sets; /* Address set table. */ > const struct shash *port_groups; /* Port group table. */ > - struct sset *addr_sets_ref; /* The set of address set > referenced. */ > + struct sset *addr_sets_ref; /* The set of address set > referenced. */ > + struct sset *port_groups_ref; /* The set of port groups > referenced. */ > bool not; /* True inside odd number of NOT > operators. */ > unsigned int paren_depth; /* Depth of nested parentheses. */ > }; > @@ -782,6 +783,10 @@ static bool > parse_port_group(struct expr_context *ctx, struct expr_constant_set *cs, > size_t *allocated_values) > { > + if (ctx->port_groups_ref) { > + sset_add(ctx->port_groups_ref, ctx->lexer->token.s); > + } > + > struct expr_constant_set *port_group > = (ctx->port_groups > ? shash_find_data(ctx->port_groups, ctx->lexer->token.s) > @@ -1296,13 +1301,15 @@ struct expr * > expr_parse(struct lexer *lexer, const struct shash *symtab, > const struct shash *addr_sets, > const struct shash *port_groups, > - struct sset *addr_sets_ref) > + struct sset *addr_sets_ref, > + struct sset *port_groups_ref) > { > struct expr_context ctx = { .lexer = lexer, > .symtab = symtab, > .addr_sets = addr_sets, > .port_groups = port_groups, > - .addr_sets_ref = addr_sets_ref }; > + .addr_sets_ref = addr_sets_ref, > + .port_groups_ref = port_groups_ref }; > return lexer->error ? NULL : expr_parse__(&ctx); > } > > @@ -1317,6 +1324,7 @@ expr_parse_string(const char *s, const struct shash > *symtab, > const struct shash *addr_sets, > const struct shash *port_groups, > struct sset *addr_sets_ref, > + struct sset *port_groups_ref, > char **errorp) > { > struct lexer lexer; > @@ -1324,7 +1332,7 @@ expr_parse_string(const char *s, const struct shash > *symtab, > lexer_init(&lexer, s); > lexer_get(&lexer); > struct expr *expr = expr_parse(&lexer, symtab, addr_sets, port_groups, > - addr_sets_ref); > + addr_sets_ref, port_groups_ref); > lexer_force_end(&lexer); > *errorp = lexer_steal_error(&lexer); > if (*errorp) { > @@ -1550,7 +1558,8 @@ expr_get_level(const struct expr *expr) > static enum expr_level > expr_parse_level(const char *s, const struct shash *symtab, char **errorp) > { > - struct expr *expr = expr_parse_string(s, symtab, NULL, NULL, NULL, > errorp); > + struct expr *expr = expr_parse_string(s, symtab, NULL, NULL, NULL, > NULL, > + errorp); > enum expr_level level = expr ? expr_get_level(expr) : EXPR_L_NOMINAL; > expr_destroy(expr); > return level; > @@ -1721,7 +1730,7 @@ parse_and_annotate(const char *s, const struct shash > *symtab, > char *error; > struct expr *expr; > > - expr = expr_parse_string(s, symtab, NULL, NULL, NULL, &error); > + expr = expr_parse_string(s, symtab, NULL, NULL, NULL, NULL, &error); > if (expr) { > expr = expr_annotate_(expr, symtab, nesting, &error); > } > @@ -3445,7 +3454,8 @@ expr_parse_microflow(const char *s, const struct > shash *symtab, > lexer_init(&lexer, s); > lexer_get(&lexer); > > - struct expr *e = expr_parse(&lexer, symtab, addr_sets, port_groups, > NULL); > + struct expr *e = expr_parse(&lexer, symtab, addr_sets, port_groups, > + NULL, NULL); > lexer_force_end(&lexer); > > if (e) { > diff --git a/tests/test-ovn.c b/tests/test-ovn.c > index c16f9c5..5366905 100644 > --- a/tests/test-ovn.c > +++ b/tests/test-ovn.c > @@ -289,7 +289,7 @@ test_parse_expr__(int steps) > char *error; > > expr = expr_parse_string(ds_cstr(&input), &symtab, &addr_sets, > - &port_groups, NULL, &error); > + &port_groups, NULL, NULL, &error); > if (!error && steps > 0) { > expr = expr_annotate(expr, &symtab, &error); > } > @@ -413,8 +413,8 @@ test_evaluate_expr(struct ovs_cmdl_context *ctx) > while (!ds_get_test_line(&input, stdin)) { > struct expr *expr; > > - expr = expr_parse_string(ds_cstr(&input), &symtab, NULL, NULL, > NULL, > - &error); > + expr = expr_parse_string(ds_cstr(&input), &symtab, NULL, NULL, > + NULL, NULL, &error); > if (!error) { > expr = expr_annotate(expr, &symtab, &error); > } > @@ -889,7 +889,7 @@ test_tree_shape_exhaustively(struct expr *expr, struct > shash *symtab, > > char *error; > modified = expr_parse_string(ds_cstr(&s), symtab, NULL, > - NULL, NULL, &error); > + NULL, NULL, NULL, &error); > if (error) { > fprintf(stderr, "%s fails to parse (%s)\n", > ds_cstr(&s), error); > diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c > index 19b82e6..2645438 100644 > --- a/utilities/ovn-trace.c > +++ b/utilities/ovn-trace.c > @@ -866,7 +866,7 @@ read_flows(void) > char *error; > struct expr *match; > match = expr_parse_string(sblf->match, &symtab, &address_sets, > - &port_groups, NULL, &error); > + &port_groups, NULL, NULL, &error); > if (error) { > VLOG_WARN("%s: parsing expression failed (%s)", > sblf->match, error); > -- > 1.8.3.1 > > Tested-By: Daniel Alvarez <dalvarez@redhat.com> > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
diff --git a/controller/lflow.c b/controller/lflow.c index 36150bd..a689320 100644 --- a/controller/lflow.c +++ b/controller/lflow.c @@ -616,14 +616,21 @@ consider_logical_flow( struct expr *expr; struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref); + struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref); expr = expr_parse_string(lflow->match, &symtab, addr_sets, port_groups, - &addr_sets_ref, &error); + &addr_sets_ref, &port_groups_ref, &error); const char *addr_set_name; SSET_FOR_EACH (addr_set_name, &addr_sets_ref) { lflow_resource_add(lfrr, REF_TYPE_ADDRSET, addr_set_name, &lflow->header_.uuid); } + const char *port_group_name; + SSET_FOR_EACH (port_group_name, &port_groups_ref) { + lflow_resource_add(lfrr, REF_TYPE_PORTGROUP, port_group_name, + &lflow->header_.uuid); + } sset_destroy(&addr_sets_ref); + sset_destroy(&port_groups_ref); if (!error) { if (prereqs) { diff --git a/include/ovn/expr.h b/include/ovn/expr.h index 22f633e..21bf51c 100644 --- a/include/ovn/expr.h +++ b/include/ovn/expr.h @@ -390,11 +390,13 @@ void expr_print(const struct expr *); struct expr *expr_parse(struct lexer *, const struct shash *symtab, const struct shash *addr_sets, const struct shash *port_groups, - struct sset *addr_sets_ref); + struct sset *addr_sets_ref, + struct sset *port_groups_ref); struct expr *expr_parse_string(const char *, const struct shash *symtab, const struct shash *addr_sets, const struct shash *port_groups, struct sset *addr_sets_ref, + struct sset *port_groups_ref, char **errorp); struct expr *expr_clone(struct expr *); diff --git a/lib/actions.c b/lib/actions.c index 586d7b7..051e6c8 100644 --- a/lib/actions.c +++ b/lib/actions.c @@ -240,8 +240,8 @@ add_prerequisite(struct action_context *ctx, const char *prerequisite) struct expr *expr; char *error; - expr = expr_parse_string(prerequisite, ctx->pp->symtab, NULL, NULL, NULL, - &error); + expr = expr_parse_string(prerequisite, ctx->pp->symtab, NULL, NULL, + NULL, NULL, &error); ovs_assert(!error); ctx->prereqs = expr_combine(EXPR_T_AND, ctx->prereqs, expr); } diff --git a/lib/expr.c b/lib/expr.c index 71de615..e5e4d21 100644 --- a/lib/expr.c +++ b/lib/expr.c @@ -480,7 +480,8 @@ struct expr_context { const struct shash *symtab; /* Symbol table. */ const struct shash *addr_sets; /* Address set table. */ const struct shash *port_groups; /* Port group table. */ - struct sset *addr_sets_ref; /* The set of address set referenced. */ + struct sset *addr_sets_ref; /* The set of address set referenced. */ + struct sset *port_groups_ref; /* The set of port groups referenced. */ bool not; /* True inside odd number of NOT operators. */ unsigned int paren_depth; /* Depth of nested parentheses. */ }; @@ -782,6 +783,10 @@ static bool parse_port_group(struct expr_context *ctx, struct expr_constant_set *cs, size_t *allocated_values) { + if (ctx->port_groups_ref) { + sset_add(ctx->port_groups_ref, ctx->lexer->token.s); + } + struct expr_constant_set *port_group = (ctx->port_groups ? shash_find_data(ctx->port_groups, ctx->lexer->token.s) @@ -1296,13 +1301,15 @@ struct expr * expr_parse(struct lexer *lexer, const struct shash *symtab, const struct shash *addr_sets, const struct shash *port_groups, - struct sset *addr_sets_ref) + struct sset *addr_sets_ref, + struct sset *port_groups_ref) { struct expr_context ctx = { .lexer = lexer, .symtab = symtab, .addr_sets = addr_sets, .port_groups = port_groups, - .addr_sets_ref = addr_sets_ref }; + .addr_sets_ref = addr_sets_ref, + .port_groups_ref = port_groups_ref }; return lexer->error ? NULL : expr_parse__(&ctx); } @@ -1317,6 +1324,7 @@ expr_parse_string(const char *s, const struct shash *symtab, const struct shash *addr_sets, const struct shash *port_groups, struct sset *addr_sets_ref, + struct sset *port_groups_ref, char **errorp) { struct lexer lexer; @@ -1324,7 +1332,7 @@ expr_parse_string(const char *s, const struct shash *symtab, lexer_init(&lexer, s); lexer_get(&lexer); struct expr *expr = expr_parse(&lexer, symtab, addr_sets, port_groups, - addr_sets_ref); + addr_sets_ref, port_groups_ref); lexer_force_end(&lexer); *errorp = lexer_steal_error(&lexer); if (*errorp) { @@ -1550,7 +1558,8 @@ expr_get_level(const struct expr *expr) static enum expr_level expr_parse_level(const char *s, const struct shash *symtab, char **errorp) { - struct expr *expr = expr_parse_string(s, symtab, NULL, NULL, NULL, errorp); + struct expr *expr = expr_parse_string(s, symtab, NULL, NULL, NULL, NULL, + errorp); enum expr_level level = expr ? expr_get_level(expr) : EXPR_L_NOMINAL; expr_destroy(expr); return level; @@ -1721,7 +1730,7 @@ parse_and_annotate(const char *s, const struct shash *symtab, char *error; struct expr *expr; - expr = expr_parse_string(s, symtab, NULL, NULL, NULL, &error); + expr = expr_parse_string(s, symtab, NULL, NULL, NULL, NULL, &error); if (expr) { expr = expr_annotate_(expr, symtab, nesting, &error); } @@ -3445,7 +3454,8 @@ expr_parse_microflow(const char *s, const struct shash *symtab, lexer_init(&lexer, s); lexer_get(&lexer); - struct expr *e = expr_parse(&lexer, symtab, addr_sets, port_groups, NULL); + struct expr *e = expr_parse(&lexer, symtab, addr_sets, port_groups, + NULL, NULL); lexer_force_end(&lexer); if (e) { diff --git a/tests/test-ovn.c b/tests/test-ovn.c index c16f9c5..5366905 100644 --- a/tests/test-ovn.c +++ b/tests/test-ovn.c @@ -289,7 +289,7 @@ test_parse_expr__(int steps) char *error; expr = expr_parse_string(ds_cstr(&input), &symtab, &addr_sets, - &port_groups, NULL, &error); + &port_groups, NULL, NULL, &error); if (!error && steps > 0) { expr = expr_annotate(expr, &symtab, &error); } @@ -413,8 +413,8 @@ test_evaluate_expr(struct ovs_cmdl_context *ctx) while (!ds_get_test_line(&input, stdin)) { struct expr *expr; - expr = expr_parse_string(ds_cstr(&input), &symtab, NULL, NULL, NULL, - &error); + expr = expr_parse_string(ds_cstr(&input), &symtab, NULL, NULL, + NULL, NULL, &error); if (!error) { expr = expr_annotate(expr, &symtab, &error); } @@ -889,7 +889,7 @@ test_tree_shape_exhaustively(struct expr *expr, struct shash *symtab, char *error; modified = expr_parse_string(ds_cstr(&s), symtab, NULL, - NULL, NULL, &error); + NULL, NULL, NULL, &error); if (error) { fprintf(stderr, "%s fails to parse (%s)\n", ds_cstr(&s), error); diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c index 19b82e6..2645438 100644 --- a/utilities/ovn-trace.c +++ b/utilities/ovn-trace.c @@ -866,7 +866,7 @@ read_flows(void) char *error; struct expr *match; match = expr_parse_string(sblf->match, &symtab, &address_sets, - &port_groups, NULL, &error); + &port_groups, NULL, NULL, &error); if (error) { VLOG_WARN("%s: parsing expression failed (%s)", sblf->match, error);
The commit that adds incremental processing for port-group changes doesn't store logical flow references for port groups. If a port group is updated (e.g., a port is added) no logical flow recalculation will be performed. To fix this, when parsing the flow expression also store the referenced port groups and bind them to the logical flows that depend on them. If the port group is updated then the logical flows referring them will also be reinstalled. Reported-by: Daniel Alvarez <dalvarez@redhat.com> Reported-at: https://bugzilla.redhat.com/1778164 CC: Han Zhou <hzhou@ovn.org> Fixes: 978f5e90af0a ("ovn-controller: Incremental processing for port-group changes.") Signed-off-by: Dumitru Ceara <dceara@redhat.com> Change-Id: Id39695f022f16b598fd8416cd2494e7dab3bf11b --- controller/lflow.c | 9 ++++++++- include/ovn/expr.h | 4 +++- lib/actions.c | 4 ++-- lib/expr.c | 24 +++++++++++++++++------- tests/test-ovn.c | 8 ++++---- utilities/ovn-trace.c | 2 +- 6 files changed, 35 insertions(+), 16 deletions(-)