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

["Lukas Wagner" <l.wagner@proxmox.com>]

On Thu Oct 30, 2025 at 3:33 PM CET, 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]. This also needs [3] to be
> present. 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
> [3] https://lore.proxmox.com/yew-devel/20251029173528.378487-1-h.laimer@proxmox.com/T/#u
>

Some general high-level notes about this, I have not yet checked the
code yet:

I'm not sure whether this 'side-by-side' view of the remote/node/guest tree
and the table for the firewall rules is a good idea. The table has 12
columns, none of which could be hidden by default without hiding
potentially crucial information. Even at a width of 1920 pixels, 3 1/2
columns are outside the view port and only accessible when scrolling
horizontally. Some of the columns are also too narrow by default, like
the ones for Source/Dest IP addresses, so the table might need even more
space than it already does.

Hannes and I briefly discussed the issue off-list already, here are some
of the ideas that popped up:

  - Hiding some columns: Not ideal, since potentially any column can
    contain important information about the rule

  - We could leave out the 'x out of y rules enabled' in the tree
    and make the tree as narrow as possible. I've played around with
    a 1:4 flex ratio, which *could* work - the table still barely
    fits, e.g. 'Comment' entries longer than one or two words would
    still overflow the view

  - Change the layout from two cards next to each other to two cards on
    top of each other: Possible, but not ideal, since then the tree view
    is limited in height - leading to more scrolling and possibly worse
    UX when having a large number of remotes/nodes/guests.

  - Show rule list in a dialog: The tree view would then become
    full-width (similar to the SDN view), with a dialog showing the
    ruleset that could be opened via some button.
    This could work, but could become a bit awkward once we want to
    support editing rules as well - since then you would spawn the "Edit
    dialog" from the "Rule List Dialog". We already have one other
    instance where we spawn a dialog from within another dialog
    (Fingerprint confirmation when adding a remote), but I think this
    should be avoided if possible.

  - Similar to the previous, but without a pop-up:
    Make the tree view full-width and add a
    'Show Rules >' button for each tree item in the furthest right
    column. If pressed, the tree view as a whole hides/collapses/moves to the
    left and yields space for a full-width list view for the firewall
    rules. Somewhere there then should be some "< Show Tree View" (or
    some other text) button, which can be pressed to hide the list view
    again and expand the tree view widget horizontally again.

    In (rather crude) ASCII art this could be:

	----------------------------------------------------------
	    * remote-a
	       + node-a                              (>)
		 * VM 100                            (>)
		 * VM 101                            (>)
	       + node-b                              (>)
	       + node-c                              (>)
	    * remote-b
	       + node-a                              (>)
	       + node-b                              (>)
	       + node-c                              (>)

	----------------------------------------------------------

	and when the (>) button is pressed, the entire view changes to the
	rule list. If (< back) is pressed, the tree is shown again.

	----------------------------------------------------------
	(< back)
	    
	   Enabled | Type | Action | Macro | Interface | Protocol | ...
	      x    | in   | ACCEPT |     - | vmbr0     | tcp      | ...

	----------------------------------------------------------


Some other things that I've noticed:

  - The check-mark or - in the Enabled column should be centered

  - A button to quickly go to the firewall rules page in the PVE Web UI
    would be nice, at least as long was we cannot edit rules in PDM
    directly.

  - Could make sense to add an explict rule numbering column, just to
    emphasize that the order of rules is important (same as in PVE).

  - In the 'Edit Cluster Firewall Dialog', there are two 'Enable'
    checkboxes, one says 'Enable Firewall' and the other one just 'Enable'.
    I assume the latter refers to rate limiting? IMO this one is a bit
    confusing and could use some polish. Maybe something like
       
       Log Rate Limiting   [x]
             Rate          [        ]
	     Burst         [        ]

     With Rate and Burst disabled/greyed out if 'Log Rate Limiting' is
     not ticked (assuming that I understood the options correctly, I
     have not really checked how they work exactly)




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