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

["Michael Köppl" <m.koeppl@proxmox.com>]

Gave this another quick spin, focusing on the changes since v4. The more
important issues I reported yesterday are fixed now. Also had a look at
the implementation (taking into account review comments from previous
versions) and overall this version lgtm! Please consider this:

Reviewed-by: Michael Köppl <m.koeppl@proxmox.com>
Tested-by: Michael Köppl <m.koeppl@proxmox.com>

On Thu Nov 27, 2025 at 10:21 AM CET, Hannes Laimer wrote:
> 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.
> 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/
>
> v5, thanks @Michael
>  - use correct default if node does not have firewall enable set at all
>  - ui: also refresh rules panel
>
> v4, thanks @Stefan
>  * UI:
>    - fix sorting, now stable
>    - fix problem with dirty-detection for the log-ratelimit field(s)
>    - keep rules selection during tree refresh
>    - properly show errors
>    - minior code improvements
>    - cache columns for rules tablem don't re-create them every time
>  * handfull of non-specific, small code improvments I've noticed throughout
>    the series
>  * still needs [2] for the reason mentioned in v3
>
>
> 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 [2], 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
>  
> [2] 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 [3] anymore, since it was applied already )
>
> [3] 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 | 1363 +++++++++++++++++++++++++-
>  4 files changed, 1587 insertions(+), 397 deletions(-)
>
>
> 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        | 857 ++++++++++++++++++++++++++
>  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    | 153 +++++
>  ui/src/remotes/firewall/mod.rs        |  30 +
>  ui/src/remotes/firewall/tree.rs       | 673 ++++++++++++++++++++
>  ui/src/remotes/firewall/types.rs      | 284 +++++++++
>  ui/src/remotes/firewall/ui_helpers.rs | 182 ++++++
>  ui/src/remotes/mod.rs                 |  10 +
>  14 files changed, 2503 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
>
>
> 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        | 495 ++++++++++++++++++++++++++++
>  src/firewall/rules.rs               | 275 ++++++++++++++++
>  src/form/mod.rs                     |  70 ++++
>  src/lib.rs                          |   3 +
>  7 files changed, 1330 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
>
>
> Summary over all repositories:
>   25 files changed, 5420 insertions(+), 397 deletions(-)



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