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

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

On Fri, Sep 11, 2026 at 11:10:30AM +0200, Thomas Ellmenreich wrote:
> 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.
> 
You're right, error might be more suitable here. I originally opted for
warn since I felt that error should be kept for cases that actually
break the firewall's intended functionality, but I guess one could argue
that that is the case here. 
> > +        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`?
Ack, see above
> 
> > +                    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.
> 
The default logging level for the proxmox-firewall is WARN, so this is
not shown unless one actively changes it. I do agree that debug makes
more sense though, will change that, thanks!
> > +                cache.remove(name);
> > +                _ = std::fs::remove_file(&out);
> > +            }
> > +        }
> > +    }
> > +}
> 
> [snip]