From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id B18351FF1A6 for ; Fri, 5 Dec 2025 11:55:02 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 0E399147B2; Fri, 5 Dec 2025 11:55:29 +0100 (CET) Message-ID: <46d69814-f3a8-4ce9-888c-e78f52e9d526@proxmox.com> Date: Fri, 5 Dec 2025 11:54:55 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox VE development discussion , Robert Obkircher References: <20251202125343.91927-1-r.obkircher@proxmox.com> Content-Language: en-US From: Stefan Hanreich In-Reply-To: <20251202125343.91927-1-r.obkircher@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.726 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH v1 proxmox-firewall] fix #7068: show rule comments in nftables output X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" 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 > --- > 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, > statements: Vec, > terminal_statements: Vec, > + comment: Option, > } > > impl NftRule { > + /// from NFTNL_UDATA_COMMENT_MAXLEN > + pub const MAX_COMMENT_LEN: usize = 128; > + > pub fn from_terminal_statements(terminal_statements: Vec) -> 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 { > @@ -175,11 +204,17 @@ impl ToNftRules for Rule { > fn to_nft_rules(&self, rules: &mut Vec, 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