From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id A3D761FF135 for ; Thu, 02 Jul 2026 15:14:48 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id CFEB32142C; Thu, 02 Jul 2026 15:14:47 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Thu, 02 Jul 2026 15:14:44 +0200 Message-Id: From: "Daniel Kral" To: "David Riley" , Subject: Re: [PATCH pve-access-control v4 2/5] fix #7520: test: add unit tests for sdn acl pruning logic X-Mailer: aerc 0.21.0-147-g43ac7b685014-dirty References: <20260626105258.56914-1-d.riley@proxmox.com> <20260626105258.56914-3-d.riley@proxmox.com> In-Reply-To: <20260626105258.56914-3-d.riley@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1782998078400 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.075 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: L25HNCEOWYP7IBEHB75ZR3G6SJE2CYIL X-Message-ID-Hash: L25HNCEOWYP7IBEHB75ZR3G6SJE2CYIL X-MailFrom: d.kral@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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=3D7520 > Signed-off-by: David Riley > --- > 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 =3D [ [ snip ] > +]; > + > +# mocking > +my $cluster_module =3D Test::MockModule->new('PVE::Cluster'); > +$cluster_module->noop('cfs_update'); > + > +my $mocked_user_cfg_tree; > +my $access_control_module =3D Test::MockModule->new('PVE::AccessControl'= ); > + > +$access_control_module->mock( > + 'cfs_read_file', > + sub { > + my ($filename) =3D @_; > + 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) =3D @_; > + if ($filename eq 'user.cfg') { > + $mocked_user_cfg_tree =3D $cfg; > + } > + }, > +); > + > +$access_control_module->mock( > + 'lock_user_config', > + sub { > + my ($code, $errmsg) =3D @_; > + eval { $code->() }; > + if (my $err =3D $@) { > + die "$errmsg: $err\n"; > + } > + }, > +); nit: can be written as $access_control_module->mock( cfs_read_file =3D> sub { ... }, cfs_write_file =3D> sub { ... }, lock_user_config =3D> sub { ... }, ); > + > +my $rpcenv =3D PVE::RPCEnvironment->init('cli'); > + > +# helper > +sub check_roles { > + my ($user, $path, $expected_result) =3D @_; > + > + my $roles =3D PVE::AccessControl::roles($rpcenv->{user_cfg}, $user, = $path); > + my $res =3D join(',', sort keys %$roles); > + > + is($res, $expected_result, "Roles for $user on $path should be '$exp= ected_result'"); > +} > + > +sub main { > + for my $test ($tests->@*) { > + subtest $test->{description} =3D> sub { > + $mocked_user_cfg_tree =3D > + PVE::AccessControl::parse_user_config("user.cfg", $test-= >{raw}); > + $rpcenv->{user_cfg} =3D $mocked_user_cfg_tree; > + > + eval { PVE::AccessControl::remove_sdn_resource_access($test-= >{prune_paths}); }; > + my $err =3D $@; > + > + if ($test->{expected_error}) { > + ok($err, $test->{error_desc}); > + like($err, $test->{expected_error}, "Error message match= es expected string"); > + } else { > + is($err, '', "Pruning completed without errors"); > + > + $rpcenv->{user_cfg} =3D $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?