Re: [PATCH pve-access-control v4 2/5] fix #7520: test: add unit tests for sdn acl pruning logic

["Daniel Kral" <d.kral@proxmox.com>]

Nice unit test cases!

Some small inline notes.

The fix #7520 in the patch subject might be unnecessary as test cases
aren't directly related to the bugzilla entry, but no hard feelings
about this.

On Fri Jun 26, 2026 at 12:52 PM CEST, David Riley wrote:
> Add unit tests to validate the behavior of the SDN resource access
> pruning logic.
>
> The tests cover all designated SDN resource paths including zones,
> vnets (with/without tags), controllers, prefix-lists, route-maps, and
> fabrics, ensuring explicit permissions are dropped when resources are
> deleted.
>
> Link: https://bugzilla.proxmox.com/show_bug.cgi?id=7520
> Signed-off-by: David Riley <d.riley@proxmox.com>
> ---
>  src/test/Makefile                |   1 +
>  src/test/sdn_acl_pruning_test.pl | 148 +++++++++++++++++++++++++++++++
>  2 files changed, 149 insertions(+)
>  create mode 100644 src/test/sdn_acl_pruning_test.pl
>
> diff --git a/src/test/Makefile b/src/test/Makefile
> index 53f2da7..2f44186 100644
> --- a/src/test/Makefile
> +++ b/src/test/Makefile
> @@ -14,3 +14,4 @@ check:
>  	perl -I.. perm-test8.pl
>  	perl -I.. realm_sync_test.pl
>  	perl -I.. api-tests.pl
> +	perl -I.. sdn_acl_pruning_test.pl
> diff --git a/src/test/sdn_acl_pruning_test.pl b/src/test/sdn_acl_pruning_test.pl
> new file mode 100644
> index 0000000..f9d4582
> --- /dev/null
> +++ b/src/test/sdn_acl_pruning_test.pl
> @@ -0,0 +1,148 @@
> +#!/usr/bin/env perl
> +
> +use strict;
> +use warnings;

for new modules/scripts:

use v5.36;

> +
> +use lib qw(..);
> +
> +use PVE::AccessControl;
> +use PVE::Tools;
> +use PVE::RPCEnvironment;

should be ideally sorted alphabetically [0]

> +
> +use Test::More;
> +use Test::MockModule;

these should be sorted and before the PVE packages [0]

[0] https://pve.proxmox.com/wiki/Perl_Style_Guide#Module_Dependencies

> +
> +# test cases
> +my $tests = [

[ snip ]

> +];
> +
> +# mocking
> +my $cluster_module = Test::MockModule->new('PVE::Cluster');
> +$cluster_module->noop('cfs_update');
> +
> +my $mocked_user_cfg_tree;
> +my $access_control_module = Test::MockModule->new('PVE::AccessControl');
> +
> +$access_control_module->mock(
> +    'cfs_read_file',
> +    sub {
> +        my ($filename) = @_;
> +        if ($filename eq 'user.cfg') {
> +            return $mocked_user_cfg_tree;
> +        }
> +        die "mock cfs_read_file: unexpected file $filename";
> +    },
> +);
> +
> +$access_control_module->mock(
> +    'cfs_write_file',
> +    sub {
> +        my ($filename, $cfg) = @_;
> +        if ($filename eq 'user.cfg') {
> +            $mocked_user_cfg_tree = $cfg;
> +        }
> +    },
> +);
> +
> +$access_control_module->mock(
> +    'lock_user_config',
> +    sub {
> +        my ($code, $errmsg) = @_;
> +        eval { $code->() };
> +        if (my $err = $@) {
> +            die "$errmsg: $err\n";
> +        }
> +    },
> +);

nit: can be written as

    $access_control_module->mock(
        cfs_read_file => sub { ... },
        cfs_write_file => sub { ... },
        lock_user_config => sub { ... },
    );


> +
> +my $rpcenv = PVE::RPCEnvironment->init('cli');
> +
> +# helper
> +sub check_roles {
> +    my ($user, $path, $expected_result) = @_;
> +
> +    my $roles = PVE::AccessControl::roles($rpcenv->{user_cfg}, $user, $path);
> +    my $res = join(',', sort keys %$roles);
> +
> +    is($res, $expected_result, "Roles for $user on $path should be '$expected_result'");
> +}
> +
> +sub main {
> +    for my $test ($tests->@*) {
> +        subtest $test->{description} => sub {
> +            $mocked_user_cfg_tree =
> +                PVE::AccessControl::parse_user_config("user.cfg", $test->{raw});
> +            $rpcenv->{user_cfg} = $mocked_user_cfg_tree;
> +
> +            eval { PVE::AccessControl::remove_sdn_resource_access($test->{prune_paths}); };
> +            my $err = $@;
> +
> +            if ($test->{expected_error}) {
> +                ok($err, $test->{error_desc});
> +                like($err, $test->{expected_error}, "Error message matches expected string");
> +            } else {
> +                is($err, '', "Pruning completed without errors");
> +
> +                $rpcenv->{user_cfg} = $mocked_user_cfg_tree;
> +
> +                for my $role_check ($test->{expected_roles}->@*) {
> +                    check_roles(@$role_check);
> +                }
> +            }
> +        };
> +    }
> +
> +    done_testing();
> +}
> +
> +main();

for the perl script, the for loop + done_testing() could be the last
lines, so no need for the main() subroutine here ;)

> +exit(0);

Hm, is the exit(0) really needed here?