Re: [ovs-dev] [PATCH] ovsdb: Don't iterate over rows on empty mutation.
On 2/5/24 06:10, 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
> Reported-at: https://issues.redhat.com/browse/FDP-359
> Signed-off-by: Mike Pattrick
> ---
> ovsdb/execution.c | 9 -
> ovsdb/mutation.h| 5 +
> tests/ovsdb-rbac.at | 19 +++
> 3 files changed, 32 insertions(+), 1 deletion(-)
Hi, Mike. Thanks for the patch!
See some comments inline.
>
> diff --git a/ovsdb/execution.c b/ovsdb/execution.c
> index 8c20c3b54..26bc49641 100644
> --- a/ovsdb/execution.c
> +++ b/ovsdb/execution.c
> @@ -609,7 +609,14 @@ ovsdb_execute_mutate(struct ovsdb_execution *x, struct
> ovsdb_parser *parser,
> error = ovsdb_condition_from_json(table->schema, where, x->symtab,
>);
> }
> -if (!error) {
> +if (!error &&
> +ovsdb_condition_empty() &&
> +ovsdb_mutation_set_empty()) {
> +/* Special case with no conditions or mutations, just return the row
> + * count. */
> +json_object_put(result, "count",
> +json_integer_create(hmap_count(>rows)));
> +} else if (!error) {
> mr.n_matches = 0;
> mr.txn = x->txn;
> mr.mutations =
> diff --git a/ovsdb/mutation.h b/ovsdb/mutation.h
> index 7566ef199..3989c7b8a 100644
> --- a/ovsdb/mutation.h
> +++ b/ovsdb/mutation.h
> @@ -68,5 +68,10 @@ struct json *ovsdb_mutation_set_to_json(const struct
> ovsdb_mutation_set *);
> 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;
An empty line would be nice.
> +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-rbac.at b/tests/ovsdb-rbac.at
> index 3172e4bf5..741651723 100644
> --- a/tests/ovsdb-rbac.at
> +++ b/tests/ovsdb-rbac.at
> @@ -355,6 +355,25 @@ 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
> +# where and mutations are both empty.
While I understand why you did that ("select" operation doesn't require
RBAC checks and an empty mutation is similar to selection), this introduces
an unclear difference between empty mutations with and without condition.
One performs RBAC check and the other does not. We should be consistent.
Would be also nice to have a non-RBAC test that checks the number of rows
mid-transaction, i.e. adds a row, runs empty mutation and removes some other
row after that. Somewhere in tests/ovsdb-execution.at. I don't think we
have any tests with empty mutations list.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] ovsdb: Don't iterate over rows on empty mutation.
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
Reported-at: https://issues.redhat.com/browse/FDP-359
Signed-off-by: Mike Pattrick
---
ovsdb/execution.c | 9 -
ovsdb/mutation.h| 5 +
tests/ovsdb-rbac.at | 19 +++
3 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/ovsdb/execution.c b/ovsdb/execution.c
index 8c20c3b54..26bc49641 100644
--- a/ovsdb/execution.c
+++ b/ovsdb/execution.c
@@ -609,7 +609,14 @@ ovsdb_execute_mutate(struct ovsdb_execution *x, struct
ovsdb_parser *parser,
error = ovsdb_condition_from_json(table->schema, where, x->symtab,
);
}
-if (!error) {
+if (!error &&
+ovsdb_condition_empty() &&
+ovsdb_mutation_set_empty()) {
+/* Special case with no conditions or mutations, just return the row
+ * count. */
+json_object_put(result, "count",
+json_integer_create(hmap_count(>rows)));
+} else if (!error) {
mr.n_matches = 0;
mr.txn = x->txn;
mr.mutations =
diff --git a/ovsdb/mutation.h b/ovsdb/mutation.h
index 7566ef199..3989c7b8a 100644
--- a/ovsdb/mutation.h
+++ b/ovsdb/mutation.h
@@ -68,5 +68,10 @@ struct json *ovsdb_mutation_set_to_json(const struct
ovsdb_mutation_set *);
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-rbac.at b/tests/ovsdb-rbac.at
index 3172e4bf5..741651723 100644
--- a/tests/ovsdb-rbac.at
+++ b/tests/ovsdb-rbac.at
@@ -355,6 +355,25 @@ 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
+# where and mutations are both 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": []}
+ ]']], [0], [stdout], [ignore])
+cat stdout >> output
+AT_CHECK([uuidfilt stdout], [0], [[[{"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.
--
2.39.3
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
