From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 520E81FF1A6 for ; Fri, 5 Dec 2025 14:39:29 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 3EEA81E1C1; Fri, 5 Dec 2025 14:39:55 +0100 (CET) Message-ID: <75f887c2-fce6-444e-829c-977784dca365@proxmox.com> Date: Fri, 5 Dec 2025 14:39:50 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Stefan Hanreich , Proxmox VE development discussion References: <20251202125343.91927-1-r.obkircher@proxmox.com> <46d69814-f3a8-4ce9-888c-e78f52e9d526@proxmox.com> Content-Language: en-US, de-AT From: Robert Obkircher In-Reply-To: <46d69814-f3a8-4ce9-888c-e78f52e9d526@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1764941942966 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.068 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 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [rule.rs, insta.rs] 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-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" 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 >> --- >> 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/ 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 { >> @@ -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