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

[David Riley <d.riley@proxmox.com>]

Thanks for taking a look.

I'll also adapt the tests from patch 4/5 to include the feedback
you gave me here as well.

Some comments inline.

On 7/2/26 3:14 PM, Daniel Kral wrote:
> 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;

Will fix, and I will sort the dependencies below as well.

>> +
>> +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 { ... },
>      );
>

Ah that makes it a lot cleaner. Thanks for the heads up.


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


I initially thought I might add some other test cases here, but never actually
did. I'll clean it up.

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

No it's not needed. I'll remove it.