[PATCH access-control/network v5 0/7] fix #7520: sdn: prune orphaned ACLs and handle VNet migrations

[PATCH access-control/network v5 0/7] fix #7520: sdn: prune orphaned ACLs and handle VNet migrations

[David Riley]

Thanks @Daniel K. for the thorough review.

Difference from v4:
* Instead of passing arrays of path segments [<zone>, <vnet>], paths
  are now passed as strings <zone>/<vnet> to match the actual ACL
  paths as there was no real benefit of using arrays.
* Moved helper functions into seperate commits and updated the commit
  messages accordingly.
* Fix: style nits (post if, for loop dereferencing, ...)
* Fix: tests nits (grouped mocking, removed main subroutine)

Difference from v3:
* Refactored tests
* Perl style guide refactor (for instead of foreach)
* Update: route_map_suffix regex used for detecting digits to match
  style guide recommendation

Difference from v2:
* Relocate VNet ACLs to the new zone path when a VNet is moved,
  including a validation check to abort on path conflicts.
* Refactor diff generation in preparation for an upcoming patch
  series, resolving #7294 [0]. The upcoming series will hook into the
  VNet diff to clean up pool members.
* Add unit testing for pruning and migration mechanism 

=== Original Cover Letter ===

Implement a pruning mechanism to clean up orphaned SDN ACL entries by
comparing the running configuration with the newly compiled state
during configuration commit.

This ensures state consistency for manual applies via the UI/API
as well as during the automatic configuration reload on system boot.
The pruning covers: 
* Zones
* VNets
* Fabrics
* Controllers
* Route maps
* Prefix lists 

IPAMs and DNS are excluded as they are not staged.

Link: https://bugzilla.proxmox.com/show_bug.cgi?id=7520

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=7294


pve-access-control:

David Riley (6):
  permission: add acl tree node parent lookup without node creation
  fix #7520: sdn: add pruning SDN resources ACLs
  test: add sdn acl pruning tests
  permission: add helper to check for active permissions
  fix #7520: sdn: add VNet ACL migration
  test: add sdn acl migration tests

 src/PVE/AccessControl.pm           | 155 ++++++++++++++++++++++++++
 src/test/Makefile                  |   3 +
 src/test/sdn_acl_migration_test.pl | 171 +++++++++++++++++++++++++++++
 src/test/sdn_acl_pruning_test.pl   | 134 ++++++++++++++++++++++
 4 files changed, 463 insertions(+)
 create mode 100644 src/test/sdn_acl_migration_test.pl
 create mode 100644 src/test/sdn_acl_pruning_test.pl


pve-network:

David Riley (1):
  fix #7520: config: prune orphaned and relocate moved VNet ACLs

 src/PVE/Network/SDN.pm | 115 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 115 insertions(+)


Summary over all repositories:
  5 files changed, 578 insertions(+), 0 deletions(-)

-- 
Generated by murpp 0.11.0

[PATCH pve-access-control v5 1/7] permission: add acl tree node parent lookup without node creation

[David Riley]

Add helper function to look up ACL nodes by path without creating
missing nodes along the path. This is necessary for safely locating
paths intended for deletion or migration without side effects.

This is in preparation for the following SDN resource ACL pruning and
VNet migration changes.

Signed-off-by: David Riley <d.riley@proxmox.com>
---
 src/PVE/AccessControl.pm | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
index 0d632b3..e26dfce 100644
--- a/src/PVE/AccessControl.pm
+++ b/src/PVE/AccessControl.pm
@@ -1000,6 +1000,31 @@ sub find_acl_tree_node {
     return $node;
 }
 
+# finds an ACL node by path without autovivifying (creating) missing
+# nodes. returns a list containing the parent node, the final path
+# segment, and the target node itself.
+sub find_acl_tree_node_parent {
+    my ($root, $path) = @_;
+
+    my $current = $root;
+    my $parent = undef;
+    my $last_key = undef;
+
+    my @split_path = split('/', $path);
+
+    for my $segment (@split_path) {
+        if (defined($current->{children}) && defined($current->{children}->{$segment})) {
+            $parent = $current;
+            $last_key = $segment;
+            $current = $current->{children}->{$segment};
+        } else {
+            return (undef, undef, undef);
+        }
+    }
+
+    return ($parent, $last_key, $current);
+}
+
 sub add_user_group {
     my ($username, $usercfg, $group) = @_;
 
-- 
2.47.3

[PATCH pve-access-control v5 2/7] fix #7520: sdn: add pruning SDN resources ACLs

[David Riley]

Add a cleanup routine to remove ACLs under the '/sdn/' path when the
corresponding resources are deleted.

This is a preparatory change that does not have an immediate effect on
its own, but lays the foundation for preventing dangling permission
states and ensures configuration consistency.

Link: https://bugzilla.proxmox.com/show_bug.cgi?id=7520
Signed-off-by: David Riley <d.riley@proxmox.com>
---
 src/PVE/AccessControl.pm | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
index e26dfce..8e2b4f2 100644
--- a/src/PVE/AccessControl.pm
+++ b/src/PVE/AccessControl.pm
@@ -1999,6 +1999,33 @@ sub remove_vm_from_pool {
     lock_user_config($delVMfromPoolFn, "pool cleanup for VM $vmid failed");
 }
 
+sub remove_sdn_resource_access {
+    my ($paths) = @_; # [ 'zones/<zone>', 'zones/<zone>/<vnet>' ]
+
+    my $delete_resource_fn = sub {
+        my $usercfg = cfs_read_file("user.cfg");
+        my $modified = 0;
+
+        for my $path (@$paths) {
+            my $full_path = "sdn/$path";
+
+            my $current = $usercfg->{acl_root};
+            my ($parent, $last_key) = find_acl_tree_node_parent($current, $full_path);
+
+            if ($parent && $last_key) {
+                delete $parent->{children}->{$last_key};
+                $modified = 1;
+            }
+        }
+
+        if ($modified) {
+            cfs_write_file("user.cfg", $usercfg);
+        }
+    };
+
+    lock_user_config($delete_resource_fn, "SDN ACL cleanup failed");
+}
+
 my $USER_CONTROLLED_TFA_TYPES = {
     u2f => 1,
     oath => 1,
-- 
2.47.3

[PATCH pve-access-control v5 3/7] test: add sdn acl pruning tests

[David Riley]

Add unit tests to validate the behavior of the SDN resource ACL
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.

Signed-off-by: David Riley <d.riley@proxmox.com>
---
 src/test/Makefile                |   1 +
 src/test/sdn_acl_pruning_test.pl | 134 +++++++++++++++++++++++++++++++
 2 files changed, 135 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..f407c89
--- /dev/null
+++ b/src/test/sdn_acl_pruning_test.pl
@@ -0,0 +1,134 @@
+#!/usr/bin/env perl
+
+use v5.36;
+use strict;
+use warnings;
+
+use lib qw(..);
+
+use Test::MockModule;
+use Test::More;
+
+use PVE::AccessControl;
+use PVE::RPCEnvironment;
+use PVE::Tools;
+
+# test cases
+my $tests = [
+    {
+        description => 'successful pruning of sdn resource acls',
+        raw => <<~EOF,
+        user:user1\@pve:1:0::::::
+        user:user2\@pve:1:0::::::
+        user:user3\@pve:1:0::::::
+        role:SDNAdmin:SDN.Allocate,SDN.Audit
+        role:SDNAudit:SDN.Audit
+        acl:1:/sdn/zones/zone1:user3\@pve:SDNAdmin:
+        acl:1:/sdn/zones/zone2:user3\@pve:SDNAdmin:
+        acl:1:/sdn/zones/zone1/vnet1:user1\@pve:SDNAdmin:
+        acl:1:/sdn/zones/zone1/vnet1/10:user1\@pve:SDNAdmin:
+        acl:1:/sdn/zones/zone1/vnet2:user1\@pve:SDNAdmin:
+        acl:1:/sdn/zones/zone1/vnet2/100:user1\@pve:SDNAudit:
+        acl:1:/sdn/controllers/control:user2\@pve:SDNAdmin:
+        acl:1:/sdn/prefix-lists/list:user2\@pve:SDNAdmin:
+        acl:1:/sdn/route-maps/map:user2\@pve:SDNAdmin:
+        acl:1:/sdn/fabrics/fabric:user2\@pve:SDNAdmin:
+        EOF
+        prune_paths => [
+            'zones/zone2',
+            'zones/zone1/vnet1',
+            'zones/zone1/vnet2/100',
+            'controllers/control',
+            'prefix-lists/list',
+            'route-maps/map',
+            'fabrics/fabric',
+        ],
+        expected_roles => [
+            # zone 1 remains, zone 2 is pruned
+            ['user3@pve', '/sdn/zones/zone1', 'SDNAdmin'],
+            ['user3@pve', '/sdn/zones/zone2', ''],
+
+            # vnet1 is completely pruned
+            ['user1@pve', '/sdn/zones/zone1/vnet1', ''],
+            ['user1@pve', '/sdn/zones/zone1/vnet1/10', ''],
+
+            # vnet2 remains, but the specific vnet2/100 override (SDNAudit) is removed,
+            # so it falls back to inheriting SDNAdmin from vnet2
+            ['user1@pve', '/sdn/zones/zone1/vnet2', 'SDNAdmin'],
+            ['user1@pve', '/sdn/zones/zone1/vnet2/100', 'SDNAdmin'],
+
+            # other SDN resources pruned
+            ['user2@pve', '/sdn/controllers/control', ''],
+            ['user2@pve', '/sdn/prefix-lists/list', ''],
+            ['user2@pve', '/sdn/route-maps/map', ''],
+            ['user2@pve', '/sdn/fabrics/fabric', ''],
+        ],
+    },
+];
+
+# 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";
+    },
+    'cfs_write_file' => sub {
+        my ($filename, $cfg) = @_;
+        if ($filename eq 'user.cfg') {
+            $mocked_user_cfg_tree = $cfg;
+        }
+    },
+    'lock_user_config' => sub {
+        my ($code, $errmsg) = @_;
+        eval { $code->() };
+        if (my $err = $@) {
+            die "$errmsg: $err\n";
+        }
+    },
+);
+
+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'");
+}
+
+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();
-- 
2.47.3

[PATCH pve-access-control v5 4/7] permission: add helper to check for active permissions

[David Riley]

Add recursive helper to determine if a specific node or any of its
descendants in the ACL tree contain configured permissions.

This is in preparation for subsequent SDN VNet ACL migration changes.

Signed-off-by: David Riley <d.riley@proxmox.com>
---
 src/PVE/AccessControl.pm | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
index 8e2b4f2..1047226 100644
--- a/src/PVE/AccessControl.pm
+++ b/src/PVE/AccessControl.pm
@@ -977,6 +977,25 @@ sub iterate_acl_tree {
     }
 }
 
+# recursively checks a node and all its children for active permissions
+sub acl_tree_has_permissions {
+    my ($node) = @_;
+
+    for my $key (keys %$node) {
+        if ($key ne 'children') {
+            return 1;
+        }
+    }
+
+    for my $child_key (keys $node->{children}->%*) {
+        if (acl_tree_has_permissions($node->{children}->{$child_key})) {
+            return 1;
+        }
+    }
+
+    return 0;
+}
+
 # find ACL node corresponding to normalized $path under $root
 sub find_acl_tree_node {
     my ($root, $path) = @_;
-- 
2.47.3

[PATCH pve-access-control v5 5/7] fix #7520: sdn: add VNet ACL migration

[David Riley]

Add routine to enable the relocation of ACL entries when their
corresponding SDN resources are moved. This prevents stale entries
when VNets are moved between zones.

This is a preparatory change that does not have an immediate effect on
its own, but lays the foundation for safely relocating ACL nodes and
rejecting conflicting destination paths.

Link: https://bugzilla.proxmox.com/show_bug.cgi?id=7520
Signed-off-by: David Riley <d.riley@proxmox.com>
---
 src/PVE/AccessControl.pm | 84 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
index 1047226..4faeb8f 100644
--- a/src/PVE/AccessControl.pm
+++ b/src/PVE/AccessControl.pm
@@ -2045,6 +2045,90 @@ sub remove_sdn_resource_access {
     lock_user_config($delete_resource_fn, "SDN ACL cleanup failed");
 }
 
+sub migrate_sdn_resource_access {
+    my ($migrations) = @_;
+    # [ { src_path => 'zones/<zoney>/<vnet>', dest_path => 'zones/<zonex>/<vnet>' } ]
+
+    return if !$migrations;
+
+    my $migrate_fn = sub {
+        my $usercfg = cfs_read_file("user.cfg");
+        my $modified = 0;
+
+        my @prepared_moves = ();
+        for my $migration (@$migrations) {
+
+            my $invalid_key = 0;
+
+            for my $key (qw(src_path dest_path)) {
+                if (!defined($migration->{$key})) {
+                    warn "Invalid '$key': must be an array reference.\n";
+                    $invalid_key = 1;
+                    last;
+                }
+            }
+
+            next if $invalid_key;
+
+            my $src_path = "sdn/$migration->{src_path}";
+            my $dest_path = "sdn/$migration->{dest_path}";
+
+            my ($src_parent, $src_last_key, $acl_node) =
+                find_acl_tree_node_parent($usercfg->{acl_root}, $src_path);
+
+            if (defined($src_parent) && defined($src_last_key) && defined($acl_node)) {
+                # probe dest for conflicts
+                my (undef, undef, $existing_dest_node) =
+                    find_acl_tree_node_parent($usercfg->{acl_root}, $dest_path);
+
+                if (acl_tree_has_permissions($existing_dest_node)) {
+                    my $conflict_path = "/$dest_path";
+                    my $source_path = "/$src_path";
+                    die
+                        "Destination '$conflict_path' already has permissions configured (Source:"
+                        . " '$source_path'). Please remove the target ACLs manually before"
+                        . " retrying.\n";
+                }
+
+                # stage move
+                push(
+                    @prepared_moves,
+                    {
+                        src_parent => $src_parent,
+                        src_last_key => $src_last_key,
+                        dest_path => $dest_path,
+                        node => $acl_node,
+                    },
+                );
+            }
+        }
+
+        for my $move (@prepared_moves) {
+            delete $move->{src_parent}->{children}->{ $move->{src_last_key} };
+
+            my $current = $usercfg->{acl_root};
+            my @dest_segments = split('/', $move->{dest_path});
+            my $dest_last_key = pop @dest_segments;
+
+            for my $segment (@dest_segments) {
+                $current->{children}->{$segment} = {}
+                    if !defined($current->{children}->{$segment});
+
+                $current = $current->{children}->{$segment};
+            }
+            $current->{children}->{$dest_last_key} = $move->{node};
+
+            $modified = 1;
+        }
+
+        if ($modified) {
+            cfs_write_file("user.cfg", $usercfg);
+        }
+    };
+
+    lock_user_config($migrate_fn, "ACL migration failed");
+}
+
 my $USER_CONTROLLED_TFA_TYPES = {
     u2f => 1,
     oath => 1,
-- 
2.47.3

[PATCH pve-access-control v5 6/7] test: add sdn acl migration tests

[David Riley]

Add unit tests to validate the behavior of the SDN VNet ACL migration
logic.

The tests cover multiple migration edge cases:
* VNet migration between zones
* Subtree retention, moving multiple VLAN tags under a VNet
* Unconfigured resources, ensuring empty source paths do not create
  empty target nodes
* Conflict prevention, aborting on destination collisions

Signed-off-by: David Riley <d.riley@proxmox.com>
---
 src/test/Makefile                  |   2 +
 src/test/sdn_acl_migration_test.pl | 171 +++++++++++++++++++++++++++++
 2 files changed, 173 insertions(+)
 create mode 100644 src/test/sdn_acl_migration_test.pl

diff --git a/src/test/Makefile b/src/test/Makefile
index 2f44186..7d4267f 100644
--- a/src/test/Makefile
+++ b/src/test/Makefile
@@ -15,3 +15,5 @@ check:
 	perl -I.. realm_sync_test.pl
 	perl -I.. api-tests.pl
 	perl -I.. sdn_acl_pruning_test.pl
+	perl -I.. sdn_acl_migration_test.pl
+
diff --git a/src/test/sdn_acl_migration_test.pl b/src/test/sdn_acl_migration_test.pl
new file mode 100644
index 0000000..a9323a4
--- /dev/null
+++ b/src/test/sdn_acl_migration_test.pl
@@ -0,0 +1,171 @@
+#!/usr/bin/env perl
+
+use v5.36;
+use strict;
+use warnings;
+
+use lib qw(..);
+
+use Test::MockModule;
+use Test::More;
+
+use PVE::AccessControl;
+use PVE::RPCEnvironment;
+use PVE::Tools;
+
+# test cases
+my $tests = [
+    {
+        description => 'successful migration of vnets and child vlans',
+        raw => <<~EOF,
+        user:user1\@pve:1:0::::::
+        role:SDNAdmin:SDN.Allocate,SDN.Audit
+        acl:1:/sdn/zones/zone1/vnet1:user1\@pve:SDNAdmin:
+        acl:1:/sdn/zones/zone1/vnet2:user1\@pve:SDNAdmin:
+        acl:1:/sdn/zones/zone1/vnet2/100:user1\@pve:SDNAdmin:
+        acl:1:/sdn/zones/zone1/vnet2/200:user1\@pve:SDNAdmin:
+        EOF
+        migrations => [
+            {
+                src_path => 'zones/zone1/vnet1',
+                dest_path => 'zones/zone2/vnet1',
+            },
+            {
+                src_path => 'zones/zone1/vnet2',
+                dest_path => 'zones/zone2/vnet2',
+            },
+            {
+                src_path => 'zones/zone1/vnet4',
+                dest_path => 'zones/zone2/vnet4',
+            }, # fictional move
+        ],
+        expected_roles => [
+            # src paths, should be empty after the migration
+            ['user1@pve', '/sdn/zones/zone1/vnet1', ''],
+            ['user1@pve', '/sdn/zones/zone1/vnet2', ''],
+            ['user1@pve', '/sdn/zones/zone1/vnet2/100', ''],
+            ['user1@pve', '/sdn/zones/zone1/vnet2/200', ''],
+
+            # dest paths, should have the migrated permissions
+            ['user1@pve', '/sdn/zones/zone2/vnet1', 'SDNAdmin'],
+            ['user1@pve', '/sdn/zones/zone2/vnet2', 'SDNAdmin'],
+            ['user1@pve', '/sdn/zones/zone2/vnet2/100', 'SDNAdmin'],
+            ['user1@pve', '/sdn/zones/zone2/vnet2/200', 'SDNAdmin'],
+
+            # fictional move, should be empty
+            ['user1@pve', '/sdn/zones/zone1/vnet4', ''],
+            ['user1@pve', '/sdn/zones/zone2/vnet4', ''],
+        ],
+    },
+    {
+        description => 'conflict without vlan tag',
+        raw => <<~EOF,
+        user:user1\@pve:1:0::::::
+        user:user2\@pve:1:0::::::
+        role:SDNAdmin:SDN.Allocate,SDN.Audit
+        acl:1:/sdn/zones/zone1/vnet3:user1\@pve:SDNAdmin:
+        acl:1:/sdn/zones/zone2/vnet3:user2\@pve:SDNAdmin:
+        EOF
+        migrations => [
+            {
+                src_path => 'zones/zone1/vnet3',
+                dest_path => 'zones/zone2/vnet3',
+            },
+        ],
+        expected_error => qr/already has permissions configured/,
+        error_desc => 'Migration must abort if direct target destination node already exists',
+    },
+    {
+        description => 'conflict with vlan tag',
+        raw => <<~EOF,
+        user:user1\@pve:1:0::::::
+        user:user2\@pve:1:0::::::
+        role:SDNAdmin:SDN.Allocate,SDN.Audit
+        acl:1:/sdn/zones/zone1/vnet5:user1\@pve:SDNAdmin:
+        acl:1:/sdn/zones/zone2/vnet5/100:user2\@pve:SDNAdmin:
+        EOF
+        migrations => [
+            {
+                src_path => 'zones/zone1/vnet5',
+                dest_path => 'zones/zone2/vnet5',
+            },
+        ],
+        expected_error => qr/already has permissions configured/,
+        error_desc =>
+            'Migration must abort if a child of the target destination node already exists',
+    },
+];
+
+# 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";
+    },
+    'cfs_write_file' => sub {
+        my ($filename, $cfg) = @_;
+        if ($filename eq 'user.cfg') {
+            $mocked_user_cfg_tree = $cfg;
+        }
+    },
+    'lock_user_config' => sub {
+        my ($code, $errmsg) = @_;
+        eval { $code->() };
+        if (my $err = $@) {
+            die "$errmsg: $err\n";
+        }
+    },
+);
+
+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 test_migration {
+    my ($case) = @_;
+
+    $mocked_user_cfg_tree = PVE::AccessControl::parse_user_config("user.cfg", $case->{raw});
+    $rpcenv->{user_cfg} = $mocked_user_cfg_tree;
+
+    eval { PVE::AccessControl::migrate_sdn_resource_access($case->{migrations}); };
+    my $err = $@;
+
+    if ($case->{expected_error}) {
+        ok($err, $case->{error_desc});
+        like($err, $case->{expected_error}, "Error message matches expected conflict string");
+    } else {
+        is($err, '', "Migration completed without errors");
+
+        $rpcenv->{user_cfg} = $mocked_user_cfg_tree;
+
+        for my $role_check ($case->{expected_roles}->@*) {
+            check_roles(@$role_check);
+        }
+    }
+}
+
+for my $case ($tests->@*) {
+    subtest $case->{description} => sub {
+        test_migration($case);
+    };
+}
+
+done_testing();
+
-- 
2.47.3

[PATCH pve-network v5 7/7] fix #7520: config: prune orphaned and relocate moved VNet ACLs

[David Riley]

Compare the running configuration with the newly compiled state during
config commit to track configuration changes across:
* Zones
* VNets
* Fabrics
* Controllers
* Route maps
* Prefix lists

Based on this comparison, identify and prune orphaned SDN ACL entries
to ensure state consistency for manual applies via the UI/API as well
as during automatic configuration reloads on system boot.

Additionally, reject conflicting destination paths when a VNet is
moved between zones to prevent configuration overwrites. If the target
path is clear, relocate the corresponding ACLs to the updated zone.

Link: https://bugzilla.proxmox.com/show_bug.cgi?id=7520

Suggested-by: Stefan Hanreich <s.hanreich@proxmox.com>
Signed-off-by: David Riley <d.riley@proxmox.com>
---
 src/PVE/Network/SDN.pm | 115 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 115 insertions(+)

diff --git a/src/PVE/Network/SDN.pm b/src/PVE/Network/SDN.pm
index 33a3cf3..5da425e 100644
--- a/src/PVE/Network/SDN.pm
+++ b/src/PVE/Network/SDN.pm
@@ -10,6 +10,7 @@ use LWP::UserAgent;
 use Net::SSLeay;
 use UUID;
 
+use PVE::AccessControl;
 use PVE::Cluster qw(cfs_read_file cfs_write_file cfs_lock_file);
 use PVE::INotify;
 use PVE::RESTEnvironment qw(log_warn);
@@ -238,11 +239,125 @@ sub compile_running_cfg {
 }
 
 sub commit_config {
+    my $old_cfg = cfs_read_file($RUNNING_CFG_FILENAME);
     my $cfg = compile_running_cfg();
 
+    cleanup_access_control($old_cfg, $cfg);
+
     cfs_write_file($RUNNING_CFG_FILENAME, $cfg);
 }
 
+sub cleanup_access_control {
+    my ($old_cfg, $cfg) = @_;
+
+    my $route_map_paths = diff_route_maps($old_cfg, $cfg);
+    my $generic_paths = diff_generic_resources($old_cfg, $cfg);
+    my ($vnet_delete_paths, $vnet_move_paths) = diff_vnets($old_cfg, $cfg);
+
+    if (@$vnet_move_paths) {
+        PVE::AccessControl::migrate_sdn_resource_access($vnet_move_paths);
+    }
+
+    my @paths_to_delete = (@$vnet_delete_paths, @$route_map_paths, @$generic_paths);
+    if (@paths_to_delete) {
+        PVE::AccessControl::remove_sdn_resource_access(\@paths_to_delete);
+    }
+}
+
+sub diff_generic_resources {
+    my ($old_cfg, $cfg) = @_;
+
+    my @paths_to_delete = ();
+
+    my @types = qw(zones controllers fabrics prefix-lists);
+    for my $type (@types) {
+        if (!defined($old_cfg->{$type}) || !defined($old_cfg->{$type}->{ids})) {
+            next;
+        }
+
+        my $old_ids = $old_cfg->{$type}->{ids};
+        my $new_ids = $cfg->{$type}->{ids};
+
+        for my $id (keys %$old_ids) {
+            next if defined($new_ids->{$id});
+
+            push(@paths_to_delete, "$type/$id");
+        }
+    }
+
+    return \@paths_to_delete;
+}
+
+sub diff_route_maps {
+    my ($old_cfg, $cfg) = @_;
+
+    my @paths_to_delete = ();
+
+    if (defined($old_cfg->{'route-maps'}) && defined($old_cfg->{'route-maps'}->{ids})) {
+        my $old_route_maps = $old_cfg->{'route-maps'}->{ids};
+        my $route_map_suffix_regex = qr/_[0-9]+$/;
+
+        my %active_route_maps = ();
+        if (defined($cfg->{'route-maps'}->{ids})) {
+            for my $id (keys $cfg->{'route-maps'}->{ids}->%*) {
+                (my $base_name = $id) =~ s/$route_map_suffix_regex//;
+                $active_route_maps{$base_name} = 1;
+            }
+        }
+
+        my %queued_route_maps = ();
+        for my $id (keys %$old_route_maps) {
+            next if defined($cfg->{'route-maps'}->{ids}->{$id});
+
+            (my $base_name = $id) =~ s/$route_map_suffix_regex//;
+
+            next if ($active_route_maps{$base_name});
+
+            next if ($queued_route_maps{$base_name});
+
+            push(@paths_to_delete, "route-maps/$base_name");
+            $queued_route_maps{$base_name} = 1;
+        }
+    }
+
+    return \@paths_to_delete;
+}
+
+sub diff_vnets {
+    my ($old_cfg, $cfg) = @_;
+
+    my @paths_to_delete = ();
+    my @paths_to_move = ();
+
+    if (defined($old_cfg->{vnets}->{ids})) {
+        my $old_vnets = $old_cfg->{vnets}->{ids};
+        my $new_vnets = $cfg->{vnets}->{ids};
+
+        for my $vnetid (keys %$old_vnets) {
+            my $old_zone = $old_vnets->{$vnetid}->{zone};
+
+            if (!defined($new_vnets->{$vnetid})) {
+                push(@paths_to_delete, "zones/$old_zone/$vnetid");
+                next;
+            }
+
+            my $new_zone = $new_vnets->{$vnetid}->{zone};
+
+            if ($old_zone ne $new_zone) {
+                push(
+                    @paths_to_move,
+                    {
+                        src_path => "zones/$old_zone/$vnetid",
+                        dest_path => "zones/$new_zone/$vnetid",
+                    },
+                );
+            }
+        }
+    }
+
+    return (\@paths_to_delete, \@paths_to_move);
+}
+
 sub has_pending_changes {
     my $running_cfg = PVE::Network::SDN::running_config();
 
-- 
2.47.3