[PATCH firewall 0/3] fix #1965: cache the cluster firewall config

[PATCH firewall 0/3] fix #1965: cache the cluster firewall config

[Arthur Bied-Charreton]

The cluster firewall config is is not currently cached, so it is re-
parsed on every load. This can use a noticeable amount of CPU if the
firewall configuration contains large ipsets, for example.

These changes are based on an old series by Stefan Hrdlicka [0]. The
only major diff compared to that series is that the in-memory firewall
config now includes SDN ipsets. Those are not stored in cluster.fw, so
changes to those do not create new versions for it and therefore need to
be handled separately, see commit messages for details.

[0] https://lists.proxmox.com/pipermail/pve-devel/2022-November/055051.html


pve-firewall:

Arthur Bied-Charreton (3):
  firewall: run make tidy
  firewall: parser: factor out error logging
  fix #1965: firewall: cache cluster config

 src/PVE/API2/Firewall/Helpers.pm |   3 +-
 src/PVE/Firewall.pm              | 124 ++++++++++++++++++++++---------
 2 files changed, 89 insertions(+), 38 deletions(-)


Summary over all repositories:
  2 files changed, 89 insertions(+), 38 deletions(-)

-- 
Generated by murpp 0.12.1

[PATCH pve-firewall 1/3] firewall: run make tidy

[Arthur Bied-Charreton]

No functional changes intended.

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 src/PVE/API2/Firewall/Helpers.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/PVE/API2/Firewall/Helpers.pm b/src/PVE/API2/Firewall/Helpers.pm
index d0decae..4c78924 100644
--- a/src/PVE/API2/Firewall/Helpers.pm
+++ b/src/PVE/API2/Firewall/Helpers.pm
@@ -52,7 +52,8 @@ sub check_vnet_access {
     my ($vnetid, $privileges) = @_;
 
     my $running_config = PVE::Network::SDN::Vnets::config(1);
-    my $_vnet = PVE::Network::SDN::Vnets::api_get_vnet_checked($vnetid, $privileges, $running_config);
+    my $_vnet =
+        PVE::Network::SDN::Vnets::api_get_vnet_checked($vnetid, $privileges, $running_config);
 }
 
 1;
-- 
2.47.3

[PATCH pve-firewall 2/3] firewall: parser: factor out error logging

[Arthur Bied-Charreton]

The next commit will add a new caller.

Also print errors in deterministic order, no functional changes intended
besides that.

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 src/PVE/Firewall.pm | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index dd5d92c..a1fd6a3 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -3343,6 +3343,19 @@ sub get_mark_values {
     return ($value, $mask);
 }
 
+my $log_rule_errors = sub {
+    my ($prefix, $line, $rule) = @_;
+
+    # The verbose flag really means we're running from the CLI and want
+    # output on the console - in the other case we really want such errors
+    # to go into the syslog instead.
+    my $log = $verbose ? sub { warn @_ } : sub { syslog(err => @_) };
+    $log->("$prefix - errors in rule parameters: $line\n");
+    for my $p (sort keys $rule->{errors}->%*) {
+        $log->("  $p: $rule->{errors}->{$p}\n");
+    }
+};
+
 sub parse_fw_rule {
     my ($prefix, $line, $cluster_conf, $fw_conf, $rule_env) = @_;
 
@@ -3417,16 +3430,7 @@ sub parse_fw_rule {
     die "unable to parse rule parameters: $line\n" if length($line);
 
     $rule = verify_rule($rule, $cluster_conf, $fw_conf, $rule_env, 1);
-    if ($rule->{errors}) {
-        # The verbose flag really means we're running from the CLI and want
-        # output on the console - in the other case we really want such errors
-        # to go into the syslog instead.
-        my $log = $verbose ? sub { warn @_ } : sub { syslog(err => @_) };
-        $log->("$prefix - errors in rule parameters: $orig_line\n");
-        foreach my $p (keys %{ $rule->{errors} }) {
-            $log->("  $p: $rule->{errors}->{$p}\n");
-        }
-    }
+    $log_rule_errors->($prefix, $orig_line, $rule) if $rule->{errors};
 
     return $rule;
 }
-- 
2.47.3

[PATCH pve-firewall 3/3] fix #1965: firewall: cache cluster config

[Arthur Bied-Charreton]

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 <s.hrdlicka@proxmox.com>
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 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

Re: [PATCH pve-firewall 3/3] fix #1965: firewall: cache cluster config

[Stefan Hanreich]

On 11/4/26 3:20 PM, Arthur Bied-Charreton wrote:

[...]

> +    # 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);

verify_rule can die, so we should probably eval here? Or at least in the loop below?

Re: [PATCH pve-firewall 3/3] fix #1965: firewall: cache cluster config

[Stefan Hanreich]

On 11/4/26 3:20 PM, Arthur Bied-Charreton wrote:
> 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)
That is with eea1b88b9 applied already? In that case the gain seems
marginal? Are there other possible configurations that would get
a bigger speed up. Because the core problem seems to be addressed
with eea1b88b9 alone and then we don't have the added logic of
re-verifying / caching?

Re: [PATCH pve-firewall 3/3] fix #1965: firewall: cache cluster config

[Arthur Bied-Charreton]

On Wed, Nov 04, 2026 at 05:36:44PM +0100, Stefan Hanreich wrote:
> On 11/4/26 3:20 PM, Arthur Bied-Charreton wrote:
> > 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)
> That is with eea1b88b9 applied already? In that case the gain seems
> marginal? Are there other possible configurations that would get
> a bigger speed up. Because the core problem seems to be addressed
> with eea1b88b9 alone and then we don't have the added logic of
> re-verifying / caching?
> 
Yes, this already includes eea1b88. Without that commit, the same
config parses in 1.66s on my test cluster.

I reproduced this user's configuration as described here [0] (with 
50/50 IPv6 and IPv4):

    - 29,430 lines 
    - Approximately 468 KB
    - 13 IPSet definitions
    - 29,225 IPSet entries
    - 26 Security Groups
    - 83 Security Group entries

The user reports ~7.3s response time for firewall endpoitns with 6.0.6,
which has the regression from 99343fe but not yet eea1b88, and ~1.5s on
their PVE 8 cluster, i.e. the pre-regression parser. On my (idle) test
cluster the reproduced config gives ~2s and ~0.5s, respectively, so it
seems like their hardware is ~3x slower. eea1b88 brings them back to the
PVE 8 numbers.

The cache addresses an issue that predates the regression eea1b88 fixes.
I do agree it seems marginal on my tests, but given a lot of our
customers seem to be running slower hardware, a large cluster config
might still be costing almost a second per 10 second firewall cycle,
unconditionally, with a config nearing the 1mb pmxcfs size limit.

> Are there other possible configurations that would get a bigger speed
> up.
Rules with both literal source and literal destination address are more
expensive to parse (got up to 1.986s avg on a config with ~18k rules).
IPv6 vs. IPv4 did not make any meaningful difference after eea1b88b9. 

I agree that the caching logic is a little scary here because of the SDN
re-verification. I have no particularly strong feelings about this given
the nftables config parser is a lot faster anyway. I just think the
numbers on my machine might not be reflecting the actual cost on typical
customer hardware. 

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=8261

Re: [PATCH pve-firewall 3/3] fix #1965: firewall: cache cluster config

[Arthur Bied-Charreton]

On Wed, Nov 04, 2026 at 05:15:20PM +0100, Stefan Hanreich wrote:
> On 11/4/26 3:20 PM, Arthur Bied-Charreton wrote:
> 
> [...]
> 
> > +    # 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);
> 
> verify_rule can die, so we should probably eval here? Or at least in the loop below?
> 
good point, thanks! will fix that if we decide we want this change.