[PATCH pve-storage v2 44/50] 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             |  18 +++
 src/PVE/Storage/Common/test/parser_tests.pl |  50 ++++++++
 src/PVE/Storage/DirPlugin.pm                |   1 +
 src/PVE/Storage/Plugin.pm                   |  39 +++++-
 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, 304 insertions(+), 4 deletions(-)

diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
index e75b4fe0..b5b41dbe 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 296a6c4d..1d021afa 100644
--- a/src/PVE/Storage/Common/Parse.pm
+++ b/src/PVE/Storage/Common/Parse.pm
@@ -60,6 +60,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)+ / )*
+!xn;
+
 my $RE_PARENT_DIR = quotemeta('..');
 my $RE_CONTAINS_PARENT_DIR = qr!
     ( ^$RE_PARENT_DIR/ )  #  ../ --> Beginning of path
@@ -76,6 +80,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;
@@ -209,6 +214,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});
@@ -312,6 +326,10 @@ These are:
         volname => 'iso/custom-debian.iso',
     }
 
+Volume types that support subdirectories in their C<path> part also have a
+C<dir> part if a subdirectory is specified. This is currently limited to the
+C<iso> volume type.
+
 The following volume types have more specific parts in addition to the common
 ones:
 
diff --git a/src/PVE/Storage/Common/test/parser_tests.pl b/src/PVE/Storage/Common/test/parser_tests.pl
index 2869d3f0..204002d0 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 e2f24480..e96ab2d5 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -234,6 +234,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',
@@ -1696,10 +1705,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 {
@@ -1783,10 +1798,22 @@ 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) {
+                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;
@@ -1804,6 +1831,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();
 
@@ -1821,7 +1852,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 53ce5116..b1f72091 100644
--- a/src/test/list_volumes_test.pm
+++ b/src/test/list_volumes_test.pm
@@ -1228,6 +1228,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