Re: [PATCH proxmox-firewall 11/13] firewall: dump config to local directory after apply

["Thomas Ellmenreich" <t.ellmenreich@proxmox.com>]

On Tue Jul 21, 2026 at 3:54 PM CEST, Arthur Bied-Charreton wrote:
> The firewall configuration files are stored on pmxcfs, which is only
> available when pve-cluster is up. To be able to restore firewall rules
> before the network comes up and close the boot-time window in which the
> PVE host is unprotected, dump the required configuration files to a
> local directory after every apply.
>
> The last dumped version is cached for each file, the daemon only
> actually writes to disk if there has been a change since the last write
> or the dump file does not exist.
>
> Ownership of the dumps is keyed on the nftables option in the host
> config. When the firewall is disabled, the dumps are only removed if
> nftables is enabled - otherwise that responsibility is on pve-firewall.
>
> Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>

[snip]

> diff --git a/proxmox-firewall/src/bin/proxmox-firewall.rs b/proxmox-firewall/src/bin/proxmox-firewall.rs
> index 27fd67d..5b20ca0 100644
> --- a/proxmox-firewall/src/bin/proxmox-firewall.rs
> +++ b/proxmox-firewall/src/bin/proxmox-firewall.rs

[snip]

> @@ -42,14 +48,58 @@ fn remove_firewall() -> Result<(), std::io::Error> {
>      Ok(())
>  }
>  
> +fn dump_config(cfg: &FirewallConfig, cache: &mut HashMap<&str, Vec<u8>>) {
> +    let dump_dir = Path::new(DUMP_DIR);
> +    if !dump_dir.exists()
> +        && let Err(e) = fs::create_path(dump_dir, None, None)
> +    {
> +        log::warn!("could not create {DUMP_DIR}: {e} - config will not be dumped");
                ^
Should this `warn` maybe be an `error` since not dumping the config
means having no firewall during startup which could be quite grave for
certain usecases? But just a thought, maybe I'm missing something.

> +        return;
> +    }
> +
> +    if let Err(e) = std::fs::set_permissions(dump_dir, std::fs::Permissions::from_mode(0o700)) {
> +        log::warn!("could not set permissions on {DUMP_DIR}: {e}");
> +    }
> +
> +    for (bytes, name) in [
> +        (cfg.cluster_raw(), "cluster.fw"),
> +        (cfg.host_raw(), "host.fw"),
> +        (cfg.sdn_raw(), "sdn.json"),
> +    ] {
> +        let out = dump_dir.join(name);
> +        match bytes {
> +            Some(b) if !out.exists() || b != cache.get(name).map(Vec::as_slice).unwrap_or(&[]) => {
> +                match fs::replace_file(&out, b, fs::CreateOptions::new(), true) {
> +                    Err(e) => log::warn!("could not dump {name} to {out:?}: {e}"),
                                        ^
                                same with this `warn`?

> +                    Ok(()) => {
> +                        log::info!("successfully dumped {out:?}");
> +                        cache.insert(name, b.to_vec());
> +                    }
> +                }
> +            }
> +            Some(_) => log::info!("no changes to {name} since last dump to {out:?}"),
> +            None => {
> +                log::info!("nothing to dump for {name}");
                        ^
        This could maybe be removed or downgraded to `debug` or `trace`
        as it doesn't provide vital information and thus clutters the
        log in most cases.

> +                cache.remove(name);
> +                _ = std::fs::remove_file(&out);
> +            }
> +        }
> +    }
> +}

[snip]