Re: [pve-devel] [PATCH storage] check volume access: handle new 'ct-vol' and 'vm-vol' vtypes

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

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