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 C1D791FF0C0 for ; Wed, 04 Nov 2026 15:20:07 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 16EBD21614; Wed, 04 Nov 2026 15:19:43 +0100 (CET) From: Arthur Bied-Charreton To: pve-devel@lists.proxmox.com Subject: [PATCH pve-firewall 3/3] fix #1965: firewall: cache cluster config Date: Wed, 4 Nov 2026 15:19:31 +0100 Message-ID: <20261104141931.399659-4-a.bied-charreton@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20261104141931.399659-1-a.bied-charreton@proxmox.com> References: <20261104141931.399659-1-a.bied-charreton@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1793801977844 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.677 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: 52REMFIMUK2Y5EKXQZO2O5YNJUAPLSOZ X-Message-ID-Hash: 52REMFIMUK2Y5EKXQZO2O5YNJUAPLSOZ 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 X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Large firewall configs can take a noticeable amount of time to parse, as noted by the user on the linked issue. Register the config with cfs_register_file in order to avoid re-parsing it unless cluster.fw changed. Rule verification still depends on the SDN config, which can change independently. A rule referencing an SDN ipset that did not exist when it was parsed and cached is still stored with errors, even if that rule becomes valid later (e.g. if the rule references 'foo-dhcp' and the VNet 'foo' is created later). For this reason, the cached config keeps the SDN config it was verified against last time it was fully parsed. If the ipset names yielded by the current SDN config differ from what is stored, all cluster and group rules are re-verified against the new ones. The ccache cannot be invalidated from pve-firewall, so after an SDN change this happens on every load until cluster.fw is parsed again due to a new version or a daemon restart. Errors found on re-verification are logged to preserve the original behavior. Benchmark with two 25k-entry ipsets and 1050 rules (50 loads): without caching: first load 0.2447s, then avg 0.2509s (min 0.2301 max 0.2835) cached, not re-verified (steady state): first load 0.2694s, then avg 0.0157s (min 0.0131 max 0.0204) cached, re-verified (worst case, transient until next parse): avg 0.0721s (min 0.0657 max 0.1012) Also note that saving now goes through cfs_write_file so an empty config now results in an empty cluster.fw instead of none at all, but the parser treats both the same way. Link: https://bugzilla.proxmox.com/show_bug.cgi?id=1965 Originally-by: Stefan Hrdlicka Signed-off-by: Arthur Bied-Charreton --- src/PVE/Firewall.pm | 100 ++++++++++++++++++++++++++++++++------------ 1 file changed, 73 insertions(+), 27 deletions(-) diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm index a1fd6a3..4608a60 100644 --- a/src/PVE/Firewall.pm +++ b/src/PVE/Firewall.pm @@ -29,9 +29,14 @@ use PVE::Firewall::Helpers; use PVE::RS::Firewall::SDN; my $pvefw_conf_dir = "/etc/pve/firewall"; -my $clusterfw_conf_filename = "$pvefw_conf_dir/cluster.fw"; my $vnetfw_conf_dir = "/etc/pve/sdn/firewall"; +PVE::Cluster::cfs_register_file( + 'firewall/cluster.fw', + \&parse_clusterfw_conf, + \&format_clusterfw_conf, +); + # dynamically include PVE::QemuServer and PVE::LXC # to avoid dependency problems my $have_qemu_server; @@ -3632,11 +3637,6 @@ sub parse_alias { sub generic_fw_config_parser { my ($filename, $cluster_conf, $empty_conf, $rule_env) = @_; - my $section; - my $group; - - my $res = $empty_conf; - my $raw; if ($filename =~ m!^/etc/pve/(.*)$!) { $raw = PVE::Cluster::get_config($1); @@ -3644,7 +3644,15 @@ sub generic_fw_config_parser { $raw = eval { PVE::Tools::file_get_contents($filename) }; # ignore errors } return {} if !$raw; + return _generic_fw_config_parser($filename, $cluster_conf, $empty_conf, $rule_env, $raw); +} + +sub _generic_fw_config_parser { + my ($filename, $cluster_conf, $empty_conf, $rule_env, $raw) = @_; + my $section; + my $group; + my $res = $empty_conf; my $curr_group_keys = {}; my $linenr = 0; @@ -4229,26 +4237,43 @@ sub lock_clusterfw_conf { sub load_clusterfw_conf { my ($filename) = @_; - $filename = $clusterfw_conf_filename if !defined($filename); + my $cluster_conf; + if (defined($filename)) { # tests read from a plain file + my $raw = eval { file_get_contents($filename) }; + $cluster_conf = parse_clusterfw_conf($filename, $raw); + } else { + $cluster_conf = PVE::Cluster::cfs_read_file('firewall/cluster.fw'); + } + + $set_global_log_ratelimit->($cluster_conf->{options}); my $sdn_conf = load_sdn_conf(); - my $empty_conf = { - rules => [], - options => {}, - aliases => {}, - groups => {}, - group_comments => {}, - ipset => {}, - ipset_comments => {}, - sdn => $sdn_conf, + # cluster.fw is not bumped on SDN changes, however a change in the SDN config may make a + # rule that previously failed to parse valid again, or invalidate a previously valid one. + # re-verify all rules if the ipsets names yielded by the current SDN config differ from the + # stored ones. + my $names = sub { join(',', sort keys(($_[0] // {})->{ipset}->%*)) }; + my $sdn_ipsets_changed = $names->($sdn_conf) ne $names->($cluster_conf->{sdn}); + $cluster_conf->{sdn} = $sdn_conf; + + return $cluster_conf if !$sdn_ipsets_changed; + + my $reverify = sub { + my ($rules, $conf, $rule_env) = @_; + for my $rule ($rules->@*) { + delete $rule->{errors}; + verify_rule($rule, $cluster_conf, $conf, $rule_env, 1); + if ($rule->{errors}) { + chomp(my $line = $format_rules->([$rule], $rule_env_iface_lookup->{$rule_env})); + $log_rule_errors->("$pvefw_conf_dir/cluster.fw", $line, $rule); + } + } }; - my $cluster_conf = generic_fw_config_parser($filename, $empty_conf, $empty_conf, 'cluster'); - $set_global_log_ratelimit->($cluster_conf->{options}); - - if (!$cluster_conf->{sdn}) { - $cluster_conf->{sdn} = $sdn_conf; + $reverify->($cluster_conf->{rules} // [], $cluster_conf, 'cluster'); + for my $r (values(($cluster_conf->{groups} // {})->%*)) { + $reverify->($r, undef, 'group'); } return $cluster_conf; @@ -4265,9 +4290,35 @@ sub load_sdn_conf { return $sdn_config // $empty_sdn_config; } +sub parse_clusterfw_conf { + my ($filename, $raw) = @_; + + return {} if !$raw; + + my $empty_conf = { + rules => [], + options => {}, + aliases => {}, + groups => {}, + group_comments => {}, + ipset => {}, + ipset_comments => {}, + sdn => load_sdn_conf(), + }; + + return _generic_fw_config_parser($filename, $empty_conf, $empty_conf, 'cluster', $raw); +} + sub save_clusterfw_conf { my ($cluster_conf) = @_; + mkdir $pvefw_conf_dir; + PVE::Cluster::cfs_write_file('firewall/cluster.fw', $cluster_conf); +} + +sub format_clusterfw_conf { + my ($filename, $cluster_conf) = @_; + my $raw = ''; my $options = $cluster_conf->{options}; @@ -4300,12 +4351,7 @@ sub save_clusterfw_conf { } } - if ($raw) { - mkdir $pvefw_conf_dir; - PVE::Tools::file_set_contents($clusterfw_conf_filename, $raw); - } else { - unlink $clusterfw_conf_filename; - } + return $raw; } sub lock_hostfw_conf : prototype($$$@) { -- 2.47.3