Re: [RFC proxmox 03/22] firewall-api-types: add firewall policy types

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Mon, Feb 16, 2026 at 11:43:41AM +0100, Dietmar Maurer wrote:
> Generated from perl api.
> 
> Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
> ---
>  proxmox-firewall-api-types/Cargo.toml    |   5 +-
>  proxmox-firewall-api-types/src/lib.rs    |   3 +-
>  proxmox-firewall-api-types/src/policy.rs | 151 +++++++++++++++++++++++
>  3 files changed, 157 insertions(+), 2 deletions(-)
>  create mode 100644 proxmox-firewall-api-types/src/policy.rs
> 
> diff --git a/proxmox-firewall-api-types/Cargo.toml b/proxmox-firewall-api-types/Cargo.toml
> index 515d1efc..3122d813 100644
> --- a/proxmox-firewall-api-types/Cargo.toml
> +++ b/proxmox-firewall-api-types/Cargo.toml
> @@ -9,12 +9,15 @@ license.workspace = true
>  repository.workspace = true
>  exclude.workspace = true
>  
> +[features]
> +enum-fallback = ["dep:proxmox-fixed-string"]
> +
>  [dependencies]
>  anyhow.workspace = true
>  regex.workspace = true
> -proxmox-fixed-string.workspace = true
>  
>  serde = { workspace = true, features = [ "derive" ] }
>  serde_plain = { workspace = true }

You could replace this with `proxmox-serde`. We also already had
`serde-with` in some places for the same purpose (which now use
`proxmox-serde` I think).

>  proxmox-schema = { workspace = true, features = ["api-macro"] }
>  proxmox-serde = { workspace = true, features = ["perl"] }
> +proxmox-fixed-string = { workspace = true, optional = true }
> diff --git a/proxmox-firewall-api-types/src/lib.rs b/proxmox-firewall-api-types/src/lib.rs
> index 7c4a64a7..b8004c76 100644
> --- a/proxmox-firewall-api-types/src/lib.rs
> +++ b/proxmox-firewall-api-types/src/lib.rs
> @@ -1 +1,2 @@
> -// TODO: add code here
> +mod policy;
> +pub use policy::{FirewallFWPolicy, FirewallIOPolicy};
> diff --git a/proxmox-firewall-api-types/src/policy.rs b/proxmox-firewall-api-types/src/policy.rs
> new file mode 100644
> index 00000000..274fbe20
> --- /dev/null
> +++ b/proxmox-firewall-api-types/src/policy.rs
> @@ -0,0 +1,151 @@
> +use serde::{Deserialize, Serialize};
> +
> +#[cfg(feature = "enum-fallback")]
> +use proxmox_fixed_string::FixedString;
> +use proxmox_schema::api;
> +
> +#[api]
> +/// Firewall forward policy.
> +#[derive(Clone, Copy, Debug, Default, Eq, PartialEq, Deserialize, Serialize)]
> +pub enum FirewallFWPolicy {
> +    #[serde(rename = "ACCEPT")]
> +    #[default]
> +    /// ACCEPT.
> +    Accept,
> +    #[serde(rename = "DROP")]
> +    /// DROP.
> +    Drop,
> +    #[cfg(feature = "enum-fallback")]
> +    #[serde(untagged)]
> +    /// Unknwon
> +    UnknownEnumValue(FixedString),
> +}
> +serde_plain::derive_display_from_serialize!(FirewallFWPolicy);
> +serde_plain::derive_fromstr_from_deserialize!(FirewallFWPolicy);
> +
> +#[api]
> +/// Firewall IO policy.
> +#[derive(Clone, Copy, Debug, Eq, PartialEq, Deserialize, Serialize)]
> +pub enum FirewallIOPolicy {
> +    #[serde(rename = "ACCEPT")]
> +    /// ACCEPT.
> +    Accept,
> +    #[serde(rename = "DROP")]
> +    /// DROP.
> +    Drop,
> +    #[serde(rename = "REJECT")]
> +    /// REJECT.
> +    Reject,
> +    #[cfg(feature = "enum-fallback")]
> +    #[serde(untagged)]
> +    /// Unknwon
> +    UnknownEnumValue(FixedString),
> +}
> +serde_plain::derive_display_from_serialize!(FirewallIOPolicy);
> +serde_plain::derive_fromstr_from_deserialize!(FirewallIOPolicy);
> +
> +#[cfg(test)]
> +mod tests {
> +    use super::*;
> +
> +    #[test]
> +    fn test_firewall_fw_policy_default() {
> +        assert_eq!(FirewallFWPolicy::default(), FirewallFWPolicy::Accept);
> +    }
> +
> +    #[test]
> +    #[cfg(not(feature = "enum-fallback"))]
> +    fn test_firewall_fw_policy_serde_invalid() {
> +        serde_plain::from_str::<FirewallFWPolicy>("REJECT")
> +            .expect_err("REJECT is not valid for FWPolicy");

Without `serde_plain` these types of tests could all be of the form:

    "REJECT"
        .parse::<FirewallFWPolicy>()
        .expect_err("REJECT is not valid for FWPolicy");

> +        serde_plain::from_str::<FirewallFWPolicy>("accept")
> +            .expect_err("lowercase should be invalid");
> +        serde_plain::from_str::<FirewallFWPolicy>("").expect_err("empty string should be invalid");
> +    }
> +
> +    #[test]
> +    fn test_firewall_fw_policy_roundtrip() {

This tests Display and FromStr implementations generated by a crate
specifically meant to add these to a simple enum.
Testing this is the job of that external crate.

(After all, we don't test whether the implementation generated by
`#[derive(PartialEq)]` does the correct thing either for all the enum
values...)

> +        for policy in [FirewallFWPolicy::Accept, FirewallFWPolicy::Drop] {
> +            let serialized = serde_plain::to_string(&policy).expect("serialize");
> +            let parsed: FirewallFWPolicy =
> +                serde_plain::from_str(&serialized).expect("roundtrip parse");
> +            assert_eq!(policy, parsed);
> +        }
> +    }
> +
> +    #[test]
> +    fn test_firewall_fw_policy_serde() {

Similar to the above. If we want to maintain a list of "this is the
value perl uses" *in addition to `#[serde(rename)]`, then why not
iterate through an array of `(FirewallFWPolicy::Foo, "FOO")` tuples.

But that's just another list to maintain - if anything, we should
involve pve-api-types in this without having to manually maintain
the wire foramt in multiple places like this.

> +        let accept = FirewallFWPolicy::Accept;
> +        let serialized = serde_plain::to_string(&accept).expect("serialize");
> +        assert_eq!(serialized, "ACCEPT");
> +
> +        let parsed: FirewallFWPolicy = serde_plain::from_str(&serialized).expect("deserialize");
> +        assert_eq!(parsed, accept);
> +
> +        let drop = FirewallFWPolicy::Drop;
> +        let serialized = serde_plain::to_string(&drop).expect("serialize");
> +        assert_eq!(serialized, "DROP");
> +
> +        let parsed: FirewallFWPolicy = serde_plain::from_str(&serialized).expect("deserialize");
> +        assert_eq!(parsed, drop);
> +    }
> +
> +    #[test]
> +    #[cfg(feature = "enum-fallback")]
> +    fn test_firewall_fw_policy_serde_enum_fallback() {

Meh... fine... but technically the job of the `serde` crate to test that
`serde(untagged)` on an enum actually works...

> +        let unknown = FirewallFWPolicy::UnknownEnumValue(FixedString::new("TEST").unwrap());
> +        let serialized = serde_plain::to_string(&unknown).expect("serialize");

And could use `.to_string()` instead of serde_plain here, too.

> +        assert_eq!(serialized, "TEST");
> +
> +        let parsed: FirewallFWPolicy = serde_plain::from_str(&serialized).expect("deserialize");

and then `serialized.parse().expect(…)`.

> +        assert_eq!(parsed, unknown);
> +    }
> +
> +    #[test]
> +    #[cfg(not(feature = "enum-fallback"))]
> +    fn test_firewall_io_policy_serde_invalid() {
> +        serde_plain::from_str::<FirewallIOPolicy>("ALLOW").expect_err("ALLOW is not valid");
> +        serde_plain::from_str::<FirewallIOPolicy>("drop").expect_err("lowercase should be invalid");
> +        serde_plain::from_str::<FirewallIOPolicy>("").expect_err("empty string should be invalid");
> +    }
> +
> +    #[test]
> +    fn test_firewall_io_policy_roundtrip() {

Like for FW policy.

> +        for policy in [
> +            FirewallIOPolicy::Accept,
> +            FirewallIOPolicy::Drop,
> +            FirewallIOPolicy::Reject,
> +        ] {
> +            let serialized = serde_plain::to_string(&policy).expect("serialize");
> +            let parsed: FirewallIOPolicy =
> +                serde_plain::from_str(&serialized).expect("roundtrip parse");
> +            assert_eq!(policy, parsed);
> +        }
> +    }
> +
> +    #[test]
> +    fn test_firewall_io_policy_serde() {
> +        for (policy, expected) in [
> +            (FirewallIOPolicy::Accept, "ACCEPT"),
> +            (FirewallIOPolicy::Drop, "DROP"),
> +            (FirewallIOPolicy::Reject, "REJECT"),

So here we do use a list, but the other argument from the FW policy
variant still stands :S

> +        ] {
> +            let serialized = serde_plain::to_string(&policy).expect("serialize");
> +            assert_eq!(serialized, expected);
> +
> +            let parsed: FirewallIOPolicy = serde_plain::from_str(&serialized).expect("deserialize");
> +            assert_eq!(parsed, policy);
> +        }
> +    }
> +
> +    #[test]
> +    #[cfg(feature = "enum-fallback")]
> +    fn test_firewall_io_policy_serde_enum_fallback() {
> +        let unknown = FirewallIOPolicy::UnknownEnumValue(FixedString::new("TEST").unwrap());
> +        let serialized = serde_plain::to_string(&unknown).expect("serialize");
> +        assert_eq!(serialized, "TEST");
> +
> +        let parsed: FirewallIOPolicy = serde_plain::from_str(&serialized).expect("deserialize");
> +        assert_eq!(parsed, unknown);
> +    }
> +}
> -- 
> 2.47.3