Re: [RFC proxmox 04/22] firewall-api-types: add logging types

[Stefan Hanreich <s.hanreich@proxmox.com>]

comments inline

On 2/16/26 11:45 AM, Dietmar Maurer wrote:
> This adds several firewall logging-related types:
> - FirewallLogLevel: enum for syslog-style log levels (emerg to nolog)
> - FirewallLogRateLimit: configuration for rate-limiting log messages
> - FirewallPacketRate: packet rate representation (e.g., '100/second')
> - FirewallPacketRateTimescale: time units for rate limiting
> 
> Includes comprehensive tests for parsing, display, and roundtrip
> serialization.
> 
> Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
> ---
>  proxmox-firewall-api-types/src/lib.rs |   5 +
>  proxmox-firewall-api-types/src/log.rs | 312 ++++++++++++++++++++++++++
>  2 files changed, 317 insertions(+)
>  create mode 100644 proxmox-firewall-api-types/src/log.rs
> 
> diff --git a/proxmox-firewall-api-types/src/lib.rs b/proxmox-firewall-api-types/src/lib.rs
> index b8004c76..d9ff4548 100644
> --- a/proxmox-firewall-api-types/src/lib.rs
> +++ b/proxmox-firewall-api-types/src/lib.rs
> @@ -1,2 +1,7 @@
> +mod log;
> +pub use log::{
> +    FirewallLogLevel, FirewallLogRateLimit, FirewallPacketRate, FirewallPacketRateTimescale,
> +};
> +
>  mod policy;
>  pub use policy::{FirewallFWPolicy, FirewallIOPolicy};
> diff --git a/proxmox-firewall-api-types/src/log.rs b/proxmox-firewall-api-types/src/log.rs
> new file mode 100644
> index 00000000..fb2df49e
> --- /dev/null
> +++ b/proxmox-firewall-api-types/src/log.rs
> @@ -0,0 +1,312 @@
> +use std::fmt;
> +use std::str::FromStr;
> +
> +use anyhow::{bail, Error};
> +use serde::{Deserialize, Serialize};
> +
> +use proxmox_schema::api;
> +
> +#[cfg(feature = "enum-fallback")]
> +use proxmox_fixed_string::FixedString;
> +
> +/// Firewall log rate limit time scales.
> +#[derive(Copy, Clone, Default, PartialEq)]
> +#[cfg_attr(test, derive(Debug))]
> +pub enum FirewallPacketRateTimescale {
> +    /// second
> +    #[default]
> +    Second,
> +    /// minute
> +    Minute,
> +    /// hour
> +    Hour,
> +    /// day
> +    Day,
> +    #[cfg(feature = "enum-fallback")]
> +    /// Unknown variants for forward compatibility.
> +    UnknownEnumValue(FixedString),
> +}
> +
> +impl FromStr for FirewallPacketRateTimescale {
> +    type Err = Error;
> +
> +    fn from_str(str: &str) -> Result<Self, Error> {
> +        match str {
> +            "second" => Ok(FirewallPacketRateTimescale::Second),
> +            "minute" => Ok(FirewallPacketRateTimescale::Minute),
> +            "hour" => Ok(FirewallPacketRateTimescale::Hour),
> +            "day" => Ok(FirewallPacketRateTimescale::Day),
> +            "" => bail!("empty time scale specification"),
> +            #[cfg(not(feature = "enum-fallback"))]
> +            _ => bail!("Invalid time scale provided"),
> +            #[cfg(feature = "enum-fallback")]
> +            other => Ok(FirewallPacketRateTimescale::UnknownEnumValue(
> +                FixedString::from_str(other)?,
> +            )),
> +        }
> +    }
> +}
> +
> +impl fmt::Display for FirewallPacketRateTimescale {
> +    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
> +        match self {
> +            FirewallPacketRateTimescale::Second => write!(f, "second"),
> +            FirewallPacketRateTimescale::Minute => write!(f, "minute"),
> +            FirewallPacketRateTimescale::Hour => write!(f, "hour"),
> +            FirewallPacketRateTimescale::Day => write!(f, "day"),
> +            #[cfg(feature = "enum-fallback")]
> +            &FirewallPacketRateTimescale::UnknownEnumValue(scale) => scale.fmt(f),
> +        }
> +    }
> +}
> +
> +/// Packet rate for log rate limiting.
> +#[derive(Copy, Clone, PartialEq)]
> +#[cfg_attr(test, derive(Debug))]
> +pub struct FirewallPacketRate {
> +    /// Number of packets
> +    pub packets: u64,
> +    /// Time scale for the rate
> +    pub timescale: FirewallPacketRateTimescale,
> +}
> +
> +serde_plain::derive_deserialize_from_fromstr!(FirewallPacketRate, "valid packet rate");
> +serde_plain::derive_serialize_from_display!(FirewallPacketRate);

we have similar macros in proxmox-serde as well [1]. Would potentially
save an external dependency? applies to all instances in this patch
series ofc.

[1]
https://git.proxmox.com/?p=proxmox.git;a=blob;f=proxmox-serde/src/serde_macros.rs;h=9d88218361c805cd5b2fa6056e0c49dda9636fb2;hb=HEAD

> +impl fmt::Display for FirewallPacketRate {
> +    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
> +        write!(f, "{}/{}", self.packets, self.timescale)
> +    }
> +}
> +
> +impl FromStr for FirewallPacketRate {
> +    type Err = Error;
> +
> +    fn from_str(str: &str) -> Result<Self, Error> {
> +        match str.split_once('/') {
> +            None => Ok(FirewallPacketRate {
> +                packets: u64::from_str(str)?,
> +                timescale: FirewallPacketRateTimescale::default(),
> +            }),
> +            Some((rate, unit)) => Ok(FirewallPacketRate {
> +                packets: u64::from_str(rate)?,
> +                timescale: FirewallPacketRateTimescale::from_str(unit)?,
> +            }),
> +        }
> +    }
> +}
> +
> +#[api(
> +    default_key: "enable",
> +    properties: {
> +        burst: {
> +            default: 5,
> +            minimum: 0,
> +            optional: true,
> +            type: Integer,
> +        },
> +        enable: {
> +            default: true,
> +        },
> +        rate: {
> +            default: "1/second",
> +            optional: true,
> +            type: String,
> +        },
> +    },
> +)]
> +/// Firewall log rate limit configuration.
> +#[derive(Deserialize, Serialize, Clone, PartialEq)]
> +#[cfg_attr(test, derive(Debug))]
> +pub struct FirewallLogRateLimit {
> +    /// Initial burst of packages which will always get logged before the rate
> +    /// is applied
> +    #[serde(deserialize_with = "proxmox_serde::perl::deserialize_u64")]
> +    #[serde(default, skip_serializing_if = "Option::is_none")]
> +    pub burst: Option<u64>,
> +
> +    /// Enable or disable log rate limiting
> +    #[serde(deserialize_with = "proxmox_serde::perl::deserialize_bool")]
> +    pub enable: bool,
> +
> +    /// Frequency with which the burst bucket gets refilled
> +    #[serde(default, skip_serializing_if = "Option::is_none")]
> +    pub rate: Option<FirewallPacketRate>,
> +}
> +
> +#[api]
> +/// Firewall log levels.
> +#[derive(Clone, Copy, Debug, Default, Eq, PartialEq, Deserialize, Serialize)]
> +pub enum FirewallLogLevel {
> +    #[serde(rename = "emerg")]
> +    /// emerg.
> +    Emergency,
> +    #[serde(rename = "alert")]
> +    /// alert.
> +    Alert,
> +    #[serde(rename = "crit")]
> +    /// crit.
> +    Critical,
> +    #[serde(rename = "err")]
> +    /// err.
> +    Error,
> +    #[serde(rename = "warning")]
> +    /// warning.
> +    Warning,
> +    #[serde(rename = "notice")]
> +    /// notice.
> +    Notice,
> +    #[serde(rename = "info")]
> +    /// info.
> +    Info,
> +    #[serde(rename = "debug")]
> +    /// debug.
> +    Debug,
> +    #[serde(rename = "nolog")]
> +    #[default]
> +    /// nolog.
> +    Nolog,
> +}
> +
> +serde_plain::derive_display_from_serialize!(FirewallLogLevel);
> +serde_plain::derive_fromstr_from_deserialize!(FirewallLogLevel);
> +
> +#[cfg(test)]
> +mod tests {
> +    use proxmox_schema::property_string::PropertyString;
> +
> +    use super::*;
> +
> +    #[test]
> +    fn test_parse_rate_limit() {
> +        let mut parsed_rate_limit: PropertyString<FirewallLogRateLimit> =
> +            serde_plain::from_str("1,burst=123,rate=44").expect("valid rate limit");
> +
> +        assert_eq!(
> +            parsed_rate_limit.into_inner(),
> +            FirewallLogRateLimit {
> +                enable: true,
> +                burst: Some(123),
> +                rate: Some(FirewallPacketRate {
> +                    packets: 44,
> +                    timescale: FirewallPacketRateTimescale::Second,
> +                }),
> +            }
> +        );
> +
> +        parsed_rate_limit = serde_plain::from_str("1").expect("valid rate limit");
> +
> +        assert_eq!(
> +            parsed_rate_limit.into_inner(),
> +            FirewallLogRateLimit {
> +                enable: true,
> +                burst: None,
> +                rate: None
> +            }
> +        );
> +
> +        parsed_rate_limit =
> +            serde_plain::from_str("enable=0,rate=123/hour").expect("valid rate limit");
> +
> +        assert_eq!(
> +            parsed_rate_limit.into_inner(),
> +            FirewallLogRateLimit {
> +                enable: false,
> +                burst: None,
> +                rate: Some(FirewallPacketRate {
> +                    packets: 123,
> +                    timescale: FirewallPacketRateTimescale::Hour,
> +                }),
> +            }
> +        );
> +
> +        serde_plain::from_str::<PropertyString<FirewallLogRateLimit>>("2")
> +            .expect_err("invalid value for enable");
> +
> +        serde_plain::from_str::<PropertyString<FirewallLogRateLimit>>("enabled=0,rate=123")
> +            .expect_err("invalid key in log ratelimit");
> +
> +        #[cfg(not(feature = "enum-fallback"))]
> +        serde_plain::from_str::<PropertyString<FirewallLogRateLimit>>("enable=0,rate=123/proxmox,")
> +            .expect_err("invalid unit for rate");
> +    }
> +
> +    #[test]
> +    fn test_packet_rate_parse() {
> +        // Test parsing with all timescales
> +        let rate: FirewallPacketRate = "100/second".parse().expect("valid rate");
> +        assert_eq!(rate.packets, 100);
> +        assert_eq!(rate.timescale, FirewallPacketRateTimescale::Second);
> +
> +        let rate: FirewallPacketRate = "50/minute".parse().expect("valid rate");
> +        assert_eq!(rate.packets, 50);
> +        assert_eq!(rate.timescale, FirewallPacketRateTimescale::Minute);
> +
> +        let rate: FirewallPacketRate = "10/hour".parse().expect("valid rate");
> +        assert_eq!(rate.packets, 10);
> +        assert_eq!(rate.timescale, FirewallPacketRateTimescale::Hour);
> +
> +        let rate: FirewallPacketRate = "1/day".parse().expect("valid rate");
> +        assert_eq!(rate.packets, 1);
> +        assert_eq!(rate.timescale, FirewallPacketRateTimescale::Day);
> +
> +        // Test default timescale when no unit specified
> +        let rate: FirewallPacketRate = "42".parse().expect("valid rate without unit");
> +        assert_eq!(rate.packets, 42);
> +        assert_eq!(rate.timescale, FirewallPacketRateTimescale::Second);
> +    }
> +
> +    #[test]
> +    fn test_packet_rate_display() {
> +        let rate = FirewallPacketRate {
> +            packets: 100,
> +            timescale: FirewallPacketRateTimescale::Second,
> +        };
> +        assert_eq!(rate.to_string(), "100/second");
> +
> +        let rate = FirewallPacketRate {
> +            packets: 5,
> +            timescale: FirewallPacketRateTimescale::Hour,
> +        };
> +        assert_eq!(rate.to_string(), "5/hour");
> +    }
> +
> +    #[test]
> +    fn test_packet_rate_roundtrip() {
> +        let original = FirewallPacketRate {
> +            packets: 123,
> +            timescale: FirewallPacketRateTimescale::Minute,
> +        };
> +        let serialized = original.to_string();
> +        let parsed: FirewallPacketRate = serialized.parse().expect("roundtrip parse");
> +        assert_eq!(original, parsed);
> +    }
> +
> +    #[test]
> +    fn test_packet_rate_parse_errors() {
> +        // Empty timescale
> +        "100/"
> +            .parse::<FirewallPacketRate>()
> +            .expect_err("empty timescale");
> +
> +        // Invalid timescale
> +        #[cfg(not(feature = "enum-fallback"))]
> +        "100/invalid"
> +            .parse::<FirewallPacketRate>()
> +            .expect_err("invalid timescale");
> +        #[cfg(feature = "enum-fallback")]
> +        "100/invalid"
> +            .parse::<FirewallPacketRate>()
> +            .expect("valid timescale (enum fallback feature)");
> +
> +        // Invalid packet count
> +        "abc/second"
> +            .parse::<FirewallPacketRate>()
> +            .expect_err("invalid packet count");
> +
> +        // Negative number
> +        "-5/second"
> +            .parse::<FirewallPacketRate>()
> +            .expect_err("negative packet count");
> +    }
> +}