Re: [PATCH pve-firewall 1/5] Add helpers for updating alias and ipset references

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

On Thu, Jul 16, 2026 at 03:17:09PM +0200, Stefan Hanreich wrote:
> 
> 
> On 4/7/26 9:36 AM, Arthur Bied-Charreton wrote:
> > Both ipsets and aliases require similar logic, where we need to
> > be able to iterate over all firewall configs in the cluster (cluster,
> > nodes and guests) to find and update all references to a given object.
> > 
> > Add filter_map() and foreach_conf_in_env() as shared helpers that take
> > closures, allowing the ipset and alias handlers to reuse some of the
> > traversal logic.
> > 
> > Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> > ---
> >  src/PVE/API2/Firewall/Helpers.pm | 66 ++++++++++++++++++++++++++++++++
> >  1 file changed, 66 insertions(+)
> > 
> > diff --git a/src/PVE/API2/Firewall/Helpers.pm b/src/PVE/API2/Firewall/Helpers.pm
> > index 0fb71f7..8a8759a 100644
> > --- a/src/PVE/API2/Firewall/Helpers.pm
> > +++ b/src/PVE/API2/Firewall/Helpers.pm
> > @@ -4,8 +4,74 @@ use strict;
> >  use warnings;
> >  
> >  use PVE::Cluster;
> > +use PVE::Firewall;
> >  use PVE::Network::SDN::Vnets;
> >  use PVE::RPCEnvironment;
> > +use base 'Exporter';
> > +our @EXPORT_OK = qw(filter_map foreach_conf_in_env);
> > +
> > +# Apply $action to each item in $items for which $matches->($item) is true. Remove item
> > +# if $matches->($item) is true and $action->($item) returns undef.
> > +#
> > +# Returns the updated items arrayref and a boolean indicating whether any item was matched.
> 
> I've started to introduce the POD format for comments (e.g. in
> src/PVE/Firewall/Helpers.pm) for new functions in the Firewall, so we
> could utilize that here as well?
> 
will do!
> > +sub filter_map {
> 
> The name is imo a bit misleading, since the result still includes
> everything that *doesn't* match, so there's not really any filtering
> included. I, personally, would think from the name of this function that
> it filters everything according to matches, and then maps it via
> $action. I'd think of this more like a simple map operation rather, that
> returns the item as-is if it doesn't fit a given criteria, otherwise
> transforms it.
yea, this name is a remnant of a previous implementation that i should
have revisited before submitting, sorry about that. will find a better
name in v2. 
> 
> > +    my ($items, $action, $matches) = @_;
> > +    my @result;
> > +    my $modified = 0;
> > +    for my $item (@{ $items // [] }) {
> > +        if ($matches->($item)) {
> > +            $modified = 1;
> > +            my $new = $action->($item);
> > +            push @result, $new if defined $new;
> > +        } else {
> > +            push @result, $item;
> > +        }
> > +    }
> > +    return (\@result, $modified);
> > +}
> > +
> > +# Apply $rewrite to the main firewall config and, if $rule_env is 'cluster', to all guest
> > +# and host firewall configs across the cluster. Configs where $rewrite returns true are saved.
> > +# The caller is responsible for locking and saving the cluster config. Guest and host
> > +# configs are locked by this function.
> > +sub foreach_conf_in_env {
> > +    my ($conf, $rule_env, $rewrite) = @_;
> > +
> > +    $rewrite->($conf, $rule_env, 0);
> > +
> > +    return if $rule_env ne 'cluster';
> > +
> > +    my $vmlist = PVE::Cluster::get_vmlist();
> > +    for my $vmid (keys %{ ($vmlist // {})->{ids} // {} }) {
> > +        PVE::Firewall::lock_vmfw_conf(
> > +            $vmid,
> > +            10,
> > +            sub {
> > +                my $type = $vmlist->{ids}->{$vmid}->{type};
> > +                my $env = $type eq 'lxc' ? 'ct' : 'vm';
> > +                my $guest_conf = PVE::Firewall::load_vmfw_conf($conf, $env, $vmid);
> > +                if ($rewrite->($guest_conf, 'cluster', 1)) {
> > +                    PVE::Firewall::save_vmfw_conf($vmid, $guest_conf);
> > +                }
> > +            },
> > +        );
> > +    }
> > +
> > +    for my $node (@{ PVE::Cluster::get_nodelist() }) {
> > +        my $host_conf_path = "/etc/pve/nodes/$node/host.fw";
> > +        PVE::Firewall::lock_hostfw_conf(
> > +            $node,
> > +            10,
> > +            sub {
> > +                my $host_conf = PVE::Firewall::load_hostfw_conf($conf, $host_conf_path);
> > +                return if !defined($host_conf);
> > +                if ($rewrite->($host_conf, 'cluster', 0)) {
> > +                    PVE::Firewall::save_hostfw_conf($host_conf, $host_conf_path);
> > +                }
> > +            },
> > +        );
> > +    }
> > +}
> 
> This is missing the VNet firewall configurations entirely, they do not
> get updated as a consequence.
> 
totally missed that, thanks for noticing! will fix in v2. 
> 
> 
>