On Wed, Feb 28, 2024 at 8:41 AM Ilya Maximets <i.maxim...@ovn.org> wrote:
>
> On 2/22/24 17:37, Mike Pattrick wrote:
> > Previously when an empty mutation was used to count the number of rows
> > in a table, OVSDB would iterate over all rows twice. First to perform an
> > RBAC check, and then to perform the no-operation.
> >
> > This change adds a short circuit to mutate operations with no conditions
> > and an empty mutation set, returning immediately. One notable change in
> > functionality is not performing the RBAC check in this condition, as no
> > mutation actually takes place.
> >
> > Reported-by: Terry Wilson <twil...@redhat.com>
> > Reported-at: https://issues.redhat.com/browse/FDP-359
> > Signed-off-by: Mike Pattrick <m...@redhat.com>
> > ---
> > v2: Added additional non-rbac tests, and support for conditional
> > counting without the rbac check
> > ---
> > ovsdb/execution.c | 26 +++++++++++++++++++-
> > ovsdb/mutation.h | 6 +++++
> > tests/ovsdb-execution.at | 51 ++++++++++++++++++++++++++++++++++++++++
> > tests/ovsdb-rbac.at | 23 ++++++++++++++++++
> > 4 files changed, 105 insertions(+), 1 deletion(-)
>
> Hi, Mike. Thanks for v2! I didn't test, but it looks good in general.
> See one comment inline.
>
> Best regards, Ilya Maximets.
>
> >
> > diff --git a/ovsdb/execution.c b/ovsdb/execution.c
> > index 8c20c3b54..7ed700632 100644
> > --- a/ovsdb/execution.c
> > +++ b/ovsdb/execution.c
> > @@ -585,6 +585,19 @@ mutate_row_cb(const struct ovsdb_row *row, void *mr_)
> > return *mr->error == NULL;
> > }
> >
> > +struct count_row_cbdata {
> > + size_t n_matches;
> > +};
>
> Do we actually need this structure? It only has one element.
> We should be able to just pass a counter around directly.
It seemed more thematic to me at the time, but I can change this.
-M
>
> > +
> > +static bool
> > +count_row_cb(const struct ovsdb_row *row OVS_UNUSED, void *cr_)
> > +{
> > + struct count_row_cbdata *cr = cr_;
> > +
> > + cr->n_matches++;
> > + return true;
> > +}
> > +
> > static struct ovsdb_error *
> > ovsdb_execute_mutate(struct ovsdb_execution *x, struct ovsdb_parser
> > *parser,
> > struct json *result)
> > @@ -609,7 +622,18 @@ ovsdb_execute_mutate(struct ovsdb_execution *x, struct
> > ovsdb_parser *parser,
> > error = ovsdb_condition_from_json(table->schema, where, x->symtab,
> > &condition);
> > }
> > - if (!error) {
> > + if (!error && ovsdb_mutation_set_empty(&mutations)) {
> > + /* Special case with no mutations, just return the row count. */
> > + if (ovsdb_condition_empty(&condition)) {
> > + json_object_put(result, "count",
> > + json_integer_create(hmap_count(&table->rows)));
> > + } else {
> > + struct count_row_cbdata cr = {};
> > + ovsdb_query(table, &condition, count_row_cb, &cr);
> > + json_object_put(result, "count",
> > + json_integer_create(cr.n_matches));
> > + }
> > + } else if (!error) {
> > mr.n_matches = 0;
> > mr.txn = x->txn;
> > mr.mutations = &mutations;
> > diff --git a/ovsdb/mutation.h b/ovsdb/mutation.h
> > index 7566ef199..05d4a262a 100644
> > --- a/ovsdb/mutation.h
> > +++ b/ovsdb/mutation.h
> > @@ -69,4 +69,10 @@ void ovsdb_mutation_set_destroy(struct
> > ovsdb_mutation_set *);
> > struct ovsdb_error *ovsdb_mutation_set_execute(
> > struct ovsdb_row *, const struct ovsdb_mutation_set *)
> > OVS_WARN_UNUSED_RESULT;
> >
> > +static inline bool ovsdb_mutation_set_empty(
> > + const struct ovsdb_mutation_set *ms)
> > +{
> > + return ms->n_mutations == 0;
> > +}
> > +
> > #endif /* ovsdb/mutation.h */
> > diff --git a/tests/ovsdb-execution.at b/tests/ovsdb-execution.at
> > index fd1c7a239..1ffa2b738 100644
> > --- a/tests/ovsdb-execution.at
> > +++ b/tests/ovsdb-execution.at
> > @@ -1201,4 +1201,55 @@ OVSDB_CHECK_EXECUTION([garbage collection],
> > [{"rows":[]}]
> > ]])])
> >
> > +OVSDB_CHECK_EXECUTION([insert rows, count with mutation],
> > + [ordinal_schema],
> > + [[[["ordinals",
> > + {"op": "insert",
> > + "table": "ordinals",
> > + "row": {"number": 0, "name": "zero"},
> > + "uuid-name": "first"}]]],
> > + [[["ordinals",
> > + {"op": "insert",
> > + "table": "ordinals",
> > + "row": {"number": 1, "name": "one"},
> > + "uuid-name": "first"}]]],
> > + [[["ordinals",
> > + {"op": "mutate",
> > + "table": "ordinals",
> > + "where": [["name", "==", "zero"]],
> > + "mutations": []}]]],
> > + [[["ordinals",
> > + {"op": "mutate",
> > + "table": "ordinals",
> > + "where": [["name", "==", "one"]],
> > + "mutations": []}]]],
> > + [[["ordinals",
> > + {"op": "insert",
> > + "table": "ordinals",
> > + "row": {"number": 2, "name": "one"},
> > + "uuid-name": "first"}]]],
> > + [[["ordinals",
> > + {"op": "mutate",
> > + "table": "ordinals",
> > + "where": [["name", "==", "one"]],
> > + "mutations": []}]]],
> > + [[["ordinals",
> > + {"op": "delete",
> > + "table": "ordinals",
> > + "where": [["name", "==", "zero"]]}]]],
> > + [[["ordinals",
> > + {"op": "mutate",
> > + "table": "ordinals",
> > + "where": [],
> > + "mutations": []}]]]],
> > + [[[{"uuid":["uuid","<0>"]}]
> > +[{"uuid":["uuid","<1>"]}]
> > +[{"count":1}]
> > +[{"count":1}]
> > +[{"uuid":["uuid","<2>"]}]
> > +[{"count":2}]
> > +[{"count":1}]
> > +[{"count":2}]
> > +]])
> > +
> > EXECUTION_EXAMPLES
> > diff --git a/tests/ovsdb-rbac.at b/tests/ovsdb-rbac.at
> > index 3172e4bf5..c1e5a9134 100644
> > --- a/tests/ovsdb-rbac.at
> > +++ b/tests/ovsdb-rbac.at
> > @@ -355,6 +355,29 @@ AT_CHECK([uuidfilt stdout], [0], [[[{"details":"RBAC
> > rules for client \"client-2
> > ], [ignore])
> >
> > # Test 14:
> > +# Count the rows in other_colors. This should pass even though the RBAC
> > +# authorization would fail because "client-2" does not match the
> > +# "creator" column for this row. Because the RBAC check is bypassed when
> > +# mutation is empty.
> > +AT_CHECK([ovsdb-client transact ssl:127.0.0.1:$SSL_PORT \
> > + --private-key=$RBAC_PKIDIR/client-2-privkey.pem \
> > + --certificate=$RBAC_PKIDIR/client-2-cert.pem \
> > + --ca-cert=$RBAC_PKIDIR/pki/switchca/cacert.pem \
> > + ['["mydb",
> > + {"op": "mutate",
> > + "table": "other_colors",
> > + "where": [],
> > + "mutations": []},
> > + {"op": "mutate",
> > + "table": "other_colors",
> > + "where": [["name", "==", "seafoam"]],
> > + "mutations": []}
> > + ]']], [0], [stdout], [ignore])
> > +cat stdout >> output
> > +AT_CHECK([uuidfilt stdout], [0], [[[{"count":1},{"count":1}]]
> > +], [ignore])
> > +
> > +# Test 15:
> > # Attempt to delete a row from the "other_colors" table. This should pass
> > # the RBAC authorization test because "client-1" does matches the
> > # "creator" column for this row.
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
