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 101E51FF191 for ; Tue, 4 Nov 2025 15:18:58 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 14682EF85; Tue, 4 Nov 2025 15:19:37 +0100 (CET) Mime-Version: 1.0 Date: Tue, 04 Nov 2025 15:19:02 +0100 Message-Id: From: "Lukas Wagner" To: "Proxmox Datacenter Manager development discussion" , "Hannes Laimer" X-Mailer: aerc 0.21.0-0-g5549850facc2-dirty References: <20251030143406.193744-1-h.laimer@proxmox.com> In-Reply-To: <20251030143406.193744-1-h.laimer@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1762265925335 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.373 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_ASCII_DIVIDERS 0.8 Email that uses ascii formatting dividers and possible spam tricks KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. 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, -datacenter-manager} 00/13] add basic integration of PVE firewall 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-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pdm-devel-bounces@lists.proxmox.com Sender: "pdm-devel" 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