From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 8C8981FF0E6 for ; Fri, 24 Jul 2026 10:52:55 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id E0D01214A2; Fri, 24 Jul 2026 10:52:54 +0200 (CEST) Date: Fri, 24 Jul 2026 10:52:49 +0200 From: Arthur Bied-Charreton To: Stefan Hanreich Subject: Re: [PATCH pve-firewall 1/5] Add helpers for updating alias and ipset references Message-ID: <4mvjlg76hifx6lgpfgz2fgfqhxirxw6xllcu2xq3v264vxbf7d@4mchf4gp2g5w> References: <20260407073658.90818-1-a.bied-charreton@proxmox.com> <20260407073658.90818-2-a.bied-charreton@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1784883140253 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.865 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_LOW -0.7 Sender listed at https://www.dnswl.org/, low trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: 646UP7R3R3J5TB5R6RKIYZF5APHMWSU3 X-Message-ID-Hash: 646UP7R3R3J5TB5R6RKIYZF5APHMWSU3 X-MailFrom: a.bied-charreton@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: pve-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 > > --- > > 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. > > >