Re: [pve-devel] [PATCH ve-rs 1/2] fix: firewall: introduce iptables to nftables mapping for icmpv6-types

From: Gabriel Goller <g.goller@proxmox.com>
To: Stefan Hanreich <s.hanreich@proxmox.com>
Cc: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH ve-rs 1/2] fix: firewall: introduce iptables to nftables mapping for icmpv6-types
Date: Mon, 6 Oct 2025 12:14:28 +0200	[thread overview]
Message-ID: <dmmfck4sc5jcfoue4wzswa5rxxax4vqwvyvqyoazziu2wkmto7@q2lrvumu2qcd> (raw)
In-Reply-To: <0d1a852c-876e-4484-bf19-35d616c7542a@proxmox.com>

On 01.10.2025 18:46, Stefan Hanreich wrote:
>On 9/16/25 11:32 AM, Gabriel Goller wrote:
>> nftables changed the names of the icmpv6-types and they don't overlap
>> completely with the old iptables names. Introduce a mapping that
>> converts old names into the new ones. A few of these are not supported,
>> see here for more info:
>> https://wiki.nftables.org/wiki-nftables/index.php/Supported_features_compared_to_xtables#icmp6
>
>Did you find a reasoning for that? Are they not in use anymore /
>deprecated? Then I guess we should not make that a hard error, but
>possibly a warning and soft failure? In the other case (still in use), I
>think we should still try to generate rules for them.
>
>Since those are configurations that users can have pre-existing, we
>should handle them gracefully instead of just erroring out on
>encountering them.

You're right I forgot erroring here in the firewall is not good.

>There are even other possible values that are still not considered here
>like 'TOS-network-unreachable'. Since they are all mappable to a numeric
>type/code combo - we should take all possible values for the field [1]
>[2] to preserve compatibility with existing configurations?
>
>Not sure if they're accurate, but pve-manager seems to have the
>respective information on type / code combinations [3]. Can take a
>closer look at it and send a follow-up.

Yep, I introced a mapping to type/code for icmpv6 and icmp types as
well!

>Not sure if this is a blocker, it might be a bit too obscure / niche to
>prevent this series from getting merged... - can always just do a follow-up.
>
>[1]
>https://git.proxmox.com/?p=pve-firewall.git;a=blob;f=src/PVE/Firewall.pm;h=49430b174bb2fdd56ce586f90bf929c5648f9060;hb=HEAD#l785
>[2]
>https://git.proxmox.com/?p=pve-firewall.git;a=blob;f=src/PVE/Firewall.pm;h=49430b174bb2fdd56ce586f90bf929c5648f9060;hb=HEAD#l826
>[3]
>https://git.proxmox.com/?p=pve-manager.git;a=blob;f=www/manager6/grid/FirewallRules.js;h=0db817ebce0e9254d18f172a6e02a7a12e7a481c;hb=HEAD#l83
>

Thanks for the review!

_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel