Re: [PATCH pve-firewall v2 03/12] firewall: dump configs locally after applying

[Stefan Hanreich <s.hanreich@proxmox.com>]

> ... 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 <a.bied-charreton@proxmox.com>
> Reviewed-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
> Tested-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
>
> 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 <s.hanreich@proxmox.com>