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:37:58AM +0200, Arthur Bied-Charreton wrote:
> 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. 

forgot to add: the commit message should reflect that better, will adapt
it of course :)
> 
> [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([]))
> > > +    }
> > > +}
> 
> 
> 
>