Re: [pdm-devel] [PATCH proxmox{, -yew-comp, -datacenter-manager} v3 00/12] add basic integration of PVE firewall

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

Hi!

bit late to the party (sorry!) - but finally gave this a spin today.
Some things I noticed while testing:

* Refreshing while an entry is selected resets the rule panel to 'no
  entry selected'

* The panel shows rules referencing security groups, but there is no way
  of seeing the contents of the security group
  Potentially tricky to implement with how the current PVE API looks
  like, I suppose, but would be a nice addition in the future imo?

* nit: can submit a form with no changes - but clicking reset disables
  updating (potentially an issue with change detection?)

* Order of elements in the left panel sometimes shuffled on reload

* Not sure I like that the whole components gets padding depending on
  the collapsed state of the right panel

There are quite a few firewall types in proxmox-ve-rs already, we might
want to make an effort to de-duplicate some of those types potentially?
LogRateLimit came to mind, for instance - but I'm sure there's more.
Would avoid potential subtle differences and improve maintainability imo.

Have some minor stuff w.r.t the code for the individual patches, nothing
critical. Although rendering error messages would be a quite nice
addition imo.


On 11/10/25 6:25 PM, Hannes Laimer wrote:
> This adds a basic UI for displaying the status of the firewall on remotes,
> nodes and guests in a tree. Status includes whether the firewall is
> enabled and the count of enabled rules. These rules are also shown in a
> panel once an enetity in the tree is selected. Firewall options can be
> edited, most useful is probably enable/disable, but generally all
> options are exposed(since we had the types anyway).
> 
> Generally loading the status involves 2 requests per entity, so the PDM
> server has to do quite a bit of work collecting all the relevant data.
> That is the reason we have multiple status endpoints
>  - for all pve remotes
>  - for a specific remote
>  - for a specific node
> a bit more context on the commit adding these endpoints. With these we
> can limit the number of requests the PDM potentially has to do. In this
> context a cache could also make sense, should be somewhat straight
> forward integrating something like Dominik proposed in [1]. But since
> these are configs, caches would have to be really short lived, but still,
> they could help with different useres requesting the same data at close
> to the same time.
> 
> Firewall options edit form and the firewall rules tables were added to
> yew-comp as they are not necesarrily PDM specific. I tried having them
> in a way so it would not be too complicated reusing them in other places
> at some point.
> 
> This also includes an updated pve-api.json, some api endpoint specs did
> require minor adjustments so they'd work with the type generator. This
> includes the not yet applied changes in [2]. Generally this is build
> with the latest master of proxmox-yew-comp and proxmox-yew-widget-toolkit.
> 
> Notes: node or guest firewalls could be enabled, but end up being masked
> by the cluster setting. I tried visualizing that by having the checkmark
> normal if masked and green if not.
> 
> [1] https://lore.proxmox.com/pdm-devel/20251017120315.2723235-1-d.csapak@proxmox.com/
> [2] https://lore.proxmox.com/pve-devel/20251023141546.105302-1-h.laimer@proxmox.com/T/#u
> 
> 
> v3, thanks @Lukas and @Michael
>  * UI:
>    - fixed(as in !dynamic and as in !problem anymore) rule status text col width
>    - align option edit form fields with the ones present in the current
>      PVE ui
>    - set defaults for checkboxes, note: this needs [3], without it
>      having a default defined for checkboxes leads to the form being
>      marked as dirty immediately
>    - add missing .max(99) to field
>    - only show remotes of type `pve` in remote filter
>  * add doc strings to pub stuff, also made some thing private
>  * drop default value for firewall IO policy and fix default for forward
>    policy
>  * fixed problem with how cluster firewall enable field was mapped from
>    int to a bool
>  
> [3] https://lore.proxmox.com/yew-devel/20251110161831.261526-1-h.laimer@proxmox.com/T/#u
> 
> 
> v2, thanks a lot @Dominik, @Lukas and @Thomas
>  * rebased onto master
>  * UI improvements
>    - move filters into tree panel
>    - shrink status tree panel
>    - the firewall rules table now doesn't always show all the columns,
>      instead we have a new column that shows only the things that are
>      set. We save a lot of space like that, also, most of the columns are
>      empty.
>    - added toggle button that collapses the status tree and shows the
>      rules tables "full-screen". With the current UI changes this should
>      not really be needed unless a really small screen is used.
>      Nontheless it may be useful, so I kept it it.
>    - for the cluster options form I put a border around the log ratelimit
>      fields, that should help separating them from the rest of the
>      options.
>  * concurrently fetch status data for `all remotes` and `single remote`,
>    was sequential in v1
> (* this doesn't include [4] anymore, since it was applied already )
> 
> [4] https://git.proxmox.com/?p=proxmox.git;a=commit;h=eb41684db1a6d13f4ae3d95761e40db5a7c333ce
> 
> 
> proxmox:
> 
> Hannes Laimer (4):
>   pve-api-types: update pve-api.json
>   pve-api-types: add get/update firewall options endpoints
>   pve-api-types: add list firewall rules endpoints
>   pve-api-types: regenerate
> 
>  pve-api-types/generate.pl            |   53 +
>  pve-api-types/pve-api.json           |  362 +------
>  pve-api-types/src/generated/code.rs  |  206 +++-
>  pve-api-types/src/generated/types.rs | 1367 ++++++++++++++++++++++++--
>  4 files changed, 1583 insertions(+), 405 deletions(-)
> 
> 
> proxmox-yew-comp:
> 
> Hannes Laimer (4):
>   form: add helpers for extractig data out of schemas
>   firewall: add FirewallContext
>   firewall: add options edit form
>   firewall: add rules table
> 
>  src/firewall/context.rs             | 142 +++++++++
>  src/firewall/log_ratelimit_field.rs | 334 ++++++++++++++++++++
>  src/firewall/mod.rs                 |  11 +
>  src/firewall/options_edit.rs        | 458 ++++++++++++++++++++++++++++
>  src/firewall/rules.rs               | 278 +++++++++++++++++
>  src/form/mod.rs                     |  70 +++++
>  src/lib.rs                          |   3 +
>  7 files changed, 1296 insertions(+)
>  create mode 100644 src/firewall/context.rs
>  create mode 100644 src/firewall/log_ratelimit_field.rs
>  create mode 100644 src/firewall/mod.rs
>  create mode 100644 src/firewall/options_edit.rs
>  create mode 100644 src/firewall/rules.rs
> 
> 
> proxmox-datacenter-manager:
> 
> Hannes Laimer (4):
>   pdm-api-types: add firewall status types
>   api: firewall: add option, rules and status endpoints
>   pdm-client: add api methods for firewall options, rules and status
>     endpoints
>   ui: add firewall status tree
> 
>  lib/pdm-api-types/src/firewall.rs     | 173 ++++++
>  lib/pdm-api-types/src/lib.rs          |   2 +
>  lib/pdm-client/src/lib.rs             | 133 ++++
>  server/src/api/pve/firewall.rs        | 858 ++++++++++++++++++++++++++
>  server/src/api/pve/lxc.rs             |   1 +
>  server/src/api/pve/mod.rs             |   3 +
>  server/src/api/pve/node.rs            |   1 +
>  server/src/api/pve/qemu.rs            |   1 +
>  ui/src/remotes/firewall/columns.rs    | 154 +++++
>  ui/src/remotes/firewall/mod.rs        |  30 +
>  ui/src/remotes/firewall/tree.rs       | 662 ++++++++++++++++++++
>  ui/src/remotes/firewall/types.rs      | 284 +++++++++
>  ui/src/remotes/firewall/ui_helpers.rs | 166 +++++
>  ui/src/remotes/mod.rs                 |  10 +
>  14 files changed, 2478 insertions(+)
>  create mode 100644 lib/pdm-api-types/src/firewall.rs
>  create mode 100644 server/src/api/pve/firewall.rs
>  create mode 100644 ui/src/remotes/firewall/columns.rs
>  create mode 100644 ui/src/remotes/firewall/mod.rs
>  create mode 100644 ui/src/remotes/firewall/tree.rs
>  create mode 100644 ui/src/remotes/firewall/types.rs
>  create mode 100644 ui/src/remotes/firewall/ui_helpers.rs
> 
> 
> Summary over all repositories:
>   25 files changed, 5357 insertions(+), 405 deletions(-)
> 



_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel