Re: [PATCH proxmox-firewall 12/13] firewall: add restore command

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

On Fri, Sep 11, 2026 at 11:14:30AM +0200, Thomas Ellmenreich wrote:
> One comment inline ;)
> 
> On Tue Jul 21, 2026 at 3:54 PM CEST, Arthur Bied-Charreton wrote:
> > 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 handle_firewall() along with a local firewall config loader.
> > The config files are loaded from the local dump 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 cluster and host config, a user-provided .override or a .new
> > written by the pve-network-interface-pinning tool take precedence over
> > the plain dump when present (.override -> .new -> plain).
> >
> > Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> > ---
> 
> [snip]
> 
> > diff --git a/proxmox-firewall/src/config.rs b/proxmox-firewall/src/config.rs
> > index eb8dd21..83b9161 100644
> > --- a/proxmox-firewall/src/config.rs
> > +++ b/proxmox-firewall/src/config.rs
> 
> [snip]
> 
> > @@ -247,6 +249,81 @@ impl FirewallConfigLoader for PveFirewallConfigLoader {
> >      }
> >  }
> >  
> > +#[derive(Debug, Default)]
> > +pub struct LocalFirewallConfigLoader {}
> > +
> > +impl LocalFirewallConfigLoader {
> > +    pub fn new() -> Self {
> > +        Self::default()
> > +    }
> > +}
> > +
> > +fn load_first_of(candidates: &[&str], what: &str) -> Result<Option<Box<dyn BufRead>>, Error> {
> > +    for p in candidates.iter().map(|p| Path::new(DUMP_DIR).join(p)) {
> > +        if let Some(fd) = open_config_file(&p)? {
> > +            log::info!("loaded {what} config from {p:?}");
> > +            let reader = Box::new(BufReader::new(fd)) as Box<dyn BufRead>;
> > +            return Ok(Some(reader));
> > +        }
> > +    }
> > +    Ok(None)
> > +}
> > +
> > +impl FirewallConfigLoader for LocalFirewallConfigLoader {
> > +    fn cluster(&self) -> Result<Option<Box<dyn BufRead>>, Error> {
> > +        load_first_of(&["cluster.fw.override", "cluster.fw"], "cluster")
>                                                  ^
>             Reading the commit message it sounds like a 'cluster.fw.new'
>             should also exist? Whats the reason for that not being the
>             case?
> 
host.fw.new files are created by the pve-network-interface-pinning tool
[0][1] when changing network interface pinnings, since this only takes
effect at the next boot and the old config should still be used until
then. This tool never creates a .new file for the cluster config, since
network interface name pinnings are node-specific. 

[0] https://git.proxmox.com/?p=pve-manager.git;a=blob;f=PVE/CLI/pve_network_interface_pinning.pm;h=9dff181dcd9d616e725380e98aba2fb36c6efbcf;hb=refs/heads/master
[1] https://lore.proxmox.com/pve-devel/20260721135407.372150-2-a.bied-charreton@proxmox.com/
> > +    }
> > +
> > +    fn host(&self) -> Result<Option<Box<dyn BufRead>>, Error> {
> > +        load_first_of(&["host.fw.override", "host.fw.new", "host.fw"], "host")
> > +    }
> > +
> > +    fn sdn_running_config(&self) -> Result<Option<Box<dyn BufRead>>, Error> {
> > +        let path = Path::new(DUMP_DIR).join("sdn.json");
> > +        if let Some(fd) = open_config_file(&path)? {
> > +            let reader = Box::new(BufReader::new(fd)) as Box<dyn BufRead>;
> > +            return Ok(Some(reader));
> > +        }
> > +
> > +        Ok(None)
> > +    }
> > +
> > +    fn guest_config(
> > +        &self,
> > +        _: &Vmid,
> > +        _: &GuestEntry,
> > +    ) -> Result<Option<Box<dyn io::BufRead>>, Error> {
> > +        Ok(None)
> > +    }
> > +
> > +    fn guest_firewall_config(&self, _: &Vmid) -> Result<Option<Box<dyn io::BufRead>>, Error> {
> > +        Ok(None)
> > +    }
> > +
> > +    fn guest_list(&self) -> Result<GuestMap, Error> {
> > +        Ok(GuestMap::from(HashMap::new()))
> > +    }
> > +
> > +    fn bridge_firewall_config(
> > +        &self,
> > +        _: &BridgeName,
> > +    ) -> Result<Option<Box<dyn io::BufRead>>, Error> {
> > +        Ok(None)
> > +    }
> > +
> > +    fn bridge_list(&self) -> Result<Vec<BridgeName>, Error> {
> > +        Ok(vec![])
> > +    }
> > +
> > +    fn ipam(&self) -> Result<Option<Box<dyn io::BufRead>>, Error> {
> > +        Ok(None)
> > +    }
> > +
> > +    fn interface_mapping(&self) -> Result<AltnameMapping, Error> {
> > +        Ok(AltnameMapping::from_iter([]))
> > +    }
> > +}