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

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

superseded-by: 
https://lore.proxmox.com/pve-devel/20251215150906.257151-1-r.obkircher@proxmox.com/

On 12/5/25 14:39, Robert Obkircher wrote:
>
> 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
>
>


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