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.
> +
> +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
