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

[Stefan Hanreich <s.hanreich@proxmox.com>]

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.

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/

> +    }
> +}
> +
> +// 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