Re: [PATCH pve-firewall v2 05/12] firewall: add restore command

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

> Add a 'restore' command that recompiles and applies the firewall from
> the config dumped to /var/lib/pve/firewall, to bridge the boot window
> before pmxcfs (and thus the real config) is available.
> 
> It reuses update() with a local flag: configs are loaded from the local
> dumps instead of pmxcfs and nothing is dumped back. A missing or
> unreadable SDN dump is treated as empty, and rules referencing an
> unavailable IPSet are skipped, so a partial dump still restores the rest
> on a best-effort basis.
> 
> For the host config, a host.fw.new written by the
> pve-network-interface-pinning tool takes precedence over the plain dump
> when present. No .new file is ever created for the cluster config, so
> that is only loaded from cluster.fw.
> 
> 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/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
> index 81901e3..6779bbc 100644
> --- a/src/PVE/Firewall.pm
> +++ b/src/PVE/Firewall.pm
> @@ -4196,11 +4196,11 @@ sub lock_clusterfw_conf {
>  }
>  
>  sub load_clusterfw_conf {
> -    my ($filename) = @_;
> +    my ($filename, $sdn_conf) = @_;
>  
>      $filename = $clusterfw_conf_filename if !defined($filename);
>  
> -    my $sdn_conf = load_sdn_conf();
> +    $sdn_conf //= load_sdn_conf();
>  
>      my $empty_conf = {
>          rules => [],
> @@ -5573,14 +5573,31 @@ sub delete_dumps_if_owned {
>      delete_dumps() if $dump_cache->%*;
>  }
>  
> +my sub get_local_config_dump_path {
> +    my ($filename) = @_;
> +
> +    my $new = "$filename.new";
> +    return $new if -f $new;
> +    return $filename;
> +}
> +
>  sub update {
> +    my ($local) = @_;
> +
>      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);
> +        my $cfw_conf_path = $local ? get_local_config_dump_path($cfw_dump_path) : undef;
> +        my $hfw_conf_path = $local ? get_local_config_dump_path($hfw_dump_path) : undef;
> +        my $sdn_conf = undef;
> +        $sdn_conf = eval { decode_json(PVE::File::file_get_contents($sdn_dump_path)) }
> +            if $local;
> +        $sdn_conf = undef if $local && (!ref($sdn_conf) || ref($sdn_conf->{ipset}) ne 'HASH');
> +
> +        my $cluster_conf = load_clusterfw_conf($cfw_conf_path, $sdn_conf);
> +        my $hostfw_conf = load_hostfw_conf($cluster_conf, $hfw_conf_path);
>  
>          if (!is_enabled_and_not_nftables($cluster_conf, $hostfw_conf)) {
>              PVE::Firewall::remove_pvefw_chains();
> @@ -5588,10 +5605,13 @@ sub update {
>              # 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};
> +            delete_dumps() if !$local && !$cluster_conf->{options}->{enable};
> +            syslog('info', "iptables firewall is not enabled, not restoring rules\n") if $local;
>              return;
>          }
>  
> +        syslog('info', "restoring last remembered ruleset from $dump_dir\n") if $local;
> +
>          # 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));
> @@ -5601,6 +5621,8 @@ sub update {
>  
>          apply_ruleset($ruleset, $hostfw_conf, $ipset_ruleset, $rulesetv6, $ebtables_ruleset);
>  
> +        return if $local;
> +
>          eval {
>              mkdir($dump_dir);
>              chmod(0700, $dump_dir);

Imo moving this into its own restore function wouldn't hurt. Theres'
quite a few divergent paths here now and a lot of conditionals
involving $local. Imo untangling this would make everything easier to
reason about, reduce the potential for unwanted regressions and make
the code a lot clearer (at the cost of some slight duplication...).

-- 
Stefan Hanreich <s.hanreich@proxmox.com>