[sql-52] firewalldb: add migration code for actions from kvdb to SQL #1143

ViktorTigerstrom · 2025-09-11T02:08:40Z

This PR introduces the migration logic for transitioning the actions from kvdb to SQL.

Note that as of this PR, the migration is not yet triggered by any production code, i.e. only tests execute the migration logic.

Part of #917

I will cleanup the code a little bit and add some more details to the commit messages before requesting reviews.

Update the assertEqualActions function under kvdb builds, to not mutate the passed action references.

This commit introduces the migration logic for transitioning the actions store from kvdb to SQL. Note that as of this commit, the migration is not yet triggered by any production code, i.e. only tests execute the migration logic.

gemini-code-assist

Summary of Changes

Hello @ViktorTigerstrom, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request is a foundational step in transitioning the firewalldb to a SQL-based system. It focuses on implementing the intricate migration process for 'action' records, ensuring that historical data is accurately transferred and correctly associated with related sessions and accounts. The current changes are primarily focused on the migration mechanism and its robust validation through testing, laying the groundwork for future production deployment.

Highlights

Action Data Migration: Introduced comprehensive logic to migrate 'action' data from the existing KV (BoltDB) store to the new SQL database within the firewalldb component. This includes handling various scenarios for linking actions to sessions and accounts.
New SQL Queries and Interfaces: Added new SQL queries and corresponding Go interfaces/implementations for GetAction (to retrieve action details from SQL) and UpdateAccountAliasForTests (a utility for testing account alias updates).
Extensive Test Coverage for Migration Logic: Significantly expanded the test suite for database migration, including numerous scenarios to validate the correctness of action migration. These tests cover cases with no linked entities, linked sessions, linked accounts, filtered entities, and complex scenarios involving multiple colliding entities.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This PR introduces the migration logic for actions from kvdb to SQL. The changes are extensive and include new SQL queries, migration logic, and a comprehensive set of tests. The migration logic correctly handles various scenarios of linking actions to sessions and accounts, with good heuristics for resolving ambiguities. The test suite is thorough, covering many edge cases. I've found a few issues, mostly minor comment discrepancies and a couple of bugs in the test code that prevent certain scenarios from being tested. Overall, this is a solid piece of work.

gemini-code-assist · 2025-09-11T02:10:27Z

firewalldb/sql_migration_test.go

+	actionReq2.SessionID = fn.None[session.ID]()
+	actionReq2.AccountID = fn.None[accounts.AccountID]()
+
+	addActionFromReq(actionReq1)


There seems to be a copy-paste error here. actionReq2 is prepared on the lines above, but actionReq1 is passed to addActionFromReq. This means the test case for an action with an actor name is not being executed correctly.

Suggested change

addActionFromReq(actionReq1)

addActionFromReq(actionReq2)

gemini-code-assist · 2025-09-11T02:10:27Z

firewalldb/sql_migration_test.go

+func randFilteredSession(t *testing.T, ctx context.Context,
+	sessStore session.Store) *session.Session {
+
+	if rand.Intn(1) == 0 {


rand.Intn(1) will always return 0, which means the else branch for testing revoked sessions is never executed. This should probably be rand.Intn(2) to test both cases.

Suggested change

if rand.Intn(1) == 0 {

if rand.Intn(2) == 0 {

gemini-code-assist · 2025-09-11T02:10:28Z

firewalldb/sql_migration.go

+	// based on the macaroon identifier (which is the first 4 bytes of the
+	// root key ID).


The comment states that the macaroon identifier is the first 4 bytes of the root key ID, but the implementation in mapMacIds uses the last 4 bytes. This seems to be a discrepancy. The lnd implementation of macaroon IDs uses the last 4 bytes of the root key ID. Please correct the comment for consistency.

based on the macaroon identifier (which is the last 4 bytes of the root key ID).

gemini-code-assist · 2025-09-11T02:10:28Z

firewalldb/sql_migration.go

+		// Alternative (1) above.
+		insertParams, err = paramsFromSession(
+			action, actSession, macRootKeyID,
+		)
+	case hasAccounts:
+		// Alternative (2) above.
+		insertParams, err = paramsFromAccounts(
+			ctx, accountsDB, action, actAccounts, macRootKeyID)


The comments for the switch cases seem to be swapped. case hasSessions: should correspond to alternative (2) from the logic description, and case hasAccounts: should correspond to alternative (1).

// Alternative (2) above. insertParams, err = paramsFromSession( action, actSession, macRootKeyID, ) case hasAccounts: // Alternative (1) above. insertParams, err = paramsFromAccounts( ctx, accountsDB, action, actAccts, macRootKeyID)

ViktorTigerstrom added 6 commits September 9, 2025 17:39

sqlc+firewalldb: add GetAction SQL query

72a233e

multi: add UpdateAccountAliasForTests query

47ff4aa

firewalldb: update kvdb assertEqualActions

27e0335

Update the assertEqualActions function under kvdb builds, to not mutate the passed action references.

firewalldb: add expectedActions to mig tests res

26cd843

firewalldb: pass accountStore & rootKeyStore to mig tests

68a38fe

firewalldb: add actions SQL migration

2bd9a9c

This commit introduces the migration logic for transitioning the actions store from kvdb to SQL. Note that as of this commit, the migration is not yet triggered by any production code, i.e. only tests execute the migration logic.

ViktorTigerstrom added the no-changelog This PR is does not require a release notes entry label Sep 11, 2025

gemini-code-assist bot reviewed Sep 11, 2025

View reviewed changes

ViktorTigerstrom mentioned this pull request Sep 15, 2025

[sql-53] firewalldb: actions migration prep 2 - persist full macaroon root key ID in the SQL actions store #1146

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[sql-52] firewalldb: add migration code for actions from kvdb to SQL #1143

[sql-52] firewalldb: add migration code for actions from kvdb to SQL #1143

Uh oh!

ViktorTigerstrom commented Sep 11, 2025

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot Sep 11, 2025

Uh oh!

gemini-code-assist bot Sep 11, 2025

Uh oh!

gemini-code-assist bot Sep 11, 2025

Uh oh!

gemini-code-assist bot Sep 11, 2025

Uh oh!

Uh oh!

		// based on the macaroon identifier (which is the first 4 bytes of the
		// root key ID).

[sql-52] firewalldb: add migration code for actions from kvdb to SQL #1143

Are you sure you want to change the base?

[sql-52] firewalldb: add migration code for actions from kvdb to SQL #1143

Uh oh!

Conversation

ViktorTigerstrom commented Sep 11, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!