* [pve-devel] [PATCH storage] check volume access: handle new 'ct-vol' and 'vm-vol' vtypes
@ 2025-07-30 13:23 Fiona Ebner
2025-07-30 13:47 ` Fabian Grünbichler
0 siblings, 1 reply; 2+ messages in thread
From: Fiona Ebner @ 2025-07-30 13:23 UTC (permalink / raw)
To: pve-devel
In the tests, legacy volids resulting in 'images' vtype will be
allowed for both 'ct-vol' and 'vm-vol'.
New test cases for new-form guest image volids are added too.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
Is based on top of Wolfgang's staff repo as well as [0].
[0]: https://lore.proxmox.com/pve-devel/20250730130506.96278-1-f.ebner@proxmox.com/T/#u
src/PVE/Storage.pm | 31 ++++++++++++++++++---
src/test/run_volume_access_tests.pl | 43 +++++++++++++++++++++++++++++
2 files changed, 70 insertions(+), 4 deletions(-)
diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index 91a0278..e6dabc3 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -646,9 +646,32 @@ sub check_volume_access {
if ($sid) {
my ($vtype, undef, $ownervm) = parse_volname($cfg, $volid);
- # Need to allow 'images' when expecting 'rootdir' too - not cleanly separated in plugins.
- die "unable to use volume $volid - content type needs to be '$type'\n"
- if defined($type) && $vtype ne $type && ($type ne 'rootdir' || $vtype ne 'images');
+ my $is_guest_image_vtype =
+ $vtype eq 'ct-vol' || $vtype eq 'vm-vol' || $vtype eq 'images' || $vtype eq 'rootdir';
+
+ if (defined($type)) {
+ my $msg = "unable to use volume $volid - volume type needs to be '$type'";
+
+ # TODO PVE 10.x die or remove completely, once all callers are adapted
+ $type = 'vm-vol' if $type eq 'images';
+ $type = 'ct-vol' if $type eq 'rootdir';
+
+ if ($is_guest_image_vtype) {
+ if ($type eq 'vm-vol') {
+ die "$msg or 'images'\n" if $vtype ne $type && $vtype ne 'images';
+ } elsif ($type eq 'ct-vol') {
+ # need to allow 'images' when expecting 'ct-vol' too for backwards-compat
+ die "$msg or 'rootdir' or 'images'\n"
+ if $vtype ne $type && $vtype ne 'rootdir' && $vtype ne 'images';
+ } else {
+ # $type is not for guest-images, but $vtype is, disallow
+ die "$msg\n";
+ }
+ } else {
+ # can check directly
+ die "$msg\n" if $vtype ne $type;
+ }
+ }
return if $rpcenv->check($user, "/storage/$sid", ['Datastore.Allocate'], 1);
@@ -664,7 +687,7 @@ sub check_volume_access {
} elsif ($vtype eq 'backup' && $ownervm) {
$rpcenv->check($user, "/storage/$sid", ['Datastore.AllocateSpace']);
$rpcenv->check($user, "/vms/$ownervm", ['VM.Backup']);
- } elsif (($vtype eq 'images' || $vtype eq 'rootdir') && $ownervm) {
+ } elsif ($is_guest_image_vtype && $ownervm) {
$rpcenv->check($user, "/storage/$sid", ['Datastore.Audit']);
$rpcenv->check($user, "/vms/$ownervm", ['VM.Config.Disk']);
} else {
diff --git a/src/test/run_volume_access_tests.pl b/src/test/run_volume_access_tests.pl
index ce9fc2e..9744b65 100755
--- a/src/test/run_volume_access_tests.pl
+++ b/src/test/run_volume_access_tests.pl
@@ -78,6 +78,28 @@ my @tests = (
'backup' => 1,
},
},
+ {
+ volid => 'dir:100/vol-vm-100-disk-0.qcow2',
+ denied_users => {
+ 'backup@pve' => 1,
+ 'dsaudit@pve' => 1,
+ },
+ allowed_types => {
+ 'images' => 1,
+ 'vm-vol' => 1,
+ },
+ },
+ {
+ volid => 'dir:100/vol-ct-100-disk-0.raw',
+ denied_users => {
+ 'backup@pve' => 1,
+ 'dsaudit@pve' => 1,
+ },
+ allowed_types => {
+ 'rootdir' => 1,
+ 'ct-vol' => 1,
+ },
+ },
{
volid => 'dir:100/vm-100-disk-0.qcow2',
denied_users => {
@@ -87,6 +109,8 @@ my @tests = (
allowed_types => {
'images' => 1,
'rootdir' => 1,
+ 'ct-vol' => 1,
+ 'vm-vol' => 1,
},
},
{
@@ -105,6 +129,17 @@ my @tests = (
'iso' => 1,
},
},
+ {
+ volid => 'dir:111/subvol-ct-111-disk-0.subvol',
+ denied_users => {
+ 'backup@pve' => 1,
+ 'dsaudit@pve' => 1,
+ },
+ allowed_types => {
+ 'rootdir' => 1,
+ 'ct-vol' => 1,
+ },
+ },
{
volid => 'dir:111/subvol-111-disk-0.subvol',
denied_users => {
@@ -114,6 +149,8 @@ my @tests = (
allowed_types => {
'images' => 1,
'rootdir' => 1,
+ 'ct-vol' => 1,
+ 'vm-vol' => 1,
},
},
# test different VM IDs
@@ -138,6 +175,8 @@ my @tests = (
allowed_types => {
'images' => 1,
'rootdir' => 1,
+ 'ct-vol' => 1,
+ 'vm-vol' => 1,
},
},
{
@@ -155,6 +194,8 @@ my @tests = (
allowed_types => {
'images' => 1,
'rootdir' => 1,
+ 'ct-vol' => 1,
+ 'vm-vol' => 1,
},
},
{
@@ -184,6 +225,8 @@ my @tests = (
allowed_types => {
'images' => 1,
'rootdir' => 1,
+ 'ct-vol' => 1,
+ 'vm-vol' => 1,
},
},
# test paths
--
2.47.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [pve-devel] [PATCH storage] check volume access: handle new 'ct-vol' and 'vm-vol' vtypes
2025-07-30 13:23 [pve-devel] [PATCH storage] check volume access: handle new 'ct-vol' and 'vm-vol' vtypes Fiona Ebner
@ 2025-07-30 13:47 ` Fabian Grünbichler
0 siblings, 0 replies; 2+ messages in thread
From: Fabian Grünbichler @ 2025-07-30 13:47 UTC (permalink / raw)
To: Proxmox VE development discussion
On July 30, 2025 3:23 pm, Fiona Ebner wrote:
> In the tests, legacy volids resulting in 'images' vtype will be
> allowed for both 'ct-vol' and 'vm-vol'.
>
> New test cases for new-form guest image volids are added too.
>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>
> Is based on top of Wolfgang's staff repo as well as [0].
>
> [0]: https://lore.proxmox.com/pve-devel/20250730130506.96278-1-f.ebner@proxmox.com/T/#u
>
> src/PVE/Storage.pm | 31 ++++++++++++++++++---
> src/test/run_volume_access_tests.pl | 43 +++++++++++++++++++++++++++++
> 2 files changed, 70 insertions(+), 4 deletions(-)
>
> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> index 91a0278..e6dabc3 100755
> --- a/src/PVE/Storage.pm
> +++ b/src/PVE/Storage.pm
> @@ -646,9 +646,32 @@ sub check_volume_access {
> if ($sid) {
> my ($vtype, undef, $ownervm) = parse_volname($cfg, $volid);
>
> - # Need to allow 'images' when expecting 'rootdir' too - not cleanly separated in plugins.
> - die "unable to use volume $volid - content type needs to be '$type'\n"
> - if defined($type) && $vtype ne $type && ($type ne 'rootdir' || $vtype ne 'images');
> + my $is_guest_image_vtype =
> + $vtype eq 'ct-vol' || $vtype eq 'vm-vol' || $vtype eq 'images' || $vtype eq 'rootdir';
this one here should be a helper invocation - see my replies to
Wolfgang's patch series
> +
> + if (defined($type)) {
> + my $msg = "unable to use volume $volid - volume type needs to be '$type'";
> +
> + # TODO PVE 11.x die or remove completely, once all callers are adapted
> + $type = 'vm-vol' if $type eq 'images';
> + $type = 'ct-vol' if $type eq 'rootdir';
couldn't we just adjust all callers right now? there aren't many that
set $type in the first place, and even less that set it to a vdisk
type..
> +
> + if ($is_guest_image_vtype) {
> + if ($type eq 'vm-vol') {
> + die "$msg or 'images'\n" if $vtype ne $type && $vtype ne 'images';
> + } elsif ($type eq 'ct-vol') {
> + # need to allow 'images' when expecting 'ct-vol' too for backwards-compat
> + die "$msg or 'rootdir' or 'images'\n"
> + if $vtype ne $type && $vtype ne 'rootdir' && $vtype ne 'images';
> + } else {
> + # $type is not for guest-images, but $vtype is, disallow
> + die "$msg\n";
> + }
> + } else {
> + # can check directly
> + die "$msg\n" if $vtype ne $type;
> + }
> + }
>
> return if $rpcenv->check($user, "/storage/$sid", ['Datastore.Allocate'], 1);
>
> @@ -664,7 +687,7 @@ sub check_volume_access {
> } elsif ($vtype eq 'backup' && $ownervm) {
> $rpcenv->check($user, "/storage/$sid", ['Datastore.AllocateSpace']);
> $rpcenv->check($user, "/vms/$ownervm", ['VM.Backup']);
> - } elsif (($vtype eq 'images' || $vtype eq 'rootdir') && $ownervm) {
> + } elsif ($is_guest_image_vtype && $ownervm) {
> $rpcenv->check($user, "/storage/$sid", ['Datastore.Audit']);
> $rpcenv->check($user, "/vms/$ownervm", ['VM.Config.Disk']);
> } else {
> diff --git a/src/test/run_volume_access_tests.pl b/src/test/run_volume_access_tests.pl
> index ce9fc2e..9744b65 100755
> --- a/src/test/run_volume_access_tests.pl
> +++ b/src/test/run_volume_access_tests.pl
> @@ -78,6 +78,28 @@ my @tests = (
> 'backup' => 1,
> },
> },
> + {
> + volid => 'dir:100/vol-vm-100-disk-0.qcow2',
> + denied_users => {
> + 'backup@pve' => 1,
> + 'dsaudit@pve' => 1,
> + },
> + allowed_types => {
> + 'images' => 1,
> + 'vm-vol' => 1,
> + },
> + },
> + {
> + volid => 'dir:100/vol-ct-100-disk-0.raw',
> + denied_users => {
> + 'backup@pve' => 1,
> + 'dsaudit@pve' => 1,
> + },
> + allowed_types => {
> + 'rootdir' => 1,
> + 'ct-vol' => 1,
> + },
> + },
> {
> volid => 'dir:100/vm-100-disk-0.qcow2',
> denied_users => {
> @@ -87,6 +109,8 @@ my @tests = (
> allowed_types => {
> 'images' => 1,
> 'rootdir' => 1,
> + 'ct-vol' => 1,
> + 'vm-vol' => 1,
> },
> },
> {
> @@ -105,6 +129,17 @@ my @tests = (
> 'iso' => 1,
> },
> },
> + {
> + volid => 'dir:111/subvol-ct-111-disk-0.subvol',
> + denied_users => {
> + 'backup@pve' => 1,
> + 'dsaudit@pve' => 1,
> + },
> + allowed_types => {
> + 'rootdir' => 1,
> + 'ct-vol' => 1,
> + },
> + },
> {
> volid => 'dir:111/subvol-111-disk-0.subvol',
> denied_users => {
> @@ -114,6 +149,8 @@ my @tests = (
> allowed_types => {
> 'images' => 1,
> 'rootdir' => 1,
> + 'ct-vol' => 1,
> + 'vm-vol' => 1,
> },
> },
> # test different VM IDs
> @@ -138,6 +175,8 @@ my @tests = (
> allowed_types => {
> 'images' => 1,
> 'rootdir' => 1,
> + 'ct-vol' => 1,
> + 'vm-vol' => 1,
> },
> },
> {
> @@ -155,6 +194,8 @@ my @tests = (
> allowed_types => {
> 'images' => 1,
> 'rootdir' => 1,
> + 'ct-vol' => 1,
> + 'vm-vol' => 1,
> },
> },
> {
> @@ -184,6 +225,8 @@ my @tests = (
> allowed_types => {
> 'images' => 1,
> 'rootdir' => 1,
> + 'ct-vol' => 1,
> + 'vm-vol' => 1,
> },
> },
> # test paths
> --
> 2.47.2
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2025-07-30 13:45 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-07-30 13:23 [pve-devel] [PATCH storage] check volume access: handle new 'ct-vol' and 'vm-vol' vtypes Fiona Ebner
2025-07-30 13:47 ` Fabian Grünbichler
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox