[pve-devel] [PATCH storage 2/4] Recursive search for iso and vztmpl
Dominik Csapak
d.csapak at proxmox.com
Wed Apr 8 16:10:56 CEST 2020
comments inline
On 4/2/20 1:34 PM, Dominic Jäger wrote:
> Each content type has a predefined location in a storage where they can be
> found. iso and vztmpl files can now be found and and also uploaded in
> subdirectories of those locations, too.
>
> Add a parameter "recursive" (default off) to the API at
> storage/{storage}/content, to perform this search.
>
> Signed-off-by: Dominic Jäger <d.jaeger at proxmox.com>
> ---
> Changes since RFC:
> * Separate tests for existing and new stuff into 2 patches
> * Die when .. appears in a path
> * Make feature opt-in (relevant for symlinks)
>
> PVE/API2/Storage/Content.pm | 9 +++-
> PVE/API2/Storage/Status.pm | 12 +++---
> PVE/Storage.pm | 4 +-
> PVE/Storage/Plugin.pm | 85 +++++++++++++++++++++++++++++++++----
> test/run_plugin_tests.pl | 57 ++++++++++++++++++++++++-
> 5 files changed, 149 insertions(+), 18 deletions(-)
>
> diff --git a/PVE/API2/Storage/Content.pm b/PVE/API2/Storage/Content.pm
> index 63fa4fc..cd302e9 100644
> --- a/PVE/API2/Storage/Content.pm
> +++ b/PVE/API2/Storage/Content.pm
> @@ -44,6 +44,12 @@ __PACKAGE__->register_method ({
> optional => 1,
> completion => \&PVE::Cluster::complete_vmid,
> }),
> + recursive => {
> + description => "Search recursively.",
> + type => 'boolean',
> + optional => 1,
> + default => 0,
> + },
> },
> },
> returns => {
> @@ -102,7 +108,8 @@ __PACKAGE__->register_method ({
>
> my $cfg = PVE::Storage::config();
>
> - my $vollist = PVE::Storage::volume_list($cfg, $storeid, $param->{vmid}, $param->{content});
> + my $vollist = PVE::Storage::volume_list($cfg, $storeid, $param->{vmid},
> + $param->{content}, {recursive => $param->{recursive}});
>
> my $res = [];
> foreach my $item (@$vollist) {
> diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
> index 14f5930..b8caf9c 100644
> --- a/PVE/API2/Storage/Status.pm
> +++ b/PVE/API2/Storage/Status.pm
> @@ -403,25 +403,25 @@ __PACKAGE__->register_method ({
> my $filename = $param->{filename};
>
> chomp $filename;
> - $filename =~ s/^.*[\/\\]//;
> - $filename =~ s/[^-a-zA-Z0-9_.]/_/g;
> + # Prevent entering other locations without permission
> + die "Filename must not contain two dots '..'" if $filename =~ m/\.\./;
i'd argue that this check is wrong...
i get what you intend but what about files named like:
my.very.cool...iso.iso
would also be filtered out but is a very valid filename
'..' is only forbidden at the beginning, the end or between two '/'
so e.g.
m!(?:^\.\./|/\.\./|/\.\.$)!
(well, this is certainly not a readable regex^^; also i am
certainly missing a case, i think...)
> + $filename =~ s/^[\/|\\]*//; # files are uploaded relative to storage path
are '|' intentionally removed here? in [] the characters are or'd anyway
> + $filename =~ s/[^-a-zA-Z0-9_.\/\\]/_/g;
>
> my $path;
> -
> if ($content eq 'iso') {
> - if ($filename !~ m![^/]+$PVE::Storage::iso_extension_re$!) {
> + if ($filename !~ m!$PVE::Storage::iso_extension_re$!) {
> raise_param_exc({ filename => "missing '.iso' or '.img' extension" });
> }
> $path = PVE::Storage::get_iso_dir($cfg, $param->{storage});
> } elsif ($content eq 'vztmpl') {
> - if ($filename !~ m![^/]+\.tar\.[gx]z$!) {
> + if ($filename !~ m!\.tar\.[gx]z$!) {
> raise_param_exc({ filename => "missing '.tar.gz' or '.tar.xz' extension" });
> }
> $path = PVE::Storage::get_vztmpl_dir($cfg, $param->{storage});
> } else {
> raise_param_exc({ content => "upload content type '$content' not allowed" });
> }
> -
> die "storage '$param->{storage}' does not support '$content' content\n"
> if !$scfg->{content}->{$content};
>
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index 60b8310..9ec40a5 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -866,7 +866,7 @@ sub template_list {
> }
>
> sub volume_list {
> - my ($cfg, $storeid, $vmid, $content) = @_;
> + my ($cfg, $storeid, $vmid, $content, $param) = @_;
>
> my @ctypes = qw(rootdir images vztmpl iso backup snippets);
>
> @@ -880,7 +880,7 @@ sub volume_list {
>
> activate_storage($cfg, $storeid);
>
> - my $res = $plugin->list_volumes($storeid, $scfg, $vmid, $cts);
> + my $res = $plugin->list_volumes($storeid, $scfg, $vmid, $cts, $param);
>
> @$res = sort {lc($a->{volid}) cmp lc ($b->{volid}) } @$res;
>
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index 8c0dae1..e81c89f 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -415,6 +415,9 @@ sub parse_name_dir {
> die "unable to parse volume filename '$name'\n";
> }
>
> +# for iso and vztmpl returns
> +# [0] format
> +# [1] notdir (=basename+suffix; no directories)
nit: again the 'notdir' here...
> sub parse_volname {
> my ($class, $volname) = @_;
>
> @@ -428,9 +431,11 @@ sub parse_volname {
> my ($vmid, $name) = ($1, $2);
> my (undef, $format, $isBase) = parse_name_dir($name);
> return ('images', $name, $vmid, undef, undef, $isBase, $format);
> - } elsif ($volname =~ m!^iso/([^/]+$PVE::Storage::iso_extension_re)$!) {
> + } elsif ($volname =~ m!^iso/(?:.+/)*([^/]+$PVE::Storage::iso_extension_re)$!) {
nit:
(?:.+/)* could also be (?:.+/)?
since this will not repeat anyway,
since the .+ is greedy and will include '/',
the ? makes it clearer that this is just optional
> + die "volname must not contain two dots '..'" if $volname =~ m!\.\.!;
see above
> return ('iso', $1);
> - } elsif ($volname =~ m!^vztmpl/([^/]+\.tar\.[gx]z)$!) {
> + } elsif ($volname =~ m!^vztmpl/(?:.+/)*([^/]+\.tar\.[gx]z)$!) {
same note about */?
> + die "volname must not contain two dots '..'" if $volname =~ m!\.\.!;
see above
> return ('vztmpl', $1);
> } elsif ($volname =~ m!^rootdir/(\d+)$!) {
> return ('rootdir', $1, $1);
> @@ -485,7 +490,14 @@ sub filesystem_path {
>
> $dir .= "/$vmid" if $vtype eq 'images';
>
> - my $path = "$dir/$name";
> + my $path;
> + if ($vtype eq 'iso' || $vtype eq 'vztmpl') {
> + # remove vtype => file name relative to storage path
> + $volname =~ m!^$vtype/(.+)!;
> + $path = "$dir/$1";
> + } else {
> + $path = "$dir/$name";
> + }
>
> return wantarray ? ($path, $vmid, $vtype) : $path;
> }
> @@ -910,6 +922,61 @@ sub list_images {
> return $res;
> }
>
> +
> +# returns if file name fits $type
> +# [0] volname
> +# [1] format
> +# undef otherwise
> +sub get_file_properties {
> + my ($fn, $storage_path, $type) = @_;
> + die "fn must not contain two dots '..'" if $fn =~ m/\.\./;
see above
> + if ($type eq 'iso') {
> + if ($fn =~ m!$storage_path/(.+$PVE::Storage::iso_extension_re)$!i) {
> + return ["$type/$1", $type];
> + } else {
> + return undef;
> + }
> + } elsif ($type eq 'vztmpl') {
> + if ($fn =~ m!$storage_path/(.+\.tar\.([gx]z))$!) {
> + return ["$type/$1", "t$2"];
> + } else {
> + return undef;
> + }
> + } else {
> + die "Invalid parameter type=$type";
> + }
> +}
> +
> +# $type = <iso|vztmpl>
> +sub get_subdir_files_rec {
any special reason to make this a full-fledged sub
instead of a private my $foo = sub {}; ?
if it is about recursive access just do:
my $foo;
$foo = sub {
$foo->();
};
> + my ($sid, $path, $type, $storage_path) = @_;
> + die "Invalid parameter" if not ($type eq 'iso' || $type eq 'vztmpl');
any reason for the 'not' vs '!' ? (not that it's wrong, just not our style)
> + die "Path must not contain ../" if $path =~ m!.*\.\./.*!;
ha, its better here, but the .* front and back are useless and its also
wrong: /myweirddirectory../
> +
> + # Each call needs to know the original storage path
> + if (!defined $storage_path) {
nit: please use parenthesis for defined
> + $storage_path = $path;
> + }
> +
> + my @result = ();
> +
> + # Symlinks have to be set up via shell. At least as long recursive scanning
> + # is opt-in handling loops is out of scope. Instead, just follow the links.
> + if (-d $path) {
> + foreach my $fn (<$path/*>) {
> + push @result, get_subdir_files_rec($sid, $fn, $type, $storage_path);
> + }
> + } else {
> + my $properties = get_file_properties($path, $storage_path, $type);
> + if ($properties) {
> + my $file = { volid => "$sid:$$properties[0]", format => $$properties[1] };
please use $foo->[0]
see our style guideline:
https://pve.proxmox.com/wiki/Perl_Style_Guide#Perl_syntax_choices
> + $file->{size} = -s $path // 0;
> + push @result, $file;
> + }
> + }
> + return @result;
> +}
> +
> # list templates ($tt = <iso|vztmpl|backup|snippets>)
> my $get_subdir_files = sub {
> my ($sid, $path, $tt, $vmid) = @_;
> @@ -982,7 +1049,7 @@ my $get_subdir_files = sub {
> };
>
> sub list_volumes {
> - my ($class, $storeid, $scfg, $vmid, $content_types) = @_;
> + my ($class, $storeid, $scfg, $vmid, $content_types, $param) = @_;
>
> my $res = [];
> my $vmlist = PVE::Cluster::get_vmlist();
> @@ -994,10 +1061,12 @@ sub list_volumes {
> } elsif ($scfg->{path}) {
> my $path = $class->get_subdir($scfg, $type);
>
> - if ($type eq 'iso' && !defined($vmid)) {
> - $data = $get_subdir_files->($storeid, $path, 'iso');
> - } elsif ($type eq 'vztmpl'&& !defined($vmid)) {
> - $data = $get_subdir_files->($storeid, $path, 'vztmpl');
> + if (($type eq 'iso' || $type eq 'vztmpl') && !defined($vmid)) {
> + if ($param->{recursive}) {
> + $data = [get_subdir_files_rec($storeid, $path, $type)];
> + } else {
> + $data = $get_subdir_files->($storeid, $path, $type);
> + }
> } elsif ($type eq 'backup') {
> $data = $get_subdir_files->($storeid, $path, 'backup', $vmid);
> } elsif ($type eq 'snippets') {
> diff --git a/test/run_plugin_tests.pl b/test/run_plugin_tests.pl
> index cd93430..7f627b1 100755
> --- a/test/run_plugin_tests.pl
> +++ b/test/run_plugin_tests.pl
> @@ -3,7 +3,7 @@ use strict;
> use warnings;
>
> use lib ('.', '..');
> -use Test::More tests => 32;
> +use Test::More tests => 42;
> use Test::MockModule qw(new);
> use File::Temp qw(tempdir);
> use File::Path qw(make_path);
> @@ -13,16 +13,23 @@ use PVE::Storage;
>
> my $plugin = 'PVE::Storage::Plugin';
> my $basename = 'test';
> +my $subdir_regular = 'some/more/subdirectories/';
>
> my $iso_type = 'iso';
> my $iso_suffix = '.iso';
> my $iso_notdir = "$basename$iso_suffix";
> my $iso_volname = "$iso_type/$iso_notdir";
> +my $iso_volname_with_subdirs = "$iso_type/$subdir_regular$iso_notdir";
> +# Subdirectories should be treated like normal folders, even if their name
> +# is equal to our predefined storage schema
> +my $subdirs_named_iso = 'a/template/iso/b/template/iso/c/template/iso/';
>
> my $vztmpl_type = 'vztmpl';
> my $vztmpl_suffix = '.tar.gz';
> my $vztmpl_notdir = "$basename$vztmpl_suffix";
> my $vztmpl_volname = "$vztmpl_type/$vztmpl_notdir";
> +my $vztmpl_volname_with_subdirs = "$vztmpl_type/$subdir_regular$vztmpl_notdir";
> +my $subdir_named_cache = 'a/template/cache/b/template/cache/c/template/cache/';
>
> my $iso_with_dots = "$iso_type/../$iso_notdir";
> my $vztmpl_with_dots = "$vztmpl_type/../$vztmpl_notdir";
> @@ -48,11 +55,25 @@ is (($plugin->parse_volname($iso_volname))[$type_index],
> $iso_type, 'parse_volname: type for iso');
> is (($plugin->parse_volname($iso_volname))[$notdir_index],
> $iso_notdir, 'parse_volname: notdir for iso');
> +is (($plugin->parse_volname($iso_volname_with_subdirs))[$type_index],
> + $iso_type, 'parse_volname: type for iso in subdir');
> +is (($plugin->parse_volname($iso_volname_with_subdirs))[$notdir_index],
> + $iso_notdir, 'parse_volname: notdir for iso in subdir');
> +eval { $plugin->parse_volname($iso_with_dots) };
> +like ($@, qr/volname must not contain two dots/,
> + 'parse_volname: forbid two dots .. for iso');
>
> is (($plugin->parse_volname($vztmpl_volname))[$type_index],
> $vztmpl_type, 'parse_volname: type for vztmpl');
> is (($plugin->parse_volname($vztmpl_volname))[$notdir_index],
> $vztmpl_notdir, 'parse_volname: notdir for vztmpl');
> +is (($plugin->parse_volname($vztmpl_volname_with_subdirs))[$type_index],
> + $vztmpl_type, 'parse_volname: type for vztmpl in subdir');
> +is (($plugin->parse_volname($vztmpl_volname_with_subdirs))[$notdir_index],
> + $vztmpl_notdir, 'parse_volname: notdir for vztmpl in subdir');
> +eval { $plugin->parse_volname($vztmpl_with_dots) };
> +like ($@, qr/volname must not contain two dots/,
> + 'parse_volname: forbid two dots .. for vztmpl');
>
> is (($plugin->parse_volname($raw_image_volname))[$type_index],
> $image_type, 'parse_volname: type for raw image');
> @@ -91,9 +112,15 @@ is ($plugin->get_subdir($scfg_with_path, 'rootdir'),
> is ($plugin->filesystem_path($scfg_with_path, $iso_volname),
> "$scfg_with_path->{path}/template/$iso_volname",
> 'filesystem_path for iso');
> +is ($plugin->filesystem_path($scfg_with_path, $iso_volname_with_subdirs),
> + "$scfg_with_path->{path}/template/$iso_volname_with_subdirs",
> + 'filesystem_path for iso in subdirs');
> is ($plugin->filesystem_path($scfg_with_path, $vztmpl_volname),
> "$scfg_with_path->{path}/template/cache/$vztmpl_notdir",
> 'filesystem_path for vztmpl');
> +is ($plugin->filesystem_path($scfg_with_path, $vztmpl_volname_with_subdirs),
> + "$scfg_with_path->{path}/template/cache/$subdir_regular$vztmpl_notdir",
> + 'filesystem_path for vztmpl in subdirs');
> is ($plugin->filesystem_path($scfg_with_path, $raw_image_volname),
> "$scfg_with_path->{path}/images/$raw_image_volname",
> 'filesystem_path for raw images');
> @@ -142,12 +169,40 @@ my $paths = [
> ];
> add_test_files($paths);
>
> +my $storage_dir_rec = File::Temp->newdir();
> +my $recursive_paths = [
> + "$storage_dir_rec/template/cache",
> + "$storage_dir_rec/template/iso",
> + "$storage_dir_rec/images/$vmid",
> + "$storage_dir_rec/template/cache/$subdir_regular",
> + "$storage_dir_rec/template/cache/$subdirs_named_iso",
> + "$storage_dir_rec/template/cache/$subdir_named_cache",
> + "$storage_dir_rec/template/iso/$subdir_regular",
> + "$storage_dir_rec/template/iso/$subdirs_named_iso",
> + "$storage_dir_rec/template/iso/$subdir_named_cache",
> +];
> +add_test_files($recursive_paths);
> +
> note 'Tests without recursion';
> # note "Temp dir is:\n", `tree $storage_dir_no_rec`;
> my $scfg_no_rec = { path => $storage_dir_no_rec };
> test_list_volumes($scfg_no_rec, [$iso_type], [$iso_volname]);
> test_list_volumes($scfg_no_rec, [$vztmpl_type], [$vztmpl_volname]);
>
> +note 'Tests with recursion';
> +# note "Temp dir is:\n", `tree $storage_dir_rec`;
> +my $scfg_rec = { path => $storage_dir_rec };
> +my $expected_volnames_iso = [$iso_volname, $iso_volname_with_subdirs,
> + "$iso_type/$subdir_named_cache$iso_notdir",
> + "$iso_type/$subdirs_named_iso$iso_notdir"];
> +my $expected_volnames_vztmpl = [$vztmpl_volname, $vztmpl_volname_with_subdirs,
> + "$vztmpl_type/$subdir_named_cache$vztmpl_notdir",
> + "$vztmpl_type/$subdirs_named_iso$vztmpl_notdir"];
> +test_list_volumes($scfg_rec, [$iso_type], $expected_volnames_iso,
> + { recursive => 1 });
> +test_list_volumes($scfg_rec, [$vztmpl_type],$expected_volnames_vztmpl,
> + { recursive => 1 });
> +
> my $scfg_with_type = { path => $storage_dir_no_rec, type => 'dir' };
> my $plugin_mock = Test::MockModule->new('PVE::Cluster');
> $plugin_mock->redefine('get_vmlist' => sub { return undef });
>
More information about the pve-devel
mailing list