[Feat] Server Networking (IP Management)#1141
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR implements a complete server IP address management system for Vito, enabling administrators to discover, manage, and persist network interface configurations on remote servers via SSH and netplan. ChangesServer Network IP Management
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new “Server Network” feature area for managing server IP addresses, including discovery via SSH, persistence via netplan, and UI + realtime table updates.
Changes:
- Introduces
ServerIpAddresspersistence (migration/model/factory), enums (family/type/status), policy, and API resource. - Implements server IP discovery/reconciliation (
RefreshServerIps) and apply/persist jobs (PersistServerIpsJob,RefreshServerIpsJob) with netplan SSH templates. - Adds Inertia/React UI for listing IPs and performing actions (refresh, add, delete, set primary) including a registered dialog.
Reviewed changes
Copilot reviewed 28 out of 29 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Feature/ServerNetworkTest.php | Feature coverage for network page, IP CRUD-ish actions, and refresh/reconcile behavior. |
| resources/views/ssh/network/netplan.blade.php | Generates Vito-managed netplan YAML and embeds a managed-IP marker. |
| resources/views/ssh/network/list-network-addresses.blade.php | SSH script to read ip -j addr plus managed marker from netplan file. |
| resources/views/ssh/network/apply-netplan.blade.php | SSH script to apply/remove Vito netplan and run netplan generate/apply. |
| resources/js/types/server-ip.d.ts | Frontend type for server IP address payloads. |
| resources/js/pages/server-network/index.tsx | Network page UI with refresh + add dialog + IP table actions. |
| resources/js/pages/server-network/components/set-primary.tsx | “Set as primary” confirm action wiring. |
| resources/js/pages/server-network/components/form.tsx | Registered dialog form for adding a single IP or a range. |
| resources/js/pages/server-network/components/delete.tsx | “Delete” confirm action wiring. |
| resources/js/layouts/server/layout.tsx | Adds “Network” entry to the server sidebar navigation. |
| resources/js/components/dialogs/registry.ts | Registers serverIpForm dialog in the global dialog registry. |
| database/migrations/2026_06_03_201231_create_server_ip_addresses_table.php | Adds server_ip_addresses table and indexes/uniqueness. |
| database/factories/ServerIpAddressFactory.php | Factory for generating server IP address records in tests. |
| app/Tables/Servers/ServerIpAddressTable.php | InertiaTable definition for server IP address listing (columns + ordering + realtime). |
| app/Support/Testing/SSHFake.php | Extends SSH fake to expose uploaded content for assertions. |
| app/Policies/ServerIpAddressPolicy.php | Authorization rules for viewing/creating/updating/deleting server IP addresses. |
| app/Models/ServerIpAddress.php | New model with enum casts + helpers for IP family/type classification. |
| app/Models/Server.php | Adds ipAddresses() relation for server ↔ IP addresses. |
| app/Jobs/ServerIp/RefreshServerIpsJob.php | Job wrapper to refresh IPs post-install / on-demand. |
| app/Jobs/ServerIp/PersistServerIpsJob.php | Job to refresh, write netplan for managed IPs, apply netplan, and finalize statuses/deletes. |
| app/Http/Resources/ServerIpAddressResource.php | API resource for consistent enum text/color exposure to frontend. |
| app/Http/Controllers/ServerNetworkController.php | Inertia endpoints for network page, refresh, create, set-primary, delete. |
| app/Facades/SSH.php | Updates facade docblock for new fake helper method. |
| app/Enums/IpAddressType.php | Enum for IP type display (public/private/unknown). |
| app/Enums/IpAddressStatus.php | Enum for IP status display (configuring/configured/deleting/failed). |
| app/Enums/IpAddressFamily.php | Enum for IP family display (IPv4/IPv6). |
| app/Actions/ServerIp/RefreshServerIps.php | Parses ip -j output and reconciles discovered IPs with DB, preserving “managed” markers. |
| app/Actions/ServerIp/ManageServerIp.php | Validates and creates/deletes IPs, expands ranges, queues persist/apply, sets primary. |
| app/Actions/Server/InstallServer.php | Dispatches a refresh IPs job after server installation completes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
tests/Feature/ServerNetworkTest.php (2)
17-324: ⚡ Quick winAdd deny-path coverage for the new network endpoints.
This suite exercises happy paths and validation, but it does not prove the middleware/policy boundaries on
index,store,refresh,primary, anddestroy. A couple of cross-project / unauthorized cases here would catch the highest-risk regressions on this surface.As per coding guidelines:
**/*.{php,ts,tsx}: Every endpoint (web and API) must be protected by appropriate middleware and PoliciesandAPI keys are project-scoped — enforce project boundaries in queries and policies`.🤖 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 `@tests/Feature/ServerNetworkTest.php` around lines 17 - 324, Add deny-path tests to ServerNetworkTest covering unauthorized/cross-project access for the network endpoints: assert that an actor from another project (or a user without permission) cannot access the index (route 'servers.network'), cannot create/store IPs ('servers.network.ips.store'), cannot refresh ('servers.network.refresh'), cannot mark primary ('servers.network.ips.primary'), and cannot delete ('servers.network.ips.destroy'). For each endpoint, create tests that use a different user/server pairing (or revoke permissions), call the route, and assert the request is forbidden or returns the appropriate error/redirect (e.g., assertForbidden or assertSessionHasErrors/assertRedirect as used elsewhere). Ensure these tests reference the same unique routes/methods (ServerNetworkTest::test_see_network_page, test_add_ip_address, test_refresh_pulls_ips_from_server, test_set_ip_as_primary_updates_server_ip, test_delete_managed_ip_address) so they exercise middleware/policy enforcement and project-scoped access control.
209-223: ⚡ Quick winRemove the SSH fake requirement for the primary-assignment test (
test_set_ip_as_primary_updates_server_ip)
ManageServerIp::setPrimary()only updatesservers.ip/servers.local_ipandserver_ip_addresses.is_primaryin a DB transaction and then dispatches aSocketEventDTO; it does not enqueuePersistServerIpsJobor callSSH, soSSH::fake()isn’t needed for this flow.
Also consider adding deny-path coverage (unauthenticated/forbidden/other-project) for theservers.network.ips.primaryendpoint.🤖 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 `@tests/Feature/ServerNetworkTest.php` around lines 209 - 223, The test test_set_ip_as_primary_updates_server_ip should not call SSH::fake() because ManageServerIp::setPrimary only updates DB rows (servers.ip/servers.local_ip and server_ip_addresses.is_primary) and dispatches a SocketEventDTO; remove any SSH::fake() setup in this test and rely on the DB assertions and response check after posting to the servers.network.ips.primary route; additionally add complementary deny-path tests (e.g., unauthenticated, forbidden, other-project) for the servers.network.ips.primary endpoint to cover access control.resources/js/pages/server-network/components/form.tsx (1)
129-134: ⚡ Quick winUse a numeric input for
prefix_length.This field is integer-only on the backend, but
type="text"still lets values like/24orabcthrough until the 422. Switching to a numeric input here prevents avoidable round-trips and gives a better mobile keyboard.♻️ Proposed fix
<Input - type="text" + type="number" + inputMode="numeric" + min={1} + max={128} + step={1} id="prefix_length" placeholder="e.g. 24" value={form.data.prefix_length} onChange={(e) => form.setData('prefix_length', e.target.value)} />🤖 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 `@resources/js/pages/server-network/components/form.tsx` around lines 129 - 134, The prefix_length field uses a text input which allows non-numeric values; change the Input for id "prefix_length" to be numeric so the browser enforces integers and shows the numeric keyboard on mobile. Update the Input's type from "text" to "number" and ensure the onChange handler for form.setData('prefix_length', ...) converts the input appropriately (e.g., keep using e.target.value but validate/parse to an integer before sending to the backend or let the form store the numeric string if consistent with form.data.prefix_length). Locate the Input component instance (id="prefix_length", value={form.data.prefix_length}, onChange={form.setData('prefix_length', ...)}) and make the type change and any minimal parsing/validation to keep form.data consistent.
🤖 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/Actions/ServerIp/ManageServerIp.php`:
- Around line 30-33: The transaction currently sets $created only when inserting
new IP rows, and on retries a previously-committed insert can be skipped causing
$created to remain false and no persist job to be queued; modify
ManageServerIp::expandIps / the DB::transaction block so that after the
transaction (or by checking inside before returning) you detect any server IP
rows in CONFIGURING or DELETING state (not just newly-inserted ones) and call
queueApply(...) when such pending rows exist; specifically update the
transaction closure and the post-transaction logic around the $created flag (and
the similar block at the other occurrence at lines 58-60) to enqueue the persist
job whenever there are pending CONFIGURING/DELETING rows for the server,
ensuring retries still schedule queueApply even if the insert was already
committed.
In `@app/Actions/ServerIp/RefreshServerIps.php`:
- Around line 145-150: The fast-path in RefreshServerIps.php that checks
$row->is_managed only updates interface and is_primary and then continues, which
preserves stale managed states; change this branch to copy all discovered fields
from $entry into $row (e.g. any fields you treat as discovered like address,
netmask, mac, interface, is_primary — use the same keys from $entry) and then,
unless $row->state === 'DELETING', set $row->state = 'CONFIGURED' (or the
constant used for CONFIGURED in your model) before calling $row->save(); keep
the existing behavior of leaving DELETING untouched so deletions aren’t
interrupted and ensure ServerIpAddressResource will see the normalized
CONFIGURED state.
In `@app/Http/Controllers/ServerNetworkController.php`:
- Around line 45-49: The controller currently swallows SSHError in
ServerNetworkController::refresh() by wrapping
app(RefreshServerIps::class)->handle($server) in a try/catch and returning a
back()->with('error', ...). Remove the try/catch so the call becomes a direct
invocation of RefreshServerIps::handle($server) and allow SSHError to bubble to
the global exception handler; ensure no alternative error handling remains in
refresh() for SSHError and update any local comments accordingly.
In `@app/Models/ServerIpAddress.php`:
- Around line 62-68: In ServerIpAddress::classifyType() the current use of
filter_var with FILTER_FLAG_NO_RES_RANGE causes reserved-but-test addresses like
203.0.113.10 to be treated as non-public; update the logic to either remove
FILTER_FLAG_NO_RES_RANGE from the filter_var flags or replace the filter with an
explicit private-range check (e.g., validate IP with FILTER_FLAG_NO_PRIV_RANGE
for IPv4/IPv6 and then separately check RFC1918/ULA ranges) so that
classifyType() returns IpAddressType::PUBLIC for non-RFC1918/ULA addresses;
refer to the classifyType() method and IpAddressType constants when making the
change.
In `@app/Support/Testing/SSHFake.php`:
- Around line 163-165: The getUploadedContent() accessor can throw if
$uploadedContent is uninitialized; update the SSHFake class by initializing the
property $uploadedContent to an empty string (e.g. private string
$uploadedContent = '') so getUploadedContent() can safely return a string even
before upload() is called; alternatively, if you want nullable semantics, change
the property to ?string $uploadedContent = null and adjust getUploadedContent()
to return ?string (and any callers) — prefer initializing to '' to minimize
call-site changes.
In `@resources/js/pages/server-network/index.tsx`:
- Around line 59-64: The Docs link currently nests a Button (native <button>)
inside an <a target="_blank>, which creates invalid/unsafe interactive nesting
and also omits rel="noopener noreferrer"; fix by rendering the link as an anchor
styled like the Button instead of nesting — e.g. make the outer element the
anchor (use Button's prop to render as an anchor such as Button as="a" or
component="a" / pass href to Button) or replace the inner Button with
non-interactive markup, add href and rel="noopener noreferrer" and keep
target="_blank"; update the JSX where Button and BookOpenIcon are used for the
Docs link to remove nested interactive elements and include rel="noopener
noreferrer".
In `@resources/views/ssh/network/apply-netplan.blade.php`:
- Around line 6-7: Replace the current sequential invocation so that "sudo
netplan apply" only runs if "sudo netplan generate" succeeds: modify the
template where the two commands appear ("sudo netplan generate" and "sudo
netplan apply") to chain them with && instead of running unconditionally,
ensuring apply is skipped on generate failure.
In `@resources/views/ssh/network/netplan.blade.php`:
- Around line 1-12: The template currently emits raw values via {!! !!} for
$managedIps, $interface, and $address; replace those with escaped, quoted output
so untrusted data cannot break the generated YAML. Specifically, change the
managed-IPs line to render an escaped string of the imploded $managedIps (escape
each element before joining), render interface names as quoted, escaped keys
instead of raw tokens, and render each address as a quoted, escaped YAML scalar
in the addresses list; use Blade’s escaping helpers ({{ ... }} or e(...)) to
perform the escaping and ensure surrounding quotes are present so any special
characters are preserved as data, not YAML structure.
---
Nitpick comments:
In `@resources/js/pages/server-network/components/form.tsx`:
- Around line 129-134: The prefix_length field uses a text input which allows
non-numeric values; change the Input for id "prefix_length" to be numeric so the
browser enforces integers and shows the numeric keyboard on mobile. Update the
Input's type from "text" to "number" and ensure the onChange handler for
form.setData('prefix_length', ...) converts the input appropriately (e.g., keep
using e.target.value but validate/parse to an integer before sending to the
backend or let the form store the numeric string if consistent with
form.data.prefix_length). Locate the Input component instance
(id="prefix_length", value={form.data.prefix_length},
onChange={form.setData('prefix_length', ...)}) and make the type change and any
minimal parsing/validation to keep form.data consistent.
In `@tests/Feature/ServerNetworkTest.php`:
- Around line 17-324: Add deny-path tests to ServerNetworkTest covering
unauthorized/cross-project access for the network endpoints: assert that an
actor from another project (or a user without permission) cannot access the
index (route 'servers.network'), cannot create/store IPs
('servers.network.ips.store'), cannot refresh ('servers.network.refresh'),
cannot mark primary ('servers.network.ips.primary'), and cannot delete
('servers.network.ips.destroy'). For each endpoint, create tests that use a
different user/server pairing (or revoke permissions), call the route, and
assert the request is forbidden or returns the appropriate error/redirect (e.g.,
assertForbidden or assertSessionHasErrors/assertRedirect as used elsewhere).
Ensure these tests reference the same unique routes/methods
(ServerNetworkTest::test_see_network_page, test_add_ip_address,
test_refresh_pulls_ips_from_server, test_set_ip_as_primary_updates_server_ip,
test_delete_managed_ip_address) so they exercise middleware/policy enforcement
and project-scoped access control.
- Around line 209-223: The test test_set_ip_as_primary_updates_server_ip should
not call SSH::fake() because ManageServerIp::setPrimary only updates DB rows
(servers.ip/servers.local_ip and server_ip_addresses.is_primary) and dispatches
a SocketEventDTO; remove any SSH::fake() setup in this test and rely on the DB
assertions and response check after posting to the servers.network.ips.primary
route; additionally add complementary deny-path tests (e.g., unauthenticated,
forbidden, other-project) for the servers.network.ips.primary endpoint to cover
access control.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 99d3528b-a189-4ed5-9dd9-975bc7fd7fec
📒 Files selected for processing (29)
app/Actions/Server/InstallServer.phpapp/Actions/ServerIp/ManageServerIp.phpapp/Actions/ServerIp/RefreshServerIps.phpapp/Enums/IpAddressFamily.phpapp/Enums/IpAddressStatus.phpapp/Enums/IpAddressType.phpapp/Facades/SSH.phpapp/Http/Controllers/ServerNetworkController.phpapp/Http/Resources/ServerIpAddressResource.phpapp/Jobs/ServerIp/PersistServerIpsJob.phpapp/Jobs/ServerIp/RefreshServerIpsJob.phpapp/Models/Server.phpapp/Models/ServerIpAddress.phpapp/Policies/ServerIpAddressPolicy.phpapp/Support/Testing/SSHFake.phpapp/Tables/Servers/ServerIpAddressTable.phpdatabase/factories/ServerIpAddressFactory.phpdatabase/migrations/2026_06_03_201231_create_server_ip_addresses_table.phpresources/js/components/dialogs/registry.tsresources/js/layouts/server/layout.tsxresources/js/pages/server-network/components/delete.tsxresources/js/pages/server-network/components/form.tsxresources/js/pages/server-network/components/set-primary.tsxresources/js/pages/server-network/index.tsxresources/js/types/server-ip.d.tsresources/views/ssh/network/apply-netplan.blade.phpresources/views/ssh/network/list-network-addresses.blade.phpresources/views/ssh/network/netplan.blade.phptests/Feature/ServerNetworkTest.php
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/Actions/ServerIp/RefreshServerIps.php (1)
145-165: ⚡ Quick winExtract the common discovered-field sync into one helper.
Both existing-row branches now duplicate the same resync assignments for interface, prefix, family, type, primary, and dynamic, with only lifecycle fields differing. Pulling the shared writes into a small private method will reduce the chance that the managed and unmanaged paths drift again.
♻️ Proposed refactor
+ /** + * `@param` array{ip: string, prefix_length: int, family: string, interface: string, dynamic: bool} $entry + */ + private function syncDiscoveredFields(ServerIpAddress $row, array $entry, bool $isPrimary): void + { + $row->interface = $entry['interface']; + $row->prefix_length = $entry['prefix_length']; + $row->family = IpAddressFamily::from($entry['family']); + $row->type = ServerIpAddress::classifyType($entry['ip']); + $row->is_primary = $isPrimary; + $row->is_dynamic = $entry['dynamic']; + } + private function reconcile(Server $server, array $discovered, array $managedIps): void { DB::transaction(function () use ($server, $discovered, $managedIps): void { @@ if ($row instanceof ServerIpAddress) { if ($row->is_managed) { - $row->interface = $entry['interface']; - $row->prefix_length = $entry['prefix_length']; - $row->family = IpAddressFamily::from($entry['family']); - $row->type = ServerIpAddress::classifyType($entry['ip']); - $row->is_primary = $isPrimary; - $row->is_dynamic = $entry['dynamic']; + $this->syncDiscoveredFields($row, $entry, $isPrimary); $row->save(); continue; } - $row->interface = $entry['interface']; - $row->prefix_length = $entry['prefix_length']; - $row->family = IpAddressFamily::from($entry['family']); - $row->type = ServerIpAddress::classifyType($entry['ip']); + $this->syncDiscoveredFields($row, $entry, $isPrimary); $row->status = IpAddressStatus::CONFIGURED; - $row->is_primary = $isPrimary; - $row->is_dynamic = $entry['dynamic']; $row->is_managed = $isManaged; $row->save();🤖 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/Actions/ServerIp/RefreshServerIps.php` around lines 145 - 165, Extract the duplicated discovered-field assignments into a private helper (e.g., syncDiscoveredFields) and call it from both branches in RefreshServerIps::handle (or the method containing the shown diff); the helper should accept the ServerIpAddress $row, the $entry array and $isPrimary flag and set $row->interface, $row->prefix_length, $row->family = IpAddressFamily::from(...), $row->type = ServerIpAddress::classifyType(...), $row->is_primary and $row->is_dynamic (from $entry['dynamic']), then save or return the mutated $row; leave lifecycle-specific fields (like $row->status and $row->is_managed) to be set in the managed/unmanaged branches before or after calling the helper so the two paths only differ in lifecycle handling.
🤖 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.
Nitpick comments:
In `@app/Actions/ServerIp/RefreshServerIps.php`:
- Around line 145-165: Extract the duplicated discovered-field assignments into
a private helper (e.g., syncDiscoveredFields) and call it from both branches in
RefreshServerIps::handle (or the method containing the shown diff); the helper
should accept the ServerIpAddress $row, the $entry array and $isPrimary flag and
set $row->interface, $row->prefix_length, $row->family =
IpAddressFamily::from(...), $row->type = ServerIpAddress::classifyType(...),
$row->is_primary and $row->is_dynamic (from $entry['dynamic']), then save or
return the mutated $row; leave lifecycle-specific fields (like $row->status and
$row->is_managed) to be set in the managed/unmanaged branches before or after
calling the helper so the two paths only differ in lifecycle handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 06ede716-0d3d-4399-84d2-024ed3f93976
📒 Files selected for processing (4)
app/Actions/ServerIp/RefreshServerIps.phpapp/Http/Controllers/ServerNetworkController.phpapp/Support/Testing/SSHFake.phptests/Feature/ServerNetworkTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- app/Support/Testing/SSHFake.php
- app/Http/Controllers/ServerNetworkController.php
saeedvaziry
approved these changes
Jun 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces a comprehensive set of changes to enable advanced management of server IP addresses, including CRUD operations, status tracking, and reconciliation of IPs with the server's actual network configuration. It adds new actions, enums, a controller, and a resource for handling server IP addresses, along with jobs to refresh and persist IPs.
Summary by CodeRabbit
Release Notes