diff --git a/docs/review/findings.json b/docs/review/findings.json index 5a82291..4db20e6 100644 --- a/docs/review/findings.json +++ b/docs/review/findings.json @@ -2816,6 +2816,37 @@ "verified_by": "dotnet build + dotnet test (382 passed)" } }, + { + "id": "F086", + "title": "New-PveVm -DiskSize and New-PveContainer -RootFsSize pass unit suffix verbatim, LVM rejects 'NG'", + "category": "api_contract", + "severity": "high", + "status": "resolved", + "first_detected": "2026-05-20", + "github_issue": 58, + "files": [ + "src/PSProxmoxVE/Cmdlets/Vms/NewPveVmCmdlet.cs", + "src/PSProxmoxVE/Cmdlets/Containers/NewPveContainerCmdlet.cs" + ], + "description": "Disk and rootfs sizes are interpolated directly into the disk spec as ':'. The parameter docstring advertises 'e.g. 32G' but on LVM/LVM-thin storages PVE parses the value after the colon as a volume name unless it is a bare integer, failing with 'unable to parse lvm volume name \"32G\"'. File-backed storages (NFS, directory) accept either form, masking the bug in mixed environments.", + "scan_history": [ + { + "scan_date": "2026-05-20", + "local_id": null, + "status": "new" + }, + { + "scan_date": "2026-05-20", + "local_id": null, + "status": "fixed" + } + ], + "resolution": { + "scan_date": "2026-05-20", + "evidence": "Added SizeParser.NormalizeToGibibytes() which strips G/GB/T/TB suffixes, rejects sub-GB units with a clear error, and converts TB overflows to ArgumentException. New-PveVm and New-PveContainer normalize -DiskSize and -RootFsSize before ShouldProcess so typos are caught with -WhatIf, regardless of whether the matching -DiskStorage/-RootFsStorage was supplied.", + "verified_by": "dotnet build + dotnet test (577 passed, 32 SizeParserTests) + Pester (39 passed, new DiskSize/RootFsSize validation contexts)" + } + }, { "id": "F087", "title": "HttpClient uses 100s default timeout; Send-PveFile and other long-running calls fail on large payloads", @@ -2832,7 +2863,7 @@ "src/PSProxmoxVE/Cmdlets/Storage/InvokePveStorageDownloadCmdlet.cs", "src/PSProxmoxVE/Cmdlets/Connection/ConnectPveServerCmdlet.cs" ], - "description": "PveHttpClient constructs HttpClient without setting Timeout, so .NET's 100s default applies to every request. Send-PveFile, Invoke-PveStorageDownload, and Connect-PveServer expose no way to override it, so multi-GB ISO uploads on a real LAN reliably trip the 100s timeout with TaskCanceledException.", + "description": "PveHttpClient constructs HttpClient without setting Timeout, so .NET's 100s default applies to every request. Send-PveFile, Invoke-PveStorageDownload, and Connect-PveServer expose no way to override it, so multi-GB ISO uploads on a real LAN reliably trip the 100s timeout with TaskCanceledException. PveHttpClient.SendAsync also failed to surface the timeout as a PveApiException, leaking the raw TaskCanceledException to callers.", "scan_history": [ { "scan_date": "2026-05-20", @@ -2847,8 +2878,8 @@ ], "resolution": { "scan_date": "2026-05-20", - "evidence": "Added PveSession.Timeout (default 100s) and a TimeSpan? override on PveHttpClient. Connect-PveServer exposes -TimeoutSeconds to set the session-default; Send-PveFile and Invoke-PveStorageDownload expose -TimeoutSeconds with a 30-minute implicit default so large uploads/downloads do not trip the 100s HttpClient default. -TimeoutSeconds 0 means Timeout.InfiniteTimeSpan.", - "verified_by": "dotnet build + dotnet test (550 passed, 5 new timeout tests)" + "evidence": "Added PveSession.Timeout (default 100s) and a TimeSpan? override on PveHttpClient. Connect-PveServer exposes -TimeoutSeconds to set the session-default; Send-PveFile and Invoke-PveStorageDownload expose -TimeoutSeconds with a 30-minute implicit default so large uploads/downloads do not trip the 100s HttpClient default. -TimeoutSeconds 0 means Timeout.InfiniteTimeSpan. PveHttpClient.SendAsync now catches the TimeoutException-wrapped TaskCanceledException and rethrows it as PveApiException(RequestTimeout) with the resource path.", + "verified_by": "dotnet build + dotnet test (passed including new SendAsync_TimeoutFires xUnit test) + Pester (-TimeoutSeconds coverage on all three cmdlets)" } } ] diff --git a/src/PSProxmoxVE.Core/Utilities/SizeParser.cs b/src/PSProxmoxVE.Core/Utilities/SizeParser.cs new file mode 100644 index 0000000..f3fa34d --- /dev/null +++ b/src/PSProxmoxVE.Core/Utilities/SizeParser.cs @@ -0,0 +1,79 @@ +using System; +using System.Globalization; +using System.Text.RegularExpressions; + +namespace PSProxmoxVE.Core.Utilities +{ + /// + /// Parses storage size strings (e.g. "32G", "1T", "60") and normalizes them + /// to a bare integer count of gibibytes for use in PVE disk specs. + /// + /// + /// PVE accepts a size suffix on file-backed storages (NFS, directory) but parses + /// the value after the colon as a volume name on LVM-backed storages — so + /// local-lvm:32G fails with "unable to parse lvm volume name '32G'" while + /// local-lvm:32 works on every storage type. Cmdlets that build disk specs + /// must normalize size inputs through this helper before joining with the storage. + /// + public static class SizeParser + { + private static readonly Regex Pattern = new Regex( + @"^\s*(?\d+)\s*(?[A-Za-z]*)\s*$", + RegexOptions.Compiled); + + /// + /// Parses a size string and returns the value as a bare integer count of GiB. + /// Accepts values like "60", "60G", "60GB" (= 60), "1T", "1TB" (= 1024). + /// Sub-GB units are rejected because PVE disk allocation is GB-granular. + /// + /// The size string supplied by the user. + /// Parameter name used in the error message. + /// The size in whole GiB as a string, suitable for direct use in disk specs. + /// The input cannot be parsed or uses an unsupported unit. + public static string NormalizeToGibibytes(string value, string parameterName = "size") + { + if (string.IsNullOrWhiteSpace(value)) + throw new ArgumentException($"{parameterName} must not be null or empty.", parameterName); + + var match = Pattern.Match(value); + if (!match.Success) + throw new ArgumentException( + $"{parameterName} '{value}' is not a valid size. Expected a positive integer optionally suffixed with G, GB, T, or TB (e.g. '32G', '1T', '60').", + parameterName); + + if (!long.TryParse(match.Groups["num"].Value, NumberStyles.Integer, CultureInfo.InvariantCulture, out var num) || num <= 0) + throw new ArgumentException( + $"{parameterName} '{value}' must be a positive integer.", + parameterName); + + var unit = match.Groups["unit"].Value.ToUpperInvariant(); + long gib; + switch (unit) + { + case "": + case "G": + case "GB": + case "GIB": + gib = num; + break; + case "T": + case "TB": + case "TIB": + try { gib = checked(num * 1024L); } + catch (OverflowException) + { + throw new ArgumentException( + $"{parameterName} '{value}' is too large to represent in GiB.", + parameterName); + } + break; + default: + throw new ArgumentException( + $"{parameterName} '{value}' uses unsupported unit '{unit}'. Use G, GB, T, or TB. Sub-GB units (M, MB, K, KB) are not supported by PVE disk allocation.", + parameterName); + } + + return gib.ToString(CultureInfo.InvariantCulture); + } + } +} diff --git a/src/PSProxmoxVE/Cmdlets/Containers/NewPveContainerCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Containers/NewPveContainerCmdlet.cs index c586338..42b4d9d 100644 --- a/src/PSProxmoxVE/Cmdlets/Containers/NewPveContainerCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Containers/NewPveContainerCmdlet.cs @@ -6,6 +6,7 @@ using Newtonsoft.Json.Linq; using PSProxmoxVE.Core.Client; using PSProxmoxVE.Core.Models.Vms; using PSProxmoxVE.Core.Services; +using PSProxmoxVE.Core.Utilities; namespace PSProxmoxVE.Cmdlets.Containers { @@ -59,9 +60,13 @@ namespace PSProxmoxVE.Cmdlets.Containers public int? Cores { get; set; } /// - /// Size of the root filesystem (e.g., "8G"). + /// + /// Size of the root filesystem. Accepts a bare integer in GiB ("8") or a value + /// suffixed with G/GB/T/TB (case-insensitive); the value is normalized to a + /// bare GiB count before being sent to the API. + /// /// - [Parameter(Mandatory = false, HelpMessage = "Size of the root filesystem (e.g. 8G).")] + [Parameter(Mandatory = false, HelpMessage = "Size of the root filesystem in GiB (e.g. 8 or 8G).")] public string? RootFsSize { get; set; } /// @@ -125,6 +130,13 @@ namespace PSProxmoxVE.Cmdlets.Containers protected override void ProcessRecord() { + // Validate -RootFsSize before ShouldProcess so typos like "512M" are rejected + // even with -WhatIf, and so the error is raised regardless of whether + // -RootFsStorage is also supplied. + string? rootFsSizeGib = null; + if (!string.IsNullOrEmpty(RootFsSize)) + rootFsSizeGib = SizeParser.NormalizeToGibibytes(RootFsSize!, nameof(RootFsSize)); + if (!ShouldProcess($"Container on node '{Node}'", "New-PveContainer")) return; @@ -160,8 +172,8 @@ namespace PSProxmoxVE.Cmdlets.Containers if (!string.IsNullOrEmpty(RootFsStorage)) { var rootFsValue = RootFsStorage!; - if (!string.IsNullOrEmpty(RootFsSize)) - rootFsValue += $":{RootFsSize}"; + if (rootFsSizeGib != null) + rootFsValue += $":{rootFsSizeGib}"; config["rootfs"] = rootFsValue; } diff --git a/src/PSProxmoxVE/Cmdlets/Vms/NewPveVmCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Vms/NewPveVmCmdlet.cs index fb363f2..0c90225 100644 --- a/src/PSProxmoxVE/Cmdlets/Vms/NewPveVmCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Vms/NewPveVmCmdlet.cs @@ -4,6 +4,7 @@ using Newtonsoft.Json.Linq; using PSProxmoxVE.Core.Client; using PSProxmoxVE.Core.Models.Vms; using PSProxmoxVE.Core.Services; +using PSProxmoxVE.Core.Utilities; namespace PSProxmoxVE.Cmdlets.Vms { @@ -74,9 +75,13 @@ namespace PSProxmoxVE.Cmdlets.Vms public string? Machine { get; set; } /// - /// Size of the primary disk (e.g., "32G"). + /// + /// Size of the primary disk. Accepts a bare integer in GiB ("32") or a value + /// suffixed with G/GB/T/TB (case-insensitive); the value is normalized to a + /// bare GiB count before being sent to the API. + /// /// - [Parameter(Mandatory = false, HelpMessage = "Size of the primary disk (e.g. 32G).")] + [Parameter(Mandatory = false, HelpMessage = "Size of the primary disk in GiB (e.g. 32 or 32G).")] public string? DiskSize { get; set; } /// @@ -123,6 +128,13 @@ namespace PSProxmoxVE.Cmdlets.Vms protected override void ProcessRecord() { + // Validate -DiskSize before ShouldProcess so typos like "512M" are rejected + // even with -WhatIf, and so the error is raised regardless of whether + // -DiskStorage is also supplied. + string? diskSizeGib = null; + if (!string.IsNullOrEmpty(DiskSize)) + diskSizeGib = SizeParser.NormalizeToGibibytes(DiskSize!, nameof(DiskSize)); + if (!ShouldProcess($"VM on node '{Node}'", "New-PveVm")) return; @@ -161,9 +173,9 @@ namespace PSProxmoxVE.Cmdlets.Vms if (!string.IsNullOrEmpty(OsType)) config["ostype"] = OsType!; - if (!string.IsNullOrEmpty(DiskStorage) && !string.IsNullOrEmpty(DiskSize)) + if (!string.IsNullOrEmpty(DiskStorage) && diskSizeGib != null) { - var diskValue = $"{DiskStorage}:{DiskSize}"; + var diskValue = $"{DiskStorage}:{diskSizeGib}"; if (!string.IsNullOrEmpty(DiskFormat)) diskValue += $",format={DiskFormat}"; config["virtio0"] = diskValue; diff --git a/tests/PSProxmoxVE.Core.Tests/Utilities/SizeParserTests.cs b/tests/PSProxmoxVE.Core.Tests/Utilities/SizeParserTests.cs new file mode 100644 index 0000000..4961215 --- /dev/null +++ b/tests/PSProxmoxVE.Core.Tests/Utilities/SizeParserTests.cs @@ -0,0 +1,90 @@ +using System; +using PSProxmoxVE.Core.Utilities; +using Xunit; + +namespace PSProxmoxVE.Core.Tests.Utilities +{ + public class SizeParserTests + { + [Theory] + [InlineData("32", "32")] + [InlineData("32G", "32")] + [InlineData("32g", "32")] + [InlineData("32GB", "32")] + [InlineData("32gb", "32")] + [InlineData("32GiB", "32")] + [InlineData("1T", "1024")] + [InlineData("1t", "1024")] + [InlineData("1TB", "1024")] + [InlineData("1TiB", "1024")] + [InlineData("2T", "2048")] + [InlineData(" 60G ", "60")] + [InlineData("60 G", "60")] + public void NormalizeToGibibytes_AcceptedInputs_ReturnsBareGibibyteString(string input, string expected) + { + var result = SizeParser.NormalizeToGibibytes(input); + Assert.Equal(expected, result); + } + + [Theory] + [InlineData("512M")] + [InlineData("512MB")] + [InlineData("1024K")] + [InlineData("1024KB")] + [InlineData("100B")] + [InlineData("1P")] + [InlineData("1PB")] + public void NormalizeToGibibytes_UnsupportedUnit_Throws(string input) + { + var ex = Assert.Throws(() => SizeParser.NormalizeToGibibytes(input)); + Assert.Contains("unsupported unit", ex.Message, StringComparison.OrdinalIgnoreCase); + } + + [Theory] + [InlineData("")] + [InlineData(" ")] + [InlineData(null)] + public void NormalizeToGibibytes_EmptyOrWhitespace_Throws(string? input) + { + Assert.Throws(() => SizeParser.NormalizeToGibibytes(input!)); + } + + [Theory] + [InlineData("abc")] + [InlineData("G")] + [InlineData("-32")] + [InlineData("32.5G")] + [InlineData("32 G B")] + public void NormalizeToGibibytes_Malformed_Throws(string input) + { + Assert.Throws(() => SizeParser.NormalizeToGibibytes(input)); + } + + [Theory] + [InlineData("0")] + [InlineData("0G")] + public void NormalizeToGibibytes_Zero_Throws(string input) + { + var ex = Assert.Throws(() => SizeParser.NormalizeToGibibytes(input)); + Assert.Contains("positive", ex.Message, StringComparison.OrdinalIgnoreCase); + } + + [Fact] + public void NormalizeToGibibytes_UsesProvidedParameterNameInError() + { + var ex = Assert.Throws(() => SizeParser.NormalizeToGibibytes("512M", "DiskSize")); + Assert.Equal("DiskSize", ex.ParamName); + Assert.Contains("DiskSize", ex.Message); + } + + [Fact] + public void NormalizeToGibibytes_TerabyteOverflow_ThrowsArgumentException() + { + // long.MaxValue with a T suffix overflows when multiplied by 1024. + var input = long.MaxValue.ToString(System.Globalization.CultureInfo.InvariantCulture) + "T"; + var ex = Assert.Throws(() => SizeParser.NormalizeToGibibytes(input, "DiskSize")); + Assert.Equal("DiskSize", ex.ParamName); + Assert.Contains("too large", ex.Message, StringComparison.OrdinalIgnoreCase); + } + } +} diff --git a/tests/PSProxmoxVE.Tests/Containers/New-PveContainer.Tests.ps1 b/tests/PSProxmoxVE.Tests/Containers/New-PveContainer.Tests.ps1 new file mode 100644 index 0000000..8b989f8 --- /dev/null +++ b/tests/PSProxmoxVE.Tests/Containers/New-PveContainer.Tests.ps1 @@ -0,0 +1,68 @@ +#Requires -Module Pester +<# +.SYNOPSIS + Pester 5 tests for New-PveContainer. + All tests are fully offline — no live Proxmox VE target is required. +#> + +BeforeAll { + . $PSScriptRoot/../_TestHelper.ps1 +} + +Describe 'New-PveContainer' { + + Context 'Command existence' { + It 'Should be available after module import' { + Get-Command 'New-PveContainer' -ErrorAction SilentlyContinue | + Should -Not -BeNullOrEmpty + } + + It 'Should be a CmdletInfo (binary cmdlet)' { + (Get-Command 'New-PveContainer').CommandType | Should -Be 'Cmdlet' + } + } + + Context 'ShouldProcess support' { + BeforeAll { + $script:Cmd = Get-Command 'New-PveContainer' + } + + It 'Should support ShouldProcess (WhatIf parameter present)' { + $script:Cmd.Parameters.ContainsKey('WhatIf') | Should -BeTrue + } + + It 'Should support ShouldProcess (Confirm parameter present)' { + $script:Cmd.Parameters.ContainsKey('Confirm') | Should -BeTrue + } + } + + Context 'RootFsSize validation' { + # Validation runs before ShouldProcess so -WhatIf is enough to exercise it + # without an active session. + + It 'Should reject sub-GB units (e.g. 512M)' { + { New-PveContainer -Node 'pve-node1' -RootFsStorage 'local-lvm' -RootFsSize '512M' -WhatIf -ErrorAction Stop } | + Should -Throw '*unsupported unit*' + } + + It 'Should reject sub-GB units even when -RootFsStorage is omitted' { + { New-PveContainer -Node 'pve-node1' -RootFsSize '512M' -WhatIf -ErrorAction Stop } | + Should -Throw '*unsupported unit*' + } + + It 'Should reject malformed input (e.g. 8.5G)' { + { New-PveContainer -Node 'pve-node1' -RootFsStorage 'local-lvm' -RootFsSize '8.5G' -WhatIf -ErrorAction Stop } | + Should -Throw '*not a valid size*' + } + + It 'Should accept a bare integer with -WhatIf' { + { New-PveContainer -Node 'pve-node1' -RootFsStorage 'local-lvm' -RootFsSize '8' -WhatIf -ErrorAction Stop } | + Should -Not -Throw + } + + It 'Should accept "8G" with -WhatIf' { + { New-PveContainer -Node 'pve-node1' -RootFsStorage 'local-lvm' -RootFsSize '8G' -WhatIf -ErrorAction Stop } | + Should -Not -Throw + } + } +} diff --git a/tests/PSProxmoxVE.Tests/Vms/New-PveVm.Tests.ps1 b/tests/PSProxmoxVE.Tests/Vms/New-PveVm.Tests.ps1 index d3da01d..e26116e 100644 --- a/tests/PSProxmoxVE.Tests/Vms/New-PveVm.Tests.ps1 +++ b/tests/PSProxmoxVE.Tests/Vms/New-PveVm.Tests.ps1 @@ -133,4 +133,39 @@ Describe 'New-PveVm' { Should -Throw '*No active Proxmox VE session*' } } + + Context 'DiskSize validation' { + # Validation runs before ShouldProcess so -WhatIf is enough to exercise it + # without an active session. + + It 'Should reject sub-GB units (e.g. 512M)' { + { New-PveVm -Node 'pve-node1' -DiskStorage 'local-lvm' -DiskSize '512M' -WhatIf -ErrorAction Stop } | + Should -Throw '*unsupported unit*' + } + + It 'Should reject sub-GB units even when -DiskStorage is omitted' { + { New-PveVm -Node 'pve-node1' -DiskSize '512M' -WhatIf -ErrorAction Stop } | + Should -Throw '*unsupported unit*' + } + + It 'Should reject malformed input (e.g. 32.5G)' { + { New-PveVm -Node 'pve-node1' -DiskStorage 'local-lvm' -DiskSize '32.5G' -WhatIf -ErrorAction Stop } | + Should -Throw '*not a valid size*' + } + + It 'Should accept a bare integer with -WhatIf' { + { New-PveVm -Node 'pve-node1' -DiskStorage 'local-lvm' -DiskSize '32' -WhatIf -ErrorAction Stop } | + Should -Not -Throw + } + + It 'Should accept "32G" with -WhatIf' { + { New-PveVm -Node 'pve-node1' -DiskStorage 'local-lvm' -DiskSize '32G' -WhatIf -ErrorAction Stop } | + Should -Not -Throw + } + + It 'Should accept "1T" with -WhatIf' { + { New-PveVm -Node 'pve-node1' -DiskStorage 'local-lvm' -DiskSize '1T' -WhatIf -ErrorAction Stop } | + Should -Not -Throw + } + } }