From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 0BB621FF0B3 for ; Wed, 11 Nov 2026 14:21:09 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 6B06721828; Wed, 11 Nov 2026 14:19:45 +0100 (CET) From: Arthur Bied-Charreton To: pve-devel@lists.proxmox.com Subject: [PATCH container/docs/firewall/manager/network/qemu-server v6 00/28] handle dangling references when firewall objects go away Date: Wed, 11 Nov 2026 14:18:33 +0100 Message-ID: <20261111131901.1791124-1-a.bied-charreton@proxmox.com> X-Mailer: git-send-email 2.47.3 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1794403147642 X-SPAM-LEVEL: Spam detection results: 1 AWL -2.017 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) POISEN_SPAM_PILL 0.1 Meta: its spam POISEN_SPAM_PILL_1 0.1 random spam to be learned in bayes POISEN_SPAM_PILL_3 0.1 random spam to be learned in bayes RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_DBL_SPAM 5 Contains a spam URL listed in the Spamhaus DBL blocklist [sdn.pm] Message-ID-Hash: KZ32O2PTDFOKRSCWYZSHLXSK3KMPKNXI X-Message-ID-Hash: KZ32O2PTDFOKRSCWYZSHLXSK3KMPKNXI X-MailFrom: a.bied-charreton@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Renaming or deleting a firewall object (an IPSet, alias or security group) that rules still reference leaves those references dangling. The firewall fails to parse the affected rules and drops them from the generated ruleset. For security groups, the reference reaches the backend, which creates empty chains on iptables, and causes the ruleset to fail to load on nftables [1]. This is especially bad in the rename case, where one might reasonably expect the references to follow the new object name and walk away thinking the firewall rules are active. This series makes the affected operations offer to deal with the references instead of leaving them behind with the following API options: POST .../firewall/ipset dangling-references keep|disable|drop|rename force-rename boolean PUT .../firewall/aliases/{name} dangling-references keep|disable|drop|rename force-rename boolean POST /cluster/firewall/groups dangling-references keep|disable|drop|rename force-rename boolean DELETE .../ipset/{name} dangling-references keep|disable|drop DELETE .../aliases/{name} dangling-references keep|disable|drop DELETE /cluster/firewall/groups/{name} dangling-references keep|disable|drop PUT /cluster/sdn dangling-ipset-references keep|disable|drop DELETE /nodes/{node}/qemu/{vmid} dangling-ipset-references keep|disable|drop DELETE /nodes/{node}/lxc/{vmid} dangling-ipset-references keep|disable|drop Overview: - 'rename' rewrites the references to point at the new name, 'disable' disables referencing rules, and 'drop' removes them. An IPSet member has no disabled state so those are also removed with 'disable'. 'keep' laves the references as they are. - For a cluster object, all downstream configs in the cluster (guest, host and vnet firewalls) are updated as well, locking and saving each one (as was already partly done for security group renames). On a rename, the new object is persisted before its references are rewritten because otherwise concurrent compilations may see references to objects that do not exist yet. - If a cluster-wide rename is interrupted partway, the object exists under both names and the next attempt fails because the target name already exists. Setting 'force-rename' skips that check so the rename can be retried. - The API keeps backwards compatible defaults ('keep' everywhere except for POST /cluster/firewall/groups, which was already renaming by default and will keep doing that). The UI defaults to renaming/ dropping dangling references. - Passing 'dangling-references' runs the rename/delete as a task for better observability. Every change made to configs is logged. Requests that do not pass it (i.e. every existing client) still run synchronously as before. - SDN apply and guest destroy treat the reference update as best effort. If it fails, a warning is logged and the operation still completes. At that point the guest is already half-deleted and the SDN config is about to be commiteed, failing there would be wrong for guests and block the SDN apply. - Guest destroy and SDN apply can change firewall configs that the caller would technically need different permissions for. They do not require additional permissions, see [0]. The same applies to the task log, since the operation already lets the caller change those configs. Known limitations: - Each config is locked and saved on its own, so a cluster-wide rename is not atomic, which leaves two race windows. This is pre-existing behavior, already present in security group renames and manual edits via the API/GUI. a. A compilation that reads the clsuter config before the new name was saved and a guest config after it was rewritten skips rules referencing the object *for the current cycle*. Closing this would need the compliation to detect mixed reads and retry, which I deemed out of scope for this series. b. A rule added to a config that was already updated keeps the old name. AFAICT the only way to properly solve this would be holding locks for *all* firewall configs while editing, which would stall all edits for the duration of the rename and therefore does not seem like a great option. - An unscoped reference stays unscoped when renamed. If a guest defines an object under the target name, the guest's references start resolving to that object after a rename. Rewriting them with an explicit scope would change configs that are (sadly) sometimes managed outside of the API, so this is accepted for now. We plan to address this with a breaking change in PVE 10. - Recovering from an interrupted rename is not covered by unit tests as that would mean mocking most of PVE::Firewall and PVE::Cluster. It was tested end to end on a test cluster. Pre-built packages on sani under: abied-charreton/firewall-object-references/v6/ Build dependencies: pve-container <- pve-firewall qemu-server <- pve-firewall pve-manager <- pve-docs Runtime dependencies: pve-network <- pve-firewall qemu-server <- pve-firewall pve-container <- pve-firewall pve-manager <- pve-firewall pve-manager <- libpve-network-api-perl pve-manager <- qemu-server pve-manager <- pve-container Changes since [v5]: - Make 'force-rename' an extra boolean parameter so a retry can also be done with 'disable' or 'drop', not only with 'rename' - Only run renames/deletes as tasks when 'dangling-references' is passed so existing clients keep the same behavior - Check that an alias exists before deleting it - Log reference changes only after successful saves - Walk the vnet configs present on disk instead of the ones from the running config - Add monkey patches to unit tests to prevent them from walking the live cluster - Use single gettext string for dangling references hints - New API options are now shared definitions in pve-firewall - A failed reference update in SDN apply logs the warning in the task log instead of only the node's journal [v5] https://lore.proxmox.com/pve-devel/20261103084328.203492-1-a.bied-charreton@proxmox.com/ [v4] https://lore.proxmox.com/pve-devel/20261022124708.429069-1-a.bied-charreton@proxmox.com/ [v3] https://lore.proxmox.com/pve-devel/20260925094230.844917-1-a.bied-charreton@proxmox.com/ [v2] https://lore.proxmox.com/pve-devel/20260818133413.450776-1-a.bied-charreton@proxmox.com/ [v1] https://lore.proxmox.com/pve-devel/20260407073658.90818-1-a.bied-charreton@proxmox.com/ [0] https://lore.proxmox.com/pve-devel/awaoshjzbr7adisjngsrcts4zs3hxgoxw2lgcoq66gtiap3al2@mpxrbmtyfcwj/ [1] https://bugzilla.proxmox.com/show_bug.cgi?id=8176 pve-firewall: Arthur Bied-Charreton (15): firewall: run make tidy firewall: move object reference helpers to their own module firewall: references: add helpers to update object references firewall: references: log the references an object update touched firewall: references: add wrappers for the SDN-generated IPSets firewall: parser: do not log errors for disabled rules firewall: ipset: add option to handle dangling references on rename firewall: ipset: add option to handle dangling references on delete firewall: aliases: add option to handle dangling references on rename firewall: aliases: add option to handle dangling references on delete firewall: api: run reference updates as tasks firewall: tests: add tests for object reference updates firewall: references: handle security groups firewall: groups: add option to handle dangling references on rename fix #8176: firewall: groups: handle dangling references on delete src/PVE/API2/Firewall/Aliases.pm | 209 ++++++++--- src/PVE/API2/Firewall/Cluster.pm | 6 +- src/PVE/API2/Firewall/Groups.pm | 182 +++++----- src/PVE/API2/Firewall/Helpers.pm | 3 +- src/PVE/API2/Firewall/IPSet.pm | 200 ++++++++--- src/PVE/API2/Firewall/Rules.pm | 49 ++- src/PVE/API2/Firewall/VM.pm | 7 +- src/PVE/Firewall.pm | 44 ++- src/PVE/Firewall/Helpers.pm | 38 -- src/PVE/Firewall/Makefile | 2 +- src/PVE/Firewall/References.pm | 589 +++++++++++++++++++++++++++++++ src/test/Makefile | 2 +- src/test/test_update_refs.pl | 417 ++++++++++++++++++++++ 13 files changed, 1490 insertions(+), 258 deletions(-) create mode 100644 src/PVE/Firewall/References.pm create mode 100755 src/test/test_update_refs.pl pve-network: Arthur Bied-Charreton (1): apply: add option to handle dangling references on VNet deletion src/PVE/API2/Network/SDN.pm | 39 +++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) qemu-server: Arthur Bied-Charreton (1): api: destroy_vm: add option to handle dangling IPSet references src/PVE/API2/Qemu.pm | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) pve-container: Arthur Bied-Charreton (1): api: destroy_vm: add option to handle dangling IPSet references src/PVE/API2/LXC.pm | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) pve-manager: Arthur Bied-Charreton (8): ui: firewall: add a common widget for handling dangling references ui: firewall: ipset: add controls for dangling references ui: firewall: aliases: add controls for dangling references ui: sdn: apply: add control for dangling IPSet references ui: guest destroy: use let for non-constant variable bindings ui: guest destroy: add control for dangling IPSet references ui: firewall: show the task for reference updates ui: firewall: groups: add controls for dangling references www/manager6/Makefile | 1 + www/manager6/Utils.js | 6 + www/manager6/dc/SecurityGroups.js | 81 ++++++++++++- www/manager6/grid/FirewallAliases.js | 125 ++++++++++++++++---- www/manager6/grid/FirewallObjectCommon.js | 136 ++++++++++++++++++++++ www/manager6/panel/IPSet.js | 84 +++++++++++-- www/manager6/sdn/StatusView.js | 119 +++++++++++++++---- www/manager6/window/SafeDestroyGuest.js | 70 ++++++++++- 8 files changed, 565 insertions(+), 57 deletions(-) create mode 100644 www/manager6/grid/FirewallObjectCommon.js pve-docs: Arthur Bied-Charreton (2): firewall: remove trailing spaces firewall: document renaming and removing objects pve-firewall.adoc | 78 +++++++++++++++++++++++++++++++++++++++++++---- pvesdn.adoc | 9 ++++-- 2 files changed, 78 insertions(+), 9 deletions(-) Summary over all repositories: 26 files changed, 2210 insertions(+), 324 deletions(-) -- Generated by murpp 0.12.1