[PATCH pve-storage v1 48/54] fix #2884: implement nested subdir scanning and support 'iso' vtype

["Max R. Carrara" <m.carrara@proxmox.com>]

Introduce a new 'max-scan-depth' property for directory-based
storages. This property makes it so that files for specific volume
types may also be put into (nested) subdirectories inside the volume
type's directory.

The maximum allowed depth is between 0 and 50, with 0 being the
default, which corresponds to the current behavior.

Support nested subdirectories for ISOs (the 'iso' vtype) only for now.

Achieve all of this by calling the `get_subdir_files()` helper in
`PVE::Storage::Plugin` recursively. Note that the default recursion
limit in Perl appears to be exactly 100 [0], way above the maximum
value of 50 for the 'max-scan-depth' property. Should we need deeper
nesting, we can always make the subroutine iterative instead of
recursive later.

Add additional test cases wherever applicable to account for nested
subdirectories, including cases that check whether the limit set by
the property is upheld, and also cases that check for the existence of
parent directory references ('..') in volume names.

[0]: https://perldoc.perl.org/perl5101delta#Deep-recursion-on-subroutine-%22%25s%22

Originally-by: Noel Ullreich <n.ullreich@proxmox.com>
Signed-off-by: Max R. Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/BTRFSPlugin.pm              |   1 +
 src/PVE/Storage/CephFSPlugin.pm             |   1 +
 src/PVE/Storage/Common/Parse.pm             |  14 +++
 src/PVE/Storage/Common/test/parser_tests.pl |  50 ++++++++
 src/PVE/Storage/DirPlugin.pm                |   1 +
 src/PVE/Storage/Plugin.pm                   |  41 ++++++-
 src/test/filesystem_path_test.pm            |   8 ++
 src/test/list_volumes_test.pm               | 127 ++++++++++++++++++++
 src/test/parse_volname_test.pm              |  56 +++++++++
 src/test/path_to_volume_id_test.pm          |   7 ++
 10 files changed, 302 insertions(+), 4 deletions(-)

diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
index 67b442f6..0a29fd31 100644
--- a/src/PVE/Storage/BTRFSPlugin.pm
+++ b/src/PVE/Storage/BTRFSPlugin.pm
@@ -68,6 +68,7 @@ sub properties {
 sub options {
     return {
         path => { fixed => 1 },
+        'max-scan-depth' => { optional => 1 },
         nodes => { optional => 1 },
         shared => { optional => 1 },
         disable => { optional => 1 },
diff --git a/src/PVE/Storage/CephFSPlugin.pm b/src/PVE/Storage/CephFSPlugin.pm
index fbc97113..b31f53d9 100644
--- a/src/PVE/Storage/CephFSPlugin.pm
+++ b/src/PVE/Storage/CephFSPlugin.pm
@@ -140,6 +140,7 @@ sub options {
     return {
         path => { fixed => 1 },
         'content-dirs' => { optional => 1 },
+        'max-scan-depth' => { optional => 1 },
         monhost => { optional => 1 },
         nodes => { optional => 1 },
         subdir => { optional => 1 },
diff --git a/src/PVE/Storage/Common/Parse.pm b/src/PVE/Storage/Common/Parse.pm
index 9fb45b0c..f98b702d 100644
--- a/src/PVE/Storage/Common/Parse.pm
+++ b/src/PVE/Storage/Common/Parse.pm
@@ -57,6 +57,10 @@ my $RE_SAFE_CHAR_CLASS = qr/[a-zA-Z0-9\-\.\+\=\_]/;

 my $RE_SAFE_CHAR_WITH_WHITESPACE_CLASS = qr/[ a-zA-Z0-9\-\.\+\=\_]/;

+my $RE_DIRECTORY_COMPONENTS = qr!
+    ( ($RE_SAFE_CHAR_WITH_WHITESPACE_CLASS)+ / )*?  # NOTE: non-greedy matching
+!xn;
+
 my $RE_PARENT_DIR = quotemeta('..');
 my $RE_CONTAINS_PARENT_DIR = qr!
     ( ^$RE_PARENT_DIR/ )  #  ../ --> Beginning of path
@@ -73,6 +77,7 @@ my $RE_VMID = qr![1-9][0-9]{2,8}!;

 my $RE_ISO_FILE_PATH = qr!
     (?<path>
+        (?<dir> $RE_DIRECTORY_COMPONENTS )?
         (?<file> [^/]+ \. (?<ext> (?i: iso|img) ) )
     )
 !xn;
@@ -206,6 +211,15 @@ my sub format_named_groups(%groups) {

     my @disk_path_components = ();

+    if (defined($result->{dir})) {
+        if ($result->{dir} eq '') {
+            delete $result->{dir};
+        } else {
+            $result->{dir} = strip_trailing_path_separators($result->{dir});
+            push(@disk_path_components, $result->{dir});
+        }
+    }
+
     if (defined($result->{file})) {
         $result->{file} = strip_leading_path_separators($result->{file});
         push(@disk_path_components, $result->{file});
diff --git a/src/PVE/Storage/Common/test/parser_tests.pl b/src/PVE/Storage/Common/test/parser_tests.pl
index b6e42307..ad122cd2 100755
--- a/src/PVE/Storage/Common/test/parser_tests.pl
+++ b/src/PVE/Storage/Common/test/parser_tests.pl
@@ -73,6 +73,32 @@ my $volname_cases_iso_valid = [
             volname => 'iso/Fedora.Img',
         },
     },
+
+    # subdirectories
+    {
+        path => 'subdir/custom-debian.iso',
+        expected => {
+            file => 'custom-debian.iso',
+            ext => 'iso',
+            dir => 'subdir',
+            'disk-path' => 'subdir/custom-debian.iso',
+            path => 'subdir/custom-debian.iso',
+            vtype => 'iso',
+            volname => 'iso/subdir/custom-debian.iso',
+        },
+    },
+    {
+        path => 'deeply/nested/dir/hannah-montana-linux.IMG',
+        expected => {
+            file => 'hannah-montana-linux.IMG',
+            ext => 'IMG',
+            dir => 'deeply/nested/dir',
+            'disk-path' => 'deeply/nested/dir/hannah-montana-linux.IMG',
+            path => 'deeply/nested/dir/hannah-montana-linux.IMG',
+            vtype => 'iso',
+            volname => 'iso/deeply/nested/dir/hannah-montana-linux.IMG',
+        },
+    },
 ];

 my $volname_cases_iso_invalid = [
@@ -84,6 +110,30 @@ my $volname_cases_iso_invalid = [
         },
         expected => undef,
     },
+    {
+        description => "Parent dir reference in path (beginning) (iso)",
+        args => {
+            path => '../custom-debian.iso',
+            vtype => 'iso',
+        },
+        expected => undef,
+    },
+    {
+        description => "Parent dir reference in path (middle) (iso)",
+        args => {
+            path => 'subdir/../custom-debian.iso',
+            vtype => 'iso',
+        },
+        expected => undef,
+    },
+    {
+        description => "Parent dir reference in path (end) (iso)",
+        args => {
+            path => 'subdir/custom-debian.iso/..',
+            vtype => 'iso',
+        },
+        expected => undef,
+    },
 ];

 my $volname_cases_vztmpl_valid = [
diff --git a/src/PVE/Storage/DirPlugin.pm b/src/PVE/Storage/DirPlugin.pm
index 80c4a031..58b942e7 100644
--- a/src/PVE/Storage/DirPlugin.pm
+++ b/src/PVE/Storage/DirPlugin.pm
@@ -81,6 +81,7 @@ sub options {
     return {
         path => { fixed => 1 },
         'content-dirs' => { optional => 1 },
+        'max-scan-depth' => { optional => 1 },
         nodes => { optional => 1 },
         shared => { optional => 1 },
         disable => { optional => 1 },
diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 101e0b6d..0fbcbd4b 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -233,6 +233,15 @@ my $defaultData = {
             format => "pve-dir-override-list",
             optional => 1,
         },
+        'max-scan-depth' => {
+            description => "Maximum depth of subdirectories to traverse when searching for"
+                . " ISOs in directories.",
+            type => 'integer',
+            default => 0,
+            minimum => 0,
+            maximum => 50,
+            optional => 1,
+        },
         options => {
             description => "NFS/CIFS mount options (see 'man nfs' or 'man mount.cifs')",
             type => 'string',
@@ -1669,10 +1678,16 @@ sub list_images {

 # $vtype = <iso|vztmpl|backup|snippets|import>
 my sub get_subdir_files {
-    my ($storeid, $scfg, $vtype, $vmid) = @_;
+    use feature 'current_sub'; # Needed for the __SUB__ token further below
+
+    my ($storeid, $scfg, $vtype, $vmid, $remaining_depth, $current_path) = @_;

     my $vtype_subdir = plugin_get_vtype_subdir($scfg, $vtype);

+    if (!defined($current_path)) {
+        $current_path = $vtype_subdir;
+    }
+
     my $res = [];

     my $get_subdir_file_info = sub {
@@ -1752,10 +1767,24 @@ my sub get_subdir_files {
         return;
     };

-    for my $path (<$vtype_subdir/*>) {
+    for my $path (<$current_path/*>) {
         my $st = File::stat::stat($path);

-        next if (!$st || S_ISDIR($st->mode));
+        next if !$st;
+
+        if (S_ISDIR($st->mode)) {
+            if (defined($remaining_depth) && $remaining_depth > 0) {
+                # Note: Have to call the subroutine via __SUB__->(...) because
+                # lexical subroutines otherwise cannot reference themselves
+                my $inner_res = __SUB__->(
+                    $storeid, $scfg, $vtype, $vmid, $remaining_depth - 1, $path,
+                );
+
+                push($res->@*, $inner_res->@*);
+            }
+
+            next;
+        }

         if (defined(my $info = $get_subdir_file_info->($path, $st))) {
             $info->{size} = $st->size;
@@ -1773,6 +1802,10 @@ my sub get_subdir_files {
 sub list_volumes {
     my ($class, $storeid, $scfg, $vmid, $content_types) = @_;

+    my $depth = $scfg->{'max-scan-depth'} // 0;
+    $depth = 0 if $depth < 0;
+    $depth = 50 if $depth > 50;
+
     my $res = [];
     my $vmlist = PVE::Cluster::get_vmlist();

@@ -1786,7 +1819,7 @@ sub list_volumes {
         return if !$scfg->{path};

         if ($type eq 'iso' && !defined($vmid)) {
-            return get_subdir_files($storeid, $scfg, 'iso', undef);
+            return get_subdir_files($storeid, $scfg, 'iso', undef, $depth);
         }

         if ($type eq 'vztmpl' && !defined($vmid)) {
diff --git a/src/test/filesystem_path_test.pm b/src/test/filesystem_path_test.pm
index 5a715ce9..26a74a0d 100644
--- a/src/test/filesystem_path_test.pm
+++ b/src/test/filesystem_path_test.pm
@@ -48,6 +48,14 @@ my $tests = [
             "$DEFAULT_STORAGE_DIR/template/iso/my-awesome-proxmox.iso", undef, 'iso',
         ],
     },
+    {
+        volname => 'iso/foo/bar/baz/my-awesome-proxmox.iso',
+        snapname => undef,
+        expected => [
+            "$DEFAULT_STORAGE_DIR/template/iso/foo/bar/baz/my-awesome-proxmox.iso", undef,
+            'iso',
+        ],
+    },
     {
         volname => "backup/vzdump-qemu-1234-2020_03_30-21_12_40.vma",
         snapname => undef,
diff --git a/src/test/list_volumes_test.pm b/src/test/list_volumes_test.pm
index ce35c782..0daaba94 100644
--- a/src/test/list_volumes_test.pm
+++ b/src/test/list_volumes_test.pm
@@ -1296,6 +1296,133 @@ my $test_param_list = [
             },
         ],
     },
+    {
+        description => "VMID: none, no nested subdirectories when using defaults",
+        storeid => $DEFAULT_STOREID,
+        scfg => $DEFAULT_SCFG,
+        vmid => undef,
+        vtypes => ['iso', 'vztmpl', 'snippets', 'import'],
+        cases => [
+            {
+                file => "$DEFAULT_STORAGE_PATH/template/iso/some-installer.iso",
+                expected => {
+                    content => 'iso',
+                    ctime => $DEFAULT_CTIME,
+                    format => 'iso',
+                    size => $DEFAULT_SIZE,
+                    volid => 'local:iso/some-installer.iso',
+                },
+            },
+            {
+                file => "$DEFAULT_STORAGE_PATH/template/iso/1/some-installer.iso",
+                expected => undef,
+            },
+            {
+                file => "$DEFAULT_STORAGE_PATH/template/iso/1/2/3/4/5/some-installer.iso",
+                expected => undef,
+            },
+        ],
+    },
+    {
+        description => "VMID: none, nested subdirectories allowed, max-scan-depth = 1",
+        storeid => $DEFAULT_STOREID,
+        scfg => {
+            type => 'dir',
+            path => $DEFAULT_STORAGE_PATH,
+            shared => 0,
+            'max-scan-depth' => 1,
+            content => {
+                iso => 1,
+                vztmpl => 1,
+                snippets => 1,
+                import => 1,
+            },
+        },
+        vmid => undef,
+        vtypes => ['iso', 'vztmpl', 'snippets', 'import'],
+        cases => [
+            {
+                file => "$DEFAULT_STORAGE_PATH/template/iso/some-installer.iso",
+                expected => {
+                    content => 'iso',
+                    ctime => $DEFAULT_CTIME,
+                    format => 'iso',
+                    size => $DEFAULT_SIZE,
+                    volid => 'local:iso/some-installer.iso',
+                },
+            },
+            {
+                file => "$DEFAULT_STORAGE_PATH/template/iso/1/some-installer.iso",
+                expected => {
+                    content => 'iso',
+                    ctime => $DEFAULT_CTIME,
+                    format => 'iso',
+                    size => $DEFAULT_SIZE,
+                    volid => 'local:iso/1/some-installer.iso',
+                },
+            },
+            {
+                # Exceeds max-scan-depth
+                file => "$DEFAULT_STORAGE_PATH/template/iso/1/2/some-installer.iso",
+                expected => undef,
+            },
+        ],
+    },
+    {
+        description => "VMID: none, nested subdirectories allowed, max-scan-depth = 5",
+        storeid => $DEFAULT_STOREID,
+        scfg => {
+            type => 'dir',
+            path => $DEFAULT_STORAGE_PATH,
+            shared => 0,
+            'max-scan-depth' => 5,
+            content => {
+                iso => 1,
+                vztmpl => 1,
+                snippets => 1,
+                import => 1,
+            },
+        },
+        vmid => undef,
+        vtypes => ['iso', 'vztmpl', 'snippets', 'import'],
+        cases => [
+            {
+                file => "$DEFAULT_STORAGE_PATH/template/iso/some-installer.iso",
+                expected => {
+                    content => 'iso',
+                    ctime => $DEFAULT_CTIME,
+                    format => 'iso',
+                    size => $DEFAULT_SIZE,
+                    volid => 'local:iso/some-installer.iso',
+                },
+            },
+            {
+                file => "$DEFAULT_STORAGE_PATH/template/iso/1/some-installer.iso",
+                expected => {
+                    content => 'iso',
+                    ctime => $DEFAULT_CTIME,
+                    format => 'iso',
+                    size => $DEFAULT_SIZE,
+                    volid => 'local:iso/1/some-installer.iso',
+                },
+            },
+            {
+                file => "$DEFAULT_STORAGE_PATH/template/iso/1/2/3/4/5/some-installer.iso",
+                expected => {
+                    content => 'iso',
+                    ctime => $DEFAULT_CTIME,
+                    format => 'iso',
+                    size => $DEFAULT_SIZE,
+                    volid => 'local:iso/1/2/3/4/5/some-installer.iso',
+                },
+            },
+            {
+                # Exceeds max-scan-depth
+                file => "$DEFAULT_STORAGE_PATH/template/iso/1/2/3/4/5/6/some-installer.iso",
+                expected => undef,
+            },
+        ],
+    },
 ];

 # Additional test cases that cannot be constructed within the list above
diff --git a/src/test/parse_volname_test.pm b/src/test/parse_volname_test.pm
index 1dcec9d6..24d1ed0e 100644
--- a/src/test/parse_volname_test.pm
+++ b/src/test/parse_volname_test.pm
@@ -167,10 +167,66 @@ my $tests = [
                 volname => "iso/$file_name",
                 expected => ['iso', "$file_name", undef, undef, undef, undef, 'raw'],
             },
+            {
+                description => "ISO image, $suffix, subdirectory",
+                volname => "iso/foo/$file_name",
+                expected => [
+                    'iso', "foo/$file_name", undef, undef, undef, undef, 'raw',
+                ],
+            },
+            {
+                description => "ISO image, $suffix, nested subdirectories",
+                volname => "iso/foo/bar/baz/$file_name",
+                expected => [
+                    'iso', "foo/bar/baz/$file_name", undef, undef, undef, undef, 'raw',
+                ],
+            },
+            {
+                description => "ISO image, $suffix, subdirectory with same name as file",
+                volname => "iso/$file_name/$file_name",
+                expected => [
+                    'iso', "$file_name/$file_name", undef, undef, undef, undef, 'raw',
+                ],
+            },
         );

         push($tests->@*, @extra_tests);
     }
+
+    # Failed tests
+    {
+        my $file_name = "$prefix.iso";
+
+        my @extra_failed_tests = (
+            {
+                description =>
+                    "ISO image, iso, parent directory reference before volume type prefix",
+                volname => "../iso/$file_name",
+                expected => "unable to parse directory volume name '../iso/$file_name'\n",
+            },
+            {
+                description =>
+                    "ISO image, iso, parent directory reference at beginning of volume path",
+                volname => "iso/../$file_name",
+                expected => "unable to parse directory volume name 'iso/../$file_name'\n",
+            },
+            {
+                description =>
+                    "ISO image, iso, parent directory reference at end of volume path",
+                volname => "iso/$file_name/..",
+                expected => "unable to parse directory volume name 'iso/$file_name/..'\n",
+            },
+            {
+                description =>
+                    "ISO image, iso, parent directory reference between dir components of volume path",
+                volname => "iso/foo/../bar/$file_name",
+                expected =>
+                    "unable to parse directory volume name 'iso/foo/../bar/$file_name'\n",
+            },
+        );
+
+        push($tests->@*, @extra_failed_tests);
+    }
 }

 # Test cases for container templates
diff --git a/src/test/path_to_volume_id_test.pm b/src/test/path_to_volume_id_test.pm
index 6c7d0d67..bc87d289 100644
--- a/src/test/path_to_volume_id_test.pm
+++ b/src/test/path_to_volume_id_test.pm
@@ -123,6 +123,13 @@ my $tests = [
             'iso', 'local:iso/yet-again-a-installation-disk.iso',
         ],
     },
+    {
+        description => 'ISO file, nested subdirectories',
+        file => "$DEFAULT_STORAGE_DIR/template/iso/foo/bar/hannah-montana-linux-installer.iso",
+        expected => [
+            'iso', 'local:iso/foo/bar/hannah-montana-linux-installer.iso',
+        ],
+    },
     {
         description => 'CT template, tar.gz',
         file => "$DEFAULT_STORAGE_DIR/template/cache/debian-10.0-standard_10.0-1_amd64.tar.gz",
--
2.47.3