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 <[email protected]> > --- > 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 [email protected] https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
