From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id BA3181FF15E for ; Mon, 10 Nov 2025 15:51:49 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 32C9519758; Mon, 10 Nov 2025 15:52:35 +0100 (CET) Message-ID: Date: Mon, 10 Nov 2025 15:51:58 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox Datacenter Manager development discussion , Lukas Wagner References: <20251105163546.450094-1-h.laimer@proxmox.com> <20251105163546.450094-9-h.laimer@proxmox.com> Content-Language: en-US From: Hannes Laimer In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1762786296805 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.046 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 SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pdm-devel] [PATCH proxmox-yew-comp v2 4/4] firewall: add rules table X-BeenThere: pdm-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Datacenter Manager development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Datacenter Manager development discussion Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pdm-devel-bounces@lists.proxmox.com Sender: "pdm-devel" small comment inline, I'll change the rest for v3. Thanks for the review! On 11/7/25 13:27, Lukas Wagner wrote: > Looking great, I think this new style should work pretty well. > > General note, since this is a component library, the public structs/fns > should have doc comments. > > Some further notes inline. > > On Wed Nov 5, 2025 at 5:35 PM CET, Hannes Laimer wrote: >> Displays the list of firewall rules, this is read-only currently, so it >> doesn't include any buttons for editing or adding rules. >> >> Signed-off-by: Hannes Laimer >> --- >> src/firewall/mod.rs | 3 + >> src/firewall/rules.rs | 253 ++++++++++++++++++++++++++++++++++++++++++ >> src/lib.rs | 2 +- >> 3 files changed, 257 insertions(+), 1 deletion(-) >> create mode 100644 src/firewall/rules.rs >> >> diff --git a/src/firewall/mod.rs b/src/firewall/mod.rs >> index 379b958..8cc4977 100644 >> --- a/src/firewall/mod.rs >> +++ b/src/firewall/mod.rs >> @@ -4,5 +4,8 @@ pub use context::FirewallContext; >> mod options_edit; >> pub use options_edit::EditFirewallOptions; >> >> +mod rules; >> +pub use rules::FirewallRules; >> + >> mod log_ratelimit_field; >> pub use log_ratelimit_field::LogRatelimitField; >> diff --git a/src/firewall/rules.rs b/src/firewall/rules.rs >> new file mode 100644 >> index 0000000..0add7db >> --- /dev/null >> +++ b/src/firewall/rules.rs >> @@ -0,0 +1,253 @@ >> +use std::rc::Rc; >> + >> +use yew::html::{IntoEventCallback, IntoPropValue}; >> +use yew::virtual_dom::{Key, VComp, VNode}; >> + >> +use pwt::prelude::*; >> +use pwt::state::{Loader, LoaderState, SharedStateObserver, Store}; >> +use pwt::widget::data_table::{DataTable, DataTableColumn, DataTableHeader}; >> +use pwt::widget::Container; >> +use pwt_macros::builder; >> + >> +use super::context::FirewallContext; >> + >> +#[derive(Clone, PartialEq, Properties)] >> +#[builder] >> +pub struct FirewallRules { >> + #[builder(IntoPropValue, into_prop_value)] >> + pub context: FirewallContext, >> + >> + #[builder_cb(IntoEventCallback, into_event_callback, ())] >> + #[prop_or_default] >> + pub on_close: Option>, >> +} >> + >> +impl FirewallRules { >> + pub fn cluster(remote: impl Into) -> Self { >> + yew::props!(Self { >> + context: FirewallContext::cluster(remote), >> + }) >> + } >> + >> + pub fn node(remote: impl Into, node: impl Into) -> Self { >> + yew::props!(Self { >> + context: FirewallContext::node(remote, node), >> + }) >> + } >> + >> + pub fn guest( >> + remote: impl Into, >> + node: impl Into, >> + vmid: u64, >> + vmtype: impl Into, >> + ) -> Self { >> + yew::props!(Self { >> + context: FirewallContext::guest(remote, node, vmid, vmtype), >> + }) >> + } >> +} >> + >> +pub enum FirewallMsg { >> + DataChange, >> +} > > I *think* this one does not need to be public > we have ``` type Message = FirewallMsg; ``` in the component later, we'd leak the type if it was private >> + >> +#[doc(hidden)] >> +pub struct ProxmoxFirewallRules { > > Along with this one > >> + store: Store, >> + loader: Loader>, >> + _listener: SharedStateObserver>>, >> +} >> + >> +fn pill(text: impl Into) -> Container { >> + Container::from_tag("span") >> + .style("display", "inline-block") >> + .style("margin", "0 1px") >> + .style("background-color", "var(--pwt-color-neutral-container)") >> + .style("color", "var(--pwt-color-on-neutral-container)") >> + .style("border-radius", "var(--pwt-button-corner-shape)") >> + .style("padding-inline", "var(--pwt-spacer-2)") >> + .with_child(text.into()) >> +} >> + >> +fn format_firewall_rule(rule: &pve_api_types::ListFirewallRules) -> Html { >> + let mut parts: Vec = Vec::new(); >> + >> + if let Some(iface) = &rule.iface { >> + parts.push(pill(format!("iface: {iface}")).into()); >> + } >> + >> + if let Some(macro_name) = &rule.r#macro { >> + parts.push(pill(format!("macro: {}", macro_name)).into()); > > You can inline the variable into the format! here - same for the > format!s below. > >> + } >> + >> + if let Some(proto) = &rule.proto { >> + let mut proto_str = proto.to_uppercase(); >> + if proto.eq("icmp") || proto.eq("icmpv6") || proto.eq("ipv6-icmp") { > > It's more idiomatic to use `==` instead of .eq > >> + if let Some(icmp_type) = &rule.icmp_type { >> + proto_str = format!("{}, {}", proto_str, icmp_type); >> + } >> + } >> + parts.push(pill(tr!("proto: {}", proto_str)).into()); > > Any reason why this one specifically should be translated? > I'd be tempted to not translate any of these abbreviations for new until > somebody complains about it, but maybe others have different opinions > here. > >> + } >> + >> + match (&rule.source, &rule.sport) { >> + (Some(src), Some(sport)) => { >> + parts.push(pill(format!("src: {}, port: {}", src, sport)).into()); >> + } >> + (Some(src), None) => { >> + parts.push(pill(format!("src: {}", src)).into()); >> + } >> + (None, Some(sport)) => { >> + parts.push(pill(format!("src: any, port: {}", sport)).into()); > > I know it comes from the API type, but if possible, use something like > `source_port` or `src_port` as variable names. > >> + } >> + _ => {} >> + } >> + >> + match (&rule.dest, &rule.dport) { >> + (Some(dst), Some(dport)) => { >> + parts.push(pill(format!("dest: {}:, port: {}", dst, dport)).into()); > ^ > superfluous ':' here > >> + } >> + (Some(dst), None) => { >> + parts.push(pill(format!("dest: {}", dst)).into()); >> + } >> + (None, Some(dport)) => { >> + parts.push(pill(format!("dest: any, port: {}", dport)).into()); >> + } >> + _ => {} >> + } >> + >> + if parts.is_empty() { >> + return "-".into(); > > Personally I'd just leave the row completely empty if there are no > rules, but that's just a matter of personal preference, no need to > change if you think this is better. > >> + } >> + >> + parts >> + .into_iter() >> + .enumerate() >> + .flat_map(|(i, part)| { >> + if i == 0 { >> + vec![part] >> + } else { >> + vec![" ".into(), part] >> + } >> + }) >> + .collect::() >> +} >> + >> +impl ProxmoxFirewallRules { >> + fn update_data(&mut self) { >> + if let Some(Ok(data)) = &self.loader.read().data { >> + self.store.set_data((**data).clone()); >> + } >> + } >> + >> + fn columns() -> Rc>> { >> + Rc::new(vec![ >> + DataTableColumn::new("") >> + .width("30px") >> + .justify("right") >> + .show_menu(false) >> + .resizable(false) >> + .render(|rule: &pve_api_types::ListFirewallRules| html! {&rule.pos}) >> + .into(), >> + DataTableColumn::new(tr!("On")) >> + .width("40px") >> + .justify("center") >> + .resizable(false) >> + .render( >> + |rule: &pve_api_types::ListFirewallRules| match rule.enable { >> + Some(1) => html! {}, >> + Some(0) | None => html! {}, > > Any reason why you use the html! macro here and not Fa::new("check") or > Fa::new("minus")? > no :) >> + _ => html! {"-"}, >> + }, >> + ) >> + .into(), >> + DataTableColumn::new(tr!("Type")) >> + .width("80px") >> + .render(|rule: &pve_api_types::ListFirewallRules| html! {&rule.ty}) >> + .into(), >> + DataTableColumn::new(tr!("Action")) >> + .width("100px") >> + .render(|rule: &pve_api_types::ListFirewallRules| html! {&rule.action}) >> + .into(), >> + DataTableColumn::new(tr!("Rule")) >> + .flex(1) >> + .render(|rule: &pve_api_types::ListFirewallRules| format_firewall_rule(rule)) >> + .into(), >> + DataTableColumn::new(tr!("Comment")) >> + .width("150px") >> + .render(|rule: &pve_api_types::ListFirewallRules| { >> + rule.comment.as_deref().unwrap_or("-").into() >> + }) >> + .into(), >> + ]) >> + } >> +} >> + >> +impl Component for ProxmoxFirewallRules { >> + type Message = FirewallMsg; >> + type Properties = FirewallRules; >> + >> + fn create(ctx: &Context) -> Self { >> + let props = ctx.props(); >> + >> + let url: AttrValue = props.context.rules_url().into(); >> + >> + let store = Store::with_extract_key(|item: &pve_api_types::ListFirewallRules| { >> + Key::from(item.pos.to_string()) >> + }); >> + >> + let loader = Loader::new().loader({ >> + let url = url.clone(); >> + move || { >> + let url = url.clone(); >> + async move { crate::http_get(url.to_string(), None).await } >> + } >> + }); >> + >> + let _listener = loader.add_listener(ctx.link().callback(|_| FirewallMsg::DataChange)); >> + >> + loader.load(); >> + >> + let mut me = Self { >> + store, >> + loader, >> + _listener, >> + }; >> + >> + me.update_data(); >> + me >> + } >> + >> + fn update(&mut self, _ctx: &Context, msg: Self::Message) -> bool { >> + match msg { >> + FirewallMsg::DataChange => { >> + self.update_data(); >> + true >> + } >> + } >> + } >> + >> + fn view(&self, _ctx: &Context) -> Html { >> + self.loader.render(|_data| -> Html { >> + if self.store.data_len() == 0 { >> + Container::new() >> + .padding(2) >> + .with_child(tr!("No firewall rules configured")) >> + .into() >> + } else { >> + let columns = Self::columns(); >> + DataTable::new(columns, self.store.clone()) >> + .show_header(true) >> + .striped(true) >> + .into() >> + } >> + }) >> + } >> +} >> + >> +impl From for VNode { >> + fn from(val: FirewallRules) -> Self { >> + let comp = VComp::new::(Rc::new(val), None); >> + VNode::from(comp) >> + } >> +} >> diff --git a/src/lib.rs b/src/lib.rs >> index 852d65d..d7d8c7e 100644 >> --- a/src/lib.rs >> +++ b/src/lib.rs >> @@ -130,7 +130,7 @@ mod rrd_timeframe_selector; >> pub use rrd_timeframe_selector::{RRDTimeframe, RRDTimeframeSelector}; >> >> mod firewall; >> -pub use firewall::{EditFirewallOptions, FirewallContext}; >> +pub use firewall::{EditFirewallOptions, FirewallContext, FirewallRules}; >> >> mod running_tasks; >> pub use running_tasks::{ProxmoxRunningTasks, RunningTasks}; > > > > _______________________________________________ > pdm-devel mailing list > pdm-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel > > _______________________________________________ pdm-devel mailing list pdm-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel