[pve-devel] [PATCH access-control/manager 0/4] fix #1148: nested pools

[pve-devel] [PATCH access-control/manager 0/4] fix #1148: nested pools

[Fabian Grünbichler]

this series extends the poolname to allow (for now) two '/' inside the
name to nest pools like this:

parent/child/grandchild

since '/' is a bad character for path parameters, some API shuffling is
needed. other approaches that were tried but discarded, or rejected
outright:
- urlencoding
- systemd-escape
- manual encoding/escaping
- just encoding the parent/children in user.cfg, but not in the pool name

switching over some of the UI pool lists to also nest them visually should
be do-able as a follow-up.

changes since RFC:
- some style improvements, RE adaptation

pve-access-control:

Fabian Grünbichler (2):
  allow up to three levels of pool nesting
  pools: record parent/subpool information

 src/PVE/AccessControl.pm  | 26 +++++++++++++++++++++++---
 src/test/parser_writer.pl |  4 ++++
 src/test/perm-test6.pl    | 16 ++++++++++++++++
 src/test/test6.cfg        |  5 +++++
 4 files changed, 48 insertions(+), 3 deletions(-)

pve-manager:

Fabian Grünbichler (2):
  api: pools: support nested pools
  ui: pools: switch to new API endpoints

 PVE/API2/Pool.pm                 | 243 +++++++++++++++++++++++--------
 www/manager6/dc/PoolView.js      |   3 +
 www/manager6/grid/PoolMembers.js |  14 +-
 www/manager6/pool/StatusView.js  |   2 +-
 4 files changed, 196 insertions(+), 66 deletions(-)

-- 
2.39.2

[pve-devel] [PATCH access-control 1/2] fix #1148: allow up to three levels of pool nesting

[Fabian Grünbichler]

with ACLs being inherited along the pool hierarchy.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    v1: encode max pool nesting in REs

 src/PVE/AccessControl.pm | 10 ++++++++--
 src/test/perm-test6.pl   | 16 ++++++++++++++++
 src/test/test6.cfg       |  5 +++++
 3 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
index 9600e59..4e3b077 100644
--- a/src/PVE/AccessControl.pm
+++ b/src/PVE/AccessControl.pm
@@ -1264,7 +1264,7 @@ sub check_path {
 	|/nodes
 	|/nodes/[[:alnum:]\.\-\_]+
 	|/pool
-	|/pool/[[:alnum:]\.\-\_]+
+	|/pool/[A-Za-z0-9\.\-_]+(?:/[A-Za-z0-9\.\-_]+){0,2}
 	|/sdn
 	|/sdn/controllers
 	|/sdn/controllers/[[:alnum:]\_\-]+
@@ -1318,8 +1318,14 @@ PVE::JSONSchema::register_format('pve-poolid', \&verify_poolname);
 sub verify_poolname {
     my ($poolname, $noerr) = @_;
 
-    if ($poolname !~ m/^[A-Za-z0-9\.\-_]+$/) {
+    if (split("/", $poolname) > 3) {
+	die "pool name '$poolname' nested too deeply (max levels = 3)\n" if !$noerr;
 
+	return undef;
+    }
+
+    # also adapt check_path above if changed!
+    if ($poolname !~ m!^[A-Za-z0-9\.\-_]+(?:/[A-Za-z0-9\.\-_]+){0,2}$!) {
 	die "pool name '$poolname' contains invalid characters\n" if !$noerr;
 
 	return undef;
diff --git a/src/test/perm-test6.pl b/src/test/perm-test6.pl
index 0b0d036..c2d40fc 100755
--- a/src/test/perm-test6.pl
+++ b/src/test/perm-test6.pl
@@ -75,6 +75,22 @@ check_roles('User4@pve', '/vms/500', '');
 # with pool
 check_permissions('User4@pve', '/vms/500', '');
 
+# without pool, checking no access on parent pool
+check_roles('intern@pve', '/vms/600', '');
+# once more, with VM in nested pool
+check_roles('intern@pve', '/vms/700', '');
+# with propagated ACL
+check_roles('User4@pve', '/vms/700', '');
+# with pool, checking no access on parent pool
+check_permissions('intern@pve', '/vms/600', '');
+# once more, with VM in nested pool
+check_permissions('intern@pve', '/vms/700', 'VM.Audit');
+# with propagated ACL
+check_permissions('User4@pve', '/vms/700', 'VM.Console');
+
+# check nested pool permissions
+check_roles('intern@pve', '/pool/marketing/interns', 'RoleINTERN');
+check_roles('User4@pve', '/pool/marketing/interns', 'RoleMARKETING');
 
 check_permissions('User1@pve', '/vms/600', 'VM.Console');
 check_permissions('User2@pve', '/vms/600', 'VM.Console');
diff --git a/src/test/test6.cfg b/src/test/test6.cfg
index 4986910..661f56a 100644
--- a/src/test/test6.cfg
+++ b/src/test/test6.cfg
@@ -2,16 +2,20 @@ user:User1@pve:1:
 user:User2@pve:1:
 user:User3@pve:1:
 user:User4@pve:1:
+user:intern@pve:1:
 
 group:DEVEL:User1@pve,User2@pve,User3@pve:
 group:MARKETING:User1@pve,User4@pve:
+group:INTERNS:intern@pve:
 
 role:RoleDEVEL:VM.PowerMgmt:
 role:RoleMARKETING:VM.Console:
+role:RoleINTERN:VM.Audit:
 role:RoleTEST1:VM.Console:
 
 acl:1:/pool/devel:@DEVEL:RoleDEVEL:
 acl:1:/pool/marketing:@MARKETING:RoleMARKETING:
+acl:1:/pool/marketing/interns:@INTERNS:RoleINTERN:
 
 acl:1:/vms:@DEVEL:RoleTEST1:
 acl:1:/vms:User3@pve:NoAccess:
@@ -19,3 +23,4 @@ acl:1:/vms/300:@MARKETING:RoleTEST1:
 
 pool:devel:MITS development:500,501,502:store1 store2:
 pool:marketing:MITS marketing:600:store1:
+pool:marketing/interns:MITS marketing intern:700:store3:
-- 
2.39.2

[pve-devel] [PATCH access-control 2/2] pools: record parent/subpool information

[Fabian Grünbichler]

and ensure a missing intermediate pool exists at all times.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    v1: adapt RE based on Wolfgang's feedback, post-if style
    
    a "missing link" should never happen when modifying via the API (both deletion
    with children and addition without the parent existing is blocked there), but
    it could happen when manually editing the config.

 src/PVE/AccessControl.pm  | 16 +++++++++++++++-
 src/test/parser_writer.pl |  4 ++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
index 4e3b077..461a64e 100644
--- a/src/PVE/AccessControl.pm
+++ b/src/PVE/AccessControl.pm
@@ -1529,7 +1529,21 @@ sub parse_user_config {
 	    }
 
 	    # make sure to add the pool (even if there are no members)
-	    $cfg->{pools}->{$pool} = { vms => {}, storage => {} } if !$cfg->{pools}->{$pool};
+	    $cfg->{pools}->{$pool} = { vms => {}, storage => {}, pools => {} }
+		if !$cfg->{pools}->{$pool};
+
+	    if ($pool =~ m!/!) {
+		my $curr = $pool;
+		while ($curr =~ m!^(.+)/[^/]+$!) {
+		    # ensure nested pool info is correctly recorded
+		    my $parent = $1;
+		    $cfg->{pools}->{$curr}->{parent} = $parent;
+		    $cfg->{pools}->{$parent} = { vms => {}, storage => {}, pools => {} }
+			if !$cfg->{pools}->{$parent};
+		    $cfg->{pools}->{$parent}->{pools}->{$curr} = 1;
+		    $curr = $parent;
+		}
+	    }
 
 	    $cfg->{pools}->{$pool}->{comment} = PVE::Tools::decode_text($comment) if $comment;
 
diff --git a/src/test/parser_writer.pl b/src/test/parser_writer.pl
index 65a70eb..80c346b 100755
--- a/src/test/parser_writer.pl
+++ b/src/test/parser_writer.pl
@@ -237,21 +237,25 @@ my $default_cfg = {
 	'id' => 'testpool',
 	vms => {},
 	storage => {},
+	pools => {},
     },
     test_pool_members => {
 	'id' => 'testpool',
 	vms => { 123 => 1, 1234 => 1},
 	storage => { 'local' => 1, 'local-zfs' => 1},
+	pools => {},
     },
     test_pool_duplicate_vms => {
 	'id' => 'test_duplicate_vms',
 	vms => {},
 	storage => {},
+	pools => {},
     },
     test_pool_duplicate_storages => {
 	'id' => 'test_duplicate_storages',
 	vms => {},
 	storage => { 'local' => 1, 'local-zfs' => 1},
+	pools => {},
     },
     acl_simple_user => {
 	'path' => '/',
-- 
2.39.2

[pve-devel] [PATCH manager 1/2] fix #1148: api: pools: support nested pools

[Fabian Grünbichler]

since poolid can now contain `/`, it's not possible to use it (properly) as
path parameter anymore.

accordingly:
- merge `read_pool` (`GET /pools/{poolid}`) into 'index' (`GET
  /pools/?poolid={poolid}`) (requires clients to extract the only member of the returned array if they want to query an individual pool)
- move `update_pool` to `/pools`, deprecating the old variant with path parameter
- move `delete_pool` to `/pools`, deprecating the old variant with path parameter
- deprecate `read_pool` API endpoint

pool creation is blocked for nested pools where the parent does not already
exist. similarly, the checks for deletion are extended to block deletion if
sub-pools still exist.

the old API endpoints continue to work for non-nested pools. `pvesh ls /pools`
is semi-broken for nested pools, listing the entries, but no methods on them,
since they reference the old API. fixing this would require extending the REST
handling to support a new type of child reference.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    requires bumped pve-access-control

 PVE/API2/Pool.pm | 243 +++++++++++++++++++++++++++++++++++------------
 1 file changed, 184 insertions(+), 59 deletions(-)

diff --git a/PVE/API2/Pool.pm b/PVE/API2/Pool.pm
index 51ac71941..54e744558 100644
--- a/PVE/API2/Pool.pm
+++ b/PVE/API2/Pool.pm
@@ -20,14 +20,26 @@ __PACKAGE__->register_method ({
     name => 'index',
     path => '',
     method => 'GET',
-    description => "Pool index.",
+    description => "List pools or get pool configuration.",
     permissions => {
-	description => "List all pools where you have Pool.Audit permissions on /pool/<pool>.",
+	description => "List all pools where you have Pool.Audit permissions on /pool/<pool>, or the pool specific with {poolid}",
 	user => 'all',
     },
     parameters => {
 	additionalProperties => 0,
-	properties => {},
+	properties => {
+	    poolid => {
+		type => 'string',
+		format => 'pve-poolid',
+		optional => 1,
+	    },
+	    type => {
+		type => 'string',
+		enum => [ 'qemu', 'lxc', 'storage' ],
+		optional => 1,
+		requires => 'poolid',
+	    },
+	},
     },
     returns => {
 	type => 'array',
@@ -35,6 +47,38 @@ __PACKAGE__->register_method ({
 	    type => "object",
 	    properties => {
 		poolid => { type => 'string' },
+		comment => {
+		    type => 'string',
+		    optional => 1,
+		},
+		members => {
+		    type => 'array',
+		    optional => 1,
+		    items => {
+			type => "object",
+			additionalProperties => 1,
+			properties => {
+			    type => {
+				type => 'string',
+				enum => [ 'qemu', 'lxc', 'openvz', 'storage' ],
+			    },
+			    id => {
+				type => 'string',
+			    },
+			    node => {
+				type => 'string',
+			    },
+			    vmid => {
+				type => 'integer',
+				optional => 1,
+			    },
+			    storage => {
+				type => 'string',
+				optional => 1,
+			    },
+			},
+		    },
+		},
 	    },
 	},
 	links => [ { rel => 'child', href => "{poolid}" } ],
@@ -47,15 +91,63 @@ __PACKAGE__->register_method ({
 
 	my $usercfg = $rpcenv->{user_cfg};
 
-
 	my $res = [];
-	for my $pool (sort keys %{$usercfg->{pools}}) {
-	    next if !$rpcenv->check($authuser, "/pool/$pool", [ 'Pool.Audit' ], 1);
+	if (my $poolid = $param->{poolid}) {
+	    $rpcenv->check($authuser, "/pool/$poolid", [ 'Pool.Audit' ], 1);
 
-	    my $entry = { poolid => $pool };
-	    my $pool_config = $usercfg->{pools}->{$pool};
-	    $entry->{comment} = $pool_config->{comment} if defined($pool_config->{comment});
-	    push @$res, $entry;
+	    my $vmlist = PVE::Cluster::get_vmlist() || {};
+	    my $idlist = $vmlist->{ids} || {};
+
+	    my $rrd = PVE::Cluster::rrd_dump();
+
+	    my $pool_config = $usercfg->{pools}->{$poolid};
+
+	    die "pool '$poolid' does not exist\n" if !$pool_config;
+
+	    my $members = [];
+	    for my $vmid (sort keys %{$pool_config->{vms}}) {
+		my $vmdata = $idlist->{$vmid};
+		next if !$vmdata || defined($param->{type}) && $param->{type} ne $vmdata->{type};
+		my $entry = PVE::API2Tools::extract_vm_stats($vmid, $vmdata, $rrd);
+		push @$members, $entry;
+	    }
+
+	    my $nodename = PVE::INotify::nodename();
+	    my $cfg = PVE::Storage::config();
+	    if (!defined($param->{type}) || $param->{type} eq 'storage') {
+		for my $storeid (sort keys %{$pool_config->{storage}}) {
+		    my $scfg = PVE::Storage::storage_config ($cfg, $storeid, 1);
+		    next if !$scfg;
+
+		    my $storage_node = $nodename; # prefer local node
+		    if ($scfg->{nodes} && !$scfg->{nodes}->{$storage_node}) {
+			for my $node (sort keys(%{$scfg->{nodes}})) {
+			    $storage_node = $node;
+			    last;
+			}
+		    }
+
+		    my $entry = PVE::API2Tools::extract_storage_stats($storeid, $scfg, $storage_node, $rrd);
+		    push @$members, $entry;
+		}
+	    }
+
+	    my $pool_info = {
+		members => $members,
+	    };
+	    $pool_info->{comment} = $pool_config->{comment} if defined($pool_config->{comment});
+	    $pool_info->{poolid} = $poolid;
+
+	    push @$res, $pool_info;
+	} else {
+	    for my $pool (sort keys %{$usercfg->{pools}}) {
+		next if !$rpcenv->check($authuser, "/pool/$pool", [ 'Pool.Audit' ], 1);
+
+		my $entry = { poolid => $pool };
+		my $pool_config = $usercfg->{pools}->{$pool};
+		$entry->{comment} = $pool_config->{comment} if defined($pool_config->{comment});
+		push @$res, $entry;
+	    }
 	}
 
 	return $res;
@@ -92,6 +184,11 @@ __PACKAGE__->register_method ({
 	    my $pool = $param->{poolid};
 
 	    die "pool '$pool' already exists\n" if $usercfg->{pools}->{$pool};
+	    if ($pool =~ m!^(.*)/[^/]+$!) {
+		my $parent = $1;
+		die "parent '$parent' of pool '$pool' does not exist\n"
+		    if !defined($usercfg->{pools}->{$parent});
+	    }
 
 	    $usercfg->{pools}->{$pool} = {
 		vms => {},
@@ -107,7 +204,7 @@ __PACKAGE__->register_method ({
     }});
 
 __PACKAGE__->register_method ({
-    name => 'update_pool',
+    name => 'update_pool_deprecated',
     protected => 1,
     path => '{poolid}',
     method => 'PUT',
@@ -115,9 +212,56 @@ __PACKAGE__->register_method ({
 	description => "You also need the right to modify permissions on any object you add/delete.",
 	check => ['perm', '/pool/{poolid}', ['Pool.Allocate']],
     },
-    description => "Update pool data.",
+    description => "Update pool data (deprecated, no support for nested pools - use 'PUT /pools/?poolid={poolid}' instead).",
     parameters => {
-   	additionalProperties => 0,
+	additionalProperties => 0,
+	properties => {
+	    poolid => { type => 'string', format => 'pve-poolid' },
+	    comment => { type => 'string', optional => 1 },
+	    vms => {
+		description => 'List of guest VMIDs to add or remove from this pool.',
+		type => 'string',  format => 'pve-vmid-list',
+		optional => 1,
+	    },
+	    storage => {
+		description => 'List of storage IDs to add or remove from this pool.',
+		type => 'string',  format => 'pve-storage-id-list',
+		optional => 1,
+	    },
+	    'allow-move' => {
+		description => 'Allow adding a guest even if already in another pool.'
+		    .' The guest will be removed from its current pool and added to this one.',
+		type => 'boolean',
+		optional => 1,
+		default => 0,
+	    },
+	    delete => {
+		description => 'Remove the passed VMIDs and/or storage IDs instead of adding them.',
+		type => 'boolean',
+		optional => 1,
+		default => 0,
+	    },
+	},
+    },
+    returns => { type => 'null' },
+    code => sub {
+	my ($param) = @_;
+
+	return __PACKAGE__->update_pool($param);
+    }});
+
+__PACKAGE__->register_method ({
+    name => 'update_pool',
+    protected => 1,
+    path => '',
+    method => 'PUT',
+    permissions => {
+	description => "You also need the right to modify permissions on any object you add/delete.",
+	check => ['perm', '/pool/{poolid}', ['Pool.Allocate']],
+    },
+    description => "Update pool.",
+    parameters => {
+	additionalProperties => 0,
 	properties => {
 	    poolid => { type => 'string', format => 'pve-poolid' },
 	    comment => { type => 'string', optional => 1 },
@@ -215,7 +359,7 @@ __PACKAGE__->register_method ({
     permissions => {
 	check => ['perm', '/pool/{poolid}', ['Pool.Audit']],
     },
-    description => "Get pool configuration.",
+    description => "Get pool configuration (deprecated, no support for nested pools, use 'GET /pools/?poolid={poolid}').",
     parameters => {
 	additionalProperties => 0,
 	properties => {
@@ -270,60 +414,38 @@ __PACKAGE__->register_method ({
     code => sub {
 	my ($param) = @_;
 
-	my $usercfg = cfs_read_file("user.cfg");
-
-	my $vmlist = PVE::Cluster::get_vmlist() || {};
-	my $idlist = $vmlist->{ids} || {};
-
-	my $rrd = PVE::Cluster::rrd_dump();
-
-	my $pool = $param->{poolid};
-
-	my $pool_config = $usercfg->{pools}->{$pool};
-
-	die "pool '$pool' does not exist\n" if !$pool_config;
-
-	my $members = [];
-	for my $vmid (sort keys %{$pool_config->{vms}}) {
-	    my $vmdata = $idlist->{$vmid};
-	    next if !$vmdata || defined($param->{type}) && $param->{type} ne $vmdata->{type};
-	    my $entry = PVE::API2Tools::extract_vm_stats($vmid, $vmdata, $rrd);
-	    push @$members, $entry;
-	}
+	my $pool_info = __PACKAGE__->index($param);
+	return $pool_info->[0];
+    }});
 
-	my $nodename = PVE::INotify::nodename();
-	my $cfg = PVE::Storage::config();
-	if (!defined($param->{type}) || $param->{type} eq 'storage') {
-	    for my $storeid (sort keys %{$pool_config->{storage}}) {
-		my $scfg = PVE::Storage::storage_config ($cfg, $storeid, 1);
-		next if !$scfg;
-
-		my $storage_node = $nodename; # prefer local node
-		if ($scfg->{nodes} && !$scfg->{nodes}->{$storage_node}) {
-		    for my $node (sort keys(%{$scfg->{nodes}})) {
-			$storage_node = $node;
-			last;
-		    }
-		}
 
-		my $entry = PVE::API2Tools::extract_storage_stats($storeid, $scfg, $storage_node, $rrd);
-		push @$members, $entry;
-	    }
+__PACKAGE__->register_method ({
+    name => 'delete_pool_deprecated',
+    protected => 1,
+    path => '{poolid}',
+    method => 'DELETE',
+    permissions => {
+	description => "You can only delete empty pools (no members).",
+	check => ['perm', '/pool/{poolid}', ['Pool.Allocate']],
+    },
+    description => "Delete pool (deprecated, no support for nested pools, use 'DELETE /pools/?poolid={poolid}').",
+    parameters => {
+	additionalProperties => 0,
+	properties => {
+	    poolid => { type => 'string', format => 'pve-poolid' },
 	}
+    },
+    returns => { type => 'null' },
+    code => sub {
+	my ($param) = @_;
 
-	my $res = {
-	    members => $members,
-	};
-	$res->{comment} = $pool_config->{comment} if defined($pool_config->{comment});
-
-	return $res;
+	return __PACKAGE__->delete_pool($param);
     }});
 
-
 __PACKAGE__->register_method ({
     name => 'delete_pool',
     protected => 1,
-    path => '{poolid}',
+    path => '',
     method => 'DELETE',
     permissions => {
 	description => "You can only delete empty pools (no members).",
@@ -354,6 +476,9 @@ __PACKAGE__->register_method ({
 
 	    my $pool_config = $usercfg->{pools}->{$pool};
 	    die "pool '$pool' does not exist\n" if !$pool_config;
+	    for my $subpool (sort keys %{$pool_config->{pools}}) {
+		die "pool '$pool' is not empty (contains pool '$subpool')\n";
+	    }
 
 	    for my $vmid (sort keys %{$pool_config->{vms}}) {
 		next if !$idlist->{$vmid}; # ignore destroyed guests
-- 
2.39.2

[pve-devel] [PATCH manager 2/2] ui: pools: switch to new API endpoints

[Fabian Grünbichler]

which support nested pools. mostly straight-forward, only pool deletion and the
members grid need some special attention.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    Thanks Dominik for pointing me at the 'root' part of the proxy :)

 www/manager6/dc/PoolView.js      |  3 +++
 www/manager6/grid/PoolMembers.js | 14 ++++++++------
 www/manager6/pool/StatusView.js  |  2 +-
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/www/manager6/dc/PoolView.js b/www/manager6/dc/PoolView.js
index db97cbe72..741b2025b 100644
--- a/www/manager6/dc/PoolView.js
+++ b/www/manager6/dc/PoolView.js
@@ -31,6 +31,9 @@ Ext.define('PVE.dc.PoolView', {
 	    callback: function() {
 		reload();
 	    },
+	    getUrl: function(rec) {
+		return '/pools/?poolid=' + rec.getId();
+	    },
 	});
 
 	var run_editor = function() {
diff --git a/www/manager6/grid/PoolMembers.js b/www/manager6/grid/PoolMembers.js
index 74950d80e..75f20cab1 100644
--- a/www/manager6/grid/PoolMembers.js
+++ b/www/manager6/grid/PoolMembers.js
@@ -17,8 +17,9 @@ Ext.define('PVE.pool.AddVM', {
 	    throw "no pool specified";
 	}
 
-	me.url = "/pools/" + me.pool;
+	me.url = '/pools/';
 	me.method = 'PUT';
+	me.extraRequestParams.poolid = me.pool;
 
 	var vmsField = Ext.create('Ext.form.field.Text', {
 	    name: 'vms',
@@ -120,8 +121,9 @@ Ext.define('PVE.pool.AddStorage', {
 
 	me.isCreate = true;
 	me.isAdd = true;
-	me.url = "/pools/" + me.pool;
+	me.url = "/pools/";
 	me.method = 'PUT';
+	me.extraRequestParams.poolid = me.pool;
 
 	Ext.apply(me, {
 	    subject: gettext('Storage'),
@@ -168,8 +170,8 @@ Ext.define('PVE.grid.PoolMembers', {
 	    ],
 	    proxy: {
 		type: 'proxmox',
-		root: 'data.members',
-		url: "/api2/json/pools/" + me.pool,
+		root: 'data[0].members',
+		url: "/api2/json/pools/?poolid=" + me.pool,
 	    },
 	});
 
@@ -192,7 +194,7 @@ Ext.define('PVE.grid.PoolMembers', {
 					 "'" + rec.data.id + "'");
 	    },
 	    handler: function(btn, event, rec) {
-		var params = { 'delete': 1 };
+		var params = { 'delete': 1, poolid: me.pool };
 		if (rec.data.type === 'storage') {
 		    params.storage = rec.data.storage;
 		} else if (rec.data.type === 'qemu' || rec.data.type === 'lxc' || rec.data.type === 'openvz') {
@@ -202,7 +204,7 @@ Ext.define('PVE.grid.PoolMembers', {
 		}
 
 		Proxmox.Utils.API2Request({
-		    url: '/pools/' + me.pool,
+		    url: '/pools/',
 		    method: 'PUT',
 		    params: params,
 		    waitMsgTarget: me,
diff --git a/www/manager6/pool/StatusView.js b/www/manager6/pool/StatusView.js
index 302ae5ab0..3d46b3b1a 100644
--- a/www/manager6/pool/StatusView.js
+++ b/www/manager6/pool/StatusView.js
@@ -24,7 +24,7 @@ Ext.define('PVE.pool.StatusView', {
 	};
 
 	Ext.apply(me, {
-	    url: "/api2/json/pools/" + pool,
+	    url: "/api2/json/pools/?poolid=" + pool,
 	    rows: rows,
 	});
 
-- 
2.39.2

[pve-devel] applied-series: [PATCH access-control/manager 0/4] fix #1148: nested pools

[Wolfgang Bumiller]

applied series, bumped access control and manager's dep on it

On Mon, Nov 20, 2023 at 08:22:38AM +0100, Fabian Grünbichler wrote:
> this series extends the poolname to allow (for now) two '/' inside the
> name to nest pools like this:
> 
> parent/child/grandchild
> 
> since '/' is a bad character for path parameters, some API shuffling is
> needed. other approaches that were tried but discarded, or rejected
> outright:
> - urlencoding
> - systemd-escape
> - manual encoding/escaping
> - just encoding the parent/children in user.cfg, but not in the pool name
> 
> switching over some of the UI pool lists to also nest them visually should
> be do-able as a follow-up.
> 
> changes since RFC:
> - some style improvements, RE adaptation
> 
> pve-access-control:
> 
> Fabian Grünbichler (2):
>   allow up to three levels of pool nesting
>   pools: record parent/subpool information
> 
>  src/PVE/AccessControl.pm  | 26 +++++++++++++++++++++++---
>  src/test/parser_writer.pl |  4 ++++
>  src/test/perm-test6.pl    | 16 ++++++++++++++++
>  src/test/test6.cfg        |  5 +++++
>  4 files changed, 48 insertions(+), 3 deletions(-)
> 
> pve-manager:
> 
> Fabian Grünbichler (2):
>   api: pools: support nested pools
>   ui: pools: switch to new API endpoints
> 
>  PVE/API2/Pool.pm                 | 243 +++++++++++++++++++++++--------
>  www/manager6/dc/PoolView.js      |   3 +
>  www/manager6/grid/PoolMembers.js |  14 +-
>  www/manager6/pool/StatusView.js  |   2 +-
>  4 files changed, 196 insertions(+), 66 deletions(-)
> 
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH access-control 2/2] pools: record parent/subpool information

[Fabian Grünbichler]

> Wolfgang Bumiller <w.bumiller@proxmox.com> hat am 17.11.2023 11:10 CET geschrieben:
> 
>  
> On Thu, Nov 16, 2023 at 04:31:26PM +0100, Fabian Grünbichler wrote:
> > and ensure a missing intermediate pool exists at all times.
> > 
> > Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> > ---
> > 
> > Notes:
> >     a "missing link" should never happen when modifying via the API (both deletion
> >     with children and addition without the parent existing is blocked there), but
> >     it could happen when manually editing the config.
> > 
> >  src/PVE/AccessControl.pm  | 14 +++++++++++++-
> >  src/test/parser_writer.pl |  4 ++++
> >  2 files changed, 17 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
> > index d9ae611..e33f844 100644
> > --- a/src/PVE/AccessControl.pm
> > +++ b/src/PVE/AccessControl.pm
> > @@ -1529,7 +1529,19 @@ sub parse_user_config {
> >  	    }
> >  
> >  	    # make sure to add the pool (even if there are no members)
> > -	    $cfg->{pools}->{$pool} = { vms => {}, storage => {} } if !$cfg->{pools}->{$pool};
> > +	    $cfg->{pools}->{$pool} = { vms => {}, storage => {}, pools => {} } if !$cfg->{pools}->{$pool};
> > +
> > +	    if ($pool =~ m!/!) {
> > +		my $curr = $pool;
> > +		while ($curr =~ m!^(.*)/[^/]+$!) {
> 
> I wonder if we should use `.+` instead of `.*`.
> This way it would work the same even with a leading slash.
> That said, we don't allow leading slashes and there's a verify_poolname
> further up in the function so it doesn't really matter much.
> We just need to be careful that we never allow/introduce leading slashes
> anywhere, otherwise this runs with a final iteration where $parent is an
> empty string.

ack.

> > +		    # ensure nested pool info is correctly recorded
> > +		    my $parent = $1;
> > +		    $cfg->{pools}->{$curr}->{parent} = $parent;
> > +		    $cfg->{pools}->{$parent} = { vms => {}, storage => {}, pools => {} } if !$cfg->{pools}->{$parent};
> 
> (could use //= instead of the suffix if, IMO a bit easier to read (and
> doesn't break the 100 char limit :p)

that style is used across the whole parser here, I am always a bit hesitant to mix styles within a sub as IMHO that makes it harder to parse.

move the post-if to its own line, and optional follow-up to convert the whole parser to drop post ifs for initialization? ;)

Re: [pve-devel] [PATCH access-control 2/2] pools: record parent/subpool information

[Wolfgang Bumiller]

On Thu, Nov 16, 2023 at 04:31:26PM +0100, Fabian Grünbichler wrote:
> and ensure a missing intermediate pool exists at all times.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
> 
> Notes:
>     a "missing link" should never happen when modifying via the API (both deletion
>     with children and addition without the parent existing is blocked there), but
>     it could happen when manually editing the config.
> 
>  src/PVE/AccessControl.pm  | 14 +++++++++++++-
>  src/test/parser_writer.pl |  4 ++++
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
> index d9ae611..e33f844 100644
> --- a/src/PVE/AccessControl.pm
> +++ b/src/PVE/AccessControl.pm
> @@ -1529,7 +1529,19 @@ sub parse_user_config {
>  	    }
>  
>  	    # make sure to add the pool (even if there are no members)
> -	    $cfg->{pools}->{$pool} = { vms => {}, storage => {} } if !$cfg->{pools}->{$pool};
> +	    $cfg->{pools}->{$pool} = { vms => {}, storage => {}, pools => {} } if !$cfg->{pools}->{$pool};
> +
> +	    if ($pool =~ m!/!) {
> +		my $curr = $pool;
> +		while ($curr =~ m!^(.*)/[^/]+$!) {

I wonder if we should use `.+` instead of `.*`.
This way it would work the same even with a leading slash.
That said, we don't allow leading slashes and there's a verify_poolname
further up in the function so it doesn't really matter much.
We just need to be careful that we never allow/introduce leading slashes
anywhere, otherwise this runs with a final iteration where $parent is an
empty string.

> +		    # ensure nested pool info is correctly recorded
> +		    my $parent = $1;
> +		    $cfg->{pools}->{$curr}->{parent} = $parent;
> +		    $cfg->{pools}->{$parent} = { vms => {}, storage => {}, pools => {} } if !$cfg->{pools}->{$parent};

(could use //= instead of the suffix if, IMO a bit easier to read (and
doesn't break the 100 char limit :p)

> +		    $cfg->{pools}->{$parent}->{pools}->{$curr} = 1;
> +		    $curr = $parent;
> +		}
> +	    }
>  
>  	    $cfg->{pools}->{$pool}->{comment} = PVE::Tools::decode_text($comment) if $comment;
>  
> diff --git a/src/test/parser_writer.pl b/src/test/parser_writer.pl
> index 65a70eb..80c346b 100755
> --- a/src/test/parser_writer.pl
> +++ b/src/test/parser_writer.pl
> @@ -237,21 +237,25 @@ my $default_cfg = {
>  	'id' => 'testpool',
>  	vms => {},
>  	storage => {},
> +	pools => {},
>      },
>      test_pool_members => {
>  	'id' => 'testpool',
>  	vms => { 123 => 1, 1234 => 1},
>  	storage => { 'local' => 1, 'local-zfs' => 1},
> +	pools => {},
>      },
>      test_pool_duplicate_vms => {
>  	'id' => 'test_duplicate_vms',
>  	vms => {},
>  	storage => {},
> +	pools => {},
>      },
>      test_pool_duplicate_storages => {
>  	'id' => 'test_duplicate_storages',
>  	vms => {},
>  	storage => { 'local' => 1, 'local-zfs' => 1},
> +	pools => {},
>      },
>      acl_simple_user => {
>  	'path' => '/',
> -- 
> 2.39.2

[pve-devel] [PATCH access-control 2/2] pools: record parent/subpool information

[Fabian Grünbichler]

and ensure a missing intermediate pool exists at all times.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    a "missing link" should never happen when modifying via the API (both deletion
    with children and addition without the parent existing is blocked there), but
    it could happen when manually editing the config.

 src/PVE/AccessControl.pm  | 14 +++++++++++++-
 src/test/parser_writer.pl |  4 ++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
index d9ae611..e33f844 100644
--- a/src/PVE/AccessControl.pm
+++ b/src/PVE/AccessControl.pm
@@ -1529,7 +1529,19 @@ sub parse_user_config {
 	    }
 
 	    # make sure to add the pool (even if there are no members)
-	    $cfg->{pools}->{$pool} = { vms => {}, storage => {} } if !$cfg->{pools}->{$pool};
+	    $cfg->{pools}->{$pool} = { vms => {}, storage => {}, pools => {} } if !$cfg->{pools}->{$pool};
+
+	    if ($pool =~ m!/!) {
+		my $curr = $pool;
+		while ($curr =~ m!^(.*)/[^/]+$!) {
+		    # ensure nested pool info is correctly recorded
+		    my $parent = $1;
+		    $cfg->{pools}->{$curr}->{parent} = $parent;
+		    $cfg->{pools}->{$parent} = { vms => {}, storage => {}, pools => {} } if !$cfg->{pools}->{$parent};
+		    $cfg->{pools}->{$parent}->{pools}->{$curr} = 1;
+		    $curr = $parent;
+		}
+	    }
 
 	    $cfg->{pools}->{$pool}->{comment} = PVE::Tools::decode_text($comment) if $comment;
 
diff --git a/src/test/parser_writer.pl b/src/test/parser_writer.pl
index 65a70eb..80c346b 100755
--- a/src/test/parser_writer.pl
+++ b/src/test/parser_writer.pl
@@ -237,21 +237,25 @@ my $default_cfg = {
 	'id' => 'testpool',
 	vms => {},
 	storage => {},
+	pools => {},
     },
     test_pool_members => {
 	'id' => 'testpool',
 	vms => { 123 => 1, 1234 => 1},
 	storage => { 'local' => 1, 'local-zfs' => 1},
+	pools => {},
     },
     test_pool_duplicate_vms => {
 	'id' => 'test_duplicate_vms',
 	vms => {},
 	storage => {},
+	pools => {},
     },
     test_pool_duplicate_storages => {
 	'id' => 'test_duplicate_storages',
 	vms => {},
 	storage => { 'local' => 1, 'local-zfs' => 1},
+	pools => {},
     },
     acl_simple_user => {
 	'path' => '/',
-- 
2.39.2

[pve-devel] [PATCH access-control 2/2] pools: record parent/subpool information

[Fabian Grünbichler]

and ensure a missing intermediate pool exists at all times.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    a "missing link" should never happen when modifying via the API (both deletion
    with children and addition without the parent existing is blocked there), but
    it could happen when manually editing the config.

 src/PVE/AccessControl.pm  | 14 +++++++++++++-
 src/test/parser_writer.pl |  4 ++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
index d9ae611..e33f844 100644
--- a/src/PVE/AccessControl.pm
+++ b/src/PVE/AccessControl.pm
@@ -1529,7 +1529,19 @@ sub parse_user_config {
 	    }
 
 	    # make sure to add the pool (even if there are no members)
-	    $cfg->{pools}->{$pool} = { vms => {}, storage => {} } if !$cfg->{pools}->{$pool};
+	    $cfg->{pools}->{$pool} = { vms => {}, storage => {}, pools => {} } if !$cfg->{pools}->{$pool};
+
+	    if ($pool =~ m!/!) {
+		my $curr = $pool;
+		while ($curr =~ m!^(.*)/[^/]+$!) {
+		    # ensure nested pool info is correctly recorded
+		    my $parent = $1;
+		    $cfg->{pools}->{$curr}->{parent} = $parent;
+		    $cfg->{pools}->{$parent} = { vms => {}, storage => {}, pools => {} } if !$cfg->{pools}->{$parent};
+		    $cfg->{pools}->{$parent}->{pools}->{$curr} = 1;
+		    $curr = $parent;
+		}
+	    }
 
 	    $cfg->{pools}->{$pool}->{comment} = PVE::Tools::decode_text($comment) if $comment;
 
diff --git a/src/test/parser_writer.pl b/src/test/parser_writer.pl
index 65a70eb..80c346b 100755
--- a/src/test/parser_writer.pl
+++ b/src/test/parser_writer.pl
@@ -237,21 +237,25 @@ my $default_cfg = {
 	'id' => 'testpool',
 	vms => {},
 	storage => {},
+	pools => {},
     },
     test_pool_members => {
 	'id' => 'testpool',
 	vms => { 123 => 1, 1234 => 1},
 	storage => { 'local' => 1, 'local-zfs' => 1},
+	pools => {},
     },
     test_pool_duplicate_vms => {
 	'id' => 'test_duplicate_vms',
 	vms => {},
 	storage => {},
+	pools => {},
     },
     test_pool_duplicate_storages => {
 	'id' => 'test_duplicate_storages',
 	vms => {},
 	storage => { 'local' => 1, 'local-zfs' => 1},
+	pools => {},
     },
     acl_simple_user => {
 	'path' => '/',
-- 
2.39.2