Re: [pve-devel] [PATCH v1 proxmox-firewall] fix #7068: show rule comments in nftables output

[Robert Obkircher <r.obkircher@proxmox.com>]

On 12/5/25 11:54, Stefan Hanreich wrote:
> Tested this on my machine:
> * "normal" comments
> * comments that are too long
> * comments that are too long and do not truncate nicely at the 128
> boundary
> * comments in security groups
> * emojis in comments
>
> We might wanna consider hiding this functionality behind an option in
> the cluster firewall.

Ok, I will add that. Such an option will also make it easier for users 
to discover that they can now use comments to debug the output.


>
> lgtm so far! one comment inline
>
> On 12/2/25 1:53 PM, Robert Obkircher wrote:
>> Add rule comments from the config file to the generated nft rules.
>> Truncate them to at most 128 bytes to match the limit in libnftnl.
>>
>> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
>> ---
>>   proxmox-firewall/src/rule.rs | 37 +++++++++++++++++++++++++++++++++++-
>>   1 file changed, 36 insertions(+), 1 deletion(-)
>>
>> diff --git a/proxmox-firewall/src/rule.rs b/proxmox-firewall/src/rule.rs
>> index b79f91c..f6c5e44 100644
>> --- a/proxmox-firewall/src/rule.rs
>> +++ b/proxmox-firewall/src/rule.rs
>> @@ -36,14 +36,19 @@ pub(crate) struct NftRule {
>>       family: Option<Family>,
>>       statements: Vec<Statement>,
>>       terminal_statements: Vec<Statement>,
>> +    comment: Option<String>,
>>   }
>>   
>>   impl NftRule {
>> +    /// from NFTNL_UDATA_COMMENT_MAXLEN
>> +    pub const MAX_COMMENT_LEN: usize = 128;
>> +
>>       pub fn from_terminal_statements(terminal_statements: Vec<Statement>) -> Self {
>>           Self {
>>               family: None,
>>               statements: Vec::new(),
>>               terminal_statements,
>> +            comment: None,
>>           }
>>       }
>>   
>> @@ -52,6 +57,7 @@ impl NftRule {
>>               family: None,
>>               statements: Vec::new(),
>>               terminal_statements: vec![terminal_statement],
>> +            comment: None,
>>           }
>>       }
>>   
>> @@ -81,6 +87,24 @@ impl NftRule {
>>           ipfilter.to_nft_rules(&mut rules, env)?;
>>           Ok(rules)
>>       }
>> +
>> +    pub fn set_comment(&mut self, comment: &str) {
>> +        self.comment =
>> +            Some(comment[..my_floor_char_boundary(comment, Self::MAX_COMMENT_LEN)].to_string());
> Might make sense imo to add a test case with a comment that does not
> nicely truncate at that index to have this covered. Potentially also
> some "normal" comments.
>
> There are snapshot tests in this repository that take a fw configuration
> and generate the full JSON for the ruleset - would be a good addition
> there imo. They use insta [1] and you'd need to regenerate the snapshots
> after changing the input files.
>
> [1] https://insta.rs/
I've seen the snapshot tests while working on the comment parser fix, 
but in this case a normal unit test might be better, because it can 
check the length of the output.

Something that seems very hard to test is that our serialization could 
potentially escape characters that are not unescaped by the nftables parser.

Another problem that I deliberately ignored is that truncating to a code 
point can change the meaning of the last grapheme cluster.

>
>> +    }
>> +}
>> +
>> +// TODO: replace with str::floor_char_boundary once rustc 1.91.0 is available
>> +fn my_floor_char_boundary(s: &str, index: usize) -> usize {
>> +    if index >= s.len() {
>> +        s.len()
>> +    } else {
>> +        s.char_indices()
>> +            .map(|(i, _)| i)
>> +            .take_while(|i| *i <= index)
>> +            .last()
>> +            .unwrap_or(0)
>> +    }
>>   }
>>   
>>   impl Deref for NftRule {
>> @@ -101,7 +125,12 @@ impl NftRule {
>>       pub fn into_add_rule(self, chain: ChainPart) -> AddRule {
>>           let statements = self.statements.into_iter().chain(self.terminal_statements);
>>   
>> -        AddRule::from_statements(chain, statements)
>> +        let result = AddRule::from_statements(chain, statements);
>> +        if let Some(comment) = self.comment {
>> +            result.with_comment(comment)
>> +        } else {
>> +            result
>> +        }
>>       }
>>   
>>       pub fn family(&self) -> Option<Family> {
>> @@ -175,11 +204,17 @@ impl ToNftRules for Rule {
>>       fn to_nft_rules(&self, rules: &mut Vec<NftRule>, env: &NftRuleEnv) -> Result<(), Error> {
>>           log::trace!("generating nft rules for config rule {self:?}");
>>   
>> +        let before = rules.len();
>>           match self.kind() {
>>               Kind::Match(rule) => rule.to_nft_rules(rules, env)?,
>>               Kind::Group(group) => group.to_nft_rules(rules, env)?,
>>           };
>>   
>> +        if let Some(comment) = self.comment() {
>> +            for nft_rule in &mut rules[before..] {
>> +                nft_rule.set_comment(comment);
>> +            }
>> +        }
>>           Ok(())
>>       }
>>   }
>


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