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

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

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?

> +    }
> +
> +    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([]))
> +    }
> +}