From 6f33893454567d913919340855bdd99727b3544c Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Wed, 29 May 2024 15:31:36 -0400 Subject: [PATCH 1/2] firewalldb: fetch correct bucket for session level temp KV store In this commit, we fix a bug which would result in the "perm" store being used for the LocalTemp KV DB _if and only if_ the KV store being requested was a session level one (as opposed to a session feature level one). It is ok for us to just change this here since as of this commit, the session level KV store is not used at all and so nothing will be in that store yet. --- firewalldb/kvstores.go | 2 +- firewalldb/kvstores_test.go | 27 ++++++++++++++++++++++++--- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/firewalldb/kvstores.go b/firewalldb/kvstores.go index 76e4b9d88..86213bd4f 100644 --- a/firewalldb/kvstores.go +++ b/firewalldb/kvstores.go @@ -268,7 +268,7 @@ func (tx *kvStoreTx) GlobalTemp() KVStore { // // NOTE: this is part of the KVStoreTx interface. func (tx *kvStoreTx) LocalTemp() KVStore { - fn := getSessionRuleBucket(true, tx.ruleName, tx.groupID) + fn := getSessionRuleBucket(false, tx.ruleName, tx.groupID) if tx.featureName != "" { fn = getSessionFeatureRuleBucket( false, tx.ruleName, tx.groupID, tx.featureName, diff --git a/firewalldb/kvstores_test.go b/firewalldb/kvstores_test.go index 607dc233a..86f650f76 100644 --- a/firewalldb/kvstores_test.go +++ b/firewalldb/kvstores_test.go @@ -60,18 +60,39 @@ func TestKVStoreTxs(t *testing.T) { // TestTempAndPermStores tests that the kv stores stored under the `temp` bucket // are properly deleted and re-initialised upon restart but that anything under -// the `perm` bucket is retained. +// the `perm` bucket is retained. We repeat the test for both the session level +// KV stores and the session feature level stores. func TestTempAndPermStores(t *testing.T) { + t.Run("session level kv store", func(t *testing.T) { + testTempAndPermStores(t, false) + }) + + t.Run("session feature level kv store", func(t *testing.T) { + testTempAndPermStores(t, true) + }) +} + +// testTempAndPermStores tests that the kv stores stored under the `temp` bucket +// are properly deleted and re-initialised upon restart but that anything under +// the `perm` bucket is retained. If featureSpecificStore is true, then this +// will test the session feature level KV stores. Otherwise, it will test the +// session level KV stores. +func testTempAndPermStores(t *testing.T, featureSpecificStore bool) { ctx := context.Background() tmpDir := t.TempDir() + var featureName string + if featureSpecificStore { + featureName = "auto-fees" + } + db, err := NewDB(tmpDir, "test.db", nil) require.NoError(t, err) t.Cleanup(func() { _ = db.Close() }) - store := db.GetKVStores("test-rule", [4]byte{1, 1, 1, 1}, "auto-fees") + store := db.GetKVStores("test-rule", [4]byte{1, 1, 1, 1}, featureName) err = store.Update(func(tx KVStoreTx) error { // Set an item in the temp store. @@ -119,7 +140,7 @@ func TestTempAndPermStores(t *testing.T) { _ = db.Close() _ = os.RemoveAll(tmpDir) }) - store = db.GetKVStores("test-rule", [4]byte{1, 1, 1, 1}, "auto-fees") + store = db.GetKVStores("test-rule", [4]byte{1, 1, 1, 1}, featureName) // The temp store should no longer have the stored value but the perm // store should . From 0259fc852fd472cb31e5abc46b41c244b46c16f8 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Thu, 30 May 2024 09:52:43 -0400 Subject: [PATCH 2/2] firewalldb: add comment about reasoning behind persistent temp KV stores --- firewalldb/kvstores.go | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/firewalldb/kvstores.go b/firewalldb/kvstores.go index 86213bd4f..5b653a200 100644 --- a/firewalldb/kvstores.go +++ b/firewalldb/kvstores.go @@ -9,8 +9,11 @@ import ( /* The KVStores are stored in the following structure in the KV db. Note that -the `perm` and `temp` buckets are identical in structure. The only difference is -that the `temp` bucket is cleared on restart of the db. +the `perm` and `temp` buckets are identical in structure. The only difference +is that the `temp` bucket is cleared on restart of the db. The reason persisting +the temporary store changes instead of just keeping an in-memory store is that +we can then guarantee idempotency if changes are made to both the permanent and +temporary stores. rules -> perm -> rule-name -> global -> {k:v} -> sessions -> group ID -> session-kv-store -> {k:v} @@ -81,11 +84,17 @@ type KVStoreTx interface { Local() KVStore // GlobalTemp is similar to the Global store except that its contents - // is cleared upon restart of the database. + // is cleared upon restart of the database. The reason persisting the + // temporary store changes instead of just keeping an in-memory store is + // that we can then guarantee idempotency if changes are made to both + // the permanent and temporary stores. GlobalTemp() KVStore // LocalTemp is similar to the Local store except that its contents is - // cleared upon restart of the database. + // cleared upon restart of the database. The reason persisting the + // temporary store changes instead of just keeping an in-memory store is + // that we can then guarantee idempotency if changes are made to both + // the permanent and temporary stores. LocalTemp() KVStore }