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 40A1C1FF0E9 for ; Thu, 16 Jul 2026 15:17:45 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 819F321458; Thu, 16 Jul 2026 15:17:44 +0200 (CEST) Message-ID: Date: Thu, 16 Jul 2026 15:17:09 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH pve-firewall 1/5] Add helpers for updating alias and ipset references To: pve-devel@lists.proxmox.com References: <20260407073658.90818-1-a.bied-charreton@proxmox.com> <20260407073658.90818-2-a.bied-charreton@proxmox.com> Content-Language: en-US From: Stefan Hanreich In-Reply-To: <20260407073658.90818-2-a.bied-charreton@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.275 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: 62AGYFJCIZKCGF4OWHJ6TS3BM2RY7DP7 X-Message-ID-Hash: 62AGYFJCIZKCGF4OWHJ6TS3BM2RY7DP7 X-MailFrom: s.hanreich@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 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 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? > +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.