Re: [PATCH firewall/manager 0/5] Allow updating references to firewall objects when editing them

[Arthur Bied-Charreton <a.bied-charreton@proxmox.com>]

On Thu, Jul 16, 2026 at 03:17:40PM +0200, Stefan Hanreich wrote:
> Thanks for tackling this! I've added some comments on the individual
> patches and did some testing
> 
thanks for the review!
> An idea for an additional feature I noticed while testing: We
> auto-generate IPSets for SDN objects. It might make sense to add similar
> functionality when deleting SDN objects, so there are no dangling IPSet
> references after deleting an SDN object. This could be done in a v2, or
> even in a completely separate patch series.
good point, will look into that. 
> 
> 
> On 4/7/26 9:36 AM, Arthur Bied-Charreton wrote:
> > Renaming/deleting ipsets or aliases that are referenced by other rules,
> > security groups, or ipsets currently leaves dangling references behind.
> > This creates broken firewall configurations where the rules with those
> > dangling references fail to parse, meaning they are ignored by the
> > firewall.
> > 
> > This is especially dangerous in the renaming case, where a user might
> > reasonably expect references to update automatically, meaning they may
> > go about their day after renaming an object without realizing a whole
> > set of firewall rules is now inactive.
> > 
> > This patch series addresses this by adding optional parameters to the
> > related firewall endpoints that allow updating/deleting all references
> > to an object when editing/removing it. The UI patches add checkboxes to
> > the edit/remove windows to optionally pass this parameter.
> > 
> > The casing in the firewall API is inconsistent: ipset names are stored
> > as-is in the SectionConfig (e.g. 'FOO'), but lowercased when the config
> > is loaded. This means a rule referencing '+dc/FOO' will fail to match
> > its own definition after a load. In practice this is self-correcting:
> > lowercasing is applied in-place, so definitions are written back in
> > lowercase on the next read-write cycle, and the API does not allow
> > creating rules with references that do not match anything. There is
> > however a window between object creation and that first cycle where
> > references can be broken. To avoid false negatives in that window,
> > this series uses case-insensitive matching when scanning for
> > references, and renamed references are always written back lowercased.
> > 
> > Stefan and I talked about this off-list and came to the conclusion that
> > enforcing either casing may break existing configs that have been
> > manually edited, so a more general fix would be better folded into a
> > major release.
> > 
> > The UI patches introduce some code duplication. I tried to generalize
> > the remove button with an optional extra URL parameter, but I did not
> > find a good name for it, nor did I find a nice common ground that did
> > not involve making the call sites even more verbose with configuration
> > options, which is why I ended up leaving it like this. I am of course
> > open to suggestions.
> > 
> > The original plan was to have an upfront check for would-be-dangling
> > references before allowing any edit to an object, however this can be a
> > very expensive operation, since an object defined at the cluster level
> > may be referenced by guest and host configurations in the whole cluster.
> > A check like this would trigger an unconditional scan of all those
> > configs when clicking the remove buttons, which is why it is kept
> > opt-in in the UI.
> > 
> > 
> > pve-firewall:
> > 
> > Arthur Bied-Charreton (3):
> >   Add helpers for updating alias and ipset references
> >   ipset: Add option to update references on rename/delete
> >   aliases: Add option to update references on rename/delete
> > 
> >  src/PVE/API2/Firewall/Aliases.pm | 115 +++++++++++++++++++++++++++++++
> >  src/PVE/API2/Firewall/Helpers.pm |  66 ++++++++++++++++++
> >  src/PVE/API2/Firewall/IPSet.pm   | 108 ++++++++++++++++++++++++++++-
> >  3 files changed, 288 insertions(+), 1 deletion(-)
> > 
> > 
> > pve-manager:
> > 
> > Arthur Bied-Charreton (2):
> >   ipset: Add option to update references on rename/delete
> >   aliases: Add option to update references on rename/delete
> > 
> >  www/manager6/grid/FirewallAliases.js | 101 +++++++++++++++++++++------
> >  www/manager6/panel/IPSet.js          |  60 +++++++++++++++-
> >  2 files changed, 136 insertions(+), 25 deletions(-)
> > 
> > 
> > Summary over all repositories:
> >   5 files changed, 424 insertions(+), 26 deletions(-)
> > 
> 
> 
> 
> 
>