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

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

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