* [pve-devel] [PATCH v1 proxmox-firewall] fix #7068: show rule comments in nftables output
@ 2025-12-02 12:52 Robert Obkircher
2025-12-05 10:54 ` Stefan Hanreich
0 siblings, 1 reply; 3+ messages in thread
From: Robert Obkircher @ 2025-12-02 12:52 UTC (permalink / raw)
To: pve-devel
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());
+ }
+}
+
+// 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(())
}
}
--
2.47.3
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [pve-devel] [PATCH v1 proxmox-firewall] fix #7068: show rule comments in nftables output
2025-12-02 12:52 [pve-devel] [PATCH v1 proxmox-firewall] fix #7068: show rule comments in nftables output Robert Obkircher
@ 2025-12-05 10:54 ` Stefan Hanreich
2025-12-05 13:39 ` Robert Obkircher
0 siblings, 1 reply; 3+ messages in thread
From: Stefan Hanreich @ 2025-12-05 10:54 UTC (permalink / raw)
To: Proxmox VE development discussion, Robert Obkircher
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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [pve-devel] [PATCH v1 proxmox-firewall] fix #7068: show rule comments in nftables output
2025-12-05 10:54 ` Stefan Hanreich
@ 2025-12-05 13:39 ` Robert Obkircher
0 siblings, 0 replies; 3+ messages in thread
From: Robert Obkircher @ 2025-12-05 13:39 UTC (permalink / raw)
To: Stefan Hanreich, Proxmox VE development discussion
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 <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/
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<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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-12-05 13:39 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-12-02 12:52 [pve-devel] [PATCH v1 proxmox-firewall] fix #7068: show rule comments in nftables output Robert Obkircher
2025-12-05 10:54 ` Stefan Hanreich
2025-12-05 13:39 ` Robert Obkircher
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.