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

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

On Thu, Jul 16, 2026 at 03:17:25PM +0200, Stefan Hanreich wrote:
> On 4/7/26 9:36 AM, Arthur Bied-Charreton wrote:
> > Renaming or deleting an IPSet 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/ipset 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.
> > 
> > Renaming a cluster ipset referenced by 10.000 different config files in
> > a 3-node test cluster takes about 4.8 seconds on my machine. Doing the
> > same with an unreferenced ipset (i.e. config files checked but not
> > written back due to lack of changes) takes about 2 seconds.
> > 
> > Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> > ---
[...]
> > +
> > +sub rename_ipset_refs {
> > +    my ($conf, $ipset, $new_name, $rule_env) = @_;
> > +    my $lc_ipset = lc($ipset);
> > +    return rewrite_ipset_refs_in_env(
> > +        $conf,
> > +        $lc_ipset,
> > +        $rule_env,
> > +        sub {
> > +            my ($rule) = @_;
> > +            for my $field (qw(source dest)) {
> > +                my $val = lc($rule->{$field} // '');
> > +                if ($val eq "+dc/$lc_ipset") { $rule->{$field} = "+dc/$new_name" }
> > +                elsif ($val eq "+$lc_ipset") { $rule->{$field} = "+$new_name" }
> > +                elsif ($val eq "+guest/$lc_ipset") { $rule->{$field} = "+guest/$new_name" }
> 
> In the Firewall parsing logic itself (verify_rule in Firewall.pm), we do
> use regex matching:
> 
>   $value =~ m@^\+(guest/|dc/|sdn/)?(${ipset_name_pattern})$@
> 
> We could utilize that to rewrite this as:
> 
> if ($value =~ m@^\+(guest/|dc/)?(${ipset_name_pattern})$@) {
>   $rule->{$field} = "$1/$new_name" if $2 eq $lc_ipset;
> }
> 
> but that might be a bit overkill / overcomplicated.
> 
> I just figured I'd mention it since i've often written similar if /
> elsif chains myself that might have been better expressed by utilizing
> perl regexes.
> 
thanks for mentioning! although i have to say i find the regex approach
a lot harder to reason about here, i think i would stick with the
if/elsif chain if that's okay with you. 
> > +            }
> > +            return $rule;
> > +        },
> > +    );
> > +}
> > +
> 
> All the helpers contained here, might better fit in the general Helper
> module, rather than the API module itself. Particularly if we implement
> similar functionality when deleting SDN VNets, pve-network would be able
> to use the Helper module instead of having to import API modules.
> 
good point, will move those over
> >  sub register_delete_ipset {
> >      my ($class) = @_;
> >  
> > @@ -139,6 +226,12 @@ sub register_delete_ipset {
> >          optional => 1,
> >          description => 'Delete all members of the IPSet, if there are any.',
> >      };
> > +    $properties->{'delete-references'} = {
> > +        type => 'boolean',
> > +        optional => 1,
> > +        description => 'Delete dangling references after deleting the IPSet',
> > +        default => 0,
> > +    };
> >  
> >      $class->register_method({
> >          name => 'delete_ipset',
> > @@ -165,8 +258,10 @@ sub register_delete_ipset {
> >                      die "IPSet '$param->{name}' is not empty\n"
> >                          if scalar(@$ipset) && !$param->{force};
> >  
> > -                    $class->save_ipset($param, $fw_conf, undef);
> > +                    delete_ipset_refs($fw_conf, $param->{name}, $class->rule_env())
> > +                        if $param->{'delete-references'};
> >  
> > +                    $class->save_ipset($param, $fw_conf, undef);
> >                  },
> >              );
> >  
> > @@ -654,6 +749,13 @@ sub register_create {
> >          },
> >      );
> >  
> > +    $properties->{'update-references'} = {
> > +        type => 'boolean',
> > +        optional => 1,
> > +        description => 'Update dangling references when renaming an IPSet.',
> > +        default => 0,
> > +    };
> > +
> >      $class->register_method({
> >          name => 'create_ipset',
> >          path => '',
> > @@ -688,6 +790,10 @@ sub register_create {
> >                              if $fw_conf->{ipset}->{ $param->{name} }
> >                              && $param->{name} ne $param->{rename};
> >  
> > +                        PVE::API2::Firewall::IPSetBase::rename_ipset_refs(
> > +                            $fw_conf, $param->{rename}, $param->{name}, $class->rule_env(),
> > +                        ) if $param->{'update-references'};
> 
> This suffers from timing issues, doesn't it? We update the references in
> all configuration files, but only save the new IPSet *after* we have
> updated all references. This leaves a window where rules get skipped by
> the firewall since they reference a not-yet-existing IPSet.
> 
> In order to get atomicity we'd have to do it in two steps, wouldn't we?
> * Save the new ipset and write it to the configuration, but still keep
> the old one around.
> * update the references one-by-one, now at every point in time a rule
> references an IPSet that actually exists in the configuration
> * Delete the old IPSet
> 
> Otherwise, introducing a sleep inbetween the rename call and the
> save_config call seems to make this surface easily:
> 
> * Create an IPSet at cluster level
> * Reference the IPSet in a host rule
> * Update the name of the IPSet + update-references
> 
> Some firewall cycles fail due to the rules getting updated, but not the
> IPSet name itself:
> 
that's a very good point, thanks for catching that! i will go with your
proposed alternative (create new & keep old -> update references ->
delete old) in v2.  
> > Jul 16 14:48:28 fw-testi pve-firewall[1213]: /etc/pve/nodes/fw-testi/host.fw (line 7) - errors in rule parameters: IN ACCEPT -source +dc/owo -log nolog
> > Jul 16 14:48:28 fw-testi pve-firewall[1213]:   source: no such ipset 'owo'
> 
> Delete seems fine, since all references are deleted and only then the
> IPSet itself afterwards.
> 
> 
> 
> 
>