feat(vm-manager): add Cloud-Init support for Unraid VMs#2536
Open
retrozenith wants to merge 13 commits intounraid:masterfrom
Open
feat(vm-manager): add Cloud-Init support for Unraid VMs#2536retrozenith wants to merge 13 commits intounraid:masterfrom
retrozenith wants to merge 13 commits intounraid:masterfrom
Conversation
Implement Cloud-Init configuration UI and backend for automatic VM provisioning on first boot. This enables zero-touch deployment of Linux VMs using cloud images. Features: - Add Cloud-Init section in VM Advanced View with enable toggle - Implement Basic mode with form-based configuration: - Identity: hostname, default user, password, SSH keys, root login - System: timezone - Packages: update/upgrade toggle, package installation list - Commands: arbitrary shell commands on boot - Implement Advanced mode for raw user-data and network-config YAML - Persist all settings to cloud-init.json alongside VM files - Generate VFAT disk image (cloud-init.img) with cidata label - Attach Cloud-Init disk as VirtIO device for optimal performance - Support device type override in libvirt XML generation Technical changes: - Custom.form.php: UI components and save/load logic for create/update - libvirt_helpers.php: create_cloud_init_iso() with VFAT generation - libvirt.php: Added device attribute passthrough for disk elements (required) Tested with: - Ubuntu 24.04.2 LTS Cloud Image (noble-server-cloudimg-amd64.img) Source: https://cloud-images.ubuntu.com/noble/current/ Date: 2026-01-08 - Verified: hostname, timezone, user creation, password auth, ssh key, package installation, and runcmd execution all applied correctly - Confirmed VFAT cidata label detected by Cloud-Init NoCloud datasource
Contributor
WalkthroughAdds Cloud‑Init support (user-data/network-config generation and ISO creation), integrates Cloud‑Init UI and persistence into VM create/edit flows, records per-disk Changes
Sequence DiagramsequenceDiagram
participant User
participant Form as VM Form Handler
participant CloudInit as Cloud‑Init Generator
participant FS as File System
participant Libvirt as VM Manager
User->>Form: Submit VM form (cloudinit enabled)
Form->>Form: Validate inputs / select mode
Form->>CloudInit: generate user-data & network-config
CloudInit->>FS: create_cloud_init_iso (dd, format, mount, write files, unmount)
CloudInit-->>Form: return ISO path or error
Form->>FS: write cloud-init.json / cloud-init.user-data / network-config
Form->>Libvirt: attach cloud-init ISO as disk (include deviceType)
Libvirt->>FS: persist updated VM config
Form->>User: Report creation/update result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![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: 5
🤖 Fix all issues with AI agents
In `@emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php`:
- Around line 53-58: The three file write calls using
file_put_contents($strMountPoint . '/user-data', $strUserData),
file_put_contents($strMountPoint . '/network-config', $strNetworkConfig) and
file_put_contents($strMountPoint . '/meta-data', ...) need explicit error
handling: check each file_put_contents return value for === false and handle
failures (log an error via existing logger or trigger an exception/return an
error code) so a failed write (permission/disk/full) is reported and the caller
can abort/clean up rather than silently producing a bad cloud-init image;
include the target path ($strMountPoint . '/...') in the error message to aid
debugging.
- Around line 62-70: The rmdir($strMountPoint) call is occurring before checking
the umount result, which can remove the mountpoint even when umount failed;
change the order so you first run exec("umount " .
escapeshellarg($strMountPoint) ..., check if $return_var !== 0 and on failure
log the error (using error_log("Cloud-Init image unmount failed: " .
implode("\n", $output))) and return false, and only after a successful umount
call rmdir($strMountPoint); optionally log or handle rmdir failures after the
successful unmount.
In `@emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php`:
- Around line 388-499: Extract the duplicated cloud-init handling into a
reusable helper in libvirt_helpers.php (e.g., function
process_cloud_init_config($strVMPath, $cloudinitPost)) that ensures the VM path
exists, builds and writes cloud-init.json, generates user-data either by calling
generate_cloud_init_userdata($cloudinitPost) for 'basic' mode or using provided
userdata, writes cloud-init.user-data and cloud-init.network-config, and invokes
create_cloud_init_iso($strVMPath, $strUserData, $strNetworkConfig); replace the
duplicated blocks in both the creation flow and the update flow with calls to
process_cloud_init_config($_POST domain path, $_POST['cloudinit']) so all logic
is centralized and retained.
- Around line 178-181: The directory creation uses overly permissive mode 0777
for $strVMPath; change the mkdir call in Custom.form.php to create the VM
directory with restrictive permissions (e.g., 0700 or 0755) and ensure ownership
is set to the appropriate user (use chown/chgrp after mkdir or pass correct
uid/gid where applicable); keep the existing is_dir check and recursive flag but
replace 0777 with the chosen restrictive mode and add a safe chown to the
daemon/user that should own VM data.
- Around line 205-272: The code concatenates user-controlled values into
$strUserData inside the cloudInitMode == 'basic' branch (variables: $hostname,
$timezone, $user, $pass, $keys, $packages, $runcmd, arrays
$arrPkg/$arrKeys/$arrCmd), which creates a YAML injection risk; stop building
YAML by string concatenation and instead construct a PHP associative
array/structure representing the cloud-config (users, packages, runcmd, etc.)
and serialize it with a proper YAML library (e.g., yaml_emit or
Symfony\Component\Yaml\Yaml::dump) so all values are correctly escaped/quoted,
or if a library is unavailable, ensure every scalar and multiline value is
safely escaped/quoted (use literal block style for multiline fields like runcmd
and parse $keys/$packages into arrays) before writing to $strUserData.
🧹 Nitpick comments (2)
emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php (1)
46-51: Consider resetting$outputarray before eachexeccall.The
$outputarray accumulates results from previous commands (format, mount, umount). If mount fails, the error log at line 49 may include output from the format command, making debugging harder.Proposed fix
// Mount image + $output = []; exec("mount -o loop " . escapeshellarg($strImgPath) . " " . escapeshellarg($strMountPoint) . " 2>&1", $output, $return_var);Similarly before the umount call:
// Sync and Unmount exec("sync"); + $output = []; exec("umount " . escapeshellarg($strMountPoint) . " 2>&1", $output, $return_var);emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php (1)
563-575: Remove commented-out debug logging.Debug logging code is commented out but left in the codebase. This adds noise and could accidentally be enabled in production.
🧹 Suggested fix
- // Debug logging to specific file - // file_put_contents("/var/log/cloudinit_debug.log", "Loading config for $strVMName from $strVMPath\n", FILE_APPEND); - // Load JSON config first for UI fields if (file_exists($strVMPath . '/cloud-init.json')) { $json_content = file_get_contents($strVMPath . '/cloud-init.json'); $arrCloudInitConfig = json_decode($json_content, true); if ($arrCloudInitConfig) { $arrConfig['cloudinit'] = array_merge($arrConfig['cloudinit'], $arrCloudInitConfig); - // file_put_contents("/var/log/cloudinit_debug.log", "Loaded JSON: " . print_r($arrCloudInitConfig, true) . "\n", FILE_APPEND); - } else { - // file_put_contents("/var/log/cloudinit_debug.log", "Failed to decode JSON: $json_content\n", FILE_APPEND); }
Exclude xmldesc textarea from form view POST data serialization
Refactored the duplicate Cloud-Init YAML generation logic into a reusable helper function 'generate_cloud_init_userdata'. This updates both the VM creation and update paths to use the shared function. Addresses CodeRabbit feedback: unraid#2536 (comment)
Replaced manual string concatenation in Cloud-Init YAML generation with structured array construction and a safe YAML encoder 'my_yaml_encode'. This ensures user input is properly quoted and escaped, preventing potential YAML injection attacks where new keys could be injected via newlines. Addresses CodeRabbit feedback: unraid#2536 (comment)
Implemented feedback from CodeRabbit to improve robustness and security: - Added error handling for Cloud-Init file writes in 'libvirt_helpers.php'. - Corrected race condition ensuring `rmdir` only runs after successful `umount`. - Deepened security by restricting new VM directory permissions to `0755`. - Added cleanup of `cloud-init.*` files in 'domain_delete'. Addresses following CodeRabbit feedbacks: - unraid#2536 (comment) - unraid#2536 (comment) - unraid#2536 (comment)
Removed temporary file_put_contents debug logging calls and associated dead code (empty else block) from Custom.form.php that were used during development of the Cloud-Init refactoring.
Resets the $output array before exec calls in create_cloud_init_iso to prevent output accumulation. This ensures that error logs for a specific command (like mount) do not contain unrelated output from previous commands (like formatting).
Contributor
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 (1)
emhttp/plugins/dynamix.vm.manager/include/libvirt.php (1)
1778-1801: Guard$pathinfo/$diskwhen falling back to default VM directories.When the primary disk path is missing,
$pathinfoand$diskare undefined but still used to build paths andunlink(). This raises notices and can target"$dir/.cfg"/"$dir/.xml"accidentally.🔧 Suggested fix (safe defaults + conditional deletes)
- $dir = ''; + $dir = ''; + $disk = null; + $pathinfo = null; if (!empty($disks) && array_key_exists('file', $disks[0])) { $disk = $disks[0]['file']; $pathinfo = pathinfo($disk); $dir = $pathinfo['dirname']; } elseif (is_dir("/mnt/user/domains/$domain")) { $dir = "/mnt/user/domains/$domain"; } elseif (is_dir("/mnt/cache/domains/$domain")) { $dir = "/mnt/cache/domains/$domain"; } if ($dir) { // remove the vm config $cfg_vm = $dir.'/'.$domain.'.cfg'; if (is_file($dir.'/cloud-init.img')) unlink($dir.'/cloud-init.img'); if (is_file($dir.'/cloud-init.json')) unlink($dir.'/cloud-init.json'); if (is_file($dir.'/cloud-init.user-data')) unlink($dir.'/cloud-init.user-data'); if (is_file($dir.'/cloud-init.network-config')) unlink($dir.'/cloud-init.network-config'); if (is_file($cfg_vm)) unlink($cfg_vm); - $cfg = $dir.'/'.$pathinfo['filename'].'.cfg'; - $xml = $dir.'/'.$pathinfo['filename'].'.xml'; - if (is_file($disk)) unlink($disk); - if (is_file($cfg)) unlink($cfg); - if (is_file($xml)) unlink($xml); + if ($pathinfo) { + $cfg = $dir.'/'.$pathinfo['filename'].'.cfg'; + $xml = $dir.'/'.$pathinfo['filename'].'.xml'; + if (is_file($cfg)) unlink($cfg); + if (is_file($xml)) unlink($xml); + } + if ($disk && is_file($disk)) unlink($disk);
🤖 Fix all issues with AI agents
In `@emhttp/plugins/dynamix.vm.manager/include/libvirt.php`:
- Around line 619-621: Validate and whitelist the disk device type before
emitting it into the libvirt XML: replace the direct use of $disk['deviceType']
when assigning $strDevice (used in the $diskstr XML assembly) with a whitelist
check that only allows "disk" or "lun" and falls back to "disk" for any
other/empty value; perform this validation where $strDevice is set so the
subsequent "$diskstr .= ... device='".$strDevice."'" construction can only ever
include an allowed value.
In `@emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php`:
- Around line 174-227: The cloud-init ISO creation currently continues silently
on failure; after calling create_cloud_init_iso($strVMPath, $strUserData,
$strNetworkConfig) check the returned $isoPath and if it is false/empty, stop
further processing and surface an error (e.g., set an error message and abort
the form submission/VM creation flow) instead of proceeding to add the disk;
update the block that currently does "if ($isoPath) { ...
$_POST['disk'][$newDiskIndex] = ..." to handle the failure case by
returning/throwing an error or setting a visible failure state so callers of
create_cloud_init_iso, the cloud-init.json write and the VM creation are not
performed when $isoPath is invalid.
- Around line 323-367: Capture the ISO path returned by create_cloud_init_iso by
changing the call to assign its return to $isoPath
(create_cloud_init_iso($strVMPath, $strUserData, $strNetworkConfig) → $isoPath =
create_cloud_init_iso(...)), then if $isoPath is non-empty append a new disk
entry to $_POST['disk'] (use count($_POST['disk'] ?? []) for index) with keys
'source' => $isoPath, 'bus' => 'sata', 'type' => 'cdrom' so the cloud-init ISO
is attached during the update path; relevant symbols: create_cloud_init_iso,
$isoPath, $_POST['disk'], $strVMPath, $strUserData, $strNetworkConfig.
- Abort VM creation if cloud-init ISO generation fails, preventing invalid VM states. - Capture generated ISO path during VM updates and automatically attach it as a SATA CD-ROM device. Addresses CodeRabbit feedback: unraid#2536 (comment)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php`:
- Around line 222-229: The new disk added to $_POST['disk'] uses keys that don't
match the update path and what config_to_xml() expects; change the disk entry
created in the add-Cloud-Init block so it uses the same schema as
config_to_xml(): include 'deviceType' and 'image' (not 'source'/'type'), set
'driver' and 'bus' consistently (e.g., 'driver'=>'raw', 'bus'=>'virtio') so
$_POST['disk'][$newDiskIndex] uses the same keys as other code paths and
config_to_xml() can process it.
- The recent commit 8ad110d incorrectly used 'source' and 'type' keys for the Cloud-Init ISO disk attachment in the update path. - This change standardizes the schema to use 'deviceType', 'image', 'driver', and 'bus' keys, allowing config_to_xml to correctly process the disk. Fixes regression introduced in: unraid@8ad110d
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php`:
- Around line 368-378: The update path currently calls
create_cloud_init_iso($strVMPath, $strUserData, $strNetworkConfig) and silently
continues if it returns false, causing cloud-init to be omitted; change this to
mirror the create path by checking $isoPath and, on failure, log/emit an error
and halt the update (e.g., return/exit with an error message) so the user is
notified and the VM update does not proceed without the cloud-init disk; update
the block around create_cloud_init_iso and the subsequent $_POST['disk']
population to perform this error check and early exit.
- Ensure VM update is aborted if Cloud-Init ISO creation fails. - This consistency prevents silent failures where the VM would be updated without the intended Cloud-Init configuration. Addresses CodeRabbit feedback: unraid#2536 (comment)
…andling - Fixes a bug where Cloud-Init disks were duplicated on VM updates by implementing smarter disk replacement logic. - Adds support for custom ISO paths with automatic directory handling and validation. - Implements security hardening to restrict paths to /mnt/ and prevent injection. - improving error handling to strictly abort operations on ISO creation failure.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php`:
- Around line 217-229: The current start-with '/mnt/' check can be bypassed via
path traversal; update the validation around $customISOPath (the same checks
that use strpos and preg_match) to explicitly reject any '..' path segments
before proceeding (e.g., detect sequences like '/..' or '/./..' via a regex such
as checking for '(^|/)\\.\\.(/|$)' or a simple strpos for '..'), and apply this
hardening in both the create and update blocks where $customISOPath is validated
so paths containing '..' are rejected early.
🧹 Nitpick comments (1)
emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php (1)
200-202: Remove the duplicateuserdataentry in the Cloud‑Init config array.
Duplicate keys are silently overwritten and add confusion for future edits.🧹 Suggested cleanup (apply in both blocks)
- 'userdata' => $_POST['cloudinit']['userdata'] ?? '', - 'userdata' => $_POST['cloudinit']['userdata'] ?? '', + 'userdata' => $_POST['cloudinit']['userdata'] ?? '',Also applies to: 367-368
- Reject paths containing '..' segments to prevent directory traversal outside /mnt/. - This ensures that the 'starts with /mnt/' check cannot be bypassed. Addresses CodeRabbit feedback: unraid#2536 (comment)
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
1 participant
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.
Details:
Implement Cloud-Init configuration UI and backend for automatic VM provisioning on first boot. This enables zero-touch deployment of Linux VMs using cloud images.
Previews:
Features:
Technical changes:
Tested with:
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.