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

[pve-devel] [RFC 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

RFC for now, but getting some feedback while I'm finishing up the
resource limit series based upon it might be sensible.

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

pve-access-control:

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

 src/PVE/AccessControl.pm  | 24 +++++++++++++++++++++---
 src/test/parser_writer.pl |  4 ++++
 src/test/perm-test6.pl    | 16 ++++++++++++++++
 src/test/test6.cfg        |  5 +++++
 4 files changed, 46 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] 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>
---
 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..d9ae611 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/(:?[[:alnum:]\.\-\_]+\/?)+
 	|/sdn
 	|/sdn/controllers
 	|/sdn/controllers/[[:alnum:]\_\-]+
@@ -1318,13 +1318,19 @@ PVE::JSONSchema::register_format('pve-poolid', \&verify_poolname);
 sub verify_poolname {
     my ($poolname, $noerr) = @_;
 
-    if ($poolname !~ m/^[A-Za-z0-9\.\-_]+$/) {
+    if ($poolname !~ m!^(?:[A-Za-z0-9\.\-_]+/?)+$!) {
 
 	die "pool name '$poolname' contains invalid characters\n" if !$noerr;
 
 	return undef;
     }
 
+    if (split("/", $poolname) > 3) {
+	die "pool name '$poolname' nested too deeply (max levels = 3)\n" if !$noerr;
+
+	return undef;
+    }
+
     return $poolname;
 }
 
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

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

[Wolfgang Bumiller]

On Thu, Nov 16, 2023 at 04:31:25PM +0100, Fabian Grünbichler wrote:
> with ACLs being inherited along the pool hierarchy.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
>  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..d9ae611 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/(:?[[:alnum:]\.\-\_]+\/?)+

Should we incorporate the 3 level limit here?
    eg. [chars]+(?:/[chars]+){0,2}
Although regex would differ from the one used below (although it could
use the same with only the `{0,2}` bit removed...).

>  	|/sdn
>  	|/sdn/controllers
>  	|/sdn/controllers/[[:alnum:]\_\-]+

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

[Fabian Grünbichler]

> Wolfgang Bumiller <w.bumiller@proxmox.com> hat am 17.11.2023 11:00 CET geschrieben:
> 
>  
> On Thu, Nov 16, 2023 at 04:31:25PM +0100, Fabian Grünbichler wrote:
> > with ACLs being inherited along the pool hierarchy.
> > 
> > Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> > ---
> >  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..d9ae611 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/(:?[[:alnum:]\.\-\_]+\/?)+
> 
> Should we incorporate the 3 level limit here?
>     eg. [chars]+(?:/[chars]+){0,2}
> Although regex would differ from the one used below (although it could
> use the same with only the `{0,2}` bit removed...).

well, there is no harm in accepting a sub-ACL path that has no effect. we usually have the opposite issue (forgetting to add/extending the entries here), but I also don't mind adding it here and a reminder comment below where the limit is enforced for the config/parameter values.
 
> >  	|/sdn
> >  	|/sdn/controllers
> >  	|/sdn/controllers/[[:alnum:]\_\-]+

[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

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

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? ;)

[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

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

[Philipp Hufnagl]

I think you made a mistake when sending this patch.

There are only one 3 out of 4 files attached and

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

appears to be double.

[pve-devel] [PATCH manager 1/2] 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

Re: [pve-devel] [PATCH manager 1/2] api: pools: support nested pools

[Wolfgang Bumiller]

minor issue

On Fri, Nov 17, 2023 at 08:09:10AM +0100, Fabian Grünbichler wrote:
> 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
> @@ -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}}) {

would prefer $pool_config->{pools}->%*

> +		die "pool '$pool' is not empty (contains pool '$subpool')\n";

a 'for' loop for a single error entry, looks like you meant to do
something like this instead:

    if (length(my $subpools = join(', ', sort keys $pool_config->{pools}->%*))) {
        die "pool '$pool' is not empty (contains pool '$subpools')\n";
    }

> +	    }
>  
>  	    for my $vmid (sort keys %{$pool_config->{vms}}) {
>  		next if !$idlist->{$vmid}; # ignore destroyed guests
> -- 
> 2.39.2

Re: [pve-devel] [PATCH manager 1/2] api: pools: support nested pools

[Fabian Grünbichler]

> Wolfgang Bumiller <w.bumiller@proxmox.com> hat am 17.11.2023 12:58 CET geschrieben: 
> minor issue

> > 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
> > @@ -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}}) {
> 
> would prefer $pool_config->{pools}->%*

I'll add a follow-up patch that changes the whole sub as clean-up, I used the old style to be internally consist with the loops below ;)

[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 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