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