From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 455431FF13F for ; Thu, 12 Mar 2026 11:18:17 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 65E7FE4C6; Thu, 12 Mar 2026 11:18:13 +0100 (CET) Date: Thu, 12 Mar 2026 11:17:39 +0100 From: Wolfgang Bumiller To: Dietmar Maurer Subject: Re: [RFC proxmox 03/22] firewall-api-types: add firewall policy types Message-ID: References: <20260216104401.3959270-1-dietmar@proxmox.com> <20260216104401.3959270-4-dietmar@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20260216104401.3959270-4-dietmar@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1773310623423 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.082 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_MSPIKE_H2 0.001 Average reputation (+2) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: 7WHBV44V2BWUZ46PD7QEOTHV42JW3IAO X-Message-ID-Hash: 7WHBV44V2BWUZ46PD7QEOTHV42JW3IAO X-MailFrom: w.bumiller@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: pve-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Mon, Feb 16, 2026 at 11:43:41AM +0100, Dietmar Maurer wrote: > Generated from perl api. > > Signed-off-by: Dietmar Maurer > --- > 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::("REJECT") > + .expect_err("REJECT is not valid for FWPolicy"); Without `serde_plain` these types of tests could all be of the form: "REJECT" .parse::() .expect_err("REJECT is not valid for FWPolicy"); > + serde_plain::from_str::("accept") > + .expect_err("lowercase should be invalid"); > + serde_plain::from_str::("").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::("ALLOW").expect_err("ALLOW is not valid"); > + serde_plain::from_str::("drop").expect_err("lowercase should be invalid"); > + serde_plain::from_str::("").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