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

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

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?

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

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