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

[Stefan Hanreich <s.hanreich@proxmox.com>]

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>
> ---
>  src/PVE/API2/Firewall/IPSet.pm | 108 ++++++++++++++++++++++++++++++++-
>  1 file changed, 107 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/API2/Firewall/IPSet.pm b/src/PVE/API2/Firewall/IPSet.pm
> index 43a51a7..9896fdd 100644
> --- a/src/PVE/API2/Firewall/IPSet.pm
> +++ b/src/PVE/API2/Firewall/IPSet.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);
> @@ -128,6 +129,92 @@ sub register_get_ipset {
>      });
>  }
>  
> +# Apply $rewrite to each rule in $conf referencing $ipset, modifying $conf in place.
> +#
> +# $rewrite ->($rule) is expected to return the new rule, or undef if the goal is to remove matches.
> +#
> +# Returns a boolean indicating whether $conf was modified.
> +#
> +# If $is_guest is set to 1, references will be solved assuming $conf is a guest FW config.
> +# This is important because 2 different IPSets with the same name may be defined at the
> +# cluster level and at the guest level, in which case the guest's IPSet shadows the cluster's.
> +sub rewrite_ipset_refs_in_conf {
> +    my ($conf, $ipset, $rule_env, $is_guest, $rewrite) = @_;
> +    my $modified = 0;
> +
> +    my @patterns;
> +    if ($rule_env eq 'cluster') {
> +        @patterns = ("+dc/$ipset");
> +        # Guest config ipsets may shadow cluster config ones
> +        push @patterns, "+$ipset" if !($is_guest && $conf->{ipset}->{$ipset});
> +    } else {
> +        @patterns = ("+$ipset", "+guest/$ipset");
> +    }
> +
> +    my $rule_matches = sub {
> +        my ($rule) = @_;
> +        grep { ($rule->{source} // '') eq $_ || ($rule->{dest} // '') eq $_ } @patterns;
> +    };
> +
> +    ($conf->{rules}, my $changed) = filter_map($conf->{rules}, $rewrite, $rule_matches);
> +    $modified ||= $changed;
> +
> +    if ($rule_env eq 'cluster') {
> +        my $groups = $conf->{groups} // {};
> +        for my $group (keys %$groups) {
> +            ($groups->{$group}, $changed) = filter_map($groups->{$group}, $rewrite, $rule_matches);
> +            $modified ||= $changed;
> +        }
> +    }
> +
> +    return $modified;
> +}
> +
> +# Apply $rewrite to each rule in the current environment ($rule_env) referencing $ipset, modifying
> +# $conf in place. The caller is responsible for locking and saving $conf.
> +#
> +# If $rule_env is 'cluster', this function will also map over all guest and node firewall configs
> +# in the cluster, locking and saving them in the process, since those may reference IPSets defined
> +# in the cluster config.
> +sub rewrite_ipset_refs_in_env {
> +    my ($conf, $ipset, $rule_env, $rewrite) = @_;
> +    return foreach_conf_in_env(
> +        $conf,
> +        $rule_env,
> +        sub {
> +            my ($fw_conf, $effective_env, $is_guest) = @_;
> +            return rewrite_ipset_refs_in_conf(
> +                $fw_conf, $ipset, $effective_env, $is_guest, $rewrite,
> +            );
> +        },
> +    );
> +}
> +
> +sub delete_ipset_refs {
> +    my ($conf, $ipset, $rule_env) = @_;
> +    return rewrite_ipset_refs_in_env($conf, lc($ipset), $rule_env, sub { undef });
> +}
> +
> +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.

> +            }
> +            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.

>  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:

> 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.