[pve-devel] [PATCH v1 proxmox-ve-rs] fix #6399: firewall: fix parsing rule comments that contain hash signs

[Stefan Hanreich]

Ran this on a test machine, could reproduce the issue without the patch
applied and confirm that it is fixed with the patch applied.

some small comments inline

Consider this:

Tested-by: Stefan Hanreich <s.hanreich at proxmox.com>
Reviewed-by: Stefan Hanreich <s.hanreich at proxmox.com>

On 11/28/25 2:24 PM, Robert Obkircher wrote:
> Match the behavior of the corresponding perl code in parse_fw_rule and
> treat everything after the first '#' on a line as the comment. This
> works because validation prevents fields like the interface name from
> contianing that symbol.
> 
> This fixes a bug where the firewall wouldn't start if a comment on a
> rule contained a number sign.
> 
> Signed-off-by: Robert Obkircher <r.obkircher at proxmox.com>
> ---
> The test in cluster.rs might be a bit overkill. Also, rust-analyzer
> doesn't seem to work correctly at the end of that file as I can't seem
> navigate to the defninitions of fields or methods.

Don't think so - always good to have additional tests to have our bases
covered - it also gives me as a reviewer additional confidence in your
patch series.

> 
> 
>  proxmox-ve-config/src/firewall/cluster.rs    | 66 +++++++++++++++++++-
>  proxmox-ve-config/src/firewall/types/rule.rs |  8 ++-
>  2 files changed, 70 insertions(+), 4 deletions(-)
> 
> diff --git a/proxmox-ve-config/src/firewall/cluster.rs b/proxmox-ve-config/src/firewall/cluster.rs
> index 69d3bcd..b8ecb69 100644
> --- a/proxmox-ve-config/src/firewall/cluster.rs
> +++ b/proxmox-ve-config/src/firewall/cluster.rs
> @@ -147,7 +147,8 @@ mod tests {
>          log::{LogLevel, LogRateLimitTimescale},
>          rule::{Kind, RuleGroup},
>          rule_match::{
> -            Icmpv6, Icmpv6Code, Icmpv6Type, IpAddrMatch, IpMatch, Ports, Protocol, RuleMatch, Tcp, Udp
> +            Icmpv6, Icmpv6Code, Icmpv6Type, IpAddrMatch, IpMatch, Ports, Protocol, RuleMatch, Tcp,
> +            Udp,

This seems to be a simple auto-format change unrelated to the patch? Imo
always better to submit them up-front / separately.

>          },
>      };
>  
> @@ -394,4 +395,67 @@ IN BGP(REJECT) -log crit -source 1.2.3.4
>          assert!(empty_config.config.ipsets.is_empty());
>          assert!(empty_config.config.groups.is_empty());
>      }
> +
> +    #[test]
> +    fn test_parse_config_comments() {
> +        const CONFIG: &str = r#"
> +# ignore lines that start with a # symbol
> +[OPTIONS]
> +[ALIASES]
> +
> +alias1 192.168.0.1 # comment # on alias1
> +
> +[IPSET ipset1] # comment # on ipset1
> +
> +dc/alias1 # comment # on ipset1 #1
> +
> +[RULES]
> +
> +GROUP testgroup # comment # on rule #1
> +IN ACCEPT -p udp -dport 1234 -log nolog # comment # on rule #2
> +
> +[group testgroup] # comment # on testgroup
> +
> +IN ACCEPT -source dc/alias1 -p tcp -sport 1111 -log nolog # comment # on testgroup #1
> +
> +"#;
> +
> +        let mut config = CONFIG.as_bytes();
> +        let config = Config::parse(&mut config).unwrap();
> +
> +        assert_eq!(config.config.aliases.len(), 1);
> +        assert_eq!(
> +            config.config.aliases["alias1"].comment(),
> +            Some("comment # on alias1")
> +        );
> +
> +        assert_eq!(config.config.ipsets.len(), 1);
> +
> +        let ipset1 = &config.config.ipsets["ipset1"];
> +        assert_eq!(ipset1.comment.as_deref(), Some("comment # on ipset1"));
> +
> +        assert_eq!(ipset1.len(), 1);
> +        assert_eq!(ipset1[0].comment.as_deref(), Some("comment # on ipset1 #1"));
> +
> +        assert_eq!(config.config.rules.len(), 2);
> +        assert_eq!(
> +            config.config.rules[0].comment.as_deref(),
> +            Some("comment # on rule #1")
> +        );
> +        assert_eq!(
> +            config.config.rules[1].comment.as_deref(),
> +            Some("comment # on rule #2")
> +        );
> +
> +        assert_eq!(config.config.groups.len(), 1);
> +
> +        let entry = &config.config.groups["testgroup"];
> +        assert_eq!(entry.comment(), Some("comment # on testgroup"));
> +
> +        assert_eq!(entry.rules().len(), 1);
> +        assert_eq!(
> +            entry.rules()[0].comment.as_deref(),
> +            Some("comment # on testgroup #1")
> +        );
> +    }
>  }
> diff --git a/proxmox-ve-config/src/firewall/types/rule.rs b/proxmox-ve-config/src/firewall/types/rule.rs
> index 811256c..2648fa8 100644
> --- a/proxmox-ve-config/src/firewall/types/rule.rs
> +++ b/proxmox-ve-config/src/firewall/types/rule.rs
> @@ -115,7 +115,7 @@ impl FromStr for Rule {
>              bail!("rule must not contain any newlines!");
>          }
>  
> -        let (line, comment) = match input.rsplit_once('#') {
> +        let (line, comment) = match input.split_once('#') {
>              Some((line, comment)) if !comment.is_empty() => (line.trim(), Some(comment.trim())),
>              _ => (input.trim(), None),
>          };
> @@ -261,13 +261,15 @@ mod tests {
>  
>      #[test]
>      fn test_parse_rule() {
> -        let mut rule: Rule = "|GROUP tgr -i eth0 # acomm".parse().expect("valid rule");
> +        let mut rule: Rule = "|GROUP tgr -i eth0 # acomm #comment"
> +            .parse()
> +            .expect("valid rule");
>  
>          assert_eq!(
>              rule,
>              Rule {
>                  disabled: true,
> -                comment: Some("acomm".to_string()),
> +                comment: Some("acomm #comment".to_string()),
>                  kind: Kind::Group(RuleGroup {
>                      group: "tgr".to_string(),
>                      iface: Some("eth0".to_string()),