[pdm-devel] [PATCH proxmox-yew-comp v2 4/4] firewall: add rules table
Hannes Laimer
h.laimer at proxmox.com
Mon Nov 10 15:51:58 CET 2025
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 <h.laimer at proxmox.com>
>> ---
>> 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<Callback<()>>,
>> +}
>> +
>> +impl FirewallRules {
>> + pub fn cluster(remote: impl Into<AttrValue>) -> Self {
>> + yew::props!(Self {
>> + context: FirewallContext::cluster(remote),
>> + })
>> + }
>> +
>> + pub fn node(remote: impl Into<AttrValue>, node: impl Into<AttrValue>) -> Self {
>> + yew::props!(Self {
>> + context: FirewallContext::node(remote, node),
>> + })
>> + }
>> +
>> + pub fn guest(
>> + remote: impl Into<AttrValue>,
>> + node: impl Into<AttrValue>,
>> + vmid: u64,
>> + vmtype: impl Into<AttrValue>,
>> + ) -> 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<pve_api_types::ListFirewallRules>,
>> + loader: Loader<Vec<pve_api_types::ListFirewallRules>>,
>> + _listener: SharedStateObserver<LoaderState<Vec<pve_api_types::ListFirewallRules>>>,
>> +}
>> +
>> +fn pill(text: impl Into<AttrValue>) -> 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<VNode> = 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::<Html>()
>> +}
>> +
>> +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<Vec<DataTableHeader<pve_api_types::ListFirewallRules>>> {
>> + 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! {<i class="fa fa-check"></i>},
>> + Some(0) | None => html! {<i class="fa fa fa-minus"></i>},
>
> 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>) -> 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<Self>, msg: Self::Message) -> bool {
>> + match msg {
>> + FirewallMsg::DataChange => {
>> + self.update_data();
>> + true
>> + }
>> + }
>> + }
>> +
>> + fn view(&self, _ctx: &Context<Self>) -> 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<FirewallRules> for VNode {
>> + fn from(val: FirewallRules) -> Self {
>> + let comp = VComp::new::<ProxmoxFirewallRules>(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 at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel
>
>
More information about the pdm-devel
mailing list