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

[Arthur Bied-Charreton <a.bied-charreton@proxmox.com>]

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