| Message ID | 20240222163711.362897-1-mkp@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2] ovsdb: Don't iterate over rows on empty mutation. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | fail | test: fail |
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 <twilson@redhat.com> > Reported-at: https://issues.redhat.com/browse/FDP-359 > Signed-off-by: Mike Pattrick <mkp@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.
On Wed, Feb 28, 2024 at 8:41 AM Ilya Maximets <i.maximets@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 <twilson@redhat.com> > > Reported-at: https://issues.redhat.com/browse/FDP-359 > > Signed-off-by: Mike Pattrick <mkp@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. >
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; +}; + +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.
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 <twilson@redhat.com> Reported-at: https://issues.redhat.com/browse/FDP-359 Signed-off-by: Mike Pattrick <mkp@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(-)