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 9D4511FF0B3 for ; Wed, 11 Nov 2026 14:23:20 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 45158219FF; Wed, 11 Nov 2026 14:20:15 +0100 (CET) From: Arthur Bied-Charreton To: pve-devel@lists.proxmox.com Subject: [PATCH pve-firewall v6 11/28] firewall: api: run reference updates as tasks Date: Wed, 11 Nov 2026 14:18:44 +0100 Message-ID: <20261111131901.1791124-12-a.bied-charreton@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20261111131901.1791124-1-a.bied-charreton@proxmox.com> References: <20261111131901.1791124-1-a.bied-charreton@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1794403148404 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.602 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) 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 Message-ID-Hash: XBYBFH5N4R7XRURQWJUZHP2HO7ZX5TNP X-Message-ID-Hash: XBYBFH5N4R7XRURQWJUZHP2HO7ZX5TNP 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: A 'dangling-references' action other than 'keep' rewrites the rules that referenced the object, which for a cluster-scoped object means going through all guest, host and vnet firewall configs in the cluster, potentially updating each of them. The update_refs() helper logs each change made to each config. Fork a worker when the request passes 'dangling-references' so the log lands in the task log, which is visible cluster-wide, instead of the syslog. A request without the parameter, i.e. what every existing client sends, keeps running synchronously and returning null for backwards compatibility. Checks that can be performed without holding the lock are made before forking so a rejected request can still fail right away instead of requiring the caller to go read the task. Signed-off-by: Arthur Bied-Charreton --- src/PVE/API2/Firewall/Aliases.pm | 233 ++++++++++++++++++------------ src/PVE/API2/Firewall/IPSet.pm | 238 ++++++++++++++++++------------- src/PVE/Firewall/References.pm | 20 +++ 3 files changed, 299 insertions(+), 192 deletions(-) diff --git a/src/PVE/API2/Firewall/Aliases.pm b/src/PVE/API2/Firewall/Aliases.pm index cdbe68c..301ca88 100644 --- a/src/PVE/API2/Firewall/Aliases.pm +++ b/src/PVE/API2/Firewall/Aliases.pm @@ -3,7 +3,7 @@ package PVE::API2::Firewall::AliasesBase; use strict; use warnings; -use PVE::Firewall::References qw(update_refs); +use PVE::Firewall::References qw(update_refs run_ref_update); use PVE::Exception qw(raise raise_param_exc); use PVE::JSONSchema qw(get_standard_option); @@ -84,6 +84,44 @@ my $aliases_to_list = sub { return $list; }; +my $check_digest = sub { + my ($aliases, $param) = @_; + + my $list = &$aliases_to_list($aliases); + my (undef, $digest) = PVE::Firewall::copy_list_with_digest($list); + + PVE::Tools::assert_if_modified($digest, $param->{digest}); +}; + +my $check_update = sub { + my ($aliases, $param, $force) = @_; + + $check_digest->($aliases, $param); + + my $name = lc($param->{name}); + raise_param_exc({ name => "no such alias" }) if !$aliases->{$name}; + + my $rename = $param->{rename}; + $rename = lc($rename) if $rename; + + if ( + $rename + && $name ne $rename + && defined($aliases->{$rename}) + && !$force + ) { + raise_param_exc({ name => "alias '$param->{rename}' already exists" }); + } +}; + +my $check_delete = sub { + my ($aliases, $param) = @_; + + $check_digest->($aliases, $param); + + raise_param_exc({ name => "no such alias" }) if !$aliases->{ lc($param->{name}) }; +}; + sub register_get_aliases { my ($class) = @_; @@ -220,7 +258,6 @@ sub register_update_alias { $properties->{cidr} = { $api_properties->{cidr}->%*, optional => 1 }; $properties->{comment} = $api_properties->{comment}; $properties->{digest} = get_standard_option('pve-config-digest'); - $properties->{'dangling-references'} = get_standard_option('pve-dangling-references-rename'); $properties->{'force-rename'} = get_standard_option('pve-dangling-references-force-rename'); @@ -235,85 +272,88 @@ sub register_update_alias { additionalProperties => 0, properties => $properties, }, - returns => { type => "null" }, + returns => { type => 'string', optional => 1 }, code => sub { my ($param) = @_; - $class->lock_config( - $param, - sub { - my ($param) = @_; - - my $dangling_refs = $param->{'dangling-references'} - // $properties->{'dangling-references'}->{default}; - my $force = $param->{'force-rename'} - // $properties->{'force-rename'}->{default}; - - my ($fw_conf, $aliases) = $class->load_config($param); - - my $list = &$aliases_to_list($aliases); - - my (undef, $digest) = PVE::Firewall::copy_list_with_digest($list); - - PVE::Tools::assert_if_modified($digest, $param->{digest}); - - my $name = lc($param->{name}); - - raise_param_exc({ name => "no such alias" }) if !$aliases->{$name}; + my $dangling_refs = $param->{'dangling-references'} + // $properties->{'dangling-references'}->{default}; + my $force = $param->{'force-rename'} // $properties->{'force-rename'}->{default}; - my $data = { - name => $param->{name}, - cidr => $param->{cidr} // $aliases->{$name}->{cidr}, - comment => $param->{comment} // $aliases->{$name}->{comment}, - }; + my $renaming = $param->{rename} && lc($param->{name}) ne lc($param->{rename}); - $aliases->{$name} = $data; + my $code = sub { + $class->lock_config( + $param, + sub { + my ($param) = @_; - my $rename = $param->{rename}; - $rename = lc($rename) if $rename; + my ($fw_conf, $aliases) = $class->load_config($param); - if ($rename && ($name ne $rename)) { - raise_param_exc({ name => "alias '$param->{rename}' already exists" }) - if defined($aliases->{$rename}) - && !$force; + $check_update->($aliases, $param, $force); - $aliases->{$rename} = - { $aliases->{$name}->%*, name => $param->{rename} }; + my $name = lc($param->{name}); - my $saved = 0; - my $save = sub { - $class->save_aliases($param, $fw_conf, $aliases); - $saved = 1; + my $data = { + name => $param->{name}, + cidr => $param->{cidr} // $aliases->{$name}->{cidr}, + comment => $param->{comment} // $aliases->{$name}->{comment}, }; - eval { - update_refs({ - conf => $fw_conf, - kind => 'aliases', - old => $name, - new => $param->{rename}, - env => $class->rule_env(), - action => $dangling_refs eq 'keep' ? undef : $dangling_refs, - subject => - "alias '$param->{name}' renamed to '$param->{rename}'", - save => $save, - }); - }; - if (my $err = $@) { - die "rename interrupted, the alias exists under both names; " - . "retry with 'force-rename' to finish: $err" - if $saved; - die "could not update references to alias '$name': $err"; + $aliases->{$name} = $data; + + my $rename = $param->{rename}; + $rename = lc($rename) if $rename; + + if ($rename && ($name ne $rename)) { + $aliases->{$rename} = + { $aliases->{$name}->%*, name => $param->{rename} }; + + my $saved = 0; + my $save = sub { + $class->save_aliases($param, $fw_conf, $aliases); + $saved = 1; + }; + + eval { + update_refs({ + conf => $fw_conf, + kind => 'aliases', + old => $name, + new => $param->{rename}, + env => $class->rule_env(), + action => $dangling_refs eq 'keep' ? undef : $dangling_refs, + subject => + "alias '$param->{name}' renamed to '$param->{rename}'", + save => $save, + }); + }; + if (my $err = $@) { + die "rename interrupted, the alias exists under both names; " + . "retry with 'force-rename' to finish: $err" + if $saved; + die "could not update references to alias '$name': $err"; + } + + delete $aliases->{$name}; } - delete $aliases->{$name}; - } + $class->save_aliases($param, $fw_conf, $aliases); + }, + ); + }; - $class->save_aliases($param, $fw_conf, $aliases); - }, - ); + if (!$renaming || !defined($param->{'dangling-references'})) { + $code->(); + return undef; + } - return undef; + # fail on the request instead of inside the task. + my (undef, $aliases) = $class->load_config($param); + + $check_update->($aliases, $param, $force); + + return run_ref_update('fw-alias-rename', $param->{vmid} // $param->{name}, $code); }, }); } @@ -338,43 +378,52 @@ sub register_delete_alias { additionalProperties => 0, properties => $properties, }, - returns => { type => "null" }, + returns => { type => 'string', optional => 1 }, code => sub { my ($param) = @_; - $class->lock_config( - $param, - sub { - my ($param) = @_; + my $action = $param->{'dangling-references'} + // $properties->{'dangling-references'}->{default}; - my ($fw_conf, $aliases) = $class->load_config($param); + my $code = sub { + $class->lock_config( + $param, + sub { + my ($param) = @_; - my $list = &$aliases_to_list($aliases); - my (undef, $digest) = PVE::Firewall::copy_list_with_digest($list); - PVE::Tools::assert_if_modified($digest, $param->{digest}); + my ($fw_conf, $aliases) = $class->load_config($param); - my $name = lc($param->{name}); - raise_param_exc({ name => "no such alias" }) if !$aliases->{$name}; + $check_delete->($aliases, $param); - my $action = $param->{'dangling-references'} - // $properties->{'dangling-references'}->{default}; + my $name = lc($param->{name}); - update_refs({ - conf => $fw_conf, - kind => 'aliases', - old => $param->{name}, - env => $class->rule_env(), - action => $action eq 'keep' ? undef : $action, - subject => "alias '$param->{name}' deleted", - }); + update_refs({ + conf => $fw_conf, + kind => 'aliases', + old => $param->{name}, + env => $class->rule_env(), + action => $action eq 'keep' ? undef : $action, + subject => "alias '$param->{name}' deleted", + }); - delete $aliases->{$name}; + delete $aliases->{$name}; - $class->save_aliases($param, $fw_conf, $aliases); - }, - ); + $class->save_aliases($param, $fw_conf, $aliases); + }, + ); + }; - return undef; + if (!defined($param->{'dangling-references'})) { + $code->(); + return undef; + } + + # fail on the request instead of inside the task + my (undef, $aliases) = $class->load_config($param); + + $check_delete->($aliases, $param); + + return run_ref_update('fw-alias-delete', $param->{vmid} // $param->{name}, $code); }, }); } diff --git a/src/PVE/API2/Firewall/IPSet.pm b/src/PVE/API2/Firewall/IPSet.pm index 144baf6..05be0fe 100644 --- a/src/PVE/API2/Firewall/IPSet.pm +++ b/src/PVE/API2/Firewall/IPSet.pm @@ -2,7 +2,7 @@ package PVE::API2::Firewall::IPSetBase; use strict; use warnings; -use PVE::Firewall::References qw(update_refs); +use PVE::Firewall::References qw(update_refs run_ref_update); use PVE::Exception qw(raise raise_param_exc); use PVE::JSONSchema qw(get_standard_option); @@ -154,38 +154,50 @@ sub register_delete_ipset { additionalProperties => 0, properties => $properties, }, - returns => { type => 'null' }, + returns => { type => 'string', optional => 1 }, code => sub { my ($param) = @_; - $class->lock_config( - $param, - sub { - my ($param) = @_; + my $action = $param->{'dangling-references'} + // $properties->{'dangling-references'}->{default}; - my ($cluster_conf, $fw_conf, $ipset) = $class->load_config($param); + my $code = sub { + $class->lock_config( + $param, + sub { + my ($param) = @_; - die "IPSet '$param->{name}' is not empty\n" - if scalar(@$ipset) && !$param->{force}; + my ($cluster_conf, $fw_conf, $ipset) = $class->load_config($param); - my $action = $param->{'dangling-references'} - // $properties->{'dangling-references'}->{default}; + die "IPSet '$param->{name}' is not empty\n" + if scalar(@$ipset) && !$param->{force}; - update_refs({ - conf => $fw_conf, - kind => 'ipset', - old => $param->{name}, - env => $class->rule_env(), - action => $action eq 'keep' ? undef : $action, - subject => "ipset '$param->{name}' deleted", - }); + update_refs({ + conf => $fw_conf, + kind => 'ipset', + old => $param->{name}, + env => $class->rule_env(), + action => $action eq 'keep' ? undef : $action, + subject => "ipset '$param->{name}' deleted", + }); - $class->save_ipset($param, $fw_conf, undef); + $class->save_ipset($param, $fw_conf, undef); + }, + ); + }; - }, - ); + if (!defined($param->{'dangling-references'})) { + $code->(); + return undef; + } - return undef; + # fail on the request instead of inside the task + my (undef, undef, $ipset) = $class->load_config($param); + + die "IPSet '$param->{name}' is not empty\n" + if scalar($ipset->@*) && !$param->{force}; + + return run_ref_update('fw-ipset-delete', $param->{vmid} // $param->{name}, $code); }, }); } @@ -548,7 +560,7 @@ package PVE::API2::Firewall::BaseIPSetList; use strict; use warnings; -use PVE::Firewall::References qw(update_refs); +use PVE::Firewall::References qw(update_refs run_ref_update); use PVE::JSONSchema qw(get_standard_option); use PVE::Exception qw(raise_param_exc); use PVE::Firewall; @@ -616,6 +628,25 @@ my $get_ipset_list = sub { return wantarray ? ($list, $digest) : $list; }; +my $check_rename = sub { + my ($conf, $param, $force) = @_; + + my (undef, $digest) = &$get_ipset_list($conf); + PVE::Tools::assert_if_modified($digest, $param->{digest}); + + raise_param_exc({ name => "IPSet '$param->{rename}' does not exist" }) + if !$conf->{ipset}->{ $param->{rename} }; + + # prevent overwriting existing ipset + if ( + $param->{name} ne $param->{rename} + && $conf->{ipset}->{ $param->{name} } + && !$force + ) { + raise_param_exc({ name => "IPSet '$param->{name}' already exists" }); + } +}; + sub register_index { my ($class) = @_; @@ -690,93 +721,100 @@ sub register_create { additionalProperties => 0, properties => $properties, }, - returns => { type => 'null' }, + returns => { type => 'string', optional => 1 }, code => sub { my ($param) = @_; - $class->lock_config( - $param, - sub { - my ($param) = @_; - - my $dangling_refs = $param->{'dangling-references'} - // $properties->{'dangling-references'}->{default}; - my $force = $param->{'force-rename'} - // $properties->{'force-rename'}->{default}; - - my ($cluster_conf, $fw_conf) = $class->load_config($param); - - if ($param->{rename}) { - my (undef, $digest) = &$get_ipset_list($fw_conf); - PVE::Tools::assert_if_modified($digest, $param->{digest}); - - raise_param_exc({ name => "IPSet '$param->{rename}' does not exist" }) - if !$fw_conf->{ipset}->{ $param->{rename} }; - - if ($param->{name} ne $param->{rename}) { - # prevent overwriting existing ipset - raise_param_exc({ name => "IPSet '$param->{name}' already exists" }) - if $fw_conf->{ipset}->{ $param->{name} } - && !$force; - - $fw_conf->{ipset}->{ $param->{name} } = - $fw_conf->{ipset}->{ $param->{rename} }; - - my $saved = 0; - my $save = sub { - $class->save_config($param, $fw_conf); - $saved = 1; - }; - - eval { - update_refs({ - conf => $fw_conf, - kind => 'ipset', - old => $param->{rename}, - new => $param->{name}, - env => $class->rule_env(), - action => $dangling_refs eq 'keep' ? undef : $dangling_refs, - subject => - "ipset '$param->{rename}' renamed to '$param->{name}'", - save => $save, - }); - }; - if (my $err = $@) { - die "rename interrupted, the IPSet exists under both names; " - . "retry with 'force-rename' to finish: $err" - if $saved; - die "could not update references to IPSet " - . "'$param->{rename}': $err"; + my $dangling_refs = $param->{'dangling-references'} + // $properties->{'dangling-references'}->{default}; + my $force = $param->{'force-rename'} // $properties->{'force-rename'}->{default}; + + my $renaming = $param->{rename} && $param->{name} ne $param->{rename}; + + my $code = sub { + $class->lock_config( + $param, + sub { + my ($param) = @_; + + my ($cluster_conf, $fw_conf) = $class->load_config($param); + + if ($param->{rename}) { + $check_rename->($fw_conf, $param, $force); + + if ($param->{name} ne $param->{rename}) { + $fw_conf->{ipset}->{ $param->{name} } = + $fw_conf->{ipset}->{ $param->{rename} }; + + my $saved = 0; + my $save = sub { + $class->save_config($param, $fw_conf); + $saved = 1; + }; + + eval { + update_refs({ + conf => $fw_conf, + kind => 'ipset', + old => $param->{rename}, + new => $param->{name}, + env => $class->rule_env(), + action => $dangling_refs eq 'keep' + ? undef + : $dangling_refs, + subject => + "ipset '$param->{rename}' renamed to '$param->{name}'", + save => $save, + }); + }; + if (my $err = $@) { + die + "rename interrupted, the IPSet exists under both names; " + . "retry with 'force-rename' to finish: $err" + if $saved; + die "could not update references to IPSet " + . "'$param->{rename}': $err"; + } + + delete $fw_conf->{ipset}->{ $param->{rename} }; + + if ( + my $comment = + delete $fw_conf->{ipset_comments}->{ $param->{rename} } + ) { + $fw_conf->{ipset_comments}->{ $param->{name} } = $comment; + } } - delete $fw_conf->{ipset}->{ $param->{rename} }; - - if ( - my $comment = - delete $fw_conf->{ipset_comments}->{ $param->{rename} } - ) { - $fw_conf->{ipset_comments}->{ $param->{name} } = $comment; + $fw_conf->{ipset_comments}->{ $param->{name} } = $param->{comment} + if defined($param->{comment}); + } else { + foreach my $name (keys %{ $fw_conf->{ipset} }) { + raise_param_exc({ name => "IPSet '$name' already exists" }) + if $name eq $param->{name}; } - } - $fw_conf->{ipset_comments}->{ $param->{name} } = $param->{comment} - if defined($param->{comment}); - } else { - foreach my $name (keys %{ $fw_conf->{ipset} }) { - raise_param_exc({ name => "IPSet '$name' already exists" }) - if $name eq $param->{name}; + $fw_conf->{ipset}->{ $param->{name} } = []; + $fw_conf->{ipset_comments}->{ $param->{name} } = $param->{comment} + if defined($param->{comment}); } - $fw_conf->{ipset}->{ $param->{name} } = []; - $fw_conf->{ipset_comments}->{ $param->{name} } = $param->{comment} - if defined($param->{comment}); - } + $class->save_config($param, $fw_conf); + }, + ); + }; - $class->save_config($param, $fw_conf); - }, - ); + if (!$renaming || !defined($param->{'dangling-references'})) { + $code->(); + return undef; + } - return undef; + # fail on the request instead of inside the task + my (undef, $fw_conf) = $class->load_config($param); + + $check_rename->($fw_conf, $param, $force); + + return run_ref_update('fw-ipset-rename', $param->{vmid} // $param->{rename}, $code); }, }); } diff --git a/src/PVE/Firewall/References.pm b/src/PVE/Firewall/References.pm index b106651..2fb8767 100644 --- a/src/PVE/Firewall/References.pm +++ b/src/PVE/Firewall/References.pm @@ -12,6 +12,7 @@ use base 'Exporter'; our @EXPORT_OK = qw( collect_refs update_refs + run_ref_update ); sub collect_refs { @@ -441,6 +442,25 @@ sub update_refs { return $count; } +=head3 run_ref_update($type, $id, $code) + +Run C<$code> as a worker task of type C<$type> for C<$id>. + +C<$id> is the guest a guest-scoped object belongs to so the task is logged with that guest, and the +name of the object otherwise. + +Return the UPID of the task. + +=cut + +sub run_ref_update { + my ($type, $id, $code) = @_; + + my $rpcenv = PVE::RESTEnvironment::get(); + + return $rpcenv->fork_worker($type, $id, $rpcenv->get_user(), $code); +} + =head3 format_ref_change($change) Format one recorded config change as a single line, e.g. -- 2.47.3