firewalldb: handle deleted sessions in kv stores mig

If the user has deleted their session.db file, but kept their
rules.db file, there can exist kv entry values that point to a now
deleted session ID. Such kv entries should be ignored during the
migration, as they are cannot be used anymore.

This commit updates the migration to handle this case.

Author: ViktorT-11

firewalldb/sql_migration.go

@@ -139,7 +139,7 @@ func migrateKVStoresDBToSQL(ctx context.Context, kvStore *bbolt.DB,
 	// 1) Collect all key-value pairs from the KV store.
 	err := kvStore.View(func(tx *bbolt.Tx) error {
 		var err error
-		pairs, err = collectAllPairs(tx)
+		pairs, err = collectAllPairs(sessMap, tx)
 		return err
 	})
 	if err != nil {
@@ -198,7 +198,9 @@ func migrateKVStoresDBToSQL(ctx context.Context, kvStore *bbolt.DB,
 // designed to iterate over all buckets and values that exist in the KV store.
 // That ensures that we find all stores and values that exist in the KV store,
 // and can be sure that the kv store actually follows the expected structure.
-func collectAllPairs(tx *bbolt.Tx) ([]*kvEntry, error) {
+func collectAllPairs(sessMap map[[4]byte]sqlc.Session,
+	tx *bbolt.Tx) ([]*kvEntry, error) {
+
 	var entries []*kvEntry
 	for _, perm := range []bool{true, false} {
 		mainBucket, err := getMainBucket(tx, false, perm)
@@ -228,7 +230,7 @@ func collectAllPairs(tx *bbolt.Tx) ([]*kvEntry, error) {
 			}
 
 			pairs, err := collectRulePairs(
-				ruleBucket, perm, string(rule),
+				sessMap, ruleBucket, perm, string(rule),
 			)
 			if err != nil {
 				return err
@@ -248,8 +250,8 @@ func collectAllPairs(tx *bbolt.Tx) ([]*kvEntry, error) {
 
 // collectRulePairs processes a single rule bucket, which should contain the
 // global and session-kv-store key buckets.
-func collectRulePairs(bkt *bbolt.Bucket, perm bool, rule string) ([]*kvEntry,
-	error) {
+func collectRulePairs(sessMap map[[4]byte]sqlc.Session, bkt *bbolt.Bucket,
+	perm bool, rule string) ([]*kvEntry, error) {
 
 	var params []*kvEntry
 
@@ -281,6 +283,25 @@ func collectRulePairs(bkt *bbolt.Bucket, perm bool, rule string) ([]*kvEntry,
 					"under %s bucket", sessKVStoreBucketKey)
 			}
 
+			var alias [4]byte
+			copy(alias[:], groupAlias)
+			if _, ok := sessMap[alias]; !ok {
+				// If we can't find the session group in the
+				// SQL db, that indicates that the session was
+				// never migrated from KVDB. This likely means
+				// that the user deleted their session.db file,
+				// but kept the rules.db file. As the KV entries
+				// are useless when the session no longer
+				// exists, we can just skip the migration of the
+				// KV entries for this group.
+				log.Warnf("Skipping migration of KV store "+
+					"entries for session group %x, as the "+
+					"session group was not found",
+					groupAlias)
+
+				return nil
+			}
+
 			groupBucket := sessBkt.Bucket(groupAlias)
 			if groupBucket == nil {
 				return fmt.Errorf("group bucket for group "+
@@ -413,6 +434,9 @@ func insertPair(ctx context.Context, tx SQLQueries,
 
 		sess, ok := sessMap[groupAlias]
 		if !ok {
+			// This should be unreachable, as we check for the
+			// existence of the session group when collecting
+			// the kv pairs.
 			err = fmt.Errorf("session group %x not found in map",
 				alias)
 		}

firewalldb/sql_migration_test.go

@@ -418,10 +418,26 @@ func TestFirewallDBMigration(t *testing.T) {
 			name:       "session specific kv entries",
 			populateDB: sessionSpecificEntries,
 		},
+		{
+			name:       "session specific kv entries deleted session",
+			populateDB: sessionSpecificEntriesDeletedSession,
+		},
+		{
+			name:       "session specific kv entries deleted and existing sessions",
+			populateDB: sessionSpecificEntriesDeletedAndExistingSessions,
+		},
 		{
 			name:       "feature specific kv entries",
 			populateDB: featureSpecificEntries,
 		},
+		{
+			name:       "feature specific kv entries deleted session",
+			populateDB: featureSpecificEntriesDeletedSession,
+		},
+		{
+			name:       "feature specific kv entries deleted and existing sessions",
+			populateDB: featureSpecificEntriesDeletedAndExistingSessions,
+		},
 		{
 			name:       "all kv entry combinations",
 			populateDB: allEntryCombinations,
@@ -585,6 +601,61 @@ func sessionSpecificEntries(t *testing.T, ctx context.Context, boltDB *BoltDB,
 	)
 }
 
+// sessionSpecificEntriesDeletedSession populates the kv store with one session
+// specific entry for the local temp store, and one session specific entry for
+// the perm local store. Once populated, the session that the entries are linked
+// to is deleted. When migrating, we therefore expect that the kv entries linked
+// to the deleted session are not migrated.
+func sessionSpecificEntriesDeletedSession(t *testing.T, ctx context.Context,
+	boltDB *BoltDB, sessionStore session.Store, _ accounts.Store,
+	_ *rootKeyMockStore) *expectedResult {
+
+	groupAlias := getNewSessionAlias(t, ctx, sessionStore)
+
+	_ = insertTempAndPermEntry(
+		t, ctx, boltDB, testRuleName, groupAlias, fn.None[string](),
+		testEntryKey, testEntryValue,
+	)
+
+	err := sessionStore.DeleteReservedSessions(ctx)
+	require.NoError(t, err)
+
+	return &expectedResult{
+		// Since the session the kx entries were linked to has been
+		// deleted, we expect no kv entries to be migrated.
+		kvEntries: []*kvEntry{},
+		privPairs: make(privacyPairs),
+		actions:   []*Action{},
+	}
+}
+
+// sessionSpecificEntriesDeletedAndExistingSessions populates the kv store with
+// two sessions and their corresponding kv entries.
+// One of the sessions is deleted prior to the migration though, and therefore
+// the migration should only migrate the kv entries linked to the still existing
+// session.
+func sessionSpecificEntriesDeletedAndExistingSessions(t *testing.T,
+	ctx context.Context, boltDB *BoltDB, sessionStore session.Store,
+	_ accounts.Store, _ *rootKeyMockStore) *expectedResult {
+
+	groupAlias1 := getNewSessionAlias(t, ctx, sessionStore)
+
+	_ = insertTempAndPermEntry(
+		t, ctx, boltDB, testRuleName, groupAlias1, fn.None[string](),
+		testEntryKey, testEntryValue,
+	)
+
+	err := sessionStore.DeleteReservedSessions(ctx)
+	require.NoError(t, err)
+
+	groupAlias2 := getNewSessionAlias(t, ctx, sessionStore)
+
+	return insertTempAndPermEntry(
+		t, ctx, boltDB, testRuleName2, groupAlias2, fn.None[string](),
+		testEntryKey2, testEntryValue,
+	)
+}
+
 // featureSpecificEntries populates the kv store with one feature specific
 // entry for the local temp store, and one feature specific entry for the perm
 // local store.
@@ -600,6 +671,61 @@ func featureSpecificEntries(t *testing.T, ctx context.Context, boltDB *BoltDB,
 	)
 }
 
+// featureSpecificEntriesDeletedSession populates the kv store with one feature
+// specific entry for the local temp store, and one feature specific entry for
+// the perm local store. Once populated, the session that the entries are linked
+// to is deleted. When migrating, we therefore expect that the kv entries linked
+// to the deleted session are not migrated.
+func featureSpecificEntriesDeletedSession(t *testing.T, ctx context.Context,
+	boltDB *BoltDB, sessionStore session.Store, _ accounts.Store,
+	_ *rootKeyMockStore) *expectedResult {
+
+	groupAlias := getNewSessionAlias(t, ctx, sessionStore)
+
+	_ = insertTempAndPermEntry(
+		t, ctx, boltDB, testRuleName, groupAlias,
+		fn.Some(testFeatureName), testEntryKey, testEntryValue,
+	)
+
+	err := sessionStore.DeleteReservedSessions(ctx)
+	require.NoError(t, err)
+
+	return &expectedResult{
+		// Since the session the kv entries were linked to has been
+		// deleted, we expect no kv entries to be migrated.
+		kvEntries: []*kvEntry{},
+		privPairs: make(privacyPairs),
+		actions:   []*Action{},
+	}
+}
+
+// featureSpecificEntriesDeletedAndExistingSessions populates the kv store with
+// two sessions and their corresponding feature specific kv entries.
+// One of the sessions is deleted prior to the migration though, and therefore
+// the migration should only migrate the kv entries linked to the still existing
+// session.
+func featureSpecificEntriesDeletedAndExistingSessions(t *testing.T,
+	ctx context.Context, boltDB *BoltDB, sessionStore session.Store,
+	_ accounts.Store, _ *rootKeyMockStore) *expectedResult {
+
+	groupAlias1 := getNewSessionAlias(t, ctx, sessionStore)
+
+	_ = insertTempAndPermEntry(
+		t, ctx, boltDB, testRuleName, groupAlias1,
+		fn.Some(testFeatureName), testEntryKey, testEntryValue,
+	)
+
+	err := sessionStore.DeleteReservedSessions(ctx)
+	require.NoError(t, err)
+
+	groupAlias2 := getNewSessionAlias(t, ctx, sessionStore)
+
+	return insertTempAndPermEntry(
+		t, ctx, boltDB, testRuleName2, groupAlias2,
+		fn.Some(testFeatureName2), testEntryKey2, testEntryValue,
+	)
+}
+
 // allEntryCombinations adds all types of different entries at all possible
 // levels of the kvstores, including multple entries with the same
 // ruleName, groupAlias and featureName. The test aims to cover all possible