Re: [pve-devel] [RFC container/firewall/manager/proxmox-firewall/qemu-server 00/37] proxmox firewall nftables implementation

[Lukas Wagner <l.wagner@proxmox.com>]

On  2024-04-02 19:15, Stefan Hanreich wrote:
> ## Introduction
> This RFC provides a drop-in replacement for the current pve-firewall package
> that is based on Rust and nftables.
> 
> It consists of three crates:
> * proxmox-ve-config
>   for parsing firewall and guest configuration files, as well as some helpers
>   to access host configuration (particularly networking)
> * proxmox-nftables
>   contains bindings for libnftables as well as types that implement the JSON
>   schema defined by libnftables-json
> * proxmox-firewall
>   uses the other two crates to read the firewall configuration and create the
>   respective nftables configuration
> 

Great work!

Did a relatively shallow review of the Rust parts, digging deeper only into
a smaller subset of the code.
Some aspects where I see room for improvement are mostly documentation,
as Max already mentioned, and some more automated testing. I think it would
greatly benefit the long-term maintainability of this tool to test the
the full 'config files' --> 'Command' transformation. This would require some
refactoring in the part reading the configuration, because currently the
config paths seem to be mostly hard coded. 
Since `Command` is serializable anyway, we could have a nice test suite of
firewall/VM config files and expected commands as JSON dumps. 
This will be tedious to setup at first, but will help to detect any unwanted
regressions in the long-term.

-- 
- Lukas