Merge pull request #763 from ellemouton/fixLocalTempStoreBucket

firewalldb: fetch correct bucket for session level temp KV store

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
 }
 
@@ -268,7 +277,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,

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 .