diff mbox series

[ovs-dev,2/6] ovsdb: Check for ephemeral columns before writing a new schema.

Message ID 20230327194301.3773536-3-i.maximets@ovn.org
State Accepted
Commit a73b0206ba6f3991ac1550c7c07f11fa4237a898
Headers show
Series ovsdb: conversion: Bug fixes & Optimizations. | expand

Checks

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 success test: success

Commit Message

Ilya Maximets March 27, 2023, 7:42 p.m. UTC 
Clustered databases do not support ephemeral columns, but ovsdb-server
checks for them after the conversion result is read from the storage.
It's much easier to recover if this constraint is checked before writing
to the storage instead.

It's not a big problem, because the check is always performed by the
native ovsdb clients before sending a conversion request.  But the
server, in general, should not trust clients to do the right thing.

Check in the update_schema() remains, because we shouldn't blindly
trust the storage.

Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 ovsdb/storage.c     | 24 +++++++++++++++++-------
 ovsdb/storage.h     |  2 +-
 ovsdb/transaction.c |  2 +-
 ovsdb/transaction.h |  3 ++-
 ovsdb/trigger.c     |  5 +++--
 5 files changed, 24 insertions(+), 12 deletions(-)

Comments

Simon Horman March 31, 2023, 11:54 a.m. UTC | #1 
On Mon, Mar 27, 2023 at 09:42:57PM +0200, Ilya Maximets wrote:
> Clustered databases do not support ephemeral columns, but ovsdb-server
> checks for them after the conversion result is read from the storage.
> It's much easier to recover if this constraint is checked before writing
> to the storage instead.
> 
> It's not a big problem, because the check is always performed by the
> native ovsdb clients before sending a conversion request.  But the
> server, in general, should not trust clients to do the right thing.
> 
> Check in the update_schema() remains, because we shouldn't blindly
> trust the storage.
> 
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

Reviewed-by: Simon Horman <simon.horman@corigine.com>
Dumitru Ceara April 6, 2023, 8:20 a.m. UTC | #2 
On 3/27/23 21:42, Ilya Maximets wrote:
> Clustered databases do not support ephemeral columns, but ovsdb-server
> checks for them after the conversion result is read from the storage.
> It's much easier to recover if this constraint is checked before writing
> to the storage instead.
> 
> It's not a big problem, because the check is always performed by the
> native ovsdb clients before sending a conversion request.  But the
> server, in general, should not trust clients to do the right thing.
> 
> Check in the update_schema() remains, because we shouldn't blindly
> trust the storage.
> 
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> ---

Looks good to me, thanks!

Acked-by: Dumitru Ceara <dceara@redhat.com>
diff mbox series

Patch

diff --git a/ovsdb/storage.c b/ovsdb/storage.c
index e8f95ce64..6c395106c 100644
--- a/ovsdb/storage.c
+++ b/ovsdb/storage.c
@@ -623,7 +623,7 @@  ovsdb_storage_store_snapshot(struct ovsdb_storage *storage,
 
 struct ovsdb_write * OVS_WARN_UNUSED_RESULT
 ovsdb_storage_write_schema_change(struct ovsdb_storage *storage,
-                                  const struct json *schema,
+                                  const struct ovsdb_schema *schema,
                                   const struct json *data,
                                   const struct uuid *prereq,
                                   struct uuid *resultp)
@@ -633,13 +633,23 @@  ovsdb_storage_write_schema_change(struct ovsdb_storage *storage,
     if (storage->error) {
         w->error = ovsdb_error_clone(storage->error);
     } else if (storage->raft) {
-        struct json *txn_json = json_array_create_2(json_clone(schema),
-                                                    json_clone(data));
-        w->command = raft_command_execute(storage->raft, txn_json,
-                                          prereq, &result);
-        json_destroy(txn_json);
+        /* Clustered storage doesn't support ephemeral columns. */
+        w->error = ovsdb_schema_check_for_ephemeral_columns(schema);
+        if (!w->error) {
+            struct json *schema_json, *txn_json;
+
+            schema_json = ovsdb_schema_to_json(schema);
+            txn_json = json_array_create_2(schema_json, json_clone(data));
+            w->command = raft_command_execute(storage->raft, txn_json,
+                                              prereq, &result);
+            json_destroy(txn_json);
+        }
     } else if (storage->log) {
-        w->error = ovsdb_storage_store_snapshot__(storage, schema, data, 0);
+        struct json *schema_json = ovsdb_schema_to_json(schema);
+
+        w->error = ovsdb_storage_store_snapshot__(storage, schema_json,
+                                                  data, 0);
+        json_destroy(schema_json);
     } else {
         /* When 'error' and 'command' are both null, it indicates that the
          * command is complete.  This is fine since this unbacked storage drops
diff --git a/ovsdb/storage.h b/ovsdb/storage.h
index a1fdaa564..05f40ce93 100644
--- a/ovsdb/storage.h
+++ b/ovsdb/storage.h
@@ -85,7 +85,7 @@  struct ovsdb_error *ovsdb_storage_store_snapshot(struct ovsdb_storage *storage,
 
 struct ovsdb_write *ovsdb_storage_write_schema_change(
     struct ovsdb_storage *,
-    const struct json *schema, const struct json *data,
+    const struct ovsdb_schema *, const struct json *data,
     const struct uuid *prereq, struct uuid *result)
     OVS_WARN_UNUSED_RESULT;
 
diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c
index 03541af85..f01de2a34 100644
--- a/ovsdb/transaction.c
+++ b/ovsdb/transaction.c
@@ -1251,7 +1251,7 @@  ovsdb_txn_precheck_prereq(const struct ovsdb *db)
 
 struct ovsdb_txn_progress *
 ovsdb_txn_propose_schema_change(struct ovsdb *db,
-                                const struct json *schema,
+                                const struct ovsdb_schema *schema,
                                 const struct json *data)
 {
     struct ovsdb_txn_progress *progress = xzalloc(sizeof *progress);
diff --git a/ovsdb/transaction.h b/ovsdb/transaction.h
index 6b5bb7f24..9991f34d2 100644
--- a/ovsdb/transaction.h
+++ b/ovsdb/transaction.h
@@ -21,6 +21,7 @@ 
 
 struct json;
 struct ovsdb;
+struct ovsdb_schema;
 struct ovsdb_table;
 struct uuid;
 
@@ -41,7 +42,7 @@  struct ovsdb_error *ovsdb_txn_propose_commit_block(struct ovsdb_txn *,
 void ovsdb_txn_complete(struct ovsdb_txn *);
 
 struct ovsdb_txn_progress *ovsdb_txn_propose_schema_change(
-    struct ovsdb *, const struct json *schema, const struct json *data);
+    struct ovsdb *, const struct ovsdb_schema *, const struct json *data);
 
 bool ovsdb_txn_progress_is_complete(const struct ovsdb_txn_progress *);
 const struct ovsdb_error *ovsdb_txn_progress_get_error(
diff --git a/ovsdb/trigger.c b/ovsdb/trigger.c
index 01bb80e28..3c93ae580 100644
--- a/ovsdb/trigger.c
+++ b/ovsdb/trigger.c
@@ -274,8 +274,8 @@  ovsdb_trigger_try(struct ovsdb_trigger *t, long long int now)
             if (!error) {
                 error = ovsdb_convert(t->db, new_schema, &newdb);
             }
-            ovsdb_schema_destroy(new_schema);
             if (error) {
+                ovsdb_schema_destroy(new_schema);
                 trigger_convert_error(t, error);
                 return false;
             }
@@ -286,7 +286,8 @@  ovsdb_trigger_try(struct ovsdb_trigger *t, long long int now)
 
             /* Propose the change. */
             t->progress = ovsdb_txn_propose_schema_change(
-                t->db, new_schema_json, txn_json);
+                t->db, new_schema, txn_json);
+            ovsdb_schema_destroy(new_schema);
             json_destroy(txn_json);
             t->reply = jsonrpc_create_reply(json_object_create(),
                                             t->request->id);