refactor(wireguard): ip allocation refactor - #713
Conversation
- Add `wireguard.py` for handling WireGuard subnet allocation, including functions for managing peer IPs, namespaces, and user allocations. - Introduce `wireguard_subnets` table migration to store subnet allocation data, including next offsets and free offsets. - Implement backfill logic for existing users' peer IPs during migration, ensuring proper allocation based on core configurations. - Create unit tests for WireGuard allocation logic, covering subnet management, IP rendering, and allocation functions.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe PR replaces global WireGuard peer-IP allocation with subnet-aware database pools, lifecycle reconciliation, key preparation, migration backfill, and subnet usage reporting. It also removes manual bulk peer-IP management from the API and dashboard and makes core initialization skip invalid configurations. ChangesWireGuard allocation and persistence
Removal of manual WireGuard peer-IP management
Core loading resilience and formatting
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant CoreOperation
participant wireguard
participant Database
participant Subscription
Admin->>CoreOperation: create or modify WireGuard core
CoreOperation->>wireguard: reconcile_wireguard_subnets
wireguard->>Database: rebuild subnet pools and user peer_ips
Database-->>wireguard: reconciled allocation state
wireguard-->>CoreOperation: affected user ids
CoreOperation->>Subscription: resync affected users
Subscription->>wireguard: pick_peer_ip_for_inbound
wireguard-->>Subscription: inbound-matching peer_ips
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ImMohammad20000
changed the title
M03ED
changed the title
M03ED
changed the title
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/operation/user.py (1)
819-834: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBulk template apply can surface an unhandled 500 on WireGuard subnet exhaustion.
_apply_modified_usernow wrapscrud_modify_userintry/except ValueErrorto translate WG subnet exhaustion into a 400 (Line 831-834). Howeverbulk_apply_template_to_userscallscrud_modify_userdirectly in its own loop and only catches genericExceptionto rollback-and-re-raise (no ValueError→400 translation). Sinceload_base_user_argssetsgroup_idsfrom the template, applying a template that assigns a WG group to many users can legitimately trigger the same "subnet exhausted"ValueErrorfromcrud_modify_user, but here it will propagate raw instead of becoming a clean 400 — inconsistent with every other WG-exhaustion path in this file (create_user,_persist_bulk_users,_apply_modified_user).🔧 Proposed fix
try: for db_user, modified_user_model, validated_groups, _, _ in prepared_updates: if user_template.reset_usages: await reset_user_data_usage( db, db_user, clean_chart_data=usage_settings.reset_user_usage_clean_chart_data, commit=False, ) self._apply_explicit_null_hwid_limit(db_user, modified_user_model) - await crud_modify_user( - db, - db_user, - modified_user_model, - groups=validated_groups, - commit=False, - ) + try: + await crud_modify_user( + db, + db_user, + modified_user_model, + groups=validated_groups, + commit=False, + ) + except ValueError as exc: # WireGuard subnet exhausted + await self.raise_error(message=str(exc), code=400, db=db) if db_user.id is not None: modified_user_ids.append(db_user.id) await db.commit() except Exception: await db.rollback() raise🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/operation/user.py` around lines 819 - 834, Update bulk_apply_template_to_users so its crud_modify_user loop catches WireGuard subnet-exhaustion ValueError and translates it through the same raise_error(..., code=400, db=...) path used by create_user, _persist_bulk_users, and _apply_modified_user. Preserve the existing rollback behavior for other exceptions.app/utils/wireguard.py (1)
14-36: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftapp/utils/wireguard.py:14-36 — Back WireGuard
public_keyuniqueness with a DB constraint
wireguard_public_key_in_useis only an application-side check. Without a unique index/constraint onusers.proxy_settings->wireguard->public_key, concurrent create/update requests can still persist duplicate WireGuard keys and break peer identity for one user. Add a DB-backed uniqueness guarantee, or handle the resultingIntegrityErrorif it already exists.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/utils/wireguard.py` around lines 14 - 36, Add a database-level unique constraint or unique index for the WireGuard public key JSON value used by `wireguard_public_key_in_use`, and ensure create/update persistence handles any resulting `IntegrityError` consistently. Keep the existing application-side validation and allow users without a public key to remain unaffected.
🧹 Nitpick comments (2)
app/db/migrations/versions/3c1a7e5b9d20_add_wireguard_subnets_pool_table.py (1)
246-256: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBackfill
free_offsetsis not bounded like the runtime path.The runtime allocator caps each row's free-list via
_trim_free/FREE_OFFSETS_CAP(seeapp/db/crud/wireguard.pyLines 215-218, 524-526) to bound JSON size. This migration writes the fullfreelist uncapped. When an existing peer IP is preserved at a high offset in a large subnet,next_offsetbecomes large andfreecan contain tens of thousands of entries, producing a bloated pool row until the next reconcile trims it. Apply the same cap here for consistency.Proposed fix
free = [ offset for offset in range(1, next_offset) if offset not in offsets and offset not in ns["reserved"] and offset < ns["subnet"].num_addresses - 1 ] + if len(free) > 10000: # keep in sync with FREE_OFFSETS_CAP + free = sorted(free)[:10000] pool_rows.append({"network": key, "next_offset": next_offset, "free_offsets": free})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/db/migrations/versions/3c1a7e5b9d20_add_wireguard_subnets_pool_table.py` around lines 246 - 256, Cap the migration’s computed free offset list before appending it to pool_rows, matching the runtime allocator’s FREE_OFFSETS_CAP/_trim_free behavior in the wireguard CRUD path. Update the free_offsets value in the loop over namespaces while preserving the existing offset filtering and next_offset calculation.app/utils/wireguard.py (1)
39-42: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUncached
get_wg_cores()lookup on every user prep call causes N+1 queries in bulk paths.
user_has_wireguard_accessre-queriescore_configson every invocation with no caching, and it's invoked unconditionally on every single-user and bulk-user modify/create path — turning bulk operations into one extra DB round-trip per user for a value that's identical across the whole batch.
app/utils/wireguard.py#L39-L42: root cause — memoize/cacheget_wg_cores(db)per request or accept a precomputedwg_tags/access flag instead of querying insideuser_has_wireguard_access.app/operation/user.py#L371-L378:_persist_bulk_users's per-user loop triggers oneget_wg_coresquery per created user; compute WG access once for the sharedgroupsand reuse across the loop.app/operation/user.py#L789-L813:_prepare_modified_user(called for every singlemodify_user) re-runs the same uncached check even for non-WG changes (e.g. pure status/note edits).app/operation/user.py#L1125-L1136:_bulk_set_user_status's per-user loop compounds the same redundant query across all selected users.app/operation/user.py#L1814-L1832:bulk_apply_template_to_users's per-user loop has the same N+1 pattern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/utils/wireguard.py` around lines 39 - 42, The uncached WireGuard core lookup causes one database query per user during bulk operations. Update app/utils/wireguard.py lines 39-42 in user_has_wireguard_access to accept or reuse precomputed wg_tags/access data, then compute that value once and reuse it in app/operation/user.py lines 371-378 (_persist_bulk_users), 1125-1136 (_bulk_set_user_status), and 1814-1832 (bulk_apply_template_to_users); avoid rechecking for unrelated changes in _prepare_modified_user at lines 789-813, while preserving single-user access behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/db/crud/wireguard.py`:
- Around line 382-384: Update new_ips construction in sync_users_allocations to
use the same canonical _peer_sort_key ordering as reconcile_wireguard_subnets
before comparing it with old_ips. Preserve the existing IP rendering and ensure
the comparison reflects peer changes rather than CIDR key ordering.
- Around line 206-212: In the group operation flow, wrap the
sync_users_allocations call in app/operation/group.py with ValueError handling
and return an HTTP 400 when WireGuard address allocation is exhausted. Preserve
reconcile_wireguard_subnets’s existing warn-and-continue behavior, and apply the
handling to both group and bulk-group changes without altering successful
synchronization.
In `@app/utils/reality_scan.py`:
- Line 184: Parenthesize the exception tuples in all three handlers in
app/utils/reality_scan.py: lines 184-184 should catch asyncio.TimeoutError and
TimeoutError as a tuple, while lines 414-414 and 516-516 should each catch
socket.timeout and TimeoutError as tuples, preserving the existing handler
behavior.
---
Outside diff comments:
In `@app/operation/user.py`:
- Around line 819-834: Update bulk_apply_template_to_users so its
crud_modify_user loop catches WireGuard subnet-exhaustion ValueError and
translates it through the same raise_error(..., code=400, db=...) path used by
create_user, _persist_bulk_users, and _apply_modified_user. Preserve the
existing rollback behavior for other exceptions.
In `@app/utils/wireguard.py`:
- Around line 14-36: Add a database-level unique constraint or unique index for
the WireGuard public key JSON value used by `wireguard_public_key_in_use`, and
ensure create/update persistence handles any resulting `IntegrityError`
consistently. Keep the existing application-side validation and allow users
without a public key to remain unaffected.
---
Nitpick comments:
In `@app/db/migrations/versions/3c1a7e5b9d20_add_wireguard_subnets_pool_table.py`:
- Around line 246-256: Cap the migration’s computed free offset list before
appending it to pool_rows, matching the runtime allocator’s
FREE_OFFSETS_CAP/_trim_free behavior in the wireguard CRUD path. Update the
free_offsets value in the loop over namespaces while preserving the existing
offset filtering and next_offset calculation.
In `@app/utils/wireguard.py`:
- Around line 39-42: The uncached WireGuard core lookup causes one database
query per user during bulk operations. Update app/utils/wireguard.py lines 39-42
in user_has_wireguard_access to accept or reuse precomputed wg_tags/access data,
then compute that value once and reuse it in app/operation/user.py lines 371-378
(_persist_bulk_users), 1125-1136 (_bulk_set_user_status), and 1814-1832
(bulk_apply_template_to_users); avoid rechecking for unrelated changes in
_prepare_modified_user at lines 789-813, while preserving single-user access
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d1979161-6be9-4445-94ee-c26771f95c72
📒 Files selected for processing (42)
.env.exampleapp/core/manager.pyapp/db/crud/bulk.pyapp/db/crud/user.pyapp/db/crud/wireguard.pyapp/db/migrations/versions/3c1a7e5b9d20_add_wireguard_subnets_pool_table.pyapp/db/migrations/versions/a1b2c3d4e5f6_add_wireguard_settings_to_proxy_settings.pyapp/db/models.pyapp/models/host.pyapp/models/reality_scan.pyapp/models/system.pyapp/models/user.pyapp/operation/core.pyapp/operation/group.pyapp/operation/user.pyapp/routers/system.pyapp/routers/user.pyapp/subscription/share.pyapp/utils/ip_pool.pyapp/utils/reality_scan.pyapp/utils/wireguard.pyapp/utils/wireguard_pool.pyconfig.pydashboard/public/statics/locales/en.jsondashboard/public/statics/locales/fa.jsondashboard/public/statics/locales/ru.jsondashboard/public/statics/locales/zh.jsondashboard/src/app/router.tsxdashboard/src/components/layout/sidebar.tsxdashboard/src/components/layout/tabbed-route-suspense-fallback.tsxdashboard/src/features/bulk/components/bulk-flow.tsxdashboard/src/features/users/dialogs/user-modal.tsxdashboard/src/features/users/forms/user-form.tsdashboard/src/pages/_dashboard.bulk.tsxdashboard/src/pages/_dashboard.bulk.wireguard.tsxdashboard/src/service/api/index.tsdashboard/src/utils/rbac.tstests/api/test_bulk.pytests/api/test_user.pytests/test_core_manager_skip_broken.pytests/test_reality_scan_unit.pytests/test_wireguard_alloc_unit.py
💤 Files with no reviewable changes (15)
- app/utils/wireguard_pool.py
- dashboard/src/pages/_dashboard.bulk.wireguard.tsx
- app/utils/ip_pool.py
- .env.example
- dashboard/src/features/users/forms/user-form.ts
- dashboard/src/components/layout/tabbed-route-suspense-fallback.tsx
- app/db/crud/bulk.py
- dashboard/src/app/router.tsx
- dashboard/public/statics/locales/en.json
- dashboard/public/statics/locales/ru.json
- dashboard/src/components/layout/sidebar.tsx
- app/db/migrations/versions/a1b2c3d4e5f6_add_wireguard_settings_to_proxy_settings.py
- dashboard/public/statics/locales/zh.json
- dashboard/src/service/api/index.ts
- dashboard/public/statics/locales/fa.json
| try: | ||
| infos = await asyncio.wait_for(loop.getaddrinfo(host, None, type=socket.SOCK_STREAM), timeout=timeout) | ||
| except (asyncio.TimeoutError, TimeoutError): | ||
| except asyncio.TimeoutError, TimeoutError: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify that multiple exception types without parentheses cause a SyntaxError.
python3 -c "
try:
pass
except ValueError, TypeError:
pass
"Repository: PasarGuard/panel
Length of output: 307
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show file size and the relevant regions around each reported site.
wc -l app/utils/reality_scan.py
sed -n '160,205p' app/utils/reality_scan.py
printf '\n---\n'
sed -n '395,430p' app/utils/reality_scan.py
printf '\n---\n'
sed -n '495,530p' app/utils/reality_scan.py
# Check for Python version hints in the file and repository.
printf '\n=== version hints ===\n'
rg -n "python3|python2|#!/usr/bin/env python|from __future__|except .*," app/utils/reality_scan.py pyproject.toml setup.py requirements*.txt README* .github -g '!**/.git/**' || trueRepository: PasarGuard/panel
Length of output: 5315
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Confirm that the specific syntax pattern in Python 3 is a parse-time error,
# and compare with the parenthesized form.
python3 - <<'PY'
samples = {
"bad": "try:\n pass\nexcept asyncio.TimeoutError, TimeoutError:\n pass\n",
"good": "try:\n pass\nexcept (asyncio.TimeoutError, TimeoutError):\n pass\n",
}
for name, src in samples.items():
try:
compile(src, f"<{name}>", "exec")
print(name, "OK")
except SyntaxError as e:
print(name, "SyntaxError:", e.msg)
PYRepository: PasarGuard/panel
Length of output: 226
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the nearby context to determine whether these handlers are in a Python 3 module.
python3 - <<'PY'
from pathlib import Path
path = Path("app/utils/reality_scan.py")
text = path.read_text()
for lineno in (184, 414, 516):
lines = text.splitlines()
start = max(0, lineno - 5)
end = min(len(lines), lineno + 4)
print(f"\n--- {path}:{lineno} ---")
for i in range(start, end):
print(f"{i+1}: {lines[i]}")
PYRepository: PasarGuard/panel
Length of output: 1635
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the file header and the three reported regions.
sed -n '1,40p' app/utils/reality_scan.py
printf '\n---\n'
sed -n '175,190p' app/utils/reality_scan.py
printf '\n---\n'
sed -n '408,420p' app/utils/reality_scan.py
printf '\n---\n'
sed -n '508,522p' app/utils/reality_scan.pyRepository: PasarGuard/panel
Length of output: 2844
Parenthesize these exception tuples.
except asyncio.TimeoutError, TimeoutError: and the two socket.timeout handlers are invalid Python 3 syntax and prevent app/utils/reality_scan.py from parsing. Fix all three sites:
app/utils/reality_scan.py#L184-L184:except (asyncio.TimeoutError, TimeoutError):app/utils/reality_scan.py#L414-L414:except (socket.timeout, TimeoutError):app/utils/reality_scan.py#L516-L516:except (socket.timeout, TimeoutError):
📍 Affects 1 file
app/utils/reality_scan.py#L184-L184(this comment)app/utils/reality_scan.py#L414-L414app/utils/reality_scan.py#L516-L516
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/utils/reality_scan.py` at line 184, Parenthesize the exception tuples in
all three handlers in app/utils/reality_scan.py: lines 184-184 should catch
asyncio.TimeoutError and TimeoutError as a tuple, while lines 414-414 and
516-516 should each catch socket.timeout and TimeoutError as tuples, preserving
the existing handler behavior.
- Implemented WireGuardSubnetsList component to display usage of WireGuard subnets. - Added a new route for WireGuard subnets in the dashboard. - Updated RBAC to allow access to WireGuard subnets page.
| FREE_OFFSETS_CAP = FREE_IPS_LIMIT | ||
|
|
||
| IpNetwork = IPv4Network | IPv6Network | ||
| IpAddress = IPv4Address | IPv6Address |
There was a problem hiding this comment.
create a new type using type keyword
2 participants
Summary
Type of change
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Updates