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 CF1C31FF0BC for ; Thu, 15 Oct 2026 14:45:56 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id B44622163F; Thu, 15 Oct 2026 14:45:38 +0200 (CEST) MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Subject: Re: [PATCH pve-firewall v2 03/12] firewall: dump configs locally after applying From: Stefan Hanreich To: Arthur Bied-Charreton In-Reply-To: <20261001122625.348730-4-a.bied-charreton@proxmox.com> References: <20261001122625.348730-1-a.bied-charreton@proxmox.com> <20261001122625.348730-4-a.bied-charreton@proxmox.com> Date: Thu, 15 Oct 2026 14:45:23 +0200 Message-Id: <179206832307.200960.4312066992851317571.b4-review@b4> X-Mailer: b4 0.16-dev-fbf84 X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1792068331079 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.471 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_MED -2.3 Sender listed at https://www.dnswl.org/, medium 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: RB4VLRJ3F445U2ZRAIZL3DZK6DBXDDGQ X-Message-ID-Hash: RB4VLRJ3F445U2ZRAIZL3DZK6DBXDDGQ 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 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: > ... and remove them when the firewall is disabled in the config or the > daemon is stopped. > > The firewall config lives on pmxcfs, which is only available once > pve-cluster is up, i.e. well after the network. To be able to restore > rules before that, dump the config files needed to recompile the ruleset > (cluster.fw, host.fw and the SDN config as sdn.json) to a local > directory after every successful apply. The guest configs are not > dumped, since pve-guests does not start until the actual firewall daemon > is running. > > The last dumped content is cached per file, so the daemon only writes > when something changed since the last dump, or if the dump file does not > exist at all. > > Ownership of the dumps is keyed on the nftables option in the host > config. If the firewall is disabled cluster-wide, the dumps are removed. > If nftables is enabled, they are left in place, since proxmox-firewall > owns them in that case. > > The dumps are also removed when the daemon is stopped, to prevent a > ruleset restored at boot from living forever if no daemon starts > afterwards. > > Signed-off-by: Arthur Bied-Charreton > Reviewed-by: Thomas Ellmenreich > Tested-by: Thomas Ellmenreich > > diff --git a/debian/dirs b/debian/dirs > index c9e3b54..e472f56 100644 > --- a/debian/dirs > +++ b/debian/dirs > @@ -1 +1,2 @@ > /var/lib/pve-firewall > +/var/lib/pve/firewall > diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm > index 592e5fe..81901e3 100644 > --- a/src/PVE/Firewall.pm > +++ b/src/PVE/Firewall.pm > @@ -17,6 +17,7 @@ use Storable qw(dclone); > use PVE::Cluster; > use PVE::Corosync; > use PVE::Exception qw(raise raise_param_exc); > +use PVE::File; > use PVE::INotify; > use PVE::JSONSchema qw(register_standard_option get_standard_option); > use PVE::Network; > @@ -32,6 +33,8 @@ my $pvefw_conf_dir = "/etc/pve/firewall"; > my $clusterfw_conf_filename = "$pvefw_conf_dir/cluster.fw"; > my $vnetfw_conf_dir = "/etc/pve/sdn/firewall"; > > +my $dump_dir = '/var/lib/pve/firewall'; We moved from using those abbreviations in names / paths to just proxmox in general. E.g. the firewall binary is located in /usr/libexec/proxmox. > + > # dynamically include PVE::QemuServer and PVE::LXC > # to avoid dependency problems > my $have_qemu_server; > @@ -4231,7 +4234,7 @@ sub load_sdn_conf { > return $sdn_config // $empty_sdn_config; > } > > -sub save_clusterfw_conf { > +sub serialize_clusterfw_conf { > my ($cluster_conf) = @_; > > my $raw = ''; > @@ -4266,11 +4269,21 @@ sub save_clusterfw_conf { > } > } > > + return $raw; > +} > + > +sub save_clusterfw_conf { > + my ($cluster_conf, $filename) = @_; > + > + $filename = $clusterfw_conf_filename if !defined($filename); > + > + my $raw = serialize_clusterfw_conf($cluster_conf); > + > if ($raw) { > mkdir $pvefw_conf_dir; > - PVE::Tools::file_set_contents($clusterfw_conf_filename, $raw); > + PVE::Tools::file_set_contents($filename, $raw); > } else { > - unlink $clusterfw_conf_filename; > + unlink($filename); > } > } > > @@ -4294,11 +4307,8 @@ sub load_hostfw_conf { > return generic_fw_config_parser($filename, $cluster_conf, $empty_conf, 'host'); > } > > -sub save_hostfw_conf { > - my ($hostfw_conf, $filename) = @_; > - > - $filename = $hostfw_conf_filename if !defined($filename); > - > +sub serialize_hostfw_conf { > + my ($hostfw_conf) = @_; > my $raw = ''; > > my $options = $hostfw_conf->{options}; > @@ -4311,6 +4321,16 @@ sub save_hostfw_conf { > $raw .= "\n"; > } > > + return $raw; > +} > + > +sub save_hostfw_conf { > + my ($hostfw_conf, $filename) = @_; > + > + $filename = $hostfw_conf_filename if !defined($filename); > + > + my $raw = serialize_hostfw_conf($hostfw_conf); > + > if ($raw) { > PVE::Tools::file_set_contents($filename, $raw); > } else { > @@ -5432,6 +5452,14 @@ sub remove_pvefw_chains { > > } > > +sub delete_dumps { > + > + unlink "$dump_dir/cluster.fw"; > + unlink "$dump_dir/host.fw"; > + unlink "$dump_dir/sdn.json"; > + > +} > + > sub remove_pvefw_chains_iptables { > my ($iptablescmd, $table) = @_; > > @@ -5530,21 +5558,57 @@ sub init { > # load required modules here > } > > +my $dump_cache = {}; > + > +sub dump_if_changed { > + my ($f, $data) = @_; > + return if -f $f && defined($dump_cache->{$f}) && $dump_cache->{$f} eq $data; > + PVE::File::file_set_contents($f, $data); > + $dump_cache->{$f} = $data; > + syslog('info', "dumped $f\n"); > +} > + > +# The cache is only filled once this daemon wrote the dumps, i.e. while it owns them. > +sub delete_dumps_if_owned { > + delete_dumps() if $dump_cache->%*; > +} > + > sub update { > my $code = sub { > + my $cfw_dump_path = "$dump_dir/cluster.fw"; > + my $hfw_dump_path = "$dump_dir/host.fw"; > + my $sdn_dump_path = "$dump_dir/sdn.json"; > > my $cluster_conf = load_clusterfw_conf(); > my $hostfw_conf = load_hostfw_conf($cluster_conf); > > if (!is_enabled_and_not_nftables($cluster_conf, $hostfw_conf)) { > PVE::Firewall::remove_pvefw_chains(); > + $dump_cache = {}; > + # disabled in config: drop stale dumps so the pre-network restore does not resurrect a > + # disabled ruleset. leave them in nftables mode, since in that case proxmox-firewall > + # owns the dump directory. > + delete_dumps() if !$cluster_conf->{options}->{enable}; > return; > } > > + # compile() rewrites rule actions in-place. snapshot the untouched configs now for the > + # post-apply boot-time dump. > + my ($cfw_dump, $hfw_dump) = (dclone($cluster_conf), dclone($hostfw_conf)); > + > my ($ruleset, $ipset_ruleset, $rulesetv6, $ebtables_ruleset) = > compile($cluster_conf, $hostfw_conf); > > apply_ruleset($ruleset, $hostfw_conf, $ipset_ruleset, $rulesetv6, $ebtables_ruleset); > + > + eval { > + mkdir($dump_dir); mkdir takes a mode parameter directly, use make_path here as well as we do in the network interface pinning? This also prevents us from changing the permissions every update cycle... > + chmod(0700, $dump_dir); > + dump_if_changed($cfw_dump_path, serialize_clusterfw_conf($cfw_dump)); > + dump_if_changed($hfw_dump_path, serialize_hostfw_conf($hfw_dump)); > + dump_if_changed($sdn_dump_path, JSON->new->utf8->canonical->encode($cfw_dump->{sdn})); Did you check the performance impact of serializing and comparing the whole configuration file? It shouldn't matter too much afaict, but a quick check wouldn't hurt... -- Stefan Hanreich