Re: [PATCH pve-firewall 3/5] aliases: Add option to update references on rename/delete

[Arthur Bied-Charreton <a.bied-charreton@proxmox.com>]

On Thu, Jul 16, 2026 at 03:17:32PM +0200, Stefan Hanreich wrote:
> 
> 
> On 4/7/26 9:36 AM, Arthur Bied-Charreton wrote:
> > Renaming or deleting an alias that is referenced by rules or security
> > groups would leave dangling references, creating broken configurations.
> > 
> > Add option to the POST and DELETE endpoints for /firewall/aliases to
> > update or delete those references from cluster, hosts, and guests
> > firewall configs.
> > 
> > If these endpoints are hit from the cluster environment (/cluster/),
> > all firewall config files in the cluster (cluster + all nodes + all
> > guests) have to be checked and possibly updated.
> > 
> > Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> > ---
> >  src/PVE/API2/Firewall/Aliases.pm | 115 +++++++++++++++++++++++++++++++
> >  1 file changed, 115 insertions(+)
> > 
> > diff --git a/src/PVE/API2/Firewall/Aliases.pm b/src/PVE/API2/Firewall/Aliases.pm
> > index eaafe68..2a06822 100644
> > --- a/src/PVE/API2/Firewall/Aliases.pm
> > +++ b/src/PVE/API2/Firewall/Aliases.pm
> > @@ -5,6 +5,7 @@ use warnings;
> >  use PVE::Exception qw(raise raise_param_exc);
> >  use PVE::JSONSchema qw(get_standard_option);
> >  
> > +use PVE::API2::Firewall::Helpers qw(filter_map foreach_conf_in_env);
> >  use PVE::Firewall;
> >  
> >  use base qw(PVE::RESTHandler);
> > @@ -217,6 +218,12 @@ sub register_update_alias {
> >      $properties->{cidr} = $api_properties->{cidr};
> >      $properties->{comment} = $api_properties->{comment};
> >      $properties->{digest} = get_standard_option('pve-config-digest');
> > +    $properties->{'update-references'} = {
> > +        type => 'boolean',
> > +        optional => 1,
> > +        description => 'Update dangling references when renaming the alias.',
> > +        default => 0,
> > +    };
> >  
> >      $class->register_method({
> >          name => 'update_alias',
> > @@ -261,6 +268,9 @@ sub register_update_alias {
> >                      if ($rename && ($name ne $rename)) {
> >                          raise_param_exc({ name => "alias '$param->{rename}' already exists" })
> >                              if defined($aliases->{$rename});
> > +                        rename_alias_refs(
> > +                            $fw_conf, $name, $param->{rename}, $class->rule_env(),
> > +                        ) if $param->{'update-references'};
> 
> This suffers from the same timing issues as the IPSet renaming.
> 
ack, will fix in v2, thanks!
> >                          $aliases->{$name}->{name} = $param->{rename};
> >                          $aliases->{$rename} = $aliases->{$name};
> >                          delete $aliases->{$name};
> > @@ -282,6 +292,12 @@ sub register_delete_alias {
[...]
> > +
> > +sub rename_alias_refs {
> > +    my ($conf, $alias, $new_name, $rule_env) = @_;
> > +    my $lc_alias = lc($alias);
> > +    return rewrite_alias_refs_in_env(
> > +        $conf,
> > +        $lc_alias,
> > +        $rule_env,
> > +        sub {
> > +            my ($obj) = @_;
> > +            for my $field (qw(source dest cidr)) {
> > +                my $val = lc($obj->{$field} // '');
> > +                if ($val eq "dc/$lc_alias") { $obj->{$field} = "dc/$new_name" }
> > +                elsif ($val eq $lc_alias) { $obj->{$field} = $new_name }
> > +                elsif ($val eq "guest/$lc_alias") { $obj->{$field} = "guest/$new_name" }
> > +            }
> > +            return $obj;
> > +        },
> > +    );
> > +}
> > +
> 
> same point w.r.t moving them to the Helper module. Also, this seems
> quite similar to the respective IPSet function (minus the name parsing).
> Would it be possible to further simplify this and re-use the functions
> across the ipset / alias functions?
> 
will move the functions to the helper module. regarding the similarity,
i did have a more generic approach in the beginning, which i ended up
dropping because it felt too complicated. i will revisit this for v2,
i'm sure there is a good way to generalize this. 
> 
> 
> 
>