[PATCH container/docs/firewall/manager/network/qemu-server v6 00/28] handle dangling references when firewall objects go away

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

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