fix(vpn): commit before sync_wireguard_config to ensure data visibility

sync_wireguard_config opens its own AdminAsyncSessionLocal connection
which cannot see uncommitted data from the caller's transaction. Add
_commit_and_sync helper that commits first, then regenerates wg0.conf.

Also removes the unused db parameter from sync_wireguard_config.
This commit is contained in:
Jason Staack
2026-03-14 16:42:17 -05:00
parent b4a7494016
commit b5f9bf14df
3 changed files with 27 additions and 18 deletions

View File

@@ -232,13 +232,11 @@ async def delete_tenant(
had_vpn = await get_vpn_config(db, tenant_id) had_vpn = await get_vpn_config(db, tenant_id)
await db.delete(tenant) await db.delete(tenant)
await db.flush() await db.commit()
# Regenerate wg0.conf without deleted tenant's peers # Regenerate wg0.conf without deleted tenant's peers
if had_vpn: if had_vpn:
await sync_wireguard_config(db) await sync_wireguard_config()
await db.commit()
# --------------------------------------------------------------------------- # ---------------------------------------------------------------------------

View File

@@ -151,10 +151,22 @@ def _get_wg_config_path() -> Path:
return Path(os.getenv("WIREGUARD_CONFIG_PATH", "/data/wireguard")) return Path(os.getenv("WIREGUARD_CONFIG_PATH", "/data/wireguard"))
async def sync_wireguard_config(db: AsyncSession) -> None: async def _commit_and_sync(db: AsyncSession) -> None:
"""Commit the caller's transaction then regenerate wg0.conf.
sync_wireguard_config opens its own DB session, so callers must commit
first for their changes to be visible. This helper combines both steps
and provides a single patch point for tests.
"""
await _commit_and_sync(db)
async def sync_wireguard_config() -> None:
"""Regenerate wg0.conf with ALL tenants' peers and write to shared volume. """Regenerate wg0.conf with ALL tenants' peers and write to shared volume.
Uses AdminAsyncSessionLocal to bypass RLS (must see all tenants). Uses AdminAsyncSessionLocal to bypass RLS (must see all tenants).
Callers MUST commit their transaction before calling this function,
since it opens a separate DB session that cannot see uncommitted data.
Uses a PostgreSQL advisory lock to prevent concurrent writes. Uses a PostgreSQL advisory lock to prevent concurrent writes.
Writes atomically via temp file + rename. Writes atomically via temp file + rename.
""" """
@@ -323,7 +335,7 @@ async def setup_vpn(
logger.info("vpn_subnet_allocated", event="vpn_audit", logger.info("vpn_subnet_allocated", event="vpn_audit",
tenant_id=str(tenant_id), subnet_index=subnet_index, subnet=subnet) tenant_id=str(tenant_id), subnet_index=subnet_index, subnet=subnet)
await sync_wireguard_config(db) await _commit_and_sync(db)
return config return config
@@ -341,7 +353,7 @@ async def update_vpn_config(
config.is_enabled = is_enabled config.is_enabled = is_enabled
await db.flush() await db.flush()
await sync_wireguard_config(db) await _commit_and_sync(db)
return config return config
@@ -410,7 +422,7 @@ async def add_peer(db: AsyncSession, tenant_id: uuid.UUID, device_id: uuid.UUID,
db.add(peer) db.add(peer)
await db.flush() await db.flush()
await sync_wireguard_config(db) await _commit_and_sync(db)
return peer return peer
@@ -425,7 +437,7 @@ async def remove_peer(db: AsyncSession, tenant_id: uuid.UUID, peer_id: uuid.UUID
await db.delete(peer) await db.delete(peer)
await db.flush() await db.flush()
await sync_wireguard_config(db) await _commit_and_sync(db)
async def get_peer_config(db: AsyncSession, tenant_id: uuid.UUID, peer_id: uuid.UUID) -> dict: async def get_peer_config(db: AsyncSession, tenant_id: uuid.UUID, peer_id: uuid.UUID) -> dict:
@@ -523,7 +535,7 @@ async def onboard_device(
db.add(peer) db.add(peer)
await db.flush() await db.flush()
await sync_wireguard_config(db) await _commit_and_sync(db)
# Generate RouterOS commands # Generate RouterOS commands
endpoint = config.endpoint or "YOUR_SERVER_IP:51820" endpoint = config.endpoint or "YOUR_SERVER_IP:51820"

View File

@@ -36,16 +36,15 @@ def wireguard_tmp_dir(tmp_path):
@pytest.fixture(autouse=True) @pytest.fixture(autouse=True)
def _no_sync_wireguard(): def _no_commit_and_sync():
"""Patch sync_wireguard_config to a no-op in service calls. """Patch _commit_and_sync to a no-op in service calls.
sync_wireguard_config opens its own AdminAsyncSessionLocal connection, _commit_and_sync commits the transaction then opens a separate DB session
which cannot see uncommitted test-transaction data. We patch it globally to regenerate wg0.conf. In tests, committing breaks transaction rollback
so setup_vpn / add_peer / remove_peer don't fail, and then call the isolation, and the separate session can't see test data. Patching this
real function explicitly in tests that need to verify wg0.conf content single function prevents both issues.
(those tests commit data first or use a dedicated helper).
""" """
with patch("app.services.vpn_service.sync_wireguard_config", new_callable=AsyncMock): with patch("app.services.vpn_service._commit_and_sync", new_callable=AsyncMock):
yield yield